Hi Radim, On Tue, Nov 25, 2025 at 10:59 PM Radim Krčmář <[email protected]> wrote:
> 2025-11-21T13:04:09+08:00, <[email protected]>: > > From: Frank Chang <[email protected]> > > > > This helper returns the current effective privilege mode. > > > > Signed-off-by: Frank Chang <[email protected]> > > --- > > 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 > Are you saying that we "should" define it in a header file? > 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ář <[email protected]> > > Thanks. >
