[PATCH 0/3] Fix Zjpm implementation

[PATCH 0/3] Fix Zjpm implementation

[frank.chang]

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

The current Zjpm implementation has two issues:

1. The address is shifted before obtaining the correct PMLEN value.
2. riscv_pm_get_pmm() does not handle VS/VU modes correctly.

This patchset fixes the above issues and also renames
riscv_pm_get_virt_pmm() to riscv_pm_get_vm_ldst_pmm(), as the helper
is only used when checking the PMM configuration for virtual-machine
load/store instructions (HLV.* and HSV.*), rather than for VS/VU modes.

Frank Chang (2):
  target/riscv: Fix pointer masking PMM field selection logic
  target/riscv: Rename riscv_pm_get_virt_pmm() to
    riscv_pm_get_vm_ldst_pmm()

Yong-Xuan Wang (1):
  target/riscv: fix address masking

 target/riscv/cpu.h        |  2 +-
 target/riscv/cpu_helper.c | 24 ++++++++++++++++--------
 target/riscv/internals.h  |  4 ++--
 3 files changed, 19 insertions(+), 11 deletions(-)

--
2.43.0

[PATCH 1/3] target/riscv: fix address masking

[frank.chang]

From: Yong-Xuan Wang <yongxuan.wang@sifive.com>

The pmlen should get the corresponding value before shifting address.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 target/riscv/internals.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 172296f12e2..9b3f01144d2 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -203,8 +203,8 @@ static inline target_ulong adjust_addr_body(CPURISCVState *env,
     if (!is_virt_addr) {
         signext = riscv_cpu_virt_mem_enabled(env);
     }
-    addr = addr << pmlen;
     pmlen = riscv_pm_get_pmlen(pmm);
+    addr = addr << pmlen;
 
     /* sign/zero extend masked address by N-1 bit */
     if (signext) {
-- 
2.43.0

Re: [PATCH 1/3] target/riscv: fix address masking

[Daniel Henrique Barboza]

On 11/18/25 7:59 AM, frank.chang@sifive.com wrote:
> From: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> 
> The pmlen should get the corresponding value before shifting address.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---

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

>   target/riscv/internals.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
> index 172296f12e2..9b3f01144d2 100644
> --- a/target/riscv/internals.h
> +++ b/target/riscv/internals.h
> @@ -203,8 +203,8 @@ static inline target_ulong adjust_addr_body(CPURISCVState *env,
>       if (!is_virt_addr) {
>           signext = riscv_cpu_virt_mem_enabled(env);
>       }
> -    addr = addr << pmlen;
>       pmlen = riscv_pm_get_pmlen(pmm);
> +    addr = addr << pmlen;
>   
>       /* sign/zero extend masked address by N-1 bit */
>       if (signext) {

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

[frank.chang]

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

[Daniel Henrique Barboza]

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;

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

[Radim Krčmář]

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

[Frank Chang]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

On Tue, Nov 18, 2025 at 9:42 PM Radim Krčmář <rkrcmar@ventanamicro.com>
wrote:

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

Hi Radim,

Thanks for the review.

I've sent out the v2 patchset to address these issues:
https://lore.kernel.org/qemu-riscv/20251121050413.3718427-1-frank.chang@sifive.com/

Regards,
Frank Chang


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

[-- Attachment #2: Type: text/html, Size: 4098 bytes --]

[PATCH 3/3] target/riscv: Rename riscv_pm_get_virt_pmm() to riscv_pm_get_vm_ldst_pmm()

[frank.chang]

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

Rename riscv_pm_get_virt_pmm() to riscv_pm_get_vm_ldst_pmm() to better
reflect its actual usage. This function is used when checking the PMM
configuration for virtual-machine load/store instructions (HLV.* and HSV.*).

No functional change intended.

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

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 8899bf7667a..abc87e64648 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -846,7 +846,7 @@ bool riscv_cpu_is_32bit(RISCVCPU *cpu);
 
 bool riscv_cpu_virt_mem_enabled(CPURISCVState *env);
 RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env);
-RISCVPmPmm riscv_pm_get_virt_pmm(CPURISCVState *env);
+RISCVPmPmm riscv_pm_get_vm_ldst_pmm(CPURISCVState *env);
 uint32_t riscv_pm_get_pmlen(RISCVPmPmm pmm);
 
 RISCVException riscv_csrr(CPURISCVState *env, int csrno,
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 112093012b0..40b1e8da471 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -190,7 +190,7 @@ RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
 #endif
 }
 
-RISCVPmPmm riscv_pm_get_virt_pmm(CPURISCVState *env)
+RISCVPmPmm riscv_pm_get_vm_ldst_pmm(CPURISCVState *env)
 {
 #ifndef CONFIG_USER_ONLY
     int priv_mode = cpu_address_mode(env);
diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 9b3f01144d2..b17b661e2a8 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -190,7 +190,7 @@ static inline target_ulong adjust_addr_body(CPURISCVState *env,
 
     /* get pmm field depending on whether addr is */
     if (is_virt_addr) {
-        pmm = riscv_pm_get_virt_pmm(env);
+        pmm = riscv_pm_get_vm_ldst_pmm(env);
     } else {
         pmm = riscv_pm_get_pmm(env);
     }
-- 
2.43.0

Re: [PATCH 3/3] target/riscv: Rename riscv_pm_get_virt_pmm() to riscv_pm_get_vm_ldst_pmm()

[Daniel Henrique Barboza]

On 11/18/25 7:59 AM, frank.chang@sifive.com wrote:
> From: Frank Chang <frank.chang@sifive.com>
> 
> Rename riscv_pm_get_virt_pmm() to riscv_pm_get_vm_ldst_pmm() to better
> reflect its actual usage. This function is used when checking the PMM
> configuration for virtual-machine load/store instructions (HLV.* and HSV.*).
> 
> No functional change intended.
> 
> Signed-off-by: Frank Chang <frank.chang@sifive.com>
> ---

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

>   target/riscv/cpu.h        | 2 +-
>   target/riscv/cpu_helper.c | 2 +-
>   target/riscv/internals.h  | 2 +-
>   3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 8899bf7667a..abc87e64648 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -846,7 +846,7 @@ bool riscv_cpu_is_32bit(RISCVCPU *cpu);
>   
>   bool riscv_cpu_virt_mem_enabled(CPURISCVState *env);
>   RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env);
> -RISCVPmPmm riscv_pm_get_virt_pmm(CPURISCVState *env);
> +RISCVPmPmm riscv_pm_get_vm_ldst_pmm(CPURISCVState *env);
>   uint32_t riscv_pm_get_pmlen(RISCVPmPmm pmm);
>   
>   RISCVException riscv_csrr(CPURISCVState *env, int csrno,
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 112093012b0..40b1e8da471 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -190,7 +190,7 @@ RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
>   #endif
>   }
>   
> -RISCVPmPmm riscv_pm_get_virt_pmm(CPURISCVState *env)
> +RISCVPmPmm riscv_pm_get_vm_ldst_pmm(CPURISCVState *env)
>   {
>   #ifndef CONFIG_USER_ONLY
>       int priv_mode = cpu_address_mode(env);
> diff --git a/target/riscv/internals.h b/target/riscv/internals.h
> index 9b3f01144d2..b17b661e2a8 100644
> --- a/target/riscv/internals.h
> +++ b/target/riscv/internals.h
> @@ -190,7 +190,7 @@ static inline target_ulong adjust_addr_body(CPURISCVState *env,
>   
>       /* get pmm field depending on whether addr is */
>       if (is_virt_addr) {
> -        pmm = riscv_pm_get_virt_pmm(env);
> +        pmm = riscv_pm_get_vm_ldst_pmm(env);
>       } else {
>           pmm = riscv_pm_get_pmm(env);
>       }