| Message ID | 20230419032725.29721-2-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix PMP related problem | expand |
On 2023/4/19 11:27, Weiwei Li wrote: > PMP entries before the matched PMP entry(including the matched PMP entry) > may overlap partial of the tlb page, which may make different regions in > that page have different permission rights, such as for > PMP0(0x80000008~0x8000000F, R) and PMP1(0x80001000~0x80001FFF, RWX)) > write access to 0x80000000 will match PMP1. However we cannot cache the tlb > for it since this will make the write access to 0x80000008 bypass the check > of PMP0. So we should check all of them and set the tlb size to 1 in this > case. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > target/riscv/cpu_helper.c | 7 ++----- > target/riscv/pmp.c | 39 ++++++++++++++++++++++++++------------- > target/riscv/pmp.h | 3 +-- > 3 files changed, 29 insertions(+), 20 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..22f3b3f217 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -601,28 +601,41 @@ target_ulong mseccfg_csr_read(CPURISCVState *env) > } > > /* > - * Calculate the TLB size if the start address or the end address of > + * Calculate the TLB size if any start address or the end address of > * PMP entry is presented in the TLB page. > */ If we don't pass the start address or the end address parameter, we can comment this function: Calculate the TLB size according to the PMP rule with the highest priority. > -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; > + > + 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; > > - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { > - return TARGET_PAGE_SIZE; > - } else { > /* > - * 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. > + * If any start address or the end address of PMP entry is presented > + * in the TLB page and cannot override the whole TLB page we drop the > + * size to 1. > * This means the result isn't cached in the TLB and is only used for > * a single translation. > */ > - 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; > + } > } > + > + return TARGET_PAGE_SIZE; This implicitly require a success return from the get_physical_address_pmp call. If we want this function to be a independent one, we should return 0 to indicate no tlb size can return. Zhiwei > } > > /* > 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);
On 2023/4/20 19:58, LIU Zhiwei wrote: > > On 2023/4/19 11:27, Weiwei Li wrote: >> PMP entries before the matched PMP entry(including the matched PMP >> entry) >> may overlap partial of the tlb page, which may make different regions in >> that page have different permission rights, such as for >> PMP0(0x80000008~0x8000000F, R) and PMP1(0x80001000~0x80001FFF, RWX)) >> write access to 0x80000000 will match PMP1. However we cannot cache >> the tlb >> for it since this will make the write access to 0x80000008 bypass the >> check >> of PMP0. So we should check all of them and set the tlb size to 1 in >> this >> case. >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> target/riscv/cpu_helper.c | 7 ++----- >> target/riscv/pmp.c | 39 ++++++++++++++++++++++++++------------- >> target/riscv/pmp.h | 3 +-- >> 3 files changed, 29 insertions(+), 20 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..22f3b3f217 100644 >> --- a/target/riscv/pmp.c >> +++ b/target/riscv/pmp.c >> @@ -601,28 +601,41 @@ target_ulong mseccfg_csr_read(CPURISCVState *env) >> } >> /* >> - * Calculate the TLB size if the start address or the end address of >> + * Calculate the TLB size if any start address or the end address of >> * PMP entry is presented in the TLB page. >> */ > > If we don't pass the start address or the end address parameter, we > can comment this function: > > Calculate the TLB size according to the PMP rule with the highest > priority. This seems also not accurate. It's the first(highest priority) PMP entry that overlap with the tlb page really matters. Maybe we can modify it to: "Calculate the TLB size. Return 1 if the first PMP entry that overlaps with the TLB page doesn't cover the whole page . Return 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; >> + >> + 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; >> - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { >> - return TARGET_PAGE_SIZE; >> - } else { >> /* >> - * 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. >> + * If any start address or the end address of PMP entry is >> presented >> + * in the TLB page and cannot override the whole TLB page we >> drop the >> + * size to 1. >> * This means the result isn't cached in the TLB and is >> only used for >> * a single translation. >> */ >> - 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; >> + } >> } >> + >> + return TARGET_PAGE_SIZE; > > This implicitly require a success return from the > get_physical_address_pmp call. If we want this function to be a > independent one, we should return 0 to indicate no tlb size can return. Yeah. I think the default should be TARGET_PAGE_SIZE if no PMP entry overlap with the tlb page, since in this case, tlb_size should be TARGET_PAGE_SIZE if the translation(inlcuding PMP check) successes while tlb_size will be ignored if the translation fails. Regards, Weiwei Li > > Zhiwei > >> } >> /* >> 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);
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..22f3b3f217 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -601,28 +601,41 @@ target_ulong mseccfg_csr_read(CPURISCVState *env) } /* - * Calculate the TLB size if the start address or the end address of + * Calculate the TLB size if any start address or the end address of * PMP entry is presented in the TLB page. */ -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; + + 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; - if (pmp_sa <= tlb_sa && pmp_ea >= tlb_ea) { - return TARGET_PAGE_SIZE; - } else { /* - * 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. + * If any start address or the end address of PMP entry is presented + * in the TLB page and cannot override the whole TLB page we drop the + * size to 1. * This means the result isn't cached in the TLB and is only used for * a single translation. */ - 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; + } } + + 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);