[PATCH v6 0/9] make write_misa a no-op and FEATURE_* cleanups

[PATCH v6 0/9] make write_misa a no-op and FEATURE_* cleanups

[Daniel Henrique Barboza]

Hi,

This version contains a change in patch 2, where riscv_cpu_cfg() now
returns a const pointer to the RISCVCPUConfig struct instead of the
struct itself.

Minor changes were made in the remaining patches due to that.


Changes from v5:
- patches without review/acks: patch 2
- patch 2:
  - riscv_cpu_cfg() now returns a const pointer to RISCVCPUConfig
- v5 link: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg04906.html

Daniel Henrique Barboza (9):
  target/riscv: turn write_misa() into an official no-op
  target/riscv: introduce riscv_cpu_cfg()
  target/riscv: remove RISCV_FEATURE_DEBUG
  target/riscv/cpu.c: error out if EPMP is enabled without PMP
  target/riscv: remove RISCV_FEATURE_EPMP
  target/riscv: remove RISCV_FEATURE_PMP
  hw/riscv/virt.c: do not use RISCV_FEATURE_MMU in
    create_fdt_socket_cpus()
  target/riscv: remove RISCV_FEATURE_MMU
  target/riscv/cpu: remove CPUArchState::features and friends

 hw/riscv/virt.c           |  7 +++--
 target/riscv/cpu.c        | 19 +++---------
 target/riscv/cpu.h        | 28 +++--------------
 target/riscv/cpu_helper.c |  6 ++--
 target/riscv/csr.c        | 65 +++------------------------------------
 target/riscv/machine.c    | 11 +++----
 target/riscv/monitor.c    |  2 +-
 target/riscv/op_helper.c  |  2 +-
 target/riscv/pmp.c        |  8 ++---
 9 files changed, 31 insertions(+), 117 deletions(-)

-- 
2.39.1

[PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Daniel Henrique Barboza]

At this moment, and apparently since ever, we have no way of enabling
RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
the nuts and bolts that handles how to write this CSR, has always been a
no-op as well because write_misa() will always exit earlier.

This seems to be benign in the majority of cases. Booting an Ubuntu
'virt' guest and logging all the calls to 'write_misa' shows that no
writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
RISC-V extensions after the machine is powered on, seems to be a niche
use.

Before proceeding, let's recap what the spec says about MISA. It is a
CSR that is divided in 3 fields:

- MXL, Machine XLEN, described as "may be writable";

- MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
fixed value if MISA is zero;

- Extensions is defined as "a WARL field that can contain writable bits
where the implementation allows the supported ISA to be modified"

Thus what we have today (write_misa() being a no-op) is already a valid
spec implementation. We're not obliged to have a particular set of MISA
writable bits, and at this moment we have none.

Given that allowing the dormant code to write MISA can cause tricky bugs
to solve later on, and we don't have a particularly interesting case of
writing MISA to support today, and we're already not violating the
specification, let's erase all the body of write_misa() and turn it into
an official no-op instead of an accidental one. We'll keep consistent
with what we provide users today but with 50+ less lines to maintain.

RISCV_FEATURE_MISA enum is erased in the process since there's no one
else using it.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.h |  1 -
 target/riscv/csr.c | 55 ----------------------------------------------
 2 files changed, 56 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 7128438d8e..01803a020d 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -89,7 +89,6 @@ enum {
     RISCV_FEATURE_MMU,
     RISCV_FEATURE_PMP,
     RISCV_FEATURE_EPMP,
-    RISCV_FEATURE_MISA,
     RISCV_FEATURE_DEBUG
 };
 
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 1b0a0c1693..f7862ff4a4 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
 static RISCVException write_misa(CPURISCVState *env, int csrno,
                                  target_ulong val)
 {
-    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
-        /* drop write to misa */
-        return RISCV_EXCP_NONE;
-    }
-
-    /* 'I' or 'E' must be present */
-    if (!(val & (RVI | RVE))) {
-        /* It is not, drop write to misa */
-        return RISCV_EXCP_NONE;
-    }
-
-    /* 'E' excludes all other extensions */
-    if (val & RVE) {
-        /* when we support 'E' we can do "val = RVE;" however
-         * for now we just drop writes if 'E' is present.
-         */
-        return RISCV_EXCP_NONE;
-    }
-
-    /*
-     * misa.MXL writes are not supported by QEMU.
-     * Drop writes to those bits.
-     */
-
-    /* Mask extensions that are not supported by this hart */
-    val &= env->misa_ext_mask;
-
-    /* Mask extensions that are not supported by QEMU */
-    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
-
-    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
-    if ((val & RVD) && !(val & RVF)) {
-        val &= ~RVD;
-    }
-
-    /* Suppress 'C' if next instruction is not aligned
-     * TODO: this should check next_pc
-     */
-    if ((val & RVC) && (GETPC() & ~3) != 0) {
-        val &= ~RVC;
-    }
-
-    /* If nothing changed, do nothing. */
-    if (val == env->misa_ext) {
-        return RISCV_EXCP_NONE;
-    }
-
-    if (!(val & RVF)) {
-        env->mstatus &= ~MSTATUS_FS;
-    }
-
-    /* flush translation cache */
-    tb_flush(env_cpu(env));
-    env->misa_ext = val;
-    env->xl = riscv_cpu_mxl(env);
     return RISCV_EXCP_NONE;
 }
 
-- 
2.39.1

[PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[Daniel Henrique Barboza]

We're going to do changes that requires accessing the RISCVCPUConfig
struct from the RISCVCPU, having access only to a CPURISCVState 'env'
pointer. Add a helper to make the code easier to read.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 01803a020d..5e9626837b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
 #endif
 #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
 
+static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)
+{
+    return &env_archcpu(env)->cfg;
+}
+
 #if defined(TARGET_RISCV32)
 #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
 #else
-- 
2.39.1

[PATCH v6 3/9] target/riscv: remove RISCV_FEATURE_DEBUG

[Daniel Henrique Barboza]

RISCV_FEATURE_DEBUG will always follow the value defined by
cpu->cfg.debug flag. Read the flag instead.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c        | 6 +-----
 target/riscv/cpu.h        | 1 -
 target/riscv/cpu_helper.c | 2 +-
 target/riscv/csr.c        | 2 +-
 target/riscv/machine.c    | 3 +--
 5 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 93b52b826c..e34a5e3f11 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -637,7 +637,7 @@ static void riscv_cpu_reset_hold(Object *obj)
     set_default_nan_mode(1, &env->fp_status);
 
 #ifndef CONFIG_USER_ONLY
-    if (riscv_feature(env, RISCV_FEATURE_DEBUG)) {
+    if (cpu->cfg.debug) {
         riscv_trigger_init(env);
     }
 
@@ -935,10 +935,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    if (cpu->cfg.debug) {
-        riscv_set_feature(env, RISCV_FEATURE_DEBUG);
-    }
-
 
 #ifndef CONFIG_USER_ONLY
     if (cpu->cfg.ext_sstc) {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5e9626837b..2afb705930 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -89,7 +89,6 @@ enum {
     RISCV_FEATURE_MMU,
     RISCV_FEATURE_PMP,
     RISCV_FEATURE_EPMP,
-    RISCV_FEATURE_DEBUG
 };
 
 /* Privileged specification version */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index ad8d82662c..4cdd247c6c 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -105,7 +105,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
         flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
                            get_field(env->mstatus_hs, MSTATUS_VS));
     }
-    if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
+    if (cpu->cfg.debug && !icount_enabled()) {
         flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
     }
 #endif
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index f7862ff4a4..58af2c0e66 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -437,7 +437,7 @@ static RISCVException epmp(CPURISCVState *env, int csrno)
 
 static RISCVException debug(CPURISCVState *env, int csrno)
 {
-    if (riscv_feature(env, RISCV_FEATURE_DEBUG)) {
+    if (riscv_cpu_cfg(env)->debug) {
         return RISCV_EXCP_NONE;
     }
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index c6ce318cce..4634968898 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -226,9 +226,8 @@ static const VMStateDescription vmstate_kvmtimer = {
 static bool debug_needed(void *opaque)
 {
     RISCVCPU *cpu = opaque;
-    CPURISCVState *env = &cpu->env;
 
-    return riscv_feature(env, RISCV_FEATURE_DEBUG);
+    return cpu->cfg.debug;
 }
 
 static int debug_post_load(void *opaque, int version_id)
-- 
2.39.1

[PATCH v6 4/9] target/riscv/cpu.c: error out if EPMP is enabled without PMP

[Daniel Henrique Barboza]

Instead of silently ignoring the EPMP setting if there is no PMP
available, error out informing the user that EPMP depends on PMP
support:

$ ./qemu-system-riscv64 -cpu rv64,pmp=false,x-epmp=true
qemu-system-riscv64: Invalid configuration: EPMP requires PMP support

This will force users to pick saner options in the QEMU command line.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e34a5e3f11..4585ca74dc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -925,13 +925,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 
     if (cpu->cfg.pmp) {
         riscv_set_feature(env, RISCV_FEATURE_PMP);
+    }
+
+    if (cpu->cfg.epmp) {
+        riscv_set_feature(env, RISCV_FEATURE_EPMP);
 
         /*
          * Enhanced PMP should only be available
          * on harts with PMP support
          */
-        if (cpu->cfg.epmp) {
-            riscv_set_feature(env, RISCV_FEATURE_EPMP);
+        if (!cpu->cfg.pmp) {
+            error_setg(errp, "Invalid configuration: EPMP requires PMP support");
+            return;
         }
     }
 
-- 
2.39.1

[PATCH v6 5/9] target/riscv: remove RISCV_FEATURE_EPMP

[Daniel Henrique Barboza]

RISCV_FEATURE_EPMP is always set to the same value as the cpu->cfg.epmp
flag. Use the flag directly.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c | 10 +++-------
 target/riscv/cpu.h |  1 -
 target/riscv/csr.c |  2 +-
 target/riscv/pmp.c |  4 ++--
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4585ca74dc..71b2042d73 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -927,17 +927,13 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         riscv_set_feature(env, RISCV_FEATURE_PMP);
     }
 
-    if (cpu->cfg.epmp) {
-        riscv_set_feature(env, RISCV_FEATURE_EPMP);
-
+    if (cpu->cfg.epmp && !cpu->cfg.pmp) {
         /*
          * Enhanced PMP should only be available
          * on harts with PMP support
          */
-        if (!cpu->cfg.pmp) {
-            error_setg(errp, "Invalid configuration: EPMP requires PMP support");
-            return;
-        }
+        error_setg(errp, "Invalid configuration: EPMP requires PMP support");
+        return;
     }
 
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 2afb705930..6d659d74fa 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -88,7 +88,6 @@
 enum {
     RISCV_FEATURE_MMU,
     RISCV_FEATURE_PMP,
-    RISCV_FEATURE_EPMP,
 };
 
 /* Privileged specification version */
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 58af2c0e66..cdc68d3676 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -428,7 +428,7 @@ static RISCVException pmp(CPURISCVState *env, int csrno)
 
 static RISCVException epmp(CPURISCVState *env, int csrno)
 {
-    if (env->priv == PRV_M && riscv_feature(env, RISCV_FEATURE_EPMP)) {
+    if (env->priv == PRV_M && riscv_cpu_cfg(env)->epmp) {
         return RISCV_EXCP_NONE;
     }
 
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 4bc4113531..aa4d1996e9 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -88,7 +88,7 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
     if (pmp_index < MAX_RISCV_PMPS) {
         bool locked = true;
 
-        if (riscv_feature(env, RISCV_FEATURE_EPMP)) {
+        if (riscv_cpu_cfg(env)->epmp) {
             /* mseccfg.RLB is set */
             if (MSECCFG_RLB_ISSET(env)) {
                 locked = false;
@@ -239,7 +239,7 @@ static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
 {
     bool ret;
 
-    if (riscv_feature(env, RISCV_FEATURE_EPMP)) {
+    if (riscv_cpu_cfg(env)->epmp) {
         if (MSECCFG_MMWP_ISSET(env)) {
             /*
              * The Machine Mode Whitelist Policy (mseccfg.MMWP) is set
-- 
2.39.1

[PATCH v6 6/9] target/riscv: remove RISCV_FEATURE_PMP

[Daniel Henrique Barboza]

RISCV_FEATURE_PMP is being set via riscv_set_feature() by mirroring the
cpu->cfg.pmp flag. Use the flag instead.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c        | 4 ----
 target/riscv/cpu.h        | 1 -
 target/riscv/cpu_helper.c | 2 +-
 target/riscv/csr.c        | 2 +-
 target/riscv/machine.c    | 3 +--
 target/riscv/op_helper.c  | 2 +-
 target/riscv/pmp.c        | 2 +-
 7 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 71b2042d73..7b1360d6ba 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -923,10 +923,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         riscv_set_feature(env, RISCV_FEATURE_MMU);
     }
 
-    if (cpu->cfg.pmp) {
-        riscv_set_feature(env, RISCV_FEATURE_PMP);
-    }
-
     if (cpu->cfg.epmp && !cpu->cfg.pmp) {
         /*
          * Enhanced PMP should only be available
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6d659d74fa..119a022af9 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -87,7 +87,6 @@
    so a cpu features bitfield is required, likewise for optional PMP support */
 enum {
     RISCV_FEATURE_MMU,
-    RISCV_FEATURE_PMP,
 };
 
 /* Privileged specification version */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 4cdd247c6c..292b6b3168 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -706,7 +706,7 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
     pmp_priv_t pmp_priv;
     int pmp_index = -1;
 
-    if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
+    if (!riscv_cpu_cfg(env)->pmp) {
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         return TRANSLATE_SUCCESS;
     }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index cdc68d3676..d0ab00d870 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -419,7 +419,7 @@ static int aia_hmode32(CPURISCVState *env, int csrno)
 
 static RISCVException pmp(CPURISCVState *env, int csrno)
 {
-    if (riscv_feature(env, RISCV_FEATURE_PMP)) {
+    if (riscv_cpu_cfg(env)->pmp) {
         return RISCV_EXCP_NONE;
     }
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 4634968898..67e9e56853 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -27,9 +27,8 @@
 static bool pmp_needed(void *opaque)
 {
     RISCVCPU *cpu = opaque;
-    CPURISCVState *env = &cpu->env;
 
-    return riscv_feature(env, RISCV_FEATURE_PMP);
+    return cpu->cfg.pmp;
 }
 
 static int pmp_post_load(void *opaque, int version_id)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 48f918b71b..9c0b91c88f 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -195,7 +195,7 @@ target_ulong helper_mret(CPURISCVState *env)
     uint64_t mstatus = env->mstatus;
     target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
 
-    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
+    if (riscv_cpu_cfg(env)->pmp &&
         !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
         riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
     }
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index aa4d1996e9..205bfbe090 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -265,7 +265,7 @@ static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
         }
     }
 
-    if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) {
+    if (!riscv_cpu_cfg(env)->pmp || (mode == PRV_M)) {
         /*
          * Privileged spec v1.10 states if HW doesn't implement any PMP entry
          * or no PMP entry matches an M-Mode access, the access succeeds.
-- 
2.39.1

[PATCH v6 7/9] hw/riscv/virt.c: do not use RISCV_FEATURE_MMU in create_fdt_socket_cpus()

[Daniel Henrique Barboza]

Read cpu_ptr->cfg.mmu directly. As a bonus, use cpu_ptr in
riscv_isa_string().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 hw/riscv/virt.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 86c4adc0c9..49f2c157f7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -232,20 +232,21 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
     bool is_32_bit = riscv_is_32bit(&s->soc[0]);
 
     for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
+        RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
+
         cpu_phandle = (*phandle)++;
 
         cpu_name = g_strdup_printf("/cpus/cpu@%d",
             s->soc[socket].hartid_base + cpu);
         qemu_fdt_add_subnode(ms->fdt, cpu_name);
-        if (riscv_feature(&s->soc[socket].harts[cpu].env,
-                          RISCV_FEATURE_MMU)) {
+        if (cpu_ptr->cfg.mmu) {
             qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type",
                                     (is_32_bit) ? "riscv,sv32" : "riscv,sv48");
         } else {
             qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type",
                                     "riscv,none");
         }
-        name = riscv_isa_string(&s->soc[socket].harts[cpu]);
+        name = riscv_isa_string(cpu_ptr);
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
         g_free(name);
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");
-- 
2.39.1

[PATCH v6 8/9] target/riscv: remove RISCV_FEATURE_MMU

[Daniel Henrique Barboza]

RISCV_FEATURE_MMU is set whether cpu->cfg.mmu is set, so let's just use
the flag directly instead.

With this change the enum is also removed. It is worth noticing that
this enum, and all the RISCV_FEATURES_* that were contained in it,
predates the existence of the cpu->cfg object. Today, using cpu->cfg is
an easier way to retrieve all the features and extensions enabled in the
hart.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.c        | 4 ----
 target/riscv/cpu.h        | 7 -------
 target/riscv/cpu_helper.c | 2 +-
 target/riscv/csr.c        | 4 ++--
 target/riscv/monitor.c    | 2 +-
 target/riscv/pmp.c        | 2 +-
 6 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7b1360d6ba..075033006c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -919,10 +919,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    if (cpu->cfg.mmu) {
-        riscv_set_feature(env, RISCV_FEATURE_MMU);
-    }
-
     if (cpu->cfg.epmp && !cpu->cfg.pmp) {
         /*
          * Enhanced PMP should only be available
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 119a022af9..0519d2ab0c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -81,13 +81,6 @@
 #define RVH RV('H')
 #define RVJ RV('J')
 
-/* S extension denotes that Supervisor mode exists, however it is possible
-   to have a core that support S mode but does not have an MMU and there
-   is currently no bit in misa to indicate whether an MMU exists or not
-   so a cpu features bitfield is required, likewise for optional PMP support */
-enum {
-    RISCV_FEATURE_MMU,
-};
 
 /* Privileged specification version */
 enum {
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 292b6b3168..eda2293470 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -796,7 +796,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
         mode = PRV_U;
     }
 
-    if (mode == PRV_M || !riscv_feature(env, RISCV_FEATURE_MMU)) {
+    if (mode == PRV_M || !riscv_cpu_cfg(env)->mmu) {
         *physical = addr;
         *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         return TRANSLATE_SUCCESS;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d0ab00d870..fcc271c93c 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2569,7 +2569,7 @@ static RISCVException rmw_siph(CPURISCVState *env, int csrno,
 static RISCVException read_satp(CPURISCVState *env, int csrno,
                                 target_ulong *val)
 {
-    if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
+    if (!riscv_cpu_cfg(env)->mmu) {
         *val = 0;
         return RISCV_EXCP_NONE;
     }
@@ -2588,7 +2588,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno,
 {
     target_ulong vm, mask;
 
-    if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
+    if (!riscv_cpu_cfg(env)->mmu) {
         return RISCV_EXCP_NONE;
     }
 
diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
index 236f93b9f5..f36ddfa967 100644
--- a/target/riscv/monitor.c
+++ b/target/riscv/monitor.c
@@ -218,7 +218,7 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
+    if (!riscv_cpu_cfg(env)->mmu) {
         monitor_printf(mon, "S-mode MMU unavailable\n");
         return;
     }
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 205bfbe090..a08cd95658 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -315,7 +315,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
     }
 
     if (size == 0) {
-        if (riscv_feature(env, RISCV_FEATURE_MMU)) {
+        if (riscv_cpu_cfg(env)->mmu) {
             /*
              * If size is unknown (0), assume that all bytes
              * from addr to the end of the page will be accessed.
-- 
2.39.1

[PATCH v6 9/9] target/riscv/cpu: remove CPUArchState::features and friends

[Daniel Henrique Barboza]

The attribute is no longer used since we can retrieve all the enabled
features in the hart by using cpu->cfg instead.

Remove env->feature, riscv_feature() and riscv_set_feature(). We also
need to bump vmstate_riscv_cpu version_id and minimal_version_id since
'features' is no longer being migrated.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 target/riscv/cpu.h     | 12 ------------
 target/riscv/machine.c |  5 ++---
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0519d2ab0c..9897305184 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -173,8 +173,6 @@ struct CPUArchState {
     /* 128-bit helpers upper part return value */
     target_ulong retxh;
 
-    uint32_t features;
-
 #ifdef CONFIG_USER_ONLY
     uint32_t elf_flags;
 #endif
@@ -524,16 +522,6 @@ static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
     return (env->misa_ext & ext) != 0;
 }
 
-static inline bool riscv_feature(CPURISCVState *env, int feature)
-{
-    return env->features & (1ULL << feature);
-}
-
-static inline void riscv_set_feature(CPURISCVState *env, int feature)
-{
-    env->features |= (1ULL << feature);
-}
-
 #include "cpu_user.h"
 
 extern const char * const riscv_int_regnames[];
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 67e9e56853..9c455931d8 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -331,8 +331,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
 
 const VMStateDescription vmstate_riscv_cpu = {
     .name = "cpu",
-    .version_id = 6,
-    .minimum_version_id = 6,
+    .version_id = 7,
+    .minimum_version_id = 7,
     .post_load = riscv_cpu_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
@@ -351,7 +351,6 @@ const VMStateDescription vmstate_riscv_cpu = {
         VMSTATE_UINT32(env.misa_ext, RISCVCPU),
         VMSTATE_UINT32(env.misa_mxl_max, RISCVCPU),
         VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
-        VMSTATE_UINT32(env.features, RISCVCPU),
         VMSTATE_UINTTL(env.priv, RISCVCPU),
         VMSTATE_UINTTL(env.virt, RISCVCPU),
         VMSTATE_UINT64(env.resetvec, RISCVCPU),
-- 
2.39.1

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[Bin Meng]

On Fri, Feb 17, 2023 at 5:57 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We're going to do changes that requires accessing the RISCVCPUConfig
> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> pointer. Add a helper to make the code easier to read.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.h | 5 +++++
>  1 file changed, 5 insertions(+)
>

Looks like the RB tag is missing somehow?

Reviewed-by: Bin Meng <bmeng@tinylab.org>

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[Bin Meng]

On Fri, Feb 17, 2023 at 8:45 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Feb 17, 2023 at 5:57 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > We're going to do changes that requires accessing the RISCVCPUConfig
> > struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> > pointer. Add a helper to make the code easier to read.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >  target/riscv/cpu.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
>
> Looks like the RB tag is missing somehow?

Never mind. I see the difference :)

Regards,
Bin

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> At this moment, and apparently since ever, we have no way of enabling
> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
> the nuts and bolts that handles how to write this CSR, has always been a
> no-op as well because write_misa() will always exit earlier.
>
> This seems to be benign in the majority of cases. Booting an Ubuntu
> 'virt' guest and logging all the calls to 'write_misa' shows that no
> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
> RISC-V extensions after the machine is powered on, seems to be a niche
> use.
>
> Before proceeding, let's recap what the spec says about MISA. It is a
> CSR that is divided in 3 fields:
>
> - MXL, Machine XLEN, described as "may be writable";
>
> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
> fixed value if MISA is zero;
>
> - Extensions is defined as "a WARL field that can contain writable bits
> where the implementation allows the supported ISA to be modified"
>
> Thus what we have today (write_misa() being a no-op) is already a valid
> spec implementation. We're not obliged to have a particular set of MISA
> writable bits, and at this moment we have none.

Hi Daniel,

I see there has been a discussion on this topic. And as no-op has no 
harmfulness for current implementation.
However, I still think we should make misa writable as default, which is 
also a valid spec implementation.

One reason is that may be we need to dynamic write  access for some cpus 
in the future. The other is we should
make QEMU a more useful implementation, not just a legal implementation. 
We have done in many aspects on this direction.

I prefer your implementation before v4. It's not a complicated 
implementation. And I think the other extensions on QEMU currently
can mostly be configurable already.

Your work is a good step towards to unify the configuration and the 
check.  I think two more steps we can go further.

1) Remove RVI/RVF and the similar macros, and add fields for them in the 
configuration struct.

2) Unify the check about configuration. write_misa and cpu_realize_fn 
can use the same check function.


As we have done these two steps, I think we can go more closely for the 
profile extension.


Zhiwei

> Given that allowing the dormant code to write MISA can cause tricky bugs
> to solve later on, and we don't have a particularly interesting case of
> writing MISA to support today, and we're already not violating the
> specification, let's erase all the body of write_misa() and turn it into
> an official no-op instead of an accidental one. We'll keep consistent
> with what we provide users today but with 50+ less lines to maintain.
>
> RISCV_FEATURE_MISA enum is erased in the process since there's no one
> else using it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.h |  1 -
>   target/riscv/csr.c | 55 ----------------------------------------------
>   2 files changed, 56 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7128438d8e..01803a020d 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -89,7 +89,6 @@ enum {
>       RISCV_FEATURE_MMU,
>       RISCV_FEATURE_PMP,
>       RISCV_FEATURE_EPMP,
> -    RISCV_FEATURE_MISA,
>       RISCV_FEATURE_DEBUG
>   };
>   
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1b0a0c1693..f7862ff4a4 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
> -        /* drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'I' or 'E' must be present */
> -    if (!(val & (RVI | RVE))) {
> -        /* It is not, drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'E' excludes all other extensions */
> -    if (val & RVE) {
> -        /* when we support 'E' we can do "val = RVE;" however
> -         * for now we just drop writes if 'E' is present.
> -         */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /*
> -     * misa.MXL writes are not supported by QEMU.
> -     * Drop writes to those bits.
> -     */
> -
> -    /* Mask extensions that are not supported by this hart */
> -    val &= env->misa_ext_mask;
> -
> -    /* Mask extensions that are not supported by QEMU */
> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
> -
> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> -    if ((val & RVD) && !(val & RVF)) {
> -        val &= ~RVD;
> -    }
> -
> -    /* Suppress 'C' if next instruction is not aligned
> -     * TODO: this should check next_pc
> -     */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;
> -    }
> -
> -    /* If nothing changed, do nothing. */
> -    if (val == env->misa_ext) {
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    if (!(val & RVF)) {
> -        env->mstatus &= ~MSTATUS_FS;
> -    }
> -
> -    /* flush translation cache */
> -    tb_flush(env_cpu(env));
> -    env->misa_ext = val;
> -    env->xl = riscv_cpu_mxl(env);
>       return RISCV_EXCP_NONE;
>   }
>   

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> We're going to do changes that requires accessing the RISCVCPUConfig
> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> pointer. Add a helper to make the code easier to read.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 01803a020d..5e9626837b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
>   #endif
>   #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
>   
> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)
> +{
> +    return &env_archcpu(env)->cfg;
> +}
> +
>   #if defined(TARGET_RISCV32)
>   #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
>   #else

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> We're going to do changes that requires accessing the RISCVCPUConfig
> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> pointer. Add a helper to make the code easier to read.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 01803a020d..5e9626837b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
>   #endif
>   #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
>   
> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)

Maybe we should

static inline const* RISCVCPUConfig riscv_cpu_cfg(CPURISCVState *env) or just
static inline RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)

Zhiwei

> +{
> +    return &env_archcpu(env)->cfg;
> +}
> +
>   #if defined(TARGET_RISCV32)
>   #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
>   #else

Re: [PATCH v6 3/9] target/riscv: remove RISCV_FEATURE_DEBUG

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> RISCV_FEATURE_DEBUG will always follow the value defined by
> cpu->cfg.debug flag. Read the flag instead.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.c        | 6 +-----
>   target/riscv/cpu.h        | 1 -
>   target/riscv/cpu_helper.c | 2 +-
>   target/riscv/csr.c        | 2 +-
>   target/riscv/machine.c    | 3 +--
>   5 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 93b52b826c..e34a5e3f11 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -637,7 +637,7 @@ static void riscv_cpu_reset_hold(Object *obj)
>       set_default_nan_mode(1, &env->fp_status);
>   
>   #ifndef CONFIG_USER_ONLY
> -    if (riscv_feature(env, RISCV_FEATURE_DEBUG)) {
> +    if (cpu->cfg.debug) {
>           riscv_trigger_init(env);
>       }
>   
> @@ -935,10 +935,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>           }
>       }
>   
> -    if (cpu->cfg.debug) {
> -        riscv_set_feature(env, RISCV_FEATURE_DEBUG);
> -    }
> -
>   
>   #ifndef CONFIG_USER_ONLY
>       if (cpu->cfg.ext_sstc) {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 5e9626837b..2afb705930 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -89,7 +89,6 @@ enum {
>       RISCV_FEATURE_MMU,
>       RISCV_FEATURE_PMP,
>       RISCV_FEATURE_EPMP,
> -    RISCV_FEATURE_DEBUG
>   };
>   
>   /* Privileged specification version */
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index ad8d82662c..4cdd247c6c 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -105,7 +105,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>           flags = FIELD_DP32(flags, TB_FLAGS, MSTATUS_HS_VS,
>                              get_field(env->mstatus_hs, MSTATUS_VS));
>       }
> -    if (riscv_feature(env, RISCV_FEATURE_DEBUG) && !icount_enabled()) {
> +    if (cpu->cfg.debug && !icount_enabled()) {
>           flags = FIELD_DP32(flags, TB_FLAGS, ITRIGGER, env->itrigger_enabled);
>       }
>   #endif
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index f7862ff4a4..58af2c0e66 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -437,7 +437,7 @@ static RISCVException epmp(CPURISCVState *env, int csrno)
>   
>   static RISCVException debug(CPURISCVState *env, int csrno)
>   {
> -    if (riscv_feature(env, RISCV_FEATURE_DEBUG)) {
> +    if (riscv_cpu_cfg(env)->debug) {
>           return RISCV_EXCP_NONE;
>       }
>   
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index c6ce318cce..4634968898 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -226,9 +226,8 @@ static const VMStateDescription vmstate_kvmtimer = {
>   static bool debug_needed(void *opaque)
>   {
>       RISCVCPU *cpu = opaque;
> -    CPURISCVState *env = &cpu->env;
>   
> -    return riscv_feature(env, RISCV_FEATURE_DEBUG);
> +    return cpu->cfg.debug;
>   }
>   

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   static int debug_post_load(void *opaque, int version_id)

Re: [PATCH v6 4/9] target/riscv/cpu.c: error out if EPMP is enabled without PMP

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> Instead of silently ignoring the EPMP setting if there is no PMP
> available, error out informing the user that EPMP depends on PMP
> support:
>
> $ ./qemu-system-riscv64 -cpu rv64,pmp=false,x-epmp=true
> qemu-system-riscv64: Invalid configuration: EPMP requires PMP support
>
> This will force users to pick saner options in the QEMU command line.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e34a5e3f11..4585ca74dc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -925,13 +925,18 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>   
>       if (cpu->cfg.pmp) {
>           riscv_set_feature(env, RISCV_FEATURE_PMP);
> +    }
> +
> +    if (cpu->cfg.epmp) {
> +        riscv_set_feature(env, RISCV_FEATURE_EPMP);
>   
>           /*
>            * Enhanced PMP should only be available
>            * on harts with PMP support
>            */
> -        if (cpu->cfg.epmp) {
> -            riscv_set_feature(env, RISCV_FEATURE_EPMP);
> +        if (!cpu->cfg.pmp) {
> +            error_setg(errp, "Invalid configuration: EPMP requires PMP support");
> +            return;

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           }
>       }
>   

Re: [PATCH v6 5/9] target/riscv: remove RISCV_FEATURE_EPMP

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> RISCV_FEATURE_EPMP is always set to the same value as the cpu->cfg.epmp
> flag. Use the flag directly.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 10 +++-------
>   target/riscv/cpu.h |  1 -
>   target/riscv/csr.c |  2 +-
>   target/riscv/pmp.c |  4 ++--
>   4 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4585ca74dc..71b2042d73 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -927,17 +927,13 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>           riscv_set_feature(env, RISCV_FEATURE_PMP);
>       }
>   
> -    if (cpu->cfg.epmp) {
> -        riscv_set_feature(env, RISCV_FEATURE_EPMP);
> -
> +    if (cpu->cfg.epmp && !cpu->cfg.pmp) {
>           /*
>            * Enhanced PMP should only be available
>            * on harts with PMP support
>            */
> -        if (!cpu->cfg.pmp) {
> -            error_setg(errp, "Invalid configuration: EPMP requires PMP support");
> -            return;
> -        }
> +        error_setg(errp, "Invalid configuration: EPMP requires PMP support");
> +        return;
>       }
>   
>   
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 2afb705930..6d659d74fa 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -88,7 +88,6 @@
>   enum {
>       RISCV_FEATURE_MMU,
>       RISCV_FEATURE_PMP,
> -    RISCV_FEATURE_EPMP,
>   };
>   
>   /* Privileged specification version */
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58af2c0e66..cdc68d3676 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -428,7 +428,7 @@ static RISCVException pmp(CPURISCVState *env, int csrno)
>   
>   static RISCVException epmp(CPURISCVState *env, int csrno)
>   {
> -    if (env->priv == PRV_M && riscv_feature(env, RISCV_FEATURE_EPMP)) {
> +    if (env->priv == PRV_M && riscv_cpu_cfg(env)->epmp) {
>           return RISCV_EXCP_NONE;
>       }
>   
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 4bc4113531..aa4d1996e9 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -88,7 +88,7 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
>       if (pmp_index < MAX_RISCV_PMPS) {
>           bool locked = true;
>   
> -        if (riscv_feature(env, RISCV_FEATURE_EPMP)) {
> +        if (riscv_cpu_cfg(env)->epmp) {
>               /* mseccfg.RLB is set */
>               if (MSECCFG_RLB_ISSET(env)) {
>                   locked = false;
> @@ -239,7 +239,7 @@ static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
>   {
>       bool ret;
>   
> -    if (riscv_feature(env, RISCV_FEATURE_EPMP)) {
> +    if (riscv_cpu_cfg(env)->epmp) {

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           if (MSECCFG_MMWP_ISSET(env)) {
>               /*
>                * The Machine Mode Whitelist Policy (mseccfg.MMWP) is set

Re: [PATCH v6 6/9] target/riscv: remove RISCV_FEATURE_PMP

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> RISCV_FEATURE_PMP is being set via riscv_set_feature() by mirroring the
> cpu->cfg.pmp flag. Use the flag instead.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.c        | 4 ----
>   target/riscv/cpu.h        | 1 -
>   target/riscv/cpu_helper.c | 2 +-
>   target/riscv/csr.c        | 2 +-
>   target/riscv/machine.c    | 3 +--
>   target/riscv/op_helper.c  | 2 +-
>   target/riscv/pmp.c        | 2 +-
>   7 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 71b2042d73..7b1360d6ba 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -923,10 +923,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>           riscv_set_feature(env, RISCV_FEATURE_MMU);
>       }
>   
> -    if (cpu->cfg.pmp) {
> -        riscv_set_feature(env, RISCV_FEATURE_PMP);
> -    }
> -
>       if (cpu->cfg.epmp && !cpu->cfg.pmp) {
>           /*
>            * Enhanced PMP should only be available
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6d659d74fa..119a022af9 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -87,7 +87,6 @@
>      so a cpu features bitfield is required, likewise for optional PMP support */
>   enum {
>       RISCV_FEATURE_MMU,
> -    RISCV_FEATURE_PMP,
>   };
>   
>   /* Privileged specification version */
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 4cdd247c6c..292b6b3168 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -706,7 +706,7 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
>       pmp_priv_t pmp_priv;
>       int pmp_index = -1;
>   
> -    if (!riscv_feature(env, RISCV_FEATURE_PMP)) {
> +    if (!riscv_cpu_cfg(env)->pmp) {
>           *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>           return TRANSLATE_SUCCESS;
>       }
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index cdc68d3676..d0ab00d870 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -419,7 +419,7 @@ static int aia_hmode32(CPURISCVState *env, int csrno)
>   
>   static RISCVException pmp(CPURISCVState *env, int csrno)
>   {
> -    if (riscv_feature(env, RISCV_FEATURE_PMP)) {
> +    if (riscv_cpu_cfg(env)->pmp) {
>           return RISCV_EXCP_NONE;
>       }
>   
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 4634968898..67e9e56853 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -27,9 +27,8 @@
>   static bool pmp_needed(void *opaque)
>   {
>       RISCVCPU *cpu = opaque;
> -    CPURISCVState *env = &cpu->env;
>   
> -    return riscv_feature(env, RISCV_FEATURE_PMP);
> +    return cpu->cfg.pmp;
>   }
>   
>   static int pmp_post_load(void *opaque, int version_id)
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 48f918b71b..9c0b91c88f 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -195,7 +195,7 @@ target_ulong helper_mret(CPURISCVState *env)
>       uint64_t mstatus = env->mstatus;
>       target_ulong prev_priv = get_field(mstatus, MSTATUS_MPP);
>   
> -    if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> +    if (riscv_cpu_cfg(env)->pmp &&
>           !pmp_get_num_rules(env) && (prev_priv != PRV_M)) {
>           riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
>       }
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index aa4d1996e9..205bfbe090 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -265,7 +265,7 @@ static bool pmp_hart_has_privs_default(CPURISCVState *env, target_ulong addr,
>           }
>       }
>   
> -    if ((!riscv_feature(env, RISCV_FEATURE_PMP)) || (mode == PRV_M)) {
> +    if (!riscv_cpu_cfg(env)->pmp || (mode == PRV_M)) {

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           /*
>            * Privileged spec v1.10 states if HW doesn't implement any PMP entry
>            * or no PMP entry matches an M-Mode access, the access succeeds.

Re: [PATCH v6 7/9] hw/riscv/virt.c: do not use RISCV_FEATURE_MMU in create_fdt_socket_cpus()

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> Read cpu_ptr->cfg.mmu directly. As a bonus, use cpu_ptr in
> riscv_isa_string().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   hw/riscv/virt.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 86c4adc0c9..49f2c157f7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -232,20 +232,21 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>       bool is_32_bit = riscv_is_32bit(&s->soc[0]);
>   
>       for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
> +        RISCVCPU *cpu_ptr = &s->soc[socket].harts[cpu];
> +
>           cpu_phandle = (*phandle)++;
>   
>           cpu_name = g_strdup_printf("/cpus/cpu@%d",
>               s->soc[socket].hartid_base + cpu);
>           qemu_fdt_add_subnode(ms->fdt, cpu_name);
> -        if (riscv_feature(&s->soc[socket].harts[cpu].env,
> -                          RISCV_FEATURE_MMU)) {
> +        if (cpu_ptr->cfg.mmu) {
>               qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type",
>                                       (is_32_bit) ? "riscv,sv32" : "riscv,sv48");
>           } else {
>               qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type",
>                                       "riscv,none");
>           }
> -        name = riscv_isa_string(&s->soc[socket].harts[cpu]);
> +        name = riscv_isa_string(cpu_ptr);

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
>           g_free(name);
>           qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");

Re: [PATCH v6 8/9] target/riscv: remove RISCV_FEATURE_MMU

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> RISCV_FEATURE_MMU is set whether cpu->cfg.mmu is set, so let's just use
> the flag directly instead.
>
> With this change the enum is also removed. It is worth noticing that
> this enum, and all the RISCV_FEATURES_* that were contained in it,
> predates the existence of the cpu->cfg object. Today, using cpu->cfg is
> an easier way to retrieve all the features and extensions enabled in the
> hart.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.c        | 4 ----
>   target/riscv/cpu.h        | 7 -------
>   target/riscv/cpu_helper.c | 2 +-
>   target/riscv/csr.c        | 4 ++--
>   target/riscv/monitor.c    | 2 +-
>   target/riscv/pmp.c        | 2 +-
>   6 files changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7b1360d6ba..075033006c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -919,10 +919,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>           }
>       }
>   
> -    if (cpu->cfg.mmu) {
> -        riscv_set_feature(env, RISCV_FEATURE_MMU);
> -    }
> -
>       if (cpu->cfg.epmp && !cpu->cfg.pmp) {
>           /*
>            * Enhanced PMP should only be available
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 119a022af9..0519d2ab0c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -81,13 +81,6 @@
>   #define RVH RV('H')
>   #define RVJ RV('J')
>   
> -/* S extension denotes that Supervisor mode exists, however it is possible
> -   to have a core that support S mode but does not have an MMU and there
> -   is currently no bit in misa to indicate whether an MMU exists or not
> -   so a cpu features bitfield is required, likewise for optional PMP support */
> -enum {
> -    RISCV_FEATURE_MMU,
> -};
>   
>   /* Privileged specification version */
>   enum {
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 292b6b3168..eda2293470 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -796,7 +796,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>           mode = PRV_U;
>       }
>   
> -    if (mode == PRV_M || !riscv_feature(env, RISCV_FEATURE_MMU)) {
> +    if (mode == PRV_M || !riscv_cpu_cfg(env)->mmu) {
>           *physical = addr;
>           *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>           return TRANSLATE_SUCCESS;
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d0ab00d870..fcc271c93c 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2569,7 +2569,7 @@ static RISCVException rmw_siph(CPURISCVState *env, int csrno,
>   static RISCVException read_satp(CPURISCVState *env, int csrno,
>                                   target_ulong *val)
>   {
> -    if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> +    if (!riscv_cpu_cfg(env)->mmu) {
>           *val = 0;
>           return RISCV_EXCP_NONE;
>       }
> @@ -2588,7 +2588,7 @@ static RISCVException write_satp(CPURISCVState *env, int csrno,
>   {
>       target_ulong vm, mask;
>   
> -    if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> +    if (!riscv_cpu_cfg(env)->mmu) {
>           return RISCV_EXCP_NONE;
>       }
>   
> diff --git a/target/riscv/monitor.c b/target/riscv/monitor.c
> index 236f93b9f5..f36ddfa967 100644
> --- a/target/riscv/monitor.c
> +++ b/target/riscv/monitor.c
> @@ -218,7 +218,7 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict)
>           return;
>       }
>   
> -    if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> +    if (!riscv_cpu_cfg(env)->mmu) {
>           monitor_printf(mon, "S-mode MMU unavailable\n");
>           return;
>       }
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 205bfbe090..a08cd95658 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -315,7 +315,7 @@ int pmp_hart_has_privs(CPURISCVState *env, target_ulong addr,
>       }
>   
>       if (size == 0) {
> -        if (riscv_feature(env, RISCV_FEATURE_MMU)) {
> +        if (riscv_cpu_cfg(env)->mmu) {

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>               /*
>                * If size is unknown (0), assume that all bytes
>                * from addr to the end of the page will be accessed.

Re: [PATCH v6 9/9] target/riscv/cpu: remove CPUArchState::features and friends

[LIU Zhiwei]

On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> The attribute is no longer used since we can retrieve all the enabled
> features in the hart by using cpu->cfg instead.
>
> Remove env->feature, riscv_feature() and riscv_set_feature(). We also
> need to bump vmstate_riscv_cpu version_id and minimal_version_id since
> 'features' is no longer being migrated.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/cpu.h     | 12 ------------
>   target/riscv/machine.c |  5 ++---
>   2 files changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0519d2ab0c..9897305184 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -173,8 +173,6 @@ struct CPUArchState {
>       /* 128-bit helpers upper part return value */
>       target_ulong retxh;
>   
> -    uint32_t features;
> -
>   #ifdef CONFIG_USER_ONLY
>       uint32_t elf_flags;
>   #endif
> @@ -524,16 +522,6 @@ static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
>       return (env->misa_ext & ext) != 0;
>   }
>   
> -static inline bool riscv_feature(CPURISCVState *env, int feature)
> -{
> -    return env->features & (1ULL << feature);
> -}
> -
> -static inline void riscv_set_feature(CPURISCVState *env, int feature)
> -{
> -    env->features |= (1ULL << feature);
> -}
> -
>   #include "cpu_user.h"
>   
>   extern const char * const riscv_int_regnames[];
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 67e9e56853..9c455931d8 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -331,8 +331,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
>   
>   const VMStateDescription vmstate_riscv_cpu = {
>       .name = "cpu",
> -    .version_id = 6,
> -    .minimum_version_id = 6,
> +    .version_id = 7,
> +    .minimum_version_id = 7,
>       .post_load = riscv_cpu_post_load,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> @@ -351,7 +351,6 @@ const VMStateDescription vmstate_riscv_cpu = {
>           VMSTATE_UINT32(env.misa_ext, RISCVCPU),
>           VMSTATE_UINT32(env.misa_mxl_max, RISCVCPU),
>           VMSTATE_UINT32(env.misa_ext_mask, RISCVCPU),
> -        VMSTATE_UINT32(env.features, RISCVCPU),

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>           VMSTATE_UINTTL(env.priv, RISCVCPU),
>           VMSTATE_UINTTL(env.virt, RISCVCPU),
>           VMSTATE_UINT64(env.resetvec, RISCVCPU),

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[LIU Zhiwei]

On 2023/2/17 9:50, LIU Zhiwei wrote:
>
> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>> We're going to do changes that requires accessing the RISCVCPUConfig
>> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
>> pointer. Add a helper to make the code easier to read.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 01803a020d..5e9626837b 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -653,6 +653,11 @@ static inline RISCVMXL 
>> riscv_cpu_mxl(CPURISCVState *env)
>>   #endif
>>   #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
>>   +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)
>
> Maybe we should
>
> static inline const* RISCVCPUConfig riscv_cpu_cfg(CPURISCVState *env) 
> or just
> static inline RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)

Ignore this comment. I see that you never change the fields from this 
pointer.

Zhiwei

>
> Zhiwei
>
>> +{
>> +    return &env_archcpu(env)->cfg;
>> +}
>> +
>>   #if defined(TARGET_RISCV32)
>>   #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
>>   #else

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[weiwei]

On 2023/2/17 05:55, Daniel Henrique Barboza wrote:
> At this moment, and apparently since ever, we have no way of enabling
> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
> the nuts and bolts that handles how to write this CSR, has always been a
> no-op as well because write_misa() will always exit earlier.
>
> This seems to be benign in the majority of cases. Booting an Ubuntu
> 'virt' guest and logging all the calls to 'write_misa' shows that no
> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
> RISC-V extensions after the machine is powered on, seems to be a niche
> use.
>
> Before proceeding, let's recap what the spec says about MISA. It is a
> CSR that is divided in 3 fields:
>
> - MXL, Machine XLEN, described as "may be writable";
>
> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
> fixed value if MISA is zero;
>
> - Extensions is defined as "a WARL field that can contain writable bits
> where the implementation allows the supported ISA to be modified"
>
> Thus what we have today (write_misa() being a no-op) is already a valid
> spec implementation. We're not obliged to have a particular set of MISA
> writable bits, and at this moment we have none.
>
> Given that allowing the dormant code to write MISA can cause tricky bugs
> to solve later on, and we don't have a particularly interesting case of
> writing MISA to support today, and we're already not violating the
> specification, let's erase all the body of write_misa() and turn it into
> an official no-op instead of an accidental one. We'll keep consistent
> with what we provide users today but with 50+ less lines to maintain.
>
> RISCV_FEATURE_MISA enum is erased in the process since there's no one
> else using it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,

Weiwei Li
> ---
>   target/riscv/cpu.h |  1 -
>   target/riscv/csr.c | 55 ----------------------------------------------
>   2 files changed, 56 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 7128438d8e..01803a020d 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -89,7 +89,6 @@ enum {
>       RISCV_FEATURE_MMU,
>       RISCV_FEATURE_PMP,
>       RISCV_FEATURE_EPMP,
> -    RISCV_FEATURE_MISA,
>       RISCV_FEATURE_DEBUG
>   };
>   
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 1b0a0c1693..f7862ff4a4 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
> -        /* drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'I' or 'E' must be present */
> -    if (!(val & (RVI | RVE))) {
> -        /* It is not, drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'E' excludes all other extensions */
> -    if (val & RVE) {
> -        /* when we support 'E' we can do "val = RVE;" however
> -         * for now we just drop writes if 'E' is present.
> -         */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /*
> -     * misa.MXL writes are not supported by QEMU.
> -     * Drop writes to those bits.
> -     */
> -
> -    /* Mask extensions that are not supported by this hart */
> -    val &= env->misa_ext_mask;
> -
> -    /* Mask extensions that are not supported by QEMU */
> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
> -
> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> -    if ((val & RVD) && !(val & RVF)) {
> -        val &= ~RVD;
> -    }
> -
> -    /* Suppress 'C' if next instruction is not aligned
> -     * TODO: this should check next_pc
> -     */
> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> -        val &= ~RVC;
> -    }
> -
> -    /* If nothing changed, do nothing. */
> -    if (val == env->misa_ext) {
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    if (!(val & RVF)) {
> -        env->mstatus &= ~MSTATUS_FS;
> -    }
> -
> -    /* flush translation cache */
> -    tb_flush(env_cpu(env));
> -    env->misa_ext = val;
> -    env->xl = riscv_cpu_mxl(env);
>       return RISCV_EXCP_NONE;
>   }
>   

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[weiwei]

On 2023/2/17 05:55, Daniel Henrique Barboza wrote:
> We're going to do changes that requires accessing the RISCVCPUConfig
> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> pointer. Add a helper to make the code easier to read.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>

Regards,

Weiwei Li
> ---
>   target/riscv/cpu.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 01803a020d..5e9626837b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
>   #endif
>   #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
>   
> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)
> +{
> +    return &env_archcpu(env)->cfg;
> +}
> +
>   #if defined(TARGET_RISCV32)
>   #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
>   #else

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[LIU Zhiwei]

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


On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> We're going to do changes that requires accessing the RISCVCPUConfig
> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> pointer. Add a helper to make the code easier to read.
>
> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 01803a020d..5e9626837b 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
>   #endif
>   #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
>   
> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)
> +{
> +    return &env_archcpu(env)->cfg;
> +}
> +

There many places in branch should use this interface, not just in this 
patch set.

For example,

static RISCVException seed(CPURISCVState *env, int csrno)
{
     RISCVCPU *cpu = env_archcpu(env);

     if (!cpu->cfg.ext_zkr) {
         return RISCV_EXCP_ILLEGAL_INST;
     }

The cpu here will not be used, except referring to the cfg.

Do you mind to unify the use?

Zhiwei

>   #if defined(TARGET_RISCV32)
>   #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
>   #else

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

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[Richard Henderson]

On 2/16/23 11:55, Daniel Henrique Barboza wrote:
> We're going to do changes that requires accessing the RISCVCPUConfig
> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
> pointer. Add a helper to make the code easier to read.
> 
> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.h | 5 +++++
>   1 file changed, 5 insertions(+)

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

r~

Re: [PATCH v6 2/9] target/riscv: introduce riscv_cpu_cfg()

[Daniel Henrique Barboza]

On 2/16/23 23:55, LIU Zhiwei wrote:
> 
> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>> We're going to do changes that requires accessing the RISCVCPUConfig
>> struct from the RISCVCPU, having access only to a CPURISCVState 'env'
>> pointer. Add a helper to make the code easier to read.
>>
>> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 01803a020d..5e9626837b 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
>>   #endif
>>   #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env)))
>>   
>> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env)
>> +{
>> +    return &env_archcpu(env)->cfg;
>> +}
>> +
> 
> There many places in branch should use this interface, not just in this patch set.
> 
> For example,
> 
> static RISCVException seed(CPURISCVState *env, int csrno)
> {
>      RISCVCPU *cpu = env_archcpu(env);
> 
>      if (!cpu->cfg.ext_zkr) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> 
> The cpu here will not be used, except referring to the cfg.
> 
> Do you mind to unify the use?

Sure, I can look into that to make the cfg access more uniform across the code.

I believe this would be too much to do in this series though. Let's do it as a
follow-up.


Thanks.


Daniel

> 
> Zhiwei
> 
>>   #if defined(TARGET_RISCV32)
>>   #define cpu_recompute_xl(env)  ((void)(env), MXL_RV32)
>>   #else

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Daniel Henrique Barboza]

Hey,

On 2/16/23 22:42, LIU Zhiwei wrote:
> 
> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>> At this moment, and apparently since ever, we have no way of enabling
>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
>> the nuts and bolts that handles how to write this CSR, has always been a
>> no-op as well because write_misa() will always exit earlier.
>>
>> This seems to be benign in the majority of cases. Booting an Ubuntu
>> 'virt' guest and logging all the calls to 'write_misa' shows that no
>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
>> RISC-V extensions after the machine is powered on, seems to be a niche
>> use.
>>
>> Before proceeding, let's recap what the spec says about MISA. It is a
>> CSR that is divided in 3 fields:
>>
>> - MXL, Machine XLEN, described as "may be writable";
>>
>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
>> fixed value if MISA is zero;
>>
>> - Extensions is defined as "a WARL field that can contain writable bits
>> where the implementation allows the supported ISA to be modified"
>>
>> Thus what we have today (write_misa() being a no-op) is already a valid
>> spec implementation. We're not obliged to have a particular set of MISA
>> writable bits, and at this moment we have none.
> 
> Hi Daniel,
> 
> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation.
> However, I still think we should make misa writable as default, which is also a valid spec implementation.
> 
> One reason is that may be we need to dynamic write  access for some cpus in the future. The other is we should
> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction.
> 
> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently
> can mostly be configurable already.

I don't have a strong opinion in this matter to be honest. My problems with the
existing code are:

- the code is untested. I cannot say that this was never tested, but I can say that
this has been mostly untested ever since introduced. Which is normal for a code that
is 'dormant'.

- the code is dormant and most likely with bugs, but it's still maintained. For
example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
to make changes here. So we have the upkeep but no benefits.

- we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
applications seems to care either.


All this said, I think we can reach a consensus of keeping it if we can at least come
up with a way of testing it.


> 
> Your work is a good step towards to unify the configuration and the check.  I think two more steps we can go further.
> 
> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct.
> 
> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function.
> 
> 
> As we have done these two steps, I think we can go more closely for the profile extension.


Is this the extension you're taking about?

https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc


This looks like a good reason to keep the code. Let's see if anyone else has an opinion
about it. We can do the improvements you mentioned above as a follow-up (this series was
really about removing RISC_FEATURE_*) if we decide to keep it.



Thanks,


Daniel

> 
> 
> Zhiwei
> 
>> Given that allowing the dormant code to write MISA can cause tricky bugs
>> to solve later on, and we don't have a particularly interesting case of
>> writing MISA to support today, and we're already not violating the
>> specification, let's erase all the body of write_misa() and turn it into
>> an official no-op instead of an accidental one. We'll keep consistent
>> with what we provide users today but with 50+ less lines to maintain.
>>
>> RISCV_FEATURE_MISA enum is erased in the process since there's no one
>> else using it.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> ---
>>   target/riscv/cpu.h |  1 -
>>   target/riscv/csr.c | 55 ----------------------------------------------
>>   2 files changed, 56 deletions(-)
>>
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 7128438d8e..01803a020d 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -89,7 +89,6 @@ enum {
>>       RISCV_FEATURE_MMU,
>>       RISCV_FEATURE_PMP,
>>       RISCV_FEATURE_EPMP,
>> -    RISCV_FEATURE_MISA,
>>       RISCV_FEATURE_DEBUG
>>   };
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 1b0a0c1693..f7862ff4a4 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>>                                    target_ulong val)
>>   {
>> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
>> -        /* drop write to misa */
>> -        return RISCV_EXCP_NONE;
>> -    }
>> -
>> -    /* 'I' or 'E' must be present */
>> -    if (!(val & (RVI | RVE))) {
>> -        /* It is not, drop write to misa */
>> -        return RISCV_EXCP_NONE;
>> -    }
>> -
>> -    /* 'E' excludes all other extensions */
>> -    if (val & RVE) {
>> -        /* when we support 'E' we can do "val = RVE;" however
>> -         * for now we just drop writes if 'E' is present.
>> -         */
>> -        return RISCV_EXCP_NONE;
>> -    }
>> -
>> -    /*
>> -     * misa.MXL writes are not supported by QEMU.
>> -     * Drop writes to those bits.
>> -     */
>> -
>> -    /* Mask extensions that are not supported by this hart */
>> -    val &= env->misa_ext_mask;
>> -
>> -    /* Mask extensions that are not supported by QEMU */
>> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
>> -
>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>> -    if ((val & RVD) && !(val & RVF)) {
>> -        val &= ~RVD;
>> -    }
>> -
>> -    /* Suppress 'C' if next instruction is not aligned
>> -     * TODO: this should check next_pc
>> -     */
>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>> -        val &= ~RVC;
>> -    }
>> -
>> -    /* If nothing changed, do nothing. */
>> -    if (val == env->misa_ext) {
>> -        return RISCV_EXCP_NONE;
>> -    }
>> -
>> -    if (!(val & RVF)) {
>> -        env->mstatus &= ~MSTATUS_FS;
>> -    }
>> -
>> -    /* flush translation cache */
>> -    tb_flush(env_cpu(env));
>> -    env->misa_ext = val;
>> -    env->xl = riscv_cpu_mxl(env);
>>       return RISCV_EXCP_NONE;
>>   }

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Andrew Jones]

On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote:
> Hey,
> 
> On 2/16/23 22:42, LIU Zhiwei wrote:
> > 
> > On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> > > At this moment, and apparently since ever, we have no way of enabling
> > > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
> > > the nuts and bolts that handles how to write this CSR, has always been a
> > > no-op as well because write_misa() will always exit earlier.
> > > 
> > > This seems to be benign in the majority of cases. Booting an Ubuntu
> > > 'virt' guest and logging all the calls to 'write_misa' shows that no
> > > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
> > > RISC-V extensions after the machine is powered on, seems to be a niche
> > > use.
> > > 
> > > Before proceeding, let's recap what the spec says about MISA. It is a
> > > CSR that is divided in 3 fields:
> > > 
> > > - MXL, Machine XLEN, described as "may be writable";
> > > 
> > > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
> > > fixed value if MISA is zero;
> > > 
> > > - Extensions is defined as "a WARL field that can contain writable bits
> > > where the implementation allows the supported ISA to be modified"
> > > 
> > > Thus what we have today (write_misa() being a no-op) is already a valid
> > > spec implementation. We're not obliged to have a particular set of MISA
> > > writable bits, and at this moment we have none.
> > 
> > Hi Daniel,
> > 
> > I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation.
> > However, I still think we should make misa writable as default, which is also a valid spec implementation.
> > 
> > One reason is that may be we need to dynamic write  access for some cpus in the future. The other is we should
> > make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction.
> > 
> > I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently
> > can mostly be configurable already.
> 
> I don't have a strong opinion in this matter to be honest. My problems with the
> existing code are:
> 
> - the code is untested. I cannot say that this was never tested, but I can say that
> this has been mostly untested ever since introduced. Which is normal for a code that
> is 'dormant'.
> 
> - the code is dormant and most likely with bugs, but it's still maintained. For
> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
> to make changes here. So we have the upkeep but no benefits.
> 
> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
> applications seems to care either.
> 
> 
> All this said, I think we can reach a consensus of keeping it if we can at least come
> up with a way of testing it.
> 
> 
> > 
> > Your work is a good step towards to unify the configuration and the check.  I think two more steps we can go further.
> > 
> > 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct.
> > 
> > 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function.
> > 
> > 
> > As we have done these two steps, I think we can go more closely for the profile extension.
> 
> 
> Is this the extension you're taking about?
> 
> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
> 
> 
> This looks like a good reason to keep the code. Let's see if anyone else has an opinion
> about it. We can do the improvements you mentioned above as a follow-up (this series was
> really about removing RISC_FEATURE_*) if we decide to keep it.

If we decide to keep it and not guard it by default, then we should test
and fix it now. Also, as we're already aware that it has insufficient
sanity checks for extension dependencies, then we should fix our general
extension dependency checking now too, in order to apply that to this.
IOW, trying to keep this, without some guard on it, opens a can of worms.
My vote is the same as it was before, merge this series and then revisit
this function when someone has a use/test case for it. Nobody said this
was never going to have a different implementation, just that the current
implementation is known-buggy and there's no reason to expose it now.

My only concern with the code deletion is that git-blame doesn't blame
deleted code. I think we should add a comment describing the history
which includes a git commit reference which can be used to see the
latest implementation.

Thanks,
drew

> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > 
> > Zhiwei
> > 
> > > Given that allowing the dormant code to write MISA can cause tricky bugs
> > > to solve later on, and we don't have a particularly interesting case of
> > > writing MISA to support today, and we're already not violating the
> > > specification, let's erase all the body of write_misa() and turn it into
> > > an official no-op instead of an accidental one. We'll keep consistent
> > > with what we provide users today but with 50+ less lines to maintain.
> > > 
> > > RISCV_FEATURE_MISA enum is erased in the process since there's no one
> > > else using it.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >   target/riscv/cpu.h |  1 -
> > >   target/riscv/csr.c | 55 ----------------------------------------------
> > >   2 files changed, 56 deletions(-)
> > > 
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 7128438d8e..01803a020d 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -89,7 +89,6 @@ enum {
> > >       RISCV_FEATURE_MMU,
> > >       RISCV_FEATURE_PMP,
> > >       RISCV_FEATURE_EPMP,
> > > -    RISCV_FEATURE_MISA,
> > >       RISCV_FEATURE_DEBUG
> > >   };
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 1b0a0c1693..f7862ff4a4 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> > >   static RISCVException write_misa(CPURISCVState *env, int csrno,
> > >                                    target_ulong val)
> > >   {
> > > -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
> > > -        /* drop write to misa */
> > > -        return RISCV_EXCP_NONE;
> > > -    }
> > > -
> > > -    /* 'I' or 'E' must be present */
> > > -    if (!(val & (RVI | RVE))) {
> > > -        /* It is not, drop write to misa */
> > > -        return RISCV_EXCP_NONE;
> > > -    }
> > > -
> > > -    /* 'E' excludes all other extensions */
> > > -    if (val & RVE) {
> > > -        /* when we support 'E' we can do "val = RVE;" however
> > > -         * for now we just drop writes if 'E' is present.
> > > -         */
> > > -        return RISCV_EXCP_NONE;
> > > -    }
> > > -
> > > -    /*
> > > -     * misa.MXL writes are not supported by QEMU.
> > > -     * Drop writes to those bits.
> > > -     */
> > > -
> > > -    /* Mask extensions that are not supported by this hart */
> > > -    val &= env->misa_ext_mask;
> > > -
> > > -    /* Mask extensions that are not supported by QEMU */
> > > -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
> > > -
> > > -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> > > -    if ((val & RVD) && !(val & RVF)) {
> > > -        val &= ~RVD;
> > > -    }
> > > -
> > > -    /* Suppress 'C' if next instruction is not aligned
> > > -     * TODO: this should check next_pc
> > > -     */
> > > -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > > -        val &= ~RVC;
> > > -    }
> > > -
> > > -    /* If nothing changed, do nothing. */
> > > -    if (val == env->misa_ext) {
> > > -        return RISCV_EXCP_NONE;
> > > -    }
> > > -
> > > -    if (!(val & RVF)) {
> > > -        env->mstatus &= ~MSTATUS_FS;
> > > -    }
> > > -
> > > -    /* flush translation cache */
> > > -    tb_flush(env_cpu(env));
> > > -    env->misa_ext = val;
> > > -    env->xl = riscv_cpu_mxl(env);
> > >       return RISCV_EXCP_NONE;
> > >   }

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Daniel Henrique Barboza]

On 2/21/23 14:06, Andrew Jones wrote:
> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote:
>> Hey,
>>
>> On 2/16/23 22:42, LIU Zhiwei wrote:
>>>
>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>>>> At this moment, and apparently since ever, we have no way of enabling
>>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
>>>> the nuts and bolts that handles how to write this CSR, has always been a
>>>> no-op as well because write_misa() will always exit earlier.
>>>>
>>>> This seems to be benign in the majority of cases. Booting an Ubuntu
>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no
>>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
>>>> RISC-V extensions after the machine is powered on, seems to be a niche
>>>> use.
>>>>
>>>> Before proceeding, let's recap what the spec says about MISA. It is a
>>>> CSR that is divided in 3 fields:
>>>>
>>>> - MXL, Machine XLEN, described as "may be writable";
>>>>
>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
>>>> fixed value if MISA is zero;
>>>>
>>>> - Extensions is defined as "a WARL field that can contain writable bits
>>>> where the implementation allows the supported ISA to be modified"
>>>>
>>>> Thus what we have today (write_misa() being a no-op) is already a valid
>>>> spec implementation. We're not obliged to have a particular set of MISA
>>>> writable bits, and at this moment we have none.
>>>
>>> Hi Daniel,
>>>
>>> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation.
>>> However, I still think we should make misa writable as default, which is also a valid spec implementation.
>>>
>>> One reason is that may be we need to dynamic write  access for some cpus in the future. The other is we should
>>> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction.
>>>
>>> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently
>>> can mostly be configurable already.
>>
>> I don't have a strong opinion in this matter to be honest. My problems with the
>> existing code are:
>>
>> - the code is untested. I cannot say that this was never tested, but I can say that
>> this has been mostly untested ever since introduced. Which is normal for a code that
>> is 'dormant'.
>>
>> - the code is dormant and most likely with bugs, but it's still maintained. For
>> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
>> to make changes here. So we have the upkeep but no benefits.
>>
>> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
>> applications seems to care either.
>>
>>
>> All this said, I think we can reach a consensus of keeping it if we can at least come
>> up with a way of testing it.
>>
>>
>>>
>>> Your work is a good step towards to unify the configuration and the check.  I think two more steps we can go further.
>>>
>>> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct.
>>>
>>> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function.
>>>
>>>
>>> As we have done these two steps, I think we can go more closely for the profile extension.
>>
>>
>> Is this the extension you're taking about?
>>
>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc

Zhiwei, I looked it up and at first I don't understand how writing MISA is
related to this profile extension. Are you suggesting that the firmware
can choose to run a specific profile and then the hardware must adapt to
it on the fly? Because if not, then we can implement profiles by just
passing them in the QEMU command line.

>>
>>
>> This looks like a good reason to keep the code. Let's see if anyone else has an opinion
>> about it. We can do the improvements you mentioned above as a follow-up (this series was
>> really about removing RISC_FEATURE_*) if we decide to keep it.
> 
> If we decide to keep it and not guard it by default, then we should test
> and fix it now. Also, as we're already aware that it has insufficient
> sanity checks for extension dependencies, then we should fix our general
> extension dependency checking now too, in order to apply that to this.
> IOW, trying to keep this, without some guard on it, opens a can of worms.
> My vote is the same as it was before, merge this series and then revisit
> this function when someone has a use/test case for it. Nobody said this
> was never going to have a different implementation, just that the current
> implementation is known-buggy and there's no reason to expose it now.


It wouldn't be not guarded by default. In fact, in case we decide to go back
to what we were doing a couple of versions ago, I would rename the 'misa-w'
attribute to 'x-misa-w'.

The 'x' would be an indication that this is really something experimental and
expectations must be set accordingly if the user decides to enable it. In
reality, what this 'x-misa-w' would do is to give us more time to stabilize
the code inside write_misa(). Ideally we would get rid of it when the code
is stable.



Daniel

> 
> My only concern with the code deletion is that git-blame doesn't blame
> deleted code. I think we should add a comment describing the history
> which includes a git commit reference which can be used to see the
> latest implementation.
> 
> Thanks,
> drew
> 
>>
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>>
>>>
>>> Zhiwei
>>>
>>>> Given that allowing the dormant code to write MISA can cause tricky bugs
>>>> to solve later on, and we don't have a particularly interesting case of
>>>> writing MISA to support today, and we're already not violating the
>>>> specification, let's erase all the body of write_misa() and turn it into
>>>> an official no-op instead of an accidental one. We'll keep consistent
>>>> with what we provide users today but with 50+ less lines to maintain.
>>>>
>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one
>>>> else using it.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>> ---
>>>>    target/riscv/cpu.h |  1 -
>>>>    target/riscv/csr.c | 55 ----------------------------------------------
>>>>    2 files changed, 56 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>> index 7128438d8e..01803a020d 100644
>>>> --- a/target/riscv/cpu.h
>>>> +++ b/target/riscv/cpu.h
>>>> @@ -89,7 +89,6 @@ enum {
>>>>        RISCV_FEATURE_MMU,
>>>>        RISCV_FEATURE_PMP,
>>>>        RISCV_FEATURE_EPMP,
>>>> -    RISCV_FEATURE_MISA,
>>>>        RISCV_FEATURE_DEBUG
>>>>    };
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index 1b0a0c1693..f7862ff4a4 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>>>    static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>                                     target_ulong val)
>>>>    {
>>>> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
>>>> -        /* drop write to misa */
>>>> -        return RISCV_EXCP_NONE;
>>>> -    }
>>>> -
>>>> -    /* 'I' or 'E' must be present */
>>>> -    if (!(val & (RVI | RVE))) {
>>>> -        /* It is not, drop write to misa */
>>>> -        return RISCV_EXCP_NONE;
>>>> -    }
>>>> -
>>>> -    /* 'E' excludes all other extensions */
>>>> -    if (val & RVE) {
>>>> -        /* when we support 'E' we can do "val = RVE;" however
>>>> -         * for now we just drop writes if 'E' is present.
>>>> -         */
>>>> -        return RISCV_EXCP_NONE;
>>>> -    }
>>>> -
>>>> -    /*
>>>> -     * misa.MXL writes are not supported by QEMU.
>>>> -     * Drop writes to those bits.
>>>> -     */
>>>> -
>>>> -    /* Mask extensions that are not supported by this hart */
>>>> -    val &= env->misa_ext_mask;
>>>> -
>>>> -    /* Mask extensions that are not supported by QEMU */
>>>> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
>>>> -
>>>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>>>> -    if ((val & RVD) && !(val & RVF)) {
>>>> -        val &= ~RVD;
>>>> -    }
>>>> -
>>>> -    /* Suppress 'C' if next instruction is not aligned
>>>> -     * TODO: this should check next_pc
>>>> -     */
>>>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>>>> -        val &= ~RVC;
>>>> -    }
>>>> -
>>>> -    /* If nothing changed, do nothing. */
>>>> -    if (val == env->misa_ext) {
>>>> -        return RISCV_EXCP_NONE;
>>>> -    }
>>>> -
>>>> -    if (!(val & RVF)) {
>>>> -        env->mstatus &= ~MSTATUS_FS;
>>>> -    }
>>>> -
>>>> -    /* flush translation cache */
>>>> -    tb_flush(env_cpu(env));
>>>> -    env->misa_ext = val;
>>>> -    env->xl = riscv_cpu_mxl(env);
>>>>        return RISCV_EXCP_NONE;
>>>>    }

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Andrew Jones]

On Tue, Feb 21, 2023 at 03:22:45PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 2/21/23 14:06, Andrew Jones wrote:
> > On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote:
> > > Hey,
> > > 
> > > On 2/16/23 22:42, LIU Zhiwei wrote:
> > > > 
> > > > On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
> > > > > At this moment, and apparently since ever, we have no way of enabling
> > > > > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
> > > > > the nuts and bolts that handles how to write this CSR, has always been a
> > > > > no-op as well because write_misa() will always exit earlier.
> > > > > 
> > > > > This seems to be benign in the majority of cases. Booting an Ubuntu
> > > > > 'virt' guest and logging all the calls to 'write_misa' shows that no
> > > > > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
> > > > > RISC-V extensions after the machine is powered on, seems to be a niche
> > > > > use.
> > > > > 
> > > > > Before proceeding, let's recap what the spec says about MISA. It is a
> > > > > CSR that is divided in 3 fields:
> > > > > 
> > > > > - MXL, Machine XLEN, described as "may be writable";
> > > > > 
> > > > > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
> > > > > fixed value if MISA is zero;
> > > > > 
> > > > > - Extensions is defined as "a WARL field that can contain writable bits
> > > > > where the implementation allows the supported ISA to be modified"
> > > > > 
> > > > > Thus what we have today (write_misa() being a no-op) is already a valid
> > > > > spec implementation. We're not obliged to have a particular set of MISA
> > > > > writable bits, and at this moment we have none.
> > > > 
> > > > Hi Daniel,
> > > > 
> > > > I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation.
> > > > However, I still think we should make misa writable as default, which is also a valid spec implementation.
> > > > 
> > > > One reason is that may be we need to dynamic write  access for some cpus in the future. The other is we should
> > > > make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction.
> > > > 
> > > > I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently
> > > > can mostly be configurable already.
> > > 
> > > I don't have a strong opinion in this matter to be honest. My problems with the
> > > existing code are:
> > > 
> > > - the code is untested. I cannot say that this was never tested, but I can say that
> > > this has been mostly untested ever since introduced. Which is normal for a code that
> > > is 'dormant'.
> > > 
> > > - the code is dormant and most likely with bugs, but it's still maintained. For
> > > example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
> > > to make changes here. So we have the upkeep but no benefits.
> > > 
> > > - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
> > > applications seems to care either.
> > > 
> > > 
> > > All this said, I think we can reach a consensus of keeping it if we can at least come
> > > up with a way of testing it.
> > > 
> > > 
> > > > 
> > > > Your work is a good step towards to unify the configuration and the check.  I think two more steps we can go further.
> > > > 
> > > > 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct.
> > > > 
> > > > 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function.
> > > > 
> > > > 
> > > > As we have done these two steps, I think we can go more closely for the profile extension.
> > > 
> > > 
> > > Is this the extension you're taking about?
> > > 
> > > https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
> 
> Zhiwei, I looked it up and at first I don't understand how writing MISA is
> related to this profile extension. Are you suggesting that the firmware
> can choose to run a specific profile and then the hardware must adapt to
> it on the fly? Because if not, then we can implement profiles by just
> passing them in the QEMU command line.
> 
> > > 
> > > 
> > > This looks like a good reason to keep the code. Let's see if anyone else has an opinion
> > > about it. We can do the improvements you mentioned above as a follow-up (this series was
> > > really about removing RISC_FEATURE_*) if we decide to keep it.
> > 
> > If we decide to keep it and not guard it by default, then we should test
> > and fix it now. Also, as we're already aware that it has insufficient
> > sanity checks for extension dependencies, then we should fix our general
> > extension dependency checking now too, in order to apply that to this.
> > IOW, trying to keep this, without some guard on it, opens a can of worms.
> > My vote is the same as it was before, merge this series and then revisit
> > this function when someone has a use/test case for it. Nobody said this
> > was never going to have a different implementation, just that the current
> > implementation is known-buggy and there's no reason to expose it now.
> 
> 
> It wouldn't be not guarded by default. In fact, in case we decide to go back
> to what we were doing a couple of versions ago, I would rename the 'misa-w'
> attribute to 'x-misa-w'.
> 
> The 'x' would be an indication that this is really something experimental and
> expectations must be set accordingly if the user decides to enable it. In
> reality, what this 'x-misa-w' would do is to give us more time to stabilize
> the code inside write_misa(). Ideally we would get rid of it when the code
> is stable.

I'm good with bringing the code back under x-misa-w, which is off by
default.

Thanks,
drew

> 
> 
> 
> Daniel
> 
> > 
> > My only concern with the code deletion is that git-blame doesn't blame
> > deleted code. I think we should add a comment describing the history
> > which includes a git commit reference which can be used to see the
> > latest implementation.
> > 
> > Thanks,
> > drew
> > 
> > > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > 
> > > Daniel
> > > 
> > > > 
> > > > 
> > > > Zhiwei
> > > > 
> > > > > Given that allowing the dormant code to write MISA can cause tricky bugs
> > > > > to solve later on, and we don't have a particularly interesting case of
> > > > > writing MISA to support today, and we're already not violating the
> > > > > specification, let's erase all the body of write_misa() and turn it into
> > > > > an official no-op instead of an accidental one. We'll keep consistent
> > > > > with what we provide users today but with 50+ less lines to maintain.
> > > > > 
> > > > > RISCV_FEATURE_MISA enum is erased in the process since there's no one
> > > > > else using it.
> > > > > 
> > > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > > > Reviewed-by: Bin Meng <bmeng@tinylab.org>
> > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > > > > ---
> > > > >    target/riscv/cpu.h |  1 -
> > > > >    target/riscv/csr.c | 55 ----------------------------------------------
> > > > >    2 files changed, 56 deletions(-)
> > > > > 
> > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > > > index 7128438d8e..01803a020d 100644
> > > > > --- a/target/riscv/cpu.h
> > > > > +++ b/target/riscv/cpu.h
> > > > > @@ -89,7 +89,6 @@ enum {
> > > > >        RISCV_FEATURE_MMU,
> > > > >        RISCV_FEATURE_PMP,
> > > > >        RISCV_FEATURE_EPMP,
> > > > > -    RISCV_FEATURE_MISA,
> > > > >        RISCV_FEATURE_DEBUG
> > > > >    };
> > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > > > index 1b0a0c1693..f7862ff4a4 100644
> > > > > --- a/target/riscv/csr.c
> > > > > +++ b/target/riscv/csr.c
> > > > > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> > > > >    static RISCVException write_misa(CPURISCVState *env, int csrno,
> > > > >                                     target_ulong val)
> > > > >    {
> > > > > -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
> > > > > -        /* drop write to misa */
> > > > > -        return RISCV_EXCP_NONE;
> > > > > -    }
> > > > > -
> > > > > -    /* 'I' or 'E' must be present */
> > > > > -    if (!(val & (RVI | RVE))) {
> > > > > -        /* It is not, drop write to misa */
> > > > > -        return RISCV_EXCP_NONE;
> > > > > -    }
> > > > > -
> > > > > -    /* 'E' excludes all other extensions */
> > > > > -    if (val & RVE) {
> > > > > -        /* when we support 'E' we can do "val = RVE;" however
> > > > > -         * for now we just drop writes if 'E' is present.
> > > > > -         */
> > > > > -        return RISCV_EXCP_NONE;
> > > > > -    }
> > > > > -
> > > > > -    /*
> > > > > -     * misa.MXL writes are not supported by QEMU.
> > > > > -     * Drop writes to those bits.
> > > > > -     */
> > > > > -
> > > > > -    /* Mask extensions that are not supported by this hart */
> > > > > -    val &= env->misa_ext_mask;
> > > > > -
> > > > > -    /* Mask extensions that are not supported by QEMU */
> > > > > -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
> > > > > -
> > > > > -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> > > > > -    if ((val & RVD) && !(val & RVF)) {
> > > > > -        val &= ~RVD;
> > > > > -    }
> > > > > -
> > > > > -    /* Suppress 'C' if next instruction is not aligned
> > > > > -     * TODO: this should check next_pc
> > > > > -     */
> > > > > -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > > > > -        val &= ~RVC;
> > > > > -    }
> > > > > -
> > > > > -    /* If nothing changed, do nothing. */
> > > > > -    if (val == env->misa_ext) {
> > > > > -        return RISCV_EXCP_NONE;
> > > > > -    }
> > > > > -
> > > > > -    if (!(val & RVF)) {
> > > > > -        env->mstatus &= ~MSTATUS_FS;
> > > > > -    }
> > > > > -
> > > > > -    /* flush translation cache */
> > > > > -    tb_flush(env_cpu(env));
> > > > > -    env->misa_ext = val;
> > > > > -    env->xl = riscv_cpu_mxl(env);
> > > > >        return RISCV_EXCP_NONE;
> > > > >    }

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Daniel Henrique Barboza]

On 2/21/23 15:28, Andrew Jones wrote:
> On Tue, Feb 21, 2023 at 03:22:45PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 2/21/23 14:06, Andrew Jones wrote:
>>> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote:
>>>> Hey,
>>>>
>>>> On 2/16/23 22:42, LIU Zhiwei wrote:
>>>>>
>>>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>>>>>> At this moment, and apparently since ever, we have no way of enabling
>>>>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
>>>>>> the nuts and bolts that handles how to write this CSR, has always been a
>>>>>> no-op as well because write_misa() will always exit earlier.
>>>>>>
>>>>>> This seems to be benign in the majority of cases. Booting an Ubuntu
>>>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no
>>>>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
>>>>>> RISC-V extensions after the machine is powered on, seems to be a niche
>>>>>> use.
>>>>>>
>>>>>> Before proceeding, let's recap what the spec says about MISA. It is a
>>>>>> CSR that is divided in 3 fields:
>>>>>>
>>>>>> - MXL, Machine XLEN, described as "may be writable";
>>>>>>
>>>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
>>>>>> fixed value if MISA is zero;
>>>>>>
>>>>>> - Extensions is defined as "a WARL field that can contain writable bits
>>>>>> where the implementation allows the supported ISA to be modified"
>>>>>>
>>>>>> Thus what we have today (write_misa() being a no-op) is already a valid
>>>>>> spec implementation. We're not obliged to have a particular set of MISA
>>>>>> writable bits, and at this moment we have none.
>>>>>
>>>>> Hi Daniel,
>>>>>
>>>>> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation.
>>>>> However, I still think we should make misa writable as default, which is also a valid spec implementation.
>>>>>
>>>>> One reason is that may be we need to dynamic write  access for some cpus in the future. The other is we should
>>>>> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction.
>>>>>
>>>>> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently
>>>>> can mostly be configurable already.
>>>>
>>>> I don't have a strong opinion in this matter to be honest. My problems with the
>>>> existing code are:
>>>>
>>>> - the code is untested. I cannot say that this was never tested, but I can say that
>>>> this has been mostly untested ever since introduced. Which is normal for a code that
>>>> is 'dormant'.
>>>>
>>>> - the code is dormant and most likely with bugs, but it's still maintained. For
>>>> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
>>>> to make changes here. So we have the upkeep but no benefits.
>>>>
>>>> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
>>>> applications seems to care either.
>>>>
>>>>
>>>> All this said, I think we can reach a consensus of keeping it if we can at least come
>>>> up with a way of testing it.
>>>>
>>>>
>>>>>
>>>>> Your work is a good step towards to unify the configuration and the check.  I think two more steps we can go further.
>>>>>
>>>>> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct.
>>>>>
>>>>> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function.
>>>>>
>>>>>
>>>>> As we have done these two steps, I think we can go more closely for the profile extension.
>>>>
>>>>
>>>> Is this the extension you're taking about?
>>>>
>>>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
>>
>> Zhiwei, I looked it up and at first I don't understand how writing MISA is
>> related to this profile extension. Are you suggesting that the firmware
>> can choose to run a specific profile and then the hardware must adapt to
>> it on the fly? Because if not, then we can implement profiles by just
>> passing them in the QEMU command line.
>>
>>>>
>>>>
>>>> This looks like a good reason to keep the code. Let's see if anyone else has an opinion
>>>> about it. We can do the improvements you mentioned above as a follow-up (this series was
>>>> really about removing RISC_FEATURE_*) if we decide to keep it.
>>>
>>> If we decide to keep it and not guard it by default, then we should test
>>> and fix it now. Also, as we're already aware that it has insufficient
>>> sanity checks for extension dependencies, then we should fix our general
>>> extension dependency checking now too, in order to apply that to this.
>>> IOW, trying to keep this, without some guard on it, opens a can of worms.
>>> My vote is the same as it was before, merge this series and then revisit
>>> this function when someone has a use/test case for it. Nobody said this
>>> was never going to have a different implementation, just that the current
>>> implementation is known-buggy and there's no reason to expose it now.
>>
>>
>> It wouldn't be not guarded by default. In fact, in case we decide to go back
>> to what we were doing a couple of versions ago, I would rename the 'misa-w'
>> attribute to 'x-misa-w'.
>>
>> The 'x' would be an indication that this is really something experimental and
>> expectations must be set accordingly if the user decides to enable it. In
>> reality, what this 'x-misa-w' would do is to give us more time to stabilize
>> the code inside write_misa(). Ideally we would get rid of it when the code
>> is stable.
> 
> I'm good with bringing the code back under x-misa-w, which is off by
> default.

Ok! If no one else opposes I'll re-send v7 with write_misa() gated with x-misa-w.



Daniel

> 
> Thanks,
> drew
> 
>>
>>
>>
>> Daniel
>>
>>>
>>> My only concern with the code deletion is that git-blame doesn't blame
>>> deleted code. I think we should add a comment describing the history
>>> which includes a git commit reference which can be used to see the
>>> latest implementation.
>>>
>>> Thanks,
>>> drew
>>>
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>>
>>>> Daniel
>>>>
>>>>>
>>>>>
>>>>> Zhiwei
>>>>>
>>>>>> Given that allowing the dormant code to write MISA can cause tricky bugs
>>>>>> to solve later on, and we don't have a particularly interesting case of
>>>>>> writing MISA to support today, and we're already not violating the
>>>>>> specification, let's erase all the body of write_misa() and turn it into
>>>>>> an official no-op instead of an accidental one. We'll keep consistent
>>>>>> with what we provide users today but with 50+ less lines to maintain.
>>>>>>
>>>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one
>>>>>> else using it.
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>>>> ---
>>>>>>     target/riscv/cpu.h |  1 -
>>>>>>     target/riscv/csr.c | 55 ----------------------------------------------
>>>>>>     2 files changed, 56 deletions(-)
>>>>>>
>>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>>> index 7128438d8e..01803a020d 100644
>>>>>> --- a/target/riscv/cpu.h
>>>>>> +++ b/target/riscv/cpu.h
>>>>>> @@ -89,7 +89,6 @@ enum {
>>>>>>         RISCV_FEATURE_MMU,
>>>>>>         RISCV_FEATURE_PMP,
>>>>>>         RISCV_FEATURE_EPMP,
>>>>>> -    RISCV_FEATURE_MISA,
>>>>>>         RISCV_FEATURE_DEBUG
>>>>>>     };
>>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>>> index 1b0a0c1693..f7862ff4a4 100644
>>>>>> --- a/target/riscv/csr.c
>>>>>> +++ b/target/riscv/csr.c
>>>>>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>>>>>     static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>>>                                      target_ulong val)
>>>>>>     {
>>>>>> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
>>>>>> -        /* drop write to misa */
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* 'I' or 'E' must be present */
>>>>>> -    if (!(val & (RVI | RVE))) {
>>>>>> -        /* It is not, drop write to misa */
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* 'E' excludes all other extensions */
>>>>>> -    if (val & RVE) {
>>>>>> -        /* when we support 'E' we can do "val = RVE;" however
>>>>>> -         * for now we just drop writes if 'E' is present.
>>>>>> -         */
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    /*
>>>>>> -     * misa.MXL writes are not supported by QEMU.
>>>>>> -     * Drop writes to those bits.
>>>>>> -     */
>>>>>> -
>>>>>> -    /* Mask extensions that are not supported by this hart */
>>>>>> -    val &= env->misa_ext_mask;
>>>>>> -
>>>>>> -    /* Mask extensions that are not supported by QEMU */
>>>>>> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
>>>>>> -
>>>>>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>>>>>> -    if ((val & RVD) && !(val & RVF)) {
>>>>>> -        val &= ~RVD;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* Suppress 'C' if next instruction is not aligned
>>>>>> -     * TODO: this should check next_pc
>>>>>> -     */
>>>>>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>>>>>> -        val &= ~RVC;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* If nothing changed, do nothing. */
>>>>>> -    if (val == env->misa_ext) {
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    if (!(val & RVF)) {
>>>>>> -        env->mstatus &= ~MSTATUS_FS;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* flush translation cache */
>>>>>> -    tb_flush(env_cpu(env));
>>>>>> -    env->misa_ext = val;
>>>>>> -    env->xl = riscv_cpu_mxl(env);
>>>>>>         return RISCV_EXCP_NONE;
>>>>>>     }

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[LIU Zhiwei]

On 2023/2/22 2:22, Daniel Henrique Barboza wrote:
>
>
> On 2/21/23 14:06, Andrew Jones wrote:
>> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote:
>>> Hey,
>>>
>>> On 2/16/23 22:42, LIU Zhiwei wrote:
>>>>
>>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>>>>> At this moment, and apparently since ever, we have no way of enabling
>>>>> RISCV_FEATURE_MISA. This means that all the code from 
>>>>> write_misa(), all
>>>>> the nuts and bolts that handles how to write this CSR, has always 
>>>>> been a
>>>>> no-op as well because write_misa() will always exit earlier.
>>>>>
>>>>> This seems to be benign in the majority of cases. Booting an Ubuntu
>>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no
>>>>> writes to MISA CSR was attempted. Writing MISA, i.e. 
>>>>> enabling/disabling
>>>>> RISC-V extensions after the machine is powered on, seems to be a 
>>>>> niche
>>>>> use.
>>>>>
>>>>> Before proceeding, let's recap what the spec says about MISA. It is a
>>>>> CSR that is divided in 3 fields:
>>>>>
>>>>> - MXL, Machine XLEN, described as "may be writable";
>>>>>
>>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL 
>>>>> or a
>>>>> fixed value if MISA is zero;
>>>>>
>>>>> - Extensions is defined as "a WARL field that can contain writable 
>>>>> bits
>>>>> where the implementation allows the supported ISA to be modified"
>>>>>
>>>>> Thus what we have today (write_misa() being a no-op) is already a 
>>>>> valid
>>>>> spec implementation. We're not obliged to have a particular set of 
>>>>> MISA
>>>>> writable bits, and at this moment we have none.
>>>>
>>>> Hi Daniel,
>>>>
>>>> I see there has been a discussion on this topic. And as no-op has 
>>>> no harmfulness for current implementation.
>>>> However, I still think we should make misa writable as default, 
>>>> which is also a valid spec implementation.
>>>>
>>>> One reason is that may be we need to dynamic write  access for some 
>>>> cpus in the future. The other is we should
>>>> make QEMU a more useful implementation, not just a legal 
>>>> implementation. We have done in many aspects on this direction.
>>>>
>>>> I prefer your implementation before v4. It's not a complicated 
>>>> implementation. And I think the other extensions on QEMU currently
>>>> can mostly be configurable already.
>>>
>>> I don't have a strong opinion in this matter to be honest. My 
>>> problems with the
>>> existing code are:
>>>
>>> - the code is untested. I cannot say that this was never tested, but 
>>> I can say that
>>> this has been mostly untested ever since introduced. Which is normal 
>>> for a code that
>>> is 'dormant'.
>>>
>>> - the code is dormant and most likely with bugs, but it's still 
>>> maintained. For
>>> example we have e91a7227 ("target/riscv: Split misa.mxl and 
>>> misa.ext") that had
>>> to make changes here. So we have the upkeep but no benefits.
>>>
>>> - we don't have an use case for it. Most OSes doesn't seem to care, 
>>> and afaik no
>>> applications seems to care either.
>>>
>>>
>>> All this said, I think we can reach a consensus of keeping it if we 
>>> can at least come
>>> up with a way of testing it.
>>>
>>>
>>>>
>>>> Your work is a good step towards to unify the configuration and the 
>>>> check.  I think two more steps we can go further.
>>>>
>>>> 1) Remove RVI/RVF and the similar macros, and add fields for them 
>>>> in the configuration struct.
>>>>
>>>> 2) Unify the check about configuration. write_misa and 
>>>> cpu_realize_fn can use the same check function.
>>>>
>>>>
>>>> As we have done these two steps, I think we can go more closely for 
>>>> the profile extension.
>>>
>>>
>>> Is this the extension you're taking about?
>>>
>>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
>
> Zhiwei, I looked it up and at first I don't understand how writing 
> MISA is
> related to this profile extension. Are you suggesting that the firmware
> can choose to run a specific profile and then the hardware must adapt to
> it on the fly? Because if not, then we can implement profiles by just
> passing them in the QEMU command line.

No, there is not a directly dependency relation between writing MISA and 
profile extension.
I think we have too many places for configuring the extensions ( ISA or 
Non-ISA) and we don't have a unified check function.

For example:

1) The cpu init function.

2) The -cpu property

3) wite_misa

And if supported,  the profile will be converted into the combination of 
extensions in cfg. It will add a new configuration point.
Thus I think we should unify the check function before introduce the 
more configuration place.

My initial idea is that we always use the cfg fields for configuration 
and misa is only a mapping of combination cfg fields. And the unified 
check function
only works on cfg field.

And of course,  you can omit this comment for this patch set.

Zhiwei

>
>>>
>>>
>>> This looks like a good reason to keep the code. Let's see if anyone 
>>> else has an opinion
>>> about it. We can do the improvements you mentioned above as a 
>>> follow-up (this series was
>>> really about removing RISC_FEATURE_*) if we decide to keep it.
>>
>> If we decide to keep it and not guard it by default, then we should test
>> and fix it now. Also, as we're already aware that it has insufficient
>> sanity checks for extension dependencies, then we should fix our general
>> extension dependency checking now too, in order to apply that to this.
>> IOW, trying to keep this, without some guard on it, opens a can of 
>> worms.
>> My vote is the same as it was before, merge this series and then revisit
>> this function when someone has a use/test case for it. Nobody said this
>> was never going to have a different implementation, just that the 
>> current
>> implementation is known-buggy and there's no reason to expose it now.
>
>
> It wouldn't be not guarded by default. In fact, in case we decide to 
> go back
> to what we were doing a couple of versions ago, I would rename the 
> 'misa-w'
> attribute to 'x-misa-w'.
>
> The 'x' would be an indication that this is really something 
> experimental and
> expectations must be set accordingly if the user decides to enable it. In
> reality, what this 'x-misa-w' would do is to give us more time to 
> stabilize
> the code inside write_misa(). Ideally we would get rid of it when the 
> code
> is stable.
>
>
>
> Daniel
>
>>
>> My only concern with the code deletion is that git-blame doesn't blame
>> deleted code. I think we should add a comment describing the history
>> which includes a git commit reference which can be used to see the
>> latest implementation.
>>
>> Thanks,
>> drew
>>
>>>
>>>
>>>
>>> Thanks,
>>>
>>>
>>> Daniel
>>>
>>>>
>>>>
>>>> Zhiwei
>>>>
>>>>> Given that allowing the dormant code to write MISA can cause 
>>>>> tricky bugs
>>>>> to solve later on, and we don't have a particularly interesting 
>>>>> case of
>>>>> writing MISA to support today, and we're already not violating the
>>>>> specification, let's erase all the body of write_misa() and turn 
>>>>> it into
>>>>> an official no-op instead of an accidental one. We'll keep consistent
>>>>> with what we provide users today but with 50+ less lines to maintain.
>>>>>
>>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one
>>>>> else using it.
>>>>>
>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>>> ---
>>>>>    target/riscv/cpu.h |  1 -
>>>>>    target/riscv/csr.c | 55 
>>>>> ----------------------------------------------
>>>>>    2 files changed, 56 deletions(-)
>>>>>
>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>> index 7128438d8e..01803a020d 100644
>>>>> --- a/target/riscv/cpu.h
>>>>> +++ b/target/riscv/cpu.h
>>>>> @@ -89,7 +89,6 @@ enum {
>>>>>        RISCV_FEATURE_MMU,
>>>>>        RISCV_FEATURE_PMP,
>>>>>        RISCV_FEATURE_EPMP,
>>>>> -    RISCV_FEATURE_MISA,
>>>>>        RISCV_FEATURE_DEBUG
>>>>>    };
>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>> index 1b0a0c1693..f7862ff4a4 100644
>>>>> --- a/target/riscv/csr.c
>>>>> +++ b/target/riscv/csr.c
>>>>> @@ -1329,61 +1329,6 @@ static RISCVException 
>>>>> read_misa(CPURISCVState *env, int csrno,
>>>>>    static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>>                                     target_ulong val)
>>>>>    {
>>>>> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
>>>>> -        /* drop write to misa */
>>>>> -        return RISCV_EXCP_NONE;
>>>>> -    }
>>>>> -
>>>>> -    /* 'I' or 'E' must be present */
>>>>> -    if (!(val & (RVI | RVE))) {
>>>>> -        /* It is not, drop write to misa */
>>>>> -        return RISCV_EXCP_NONE;
>>>>> -    }
>>>>> -
>>>>> -    /* 'E' excludes all other extensions */
>>>>> -    if (val & RVE) {
>>>>> -        /* when we support 'E' we can do "val = RVE;" however
>>>>> -         * for now we just drop writes if 'E' is present.
>>>>> -         */
>>>>> -        return RISCV_EXCP_NONE;
>>>>> -    }
>>>>> -
>>>>> -    /*
>>>>> -     * misa.MXL writes are not supported by QEMU.
>>>>> -     * Drop writes to those bits.
>>>>> -     */
>>>>> -
>>>>> -    /* Mask extensions that are not supported by this hart */
>>>>> -    val &= env->misa_ext_mask;
>>>>> -
>>>>> -    /* Mask extensions that are not supported by QEMU */
>>>>> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | 
>>>>> RVV);
>>>>> -
>>>>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>>>>> -    if ((val & RVD) && !(val & RVF)) {
>>>>> -        val &= ~RVD;
>>>>> -    }
>>>>> -
>>>>> -    /* Suppress 'C' if next instruction is not aligned
>>>>> -     * TODO: this should check next_pc
>>>>> -     */
>>>>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>>>>> -        val &= ~RVC;
>>>>> -    }
>>>>> -
>>>>> -    /* If nothing changed, do nothing. */
>>>>> -    if (val == env->misa_ext) {
>>>>> -        return RISCV_EXCP_NONE;
>>>>> -    }
>>>>> -
>>>>> -    if (!(val & RVF)) {
>>>>> -        env->mstatus &= ~MSTATUS_FS;
>>>>> -    }
>>>>> -
>>>>> -    /* flush translation cache */
>>>>> -    tb_flush(env_cpu(env));
>>>>> -    env->misa_ext = val;
>>>>> -    env->xl = riscv_cpu_mxl(env);
>>>>>        return RISCV_EXCP_NONE;
>>>>>    }

Re: [PATCH v6 1/9] target/riscv: turn write_misa() into an official no-op

[Daniel Henrique Barboza]

On 2/22/23 06:21, LIU Zhiwei wrote:
> 
> On 2023/2/22 2:22, Daniel Henrique Barboza wrote:
>>
>>
>> On 2/21/23 14:06, Andrew Jones wrote:
>>> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote:
>>>> Hey,
>>>>
>>>> On 2/16/23 22:42, LIU Zhiwei wrote:
>>>>>
>>>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote:
>>>>>> At this moment, and apparently since ever, we have no way of enabling
>>>>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all
>>>>>> the nuts and bolts that handles how to write this CSR, has always been a
>>>>>> no-op as well because write_misa() will always exit earlier.
>>>>>>
>>>>>> This seems to be benign in the majority of cases. Booting an Ubuntu
>>>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no
>>>>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling
>>>>>> RISC-V extensions after the machine is powered on, seems to be a niche
>>>>>> use.
>>>>>>
>>>>>> Before proceeding, let's recap what the spec says about MISA. It is a
>>>>>> CSR that is divided in 3 fields:
>>>>>>
>>>>>> - MXL, Machine XLEN, described as "may be writable";
>>>>>>
>>>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a
>>>>>> fixed value if MISA is zero;
>>>>>>
>>>>>> - Extensions is defined as "a WARL field that can contain writable bits
>>>>>> where the implementation allows the supported ISA to be modified"
>>>>>>
>>>>>> Thus what we have today (write_misa() being a no-op) is already a valid
>>>>>> spec implementation. We're not obliged to have a particular set of MISA
>>>>>> writable bits, and at this moment we have none.
>>>>>
>>>>> Hi Daniel,
>>>>>
>>>>> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation.
>>>>> However, I still think we should make misa writable as default, which is also a valid spec implementation.
>>>>>
>>>>> One reason is that may be we need to dynamic write  access for some cpus in the future. The other is we should
>>>>> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction.
>>>>>
>>>>> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently
>>>>> can mostly be configurable already.
>>>>
>>>> I don't have a strong opinion in this matter to be honest. My problems with the
>>>> existing code are:
>>>>
>>>> - the code is untested. I cannot say that this was never tested, but I can say that
>>>> this has been mostly untested ever since introduced. Which is normal for a code that
>>>> is 'dormant'.
>>>>
>>>> - the code is dormant and most likely with bugs, but it's still maintained. For
>>>> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had
>>>> to make changes here. So we have the upkeep but no benefits.
>>>>
>>>> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no
>>>> applications seems to care either.
>>>>
>>>>
>>>> All this said, I think we can reach a consensus of keeping it if we can at least come
>>>> up with a way of testing it.
>>>>
>>>>
>>>>>
>>>>> Your work is a good step towards to unify the configuration and the check.  I think two more steps we can go further.
>>>>>
>>>>> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct.
>>>>>
>>>>> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function.
>>>>>
>>>>>
>>>>> As we have done these two steps, I think we can go more closely for the profile extension.
>>>>
>>>>
>>>> Is this the extension you're taking about?
>>>>
>>>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
>>
>> Zhiwei, I looked it up and at first I don't understand how writing MISA is
>> related to this profile extension. Are you suggesting that the firmware
>> can choose to run a specific profile and then the hardware must adapt to
>> it on the fly? Because if not, then we can implement profiles by just
>> passing them in the QEMU command line.
> 
> No, there is not a directly dependency relation between writing MISA and profile extension.
> I think we have too many places for configuring the extensions ( ISA or Non-ISA) and we don't have a unified check function.
> 
> For example:
> 
> 1) The cpu init function.
> 
> 2) The -cpu property
> 
> 3) wite_misa
> 
> And if supported,  the profile will be converted into the combination of extensions in cfg. It will add a new configuration point.
> Thus I think we should unify the check function before introduce the more configuration place.
> 
> My initial idea is that we always use the cfg fields for configuration and misa is only a mapping of combination cfg fields. And the unified check function
> only works on cfg field.
> 
> And of course,  you can omit this comment for this patch set.

What I'm going to do then is to go back to the approach taken in v3, but gating
the write_misa() implementation with a x-misa-w experimental attribute. The
attribute will be default off. This will give us time to work on the cfg
improvements you talked about while still having the write_misa() code around.

I have some ideas about how we can have a centered configuration point like you
suggested. The code freeze is March 7th, so no promises on having this fully
done for this release cycle. A few cleanups here and there would be nice, so
let's start small and see how that goes.


Thanks,

Daniel

> 
> Zhiwei
> 
>>
>>>>
>>>>
>>>> This looks like a good reason to keep the code. Let's see if anyone else has an opinion
>>>> about it. We can do the improvements you mentioned above as a follow-up (this series was
>>>> really about removing RISC_FEATURE_*) if we decide to keep it.
>>>
>>> If we decide to keep it and not guard it by default, then we should test
>>> and fix it now. Also, as we're already aware that it has insufficient
>>> sanity checks for extension dependencies, then we should fix our general
>>> extension dependency checking now too, in order to apply that to this.
>>> IOW, trying to keep this, without some guard on it, opens a can of worms.
>>> My vote is the same as it was before, merge this series and then revisit
>>> this function when someone has a use/test case for it. Nobody said this
>>> was never going to have a different implementation, just that the current
>>> implementation is known-buggy and there's no reason to expose it now.
>>
>>
>> It wouldn't be not guarded by default. In fact, in case we decide to go back
>> to what we were doing a couple of versions ago, I would rename the 'misa-w'
>> attribute to 'x-misa-w'.
>>
>> The 'x' would be an indication that this is really something experimental and
>> expectations must be set accordingly if the user decides to enable it. In
>> reality, what this 'x-misa-w' would do is to give us more time to stabilize
>> the code inside write_misa(). Ideally we would get rid of it when the code
>> is stable.
>>
>>
>>
>> Daniel
>>
>>>
>>> My only concern with the code deletion is that git-blame doesn't blame
>>> deleted code. I think we should add a comment describing the history
>>> which includes a git commit reference which can be used to see the
>>> latest implementation.
>>>
>>> Thanks,
>>> drew
>>>
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>>
>>>> Daniel
>>>>
>>>>>
>>>>>
>>>>> Zhiwei
>>>>>
>>>>>> Given that allowing the dormant code to write MISA can cause tricky bugs
>>>>>> to solve later on, and we don't have a particularly interesting case of
>>>>>> writing MISA to support today, and we're already not violating the
>>>>>> specification, let's erase all the body of write_misa() and turn it into
>>>>>> an official no-op instead of an accidental one. We'll keep consistent
>>>>>> with what we provide users today but with 50+ less lines to maintain.
>>>>>>
>>>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one
>>>>>> else using it.
>>>>>>
>>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org>
>>>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>>>>> ---
>>>>>>    target/riscv/cpu.h |  1 -
>>>>>>    target/riscv/csr.c | 55 ----------------------------------------------
>>>>>>    2 files changed, 56 deletions(-)
>>>>>>
>>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>>>> index 7128438d8e..01803a020d 100644
>>>>>> --- a/target/riscv/cpu.h
>>>>>> +++ b/target/riscv/cpu.h
>>>>>> @@ -89,7 +89,6 @@ enum {
>>>>>>        RISCV_FEATURE_MMU,
>>>>>>        RISCV_FEATURE_PMP,
>>>>>>        RISCV_FEATURE_EPMP,
>>>>>> -    RISCV_FEATURE_MISA,
>>>>>>        RISCV_FEATURE_DEBUG
>>>>>>    };
>>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>>> index 1b0a0c1693..f7862ff4a4 100644
>>>>>> --- a/target/riscv/csr.c
>>>>>> +++ b/target/riscv/csr.c
>>>>>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>>>>>    static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>>>                                     target_ulong val)
>>>>>>    {
>>>>>> -    if (!riscv_feature(env, RISCV_FEATURE_MISA)) {
>>>>>> -        /* drop write to misa */
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* 'I' or 'E' must be present */
>>>>>> -    if (!(val & (RVI | RVE))) {
>>>>>> -        /* It is not, drop write to misa */
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* 'E' excludes all other extensions */
>>>>>> -    if (val & RVE) {
>>>>>> -        /* when we support 'E' we can do "val = RVE;" however
>>>>>> -         * for now we just drop writes if 'E' is present.
>>>>>> -         */
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    /*
>>>>>> -     * misa.MXL writes are not supported by QEMU.
>>>>>> -     * Drop writes to those bits.
>>>>>> -     */
>>>>>> -
>>>>>> -    /* Mask extensions that are not supported by this hart */
>>>>>> -    val &= env->misa_ext_mask;
>>>>>> -
>>>>>> -    /* Mask extensions that are not supported by QEMU */
>>>>>> -    val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV);
>>>>>> -
>>>>>> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
>>>>>> -    if ((val & RVD) && !(val & RVF)) {
>>>>>> -        val &= ~RVD;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* Suppress 'C' if next instruction is not aligned
>>>>>> -     * TODO: this should check next_pc
>>>>>> -     */
>>>>>> -    if ((val & RVC) && (GETPC() & ~3) != 0) {
>>>>>> -        val &= ~RVC;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* If nothing changed, do nothing. */
>>>>>> -    if (val == env->misa_ext) {
>>>>>> -        return RISCV_EXCP_NONE;
>>>>>> -    }
>>>>>> -
>>>>>> -    if (!(val & RVF)) {
>>>>>> -        env->mstatus &= ~MSTATUS_FS;
>>>>>> -    }
>>>>>> -
>>>>>> -    /* flush translation cache */
>>>>>> -    tb_flush(env_cpu(env));
>>>>>> -    env->misa_ext = val;
>>>>>> -    env->xl = riscv_cpu_mxl(env);
>>>>>>        return RISCV_EXCP_NONE;
>>>>>>    }