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

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

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.