Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV

[Weiwei Li]

On 2023/6/2 07:03, Alistair Francis wrote:
> On Thu, Jun 1, 2023 at 4:43 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> > On 2023/6/1 13:27, Alistair Francis wrote:
> > > On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> > > > Normally, MPRV can be set to 1 only in M mode (It will be cleared
> > > > when returning to lower-privilege mode by MRET/SRET).
> > > >
> > > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > > > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > > > ---
> > > >    target/riscv/cpu_helper.c | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > index bd892c05d4..45baf95c77 100644
> > > > --- a/target/riscv/cpu_helper.c
> > > > +++ b/target/riscv/cpu_helper.c
> > > > @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> > > >        if (!ifetch) {
> > > >            uint64_t status = env->mstatus;
> > > >
> > > > -        if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> > > > +        if (get_field(status, MSTATUS_MPRV)) {
> > > The original check is correct though, why remove it?
> > Yeah. As described in the commit message, I think MPRV can only be set
> > to 1 in M mode normally
> That's true. I do feel that keeping the check makes the code easier to
> follow. Otherwise future developers need to check to see how MPRV can
> be set. The current code is explicit and obviously follows the spec.
>
> For a performance gain I think it's worth making the trade off, but it
> doesn't sound like we really get any gain here.

Yeah. It's acceptable to me.

Just another question: whether MPRV is truly limited to work on M mode?

I can only find following description in the note:

"The MPRV and MXR mechanisms *were* conceived to improve the efficiency 
of M-mode routines
that emulate missing hardware features, e.g., misaligned loads and stores."

To some degree, It seems not limit them to work on other mode.

Even though MPRV normally can be set to 1 in M mode, it seems possible 
to set it to 1 in other mode by gdbstub.

Regards,

Weiwei Li

> Alistair
>
> > which means check on MPRV is enough in this case. So I remove the check
> > on mode here.
> >
> > Regards,
> >
> > Weiwei Li
> >
> > > Alistair
> > >
> > > >                mode = get_field(env->mstatus, MSTATUS_MPP);
> > > >                virt = get_field(env->mstatus, MSTATUS_MPV) &&
> > > >                       (mode != PRV_M);
> > > > --
> > > > 2.25.1