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

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

2025-11-21T13:04:10+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 selection logic for each mode is summarized below:
>
>   * mstatus.MXR = 1: pointer masking disabled
>
>   * 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>
> ---
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>  RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
>  {
>  #ifndef CONFIG_USER_ONLY
> -    int priv_mode = cpu_address_mode(env);
> +    int priv_mode;
> +    bool virt;
>  
> -    if (get_field(env->mstatus, MSTATUS_MPRV) &&
> -        get_field(env->mstatus, MSTATUS_MXR)) {
> +    if (get_field(env->mstatus, MSTATUS_MXR)) {
>          return PMM_FIELD_DISABLED;
>      }

MSTATUS_MXR doesn't disable Pointer masking in M-mode, and we also need
to consider virt, where vsstatus.MXR is in effect as well.

>  
> +    riscv_cpu_eff_priv(env, &priv_mode, &virt);

I think you could put something like this here:

      if ((mode != PRV_M && get_field(env->mstatus, MSTATUS_MXR)) ||
          (virt && get_field(env->vsstatus, MSTATUS_MXR))) {
          return PMM_FIELD_DISABLED;
      }

>      /* Get current PMM field */
>      switch (priv_mode) {
>      case PRV_M:
> @@ -189,22 +222,30 @@ RISCVPmPmm riscv_pm_get_pmm(CPURISCVState *env)
>      case PRV_U:
> -        if (riscv_has_ext(env, RVS)) {
> +        if (!virt) {
>              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);
>              }
>          }

virt doesn't really matter, the original code was correct.

Thanks.