[PATCH 0/6] target/riscv: Fix PMP related problem

[PATCH 0/6] target/riscv: Fix PMP related problem

[Weiwei Li]

This patchset tries to fix the PMP bypass problem issue https://gitlab.com/qemu-project/qemu/-/issues/1542

The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix

Weiwei Li (6):
  target/riscv: Update pmp_get_tlb_size()
  target/riscv: Move pmp_get_tlb_size apart from
    get_physical_address_pmp
  target/riscv: flush tlb when pmpaddr is updated
  target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
  target/riscv: flush tb when PMP entry changes
  accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
    re-filled

 accel/tcg/cputlb.c        |  7 -----
 target/riscv/cpu_helper.c | 19 ++++---------
 target/riscv/pmp.c        | 60 ++++++++++++++++++++++++++-------------
 target/riscv/pmp.h        |  3 +-
 4 files changed, 47 insertions(+), 42 deletions(-)

-- 
2.25.1

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

[Weiwei Li]

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

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

[Alistair Francis]

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

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

[Weiwei Li]

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

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

[LIU Zhiwei]

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

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

[Weiwei Li]

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

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

[LIU Zhiwei]

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

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

[Weiwei Li]

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

[PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp

[Weiwei Li]

pmp_get_tlb_size have no relationship with pmp-related permission check
currently.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu_helper.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 075fc0538a..83c9699a6d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -676,14 +676,11 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
  *
  * @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,
+static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
                                     int size, MMUAccessType access_type,
                                     int mode)
 {
@@ -703,9 +700,6 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
     }
 
     *prot = pmp_priv_to_page_prot(pmp_priv);
-    if (tlb_size != NULL) {
-        *tlb_size = pmp_get_tlb_size(env, addr);
-    }
 
     return TRANSLATE_SUCCESS;
 }
@@ -905,7 +899,7 @@ restart:
         }
 
         int pmp_prot;
-        int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
+        int pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr,
                                                sizeof(target_ulong),
                                                MMU_DATA_LOAD, PRV_S);
         if (pmp_ret != TRANSLATE_SUCCESS) {
@@ -1300,8 +1294,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
             prot &= prot2;
 
             if (ret == TRANSLATE_SUCCESS) {
-                ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+                ret = get_physical_address_pmp(env, &prot_pmp, pa,
                                                size, access_type, mode);
+                tlb_size = pmp_get_tlb_size(env, pa);
 
                 qemu_log_mask(CPU_LOG_MMU,
                               "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
@@ -1333,8 +1328,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                       __func__, address, ret, pa, prot);
 
         if (ret == TRANSLATE_SUCCESS) {
-            ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
+            ret = get_physical_address_pmp(env, &prot_pmp, pa,
                                            size, access_type, mode);
+            tlb_size = pmp_get_tlb_size(env, pa);
 
             qemu_log_mask(CPU_LOG_MMU,
                           "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
-- 
2.25.1

Re: [PATCH 2/6] target/riscv: Move pmp_get_tlb_size apart from get_physical_address_pmp

[Alistair Francis]

On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> pmp_get_tlb_size have no relationship with pmp-related permission check
> currently.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 075fc0538a..83c9699a6d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -676,14 +676,11 @@ void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>   *
>   * @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,
> +static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr,
>                                      int size, MMUAccessType access_type,
>                                      int mode)
>  {
> @@ -703,9 +700,6 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
>      }
>
>      *prot = pmp_priv_to_page_prot(pmp_priv);
> -    if (tlb_size != NULL) {
> -        *tlb_size = pmp_get_tlb_size(env, addr);
> -    }
>
>      return TRANSLATE_SUCCESS;
>  }
> @@ -905,7 +899,7 @@ restart:
>          }
>
>          int pmp_prot;
> -        int pmp_ret = get_physical_address_pmp(env, &pmp_prot, NULL, pte_addr,
> +        int pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr,
>                                                 sizeof(target_ulong),
>                                                 MMU_DATA_LOAD, PRV_S);
>          if (pmp_ret != TRANSLATE_SUCCESS) {
> @@ -1300,8 +1294,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>              prot &= prot2;
>
>              if (ret == TRANSLATE_SUCCESS) {
> -                ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> +                ret = get_physical_address_pmp(env, &prot_pmp, pa,
>                                                 size, access_type, mode);
> +                tlb_size = pmp_get_tlb_size(env, pa);
>
>                  qemu_log_mask(CPU_LOG_MMU,
>                                "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> @@ -1333,8 +1328,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                        __func__, address, ret, pa, prot);
>
>          if (ret == TRANSLATE_SUCCESS) {
> -            ret = get_physical_address_pmp(env, &prot_pmp, &tlb_size, pa,
> +            ret = get_physical_address_pmp(env, &prot_pmp, pa,
>                                             size, access_type, mode);
> +            tlb_size = pmp_get_tlb_size(env, pa);
>
>              qemu_log_mask(CPU_LOG_MMU,
>                            "%s PMP address=" HWADDR_FMT_plx " ret %d prot"
> --
> 2.25.1
>
>

[PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated

[Weiwei Li]

TLB should be flushed not only for pmpcfg csr changes, but also for
pmpaddr csr changes.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/pmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 4f9389e73c..6d4813806b 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
         if (!pmp_is_locked(env, addr_index)) {
             env->pmp_state.pmp[addr_index].addr_reg = val;
             pmp_update_rule(env, addr_index);
+            tlb_flush(env_cpu(env));
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "ignoring pmpaddr write - locked\n");
-- 
2.25.1

Re: [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated

[Alistair Francis]

On Thu, Apr 13, 2023 at 7:03 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> TLB should be flushed not only for pmpcfg csr changes, but also for
> pmpaddr csr changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/pmp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 4f9389e73c..6d4813806b 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>          if (!pmp_is_locked(env, addr_index)) {
>              env->pmp_state.pmp[addr_index].addr_reg = val;
>              pmp_update_rule(env, addr_index);
> +            tlb_flush(env_cpu(env));
>          } else {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "ignoring pmpaddr write - locked\n");
> --
> 2.25.1
>
>

Re: [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated

[LIU Zhiwei]

On 2023/4/13 17:01, Weiwei Li wrote:
> TLB should be flushed not only for pmpcfg csr changes, but also for
> pmpaddr csr changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/pmp.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 4f9389e73c..6d4813806b 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>           if (!pmp_is_locked(env, addr_index)) {
>               env->pmp_state.pmp[addr_index].addr_reg = val;
>               pmp_update_rule(env, addr_index);
> +            tlb_flush(env_cpu(env));

Can we always flush tlb in pmp_update_rule?

Zhiwei

>           } else {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "ignoring pmpaddr write - locked\n");

Re: [PATCH 3/6] target/riscv: flush tlb when pmpaddr is updated

[Weiwei Li]

On 2023/4/18 15:11, LIU Zhiwei wrote:
>
> On 2023/4/13 17:01, Weiwei Li wrote:
>> TLB should be flushed not only for pmpcfg csr changes, but also for
>> pmpaddr csr changes.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/pmp.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>> index 4f9389e73c..6d4813806b 100644
>> --- a/target/riscv/pmp.c
>> +++ b/target/riscv/pmp.c
>> @@ -537,6 +537,7 @@ void pmpaddr_csr_write(CPURISCVState *env, 
>> uint32_t addr_index,
>>           if (!pmp_is_locked(env, addr_index)) {
>>               env->pmp_state.pmp[addr_index].addr_reg = val;
>>               pmp_update_rule(env, addr_index);
>> +            tlb_flush(env_cpu(env));
>
> Can we always flush tlb in pmp_update_rule?

I considered this way, However  this may lead to tlb being flushed 
several times when we update pmpcfg csrs.

Regards,

Weiwei Li

>
> Zhiwei
>
>>           } else {
>>               qemu_log_mask(LOG_GUEST_ERROR,
>>                             "ignoring pmpaddr write - locked\n");

[PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes

[Weiwei Li]

TLB needn't be flushed when pmpcfg/pmpaddr don't changes.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/pmp.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 6d4813806b..aced23c4d5 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -26,7 +26,7 @@
 #include "trace.h"
 #include "exec/exec-all.h"
 
-static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
+static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
                           uint8_t val);
 static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
 static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
@@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
  * Accessor to set the cfg reg for a specific PMP/HART
  * Bounds checks and relevant lock bit.
  */
-static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
+static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
 {
     if (pmp_index < MAX_RISCV_PMPS) {
         bool locked = true;
@@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
 
         if (locked) {
             qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
-        } else {
+        } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
             env->pmp_state.pmp[pmp_index].cfg_reg = val;
             pmp_update_rule(env, pmp_index);
+            return true;
         }
     } else {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "ignoring pmpcfg write - out of bounds\n");
     }
+
+    return false;
 }
 
 static void pmp_decode_napot(target_ulong a, target_ulong *sa,
@@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
     int i;
     uint8_t cfg_val;
     int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
+    bool modified = false;
 
     trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
 
     for (i = 0; i < pmpcfg_nums; i++) {
         cfg_val = (val >> 8 * i)  & 0xff;
-        pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
+        modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
     }
 
     /* If PMP permission of any addr has been changed, flush TLB pages. */
-    tlb_flush(env_cpu(env));
+    if (modified) {
+        tlb_flush(env_cpu(env));
+    }
 }
 
 
@@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
         }
 
         if (!pmp_is_locked(env, addr_index)) {
-            env->pmp_state.pmp[addr_index].addr_reg = val;
-            pmp_update_rule(env, addr_index);
-            tlb_flush(env_cpu(env));
+            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
+                env->pmp_state.pmp[addr_index].addr_reg = val;
+                pmp_update_rule(env, addr_index);
+                tlb_flush(env_cpu(env));
+            }
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "ignoring pmpaddr write - locked\n");
-- 
2.25.1

Re: [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes

[Alistair Francis]

On Thu, Apr 13, 2023 at 7:04 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> TLB needn't be flushed when pmpcfg/pmpaddr don't changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/pmp.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 6d4813806b..aced23c4d5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -26,7 +26,7 @@
>  #include "trace.h"
>  #include "exec/exec-all.h"
>
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
>                            uint8_t val);
>  static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
>  static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
> @@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
>   * Accessor to set the cfg reg for a specific PMP/HART
>   * Bounds checks and relevant lock bit.
>   */
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>  {
>      if (pmp_index < MAX_RISCV_PMPS) {
>          bool locked = true;
> @@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>
>          if (locked) {
>              qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> -        } else {
> +        } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
>              env->pmp_state.pmp[pmp_index].cfg_reg = val;
>              pmp_update_rule(env, pmp_index);
> +            return true;
>          }
>      } else {
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "ignoring pmpcfg write - out of bounds\n");
>      }
> +
> +    return false;
>  }
>
>  static void pmp_decode_napot(target_ulong a, target_ulong *sa,
> @@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>      int i;
>      uint8_t cfg_val;
>      int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
> +    bool modified = false;
>
>      trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>
>      for (i = 0; i < pmpcfg_nums; i++) {
>          cfg_val = (val >> 8 * i)  & 0xff;
> -        pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> +        modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
>      }
>
>      /* If PMP permission of any addr has been changed, flush TLB pages. */
> -    tlb_flush(env_cpu(env));
> +    if (modified) {
> +        tlb_flush(env_cpu(env));
> +    }
>  }
>
>
> @@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>          }
>
>          if (!pmp_is_locked(env, addr_index)) {
> -            env->pmp_state.pmp[addr_index].addr_reg = val;
> -            pmp_update_rule(env, addr_index);
> -            tlb_flush(env_cpu(env));
> +            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> +                env->pmp_state.pmp[addr_index].addr_reg = val;
> +                pmp_update_rule(env, addr_index);
> +                tlb_flush(env_cpu(env));
> +            }
>          } else {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "ignoring pmpaddr write - locked\n");
> --
> 2.25.1
>
>

Re: [PATCH 4/6] target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes

[LIU Zhiwei]

On 2023/4/13 17:01, Weiwei Li wrote:
> TLB needn't be flushed when pmpcfg/pmpaddr don't changes.

If we flush the tlb in pmp_update_rules, we don't need this patch.

Zhiwei

>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/pmp.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 6d4813806b..aced23c4d5 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -26,7 +26,7 @@
>   #include "trace.h"
>   #include "exec/exec-all.h"
>   
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
>                             uint8_t val);
>   static uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t addr_index);
>   static void pmp_update_rule(CPURISCVState *env, uint32_t pmp_index);
> @@ -83,7 +83,7 @@ static inline uint8_t pmp_read_cfg(CPURISCVState *env, uint32_t pmp_index)
>    * Accessor to set the cfg reg for a specific PMP/HART
>    * Bounds checks and relevant lock bit.
>    */
> -static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> +static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>   {
>       if (pmp_index < MAX_RISCV_PMPS) {
>           bool locked = true;
> @@ -119,14 +119,17 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>   
>           if (locked) {
>               qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write - locked\n");
> -        } else {
> +        } else if (env->pmp_state.pmp[pmp_index].cfg_reg != val) {
>               env->pmp_state.pmp[pmp_index].cfg_reg = val;
>               pmp_update_rule(env, pmp_index);
> +            return true;
>           }
>       } else {
>           qemu_log_mask(LOG_GUEST_ERROR,
>                         "ignoring pmpcfg write - out of bounds\n");
>       }
> +
> +    return false;
>   }
>   
>   static void pmp_decode_napot(target_ulong a, target_ulong *sa,
> @@ -477,16 +480,19 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>       int i;
>       uint8_t cfg_val;
>       int pmpcfg_nums = 2 << riscv_cpu_mxl(env);
> +    bool modified = false;
>   
>       trace_pmpcfg_csr_write(env->mhartid, reg_index, val);
>   
>       for (i = 0; i < pmpcfg_nums; i++) {
>           cfg_val = (val >> 8 * i)  & 0xff;
> -        pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
> +        modified |= pmp_write_cfg(env, (reg_index * 4) + i, cfg_val);
>       }
>   
>       /* If PMP permission of any addr has been changed, flush TLB pages. */
> -    tlb_flush(env_cpu(env));
> +    if (modified) {
> +        tlb_flush(env_cpu(env));
> +    }
>   }
>   
>   
> @@ -535,9 +541,11 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>           }
>   
>           if (!pmp_is_locked(env, addr_index)) {
> -            env->pmp_state.pmp[addr_index].addr_reg = val;
> -            pmp_update_rule(env, addr_index);
> -            tlb_flush(env_cpu(env));
> +            if (env->pmp_state.pmp[addr_index].addr_reg != val) {
> +                env->pmp_state.pmp[addr_index].addr_reg = val;
> +                pmp_update_rule(env, addr_index);
> +                tlb_flush(env_cpu(env));
> +            }
>           } else {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "ignoring pmpaddr write - locked\n");

[PATCH 5/6] target/riscv: flush tb when PMP entry changes

[Weiwei Li]

The translation block may also be affected when PMP entry changes.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/pmp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index aced23c4d5..c2db52361f 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -25,6 +25,7 @@
 #include "cpu.h"
 #include "trace.h"
 #include "exec/exec-all.h"
+#include "exec/tb-flush.h"
 
 static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
                           uint8_t val);
@@ -492,6 +493,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
     /* If PMP permission of any addr has been changed, flush TLB pages. */
     if (modified) {
         tlb_flush(env_cpu(env));
+        tb_flush(env_cpu(env));
     }
 }
 
@@ -545,6 +547,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
                 env->pmp_state.pmp[addr_index].addr_reg = val;
                 pmp_update_rule(env, addr_index);
                 tlb_flush(env_cpu(env));
+                tb_flush(env_cpu(env));
             }
         } else {
             qemu_log_mask(LOG_GUEST_ERROR,
-- 
2.25.1

Re: [PATCH 5/6] target/riscv: flush tb when PMP entry changes

[LIU Zhiwei]

On 2023/4/13 17:01, Weiwei Li wrote:
> The translation block may also be affected when PMP entry changes.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/pmp.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index aced23c4d5..c2db52361f 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -25,6 +25,7 @@
>   #include "cpu.h"
>   #include "trace.h"
>   #include "exec/exec-all.h"
> +#include "exec/tb-flush.h"
>   
>   static bool pmp_write_cfg(CPURISCVState *env, uint32_t addr_index,
>                             uint8_t val);
> @@ -492,6 +493,7 @@ void pmpcfg_csr_write(CPURISCVState *env, uint32_t reg_index,
>       /* If PMP permission of any addr has been changed, flush TLB pages. */
>       if (modified) {
>           tlb_flush(env_cpu(env));
> +        tb_flush(env_cpu(env));

Move this to pmp_update_rules.

Zhiwei

>       }
>   }
>   
> @@ -545,6 +547,7 @@ void pmpaddr_csr_write(CPURISCVState *env, uint32_t addr_index,
>                   env->pmp_state.pmp[addr_index].addr_reg = val;
>                   pmp_update_rule(env, addr_index);
>                   tlb_flush(env_cpu(env));
> +                tb_flush(env_cpu(env));
>               }
>           } else {
>               qemu_log_mask(LOG_GUEST_ERROR,

[PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Weiwei Li]

When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
this will make the address set with TLB_INVALID_MASK to make the page
un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
the TLB host address will be cached, and the following instructions can use
this host address directly which may lead to the bypass of PMP related check.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 accel/tcg/cputlb.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index e984a98dc4..d0bf996405 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr,
             /* TLB resize via tlb_fill may have moved the entry.  */
             index = tlb_index(env, mmu_idx, addr);
             entry = tlb_entry(env, mmu_idx, addr);
-
-            /*
-             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
-             * to force the next access through tlb_fill.  We've just
-             * called tlb_fill, so we know that this entry *is* valid.
-             */
-            flags &= ~TLB_INVALID_MASK;
         }
         tlb_addr = tlb_read_ofs(entry, elt_ofs);
     }
-- 
2.25.1

Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Daniel Henrique Barboza]

On 4/13/23 06:01, Weiwei Li wrote:
> When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
> this will make the address set with TLB_INVALID_MASK to make the page
> un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
> the TLB host address will be cached, and the following instructions can use
> this host address directly which may lead to the bypass of PMP related check.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---

For this commit I believe it's worth mentioning that it's partially reverting
commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
probe_access_internal") that was made to handle a particularity/quirk that was
present in s390x code.

At first glance this patch seems benign but we must make sure that no other
assumptions were made with this particular change in probe_access_internal().


Thanks,

Daniel

>   accel/tcg/cputlb.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index e984a98dc4..d0bf996405 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr,
>               /* TLB resize via tlb_fill may have moved the entry.  */
>               index = tlb_index(env, mmu_idx, addr);
>               entry = tlb_entry(env, mmu_idx, addr);
> -
> -            /*
> -             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
> -             * to force the next access through tlb_fill.  We've just
> -             * called tlb_fill, so we know that this entry *is* valid.
> -             */
> -            flags &= ~TLB_INVALID_MASK;
>           }
>           tlb_addr = tlb_read_ofs(entry, elt_ofs);
>       }

Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Weiwei Li]

On 2023/4/18 00:25, Daniel Henrique Barboza wrote:
>
>
> On 4/13/23 06:01, Weiwei Li wrote:
>> When PMP entry overlap part of the page, we'll set the tlb_size to 1, 
>> and
>> this will make the address set with TLB_INVALID_MASK to make the page
>> un-cached. However, if we clear TLB_INVALID_MASK when TLB is 
>> re-filled, then
>> the TLB host address will be cached, and the following instructions 
>> can use
>> this host address directly which may lead to the bypass of PMP 
>> related check.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>
> For this commit I believe it's worth mentioning that it's partially 
> reverting
> commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
> probe_access_internal") that was made to handle a particularity/quirk 
> that was
> present in s390x code.
>
> At first glance this patch seems benign but we must make sure that no 
> other
> assumptions were made with this particular change in 
> probe_access_internal().

I think this change will introduce no external function change except 
that we should

always walk the page table(fill_tlb) for memory access to that page. And 
this is needed

for pages that are partially overlapped by PMP region.

Regards,

Weiwei Li


>
>
> Thanks,
>
> Daniel
>
>>   accel/tcg/cputlb.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index e984a98dc4..d0bf996405 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState 
>> *env, target_ulong addr,
>>               /* TLB resize via tlb_fill may have moved the entry.  */
>>               index = tlb_index(env, mmu_idx, addr);
>>               entry = tlb_entry(env, mmu_idx, addr);
>> -
>> -            /*
>> -             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK 
>> immediately,
>> -             * to force the next access through tlb_fill. We've just
>> -             * called tlb_fill, so we know that this entry *is* valid.
>> -             */
>> -            flags &= ~TLB_INVALID_MASK;
>>           }
>>           tlb_addr = tlb_read_ofs(entry, elt_ofs);
>>       }

Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Richard Henderson]

On 4/17/23 18:25, Daniel Henrique Barboza wrote:
> 
> 
> On 4/13/23 06:01, Weiwei Li wrote:
>> When PMP entry overlap part of the page, we'll set the tlb_size to 1, and
>> this will make the address set with TLB_INVALID_MASK to make the page
>> un-cached. However, if we clear TLB_INVALID_MASK when TLB is re-filled, then
>> the TLB host address will be cached, and the following instructions can use
>> this host address directly which may lead to the bypass of PMP related check.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
> 
> For this commit I believe it's worth mentioning that it's partially reverting
> commit c3c8bf579b431b6b ("accel/tcg: Suppress auto-invalidate in
> probe_access_internal") that was made to handle a particularity/quirk that was
> present in s390x code.
> 
> At first glance this patch seems benign but we must make sure that no other
> assumptions were made with this particular change in probe_access_internal().
> 
> 
> Thanks,
> 
> Daniel
> 
>>   accel/tcg/cputlb.c | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index e984a98dc4..d0bf996405 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1563,13 +1563,6 @@ static int probe_access_internal(CPUArchState *env, target_ulong 
>> addr,
>>               /* TLB resize via tlb_fill may have moved the entry.  */
>>               index = tlb_index(env, mmu_idx, addr);
>>               entry = tlb_entry(env, mmu_idx, addr);
>> -
>> -            /*
>> -             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
>> -             * to force the next access through tlb_fill.  We've just
>> -             * called tlb_fill, so we know that this entry *is* valid.
>> -             */
>> -            flags &= ~TLB_INVALID_MASK;


I missed the original patch, but this is definitely wrong.

Clearing this bit locally (!) is correct because we want to inform the caller of 
probe_access_* that the access is valid.  We know that it is valid because we have just 
queried tlb_fill (and thus for riscv, PMP).

Clearing the bit locally does *not* cause the tlb entry to be cached -- the INVALID bit is 
still set within the tlb entry.  The next access will again go through tlb_fill.

What is the original problem you are seeing?  The commit message does not say.


r~

Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Richard Henderson]

On 4/18/23 09:18, Richard Henderson wrote:
>>> -            /*
>>> -             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately,
>>> -             * to force the next access through tlb_fill.  We've just
>>> -             * called tlb_fill, so we know that this entry *is* valid.
>>> -             */
>>> -            flags &= ~TLB_INVALID_MASK;
> 
> 
> I missed the original patch, but this is definitely wrong.
> 
> Clearing this bit locally (!) is correct because we want to inform the caller of 
> probe_access_* that the access is valid.  We know that it is valid because we have just 
> queried tlb_fill (and thus for riscv, PMP).
> 
> Clearing the bit locally does *not* cause the tlb entry to be cached -- the INVALID bit is 
> still set within the tlb entry.  The next access will again go through tlb_fill.
> 
> What is the original problem you are seeing?  The commit message does not say.

 From https://lore.kernel.org/qemu-devel/3ace9e9e-91cf-36e6-a18f-494fd44dffab@iscas.ac.cn/
I see that it is a problem with execution.

By eye, it appears that get_page_addr_code_hostp needs adjustment, e.g.

     (void)probe_access_internal(env, addr, 1, MMU_INST_FETCH,
                                 cpu_mmu_index(env, true), false, &p, &full, 0);
     if (p == NULL) {
         return -1;
     }
+   if (full->lg_page_size < TARGET_PAGE_BITS) {
+       return -1;
+   }
     if (hostp) {
         *hostp = p;
     }

It seems like we could do slightly better than this, perhaps by single-stepping through 
such a page, but surely this edge case is so uncommon as to not make it worthwhile to 
consider.


r~

Re: [PATCH 6/6] accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is re-filled

[Weiwei Li]

On 2023/4/18 15:36, Richard Henderson wrote:
> On 4/18/23 09:18, Richard Henderson wrote:
>>>> -            /*
>>>> -             * With PAGE_WRITE_INV, we set TLB_INVALID_MASK 
>>>> immediately,
>>>> -             * to force the next access through tlb_fill. We've just
>>>> -             * called tlb_fill, so we know that this entry *is* 
>>>> valid.
>>>> -             */
>>>> -            flags &= ~TLB_INVALID_MASK;
>>
>>
>> I missed the original patch, but this is definitely wrong.
>>
>> Clearing this bit locally (!) is correct because we want to inform 
>> the caller of probe_access_* that the access is valid. We know that 
>> it is valid because we have just queried tlb_fill (and thus for 
>> riscv, PMP).
>>
>> Clearing the bit locally does *not* cause the tlb entry to be cached 
>> -- the INVALID bit is still set within the tlb entry. The next access 
>> will again go through tlb_fill.
>>
>> What is the original problem you are seeing?  The commit message does 
>> not say.
>
> From 
> https://lore.kernel.org/qemu-devel/3ace9e9e-91cf-36e6-a18f-494fd44dffab@iscas.ac.cn/
> I see that it is a problem with execution.
Yeah. I found this problem in PMP check for instruction fetch.
>
> By eye, it appears that get_page_addr_code_hostp needs adjustment, e.g.
>
>     (void)probe_access_internal(env, addr, 1, MMU_INST_FETCH,
>                                 cpu_mmu_index(env, true), false, &p, 
> &full, 0);
>     if (p == NULL) {
>         return -1;
>     }
> +   if (full->lg_page_size < TARGET_PAGE_BITS) {
> +       return -1;
> +   }
>     if (hostp) {
>         *hostp = p;
>     }
>
> It seems like we could do slightly better than this, perhaps by 
> single-stepping through such a page, but surely this edge case is so 
> uncommon as to not make it worthwhile to consider.

OK. I'll  update and test it later.

Regards,

Weiwei Li

>
>
> r~

Re: [PATCH 0/6] target/riscv: Fix PMP related problem

[LIU Zhiwei]

On 2023/4/13 17:01, Weiwei Li wrote:
> This patchset tries to fix the PMP bypass problem issue https://gitlab.com/qemu-project/qemu/-/issues/1542

Please add your analysis of this issue here.

By the way, I think this problem is introduced by

https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html

I have commented on how to correct this patch. But by accident, it has 
been merged.

Zhiwei

>
> The port is available here:
> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>
> Weiwei Li (6):
>    target/riscv: Update pmp_get_tlb_size()
>    target/riscv: Move pmp_get_tlb_size apart from
>      get_physical_address_pmp
>    target/riscv: flush tlb when pmpaddr is updated
>    target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>    target/riscv: flush tb when PMP entry changes
>    accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>      re-filled
>
>   accel/tcg/cputlb.c        |  7 -----
>   target/riscv/cpu_helper.c | 19 ++++---------
>   target/riscv/pmp.c        | 60 ++++++++++++++++++++++++++-------------
>   target/riscv/pmp.h        |  3 +-
>   4 files changed, 47 insertions(+), 42 deletions(-)
>

Re: [PATCH 0/6] target/riscv: Fix PMP related problem

[Weiwei Li]

On 2023/4/18 11:07, LIU Zhiwei wrote:
>
> On 2023/4/13 17:01, Weiwei Li wrote:
>> This patchset tries to fix the PMP bypass problem issue 
>> https://gitlab.com/qemu-project/qemu/-/issues/1542
>
> Please add your analysis of this issue here.
>
> By the way, I think this problem is introduced by
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html

It seems have no relationship with this commit.

I think there are several problems for this issue:

1. TLB will not be cached only when the access address have matched PMP 
entry.  So the other address access  may hit the TLB(if first access of 
the page didn't hit the PMP entry)

and bypass the pmp check. This is fixed by patch 1.

2. Writing to pmpaddr  didn't trigger tlb flush. This is fixed by patch 3.

3. The tb isn't flushed when PMP permission changes, so It also may hit 
the tb and bypass the changed PMP check for instruction fetch. This is 
fixed by patch 5.

4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However 
this flag will be cleared after fill_tlb, and this will make the host 
address be cached, and let the following instruction fetch in the same 
tb bypass the PMP check. This is fixed by patch 6.

Regards,

Weiwei Li

>
> I have commented on how to correct this patch. But by accident, it has 
> been merged.
>
> Zhiwei
>
>>
>> The port is available here:
>> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>>
>> Weiwei Li (6):
>>    target/riscv: Update pmp_get_tlb_size()
>>    target/riscv: Move pmp_get_tlb_size apart from
>>      get_physical_address_pmp
>>    target/riscv: flush tlb when pmpaddr is updated
>>    target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>>    target/riscv: flush tb when PMP entry changes
>>    accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>>      re-filled
>>
>>   accel/tcg/cputlb.c        |  7 -----
>>   target/riscv/cpu_helper.c | 19 ++++---------
>>   target/riscv/pmp.c        | 60 ++++++++++++++++++++++++++-------------
>>   target/riscv/pmp.h        |  3 +-
>>   4 files changed, 47 insertions(+), 42 deletions(-)
>>

Re: [PATCH 0/6] target/riscv: Fix PMP related problem

[LIU Zhiwei]

On 2023/4/18 11:36, Weiwei Li wrote:
>
> On 2023/4/18 11:07, LIU Zhiwei wrote:
>>
>> On 2023/4/13 17:01, Weiwei Li wrote:
>>> This patchset tries to fix the PMP bypass problem issue 
>>> https://gitlab.com/qemu-project/qemu/-/issues/1542
>>
>> Please add your analysis of this issue here.
>>
>> By the way, I think this problem is introduced by
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
>
> It seems have no relationship with this commit.
>
> I think there are several problems for this issue:
>
> 1. TLB will not be cached only when the access address have matched 
> PMP entry. 
TLB will be filled only when PMP check and PTW check pass.
> So the other address access  may hit the TLB(if first access of the 
> page didn't hit the PMP entry)
This page will not be filled to TLB if the first access of the page 
didn't pass the PMP check.
>
> and bypass the pmp check. This is fixed by patch 1.

Never it should be.

Zhiwei

>
> 2. Writing to pmpaddr  didn't trigger tlb flush. This is fixed by 
> patch 3.
>
> 3. The tb isn't flushed when PMP permission changes, so It also may 
> hit the tb and bypass the changed PMP check for instruction fetch. 
> This is fixed by patch 5.
>
> 4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However 
> this flag will be cleared after fill_tlb, and this will make the host 
> address be cached, and let the following instruction fetch in the same 
> tb bypass the PMP check. This is fixed by patch 6.
>
> Regards,
>
> Weiwei Li
>
>>
>> I have commented on how to correct this patch. But by accident, it 
>> has been merged.
>>
>> Zhiwei
>>
>>>
>>> The port is available here:
>>> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>>>
>>> Weiwei Li (6):
>>>    target/riscv: Update pmp_get_tlb_size()
>>>    target/riscv: Move pmp_get_tlb_size apart from
>>>      get_physical_address_pmp
>>>    target/riscv: flush tlb when pmpaddr is updated
>>>    target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>>>    target/riscv: flush tb when PMP entry changes
>>>    accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>>>      re-filled
>>>
>>>   accel/tcg/cputlb.c        |  7 -----
>>>   target/riscv/cpu_helper.c | 19 ++++---------
>>>   target/riscv/pmp.c        | 60 
>>> ++++++++++++++++++++++++++-------------
>>>   target/riscv/pmp.h        |  3 +-
>>>   4 files changed, 47 insertions(+), 42 deletions(-)
>>>

Re: [PATCH 0/6] target/riscv: Fix PMP related problem

[Weiwei Li]

On 2023/4/18 12:47, LIU Zhiwei wrote:
>
> On 2023/4/18 11:36, Weiwei Li wrote:
>>
>> On 2023/4/18 11:07, LIU Zhiwei wrote:
>>>
>>> On 2023/4/13 17:01, Weiwei Li wrote:
>>>> This patchset tries to fix the PMP bypass problem issue 
>>>> https://gitlab.com/qemu-project/qemu/-/issues/1542
>>>
>>> Please add your analysis of this issue here.
>>>
>>> By the way, I think this problem is introduced by
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg939331.html
>>
>> It seems have no relationship with this commit.
>>
>> I think there are several problems for this issue:
>>
>> 1. TLB will not be cached only when the access address have matched 
>> PMP entry. 
> TLB will be filled only when PMP check and PTW check pass.
>> So the other address access  may hit the TLB(if first access of the 
>> page didn't hit the PMP entry)
> This page will not be filled to TLB if the first access of the page 
> didn't pass the PMP check.

I have given an example for this bypass in the replied email of patch 1.

Regards,

Weiwei Li

>>
>> and bypass the pmp check. This is fixed by patch 1.
>
> Never it should be.
>
> Zhiwei
>
>>
>> 2. Writing to pmpaddr  didn't trigger tlb flush. This is fixed by 
>> patch 3.
>>
>> 3. The tb isn't flushed when PMP permission changes, so It also may 
>> hit the tb and bypass the changed PMP check for instruction fetch. 
>> This is fixed by patch 5.
>>
>> 4. We set the tlb_size to 1 to make the TLB_INVALID_MASK set. However 
>> this flag will be cleared after fill_tlb, and this will make the host 
>> address be cached, and let the following instruction fetch in the 
>> same tb bypass the PMP check. This is fixed by patch 6.
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>> I have commented on how to correct this patch. But by accident, it 
>>> has been merged.
>>>
>>> Zhiwei
>>>
>>>>
>>>> The port is available here:
>>>> https://github.com/plctlab/plct-qemu/tree/plct-pmp-fix
>>>>
>>>> Weiwei Li (6):
>>>>    target/riscv: Update pmp_get_tlb_size()
>>>>    target/riscv: Move pmp_get_tlb_size apart from
>>>>      get_physical_address_pmp
>>>>    target/riscv: flush tlb when pmpaddr is updated
>>>>    target/riscv: Flush TLB only when pmpcfg/pmpaddr really changes
>>>>    target/riscv: flush tb when PMP entry changes
>>>>    accel/tcg: Remain TLB_INVALID_MASK in the address when TLB is
>>>>      re-filled
>>>>
>>>>   accel/tcg/cputlb.c        |  7 -----
>>>>   target/riscv/cpu_helper.c | 19 ++++---------
>>>>   target/riscv/pmp.c        | 60 
>>>> ++++++++++++++++++++++++++-------------
>>>>   target/riscv/pmp.h        |  3 +-
>>>>   4 files changed, 47 insertions(+), 42 deletions(-)
>>>>