[PATCH 0/2] target/riscv: Fix mstatus.MPP related support

[PATCH 0/2] target/riscv: Fix mstatus.MPP related support

[Weiwei Li]

This patchset tries to fix some problems in current implementation for mstatus.MPP.
- set to the least-privileged supported mode (U if U-mode is implemented, else M) after executing mret
- legalize mpp when write to mstatus

The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-mpp-fix

Weiwei Li (2):
  target/riscv: Fix the mstatus.MPP value after executing MRET
  target/riscv: Legalize MPP value in write_mstatus

 target/riscv/cpu_helper.c |  5 +----
 target/riscv/csr.c        | 14 ++++++++++++++
 target/riscv/op_helper.c  |  3 ++-
 3 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.25.1

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

[Weiwei Li]

The MPP will be set to the least-privileged supported mode (U if
U-mode is implemented, else M).

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

[PATCH 2/2] target/riscv: Legalize MPP value in write_mstatus

[Weiwei Li]

mstatus.MPP field is a WARL field, so we remain it unchanged if an
invalid value is written into it. And after this, RVH shouldn't be
passed to riscv_cpu_set_mode().

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu_helper.c |  5 +----
 target/riscv/csr.c        | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..46baf3ab7c 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -659,12 +659,9 @@ void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
 
 void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
 {
-    if (newpriv > PRV_M) {
+    if (newpriv > PRV_M || newpriv == PRV_H) {
         g_assert_not_reached();
     }
-    if (newpriv == PRV_H) {
-        newpriv = PRV_U;
-    }
     if (icount_enabled() && newpriv != env->priv) {
         riscv_itrigger_update_priv(env);
     }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d522efc0b6..a99026c3e8 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1238,6 +1238,18 @@ static bool validate_vm(CPURISCVState *env, target_ulong vm)
     return (vm & 0xf) <= satp_mode_max_from_map(cpu->cfg.satp_mode.map);
 }
 
+static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
+                                 target_ulong val)
+{
+    target_ulong new_mpp = get_field(val, MSTATUS_MPP);
+    bool mpp_invalid = (new_mpp == PRV_S && !riscv_has_ext(env, RVS)) ||
+                       (new_mpp == PRV_U && !riscv_has_ext(env, RVU)) ||
+                       (new_mpp == PRV_H);
+
+    /* Remain field unchanged if new_mpp value is invalid */
+    return mpp_invalid ? set_field(val, MSTATUS_MPP, old_mpp) : val;
+}
+
 static RISCVException write_mstatus(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
@@ -1245,6 +1257,8 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
     uint64_t mask = 0;
     RISCVMXL xl = riscv_cpu_mxl(env);
 
+    val = legalize_mpp(env, get_field(mstatus, MSTATUS_MPP), val);
+
     /* flush tlb on mstatus fields that affect VM */
     if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
             MSTATUS_MPRV | MSTATUS_SUM)) {
-- 
2.25.1

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

[Alistair Francis]

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:

"MRET then in mstatus/mstatush sets MPV=0, MPP=0,
MIE=MPIE, and MPIE=1"

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

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

[liweiwei]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]


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

(draft-20230131-c0b298a: Clarify WFI trapping behavior (#972)).

>
> "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 anxRET instruction, supposingxPP holds the valuey,xIE is 
set toxPIE; the
privilege mode is changed toy;xPIE is set to 1; andxPP is set to the 
least-privileged supported
mode (U if U-mode is implemented, else M). Ify̸=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
>>
>>

[-- Attachment #2: Type: text/html, Size: 9810 bytes --]

Re: [PATCH 2/2] target/riscv: Legalize MPP value in write_mstatus

[Alistair Francis]

On Thu, Mar 30, 2023 at 11:59 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> mstatus.MPP field is a WARL field, so we remain it unchanged if an

Only since version 1.11 of the priv spec and we do still support priv 1.10.

I think it's ok to make this change for all priv versions, as it won't
break any software running 1.10, but it's worth adding a comment or at
least a mention in the commit message.

Alistair

> invalid value is written into it. And after this, RVH shouldn't be
> passed to riscv_cpu_set_mode().
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>  target/riscv/cpu_helper.c |  5 +----
>  target/riscv/csr.c        | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..46baf3ab7c 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -659,12 +659,9 @@ void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
>
>  void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>  {
> -    if (newpriv > PRV_M) {
> +    if (newpriv > PRV_M || newpriv == PRV_H) {
>          g_assert_not_reached();
>      }
> -    if (newpriv == PRV_H) {
> -        newpriv = PRV_U;
> -    }
>      if (icount_enabled() && newpriv != env->priv) {
>          riscv_itrigger_update_priv(env);
>      }
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d522efc0b6..a99026c3e8 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1238,6 +1238,18 @@ static bool validate_vm(CPURISCVState *env, target_ulong vm)
>      return (vm & 0xf) <= satp_mode_max_from_map(cpu->cfg.satp_mode.map);
>  }
>
> +static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
> +                                 target_ulong val)
> +{
> +    target_ulong new_mpp = get_field(val, MSTATUS_MPP);
> +    bool mpp_invalid = (new_mpp == PRV_S && !riscv_has_ext(env, RVS)) ||
> +                       (new_mpp == PRV_U && !riscv_has_ext(env, RVU)) ||
> +                       (new_mpp == PRV_H);
> +
> +    /* Remain field unchanged if new_mpp value is invalid */
> +    return mpp_invalid ? set_field(val, MSTATUS_MPP, old_mpp) : val;
> +}
> +
>  static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>                                      target_ulong val)
>  {
> @@ -1245,6 +1257,8 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>      uint64_t mask = 0;
>      RISCVMXL xl = riscv_cpu_mxl(env);
>
> +    val = legalize_mpp(env, get_field(mstatus, MSTATUS_MPP), val);
> +
>      /* flush tlb on mstatus fields that affect VM */
>      if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
>              MSTATUS_MPRV | MSTATUS_SUM)) {
> --
> 2.25.1
>
>

Re: [PATCH 2/2] target/riscv: Legalize MPP value in write_mstatus

[liweiwei]

On 2023/4/6 09:26, Alistair Francis wrote:
> On Thu, Mar 30, 2023 at 11:59 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>> mstatus.MPP field is a WARL field, so we remain it unchanged if an
> Only since version 1.11 of the priv spec and we do still support priv 1.10.
>
> I think it's ok to make this change for all priv versions, as it won't
> break any software running 1.10, but it's worth adding a comment or at
> least a mention in the commit message.

OK. I'll add it in next version.

Regards,

Weiwei Li

>
> Alistair
>
>> invalid value is written into it. And after this, RVH shouldn't be
>> passed to riscv_cpu_set_mode().
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/cpu_helper.c |  5 +----
>>   target/riscv/csr.c        | 14 ++++++++++++++
>>   2 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index f88c503cf4..46baf3ab7c 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -659,12 +659,9 @@ void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, uint32_t priv,
>>
>>   void riscv_cpu_set_mode(CPURISCVState *env, target_ulong newpriv)
>>   {
>> -    if (newpriv > PRV_M) {
>> +    if (newpriv > PRV_M || newpriv == PRV_H) {
>>           g_assert_not_reached();
>>       }
>> -    if (newpriv == PRV_H) {
>> -        newpriv = PRV_U;
>> -    }
>>       if (icount_enabled() && newpriv != env->priv) {
>>           riscv_itrigger_update_priv(env);
>>       }
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index d522efc0b6..a99026c3e8 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1238,6 +1238,18 @@ static bool validate_vm(CPURISCVState *env, target_ulong vm)
>>       return (vm & 0xf) <= satp_mode_max_from_map(cpu->cfg.satp_mode.map);
>>   }
>>
>> +static target_ulong legalize_mpp(CPURISCVState *env, target_ulong old_mpp,
>> +                                 target_ulong val)
>> +{
>> +    target_ulong new_mpp = get_field(val, MSTATUS_MPP);
>> +    bool mpp_invalid = (new_mpp == PRV_S && !riscv_has_ext(env, RVS)) ||
>> +                       (new_mpp == PRV_U && !riscv_has_ext(env, RVU)) ||
>> +                       (new_mpp == PRV_H);
>> +
>> +    /* Remain field unchanged if new_mpp value is invalid */
>> +    return mpp_invalid ? set_field(val, MSTATUS_MPP, old_mpp) : val;
>> +}
>> +
>>   static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>>                                       target_ulong val)
>>   {
>> @@ -1245,6 +1257,8 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>>       uint64_t mask = 0;
>>       RISCVMXL xl = riscv_cpu_mxl(env);
>>
>> +    val = legalize_mpp(env, get_field(mstatus, MSTATUS_MPP), val);
>> +
>>       /* flush tlb on mstatus fields that affect VM */
>>       if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
>>               MSTATUS_MPRV | MSTATUS_SUM)) {
>> --
>> 2.25.1
>>
>>

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

[Alistair Francis]

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

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

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

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

[liweiwei]

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.

And MPP is WARL field.  PRV_U will be an illegal value for MPP if U-mode 
is not implemented.

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

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

[Alistair Francis]

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.

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

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

[liweiwei]

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.
OK. I'll send an issue for it.
>
>> 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

OK . Thanks! I'll try to set it later.

Regards,

Weiwei Li

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

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

[liweiwei]

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.

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

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