Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M

[LIU Zhiwei]

On 2023/6/12 11:10, Weiwei Li wrote:
> On 2023/6/12 10:45, LIU Zhiwei wrote:
> > On 2023/5/29 20:17, Weiwei Li wrote:
> > > Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
> > > when MPP=PRV_M.
> > Does MPP==PRV_M always indicate the MPV==0?
>
> No, I think . The spec doesn't restrict this. When MPP=PRV_M, MPV wll 
> be 0 in normal case.
>
> But users can set MPV=1 by write to mstatus CSR directly.

Make sense. The fields specification(WARL and others) of mstatus and 
other CSR  always not clear on the specification.  Maybe I missed 
something. But I think this field could be written by the mode software.

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

> As described in spec, "When an MRET instruction is executed, the 
> virtualization mode V is set to MPV, unless
>
> MPP=3, in which case V remains 0."
>
> MPV is just ignored if MPP = 3. This also can be seen in "table 9.5 
> Effect of MPRV on the translation and protection of explicit memory 
> accesses".
>
> Regards,
>
> Weiwei Li
>
> > Zhiwei
> >
> > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > > ---
> > >   target/riscv/cpu_helper.c | 3 ++-
> > >   target/riscv/op_helper.c  | 3 ++-
> > >   2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 09ea227ceb..bd892c05d4 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool 
> > > ifetch)
> > >             if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> > >               mode = get_field(env->mstatus, MSTATUS_MPP);
> > > -            virt = get_field(env->mstatus, MSTATUS_MPV);
> > > +            virt = get_field(env->mstatus, MSTATUS_MPV) &&
> > > +                   (mode != PRV_M);
> > >               if (virt) {
> > >                   status = env->vsstatus;
> > >               }
> > > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > > index f563dc3981..9cdb9cdd06 100644
> > > --- a/target/riscv/op_helper.c
> > > +++ b/target/riscv/op_helper.c
> > > @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
> > >           riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, 
> > > GETPC());
> > >       }
> > >   -    target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
> > > +    target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
> > > +                             (prev_priv != PRV_M);
> > >       mstatus = set_field(mstatus, MSTATUS_MIE,
> > >                           get_field(mstatus, MSTATUS_MPIE));
> > >       mstatus = set_field(mstatus, MSTATUS_MPIE, 1);