[PATCH v4 0/9] target/riscv: Add support for Smdbltrp and Ssdbltrp extensions

[PATCH v4 0/9] target/riscv: Add support for Smdbltrp and Ssdbltrp extensions

[Clément Léger]

A double trap typically arises during a sensitive phase in trap handling
operations — when an exception or interrupt occurs while the trap
handler (the component responsible for managing these events) is in a
non-reentrant state. This non-reentrancy usually occurs in the early
phase of trap handling, wherein the trap handler has not yet preserved
the necessary state to handle and resume from the trap. The occurrence
of such event is unlikely but can happen when dealing with hardware
errors.

This series adds support for Ssdbltrp and Smdbltrp ratified ISA
extensions [1]. It is based on the Smrnmi series [6].

Ssdbltrp can be tested using qemu[2], opensbi[3], linux[4] and
kvm-unit-tests[5]. Assuming you have a riscv environment available and
configured (CROSS_COMPILE), it can be built for riscv64 using the
following instructions:

Qemu:
  $ git clone https://github.com/rivosinc/qemu.git
  $ cd qemu
  $ git switch -C dbltrp_v4 dev/cleger/dbltrp_v4
  $ mkdir build && cd build
  $ ../configure --target-list=riscv64-softmmu
  $ make

OpenSBI:
  $ git clone https://github.com/rivosinc/opensbi.git
  $ cd opensbi
  $ git switch -C dbltrp_v3 dev/cleger/dbltrp_v3
  $ make O=build PLATFORM_RISCV_XLEN=64 PLATFORM=generic

Linux:
  $ git clone https://github.com/rivosinc/linux.git
  $ cd linux
  $ git switch -C dbltrp_v1 dev/cleger/dbltrp_v1
  $ export ARCH=riscv
  $ make O=build defconfig
  $ ./script/config --file build/.config --enable RISCV_DBLTRP
  $ make O=build

kvm-unit-tests:
  $ git clone https://github.com/clementleger/kvm-unit-tests.git
  $ cd kvm-unit-tests
  $ git switch -C dbltrp_v1 dev/cleger/dbltrp_v1
  $ ./configure --arch=riscv64 --cross-prefix=$CROSS_COMPILE
  $ make

You will also need kvmtool in your rootfs.

Run with kvm-unit-test test as kernel:
  $ qemu-system-riscv64 \
    -M virt \
    -cpu rv64,ssdbltrp=true,smdbltrp=true \
    -nographic \
    -serial mon:stdio \
    -bios opensbi/build/platform/generic/firmware/fw_jump.bin \
    -kernel kvm-unit-tests-dbltrp/riscv/sbi_dbltrp.flat
  ...
  [OpenSBI boot partially elided]
  Boot HART ISA Extensions  : sscofpmf,sstc,zicntr,zihpm,zicboz,zicbom,sdtrig,svadu,ssdbltrp
  ...
  ##########################################################################
  #    kvm-unit-tests
  ##########################################################################

  PASS: sbi: fwft: FWFT extension probing no error
  PASS: sbi: fwft: FWFT extension is present
  PASS: sbi: fwft: dbltrp: Get double trap enable feature value
  PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 0
  PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 0
  PASS: sbi: fwft: dbltrp: Double trap disabled, trap first time ok
  PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 1
  PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 1
  PASS: sbi: fwft: dbltrp: Trapped twice allowed ok
  INFO: sbi: fwft: dbltrp: Should generate a double trap and crash !

  sbi_trap_error: hart0: trap0: double trap handler failed (error -10)

  sbi_trap_error: hart0: trap0: mcause=0x0000000000000010 mtval=0x0000000000000000
  sbi_trap_error: hart0: trap0: mtval2=0x0000000000000003 mtinst=0x0000000000000000
  sbi_trap_error: hart0: trap0: mepc=0x00000000802000d8 mstatus=0x8000000a01006900
  sbi_trap_error: hart0: trap0: ra=0x00000000802001fc sp=0x0000000080213e70
  sbi_trap_error: hart0: trap0: gp=0x0000000000000000 tp=0x0000000080088000
  sbi_trap_error: hart0: trap0: s0=0x0000000080213e80 s1=0x0000000000000001
  sbi_trap_error: hart0: trap0: a0=0x0000000080213e80 a1=0x0000000080208193
  sbi_trap_error: hart0: trap0: a2=0x000000008020dc20 a3=0x000000000000000f
  sbi_trap_error: hart0: trap0: a4=0x0000000080210cd8 a5=0x00000000802110d0
  sbi_trap_error: hart0: trap0: a6=0x00000000802136e4 a7=0x0000000046574654
  sbi_trap_error: hart0: trap0: s2=0x0000000080210cd9 s3=0x0000000000000000
  sbi_trap_error: hart0: trap0: s4=0x0000000000000000 s5=0x0000000000000000
  sbi_trap_error: hart0: trap0: s6=0x0000000000000000 s7=0x0000000000000001
  sbi_trap_error: hart0: trap0: s8=0x0000000000002000 s9=0x0000000080083700
  sbi_trap_error: hart0: trap0: s10=0x0000000000000000 s11=0x0000000000000000
  sbi_trap_error: hart0: trap0: t0=0x0000000000000000 t1=0x0000000080213ed8
  sbi_trap_error: hart0: trap0: t2=0x0000000000001000 t3=0x0000000080213ee0
  sbi_trap_error: hart0: trap0: t4=0x0000000000000000 t5=0x000000008020f8d0
  sbi_trap_error: hart0: trap0: t6=0x0000000000000000

Run with linux and kvm-unit-test test in kvm (testing VS-mode):
  $ qemu-system-riscv64 \
    -M virt \
    -cpu rv64,ssdbltrp=true,smdbltrp=true \
    -nographic \
    -serial mon:stdio \
    -bios opensbi/build/platform/generic/firmware/fw_jump.bin \
    -kernel linux/build/arch/riscv/boot/Image
  ...
  [Linux boot partially elided]
  [    0.735079] riscv-dbltrp: Double trap handling registered
  ...

  $ lkvm run -k sbi_dbltrp.flat -m 128 -c 2
  ##########################################################################
  #    kvm-unit-tests
  ##########################################################################

  PASS: sbi: fwft: FWFT extension probing no error
  PASS: sbi: fwft: FWFT extension is present
  PASS: sbi: fwft: dbltrp: Get double trap enable feature value
  PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 0
  PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 0
  PASS: sbi: fwft: dbltrp: Double trap disabled, trap first time ok
  PASS: sbi: fwft: dbltrp: Set double trap enable feature value == 1
  PASS: sbi: fwft: dbltrp: Get double trap enable feature value == 1
  PASS: sbi: fwft: dbltrp: Trapped twice allowed ok
  INFO: sbi: fwft: dbltrp: Should generate a double trap and crash !
  [   51.939077] Guest double trap
  [   51.939323] kvm [93]: VCPU exit error -95
  [   51.939683] kvm [93]: SEPC=0x802000d8 SSTATUS=0x200004520 HSTATUS=0x200200180
  [   51.939947] kvm [93]: SCAUSE=0x10 STVAL=0x0 HTVAL=0x3 HTINST=0x0
  KVM_RUN failed: Operation not supported
  $

Testing Smbdbltrp can be done using gdb and trigger some trap. For
instance, interrupt M-mode firmware at some point, set mstatus.mdt = 1
and corrupt some register to generate a NULL pointer exception.

Link: https://github.com/riscv/riscv-isa-manual/commit/52a5742d5ab5a0792019033631b2035a493ad981 [1]
Link: https://github.com/rivosinc/qemu/tree/dev/cleger/dbltrp_v4 [2]
Link: https://github.com/rivosinc/opensbi/tree/dev/cleger/dbltrp_v3 [3]
Link: https://github.com/rivosinc/linux/tree/dev/cleger/dbltrp_v1 [4]
Link: https://github.com/clementleger/kvm-unit-tests/tree/dev/cleger/dbltrp_v1 [5]
Link: https://lore.kernel.org/all/20240902071358.1061693-1-tommy.wu@sifive.com/ [6]

---

V4:
 - Remove DTE from sstatus_v1_10_mask variable and add specific if for
   DTE masking where it's used.
 - Use mstatus_hs.sdt field rather than setting DTE to 0 in
   riscv_do_cpu_interrupt().
 - Add a fix for henvcfg value which was incorrectly set after changing
   menvcfg
 - Remove useless ext_ssdbltrp check in
   riscv_env_smode_dbltrp_enabled().
 - Remove useless mstatus clear in write_mstatus().
 - Add proper handling of SDT writing to vsstatus.
 - Add clearing of vsstatus//mstatus SDT field when DTE is disabled.
 - Fix wrong value being written for MDT/MIE in write_mstatush().
 - Rebased on Frank Snrnmi v7

V3:
 - Fix spec version from 1.12 to 1.13 for Smdbltrp and Ssdbltrp
 - Add better comments for dte/sdt computation in
   riscv_cpu_do_interrupt().
 - Move some CSR related changes to the CSRs related commits.

V2:
 - Squashed commits that added ext_s{s|m}dbltrp as suggested by Daniel

Clément Léger (9):
  target/riscv: fix henvcfg potentially containing stale bits
  target/riscv: Add Ssdbltrp CSRs handling
  target/riscv: Implement Ssdbltrp sret, mret and mnret behavior
  target/riscv: Implement Ssdbltrp exception handling
  target/riscv: Add Ssdbltrp ISA extension enable switch
  target/riscv: Add Smdbltrp CSRs handling
  target/riscv: Implement Smdbltrp sret, mret and mnret behavior
  target/riscv: Implement Smdbltrp behavior
  target/riscv: Add Smdbltrp ISA extension enable switch

 target/riscv/cpu.c        |   9 +++-
 target/riscv/cpu.h        |   1 +
 target/riscv/cpu_bits.h   |   8 +++
 target/riscv/cpu_cfg.h    |   2 +
 target/riscv/cpu_helper.c | 111 ++++++++++++++++++++++++++++++--------
 target/riscv/csr.c        |  98 ++++++++++++++++++++++++++++-----
 target/riscv/op_helper.c  |  47 +++++++++++++++-
 7 files changed, 239 insertions(+), 37 deletions(-)

-- 
2.45.2

[PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Clément Léger]

With the current implementation, if we had the current scenario:
- set bit x in menvcfg
- set bit x in henvcfg
- clear bit x in menvcfg
then, the internal variable env->henvcfg would still contain bit x due
to both a wrong menvcfg mask used in write_henvcfg() as well as a
missing update of henvcfg upon menvcfg update.
This can lead to some wrong interpretation of the context. In order to
update henvcfg upon menvcfg writing, call write_henvcfg() after writing
menvcfg and fix the mask computation used in write_henvcfg() that is
used to mesk env->menvcfg value (which could still lead to some stale
bits). The same mechanism is also applied for henvcfgh writing.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/csr.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index b84b436151..9e832e0b39 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
+                                    target_ulong val);
 static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
@@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
                 (cfg->ext_svadu ? MENVCFG_ADUE : 0);
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
+    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
 
     return RISCV_EXCP_NONE;
 }
@@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
+                                    target_ulong val);
 static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
@@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
     uint64_t valh = (uint64_t)val << 32;
 
     env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
+    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
 
     return RISCV_EXCP_NONE;
 }
@@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
+    uint64_t menvcfg_mask = 0;
     RISCVException ret;
 
     ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
@@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
     }
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
-        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
+        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
+        mask |= env->menvcfg & menvcfg_mask;
     }
 
-    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
+    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
 
     return RISCV_EXCP_NONE;
 }
@@ -2469,8 +2477,9 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
 static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
-    uint64_t mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
+    uint64_t menvcfg_mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
                                     HENVCFG_ADUE);
+    uint64_t mask = env->menvcfg & menvcfg_mask;
     uint64_t valh = (uint64_t)val << 32;
     RISCVException ret;
 
@@ -2479,7 +2488,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
         return ret;
     }
 
-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (valh & mask);
     return RISCV_EXCP_NONE;
 }
 
-- 
2.45.2

[PATCH v4 2/9] target/riscv: Add Ssdbltrp CSRs handling

[Clément Léger]

Add ext_ssdbltrp in RISCVCPUConfig and implement MSTATUS.SDT,
{H|M}ENVCFG.DTE and modify the availability of MTVAL2 based on the
presence of the Ssdbltrp ISA extension.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_bits.h   |  6 ++++
 target/riscv/cpu_cfg.h    |  1 +
 target/riscv/cpu_helper.c | 17 ++++++++++
 target/riscv/csr.c        | 68 ++++++++++++++++++++++++++++++++++-----
 5 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 832556cc34..695de5667f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -553,6 +553,7 @@ void riscv_cpu_set_geilen(CPURISCVState *env, target_ulong geilen);
 bool riscv_cpu_vector_enabled(CPURISCVState *env);
 void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
 int riscv_env_mmu_index(CPURISCVState *env, bool ifetch);
+bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt);
 G_NORETURN void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
                                                MMUAccessType access_type,
                                                int mmu_idx, uintptr_t retaddr);
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 1a5200d1d5..08cc5b2e22 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -558,6 +558,7 @@
 #define MSTATUS_TVM         0x00100000 /* since: priv-1.10 */
 #define MSTATUS_TW          0x00200000 /* since: priv-1.10 */
 #define MSTATUS_TSR         0x00400000 /* since: priv-1.10 */
+#define MSTATUS_SDT         0x01000000
 #define MSTATUS_GVA         0x4000000000ULL
 #define MSTATUS_MPV         0x8000000000ULL
 
@@ -588,6 +589,7 @@ typedef enum {
 #define SSTATUS_XS          0x00018000
 #define SSTATUS_SUM         0x00040000 /* since: priv-1.10 */
 #define SSTATUS_MXR         0x00080000
+#define SSTATUS_SDT         0x01000000
 
 #define SSTATUS64_UXL       0x0000000300000000ULL
 
@@ -777,11 +779,13 @@ typedef enum RISCVException {
 #define MENVCFG_CBIE                       (3UL << 4)
 #define MENVCFG_CBCFE                      BIT(6)
 #define MENVCFG_CBZE                       BIT(7)
+#define MENVCFG_DTE                        (1ULL << 59)
 #define MENVCFG_ADUE                       (1ULL << 61)
 #define MENVCFG_PBMTE                      (1ULL << 62)
 #define MENVCFG_STCE                       (1ULL << 63)
 
 /* For RV32 */
+#define MENVCFGH_DTE                       BIT(27)
 #define MENVCFGH_ADUE                      BIT(29)
 #define MENVCFGH_PBMTE                     BIT(30)
 #define MENVCFGH_STCE                      BIT(31)
@@ -795,11 +799,13 @@ typedef enum RISCVException {
 #define HENVCFG_CBIE                       MENVCFG_CBIE
 #define HENVCFG_CBCFE                      MENVCFG_CBCFE
 #define HENVCFG_CBZE                       MENVCFG_CBZE
+#define HENVCFG_DTE                        MENVCFG_DTE
 #define HENVCFG_ADUE                       MENVCFG_ADUE
 #define HENVCFG_PBMTE                      MENVCFG_PBMTE
 #define HENVCFG_STCE                       MENVCFG_STCE
 
 /* For RV32 */
+#define HENVCFGH_DTE                        MENVCFGH_DTE
 #define HENVCFGH_ADUE                       MENVCFGH_ADUE
 #define HENVCFGH_PBMTE                      MENVCFGH_PBMTE
 #define HENVCFGH_STCE                       MENVCFGH_STCE
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 092744360e..518102d748 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -77,6 +77,7 @@ struct RISCVCPUConfig {
     bool ext_smstateen;
     bool ext_sstc;
     bool ext_smcntrpmf;
+    bool ext_ssdbltrp;
     bool ext_svadu;
     bool ext_svinval;
     bool ext_svnapot;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 648d4ed833..b9f36e8621 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -63,6 +63,19 @@ int riscv_env_mmu_index(CPURISCVState *env, bool ifetch)
 #endif
 }
 
+bool riscv_env_smode_dbltrp_enabled(CPURISCVState *env, bool virt)
+{
+#ifdef CONFIG_USER_ONLY
+    return false;
+#else
+    if (virt) {
+        return (env->henvcfg & HENVCFG_DTE) != 0;
+    } else {
+        return (env->menvcfg & MENVCFG_DTE) != 0;
+    }
+#endif
+}
+
 void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
                           uint64_t *cs_base, uint32_t *pflags)
 {
@@ -562,6 +575,10 @@ void riscv_cpu_swap_hypervisor_regs(CPURISCVState *env)
 
     g_assert(riscv_has_ext(env, RVH));
 
+    if (riscv_env_smode_dbltrp_enabled(env, current_virt)) {
+        mstatus_mask |= MSTATUS_SDT;
+    }
+
     if (current_virt) {
         /* Current V=1 and we are about to change to V=0 */
         env->vsstatus = env->mstatus & mstatus_mask;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9e832e0b39..9aa33611f7 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -540,6 +540,15 @@ static RISCVException aia_hmode32(CPURISCVState *env, int csrno)
     return hmode32(env, csrno);
 }
 
+static RISCVException dbltrp_hmode(CPURISCVState *env, int csrno)
+{
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        return RISCV_EXCP_NONE;
+    }
+
+    return hmode(env, csrno);
+}
+
 static RISCVException pmp(CPURISCVState *env, int csrno)
 {
     if (riscv_cpu_cfg(env)->pmp) {
@@ -1600,6 +1609,13 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
         mask |= MSTATUS_VS;
     }
 
+    if (riscv_env_smode_dbltrp_enabled(env, env->virt_enabled)) {
+        mask |= MSTATUS_SDT;
+        if ((val & MSTATUS_SDT) != 0) {
+            val &= ~MSTATUS_SIE;
+        }
+    }
+
     if (xl != MXL_RV32 || env->debugger) {
         if (riscv_has_ext(env, RVH)) {
             mask |= MSTATUS_MPV | MSTATUS_GVA;
@@ -2356,7 +2372,11 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
     if (riscv_cpu_mxl(env) == MXL_RV64) {
         mask |= (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
                 (cfg->ext_sstc ? MENVCFG_STCE : 0) |
-                (cfg->ext_svadu ? MENVCFG_ADUE : 0);
+                (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
+                (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
+        if ((val & MENVCFG_DTE) == 0) {
+            env->mstatus &= ~MSTATUS_SDT;
+        }
     }
     env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
     write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
@@ -2379,9 +2399,14 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
     const RISCVCPUConfig *cfg = riscv_cpu_cfg(env);
     uint64_t mask = (cfg->ext_svpbmt ? MENVCFG_PBMTE : 0) |
                     (cfg->ext_sstc ? MENVCFG_STCE : 0) |
-                    (cfg->ext_svadu ? MENVCFG_ADUE : 0);
+                    (cfg->ext_svadu ? MENVCFG_ADUE : 0) |
+                    (cfg->ext_ssdbltrp ? MENVCFG_DTE : 0);
     uint64_t valh = (uint64_t)val << 32;
 
+    if ((valh & MENVCFG_DTE) == 0) {
+        env->mstatus &= ~MSTATUS_SDT;
+    }
+
     env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
     write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
 
@@ -2431,9 +2456,10 @@ static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
      * henvcfg.pbmte is read_only 0 when menvcfg.pbmte = 0
      * henvcfg.stce is read_only 0 when menvcfg.stce = 0
      * henvcfg.adue is read_only 0 when menvcfg.adue = 0
+     * henvcfg.dte is read_only 0 when menvcfg.dte = 0
      */
-    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
-                           env->menvcfg);
+    *val = env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE |
+                             HENVCFG_DTE) | env->menvcfg);
     return RISCV_EXCP_NONE;
 }
 
@@ -2451,10 +2477,16 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
         menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
+        if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+            menvcfg_mask |= HENVCFG_DTE;
+        }
         mask |= env->menvcfg & menvcfg_mask;
     }
 
     env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
+    if ((env->henvcfg & HENVCFG_DTE) == 0) {
+        env->vsstatus &= ~MSTATUS_SDT;
+    }
 
     return RISCV_EXCP_NONE;
 }
@@ -2469,8 +2501,8 @@ static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
         return ret;
     }
 
-    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE) |
-                            env->menvcfg)) >> 32;
+    *val = (env->henvcfg & (~(HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE |
+                              HENVCFG_DTE) | env->menvcfg)) >> 32;
     return RISCV_EXCP_NONE;
 }
 
@@ -2478,7 +2510,7 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
                                      target_ulong val)
 {
     uint64_t menvcfg_mask = env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE |
-                                    HENVCFG_ADUE);
+                                    HENVCFG_ADUE | HENVCFG_DTE);
     uint64_t mask = env->menvcfg & menvcfg_mask;
     uint64_t valh = (uint64_t)val << 32;
     RISCVException ret;
@@ -2489,6 +2521,10 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
     }
 
     env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (valh & mask);
+    if ((env->henvcfg & HENVCFG_DTE) == 0) {
+        env->vsstatus &= ~MSTATUS_SDT;
+    }
+
     return RISCV_EXCP_NONE;
 }
 
@@ -2916,6 +2952,9 @@ static RISCVException read_sstatus_i128(CPURISCVState *env, int csrno,
     if (env->xl != MXL_RV32 || env->debugger) {
         mask |= SSTATUS64_UXL;
     }
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        mask |= SSTATUS_SDT;
+    }
 
     *val = int128_make128(sstatus, add_status_sd(MXL_RV128, sstatus));
     return RISCV_EXCP_NONE;
@@ -2928,6 +2967,9 @@ static RISCVException read_sstatus(CPURISCVState *env, int csrno,
     if (env->xl != MXL_RV32 || env->debugger) {
         mask |= SSTATUS64_UXL;
     }
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        mask |= SSTATUS_SDT;
+    }
     /* TODO: Use SXL not MXL. */
     *val = add_status_sd(riscv_cpu_mxl(env), env->mstatus & mask);
     return RISCV_EXCP_NONE;
@@ -2943,6 +2985,9 @@ static RISCVException write_sstatus(CPURISCVState *env, int csrno,
             mask |= SSTATUS64_UXL;
         }
     }
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        mask |= SSTATUS_SDT;
+    }
     target_ulong newval = (env->mstatus & ~mask) | (val & mask);
     return write_mstatus(env, CSR_MSTATUS, newval);
 }
@@ -4048,6 +4093,13 @@ static RISCVException write_vsstatus(CPURISCVState *env, int csrno,
     if ((val & VSSTATUS64_UXL) == 0) {
         mask &= ~VSSTATUS64_UXL;
     }
+    if ((env->henvcfg & HENVCFG_DTE)) {
+        if ((val & SSTATUS_SDT) != 0) {
+            val &= ~SSTATUS_SIE;
+        }
+    } else {
+        val &= ~SSTATUS_SDT;
+    }
     env->vsstatus = (env->vsstatus & ~mask) | (uint64_t)val;
     return RISCV_EXCP_NONE;
 }
@@ -5255,7 +5307,7 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_VSATP]       = { "vsatp",       hmode,   read_vsatp,    write_vsatp,
                           .min_priv_ver = PRIV_VERSION_1_12_0                },
 
-    [CSR_MTVAL2]      = { "mtval2",      hmode,   read_mtval2,   write_mtval2,
+    [CSR_MTVAL2]      = { "mtval2", dbltrp_hmode, read_mtval2, write_mtval2,
                           .min_priv_ver = PRIV_VERSION_1_12_0                },
     [CSR_MTINST]      = { "mtinst",      hmode,   read_mtinst,   write_mtinst,
                           .min_priv_ver = PRIV_VERSION_1_12_0                },
-- 
2.45.2

[PATCH v4 3/9] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior

[Clément Léger]

When the Ssdbltrp extension is enabled, SSTATUS.SDT field is cleared
when executing sret. When executing mret/mnret, SSTATUS.SDT is cleared
when returning to U, VS or VU and VSSTATUS.SDT is cleared when returning
to VU from HS.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/op_helper.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 180886f32a..dabc74de39 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -287,6 +287,18 @@ target_ulong helper_sret(CPURISCVState *env)
                         get_field(mstatus, MSTATUS_SPIE));
     mstatus = set_field(mstatus, MSTATUS_SPIE, 1);
     mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
+
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        if (riscv_has_ext(env, RVH)) {
+            target_ulong prev_vu = get_field(env->hstatus, HSTATUS_SPV) &&
+                                   prev_priv == PRV_U;
+            /* Returning to VU from HS, vsstatus.sdt = 0 */
+            if (!env->virt_enabled && prev_vu) {
+                env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
+            }
+        }
+        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
+    }
     if (env->priv_ver >= PRIV_VERSION_1_12_0) {
         mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
     }
@@ -297,7 +309,6 @@ target_ulong helper_sret(CPURISCVState *env)
         target_ulong hstatus = env->hstatus;
 
         prev_virt = get_field(hstatus, HSTATUS_SPV);
-
         hstatus = set_field(hstatus, HSTATUS_SPV, 0);
 
         env->hstatus = hstatus;
@@ -328,6 +339,22 @@ static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
         riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
     }
 }
+static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
+                                   target_ulong prev_priv,
+                                   target_ulong prev_virt)
+{
+    /* If returning to U, VS or VU, sstatus.sdt = 0 */
+    if (prev_priv == PRV_U || (prev_virt &&
+        (prev_priv == PRV_S || prev_priv == PRV_U))) {
+        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
+        /* If returning to VU, vsstatus.sdt = 0 */
+        if (prev_virt && prev_priv == PRV_U) {
+            env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
+        }
+    }
+
+    return mstatus;
+}
 
 target_ulong helper_mret(CPURISCVState *env)
 {
@@ -345,6 +372,9 @@ target_ulong helper_mret(CPURISCVState *env)
     mstatus = set_field(mstatus, MSTATUS_MPP,
                         riscv_has_ext(env, RVU) ? PRV_U : PRV_M);
     mstatus = set_field(mstatus, MSTATUS_MPV, 0);
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        mstatus = ssdbltrp_mxret(env, mstatus, prev_priv, prev_virt);
+    }
     if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != PRV_M)) {
         mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
     }
@@ -378,6 +408,9 @@ target_ulong helper_mnret(CPURISCVState *env)
     if (prev_priv < PRV_M) {
         env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
     }
+    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
+        env->mstatus = ssdbltrp_mxret(env, env->mstatus, prev_priv, prev_virt);
+    }
 
     if (riscv_has_ext(env, RVH) && prev_virt) {
         riscv_cpu_swap_hypervisor_regs(env);
-- 
2.45.2

[PATCH v4 4/9] target/riscv: Implement Ssdbltrp exception handling

[Clément Léger]

When the Ssdbltrp ISA extension is enabled, if a trap happens in S-mode
while SSTATUS.SDT isn't cleared, generate a double trap exception to
M-mode.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/cpu.c        |  2 +-
 target/riscv/cpu_bits.h   |  1 +
 target/riscv/cpu_helper.c | 42 ++++++++++++++++++++++++++++++++++-----
 3 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index fed64741d1..5224eb356d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -285,7 +285,7 @@ static const char * const riscv_excp_names[] = {
     "load_page_fault",
     "reserved",
     "store_page_fault",
-    "reserved",
+    "double_trap",
     "reserved",
     "reserved",
     "reserved",
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 08cc5b2e22..0d0f253fcb 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -699,6 +699,7 @@ typedef enum RISCVException {
     RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
     RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
     RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
+    RISCV_EXCP_DOUBLE_TRAP = 0x10,
     RISCV_EXCP_SW_CHECK = 0x12, /* since: priv-1.13.0 */
     RISCV_EXCP_HW_ERR = 0x13, /* since: priv-1.13.0 */
     RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index b9f36e8621..623a3abbf7 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1715,6 +1715,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     CPURISCVState *env = &cpu->env;
     bool virt = env->virt_enabled;
     bool write_gva = false;
+    bool vsmode_exc;
     uint64_t s;
     int mode;
 
@@ -1729,6 +1730,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         !(env->mip & (1 << cause));
     bool vs_injected = env->hvip & (1 << cause) & env->hvien &&
         !(env->mip & (1 << cause));
+    bool smode_double_trap = false;
+    uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
     target_ulong tval = 0;
     target_ulong tinst = 0;
     target_ulong htval = 0;
@@ -1839,13 +1842,34 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     mode = env->priv <= PRV_S && cause < 64 &&
         (((deleg >> cause) & 1) || s_injected || vs_injected) ? PRV_S : PRV_M;
 
+    vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) || vs_injected);
+    /*
+     * Check double trap condition only if already in S-mode and targeting
+     * S-mode
+     */
+    if (cpu->cfg.ext_ssdbltrp && env->priv == PRV_S && mode == PRV_S) {
+        bool dte = (env->menvcfg & MENVCFG_DTE) != 0;
+        bool sdt = (env->mstatus & MSTATUS_SDT) != 0;
+        /* In VS or HS */
+        if (riscv_has_ext(env, RVH)) {
+            if (vsmode_exc) {
+                /* VS -> VS, use henvcfg instead of menvcfg*/
+                dte = (env->henvcfg & HENVCFG_DTE) != 0;
+            } else if (env->virt_enabled) {
+                /* VS -> HS, use mstatus_hs */
+                sdt = (env->mstatus_hs & MSTATUS_SDT) != 0;
+            }
+        }
+        smode_double_trap = dte && sdt;
+        if (smode_double_trap) {
+            mode = PRV_M;
+        }
+    }
+
     if (mode == PRV_S) {
         /* handle the trap in S-mode */
         if (riscv_has_ext(env, RVH)) {
-            uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
-
-            if (env->virt_enabled &&
-                (((hdeleg >> cause) & 1) || vs_injected)) {
+            if (vsmode_exc) {
                 /* Trap to VS mode */
                 /*
                  * See if we need to adjust cause. Yes if its VS mode interrupt
@@ -1878,6 +1902,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         s = set_field(s, MSTATUS_SPIE, get_field(s, MSTATUS_SIE));
         s = set_field(s, MSTATUS_SPP, env->priv);
         s = set_field(s, MSTATUS_SIE, 0);
+        if (riscv_env_smode_dbltrp_enabled(env, virt)) {
+            s = set_field(s, MSTATUS_SDT, 1);
+        }
         env->mstatus = s;
         sxlen = 16 << riscv_cpu_sxl(env);
         env->scause = cause | ((target_ulong)async << (sxlen - 1));
@@ -1913,9 +1940,14 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         env->mstatus = s;
         mxlen = 16 << riscv_cpu_mxl(env);
         env->mcause = cause | ((target_ulong)async << (mxlen - 1));
+        if (smode_double_trap) {
+            env->mtval2 = env->mcause;
+            env->mcause = RISCV_EXCP_DOUBLE_TRAP;
+        } else {
+            env->mtval2 = mtval2;
+        }
         env->mepc = env->pc;
         env->mtval = tval;
-        env->mtval2 = mtval2;
         env->mtinst = tinst;
 
         /*
-- 
2.45.2

[PATCH v4 5/9] target/riscv: Add Ssdbltrp ISA extension enable switch

[Clément Léger]

Add the switch to enable the Ssdbltrp ISA extension.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5224eb356d..39555364bf 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -190,6 +190,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
     ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
     ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
+    ISA_EXT_DATA_ENTRY(ssdbltrp, PRIV_VERSION_1_13_0, ext_ssdbltrp),
     ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
     ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
     ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
@@ -1506,6 +1507,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("smrnmi", ext_smrnmi, false),
     MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
     MULTI_EXT_CFG_BOOL("ssaia", ext_ssaia, false),
+    MULTI_EXT_CFG_BOOL("ssdbltrp", ext_ssdbltrp, false),
     MULTI_EXT_CFG_BOOL("svade", ext_svade, false),
     MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
     MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
-- 
2.45.2

[PATCH v4 6/9] target/riscv: Add Smdbltrp CSRs handling

[Clément Léger]

Add `ext_smdbltrp`in RISCVCPUConfig and implement MSTATUS.MDT behavior.
Also set MDT to 1 at reset according to the specification.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/cpu.c      |  3 +++
 target/riscv/cpu_bits.h |  1 +
 target/riscv/cpu_cfg.h  |  1 +
 target/riscv/csr.c      | 13 +++++++++++++
 4 files changed, 18 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 39555364bf..15b21e4f7d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -959,6 +959,9 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
             env->mstatus_hs = set_field(env->mstatus_hs,
                                         MSTATUS64_UXL, env->misa_mxl);
         }
+        if (riscv_cpu_cfg(env)->ext_smdbltrp) {
+            env->mstatus = set_field(env->mstatus, MSTATUS_MDT, 1);
+        }
     }
     env->mcause = 0;
     env->miclaim = MIP_SGEIP;
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 0d0f253fcb..b368e27ca0 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -561,6 +561,7 @@
 #define MSTATUS_SDT         0x01000000
 #define MSTATUS_GVA         0x4000000000ULL
 #define MSTATUS_MPV         0x8000000000ULL
+#define MSTATUS_MDT         0x40000000000ULL /* Smdbltrp extension */
 
 #define MSTATUS64_UXL       0x0000000300000000ULL
 #define MSTATUS64_SXL       0x0000000C00000000ULL
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 518102d748..8ac1e7fce3 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -78,6 +78,7 @@ struct RISCVCPUConfig {
     bool ext_sstc;
     bool ext_smcntrpmf;
     bool ext_ssdbltrp;
+    bool ext_smdbltrp;
     bool ext_svadu;
     bool ext_svinval;
     bool ext_svnapot;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9aa33611f7..9d2caf34ba 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1616,6 +1616,13 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
         }
     }
 
+    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
+        mask |= MSTATUS_MDT;
+        if ((val & MSTATUS_MDT) != 0) {
+            val &= ~MSTATUS_MIE;
+        }
+    }
+
     if (xl != MXL_RV32 || env->debugger) {
         if (riscv_has_ext(env, RVH)) {
             mask |= MSTATUS_MPV | MSTATUS_GVA;
@@ -1654,6 +1661,12 @@ static RISCVException write_mstatush(CPURISCVState *env, int csrno,
     uint64_t valh = (uint64_t)val << 32;
     uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV | MSTATUS_GVA : 0;
 
+    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
+        mask |= MSTATUS_MDT;
+        if ((valh & MSTATUS_MDT) != 0) {
+            mask |= MSTATUS_MIE;
+        }
+    }
     env->mstatus = (env->mstatus & ~mask) | (valh & mask);
 
     return RISCV_EXCP_NONE;
-- 
2.45.2

[PATCH v4 7/9] target/riscv: Implement Smdbltrp sret, mret and mnret behavior

[Clément Léger]

When the Ssdbltrp extension is enabled, SSTATUS.MDT field is cleared
when executing sret if executed in M-mode. When executing mret/mnret,
SSTATUS.MDT is cleared.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/op_helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index dabc74de39..64c5792af8 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -299,6 +299,9 @@ target_ulong helper_sret(CPURISCVState *env)
         }
         mstatus = set_field(mstatus, MSTATUS_SDT, 0);
     }
+    if (riscv_cpu_cfg(env)->ext_smdbltrp && env->priv >= PRV_M) {
+        mstatus = set_field(mstatus, MSTATUS_MDT, 0);
+    }
     if (env->priv_ver >= PRIV_VERSION_1_12_0) {
         mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
     }
@@ -375,6 +378,9 @@ target_ulong helper_mret(CPURISCVState *env)
     if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
         mstatus = ssdbltrp_mxret(env, mstatus, prev_priv, prev_virt);
     }
+    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
+        mstatus = set_field(mstatus, MSTATUS_MDT, 0);
+    }
     if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != PRV_M)) {
         mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
     }
@@ -412,6 +418,12 @@ target_ulong helper_mnret(CPURISCVState *env)
         env->mstatus = ssdbltrp_mxret(env, env->mstatus, prev_priv, prev_virt);
     }
 
+    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
+        if (prev_priv < PRV_M) {
+            env->mstatus = set_field(env->mstatus, MSTATUS_MDT, false);
+        }
+    }
+
     if (riscv_has_ext(env, RVH) && prev_virt) {
         riscv_cpu_swap_hypervisor_regs(env);
     }
-- 
2.45.2

[PATCH v4 8/9] target/riscv: Implement Smdbltrp behavior

[Clément Léger]

When the Smsdbltrp ISA extension is enabled, if a trap happens while
MSTATUS.MDT is already set, it will trigger an abort or an NMI is the
Smrnmi extension is available.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/cpu_helper.c | 52 +++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 623a3abbf7..8825572d5e 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1703,6 +1703,17 @@ static target_ulong riscv_transformed_insn(CPURISCVState *env,
     return xinsn;
 }
 
+static void riscv_do_nmi(CPURISCVState *env, target_ulong cause, bool virt)
+{
+    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
+    env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPV, virt);
+    env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPP, env->priv);
+    env->mncause = cause;
+    env->mnepc = env->pc;
+    env->pc = env->rnmi_irqvec;
+    riscv_cpu_set_mode(env, PRV_M, false);
+}
+
 /*
  * Handle Traps
  *
@@ -1741,15 +1752,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
     bool nnmi_excep = false;
 
     if (cpu->cfg.ext_smrnmi && env->rnmip && async) {
-        env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
-        env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPV,
-                                  env->virt_enabled);
-        env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPP,
-                                  env->priv);
-        env->mncause = cause | ((target_ulong)1U << (TARGET_LONG_BITS - 1));
-        env->mnepc = env->pc;
-        env->pc = env->rnmi_irqvec;
-        riscv_cpu_set_mode(env, PRV_M, virt);
+        riscv_do_nmi(env, cause | ((target_ulong)1U << (TARGET_LONG_BITS - 1)),
+                     virt);
         return;
     }
 
@@ -1932,11 +1936,32 @@ void riscv_cpu_do_interrupt(CPUState *cs)
             /* Trapping to M mode, virt is disabled */
             virt = false;
         }
+        /*
+         * If the hart encounters an exception while executing in M-mode,
+         * with the mnstatus.NMIE bit clear, the program counter is set to
+         * the RNMI exception trap handler address.
+         */
+        nnmi_excep = cpu->cfg.ext_smrnmi &&
+                     !get_field(env->mnstatus, MNSTATUS_NMIE) &&
+                     !async;
 
         s = env->mstatus;
         s = set_field(s, MSTATUS_MPIE, get_field(s, MSTATUS_MIE));
         s = set_field(s, MSTATUS_MPP, env->priv);
         s = set_field(s, MSTATUS_MIE, 0);
+        if (cpu->cfg.ext_smdbltrp) {
+            if (env->mstatus & MSTATUS_MDT) {
+                assert(env->priv == PRV_M);
+                if (!cpu->cfg.ext_smrnmi || nnmi_excep) {
+                    cpu_abort(CPU(cpu), "M-mode double trap\n");
+                } else {
+                    riscv_do_nmi(env, cause, false);
+                    return;
+                }
+            }
+
+            s = set_field(s, MSTATUS_MDT, 1);
+        }
         env->mstatus = s;
         mxlen = 16 << riscv_cpu_mxl(env);
         env->mcause = cause | ((target_ulong)async << (mxlen - 1));
@@ -1950,15 +1975,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
         env->mtval = tval;
         env->mtinst = tinst;
 
-        /*
-         * If the hart encounters an exception while executing in M-mode,
-         * with the mnstatus.NMIE bit clear, the program counter is set to
-         * the RNMI exception trap handler address.
-         */
-        nnmi_excep = cpu->cfg.ext_smrnmi &&
-                     !get_field(env->mnstatus, MNSTATUS_NMIE) &&
-                     !async;
-
         if (nnmi_excep) {
             env->pc = env->rnmi_excpvec;
         } else {
-- 
2.45.2

[PATCH v4 9/9] target/riscv: Add Smdbltrp ISA extension enable switch

[Clément Léger]

Add the switch to enable the Smdbltrp ISA extension.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
---
 target/riscv/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 15b21e4f7d..1323effdae 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -183,6 +183,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
     ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
     ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
+    ISA_EXT_DATA_ENTRY(smdbltrp, PRIV_VERSION_1_13_0, ext_smdbltrp),
     ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
     ISA_EXT_DATA_ENTRY(smrnmi, PRIV_VERSION_1_12_0, ext_smrnmi),
     ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
@@ -1506,6 +1507,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
 
     MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
+    MULTI_EXT_CFG_BOOL("smdbltrp", ext_smdbltrp, false),
     MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
     MULTI_EXT_CFG_BOOL("smrnmi", ext_smrnmi, false),
     MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
-- 
2.45.2

Re: [PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> With the current implementation, if we had the current scenario:
> - set bit x in menvcfg
> - set bit x in henvcfg
> - clear bit x in menvcfg
> then, the internal variable env->henvcfg would still contain bit x due
> to both a wrong menvcfg mask used in write_henvcfg() as well as a
> missing update of henvcfg upon menvcfg update.
> This can lead to some wrong interpretation of the context. In order to
> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
> menvcfg and fix the mask computation used in write_henvcfg() that is
> used to mesk env->menvcfg value (which could still lead to some stale
> bits). The same mechanism is also applied for henvcfgh writing.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  target/riscv/csr.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index b84b436151..9e832e0b39 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> +                                    target_ulong val);
>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>                                      target_ulong val)
>  {
> @@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>                  (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>      }
>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
>
>      return RISCV_EXCP_NONE;
>  }
> @@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> +                                    target_ulong val);
>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>                                       target_ulong val)
>  {
> @@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>      uint64_t valh = (uint64_t)val << 32;
>
>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
>
>      return RISCV_EXCP_NONE;
>  }
> @@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>                                      target_ulong val)
>  {
>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> +    uint64_t menvcfg_mask = 0;
>      RISCVException ret;
>
>      ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>      }
>
>      if (riscv_cpu_mxl(env) == MXL_RV64) {
> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
> +        mask |= env->menvcfg & menvcfg_mask;

This doesn't seem right.

Should it be something like

    if (riscv_cpu_mxl(env) == MXL_RV64) {
        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
    }

    mask = env->menvcfg & mask;

>      }
>
> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> +    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);

Using both menvcfg_mask and mask seems wrong here

Alistair

Re: [PATCH v4 3/9] target/riscv: Implement Ssdbltrp sret, mret and mnret behavior

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:53 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> When the Ssdbltrp extension is enabled, SSTATUS.SDT field is cleared
> when executing sret. When executing mret/mnret, SSTATUS.SDT is cleared
> when returning to U, VS or VU and VSSTATUS.SDT is cleared when returning
> to VU from HS.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

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

Alistair

> ---
>  target/riscv/op_helper.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 180886f32a..dabc74de39 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -287,6 +287,18 @@ target_ulong helper_sret(CPURISCVState *env)
>                          get_field(mstatus, MSTATUS_SPIE));
>      mstatus = set_field(mstatus, MSTATUS_SPIE, 1);
>      mstatus = set_field(mstatus, MSTATUS_SPP, PRV_U);
> +
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        if (riscv_has_ext(env, RVH)) {
> +            target_ulong prev_vu = get_field(env->hstatus, HSTATUS_SPV) &&
> +                                   prev_priv == PRV_U;
> +            /* Returning to VU from HS, vsstatus.sdt = 0 */
> +            if (!env->virt_enabled && prev_vu) {
> +                env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
> +            }
> +        }
> +        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
> +    }
>      if (env->priv_ver >= PRIV_VERSION_1_12_0) {
>          mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
>      }
> @@ -297,7 +309,6 @@ target_ulong helper_sret(CPURISCVState *env)
>          target_ulong hstatus = env->hstatus;
>
>          prev_virt = get_field(hstatus, HSTATUS_SPV);
> -
>          hstatus = set_field(hstatus, HSTATUS_SPV, 0);
>
>          env->hstatus = hstatus;
> @@ -328,6 +339,22 @@ static void check_ret_from_m_mode(CPURISCVState *env, target_ulong retpc,
>          riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
>      }
>  }
> +static target_ulong ssdbltrp_mxret(CPURISCVState *env, target_ulong mstatus,
> +                                   target_ulong prev_priv,
> +                                   target_ulong prev_virt)
> +{
> +    /* If returning to U, VS or VU, sstatus.sdt = 0 */
> +    if (prev_priv == PRV_U || (prev_virt &&
> +        (prev_priv == PRV_S || prev_priv == PRV_U))) {
> +        mstatus = set_field(mstatus, MSTATUS_SDT, 0);
> +        /* If returning to VU, vsstatus.sdt = 0 */
> +        if (prev_virt && prev_priv == PRV_U) {
> +            env->vsstatus = set_field(env->vsstatus, MSTATUS_SDT, 0);
> +        }
> +    }
> +
> +    return mstatus;
> +}
>
>  target_ulong helper_mret(CPURISCVState *env)
>  {
> @@ -345,6 +372,9 @@ target_ulong helper_mret(CPURISCVState *env)
>      mstatus = set_field(mstatus, MSTATUS_MPP,
>                          riscv_has_ext(env, RVU) ? PRV_U : PRV_M);
>      mstatus = set_field(mstatus, MSTATUS_MPV, 0);
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        mstatus = ssdbltrp_mxret(env, mstatus, prev_priv, prev_virt);
> +    }
>      if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != PRV_M)) {
>          mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
>      }
> @@ -378,6 +408,9 @@ target_ulong helper_mnret(CPURISCVState *env)
>      if (prev_priv < PRV_M) {
>          env->mstatus = set_field(env->mstatus, MSTATUS_MPRV, false);
>      }
> +    if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
> +        env->mstatus = ssdbltrp_mxret(env, env->mstatus, prev_priv, prev_virt);
> +    }
>
>      if (riscv_has_ext(env, RVH) && prev_virt) {
>          riscv_cpu_swap_hypervisor_regs(env);
> --
> 2.45.2
>
>

Re: [PATCH v4 4/9] target/riscv: Implement Ssdbltrp exception handling

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:54 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> When the Ssdbltrp ISA extension is enabled, if a trap happens in S-mode
> while SSTATUS.SDT isn't cleared, generate a double trap exception to
> M-mode.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

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

Alistair

> ---
>  target/riscv/cpu.c        |  2 +-
>  target/riscv/cpu_bits.h   |  1 +
>  target/riscv/cpu_helper.c | 42 ++++++++++++++++++++++++++++++++++-----
>  3 files changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index fed64741d1..5224eb356d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -285,7 +285,7 @@ static const char * const riscv_excp_names[] = {
>      "load_page_fault",
>      "reserved",
>      "store_page_fault",
> -    "reserved",
> +    "double_trap",
>      "reserved",
>      "reserved",
>      "reserved",
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 08cc5b2e22..0d0f253fcb 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -699,6 +699,7 @@ typedef enum RISCVException {
>      RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
>      RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
>      RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
> +    RISCV_EXCP_DOUBLE_TRAP = 0x10,
>      RISCV_EXCP_SW_CHECK = 0x12, /* since: priv-1.13.0 */
>      RISCV_EXCP_HW_ERR = 0x13, /* since: priv-1.13.0 */
>      RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index b9f36e8621..623a3abbf7 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1715,6 +1715,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      CPURISCVState *env = &cpu->env;
>      bool virt = env->virt_enabled;
>      bool write_gva = false;
> +    bool vsmode_exc;
>      uint64_t s;
>      int mode;
>
> @@ -1729,6 +1730,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          !(env->mip & (1 << cause));
>      bool vs_injected = env->hvip & (1 << cause) & env->hvien &&
>          !(env->mip & (1 << cause));
> +    bool smode_double_trap = false;
> +    uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
>      target_ulong tval = 0;
>      target_ulong tinst = 0;
>      target_ulong htval = 0;
> @@ -1839,13 +1842,34 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      mode = env->priv <= PRV_S && cause < 64 &&
>          (((deleg >> cause) & 1) || s_injected || vs_injected) ? PRV_S : PRV_M;
>
> +    vsmode_exc = env->virt_enabled && (((hdeleg >> cause) & 1) || vs_injected);
> +    /*
> +     * Check double trap condition only if already in S-mode and targeting
> +     * S-mode
> +     */
> +    if (cpu->cfg.ext_ssdbltrp && env->priv == PRV_S && mode == PRV_S) {
> +        bool dte = (env->menvcfg & MENVCFG_DTE) != 0;
> +        bool sdt = (env->mstatus & MSTATUS_SDT) != 0;
> +        /* In VS or HS */
> +        if (riscv_has_ext(env, RVH)) {
> +            if (vsmode_exc) {
> +                /* VS -> VS, use henvcfg instead of menvcfg*/
> +                dte = (env->henvcfg & HENVCFG_DTE) != 0;
> +            } else if (env->virt_enabled) {
> +                /* VS -> HS, use mstatus_hs */
> +                sdt = (env->mstatus_hs & MSTATUS_SDT) != 0;
> +            }
> +        }
> +        smode_double_trap = dte && sdt;
> +        if (smode_double_trap) {
> +            mode = PRV_M;
> +        }
> +    }
> +
>      if (mode == PRV_S) {
>          /* handle the trap in S-mode */
>          if (riscv_has_ext(env, RVH)) {
> -            uint64_t hdeleg = async ? env->hideleg : env->hedeleg;
> -
> -            if (env->virt_enabled &&
> -                (((hdeleg >> cause) & 1) || vs_injected)) {
> +            if (vsmode_exc) {
>                  /* Trap to VS mode */
>                  /*
>                   * See if we need to adjust cause. Yes if its VS mode interrupt
> @@ -1878,6 +1902,9 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          s = set_field(s, MSTATUS_SPIE, get_field(s, MSTATUS_SIE));
>          s = set_field(s, MSTATUS_SPP, env->priv);
>          s = set_field(s, MSTATUS_SIE, 0);
> +        if (riscv_env_smode_dbltrp_enabled(env, virt)) {
> +            s = set_field(s, MSTATUS_SDT, 1);
> +        }
>          env->mstatus = s;
>          sxlen = 16 << riscv_cpu_sxl(env);
>          env->scause = cause | ((target_ulong)async << (sxlen - 1));
> @@ -1913,9 +1940,14 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          env->mstatus = s;
>          mxlen = 16 << riscv_cpu_mxl(env);
>          env->mcause = cause | ((target_ulong)async << (mxlen - 1));
> +        if (smode_double_trap) {
> +            env->mtval2 = env->mcause;
> +            env->mcause = RISCV_EXCP_DOUBLE_TRAP;
> +        } else {
> +            env->mtval2 = mtval2;
> +        }
>          env->mepc = env->pc;
>          env->mtval = tval;
> -        env->mtval2 = mtval2;
>          env->mtinst = tinst;
>
>          /*
> --
> 2.45.2
>
>

Re: [PATCH v4 5/9] target/riscv: Add Ssdbltrp ISA extension enable switch

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:54 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> Add the switch to enable the Ssdbltrp ISA extension.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5224eb356d..39555364bf 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -190,6 +190,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(ssccptr, PRIV_VERSION_1_11_0, has_priv_1_11),
>      ISA_EXT_DATA_ENTRY(sscofpmf, PRIV_VERSION_1_12_0, ext_sscofpmf),
>      ISA_EXT_DATA_ENTRY(sscounterenw, PRIV_VERSION_1_12_0, has_priv_1_12),
> +    ISA_EXT_DATA_ENTRY(ssdbltrp, PRIV_VERSION_1_13_0, ext_ssdbltrp),
>      ISA_EXT_DATA_ENTRY(sstc, PRIV_VERSION_1_12_0, ext_sstc),
>      ISA_EXT_DATA_ENTRY(sstvala, PRIV_VERSION_1_12_0, has_priv_1_12),
>      ISA_EXT_DATA_ENTRY(sstvecd, PRIV_VERSION_1_12_0, has_priv_1_12),
> @@ -1506,6 +1507,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      MULTI_EXT_CFG_BOOL("smrnmi", ext_smrnmi, false),
>      MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
>      MULTI_EXT_CFG_BOOL("ssaia", ext_ssaia, false),
> +    MULTI_EXT_CFG_BOOL("ssdbltrp", ext_ssdbltrp, false),
>      MULTI_EXT_CFG_BOOL("svade", ext_svade, false),
>      MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
>      MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
> --
> 2.45.2
>
>

Re: [PATCH v4 6/9] target/riscv: Add Smdbltrp CSRs handling

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:54 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> Add `ext_smdbltrp`in RISCVCPUConfig and implement MSTATUS.MDT behavior.
> Also set MDT to 1 at reset according to the specification.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

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

Alistair

> ---
>  target/riscv/cpu.c      |  3 +++
>  target/riscv/cpu_bits.h |  1 +
>  target/riscv/cpu_cfg.h  |  1 +
>  target/riscv/csr.c      | 13 +++++++++++++
>  4 files changed, 18 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 39555364bf..15b21e4f7d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -959,6 +959,9 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>              env->mstatus_hs = set_field(env->mstatus_hs,
>                                          MSTATUS64_UXL, env->misa_mxl);
>          }
> +        if (riscv_cpu_cfg(env)->ext_smdbltrp) {
> +            env->mstatus = set_field(env->mstatus, MSTATUS_MDT, 1);
> +        }
>      }
>      env->mcause = 0;
>      env->miclaim = MIP_SGEIP;
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 0d0f253fcb..b368e27ca0 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -561,6 +561,7 @@
>  #define MSTATUS_SDT         0x01000000
>  #define MSTATUS_GVA         0x4000000000ULL
>  #define MSTATUS_MPV         0x8000000000ULL
> +#define MSTATUS_MDT         0x40000000000ULL /* Smdbltrp extension */
>
>  #define MSTATUS64_UXL       0x0000000300000000ULL
>  #define MSTATUS64_SXL       0x0000000C00000000ULL
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 518102d748..8ac1e7fce3 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -78,6 +78,7 @@ struct RISCVCPUConfig {
>      bool ext_sstc;
>      bool ext_smcntrpmf;
>      bool ext_ssdbltrp;
> +    bool ext_smdbltrp;
>      bool ext_svadu;
>      bool ext_svinval;
>      bool ext_svnapot;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 9aa33611f7..9d2caf34ba 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1616,6 +1616,13 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>          }
>      }
>
> +    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
> +        mask |= MSTATUS_MDT;
> +        if ((val & MSTATUS_MDT) != 0) {
> +            val &= ~MSTATUS_MIE;
> +        }
> +    }
> +
>      if (xl != MXL_RV32 || env->debugger) {
>          if (riscv_has_ext(env, RVH)) {
>              mask |= MSTATUS_MPV | MSTATUS_GVA;
> @@ -1654,6 +1661,12 @@ static RISCVException write_mstatush(CPURISCVState *env, int csrno,
>      uint64_t valh = (uint64_t)val << 32;
>      uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV | MSTATUS_GVA : 0;
>
> +    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
> +        mask |= MSTATUS_MDT;
> +        if ((valh & MSTATUS_MDT) != 0) {
> +            mask |= MSTATUS_MIE;
> +        }
> +    }
>      env->mstatus = (env->mstatus & ~mask) | (valh & mask);
>
>      return RISCV_EXCP_NONE;
> --
> 2.45.2
>
>

Re: [PATCH v4 7/9] target/riscv: Implement Smdbltrp sret, mret and mnret behavior

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:54 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> When the Ssdbltrp extension is enabled, SSTATUS.MDT field is cleared
> when executing sret if executed in M-mode. When executing mret/mnret,
> SSTATUS.MDT is cleared.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> ---
>  target/riscv/op_helper.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index dabc74de39..64c5792af8 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -299,6 +299,9 @@ target_ulong helper_sret(CPURISCVState *env)
>          }
>          mstatus = set_field(mstatus, MSTATUS_SDT, 0);
>      }
> +    if (riscv_cpu_cfg(env)->ext_smdbltrp && env->priv >= PRV_M) {
> +        mstatus = set_field(mstatus, MSTATUS_MDT, 0);
> +    }
>      if (env->priv_ver >= PRIV_VERSION_1_12_0) {
>          mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
>      }
> @@ -375,6 +378,9 @@ target_ulong helper_mret(CPURISCVState *env)
>      if (riscv_cpu_cfg(env)->ext_ssdbltrp) {
>          mstatus = ssdbltrp_mxret(env, mstatus, prev_priv, prev_virt);
>      }
> +    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
> +        mstatus = set_field(mstatus, MSTATUS_MDT, 0);
> +    }
>      if ((env->priv_ver >= PRIV_VERSION_1_12_0) && (prev_priv != PRV_M)) {
>          mstatus = set_field(mstatus, MSTATUS_MPRV, 0);
>      }
> @@ -412,6 +418,12 @@ target_ulong helper_mnret(CPURISCVState *env)
>          env->mstatus = ssdbltrp_mxret(env, env->mstatus, prev_priv, prev_virt);
>      }
>
> +    if (riscv_cpu_cfg(env)->ext_smdbltrp) {
> +        if (prev_priv < PRV_M) {
> +            env->mstatus = set_field(env->mstatus, MSTATUS_MDT, false);

0 instead of false, otherwise

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

Alistair

> +        }
> +    }
> +
>      if (riscv_has_ext(env, RVH) && prev_virt) {
>          riscv_cpu_swap_hypervisor_regs(env);
>      }
> --
> 2.45.2
>
>

Re: [PATCH v4 8/9] target/riscv: Implement Smdbltrp behavior

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:54 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> When the Smsdbltrp ISA extension is enabled, if a trap happens while
> MSTATUS.MDT is already set, it will trigger an abort or an NMI is the
> Smrnmi extension is available.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

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

Alistair

> ---
>  target/riscv/cpu_helper.c | 52 +++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 623a3abbf7..8825572d5e 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1703,6 +1703,17 @@ static target_ulong riscv_transformed_insn(CPURISCVState *env,
>      return xinsn;
>  }
>
> +static void riscv_do_nmi(CPURISCVState *env, target_ulong cause, bool virt)
> +{
> +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
> +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPV, virt);
> +    env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPP, env->priv);
> +    env->mncause = cause;
> +    env->mnepc = env->pc;
> +    env->pc = env->rnmi_irqvec;
> +    riscv_cpu_set_mode(env, PRV_M, false);
> +}
> +
>  /*
>   * Handle Traps
>   *
> @@ -1741,15 +1752,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>      bool nnmi_excep = false;
>
>      if (cpu->cfg.ext_smrnmi && env->rnmip && async) {
> -        env->mnstatus = set_field(env->mnstatus, MNSTATUS_NMIE, false);
> -        env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPV,
> -                                  env->virt_enabled);
> -        env->mnstatus = set_field(env->mnstatus, MNSTATUS_MNPP,
> -                                  env->priv);
> -        env->mncause = cause | ((target_ulong)1U << (TARGET_LONG_BITS - 1));
> -        env->mnepc = env->pc;
> -        env->pc = env->rnmi_irqvec;
> -        riscv_cpu_set_mode(env, PRV_M, virt);
> +        riscv_do_nmi(env, cause | ((target_ulong)1U << (TARGET_LONG_BITS - 1)),
> +                     virt);
>          return;
>      }
>
> @@ -1932,11 +1936,32 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>              /* Trapping to M mode, virt is disabled */
>              virt = false;
>          }
> +        /*
> +         * If the hart encounters an exception while executing in M-mode,
> +         * with the mnstatus.NMIE bit clear, the program counter is set to
> +         * the RNMI exception trap handler address.
> +         */
> +        nnmi_excep = cpu->cfg.ext_smrnmi &&
> +                     !get_field(env->mnstatus, MNSTATUS_NMIE) &&
> +                     !async;
>
>          s = env->mstatus;
>          s = set_field(s, MSTATUS_MPIE, get_field(s, MSTATUS_MIE));
>          s = set_field(s, MSTATUS_MPP, env->priv);
>          s = set_field(s, MSTATUS_MIE, 0);
> +        if (cpu->cfg.ext_smdbltrp) {
> +            if (env->mstatus & MSTATUS_MDT) {
> +                assert(env->priv == PRV_M);
> +                if (!cpu->cfg.ext_smrnmi || nnmi_excep) {
> +                    cpu_abort(CPU(cpu), "M-mode double trap\n");
> +                } else {
> +                    riscv_do_nmi(env, cause, false);
> +                    return;
> +                }
> +            }
> +
> +            s = set_field(s, MSTATUS_MDT, 1);
> +        }
>          env->mstatus = s;
>          mxlen = 16 << riscv_cpu_mxl(env);
>          env->mcause = cause | ((target_ulong)async << (mxlen - 1));
> @@ -1950,15 +1975,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>          env->mtval = tval;
>          env->mtinst = tinst;
>
> -        /*
> -         * If the hart encounters an exception while executing in M-mode,
> -         * with the mnstatus.NMIE bit clear, the program counter is set to
> -         * the RNMI exception trap handler address.
> -         */
> -        nnmi_excep = cpu->cfg.ext_smrnmi &&
> -                     !get_field(env->mnstatus, MNSTATUS_NMIE) &&
> -                     !async;
> -
>          if (nnmi_excep) {
>              env->pc = env->rnmi_excpvec;
>          } else {
> --
> 2.45.2
>
>

Re: [PATCH v4 9/9] target/riscv: Add Smdbltrp ISA extension enable switch

[Alistair Francis]

On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
>
> Add the switch to enable the Smdbltrp ISA extension.
>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 15b21e4f7d..1323effdae 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -183,6 +183,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(zhinxmin, PRIV_VERSION_1_12_0, ext_zhinxmin),
>      ISA_EXT_DATA_ENTRY(smaia, PRIV_VERSION_1_12_0, ext_smaia),
>      ISA_EXT_DATA_ENTRY(smcntrpmf, PRIV_VERSION_1_12_0, ext_smcntrpmf),
> +    ISA_EXT_DATA_ENTRY(smdbltrp, PRIV_VERSION_1_13_0, ext_smdbltrp),
>      ISA_EXT_DATA_ENTRY(smepmp, PRIV_VERSION_1_12_0, ext_smepmp),
>      ISA_EXT_DATA_ENTRY(smrnmi, PRIV_VERSION_1_12_0, ext_smrnmi),
>      ISA_EXT_DATA_ENTRY(smstateen, PRIV_VERSION_1_12_0, ext_smstateen),
> @@ -1506,6 +1507,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>
>      MULTI_EXT_CFG_BOOL("smaia", ext_smaia, false),
> +    MULTI_EXT_CFG_BOOL("smdbltrp", ext_smdbltrp, false),
>      MULTI_EXT_CFG_BOOL("smepmp", ext_smepmp, false),
>      MULTI_EXT_CFG_BOOL("smrnmi", ext_smrnmi, false),
>      MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> --
> 2.45.2
>
>

Re: [PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Clément Léger]

On 21/10/2024 02:46, Alistair Francis wrote:
> On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
>>
>> With the current implementation, if we had the current scenario:
>> - set bit x in menvcfg
>> - set bit x in henvcfg
>> - clear bit x in menvcfg
>> then, the internal variable env->henvcfg would still contain bit x due
>> to both a wrong menvcfg mask used in write_henvcfg() as well as a
>> missing update of henvcfg upon menvcfg update.
>> This can lead to some wrong interpretation of the context. In order to
>> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
>> menvcfg and fix the mask computation used in write_henvcfg() that is
>> used to mesk env->menvcfg value (which could still lead to some stale
>> bits). The same mechanism is also applied for henvcfgh writing.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>>  target/riscv/csr.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index b84b436151..9e832e0b39 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>>      return RISCV_EXCP_NONE;
>>  }
>>
>> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>> +                                    target_ulong val);
>>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>                                      target_ulong val)
>>  {
>> @@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>                  (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>      }
>>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
>>
>>      return RISCV_EXCP_NONE;
>>  }
>> @@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>>      return RISCV_EXCP_NONE;
>>  }
>>
>> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>> +                                    target_ulong val);
>>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>                                       target_ulong val)
>>  {
>> @@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>      uint64_t valh = (uint64_t)val << 32;
>>
>>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
>>
>>      return RISCV_EXCP_NONE;
>>  }
>> @@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>                                      target_ulong val)
>>  {
>>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
>> +    uint64_t menvcfg_mask = 0;
>>      RISCVException ret;
>>
>>      ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
>> @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>      }
>>
>>      if (riscv_cpu_mxl(env) == MXL_RV64) {
>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
>> +        mask |= env->menvcfg & menvcfg_mask;
> 
> This doesn't seem right.
> 
> Should it be something like

That is what I did before but that didn't work, henvcfg still contained
some stale bits.

> 
>     if (riscv_cpu_mxl(env) == MXL_RV64) {
>         mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>     }
> 
>     mask = env->menvcfg & mask;
> 
>>      }
>>
>> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>> +    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
> 
> Using both menvcfg_mask and mask seems wrong here

The problem is that if you use:

mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)

Then, if a bit was cleared in menvcfg before writing henvcfg (let's say
HENVCFG_ADUE), then env->henvcfg will be masked with mask =
HENVCFG_PBMTE | HENVCFG_STCE, leaving the HENVCFG_ADUE stale bit in
env->henvcfg which is wrong for the internal state.

The idea here is to actually clear any menvcfg related bit (the 1:1
bits) using the raw mask (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
to clear everything and then OR it with the value to be written (which
is masked with raw bits + menvcfg content) to avoid any stale bits.

Thanks,

Clément

> 
> Alistair

Re: [PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Alistair Francis]

On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 21/10/2024 02:46, Alistair Francis wrote:
> > On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
> >>
> >> With the current implementation, if we had the current scenario:
> >> - set bit x in menvcfg
> >> - set bit x in henvcfg
> >> - clear bit x in menvcfg
> >> then, the internal variable env->henvcfg would still contain bit x due
> >> to both a wrong menvcfg mask used in write_henvcfg() as well as a
> >> missing update of henvcfg upon menvcfg update.
> >> This can lead to some wrong interpretation of the context. In order to
> >> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
> >> menvcfg and fix the mask computation used in write_henvcfg() that is
> >> used to mesk env->menvcfg value (which could still lead to some stale
> >> bits). The same mechanism is also applied for henvcfgh writing.
> >>
> >> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >> ---
> >>  target/riscv/csr.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index b84b436151..9e832e0b39 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
> >>      return RISCV_EXCP_NONE;
> >>  }
> >>
> >> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >> +                                    target_ulong val);
> >>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >>                                      target_ulong val)
> >>  {
> >> @@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >>                  (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> >>      }
> >>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> >> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
> >>
> >>      return RISCV_EXCP_NONE;
> >>  }
> >> @@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
> >>      return RISCV_EXCP_NONE;
> >>  }
> >>
> >> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> >> +                                    target_ulong val);
> >>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> >>                                       target_ulong val)
> >>  {
> >> @@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> >>      uint64_t valh = (uint64_t)val << 32;
> >>
> >>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> >> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
> >>
> >>      return RISCV_EXCP_NONE;
> >>  }
> >> @@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >>                                      target_ulong val)
> >>  {
> >>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> >> +    uint64_t menvcfg_mask = 0;
> >>      RISCVException ret;
> >>
> >>      ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> >> @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >>      }
> >>
> >>      if (riscv_cpu_mxl(env) == MXL_RV64) {
> >> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> >> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
> >> +        mask |= env->menvcfg & menvcfg_mask;
> >
> > This doesn't seem right.
> >
> > Should it be something like
>
> That is what I did before but that didn't work, henvcfg still contained
> some stale bits.
>
> >
> >     if (riscv_cpu_mxl(env) == MXL_RV64) {
> >         mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> >     }
> >
> >     mask = env->menvcfg & mask;
> >
> >>      }
> >>
> >> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> >> +    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
> >
> > Using both menvcfg_mask and mask seems wrong here
>
> The problem is that if you use:
>
> mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
>
> Then, if a bit was cleared in menvcfg before writing henvcfg (let's say
> HENVCFG_ADUE), then env->henvcfg will be masked with mask =
> HENVCFG_PBMTE | HENVCFG_STCE, leaving the HENVCFG_ADUE stale bit in
> env->henvcfg which is wrong for the internal state.
>
> The idea here is to actually clear any menvcfg related bit (the 1:1
> bits) using the raw mask (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
> to clear everything and then OR it with the value to be written (which
> is masked with raw bits + menvcfg content) to avoid any stale bits.

When calling write_henvcfg() can't you just do:

write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)

I feel like that's clearer then the current approach

Alistair

>
> Thanks,
>
> Clément
>
> >
> > Alistair
>

Re: [PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Clément Léger]

On 23/10/2024 04:51, Alistair Francis wrote:
> On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> On 21/10/2024 02:46, Alistair Francis wrote:
>>> On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
>>>>
>>>> With the current implementation, if we had the current scenario:
>>>> - set bit x in menvcfg
>>>> - set bit x in henvcfg
>>>> - clear bit x in menvcfg
>>>> then, the internal variable env->henvcfg would still contain bit x due
>>>> to both a wrong menvcfg mask used in write_henvcfg() as well as a
>>>> missing update of henvcfg upon menvcfg update.
>>>> This can lead to some wrong interpretation of the context. In order to
>>>> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
>>>> menvcfg and fix the mask computation used in write_henvcfg() that is
>>>> used to mesk env->menvcfg value (which could still lead to some stale
>>>> bits). The same mechanism is also applied for henvcfgh writing.
>>>>
>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>> ---
>>>>  target/riscv/csr.c | 17 +++++++++++++----
>>>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index b84b436151..9e832e0b39 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>>>>      return RISCV_EXCP_NONE;
>>>>  }
>>>>
>>>> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>> +                                    target_ulong val);
>>>>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>>>                                      target_ulong val)
>>>>  {
>>>> @@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>>>                  (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>>>      }
>>>>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>>>> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
>>>>
>>>>      return RISCV_EXCP_NONE;
>>>>  }
>>>> @@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>>>>      return RISCV_EXCP_NONE;
>>>>  }
>>>>
>>>> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>>>> +                                    target_ulong val);
>>>>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>>>                                       target_ulong val)
>>>>  {
>>>> @@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>>>      uint64_t valh = (uint64_t)val << 32;
>>>>
>>>>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>>>> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
>>>>
>>>>      return RISCV_EXCP_NONE;
>>>>  }
>>>> @@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>>                                      target_ulong val)
>>>>  {
>>>>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
>>>> +    uint64_t menvcfg_mask = 0;
>>>>      RISCVException ret;
>>>>
>>>>      ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
>>>> @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>>      }
>>>>
>>>>      if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>>>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
>>>> +        mask |= env->menvcfg & menvcfg_mask;
>>>
>>> This doesn't seem right.
>>>
>>> Should it be something like
>>
>> That is what I did before but that didn't work, henvcfg still contained
>> some stale bits.
>>
>>>
>>>     if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>         mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>>>     }
>>>
>>>     mask = env->menvcfg & mask;
>>>
>>>>      }
>>>>
>>>> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>>>> +    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
>>>
>>> Using both menvcfg_mask and mask seems wrong here
>>
>> The problem is that if you use:
>>
>> mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
>>
>> Then, if a bit was cleared in menvcfg before writing henvcfg (let's say
>> HENVCFG_ADUE), then env->henvcfg will be masked with mask =
>> HENVCFG_PBMTE | HENVCFG_STCE, leaving the HENVCFG_ADUE stale bit in
>> env->henvcfg which is wrong for the internal state.
>>
>> The idea here is to actually clear any menvcfg related bit (the 1:1
>> bits) using the raw mask (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
>> to clear everything and then OR it with the value to be written (which
>> is masked with raw bits + menvcfg content) to avoid any stale bits.
> 
> When calling write_henvcfg() can't you just do:
> 
> write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)

Yeah it's clearer and I thought of that but you'll end up with the same
result since the problem does not come from the value you supply but
rather by the old masked henvcfg value used at the end of
write_henvcg()(and the mask is computed independently of the new value),
ie this line:

env->henvcfg = (env->henvcfg & ~mask) | (val & mask);

The mask is actually not containing the bits that were cleared in menvcfg.

We could actually use

write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)

But maybe we could call it prior to menvcfg update in order to clear all
currently set bit in menvcfg and mask env->henvcfg with the new value to
be written. That seems a bit off as well. But I'll take whatever you
think is clearer.

Clément

> 
> I feel like that's clearer then the current approach
> 
> Alistair
> 
>>
>> Thanks,
>>
>> Clément
>>
>>>
>>> Alistair
>>

Re: [PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Alistair Francis]

On Wed, Oct 30, 2024 at 6:57 PM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 23/10/2024 04:51, Alistair Francis wrote:
> > On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@rivosinc.com> wrote:
> >>
> >>
> >>
> >> On 21/10/2024 02:46, Alistair Francis wrote:
> >>> On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
> >>>>
> >>>> With the current implementation, if we had the current scenario:
> >>>> - set bit x in menvcfg
> >>>> - set bit x in henvcfg
> >>>> - clear bit x in menvcfg
> >>>> then, the internal variable env->henvcfg would still contain bit x due
> >>>> to both a wrong menvcfg mask used in write_henvcfg() as well as a
> >>>> missing update of henvcfg upon menvcfg update.
> >>>> This can lead to some wrong interpretation of the context. In order to
> >>>> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
> >>>> menvcfg and fix the mask computation used in write_henvcfg() that is
> >>>> used to mesk env->menvcfg value (which could still lead to some stale
> >>>> bits). The same mechanism is also applied for henvcfgh writing.
> >>>>
> >>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >>>> ---
> >>>>  target/riscv/csr.c | 17 +++++++++++++----
> >>>>  1 file changed, 13 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >>>> index b84b436151..9e832e0b39 100644
> >>>> --- a/target/riscv/csr.c
> >>>> +++ b/target/riscv/csr.c
> >>>> @@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
> >>>>      return RISCV_EXCP_NONE;
> >>>>  }
> >>>>
> >>>> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >>>> +                                    target_ulong val);
> >>>>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >>>>                                      target_ulong val)
> >>>>  {
> >>>> @@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
> >>>>                  (cfg->ext_svadu ? MENVCFG_ADUE : 0);
> >>>>      }
> >>>>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
> >>>> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
> >>>>
> >>>>      return RISCV_EXCP_NONE;
> >>>>  }
> >>>> @@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
> >>>>      return RISCV_EXCP_NONE;
> >>>>  }
> >>>>
> >>>> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
> >>>> +                                    target_ulong val);
> >>>>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> >>>>                                       target_ulong val)
> >>>>  {
> >>>> @@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
> >>>>      uint64_t valh = (uint64_t)val << 32;
> >>>>
> >>>>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
> >>>> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
> >>>>
> >>>>      return RISCV_EXCP_NONE;
> >>>>  }
> >>>> @@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >>>>                                      target_ulong val)
> >>>>  {
> >>>>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
> >>>> +    uint64_t menvcfg_mask = 0;
> >>>>      RISCVException ret;
> >>>>
> >>>>      ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
> >>>> @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
> >>>>      }
> >>>>
> >>>>      if (riscv_cpu_mxl(env) == MXL_RV64) {
> >>>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> >>>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
> >>>> +        mask |= env->menvcfg & menvcfg_mask;
> >>>
> >>> This doesn't seem right.
> >>>
> >>> Should it be something like
> >>
> >> That is what I did before but that didn't work, henvcfg still contained
> >> some stale bits.
> >>
> >>>
> >>>     if (riscv_cpu_mxl(env) == MXL_RV64) {
> >>>         mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
> >>>     }
> >>>
> >>>     mask = env->menvcfg & mask;
> >>>
> >>>>      }
> >>>>
> >>>> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> >>>> +    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
> >>>
> >>> Using both menvcfg_mask and mask seems wrong here
> >>
> >> The problem is that if you use:
> >>
> >> mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
> >>
> >> Then, if a bit was cleared in menvcfg before writing henvcfg (let's say
> >> HENVCFG_ADUE), then env->henvcfg will be masked with mask =
> >> HENVCFG_PBMTE | HENVCFG_STCE, leaving the HENVCFG_ADUE stale bit in
> >> env->henvcfg which is wrong for the internal state.
> >>
> >> The idea here is to actually clear any menvcfg related bit (the 1:1
> >> bits) using the raw mask (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
> >> to clear everything and then OR it with the value to be written (which
> >> is masked with raw bits + menvcfg content) to avoid any stale bits.
> >
> > When calling write_henvcfg() can't you just do:
> >
> > write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)
>
> Yeah it's clearer and I thought of that but you'll end up with the same
> result since the problem does not come from the value you supply but
> rather by the old masked henvcfg value used at the end of
> write_henvcg()(and the mask is computed independently of the new value),
> ie this line:
>
> env->henvcfg = (env->henvcfg & ~mask) | (val & mask);

Yeah ok.

Maybe a comment or two to describe what is going on would be enough
then, or even splitting the single line ops into multiple lines would
be clearer

Alistair

Re: [PATCH v4 1/9] target/riscv: fix henvcfg potentially containing stale bits

[Clément Léger]

On 06/11/2024 01:16, Alistair Francis wrote:
> On Wed, Oct 30, 2024 at 6:57 PM Clément Léger <cleger@rivosinc.com> wrote:
>>
>>
>>
>> On 23/10/2024 04:51, Alistair Francis wrote:
>>> On Mon, Oct 21, 2024 at 7:30 PM Clément Léger <cleger@rivosinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 21/10/2024 02:46, Alistair Francis wrote:
>>>>> On Fri, Oct 18, 2024 at 12:55 AM Clément Léger <cleger@rivosinc.com> wrote:
>>>>>>
>>>>>> With the current implementation, if we had the current scenario:
>>>>>> - set bit x in menvcfg
>>>>>> - set bit x in henvcfg
>>>>>> - clear bit x in menvcfg
>>>>>> then, the internal variable env->henvcfg would still contain bit x due
>>>>>> to both a wrong menvcfg mask used in write_henvcfg() as well as a
>>>>>> missing update of henvcfg upon menvcfg update.
>>>>>> This can lead to some wrong interpretation of the context. In order to
>>>>>> update henvcfg upon menvcfg writing, call write_henvcfg() after writing
>>>>>> menvcfg and fix the mask computation used in write_henvcfg() that is
>>>>>> used to mesk env->menvcfg value (which could still lead to some stale
>>>>>> bits). The same mechanism is also applied for henvcfgh writing.
>>>>>>
>>>>>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>>>>> ---
>>>>>>  target/riscv/csr.c | 17 +++++++++++++----
>>>>>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>>> index b84b436151..9e832e0b39 100644
>>>>>> --- a/target/riscv/csr.c
>>>>>> +++ b/target/riscv/csr.c
>>>>>> @@ -2345,6 +2345,8 @@ static RISCVException read_menvcfg(CPURISCVState *env, int csrno,
>>>>>>      return RISCV_EXCP_NONE;
>>>>>>  }
>>>>>>
>>>>>> +static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>>>> +                                    target_ulong val);
>>>>>>  static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>>>>>                                      target_ulong val)
>>>>>>  {
>>>>>> @@ -2357,6 +2359,7 @@ static RISCVException write_menvcfg(CPURISCVState *env, int csrno,
>>>>>>                  (cfg->ext_svadu ? MENVCFG_ADUE : 0);
>>>>>>      }
>>>>>>      env->menvcfg = (env->menvcfg & ~mask) | (val & mask);
>>>>>> +    write_henvcfg(env, CSR_HENVCFG, env->henvcfg);
>>>>>>
>>>>>>      return RISCV_EXCP_NONE;
>>>>>>  }
>>>>>> @@ -2368,6 +2371,8 @@ static RISCVException read_menvcfgh(CPURISCVState *env, int csrno,
>>>>>>      return RISCV_EXCP_NONE;
>>>>>>  }
>>>>>>
>>>>>> +static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>>>>>> +                                    target_ulong val);
>>>>>>  static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>>>>>                                       target_ulong val)
>>>>>>  {
>>>>>> @@ -2378,6 +2383,7 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
>>>>>>      uint64_t valh = (uint64_t)val << 32;
>>>>>>
>>>>>>      env->menvcfg = (env->menvcfg & ~mask) | (valh & mask);
>>>>>> +    write_henvcfgh(env, CSR_HENVCFGH, env->henvcfg >> 32);
>>>>>>
>>>>>>      return RISCV_EXCP_NONE;
>>>>>>  }
>>>>>> @@ -2435,6 +2441,7 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>>>>                                      target_ulong val)
>>>>>>  {
>>>>>>      uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
>>>>>> +    uint64_t menvcfg_mask = 0;
>>>>>>      RISCVException ret;
>>>>>>
>>>>>>      ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
>>>>>> @@ -2443,10 +2450,11 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
>>>>>>      }
>>>>>>
>>>>>>      if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>>>> -        mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>>>>>> +        menvcfg_mask = HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE;
>>>>>> +        mask |= env->menvcfg & menvcfg_mask;
>>>>>
>>>>> This doesn't seem right.
>>>>>
>>>>> Should it be something like
>>>>
>>>> That is what I did before but that didn't work, henvcfg still contained
>>>> some stale bits.
>>>>
>>>>>
>>>>>     if (riscv_cpu_mxl(env) == MXL_RV64) {
>>>>>         mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE);
>>>>>     }
>>>>>
>>>>>     mask = env->menvcfg & mask;
>>>>>
>>>>>>      }
>>>>>>
>>>>>> -    env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
>>>>>> +    env->henvcfg = (env->henvcfg & ~menvcfg_mask) | (val & mask);
>>>>>
>>>>> Using both menvcfg_mask and mask seems wrong here
>>>>
>>>> The problem is that if you use:
>>>>
>>>> mask |= env->menvcfg & (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
>>>>
>>>> Then, if a bit was cleared in menvcfg before writing henvcfg (let's say
>>>> HENVCFG_ADUE), then env->henvcfg will be masked with mask =
>>>> HENVCFG_PBMTE | HENVCFG_STCE, leaving the HENVCFG_ADUE stale bit in
>>>> env->henvcfg which is wrong for the internal state.
>>>>
>>>> The idea here is to actually clear any menvcfg related bit (the 1:1
>>>> bits) using the raw mask (HENVCFG_PBMTE | HENVCFG_STCE | HENVCFG_ADUE)
>>>> to clear everything and then OR it with the value to be written (which
>>>> is masked with raw bits + menvcfg content) to avoid any stale bits.
>>>
>>> When calling write_henvcfg() can't you just do:
>>>
>>> write_henvcfg(env, CSR_HENVCFG, env->henvcfg & env->menvcfg)
>>
>> Yeah it's clearer and I thought of that but you'll end up with the same
>> result since the problem does not come from the value you supply but
>> rather by the old masked henvcfg value used at the end of
>> write_henvcg()(and the mask is computed independently of the new value),
>> ie this line:
>>
>> env->henvcfg = (env->henvcfg & ~mask) | (val & mask);
> 
> Yeah ok.
> 
> Maybe a comment or two to describe what is going on would be enough
> then, or even splitting the single line ops into multiple lines would
> be clearer

Yep, that could be enough. I'll do that,

Thanks !

> 
> Alistair