1. Patchew
  2. QEMU
  3. View series

[PATCH] target/riscv/pmp: fix no pmp illegal intrs

Nikita Shubin posted 1 patch 4 years, 1 month ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211214092659.15709-1-nikita.shubin@maquefel.me
Maintainers: Alistair Francis <alistair.francis@wdc.com>, Palmer Dabbelt <palmer@dabbelt.com>, Bin Meng <bin.meng@windriver.com>
There is a newer version of this series
target/riscv/op_helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Nikita Shubin 4 years, 1 month ago 
From: Nikita Shubin <n.shubin@yadro.com>

As per the privilege specification, any access from S/U mode should fail
if no pmp region is configured and pmp is present, othwerwise access
should succeed.

Fixes: d102f19a208 (target/riscv/pmp: Raise exception if no PMP entry is configured)
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 target/riscv/op_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index ee7c24efe7..58d992e98a 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -146,7 +146,8 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
     uint64_t mstatus = env->mstatus;
     target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
 
-    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
+    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
+        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
         riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
     }
 
-- 
2.31.1
Re: [PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Richard Henderson 4 years, 1 month ago 
On 12/14/21 1:26 AM, Nikita Shubin wrote:
> -    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> +    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {

When would the number of rules become non-zero with PMP disabled?
When does this test make a difference?


r~
Re: [PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Richard Henderson 4 years, 1 month ago 
On 12/14/21 1:12 PM, Richard Henderson wrote:
> On 12/14/21 1:26 AM, Nikita Shubin wrote:
>> -    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
>> +    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> 
> When would the number of rules become non-zero with PMP disabled?
> When does this test make a difference?

Oh, nevermind, I see what you mean.


r~
Re: [PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Nikita Shubin 4 years, 1 month ago 
Hello Richard!

On Tue, 14 Dec 2021 13:13:57 -0800
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 12/14/21 1:12 PM, Richard Henderson wrote:
> > On 12/14/21 1:26 AM, Nikita Shubin wrote:  
> >> -    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> >> +    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {  
> > 
> > When would the number of rules become non-zero with PMP disabled?
> > When does this test make a difference?  
> 
> Oh, nevermind, I see what you mean.
> 

Np, let me explain in details:

The ISA states:
> Platforms vary widely in demands for physical memory protection, and
> some platforms may provide other PMP structures in addition to or
> instead of the scheme described in this section.

So we might don't have PMP at all, but if we set

qdev_prop_set_bit(DEVICE(obj), "pmp", false);

for some CPU we still end up in illegal inst on mret, cause we get
pmp_get_num_rules(env) == 0, becouse we have no PMP which leads to zero
available rules.


> 
> r~
>
Re: [PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Alistair Francis 4 years, 1 month ago 
On Wed, Dec 15, 2021 at 1:00 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> As per the privilege specification, any access from S/U mode should fail
> if no pmp region is configured and pmp is present, othwerwise access
> should succeed.
>
> Fixes: d102f19a208 (target/riscv/pmp: Raise exception if no PMP entry is configured)
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>

Whoops!

I sent a patch to fix the exact same issue :)

I'll drop mine and we can merge yours. Do you mind adding this and
resending the patch

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/585

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

Alistair

> ---
>  target/riscv/op_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index ee7c24efe7..58d992e98a 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -146,7 +146,8 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
>      uint64_t mstatus = env->mstatus;
>      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>
> -    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> +    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
>          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>      }
>
> --
> 2.31.1
>
>
[PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Nikita Shubin 4 years, 1 month ago 
From: Nikita Shubin <n.shubin@yadro.com>

As per the privilege specification, any access from S/U mode should fail
if no pmp region is configured and pmp is present, othwerwise access
should succeed.

Fixes: d102f19a208 (target/riscv/pmp: Raise exception if no PMP entry is configured)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/585
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/op_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index ee7c24efe7..58d992e98a 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -146,7 +146,8 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
     uint64_t mstatus = env->mstatus;
     target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
 
-    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
+    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
+        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
         riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
     }
 
-- 
2.31.1
Re: [PATCH] target/riscv/pmp: fix no pmp illegal intrs
Posted by Alistair Francis 4 years, 1 month ago 
On Tue, Dec 21, 2021 at 6:08 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> As per the privilege specification, any access from S/U mode should fail
> if no pmp region is configured and pmp is present, othwerwise access
> should succeed.
>
> Fixes: d102f19a208 (target/riscv/pmp: Raise exception if no PMP entry is configured)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/585
> Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/op_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index ee7c24efe7..58d992e98a 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -146,7 +146,8 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong cpu_pc_deb)
>      uint64_t mstatus = env->mstatus;
>      target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>
> -    if (!pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
> +    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> +        !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
>          riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
>      }
>
> --
> 2.31.1
>
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.