Fix Zjpm implementation

[PATCH 2/3] target/riscv: Fix pointer masking PMM field selection logic

Posted by frank.chang@sifive.com 2 days, 1 hour ago

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

mstatus.MPV only records the previous virtualization state, and does not
affect pointer masking according to the Zjpm specification.

This patch rewrites riscv_pm_get_pmm() to follow the architectural
definition of Smmpm, Smnpm, and Ssnpm.

The resulting PMM source for each mode is summarized below:

  * Smmpm + Smnpm + Ssnpm:
      M-mode:  mseccfg.PMM
      S-mode:  menvcfg.PMM
      U-mode:  senvcfg.PMM
      VS-mode: henvcfg.PMM
      VU-mode: senvcfg.PMM

  * Smmpm + Smnpm (RVS implemented):
      M-mode:  mseccfg.PMM
      S-mode:  menvcfg.PMM
      U/VS/VU: disabled (Ssnpm not present)

  * Smmpm + Smnpm (RVS not implemented):
      M-mode:  mseccfg.PMM
      U-mode:  menvcfg.PMM
      S/VS/VU: disabled (no S-mode)

  * Smmpm only:
      M-mode:  mseccfg.PMM
      Other existing modes: pointer masking disabled

Signed-off-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/cpu_helper.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index dd6c861a90e..112093012b0 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -154,22 +154,30 @@ RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
         }
         break;
     case PRV_S:
-        if (riscv_cpu_cfg(env)->ext_smnpm) {
-            if (get_field(env->mstatus, MSTATUS_MPV)) {
-                return get_field(env->henvcfg, HENVCFG_PMM);
-            } else {
+        if (!env->virt_enabled) {
+            if (riscv_cpu_cfg(env)->ext_smnpm) {
                 return get_field(env->menvcfg, MENVCFG_PMM);
             }
+        } else {
+            if (riscv_cpu_cfg(env)->ext_ssnpm) {
+                return get_field(env->henvcfg, HENVCFG_PMM);
+            }
         }
         break;
     case PRV_U:
-        if (riscv_has_ext(env, RVS)) {
+        if (!env->virt_enabled) {
             if (riscv_cpu_cfg(env)->ext_ssnpm) {
                 return get_field(env->senvcfg, SENVCFG_PMM);
             }
-        } else {
+
             if (riscv_cpu_cfg(env)->ext_smnpm) {
-                return get_field(env->menvcfg, MENVCFG_PMM);
+                if (!riscv_has_ext(env, RVS)) {
+                    return get_field(env->menvcfg, MENVCFG_PMM);
+                }
+            }
+        } else {
+            if (riscv_cpu_cfg(env)->ext_ssnpm) {
+                return get_field(env->senvcfg, SENVCFG_PMM);
             }
         }
         break;
-- 
2.43.0

Re: [PATCH 2/3] target/riscv: Fix pointer masking PMM field selection logic

Posted by Radim Krčmář 1 day, 22 hours ago

2025-11-18T18:59:35+08:00, <frank.chang@sifive.com>:
> From: Frank Chang <frank.chang@sifive.com>
>
> mstatus.MPV only records the previous virtualization state, and does not
> affect pointer masking according to the Zjpm specification.
>
> This patch rewrites riscv_pm_get_pmm() to follow the architectural
> definition of Smmpm, Smnpm, and Ssnpm.
>
> The resulting PMM source for each mode is summarized below:
>
>   * Smmpm + Smnpm + Ssnpm:
>       M-mode:  mseccfg.PMM
>       S-mode:  menvcfg.PMM
>       U-mode:  senvcfg.PMM
>       VS-mode: henvcfg.PMM
>       VU-mode: senvcfg.PMM
>
>   * Smmpm + Smnpm (RVS implemented):
>       M-mode:  mseccfg.PMM
>       S-mode:  menvcfg.PMM
>       U/VS/VU: disabled (Ssnpm not present)
>
>   * Smmpm + Smnpm (RVS not implemented):
>       M-mode:  mseccfg.PMM
>       U-mode:  menvcfg.PMM
>       S/VS/VU: disabled (no S-mode)
>
>   * Smmpm only:
>       M-mode:  mseccfg.PMM
>       Other existing modes: pointer masking disabled
>
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> @@ -154,22 +154,30 @@ RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
>          }
>          break;
>      case PRV_S:
> -        if (riscv_cpu_cfg(env)->ext_smnpm) {
> -            if (get_field(env->mstatus, MSTATUS_MPV)) {
> -                return get_field(env->henvcfg, HENVCFG_PMM);
> -            } else {
> +        if (!env->virt_enabled) {
> +            if (riscv_cpu_cfg(env)->ext_smnpm) {

It wasn't correct before, but it doesn't seem correct now either.
MPRV+MPV+MPP change the effective access mode to VS without setting
virt_enabled, and henvcfg is supposed to be used in that case.

I liked the way you described the desired behavior in the commit
message:

  M-mode:  mseccfg.PMM
  S-mode:  menvcfg.PMM
  U-mode:  senvcfg.PMM
  VS-mode: henvcfg.PMM
  VU-mode: senvcfg.PMM

Can we have a "switch (get_effective_access_mode(env))" with the same
structure?

Thanks.

---
Other bugs I noticed while skimming the adjust_addr_body() and
riscv_pm_get_pmm():
* Sign extension for HLV/HSV must be performed when vsatp.MODE != Bare.
* The sign extension also depends on the effective mode, and not on the
  current mode.
* MXR should set PMLEN=0 for all accesses that aren't M to M, not just
  when using MPRV.

Re: [PATCH 2/3] target/riscv: Fix pointer masking PMM field selection logic

Posted by Daniel Henrique Barboza 2 days, 1 hour ago


On 11/18/25 7:59 AM, frank.chang@sifive.com wrote:
> From: Frank Chang <frank.chang@sifive.com>
> 
> mstatus.MPV only records the previous virtualization state, and does not
> affect pointer masking according to the Zjpm specification.
> 
> This patch rewrites riscv_pm_get_pmm() to follow the architectural
> definition of Smmpm, Smnpm, and Ssnpm.
> 
> The resulting PMM source for each mode is summarized below:
> 
>    * Smmpm + Smnpm + Ssnpm:
>        M-mode:  mseccfg.PMM
>        S-mode:  menvcfg.PMM
>        U-mode:  senvcfg.PMM
>        VS-mode: henvcfg.PMM
>        VU-mode: senvcfg.PMM
> 
>    * Smmpm + Smnpm (RVS implemented):
>        M-mode:  mseccfg.PMM
>        S-mode:  menvcfg.PMM
>        U/VS/VU: disabled (Ssnpm not present)
> 
>    * Smmpm + Smnpm (RVS not implemented):
>        M-mode:  mseccfg.PMM
>        U-mode:  menvcfg.PMM
>        S/VS/VU: disabled (no S-mode)
> 
>    * Smmpm only:
>        M-mode:  mseccfg.PMM
>        Other existing modes: pointer masking disabled
> 
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/cpu_helper.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index dd6c861a90e..112093012b0 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -154,22 +154,30 @@ RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
>           }
>           break;
>       case PRV_S:
> -        if (riscv_cpu_cfg(env)->ext_smnpm) {
> -            if (get_field(env->mstatus, MSTATUS_MPV)) {
> -                return get_field(env->henvcfg, HENVCFG_PMM);
> -            } else {
> +        if (!env->virt_enabled) {
> +            if (riscv_cpu_cfg(env)->ext_smnpm) {
>                   return get_field(env->menvcfg, MENVCFG_PMM);
>               }
> +        } else {
> +            if (riscv_cpu_cfg(env)->ext_ssnpm) {
> +                return get_field(env->henvcfg, HENVCFG_PMM);
> +            }
>           }
>           break;
>       case PRV_U:
> -        if (riscv_has_ext(env, RVS)) {
> +        if (!env->virt_enabled) {
>               if (riscv_cpu_cfg(env)->ext_ssnpm) {
>                   return get_field(env->senvcfg, SENVCFG_PMM);
>               }
> -        } else {
> +
>               if (riscv_cpu_cfg(env)->ext_smnpm) {
> -                return get_field(env->menvcfg, MENVCFG_PMM);
> +                if (!riscv_has_ext(env, RVS)) {
> +                    return get_field(env->menvcfg, MENVCFG_PMM);
> +                }
> +            }
> +        } else {
> +            if (riscv_cpu_cfg(env)->ext_ssnpm) {
> +                return get_field(env->senvcfg, SENVCFG_PMM);
>               }
>           }
>           break;