Re: [PATCH 1/6] target/riscv: Update pmp_get_tlb_size()

[Weiwei Li <liweiwei@iscas.ac.cn>]

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
>>>>>>
>>>>>>