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

[Alistair Francis]

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.

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
> >>
> >>
>