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

[PATCH v3 0/3] 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-v3

v3:
* add patch 2 to remove PRV_H, and use PRV_RESERVED instead in some cases
* improve legalize_mpp and assert error message in patch 3

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 (3):
  target/riscv: Fix the mstatus.MPP value after executing MRET
  target/riscv: Use PRV_RESERVED instead of PRV_H
  target/riscv: Legalize MPP value in write_mstatus

 target/riscv/cpu.h        |  2 +-
 target/riscv/cpu_bits.h   |  2 +-
 target/riscv/cpu_helper.c |  8 ++------
 target/riscv/csr.c        | 32 ++++++++++++++++++++++++++++++++
 target/riscv/gdbstub.c    |  2 +-
 target/riscv/op_helper.c  |  5 +++--
 6 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] 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 v3 2/3] target/riscv: Use PRV_RESERVED instead of PRV_H

[Weiwei Li]

PRV_H has no real meaning, but just a reserved privilege mode currently.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu.h       | 2 +-
 target/riscv/cpu_bits.h  | 2 +-
 target/riscv/gdbstub.c   | 2 +-
 target/riscv/op_helper.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index cbf3de2708..4af8ebc558 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -699,7 +699,7 @@ static inline RISCVMXL cpu_recompute_xl(CPURISCVState *env)
         case PRV_U:
             xl = get_field(env->mstatus, MSTATUS64_UXL);
             break;
-        default: /* PRV_S | PRV_H */
+        default: /* PRV_S */
             xl = get_field(env->mstatus, MSTATUS64_SXL);
             break;
         }
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 101702cb4a..a16bfaf43f 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -608,7 +608,7 @@ typedef enum {
 /* Privilege modes */
 #define PRV_U 0
 #define PRV_S 1
-#define PRV_H 2 /* Reserved */
+#define PRV_RESERVED 2
 #define PRV_M 3
 
 /* RV32 satp CSR field masks */
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index fa537aed74..524bede865 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -203,7 +203,7 @@ static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
     if (n == 0) {
 #ifndef CONFIG_USER_ONLY
         cs->priv = ldtul_p(mem_buf) & 0x3;
-        if (cs->priv == PRV_H) {
+        if (cs->priv == PRV_RESERVED) {
             cs->priv = PRV_S;
         }
 #endif
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index b8a03afebb..bd21c6eeef 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -141,7 +141,7 @@ static void check_zicbo_envcfg(CPURISCVState *env, target_ulong envbits,
     }
 
     if (env->virt_enabled &&
-        (((env->priv < PRV_H) && !get_field(env->henvcfg, envbits)) ||
+        (((env->priv <= PRV_S) && !get_field(env->henvcfg, envbits)) ||
          ((env->priv < PRV_S) && !get_field(env->senvcfg, envbits)))) {
         riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, ra);
     }
-- 
2.25.1

[PATCH v3 3/3] 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 |  8 ++------
 target/riscv/csr.c        | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2310c7905f..433ea529b0 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -647,12 +647,8 @@ 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) {
-        g_assert_not_reached();
-    }
-    if (newpriv == PRV_H) {
-        newpriv = PRV_U;
-    }
+    g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);
+
     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..f4d2dcfdc8 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1230,6 +1230,32 @@ 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)
+{
+    bool valid = false;
+    target_ulong new_mpp = get_field(val, MSTATUS_MPP);
+
+    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;
+    }
+
+    /* Remain field unchanged if new_mpp value is invalid */
+    if (!valid) {
+        val = set_field(val, MSTATUS_MPP, old_mpp);
+    }
+
+    return val;
+}
+
 static RISCVException write_mstatus(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
@@ -1237,6 +1263,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 v3 2/3] target/riscv: Use PRV_RESERVED instead of PRV_H

[Richard Henderson]

On 4/6/23 18:47, Weiwei Li wrote:
> PRV_H has no real meaning, but just a reserved privilege mode currently.
> 
> Signed-off-by: Weiwei Li<liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang<wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/cpu.h       | 2 +-
>   target/riscv/cpu_bits.h  | 2 +-
>   target/riscv/gdbstub.c   | 2 +-
>   target/riscv/op_helper.c | 2 +-
>   4 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!


r~

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

[Richard Henderson]

On 4/6/23 18:47, Weiwei Li wrote:
> 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 |  8 ++------
>   target/riscv/csr.c        | 32 ++++++++++++++++++++++++++++++++
>   2 files changed, 34 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 2/3] target/riscv: Use PRV_RESERVED instead of PRV_H

[Alistair Francis]

On Fri, Apr 7, 2023 at 11:49 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> PRV_H has no real meaning, but just a reserved privilege mode currently.
>
> 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>

Alistair

> ---
>  target/riscv/cpu.h       | 2 +-
>  target/riscv/cpu_bits.h  | 2 +-
>  target/riscv/gdbstub.c   | 2 +-
>  target/riscv/op_helper.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index cbf3de2708..4af8ebc558 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -699,7 +699,7 @@ static inline RISCVMXL cpu_recompute_xl(CPURISCVState *env)
>          case PRV_U:
>              xl = get_field(env->mstatus, MSTATUS64_UXL);
>              break;
> -        default: /* PRV_S | PRV_H */
> +        default: /* PRV_S */
>              xl = get_field(env->mstatus, MSTATUS64_SXL);
>              break;
>          }
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 101702cb4a..a16bfaf43f 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -608,7 +608,7 @@ typedef enum {
>  /* Privilege modes */
>  #define PRV_U 0
>  #define PRV_S 1
> -#define PRV_H 2 /* Reserved */
> +#define PRV_RESERVED 2
>  #define PRV_M 3
>
>  /* RV32 satp CSR field masks */
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index fa537aed74..524bede865 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -203,7 +203,7 @@ static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
>      if (n == 0) {
>  #ifndef CONFIG_USER_ONLY
>          cs->priv = ldtul_p(mem_buf) & 0x3;
> -        if (cs->priv == PRV_H) {
> +        if (cs->priv == PRV_RESERVED) {
>              cs->priv = PRV_S;
>          }
>  #endif
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index b8a03afebb..bd21c6eeef 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -141,7 +141,7 @@ static void check_zicbo_envcfg(CPURISCVState *env, target_ulong envbits,
>      }
>
>      if (env->virt_enabled &&
> -        (((env->priv < PRV_H) && !get_field(env->henvcfg, envbits)) ||
> +        (((env->priv <= PRV_S) && !get_field(env->henvcfg, envbits)) ||
>           ((env->priv < PRV_S) && !get_field(env->senvcfg, envbits)))) {
>          riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, ra);
>      }
> --
> 2.25.1
>
>

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

[Alistair Francis]

On Fri, Apr 7, 2023 at 11:49 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> 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>

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

Alistair

> ---
>  target/riscv/cpu_helper.c |  8 ++------
>  target/riscv/csr.c        | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 2310c7905f..433ea529b0 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -647,12 +647,8 @@ 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) {
> -        g_assert_not_reached();
> -    }
> -    if (newpriv == PRV_H) {
> -        newpriv = PRV_U;
> -    }
> +    g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);
> +
>      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..f4d2dcfdc8 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1230,6 +1230,32 @@ 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)
> +{
> +    bool valid = false;
> +    target_ulong new_mpp = get_field(val, MSTATUS_MPP);
> +
> +    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;
> +    }
> +
> +    /* Remain field unchanged if new_mpp value is invalid */
> +    if (!valid) {
> +        val = set_field(val, MSTATUS_MPP, old_mpp);
> +    }
> +
> +    return val;
> +}
> +
>  static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>                                      target_ulong val)
>  {
> @@ -1237,6 +1263,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 v3 0/3] target/riscv: Fix mstatus.MPP related support

[Alistair Francis]

On Fri, Apr 7, 2023 at 11:49 AM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> 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-v3
>
> v3:
> * add patch 2 to remove PRV_H, and use PRV_RESERVED instead in some cases
> * improve legalize_mpp and assert error message in patch 3
>
> 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 (3):
>   target/riscv: Fix the mstatus.MPP value after executing MRET
>   target/riscv: Use PRV_RESERVED instead of PRV_H
>   target/riscv: Legalize MPP value in write_mstatus

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu.h        |  2 +-
>  target/riscv/cpu_bits.h   |  2 +-
>  target/riscv/cpu_helper.c |  8 ++------
>  target/riscv/csr.c        | 32 ++++++++++++++++++++++++++++++++
>  target/riscv/gdbstub.c    |  2 +-
>  target/riscv/op_helper.c  |  5 +++--
>  6 files changed, 40 insertions(+), 11 deletions(-)
>
> --
> 2.25.1
>
>