| Message ID | 20230413090122.65228-2-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix PMP related problem | expand |
On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: > > Not only the matched PMP entry, Any PMP entry that overlap with partial of > the tlb page may make the regions in that page have different permission > rights. So all of them should be taken into consideration. > > 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 | 34 +++++++++++++++++++++------------- > target/riscv/pmp.h | 3 +-- > 3 files changed, 24 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..4f9389e73c 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -601,28 +601,36 @@ 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++) { > + 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. This comment points out that we should have the smallest region, so I'm not clear why we need this change. Can you update the commit description to be clear on why this change is needed and what it fixes? Alistair > - * > - * 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_sa <= tlb_ea) || > + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && > + !(pmp_sa == 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); > -- > 2.25.1 > >
On 2023/4/18 10:53, Alistair Francis wrote: > On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >> Not only the matched PMP entry, Any PMP entry that overlap with partial of >> the tlb page may make the regions in that page have different permission >> rights. So all of them should be taken into consideration. >> >> 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 | 34 +++++++++++++++++++++------------- >> target/riscv/pmp.h | 3 +-- >> 3 files changed, 24 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..4f9389e73c 100644 >> --- a/target/riscv/pmp.c >> +++ b/target/riscv/pmp.c >> @@ -601,28 +601,36 @@ 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++) { >> + 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. > This comment points out that we should have the smallest region, so > I'm not clear why we need this change. Can you update the commit > description to be clear on why this change is needed and what it > fixes? This function return tlb_size to 1 to make the tlb uncached. However, In previous implementation, only the matched PMP entry of current access address is taken into consideration. Then, if other PMP entry that match other address in the same page, we may also cannot cache the tlb, since different address in that page may have different permission rights. Regards, Weiwei Li > Alistair > >> - * >> - * 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_sa <= tlb_ea) || >> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && >> + !(pmp_sa == 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); >> -- >> 2.25.1 >> >>
On 2023/4/18 11:05, Weiwei Li wrote: > > On 2023/4/18 10:53, Alistair Francis wrote: >> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >>> Not only the matched PMP entry, Any PMP entry that overlap with >>> partial of >>> the tlb page may make the regions in that page have different >>> permission >>> rights. So all of them should be taken into consideration. >>> >>> 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 | 34 +++++++++++++++++++++------------- >>> target/riscv/pmp.h | 3 +-- >>> 3 files changed, 24 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..4f9389e73c 100644 >>> --- a/target/riscv/pmp.c >>> +++ b/target/riscv/pmp.c >>> @@ -601,28 +601,36 @@ 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++) { >>> + 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. >> This comment points out that we should have the smallest region, so >> I'm not clear why we need this change. Can you update the commit >> description to be clear on why this change is needed and what it >> fixes? > > This function return tlb_size to 1 to make the tlb uncached. However, > In previous implementation, > > only the matched PMP entry of current access address is taken into > consideration. Then, if other PMP entry > > that match other address in the same page, we may also cannot cache > the tlb, since different address > > in that page may have different permission rights. It doesn't matter. As the tlb size < page size, this tlb will have a TLB_INVALID_MASK flag and never match. For this page, every access will repeat the MMU check and TLB fill. It is not fast, but with no error. Zhiwei > > Regards, > > Weiwei Li > >> Alistair >> >>> - * >>> - * 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_sa <= tlb_ea) || >>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && >>> + !(pmp_sa == 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); >>> -- >>> 2.25.1 >>> >>>
On 2023/4/18 13:18, LIU Zhiwei wrote: > > On 2023/4/18 11:05, Weiwei Li wrote: >> >> On 2023/4/18 10:53, Alistair Francis wrote: >>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote: >>>> Not only the matched PMP entry, Any PMP entry that overlap with >>>> partial of >>>> the tlb page may make the regions in that page have different >>>> permission >>>> rights. So all of them should be taken into consideration. >>>> >>>> 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 | 34 +++++++++++++++++++++------------- >>>> target/riscv/pmp.h | 3 +-- >>>> 3 files changed, 24 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..4f9389e73c 100644 >>>> --- a/target/riscv/pmp.c >>>> +++ b/target/riscv/pmp.c >>>> @@ -601,28 +601,36 @@ 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++) { >>>> + 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. >>> This comment points out that we should have the smallest region, so >>> I'm not clear why we need this change. Can you update the commit >>> description to be clear on why this change is needed and what it >>> fixes? >> >> This function return tlb_size to 1 to make the tlb uncached. However, >> In previous implementation, >> >> only the matched PMP entry of current access address is taken into >> consideration. Then, if other PMP entry >> >> that match other address in the same page, we may also cannot cache >> the tlb, since different address >> >> in that page may have different permission rights. > > It doesn't matter. As the tlb size < page size, this tlb will have a > TLB_INVALID_MASK flag and never match. This is what I want. However, tlb size will be page size without this patch in some cases. Assuming: PMP0: sa: 0x80000008 ea: 0x8000000f, rights: R PMP1: sa: 0, ea: 0xffffffff, rights: RWX If we try to write data to 0x80000000, PMP1 will be matched, In previous implementation, tlb_size will be PMP1 TARGET_PAGE_SIZE and this will be cached, since only matched PMP is checked , and PMP1 covers the whole page. Then when we try to write data to 0x80000008, the tlb will be hit, and this access bypass the PMP check of PMP0. Regards, Weiwei Li > > For this page, every access will repeat the MMU check and TLB fill. > > It is not fast, but with no error. > > Zhiwei > >> >> Regards, >> >> Weiwei Li >> >>> Alistair >>> >>>> - * >>>> - * 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_sa <= tlb_ea) || >>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && >>>> + !(pmp_sa == 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); >>>> -- >>>> 2.25.1 >>>> >>>>
On 2023/4/18 14:09, Weiwei Li wrote: > > On 2023/4/18 13:18, LIU Zhiwei wrote: >> >> On 2023/4/18 11:05, Weiwei Li wrote: >>> >>> On 2023/4/18 10:53, Alistair Francis wrote: >>>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> >>>> wrote: >>>>> Not only the matched PMP entry, Any PMP entry that overlap with >>>>> partial of >>>>> the tlb page may make the regions in that page have different >>>>> permission >>>>> rights. So all of them should be taken into consideration. >>>>> >>>>> 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 | 34 +++++++++++++++++++++------------- >>>>> target/riscv/pmp.h | 3 +-- >>>>> 3 files changed, 24 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..4f9389e73c 100644 >>>>> --- a/target/riscv/pmp.c >>>>> +++ b/target/riscv/pmp.c >>>>> @@ -601,28 +601,36 @@ 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++) { >>>>> + 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. >>>> This comment points out that we should have the smallest region, so >>>> I'm not clear why we need this change. Can you update the commit >>>> description to be clear on why this change is needed and what it >>>> fixes? >>> >>> This function return tlb_size to 1 to make the tlb uncached. >>> However, In previous implementation, >>> >>> only the matched PMP entry of current access address is taken into >>> consideration. Then, if other PMP entry >>> >>> that match other address in the same page, we may also cannot cache >>> the tlb, since different address >>> >>> in that page may have different permission rights. >> >> It doesn't matter. As the tlb size < page size, this tlb will have a >> TLB_INVALID_MASK flag and never match. > > This is what I want. However, tlb size will be page size without this > patch in some cases. > > Assuming: > > PMP0: sa: 0x80000008 ea: 0x8000000f, rights: R > > PMP1: sa: 0, ea: 0xffffffff, rights: RWX > > If we try to write data to 0x80000000, PMP1 will be matched, In > previous implementation, > > tlb_size will be PMP1 TARGET_PAGE_SIZE and this will be cached, since > only matched PMP is checked , > > and PMP1 covers the whole page. Then when we try to write data to > 0x80000008, the tlb will be hit, > > and this access bypass the PMP check of PMP0. I see. You are fixing the priority of PMP check rule. You can still pass the matched index to pmp_get_tlb_size. And only check first match index PMP rules. > > Regards, > > Weiwei Li > >> >> For this page, every access will repeat the MMU check and TLB fill. >> >> It is not fast, but with no error. >> >> Zhiwei >> >>> >>> Regards, >>> >>> Weiwei Li >>> >>>> Alistair >>>> >>>>> - * >>>>> - * 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_sa <= tlb_ea) || >>>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && >>>>> + !(pmp_sa == 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); In this way, you need not to change the declaration of pmp_get_tlb_size. Otherwise, Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei >>>>> 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 >>>>> >>>>>
On 2023/4/18 15:08, LIU Zhiwei wrote: > > On 2023/4/18 14:09, Weiwei Li wrote: >> >> On 2023/4/18 13:18, LIU Zhiwei wrote: >>> >>> On 2023/4/18 11:05, Weiwei Li wrote: >>>> >>>> On 2023/4/18 10:53, Alistair Francis wrote: >>>>> On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> >>>>> wrote: >>>>>> Not only the matched PMP entry, Any PMP entry that overlap with >>>>>> partial of >>>>>> the tlb page may make the regions in that page have different >>>>>> permission >>>>>> rights. So all of them should be taken into consideration. >>>>>> >>>>>> 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 | 34 +++++++++++++++++++++------------- >>>>>> target/riscv/pmp.h | 3 +-- >>>>>> 3 files changed, 24 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..4f9389e73c 100644 >>>>>> --- a/target/riscv/pmp.c >>>>>> +++ b/target/riscv/pmp.c >>>>>> @@ -601,28 +601,36 @@ 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++) { >>>>>> + 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. >>>>> This comment points out that we should have the smallest region, so >>>>> I'm not clear why we need this change. Can you update the commit >>>>> description to be clear on why this change is needed and what it >>>>> fixes? >>>> >>>> This function return tlb_size to 1 to make the tlb uncached. >>>> However, In previous implementation, >>>> >>>> only the matched PMP entry of current access address is taken into >>>> consideration. Then, if other PMP entry >>>> >>>> that match other address in the same page, we may also cannot >>>> cache the tlb, since different address >>>> >>>> in that page may have different permission rights. >>> >>> It doesn't matter. As the tlb size < page size, this tlb will have a >>> TLB_INVALID_MASK flag and never match. >> >> This is what I want. However, tlb size will be page size without >> this patch in some cases. >> >> Assuming: >> >> PMP0: sa: 0x80000008 ea: 0x8000000f, rights: R >> >> PMP1: sa: 0, ea: 0xffffffff, rights: RWX >> >> If we try to write data to 0x80000000, PMP1 will be matched, In >> previous implementation, >> >> tlb_size will be PMP1 TARGET_PAGE_SIZE and this will be cached, since >> only matched PMP is checked , >> >> and PMP1 covers the whole page. Then when we try to write data to >> 0x80000008, the tlb will be hit, >> >> and this access bypass the PMP check of PMP0. > > I see. You are fixing the priority of PMP check rule. Yeah. You can see it as a priority problem. > > You can still pass the matched index to pmp_get_tlb_size. And only > check first match index PMP rules. Yeah. only the PMP rules before the matched PMP need be checked. However, I prefers separate the matched index from pmp_get_tlb_size, then we can separate this function from get_physical_address_pmp (not all pmp check needs caculate the tlb size). Maybe we can improve the check to following code: 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; } then the checked index will not be larger than matched index(the matched case will match one of the above conditions). Regards, Weiwei Li > >> >> Regards, >> >> Weiwei Li >> >>> >>> For this page, every access will repeat the MMU check and TLB fill. >>> >>> It is not fast, but with no error. >>> >>> Zhiwei >>> >>>> >>>> Regards, >>>> >>>> Weiwei Li >>>> >>>>> Alistair >>>>> >>>>>> - * >>>>>> - * 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_sa <= tlb_ea) || >>>>>> + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && >>>>>> + !(pmp_sa == 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); > > In this way, you need not to change the declaration of pmp_get_tlb_size. > > Otherwise, > Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> > > Zhiwei > >>>>>> 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..4f9389e73c 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -601,28 +601,36 @@ 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++) { + 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_sa <= tlb_ea) || + (pmp_ea >= tlb_sa && pmp_ea <= tlb_ea)) && + !(pmp_sa == 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);