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

["Radim Krčmář" <rkrcmar@ventanamicro.com>]

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.