Re: [PATCH 1/2] target/riscv: Fix the mstatus.MPP value after executing MRET

[Alistair Francis]

On Thu, Apr 6, 2023 at 1:02 PM liweiwei <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/4/6 10:24, Alistair Francis wrote:
> > On Thu, Apr 6, 2023 at 12:14 PM liweiwei <liweiwei@iscas.ac.cn> wrote:
> >>
> >> On 2023/4/6 09:46, Alistair Francis wrote:
> >>> On Thu, Apr 6, 2023 at 10:56 AM liweiwei <liweiwei@iscas.ac.cn> wrote:
> >>>> On 2023/4/6 08:43, Alistair Francis wrote:
> >>>>
> >>>> On Thu, Mar 30, 2023 at 11:59 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >>>>
> >>>> The MPP will be set to the least-privileged supported mode (U if
> >>>> U-mode is implemented, else M).
> >>>>
> >>>> I don't think this is right, the spec in section 8.6.4 says this:
> >>>>
> >>>> Sorry, I didn't find this section in latest release of both privilege 
> >>>> and un-privilege spec
> >>> I updated my spec, using commit
> >>> f6b8d5c7d2dcd935b48689a337c8f5bc2be4b5e5 it's now section 9.6.4 Trap
> >>> Return
> >> Yeah. I see it. However, this is a little different from the description
> >> in section 3.1.6.1.
> > They seem to be in conflict. It's probably worth opening an issue
> > against the spec to get some clarification here.
>
> I have sent an issue for
> it(https://github.com/riscv/riscv-isa-manual/issues/1006).
>
> However, I just find it may be not a conflict. Section 9.6.4 is the spec
> for hypervisor. And when hypervisor is supported,
>
> S-mode, then U-mode should be supported too.

Ah, I didn't think to check the actual section!

Good call. I think you are right then. In which case this patch is the
correct way to go :)

Feel free to close the issue if you want to.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> Regards,
>
> Weiwei Li
>
> >
> >> And MPP is WARL field.  PRV_U will be an illegal value for MPP if U-mode
> >> is not implemented.
> > Yeah, I think you are right. It just directly goes against the mret
> > section. I suspect the mret section is wrong and needs to be updated
> >
> >> So I think description in section 3.1.6.1 seems more reasonable.
> >>
> >>>> (draft-20230131-c0b298a: Clarify WFI trapping behavior (#972)).
> >>> Also, you replied with a HTML email which loses the conversation
> >>> history (just see above). Can you fixup your client to reply with
> >>> plain text please
> >> Sorry. I don't get your problem. I replied by Thunderbird. Above is the
> > Have a look at your previous email, it's a HTML email. If I view the
> > source of the email I see this:
> >
> >      Content-Type: text/html; charset=UTF-8
> >
> > and the formatting is a little off.
> >
> > This email that I'm replying to is a plain text email. I'm not sure
> > what happened, but try to check that your responses are plain text. I
> > think there is a setting in Thunderbird to just open and reply to all
> > emails as plain text, which is probably worth turning on
> >
> > Alistair
> >
> >> title for the latest release version of the spec in riscv-isa-manual
> >> github
> >> (https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20230131-c0b298a).
> >>
> >> Regards,
> >>
> >> Weiwei Li
> >>
> >>> Alistair
> >>>
> >>>> "MRET then in mstatus/mstatush sets MPV=0, MPP=0,
> >>>> MIE=MPIE, and MPIE=1"
> >>>>
> >>>> In section 3.1.6.1, the privilege spec says this:
> >>>>
> >>>> "An MRET or SRET instruction is used to return from a trap in M-mode or 
> >>>> S-mode respectively.
> >>>> When executing an xRET instruction, supposing xPP holds the value y, xIE 
> >>>> is set to xPIE; the
> >>>> privilege mode is changed to y; xPIE is set to 1; and xPP is set to the 
> >>>> least-privileged supported
> >>>> mode (U if U-mode is implemented, else M). If y̸=M, xRET also sets 
> >>>> MPRV=0"
> >>>>
> >>>> And I think PRV_U is an illegal value for MPP if U-mode is not 
> >>>> implemented.
> >>>>
> >>>> Regards,
> >>>>
> >>>> Weiwei Li
> >>>>
> >>>> So it should just always be 0 (PRV_U is 0)
> >>>>
> >>>> Alistair
> >>>>
> >>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> >>>> ---
> >>>>    target/riscv/op_helper.c | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> >>>> index 84ee018f7d..991f06d98d 100644
> >>>> --- a/target/riscv/op_helper.c
> >>>> +++ b/target/riscv/op_helper.c
> >>>> @@ -339,7 +339,8 @@ target_ulong helper_mret(CPURISCVState *env)
> >>>>        mstatus = set_field(mstatus, MSTATUS_MIE,
> >>>>                            get_field(mstatus, MSTATUS_MPIE));
> >>>>        mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
> >>>> -    mstatus = set_field(mstatus, MSTATUS_MPP, PRV_U);
> >>>> +    mstatus = set_field(mstatus, MSTATUS_MPP,
> >>>> +                        riscv_has_ext(env, RVU) ? PRV_U : PRV_M);
> >>>>        mstatus = set_field(mstatus, MSTATUS_MPV, 0);
> >>>>        if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != 
> >>>> PRV_M)) {
> >>>>            mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>>
>