Re: [PATCH 01/11] riscv: Add privilege level to DisasContext

[LIU Zhiwei]

On 2022/9/16 14:00, Richard Henderson wrote:
> On 9/6/22 14:22, Christoph Muellner wrote:
> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >
> > This allows privileged instructions to check the required
> > privilege level in the translation without calling a helper.
> >
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >   target/riscv/translate.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index 63b04e8a94..fd241ff667 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -59,6 +59,9 @@ typedef struct DisasContext {
> >       /* pc_succ_insn points to the instruction following 
> > base.pc_next */
> >       target_ulong pc_succ_insn;
> >       target_ulong priv_ver;
> > +#ifndef CONFIG_USER_ONLY
> > +    target_ulong priv;
> > +#endif
> >       RISCVMXL misa_mxl_max;
> >       RISCVMXL xl;
> >       uint32_t misa_ext;
> > @@ -1079,6 +1082,7 @@ static void 
> > riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >       ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS;
> >       ctx->priv_ver = env->priv_ver;
> >   #if !defined(CONFIG_USER_ONLY)
> > +    ctx->priv = env->priv;
>
> Reading directly from env here is, in general, wrong.  Items that are 
> constant, like priv_ver, are ok.  But otherwise these values must be 
> recorded by cpu_get_tb_cpu_state().
>
> This instance will happen to work, because the execution context is 
> already constrained. 

Exactly. Thanks for pointing it out.

> In this case because env->priv == ctx->mem_idx (see cpu_mmu_index) so, 
> really, you don't need this new field at all.  Or, keep the field, 
> because it's usage will be more self-documentary, but copy the value 
> from ctx->mmu_idx and add a comment.
>
>
> >       if (riscv_has_ext(env, RVH)) {
> >           ctx->virt_enabled = riscv_cpu_virt_enabled(env);
> >       } else {
>
> Incidentally, this (existing) usage appears to be a bug.  I can see 
> nothing in cpu_get_tb_cpu_state that corresponds directly to this value.

Agree.

Zhiwei

> r~