* [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
@ 2023-02-09 5:52 Himanshu Chauhan
2023-02-09 9:51 ` Daniel Henrique Barboza
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Himanshu Chauhan @ 2023-02-09 5:52 UTC (permalink / raw)
To: qemu-riscv; +Cc: qemu-devel, Himanshu Chauhan
When MSECCFG.MML is set, after checking the address range in PMP if the
asked permissions are not same as programmed in PMP, the default
permissions are applied. This should only be the case when there
is no matching address is found.
This patch skips applying default rules when matching address range
is found. It returns the index of the match PMP entry.
fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
---
target/riscv/pmp.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index d85ad07caa..0dfdb35828 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
}
}
- if ((privs & *allowed_privs) == privs) {
- ret = i;
- }
+ /*
+ * If matching address range was found, the protection bits
+ * defined with PMP must be used. We shouldn't fallback on
+ * finding default privileges.
+ */
+ ret = i;
break;
}
}
--
2.39.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
2023-02-09 5:52 [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches Himanshu Chauhan
@ 2023-02-09 9:51 ` Daniel Henrique Barboza
2023-02-09 23:39 ` Alistair Francis
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2023-02-09 9:51 UTC (permalink / raw)
To: Himanshu Chauhan, qemu-riscv; +Cc: qemu-devel
On 2/9/23 02:52, Himanshu Chauhan wrote:
> When MSECCFG.MML is set, after checking the address range in PMP if the
> asked permissions are not same as programmed in PMP, the default
> permissions are applied. This should only be the case when there
> is no matching address is found.
>
> This patch skips applying default rules when matching address range
> is found. It returns the index of the match PMP entry.
>
> fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
Nit: tag starts with capital "F":
Fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/pmp.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index d85ad07caa..0dfdb35828 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> }
> }
>
> - if ((privs & *allowed_privs) == privs) {
> - ret = i;
> - }
> + /*
> + * If matching address range was found, the protection bits
> + * defined with PMP must be used. We shouldn't fallback on
> + * finding default privileges.
> + */
> + ret = i;
> break;
> }
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
2023-02-09 5:52 [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches Himanshu Chauhan
2023-02-09 9:51 ` Daniel Henrique Barboza
@ 2023-02-09 23:39 ` Alistair Francis
2023-02-10 2:41 ` Alistair Francis
2023-02-13 4:22 ` LIU Zhiwei
3 siblings, 0 replies; 7+ messages in thread
From: Alistair Francis @ 2023-02-09 23:39 UTC (permalink / raw)
To: Himanshu Chauhan; +Cc: qemu-riscv, qemu-devel
On Thu, Feb 9, 2023 at 3:53 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> When MSECCFG.MML is set, after checking the address range in PMP if the
> asked permissions are not same as programmed in PMP, the default
> permissions are applied. This should only be the case when there
> is no matching address is found.
>
> This patch skips applying default rules when matching address range
> is found. It returns the index of the match PMP entry.
>
> fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/pmp.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index d85ad07caa..0dfdb35828 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> }
> }
>
> - if ((privs & *allowed_privs) == privs) {
> - ret = i;
> - }
> + /*
> + * If matching address range was found, the protection bits
> + * defined with PMP must be used. We shouldn't fallback on
> + * finding default privileges.
> + */
> + ret = i;
> break;
> }
> }
> --
> 2.39.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
2023-02-09 5:52 [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches Himanshu Chauhan
2023-02-09 9:51 ` Daniel Henrique Barboza
2023-02-09 23:39 ` Alistair Francis
@ 2023-02-10 2:41 ` Alistair Francis
2023-02-13 4:22 ` LIU Zhiwei
3 siblings, 0 replies; 7+ messages in thread
From: Alistair Francis @ 2023-02-10 2:41 UTC (permalink / raw)
To: Himanshu Chauhan; +Cc: qemu-riscv, qemu-devel
On Thu, Feb 9, 2023 at 3:53 PM Himanshu Chauhan
<hchauhan@ventanamicro.com> wrote:
>
> When MSECCFG.MML is set, after checking the address range in PMP if the
> asked permissions are not same as programmed in PMP, the default
> permissions are applied. This should only be the case when there
> is no matching address is found.
>
> This patch skips applying default rules when matching address range
> is found. It returns the index of the match PMP entry.
>
> fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> target/riscv/pmp.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index d85ad07caa..0dfdb35828 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> }
> }
>
> - if ((privs & *allowed_privs) == privs) {
> - ret = i;
> - }
> + /*
> + * If matching address range was found, the protection bits
> + * defined with PMP must be used. We shouldn't fallback on
> + * finding default privileges.
> + */
> + ret = i;
> break;
> }
> }
> --
> 2.39.1
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
2023-02-09 5:52 [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches Himanshu Chauhan
` (2 preceding siblings ...)
2023-02-10 2:41 ` Alistair Francis
@ 2023-02-13 4:22 ` LIU Zhiwei
2023-02-13 5:21 ` Himanshu Chauhan
3 siblings, 1 reply; 7+ messages in thread
From: LIU Zhiwei @ 2023-02-13 4:22 UTC (permalink / raw)
To: Himanshu Chauhan, qemu-riscv, Alistair Francis, Bin Meng,
liweiwei, dbarboza
Cc: qemu-devel
On 2023/2/9 13:52, Himanshu Chauhan wrote:
> When MSECCFG.MML is set, after checking the address range in PMP if the
> asked permissions are not same as programmed in PMP, the default
> permissions are applied. This should only be the case when there
> is no matching address is found.
>
> This patch skips applying default rules when matching address range
> is found. It returns the index of the match PMP entry.
>
> fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
>
> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
> ---
> target/riscv/pmp.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index d85ad07caa..0dfdb35828 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
> }
> }
>
> - if ((privs & *allowed_privs) == privs) {
> - ret = i;
> - }
> + /*
> + * If matching address range was found, the protection bits
> + * defined with PMP must be used. We shouldn't fallback on
> + * finding default privileges.
> + */
> + ret = i;
Notice the return value is the matching rule index, which includes
1) the address range is matching.
2) the permission of the PMP rule and the memory access type are matching.
So we can't simply remove the second check. I think the right fix is:
if ((privs & *allowed_privs) == privs) {
ret = i;
- }
+ } else {
+ ret = -2;
+ }
The -2 return value avoids finding the default privileges. And it implies no matching rule is found.
Zhiwei
> break;
> }
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
2023-02-13 4:22 ` LIU Zhiwei
@ 2023-02-13 5:21 ` Himanshu Chauhan
2023-02-13 5:42 ` LIU Zhiwei
0 siblings, 1 reply; 7+ messages in thread
From: Himanshu Chauhan @ 2023-02-13 5:21 UTC (permalink / raw)
To: LIU Zhiwei, qemu-riscv, Alistair Francis, Bin Meng, liweiwei,
dbarboza
Cc: qemu-devel
On 13/02/23 09:52, LIU Zhiwei wrote:
>
> On 2023/2/9 13:52, Himanshu Chauhan wrote:
>> When MSECCFG.MML is set, after checking the address range in PMP if the
>> asked permissions are not same as programmed in PMP, the default
>> permissions are applied. This should only be the case when there
>> is no matching address is found.
>>
>> This patch skips applying default rules when matching address range
>> is found. It returns the index of the match PMP entry.
>>
>> fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
>>
>> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
>> ---
>> target/riscv/pmp.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>> index d85ad07caa..0dfdb35828 100644
>> --- a/target/riscv/pmp.c
>> +++ b/target/riscv/pmp.c
>> @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env,
>> target_ulong addr,
>> }
>> }
>> - if ((privs & *allowed_privs) == privs) {
>> - ret = i;
>> - }
>> + /*
>> + * If matching address range was found, the protection bits
>> + * defined with PMP must be used. We shouldn't fallback on
>> + * finding default privileges.
>> + */
>> + ret = i;
>
> Notice the return value is the matching rule index, which includes
>
> 1) the address range is matching.
>
> 2) the permission of the PMP rule and the memory access type are
> matching.
>
>
> So we can't simply remove the second check. I think the right fix is:
>
> if ((privs & *allowed_privs) == privs) {
> ret = i;
> - }
> + } else {
> + ret = -2;
> + }
>
> The -2 return value avoids finding the default privileges. And it
> implies no matching rule is found.
>
> Zhiwei
Hi Zhiwei,
In case the address range is matched and MSECCFG.MML is set, the
permission in *allowed_privs* are binding. So if the index matching is
returned, the binding permissions are applied by the caller function.
Which case does my patch break?
- Himanshu
>
>> break;
>> }
>> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches
2023-02-13 5:21 ` Himanshu Chauhan
@ 2023-02-13 5:42 ` LIU Zhiwei
0 siblings, 0 replies; 7+ messages in thread
From: LIU Zhiwei @ 2023-02-13 5:42 UTC (permalink / raw)
To: Himanshu Chauhan, qemu-riscv, Alistair Francis, Bin Meng,
liweiwei, dbarboza
Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3533 bytes --]
On 2023/2/13 13:21, Himanshu Chauhan wrote:
>
> On 13/02/23 09:52, LIU Zhiwei wrote:
>>
>> On 2023/2/9 13:52, Himanshu Chauhan wrote:
>>> When MSECCFG.MML is set, after checking the address range in PMP if the
>>> asked permissions are not same as programmed in PMP, the default
>>> permissions are applied. This should only be the case when there
>>> is no matching address is found.
>>>
>>> This patch skips applying default rules when matching address range
>>> is found. It returns the index of the match PMP entry.
>>>
>>> fixes: 824cac681c3 (target/riscv: Fix PMP propagation for tlb)
>>>
>>> Signed-off-by: Himanshu Chauhan <hchauhan@ventanamicro.com>
>>> ---
>>> target/riscv/pmp.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
>>> index d85ad07caa..0dfdb35828 100644
>>> --- a/target/riscv/pmp.c
>>> +++ b/target/riscv/pmp.c
>>> @@ -446,9 +446,12 @@ int pmp_hart_has_privs(CPURISCVState *env,
>>> target_ulong addr,
>>> }
>>> }
>>> - if ((privs & *allowed_privs) == privs) {
>>> - ret = i;
>>> - }
>>> + /*
>>> + * If matching address range was found, the protection
>>> bits
>>> + * defined with PMP must be used. We shouldn't fallback on
>>> + * finding default privileges.
>>> + */
>>> + ret = i;
>>
>> Notice the return value is the matching rule index, which includes
>>
>> 1) the address range is matching.
>>
>> 2) the permission of the PMP rule and the memory access type are
>> matching.
>>
>>
>> So we can't simply remove the second check. I think the right fix is:
>>
>> if ((privs & *allowed_privs) == privs) {
>> ret = i;
>> - }
>> + } else {
>> + ret = -2;
>> + }
>>
>> The -2 return value avoids finding the default privileges. And it
>> implies no matching rule is found.
>>
>> Zhiwei
>
> Hi Zhiwei,
>
> In case the address range is matched and MSECCFG.MML is set, the
> permission in *allowed_privs* are binding.
Yes.
> So if the index matching is returned, the binding permissions are
> applied by the caller function.
Yes. And the index will also be used. So we should check both conditions
in this function.
>
> Which case does my patch break?
Look at the get_physical_address_pmp which calls pmp_hart_has_privs,
pmp_index = pmp_hart_has_privs(env, addr, size, 1 << access_type,
&pmp_priv, mode);
if (pmp_index < 0) {
*prot = 0;
return TRANSLATE_PMP_FAIL;
}
*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);
}
returnTRANSLATE_SUCCESS;
You break the pmp_index < 0 condition. If ((privs & *allowed_privs) !=
privs, the get_physical_address_pmp should return fail.
Zhiwei
>
> - Himanshu
>
>>
>>> break;
>>> }
>>> }
[-- Attachment #2: Type: text/html, Size: 8316 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-02-13 5:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-09 5:52 [PATCH] target/riscv: Smepmp: Skip applying default rules when address matches Himanshu Chauhan
2023-02-09 9:51 ` Daniel Henrique Barboza
2023-02-09 23:39 ` Alistair Francis
2023-02-10 2:41 ` Alistair Francis
2023-02-13 4:22 ` LIU Zhiwei
2023-02-13 5:21 ` Himanshu Chauhan
2023-02-13 5:42 ` LIU Zhiwei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).