On Sun, Feb 21, 2021 at 10:35 AM Jim Shu <[email protected]> wrote: > > Currently, PMP permission checking of TLB page is bypassed if TLB hits > Fix it by propagating PMP permission to TLB page permission. > > PMP permission checking also use MMU-style API to change TLB permission > and size. > > Signed-off-by: Jim Shu <[email protected]>
Reviewed-by: Alistair Francis <[email protected]> Alistair > --- > target/riscv/cpu_helper.c | 84 +++++++++++++++++++++++++++++---------- > target/riscv/pmp.c | 80 +++++++++++++++++++++++++++---------- > target/riscv/pmp.h | 4 +- > 3 files changed, 125 insertions(+), 43 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 2f43939fb6..f6ac63bf0e 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -280,6 +280,49 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong > newpriv) > env->load_res = -1; > } > > +/* > + * get_physical_address_pmp - check PMP permission for this physical address > + * > + * Match the PMP region and check permission for this physical address and > it's > + * TLB page. Returns 0 if the permission checking was successful > + * > + * @env: CPURISCVState > + * @prot: The returned protection attributes > + * @tlb_size: TLB page size containing addr. It could be modified after PMP > + * permission checking. NULL if not set TLB page for addr. > + * @addr: The physical address to be checked permission > + * @access_type: The type of MMU access > + * @mode: Indicates current privilege level. > + */ > +static int get_physical_address_pmp(CPURISCVState *env, int *prot, > + target_ulong *tlb_size, hwaddr addr, > + int size, MMUAccessType access_type, > + int mode) > +{ > + pmp_priv_t pmp_priv; > + target_ulong tlb_size_pmp = 0; > + > + if (!riscv_feature(env, RISCV_FEATURE_PMP)) { > + *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC; > + return TRANSLATE_SUCCESS; > + } > + > + if (!pmp_hart_has_privs(env, addr, size, 1 << access_type, &pmp_priv, > + mode)) { > + *prot = 0; > + return TRANSLATE_PMP_FAIL; > + } > + > + *prot = pmp_priv_to_page_prot(pmp_priv); > + if (tlb_size != NULL) { > + if (pmp_is_range_in_tlb(env, addr & ~(*tlb_size - 1), > &tlb_size_pmp)) { > + *tlb_size = tlb_size_pmp; > + } > + } > + > + return TRANSLATE_SUCCESS; > +} > + > /* get_physical_address - get the physical address for this virtual address > * > * Do a page table walk to obtain the physical address corresponding to a > @@ -442,9 +485,11 @@ restart: > pte_addr = base + idx * ptesize; > } > > - if (riscv_feature(env, RISCV_FEATURE_PMP) && > - !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong), > - 1 << MMU_DATA_LOAD, PRV_S)) { > + int pmp_prot; > + int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, > pte_addr, > + sizeof(target_ulong), > + MMU_DATA_LOAD, PRV_S); > + if (pmp_ret != TRANSLATE_SUCCESS) { > return TRANSLATE_PMP_FAIL; > } > > @@ -682,13 +727,14 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, > int size, > #ifndef CONFIG_USER_ONLY > vaddr im_address; > hwaddr pa = 0; > - int prot, prot2; > + int prot, prot2, prot_pmp; > bool pmp_violation = false; > bool first_stage_error = true; > bool two_stage_lookup = false; > int ret = TRANSLATE_FAIL; > int mode = mmu_idx; > - target_ulong tlb_size = 0; > + /* default TLB page size */ > + target_ulong tlb_size = TARGET_PAGE_SIZE; > > env->guest_phys_fault_addr = 0; > > @@ -745,10 +791,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, > int size, > > prot &= prot2; > > - if (riscv_feature(env, RISCV_FEATURE_PMP) && > - (ret == TRANSLATE_SUCCESS) && > - !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) { > - ret = TRANSLATE_PMP_FAIL; > + if (ret == TRANSLATE_SUCCESS) { > + ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa, > + size, access_type, mode); > + prot &= prot_pmp; > } > > if (ret != TRANSLATE_SUCCESS) { > @@ -771,25 +817,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, > int size, > "%s address=%" VADDR_PRIx " ret %d physical " > TARGET_FMT_plx " prot %d\n", > __func__, address, ret, pa, prot); > - } > > - if (riscv_feature(env, RISCV_FEATURE_PMP) && > - (ret == TRANSLATE_SUCCESS) && > - !pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) { > - ret = TRANSLATE_PMP_FAIL; > + if (ret == TRANSLATE_SUCCESS) { > + ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa, > + size, access_type, mode); > + prot &= prot_pmp; > + } > } > + > if (ret == TRANSLATE_PMP_FAIL) { > pmp_violation = true; > } > > if (ret == TRANSLATE_SUCCESS) { > - if (pmp_is_range_in_tlb(env, pa & TARGET_PAGE_MASK, &tlb_size)) { > - tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1), > - prot, mmu_idx, tlb_size); > - } else { > - tlb_set_page(cs, address & TARGET_PAGE_MASK, pa & > TARGET_PAGE_MASK, > - prot, mmu_idx, TARGET_PAGE_SIZE); > - } > + tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1), > + prot, mmu_idx, tlb_size); > return true; > } else if (probe) { > return false; > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 80d0334e1b..ebd874cde3 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -217,6 +217,35 @@ static int pmp_is_in_range(CPURISCVState *env, int > pmp_index, target_ulong addr) > return result; > } > > +/* > + * Check if the address has required RWX privs when no PMP entry is matched. > + */ > +static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr, > + target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs, > + target_ulong mode) > +{ > + bool ret; > + > + if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) { > + /* > + * Privileged spec v1.10 states if HW doesn't implement any PMP entry > + * or no PMP entry matches an M-Mode access, the access succeeds. > + */ > + ret = true; > + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > + } else { > + /* > + * Other modes are not allowed to succeed if they don't * match a > rule, > + * but there are rules. We've checked for no rule earlier in this > + * function. > + */ > + ret = false; > + *allowed_privs = 0; > + } > + > + return ret; > +} > + > > /* > * Public Interface > @@ -226,18 +255,19 @@ static int pmp_is_in_range(CPURISCVState *env, int > pmp_index, target_ulong addr) > * Check if the address has required RWX privs to complete desired operation > */ > bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > - target_ulong size, pmp_priv_t privs, target_ulong mode) > + target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs, > + target_ulong mode) > { > int i = 0; > int ret = -1; > int pmp_size = 0; > target_ulong s = 0; > target_ulong e = 0; > - pmp_priv_t allowed_privs = 0; > > /* Short cut if no rules */ > if (0 == pmp_get_num_rules(env)) { > - return (env->priv == PRV_M) ? true : false; > + return pmp_hart_has_privs_default(env, addr, size, privs, > + allowed_privs, mode); > } > > if (size == 0) { > @@ -277,37 +307,25 @@ bool pmp_hart_has_privs(CPURISCVState *env, > target_ulong addr, > * check > */ > if (((s + e) == 2) && (PMP_AMATCH_OFF != a_field)) { > - allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > + *allowed_privs = PMP_READ | PMP_WRITE | PMP_EXEC; > if ((mode != PRV_M) || pmp_is_locked(env, i)) { > - allowed_privs &= env->pmp_state.pmp[i].cfg_reg; > + *allowed_privs &= env->pmp_state.pmp[i].cfg_reg; > } > > - if ((privs & allowed_privs) == privs) { > - ret = 1; > - break; > - } else { > - ret = 0; > - break; > - } > + ret = ((privs & *allowed_privs) == privs); > + break; > } > } > > /* No rule matched */ > if (ret == -1) { > - if (mode == PRV_M) { > - ret = 1; /* Privileged spec v1.10 states if no PMP entry matches > an > - * M-Mode access, the access succeeds */ > - } else { > - ret = 0; /* Other modes are not allowed to succeed if they don't > - * match a rule, but there are rules. We've checked for > - * no rule earlier in this function. */ > - } > + return pmp_hart_has_privs_default(env, addr, size, privs, > + allowed_privs, mode); > } > > return ret == 1 ? true : false; > } > > - > /* > * Handle a write to a pmpcfg CSP > */ > @@ -442,3 +460,23 @@ bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr > tlb_sa, > > return false; > } > + > +/* > + * Convert PMP privilege to TLB page privilege. > + */ > +int pmp_priv_to_page_prot(pmp_priv_t pmp_priv) > +{ > + int prot = 0; > + > + if (pmp_priv & PMP_READ) { > + prot |= PAGE_READ; > + } > + if (pmp_priv & PMP_WRITE) { > + prot |= PAGE_WRITE; > + } > + if (pmp_priv & PMP_EXEC) { > + prot |= PAGE_EXEC; > + } > + > + return prot; > +} > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h > index c8d5ef4a69..b82a30f0d5 100644 > --- a/target/riscv/pmp.h > +++ b/target/riscv/pmp.h > @@ -59,11 +59,13 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t > addr_index, > target_ulong val); > target_ulong pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index); > bool pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > - target_ulong size, pmp_priv_t priv, target_ulong mode); > + target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs, > + target_ulong mode); > bool pmp_is_range_in_tlb(CPURISCVState *env, hwaddr tlb_sa, > target_ulong *tlb_size); > void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index); > void pmp_update_rule_nums(CPURISCVState *env); > uint32_t pmp_get_num_rules(CPURISCVState *env); > +int pmp_priv_to_page_prot(pmp_priv_t pmp_priv); > > #endif > -- > 2.30.1 > >
