[PATCH for-8.1 00/17] centralize CPU extensions logic

[PATCH for-8.1 00/17] centralize CPU extensions logic

[Daniel Henrique Barboza]

Hi,

During the review of a series that did some work in the RISCV_FEATURES*
enum, Liu Zhiwei commented on how we could centralize the all the
extension validation code and integrate it with write_misa() [1].

This does at least part of what was suggested. The idea is that, ATM, we
have too many places setting cpu->cfg and the validation logic is
scattered around (e.g. there are some contraints in write_misa() that
should be applicable elsewhere). This series is an attempt to centralize
things a bit.

The main accomplishments of this series are:

- the parent device riscv-cpu no longer sets any cpu->cfg attribute. This
  was forcing init() functions to disable extensions that it wouldn't
  use just because the parent device was enabling it;

- all validations are centered in validate_set_extensions(), with
  pontual exceptions in write_misa() that has exclusive contraints;

- set_misa() now writes cpu->cfg. No need to have one function to set
  env->misa_ext and then another to set cpu->cfg;

- register_cpu_props() now only exposes user-facing props;

- all validations from validate_set_extensions() are done at the start
  of the function. Validate first, set extensions after;

- RVE is now forbidden in all validations, not just in write_misa();

- RVG is now forbidden in write_misa();

- write_misa now uses set_misa() and validate_set_extensions().



[1] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg05092.html

Daniel Henrique Barboza (17):
  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 macro
  target/riscv/cpu.c: add riscv_cpu_validate_priv_spec()
  target/riscv: move realize() validations to
    riscv_cpu_validate_set_extensions()
  target/riscv/cpu.c: remove cfg setup from riscv_cpu_init()
  target/riscv/cpu.c: avoid set_misa() in validate_set_extensions()
  target/riscv/cpu.c: set cpu config in set_misa()
  target/riscv/cpu.c: redesign register_cpu_props()
  target/riscv/cpu.c: move riscv_cpu_validate_v() up
  target/riscv: put env->misa_ext <-> cpu->cfg code into helpers
  target/riscv/cpu.c: split riscv_cpu_validate_priv_spec()
  target/riscv/cpu.c: do not allow RVE to be set
  target/riscv: add RVG
  target/riscv: do not allow RVG in write_misa()
  target/riscv: rework write_misa()

 target/riscv/cpu.c | 516 +++++++++++++++++++++++++--------------------
 target/riscv/cpu.h |   9 +-
 target/riscv/csr.c |  52 ++---
 3 files changed, 323 insertions(+), 254 deletions(-)

-- 
2.39.2

[PATCH for-8.1 01/17] target/riscv/cpu.c: add riscv_cpu_validate_v()

[Daniel Henrique Barboza]

The code that validates ext_v in riscv_cpu_validate_set_extensions() is
not properly indented - we're missing an extra indent level right after
the first check that uses cfg->elen.

In the end the 'v' verification is a bit too large in comparison with
the others, and can be put in a separated function to enhance the
readability of riscv_cpu_validate_set_extensions().

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..5060a98b6d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -802,6 +802,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, doing a set_misa() in the end.
@@ -993,44 +1033,15 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         ext |= RVH;
     }
     if (cpu->cfg.ext_v) {
-        int vext_version = VEXT_VERSION_1_00_0;
-        ext |= RVV;
-        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]");
+        Error *local_err = NULL;
+
+        riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
             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);
+
+        ext |= RVV;
     }
     if (cpu->cfg.ext_j) {
         ext |= RVJ;
-- 
2.39.2

[PATCH for-8.1 02/17] 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>
---
 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 5060a98b6d..0baed79ec2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -245,11 +245,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)
 {
@@ -839,7 +834,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.39.2

Re: [PATCH for-8.1 02/17] target/riscv/cpu.c: remove set_vext_version()

[LIU Zhiwei]

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> This setter is doing nothing else but setting env->vext_ver. Assign the
> value directly.

IMHO, No better than the older implementation.

Zhiwei

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.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 5060a98b6d..0baed79ec2 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -245,11 +245,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)
>   {
> @@ -839,7 +834,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;
>   }
>   
>   /*

[PATCH for-8.1 03/17] 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>
---
 target/riscv/cpu.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 0baed79ec2..964817b9d2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -240,11 +240,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)
 {
@@ -343,7 +338,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;
     register_cpu_props(obj);
 }
 
@@ -355,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV64, 0);
     register_cpu_props(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
@@ -366,7 +361,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);
     register_cpu_props(obj);
-    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
@@ -379,7 +374,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
     register_cpu_props(obj);
-    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);
@@ -392,7 +387,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-    set_priv_version(env, PRIV_VERSION_1_11_0);
+    env->priv_ver = PRIV_VERSION_1_11_0;
 
     cpu->cfg.ext_g = true;
     cpu->cfg.ext_c = true;
@@ -431,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV128, 0);
     register_cpu_props(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
@@ -444,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, 0);
     register_cpu_props(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
@@ -454,8 +449,9 @@ 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);
+
     register_cpu_props(obj);
-    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
@@ -468,7 +464,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
     register_cpu_props(obj);
-    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);
@@ -482,7 +478,7 @@ static void rv32_ibex_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
     register_cpu_props(obj);
-    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);
@@ -497,7 +493,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
     register_cpu_props(obj);
-    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);
@@ -1159,7 +1155,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;
     }
 
     /* Force disable extensions if priv spec version does not match */
-- 
2.39.2

Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()

[LIU Zhiwei]

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> The setter is doing nothing special. Just set env->priv_ver directly.
IMHO, No better than the older implementation.

Zhiwei

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 30 +++++++++++++-----------------
>   1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 0baed79ec2..964817b9d2 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -240,11 +240,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)
>   {
> @@ -343,7 +338,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;
>       register_cpu_props(obj);
>   }
>   
> @@ -355,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
>       set_misa(env, MXL_RV64, 0);
>       register_cpu_props(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
> @@ -366,7 +361,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);
>       register_cpu_props(obj);
> -    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
> @@ -379,7 +374,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
>   
>       set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
>       register_cpu_props(obj);
> -    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);
> @@ -392,7 +387,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>       RISCVCPU *cpu = RISCV_CPU(obj);
>   
>       set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> -    set_priv_version(env, PRIV_VERSION_1_11_0);
> +    env->priv_ver = PRIV_VERSION_1_11_0;
>   
>       cpu->cfg.ext_g = true;
>       cpu->cfg.ext_c = true;
> @@ -431,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
>       set_misa(env, MXL_RV128, 0);
>       register_cpu_props(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
> @@ -444,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
>       set_misa(env, MXL_RV32, 0);
>       register_cpu_props(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
> @@ -454,8 +449,9 @@ 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);
> +
>       register_cpu_props(obj);
> -    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
> @@ -468,7 +464,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
>   
>       set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
>       register_cpu_props(obj);
> -    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);
> @@ -482,7 +478,7 @@ static void rv32_ibex_cpu_init(Object *obj)
>   
>       set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
>       register_cpu_props(obj);
> -    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);
> @@ -497,7 +493,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>   
>       set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
>       register_cpu_props(obj);
> -    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);
> @@ -1159,7 +1155,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;
>       }
>   
>       /* Force disable extensions if priv spec version does not match */

Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()

[Daniel Henrique Barboza]

On 3/9/23 04:28, LIU Zhiwei wrote:
> 
> On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
>> The setter is doing nothing special. Just set env->priv_ver directly.
> IMHO, No better than the older implementation.

In the current context having a setter means that the function is doing
something else other than simply setting the attr. Because we're setting a
lot of other 'env' attrs directly: env->pc, env->priv, env->menvcfg and
so on. So a setter is a special function (e.g. set_misa()).

But then set_priv_version() and set_vext_version() are just setting
env->priv_ver/env->vext_version and nothing else. This means that every
time we read

"set_priv_version(env, val)"

We're either required to remember that this is just a simple setter or we spend
a few seconds looking it up to see that it's a simple setter. We could, instead,
just read

"env->priv_ver = val"

and moved on.


I really think we should get rid of all these kind of setters in the code. It's not
like these are user facing APIs that needs encapsulation.


Thanks,


Daniel



> 
> Zhiwei
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 30 +++++++++++++-----------------
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 0baed79ec2..964817b9d2 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -240,11 +240,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)
>>   {
>> @@ -343,7 +338,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;
>>       register_cpu_props(obj);
>>   }
>> @@ -355,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV64, 0);
>>       register_cpu_props(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
>> @@ -366,7 +361,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);
>>       register_cpu_props(obj);
>> -    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
>> @@ -379,7 +374,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
>>       register_cpu_props(obj);
>> -    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);
>> @@ -392,7 +387,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
>>       RISCVCPU *cpu = RISCV_CPU(obj);
>>       set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>> -    set_priv_version(env, PRIV_VERSION_1_11_0);
>> +    env->priv_ver = PRIV_VERSION_1_11_0;
>>       cpu->cfg.ext_g = true;
>>       cpu->cfg.ext_c = true;
>> @@ -431,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV128, 0);
>>       register_cpu_props(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
>> @@ -444,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV32, 0);
>>       register_cpu_props(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
>> @@ -454,8 +449,9 @@ 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);
>> +
>>       register_cpu_props(obj);
>> -    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
>> @@ -468,7 +464,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
>>       register_cpu_props(obj);
>> -    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);
>> @@ -482,7 +478,7 @@ static void rv32_ibex_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
>>       register_cpu_props(obj);
>> -    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);
>> @@ -497,7 +493,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>>       set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
>>       register_cpu_props(obj);
>> -    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);
>> @@ -1159,7 +1155,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;
>>       }
>>       /* Force disable extensions if priv spec version does not match */

Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()

[Alistair Francis]

On Fri, Mar 10, 2023 at 2:23 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 3/9/23 04:28, LIU Zhiwei wrote:
> >
> > On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> >> The setter is doing nothing special. Just set env->priv_ver directly.
> > IMHO, No better than the older implementation.
>
> In the current context having a setter means that the function is doing
> something else other than simply setting the attr. Because we're setting a
> lot of other 'env' attrs directly: env->pc, env->priv, env->menvcfg and
> so on. So a setter is a special function (e.g. set_misa()).
>
> But then set_priv_version() and set_vext_version() are just setting
> env->priv_ver/env->vext_version and nothing else. This means that every
> time we read
>
> "set_priv_version(env, val)"
>
> We're either required to remember that this is just a simple setter or we spend
> a few seconds looking it up to see that it's a simple setter. We could, instead,
> just read
>
> "env->priv_ver = val"
>
> and moved on.
>
>
> I really think we should get rid of all these kind of setters in the code. It's not
> like these are user facing APIs that needs encapsulation.

I tend to agree. I don't think they add anything. I guess you could
debate they are kind of self commenting as the function name describes
what is happening, but I think in a lot of cases it's pretty clear as
is.

Alistair

>
>
> Thanks,
>
>
> Daniel
>
>
>
> >
> > Zhiwei
> >
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   target/riscv/cpu.c | 30 +++++++++++++-----------------
> >>   1 file changed, 13 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index 0baed79ec2..964817b9d2 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -240,11 +240,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)
> >>   {
> >> @@ -343,7 +338,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;
> >>       register_cpu_props(obj);
> >>   }
> >> @@ -355,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV64, 0);
> >>       register_cpu_props(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
> >> @@ -366,7 +361,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);
> >>       register_cpu_props(obj);
> >> -    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
> >> @@ -379,7 +374,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> >>       register_cpu_props(obj);
> >> -    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);
> >> @@ -392,7 +387,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
> >>       RISCVCPU *cpu = RISCV_CPU(obj);
> >>       set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >> -    set_priv_version(env, PRIV_VERSION_1_11_0);
> >> +    env->priv_ver = PRIV_VERSION_1_11_0;
> >>       cpu->cfg.ext_g = true;
> >>       cpu->cfg.ext_c = true;
> >> @@ -431,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV128, 0);
> >>       register_cpu_props(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
> >> @@ -444,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV32, 0);
> >>       register_cpu_props(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
> >> @@ -454,8 +449,9 @@ 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);
> >> +
> >>       register_cpu_props(obj);
> >> -    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
> >> @@ -468,7 +464,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
> >>       register_cpu_props(obj);
> >> -    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);
> >> @@ -482,7 +478,7 @@ static void rv32_ibex_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
> >>       register_cpu_props(obj);
> >> -    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);
> >> @@ -497,7 +493,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
> >>       set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
> >>       register_cpu_props(obj);
> >> -    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);
> >> @@ -1159,7 +1155,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;
> >>       }
> >>       /* Force disable extensions if priv spec version does not match */
>

[PATCH for-8.1 04/17] target/riscv: add PRIV_VERSION_LATEST macro

[Daniel Henrique Barboza]

PRIV_VERSION_LATEST, at this moment assigned to PRIV_VERSION_1_12_0, is
used in all generic CPUs:

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

When a new PRIV version is made available we can just update the LATEST
macro.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 964817b9d2..62ef11180f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -338,7 +338,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;
     register_cpu_props(obj);
 }
 
@@ -350,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV64, 0);
     register_cpu_props(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
@@ -426,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV128, 0);
     register_cpu_props(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
@@ -439,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, 0);
     register_cpu_props(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 638e47c75a..af2e4b7695 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -89,6 +89,7 @@ enum {
     PRIV_VERSION_1_11_0,
     PRIV_VERSION_1_12_0,
 };
+#define PRIV_VERSION_LATEST PRIV_VERSION_1_12_0
 
 #define VEXT_VERSION_1_00_0 0x00010000
 
-- 
2.39.2

Re: [PATCH for-8.1 04/17] target/riscv: add PRIV_VERSION_LATEST macro

[Richard Henderson]

On 3/8/23 12:19, Daniel Henrique Barboza wrote:
>       PRIV_VERSION_1_11_0,
>       PRIV_VERSION_1_12_0,
>   };
> +#define PRIV_VERSION_LATEST PRIV_VERSION_1_12_0

Any reason not to make this a enumeration value:

   PRIV_VERSION_LATEST = PRIV_VERSION_1_12_0

?

r~

Re: [PATCH for-8.1 04/17] target/riscv: add PRIV_VERSION_LATEST macro

[Daniel Henrique Barboza]

On 3/8/23 20:00, Richard Henderson wrote:
> On 3/8/23 12:19, Daniel Henrique Barboza wrote:
>>       PRIV_VERSION_1_11_0,
>>       PRIV_VERSION_1_12_0,
>>   };
>> +#define PRIV_VERSION_LATEST PRIV_VERSION_1_12_0
> 
> Any reason not to make this a enumeration value:
> 
>    PRIV_VERSION_LATEST = PRIV_VERSION_1_12_0
> 
> ?

Hm, not particularly. I'll do that.


Daniel

> 
> r~

Re: [PATCH for-8.1 04/17] target/riscv: add PRIV_VERSION_LATEST macro

[LIU Zhiwei]

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> PRIV_VERSION_LATEST, at this moment assigned to PRIV_VERSION_1_12_0, is
> used in all generic CPUs:
>
> - riscv_any_cpu_init()
> - rv32_base_cpu_init()
> - rv64_base_cpu_init()
> - rv128_base_cpu_init()
>
> When a new PRIV version is made available we can just update the LATEST
> macro.

IMHO, we should remove the privileged version check in the future. It is 
a combination of many extensions and should not be a constraint as every 
extension has its own name.

Zhiwei

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 8 ++++----
>   target/riscv/cpu.h | 1 +
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 964817b9d2..62ef11180f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -338,7 +338,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;
>       register_cpu_props(obj);
>   }
>   
> @@ -350,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
>       set_misa(env, MXL_RV64, 0);
>       register_cpu_props(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
> @@ -426,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
>       set_misa(env, MXL_RV128, 0);
>       register_cpu_props(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
> @@ -439,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
>       set_misa(env, MXL_RV32, 0);
>       register_cpu_props(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 638e47c75a..af2e4b7695 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -89,6 +89,7 @@ enum {
>       PRIV_VERSION_1_11_0,
>       PRIV_VERSION_1_12_0,
>   };
> +#define PRIV_VERSION_LATEST PRIV_VERSION_1_12_0
>   
>   #define VEXT_VERSION_1_00_0 0x00010000
>   

[PATCH for-8.1 05/17] target/riscv/cpu.c: add riscv_cpu_validate_priv_spec()

[Daniel Henrique Barboza]

Put all the env->priv_spec related validation into a helper to unclog
riscv_cpu_realize a bit.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 62ef11180f..e15f829edc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -833,6 +833,48 @@ 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 i, 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;
+        }
+    }
+
+    if (priv_version >= PRIV_VERSION_1_10_0) {
+        env->priv_ver = priv_version;
+    }
+
+    /* 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, doing a set_misa() in the end.
@@ -1130,7 +1172,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);
@@ -1139,40 +1180,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;
-    }
-
-    /* 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
-        }
+    riscv_cpu_validate_priv_spec(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
     }
 
     if (cpu->cfg.epmp && !cpu->cfg.pmp) {
-- 
2.39.2

Re: [PATCH for-8.1 05/17] target/riscv/cpu.c: add riscv_cpu_validate_priv_spec()

[Richard Henderson]

On 3/8/23 12:19, Daniel Henrique Barboza wrote:
> +static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    int i, 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;
> +        }
> +    }
> +
> +    if (priv_version >= PRIV_VERSION_1_10_0) {
> +        env->priv_ver = priv_version;
> +    }

Merge these two if bodies, as the second condition is completely dependent on the first.


r~

[PATCH for-8.1 06/17] target/riscv: move realize() validations to riscv_cpu_validate_set_extensions()

[Daniel Henrique Barboza]

Center all validations that are scattered in riscv_cpu_realize() in the
same function.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e15f829edc..e2cd55320c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -881,9 +881,43 @@ static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
  */
 static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 {
+    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
+    CPUClass *cc = CPU_CLASS(mcc);
     CPURISCVState *env = &cpu->env;
+    Error *local_err = NULL;
     uint32_t ext = 0;
 
+    riscv_cpu_validate_priv_spec(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (cpu->cfg.epmp && !cpu->cfg.pmp) {
+        /*
+         * Enhanced PMP should only be available
+         * on harts with PMP support
+         */
+        error_setg(errp, "Invalid configuration: EPMP requires PMP support");
+        return;
+    }
+
+    /* 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);
+
     /* Do some ISA extension error checking */
     if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
                             cpu->cfg.ext_a && cpu->cfg.ext_f &&
@@ -1066,8 +1100,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         ext |= RVH;
     }
     if (cpu->cfg.ext_v) {
-        Error *local_err = NULL;
-
         riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
@@ -1169,9 +1201,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     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);
@@ -1180,51 +1210,17 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    riscv_cpu_validate_priv_spec(cpu, &local_err);
+    riscv_cpu_validate_set_extensions(cpu, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return;
     }
 
-    if (cpu->cfg.epmp && !cpu->cfg.pmp) {
-        /*
-         * Enhanced PMP should only be available
-         * on harts with PMP support
-         */
-        error_setg(errp, "Invalid configuration: EPMP requires PMP support");
-        return;
-    }
-
-
 #ifndef CONFIG_USER_ONLY
     if (cpu->cfg.ext_sstc) {
         riscv_timer_init(cpu);
     }
-#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);
-        return;
-    }
-
-#ifndef CONFIG_USER_ONLY
     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.39.2

[PATCH for-8.1 07/17] 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>
---
 target/riscv/cpu.c | 60 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 12 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e2cd55320c..499738d2dd 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -325,7 +325,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)
@@ -340,6 +341,12 @@ static void riscv_any_cpu_init(Object *obj)
 
     env->priv_ver = PRIV_VERSION_LATEST;
     register_cpu_props(obj);
+
+    /* 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)
@@ -358,13 +365,20 @@ 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);
     register_cpu_props(obj);
     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)
@@ -375,10 +389,14 @@ static void rv64_sifive_e_cpu_init(Object *obj)
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
     register_cpu_props(obj);
     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)
@@ -411,6 +429,10 @@ 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.ext_ifencei = true;
+    cpu->cfg.pmp = true;
 }
 
 static void rv128_base_cpu_init(Object *obj)
@@ -447,7 +469,8 @@ 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);
 
     register_cpu_props(obj);
@@ -455,6 +478,12 @@ static void rv32_sifive_u_cpu_init(Object *obj)
 #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)
@@ -465,10 +494,14 @@ static void rv32_sifive_e_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
     register_cpu_props(obj);
     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)
@@ -479,11 +512,15 @@ static void rv32_ibex_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
     register_cpu_props(obj);
     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)
@@ -494,10 +531,14 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
     register_cpu_props(obj);
     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
 
@@ -1357,11 +1398,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.39.2

[PATCH for-8.1 08/17] target/riscv/cpu.c: avoid set_misa() in validate_set_extensions()

[Daniel Henrique Barboza]

set_misa() will be tuned up to do more than it's already doing and it
will be redundant to what riscv_cpu_validate_set_extensions() does.

Note that we don't ever change env->misa_mlx in this function, so
set_misa() can be replaced by just assigning env->misa_ext and
env->misa_ext_mask to 'ext'.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 499738d2dd..dc6e444219 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1153,7 +1153,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         ext |= RVJ;
     }
 
-    set_misa(env, env->misa_mxl, ext);
+    env->misa_ext_mask = env->misa_ext = ext;
 }
 
 #ifndef CONFIG_USER_ONLY
-- 
2.39.2

[PATCH for-8.1 09/17] target/riscv/cpu.c: set cpu config in set_misa()

[Daniel Henrique Barboza]

set_misa() is setting all 'misa' related env states and nothing else.
But other functions, namely riscv_cpu_validate_set_extensions(), uses
the config object to do its job.

This creates a need to set the single letter extensions in the cfg
object to keep both in sync. At this moment this is being done by
register_cpu_props(), forcing every CPU to do a call to this function.

Let's beef up set_misa() and make the function do the sync for us. This
will relieve named CPUs to having to call register_cpu_props(), which
will then be redesigned to a more specialized role next.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 40 ++++++++++++++++++++++++++++++++--------
 target/riscv/cpu.h |  4 ++--
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index dc6e444219..08bdf861db 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -236,8 +236,40 @@ const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
 
 static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
 {
+    RISCVCPU *cpu;
+
     env->misa_mxl_max = env->misa_mxl = mxl;
     env->misa_ext_mask = env->misa_ext = ext;
+
+    /*
+     * ext = 0 will only be a thing during cpu_init() functions
+     * as a way of setting an extension-agnostic CPU. We do
+     * not support clearing misa_ext* and the ext_N flags in
+     * RISCVCPUConfig in regular circunstances.
+     */
+    if (ext == 0) {
+        return;
+    }
+
+    /*
+     * We can't use riscv_cpu_cfg() in this case because it is
+     * a read-only inline and we're going to change the values
+     * of cpu->cfg.
+     */
+    cpu = env_archcpu(env);
+
+    cpu->cfg.ext_i = ext & RVI;
+    cpu->cfg.ext_e = ext & RVE;
+    cpu->cfg.ext_m = ext & RVM;
+    cpu->cfg.ext_a = ext & RVA;
+    cpu->cfg.ext_f = ext & RVF;
+    cpu->cfg.ext_d = ext & RVD;
+    cpu->cfg.ext_v = ext & RVV;
+    cpu->cfg.ext_c = ext & RVC;
+    cpu->cfg.ext_s = ext & RVS;
+    cpu->cfg.ext_u = ext & RVU;
+    cpu->cfg.ext_h = ext & RVH;
+    cpu->cfg.ext_j = ext & RVJ;
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -340,7 +372,6 @@ static void riscv_any_cpu_init(Object *obj)
 #endif
 
     env->priv_ver = PRIV_VERSION_LATEST;
-    register_cpu_props(obj);
 
     /* inherited from parent obj via riscv_cpu_init() */
     cpu->cfg.ext_ifencei = true;
@@ -368,7 +399,6 @@ static void rv64_sifive_u_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
     CPURISCVState *env = &cpu->env;
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-    register_cpu_props(obj);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
@@ -387,7 +417,6 @@ static void rv64_sifive_e_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
-    register_cpu_props(obj);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -472,8 +501,6 @@ static void rv32_sifive_u_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
     CPURISCVState *env = &cpu->env;
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-
-    register_cpu_props(obj);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
@@ -492,7 +519,6 @@ static void rv32_sifive_e_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
-    register_cpu_props(obj);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -510,7 +536,6 @@ static void rv32_ibex_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
 
     set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
-    register_cpu_props(obj);
     env->priv_ver = PRIV_VERSION_1_11_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -529,7 +554,6 @@ 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);
-    register_cpu_props(obj);
     env->priv_ver = PRIV_VERSION_1_10_0;
 #ifndef CONFIG_USER_ONLY
     set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index af2e4b7695..f8baedd9c7 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -66,8 +66,8 @@
 #define RV(x) ((target_ulong)1 << (x - 'A'))
 
 /*
- * Consider updating register_cpu_props() when adding
- * new MISA bits here.
+ * Consider updating set_misa() when adding new
+ * MISA bits here.
  */
 #define RVI RV('I')
 #define RVE RV('E') /* E and I are mutually exclusive */
-- 
2.39.2

[PATCH for-8.1 10/17] target/riscv/cpu.c: redesign register_cpu_props()

[Daniel Henrique Barboza]

Now that the function is a no-op if 'env.misa_ext != 0', and no one that
are setting misa_ext != 0 is calling it because set_misa() is setting
the cpu cfg accordingly, remove the now deprecated code and rename the
function to register_generic_cpu_props().

This function is now doing exactly what the name says: it is creating
user-facing properties to allow changes in the CPU cfg via the QEMU
command line, setting default values if no user input is provided.

Note that there's the possibility of a CPU to set a certain misa value
and, at the same, also want user-facing flags and defaults from this
function. This is not the case since commit 26b2bc58599c ("target/riscv:
Don't expose the CPU properties on names CPUs"), but given that this is
also a possibility, clarify in the function that using this function
will overwrite existing values in cpu->cfg.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 08bdf861db..4988fd4d4b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -221,7 +221,7 @@ static const char * const riscv_intr_names[] = {
     "reserved"
 };
 
-static void register_cpu_props(Object *obj);
+static void register_generic_cpu_props(Object *obj);
 
 const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
 {
@@ -386,7 +386,7 @@ static void rv64_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV64, 0);
-    register_cpu_props(obj);
+    register_generic_cpu_props(obj);
     /* Set latest version of privileged specification */
     env->priv_ver = PRIV_VERSION_LATEST;
 #ifndef CONFIG_USER_ONLY
@@ -475,7 +475,7 @@ static void rv128_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV128, 0);
-    register_cpu_props(obj);
+    register_generic_cpu_props(obj);
     /* Set latest version of privileged specification */
     env->priv_ver = PRIV_VERSION_LATEST;
 #ifndef CONFIG_USER_ONLY
@@ -488,7 +488,7 @@ static void rv32_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV32, 0);
-    register_cpu_props(obj);
+    register_generic_cpu_props(obj);
     /* Set latest version of privileged specification */
     env->priv_ver = PRIV_VERSION_LATEST;
 #ifndef CONFIG_USER_ONLY
@@ -575,7 +575,7 @@ static void riscv_host_cpu_init(Object *obj)
 #elif defined(TARGET_RISCV64)
     set_misa(env, MXL_RV64, 0);
 #endif
-    register_cpu_props(obj);
+    register_generic_cpu_props(obj);
 }
 #endif
 
@@ -1529,44 +1529,16 @@ static Property riscv_cpu_extensions[] = {
 };
 
 /*
- * Register CPU props based on env.misa_ext. If a non-zero
- * value was set, register only the required cpu->cfg.ext_*
- * properties and leave. env.misa_ext = 0 means that we want
- * all the default properties to be registered.
+ * Register generic CPU props with user-facing flags declared
+ * in riscv_cpu_extensions[].
+ *
+ * Note that this will overwrite existing values in cpu->cfg.
  */
-static void register_cpu_props(Object *obj)
+static void register_generic_cpu_props(Object *obj)
 {
-    RISCVCPU *cpu = RISCV_CPU(obj);
-    uint32_t misa_ext = cpu->env.misa_ext;
     Property *prop;
     DeviceState *dev = DEVICE(obj);
 
-    /*
-     * If misa_ext is not zero, set cfg properties now to
-     * allow them to be read during riscv_cpu_realize()
-     * later on.
-     */
-    if (cpu->env.misa_ext != 0) {
-        cpu->cfg.ext_i = misa_ext & RVI;
-        cpu->cfg.ext_e = misa_ext & RVE;
-        cpu->cfg.ext_m = misa_ext & RVM;
-        cpu->cfg.ext_a = misa_ext & RVA;
-        cpu->cfg.ext_f = misa_ext & RVF;
-        cpu->cfg.ext_d = misa_ext & RVD;
-        cpu->cfg.ext_v = misa_ext & RVV;
-        cpu->cfg.ext_c = misa_ext & RVC;
-        cpu->cfg.ext_s = misa_ext & RVS;
-        cpu->cfg.ext_u = misa_ext & RVU;
-        cpu->cfg.ext_h = misa_ext & RVH;
-        cpu->cfg.ext_j = misa_ext & RVJ;
-
-        /*
-         * We don't want to set the default riscv_cpu_extensions
-         * in this case.
-         */
-        return;
-    }
-
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
         qdev_property_add_static(dev, prop);
     }
-- 
2.39.2

[PATCH for-8.1 11/17] target/riscv/cpu.c: move riscv_cpu_validate_v() up

[Daniel Henrique Barboza]

riscv_cpu_validate_set_extensions() will play a future role in
write_misa(). First we need to ensure that this function is validating
first and setting cfg values later. At this moment this is not the case
of the RVV validation.

Move RVV validation up. Leave the 'ext |= RVV' where it is - next patch
has plans for it.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4988fd4d4b..48838471b8 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1111,6 +1111,14 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         }
     }
 
+    if (cpu->cfg.ext_v) {
+        riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     if (cpu->cfg.ext_zk) {
         cpu->cfg.ext_zkn = true;
         cpu->cfg.ext_zkr = true;
@@ -1165,12 +1173,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         ext |= RVH;
     }
     if (cpu->cfg.ext_v) {
-        riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
-            return;
-        }
-
         ext |= RVV;
     }
     if (cpu->cfg.ext_j) {
-- 
2.39.2

[PATCH for-8.1 12/17] target/riscv: put env->misa_ext <-> cpu->cfg code into helpers

[Daniel Henrique Barboza]

The extremely tedious code that sets cpu->cfg based on misa_ext, and
vice-versa, is scattered around riscv_cpu_validate_set_extensions() and
set_misa().

Introduce helpers to do this work, cleaning up the logic of both
functions a bit. While we're at it, add a note in cpu.h informing that
any future change in MISA RV* bits should also be reflected in the
helpers as well.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 120 ++++++++++++++++++++++++---------------------
 target/riscv/cpu.h |   3 +-
 2 files changed, 65 insertions(+), 58 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 48838471b8..a564de01df 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -234,10 +234,69 @@ const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
     }
 }
 
-static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
+static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
 {
-    RISCVCPU *cpu;
+    uint32_t ext = 0;
 
+    if (cfg->ext_i) {
+        ext |= RVI;
+    }
+    if (cfg->ext_e) {
+        ext |= RVE;
+    }
+    if (cfg->ext_m) {
+        ext |= RVM;
+    }
+    if (cfg->ext_a) {
+        ext |= RVA;
+    }
+    if (cfg->ext_f) {
+        ext |= RVF;
+    }
+    if (cfg->ext_d) {
+        ext |= RVD;
+    }
+    if (cfg->ext_c) {
+        ext |= RVC;
+    }
+    if (cfg->ext_s) {
+        ext |= RVS;
+    }
+    if (cfg->ext_u) {
+        ext |= RVU;
+    }
+    if (cfg->ext_h) {
+        ext |= RVH;
+    }
+    if (cfg->ext_v) {
+        ext |= RVV;
+    }
+    if (cfg->ext_j) {
+        ext |= RVJ;
+    }
+
+    return ext;
+}
+
+static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
+                                       uint32_t misa_ext)
+{
+    cfg->ext_i = misa_ext & RVI;
+    cfg->ext_e = misa_ext & RVE;
+    cfg->ext_m = misa_ext & RVM;
+    cfg->ext_a = misa_ext & RVA;
+    cfg->ext_f = misa_ext & RVF;
+    cfg->ext_d = misa_ext & RVD;
+    cfg->ext_v = misa_ext & RVV;
+    cfg->ext_c = misa_ext & RVC;
+    cfg->ext_s = misa_ext & RVS;
+    cfg->ext_u = misa_ext & RVU;
+    cfg->ext_h = misa_ext & RVH;
+    cfg->ext_j = misa_ext & RVJ;
+}
+
+static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
+{
     env->misa_mxl_max = env->misa_mxl = mxl;
     env->misa_ext_mask = env->misa_ext = ext;
 
@@ -251,25 +310,7 @@ static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
         return;
     }
 
-    /*
-     * We can't use riscv_cpu_cfg() in this case because it is
-     * a read-only inline and we're going to change the values
-     * of cpu->cfg.
-     */
-    cpu = env_archcpu(env);
-
-    cpu->cfg.ext_i = ext & RVI;
-    cpu->cfg.ext_e = ext & RVE;
-    cpu->cfg.ext_m = ext & RVM;
-    cpu->cfg.ext_a = ext & RVA;
-    cpu->cfg.ext_f = ext & RVF;
-    cpu->cfg.ext_d = ext & RVD;
-    cpu->cfg.ext_v = ext & RVV;
-    cpu->cfg.ext_c = ext & RVC;
-    cpu->cfg.ext_s = ext & RVS;
-    cpu->cfg.ext_u = ext & RVU;
-    cpu->cfg.ext_h = ext & RVH;
-    cpu->cfg.ext_j = ext & RVJ;
+    riscv_set_cpucfg_with_misa(&env_archcpu(env)->cfg, ext);
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -1142,42 +1183,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_zksh = true;
     }
 
-    if (cpu->cfg.ext_i) {
-        ext |= RVI;
-    }
-    if (cpu->cfg.ext_e) {
-        ext |= RVE;
-    }
-    if (cpu->cfg.ext_m) {
-        ext |= RVM;
-    }
-    if (cpu->cfg.ext_a) {
-        ext |= RVA;
-    }
-    if (cpu->cfg.ext_f) {
-        ext |= RVF;
-    }
-    if (cpu->cfg.ext_d) {
-        ext |= RVD;
-    }
-    if (cpu->cfg.ext_c) {
-        ext |= RVC;
-    }
-    if (cpu->cfg.ext_s) {
-        ext |= RVS;
-    }
-    if (cpu->cfg.ext_u) {
-        ext |= RVU;
-    }
-    if (cpu->cfg.ext_h) {
-        ext |= RVH;
-    }
-    if (cpu->cfg.ext_v) {
-        ext |= RVV;
-    }
-    if (cpu->cfg.ext_j) {
-        ext |= RVJ;
-    }
+    ext = riscv_get_misa_ext_with_cpucfg(&cpu->cfg);
 
     env->misa_ext_mask = env->misa_ext = ext;
 }
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f8baedd9c7..529d8044c4 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -66,7 +66,8 @@
 #define RV(x) ((target_ulong)1 << (x - 'A'))
 
 /*
- * Consider updating set_misa() when adding new
+ * Consider updating riscv_get_misa_ext_with_cpucfg()
+ * and riscv_set_cpucfg_with_misa() when adding new
  * MISA bits here.
  */
 #define RVI RV('I')
-- 
2.39.2

[PATCH for-8.1 13/17] target/riscv/cpu.c: split riscv_cpu_validate_priv_spec()

[Daniel Henrique Barboza]

This function will validate and change/disable extensions that aren't
compatible with a certain spec version. Since the function is called
at the start of riscv_cpu_validate_set_extensions(), we're disabling
extensions without guaranteeing that they aren't being turned on again
after the validation step.

Create a new riscv_cpu_disable_priv_spec_isa_exts() helper and call it
at the end of riscv_cpu_validate_set_extensions(), right before
re-calculating the misa_ext value with the current config. This will
ensure that we're not re-enabling extensions that should be disabled by
the spec rula by accident.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index a564de01df..49f0fd2c11 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -942,7 +942,7 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
 static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
 {
     CPURISCVState *env = &cpu->env;
-    int i, priv_version = -1;
+    int priv_version = -1;
 
     if (cpu->cfg.priv_spec) {
         if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
@@ -962,6 +962,12 @@ static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
     if (priv_version >= PRIV_VERSION_1_10_0) {
         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++) {
@@ -1183,6 +1189,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         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);
+
     ext = riscv_get_misa_ext_with_cpucfg(&cpu->cfg);
 
     env->misa_ext_mask = env->misa_ext = ext;
-- 
2.39.2

[PATCH for-8.1 14/17] target/riscv/cpu.c: do not allow RVE to be set

[Daniel Henrique Barboza]

This restriction is found at the current implementation of write_misa()
in csr.c. Add it in riscv_cpu_validate_set_extensions() as well, while
also removing the checks we're doing considering that I or E can be
enabled.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 49f0fd2c11..7a5d202069 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1045,15 +1045,15 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         cpu->cfg.ext_ifencei = true;
     }
 
-    if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
-        error_setg(errp,
-                   "I and E extensions are incompatible");
+    /* We do not have RV32E support  */
+    if (cpu->cfg.ext_e) {
+        error_setg(errp, "E extension (RV32E) is not supported");
         return;
     }
 
-    if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
-        error_setg(errp,
-                   "Either I or E extension must be set");
+    /* When RV32E is supported we'll need to check for either I or E */
+    if (!cpu->cfg.ext_i) {
+        error_setg(errp, "I extension must be set");
         return;
     }
 
-- 
2.39.2

Re: [PATCH for-8.1 14/17] target/riscv/cpu.c: do not allow RVE to be set

[LIU Zhiwei]

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> This restriction is found at the current implementation of write_misa()
> in csr.c. Add it in riscv_cpu_validate_set_extensions() as well, while
> also removing the checks we're doing considering that I or E can be
> enabled.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 49f0fd2c11..7a5d202069 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1045,15 +1045,15 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           cpu->cfg.ext_ifencei = true;
>       }
>   
> -    if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
> -        error_setg(errp,
> -                   "I and E extensions are incompatible");
> +    /* We do not have RV32E support  */
> +    if (cpu->cfg.ext_e) {
> +        error_setg(errp, "E extension (RV32E) is not supported");
>           return;
>       }
>   
> -    if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
> -        error_setg(errp,
> -                   "Either I or E extension must be set");
> +    /* When RV32E is supported we'll need to check for either I or E */
> +    if (!cpu->cfg.ext_i) {
> +        error_setg(errp, "I extension must be set");

We currently have supported the RV64E and RV32E in fact. Although we 
miss some checking when decoding, the current QEMU can run programs 
written for RVE.  So we should not prohibit the RVE here.

Zhiwei

>           return;
>       }
>   

Re: [PATCH for-8.1 14/17] target/riscv/cpu.c: do not allow RVE to be set

[Daniel Henrique Barboza]

On 3/9/23 04:10, LIU Zhiwei wrote:
> 
> On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
>> This restriction is found at the current implementation of write_misa()
>> in csr.c. Add it in riscv_cpu_validate_set_extensions() as well, while
>> also removing the checks we're doing considering that I or E can be
>> enabled.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 49f0fd2c11..7a5d202069 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1045,15 +1045,15 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>           cpu->cfg.ext_ifencei = true;
>>       }
>> -    if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
>> -        error_setg(errp,
>> -                   "I and E extensions are incompatible");
>> +    /* We do not have RV32E support  */
>> +    if (cpu->cfg.ext_e) {
>> +        error_setg(errp, "E extension (RV32E) is not supported");
>>           return;
>>       }
>> -    if (!cpu->cfg.ext_i && !cpu->cfg.ext_e) {
>> -        error_setg(errp,
>> -                   "Either I or E extension must be set");
>> +    /* When RV32E is supported we'll need to check for either I or E */
>> +    if (!cpu->cfg.ext_i) {
>> +        error_setg(errp, "I extension must be set");
> 
> We currently have supported the RV64E and RV32E in fact. Although we miss some checking when decoding, the current QEMU can run programs written for RVE.  So we should not prohibit the RVE here.

Right, so I got fooled by write_misa() logic. I'll remove this patch.


Thanks,

Daniel

> 
> Zhiwei
> 
>>           return;
>>       }

[PATCH for-8.1 15/17] target/riscv: add RVG

[Daniel Henrique Barboza]

The 'G' bit in misa_ext is a virtual extension that enables a set of
extensions (i, m, a, f, d, icsr and ifencei). We'll want to avoid
setting it for write_misa(). Add it so we can gate write_misa() properly
against it.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7a5d202069..7be6a86305 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -274,6 +274,9 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
     if (cfg->ext_j) {
         ext |= RVJ;
     }
+    if (cfg->ext_g) {
+        ext |= RVG;
+    }
 
     return ext;
 }
@@ -293,6 +296,7 @@ static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
     cfg->ext_u = misa_ext & RVU;
     cfg->ext_h = misa_ext & RVH;
     cfg->ext_j = misa_ext & RVJ;
+    cfg->ext_g = misa_ext & RVG;
 }
 
 static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 529d8044c4..013a1389d6 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -82,6 +82,7 @@
 #define RVU RV('U')
 #define RVH RV('H')
 #define RVJ RV('J')
+#define RVG RV('G')
 
 
 /* Privileged specification version */
-- 
2.39.2

[PATCH for-8.1 16/17] target/riscv: do not allow RVG in write_misa()

[Daniel Henrique Barboza]

We're getting ready to use riscv_cpu_validate_set_extensions() to unify
the handling of write_misa() with the rest of the code base. But first
we need to deal with RVG.

The 'G' virtual extension enables a set of extensions in the CPU. At
this moment, this is done at the start of our validation step in
riscv_cpu_validate_set_extensions(). This means that enabling G will
enable other extensions in the CPU before resuming the validation.

This also means that, in case a write_misa() validation fails, we're
going to set cpu->cfg attributes that are unrelated to misa_ext bits
(icsr and ifencei). These would be 2 extra states that we would need to
store to fallback from a validation failure.

Since write_misa() is still on experimental state let's make our lives
easier for now and disable RVG updates.

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

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab566639e5..02a5c2a5ca 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1347,6 +1347,11 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
         return RISCV_EXCP_NONE;
     }
 
+    /* Changing 'G' state is unsupported */
+    if (val & RVG) {
+        return RISCV_EXCP_NONE;
+    }
+
     /* 'I' or 'E' must be present */
     if (!(val & (RVI | RVE))) {
         /* It is not, drop write to misa */
-- 
2.39.2

[PATCH for-8.1 17/17] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

write_misa() must use as much common logic as possible, only specifying
the bits that are exclusive to the CSR write operation and TCG
internals.

Rewrite write_misa() to work as follows:

- supress RVC right after verifying that we're not updating RVG;

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

- emulate the steps done by the cpu init() functions: set cpu->cfg using
  the desired misa value, validate it, and then commit;

- fallback if the validation step fails. We'll need to re-write cpu->cfg
  with the original misa_ext value for the hart.

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

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c |  7 +++---
 target/riscv/cpu.h |  2 ++
 target/riscv/csr.c | 53 +++++++++++++++++++++-------------------------
 3 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7be6a86305..4b2be32de3 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -281,8 +281,7 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
     return ext;
 }
 
-static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
-                                       uint32_t misa_ext)
+static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t misa_ext)
 {
     cfg->ext_i = misa_ext & RVI;
     cfg->ext_e = misa_ext & RVE;
@@ -299,7 +298,7 @@ static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
     cfg->ext_g = misa_ext & RVG;
 }
 
-static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
+void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
 {
     env->misa_mxl_max = env->misa_mxl = mxl;
     env->misa_ext_mask = env->misa_ext = ext;
@@ -995,7 +994,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly, doing a set_misa() in the end.
  */
-static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
+void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 {
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
     CPUClass *cc = CPU_CLASS(mcc);
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 013a1389d6..d64d0f8dd6 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -591,6 +591,8 @@ 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 set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
+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 02a5c2a5ca..2e75c75fcc 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1342,6 +1342,11 @@ 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 hart_ext_mask = env->misa_ext_mask;
+    uint32_t hart_ext = env->misa_ext;
+    Error *local_err = NULL;
+
     if (!riscv_cpu_cfg(env)->misa_w) {
         /* drop write to misa */
         return RISCV_EXCP_NONE;
@@ -1352,34 +1357,6 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
         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
@@ -1388,18 +1365,36 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
         val &= ~RVC;
     }
 
+    /* Mask extensions that are not supported by this hart */
+    val &= hart_ext_mask;
+
     /* If nothing changed, do nothing. */
     if (val == env->misa_ext) {
         return RISCV_EXCP_NONE;
     }
 
+    /*
+     * Validate the new configuration. Rollback to previous
+     * values if something goes wrong.
+     */
+    set_misa(env, env->misa_mxl, val);
+    riscv_cpu_validate_set_extensions(cpu, &local_err);
+    if (local_err) {
+        set_misa(env, env->misa_mxl, hart_ext);
+        return RISCV_EXCP_NONE;
+    }
+
+    /*
+     * Keep the original misa_ext_mask from the hart.
+     */
+    env->misa_ext_mask = hart_ext_mask;
+
     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.2

Re: [PATCH for-8.1 17/17] target/riscv: rework write_misa()

[LIU Zhiwei]

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> write_misa() must use as much common logic as possible, only specifying
> the bits that are exclusive to the CSR write operation and TCG
> internals.
>
> Rewrite write_misa() to work as follows:
>
> - supress RVC right after verifying that we're not updating RVG;
>
> - mask the write using misa_ext_mask to avoid enabling unsupported
>    extensions;
>
> - emulate the steps done by the cpu init() functions: set cpu->cfg using
>    the desired misa value, validate it, and then commit;
>
> - fallback if the validation step fails. We'll need to re-write cpu->cfg
>    with the original misa_ext value for the hart.
>
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c |  7 +++---
>   target/riscv/cpu.h |  2 ++
>   target/riscv/csr.c | 53 +++++++++++++++++++++-------------------------
>   3 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7be6a86305..4b2be32de3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -281,8 +281,7 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>       return ext;
>   }
>   
> -static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
> -                                       uint32_t misa_ext)
> +static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t misa_ext)
>   {
>       cfg->ext_i = misa_ext & RVI;
>       cfg->ext_e = misa_ext & RVE;
> @@ -299,7 +298,7 @@ static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>       cfg->ext_g = misa_ext & RVG;
>   }
>   
> -static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
> +void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>   {
>       env->misa_mxl_max = env->misa_mxl = mxl;
>       env->misa_ext_mask = env->misa_ext = ext;
> @@ -995,7 +994,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>    * Check consistency between chosen extensions while setting
>    * cpu->cfg accordingly, doing a set_misa() in the end.
>    */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>       CPUClass *cc = CPU_CLASS(mcc);
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 013a1389d6..d64d0f8dd6 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -591,6 +591,8 @@ 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 set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
> +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 02a5c2a5ca..2e75c75fcc 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1342,6 +1342,11 @@ 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 hart_ext_mask = env->misa_ext_mask;
> +    uint32_t hart_ext = env->misa_ext;
> +    Error *local_err = NULL;
> +
>       if (!riscv_cpu_cfg(env)->misa_w) {
>           /* drop write to misa */
>           return RISCV_EXCP_NONE;
> @@ -1352,34 +1357,6 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           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
> @@ -1388,18 +1365,36 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           val &= ~RVC;
>       }
>   
> +    /* Mask extensions that are not supported by this hart */
> +    val &= hart_ext_mask;
> +
>       /* If nothing changed, do nothing. */
>       if (val == env->misa_ext) {
>           return RISCV_EXCP_NONE;
>       }
>   
> +    /*
> +     * Validate the new configuration. Rollback to previous
> +     * values if something goes wrong.
> +     */
> +    set_misa(env, env->misa_mxl, val);
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);

Can we split the riscv_cpu_validate_set_extensions to validate and set 
functions? If so, we can remove the restore work here.
And I think the fields in misa are WARL, we should be able to write into 
each single field if it is legal.

Zhiwei

> +    if (local_err) {
> +        set_misa(env, env->misa_mxl, hart_ext);
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    /*
> +     * Keep the original misa_ext_mask from the hart.
> +     */
> +    env->misa_ext_mask = hart_ext_mask;
> +
>       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 for-8.1 17/17] target/riscv: rework write_misa()

[LIU Zhiwei]

On 2023/3/9 15:27, LIU Zhiwei wrote:
>
> On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
>> write_misa() must use as much common logic as possible, only specifying
>> the bits that are exclusive to the CSR write operation and TCG
>> internals.
>>
>> Rewrite write_misa() to work as follows:
>>
>> - supress RVC right after verifying that we're not updating RVG;
>>
>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>    extensions;
>>
>> - emulate the steps done by the cpu init() functions: set cpu->cfg using
>>    the desired misa value, validate it, and then commit;
>>
>> - fallback if the validation step fails. We'll need to re-write cpu->cfg
>>    with the original misa_ext value for the hart.
>>
>> Let's keep write_misa() as experimental for now until this logic gains
>> enough mileage.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c |  7 +++---
>>   target/riscv/cpu.h |  2 ++
>>   target/riscv/csr.c | 53 +++++++++++++++++++++-------------------------
>>   3 files changed, 29 insertions(+), 33 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 7be6a86305..4b2be32de3 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -281,8 +281,7 @@ static uint32_t 
>> riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>>       return ext;
>>   }
>>   -static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>> -                                       uint32_t misa_ext)
>> +static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t 
>> misa_ext)
>>   {
>>       cfg->ext_i = misa_ext & RVI;
>>       cfg->ext_e = misa_ext & RVE;
>> @@ -299,7 +298,7 @@ static void 
>> riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>>       cfg->ext_g = misa_ext & RVG;
>>   }
>>   -static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>> +void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>>   {
>>       env->misa_mxl_max = env->misa_mxl = mxl;
>>       env->misa_ext_mask = env->misa_ext = ext;
>> @@ -995,7 +994,7 @@ static void 
>> riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>>    * Check consistency between chosen extensions while setting
>>    * cpu->cfg accordingly, doing a set_misa() in the end.
>>    */
>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error 
>> **errp)
>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>   {
>>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>>       CPUClass *cc = CPU_CLASS(mcc);
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 013a1389d6..d64d0f8dd6 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -591,6 +591,8 @@ 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 set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
>> +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 02a5c2a5ca..2e75c75fcc 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1342,6 +1342,11 @@ 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 hart_ext_mask = env->misa_ext_mask;
>> +    uint32_t hart_ext = env->misa_ext;
>> +    Error *local_err = NULL;
>> +
>>       if (!riscv_cpu_cfg(env)->misa_w) {
>>           /* drop write to misa */
>>           return RISCV_EXCP_NONE;
>> @@ -1352,34 +1357,6 @@ static RISCVException write_misa(CPURISCVState 
>> *env, int csrno,
>>           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
>> @@ -1388,18 +1365,36 @@ static RISCVException 
>> write_misa(CPURISCVState *env, int csrno,
>>           val &= ~RVC;
>>       }
>>   +    /* Mask extensions that are not supported by this hart */
>> +    val &= hart_ext_mask;
>> +
>>       /* If nothing changed, do nothing. */
>>       if (val == env->misa_ext) {
>>           return RISCV_EXCP_NONE;
>>       }
>>   +    /*
>> +     * Validate the new configuration. Rollback to previous
>> +     * values if something goes wrong.
>> +     */
>> +    set_misa(env, env->misa_mxl, val);
>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);

The ideal sequence here is that validate_extentions(val) and set_misa().

Zhiwei

>
> Can we split the riscv_cpu_validate_set_extensions to validate and set 
> functions? If so, we can remove the restore work here.
> And I think the fields in misa are WARL, we should be able to write 
> into each single field if it is legal.
>
> Zhiwei
>
>> +    if (local_err) {
>> +        set_misa(env, env->misa_mxl, hart_ext);
>> +        return RISCV_EXCP_NONE;
>> +    }
>> +
>> +    /*
>> +     * Keep the original misa_ext_mask from the hart.
>> +     */
>> +    env->misa_ext_mask = hart_ext_mask;
>> +
>>       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 for-8.1 17/17] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

On 3/9/23 04:40, LIU Zhiwei wrote:
> 
> On 2023/3/9 15:27, LIU Zhiwei wrote:
>>
>> On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
>>> write_misa() must use as much common logic as possible, only specifying
>>> the bits that are exclusive to the CSR write operation and TCG
>>> internals.
>>>
>>> Rewrite write_misa() to work as follows:
>>>
>>> - supress RVC right after verifying that we're not updating RVG;
>>>
>>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>>    extensions;
>>>
>>> - emulate the steps done by the cpu init() functions: set cpu->cfg using
>>>    the desired misa value, validate it, and then commit;
>>>
>>> - fallback if the validation step fails. We'll need to re-write cpu->cfg
>>>    with the original misa_ext value for the hart.
>>>
>>> Let's keep write_misa() as experimental for now until this logic gains
>>> enough mileage.
>>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   target/riscv/cpu.c |  7 +++---
>>>   target/riscv/cpu.h |  2 ++
>>>   target/riscv/csr.c | 53 +++++++++++++++++++++-------------------------
>>>   3 files changed, 29 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index 7be6a86305..4b2be32de3 100644
>>> --- a/target/riscv/cpu.c
>>> +++ b/target/riscv/cpu.c
>>> @@ -281,8 +281,7 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>>>       return ext;
>>>   }
>>>   -static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>>> -                                       uint32_t misa_ext)
>>> +static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t misa_ext)
>>>   {
>>>       cfg->ext_i = misa_ext & RVI;
>>>       cfg->ext_e = misa_ext & RVE;
>>> @@ -299,7 +298,7 @@ static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>>>       cfg->ext_g = misa_ext & RVG;
>>>   }
>>>   -static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>>> +void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>>>   {
>>>       env->misa_mxl_max = env->misa_mxl = mxl;
>>>       env->misa_ext_mask = env->misa_ext = ext;
>>> @@ -995,7 +994,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>>>    * Check consistency between chosen extensions while setting
>>>    * cpu->cfg accordingly, doing a set_misa() in the end.
>>>    */
>>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>>   {
>>>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>>>       CPUClass *cc = CPU_CLASS(mcc);
>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>>> index 013a1389d6..d64d0f8dd6 100644
>>> --- a/target/riscv/cpu.h
>>> +++ b/target/riscv/cpu.h
>>> @@ -591,6 +591,8 @@ 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 set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
>>> +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 02a5c2a5ca..2e75c75fcc 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -1342,6 +1342,11 @@ 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 hart_ext_mask = env->misa_ext_mask;
>>> +    uint32_t hart_ext = env->misa_ext;
>>> +    Error *local_err = NULL;
>>> +
>>>       if (!riscv_cpu_cfg(env)->misa_w) {
>>>           /* drop write to misa */
>>>           return RISCV_EXCP_NONE;
>>> @@ -1352,34 +1357,6 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>           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
>>> @@ -1388,18 +1365,36 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>>           val &= ~RVC;
>>>       }
>>>   +    /* Mask extensions that are not supported by this hart */
>>> +    val &= hart_ext_mask;
>>> +
>>>       /* If nothing changed, do nothing. */
>>>       if (val == env->misa_ext) {
>>>           return RISCV_EXCP_NONE;
>>>       }
>>>   +    /*
>>> +     * Validate the new configuration. Rollback to previous
>>> +     * values if something goes wrong.
>>> +     */
>>> +    set_misa(env, env->misa_mxl, val);
>>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> 
> The ideal sequence here is that validate_extentions(val) and set_misa().

Yeah, after we separate between validate_extensions() and set_extensions() we
can validate it first using a blank 'val' instead of doing a set_misa() beforehand.


Thanks,

Daniel

> 
> Zhiwei
> 
>>
>> Can we split the riscv_cpu_validate_set_extensions to validate and set functions? If so, we can remove the restore work here.
>> And I think the fields in misa are WARL, we should be able to write into each single field if it is legal.
>>
>> Zhiwei
>>
>>> +    if (local_err) {
>>> +        set_misa(env, env->misa_mxl, hart_ext);
>>> +        return RISCV_EXCP_NONE;
>>> +    }
>>> +
>>> +    /*
>>> +     * Keep the original misa_ext_mask from the hart.
>>> +     */
>>> +    env->misa_ext_mask = hart_ext_mask;
>>> +
>>>       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 for-8.1 17/17] target/riscv: rework write_misa()

[Daniel Henrique Barboza]

On 3/9/23 04:27, LIU Zhiwei wrote:
> 
> On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
>> write_misa() must use as much common logic as possible, only specifying
>> the bits that are exclusive to the CSR write operation and TCG
>> internals.
>>
>> Rewrite write_misa() to work as follows:
>>
>> - supress RVC right after verifying that we're not updating RVG;
>>
>> - mask the write using misa_ext_mask to avoid enabling unsupported
>>    extensions;
>>
>> - emulate the steps done by the cpu init() functions: set cpu->cfg using
>>    the desired misa value, validate it, and then commit;
>>
>> - fallback if the validation step fails. We'll need to re-write cpu->cfg
>>    with the original misa_ext value for the hart.
>>
>> Let's keep write_misa() as experimental for now until this logic gains
>> enough mileage.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c |  7 +++---
>>   target/riscv/cpu.h |  2 ++
>>   target/riscv/csr.c | 53 +++++++++++++++++++++-------------------------
>>   3 files changed, 29 insertions(+), 33 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 7be6a86305..4b2be32de3 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -281,8 +281,7 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>>       return ext;
>>   }
>> -static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>> -                                       uint32_t misa_ext)
>> +static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t misa_ext)
>>   {
>>       cfg->ext_i = misa_ext & RVI;
>>       cfg->ext_e = misa_ext & RVE;
>> @@ -299,7 +298,7 @@ static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>>       cfg->ext_g = misa_ext & RVG;
>>   }
>> -static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>> +void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>>   {
>>       env->misa_mxl_max = env->misa_mxl = mxl;
>>       env->misa_ext_mask = env->misa_ext = ext;
>> @@ -995,7 +994,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>>    * Check consistency between chosen extensions while setting
>>    * cpu->cfg accordingly, doing a set_misa() in the end.
>>    */
>> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>   {
>>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>>       CPUClass *cc = CPU_CLASS(mcc);
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 013a1389d6..d64d0f8dd6 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -591,6 +591,8 @@ 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 set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
>> +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 02a5c2a5ca..2e75c75fcc 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1342,6 +1342,11 @@ 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 hart_ext_mask = env->misa_ext_mask;
>> +    uint32_t hart_ext = env->misa_ext;
>> +    Error *local_err = NULL;
>> +
>>       if (!riscv_cpu_cfg(env)->misa_w) {
>>           /* drop write to misa */
>>           return RISCV_EXCP_NONE;
>> @@ -1352,34 +1357,6 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>           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
>> @@ -1388,18 +1365,36 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>           val &= ~RVC;
>>       }
>> +    /* Mask extensions that are not supported by this hart */
>> +    val &= hart_ext_mask;
>> +
>>       /* If nothing changed, do nothing. */
>>       if (val == env->misa_ext) {
>>           return RISCV_EXCP_NONE;
>>       }
>> +    /*
>> +     * Validate the new configuration. Rollback to previous
>> +     * values if something goes wrong.
>> +     */
>> +    set_misa(env, env->misa_mxl, val);
>> +    riscv_cpu_validate_set_extensions(cpu, &local_err);
> 
> Can we split the riscv_cpu_validate_set_extensions to validate and set functions? If so, we can remove the restore work here.

Good idea. I'll do that.

> And I think the fields in misa are WARL, we should be able to write into each single field if it is legal.

The ISA mentions that:

=========
An implementation may impose additional constraints on the collective setting of
two or more misa fields, in which case they function collectively as a single WARL
field. An attempt to write an unsupported combination causes those bits to be set
to some supported combination.
=========

And I think  this is what we were already doing here: filter what we cannot support
before writing the CSR.


Thanks,

Daniel

> 
> Zhiwei
> 
>> +    if (local_err) {
>> +        set_misa(env, env->misa_mxl, hart_ext);
>> +        return RISCV_EXCP_NONE;
>> +    }
>> +
>> +    /*
>> +     * Keep the original misa_ext_mask from the hart.
>> +     */
>> +    env->misa_ext_mask = hart_ext_mask;
>> +
>>       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 for-8.1 00/17] centralize CPU extensions logic

[Daniel Henrique Barboza]

Just realized that the subject doesn't mention 'riscv' anywhere.

Yes, this is target/riscv specific. I'll make sure to mention that in the
future versions.

Thanks,


Daniel

On 3/8/23 17:19, Daniel Henrique Barboza wrote:
> Hi,
> 
> During the review of a series that did some work in the RISCV_FEATURES*
> enum, Liu Zhiwei commented on how we could centralize the all the
> extension validation code and integrate it with write_misa() [1].
> 
> This does at least part of what was suggested. The idea is that, ATM, we
> have too many places setting cpu->cfg and the validation logic is
> scattered around (e.g. there are some contraints in write_misa() that
> should be applicable elsewhere). This series is an attempt to centralize
> things a bit.
> 
> The main accomplishments of this series are:
> 
> - the parent device riscv-cpu no longer sets any cpu->cfg attribute. This
>    was forcing init() functions to disable extensions that it wouldn't
>    use just because the parent device was enabling it;
> 
> - all validations are centered in validate_set_extensions(), with
>    pontual exceptions in write_misa() that has exclusive contraints;
> 
> - set_misa() now writes cpu->cfg. No need to have one function to set
>    env->misa_ext and then another to set cpu->cfg;
> 
> - register_cpu_props() now only exposes user-facing props;
> 
> - all validations from validate_set_extensions() are done at the start
>    of the function. Validate first, set extensions after;
> 
> - RVE is now forbidden in all validations, not just in write_misa();
> 
> - RVG is now forbidden in write_misa();
> 
> - write_misa now uses set_misa() and validate_set_extensions().
> 
> 
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg05092.html
> 
> Daniel Henrique Barboza (17):
>    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 macro
>    target/riscv/cpu.c: add riscv_cpu_validate_priv_spec()
>    target/riscv: move realize() validations to
>      riscv_cpu_validate_set_extensions()
>    target/riscv/cpu.c: remove cfg setup from riscv_cpu_init()
>    target/riscv/cpu.c: avoid set_misa() in validate_set_extensions()
>    target/riscv/cpu.c: set cpu config in set_misa()
>    target/riscv/cpu.c: redesign register_cpu_props()
>    target/riscv/cpu.c: move riscv_cpu_validate_v() up
>    target/riscv: put env->misa_ext <-> cpu->cfg code into helpers
>    target/riscv/cpu.c: split riscv_cpu_validate_priv_spec()
>    target/riscv/cpu.c: do not allow RVE to be set
>    target/riscv: add RVG
>    target/riscv: do not allow RVG in write_misa()
>    target/riscv: rework write_misa()
> 
>   target/riscv/cpu.c | 516 +++++++++++++++++++++++++--------------------
>   target/riscv/cpu.h |   9 +-
>   target/riscv/csr.c |  52 ++---
>   3 files changed, 323 insertions(+), 254 deletions(-)
>