[v2] Fix Zjpm implementation

[PATCH v2 2/6] target/riscv: Add a helper to return the current effective priv mode

Posted by frank.chang@sifive.com 2 months, 2 weeks ago

From: Frank Chang <frank.chang@sifive.com>

This helper returns the current effective privilege mode.

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_helper.c | 55 ++++++++++++++++++++++++++++++++-------
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 8899bf7667a..ab285d7a6d1 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -607,6 +607,7 @@ target_ulong riscv_cpu_get_geilen(CPURISCVState *env);
 void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
 bool riscv_cpu_vector_enabled(CPURISCVState *env);
 void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
+bool riscv_cpu_eff_priv(CPURISCVState *env, int *priv, bool *virt);
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
 bool cpu_get_fcfien(CPURISCVState *env);
 bool cpu_get_bcfien(CPURISCVState *env);
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index dd6c861a90e..fbab8177092 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -38,6 +38,46 @@
 #include "pmp.h"
 #include "qemu/plugin.h"
 
+/*
+ * Returns the current effective privilege mode.
+ *
+ * @env: CPURISCVState
+ * @priv: The returned effective privilege mode.
+ * @virt: The returned effective virtualization mode.
+ *
+ * Returns true if the effective privilege mode is modified.
+ */
+bool riscv_cpu_eff_priv(CPURISCVState *env, int *priv, bool *virt)
+{
+#ifndef CONFIG_USER_ONLY
+    int mode = env->priv;
+    bool virt_enabled = env->virt_enabled;
+    bool mode_modified = false;
+
+#ifndef CONFIG_USER_ONLY
+    if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) {
+        mode = get_field(env->mstatus, MSTATUS_MPP);
+        virt_enabled = get_field(env->mstatus, MSTATUS_MPV) && (mode != PRV_M);
+        mode_modified = true;
+    }
+#endif
+
+    if (priv) {
+        *priv = mode;
+    }
+
+    if (virt) {
+        *virt = virt_enabled;
+    }
+
+    return mode_modified;
+#else
+    *priv = env->priv;
+    *virt = false;
+    return false;
+#endif
+}
+
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
 {
 #ifdef CONFIG_USER_ONLY
@@ -45,19 +85,14 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
 #else
     bool virt = env->virt_enabled;
     int mode = env->priv;
+    bool mode_modified = false;
 
     /* All priv -> mmu_idx mapping are here */
     if (!ifetch) {
-        uint64_t status = env->mstatus;
-
-        if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
-            mode = get_field(env->mstatus, MSTATUS_MPP);
-            virt = get_field(env->mstatus, MSTATUS_MPV) &&
-                   (mode != PRV_M);
-            if (virt) {
-                status = env->vsstatus;
-            }
-        }
+        mode_modified = riscv_cpu_eff_priv(env, &mode, &virt);
+        uint64_t status = (mode_modified && virt) ? env->vsstatus :
+                                                    env->mstatus;
+
         if (mode == PRV_S && get_field(status, MSTATUS_SUM)) {
             mode = MMUIdx_S_SUM;
         }
-- 
2.43.0

Re: [PATCH v2 2/6] target/riscv: Add a helper to return the current effective priv mode

Posted by Radim Krčmář 2 months, 2 weeks ago

2025-11-21T13:04:09+08:00, <frank.chang@sifive.com>:
> From: Frank Chang <frank.chang@sifive.com>
>
> This helper returns the current effective privilege mode.
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> @@ -38,6 +38,46 @@
> +bool riscv_cpu_eff_priv(CPURISCVState *env, int *priv, bool *virt)

I wonder if this function shouldn't be defined in a header file, so it
can be inlined, because returning values through pointers is quite
inefficient,

> +{
> +#ifndef CONFIG_USER_ONLY
> +    int mode = env->priv;
> +    bool virt_enabled = env->virt_enabled;
> +    bool mode_modified = false;
> +
> +#ifndef CONFIG_USER_ONLY

We know CONFIG_USER_ONLY is not defined at this point.

> +    if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) {
> +        mode = get_field(env->mstatus, MSTATUS_MPP);
> +        virt_enabled = get_field(env->mstatus, MSTATUS_MPV) && (mode != PRV_M);
> +        mode_modified = true;
> +    }
> +#endif
> +
> +    if (priv) {
> +        *priv = mode;
> +    }
> +
> +    if (virt) {
> +        *virt = virt_enabled;
> +    }
> +
> +    return mode_modified;
> +#else
> +    *priv = env->priv;

Since it's #ifdef CONFIG_USER_ONLY, we can just say

       *priv = PRV_U;


> +    *virt = false;
> +    return false;
> +#endif
> +}
> +
>  int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>  {
>  #ifdef CONFIG_USER_ONLY
> @@ -45,19 +85,14 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
>  #else
>      bool virt = env->virt_enabled;
>      int mode = env->priv;
> +    bool mode_modified = false;
>  
>      /* All priv -> mmu_idx mapping are here */
>      if (!ifetch) {
> -        uint64_t status = env->mstatus;
> -
> -        if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> -            mode = get_field(env->mstatus, MSTATUS_MPP);
> -            virt = get_field(env->mstatus, MSTATUS_MPV) &&
> -                   (mode != PRV_M);
> -            if (virt) {
> -                status = env->vsstatus;
> -            }
> -        }
> +        mode_modified = riscv_cpu_eff_priv(env, &mode, &virt);
> +        uint64_t status = (mode_modified && virt) ? env->vsstatus :
> +                                                    env->mstatus;

It is likely a bug that MPRV=1+MPV=1 behaves differently from virt=1,
but your patch preserves the current behavior, as it should.

I had a few nitpicks, but important parts seem fine

Reviewed-by: Radim Krčmář <rkrcmar@ventanamicro.com>

Thanks.

Re: [PATCH v2 2/6] target/riscv: Add a helper to return the current effective priv mode

Posted by Frank Chang 2 months ago

Hi Radim,

On Tue, Nov 25, 2025 at 10:59 PM Radim Krčmář <rkrcmar@ventanamicro.com>
wrote:

> 2025-11-21T13:04:09+08:00, <frank.chang@sifive.com>:
> > From: Frank Chang <frank.chang@sifive.com>
> >
> > This helper returns the current effective privilege mode.
> >
> > Signed-off-by: Frank Chang <frank.chang@sifive.com>
> > ---
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > @@ -38,6 +38,46 @@
> > +bool riscv_cpu_eff_priv(CPURISCVState *env, int *priv, bool *virt)
>
> I wonder if this function shouldn't be defined in a header file, so it
>

Are you saying that we "should" define it in a header file?


> can be inlined, because returning values through pointers is quite
> inefficient,


> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +    int mode = env->priv;
> > +    bool virt_enabled = env->virt_enabled;
> > +    bool mode_modified = false;
> > +
> > +#ifndef CONFIG_USER_ONLY
>
> We know CONFIG_USER_ONLY is not defined at this point.
>
> > +    if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) {
> > +        mode = get_field(env->mstatus, MSTATUS_MPP);
> > +        virt_enabled = get_field(env->mstatus, MSTATUS_MPV) && (mode !=
> PRV_M);
> > +        mode_modified = true;
> > +    }
> > +#endif
> > +
> > +    if (priv) {
> > +        *priv = mode;
> > +    }
> > +
> > +    if (virt) {
> > +        *virt = virt_enabled;
> > +    }
> > +
> > +    return mode_modified;
> > +#else
> > +    *priv = env->priv;
>
> Since it's #ifdef CONFIG_USER_ONLY, we can just say
>
>        *priv = PRV_U;
>
>
> > +    *virt = false;
> > +    return false;
> > +#endif
> > +}
> > +
> >  int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
> >  {
> >  #ifdef CONFIG_USER_ONLY
> > @@ -45,19 +85,14 @@ int riscv_env_mmu_index(CPURISCVState *env, bool
> ifetch)
> >  #else
> >      bool virt = env->virt_enabled;
> >      int mode = env->priv;
> > +    bool mode_modified = false;
> >
> >      /* All priv -> mmu_idx mapping are here */
> >      if (!ifetch) {
> > -        uint64_t status = env->mstatus;
> > -
> > -        if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> > -            mode = get_field(env->mstatus, MSTATUS_MPP);
> > -            virt = get_field(env->mstatus, MSTATUS_MPV) &&
> > -                   (mode != PRV_M);
> > -            if (virt) {
> > -                status = env->vsstatus;
> > -            }
> > -        }
> > +        mode_modified = riscv_cpu_eff_priv(env, &mode, &virt);
> > +        uint64_t status = (mode_modified && virt) ? env->vsstatus :
> > +                                                    env->mstatus;
>
> It is likely a bug that MPRV=1+MPV=1 behaves differently from virt=1,
> but your patch preserves the current behavior, as it should.
>
> I had a few nitpicks, but important parts seem fine
>
> Reviewed-by: Radim Krčmář <rkrcmar@ventanamicro.com>
>
> Thanks.
>

[PATCH v2 1/6] target/riscv: fix address masking
[PATCH v2 2/6] target/riscv: Add a helper to return the current effective priv mode
[PATCH v2 3/6] target/riscv: Fix pointer masking PMM field selection logic
[PATCH v2 4/6] target/riscv: Fix pointer masking for virtual-machine load/store insns
[PATCH v2 5/6] target/riscv: Rename riscv_pm_get_virt_pmm() to riscv_pm_get_vm_ldst_pmm()
[PATCH v2 6/6] target/riscv: Fix pointer masking translation mode check bug