[PATCH v6 0/5] RISC-V Smstateen support

[PATCH v6 0/5] RISC-V Smstateen support

[Mayuresh Chitale]

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

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

The patch 4/5 (AIA) can be reviewed but not merged until the
bits reserved for AIA get re-defined.

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.

Anup Patel (1):
  target/riscv: Force disable extensions if priv spec version does not
    match
*** BLURB HERE ***

Mayuresh Chitale (5):
  target/riscv: Add smstateen support
  target/riscv: smstateen check for h/senvcfg
  target/riscv: smstateen check for fcsr
  target/riscv: smstateen check for AIA/IMSIC
  target/riscv: smstateen knobs

 target/riscv/cpu.c                        |   2 +
 target/riscv/cpu.h                        |   4 +
 target/riscv/cpu_bits.h                   |  37 ++
 target/riscv/csr.c                        | 703 +++++++++++++++++++++-
 target/riscv/insn_trans/trans_rvf.c.inc   |  38 +-
 target/riscv/insn_trans/trans_rvzfh.c.inc |   4 +
 target/riscv/machine.c                    |  21 +
 7 files changed, 804 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v6 1/5] target/riscv: Add smstateen support

[Mayuresh Chitale]

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 <mchitale@ventanamicro.com>
---
 target/riscv/cpu.h      |   4 +
 target/riscv/cpu_bits.h |  37 ++++
 target/riscv/csr.c      | 370 ++++++++++++++++++++++++++++++++++++++++
 target/riscv/machine.c  |  21 +++
 4 files changed, 432 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index ffb1a18873..7f8e5b0014 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -354,6 +354,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
@@ -426,6 +429,7 @@ struct RISCVCPUConfig {
     bool ext_zkt;
     bool ext_ifencei;
     bool ext_icsr;
+    bool ext_smstateen;
     bool ext_svinval;
     bool ext_svnapot;
     bool ext_svpbmt;
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index 6be5a9e9f0..56b7c5bed6 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -199,6 +199,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
@@ -242,6 +248,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
@@ -283,6 +299,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_STATEN    (1ULL << 63)
+
 /* Enhanced Physical Memory Protection (ePMP) */
 #define CSR_MSECCFG         0x747
 #define CSR_MSECCFGH        0x757
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 235f2a011e..27032a416c 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -339,6 +339,68 @@ static RISCVException hmode32(CPURISCVState *env, int csrno)
 
 }
 
+static RISCVException mstateen(CPURISCVState *env, int csrno)
+{
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
+
+    if (!cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    return any(env, csrno);
+}
+
+static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
+{
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
+
+    if (!cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEN)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    return hmode(env, csrno);
+}
+
+static RISCVException hstateen(CPURISCVState *env, int csrno)
+{
+    return hstateen_pred(env, csrno, CSR_HSTATEEN0);
+}
+
+static RISCVException hstateenh(CPURISCVState *env, int csrno)
+{
+    return hstateen_pred(env, csrno, CSR_HSTATEEN0H);
+}
+
+static RISCVException sstateen(CPURISCVState *env, int csrno)
+{
+    bool virt = riscv_cpu_virt_enabled(env);
+    int index = csrno - CSR_SSTATEEN0;
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
+
+    if (!cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    if (virt) {
+        if (!(env->hstateen[index] & SMSTATEEN_STATEN)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+    }
+
+    return smode(env, csrno);
+}
+
 /* Checks if PointerMasking registers could be accessed */
 static RISCVException pointer_masking(CPURISCVState *env, int csrno)
 {
@@ -1699,6 +1761,263 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
+static inline void write_smstateen(CPURISCVState *env, uint64_t *reg,
+                                   uint64_t wr_mask, uint64_t new_val)
+{
+    *reg = (*reg & ~wr_mask) | (new_val & wr_mask);
+}
+
+static RISCVException read_mstateen(CPURISCVState *env, int csrno,
+                                    target_ulong *val)
+{
+    *val = env->mstateen[csrno - CSR_MSTATEEN0];
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_mstateen(CPURISCVState *env, int csrno,
+                                     uint64_t wr_mask, target_ulong new_val)
+{
+    uint64_t *reg;
+
+    reg = &env->mstateen[csrno - CSR_MSTATEEN0];
+    write_smstateen(env, reg, wr_mask, new_val);
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    uint64_t wr_mask = SMSTATEEN_STATEN;
+
+    return write_mstateen(env, csrno, wr_mask, new_val);
+}
+
+static RISCVException write_mstateen1(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_mstateen2(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_mstateen3(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException read_mstateenh(CPURISCVState *env, int csrno,
+                                      target_ulong *val)
+{
+    *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
+                                      uint64_t wr_mask, target_ulong new_val)
+{
+    uint64_t *reg, val;
+
+    reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
+    val = (uint64_t)new_val << 32;
+    val |= *reg & 0xFFFFFFFF;
+    write_smstateen(env, reg, wr_mask, val);
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    uint64_t wr_mask = SMSTATEEN_STATEN;
+
+    return write_mstateenh(env, csrno, wr_mask, new_val);
+}
+
+static RISCVException write_mstateen1h(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_mstateen2h(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_mstateen3h(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException read_hstateen(CPURISCVState *env, int csrno,
+                                    target_ulong *val)
+{
+    int index = csrno - CSR_HSTATEEN0;
+
+    *val = env->hstateen[index] & env->mstateen[index];
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_hstateen(CPURISCVState *env, int csrno,
+                                     uint64_t mask, target_ulong new_val)
+{
+    int index = csrno - CSR_HSTATEEN0;
+    uint64_t *reg, wr_mask;
+
+    reg = &env->hstateen[index];
+    wr_mask = env->mstateen[index] & mask;
+    write_smstateen(env, reg, wr_mask, new_val);
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    uint64_t wr_mask = SMSTATEEN_STATEN;
+
+    return write_hstateen(env, csrno, wr_mask, new_val);
+}
+
+static RISCVException write_hstateen1(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_hstateen2(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_hstateen3(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException read_hstateenh(CPURISCVState *env, int csrno,
+                                     target_ulong *val)
+{
+    int index = csrno - CSR_HSTATEEN0H;
+
+    *val = (env->hstateen[index] >> 32) & (env->mstateen[index] >> 32);
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
+                                      uint64_t mask, target_ulong new_val)
+{
+    int index = csrno - CSR_HSTATEEN0H;
+    uint64_t *reg, wr_mask, val;
+
+    reg = &env->hstateen[index];
+    val = (uint64_t)new_val << 32;
+    val |= *reg & 0xFFFFFFFF;
+    wr_mask = env->mstateen[index] & mask;
+    write_smstateen(env, reg, wr_mask, val);
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
+                                       target_ulong new_val)
+{
+    uint64_t wr_mask = SMSTATEEN_STATEN;
+
+    return write_hstateenh(env, csrno, wr_mask, new_val);
+}
+
+static RISCVException write_hstateen1h(CPURISCVState *env, int csrno,
+                                       target_ulong new_val)
+{
+    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_hstateen2h(CPURISCVState *env, int csrno,
+                                       target_ulong new_val)
+{
+    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_hstateen3h(CPURISCVState *env, int csrno,
+                                       target_ulong new_val)
+{
+    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException read_sstateen(CPURISCVState *env, int csrno,
+                                    target_ulong *val)
+{
+    bool virt = riscv_cpu_virt_enabled(env);
+    int index = csrno - CSR_SSTATEEN0;
+
+    *val = env->sstateen[index] & env->mstateen[index];
+    if (virt) {
+        *val &= env->hstateen[index];
+    }
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_sstateen(CPURISCVState *env, int csrno,
+                                     uint64_t mask, target_ulong new_val)
+{
+    bool virt = riscv_cpu_virt_enabled(env);
+    int index = csrno - CSR_SSTATEEN0;
+    uint64_t wr_mask;
+    uint64_t *reg;
+
+    wr_mask = env->mstateen[index] & mask;
+    if (virt) {
+        wr_mask &= env->hstateen[index];
+    }
+
+    reg = &env->sstateen[index];
+    write_smstateen(env, reg, wr_mask, new_val);
+
+    return RISCV_EXCP_NONE;
+}
+
+static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    uint64_t wr_mask = SMSTATEEN_STATEN;
+
+    return write_sstateen(env, csrno, wr_mask, new_val);
+}
+
+static RISCVException write_sstateen1(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_sstateen2(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
+static RISCVException write_sstateen3(CPURISCVState *env, int csrno,
+                                      target_ulong new_val)
+{
+    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
+}
+
 static RISCVException rmw_mip64(CPURISCVState *env, int csrno,
                                 uint64_t *ret_val,
                                 uint64_t new_val, uint64_t wr_mask)
@@ -3558,6 +3877,57 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
     [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh, write_henvcfgh,
                                           .min_priv_ver = PRIV_VERSION_1_12_0 },
 
+    /* Smstateen extension CSRs */
+    [CSR_MSTATEEN0] = { "mstateen0", mstateen, read_mstateen, write_mstateen0,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN0H] = { "mstateen0h", mstateen, read_mstateenh,
+                          write_mstateen0h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN1] = { "mstateen1", mstateen, read_mstateen, write_mstateen1,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN1H] = { "mstateen1h", mstateen, read_mstateenh,
+                          write_mstateen1h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN2] = { "mstateen2", mstateen, read_mstateen, write_mstateen2,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN2H] = { "mstateen2h", mstateen, read_mstateenh,
+                          write_mstateen2h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN3] = { "mstateen3", mstateen, read_mstateen, write_mstateen3,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_MSTATEEN3H] = { "mstateen3h", mstateen, read_mstateenh,
+                          write_mstateen3h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+
+    [CSR_HSTATEEN0] = { "hstateen0", hstateen, read_hstateen, write_hstateen0,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN0H] = { "hstateen0h", hstateenh, read_hstateenh,
+                          write_hstateen0h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN1] = { "hstateen1", hstateen, read_hstateen, write_hstateen1,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN1H] = { "hstateen1h", hstateenh, read_hstateenh,
+                          write_hstateen1h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN2] = { "hstateen2", hstateen, read_hstateen, write_hstateen2,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN2H] = { "hstateen2h", hstateenh, read_hstateenh,
+                          write_hstateen2h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN3] = { "hstateen3", hstateen, read_hstateen, write_hstateen3,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_HSTATEEN3H] = { "hstateen3h", hstateenh, read_hstateenh,
+                          write_hstateen3h,
+                          .min_priv_ver = PRIV_VERSION_1_12_0 },
+
+    [CSR_SSTATEEN0] = { "sstateen0", sstateen, read_sstateen, write_sstateen0,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_SSTATEEN1] = { "sstateen1", sstateen, read_sstateen, write_sstateen1,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_SSTATEEN2] = { "sstateen2", sstateen, read_sstateen, write_sstateen2,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
+    [CSR_SSTATEEN3] = { "sstateen3", sstateen, read_sstateen, write_sstateen3,
+                         .min_priv_ver = PRIV_VERSION_1_12_0 },
     /* Supervisor Trap Setup */
     [CSR_SSTATUS]    = { "sstatus",    smode, read_sstatus,    write_sstatus, NULL,
                                               read_sstatus_i128                 },
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index dc182ca811..ef418ac19d 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -262,6 +262,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;
@@ -372,6 +392,7 @@ const VMStateDescription vmstate_riscv_cpu = {
         &vmstate_kvmtimer,
         &vmstate_envcfg,
         &vmstate_debug,
+        &vmstate_smstateen,
         NULL
     }
 };
-- 
2.25.1

Re: [PATCH v6 1/5] target/riscv: Add smstateen support

[Weiwei Li]

在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> 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 <mchitale@ventanamicro.com>
> ---
>   target/riscv/cpu.h      |   4 +
>   target/riscv/cpu_bits.h |  37 ++++
>   target/riscv/csr.c      | 370 ++++++++++++++++++++++++++++++++++++++++
>   target/riscv/machine.c  |  21 +++
>   4 files changed, 432 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index ffb1a18873..7f8e5b0014 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -354,6 +354,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
> @@ -426,6 +429,7 @@ struct RISCVCPUConfig {
>       bool ext_zkt;
>       bool ext_ifencei;
>       bool ext_icsr;
> +    bool ext_smstateen;
>       bool ext_svinval;
>       bool ext_svnapot;
>       bool ext_svpbmt;
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index 6be5a9e9f0..56b7c5bed6 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -199,6 +199,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
> @@ -242,6 +248,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
> @@ -283,6 +299,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_STATEN    (1ULL << 63)
Maybe  SMSTATEEN_STATEEN better.
> +
>   /* Enhanced Physical Memory Protection (ePMP) */
>   #define CSR_MSECCFG         0x747
>   #define CSR_MSECCFGH        0x757
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 235f2a011e..27032a416c 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -339,6 +339,68 @@ static RISCVException hmode32(CPURISCVState *env, int csrno)
>   
>   }
>   
> +static RISCVException mstateen(CPURISCVState *env, int csrno)
> +{
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +
> +    if (!cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return any(env, csrno);
> +}
> +
> +static RISCVException hstateen_pred(CPURISCVState *env, int csrno, int base)
> +{
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +
> +    if (!cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEN)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    return hmode(env, csrno);
> +}
> +
> +static RISCVException hstateen(CPURISCVState *env, int csrno)
> +{
> +    return hstateen_pred(env, csrno, CSR_HSTATEEN0);
> +}
> +
> +static RISCVException hstateenh(CPURISCVState *env, int csrno)
> +{
> +    return hstateen_pred(env, csrno, CSR_HSTATEEN0H);
> +}
> +
> +static RISCVException sstateen(CPURISCVState *env, int csrno)
> +{
> +    bool virt = riscv_cpu_virt_enabled(env);
> +    int index = csrno - CSR_SSTATEEN0;
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
> +
> +    if (!cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    if (virt) {
> +        if (!(env->hstateen[index] & SMSTATEEN_STATEN)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +    }
> +
> +    return smode(env, csrno);
> +}
> +
>   /* Checks if PointerMasking registers could be accessed */
>   static RISCVException pointer_masking(CPURISCVState *env, int csrno)
>   {
> @@ -1699,6 +1761,263 @@ static RISCVException write_henvcfgh(CPURISCVState *env, int csrno,
>       return RISCV_EXCP_NONE;
>   }
>   
> +static inline void write_smstateen(CPURISCVState *env, uint64_t *reg,
> +                                   uint64_t wr_mask, uint64_t new_val)
> +{
> +    *reg = (*reg & ~wr_mask) | (new_val & wr_mask);
> +}
> +
> +static RISCVException read_mstateen(CPURISCVState *env, int csrno,
> +                                    target_ulong *val)
> +{
> +    *val = env->mstateen[csrno - CSR_MSTATEEN0];
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_mstateen(CPURISCVState *env, int csrno,
> +                                     uint64_t wr_mask, target_ulong new_val)
> +{
> +    uint64_t *reg;
> +
> +    reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> +    write_smstateen(env, reg, wr_mask, new_val);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    uint64_t wr_mask = SMSTATEEN_STATEN;
> +
> +    return write_mstateen(env, csrno, wr_mask, new_val);
> +}
> +
> +static RISCVException write_mstateen1(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_mstateen2(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_mstateen3(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
Mstateen1~3  can share the same write function currently. The same to 
h/sstateen1~3.
> +static RISCVException read_mstateenh(CPURISCVState *env, int csrno,
> +                                      target_ulong *val)
> +{
> +    *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_mstateenh(CPURISCVState *env, int csrno,
> +                                      uint64_t wr_mask, target_ulong new_val)
> +{
> +    uint64_t *reg, val;
> +
> +    reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> +    val = (uint64_t)new_val << 32;
> +    val |= *reg & 0xFFFFFFFF;
> +    write_smstateen(env, reg, wr_mask, val);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    uint64_t wr_mask = SMSTATEEN_STATEN;
> +
> +    return write_mstateenh(env, csrno, wr_mask, new_val);
> +}
> +
> +static RISCVException write_mstateen1h(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_mstateen2h(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_mstateen3h(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException read_hstateen(CPURISCVState *env, int csrno,
> +                                    target_ulong *val)
> +{
> +    int index = csrno - CSR_HSTATEEN0;
> +
> +    *val = env->hstateen[index] & env->mstateen[index];
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_hstateen(CPURISCVState *env, int csrno,
> +                                     uint64_t mask, target_ulong new_val)
> +{
> +    int index = csrno - CSR_HSTATEEN0;
> +    uint64_t *reg, wr_mask;
> +
> +    reg = &env->hstateen[index];
> +    wr_mask = env->mstateen[index] & mask;
> +    write_smstateen(env, reg, wr_mask, new_val);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    uint64_t wr_mask = SMSTATEEN_STATEN;
> +
> +    return write_hstateen(env, csrno, wr_mask, new_val);
> +}
> +
> +static RISCVException write_hstateen1(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_hstateen2(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_hstateen3(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException read_hstateenh(CPURISCVState *env, int csrno,
> +                                     target_ulong *val)
> +{
> +    int index = csrno - CSR_HSTATEEN0H;
> +
> +    *val = (env->hstateen[index] >> 32) & (env->mstateen[index] >> 32);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_hstateenh(CPURISCVState *env, int csrno,
> +                                      uint64_t mask, target_ulong new_val)
> +{
> +    int index = csrno - CSR_HSTATEEN0H;
> +    uint64_t *reg, wr_mask, val;
> +
> +    reg = &env->hstateen[index];
> +    val = (uint64_t)new_val << 32;
> +    val |= *reg & 0xFFFFFFFF;
> +    wr_mask = env->mstateen[index] & mask;
> +    write_smstateen(env, reg, wr_mask, val);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
> +                                       target_ulong new_val)
> +{
> +    uint64_t wr_mask = SMSTATEEN_STATEN;
> +
> +    return write_hstateenh(env, csrno, wr_mask, new_val);
> +}
> +
> +static RISCVException write_hstateen1h(CPURISCVState *env, int csrno,
> +                                       target_ulong new_val)
> +{
> +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_hstateen2h(CPURISCVState *env, int csrno,
> +                                       target_ulong new_val)
> +{
> +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_hstateen3h(CPURISCVState *env, int csrno,
> +                                       target_ulong new_val)
> +{
> +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException read_sstateen(CPURISCVState *env, int csrno,
> +                                    target_ulong *val)
> +{
> +    bool virt = riscv_cpu_virt_enabled(env);
> +    int index = csrno - CSR_SSTATEEN0;
> +
> +    *val = env->sstateen[index] & env->mstateen[index];
> +    if (virt) {
> +        *val &= env->hstateen[index];
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_sstateen(CPURISCVState *env, int csrno,
> +                                     uint64_t mask, target_ulong new_val)
> +{
> +    bool virt = riscv_cpu_virt_enabled(env);
> +    int index = csrno - CSR_SSTATEEN0;
> +    uint64_t wr_mask;
> +    uint64_t *reg;
> +
> +    wr_mask = env->mstateen[index] & mask;
> +    if (virt) {
> +        wr_mask &= env->hstateen[index];
> +    }
> +
> +    reg = &env->sstateen[index];
> +    write_smstateen(env, reg, wr_mask, new_val);
> +
> +    return RISCV_EXCP_NONE;
> +}
> +
> +static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    uint64_t wr_mask = SMSTATEEN_STATEN;
> +
> +    return write_sstateen(env, csrno, wr_mask, new_val);
> +}
> +
> +static RISCVException write_sstateen1(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_sstateen2(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
> +static RISCVException write_sstateen3(CPURISCVState *env, int csrno,
> +                                      target_ulong new_val)
> +{
> +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> +}
> +
>   static RISCVException rmw_mip64(CPURISCVState *env, int csrno,
>                                   uint64_t *ret_val,
>                                   uint64_t new_val, uint64_t wr_mask)
> @@ -3558,6 +3877,57 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
>       [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh, write_henvcfgh,
>                                             .min_priv_ver = PRIV_VERSION_1_12_0 },
>   
> +    /* Smstateen extension CSRs */
> +    [CSR_MSTATEEN0] = { "mstateen0", mstateen, read_mstateen, write_mstateen0,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },

The new lines have been updated to align with the last line in my 
previous patchset(accepted).

So it's better to make ' . ' align with ' " '.

Regards,

Weiwei Li

> +    [CSR_MSTATEEN0H] = { "mstateen0h", mstateen, read_mstateenh,
> +                          write_mstateen0h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_MSTATEEN1] = { "mstateen1", mstateen, read_mstateen, write_mstateen1,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_MSTATEEN1H] = { "mstateen1h", mstateen, read_mstateenh,
> +                          write_mstateen1h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_MSTATEEN2] = { "mstateen2", mstateen, read_mstateen, write_mstateen2,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_MSTATEEN2H] = { "mstateen2h", mstateen, read_mstateenh,
> +                          write_mstateen2h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_MSTATEEN3] = { "mstateen3", mstateen, read_mstateen, write_mstateen3,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_MSTATEEN3H] = { "mstateen3h", mstateen, read_mstateenh,
> +                          write_mstateen3h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +
> +    [CSR_HSTATEEN0] = { "hstateen0", hstateen, read_hstateen, write_hstateen0,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN0H] = { "hstateen0h", hstateenh, read_hstateenh,
> +                          write_hstateen0h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN1] = { "hstateen1", hstateen, read_hstateen, write_hstateen1,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN1H] = { "hstateen1h", hstateenh, read_hstateenh,
> +                          write_hstateen1h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN2] = { "hstateen2", hstateen, read_hstateen, write_hstateen2,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN2H] = { "hstateen2h", hstateenh, read_hstateenh,
> +                          write_hstateen2h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN3] = { "hstateen3", hstateen, read_hstateen, write_hstateen3,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_HSTATEEN3H] = { "hstateen3h", hstateenh, read_hstateenh,
> +                          write_hstateen3h,
> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> +
> +    [CSR_SSTATEEN0] = { "sstateen0", sstateen, read_sstateen, write_sstateen0,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_SSTATEEN1] = { "sstateen1", sstateen, read_sstateen, write_sstateen1,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_SSTATEEN2] = { "sstateen2", sstateen, read_sstateen, write_sstateen2,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> +    [CSR_SSTATEEN3] = { "sstateen3", sstateen, read_sstateen, write_sstateen3,
> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>       /* Supervisor Trap Setup */
>       [CSR_SSTATUS]    = { "sstatus",    smode, read_sstatus,    write_sstatus, NULL,
>                                                 read_sstatus_i128                 },
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index dc182ca811..ef418ac19d 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -262,6 +262,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;
> @@ -372,6 +392,7 @@ const VMStateDescription vmstate_riscv_cpu = {
>           &vmstate_kvmtimer,
>           &vmstate_envcfg,
>           &vmstate_debug,
> +        &vmstate_smstateen,
>           NULL
>       }
>   };

Re: [PATCH v6 1/5] target/riscv: Add smstateen support

[Mayuresh Chitale]

On Fri, 2022-07-22 at 08:31 +0800, Weiwei Li wrote:
> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > 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 <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/cpu.h      |   4 +
> >   target/riscv/cpu_bits.h |  37 ++++
> >   target/riscv/csr.c      | 370
> > ++++++++++++++++++++++++++++++++++++++++
> >   target/riscv/machine.c  |  21 +++
> >   4 files changed, 432 insertions(+)
> > 
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index ffb1a18873..7f8e5b0014 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -354,6 +354,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
> > @@ -426,6 +429,7 @@ struct RISCVCPUConfig {
> >       bool ext_zkt;
> >       bool ext_ifencei;
> >       bool ext_icsr;
> > +    bool ext_smstateen;
> >       bool ext_svinval;
> >       bool ext_svnapot;
> >       bool ext_svpbmt;
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 6be5a9e9f0..56b7c5bed6 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -199,6 +199,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
> > @@ -242,6 +248,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
> > @@ -283,6 +299,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_STATEN    (1ULL << 63)
> Maybe  SMSTATEEN_STATEEN better.
ok. Will update in the next version.
> > +
> >   /* Enhanced Physical Memory Protection (ePMP) */
> >   #define CSR_MSECCFG         0x747
> >   #define CSR_MSECCFGH        0x757
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 235f2a011e..27032a416c 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -339,6 +339,68 @@ static RISCVException hmode32(CPURISCVState
> > *env, int csrno)
> >   
> >   }
> >   
> > +static RISCVException mstateen(CPURISCVState *env, int csrno)
> > +{
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +
> > +    if (!cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return any(env, csrno);
> > +}
> > +
> > +static RISCVException hstateen_pred(CPURISCVState *env, int csrno,
> > int base)
> > +{
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +
> > +    if (!cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEN)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    return hmode(env, csrno);
> > +}
> > +
> > +static RISCVException hstateen(CPURISCVState *env, int csrno)
> > +{
> > +    return hstateen_pred(env, csrno, CSR_HSTATEEN0);
> > +}
> > +
> > +static RISCVException hstateenh(CPURISCVState *env, int csrno)
> > +{
> > +    return hstateen_pred(env, csrno, CSR_HSTATEEN0H);
> > +}
> > +
> > +static RISCVException sstateen(CPURISCVState *env, int csrno)
> > +{
> > +    bool virt = riscv_cpu_virt_enabled(env);
> > +    int index = csrno - CSR_SSTATEEN0;
> > +    CPUState *cs = env_cpu(env);
> > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > +
> > +    if (!cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    if (virt) {
> > +        if (!(env->hstateen[index] & SMSTATEEN_STATEN)) {
> > +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +        }
> > +    }
> > +
> > +    return smode(env, csrno);
> > +}
> > +
> >   /* Checks if PointerMasking registers could be accessed */
> >   static RISCVException pointer_masking(CPURISCVState *env, int
> > csrno)
> >   {
> > @@ -1699,6 +1761,263 @@ static RISCVException
> > write_henvcfgh(CPURISCVState *env, int csrno,
> >       return RISCV_EXCP_NONE;
> >   }
> >   
> > +static inline void write_smstateen(CPURISCVState *env, uint64_t
> > *reg,
> > +                                   uint64_t wr_mask, uint64_t
> > new_val)
> > +{
> > +    *reg = (*reg & ~wr_mask) | (new_val & wr_mask);
> > +}
> > +
> > +static RISCVException read_mstateen(CPURISCVState *env, int csrno,
> > +                                    target_ulong *val)
> > +{
> > +    *val = env->mstateen[csrno - CSR_MSTATEEN0];
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mstateen(CPURISCVState *env, int
> > csrno,
> > +                                     uint64_t wr_mask,
> > target_ulong new_val)
> > +{
> > +    uint64_t *reg;
> > +
> > +    reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> > +    write_smstateen(env, reg, wr_mask, new_val);
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mstateen0(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +
> > +    return write_mstateen(env, csrno, wr_mask, new_val);
> > +}
> > +
> > +static RISCVException write_mstateen1(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_mstateen2(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_mstateen3(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> Mstateen1~3  can share the same write function currently. The same
> to 
> h/sstateen1~3.
I think its better to keep them separate as it will anyway be required
once they are defined.
> > +static RISCVException read_mstateenh(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong *val)
> > +{
> > +    *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mstateenh(CPURISCVState *env, int
> > csrno,
> > +                                      uint64_t wr_mask,
> > target_ulong new_val)
> > +{
> > +    uint64_t *reg, val;
> > +
> > +    reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> > +    val = (uint64_t)new_val << 32;
> > +    val |= *reg & 0xFFFFFFFF;
> > +    write_smstateen(env, reg, wr_mask, val);
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_mstateen0h(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +
> > +    return write_mstateenh(env, csrno, wr_mask, new_val);
> > +}
> > +
> > +static RISCVException write_mstateen1h(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_mstateen2h(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_mstateen3h(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException read_hstateen(CPURISCVState *env, int csrno,
> > +                                    target_ulong *val)
> > +{
> > +    int index = csrno - CSR_HSTATEEN0;
> > +
> > +    *val = env->hstateen[index] & env->mstateen[index];
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_hstateen(CPURISCVState *env, int
> > csrno,
> > +                                     uint64_t mask, target_ulong
> > new_val)
> > +{
> > +    int index = csrno - CSR_HSTATEEN0;
> > +    uint64_t *reg, wr_mask;
> > +
> > +    reg = &env->hstateen[index];
> > +    wr_mask = env->mstateen[index] & mask;
> > +    write_smstateen(env, reg, wr_mask, new_val);
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_hstateen0(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +
> > +    return write_hstateen(env, csrno, wr_mask, new_val);
> > +}
> > +
> > +static RISCVException write_hstateen1(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_hstateen2(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_hstateen3(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException read_hstateenh(CPURISCVState *env, int
> > csrno,
> > +                                     target_ulong *val)
> > +{
> > +    int index = csrno - CSR_HSTATEEN0H;
> > +
> > +    *val = (env->hstateen[index] >> 32) & (env->mstateen[index] >>
> > 32);
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_hstateenh(CPURISCVState *env, int
> > csrno,
> > +                                      uint64_t mask, target_ulong
> > new_val)
> > +{
> > +    int index = csrno - CSR_HSTATEEN0H;
> > +    uint64_t *reg, wr_mask, val;
> > +
> > +    reg = &env->hstateen[index];
> > +    val = (uint64_t)new_val << 32;
> > +    val |= *reg & 0xFFFFFFFF;
> > +    wr_mask = env->mstateen[index] & mask;
> > +    write_smstateen(env, reg, wr_mask, val);
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_hstateen0h(CPURISCVState *env, int
> > csrno,
> > +                                       target_ulong new_val)
> > +{
> > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +
> > +    return write_hstateenh(env, csrno, wr_mask, new_val);
> > +}
> > +
> > +static RISCVException write_hstateen1h(CPURISCVState *env, int
> > csrno,
> > +                                       target_ulong new_val)
> > +{
> > +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_hstateen2h(CPURISCVState *env, int
> > csrno,
> > +                                       target_ulong new_val)
> > +{
> > +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_hstateen3h(CPURISCVState *env, int
> > csrno,
> > +                                       target_ulong new_val)
> > +{
> > +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException read_sstateen(CPURISCVState *env, int csrno,
> > +                                    target_ulong *val)
> > +{
> > +    bool virt = riscv_cpu_virt_enabled(env);
> > +    int index = csrno - CSR_SSTATEEN0;
> > +
> > +    *val = env->sstateen[index] & env->mstateen[index];
> > +    if (virt) {
> > +        *val &= env->hstateen[index];
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_sstateen(CPURISCVState *env, int
> > csrno,
> > +                                     uint64_t mask, target_ulong
> > new_val)
> > +{
> > +    bool virt = riscv_cpu_virt_enabled(env);
> > +    int index = csrno - CSR_SSTATEEN0;
> > +    uint64_t wr_mask;
> > +    uint64_t *reg;
> > +
> > +    wr_mask = env->mstateen[index] & mask;
> > +    if (virt) {
> > +        wr_mask &= env->hstateen[index];
> > +    }
> > +
> > +    reg = &env->sstateen[index];
> > +    write_smstateen(env, reg, wr_mask, new_val);
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_sstateen0(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > +
> > +    return write_sstateen(env, csrno, wr_mask, new_val);
> > +}
> > +
> > +static RISCVException write_sstateen1(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_sstateen2(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> > +static RISCVException write_sstateen3(CPURISCVState *env, int
> > csrno,
> > +                                      target_ulong new_val)
> > +{
> > +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
> > +}
> > +
> >   static RISCVException rmw_mip64(CPURISCVState *env, int csrno,
> >                                   uint64_t *ret_val,
> >                                   uint64_t new_val, uint64_t
> > wr_mask)
> > @@ -3558,6 +3877,57 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE]
> > = {
> >       [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh,
> > write_henvcfgh,
> >                                             .min_priv_ver =
> > PRIV_VERSION_1_12_0 },
> >   
> > +    /* Smstateen extension CSRs */
> > +    [CSR_MSTATEEN0] = { "mstateen0", mstateen, read_mstateen,
> > write_mstateen0,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> 
> The new lines have been updated to align with the last line in my 
> previous patchset(accepted).
> 
> So it's better to make ' . ' align with ' " '.
ok. Will update in the next version.
> > 
> 
> Regards,
> 
> Weiwei Li
> 
> > +    [CSR_MSTATEEN0H] = { "mstateen0h", mstateen, read_mstateenh,
> > +                          write_mstateen0h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_MSTATEEN1] = { "mstateen1", mstateen, read_mstateen,
> > write_mstateen1,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_MSTATEEN1H] = { "mstateen1h", mstateen, read_mstateenh,
> > +                          write_mstateen1h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_MSTATEEN2] = { "mstateen2", mstateen, read_mstateen,
> > write_mstateen2,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_MSTATEEN2H] = { "mstateen2h", mstateen, read_mstateenh,
> > +                          write_mstateen2h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_MSTATEEN3] = { "mstateen3", mstateen, read_mstateen,
> > write_mstateen3,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_MSTATEEN3H] = { "mstateen3h", mstateen, read_mstateenh,
> > +                          write_mstateen3h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +
> > +    [CSR_HSTATEEN0] = { "hstateen0", hstateen, read_hstateen,
> > write_hstateen0,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN0H] = { "hstateen0h", hstateenh, read_hstateenh,
> > +                          write_hstateen0h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN1] = { "hstateen1", hstateen, read_hstateen,
> > write_hstateen1,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN1H] = { "hstateen1h", hstateenh, read_hstateenh,
> > +                          write_hstateen1h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN2] = { "hstateen2", hstateen, read_hstateen,
> > write_hstateen2,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN2H] = { "hstateen2h", hstateenh, read_hstateenh,
> > +                          write_hstateen2h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN3] = { "hstateen3", hstateen, read_hstateen,
> > write_hstateen3,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_HSTATEEN3H] = { "hstateen3h", hstateenh, read_hstateenh,
> > +                          write_hstateen3h,
> > +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +
> > +    [CSR_SSTATEEN0] = { "sstateen0", sstateen, read_sstateen,
> > write_sstateen0,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_SSTATEEN1] = { "sstateen1", sstateen, read_sstateen,
> > write_sstateen1,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_SSTATEEN2] = { "sstateen2", sstateen, read_sstateen,
> > write_sstateen2,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> > +    [CSR_SSTATEEN3] = { "sstateen3", sstateen, read_sstateen,
> > write_sstateen3,
> > +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
> >       /* Supervisor Trap Setup */
> >       [CSR_SSTATUS]    = { "sstatus",    smode,
> > read_sstatus,    write_sstatus, NULL,
> >                                                 read_sstatus_i128  
> >                },
> > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > index dc182ca811..ef418ac19d 100644
> > --- a/target/riscv/machine.c
> > +++ b/target/riscv/machine.c
> > @@ -262,6 +262,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;
> > @@ -372,6 +392,7 @@ const VMStateDescription vmstate_riscv_cpu = {
> >           &vmstate_kvmtimer,
> >           &vmstate_envcfg,
> >           &vmstate_debug,
> > +        &vmstate_smstateen,
> >           NULL
> >       }
> >   };

Re: [PATCH v6 1/5] target/riscv: Add smstateen support

[Weiwei Li]

在 2022/7/24 下午11:39, Mayuresh Chitale 写道:
> On Fri, 2022-07-22 at 08:31 +0800, Weiwei Li wrote:
>> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
>>> 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 <mchitale@ventanamicro.com>
>>> ---
>>>    target/riscv/cpu.h      |   4 +
>>>    target/riscv/cpu_bits.h |  37 ++++
>>>    target/riscv/csr.c      | 370
>>> ++++++++++++++++++++++++++++++++++++++++
>>>    target/riscv/machine.c  |  21 +++
>>>    4 files changed, 432 insertions(+)
>>>
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index ffb1a18873..7f8e5b0014 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -354,6 +354,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
>>> @@ -426,6 +429,7 @@ struct RISCVCPUConfig {
>>>        bool ext_zkt;
>>>        bool ext_ifencei;
>>>        bool ext_icsr;
>>> +    bool ext_smstateen;
>>>        bool ext_svinval;
>>>        bool ext_svnapot;
>>>        bool ext_svpbmt;
>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>> index 6be5a9e9f0..56b7c5bed6 100644
>>> --- a/target/riscv/cpu_bits.h
>>> +++ b/target/riscv/cpu_bits.h
>>> @@ -199,6 +199,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
>>> @@ -242,6 +248,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
>>> @@ -283,6 +299,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_STATEN    (1ULL << 63)
>> Maybe  SMSTATEEN_STATEEN better.
> ok. Will update in the next version.
>>> +
>>>    /* Enhanced Physical Memory Protection (ePMP) */
>>>    #define CSR_MSECCFG         0x747
>>>    #define CSR_MSECCFGH        0x757
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index 235f2a011e..27032a416c 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -339,6 +339,68 @@ static RISCVException hmode32(CPURISCVState
>>> *env, int csrno)
>>>    
>>>    }
>>>    
>>> +static RISCVException mstateen(CPURISCVState *env, int csrno)
>>> +{
>>> +    CPUState *cs = env_cpu(env);
>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>> +
>>> +    if (!cpu->cfg.ext_smstateen) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>>> +    return any(env, csrno);
>>> +}
>>> +
>>> +static RISCVException hstateen_pred(CPURISCVState *env, int csrno,
>>> int base)
>>> +{
>>> +    CPUState *cs = env_cpu(env);
>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>> +
>>> +    if (!cpu->cfg.ext_smstateen) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>>> +    if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEN)) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>>> +    return hmode(env, csrno);
>>> +}
>>> +
>>> +static RISCVException hstateen(CPURISCVState *env, int csrno)
>>> +{
>>> +    return hstateen_pred(env, csrno, CSR_HSTATEEN0);
>>> +}
>>> +
>>> +static RISCVException hstateenh(CPURISCVState *env, int csrno)
>>> +{
>>> +    return hstateen_pred(env, csrno, CSR_HSTATEEN0H);
>>> +}
>>> +
>>> +static RISCVException sstateen(CPURISCVState *env, int csrno)
>>> +{
>>> +    bool virt = riscv_cpu_virt_enabled(env);
>>> +    int index = csrno - CSR_SSTATEEN0;
>>> +    CPUState *cs = env_cpu(env);
>>> +    RISCVCPU *cpu = RISCV_CPU(cs);
>>> +
>>> +    if (!cpu->cfg.ext_smstateen) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>>> +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>>> +    if (virt) {
>>> +        if (!(env->hstateen[index] & SMSTATEEN_STATEN)) {
>>> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> +        }
>>> +    }
>>> +
>>> +    return smode(env, csrno);
>>> +}
>>> +
>>>    /* Checks if PointerMasking registers could be accessed */
>>>    static RISCVException pointer_masking(CPURISCVState *env, int
>>> csrno)
>>>    {
>>> @@ -1699,6 +1761,263 @@ static RISCVException
>>> write_henvcfgh(CPURISCVState *env, int csrno,
>>>        return RISCV_EXCP_NONE;
>>>    }
>>>    
>>> +static inline void write_smstateen(CPURISCVState *env, uint64_t
>>> *reg,
>>> +                                   uint64_t wr_mask, uint64_t
>>> new_val)
>>> +{
>>> +    *reg = (*reg & ~wr_mask) | (new_val & wr_mask);
>>> +}
>>> +
>>> +static RISCVException read_mstateen(CPURISCVState *env, int csrno,
>>> +                                    target_ulong *val)
>>> +{
>>> +    *val = env->mstateen[csrno - CSR_MSTATEEN0];
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_mstateen(CPURISCVState *env, int
>>> csrno,
>>> +                                     uint64_t wr_mask,
>>> target_ulong new_val)
>>> +{
>>> +    uint64_t *reg;
>>> +
>>> +    reg = &env->mstateen[csrno - CSR_MSTATEEN0];
>>> +    write_smstateen(env, reg, wr_mask, new_val);
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_mstateen0(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN;
>>> +
>>> +    return write_mstateen(env, csrno, wr_mask, new_val);
>>> +}
>>> +
>>> +static RISCVException write_mstateen1(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_mstateen2(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_mstateen3(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_mstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>> Mstateen1~3  can share the same write function currently. The same
>> to
>> h/sstateen1~3.
> I think its better to keep them separate as it will anyway be required
> once they are defined.

Only a few bits in mstateen0 is used currently. It may need a long time 
before mstateen1~3 to be used.

And it's very easy to separate it out if required. So it seems 
unnecessary to copy two times to prepare

for future use currently.

Regards,

Weiwei Li

>>> +static RISCVException read_mstateenh(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong *val)
>>> +{
>>> +    *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_mstateenh(CPURISCVState *env, int
>>> csrno,
>>> +                                      uint64_t wr_mask,
>>> target_ulong new_val)
>>> +{
>>> +    uint64_t *reg, val;
>>> +
>>> +    reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
>>> +    val = (uint64_t)new_val << 32;
>>> +    val |= *reg & 0xFFFFFFFF;
>>> +    write_smstateen(env, reg, wr_mask, val);
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_mstateen0h(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN;
>>> +
>>> +    return write_mstateenh(env, csrno, wr_mask, new_val);
>>> +}
>>> +
>>> +static RISCVException write_mstateen1h(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_mstateen2h(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_mstateen3h(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException read_hstateen(CPURISCVState *env, int csrno,
>>> +                                    target_ulong *val)
>>> +{
>>> +    int index = csrno - CSR_HSTATEEN0;
>>> +
>>> +    *val = env->hstateen[index] & env->mstateen[index];
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_hstateen(CPURISCVState *env, int
>>> csrno,
>>> +                                     uint64_t mask, target_ulong
>>> new_val)
>>> +{
>>> +    int index = csrno - CSR_HSTATEEN0;
>>> +    uint64_t *reg, wr_mask;
>>> +
>>> +    reg = &env->hstateen[index];
>>> +    wr_mask = env->mstateen[index] & mask;
>>> +    write_smstateen(env, reg, wr_mask, new_val);
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_hstateen0(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN;
>>> +
>>> +    return write_hstateen(env, csrno, wr_mask, new_val);
>>> +}
>>> +
>>> +static RISCVException write_hstateen1(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_hstateen2(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_hstateen3(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_hstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException read_hstateenh(CPURISCVState *env, int
>>> csrno,
>>> +                                     target_ulong *val)
>>> +{
>>> +    int index = csrno - CSR_HSTATEEN0H;
>>> +
>>> +    *val = (env->hstateen[index] >> 32) & (env->mstateen[index] >>
>>> 32);
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_hstateenh(CPURISCVState *env, int
>>> csrno,
>>> +                                      uint64_t mask, target_ulong
>>> new_val)
>>> +{
>>> +    int index = csrno - CSR_HSTATEEN0H;
>>> +    uint64_t *reg, wr_mask, val;
>>> +
>>> +    reg = &env->hstateen[index];
>>> +    val = (uint64_t)new_val << 32;
>>> +    val |= *reg & 0xFFFFFFFF;
>>> +    wr_mask = env->mstateen[index] & mask;
>>> +    write_smstateen(env, reg, wr_mask, val);
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_hstateen0h(CPURISCVState *env, int
>>> csrno,
>>> +                                       target_ulong new_val)
>>> +{
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN;
>>> +
>>> +    return write_hstateenh(env, csrno, wr_mask, new_val);
>>> +}
>>> +
>>> +static RISCVException write_hstateen1h(CPURISCVState *env, int
>>> csrno,
>>> +                                       target_ulong new_val)
>>> +{
>>> +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_hstateen2h(CPURISCVState *env, int
>>> csrno,
>>> +                                       target_ulong new_val)
>>> +{
>>> +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_hstateen3h(CPURISCVState *env, int
>>> csrno,
>>> +                                       target_ulong new_val)
>>> +{
>>> +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException read_sstateen(CPURISCVState *env, int csrno,
>>> +                                    target_ulong *val)
>>> +{
>>> +    bool virt = riscv_cpu_virt_enabled(env);
>>> +    int index = csrno - CSR_SSTATEEN0;
>>> +
>>> +    *val = env->sstateen[index] & env->mstateen[index];
>>> +    if (virt) {
>>> +        *val &= env->hstateen[index];
>>> +    }
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_sstateen(CPURISCVState *env, int
>>> csrno,
>>> +                                     uint64_t mask, target_ulong
>>> new_val)
>>> +{
>>> +    bool virt = riscv_cpu_virt_enabled(env);
>>> +    int index = csrno - CSR_SSTATEEN0;
>>> +    uint64_t wr_mask;
>>> +    uint64_t *reg;
>>> +
>>> +    wr_mask = env->mstateen[index] & mask;
>>> +    if (virt) {
>>> +        wr_mask &= env->hstateen[index];
>>> +    }
>>> +
>>> +    reg = &env->sstateen[index];
>>> +    write_smstateen(env, reg, wr_mask, new_val);
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +
>>> +static RISCVException write_sstateen0(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN;
>>> +
>>> +    return write_sstateen(env, csrno, wr_mask, new_val);
>>> +}
>>> +
>>> +static RISCVException write_sstateen1(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_sstateen2(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>> +static RISCVException write_sstateen3(CPURISCVState *env, int
>>> csrno,
>>> +                                      target_ulong new_val)
>>> +{
>>> +    return write_sstateen(env, csrno, SMSTATEEN_STATEN, new_val);
>>> +}
>>> +
>>>    static RISCVException rmw_mip64(CPURISCVState *env, int csrno,
>>>                                    uint64_t *ret_val,
>>>                                    uint64_t new_val, uint64_t
>>> wr_mask)
>>> @@ -3558,6 +3877,57 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE]
>>> = {
>>>        [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh,
>>> write_henvcfgh,
>>>                                              .min_priv_ver =
>>> PRIV_VERSION_1_12_0 },
>>>    
>>> +    /* Smstateen extension CSRs */
>>> +    [CSR_MSTATEEN0] = { "mstateen0", mstateen, read_mstateen,
>>> write_mstateen0,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>> The new lines have been updated to align with the last line in my
>> previous patchset(accepted).
>>
>> So it's better to make ' . ' align with ' " '.
> ok. Will update in the next version.
>> Regards,
>>
>> Weiwei Li
>>
>>> +    [CSR_MSTATEEN0H] = { "mstateen0h", mstateen, read_mstateenh,
>>> +                          write_mstateen0h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_MSTATEEN1] = { "mstateen1", mstateen, read_mstateen,
>>> write_mstateen1,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_MSTATEEN1H] = { "mstateen1h", mstateen, read_mstateenh,
>>> +                          write_mstateen1h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_MSTATEEN2] = { "mstateen2", mstateen, read_mstateen,
>>> write_mstateen2,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_MSTATEEN2H] = { "mstateen2h", mstateen, read_mstateenh,
>>> +                          write_mstateen2h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_MSTATEEN3] = { "mstateen3", mstateen, read_mstateen,
>>> write_mstateen3,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_MSTATEEN3H] = { "mstateen3h", mstateen, read_mstateenh,
>>> +                          write_mstateen3h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +
>>> +    [CSR_HSTATEEN0] = { "hstateen0", hstateen, read_hstateen,
>>> write_hstateen0,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN0H] = { "hstateen0h", hstateenh, read_hstateenh,
>>> +                          write_hstateen0h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN1] = { "hstateen1", hstateen, read_hstateen,
>>> write_hstateen1,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN1H] = { "hstateen1h", hstateenh, read_hstateenh,
>>> +                          write_hstateen1h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN2] = { "hstateen2", hstateen, read_hstateen,
>>> write_hstateen2,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN2H] = { "hstateen2h", hstateenh, read_hstateenh,
>>> +                          write_hstateen2h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN3] = { "hstateen3", hstateen, read_hstateen,
>>> write_hstateen3,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_HSTATEEN3H] = { "hstateen3h", hstateenh, read_hstateenh,
>>> +                          write_hstateen3h,
>>> +                          .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +
>>> +    [CSR_SSTATEEN0] = { "sstateen0", sstateen, read_sstateen,
>>> write_sstateen0,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_SSTATEEN1] = { "sstateen1", sstateen, read_sstateen,
>>> write_sstateen1,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_SSTATEEN2] = { "sstateen2", sstateen, read_sstateen,
>>> write_sstateen2,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>> +    [CSR_SSTATEEN3] = { "sstateen3", sstateen, read_sstateen,
>>> write_sstateen3,
>>> +                         .min_priv_ver = PRIV_VERSION_1_12_0 },
>>>        /* Supervisor Trap Setup */
>>>        [CSR_SSTATUS]    = { "sstatus",    smode,
>>> read_sstatus,    write_sstatus, NULL,
>>>                                                  read_sstatus_i128
>>>                 },
>>> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
>>> index dc182ca811..ef418ac19d 100644
>>> --- a/target/riscv/machine.c
>>> +++ b/target/riscv/machine.c
>>> @@ -262,6 +262,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;
>>> @@ -372,6 +392,7 @@ const VMStateDescription vmstate_riscv_cpu = {
>>>            &vmstate_kvmtimer,
>>>            &vmstate_envcfg,
>>>            &vmstate_debug,
>>> +        &vmstate_smstateen,
>>>            NULL
>>>        }
>>>    };

Re: [PATCH v6 1/5] target/riscv: Add smstateen support

[Mayuresh Chitale]

On Mon, 2022-07-25 at 15:11 +0800, Weiwei Li wrote:
> 在 2022/7/24 下午11:39, Mayuresh Chitale 写道:
> > On Fri, 2022-07-22 at 08:31 +0800, Weiwei Li wrote:
> > > 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > > > 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 <mchitale@ventanamicro.com>
> > > > ---
> > > >    target/riscv/cpu.h      |   4 +
> > > >    target/riscv/cpu_bits.h |  37 ++++
> > > >    target/riscv/csr.c      | 370
> > > > ++++++++++++++++++++++++++++++++++++++++
> > > >    target/riscv/machine.c  |  21 +++
> > > >    4 files changed, 432 insertions(+)
> > > > 
> > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > index ffb1a18873..7f8e5b0014 100644
> > > > --- a/target/riscv/cpu.h
> > > > +++ b/target/riscv/cpu.h
> > > > @@ -354,6 +354,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
> > > > @@ -426,6 +429,7 @@ struct RISCVCPUConfig {
> > > >        bool ext_zkt;
> > > >        bool ext_ifencei;
> > > >        bool ext_icsr;
> > > > +    bool ext_smstateen;
> > > >        bool ext_svinval;
> > > >        bool ext_svnapot;
> > > >        bool ext_svpbmt;
> > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > > index 6be5a9e9f0..56b7c5bed6 100644
> > > > --- a/target/riscv/cpu_bits.h
> > > > +++ b/target/riscv/cpu_bits.h
> > > > @@ -199,6 +199,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
> > > > @@ -242,6 +248,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
> > > > @@ -283,6 +299,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_STATEN    (1ULL << 63)
> > > Maybe  SMSTATEEN_STATEEN better.
> > ok. Will update in the next version.
> > > > +
> > > >    /* Enhanced Physical Memory Protection (ePMP) */
> > > >    #define CSR_MSECCFG         0x747
> > > >    #define CSR_MSECCFGH        0x757
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index 235f2a011e..27032a416c 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > > > @@ -339,6 +339,68 @@ static RISCVException
> > > > hmode32(CPURISCVState
> > > > *env, int csrno)
> > > >    
> > > >    }
> > > >    
> > > > +static RISCVException mstateen(CPURISCVState *env, int csrno)
> > > > +{
> > > > +    CPUState *cs = env_cpu(env);
> > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +
> > > > +    if (!cpu->cfg.ext_smstateen) {
> > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > +    }
> > > > +
> > > > +    return any(env, csrno);
> > > > +}
> > > > +
> > > > +static RISCVException hstateen_pred(CPURISCVState *env, int
> > > > csrno,
> > > > int base)
> > > > +{
> > > > +    CPUState *cs = env_cpu(env);
> > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +
> > > > +    if (!cpu->cfg.ext_smstateen) {
> > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > +    }
> > > > +
> > > > +    if (!(env->mstateen[csrno - base] & SMSTATEEN_STATEN)) {
> > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > +    }
> > > > +
> > > > +    return hmode(env, csrno);
> > > > +}
> > > > +
> > > > +static RISCVException hstateen(CPURISCVState *env, int csrno)
> > > > +{
> > > > +    return hstateen_pred(env, csrno, CSR_HSTATEEN0);
> > > > +}
> > > > +
> > > > +static RISCVException hstateenh(CPURISCVState *env, int csrno)
> > > > +{
> > > > +    return hstateen_pred(env, csrno, CSR_HSTATEEN0H);
> > > > +}
> > > > +
> > > > +static RISCVException sstateen(CPURISCVState *env, int csrno)
> > > > +{
> > > > +    bool virt = riscv_cpu_virt_enabled(env);
> > > > +    int index = csrno - CSR_SSTATEEN0;
> > > > +    CPUState *cs = env_cpu(env);
> > > > +    RISCVCPU *cpu = RISCV_CPU(cs);
> > > > +
> > > > +    if (!cpu->cfg.ext_smstateen) {
> > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > +    }
> > > > +
> > > > +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
> > > > +        return RISCV_EXCP_ILLEGAL_INST;
> > > > +    }
> > > > +
> > > > +    if (virt) {
> > > > +        if (!(env->hstateen[index] & SMSTATEEN_STATEN)) {
> > > > +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return smode(env, csrno);
> > > > +}
> > > > +
> > > >    /* Checks if PointerMasking registers could be accessed */
> > > >    static RISCVException pointer_masking(CPURISCVState *env,
> > > > int
> > > > csrno)
> > > >    {
> > > > @@ -1699,6 +1761,263 @@ static RISCVException
> > > > write_henvcfgh(CPURISCVState *env, int csrno,
> > > >        return RISCV_EXCP_NONE;
> > > >    }
> > > >    
> > > > +static inline void write_smstateen(CPURISCVState *env,
> > > > uint64_t
> > > > *reg,
> > > > +                                   uint64_t wr_mask, uint64_t
> > > > new_val)
> > > > +{
> > > > +    *reg = (*reg & ~wr_mask) | (new_val & wr_mask);
> > > > +}
> > > > +
> > > > +static RISCVException read_mstateen(CPURISCVState *env, int
> > > > csrno,
> > > > +                                    target_ulong *val)
> > > > +{
> > > > +    *val = env->mstateen[csrno - CSR_MSTATEEN0];
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen(CPURISCVState *env, int
> > > > csrno,
> > > > +                                     uint64_t wr_mask,
> > > > target_ulong new_val)
> > > > +{
> > > > +    uint64_t *reg;
> > > > +
> > > > +    reg = &env->mstateen[csrno - CSR_MSTATEEN0];
> > > > +    write_smstateen(env, reg, wr_mask, new_val);
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen0(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > > > +
> > > > +    return write_mstateen(env, csrno, wr_mask, new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen1(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_mstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen2(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_mstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen3(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_mstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > Mstateen1~3  can share the same write function currently. The
> > > same
> > > to
> > > h/sstateen1~3.
> > I think its better to keep them separate as it will anyway be
> > required
> > once they are defined.
> 
> Only a few bits in mstateen0 is used currently. It may need a long
> time 
> before mstateen1~3 to be used.
> 
> And it's very easy to separate it out if required. So it seems 
> unnecessary to copy two times to prepare
> 
> for future use currently.
> 
I think its better to separate now and then modify the functions for
mstateen1-3 as and when required.
> Regards,
> 
> Weiwei Li
> 
> > > > +static RISCVException read_mstateenh(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong *val)
> > > > +{
> > > > +    *val = env->mstateen[csrno - CSR_MSTATEEN0H] >> 32;
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateenh(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      uint64_t wr_mask,
> > > > target_ulong new_val)
> > > > +{
> > > > +    uint64_t *reg, val;
> > > > +
> > > > +    reg = &env->mstateen[csrno - CSR_MSTATEEN0H];
> > > > +    val = (uint64_t)new_val << 32;
> > > > +    val |= *reg & 0xFFFFFFFF;
> > > > +    write_smstateen(env, reg, wr_mask, val);
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen0h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > > > +
> > > > +    return write_mstateenh(env, csrno, wr_mask, new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen1h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen2h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_mstateen3h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_mstateenh(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException read_hstateen(CPURISCVState *env, int
> > > > csrno,
> > > > +                                    target_ulong *val)
> > > > +{
> > > > +    int index = csrno - CSR_HSTATEEN0;
> > > > +
> > > > +    *val = env->hstateen[index] & env->mstateen[index];
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen(CPURISCVState *env, int
> > > > csrno,
> > > > +                                     uint64_t mask,
> > > > target_ulong
> > > > new_val)
> > > > +{
> > > > +    int index = csrno - CSR_HSTATEEN0;
> > > > +    uint64_t *reg, wr_mask;
> > > > +
> > > > +    reg = &env->hstateen[index];
> > > > +    wr_mask = env->mstateen[index] & mask;
> > > > +    write_smstateen(env, reg, wr_mask, new_val);
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen0(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > > > +
> > > > +    return write_hstateen(env, csrno, wr_mask, new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen1(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_hstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen2(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_hstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen3(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_hstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException read_hstateenh(CPURISCVState *env, int
> > > > csrno,
> > > > +                                     target_ulong *val)
> > > > +{
> > > > +    int index = csrno - CSR_HSTATEEN0H;
> > > > +
> > > > +    *val = (env->hstateen[index] >> 32) & (env-
> > > > >mstateen[index] >>
> > > > 32);
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateenh(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      uint64_t mask,
> > > > target_ulong
> > > > new_val)
> > > > +{
> > > > +    int index = csrno - CSR_HSTATEEN0H;
> > > > +    uint64_t *reg, wr_mask, val;
> > > > +
> > > > +    reg = &env->hstateen[index];
> > > > +    val = (uint64_t)new_val << 32;
> > > > +    val |= *reg & 0xFFFFFFFF;
> > > > +    wr_mask = env->mstateen[index] & mask;
> > > > +    write_smstateen(env, reg, wr_mask, val);
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen0h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                       target_ulong new_val)
> > > > +{
> > > > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > > > +
> > > > +    return write_hstateenh(env, csrno, wr_mask, new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen1h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                       target_ulong new_val)
> > > > +{
> > > > +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen2h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                       target_ulong new_val)
> > > > +{
> > > > +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_hstateen3h(CPURISCVState *env, int
> > > > csrno,
> > > > +                                       target_ulong new_val)
> > > > +{
> > > > +    return write_hstateenh(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException read_sstateen(CPURISCVState *env, int
> > > > csrno,
> > > > +                                    target_ulong *val)
> > > > +{
> > > > +    bool virt = riscv_cpu_virt_enabled(env);
> > > > +    int index = csrno - CSR_SSTATEEN0;
> > > > +
> > > > +    *val = env->sstateen[index] & env->mstateen[index];
> > > > +    if (virt) {
> > > > +        *val &= env->hstateen[index];
> > > > +    }
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_sstateen(CPURISCVState *env, int
> > > > csrno,
> > > > +                                     uint64_t mask,
> > > > target_ulong
> > > > new_val)
> > > > +{
> > > > +    bool virt = riscv_cpu_virt_enabled(env);
> > > > +    int index = csrno - CSR_SSTATEEN0;
> > > > +    uint64_t wr_mask;
> > > > +    uint64_t *reg;
> > > > +
> > > > +    wr_mask = env->mstateen[index] & mask;
> > > > +    if (virt) {
> > > > +        wr_mask &= env->hstateen[index];
> > > > +    }
> > > > +
> > > > +    reg = &env->sstateen[index];
> > > > +    write_smstateen(env, reg, wr_mask, new_val);
> > > > +
> > > > +    return RISCV_EXCP_NONE;
> > > > +}
> > > > +
> > > > +static RISCVException write_sstateen0(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    uint64_t wr_mask = SMSTATEEN_STATEN;
> > > > +
> > > > +    return write_sstateen(env, csrno, wr_mask, new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_sstateen1(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_sstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_sstateen2(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_sstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > > +static RISCVException write_sstateen3(CPURISCVState *env, int
> > > > csrno,
> > > > +                                      target_ulong new_val)
> > > > +{
> > > > +    return write_sstateen(env, csrno, SMSTATEEN_STATEN,
> > > > new_val);
> > > > +}
> > > > +
> > > >    static RISCVException rmw_mip64(CPURISCVState *env, int
> > > > csrno,
> > > >                                    uint64_t *ret_val,
> > > >                                    uint64_t new_val, uint64_t
> > > > wr_mask)
> > > > @@ -3558,6 +3877,57 @@ riscv_csr_operations
> > > > csr_ops[CSR_TABLE_SIZE]
> > > > = {
> > > >        [CSR_HENVCFGH] = { "henvcfgh", hmode32, read_henvcfgh,
> > > > write_henvcfgh,
> > > >                                              .min_priv_ver =
> > > > PRIV_VERSION_1_12_0 },
> > > >    
> > > > +    /* Smstateen extension CSRs */
> > > > +    [CSR_MSTATEEN0] = { "mstateen0", mstateen, read_mstateen,
> > > > write_mstateen0,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > The new lines have been updated to align with the last line in my
> > > previous patchset(accepted).
> > > 
> > > So it's better to make ' . ' align with ' " '.
> > ok. Will update in the next version.
> > > Regards,
> > > 
> > > Weiwei Li
> > > 
> > > > +    [CSR_MSTATEEN0H] = { "mstateen0h", mstateen,
> > > > read_mstateenh,
> > > > +                          write_mstateen0h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_MSTATEEN1] = { "mstateen1", mstateen, read_mstateen,
> > > > write_mstateen1,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_MSTATEEN1H] = { "mstateen1h", mstateen,
> > > > read_mstateenh,
> > > > +                          write_mstateen1h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_MSTATEEN2] = { "mstateen2", mstateen, read_mstateen,
> > > > write_mstateen2,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_MSTATEEN2H] = { "mstateen2h", mstateen,
> > > > read_mstateenh,
> > > > +                          write_mstateen2h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_MSTATEEN3] = { "mstateen3", mstateen, read_mstateen,
> > > > write_mstateen3,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_MSTATEEN3H] = { "mstateen3h", mstateen,
> > > > read_mstateenh,
> > > > +                          write_mstateen3h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +
> > > > +    [CSR_HSTATEEN0] = { "hstateen0", hstateen, read_hstateen,
> > > > write_hstateen0,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN0H] = { "hstateen0h", hstateenh,
> > > > read_hstateenh,
> > > > +                          write_hstateen0h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN1] = { "hstateen1", hstateen, read_hstateen,
> > > > write_hstateen1,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN1H] = { "hstateen1h", hstateenh,
> > > > read_hstateenh,
> > > > +                          write_hstateen1h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN2] = { "hstateen2", hstateen, read_hstateen,
> > > > write_hstateen2,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN2H] = { "hstateen2h", hstateenh,
> > > > read_hstateenh,
> > > > +                          write_hstateen2h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN3] = { "hstateen3", hstateen, read_hstateen,
> > > > write_hstateen3,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_HSTATEEN3H] = { "hstateen3h", hstateenh,
> > > > read_hstateenh,
> > > > +                          write_hstateen3h,
> > > > +                          .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +
> > > > +    [CSR_SSTATEEN0] = { "sstateen0", sstateen, read_sstateen,
> > > > write_sstateen0,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_SSTATEEN1] = { "sstateen1", sstateen, read_sstateen,
> > > > write_sstateen1,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_SSTATEEN2] = { "sstateen2", sstateen, read_sstateen,
> > > > write_sstateen2,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > > +    [CSR_SSTATEEN3] = { "sstateen3", sstateen, read_sstateen,
> > > > write_sstateen3,
> > > > +                         .min_priv_ver = PRIV_VERSION_1_12_0
> > > > },
> > > >        /* Supervisor Trap Setup */
> > > >        [CSR_SSTATUS]    = { "sstatus",    smode,
> > > > read_sstatus,    write_sstatus, NULL,
> > > >                                                  read_sstatus_i
> > > > 128
> > > >                 },
> > > > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > > index dc182ca811..ef418ac19d 100644
> > > > --- a/target/riscv/machine.c
> > > > +++ b/target/riscv/machine.c
> > > > @@ -262,6 +262,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;
> > > > @@ -372,6 +392,7 @@ const VMStateDescription vmstate_riscv_cpu
> > > > = {
> > > >            &vmstate_kvmtimer,
> > > >            &vmstate_envcfg,
> > > >            &vmstate_debug,
> > > > +        &vmstate_smstateen,
> > > >            NULL
> > > >        }
> > > >    };

[PATCH v6 2/5] target/riscv: smstateen check for h/senvcfg

[Mayuresh Chitale]

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

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/csr.c | 100 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 93 insertions(+), 7 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 27032a416c..ab06b117f9 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -40,6 +40,55 @@ 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);
+    uint64_t hstateen = env->hstateen[index];
+    uint64_t sstateen = env->sstateen[index];
+
+    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
+        return RISCV_EXCP_NONE;
+    }
+
+    if (!(env->mstateen[index] & bit)) {
+        return RISCV_EXCP_ILLEGAL_INST;
+    }
+
+    /*
+     * Treat hstateen and sstateen as read-only zero if mstateen0.staten
+     * is clear.
+     */
+    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
+        hstateen = 0;
+        sstateen = 0;
+    }
+
+    if (virt) {
+        if (!(hstateen & bit)) {
+            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
+        }
+        /*
+         * Treat sstateen as read-only zero if hstateen0.staten is clear.
+         */
+        if (!(hstateen & SMSTATEEN_STATEN)) {
+            sstateen = 0;
+        }
+    }
+
+    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
+        if (!(sstateen & bit)) {
+            return RISCV_EXCP_ILLEGAL_INST;
+        }
+    }
+
+    return RISCV_EXCP_NONE;
+}
+#endif
+
 static RISCVException fs(CPURISCVState *env, int csrno)
 {
 #if !defined(CONFIG_USER_ONLY)
@@ -1708,6 +1757,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;
 }
@@ -1716,15 +1772,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;
 }
@@ -1733,6 +1801,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;
@@ -1746,6 +1820,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;
 }
@@ -1755,9 +1836,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;
 }
 
@@ -1789,7 +1875,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_STATEN;
+    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
     return write_mstateen(env, csrno, wr_mask, new_val);
 }
@@ -1836,7 +1922,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_STATEN;
+    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
     return write_mstateenh(env, csrno, wr_mask, new_val);
 }
@@ -1885,7 +1971,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_STATEN;
+    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
     return write_hstateen(env, csrno, wr_mask, new_val);
 }
@@ -1936,7 +2022,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_STATEN;
+    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
     return write_hstateenh(env, csrno, wr_mask, new_val);
 }
@@ -1995,7 +2081,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_STATEN;
+    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
     return write_sstateen(env, csrno, wr_mask, new_val);
 }
-- 
2.25.1

Re: [PATCH v6 2/5] target/riscv: smstateen check for h/senvcfg

[Weiwei Li]

在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> Accesses to henvcfg, henvcfgh and senvcfg are allowed only if
> corresponding bit in mstateen0/hstateen0 is enabled. Otherwise an
> illegal instruction trap is generated.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/csr.c | 100 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 93 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 27032a416c..ab06b117f9 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -40,6 +40,55 @@ 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);
> +    uint64_t hstateen = env->hstateen[index];
> +    uint64_t sstateen = env->sstateen[index];
> +
> +    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (!(env->mstateen[index] & bit)) {
> +        return RISCV_EXCP_ILLEGAL_INST;
> +    }
> +
> +    /*
> +     * Treat hstateen and sstateen as read-only zero if mstateen0.staten
> +     * is clear.
> +     */
> +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
> +        hstateen = 0;
> +        sstateen = 0;
> +    }
> +
> +    if (virt) {
> +        if (!(hstateen & bit)) {
> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> +        }
> +        /*
> +         * Treat sstateen as read-only zero if hstateen0.staten is clear.
> +         */
> +        if (!(hstateen & SMSTATEEN_STATEN)) {
> +            sstateen = 0;
> +        }
> +    }
> +
> +    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> +        if (!(sstateen & bit)) {
> +            return RISCV_EXCP_ILLEGAL_INST;
> +        }
> +    }
> +
> +    return RISCV_EXCP_NONE;
> +}
> +#endif
> +
>   static RISCVException fs(CPURISCVState *env, int csrno)
>   {
>   #if !defined(CONFIG_USER_ONLY)
> @@ -1708,6 +1757,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);

I think it's better to add this check into the predicate.

By the way, sharing the same function for all related csrs  is easily 
misunderstood. However, It seems correct.

Regards,

Weiwei Li

> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       *val = env->senvcfg;
>       return RISCV_EXCP_NONE;
>   }
> @@ -1716,15 +1772,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;
>   }
> @@ -1733,6 +1801,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;
> @@ -1746,6 +1820,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;
>   }
> @@ -1755,9 +1836,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;
>   }
>   
> @@ -1789,7 +1875,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_STATEN;
> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
>       return write_mstateen(env, csrno, wr_mask, new_val);
>   }
> @@ -1836,7 +1922,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_STATEN;
> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
>       return write_mstateenh(env, csrno, wr_mask, new_val);
>   }
> @@ -1885,7 +1971,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_STATEN;
> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
>       return write_hstateen(env, csrno, wr_mask, new_val);
>   }
> @@ -1936,7 +2022,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_STATEN;
> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
>       return write_hstateenh(env, csrno, wr_mask, new_val);
>   }
> @@ -1995,7 +2081,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_STATEN;
> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
>       return write_sstateen(env, csrno, wr_mask, new_val);
>   }

Re: [PATCH v6 2/5] target/riscv: smstateen check for h/senvcfg

[Mayuresh Chitale]

On Fri, 2022-07-22 at 08:45 +0800, Weiwei Li wrote:
> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > Accesses to henvcfg, henvcfgh and senvcfg are allowed only if
> > corresponding bit in mstateen0/hstateen0 is enabled. Otherwise an
> > illegal instruction trap is generated.
> > 
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/csr.c | 100
> > +++++++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 93 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 27032a416c..ab06b117f9 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -40,6 +40,55 @@ 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);
> > +    uint64_t hstateen = env->hstateen[index];
> > +    uint64_t sstateen = env->sstateen[index];
> > +
> > +    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
> > +        return RISCV_EXCP_NONE;
> > +    }
> > +
> > +    if (!(env->mstateen[index] & bit)) {
> > +        return RISCV_EXCP_ILLEGAL_INST;
> > +    }
> > +
> > +    /*
> > +     * Treat hstateen and sstateen as read-only zero if
> > mstateen0.staten
> > +     * is clear.
> > +     */
> > +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
> > +        hstateen = 0;
> > +        sstateen = 0;
> > +    }
> > +
> > +    if (virt) {
> > +        if (!(hstateen & bit)) {
> > +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
> > +        }
> > +        /*
> > +         * Treat sstateen as read-only zero if hstateen0.staten is
> > clear.
> > +         */
> > +        if (!(hstateen & SMSTATEEN_STATEN)) {
> > +            sstateen = 0;
> > +        }
> > +    }
> > +
> > +    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
> > +        if (!(sstateen & bit)) {
> > +            return RISCV_EXCP_ILLEGAL_INST;
> > +        }
> > +    }
> > +
> > +    return RISCV_EXCP_NONE;
> > +}
> > +#endif
> > +
> >   static RISCVException fs(CPURISCVState *env, int csrno)
> >   {
> >   #if !defined(CONFIG_USER_ONLY)
> > @@ -1708,6 +1757,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);
> 
> I think it's better to add this check into the predicate.
> 
> By the way, sharing the same function for all related csrs  is
> easily 
> misunderstood. However, It seems correct.

We use the default global predicates ie hmode/smode etc for the envcfg
registers and the global predicates cant be modified to include
additional checks for envcfg registers. We could implement new
predicates for envcfg but I think the current approach is simpler.
> 
> Regards,
> 
> Weiwei Li
> 
> > +    if (ret != RISCV_EXCP_NONE) {
> > +        return ret;
> > +    }
> > +
> >       *val = env->senvcfg;
> >       return RISCV_EXCP_NONE;
> >   }
> > @@ -1716,15 +1772,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;
> >   }
> > @@ -1733,6 +1801,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;
> > @@ -1746,6 +1820,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;
> >   }
> > @@ -1755,9 +1836,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;
> >   }
> >   
> > @@ -1789,7 +1875,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_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_mstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1836,7 +1922,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_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_mstateenh(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1885,7 +1971,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_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_hstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1936,7 +2022,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_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_hstateenh(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1995,7 +2081,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_STATEN;
> > +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> >       return write_sstateen(env, csrno, wr_mask, new_val);
> >   }

Re: [PATCH v6 2/5] target/riscv: smstateen check for h/senvcfg

[Weiwei Li]

在 2022/7/28 下午2:41, Mayuresh Chitale 写道:
> On Fri, 2022-07-22 at 08:45 +0800, Weiwei Li wrote:
>> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
>>> Accesses to henvcfg, henvcfgh and senvcfg are allowed only if
>>> corresponding bit in mstateen0/hstateen0 is enabled. Otherwise an
>>> illegal instruction trap is generated.
>>>
>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>> ---
>>>    target/riscv/csr.c | 100
>>> +++++++++++++++++++++++++++++++++++++++++----
>>>    1 file changed, 93 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index 27032a416c..ab06b117f9 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -40,6 +40,55 @@ 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);
>>> +    uint64_t hstateen = env->hstateen[index];
>>> +    uint64_t sstateen = env->sstateen[index];
>>> +
>>> +    if (env->priv == PRV_M || !cpu->cfg.ext_smstateen) {
>>> +        return RISCV_EXCP_NONE;
>>> +    }
>>> +
>>> +    if (!(env->mstateen[index] & bit)) {
>>> +        return RISCV_EXCP_ILLEGAL_INST;
>>> +    }
>>> +
>>> +    /*
>>> +     * Treat hstateen and sstateen as read-only zero if
>>> mstateen0.staten
>>> +     * is clear.
>>> +     */
>>> +    if (!(env->mstateen[index] & SMSTATEEN_STATEN)) {
>>> +        hstateen = 0;
>>> +        sstateen = 0;
>>> +    }
>>> +
>>> +    if (virt) {
>>> +        if (!(hstateen & bit)) {
>>> +            return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>>> +        }
>>> +        /*
>>> +         * Treat sstateen as read-only zero if hstateen0.staten is
>>> clear.
>>> +         */
>>> +        if (!(hstateen & SMSTATEEN_STATEN)) {
>>> +            sstateen = 0;
>>> +        }
>>> +    }
>>> +
>>> +    if (env->priv == PRV_U && riscv_has_ext(env, RVS)) {
>>> +        if (!(sstateen & bit)) {
>>> +            return RISCV_EXCP_ILLEGAL_INST;
>>> +        }
>>> +    }
>>> +
>>> +    return RISCV_EXCP_NONE;
>>> +}
>>> +#endif
>>> +
>>>    static RISCVException fs(CPURISCVState *env, int csrno)
>>>    {
>>>    #if !defined(CONFIG_USER_ONLY)
>>> @@ -1708,6 +1757,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);
>> I think it's better to add this check into the predicate.
>>
>> By the way, sharing the same function for all related csrs  is
>> easily
>> misunderstood. However, It seems correct.
> We use the default global predicates ie hmode/smode etc for the envcfg
> registers and the global predicates cant be modified to include
> additional checks for envcfg registers. We could implement new
> predicates for envcfg but I think the current approach is simpler.

OK. It's acceptable to me.

Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,

Weiwei Li

>> Regards,
>>
>> Weiwei Li
>>
>>> +    if (ret != RISCV_EXCP_NONE) {
>>> +        return ret;
>>> +    }
>>> +
>>>        *val = env->senvcfg;
>>>        return RISCV_EXCP_NONE;
>>>    }
>>> @@ -1716,15 +1772,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;
>>>    }
>>> @@ -1733,6 +1801,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;
>>> @@ -1746,6 +1820,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;
>>>    }
>>> @@ -1755,9 +1836,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;
>>>    }
>>>    
>>> @@ -1789,7 +1875,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_STATEN;
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>>        return write_mstateen(env, csrno, wr_mask, new_val);
>>>    }
>>> @@ -1836,7 +1922,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_STATEN;
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>>        return write_mstateenh(env, csrno, wr_mask, new_val);
>>>    }
>>> @@ -1885,7 +1971,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_STATEN;
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>>        return write_hstateen(env, csrno, wr_mask, new_val);
>>>    }
>>> @@ -1936,7 +2022,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_STATEN;
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>>        return write_hstateenh(env, csrno, wr_mask, new_val);
>>>    }
>>> @@ -1995,7 +2081,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_STATEN;
>>> +    uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>>        return write_sstateen(env, csrno, wr_mask, new_val);
>>>    }

[PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

If smstateen is implemented and sstateen0.fcsr is clear then the
floating point operations must return illegal instruction exception.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/csr.c                        | 23 ++++++++++++++
 target/riscv/insn_trans/trans_rvf.c.inc   | 38 +++++++++++++++++++++--
 target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab06b117f9..a597b6cbc7 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -96,6 +96,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;
 }
@@ -1876,6 +1880,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
                                       target_ulong new_val)
 {
     uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
 
     return write_mstateen(env, csrno, wr_mask, new_val);
 }
@@ -1924,6 +1931,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_mstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -1973,6 +1984,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateen(env, csrno, wr_mask, new_val);
 }
 
@@ -2024,6 +2039,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
 
+    if (!riscv_has_ext(env, RVF)) {
+        wr_mask |= SMSTATEEN0_FCSR;
+    }
+
     return write_hstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -2083,6 +2102,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
 {
     uint64_t wr_mask = SMSTATEEN_STATEN | 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.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index a1d3eb52ad..c43c48336b 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -24,9 +24,43 @@
             return false; \
 } while (0)
 
+#ifndef CONFIG_USER_ONLY
+#define SMSTATEEN_CHECK(ctx) do {\
+    CPUState *cpu = ctx->cs; \
+    CPURISCVState *env = cpu->env_ptr; \
+    if (ctx->cfg_ptr->ext_smstateen && \
+        (env->priv < PRV_M)) { \
+        uint64_t stateen = env->mstateen[0]; \
+        uint64_t hstateen = env->hstateen[0]; \
+        uint64_t sstateen = env->sstateen[0]; \
+        if (!(stateen & SMSTATEEN_STATEN)) {\
+            hstateen = 0; \
+            sstateen = 0; \
+        } \
+        if (ctx->virt_enabled) { \
+            stateen &= hstateen; \
+            if (!(hstateen & SMSTATEEN_STATEN)) {\
+                sstateen = 0; \
+            } \
+        } \
+        if (env->priv == PRV_U && has_ext(ctx, RVS)) {\
+            stateen &= sstateen; \
+        } \
+        if (!(stateen & SMSTATEEN0_FCSR)) { \
+            return false; \
+        } \
+    } \
+} while (0)
+#else
+#define SMSTATEEN_CHECK(ctx)
+#endif
+
 #define REQUIRE_ZFINX_OR_F(ctx) do {\
-    if (!ctx->cfg_ptr->ext_zfinx) { \
-        REQUIRE_EXT(ctx, RVF); \
+    if (!has_ext(ctx, RVF)) { \
+        SMSTATEEN_CHECK(ctx); \
+        if (!ctx->cfg_ptr->ext_zfinx) { \
+            return false; \
+        } \
     } \
 } while (0)
 
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
index 5d07150cd0..b165ea9d58 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -17,24 +17,28 @@
  */
 
 #define REQUIRE_ZFH(ctx) do { \
+    SMSTATEEN_CHECK(ctx); \
     if (!ctx->cfg_ptr->ext_zfh) {      \
         return false;         \
     }                         \
 } while (0)
 
 #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
+    SMSTATEEN_CHECK(ctx); \
     if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
         return false;                  \
     }                                  \
 } while (0)
 
 #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
+    SMSTATEEN_CHECK(ctx); \
     if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
         return false;                         \
     }                                         \
 } while (0)
 
 #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \
+    SMSTATEEN_CHECK(ctx); \
     if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin ||          \
           ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin)) {     \
         return false;                                        \
-- 
2.25.1

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Weiwei Li]

在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> If smstateen is implemented and sstateen0.fcsr is clear then the
> floating point operations must return illegal instruction exception.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/csr.c                        | 23 ++++++++++++++
>   target/riscv/insn_trans/trans_rvf.c.inc   | 38 +++++++++++++++++++++--
>   target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
>   3 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index ab06b117f9..a597b6cbc7 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -96,6 +96,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;
>   }
> @@ -1876,6 +1880,9 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
>                                         target_ulong new_val)
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
>   
>       return write_mstateen(env, csrno, wr_mask, new_val);
>   }
> @@ -1924,6 +1931,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_mstateenh(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -1973,6 +1984,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_hstateen(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2024,6 +2039,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>   
> +    if (!riscv_has_ext(env, RVF)) {
> +        wr_mask |= SMSTATEEN0_FCSR;
> +    }
> +
>       return write_hstateenh(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2083,6 +2102,10 @@ static RISCVException write_sstateen0(CPURISCVState *env, int csrno,
>   {
>       uint64_t wr_mask = SMSTATEEN_STATEN | 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.inc b/target/riscv/insn_trans/trans_rvf.c.inc
> index a1d3eb52ad..c43c48336b 100644
> --- a/target/riscv/insn_trans/trans_rvf.c.inc
> +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> @@ -24,9 +24,43 @@
>               return false; \
>   } while (0)
>   
> +#ifndef CONFIG_USER_ONLY
> +#define SMSTATEEN_CHECK(ctx) do {\
> +    CPUState *cpu = ctx->cs; \
> +    CPURISCVState *env = cpu->env_ptr; \
> +    if (ctx->cfg_ptr->ext_smstateen && \
> +        (env->priv < PRV_M)) { \
> +        uint64_t stateen = env->mstateen[0]; \
> +        uint64_t hstateen = env->hstateen[0]; \
> +        uint64_t sstateen = env->sstateen[0]; \
> +        if (!(stateen & SMSTATEEN_STATEN)) {\
> +            hstateen = 0; \
> +            sstateen = 0; \
> +        } \
> +        if (ctx->virt_enabled) { \
> +            stateen &= hstateen; \
> +            if (!(hstateen & SMSTATEEN_STATEN)) {\
> +                sstateen = 0; \
> +            } \
> +        } \
> +        if (env->priv == PRV_U && has_ext(ctx, RVS)) {\
> +            stateen &= sstateen; \
> +        } \
> +        if (!(stateen & SMSTATEEN0_FCSR)) { \
> +            return false; \
> +        } \
> +    } \
> +} while (0)

It's better to add a space before '\'.

> +#else
> +#define SMSTATEEN_CHECK(ctx)
> +#endif
> +
>   #define REQUIRE_ZFINX_OR_F(ctx) do {\
> -    if (!ctx->cfg_ptr->ext_zfinx) { \
> -        REQUIRE_EXT(ctx, RVF); \
> +    if (!has_ext(ctx, RVF)) { \
> +        SMSTATEEN_CHECK(ctx); \
> +        if (!ctx->cfg_ptr->ext_zfinx) { \
> +            return false; \
> +        } \
>       } \
>   } while (0)

SMSTATEEN_CHECK is for CSR. and REQUIRE_ZFINX_OR_F is for Extension.
I think It's better to separate them. By the way, if we want the smallest modification
for current code, adding it to REQUIRE_FPU seems better.
Regards,
Weiwei Li

>   
> diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
> index 5d07150cd0..b165ea9d58 100644
> --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> @@ -17,24 +17,28 @@
>    */
>   
>   #define REQUIRE_ZFH(ctx) do { \
> +    SMSTATEEN_CHECK(ctx); \
>       if (!ctx->cfg_ptr->ext_zfh) {      \
>           return false;         \
>       }                         \
>   } while (0)
>   
>   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
> +    SMSTATEEN_CHECK(ctx); \
>       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
>           return false;                  \
>       }                                  \
>   } while (0)
>   
>   #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
> +    SMSTATEEN_CHECK(ctx); \
>       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
>           return false;                         \
>       }                                         \
>   } while (0)
>   
>   #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \
> +    SMSTATEEN_CHECK(ctx); \
>       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin ||          \
>             ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin)) {     \
>           return false;                                        \

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:
> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > If smstateen is implemented and sstateen0.fcsr is clear then the
> > floating point operations must return illegal instruction
> > exception.
> > 
> > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > ---
> >   target/riscv/csr.c                        | 23 ++++++++++++++
> >   target/riscv/insn_trans/trans_rvf.c.inc   | 38
> > +++++++++++++++++++++--
> >   target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
> >   3 files changed, 63 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index ab06b117f9..a597b6cbc7 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -96,6 +96,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;
> >   }
> > @@ -1876,6 +1880,9 @@ static RISCVException
> > write_mstateen0(CPURISCVState *env, int csrno,
> >                                         target_ulong new_val)
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> >   
> >       return write_mstateen(env, csrno, wr_mask, new_val);
> >   }
> > @@ -1924,6 +1931,10 @@ static RISCVException
> > write_mstateen0h(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_mstateenh(env, csrno, wr_mask, new_val);
> >   }
> >   
> > @@ -1973,6 +1984,10 @@ static RISCVException
> > write_hstateen0(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_hstateen(env, csrno, wr_mask, new_val);
> >   }
> >   
> > @@ -2024,6 +2039,10 @@ static RISCVException
> > write_hstateen0h(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
> >   
> > +    if (!riscv_has_ext(env, RVF)) {
> > +        wr_mask |= SMSTATEEN0_FCSR;
> > +    }
> > +
> >       return write_hstateenh(env, csrno, wr_mask, new_val);
> >   }
> >   
> > @@ -2083,6 +2102,10 @@ static RISCVException
> > write_sstateen0(CPURISCVState *env, int csrno,
> >   {
> >       uint64_t wr_mask = SMSTATEEN_STATEN | 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.inc
> > b/target/riscv/insn_trans/trans_rvf.c.inc
> > index a1d3eb52ad..c43c48336b 100644
> > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > @@ -24,9 +24,43 @@
> >               return false; \
> >   } while (0)
> >   
> > +#ifndef CONFIG_USER_ONLY
> > +#define SMSTATEEN_CHECK(ctx) do {\
> > +    CPUState *cpu = ctx->cs; \
> > +    CPURISCVState *env = cpu->env_ptr; \
> > +    if (ctx->cfg_ptr->ext_smstateen && \
> > +        (env->priv < PRV_M)) { \
> > +        uint64_t stateen = env->mstateen[0]; \
> > +        uint64_t hstateen = env->hstateen[0]; \
> > +        uint64_t sstateen = env->sstateen[0]; \
> > +        if (!(stateen & SMSTATEEN_STATEN)) {\
> > +            hstateen = 0; \
> > +            sstateen = 0; \
> > +        } \
> > +        if (ctx->virt_enabled) { \
> > +            stateen &= hstateen; \
> > +            if (!(hstateen & SMSTATEEN_STATEN)) {\
> > +                sstateen = 0; \
> > +            } \
> > +        } \
> > +        if (env->priv == PRV_U && has_ext(ctx, RVS)) {\eventually
> > meaning
> > +            stateen &= sstateen; \
> > +        } \
> > +        if (!(stateen & SMSTATEEN0_FCSR)) { \
> > +            return false; \
> > +        } \
> > +    } \
> > +} while (0)
> 
> It's better to add a space before '\'.
ok. will modify in the next version.
> 
> > +#else
> > +#define SMSTATEEN_CHECK(ctx)
> > +#endif
> > +
> >   #define REQUIRE_ZFINX_OR_F(ctx) do {\
> > -    if (!ctx->cfg_ptr->ext_zfinx) { \
> > -        REQUIRE_EXT(ctx, RVF); \
> > +    if (!has_ext(ctx, RVF)) { \
> > +        SMSTATEEN_CHECK(ctx); \
> > +        if (!ctx->cfg_ptr->ext_zfinx) { \
> > +            return false; \
> > +        } \
> >       } \
> >   } while (0)
> 
> SMSTATEEN_CHECK is for CSR. and REQUIRE_ZFINX_OR_F is for Extension.
> I think It's better to separate them. By the way, if we want the
> smallest modification
> for current code, adding it to REQUIRE_FPU seems better.
Actually REQUIRE_FPU is checking for mstatus.fs but as per smstateen
spec we need to check for misa.f which is done in REQUIRE_ZFINX_OR_F.
> Regards,
> Weiwei Li
> 
> >   
> > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > index 5d07150cd0..b165ea9d58 100644
> > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > @@ -17,24 +17,28 @@
> >    */
> >   
> >   #define REQUIRE_ZFH(ctx) do { \
> > +    SMSTATEEN_CHECK(ctx); \
> >       if (!ctx->cfg_ptr->ext_zfh) {      \
> >           return false;         \
> >       }                         \
> >   } while (0)
> >   
> >   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
> > +    SMSTATEEN_CHECK(ctx); \
> >       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
> >           return false;                  \
> >       }                                  \
> >   } while (0)
> >   
> >   #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
> > +    SMSTATEEN_CHECK(ctx); \
> >       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
> >           return false;                         \
> >       }                                         \
> >   } while (0)
> >   
> >   #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \
> > +    SMSTATEEN_CHECK(ctx); \
> >       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin
> > ||          \
> >             ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin)) 
> > {     \
> >           return false;                                        \

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Weiwei Li]

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


在 2022/7/24 下午11:49, Mayuresh Chitale 写道:
> On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:
>> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
>>> If smstateen is implemented and sstateen0.fcsr is clear then the
>>> floating point operations must return illegal instruction
>>> exception.
>>>
>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>> ---
>>>    target/riscv/csr.c                        | 23 ++++++++++++++
>>>    target/riscv/insn_trans/trans_rvf.c.inc   | 38
>>> +++++++++++++++++++++--
>>>    target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
>>>    3 files changed, 63 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index ab06b117f9..a597b6cbc7 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -96,6 +96,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;
>>>    }
>>> @@ -1876,6 +1880,9 @@ static RISCVException
>>> write_mstateen0(CPURISCVState *env, int csrno,
>>>                                          target_ulong new_val)
>>>    {
>>>        uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>> +    if (!riscv_has_ext(env, RVF)) {
>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>> +    }
>>>    
>>>        return write_mstateen(env, csrno, wr_mask, new_val);
>>>    }
>>> @@ -1924,6 +1931,10 @@ static RISCVException
>>> write_mstateen0h(CPURISCVState *env, int csrno,
>>>    {
>>>        uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>> +    if (!riscv_has_ext(env, RVF)) {
>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>> +    }
>>> +
>>>        return write_mstateenh(env, csrno, wr_mask, new_val);
>>>    }
>>>    
>>> @@ -1973,6 +1984,10 @@ static RISCVException
>>> write_hstateen0(CPURISCVState *env, int csrno,
>>>    {
>>>        uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>> +    if (!riscv_has_ext(env, RVF)) {
>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>> +    }
>>> +
>>>        return write_hstateen(env, csrno, wr_mask, new_val);
>>>    }
>>>    
>>> @@ -2024,6 +2039,10 @@ static RISCVException
>>> write_hstateen0h(CPURISCVState *env, int csrno,
>>>    {
>>>        uint64_t wr_mask = SMSTATEEN_STATEN | SMSTATEEN0_HSENVCFG;
>>>    
>>> +    if (!riscv_has_ext(env, RVF)) {
>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>> +    }
>>> +
>>>        return write_hstateenh(env, csrno, wr_mask, new_val);
>>>    }
>>>    
>>> @@ -2083,6 +2102,10 @@ static RISCVException
>>> write_sstateen0(CPURISCVState *env, int csrno,
>>>    {
>>>        uint64_t wr_mask = SMSTATEEN_STATEN | 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.inc
>>> b/target/riscv/insn_trans/trans_rvf.c.inc
>>> index a1d3eb52ad..c43c48336b 100644
>>> --- a/target/riscv/insn_trans/trans_rvf.c.inc
>>> +++ b/target/riscv/insn_trans/trans_rvf.c.inc
>>> @@ -24,9 +24,43 @@
>>>                return false; \
>>>    } while (0)
>>>    
>>> +#ifndef CONFIG_USER_ONLY
>>> +#define SMSTATEEN_CHECK(ctx) do {\
>>> +    CPUState *cpu = ctx->cs; \
>>> +    CPURISCVState *env = cpu->env_ptr; \
>>> +    if (ctx->cfg_ptr->ext_smstateen && \
>>> +        (env->priv < PRV_M)) { \
>>> +        uint64_t stateen = env->mstateen[0]; \
>>> +        uint64_t hstateen = env->hstateen[0]; \
>>> +        uint64_t sstateen = env->sstateen[0]; \
>>> +        if (!(stateen & SMSTATEEN_STATEN)) {\
>>> +            hstateen = 0; \
>>> +            sstateen = 0; \
>>> +        } \
>>> +        if (ctx->virt_enabled) { \
>>> +            stateen &= hstateen; \
>>> +            if (!(hstateen & SMSTATEEN_STATEN)) {\
>>> +                sstateen = 0; \
>>> +            } \
>>> +        } \
>>> +        if (env->priv == PRV_U && has_ext(ctx, RVS)) {\eventually
>>> meaning
>>> +            stateen &= sstateen; \
>>> +        } \
>>> +        if (!(stateen & SMSTATEEN0_FCSR)) { \
>>> +            return false; \
>>> +        } \
>>> +    } \
>>> +} while (0)
>> It's better to add a space before '\'.
> ok. will modify in the next version.
>>> +#else
>>> +#define SMSTATEEN_CHECK(ctx)
>>> +#endif
>>> +
>>>    #define REQUIRE_ZFINX_OR_F(ctx) do {\
>>> -    if (!ctx->cfg_ptr->ext_zfinx) { \
>>> -        REQUIRE_EXT(ctx, RVF); \
>>> +    if (!has_ext(ctx, RVF)) { \
>>> +        SMSTATEEN_CHECK(ctx); \
>>> +        if (!ctx->cfg_ptr->ext_zfinx) { \
>>> +            return false; \
>>> +        } \
>>>        } \
>>>    } while (0)
>> SMSTATEEN_CHECK is for CSR. and REQUIRE_ZFINX_OR_F is for Extension.
>> I think It's better to separate them. By the way, if we want the
>> smallest modification
>> for current code, adding it to REQUIRE_FPU seems better.
> Actually REQUIRE_FPU is checking for mstatus.fs but as per smstateen
> spec we need to check for misa.f which is done in REQUIRE_ZFINX_OR_F.

OK. It's acceptable to me  even though I prefer separating them.

However, I find another question in SMSTATEEN_CHECK: when access is 
disallowed by Xstateen.FCSR,

it's always return false  which will trigger illegal instruction 
exception finally.

However, this exception is triggered by accessing fcsr CSR which may 
trigger illegal instruction trap and virtual
instruction trap in different situation.

/"For convenience, when the stateen CSRs are implemented and misa.F = 0, 
then if bit 1 of a//
//controlling stateen0 CSR is zero, all floating-point instructions 
cause an illegal instruction trap (or virtual//
//instruction trap, if relevant), as though they all access fcsr, 
regardless of whether they really do."/

And  stateen.fcsr is only work when zfinx is enabled, so  it's better to 
SMSTATEEN_CHECK(ctx) after check for

"!ctx->cfg_ptr->ext_zfinx"

Regards,
Weiwei Li

>> Regards,
>> Weiwei Li
>>
>>>    
>>> diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc
>>> b/target/riscv/insn_trans/trans_rvzfh.c.inc
>>> index 5d07150cd0..b165ea9d58 100644
>>> --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
>>> +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
>>> @@ -17,24 +17,28 @@
>>>     */
>>>    
>>>    #define REQUIRE_ZFH(ctx) do { \
>>> +    SMSTATEEN_CHECK(ctx); \
>>>        if (!ctx->cfg_ptr->ext_zfh) {      \
>>>            return false;         \
>>>        }                         \
>>>    } while (0)
>>>    
>>>    #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
>>> +    SMSTATEEN_CHECK(ctx); \
>>>        if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) { \
>>>            return false;                  \
>>>        }                                  \
>>>    } while (0)
>>>    
>>>    #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
>>> +    SMSTATEEN_CHECK(ctx); \
>>>        if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) { \
>>>            return false;                         \
>>>        }                                         \
>>>    } while (0)
>>>    
>>>    #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do { \
>>> +    SMSTATEEN_CHECK(ctx); \
>>>        if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin
>>> ||          \
>>>              ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr->ext_zhinxmin))
>>> {     \
>>>            return false;                                        \

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

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

On Mon, 2022-07-25 at 15:23 +0800, Weiwei Li wrote:
> 
> 在 2022/7/24 下午11:49, Mayuresh Chitale 写道:
> > On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:
> > > 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > > > If smstateen is implemented and sstateen0.fcsr is clear then
> > > > the
> > > > floating point operations must return illegal instruction
> > > > exception.
> > > > 
> > > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > > > ---
> > > >   target/riscv/csr.c                        | 23 ++++++++++++++
> > > >   target/riscv/insn_trans/trans_rvf.c.inc   | 38
> > > > +++++++++++++++++++++--
> > > >   target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
> > > >   3 files changed, 63 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > index ab06b117f9..a597b6cbc7 100644
> > > > --- a/target/riscv/csr.c
> > > > +++ b/target/riscv/csr.c
> > > > @@ -96,6 +96,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;
> > > >   }
> > > > @@ -1876,6 +1880,9 @@ static RISCVException
> > > > write_mstateen0(CPURISCVState *env, int csrno,
> > > >                                         target_ulong new_val)
> > > >   {
> > > >       uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > SMSTATEEN0_HSENVCFG;
> > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > +    }
> > > >   
> > > >       return write_mstateen(env, csrno, wr_mask, new_val);
> > > >   }
> > > > @@ -1924,6 +1931,10 @@ static RISCVException
> > > > write_mstateen0h(CPURISCVState *env, int csrno,
> > > >   {
> > > >       uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > SMSTATEEN0_HSENVCFG;
> > > >   
> > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > +    }
> > > > +
> > > >       return write_mstateenh(env, csrno, wr_mask, new_val);
> > > >   }
> > > >   
> > > > @@ -1973,6 +1984,10 @@ static RISCVException
> > > > write_hstateen0(CPURISCVState *env, int csrno,
> > > >   {
> > > >       uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > SMSTATEEN0_HSENVCFG;
> > > >   
> > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > +    }
> > > > +
> > > >       return write_hstateen(env, csrno, wr_mask, new_val);
> > > >   }
> > > >   
> > > > @@ -2024,6 +2039,10 @@ static RISCVException
> > > > write_hstateen0h(CPURISCVState *env, int csrno,
> > > >   {
> > > >       uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > SMSTATEEN0_HSENVCFG;
> > > >   
> > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > +    }
> > > > +
> > > >       return write_hstateenh(env, csrno, wr_mask, new_val);
> > > >   }
> > > >   
> > > > @@ -2083,6 +2102,10 @@ static RISCVException
> > > > write_sstateen0(CPURISCVState *env, int csrno,
> > > >   {
> > > >       uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > 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.inc
> > > > b/target/riscv/insn_trans/trans_rvf.c.inc
> > > > index a1d3eb52ad..c43c48336b 100644
> > > > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > > > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > > > @@ -24,9 +24,43 @@
> > > >               return false; \
> > > >   } while (0)
> > > >   
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +#define SMSTATEEN_CHECK(ctx) do {\
> > > > +    CPUState *cpu = ctx->cs; \
> > > > +    CPURISCVState *env = cpu->env_ptr; \
> > > > +    if (ctx->cfg_ptr->ext_smstateen && \
> > > > +        (env->priv < PRV_M)) { \
> > > > +        uint64_t stateen = env->mstateen[0]; \
> > > > +        uint64_t hstateen = env->hstateen[0]; \
> > > > +        uint64_t sstateen = env->sstateen[0]; \
> > > > +        if (!(stateen & SMSTATEEN_STATEN)) {\
> > > > +            hstateen = 0; \
> > > > +            sstateen = 0; \
> > > > +        } \
> > > > +        if (ctx->virt_enabled) { \
> > > > +            stateen &= hstateen; \
> > > > +            if (!(hstateen & SMSTATEEN_STATEN)) {\
> > > > +                sstateen = 0; \
> > > > +            } \
> > > > +        } \
> > > > +        if (env->priv == PRV_U && has_ext(ctx, RVS))
> > > > {\eventually
> > > > meaning
> > > > +            stateen &= sstateen; \
> > > > +        } \
> > > > +        if (!(stateen & SMSTATEEN0_FCSR)) { \
> > > > +            return false; \
> > > > +        } \
> > > > +    } \
> > > > +} while (0)
> > > 
> > > It's better to add a space before '\'.
> > 
> > ok. will modify in the next version.
> > > > +#else
> > > > +#define SMSTATEEN_CHECK(ctx)
> > > > +#endif
> > > > +
> > > >   #define REQUIRE_ZFINX_OR_F(ctx) do {\
> > > > -    if (!ctx->cfg_ptr->ext_zfinx) { \
> > > > -        REQUIRE_EXT(ctx, RVF); \
> > > > +    if (!has_ext(ctx, RVF)) { \
> > > > +        SMSTATEEN_CHECK(ctx); \
> > > > +        if (!ctx->cfg_ptr->ext_zfinx) { \
> > > > +            return false; \
> > > > +        } \
> > > >       } \
> > > >   } while (0)
> > > 
> > > SMSTATEEN_CHECK is for CSR. and REQUIRE_ZFINX_OR_F is for
> > > Extension.
> > > I think It's better to separate them. By the way, if we want the
> > > smallest modification
> > > for current code, adding it to REQUIRE_FPU seems better.
> > 
> > Actually REQUIRE_FPU is checking for mstatus.fs but as per
> > smstateen
> > spec we need to check for misa.f which is done in
> > REQUIRE_ZFINX_OR_F.
> 
> OK. It's acceptable to me  even though I prefer separating them.
> 
> However, I find another question in SMSTATEEN_CHECK: when access is
> disallowed by Xstateen.FCSR,  
> 
> it's always return false  which will trigger illegal instruction
> exception finally.
> 
> However, this exception is triggered by accessing fcsr CSR which may
> trigger illegal instruction trap and virtual
> instruction trap in different situation.
> 
> "For convenience, when the stateen CSRs are implemented and misa.F =
> 0, then if bit 1 of a
> controlling stateen0 CSR is zero, all floating-point instructions
> cause an illegal instruction trap (or virtual
> instruction trap, if relevant), as though they all access fcsr,
> regardless of whether they really do."
> 
> And  stateen.fcsr is only work when zfinx is enabled, so  it's better
> to SMSTATEEN_CHECK(ctx) after check for 
> 
> "!ctx->cfg_ptr->ext_zfinx"

Actually the spec refers only to misa.F and not zfinx and regarding the
fcsr its :
"as though they all access fcsr, regardless of whether they really do"

> Regards,
> Weiwei Li
> > > Regards,
> > > Weiwei Li
> > > 
> > > >   
> > > > diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > > > b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > > > index 5d07150cd0..b165ea9d58 100644
> > > > --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
> > > > +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
> > > > @@ -17,24 +17,28 @@
> > > >    */
> > > >   
> > > >   #define REQUIRE_ZFH(ctx) do { \
> > > > +    SMSTATEEN_CHECK(ctx); \
> > > >       if (!ctx->cfg_ptr->ext_zfh) {      \
> > > >           return false;         \
> > > >       }                         \
> > > >   } while (0)
> > > >   
> > > >   #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
> > > > +    SMSTATEEN_CHECK(ctx); \
> > > >       if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) {
> > > > \
> > > >           return false;                  \
> > > >       }                                  \
> > > >   } while (0)
> > > >   
> > > >   #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
> > > > +    SMSTATEEN_CHECK(ctx); \
> > > >       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin)) 
> > > > { \
> > > >           return false;                         \
> > > >       }                                         \
> > > >   } while (0)
> > > >   
> > > >   #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do {
> > > > \
> > > > +    SMSTATEEN_CHECK(ctx); \
> > > >       if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin
> > > > ||          \
> > > >             ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr-
> > > > >ext_zhinxmin)) 
> > > > {     \
> > > >           return
> > > > false;                                        \

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Weiwei Li]

在 2022/7/28 下午2:15, Mayuresh Chitale 写道:
> On Mon, 2022-07-25 at 15:23 +0800, Weiwei Li wrote:
>> 在 2022/7/24 下午11:49, Mayuresh Chitale 写道:
>>> On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:
>>>> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
>>>>> If smstateen is implemented and sstateen0.fcsr is clear then
>>>>> the
>>>>> floating point operations must return illegal instruction
>>>>> exception.
>>>>>
>>>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>>>> ---
>>>>>    target/riscv/csr.c                        | 23 ++++++++++++++
>>>>>    target/riscv/insn_trans/trans_rvf.c.inc   | 38
>>>>> +++++++++++++++++++++--
>>>>>    target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
>>>>>    3 files changed, 63 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>> index ab06b117f9..a597b6cbc7 100644
>>>>> --- a/target/riscv/csr.c
>>>>> +++ b/target/riscv/csr.c
>>>>> @@ -96,6 +96,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;
>>>>>    }
>>>>> @@ -1876,6 +1880,9 @@ static RISCVException
>>>>> write_mstateen0(CPURISCVState *env, int csrno,
>>>>>                                          target_ulong new_val)
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>>    
>>>>>        return write_mstateen(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>> @@ -1924,6 +1931,10 @@ static RISCVException
>>>>> write_mstateen0h(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>>    
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>> +
>>>>>        return write_mstateenh(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>>    
>>>>> @@ -1973,6 +1984,10 @@ static RISCVException
>>>>> write_hstateen0(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>>    
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>> +
>>>>>        return write_hstateen(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>>    
>>>>> @@ -2024,6 +2039,10 @@ static RISCVException
>>>>> write_hstateen0h(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>>    
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>> +
>>>>>        return write_hstateenh(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>>    
>>>>> @@ -2083,6 +2102,10 @@ static RISCVException
>>>>> write_sstateen0(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> 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.inc
>>>>> b/target/riscv/insn_trans/trans_rvf.c.inc
>>>>> index a1d3eb52ad..c43c48336b 100644
>>>>> --- a/target/riscv/insn_trans/trans_rvf.c.inc
>>>>> +++ b/target/riscv/insn_trans/trans_rvf.c.inc
>>>>> @@ -24,9 +24,43 @@
>>>>>                return false; \
>>>>>    } while (0)
>>>>>    
>>>>> +#ifndef CONFIG_USER_ONLY
>>>>> +#define SMSTATEEN_CHECK(ctx) do {\
>>>>> +    CPUState *cpu = ctx->cs; \
>>>>> +    CPURISCVState *env = cpu->env_ptr; \
>>>>> +    if (ctx->cfg_ptr->ext_smstateen && \
>>>>> +        (env->priv < PRV_M)) { \
>>>>> +        uint64_t stateen = env->mstateen[0]; \
>>>>> +        uint64_t hstateen = env->hstateen[0]; \
>>>>> +        uint64_t sstateen = env->sstateen[0]; \
>>>>> +        if (!(stateen & SMSTATEEN_STATEN)) {\
>>>>> +            hstateen = 0; \
>>>>> +            sstateen = 0; \
>>>>> +        } \
>>>>> +        if (ctx->virt_enabled) { \
>>>>> +            stateen &= hstateen; \
>>>>> +            if (!(hstateen & SMSTATEEN_STATEN)) {\
>>>>> +                sstateen = 0; \
>>>>> +            } \
>>>>> +        } \
>>>>> +        if (env->priv == PRV_U && has_ext(ctx, RVS))
>>>>> {\eventually
>>>>> meaning
>>>>> +            stateen &= sstateen; \
>>>>> +        } \
>>>>> +        if (!(stateen & SMSTATEEN0_FCSR)) { \
>>>>> +            return false; \
>>>>> +        } \
>>>>> +    } \
>>>>> +} while (0)
>>>> It's better to add a space before '\'.
>>> ok. will modify in the next version.
>>>>> +#else
>>>>> +#define SMSTATEEN_CHECK(ctx)
>>>>> +#endif
>>>>> +
>>>>>    #define REQUIRE_ZFINX_OR_F(ctx) do {\
>>>>> -    if (!ctx->cfg_ptr->ext_zfinx) { \
>>>>> -        REQUIRE_EXT(ctx, RVF); \
>>>>> +    if (!has_ext(ctx, RVF)) { \
>>>>> +        SMSTATEEN_CHECK(ctx); \
>>>>> +        if (!ctx->cfg_ptr->ext_zfinx) { \
>>>>> +            return false; \
>>>>> +        } \
>>>>>        } \
>>>>>    } while (0)
>>>> SMSTATEEN_CHECK is for CSR. and REQUIRE_ZFINX_OR_F is for
>>>> Extension.
>>>> I think It's better to separate them. By the way, if we want the
>>>> smallest modification
>>>> for current code, adding it to REQUIRE_FPU seems better.
>>> Actually REQUIRE_FPU is checking for mstatus.fs but as per
>>> smstateen
>>> spec we need to check for misa.f which is done in
>>> REQUIRE_ZFINX_OR_F.
>> OK. It's acceptable to me  even though I prefer separating them.
>>
>> However, I find another question in SMSTATEEN_CHECK: when access is
>> disallowed by Xstateen.FCSR,
>>
>> it's always return false  which will trigger illegal instruction
>> exception finally.
>>
>> However, this exception is triggered by accessing fcsr CSR which may
>> trigger illegal instruction trap and virtual
>> instruction trap in different situation.
>>
>> "For convenience, when the stateen CSRs are implemented and misa.F =
>> 0, then if bit 1 of a
>> controlling stateen0 CSR is zero, all floating-point instructions
>> cause an illegal instruction trap (or virtual
>> instruction trap, if relevant), as though they all access fcsr,
>> regardless of whether they really do."
>>
>> And  stateen.fcsr is only work when zfinx is enabled, so  it's better
>> to SMSTATEEN_CHECK(ctx) after check for
>>
>> "!ctx->cfg_ptr->ext_zfinx"
> Actually the spec refers only to misa.F and not zfinx and regarding the
> fcsr its :
> "as though they all access fcsr, regardless of whether they really do"

Yeah, they are triggered by accessing fcsr. So they should take the same 
check as accessing fcsr here.

In above predicate fs for fcsr, if misa.F is zero and zfinx is not 
supported,illegal instruction fault is triggered.

Otherwise, stateen related check is done when misa.F is zero and may 
trigger illegal/virtual instruction fault.

both of this two cases are different in above check.

I also sent related questions in 
https://github.com/riscv/riscv-state-enable/issues/9.

Any comments are welcome.

Regards,

Weiwei Li

>
>> Regards,
>> Weiwei Li
>>>> Regards,
>>>> Weiwei Li
>>>>
>>>>>    
>>>>> diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc
>>>>> b/target/riscv/insn_trans/trans_rvzfh.c.inc
>>>>> index 5d07150cd0..b165ea9d58 100644
>>>>> --- a/target/riscv/insn_trans/trans_rvzfh.c.inc
>>>>> +++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
>>>>> @@ -17,24 +17,28 @@
>>>>>     */
>>>>>    
>>>>>    #define REQUIRE_ZFH(ctx) do { \
>>>>> +    SMSTATEEN_CHECK(ctx); \
>>>>>        if (!ctx->cfg_ptr->ext_zfh) {      \
>>>>>            return false;         \
>>>>>        }                         \
>>>>>    } while (0)
>>>>>    
>>>>>    #define REQUIRE_ZHINX_OR_ZFH(ctx) do { \
>>>>> +    SMSTATEEN_CHECK(ctx); \
>>>>>        if (!ctx->cfg_ptr->ext_zhinx && !ctx->cfg_ptr->ext_zfh) {
>>>>> \
>>>>>            return false;                  \
>>>>>        }                                  \
>>>>>    } while (0)
>>>>>    
>>>>>    #define REQUIRE_ZFH_OR_ZFHMIN(ctx) do {       \
>>>>> +    SMSTATEEN_CHECK(ctx); \
>>>>>        if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin))
>>>>> { \
>>>>>            return false;                         \
>>>>>        }                                         \
>>>>>    } while (0)
>>>>>    
>>>>>    #define REQUIRE_ZFH_OR_ZFHMIN_OR_ZHINX_OR_ZHINXMIN(ctx) do {
>>>>> \
>>>>> +    SMSTATEEN_CHECK(ctx); \
>>>>>        if (!(ctx->cfg_ptr->ext_zfh || ctx->cfg_ptr->ext_zfhmin
>>>>> ||          \
>>>>>              ctx->cfg_ptr->ext_zhinx || ctx->cfg_ptr-
>>>>>> ext_zhinxmin))
>>>>> {     \
>>>>>            return
>>>>> false;                                        \

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Ben Dooks]

On 28/07/2022 07:15, Mayuresh Chitale wrote:
> On Mon, 2022-07-25 at 15:23 +0800, Weiwei Li wrote:
>>
>> 在 2022/7/24 下午11:49, Mayuresh Chitale 写道:
>>> On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:
>>>> 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
>>>>> If smstateen is implemented and sstateen0.fcsr is clear then
>>>>> the
>>>>> floating point operations must return illegal instruction
>>>>> exception.
>>>>>
>>>>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
>>>>> ---
>>>>>    target/riscv/csr.c                        | 23 ++++++++++++++
>>>>>    target/riscv/insn_trans/trans_rvf.c.inc   | 38
>>>>> +++++++++++++++++++++--
>>>>>    target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
>>>>>    3 files changed, 63 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>> index ab06b117f9..a597b6cbc7 100644
>>>>> --- a/target/riscv/csr.c
>>>>> +++ b/target/riscv/csr.c
>>>>> @@ -96,6 +96,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;
>>>>>    }
>>>>> @@ -1876,6 +1880,9 @@ static RISCVException
>>>>> write_mstateen0(CPURISCVState *env, int csrno,
>>>>>                                          target_ulong new_val)
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>>    
>>>>>        return write_mstateen(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>> @@ -1924,6 +1931,10 @@ static RISCVException
>>>>> write_mstateen0h(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>>    
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>> +
>>>>>        return write_mstateenh(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>>    
>>>>> @@ -1973,6 +1984,10 @@ static RISCVException
>>>>> write_hstateen0(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>>    
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>> +
>>>>>        return write_hstateen(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>>    
>>>>> @@ -2024,6 +2039,10 @@ static RISCVException
>>>>> write_hstateen0h(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> SMSTATEEN0_HSENVCFG;
>>>>>    
>>>>> +    if (!riscv_has_ext(env, RVF)) {
>>>>> +        wr_mask |= SMSTATEEN0_FCSR;
>>>>> +    }
>>>>> +
>>>>>        return write_hstateenh(env, csrno, wr_mask, new_val);
>>>>>    }
>>>>>    
>>>>> @@ -2083,6 +2102,10 @@ static RISCVException
>>>>> write_sstateen0(CPURISCVState *env, int csrno,
>>>>>    {
>>>>>        uint64_t wr_mask = SMSTATEEN_STATEN |
>>>>> 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.inc
>>>>> b/target/riscv/insn_trans/trans_rvf.c.inc
>>>>> index a1d3eb52ad..c43c48336b 100644
>>>>> --- a/target/riscv/insn_trans/trans_rvf.c.inc
>>>>> +++ b/target/riscv/insn_trans/trans_rvf.c.inc
>>>>> @@ -24,9 +24,43 @@
>>>>>                return false; \
>>>>>    } while (0)
>>>>>    
>>>>> +#ifndef CONFIG_USER_ONLY
>>>>> +#define SMSTATEEN_CHECK(ctx) do {\
>>>>> +    CPUState *cpu = ctx->cs; \
>>>>> +    CPURISCVState *env = cpu->env_ptr; \
>>>>> +    if (ctx->cfg_ptr->ext_smstateen && \
>>>>> +        (env->priv < PRV_M)) { \
>>>>> +        uint64_t stateen = env->mstateen[0]; \
>>>>> +        uint64_t hstateen = env->hstateen[0]; \
>>>>> +        uint64_t sstateen = env->sstateen[0]; \
>>>>> +        if (!(stateen & SMSTATEEN_STATEN)) {\
>>>>> +            hstateen = 0; \
>>>>> +            sstateen = 0; \
>>>>> +        } \
>>>>> +        if (ctx->virt_enabled) { \
>>>>> +            stateen &= hstateen; \
>>>>> +            if (!(hstateen & SMSTATEEN_STATEN)) {\
>>>>> +                sstateen = 0; \
>>>>> +            } \
>>>>> +        } \
>>>>> +        if (env->priv == PRV_U && has_ext(ctx, RVS))
>>>>> {\eventually
>>>>> meaning
>>>>> +            stateen &= sstateen; \
>>>>> +        } \
>>>>> +        if (!(stateen & SMSTATEEN0_FCSR)) { \
>>>>> +            return false; \
>>>>> +        } \
>>>>> +    } \

given the size of that I would have thought an "static inline"
function would be easier to write and maintain for SMSTATEEN_CHECK


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCH v6 3/5] target/riscv: smstateen check for fcsr

[Mayuresh Chitale]

On Thu, 2022-07-28 at 09:09 +0100, Ben Dooks wrote:
> On 28/07/2022 07:15, Mayuresh Chitale wrote:
> > On Mon, 2022-07-25 at 15:23 +0800, Weiwei Li wrote:
> > > 在 2022/7/24 下午11:49, Mayuresh Chitale 写道:
> > > > On Fri, 2022-07-22 at 09:42 +0800, Weiwei Li wrote:
> > > > > 在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> > > > > > If smstateen is implemented and sstateen0.fcsr is clear
> > > > > > then
> > > > > > the
> > > > > > floating point operations must return illegal instruction
> > > > > > exception.
> > > > > > 
> > > > > > Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> > > > > > ---
> > > > > >    target/riscv/csr.c                        | 23
> > > > > > ++++++++++++++
> > > > > >    target/riscv/insn_trans/trans_rvf.c.inc   | 38
> > > > > > +++++++++++++++++++++--
> > > > > >    target/riscv/insn_trans/trans_rvzfh.c.inc |  4 +++
> > > > > >    3 files changed, 63 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > > index ab06b117f9..a597b6cbc7 100644
> > > > > > --- a/target/riscv/csr.c
> > > > > > +++ b/target/riscv/csr.c
> > > > > > @@ -96,6 +96,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;
> > > > > >    }
> > > > > > @@ -1876,6 +1880,9 @@ static RISCVException
> > > > > > write_mstateen0(CPURISCVState *env, int csrno,
> > > > > >                                          target_ulong
> > > > > > new_val)
> > > > > >    {
> > > > > >        uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > > > SMSTATEEN0_HSENVCFG;
> > > > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > > > +    }
> > > > > >    
> > > > > >        return write_mstateen(env, csrno, wr_mask, new_val);
> > > > > >    }
> > > > > > @@ -1924,6 +1931,10 @@ static RISCVException
> > > > > > write_mstateen0h(CPURISCVState *env, int csrno,
> > > > > >    {
> > > > > >        uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > > > SMSTATEEN0_HSENVCFG;
> > > > > >    
> > > > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > > > +    }
> > > > > > +
> > > > > >        return write_mstateenh(env, csrno, wr_mask,
> > > > > > new_val);
> > > > > >    }
> > > > > >    
> > > > > > @@ -1973,6 +1984,10 @@ static RISCVException
> > > > > > write_hstateen0(CPURISCVState *env, int csrno,
> > > > > >    {
> > > > > >        uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > > > SMSTATEEN0_HSENVCFG;
> > > > > >    
> > > > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > > > +    }
> > > > > > +
> > > > > >        return write_hstateen(env, csrno, wr_mask, new_val);
> > > > > >    }
> > > > > >    
> > > > > > @@ -2024,6 +2039,10 @@ static RISCVException
> > > > > > write_hstateen0h(CPURISCVState *env, int csrno,
> > > > > >    {
> > > > > >        uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > > > SMSTATEEN0_HSENVCFG;
> > > > > >    
> > > > > > +    if (!riscv_has_ext(env, RVF)) {
> > > > > > +        wr_mask |= SMSTATEEN0_FCSR;
> > > > > > +    }
> > > > > > +
> > > > > >        return write_hstateenh(env, csrno, wr_mask,
> > > > > > new_val);
> > > > > >    }
> > > > > >    
> > > > > > @@ -2083,6 +2102,10 @@ static RISCVException
> > > > > > write_sstateen0(CPURISCVState *env, int csrno,
> > > > > >    {
> > > > > >        uint64_t wr_mask = SMSTATEEN_STATEN |
> > > > > > 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.inc
> > > > > > b/target/riscv/insn_trans/trans_rvf.c.inc
> > > > > > index a1d3eb52ad..c43c48336b 100644
> > > > > > --- a/target/riscv/insn_trans/trans_rvf.c.inc
> > > > > > +++ b/target/riscv/insn_trans/trans_rvf.c.inc
> > > > > > @@ -24,9 +24,43 @@
> > > > > >                return false; \
> > > > > >    } while (0)
> > > > > >    
> > > > > > +#ifndef CONFIG_USER_ONLY
> > > > > > +#define SMSTATEEN_CHECK(ctx) do {\
> > > > > > +    CPUState *cpu = ctx->cs; \
> > > > > > +    CPURISCVState *env = cpu->env_ptr; \
> > > > > > +    if (ctx->cfg_ptr->ext_smstateen && \
> > > > > > +        (env->priv < PRV_M)) { \
> > > > > > +        uint64_t stateen = env->mstateen[0]; \
> > > > > > +        uint64_t hstateen = env->hstateen[0]; \
> > > > > > +        uint64_t sstateen = env->sstateen[0]; \
> > > > > > +        if (!(stateen & SMSTATEEN_STATEN)) {\
> > > > > > +            hstateen = 0; \
> > > > > > +            sstateen = 0; \
> > > > > > +        } \
> > > > > > +        if (ctx->virt_enabled) { \
> > > > > > +            stateen &= hstateen; \
> > > > > > +            if (!(hstateen & SMSTATEEN_STATEN)) {\
> > > > > > +                sstateen = 0; \
> > > > > > +            } \
> > > > > > +        } \
> > > > > > +        if (env->priv == PRV_U && has_ext(ctx, RVS))
> > > > > > {\eventually
> > > > > > meaning
> > > > > > +            stateen &= sstateen; \
> > > > > > +        } \
> > > > > > +        if (!(stateen & SMSTATEEN0_FCSR)) { \
> > > > > > +            return false; \
> > > > > > +        } \
> > > > > > +    } \
> 
> given the size of that I would have thought an "static inline"
> function would be easier to write and maintain for SMSTATEEN_CHECK
Ok. I will update in the next version.
> 
> 

[PATCH v6 4/5] target/riscv: smstateen check for AIA/IMSIC

[Mayuresh Chitale]

If smstateen is implemented then accesses to AIA
registers CSRS, IMSIC CSRs and other IMSIC registers
is controlled by setting of corresponding bits in
mstateen/hstateen registers. Otherwise an illegal
instruction trap or virtual instruction trap is
generated.

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/csr.c | 222 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 220 insertions(+), 2 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index a597b6cbc7..e40dd35d85 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -87,6 +87,42 @@ static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
 
     return RISCV_EXCP_NONE;
 }
+
+static RISCVException smstateen_aia_acc_ok(CPURISCVState *env, int csrno)
+{
+    uint64_t bit;
+
+    switch (csrno) {
+    case CSR_STOPEI:
+    case CSR_VSTOPEI:
+    case CSR_HSTATUS:
+        bit = SMSTATEEN0_IMSIC;
+        break;
+
+    case CSR_SIEH:
+    case CSR_SIPH:
+    case CSR_HVIPH:
+    case CSR_HVICTL:
+    case CSR_HVIPRIO1:
+    case CSR_HVIPRIO2:
+    case CSR_HVIPRIO1H:
+    case CSR_HVIPRIO2H:
+    case CSR_VSIEH:
+    case CSR_VSIPH:
+        bit = SMSTATEEN0_AIA;
+        break;
+
+    case CSR_SISELECT:
+    case CSR_VSISELECT:
+        bit = SMSTATEEN0_SVSLCT;
+        break;
+
+    default:
+        return RISCV_EXCP_NONE;
+    }
+
+    return smstateen_acc_ok(env, 0, bit);
+}
 #endif
 
 static RISCVException fs(CPURISCVState *env, int csrno)
@@ -1383,6 +1419,13 @@ static int rmw_xiselect(CPURISCVState *env, int csrno, target_ulong *val,
                         target_ulong new_val, target_ulong wr_mask)
 {
     target_ulong *iselect;
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
 
     /* Translate CSR number for VS-mode */
     csrno = aia_xlate_vs_csrno(env, csrno);
@@ -1465,7 +1508,9 @@ static int rmw_xireg(CPURISCVState *env, int csrno, target_ulong *val,
     bool virt;
     uint8_t *iprio;
     int ret = -EINVAL;
-    target_ulong priv, isel, vgein;
+    target_ulong priv, isel, vgein = 0;
+    CPUState *cs = env_cpu(env);
+    RISCVCPU *cpu = RISCV_CPU(cs);
 
     /* Translate CSR number for VS-mode */
     csrno = aia_xlate_vs_csrno(env, csrno);
@@ -1494,11 +1539,20 @@ static int rmw_xireg(CPURISCVState *env, int csrno, target_ulong *val,
     };
 
     /* Find the selected guest interrupt file */
-    vgein = (virt) ? get_field(env->hstatus, HSTATUS_VGEIN) : 0;
+    if (virt) {
+        if (!cpu->cfg.ext_smstateen ||
+                (env->hstateen[0] & SMSTATEEN0_IMSIC)) {
+            vgein = get_field(env->hstatus, HSTATUS_VGEIN);
+        }
+    }
 
     if (ISELECT_IPRIO0 <= isel && isel <= ISELECT_IPRIO15) {
         /* Local interrupt priority registers not available for VS-mode */
         if (!virt) {
+            if (priv == PRV_S && cpu->cfg.ext_smstateen &&
+                !(env->hstateen[0] & SMSTATEEN0_AIA)) {
+                goto done;
+            }
             ret = rmw_iprio(riscv_cpu_mxl_bits(env),
                             isel, iprio, val, new_val, wr_mask,
                             (priv == PRV_M) ? IRQ_M_EXT : IRQ_S_EXT);
@@ -1532,6 +1586,13 @@ static int rmw_xtopei(CPURISCVState *env, int csrno, target_ulong *val,
     bool virt;
     int ret = -EINVAL;
     target_ulong priv, vgein;
+    RISCVException excp;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    excp = smstateen_aia_acc_ok(env, csrno);
+    if (excp != RISCV_EXCP_NONE) {
+        return excp;
+    }
 
     /* Translate CSR number for VS-mode */
     csrno = aia_xlate_vs_csrno(env, csrno);
@@ -1884,6 +1945,10 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
         wr_mask |= SMSTATEEN0_FCSR;
     }
 
+    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
+        wr_mask |= (SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT);
+    }
+
     return write_mstateen(env, csrno, wr_mask, new_val);
 }
 
@@ -1935,6 +2000,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
         wr_mask |= SMSTATEEN0_FCSR;
     }
 
+    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
+        wr_mask |= (SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT);
+    }
+
     return write_mstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -1988,6 +2057,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
         wr_mask |= SMSTATEEN0_FCSR;
     }
 
+    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
+        wr_mask |= (SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT);
+    }
+
     return write_hstateen(env, csrno, wr_mask, new_val);
 }
 
@@ -2043,6 +2116,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
         wr_mask |= SMSTATEEN0_FCSR;
     }
 
+    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
+        wr_mask |= SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT;
+    }
+
     return write_hstateenh(env, csrno, wr_mask, new_val);
 }
 
@@ -2277,6 +2354,12 @@ static RISCVException rmw_vsieh(CPURISCVState *env, int csrno,
     uint64_t rval;
     RISCVException ret;
 
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     ret = rmw_vsie64(env, csrno, &rval,
         ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
     if (ret_val) {
@@ -2331,6 +2414,12 @@ static RISCVException rmw_sieh(CPURISCVState *env, int csrno,
     uint64_t rval;
     RISCVException ret;
 
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     ret = rmw_sie64(env, csrno, &rval,
         ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
     if (ret_val) {
@@ -2493,6 +2582,12 @@ static RISCVException rmw_vsiph(CPURISCVState *env, int csrno,
     uint64_t rval;
     RISCVException ret;
 
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     ret = rmw_vsip64(env, csrno, &rval,
         ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
     if (ret_val) {
@@ -2547,6 +2642,12 @@ static RISCVException rmw_siph(CPURISCVState *env, int csrno,
     uint64_t rval;
     RISCVException ret;
 
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     ret = rmw_sip64(env, csrno, &rval,
         ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
     if (ret_val) {
@@ -2739,6 +2840,10 @@ static RISCVException read_hstatus(CPURISCVState *env, int csrno,
 static RISCVException write_hstatus(CPURISCVState *env, int csrno,
                                     target_ulong val)
 {
+    if (smstateen_aia_acc_ok(env, csrno) != RISCV_EXCP_NONE) {
+        val &= ~HSTATUS_VGEIN;
+    }
+
     env->hstatus = val;
     if (riscv_cpu_mxl(env) != MXL_RV32 && get_field(val, HSTATUS_VSXL) != 2) {
         qemu_log_mask(LOG_UNIMP, "QEMU does not support mixed HSXLEN options.");
@@ -2799,6 +2904,12 @@ static RISCVException rmw_hidelegh(CPURISCVState *env, int csrno,
     uint64_t rval;
     RISCVException ret;
 
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     ret = rmw_hideleg64(env, csrno, &rval,
         ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
     if (ret_val) {
@@ -2845,6 +2956,12 @@ static RISCVException rmw_hviph(CPURISCVState *env, int csrno,
     uint64_t rval;
     RISCVException ret;
 
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     ret = rmw_hvip64(env, csrno, &rval,
         ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
     if (ret_val) {
@@ -2899,6 +3016,13 @@ static RISCVException write_hcounteren(CPURISCVState *env, int csrno,
 static RISCVException read_hgeie(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     if (val) {
         *val = env->hgeie;
     }
@@ -2908,6 +3032,13 @@ static RISCVException read_hgeie(CPURISCVState *env, int csrno,
 static RISCVException write_hgeie(CPURISCVState *env, int csrno,
                                   target_ulong val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     /* Only GEILEN:1 bits implemented and BIT0 is never implemented */
     val &= ((((target_ulong)1) << env->geilen) - 1) << 1;
     env->hgeie = val;
@@ -2947,6 +3078,13 @@ static RISCVException write_htinst(CPURISCVState *env, int csrno,
 static RISCVException read_hgeip(CPURISCVState *env, int csrno,
                                  target_ulong *val)
 {
+    RISCVException ret;
+
+    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     if (val) {
         *val = env->hgeip;
     }
@@ -3017,12 +3155,28 @@ static RISCVException write_htimedeltah(CPURISCVState *env, int csrno,
 
 static int read_hvictl(CPURISCVState *env, int csrno, target_ulong *val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     *val = env->hvictl;
     return RISCV_EXCP_NONE;
 }
 
 static int write_hvictl(CPURISCVState *env, int csrno, target_ulong val)
 {
+    RISCVException ret = RISCV_EXCP_NONE;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     env->hvictl = val & HVICTL_VALID_MASK;
     return RISCV_EXCP_NONE;
 }
@@ -3081,41 +3235,105 @@ static int write_hvipriox(CPURISCVState *env, int first_index,
 
 static int read_hviprio1(CPURISCVState *env, int csrno, target_ulong *val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return read_hvipriox(env, 0, env->hviprio, val);
 }
 
 static int write_hviprio1(CPURISCVState *env, int csrno, target_ulong val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return write_hvipriox(env, 0, env->hviprio, val);
 }
 
 static int read_hviprio1h(CPURISCVState *env, int csrno, target_ulong *val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return read_hvipriox(env, 4, env->hviprio, val);
 }
 
 static int write_hviprio1h(CPURISCVState *env, int csrno, target_ulong val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return write_hvipriox(env, 4, env->hviprio, val);
 }
 
 static int read_hviprio2(CPURISCVState *env, int csrno, target_ulong *val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return read_hvipriox(env, 8, env->hviprio, val);
 }
 
 static int write_hviprio2(CPURISCVState *env, int csrno, target_ulong val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return write_hvipriox(env, 8, env->hviprio, val);
 }
 
 static int read_hviprio2h(CPURISCVState *env, int csrno, target_ulong *val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return read_hvipriox(env, 12, env->hviprio, val);
 }
 
 static int write_hviprio2h(CPURISCVState *env, int csrno, target_ulong val)
 {
+    RISCVException ret;
+
+    /* Check if smstateen is enabled and this access is allowed */
+    ret = smstateen_aia_acc_ok(env, csrno);
+    if (ret != RISCV_EXCP_NONE) {
+        return ret;
+    }
+
     return write_hvipriox(env, 12, env->hviprio, val);
 }
 
-- 
2.25.1

Re: [PATCH v6 4/5] target/riscv: smstateen check for AIA/IMSIC

[Weiwei Li]

在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> If smstateen is implemented then accesses to AIA
> registers CSRS, IMSIC CSRs and other IMSIC registers
> is controlled by setting of corresponding bits in
> mstateen/hstateen registers. Otherwise an illegal
> instruction trap or virtual instruction trap is
> generated.

Is there any spec for this?

I found only two sentences for this in smstateen spec:

" bits 60:58 - Reserved for the RISC-V Advanced Interrupt Architecture"

" Setting hstateen0 bit 58 to zero prevents a virtual machine from 
accessing the hart’s IMSIC the same as
setting hstatus.VGEIN = 0"

> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/csr.c | 222 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 220 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index a597b6cbc7..e40dd35d85 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -87,6 +87,42 @@ static RISCVException smstateen_acc_ok(CPURISCVState *env, int index,
>   
>       return RISCV_EXCP_NONE;
>   }
> +
> +static RISCVException smstateen_aia_acc_ok(CPURISCVState *env, int csrno)
> +{
> +    uint64_t bit;
> +
> +    switch (csrno) {
> +    case CSR_STOPEI:
> +    case CSR_VSTOPEI:
> +    case CSR_HSTATUS:
> +        bit = SMSTATEEN0_IMSIC;
> +        break;
> +
> +    case CSR_SIEH:
> +    case CSR_SIPH:
> +    case CSR_HVIPH:
> +    case CSR_HVICTL:
> +    case CSR_HVIPRIO1:
> +    case CSR_HVIPRIO2:
> +    case CSR_HVIPRIO1H:
> +    case CSR_HVIPRIO2H:
> +    case CSR_VSIEH:
> +    case CSR_VSIPH:
> +        bit = SMSTATEEN0_AIA;
> +        break;
> +
> +    case CSR_SISELECT:
> +    case CSR_VSISELECT:
> +        bit = SMSTATEEN0_SVSLCT;
> +        break;
> +
> +    default:
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    return smstateen_acc_ok(env, 0, bit);
> +}
>   #endif
>   
>   static RISCVException fs(CPURISCVState *env, int csrno)
> @@ -1383,6 +1419,13 @@ static int rmw_xiselect(CPURISCVState *env, int csrno, target_ulong *val,
>                           target_ulong new_val, target_ulong wr_mask)
>   {
>       target_ulong *iselect;
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -1465,7 +1508,9 @@ static int rmw_xireg(CPURISCVState *env, int csrno, target_ulong *val,
>       bool virt;
>       uint8_t *iprio;
>       int ret = -EINVAL;
> -    target_ulong priv, isel, vgein;
> +    target_ulong priv, isel, vgein = 0;
> +    CPUState *cs = env_cpu(env);
> +    RISCVCPU *cpu = RISCV_CPU(cs);
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -1494,11 +1539,20 @@ static int rmw_xireg(CPURISCVState *env, int csrno, target_ulong *val,
>       };
>   
>       /* Find the selected guest interrupt file */
> -    vgein = (virt) ? get_field(env->hstatus, HSTATUS_VGEIN) : 0;
> +    if (virt) {
> +        if (!cpu->cfg.ext_smstateen ||
> +                (env->hstateen[0] & SMSTATEEN0_IMSIC)) {
> +            vgein = get_field(env->hstatus, HSTATUS_VGEIN);
> +        }
> +    }
>   
>       if (ISELECT_IPRIO0 <= isel && isel <= ISELECT_IPRIO15) {
>           /* Local interrupt priority registers not available for VS-mode */
>           if (!virt) {
> +            if (priv == PRV_S && cpu->cfg.ext_smstateen &&
> +                !(env->hstateen[0] & SMSTATEEN0_AIA)) {
> +                goto done;
> +            }
>               ret = rmw_iprio(riscv_cpu_mxl_bits(env),
>                               isel, iprio, val, new_val, wr_mask,
>                               (priv == PRV_M) ? IRQ_M_EXT : IRQ_S_EXT);
> @@ -1532,6 +1586,13 @@ static int rmw_xtopei(CPURISCVState *env, int csrno, target_ulong *val,
>       bool virt;
>       int ret = -EINVAL;
>       target_ulong priv, vgein;
> +    RISCVException excp;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    excp = smstateen_aia_acc_ok(env, csrno);
> +    if (excp != RISCV_EXCP_NONE) {
> +        return excp;
> +    }
>   
>       /* Translate CSR number for VS-mode */
>       csrno = aia_xlate_vs_csrno(env, csrno);
> @@ -1884,6 +1945,10 @@ static RISCVException write_mstateen0(CPURISCVState *env, int csrno,
>           wr_mask |= SMSTATEEN0_FCSR;
>       }
>   
> +    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
> +        wr_mask |= (SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT);
> +    }
> +
>       return write_mstateen(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -1935,6 +2000,10 @@ static RISCVException write_mstateen0h(CPURISCVState *env, int csrno,
>           wr_mask |= SMSTATEEN0_FCSR;
>       }
>   
> +    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
> +        wr_mask |= (SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT);
> +    }
> +
>       return write_mstateenh(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -1988,6 +2057,10 @@ static RISCVException write_hstateen0(CPURISCVState *env, int csrno,
>           wr_mask |= SMSTATEEN0_FCSR;
>       }
>   
> +    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
> +        wr_mask |= (SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT);
> +    }
> +
>       return write_hstateen(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2043,6 +2116,10 @@ static RISCVException write_hstateen0h(CPURISCVState *env, int csrno,
>           wr_mask |= SMSTATEEN0_FCSR;
>       }
>   
> +    if (riscv_feature(env, RISCV_FEATURE_AIA)) {
> +        wr_mask |= SMSTATEEN0_IMSIC | SMSTATEEN0_AIA | SMSTATEEN0_SVSLCT;
> +    }
> +
>       return write_hstateenh(env, csrno, wr_mask, new_val);
>   }
>   
> @@ -2277,6 +2354,12 @@ static RISCVException rmw_vsieh(CPURISCVState *env, int csrno,
>       uint64_t rval;
>       RISCVException ret;
>   
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       ret = rmw_vsie64(env, csrno, &rval,
>           ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
>       if (ret_val) {
> @@ -2331,6 +2414,12 @@ static RISCVException rmw_sieh(CPURISCVState *env, int csrno,
>       uint64_t rval;
>       RISCVException ret;
>   
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       ret = rmw_sie64(env, csrno, &rval,
>           ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
>       if (ret_val) {
> @@ -2493,6 +2582,12 @@ static RISCVException rmw_vsiph(CPURISCVState *env, int csrno,
>       uint64_t rval;
>       RISCVException ret;
>   
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       ret = rmw_vsip64(env, csrno, &rval,
>           ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
>       if (ret_val) {
> @@ -2547,6 +2642,12 @@ static RISCVException rmw_siph(CPURISCVState *env, int csrno,
>       uint64_t rval;
>       RISCVException ret;
>   
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       ret = rmw_sip64(env, csrno, &rval,
>           ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
>       if (ret_val) {
> @@ -2739,6 +2840,10 @@ static RISCVException read_hstatus(CPURISCVState *env, int csrno,
>   static RISCVException write_hstatus(CPURISCVState *env, int csrno,
>                                       target_ulong val)
>   {
> +    if (smstateen_aia_acc_ok(env, csrno) != RISCV_EXCP_NONE) {
> +        val &= ~HSTATUS_VGEIN;
> +    }
> +

Why this?  For the statement

"Setting hstateen0 bit 58 to zero prevents a virtual machine from 
accessing the hart’s IMSIC the same as
setting hstatus.VGEIN = 0"

I think  it means "setting hstateen0 bit 58 to zero" and "setting 
hstatus.VGEIN = 0" have the same function:

  " prevents a virtual machine from accessing the hart’s IMSIC", not 
"setting hstateen0 bit 58 to zero"  is to

"setting hstatus.VGEIN = 0".

Or any other statement about this ?

Regards,

Weiwei Li

>       env->hstatus = val;
>       if (riscv_cpu_mxl(env) != MXL_RV32 && get_field(val, HSTATUS_VSXL) != 2) {
>           qemu_log_mask(LOG_UNIMP, "QEMU does not support mixed HSXLEN options.");
> @@ -2799,6 +2904,12 @@ static RISCVException rmw_hidelegh(CPURISCVState *env, int csrno,
>       uint64_t rval;
>       RISCVException ret;
>   
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       ret = rmw_hideleg64(env, csrno, &rval,
>           ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
>       if (ret_val) {
> @@ -2845,6 +2956,12 @@ static RISCVException rmw_hviph(CPURISCVState *env, int csrno,
>       uint64_t rval;
>       RISCVException ret;
>   
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       ret = rmw_hvip64(env, csrno, &rval,
>           ((uint64_t)new_val) << 32, ((uint64_t)wr_mask) << 32);
>       if (ret_val) {
> @@ -2899,6 +3016,13 @@ static RISCVException write_hcounteren(CPURISCVState *env, int csrno,
>   static RISCVException read_hgeie(CPURISCVState *env, int csrno,
>                                    target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       if (val) {
>           *val = env->hgeie;
>       }
> @@ -2908,6 +3032,13 @@ static RISCVException read_hgeie(CPURISCVState *env, int csrno,
>   static RISCVException write_hgeie(CPURISCVState *env, int csrno,
>                                     target_ulong val)
>   {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       /* Only GEILEN:1 bits implemented and BIT0 is never implemented */
>       val &= ((((target_ulong)1) << env->geilen) - 1) << 1;
>       env->hgeie = val;
> @@ -2947,6 +3078,13 @@ static RISCVException write_htinst(CPURISCVState *env, int csrno,
>   static RISCVException read_hgeip(CPURISCVState *env, int csrno,
>                                    target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    ret = smstateen_acc_ok(env, 0, SMSTATEEN0_IMSIC);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       if (val) {
>           *val = env->hgeip;
>       }
> @@ -3017,12 +3155,28 @@ static RISCVException write_htimedeltah(CPURISCVState *env, int csrno,
>   
>   static int read_hvictl(CPURISCVState *env, int csrno, target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       *val = env->hvictl;
>       return RISCV_EXCP_NONE;
>   }
>   
>   static int write_hvictl(CPURISCVState *env, int csrno, target_ulong val)
>   {
> +    RISCVException ret = RISCV_EXCP_NONE;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       env->hvictl = val & HVICTL_VALID_MASK;
>       return RISCV_EXCP_NONE;
>   }
> @@ -3081,41 +3235,105 @@ static int write_hvipriox(CPURISCVState *env, int first_index,
>   
>   static int read_hviprio1(CPURISCVState *env, int csrno, target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return read_hvipriox(env, 0, env->hviprio, val);
>   }
>   
>   static int write_hviprio1(CPURISCVState *env, int csrno, target_ulong val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return write_hvipriox(env, 0, env->hviprio, val);
>   }
>   
>   static int read_hviprio1h(CPURISCVState *env, int csrno, target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return read_hvipriox(env, 4, env->hviprio, val);
>   }
>   
>   static int write_hviprio1h(CPURISCVState *env, int csrno, target_ulong val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return write_hvipriox(env, 4, env->hviprio, val);
>   }
>   
>   static int read_hviprio2(CPURISCVState *env, int csrno, target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return read_hvipriox(env, 8, env->hviprio, val);
>   }
>   
>   static int write_hviprio2(CPURISCVState *env, int csrno, target_ulong val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return write_hvipriox(env, 8, env->hviprio, val);
>   }
>   
>   static int read_hviprio2h(CPURISCVState *env, int csrno, target_ulong *val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return read_hvipriox(env, 12, env->hviprio, val);
>   }
>   
>   static int write_hviprio2h(CPURISCVState *env, int csrno, target_ulong val)
>   {
> +    RISCVException ret;
> +
> +    /* Check if smstateen is enabled and this access is allowed */
> +    ret = smstateen_aia_acc_ok(env, csrno);
> +    if (ret != RISCV_EXCP_NONE) {
> +        return ret;
> +    }
> +
>       return write_hvipriox(env, 12, env->hviprio, val);
>   }
>   

[PATCH v6 5/5] target/riscv: smstateen knobs

[Mayuresh Chitale]

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

Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
---
 target/riscv/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index db2b8e4d30..2b7ed95396 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -98,6 +98,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
     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(smstateen, true, PRIV_VERSION_1_12_0, ext_smstateen),
     ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
     ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
     ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
@@ -987,6 +988,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),
-- 
2.25.1

Re: [PATCH v6 5/5] target/riscv: smstateen knobs

[Weiwei Li]

在 2022/7/21 下午11:31, Mayuresh Chitale 写道:
> Add knobs to allow users to enable smstateen and also export it via the
> ISA extension string.
>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index db2b8e4d30..2b7ed95396 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -98,6 +98,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
>       ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
>       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(smstateen, true, PRIV_VERSION_1_12_0, ext_smstateen),
>       ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
>       ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
>       ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
> @@ -987,6 +988,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),


Reviewed-by: Weiwei Li<liweiwei@iscas.ac.cn>

Regards,
Weiwei Li