[PATCH v8 00/11] target/riscv: rework CPU extension validation

[PATCH v8 00/11] target/riscv: rework CPU extension validation

[Daniel Henrique Barboza]

Hi,

This version dropped patch 12 from v7. Alistair mentioned that it would
limiti static CPUs needlesly, since there's nothing preventing a static
CPU to allow for extension changes during runtime, and that misa-w is
enough to prevent write_misa() during runtime. I agree.

I also amended the commit message of patch 11 to remove any mention
about misa_ext_mask rollbacks (which the code wasn't doing). The patch
itself is changing validate_set_extensions() to avoid misa_ext_mask to
be overwritten during write_misa() in the first place, so no
misa_ext_mask rollback should be even mentioned.


Changes from v7:
- patch 12: drooped
- patch 11: use a sane commit message.
- v7 link: https://lists.gnu.org/archive/html/qemu-devel/2023-04/msg02916.html

Daniel Henrique Barboza (9):
  target/riscv/cpu.c: add riscv_cpu_validate_v()
  target/riscv/cpu.c: remove set_vext_version()
  target/riscv/cpu.c: remove set_priv_version()
  target/riscv: add PRIV_VERSION_LATEST
  target/riscv/cpu.c: add priv_spec validate/disable_exts helpers
  target/riscv/cpu.c: add riscv_cpu_validate_misa_mxl()
  target/riscv/cpu.c: validate extensions before riscv_timer_init()
  target/riscv/cpu.c: remove cfg setup from riscv_cpu_init()
  target/riscv: rework write_misa()

Weiwei Li (2):
  target/riscv: Mask the implicitly enabled extensions in isa_string
    based on priv version
  target/riscv: Update check for Zca/Zcf/Zcd

 target/riscv/cpu.c                      | 333 ++++++++++++++----------
 target/riscv/cpu.h                      |   3 +
 target/riscv/csr.c                      |  47 ++--
 target/riscv/insn_trans/trans_rvd.c.inc |  12 +-
 target/riscv/insn_trans/trans_rvf.c.inc |  14 +-
 target/riscv/insn_trans/trans_rvi.c.inc |   5 +-
 target/riscv/translate.c                |   5 +-
 7 files changed, 244 insertions(+), 175 deletions(-)

-- 
2.40.0

[PATCH v8 01/11] target/riscv/cpu.c: add riscv_cpu_validate_v()

[Daniel Henrique Barboza]

The RVV verification will error out if fails and it's being done at the
end of riscv_cpu_validate_set_extensions(), after we've already set some
extensions that are dependent on RVV.  Let's put it in its own function
and do it earlier.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 89 +++++++++++++++++++++++++---------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index befa64528f..feca13aefb 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -797,6 +797,46 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
     }
 }
 
+static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
+                                 Error **errp)
+{
+    int vext_version = VEXT_VERSION_1_00_0;
+
+    if (!is_power_of_2(cfg->vlen)) {
+        error_setg(errp, "Vector extension VLEN must be power of 2");
+        return;
+    }
+    if (cfg->vlen > RV_VLEN_MAX || cfg->vlen < 128) {
+        error_setg(errp,
+                   "Vector extension implementation only supports VLEN "
+                   "in the range [128, %d]", RV_VLEN_MAX);
+        return;
+    }
+    if (!is_power_of_2(cfg->elen)) {
+        error_setg(errp, "Vector extension ELEN must be power of 2");
+        return;
+    }
+    if (cfg->elen > 64 || cfg->elen < 8) {
+        error_setg(errp,
+                   "Vector extension implementation only supports ELEN "
+                   "in the range [8, 64]");
+        return;
+    }
+    if (cfg->vext_spec) {
+        if (!g_strcmp0(cfg->vext_spec, "v1.0")) {
+            vext_version = VEXT_VERSION_1_00_0;
+        } else {
+            error_setg(errp, "Unsupported vector spec version '%s'",
+                       cfg->vext_spec);
+            return;
+        }
+    } else {
+        qemu_log("vector version is not specified, "
+                 "use the default value v1.0\n");
+    }
+    set_vext_version(env, vext_version);
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
@@ -804,6 +844,7 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 {
     CPURISCVState *env = &cpu->env;
+    Error *local_err = NULL;
 
     /* Do some ISA extension error checking */
     if (riscv_has_ext(env, RVG) &&
@@ -872,8 +913,14 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    /* The V vector extension depends on the Zve64d extension */
     if (riscv_has_ext(env, RVV)) {
+        riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+
+        /* The V vector extension depends on the Zve64d extension */
         cpu->cfg.ext_zve64d = true;
     }
 
@@ -1008,46 +1055,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_zksed = true;
         cpu->cfg.ext_zksh = true;
     }
-
-    if (riscv_has_ext(env, RVV)) {
-        int vext_version = VEXT_VERSION_1_00_0;
-        if (!is_power_of_2(cpu->cfg.vlen)) {
-            error_setg(errp,
-                       "Vector extension VLEN must be power of 2");
-            return;
-        }
-        if (cpu->cfg.vlen > RV_VLEN_MAX || cpu->cfg.vlen < 128) {
-            error_setg(errp,
-                       "Vector extension implementation only supports VLEN "
-                       "in the range [128, %d]", RV_VLEN_MAX);
-            return;
-        }
-        if (!is_power_of_2(cpu->cfg.elen)) {
-            error_setg(errp,
-                       "Vector extension ELEN must be power of 2");
-            return;
-        }
-        if (cpu->cfg.elen > 64 || cpu->cfg.elen < 8) {
-            error_setg(errp,
-                       "Vector extension implementation only supports ELEN "
-                       "in the range [8, 64]");
-            return;
-        }
-        if (cpu->cfg.vext_spec) {
-            if (!g_strcmp0(cpu->cfg.vext_spec, "v1.0")) {
-                vext_version = VEXT_VERSION_1_00_0;
-            } else {
-                error_setg(errp,
-                           "Unsupported vector spec version '%s'",
-                           cpu->cfg.vext_spec);
-                return;
-            }
-        } else {
-            qemu_log("vector version is not specified, "
-                     "use the default value v1.0\n");
-        }
-        set_vext_version(env, vext_version);
-    }
 }
 
 #ifndef CONFIG_USER_ONLY
-- 
2.40.0

[PATCH v8 02/11] target/riscv/cpu.c: remove set_vext_version()

[Daniel Henrique Barboza]

This setter is doing nothing else but setting env->vext_ver. Assign the
value directly.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index feca13aefb..fed7b467e4 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -252,11 +252,6 @@ static void set_priv_version(CPURISCVState *env, int priv_ver)
     env->priv_ver = priv_ver;
 }
 
-static void set_vext_version(CPURISCVState *env, int vext_ver)
-{
-    env->vext_ver = vext_ver;
-}
-
 #ifndef CONFIG_USER_ONLY
 static uint8_t satp_mode_from_str(const char *satp_mode_str)
 {
@@ -834,7 +829,7 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
         qemu_log("vector version is not specified, "
                  "use the default value v1.0\n");
     }
-    set_vext_version(env, vext_version);
+    env->vext_ver = vext_version;
 }
 
 /*
-- 
2.40.0

[PATCH v8 03/11] target/riscv/cpu.c: remove set_priv_version()

[Daniel Henrique Barboza]

The setter is doing nothing special. Just set env->priv_ver directly.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index fed7b467e4..bec60fe585 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -247,11 +247,6 @@ static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
     env->misa_ext_mask = env->misa_ext = ext;
 }
 
-static void set_priv_version(CPURISCVState *env, int priv_ver)
-{
-    env->priv_ver = priv_ver;
-}
-
 #ifndef CONFIG_USER_ONLY
 static uint8_t satp_mode_from_str(const char *satp_mode_str)
 {
@@ -350,7 +345,7 @@ static void riscv_any_cpu_init(Object *obj)
         VM_1_10_SV32 : VM_1_10_SV57);
 #endif
 
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
 }
 
 #if defined(TARGET_RISCV64)
@@ -361,7 +356,7 @@ static void rv64_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV64, 0);
     riscv_cpu_add_user_properties(obj);
     /* Set latest version of privileged specification */
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
 #endif
@@ -371,7 +366,7 @@ static void rv64_sifive_u_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
 #endif
@@ -383,7 +378,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
     cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -396,7 +391,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV64, RVG | RVC | RVS | RVU);
-    set_priv_version(env, PRIV_VERSION_1_11_0);
+    env->priv_ver = PRIV_VERSION_1_11_0;
 
     cpu->cfg.ext_zfh = true;
     cpu->cfg.mmu = true;
@@ -430,7 +425,7 @@ static void rv128_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV128, 0);
     riscv_cpu_add_user_properties(obj);
     /* Set latest version of privileged specification */
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
 #endif
@@ -443,7 +438,7 @@ static void rv32_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, 0);
     riscv_cpu_add_user_properties(obj);
     /* Set latest version of privileged specification */
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
 #endif
@@ -453,7 +448,7 @@ static void rv32_sifive_u_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
 #endif
@@ -465,7 +460,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
     cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -478,7 +473,7 @@ static void rv32_ibex_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
-    set_priv_version(env, PRIV_VERSION_1_11_0);
+    env->priv_ver = PRIV_VERSION_1_11_0;
     cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -492,7 +487,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
     cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -1174,7 +1169,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     if (priv_version >= PRIV_VERSION_1_10_0) {
-        set_priv_version(env, priv_version);
+        env->priv_ver = priv_version;
     }
 
     riscv_cpu_validate_misa_priv(env, &local_err);
-- 
2.40.0

[PATCH v8 04/11] target/riscv: add PRIV_VERSION_LATEST

[Daniel Henrique Barboza]

All these generic CPUs are using the latest priv available, at this
moment PRIV_VERSION_1_12_0:

- riscv_any_cpu_init()
- rv32_base_cpu_init()
- rv64_base_cpu_init()
- rv128_base_cpu_init()

Create a new PRIV_VERSION_LATEST enum and use it in those cases. I'll
make it easier to update everything at once when a new priv version is
available.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 8 ++++----
 target/riscv/cpu.h | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index bec60fe585..dd35cf378f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -345,7 +345,7 @@ static void riscv_any_cpu_init(Object *obj)
         VM_1_10_SV32 : VM_1_10_SV57);
 #endif
 
-    env->priv_ver = PRIV_VERSION_1_12_0;
+    env->priv_ver = PRIV_VERSION_LATEST;
 }
 
 #if defined(TARGET_RISCV64)
@@ -356,7 +356,7 @@ static void rv64_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV64, 0);
     riscv_cpu_add_user_properties(obj);
     /* Set latest version of privileged specification */
-    env->priv_ver = PRIV_VERSION_1_12_0;
+    env->priv_ver = PRIV_VERSION_LATEST;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
 #endif
@@ -425,7 +425,7 @@ static void rv128_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV128, 0);
     riscv_cpu_add_user_properties(obj);
     /* Set latest version of privileged specification */
-    env->priv_ver = PRIV_VERSION_1_12_0;
+    env->priv_ver = PRIV_VERSION_LATEST;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
 #endif
@@ -438,7 +438,7 @@ static void rv32_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, 0);
     riscv_cpu_add_user_properties(obj);
     /* Set latest version of privileged specification */
-    env->priv_ver = PRIV_VERSION_1_12_0;
+    env->priv_ver = PRIV_VERSION_LATEST;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
 #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index de7e43126a..15423585d0 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -61,6 +61,8 @@ enum {
     PRIV_VERSION_1_10_0 = 0,
     PRIV_VERSION_1_11_0,
     PRIV_VERSION_1_12_0,
+
+    PRIV_VERSION_LATEST = PRIV_VERSION_1_12_0,
 };
 
 #define VEXT_VERSION_1_00_0 0x00010000
-- 
2.40.0

[PATCH v8 05/11] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

[Daniel Henrique Barboza]

From: Weiwei Li <liweiwei@iscas.ac.cn>

Using implicitly enabled extensions such as Zca/Zcf/Zcd instead of their
super extensions can simplify the extension related check. However, they
may have higher priv version than their super extensions. So we should mask
them in the isa_string based on priv version to make them invisible to user
if the specified priv version is lower than their minimal priv version.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index dd35cf378f..9bb0e6b180 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1721,7 +1721,8 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
     int i;
 
     for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
-        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
+        if (cpu->env.priv_ver >= isa_edata_arr[i].min_version &&
+            isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
             new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
             g_free(old);
             old = new;
-- 
2.40.0

[PATCH v8 06/11] target/riscv: Update check for Zca/Zcf/Zcd

[Daniel Henrique Barboza]

From: Weiwei Li <liweiwei@iscas.ac.cn>

Even though Zca/Zcf/Zcd can be included by C/F/D, however, their priv
version is higher than the priv version of C/F/D. So if we use check
for them instead of check for C/F/D totally, it will trigger new
problem when we try to disable the extensions based on the configured
priv version.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/insn_trans/trans_rvd.c.inc | 12 +++++++-----
 target/riscv/insn_trans/trans_rvf.c.inc | 14 ++++++++------
 target/riscv/insn_trans/trans_rvi.c.inc |  5 +++--
 target/riscv/translate.c                |  5 +++--
 4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 2c51e01c40..6bdb55ef43 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -31,9 +31,11 @@
     } \
 } while (0)
 
-#define REQUIRE_ZCD(ctx) do { \
-    if (!ctx->cfg_ptr->ext_zcd) {  \
-        return false;     \
+#define REQUIRE_ZCD_OR_DC(ctx) do { \
+    if (!ctx->cfg_ptr->ext_zcd) { \
+        if (!has_ext(ctx, RVD) || !has_ext(ctx, RVC)) { \
+            return false; \
+        } \
     } \
 } while (0)
 
@@ -67,13 +69,13 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
 
 static bool trans_c_fld(DisasContext *ctx, arg_fld *a)
 {
-    REQUIRE_ZCD(ctx);
+    REQUIRE_ZCD_OR_DC(ctx);
     return trans_fld(ctx, a);
 }
 
 static bool trans_c_fsd(DisasContext *ctx, arg_fsd *a)
 {
-    REQUIRE_ZCD(ctx);
+    REQUIRE_ZCD_OR_DC(ctx);
     return trans_fsd(ctx, a);
 }
 
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index b2de4fcf3f..c47138575a 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -30,10 +30,12 @@
     } \
 } while (0)
 
-#define REQUIRE_ZCF(ctx) do {                  \
-    if (!ctx->cfg_ptr->ext_zcf) {              \
-        return false;                          \
-    }                                          \
+#define REQUIRE_ZCF_OR_FC(ctx) do {                     \
+    if (!ctx->cfg_ptr->ext_zcf) {                       \
+        if (!has_ext(ctx, RVF) || !has_ext(ctx, RVC)) { \
+            return false;                               \
+        }                                               \
+    }                                                   \
 } while (0)
 
 static bool trans_flw(DisasContext *ctx, arg_flw *a)
@@ -69,13 +71,13 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
 
 static bool trans_c_flw(DisasContext *ctx, arg_flw *a)
 {
-    REQUIRE_ZCF(ctx);
+    REQUIRE_ZCF_OR_FC(ctx);
     return trans_flw(ctx, a);
 }
 
 static bool trans_c_fsw(DisasContext *ctx, arg_fsw *a)
 {
-    REQUIRE_ZCF(ctx);
+    REQUIRE_ZCF_OR_FC(ctx);
     return trans_fsw(ctx, a);
 }
 
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index c70c495fc5..e33f63bea1 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -56,7 +56,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
     tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2);
 
     gen_set_pc(ctx, cpu_pc);
-    if (!ctx->cfg_ptr->ext_zca) {
+    if (!has_ext(ctx, RVC) && !ctx->cfg_ptr->ext_zca) {
         TCGv t0 = tcg_temp_new();
 
         misaligned = gen_new_label();
@@ -169,7 +169,8 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond cond)
 
     gen_set_label(l); /* branch taken */
 
-    if (!ctx->cfg_ptr->ext_zca && ((ctx->base.pc_next + a->imm) & 0x3)) {
+    if (!has_ext(ctx, RVC) && !ctx->cfg_ptr->ext_zca &&
+        ((ctx->base.pc_next + a->imm) & 0x3)) {
         /* misaligned */
         gen_exception_inst_addr_mis(ctx);
     } else {
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 928da0d3f0..a889454137 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -549,7 +549,7 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong imm)
 
     /* check misaligned: */
     next_pc = ctx->base.pc_next + imm;
-    if (!ctx->cfg_ptr->ext_zca) {
+    if (!has_ext(ctx, RVC) && !ctx->cfg_ptr->ext_zca) {
         if ((next_pc & 0x3) != 0) {
             gen_exception_inst_addr_mis(ctx);
             return;
@@ -1123,7 +1123,8 @@ static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
          * The Zca extension is added as way to refer to instructions in the C
          * extension that do not include the floating-point loads and stores
          */
-        if (ctx->cfg_ptr->ext_zca && decode_insn16(ctx, opcode)) {
+        if ((has_ext(ctx, RVC) || ctx->cfg_ptr->ext_zca) &&
+            decode_insn16(ctx, opcode)) {
             return;
         }
     } else {
-- 
2.40.0

[PATCH v8 07/11] target/riscv/cpu.c: add priv_spec validate/disable_exts helpers

[Daniel Henrique Barboza]

We're doing env->priv_spec validation and assignment at the start of
riscv_cpu_realize(), which is fine, but then we're doing a force disable
on extensions that aren't compatible with the priv version.

This second step is being done too early. The disabled extensions might be
re-enabled again in riscv_cpu_validate_set_extensions() by accident. A
better place to put this code is at the end of
riscv_cpu_validate_set_extensions() after all the validations are
completed.

Add a new helper, riscv_cpu_disable_priv_spec_isa_exts(), to disable the
extesions after the validation is done. While we're at it, create a
riscv_cpu_validate_priv_spec() helper to host all env->priv_spec related
validation to unclog riscv_cpu_realize a bit.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 91 ++++++++++++++++++++++++++++------------------
 1 file changed, 56 insertions(+), 35 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9bb0e6b180..c928925544 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -827,6 +827,52 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
     env->vext_ver = vext_version;
 }
 
+static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
+{
+    CPURISCVState *env = &cpu->env;
+    int priv_version = -1;
+
+    if (cpu->cfg.priv_spec) {
+        if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
+            priv_version = PRIV_VERSION_1_12_0;
+        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
+            priv_version = PRIV_VERSION_1_11_0;
+        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
+            priv_version = PRIV_VERSION_1_10_0;
+        } else {
+            error_setg(errp,
+                       "Unsupported privilege spec version '%s'",
+                       cpu->cfg.priv_spec);
+            return;
+        }
+
+        env->priv_ver = priv_version;
+    }
+}
+
+static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
+{
+    CPURISCVState *env = &cpu->env;
+    int i;
+
+    /* Force disable extensions if priv spec version does not match */
+    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
+        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
+            (env->priv_ver < isa_edata_arr[i].min_version)) {
+            isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
+#ifndef CONFIG_USER_ONLY
+            warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
+                        " because privilege spec version does not match",
+                        isa_edata_arr[i].name, env->mhartid);
+#else
+            warn_report("disabling %s extension because "
+                        "privilege spec version does not match",
+                        isa_edata_arr[i].name);
+#endif
+        }
+    }
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
@@ -1045,6 +1091,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_zksed = true;
         cpu->cfg.ext_zksh = true;
     }
+
+    /*
+     * Disable isa extensions based on priv spec after we
+     * validated and set everything we need.
+     */
+    riscv_cpu_disable_priv_spec_isa_exts(cpu);
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -1144,7 +1196,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     CPURISCVState *env = &cpu->env;
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
     CPUClass *cc = CPU_CLASS(mcc);
-    int i, priv_version = -1;
     Error *local_err = NULL;
 
     cpu_exec_realizefn(cs, &local_err);
@@ -1153,23 +1204,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (cpu->cfg.priv_spec) {
-        if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
-            priv_version = PRIV_VERSION_1_12_0;
-        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
-            priv_version = PRIV_VERSION_1_11_0;
-        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
-            priv_version = PRIV_VERSION_1_10_0;
-        } else {
-            error_setg(errp,
-                       "Unsupported privilege spec version '%s'",
-                       cpu->cfg.priv_spec);
-            return;
-        }
-    }
-
-    if (priv_version >= PRIV_VERSION_1_10_0) {
-        env->priv_ver = priv_version;
+    riscv_cpu_validate_priv_spec(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
     }
 
     riscv_cpu_validate_misa_priv(env, &local_err);
@@ -1178,23 +1216,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    /* Force disable extensions if priv spec version does not match */
-    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
-        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i]) &&
-            (env->priv_ver < isa_edata_arr[i].min_version)) {
-            isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
-#ifndef CONFIG_USER_ONLY
-            warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
-                        " because privilege spec version does not match",
-                        isa_edata_arr[i].name, env->mhartid);
-#else
-            warn_report("disabling %s extension because "
-                        "privilege spec version does not match",
-                        isa_edata_arr[i].name);
-#endif
-        }
-    }
-
     if (cpu->cfg.epmp && !cpu->cfg.pmp) {
         /*
          * Enhanced PMP should only be available
-- 
2.40.0

[PATCH v8 08/11] target/riscv/cpu.c: add riscv_cpu_validate_misa_mxl()

[Daniel Henrique Barboza]

Let's remove more code that is open coded in riscv_cpu_realize() and put
it into a helper. Let's also add an error message instead of just
asserting out if env->misa_mxl_max != env->misa_mlx.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 50 ++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c928925544..43635144bd 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -873,6 +873,33 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
     }
 }
 
+static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
+{
+    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
+    CPUClass *cc = CPU_CLASS(mcc);
+    CPURISCVState *env = &cpu->env;
+
+    /* Validate that MISA_MXL is set properly. */
+    switch (env->misa_mxl_max) {
+#ifdef TARGET_RISCV64
+    case MXL_RV64:
+    case MXL_RV128:
+        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
+        break;
+#endif
+    case MXL_RV32:
+        cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (env->misa_mxl_max != env->misa_mxl) {
+        error_setg(errp, "misa_mxl_max must be equal to misa_mxl");
+        return;
+    }
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
@@ -1195,7 +1222,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     RISCVCPU *cpu = RISCV_CPU(dev);
     CPURISCVState *env = &cpu->env;
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
-    CPUClass *cc = CPU_CLASS(mcc);
     Error *local_err = NULL;
 
     cpu_exec_realizefn(cs, &local_err);
@@ -1204,6 +1230,12 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    riscv_cpu_validate_misa_mxl(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     riscv_cpu_validate_priv_spec(cpu, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -1232,22 +1264,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 #endif /* CONFIG_USER_ONLY */
 
-    /* Validate that MISA_MXL is set properly. */
-    switch (env->misa_mxl_max) {
-#ifdef TARGET_RISCV64
-    case MXL_RV64:
-    case MXL_RV128:
-        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
-        break;
-#endif
-    case MXL_RV32:
-        cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
-        break;
-    default:
-        g_assert_not_reached();
-    }
-    assert(env->misa_mxl_max == env->misa_mxl);
-
     riscv_cpu_validate_set_extensions(cpu, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
-- 
2.40.0

[PATCH v8 09/11] target/riscv/cpu.c: validate extensions before riscv_timer_init()

[Daniel Henrique Barboza]

There is no need to init timers if we're not even sure that our
extensions are valid. Execute riscv_cpu_validate_set_extensions() before
riscv_timer_init().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 43635144bd..2d7f0ac785 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1257,13 +1257,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-
-#ifndef CONFIG_USER_ONLY
-    if (cpu->cfg.ext_sstc) {
-        riscv_timer_init(cpu);
-    }
-#endif /* CONFIG_USER_ONLY */
-
     riscv_cpu_validate_set_extensions(cpu, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -1271,6 +1264,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 
 #ifndef CONFIG_USER_ONLY
+    if (cpu->cfg.ext_sstc) {
+        riscv_timer_init(cpu);
+    }
+
     if (cpu->cfg.pmu_num) {
         if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf) {
             cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-- 
2.40.0

[PATCH v8 10/11] target/riscv/cpu.c: remove cfg setup from riscv_cpu_init()

[Daniel Henrique Barboza]

We have 4 config settings being done in riscv_cpu_init(): ext_ifencei,
ext_icsr, mmu and pmp. This is also the constructor of the "riscv-cpu"
device, which happens to be the parent device of every RISC-V cpu.

The result is that these 4 configs are being set every time, and every
other CPU should always account for them. CPUs such as sifive_e need to
disable settings that aren't enabled simply because the parent class
happens to be enabling it.

Moving all configurations from the parent class to each CPU will
centralize the config of each CPU into its own init(), which is clearer
than having to account to whatever happens to be set in the parent
device. These settings are also being set in register_cpu_props() when
no 'misa_ext' is set, so for these CPUs we don't need changes. Named
CPUs will receive all cfgs that the parent were setting into their
init().

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 59 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2d7f0ac785..7d407321aa 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -332,7 +332,8 @@ static void set_satp_mode_default_map(RISCVCPU *cpu)
 
 static void riscv_any_cpu_init(Object *obj)
 {
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
 #if defined(TARGET_RISCV32)
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
 #elif defined(TARGET_RISCV64)
@@ -346,6 +347,12 @@ static void riscv_any_cpu_init(Object *obj)
 #endif
 
     env->priv_ver = PRIV_VERSION_LATEST;
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 }
 
 #if defined(TARGET_RISCV64)
@@ -364,12 +371,19 @@ static void rv64_base_cpu_init(Object *obj)
 
 static void rv64_sifive_u_cpu_init(Object *obj)
 {
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
 #endif
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 }
 
 static void rv64_sifive_e_cpu_init(Object *obj)
@@ -379,10 +393,14 @@ static void rv64_sifive_e_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
     env->priv_ver = PRIV_VERSION_1_10_0;
-    cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
 #endif
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.pmp = true;
 }
 
 static void rv64_thead_c906_cpu_init(Object *obj)
@@ -410,6 +428,9 @@ static void rv64_thead_c906_cpu_init(Object *obj)
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_SV39);
 #endif
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.pmp = true;
 }
 
 static void rv128_base_cpu_init(Object *obj)
@@ -446,12 +467,19 @@ static void rv32_base_cpu_init(Object *obj)
 
 static void rv32_sifive_u_cpu_init(Object *obj)
 {
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
 #endif
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 }
 
 static void rv32_sifive_e_cpu_init(Object *obj)
@@ -461,10 +489,14 @@ static void rv32_sifive_e_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
     env->priv_ver = PRIV_VERSION_1_10_0;
-    cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
 #endif
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.pmp = true;
 }
 
 static void rv32_ibex_cpu_init(Object *obj)
@@ -474,11 +506,15 @@ static void rv32_ibex_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
     env->priv_ver = PRIV_VERSION_1_11_0;
-    cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
 #endif
     cpu->cfg.epmp = true;
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.pmp = true;
 }
 
 static void rv32_imafcu_nommu_cpu_init(Object *obj)
@@ -488,10 +524,14 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
     env->priv_ver = PRIV_VERSION_1_10_0;
-    cpu->cfg.mmu = false;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
 #endif
+
+    /* inherited from parent obj via riscv_cpu_init() */
+    cpu->cfg.ext_ifencei = true;
+    cpu->cfg.ext_icsr = true;
+    cpu->cfg.pmp = true;
 }
 #endif
 
@@ -1404,11 +1444,6 @@ static void riscv_cpu_init(Object *obj)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
 
-    cpu->cfg.ext_ifencei = true;
-    cpu->cfg.ext_icsr = true;
-    cpu->cfg.mmu = true;
-    cpu->cfg.pmp = true;
-
     cpu_set_cpustate_pointers(cpu);
 
 #ifndef CONFIG_USER_ONLY
-- 
2.40.0

[PATCH v8 11/11] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

write_misa() must use as much common logic as possible. We want to open
code just the bits that are exclusive to the CSR write operation and TCG
internals.

Our validation is done with riscv_cpu_validate_set_extensions(), but we
need a small tweak first. When enabling RVG we're doing:

        env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
        env->misa_ext_mask = env->misa_ext;

This works fine for realize() time but this can potentially overwrite
env->misa_ext_mask if we reutilize the function for write_misa().

Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
misa_ext_mask as well. This won't change realize() time behavior
(misa_ext_mask will be == misa_ext) and will ensure that write_misa()
won't change misa_ext_mask by accident.

After that, rewrite write_misa() to work as follows:

- mask the write using misa_ext_mask to avoid enabling unsupported
  extensions;

- suppress RVC if the next insn isn't aligned;

- disable RVG if any of RVG dependencies are being disabled by the user;

- assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
  error, rollback env->misa_ext to its original value;

- handle RVF and MSTATUS_FS and continue as usual.

Let's keep write_misa() as experimental for now until this logic gains
enough mileage.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
---
 target/riscv/cpu.c |  4 ++--
 target/riscv/cpu.h |  1 +
 target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
 3 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d407321aa..4fa720a39d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
  */
-static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
+void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 {
     CPURISCVState *env = &cpu->env;
     Error *local_err = NULL;
@@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_ifencei = true;
 
         env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
-        env->misa_ext_mask = env->misa_ext;
+        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
     }
 
     if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 15423585d0..1f39edc687 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         bool probe, uintptr_t retaddr);
 char *riscv_isa_string(RISCVCPU *cpu);
 void riscv_cpu_list(void);
+void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
 
 #define cpu_list riscv_cpu_list
 #define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4451bd1263..4a3c57ea6f 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
 static RISCVException write_misa(CPURISCVState *env, int csrno,
                                  target_ulong val)
 {
+    RISCVCPU *cpu = env_archcpu(env);
+    uint32_t orig_misa_ext = env->misa_ext;
+    Error *local_err = NULL;
+
     if (!riscv_cpu_cfg(env)->misa_w) {
         /* 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;
 
-    /* '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
@@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
         val &= ~RVC;
     }
 
+    /* Disable RVG if any of its dependencies are disabled */
+    if (!(val & RVI && val & RVM && val & RVA &&
+          val & RVF && val & RVD)) {
+        val &= ~RVG;
+    }
+
     /* If nothing changed, do nothing. */
     if (val == env->misa_ext) {
         return RISCV_EXCP_NONE;
     }
 
-    if (!(val & RVF)) {
+    env->misa_ext = val;
+    riscv_cpu_validate_set_extensions(cpu, &local_err);
+    if (local_err != NULL) {
+        /* Rollback on validation error */
+        env->misa_ext = orig_misa_ext;
+
+        return RISCV_EXCP_NONE;
+    }
+
+    if (!(env->misa_ext & 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.40.0

Re: [PATCH v8 11/11] target/riscv: rework write_misa()

[Alistair Francis]

On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> write_misa() must use as much common logic as possible. We want to open
> code just the bits that are exclusive to the CSR write operation and TCG
> internals.
>
> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> need a small tweak first. When enabling RVG we're doing:
>
>         env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>         env->misa_ext_mask = env->misa_ext;
>
> This works fine for realize() time but this can potentially overwrite
> env->misa_ext_mask if we reutilize the function for write_misa().
>
> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> misa_ext_mask as well. This won't change realize() time behavior
> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> won't change misa_ext_mask by accident.
>
> After that, rewrite write_misa() to work as follows:
>
> - mask the write using misa_ext_mask to avoid enabling unsupported
>   extensions;
>
> - suppress RVC if the next insn isn't aligned;
>
> - disable RVG if any of RVG dependencies are being disabled by the user;
>
> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>   error, rollback env->misa_ext to its original value;
>
> - handle RVF and MSTATUS_FS and continue as usual.
>
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>  target/riscv/cpu.c |  4 ++--
>  target/riscv/cpu.h |  1 +
>  target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>  3 files changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7d407321aa..4fa720a39d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly.
>   */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>  {
>      CPURISCVState *env = &cpu->env;
>      Error *local_err = NULL;
> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          cpu->cfg.ext_ifencei = true;
>
>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> -        env->misa_ext_mask = env->misa_ext;
> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>      }
>
>      if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 15423585d0..1f39edc687 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                          bool probe, uintptr_t retaddr);
>  char *riscv_isa_string(RISCVCPU *cpu);
>  void riscv_cpu_list(void);
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>
>  #define cpu_list riscv_cpu_list
>  #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4451bd1263..4a3c57ea6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>  static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                   target_ulong val)
>  {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    uint32_t orig_misa_ext = env->misa_ext;
> +    Error *local_err = NULL;
> +
>      if (!riscv_cpu_cfg(env)->misa_w) {
>          /* 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;
>
> -    /* '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
> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>          val &= ~RVC;
>      }
>
> +    /* Disable RVG if any of its dependencies are disabled */
> +    if (!(val & RVI && val & RVM && val & RVA &&
> +          val & RVF && val & RVD)) {
> +        val &= ~RVG;
> +    }
> +
>      /* If nothing changed, do nothing. */
>      if (val == env->misa_ext) {
>          return RISCV_EXCP_NONE;
>      }
>
> -    if (!(val & RVF)) {
> +    env->misa_ext = val;
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> +    if (local_err != NULL) {
> +        /* Rollback on validation error */
> +        env->misa_ext = orig_misa_ext;

I don't think this is right though. The spec even states:

" An attempt to write an unsupported combination causes those bits to
be set to some supported combination."

So we should try to follow what the guest requested as closely as we
can, instead of just rolling back.

Alistair

> +
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (!(env->misa_ext & 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.40.0
>
>

Re: [PATCH v8 11/11] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

On 5/7/23 20:25, Alistair Francis wrote:
> On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> write_misa() must use as much common logic as possible. We want to open
>> code just the bits that are exclusive to the CSR write operation and TCG
>> internals.
>>
>> Our validation is done with riscv_cpu_validate_set_extensions(), but we
>> need a small tweak first. When enabling RVG we're doing:
>>
>>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>          env->misa_ext_mask = env->misa_ext;
>>
>> This works fine for realize() time but this can potentially overwrite
>> env->misa_ext_mask if we reutilize the function for write_misa().
>>
>> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
>> misa_ext_mask as well. This won't change realize() time behavior
>> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
>> won't change misa_ext_mask by accident.
>>
>> After that, rewrite write_misa() to work as follows:
>>
>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>    extensions;
>>
>> - suppress RVC if the next insn isn't aligned;
>>
>> - disable RVG if any of RVG dependencies are being disabled by the user;
>>
>> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>>    error, rollback env->misa_ext to its original value;
>>
>> - handle RVF and MSTATUS_FS and continue as usual.
>>
>> Let's keep write_misa() as experimental for now until this logic gains
>> enough mileage.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> ---
>>   target/riscv/cpu.c |  4 ++--
>>   target/riscv/cpu.h |  1 +
>>   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>>   3 files changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 7d407321aa..4fa720a39d 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>    * Check consistency between chosen extensions while setting
>>    * cpu->cfg accordingly.
>>    */
>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>   {
>>       CPURISCVState *env = &cpu->env;
>>       Error *local_err = NULL;
>> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>           cpu->cfg.ext_ifencei = true;
>>
>>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>> -        env->misa_ext_mask = env->misa_ext;
>> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>>       }
>>
>>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 15423585d0..1f39edc687 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>                           bool probe, uintptr_t retaddr);
>>   char *riscv_isa_string(RISCVCPU *cpu);
>>   void riscv_cpu_list(void);
>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>>
>>   #define cpu_list riscv_cpu_list
>>   #define cpu_mmu_index riscv_cpu_mmu_index
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 4451bd1263..4a3c57ea6f 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>>                                    target_ulong val)
>>   {
>> +    RISCVCPU *cpu = env_archcpu(env);
>> +    uint32_t orig_misa_ext = env->misa_ext;
>> +    Error *local_err = NULL;
>> +
>>       if (!riscv_cpu_cfg(env)->misa_w) {
>>           /* 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;
>>
>> -    /* '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
>> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>           val &= ~RVC;
>>       }
>>
>> +    /* Disable RVG if any of its dependencies are disabled */
>> +    if (!(val & RVI && val & RVM && val & RVA &&
>> +          val & RVF && val & RVD)) {
>> +        val &= ~RVG;
>> +    }
>> +
>>       /* If nothing changed, do nothing. */
>>       if (val == env->misa_ext) {
>>           return RISCV_EXCP_NONE;
>>       }
>>
>> -    if (!(val & RVF)) {
>> +    env->misa_ext = val;
>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
>> +    if (local_err != NULL) {
>> +        /* Rollback on validation error */
>> +        env->misa_ext = orig_misa_ext;
> 
> I don't think this is right though. The spec even states:
> 
> " An attempt to write an unsupported combination causes those bits to
> be set to some supported combination."

Rolling back to the previous state (which means do nothing) is a supported
combination. I don't think the spec forbids what we're doing here.


> 
> So we should try to follow what the guest requested as closely as we
> can, instead of just rolling back.

Let's say we have this:

RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV

And the userspace wants to write this (clear RVF):

RVI | RVM | RVA | RVD | RVC | RVU | RVV


What should we do in this case? Removing RVF would imply removing RVD and RVV, so
that's one alternative. Another alternative is consider that the user should
clear all bits of RVV explicitly, so we ignore the RVF clear and keep it as it
is (which we're already doing in this patch).

Note that both are right depending on the user intention. And this is why I don't
believe that follow what the user attempted to do, or "follow what the guest as
closely as we can" is viable.

An alternative, instead of keeping the original misa_ext value, is to do
misa_ext = misa_ext_mask. This would mean "in case of a failed validation,
since we don't know what the user tried to do, re-enable everything".

Anything other than keeping the existing misa_ext, like this patch is already
doing, or setting misa_ext to misa_ext_mask, will demand more code and assumptions
based on what we can't control (user intentions).


Thanks,


Daniel


> 
> Alistair
> 
>> +
>> +        return RISCV_EXCP_NONE;
>> +    }
>> +
>> +    if (!(env->misa_ext & 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.40.0
>>
>>

Re: [PATCH v8 11/11] target/riscv: rework write_misa()

[Alistair Francis]

On Mon, May 8, 2023 at 8:29 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 5/7/23 20:25, Alistair Francis wrote:
> > On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> write_misa() must use as much common logic as possible. We want to open
> >> code just the bits that are exclusive to the CSR write operation and TCG
> >> internals.
> >>
> >> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> >> need a small tweak first. When enabling RVG we're doing:
> >>
> >>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> >>          env->misa_ext_mask = env->misa_ext;
> >>
> >> This works fine for realize() time but this can potentially overwrite
> >> env->misa_ext_mask if we reutilize the function for write_misa().
> >>
> >> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> >> misa_ext_mask as well. This won't change realize() time behavior
> >> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> >> won't change misa_ext_mask by accident.
> >>
> >> After that, rewrite write_misa() to work as follows:
> >>
> >> - mask the write using misa_ext_mask to avoid enabling unsupported
> >>    extensions;
> >>
> >> - suppress RVC if the next insn isn't aligned;
> >>
> >> - disable RVG if any of RVG dependencies are being disabled by the user;
> >>
> >> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
> >>    error, rollback env->misa_ext to its original value;
> >>
> >> - handle RVF and MSTATUS_FS and continue as usual.
> >>
> >> Let's keep write_misa() as experimental for now until this logic gains
> >> enough mileage.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >> ---
> >>   target/riscv/cpu.c |  4 ++--
> >>   target/riscv/cpu.h |  1 +
> >>   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
> >>   3 files changed, 23 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 7d407321aa..4fa720a39d 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >>    * Check consistency between chosen extensions while setting
> >>    * cpu->cfg accordingly.
> >>    */
> >> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >>   {
> >>       CPURISCVState *env = &cpu->env;
> >>       Error *local_err = NULL;
> >> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >>           cpu->cfg.ext_ifencei = true;
> >>
> >>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> >> -        env->misa_ext_mask = env->misa_ext;
> >> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> >>       }
> >>
> >>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> >> index 15423585d0..1f39edc687 100644
> >> --- a/target/riscv/cpu.h
> >> +++ b/target/riscv/cpu.h
> >> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >>                           bool probe, uintptr_t retaddr);
> >>   char *riscv_isa_string(RISCVCPU *cpu);
> >>   void riscv_cpu_list(void);
> >> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
> >>
> >>   #define cpu_list riscv_cpu_list
> >>   #define cpu_mmu_index riscv_cpu_mmu_index
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index 4451bd1263..4a3c57ea6f 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> >>   static RISCVException write_misa(CPURISCVState *env, int csrno,
> >>                                    target_ulong val)
> >>   {
> >> +    RISCVCPU *cpu = env_archcpu(env);
> >> +    uint32_t orig_misa_ext = env->misa_ext;
> >> +    Error *local_err = NULL;
> >> +
> >>       if (!riscv_cpu_cfg(env)->misa_w) {
> >>           /* 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;
> >>
> >> -    /* '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
> >> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
> >>           val &= ~RVC;
> >>       }
> >>
> >> +    /* Disable RVG if any of its dependencies are disabled */
> >> +    if (!(val & RVI && val & RVM && val & RVA &&
> >> +          val & RVF && val & RVD)) {
> >> +        val &= ~RVG;
> >> +    }
> >> +
> >>       /* If nothing changed, do nothing. */
> >>       if (val == env->misa_ext) {
> >>           return RISCV_EXCP_NONE;
> >>       }
> >>
> >> -    if (!(val & RVF)) {
> >> +    env->misa_ext = val;
> >> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> >> +    if (local_err != NULL) {
> >> +        /* Rollback on validation error */
> >> +        env->misa_ext = orig_misa_ext;
> >
> > I don't think this is right though. The spec even states:
> >
> > " An attempt to write an unsupported combination causes those bits to
> > be set to some supported combination."
>
> Rolling back to the previous state (which means do nothing) is a supported
> combination. I don't think the spec forbids what we're doing here.

It doesn't forbid it, but it also doesn't really encourage it.

>
>
> >
> > So we should try to follow what the guest requested as closely as we
> > can, instead of just rolling back.
>
> Let's say we have this:
>
> RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV
>
> And the userspace wants to write this (clear RVF):
>
> RVI | RVM | RVA | RVD | RVC | RVU | RVV
>
>
> What should we do in this case? Removing RVF would imply removing RVD and RVV, so
> that's one alternative. Another alternative is consider that the user should
> clear all bits of RVV explicitly, so we ignore the RVF clear and keep it as it
> is (which we're already doing in this patch).

In this case we should keep RVF enabled, I agree.

What I'm thinking of, is what happens if we have:

RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV

and the user writes (clears RVF and RVV)

RVI | RVM | RVA | RVD | RVC | RVU

We can't disable RVF, because RVD requires it, but we can disable RVV.
So we should end up with just RVV disabled

RVI | RVM | RVA | RVF | RVD | RVC | RVU

From my interpretation of the spec, that's what we should be supporting.

>
> Note that both are right depending on the user intention. And this is why I don't
> believe that follow what the user attempted to do, or "follow what the guest as
> closely as we can" is viable.
>
> An alternative, instead of keeping the original misa_ext value, is to do
> misa_ext = misa_ext_mask. This would mean "in case of a failed validation,
> since we don't know what the user tried to do, re-enable everything".

I think that's even worse :)

>
> Anything other than keeping the existing misa_ext, like this patch is already
> doing, or setting misa_ext to misa_ext_mask, will demand more code and assumptions
> based on what we can't control (user intentions).

Hmm... That's fair. It does make the process more complex, although we
are already checking if the combination is valid.

Ok, how about we at least report a guest error if the combination is
invalid and we roll back. Then we can go ahead with the patch.

Alistair

>
>
> Thanks,
>
>
> Daniel
>
>
> >
> > Alistair
> >
> >> +
> >> +        return RISCV_EXCP_NONE;
> >> +    }
> >> +
> >> +    if (!(env->misa_ext & 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.40.0
> >>
> >>

Re: [PATCH v8 11/11] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

On 5/17/23 01:48, Alistair Francis wrote:
> On Mon, May 8, 2023 at 8:29 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 5/7/23 20:25, Alistair Francis wrote:
>>> On Fri, Apr 21, 2023 at 11:29 PM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> write_misa() must use as much common logic as possible. We want to open
>>>> code just the bits that are exclusive to the CSR write operation and TCG
>>>> internals.
>>>>
>>>> Our validation is done with riscv_cpu_validate_set_extensions(), but we
>>>> need a small tweak first. When enabling RVG we're doing:
>>>>
>>>>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>>>           env->misa_ext_mask = env->misa_ext;
>>>>
>>>> This works fine for realize() time but this can potentially overwrite
>>>> env->misa_ext_mask if we reutilize the function for write_misa().
>>>>
>>>> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
>>>> misa_ext_mask as well. This won't change realize() time behavior
>>>> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
>>>> won't change misa_ext_mask by accident.
>>>>
>>>> After that, rewrite write_misa() to work as follows:
>>>>
>>>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>>>     extensions;
>>>>
>>>> - suppress RVC if the next insn isn't aligned;
>>>>
>>>> - disable RVG if any of RVG dependencies are being disabled by the user;
>>>>
>>>> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>>>>     error, rollback env->misa_ext to its original value;
>>>>
>>>> - handle RVF and MSTATUS_FS and continue as usual.
>>>>
>>>> Let's keep write_misa() as experimental for now until this logic gains
>>>> enough mileage.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>> ---
>>>>    target/riscv/cpu.c |  4 ++--
>>>>    target/riscv/cpu.h |  1 +
>>>>    target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>>>>    3 files changed, 23 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>> index 7d407321aa..4fa720a39d 100644
>>>> --- a/target/riscv/cpu.c
>>>> +++ b/target/riscv/cpu.c
>>>> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>>>     * Check consistency between chosen extensions while setting
>>>>     * cpu->cfg accordingly.
>>>>     */
>>>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>>    {
>>>>        CPURISCVState *env = &cpu->env;
>>>>        Error *local_err = NULL;
>>>> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>>            cpu->cfg.ext_ifencei = true;
>>>>
>>>>            env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>>> -        env->misa_ext_mask = env->misa_ext;
>>>> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>>>>        }
>>>>
>>>>        if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>>> index 15423585d0..1f39edc687 100644
>>>> --- a/target/riscv/cpu.h
>>>> +++ b/target/riscv/cpu.h
>>>> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>>>                            bool probe, uintptr_t retaddr);
>>>>    char *riscv_isa_string(RISCVCPU *cpu);
>>>>    void riscv_cpu_list(void);
>>>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>>>>
>>>>    #define cpu_list riscv_cpu_list
>>>>    #define cpu_mmu_index riscv_cpu_mmu_index
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index 4451bd1263..4a3c57ea6f 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>>>>    static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>                                     target_ulong val)
>>>>    {
>>>> +    RISCVCPU *cpu = env_archcpu(env);
>>>> +    uint32_t orig_misa_ext = env->misa_ext;
>>>> +    Error *local_err = NULL;
>>>> +
>>>>        if (!riscv_cpu_cfg(env)->misa_w) {
>>>>            /* 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;
>>>>
>>>> -    /* '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
>>>> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>>            val &= ~RVC;
>>>>        }
>>>>
>>>> +    /* Disable RVG if any of its dependencies are disabled */
>>>> +    if (!(val & RVI && val & RVM && val & RVA &&
>>>> +          val & RVF && val & RVD)) {
>>>> +        val &= ~RVG;
>>>> +    }
>>>> +
>>>>        /* If nothing changed, do nothing. */
>>>>        if (val == env->misa_ext) {
>>>>            return RISCV_EXCP_NONE;
>>>>        }
>>>>
>>>> -    if (!(val & RVF)) {
>>>> +    env->misa_ext = val;
>>>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
>>>> +    if (local_err != NULL) {
>>>> +        /* Rollback on validation error */
>>>> +        env->misa_ext = orig_misa_ext;
>>>
>>> I don't think this is right though. The spec even states:
>>>
>>> " An attempt to write an unsupported combination causes those bits to
>>> be set to some supported combination."
>>
>> Rolling back to the previous state (which means do nothing) is a supported
>> combination. I don't think the spec forbids what we're doing here.
> 
> It doesn't forbid it, but it also doesn't really encourage it.
> 
>>
>>
>>>
>>> So we should try to follow what the guest requested as closely as we
>>> can, instead of just rolling back.
>>
>> Let's say we have this:
>>
>> RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV
>>
>> And the userspace wants to write this (clear RVF):
>>
>> RVI | RVM | RVA | RVD | RVC | RVU | RVV
>>
>>
>> What should we do in this case? Removing RVF would imply removing RVD and RVV, so
>> that's one alternative. Another alternative is consider that the user should
>> clear all bits of RVV explicitly, so we ignore the RVF clear and keep it as it
>> is (which we're already doing in this patch).
> 
> In this case we should keep RVF enabled, I agree.
> 
> What I'm thinking of, is what happens if we have:
> 
> RVI | RVM | RVA | RVF | RVD | RVC | RVU | RVV
> 
> and the user writes (clears RVF and RVV)
> 
> RVI | RVM | RVA | RVD | RVC | RVU
> 
> We can't disable RVF, because RVD requires it, but we can disable RVV.
> So we should end up with just RVV disabled
> 
> RVI | RVM | RVA | RVF | RVD | RVC | RVU
> 
>  From my interpretation of the spec, that's what we should be supporting.
> 
>>
>> Note that both are right depending on the user intention. And this is why I don't
>> believe that follow what the user attempted to do, or "follow what the guest as
>> closely as we can" is viable.
>>
>> An alternative, instead of keeping the original misa_ext value, is to do
>> misa_ext = misa_ext_mask. This would mean "in case of a failed validation,
>> since we don't know what the user tried to do, re-enable everything".
> 
> I think that's even worse :)
> 
>>
>> Anything other than keeping the existing misa_ext, like this patch is already
>> doing, or setting misa_ext to misa_ext_mask, will demand more code and assumptions
>> based on what we can't control (user intentions).
> 
> Hmm... That's fair. It does make the process more complex, although we
> are already checking if the combination is valid.
> 
> Ok, how about we at least report a guest error if the combination is
> invalid and we roll back. Then we can go ahead with the patch.

Sure! I'll make the change and re-send.


Daniel


> 
> Alistair
> 
>>
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>>
>>> Alistair
>>>
>>>> +
>>>> +        return RISCV_EXCP_NONE;
>>>> +    }
>>>> +
>>>> +    if (!(env->misa_ext & 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.40.0
>>>>
>>>>

Re: [PATCH v8 11/11] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

Alistair,


Since this is the only patch that is being contested is it possible to apply all
the other ones to riscv-to-apply.next?

I'm asking because I'm going to send more code that will affect cpu_init() and
riscv_cpu_realize() functions, and it would be easier if the cleanups were already
in 'next'. Otherwise I'll either have to base the new code on top of this pending
series or I'll base them under riscv-to-apply.next and we'll have to deal with
conflicts.


Thanks,

Daniel

On 4/21/23 10:27, Daniel Henrique Barboza wrote:
> write_misa() must use as much common logic as possible. We want to open
> code just the bits that are exclusive to the CSR write operation and TCG
> internals.
> 
> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> need a small tweak first. When enabling RVG we're doing:
> 
>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>          env->misa_ext_mask = env->misa_ext;
> 
> This works fine for realize() time but this can potentially overwrite
> env->misa_ext_mask if we reutilize the function for write_misa().
> 
> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> misa_ext_mask as well. This won't change realize() time behavior
> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> won't change misa_ext_mask by accident.
> 
> After that, rewrite write_misa() to work as follows:
> 
> - mask the write using misa_ext_mask to avoid enabling unsupported
>    extensions;
> 
> - suppress RVC if the next insn isn't aligned;
> 
> - disable RVG if any of RVG dependencies are being disabled by the user;
> 
> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
>    error, rollback env->misa_ext to its original value;
> 
> - handle RVF and MSTATUS_FS and continue as usual.
> 
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> ---
>   target/riscv/cpu.c |  4 ++--
>   target/riscv/cpu.h |  1 +
>   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
>   3 files changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7d407321aa..4fa720a39d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>    * Check consistency between chosen extensions while setting
>    * cpu->cfg accordingly.
>    */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>   {
>       CPURISCVState *env = &cpu->env;
>       Error *local_err = NULL;
> @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           cpu->cfg.ext_ifencei = true;
>   
>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> -        env->misa_ext_mask = env->misa_ext;
> +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>       }
>   
>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 15423585d0..1f39edc687 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                           bool probe, uintptr_t retaddr);
>   char *riscv_isa_string(RISCVCPU *cpu);
>   void riscv_cpu_list(void);
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
>   
>   #define cpu_list riscv_cpu_list
>   #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 4451bd1263..4a3c57ea6f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    uint32_t orig_misa_ext = env->misa_ext;
> +    Error *local_err = NULL;
> +
>       if (!riscv_cpu_cfg(env)->misa_w) {
>           /* 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;
>   
> -    /* '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
> @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           val &= ~RVC;
>       }
>   
> +    /* Disable RVG if any of its dependencies are disabled */
> +    if (!(val & RVI && val & RVM && val & RVA &&
> +          val & RVF && val & RVD)) {
> +        val &= ~RVG;
> +    }
> +
>       /* If nothing changed, do nothing. */
>       if (val == env->misa_ext) {
>           return RISCV_EXCP_NONE;
>       }
>   
> -    if (!(val & RVF)) {
> +    env->misa_ext = val;
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> +    if (local_err != NULL) {
> +        /* Rollback on validation error */
> +        env->misa_ext = orig_misa_ext;
> +
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    if (!(env->misa_ext & 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 v8 11/11] target/riscv: rework write_misa()

[Alistair Francis]

On Fri, May 12, 2023 at 10:42 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Alistair,
>
>
> Since this is the only patch that is being contested is it possible to apply all
> the other ones to riscv-to-apply.next?
>
> I'm asking because I'm going to send more code that will affect cpu_init() and
> riscv_cpu_realize() functions, and it would be easier if the cleanups were already
> in 'next'. Otherwise I'll either have to base the new code on top of this pending
> series or I'll base them under riscv-to-apply.next and we'll have to deal with
> conflicts.

Urgh, that's fair.

I just replied to your other thread. Let's throw a guest error print
in here and then we can merge it

Alistair

>
>
> Thanks,
>
> Daniel
>
> On 4/21/23 10:27, Daniel Henrique Barboza wrote:
> > write_misa() must use as much common logic as possible. We want to open
> > code just the bits that are exclusive to the CSR write operation and TCG
> > internals.
> >
> > Our validation is done with riscv_cpu_validate_set_extensions(), but we
> > need a small tweak first. When enabling RVG we're doing:
> >
> >          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> >          env->misa_ext_mask = env->misa_ext;
> >
> > This works fine for realize() time but this can potentially overwrite
> > env->misa_ext_mask if we reutilize the function for write_misa().
> >
> > Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> > misa_ext_mask as well. This won't change realize() time behavior
> > (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> > won't change misa_ext_mask by accident.
> >
> > After that, rewrite write_misa() to work as follows:
> >
> > - mask the write using misa_ext_mask to avoid enabling unsupported
> >    extensions;
> >
> > - suppress RVC if the next insn isn't aligned;
> >
> > - disable RVG if any of RVG dependencies are being disabled by the user;
> >
> > - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
> >    error, rollback env->misa_ext to its original value;
> >
> > - handle RVF and MSTATUS_FS and continue as usual.
> >
> > Let's keep write_misa() as experimental for now until this logic gains
> > enough mileage.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > ---
> >   target/riscv/cpu.c |  4 ++--
> >   target/riscv/cpu.h |  1 +
> >   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
> >   3 files changed, 23 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d407321aa..4fa720a39d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -944,7 +944,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >    * Check consistency between chosen extensions while setting
> >    * cpu->cfg accordingly.
> >    */
> > -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> > +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >   {
> >       CPURISCVState *env = &cpu->env;
> >       Error *local_err = NULL;
> > @@ -960,7 +960,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> >           cpu->cfg.ext_ifencei = true;
> >
> >           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> > -        env->misa_ext_mask = env->misa_ext;
> > +        env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> >       }
> >
> >       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 15423585d0..1f39edc687 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -548,6 +548,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> >                           bool probe, uintptr_t retaddr);
> >   char *riscv_isa_string(RISCVCPU *cpu);
> >   void riscv_cpu_list(void);
> > +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp);
> >
> >   #define cpu_list riscv_cpu_list
> >   #define cpu_mmu_index riscv_cpu_mmu_index
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 4451bd1263..4a3c57ea6f 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1387,39 +1387,18 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
> >   static RISCVException write_misa(CPURISCVState *env, int csrno,
> >                                    target_ulong val)
> >   {
> > +    RISCVCPU *cpu = env_archcpu(env);
> > +    uint32_t orig_misa_ext = env->misa_ext;
> > +    Error *local_err = NULL;
> > +
> >       if (!riscv_cpu_cfg(env)->misa_w) {
> >           /* 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;
> >
> > -    /* '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
> > @@ -1428,18 +1407,32 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
> >           val &= ~RVC;
> >       }
> >
> > +    /* Disable RVG if any of its dependencies are disabled */
> > +    if (!(val & RVI && val & RVM && val & RVA &&
> > +          val & RVF && val & RVD)) {
> > +        val &= ~RVG;
> > +    }
> > +
> >       /* If nothing changed, do nothing. */
> >       if (val == env->misa_ext) {
> >           return RISCV_EXCP_NONE;
> >       }
> >
> > -    if (!(val & RVF)) {
> > +    env->misa_ext = val;
> > +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> > +    if (local_err != NULL) {
> > +        /* Rollback on validation error */
> > +        env->misa_ext = orig_misa_ext;
> > +
> > +        return RISCV_EXCP_NONE;
> > +    }
> > +
> > +    if (!(env->misa_ext & 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;
> >   }
>