| Message ID | 20230428143621.142390-2-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix PMP related problem | expand |
On Sat, Apr 29, 2023 at 12:37 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > PMP entries before the matched PMP entry (including the matched PMP entry) > may only cover partial of the TLB page, which may make different regions in > that page allow different RWX privs. Such as for PMP0 (0x80000008~0x8000000F, > R) and PMP1 (0x80000000~0x80000FFF, RWX), write access to 0x80000000 will > match PMP1. However we cannot cache the translation result in the TLB since > this will make the write access to 0x80000008 bypass the check of PMP0. So we > should check all of them instead of the matched PMP entry in pmp_get_tlb_size() > and set the tlb_size to 1 in this case. > Set tlb_size to TARGET_PAGE_SIZE if PMP is not support or there is no PMP rules. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > --- > target/riscv/cpu_helper.c | 7 ++--- > target/riscv/pmp.c | 64 ++++++++++++++++++++++++++++++--------- > target/riscv/pmp.h | 3 +- > 3 files changed, 52 insertions(+), 22 deletions(-) > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 433ea529b0..075fc0538a 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -703,11 +703,8 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, > } > > *prot = pmp_priv_to_page_prot(pmp_priv); > - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) { > - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1); > - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1; > - > - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea); > + if (tlb_size != NULL) { > + *tlb_size = pmp_get_tlb_size(env, addr); > } > > return TRANSLATE_SUCCESS; > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 1f5aca42e8..ad20a319c1 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -601,28 +601,62 @@ target_ulong mseccfg_csr_read(CPURISCVState *env) > } > > /* > - * Calculate the TLB size if the start address or the end address of > - * PMP entry is presented in the TLB page. > + * Calculate the TLB size. If the PMP rules may make different regions in > + * the TLB page of 'addr' allow different RWX privs, set the size to 1 > + * (to make the translation result uncached in the TLB and only be used for > + * a single translation). Set the size to TARGET_PAGE_SIZE otherwise. I think this could be clearer, something like: Calculate the TLB size. If the matching PMP rule only matches a subset of the TLB, it's possible that earlier higher priority PMP regions will match other parts of the TLB. For example if PMP0 is (0x80000008~0x8000000F, > R) and PMP1 is (0x80000000~0x80000FFF, RWX) a write access to 0x80000000 will match PMP1. However we cannot cache the translation result in the TLB since this will make the write access to 0x80000008 bypass the check of PMP0. To avoid this we return a size of 1 (which means no cacheing) if the PMP region does not cover the entire TLB. > */ > -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index, > - target_ulong tlb_sa, target_ulong tlb_ea) > +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr) > { > - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa; > - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea; > + target_ulong pmp_sa; > + target_ulong pmp_ea; > + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1); > + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1; > + int i; > > - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { > + /* > + * If PMP is not supported or there is no PMP rule, which means the allowed > + * RWX privs of the page will not affected by PMP or PMP will provide the > + * same option (disallow accesses or allow default RWX privs) for all > + * addresses, set the size to TARGET_PAGE_SIZE. Sam here: If PMP is not supported or there are no PMP rules, the permissions of the page will not affected by PMP so we set the size to TARGET_PAGE_SIZE. Otherwise: Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > + */ > + if (!riscv_cpu_cfg(env)->pmp || !pmp_get_num_rules(env)) { > return TARGET_PAGE_SIZE; > - } else { > + } > + > + for (i = 0; i < MAX_RISCV_PMPS; i++) { > + if (pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg) == PMP_AMATCH_OFF) { > + continue; > + } > + > + pmp_sa = env->pmp_state.addr[i].sa; > + pmp_ea = env->pmp_state.addr[i].ea; > + > /* > - * At this point we have a tlb_size that is the smallest possible size > - * That fits within a TARGET_PAGE_SIZE and the PMP region. > - * > - * If the size is less then TARGET_PAGE_SIZE we drop the size to 1. > - * This means the result isn't cached in the TLB and is only used for > - * a single translation. > + * Only the first PMP entry that covers (whole or partial of) the TLB > + * page really matters: > + * If it can cover the whole page, set the size to TARGET_PAGE_SIZE. > + * The following PMP entries have lower priority and will not affect > + * the allowed RWX privs of the page. > + * If it only cover partial of the TLB page, set the size to 1 since > + * the allowed RWX privs for the covered region may be different from > + * other region of the page. > */ > - return 1; > + if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { > + return TARGET_PAGE_SIZE; > + } else if ((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) || > + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) { > + return 1; > + } > } > + > + /* > + * If no PMP entry covers any region of the TLB page, similar to the above > + * case that there is no PMP rule, PMP will provide the same option > + * (disallow accesses or allow default RWX privs) for the whole page, > + * set the size to TARGET_PAGE_SIZE. > + */ > + return TARGET_PAGE_SIZE; > } > > /* > diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h > index b296ea1fc6..0a7e24750b 100644 > --- a/target/riscv/pmp.h > +++ b/target/riscv/pmp.h > @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, > target_ulong size, pmp_priv_t privs, > pmp_priv_t *allowed_privs, > target_ulong mode); > -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index, > - target_ulong tlb_sa, target_ulong tlb_ea); > +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr); > 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); > -- > 2.25.1 > >
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 433ea529b0..075fc0538a 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -703,11 +703,8 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, } *prot = pmp_priv_to_page_prot(pmp_priv); - if ((tlb_size != NULL) && pmp_index != MAX_RISCV_PMPS) { - target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1); - target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1; - - *tlb_size = pmp_get_tlb_size(env, pmp_index, tlb_sa, tlb_ea); + if (tlb_size != NULL) { + *tlb_size = pmp_get_tlb_size(env, addr); } return TRANSLATE_SUCCESS; diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index 1f5aca42e8..ad20a319c1 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -601,28 +601,62 @@ target_ulong mseccfg_csr_read(CPURISCVState *env) } /* - * Calculate the TLB size if the start address or the end address of - * PMP entry is presented in the TLB page. + * Calculate the TLB size. If the PMP rules may make different regions in + * the TLB page of 'addr' allow different RWX privs, set the size to 1 + * (to make the translation result uncached in the TLB and only be used for + * a single translation). Set the size to TARGET_PAGE_SIZE otherwise. */ -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index, - target_ulong tlb_sa, target_ulong tlb_ea) +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr) { - target_ulong pmp_sa = env->pmp_state.addr[pmp_index].sa; - target_ulong pmp_ea = env->pmp_state.addr[pmp_index].ea; + target_ulong pmp_sa; + target_ulong pmp_ea; + target_ulong tlb_sa = addr & ~(TARGET_PAGE_SIZE - 1); + target_ulong tlb_ea = tlb_sa + TARGET_PAGE_SIZE - 1; + int i; - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { + /* + * If PMP is not supported or there is no PMP rule, which means the allowed + * RWX privs of the page will not affected by PMP or PMP will provide the + * same option (disallow accesses or allow default RWX privs) for all + * addresses, set the size to TARGET_PAGE_SIZE. + */ + if (!riscv_cpu_cfg(env)->pmp || !pmp_get_num_rules(env)) { return TARGET_PAGE_SIZE; - } else { + } + + for (i = 0; i < MAX_RISCV_PMPS; i++) { + if (pmp_get_a_field(env->pmp_state.pmp[i].cfg_reg) == PMP_AMATCH_OFF) { + continue; + } + + pmp_sa = env->pmp_state.addr[i].sa; + pmp_ea = env->pmp_state.addr[i].ea; + /* - * At this point we have a tlb_size that is the smallest possible size - * That fits within a TARGET_PAGE_SIZE and the PMP region. - * - * If the size is less then TARGET_PAGE_SIZE we drop the size to 1. - * This means the result isn't cached in the TLB and is only used for - * a single translation. + * Only the first PMP entry that covers (whole or partial of) the TLB + * page really matters: + * If it can cover the whole page, set the size to TARGET_PAGE_SIZE. + * The following PMP entries have lower priority and will not affect + * the allowed RWX privs of the page. + * If it only cover partial of the TLB page, set the size to 1 since + * the allowed RWX privs for the covered region may be different from + * other region of the page. */ - return 1; + if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { + return TARGET_PAGE_SIZE; + } else if ((pmp_sa >= tlb_sa && pmp_sa <= tlb_ea) || + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) { + return 1; + } } + + /* + * If no PMP entry covers any region of the TLB page, similar to the above + * case that there is no PMP rule, PMP will provide the same option + * (disallow accesses or allow default RWX privs) for the whole page, + * set the size to TARGET_PAGE_SIZE. + */ + return TARGET_PAGE_SIZE; } /* diff --git a/target/riscv/pmp.h b/target/riscv/pmp.h index b296ea1fc6..0a7e24750b 100644 --- a/target/riscv/pmp.h +++ b/target/riscv/pmp.h @@ -76,8 +76,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr, target_ulong size, pmp_priv_t privs, pmp_priv_t *allowed_privs, target_ulong mode); -target_ulong pmp_get_tlb_size(CPURISCVState *env, int pmp_index, - target_ulong tlb_sa, target_ulong tlb_ea); +target_ulong pmp_get_tlb_size(CPURISCVState *env, target_ulong addr); 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);