Skip to contentSkip to conversation

RISC-V Smstateen support

PATCH Superseded Mayuresh Chitale 2 participants 12 messages Oct 3, 2022 – Oct 10, 2022 RISC-V

v10 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Mayuresh Chitale

author
#0 lore raw

v10 cover RISC-V Smstateen support

This series adds support for the Smstateen specification which provides a mechanism to plug the potential covert channels which are opened by extensions that add to processor state that may not get context-switched. Currently access to *envcfg registers and floating point(fcsr) is controlled via smstateen.

These patches can also be found on riscv_smstateen_v10 branch at:
https://github.com/mdchitale/qemu.git

Changes in v10:
- Add support to generate virt instruction exception after decode failure. Use this change for smstateen fcsr failure when virt is enabled.
- Implement single write function for *smstateen1 to *smstateen3 registers.

Changes in v9:
- Rebase to latest riscv-to-apply.next
- Add reviewed by in patches 2 and 4

Changes in v8:
- Rebase to latest riscv-to-apply.next
- Fix m-mode check for hstateen
- Fix return exception type for VU mode
- Improve commit description for patch3

Changes in v7:
- Update smstateen check as per discussion on the following issue:
https://github.com/riscv/riscv-state-enable/issues/9
- Drop the smstateen AIA patch for now.
- Indentation and other fixes

Changes in v6:
- Sync with latest riscv-to-apply.next
- Make separate read/write ops for m/h/s/stateen1/2/3 regs
- Add check for mstateen.staten when reading or using h/s/stateen regs
- Add smstateen fcsr check for all floating point operations
- Move knobs to enable smstateen in a separate patch.

Changes in v5:
- Fix the order in which smstateen extension is added to the isa_edata_arr as
described in rule #3 the comment.

Changes in v4:
- Fix build issue with riscv32/riscv64-linux-user targets

Changes in v3:
- Fix coding style issues
- Fix *stateen0h index calculation

Changes in v2:
- Make h/s/envcfg bits in m/h/stateen registers as writeable by default.

Mayuresh Chitale (4):
target/riscv: Add smstateen support target/riscv: smstateen check for h/s/envcfg target/riscv: generate virtual instruction exception target/riscv: smstateen check for fcsr target/riscv: smstateen knobs

 target/riscv/cpu.c                        |   2 +
 target/riscv/cpu.h                        |   4 +
 target/riscv/cpu_bits.h                   |  37 ++
 target/riscv/csr.c                        | 414 +++++++++++++++++++++-
 target/riscv/insn_trans/trans_rvf.c.inc   |  43 ++-
 target/riscv/insn_trans/trans_rvzfh.c.inc |  12 +
 target/riscv/machine.c                    |  21 ++
 target/riscv/translate.c                  |   9 +-
 8 files changed, 537 insertions(+), 5 deletions(-)
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH v10 1/5 target/riscv: Add smstateen support

Smstateen extension specifies a mechanism to close the potential covert channels that could cause security issues.

This patch adds the CSRs defined in the specification and the corresponding predicates and read/write functions.

Signed-off-by Mayuresh Chitale <[email protected]>
Reviewed-by Weiwei Li <[email protected]>
---
 target/riscv/cpu.h      |   4 +
 target/riscv/cpu_bits.h |  37 +++++
 target/riscv/csr.c      | 316 ++++++++++++++++++++++++++++++++++++++++
 target/riscv/machine.c  |  21 +++
 4 files changed, 378 insertions(+)
target/riscv/cpu.h +4 -0
index b131fa8c8e..e02e0bd7df 100644
@@ -366,6 +366,9 @@ struct CPUArchState {
 
     /* CSRs for execution enviornment configuration */
     uint64_t menvcfg;
+    uint64_t mstateen[SMSTATEEN_MAX_COUNT];
+    uint64_t hstateen[SMSTATEEN_MAX_COUNT];
+    uint64_t sstateen[SMSTATEEN_MAX_COUNT];
     target_ulong senvcfg;
     uint64_t henvcfg;
 #endif

@@ -441,6 +444,7 @@ struct RISCVCPUConfig {
     bool ext_ifencei;
     bool ext_icsr;
     bool ext_zihintpause;
+    bool ext_smstateen;
     bool ext_sstc;
     bool ext_svinval;
     bool ext_svnapot;
target/riscv/cpu_bits.h +37 -0
index d8f5f0abed..8b0d7e20ea 100644
@@ -197,6 +197,12 @@
 /* Supervisor Configuration CSRs */
 #define CSR_SENVCFG         0x10A
 
+/* Supervisor state CSRs */
+#define CSR_SSTATEEN0       0x10C
+#define CSR_SSTATEEN1       0x10D
+#define CSR_SSTATEEN2       0x10E
+#define CSR_SSTATEEN3       0x10F
+
 /* Supervisor Trap Handling */
 #define CSR_SSCRATCH        0x140
 #define CSR_SEPC            0x141

@@ -244,6 +250,16 @@
 #define CSR_HENVCFG         0x60A
 #define CSR_HENVCFGH        0x61A
 
+/* Hypervisor state CSRs */
+#define CSR_HSTATEEN0       0x60C
+#define CSR_HSTATEEN0H      0x61C
+#define CSR_HSTATEEN1       0x60D
+#define CSR_HSTATEEN1H      0x61D
+#define CSR_HSTATEEN2       0x60E
+#define CSR_HSTATEEN2H      0x61E
+#define CSR_HSTATEEN3       0x60F
+#define CSR_HSTATEEN3H      0x61F
+
 /* Virtual CSRs */
 #define CSR_VSSTATUS        0x200
 #define CSR_VSIE            0x204

@@ -289,6 +305,27 @@
 #define CSR_MENVCFG         0x30A
 #define CSR_MENVCFGH        0x31A
 
+/* Machine state CSRs */
+#define CSR_MSTATEEN0       0x30C
+#define CSR_MSTATEEN0H      0x31C
+#define CSR_MSTATEEN1       0x30D
+#define CSR_MSTATEEN1H      0x31D
+#define CSR_MSTATEEN2       0x30E
+#define CSR_MSTATEEN2H      0x31E
+#define CSR_MSTATEEN3       0x30F
+#define CSR_MSTATEEN3H      0x31F
+
+/* Common defines for all smstateen */
+#define SMSTATEEN_MAX_COUNT 4
+#define SMSTATEEN0_CS       (1ULL << 0)
+#define SMSTATEEN0_FCSR     (1ULL << 1)
+#define SMSTATEEN0_HSCONTXT (1ULL << 57)
+#define SMSTATEEN0_IMSIC    (1ULL << 58)
+#define SMSTATEEN0_AIA      (1ULL << 59)
+#define SMSTATEEN0_SVSLCT   (1ULL << 60)
+#define SMSTATEEN0_HSENVCFG (1ULL << 62)
+#define SMSTATEEN_STATEEN   (1ULL << 63)
+
 /* Enhanced Physical Memory Protection (ePMP) */
 #define CSR_MSECCFG         0x747
 #define CSR_MSECCFGH        0x757
target/riscv/csr.c +316 -0

334 lines not on this page · raw message

target/riscv/machine.c +21 -0
index c2a94a82b3..e687f9fce0 100644
@@ -253,6 +253,26 @@ static int riscv_cpu_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool smstateen_needed(void *opaque)
+{
+    RISCVCPU *cpu = opaque;
+
+    return cpu->cfg.ext_smstateen;
+}
+
+static const VMStateDescription vmstate_smstateen = {
+    .name = "cpu/smtateen",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = smstateen_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64_ARRAY(env.mstateen, RISCVCPU, 4),
+        VMSTATE_UINT64_ARRAY(env.hstateen, RISCVCPU, 4),
+        VMSTATE_UINT64_ARRAY(env.sstateen, RISCVCPU, 4),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool envcfg_needed(void *opaque)
 {
     RISCVCPU *cpu = opaque;

@@ -364,6 +384,7 @@ const VMStateDescription vmstate_riscv_cpu = {
         &vmstate_kvmtimer,
         &vmstate_envcfg,
         &vmstate_debug,
+        &vmstate_smstateen,
         NULL
     }
 };
Signature · 1 line
2.25.1
author
#2 lore raw

PATCH v10 2/5 target/riscv: smstateen check for h/s/envcfg

In reply to Mayuresh Chitale

Accesses to henvcfg, henvcfgh and senvcfg are allowed only if the corresponding bit in mstateen0/hstateen0 is enabled. Otherwise an illegal instruction trap is generated.

Signed-off-by Mayuresh Chitale <[email protected]>
Reviewed-by Weiwei Li<[email protected]>
Reviewed-by Alistair Francis <[email protected]>
---
 target/riscv/csr.c | 87 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 7 deletions(-)
target/riscv/csr.c +80 -7
index c861424e85..71236f2b5d 100644
@@ -41,6 +41,42 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops)
 }
 
 /* Predicates */
+#if !defined(CONFIG_USER_ONLY)
+static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
+                                       uint64_t bit)
+{
+    bool virt = riscv_cpu_virt_enabled(env);
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
+
+    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_NONE;
+    }
+
+    if (!(env->mstateen[index] & bit)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (virt) {
+        if (!(env->hstateen[index] & bit)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+
+        if (env->priv == PRV_U && !(env->sstateen[index] & bit)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+    }
+
+    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
+        if (!(env->sstateen[index] & bit)) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+    }
+
+    return RISCV_EXCP_NONE;
+}
+#endif
+
 static RISCVException fs(CPURISCVState *env, int csrno)
 {
 #if !defined(CONFIG_USER_ONLY)

@@ -1874,6 +1910,13 @@ static RISCVException write_menvcfgh(CPURISCVState *env, int csrno,
 static RISCVException read_senvcfg(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->senvcfg;
     return RISCV_EXCP_NONE;
 }

@@ -1882,15 +1925,27 @@ static RISCVException write_senvcfg(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
     uint64_t mask = SENVCFG_FIOM | SENVCFG_CBIE | SENVCFG_CBCFE | SENVCFG_CBZE;
+    RISCVException ret;
 
-    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
+    env->senvcfg = (env->senvcfg & ~mask) | (val & mask);
     return RISCV_EXCP_NONE;
 }
 
 static RISCVException read_henvcfg(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->henvcfg;
     return RISCV_EXCP_NONE;
 }

@@ -1899,6 +1954,12 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
     uint64_t mask = HENVCFG_FIOM | HENVCFG_CBIE | HENVCFG_CBCFE | HENVCFG_CBZE;
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
     if (riscv_cpu_mxl(env) == MXL_RV64) {
         mask |= HENVCFG_PBMTE | HENVCFG_STCE;

@@ -1912,6 +1973,13 @@ static RISCVException write_henvcfg(CPURISCVState *env, int csrno,
 static RISCVException read_henvcfgh(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->henvcfg >> 32;
     return RISCV_EXCP_NONE;
 }

@@ -1921,9 +1989,14 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
 {
     uint64_t mask = HENVCFG_PBMTE | HENVCFG_STCE;
     uint64_t valh = (uint64_t)val << 32;
+    RISCVException ret;
 
-    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_HSENVCFG);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
+    env->henvcfg = (env->henvcfg & ~mask) | (valh & mask);
     return RISCV_EXCP_NONE;
 }
 

@@ -1949,7 +2022,7 @@ static RISCVException write_mstateen(CPURISCVState *env, int csrno,
 static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
-    uint64_t wr_mask = SMSTATEEN_STATEEN;
+    uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
     return write_mstateen(env, csrno, wr_mask, new_val);
 }

@@ -1984,7 +2057,7 @@ static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
 static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
-    uint64_t wr_mask = SMSTATEEN_STATEEN;
+    uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
     return write_mstateenh(env, csrno, wr_mask, new_val);
 }

@@ -2021,7 +2094,7 @@ static RISCVException write_hstateen(CPURISCVState *env, int csrno,
 static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
-    uint64_t wr_mask = SMSTATEEN_STATEEN;
+    uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
     return write_hstateen(env, csrno, wr_mask, new_val);
 }

@@ -2060,7 +2133,7 @@ static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
 static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
                                        target_ulong new_val)
 {
-    uint64_t wr_mask = SMSTATEEN_STATEEN;
+    uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
     return write_hstateenh(env, csrno, wr_mask, new_val);
 }

@@ -2107,7 +2180,7 @@ static RISCVException write_sstateen(CPURISCVState *env, int csrno,
 static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
-    uint64_t wr_mask = SMSTATEEN_STATEEN;
+    uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
     return write_sstateen(env, csrno, wr_mask, new_val);
 }
Signature · 1 line
2.25.1
author
#3 lore raw

PATCH v10 3/5 target/riscv: generate virtual instruction exception

In reply to Mayuresh Chitale

This patch adds a mechanism to generate a virtual instruction instruction exception instead of an illegal instruction exception during instruction decode when virt is enabled.

Signed-off-by Mayuresh Chitale <[email protected]>
---
 target/riscv/translate.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
target/riscv/translate.c +8 -1
index db123da5ec..6926b639de 100644
@@ -76,6 +76,7 @@ typedef struct DisasContext {
        to reset this known value.  */
     int frm;
     RISCVMXL ol;
+    bool virt_inst_excp;
     bool virt_enabled;
     const RISCVCPUConfig *cfg_ptr;
     bool hlsx;

@@ -243,7 +244,11 @@ static void gen_exception_illegal(DisasContext *ctx)
 {
     tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
                    offsetof(CPURISCVState, bins));
-    generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
+    if (ctx->virt_inst_excp) {
+        generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT);
+    } else {
+        generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
+    }
 }
 
 static void gen_exception_inst_addr_mis(DisasContext *ctx)

@@ -1067,6 +1072,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
         if (!has_ext(ctx, RVC)) {
             gen_exception_illegal(ctx);
         } else {
+            ctx->virt_inst_excp = false;
             ctx->opcode = opcode;
             ctx->pc_succ_insn = ctx->base.pc_next + 2;
             if (decode_insn16(ctx, opcode)) {

@@ -1078,6 +1084,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
         opcode32 = deposit32(opcode32, 16, 16,
                              translator_lduw(env, &ctx->base,
                                              ctx->base.pc_next + 2));
+        ctx->virt_inst_excp = false;
         ctx->opcode = opcode32;
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
Signature · 1 line
2.25.1
#4 lore raw

Re: [PATCH v10 3/5] target/riscv: generate virtual instruction exception

Quoted 35 lines · Mayuresh Chitale

This patch adds a mechanism to generate a virtual instruction instruction exception instead of an illegal instruction exception during instruction decode when virt is enabled.

Signed-off-by Mayuresh Chitale <[email protected]>
---
  target/riscv/translate.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)
target/riscv/translate.c +6 -1
index db123da5ec..6926b639de 100644
@@ -76,6 +76,7 @@ typedef struct DisasContext {
         to reset this known value.  */
      int frm;
      RISCVMXL ol;
+    bool virt_inst_excp;
      bool virt_enabled;
      const RISCVCPUConfig *cfg_ptr;
      bool hlsx;

@@ -243,7 +244,11 @@ static void gen_exception_illegal(DisasContext *ctx)
  {
      tcg_gen_st_i32(tcg_constant_i32(ctx->opcode), cpu_env,
                     offsetof(CPURISCVState, bins));
-    generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
+    if (ctx->virt_inst_excp) {
+        generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT);
+    } else {
+        generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
+    }
  }
 
  static void gen_exception_inst_addr_mis(DisasContext *ctx)

target/riscv/translate.c static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode) · line 1073

Quoted 2 lines
target/riscv/translate.c +0 -0
@@ -1067,6 +1072,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
          if (!has_ext(ctx, RVC)) {
              gen_exception_illegal(ctx);

I think we should also set virt_inst_excp to false before this gen_exception_illegal.

By the way, why not just put  "ctx->virt_inst_excp = false;" at the begin of the decode_opc?

Regards,

Weiwei Li

Quoted 13 lines
target/riscv/translate.c +2 -0
          } else {
+            ctx->virt_inst_excp = false;
              ctx->opcode = opcode;
              ctx->pc_succ_insn = ctx->base.pc_next + 2;
              if (decode_insn16(ctx, opcode)) {

@@ -1078,6 +1084,7 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
          opcode32 = deposit32(opcode32, 16, 16,
                               translator_lduw(env, &ctx->base,
                                               ctx->base.pc_next + 2));
+        ctx->virt_inst_excp = false;
          ctx->opcode = opcode32;
          ctx->pc_succ_insn = ctx->base.pc_next + 4;
author
#5 lore raw

PATCH v10 4/5 target/riscv: smstateen check for fcsr

In reply to Mayuresh Chitale

If smstateen is implemented and sstateen0.fcsr is clear then the floating point operations must return illegal instruction exception or virtual instruction trap, if relevant.

Signed-off-by Mayuresh Chitale <[email protected]>
---
 target/riscv/csr.c                        | 23 ++++++++++++
 target/riscv/insn_trans/trans_rvf.c.inc   | 43 +++++++++++++++++++++--
 target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++
 3 files changed, 75 insertions(+), 3 deletions(-)
target/riscv/csr.c +23 -0
index 71236f2b5d..8b25f885ec 100644
@@ -84,6 +84,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
         !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
+    }
 #endif
     return RISCV_EXCP_NONE;
 }

@@ -2023,6 +2027,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
     return write_mstateen(env, csrno, wr_mask, new_val);
 }

@@ -2059,6 +2066,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_mstateenh(env, csrno, wr_mask, new_val);
 }
 

@@ -2096,6 +2107,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateen(env, csrno, wr_mask, new_val);
 }
 

@@ -2135,6 +2150,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateenh(env, csrno, wr_mask, new_val);
 }
 

@@ -2182,6 +2201,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_sstateen(env, csrno, wr_mask, new_val);
 }
target/riscv/insn_trans/trans_rvf.c.inc +40 -3

49 lines not on this page · raw message

target/riscv/insn_trans/trans_rvzfh.c.inc +12 -0
index 5d07150cd0..6c2e338c0a 100644
@@ -20,18 +20,27 @@
     if (!ctx->cfg_ptr->ext_zfh) {      \
         return false;         \
     }                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
 } while (0)
 
 #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
     if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
         return false;                  \
     }                                  \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
 } while (0)
 
 #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
     if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
         return false;                         \
     }                                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
 } while (0)
 
 #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \

@@ -39,6 +48,9 @@
           ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin)) {     \
         return false;                                        \
     }                                                        \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
 } while (0)
 
 static bool trans_flh(DisasContext *ctx, arg_flh *a)
Signature · 1 line
2.25.1
#6 lore raw

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

Quoted 104 lines · Mayuresh Chitale

If smstateen is implemented and sstateen0.fcsr is clear then the floating point operations must return illegal instruction exception or virtual instruction trap, if relevant.

Signed-off-by Mayuresh Chitale <[email protected]>
---
  target/riscv/csr.c                        | 23 ++++++++++++
  target/riscv/insn_trans/trans_rvf.c.inc   | 43 +++++++++++++++++++++--
  target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++
  3 files changed, 75 insertions(+), 3 deletions(-)
target/riscv/csr.c +23 -0
index 71236f2b5d..8b25f885ec 100644
@@ -84,6 +84,10 @@ static RISCVException fs(CPURISCVState *env, int csrno)
          !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
          return RISCV_EXCP_ILLEGAL_INST;
      }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
+    }
  #endif
      return RISCV_EXCP_NONE;
  }

@@ -2023,6 +2027,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
                                        target_ulong new_val)
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
      return write_mstateen(env, csrno, wr_mask, new_val);
  }

@@ -2059,6 +2066,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_mstateenh(env, csrno, wr_mask, new_val);
  }
 

@@ -2096,6 +2107,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_hstateen(env, csrno, wr_mask, new_val);
  }
 

@@ -2135,6 +2150,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_hstateenh(env, csrno, wr_mask, new_val);
  }
 

@@ -2182,6 +2201,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_sstateen(env, csrno, wr_mask, new_val);
  }
target/riscv/insn_trans/trans_rvf.c.inc +13 -3
index a1d3eb52ad..93657680c6 100644
@@ -24,9 +24,46 @@
              return false; \
  } while (0)
 
-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-    if (!ctx->cfg_ptr->ext_zfinx) { \
-        REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int index)
+{
+    CPUState *cpu = ctx->cs;
+    CPURISCVState *env = cpu->env_ptr;
+    uint64_t stateen = env->mstateen[index];
+
+    if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+        return true;
+    }
+
+    if (ctx->virt_enabled) {
+        stateen &= env->hstateen[index];

target/riscv/insn_trans/trans_rvf.c.inc line 49

Quoted 10 lines
target/riscv/insn_trans/trans_rvf.c.inc +10 -0
+    }
+
+    if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+        stateen &= env->sstateen[index];
+    }
+
+    if (!(stateen & SMSTATEEN0_FCSR)) {
+        if (ctx->virt_enabled) {
+            ctx->virt_inst_excp = true;
+        }

Are there any considerations for adding  virt_inst_excp instead of directly

"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?

Regards,

Weiwei Li

Quoted 62 lines
target/riscv/insn_trans/trans_rvf.c.inc +17 -0
+        return false;
+    }
+
+    return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+    if (!has_ext(ctx, RVF)) { \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
+        if (!smstateen_fcsr_check(ctx, 0)) { \
+            return false; \
+        } \
      } \
  } while (0)
target/riscv/insn_trans/trans_rvzfh.c.inc +12 -0
index 5d07150cd0..6c2e338c0a 100644
@@ -20,18 +20,27 @@
      if (!ctx->cfg_ptr->ext_zfh) {      \
          return false;         \
      }                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
      if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
          return false;                  \
      }                                  \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
      if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
          return false;                         \
      }                                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \

@@ -39,6 +48,9 @@
            ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin)) {     \
          return false;                                        \
      }                                                        \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  static bool trans_flh(DisasContext *ctx, arg_flh *a)
author
#7 lore raw

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

Quoted 130 lines · weiwei
Quoted 115 lines · Mayuresh Chitale

If smstateen is implemented and sstateen0.fcsr is clear then the floating point
operations must return illegal instruction exception or virtual instruction
trap, if relevant.

Signed-off-by Mayuresh Chitale <[email protected]>
---
  target/riscv/csr.c                        | 23 ++++++++++++
  target/riscv/insn_trans/trans_rvf.c.inc   | 43
+++++++++++++++++++++--
  target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++
  3 files changed, 75 insertions(+), 3 deletions(-)
target/riscv/csr.c +4 -0
index 71236f2b5d..8b25f885ec 100644
@@ -84,6 +84,10 @@ static RISCVException fs(CPURISCVState *env, int
 csrno)
          !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
          return RISCV_EXCP_ILLEGAL_INST;
      }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
+    }
  #endif
      return RISCV_EXCP_NONE;

  }
target/riscv/csr.c +3 -0
@@ -2023,6 +2027,9 @@ static RISCVException
 write_mstateen0(CPURISCVState *env, int csrno,
                                        target_ulong new_val)
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
      return write_mstateen(env, csrno, wr_mask, new_val);

  }
target/riscv/csr.c +16 -0
@@ -2059,6 +2066,10 @@ static RISCVException
 write_mstateen0h(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_mstateenh(env, csrno, wr_mask, new_val);
  }

@@ -2096,6 +2107,10 @@ static RISCVException
 write_hstateen0(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_hstateen(env, csrno, wr_mask, new_val);
  }

@@ -2135,6 +2150,10 @@ static RISCVException
 write_hstateen0h(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_hstateenh(env, csrno, wr_mask, new_val);
  }

@@ -2182,6 +2201,10 @@ static RISCVException
 write_sstateen0(CPURISCVState *env, int csrno,
  {
      uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
      return write_sstateen(env, csrno, wr_mask, new_val);
  }
target/riscv/insn_trans/trans_rvf.c.inc +13 -3
index a1d3eb52ad..93657680c6 100644
@@ -24,9 +24,46 @@
              return false; \
  } while (0)
 
-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-    if (!ctx->cfg_ptr->ext_zfinx) { \
-        REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int
 index)
+{
+    CPUState *cpu = ctx->cs;
+    CPURISCVState *env = cpu->env_ptr;
+    uint64_t stateen = env->mstateen[index];
+
+    if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+        return true;
+    }
+
+    if (ctx->virt_enabled) {
+        stateen &= env->hstateen[index];

target/riscv/insn_trans/trans_rvf.c.inc line 50

Quoted 10 lines
target/riscv/insn_trans/trans_rvf.c.inc +10 -0
+    }
+
+    if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+        stateen &= env->sstateen[index];
+    }
+
+    if (!(stateen & SMSTATEEN0_FCSR)) {
+        if (ctx->virt_enabled) {
+            ctx->virt_inst_excp = true;
+        }

Are there any considerations for adding virt_inst_excp instead of directly

Quoted 5 lines

"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?

Regards,

Weiwei Li

I had considered it but I think this is a simpler approach as the rest of the code path stays the same as generating an illegal instruction exception, for e.g setting the bins value (tval). Also we need to return true from all the caller trans_* functions even if the smstateen check has failed.

Quoted 65 lines
Quoted 65 lines
target/riscv/insn_trans/trans_rvf.c.inc +17 -0
+        return false;
+    }
+
+    return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+    if (!has_ext(ctx, RVF)) { \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
+        if (!smstateen_fcsr_check(ctx, 0)) { \
+            return false; \
+        } \
      } \
  } while (0)
target/riscv/insn_trans/trans_rvzfh.c.inc +12 -0
index 5d07150cd0..6c2e338c0a 100644
@@ -20,18 +20,27 @@
      if (!ctx->cfg_ptr->ext_zfh) {      \
          return false;         \
      }                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
      if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
          return false;                  \
      }                                  \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
      if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
          return false;                         \
      }                                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)
 
  #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \

@@ -39,6 +48,9 @@
            ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin)) 
 {     \
          return false;                                        \
      }                                                        \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
  } while (0)

static bool trans_flh(DisasContext *ctx, arg_flh *a)

#8 lore raw

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

Quoted 130 lines · On 2022/10/4 14:51, [email protected] wrote:
Quoted 129 lines · weiwei
Quoted 115 lines · Mayuresh Chitale

If smstateen is implemented and sstateen0.fcsr is clear then the floating point
operations must return illegal instruction exception or virtual instruction
trap, if relevant.

Signed-off-by Mayuresh Chitale<[email protected]>
---
   target/riscv/csr.c                        | 23 ++++++++++++
   target/riscv/insn_trans/trans_rvf.c.inc   | 43
+++++++++++++++++++++--
   target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++
   3 files changed, 75 insertions(+), 3 deletions(-)
target/riscv/csr.c +4 -0
index 71236f2b5d..8b25f885ec 100644
@@ -84,6 +84,10 @@ static RISCVException fs(CPURISCVState *env, int
 csrno)
           !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
           return RISCV_EXCP_ILLEGAL_INST;
       }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
+    }
   #endif
       return RISCV_EXCP_NONE;

   }
target/riscv/csr.c +3 -0
@@ -2023,6 +2027,9 @@ static RISCVException
 write_mstateen0(CPURISCVState *env, int csrno,
                                         target_ulong new_val)
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
       return write_mstateen(env, csrno, wr_mask, new_val);

   }
target/riscv/csr.c +16 -0
@@ -2059,6 +2066,10 @@ static RISCVException
 write_mstateen0h(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_mstateenh(env, csrno, wr_mask, new_val);
   }

@@ -2096,6 +2107,10 @@ static RISCVException
 write_hstateen0(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_hstateen(env, csrno, wr_mask, new_val);
   }

@@ -2135,6 +2150,10 @@ static RISCVException
 write_hstateen0h(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_hstateenh(env, csrno, wr_mask, new_val);
   }

@@ -2182,6 +2201,10 @@ static RISCVException
 write_sstateen0(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_sstateen(env, csrno, wr_mask, new_val);
   }
target/riscv/insn_trans/trans_rvf.c.inc +13 -3
index a1d3eb52ad..93657680c6 100644
@@ -24,9 +24,46 @@
               return false; \
   } while (0)
 
-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-    if (!ctx->cfg_ptr->ext_zfinx) { \
-        REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int
 index)
+{
+    CPUState *cpu = ctx->cs;
+    CPURISCVState *env = cpu->env_ptr;
+    uint64_t stateen = env->mstateen[index];
+
+    if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+        return true;
+    }
+
+    if (ctx->virt_enabled) {
+        stateen &= env->hstateen[index];

target/riscv/insn_trans/trans_rvf.c.inc line 50

Quoted 10 lines
target/riscv/insn_trans/trans_rvf.c.inc +10 -0
+    }
+
+    if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+        stateen &= env->sstateen[index];
+    }
+
+    if (!(stateen & SMSTATEEN0_FCSR)) {
+        if (ctx->virt_enabled) {
+            ctx->virt_inst_excp = true;
+        }

Are there any considerations for adding virt_inst_excp instead of directly

Quoted 8 lines
Quoted 5 lines

"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?

Regards,

Weiwei Li

I had considered it but I think this is a simpler approach as the rest of the code path stays the same as generating an illegal instruction exception, for e.g setting the bins value (tval).

OK,  we did need to set bins value for virt instruction exception. However I prefer directly call a

new gen_exception_virt function(similar togen_exception_illegal) here.

Quoted 3 lines

Also we need to
return true from all the caller trans_* functions even if the smstateen check has failed.

False is returned when smstateen check fails currently.

Regards,

Weiwei Li

Quoted 64 lines
Quoted 64 lines
Quoted 64 lines
target/riscv/insn_trans/trans_rvf.c.inc +17 -0
+        return false;
+    }
+
+    return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+    if (!has_ext(ctx, RVF)) { \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
+        if (!smstateen_fcsr_check(ctx, 0)) { \
+            return false; \
+        } \
       } \
   } while (0)
target/riscv/insn_trans/trans_rvzfh.c.inc +12 -0
index 5d07150cd0..6c2e338c0a 100644
@@ -20,18 +20,27 @@
       if (!ctx->cfg_ptr->ext_zfh) {      \
           return false;         \
       }                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)
 
   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
           return false;                  \
       }                                  \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)
 
   #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
           return false;                         \
       }                                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)
 
   #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \

@@ -39,6 +48,9 @@
             ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin))
 {     \
           return false;                                        \
       }                                                        \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)

static bool trans_flh(DisasContext *ctx, arg_flh *a)

author
#9 lore raw

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

Quoted 102 lines · weiwei
    

    

    On 2022/10/4 14:51,
      [email protected] wrote:
Quoted 78 lines · weiwei
Quoted 69 lines · Mayuresh Chitale
Quoted 57 lines

If smstateen is implemented and sstateen0.fcsr is
clear then thefloating pointoperations must return illegal instruction exception or virtualinstructiontrap, if relevant.

Signed-off-by Mayuresh Chitale <[email protected]>

--- target/riscv/csr.c | 23
++++++++++++ target/riscv/insn_trans/trans_rvf.c.inc |
43+++++++++++++++++++++
-- target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++ 3 files changed, 75 insertions(+), 3 deletions(-)

target/riscv/csr.cindex renamed from target/riscv/csr.c +0 -0

71236f2b5d..8b25f885ec 100644--- a/target/riscv/csr.c+++ b/target/riscv/csr.c@@ -84,6 +84,10 @@ static RISCVException fs(CPURISCVState *env,
intcsrno) !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx)
{ return RISCV_EXCP_ILLEGAL_INST; }++ if
(!env->debugger && !riscv_cpu_fp_enabled(env)) {+ return
smstateen_acc_ok(env, 0,
SMSTATEEN0_FCSR);+ } #endif return
RISCV_EXCP_NONE; }@@ -2023,6 +2027,9 @@ static RISCVExceptionwrite_mstateen0(CPURISCVState *env, int
csrno, target_ulong
new_val) { uint64_t wr_mask = SMSTATEEN_STATEEN |
SMSTATEEN0_HSENVCFG;+ if (!riscv_has_ext(env, RVF))
{+ wr_mask |= SMSTATEEN0_FCSR;+ } return
write_mstateen(env, csrno, wr_mask, new_val); }@@ -2059,6 +2066,10 @@ static RISCVExceptionwrite_mstateen0h(CPURISCVState
*env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEEN |
SMSTATEEN0_HSENVCFG; + if (!riscv_has_ext(env, RVF))
{+ wr_mask |= SMSTATEEN0_FCSR;+ }+ return
write_mstateenh(env, csrno, wr_mask, new_val); } @@ -2096,6 +2107,10 @@ static RISCVExceptionwrite_hstateen0(CPURISCVState
*env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEEN |
SMSTATEEN0_HSENVCFG; + if (!riscv_has_ext(env, RVF))
{+ wr_mask |= SMSTATEEN0_FCSR;+ }+ return
write_hstateen(env, csrno, wr_mask, new_val); } @@ -2135,6 +2150,10 @@ static RISCVExceptionwrite_hstateen0h(CPURISCVState
*env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEEN |
SMSTATEEN0_HSENVCFG; + if (!riscv_has_ext(env, RVF))
{+ wr_mask |= SMSTATEEN0_FCSR;+ }+ return
write_hstateenh(env, csrno, wr_mask, new_val); } @@ -2182,6 +2201,10 @@ static RISCVExceptionwrite_sstateen0(CPURISCVState
*env, int csrno, { uint64_t wr_mask = SMSTATEEN_STATEEN |
SMSTATEEN0_HSENVCFG; + if (!riscv_has_ext(env, RVF))
{+ wr_mask |= SMSTATEEN0_FCSR;+ }+ return
write_sstateen(env, csrno, wr_mask, new_val); } diff --git a/target/riscv/insn_trans/trans_rvf.c.incb/target/riscv/insn_tr ans/trans_rvf.c.incindex a1d3eb52ad..93657680c6 100644--- a/target/riscv/insn_trans/trans_rvf.c.inc+++ b/target/riscv/insn_trans/trans_rvf.c.inc@@ -24,9 +24,46
@@ return false; \ } while (0) -#define
REQUIRE_ZFINX_OR_F(ctx) do {\- if (!ctx->cfg_ptr->ext_zfinx)
{ \- REQUIRE_EXT(ctx, RVF); \+#ifndef
CONFIG_USER_ONLY+static inline bool smstateen_fcsr_check(DisasContext *ctx,
intindex)+{+ CPUState *cpu = ctx->cs;+ CPURISCVState *env
= cpu->env_ptr;+ uint64_t stateen = env-

Quoted 9 lines
Quoted 4 lines

mstateen[index];++ if (!ctx->cfg_ptr->ext_smstateen || env-
priv == PRV_M) {+ return true;+ }++ if (ctx-
virt_enabled) {+ stateen &= env-
hstateen[index];+ }++ if (env->priv == PRV_U &&

has_ext(ctx, RVS)) {+ stateen &= env-

Quoted 1 line

sstateen[index];+ }++ if (!(stateen & SMSTATEEN0_FCSR))

{+ if (ctx->virt_enabled) {+ ctx-

Quoted 1 line

virt_inst_excp = true;+ }

Quoted 7 lines

Are there any considerations for adding virt_inst_excp
instead ofdirectly
"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here?
Regards,
Weiwei Li

Quoted 4 lines

I had considered it but I think this is a simpler approach as
the restof the code path stays the same as generating an illegal instructionexception, for e.g setting the bins value (tval).

OK, we did need to set bins value for virt instruction
exception. However I prefer directly call a

new gen_exception_virt function(similar to gen_exception_illegal)
here.

Quoted 5 lines
Quoted 3 lines

Also we need toreturn true from all the caller trans_*
functions even if the smstateencheck has failed.

False is returned when smstateen check fails currently.

Yes, however if we make this change then should return true if the check fails so that the decode_opc doesnt fall through and generate another exception. It can be done but it would be contrary to the general convention.

Quoted 51 lines

Regards,
Weiwei Li

Quoted 43 lines
Quoted 41 lines
Quoted 39 lines

+ return false;+ }++ return
true;+}+#else+#define smstateen_fcsr_check(ctx, index)
(true)+#endif++#define REQUIRE_ZFINX_OR_F(ctx) do { \+ if
(!has_ext(ctx, RVF)) { \+ if (!ctx->cfg_ptr->ext_zfinx)
{ \+ return false; \+ } \+ if
(!smstateen_fcsr_check(ctx, 0)) { \+ return false;
\+ } \ } \ } while (0) diff --git
a/target/riscv/insn_trans/trans_rvzfh.c.incb/target/riscv/insn_ trans/trans_rvzfh.c.incindex 5d07150cd0..6c2e338c0a 100644--- a/target/riscv/insn_trans/trans_rvzfh.c.inc+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc@@ -20,18 +20,27
@@ if (!ctx->cfg_ptr->ext_zfh) { \ return
false; \ } \+ if
(!smstateen_fcsr_check(ctx, 0)) { \+ return false;
\+ } \ } while (0) #define REQUIRE_ZHINX_OR_ZFH(ctx) do
{ \ if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr-

Quoted 1 line

ext_zfh) { \ return

false; \ }
\+ if (!smstateen_fcsr_check(ctx, 0)) { \+ return
false; \+ } \ } while (0) #define
REQUIRE_ZFH_OR_ZFHMIN(ctx) do { \ if (!(ctx-

Quoted 1 line

cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) {

\ return
false; \ }

                 \+    if (!smstateen_fcsr_check(ctx, 0)) {

\+ return false; \+ } \ } while (0) #define
REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \@@ -39,6
+48,9 @@ ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr-

Quoted 1 line

ext_zhinxmin)) { \ return

false; \ }
\+ if
(!smstateen_fcsr_check(ctx, 0)) { \+ return false;
\+ } \ } while (0) static bool trans_flh(DisasContext
*ctx, arg_flh *a)

Reviewed-by
#10 lore raw

Re: [PATCH v10 4/5] target/riscv: smstateen check for fcsr

Quoted 143 lines · On 2022/10/7 01:06, [email protected] wrote:
Quoted 142 lines · weiwei
Quoted 131 lines · On 2022/10/4 14:51, [email protected] wrote:
Quoted 122 lines · weiwei
Quoted 121 lines · Mayuresh Chitale

If smstateen is implemented and sstateen0.fcsr is clear then the floating point
operations must return illegal instruction exception or virtual instruction
trap, if relevant.
Signed-off-by: Mayuresh Chitale <[email protected]>

---
   target/riscv/csr.c                        | 23 ++++++++++++
   target/riscv/insn_trans/trans_rvf.c.inc   | 43
+++++++++++++++++++++--
   target/riscv/insn_trans/trans_rvzfh.c.inc | 12 +++++++
   3 files changed, 75 insertions(+), 3 deletions(-)
target/riscv/csr.c +4 -0
index 71236f2b5d..8b25f885ec 100644
@@ -84,6 +84,10 @@ static RISCVException fs(CPURISCVState *env, int
 csrno)
           !RISCV_CPU(env_cpu(env))->cfg.ext_zfinx) {
           return RISCV_EXCP_ILLEGAL_INST;
       }
+
+    if (!env->debugger && !riscv_cpu_fp_enabled(env)) {
+        return smstateen_acc_ok(env, 0, SMSTATEEN0_FCSR);
+    }
   #endif
       return RISCV_EXCP_NONE;

   }
target/riscv/csr.c +3 -0
@@ -2023,6 +2027,9 @@ static RISCVException
 write_mstateen0(CPURISCVState *env, int csrno,
                                         target_ulong new_val)
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
       return write_mstateen(env, csrno, wr_mask, new_val);

   }
target/riscv/csr.c +16 -0
@@ -2059,6 +2066,10 @@ static RISCVException
 write_mstateen0h(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_mstateenh(env, csrno, wr_mask, new_val);
   }

@@ -2096,6 +2107,10 @@ static RISCVException
 write_hstateen0(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_hstateen(env, csrno, wr_mask, new_val);
   }

@@ -2135,6 +2150,10 @@ static RISCVException
 write_hstateen0h(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_hstateenh(env, csrno, wr_mask, new_val);
   }

@@ -2182,6 +2201,10 @@ static RISCVException
 write_sstateen0(CPURISCVState *env, int csrno,
   {
       uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
       return write_sstateen(env, csrno, wr_mask, new_val);
   }
target/riscv/insn_trans/trans_rvf.c.inc +20 -3
index a1d3eb52ad..93657680c6 100644
@@ -24,9 +24,46 @@
               return false; \
   } while (0)
 
-#define REQUIRE_ZFINX_OR_F(ctx) do {\
-    if (!ctx->cfg_ptr->ext_zfinx) { \
-        REQUIRE_EXT(ctx, RVF); \
+#ifndef CONFIG_USER_ONLY
+static inline bool smstateen_fcsr_check(DisasContext *ctx, int
 index)
+{
+    CPUState *cpu = ctx->cs;
+    CPURISCVState *env = cpu->env_ptr;
+    uint64_t stateen = env->mstateen[index];
+
+    if (!ctx->cfg_ptr->ext_smstateen || env->priv == PRV_M) {
+        return true;
+    }
+
+    if (ctx->virt_enabled) {
+        stateen &= env->hstateen[index];
+    }
+
+    if (env->priv == PRV_U && has_ext(ctx, RVS)) {
+        stateen &= env->sstateen[index];
+    }
+
+    if (!(stateen & SMSTATEEN0_FCSR)) {
Quoted 8 lines

target/riscv/insn_trans/trans_rvf.c.inc line 49

Quoted 3 lines
target/riscv/insn_trans/trans_rvf.c.inc +3 -0
+        if (ctx->virt_enabled) {
+            ctx->virt_inst_excp = true;
+        }

Are there any considerations for adding virt_inst_excp instead of directly
"generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT)" here? Regards,
Weiwei Li

Quoted 3 lines

I had considered it but I think this is a simpler approach as the rest of the code path stays the same as generating an illegal instruction exception, for e.g setting the bins value (tval).

OK,  we did need to set bins value for virt instruction exception. However I prefer directly call a

new gen_exception_virt function(similar togen_exception_illegal) here.

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

Also we need to
return true from all the caller trans_* functions even if the smstateen check has failed.

False is returned when smstateen check fails currently.

Yes, however if we make this change then should return true if the check fails so that the decode_opc doesnt fall through and generate another exception. It can be done but it would be contrary to the general convention.

OK.  Acceptable to me.

Reviewed-by Weiwei Li <[email protected]>

Regards,

Weiwei Li

Quoted 68 lines
Quoted 68 lines

Regards,

Weiwei Li

Quoted 64 lines
Quoted 64 lines
Quoted 64 lines
target/riscv/insn_trans/trans_rvf.c.inc +17 -0
+        return false;
+    }
+
+    return true;
+}
+#else
+#define smstateen_fcsr_check(ctx, index) (true)
+#endif
+
+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+    if (!has_ext(ctx, RVF)) { \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
+        if (!smstateen_fcsr_check(ctx, 0)) { \
+            return false; \
+        } \
       } \
   } while (0)
target/riscv/insn_trans/trans_rvzfh.c.inc +12 -0
index 5d07150cd0..6c2e338c0a 100644
@@ -20,18 +20,27 @@
       if (!ctx->cfg_ptr->ext_zfh) {      \
           return false;         \
       }                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)
 
   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
           return false;                  \
       }                                  \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)
 
   #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
           return false;                         \
       }                                         \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)
 
   #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \

@@ -39,6 +48,9 @@
             ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin))
 {     \
           return false;                                        \
       }                                                        \
+    if (!smstateen_fcsr_check(ctx, 0)) { \
+        return false; \
+    } \
   } while (0)

static bool trans_flh(DisasContext *ctx, arg_flh *a)

author
#11 lore raw

PATCH v10 5/5 target/riscv: smstateen knobs

In reply to Mayuresh Chitale

Add knobs to allow users to enable smstateen and also export it via the ISA extension string.

Signed-off-by Mayuresh Chitale <[email protected]>
Reviewed-by Weiwei Li<[email protected]>
Reviewed-by Alistair Francis <[email protected]>
---
 target/riscv/cpu.c | 2 ++
 1 file changed, 2 insertions(+)
target/riscv/cpu.c +2 -0
index b29c88b9f0..6f065f4e5f 100644
@@ -102,6 +102,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
     ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
     ISA_EXT_DATA_ENTRY(smaia, true, PRIV_VERSION_1_12_0, ext_smaia),
+    ISA_EXT_DATA_ENTRY(smstateen, true, PRIV_VERSION_1_12_0, ext_smstateen),
     ISA_EXT_DATA_ENTRY(ssaia, true, PRIV_VERSION_1_12_0, ext_ssaia),
     ISA_EXT_DATA_ENTRY(sscofpmf, true, PRIV_VERSION_1_12_0, ext_sscofpmf),
     ISA_EXT_DATA_ENTRY(sstc, true, PRIV_VERSION_1_12_0, ext_sstc),

@@ -1012,6 +1013,7 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
+    DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
     DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
     DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
     DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
Signature · 1 line
2.25.1