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

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

[Weiwei Li]

This patchset tries to fix some problems in current implementation for mstatus.MPP

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

v2:
* Modify commit message and add comment to specify MPP field becomes a WARL field since priv version 1.11 in patch 2
* rebase on riscv-to-apply.next

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        | 18 ++++++++++++++++++
 target/riscv/op_helper.c  |  3 ++-
 3 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v2 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 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 ec9a384772..b8a03afebb 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -338,7 +338,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 v2 2/2] target/riscv: Legalize MPP value in write_mstatus

[Weiwei Li]

mstatus.MPP field is a WARL field since priv version 1.11, 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        | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2310c7905f..6148f221c3 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -647,12 +647,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 e0b871f6dc..f3ae726853 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1230,6 +1230,18 @@ static bool validate_vm(CPURISCVState *env, target_ulong vm)
            satp_mode_max_from_map(riscv_cpu_cfg(env)->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)
 {
@@ -1237,6 +1249,12 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
     uint64_t mask = 0;
     RISCVMXL xl = riscv_cpu_mxl(env);
 
+    /*
+     * MPP field have been made WARL since priv version 1.11. However,
+     * legalization for it will not break any software running on 1.10.
+     */
+    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 v2 2/2] target/riscv: Legalize MPP value in write_mstatus

[Richard Henderson]

On 4/6/23 00:25, Weiwei Li wrote:
>   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();
>       }

Nit: if (test) { assert_not_reached } -> assert(!test).

which emits a more helpful error message before abort.


r~

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

[Richard Henderson]

On 4/6/23 00:25, Weiwei Li wrote:
> +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;
> +}

Does anyone find PRV_H confusing, since that's not what it is?
I think it would be nice to remove it entirely.

This function might be better as

     bool valid = false;

     switch (new_mpp) {
     case PRV_M:
         valid = true;
         break;
     case PRV_S:
         valid = riscv_has_ext(env, RVS);
         break;
     case PRV_U:
         valid = riscv_has_ext(env, RVU);
         break;
     }
     if (!valid) {
         val = set_field(...);
     }
     return val;


r~

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

[liweiwei]

On 2023/4/7 03:28, Richard Henderson wrote:
> On 4/6/23 00:25, Weiwei Li wrote:
>>   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();
>>       }
>
> Nit: if (test) { assert_not_reached } -> assert(!test).
>
> which emits a more helpful error message before abort.
>
OK. I'll update this.

Regards,

Weiwei Li

>
> r~

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

[liweiwei]

On 2023/4/7 03:33, Richard Henderson wrote:
> On 4/6/23 00:25, Weiwei Li wrote:
>> +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;
>> +}
>
> Does anyone find PRV_H confusing, since that's not what it is?
> I think it would be nice to remove it entirely.
I agree. Maybe PRV_RESERVED is more readable in some cases.
>
> This function might be better as
>
>     bool valid = false;
>
>     switch (new_mpp) {
>     case PRV_M:
>         valid = true;
>         break;
>     case PRV_S:
>         valid = riscv_has_ext(env, RVS);
>         break;
>     case PRV_U:
>         valid = riscv_has_ext(env, RVU);
>         break;
>     }
>     if (!valid) {
>         val = set_field(...);
>     }
>     return val;
>
>
OK. This is more readable. I'll update this.

Regards,

Weiwei Li

> r~