[PATCH 0/8] riscv: detecting user choice in TCG extensions

[PATCH 0/8] riscv: detecting user choice in TCG extensions

[Daniel Henrique Barboza]

Hi,

This series, based on the work done in "[PATCH for-8.2 v6 00/11] riscv:
add 'max' CPU, deprecate 'any'", aims to solve two problem we have in
TCG properties handling:

- we are not checking for priv_ver when auto-enabling extensions during
  realize();

- we are not able to honor user choice during validation because we're
  not able to tell if the user set an extension flag or not.

The solution adopted here is a modification of what we did in the RISC-V
KVM driver for 8.1. Instead of adding an 'user_set' flag and doing a
linear search in all the extensions (now split between multiple arrays),
we'll use a hash to store the 'offset' value of the property that the
user set in the command line in the set() callback of the property. The
existence of an entry in the hash indicates that the user set a value
for that extension, and the store val indicates what the user set. This
can be used in the future in case we decide to overwrite the user input
in a corner case.

Aside from this detail, the changes to convert the existing code to use
the new structure (RISCVCPUMultiExtConfig) is quite similar to what was
already done before in the KVM driver.

I also added a new concept called 'auto_update' (any other name is
welcome) to refer to the action of changing extension state during
realize() without user knowledge. All these cases are now wrapped in a
cpu_cfg_ext_auto_update() function that checks if the priv_ver is valid
and if the user set the extension to an specific val. User choice
prevails in this case, regardless of the noble intentions we have when
auto-updating an extensions (e.g. extension dependencies).

Yes, users will have an easier time breaking validation and guest boot,
but that's the idea. Users will have more power, and the adequate dose
of responsibility that comes with it.


Daniel Henrique Barboza (8):
  target/riscv/cpu.c: use offset in isa_ext_is_enabled/update_enabled
  target/riscv: make CPUCFG() macro public
  target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()
  target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize()
  target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig
  target/riscv: use isa_ext_update_enabled() in
    init_max_cpu_extensions()
  target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update()
  target/riscv/cpu.c: consider user option with RVG

 target/riscv/cpu.c | 392 +++++++++++++++++++++++++++++----------------
 target/riscv/cpu.h |   2 +
 target/riscv/kvm.c |   8 +-
 3 files changed, 261 insertions(+), 141 deletions(-)

-- 
2.41.0

[PATCH 1/8] target/riscv/cpu.c: use offset in isa_ext_is_enabled/update_enabled

[Daniel Henrique Barboza]

We'll have future usage for a function where, given an offset of the
struct RISCVCPUConfig, the flag is updated to a certain val.

Change all existing callers to use edata->ext_enable_offset instead of
'edata'.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b5a2266eef..644ce7a018 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -153,18 +153,17 @@ static const struct isa_ext_data isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
 };
 
-static bool isa_ext_is_enabled(RISCVCPU *cpu,
-                               const struct isa_ext_data *edata)
+static bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset)
 {
-    bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
+    bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
 
     return *ext_enabled;
 }
 
-static void isa_ext_update_enabled(RISCVCPU *cpu,
-                                   const struct isa_ext_data *edata, bool en)
+static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset,
+                                   bool en)
 {
-    bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
+    bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
 
     *ext_enabled = en;
 }
@@ -1025,9 +1024,10 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
 
     /* 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]) &&
+        if (isa_ext_is_enabled(cpu, isa_edata_arr[i].ext_enable_offset) &&
             (env->priv_ver < isa_edata_arr[i].min_version)) {
-            isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
+            isa_ext_update_enabled(cpu, isa_edata_arr[i].ext_enable_offset,
+                                   false);
 #ifndef CONFIG_USER_ONLY
             warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
                         " because privilege spec version does not match",
@@ -2271,7 +2271,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
     int i;
 
     for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
-        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
+        if (isa_ext_is_enabled(cpu, isa_edata_arr[i].ext_enable_offset)) {
             new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
             g_free(old);
             old = new;
-- 
2.41.0

Re: [PATCH 1/8] target/riscv/cpu.c: use offset in isa_ext_is_enabled/update_enabled

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We'll have future usage for a function where, given an offset of the
> struct RISCVCPUConfig, the flag is updated to a certain val.
>
> Change all existing callers to use edata->ext_enable_offset instead of
> 'edata'.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b5a2266eef..644ce7a018 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -153,18 +153,17 @@ static const struct isa_ext_data isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
>  };
>
> -static bool isa_ext_is_enabled(RISCVCPU *cpu,
> -                               const struct isa_ext_data *edata)
> +static bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset)
>  {
> -    bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> +    bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
>
>      return *ext_enabled;
>  }
>
> -static void isa_ext_update_enabled(RISCVCPU *cpu,
> -                                   const struct isa_ext_data *edata, bool en)
> +static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset,
> +                                   bool en)
>  {
> -    bool *ext_enabled = (void *)&cpu->cfg + edata->ext_enable_offset;
> +    bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
>
>      *ext_enabled = en;
>  }
> @@ -1025,9 +1024,10 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>
>      /* 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]) &&
> +        if (isa_ext_is_enabled(cpu, isa_edata_arr[i].ext_enable_offset) &&
>              (env->priv_ver < isa_edata_arr[i].min_version)) {
> -            isa_ext_update_enabled(cpu, &isa_edata_arr[i], false);
> +            isa_ext_update_enabled(cpu, isa_edata_arr[i].ext_enable_offset,
> +                                   false);
>  #ifndef CONFIG_USER_ONLY
>              warn_report("disabling %s extension for hart 0x" TARGET_FMT_lx
>                          " because privilege spec version does not match",
> @@ -2271,7 +2271,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>      int i;
>
>      for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> -        if (isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> +        if (isa_ext_is_enabled(cpu, isa_edata_arr[i].ext_enable_offset)) {
>              new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
>              g_free(old);
>              old = new;
> --
> 2.41.0
>
>

[PATCH 2/8] target/riscv: make CPUCFG() macro public

[Daniel Henrique Barboza]

The RISC-V KVM driver uses a CPUCFG() macro that calculates the offset
of a certain field in the struct RISCVCPUConfig. We're going to use this
macro in target/riscv/cpu.c as well in the next patches. Make it public.

Rename it to CPU_CFG_OFFSET() for more clarity while we're at it.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 644ce7a018..3e62881d85 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -48,7 +48,7 @@ struct isa_ext_data {
 };
 
 #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
-    {#_name, _min_ver, offsetof(struct RISCVCPUConfig, _prop)}
+    {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
 
 /*
  * From vector_helper.c
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 6ea22e0eea..577abcd724 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -62,6 +62,8 @@
 const char *riscv_get_misa_ext_name(uint32_t bit);
 const char *riscv_get_misa_ext_description(uint32_t bit);
 
+#define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
+
 /* Privileged specification version */
 enum {
     PRIV_VERSION_1_10_0 = 0,
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 9d8a8982f9..9b8565d809 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -198,10 +198,8 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
     }
 }
 
-#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
-
 #define KVM_EXT_CFG(_name, _prop, _reg_id) \
-    {.name = _name, .offset = CPUCFG(_prop), \
+    {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
      .kvm_reg_id = _reg_id}
 
 static KVMCPUConfig kvm_multi_ext_cfgs[] = {
@@ -278,13 +276,13 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
 
 static KVMCPUConfig kvm_cbom_blocksize = {
     .name = "cbom_blocksize",
-    .offset = CPUCFG(cbom_blocksize),
+    .offset = CPU_CFG_OFFSET(cbom_blocksize),
     .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicbom_block_size)
 };
 
 static KVMCPUConfig kvm_cboz_blocksize = {
     .name = "cboz_blocksize",
-    .offset = CPUCFG(cboz_blocksize),
+    .offset = CPU_CFG_OFFSET(cboz_blocksize),
     .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicboz_block_size)
 };
 
-- 
2.41.0

Re: [PATCH 2/8] target/riscv: make CPUCFG() macro public

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:20 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The RISC-V KVM driver uses a CPUCFG() macro that calculates the offset
> of a certain field in the struct RISCVCPUConfig. We're going to use this
> macro in target/riscv/cpu.c as well in the next patches. Make it public.
>
> Rename it to CPU_CFG_OFFSET() for more clarity while we're at it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 2 +-
>  target/riscv/cpu.h | 2 ++
>  target/riscv/kvm.c | 8 +++-----
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 644ce7a018..3e62881d85 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -48,7 +48,7 @@ struct isa_ext_data {
>  };
>
>  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
> -    {#_name, _min_ver, offsetof(struct RISCVCPUConfig, _prop)}
> +    {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
>
>  /*
>   * From vector_helper.c
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 6ea22e0eea..577abcd724 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -62,6 +62,8 @@
>  const char *riscv_get_misa_ext_name(uint32_t bit);
>  const char *riscv_get_misa_ext_description(uint32_t bit);
>
> +#define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
> +
>  /* Privileged specification version */
>  enum {
>      PRIV_VERSION_1_10_0 = 0,
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 9d8a8982f9..9b8565d809 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -198,10 +198,8 @@ static void kvm_riscv_update_cpu_misa_ext(RISCVCPU *cpu, CPUState *cs)
>      }
>  }
>
> -#define CPUCFG(_prop) offsetof(struct RISCVCPUConfig, _prop)
> -
>  #define KVM_EXT_CFG(_name, _prop, _reg_id) \
> -    {.name = _name, .offset = CPUCFG(_prop), \
> +    {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
>       .kvm_reg_id = _reg_id}
>
>  static KVMCPUConfig kvm_multi_ext_cfgs[] = {
> @@ -278,13 +276,13 @@ static void kvm_cpu_set_multi_ext_cfg(Object *obj, Visitor *v,
>
>  static KVMCPUConfig kvm_cbom_blocksize = {
>      .name = "cbom_blocksize",
> -    .offset = CPUCFG(cbom_blocksize),
> +    .offset = CPU_CFG_OFFSET(cbom_blocksize),
>      .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicbom_block_size)
>  };
>
>  static KVMCPUConfig kvm_cboz_blocksize = {
>      .name = "cboz_blocksize",
> -    .offset = CPUCFG(cboz_blocksize),
> +    .offset = CPU_CFG_OFFSET(cboz_blocksize),
>      .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicboz_block_size)
>  };
>
> --
> 2.41.0
>
>

[PATCH 3/8] target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()

[Daniel Henrique Barboza]

During realize() time we're activating a lot of extensions based on some
criteria, e.g.:

    if (cpu->cfg.ext_zk) {
        cpu->cfg.ext_zkn = true;
        cpu->cfg.ext_zkr = true;
        cpu->cfg.ext_zkt = true;
    }

This practice resulted in at least one case where we ended up enabling
something we shouldn't: RVC enabling zca/zcd/zcf when using a CPU that
has priv_spec older than 1.12.0.

We're also not considering user choice. There's no way of doing it now
but this is about to change in the next few patches.

cpu_cfg_ext_auto_update() will check for priv version mismatches before
enabling extensions. If we have a mismatch between the current priv
version and the extension we want to enable, do not enable it. In the
near future, this same function will also consider user choice when
deciding if we're going to enable/disable an extension or not.

For now let's use it to handle zca/zcd/zcf enablement if RVC is enabled.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3e62881d85..75dc83407e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -168,6 +168,44 @@ static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset,
     *ext_enabled = en;
 }
 
+static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
+        if (isa_edata_arr[i].ext_enable_offset != ext_offset) {
+            continue;
+        }
+
+        return isa_edata_arr[i].min_version;
+    }
+
+    /* Default to oldest priv spec if no match found */
+    return PRIV_VERSION_1_10_0;
+}
+
+static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
+                                    bool value)
+{
+    CPURISCVState *env = &cpu->env;
+    bool prev_val = isa_ext_is_enabled(cpu, ext_offset);
+    int min_version;
+
+    if (prev_val == value) {
+        return;
+    }
+
+    if (value && env->priv_ver != PRIV_VERSION_LATEST) {
+        /* Do not enable it if priv_ver is older than min_version */
+        min_version = cpu_cfg_ext_get_min_version(ext_offset);
+        if (env->priv_ver < min_version) {
+            return;
+        }
+    }
+
+    isa_ext_update_enabled(cpu, ext_offset, value);
+}
+
 const char * const riscv_int_regnames[] = {
     "x0/zero", "x1/ra",  "x2/sp",  "x3/gp",  "x4/tp",  "x5/t0",   "x6/t1",
     "x7/t2",   "x8/s0",  "x9/s1",  "x10/a0", "x11/a1", "x12/a2",  "x13/a3",
@@ -1248,12 +1286,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 
     /* zca, zcd and zcf has a PRIV 1.12.0 restriction */
     if (riscv_has_ext(env, RVC) && env->priv_ver >= PRIV_VERSION_1_12_0) {
-        cpu->cfg.ext_zca = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
         if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
-            cpu->cfg.ext_zcf = true;
+            cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
         }
         if (riscv_has_ext(env, RVD)) {
-            cpu->cfg.ext_zcd = true;
+            cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcd), true);
         }
     }
 
-- 
2.41.0

Re: [PATCH 3/8] target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:22 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> During realize() time we're activating a lot of extensions based on some
> criteria, e.g.:
>
>     if (cpu->cfg.ext_zk) {
>         cpu->cfg.ext_zkn = true;
>         cpu->cfg.ext_zkr = true;
>         cpu->cfg.ext_zkt = true;
>     }
>
> This practice resulted in at least one case where we ended up enabling
> something we shouldn't: RVC enabling zca/zcd/zcf when using a CPU that
> has priv_spec older than 1.12.0.
>
> We're also not considering user choice. There's no way of doing it now
> but this is about to change in the next few patches.
>
> cpu_cfg_ext_auto_update() will check for priv version mismatches before
> enabling extensions. If we have a mismatch between the current priv
> version and the extension we want to enable, do not enable it. In the
> near future, this same function will also consider user choice when
> deciding if we're going to enable/disable an extension or not.
>
> For now let's use it to handle zca/zcd/zcf enablement if RVC is enabled.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 3e62881d85..75dc83407e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -168,6 +168,44 @@ static void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset,
>      *ext_enabled = en;
>  }
>
> +static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> +        if (isa_edata_arr[i].ext_enable_offset != ext_offset) {
> +            continue;
> +        }
> +
> +        return isa_edata_arr[i].min_version;
> +    }
> +
> +    /* Default to oldest priv spec if no match found */
> +    return PRIV_VERSION_1_10_0;
> +}
> +
> +static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
> +                                    bool value)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    bool prev_val = isa_ext_is_enabled(cpu, ext_offset);
> +    int min_version;
> +
> +    if (prev_val == value) {
> +        return;
> +    }
> +
> +    if (value && env->priv_ver != PRIV_VERSION_LATEST) {
> +        /* Do not enable it if priv_ver is older than min_version */
> +        min_version = cpu_cfg_ext_get_min_version(ext_offset);
> +        if (env->priv_ver < min_version) {
> +            return;
> +        }
> +    }
> +
> +    isa_ext_update_enabled(cpu, ext_offset, value);
> +}
> +
>  const char * const riscv_int_regnames[] = {
>      "x0/zero", "x1/ra",  "x2/sp",  "x3/gp",  "x4/tp",  "x5/t0",   "x6/t1",
>      "x7/t2",   "x8/s0",  "x9/s1",  "x10/a0", "x11/a1", "x12/a2",  "x13/a3",
> @@ -1248,12 +1286,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>
>      /* zca, zcd and zcf has a PRIV 1.12.0 restriction */
>      if (riscv_has_ext(env, RVC) && env->priv_ver >= PRIV_VERSION_1_12_0) {
> -        cpu->cfg.ext_zca = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
>          if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
> -            cpu->cfg.ext_zcf = true;
> +            cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
>          }
>          if (riscv_has_ext(env, RVD)) {
> -            cpu->cfg.ext_zcd = true;
> +            cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcd), true);
>          }
>      }
>
> --
> 2.41.0
>
>

[PATCH 4/8] target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize()

[Daniel Henrique Barboza]

Let's change the other instances in realize() where we're enabling an
extension based on a certain criteria (e.g. it's a dependency of another
extension).

We're leaving icsr and ifencei being enabled during RVG for later -
we'll want to error out in that case. Every other extension enablement
during realize is now done via cpu_cfg_ext_auto_update().

The end goal is that only cpu init() functions will handle extension
flags directly via "cpu->cfg.ext_N = true|false".

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 75dc83407e..88b263e830 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1174,7 +1174,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     }
 
     if (cpu->cfg.ext_zfh) {
-        cpu->cfg.ext_zfhmin = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zfhmin), true);
     }
 
     if (cpu->cfg.ext_zfhmin && !riscv_has_ext(env, RVF)) {
@@ -1200,17 +1200,17 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         }
 
         /* The V vector extension depends on the Zve64d extension */
-        cpu->cfg.ext_zve64d = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve64d), true);
     }
 
     /* The Zve64d extension depends on the Zve64f extension */
     if (cpu->cfg.ext_zve64d) {
-        cpu->cfg.ext_zve64f = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve64f), true);
     }
 
     /* The Zve64f extension depends on the Zve32f extension */
     if (cpu->cfg.ext_zve64f) {
-        cpu->cfg.ext_zve32f = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve32f), true);
     }
 
     if (cpu->cfg.ext_zve64d && !riscv_has_ext(env, RVD)) {
@@ -1224,7 +1224,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     }
 
     if (cpu->cfg.ext_zvfh) {
-        cpu->cfg.ext_zvfhmin = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zvfhmin), true);
     }
 
     if (cpu->cfg.ext_zvfhmin && !cpu->cfg.ext_zve32f) {
@@ -1254,7 +1254,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 
     /* Set the ISA extensions, checks should have happened above */
     if (cpu->cfg.ext_zhinx) {
-        cpu->cfg.ext_zhinxmin = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
     }
 
     if ((cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinxmin) && !cpu->cfg.ext_zfinx) {
@@ -1275,12 +1275,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     }
 
     if (cpu->cfg.ext_zce) {
-        cpu->cfg.ext_zca = true;
-        cpu->cfg.ext_zcb = true;
-        cpu->cfg.ext_zcmp = true;
-        cpu->cfg.ext_zcmt = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcb), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmp), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmt), true);
         if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
-            cpu->cfg.ext_zcf = true;
+            cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
         }
     }
 
@@ -1329,26 +1329,26 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     }
 
     if (cpu->cfg.ext_zk) {
-        cpu->cfg.ext_zkn = true;
-        cpu->cfg.ext_zkr = true;
-        cpu->cfg.ext_zkt = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkn), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkr), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkt), true);
     }
 
     if (cpu->cfg.ext_zkn) {
-        cpu->cfg.ext_zbkb = true;
-        cpu->cfg.ext_zbkc = true;
-        cpu->cfg.ext_zbkx = true;
-        cpu->cfg.ext_zkne = true;
-        cpu->cfg.ext_zknd = true;
-        cpu->cfg.ext_zknh = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkb), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkc), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkx), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkne), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zknd), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zknh), true);
     }
 
     if (cpu->cfg.ext_zks) {
-        cpu->cfg.ext_zbkb = true;
-        cpu->cfg.ext_zbkc = true;
-        cpu->cfg.ext_zbkx = true;
-        cpu->cfg.ext_zksed = true;
-        cpu->cfg.ext_zksh = true;
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkb), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkc), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkx), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zksed), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zksh), true);
     }
 
     /*
-- 
2.41.0

Re: [PATCH 4/8] target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize()

[Alistair Francis]

On Fri, Jul 28, 2023 at 10:08 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Let's change the other instances in realize() where we're enabling an
> extension based on a certain criteria (e.g. it's a dependency of another
> extension).
>
> We're leaving icsr and ifencei being enabled during RVG for later -
> we'll want to error out in that case. Every other extension enablement
> during realize is now done via cpu_cfg_ext_auto_update().
>
> The end goal is that only cpu init() functions will handle extension
> flags directly via "cpu->cfg.ext_N = true|false".
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 50 +++++++++++++++++++++++-----------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 75dc83407e..88b263e830 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1174,7 +1174,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>      }
>
>      if (cpu->cfg.ext_zfh) {
> -        cpu->cfg.ext_zfhmin = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zfhmin), true);
>      }
>
>      if (cpu->cfg.ext_zfhmin && !riscv_has_ext(env, RVF)) {
> @@ -1200,17 +1200,17 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          }
>
>          /* The V vector extension depends on the Zve64d extension */
> -        cpu->cfg.ext_zve64d = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve64d), true);
>      }
>
>      /* The Zve64d extension depends on the Zve64f extension */
>      if (cpu->cfg.ext_zve64d) {
> -        cpu->cfg.ext_zve64f = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve64f), true);
>      }
>
>      /* The Zve64f extension depends on the Zve32f extension */
>      if (cpu->cfg.ext_zve64f) {
> -        cpu->cfg.ext_zve32f = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve32f), true);
>      }
>
>      if (cpu->cfg.ext_zve64d && !riscv_has_ext(env, RVD)) {
> @@ -1224,7 +1224,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>      }
>
>      if (cpu->cfg.ext_zvfh) {
> -        cpu->cfg.ext_zvfhmin = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zvfhmin), true);
>      }
>
>      if (cpu->cfg.ext_zvfhmin && !cpu->cfg.ext_zve32f) {
> @@ -1254,7 +1254,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>
>      /* Set the ISA extensions, checks should have happened above */
>      if (cpu->cfg.ext_zhinx) {
> -        cpu->cfg.ext_zhinxmin = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
>      }
>
>      if ((cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinxmin) && !cpu->cfg.ext_zfinx) {
> @@ -1275,12 +1275,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>      }
>
>      if (cpu->cfg.ext_zce) {
> -        cpu->cfg.ext_zca = true;
> -        cpu->cfg.ext_zcb = true;
> -        cpu->cfg.ext_zcmp = true;
> -        cpu->cfg.ext_zcmt = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcb), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmp), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmt), true);
>          if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
> -            cpu->cfg.ext_zcf = true;
> +            cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
>          }
>      }
>
> @@ -1329,26 +1329,26 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>      }
>
>      if (cpu->cfg.ext_zk) {
> -        cpu->cfg.ext_zkn = true;
> -        cpu->cfg.ext_zkr = true;
> -        cpu->cfg.ext_zkt = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkn), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkr), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkt), true);
>      }
>
>      if (cpu->cfg.ext_zkn) {
> -        cpu->cfg.ext_zbkb = true;
> -        cpu->cfg.ext_zbkc = true;
> -        cpu->cfg.ext_zbkx = true;
> -        cpu->cfg.ext_zkne = true;
> -        cpu->cfg.ext_zknd = true;
> -        cpu->cfg.ext_zknh = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkb), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkc), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkx), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkne), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zknd), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zknh), true);
>      }
>
>      if (cpu->cfg.ext_zks) {
> -        cpu->cfg.ext_zbkb = true;
> -        cpu->cfg.ext_zbkc = true;
> -        cpu->cfg.ext_zbkx = true;
> -        cpu->cfg.ext_zksed = true;
> -        cpu->cfg.ext_zksh = true;
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkb), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkc), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkx), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zksed), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zksh), true);
>      }
>
>      /*
> --
> 2.41.0
>
>

[PATCH 5/8] target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig

[Daniel Henrique Barboza]

If we want to make better decisions when auto-enabling extensions during
realize() we need a way to tell if an user set an extension manually.
The RISC-V KVM driver has its own solution via a KVMCPUConfig struct
that has an 'user_set' flag that is set during the Property set()
callback. The set() callback also does init() time validations based on
the current KVM driver capabilities.

For TCG we would want a 'user_set' mechanic too, but we would look
ad-hoc via cpu_cfg_ext_auto_update() if a certain extension was user set
or not. If we copy what was made in the KVM side we would look for
'user_set' for one into 60+ extension structs spreaded in 3 arrays
(riscv_cpu_extensions, riscv_cpu_experimental_exts,
riscv_cpu_vendor_exts).

We'll still need an extension struct but we won't be using the
'user_set' flag:

- 'RISCVCPUMultiExtConfig' will be our specialized structure, similar to what
we're already doing with the MISA extensions in 'RISCVCPUMisaExtConfig'.
DEFINE_PROP_BOOL() for all 3 extensions arrays were replaced by
MULTI_EXT_CFG_BOOL(), a macro that will init our specialized struct;

- the 'multi_ext_user_opts' hash will be used to store the offset of each
extension that the user set via the set() callback, cpu_set_multi_ext_cfg().
For now we're just initializing and populating it - next patch will use
it to determine if a certain extension was user set;

- cpu_add_multi_ext_prop() is a new helper that will replace the
qdev_property_add_static() calls that our macros are doing to populate
user properties. The macro was renamed to ADD_CPU_MULTIEXT_PROPS_ARRAY()
for clarity. Note that the non-extension properties in
riscv_cpu_options[] still need to be declared via qdev().

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 88b263e830..b588f6969f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -153,6 +153,9 @@ static const struct isa_ext_data isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
 };
 
+/* Hash that stores user set extensions */
+static GHashTable *multi_ext_user_opts;
+
 static bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset)
 {
     bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
@@ -1668,6 +1671,8 @@ static void riscv_cpu_init(Object *obj)
     qdev_init_gpio_in(DEVICE(cpu), riscv_cpu_set_irq,
                       IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
 #endif /* CONFIG_USER_ONLY */
+
+    multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
 }
 
 typedef struct RISCVCPUMisaExtConfig {
@@ -1819,94 +1824,104 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
     }
 }
 
-static Property riscv_cpu_extensions[] = {
+typedef struct RISCVCPUMultiExtConfig {
+    const char *name;
+    uint32_t offset;
+    bool enabled;
+} RISCVCPUMultiExtConfig;
+
+#define MULTI_EXT_CFG_BOOL(_name, _prop, _defval) \
+    {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
+     .enabled = _defval}
+
+static RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     /* Defaults for standard extensions */
-    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
-    DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
-    DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
-    DEFINE_PROP_BOOL("Zihintpause", RISCVCPU, cfg.ext_zihintpause, true),
-    DEFINE_PROP_BOOL("Zawrs", RISCVCPU, cfg.ext_zawrs, true),
-    DEFINE_PROP_BOOL("Zfa", RISCVCPU, cfg.ext_zfa, true),
-    DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
-    DEFINE_PROP_BOOL("Zfhmin", RISCVCPU, cfg.ext_zfhmin, false),
-    DEFINE_PROP_BOOL("Zve32f", RISCVCPU, cfg.ext_zve32f, false),
-    DEFINE_PROP_BOOL("Zve64f", RISCVCPU, cfg.ext_zve64f, false),
-    DEFINE_PROP_BOOL("Zve64d", RISCVCPU, cfg.ext_zve64d, false),
-    DEFINE_PROP_BOOL("sstc", RISCVCPU, cfg.ext_sstc, true),
-
-    DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
-    DEFINE_PROP_BOOL("svadu", RISCVCPU, cfg.ext_svadu, true),
-    DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
-    DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
-    DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
-
-    DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
-    DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
-    DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
-    DEFINE_PROP_BOOL("zbkb", RISCVCPU, cfg.ext_zbkb, false),
-    DEFINE_PROP_BOOL("zbkc", RISCVCPU, cfg.ext_zbkc, false),
-    DEFINE_PROP_BOOL("zbkx", RISCVCPU, cfg.ext_zbkx, false),
-    DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
-    DEFINE_PROP_BOOL("zk", RISCVCPU, cfg.ext_zk, false),
-    DEFINE_PROP_BOOL("zkn", RISCVCPU, cfg.ext_zkn, false),
-    DEFINE_PROP_BOOL("zknd", RISCVCPU, cfg.ext_zknd, false),
-    DEFINE_PROP_BOOL("zkne", RISCVCPU, cfg.ext_zkne, false),
-    DEFINE_PROP_BOOL("zknh", RISCVCPU, cfg.ext_zknh, false),
-    DEFINE_PROP_BOOL("zkr", RISCVCPU, cfg.ext_zkr, false),
-    DEFINE_PROP_BOOL("zks", RISCVCPU, cfg.ext_zks, false),
-    DEFINE_PROP_BOOL("zksed", RISCVCPU, cfg.ext_zksed, false),
-    DEFINE_PROP_BOOL("zksh", RISCVCPU, cfg.ext_zksh, false),
-    DEFINE_PROP_BOOL("zkt", RISCVCPU, cfg.ext_zkt, false),
-
-    DEFINE_PROP_BOOL("zdinx", RISCVCPU, cfg.ext_zdinx, false),
-    DEFINE_PROP_BOOL("zfinx", RISCVCPU, cfg.ext_zfinx, false),
-    DEFINE_PROP_BOOL("zhinx", RISCVCPU, cfg.ext_zhinx, false),
-    DEFINE_PROP_BOOL("zhinxmin", RISCVCPU, cfg.ext_zhinxmin, false),
-
-    DEFINE_PROP_BOOL("zicbom", RISCVCPU, cfg.ext_icbom, true),
-    DEFINE_PROP_BOOL("zicboz", RISCVCPU, cfg.ext_icboz, true),
-
-    DEFINE_PROP_BOOL("zmmul", RISCVCPU, cfg.ext_zmmul, false),
-
-    DEFINE_PROP_BOOL("zca", RISCVCPU, cfg.ext_zca, false),
-    DEFINE_PROP_BOOL("zcb", RISCVCPU, cfg.ext_zcb, false),
-    DEFINE_PROP_BOOL("zcd", RISCVCPU, cfg.ext_zcd, false),
-    DEFINE_PROP_BOOL("zce", RISCVCPU, cfg.ext_zce, false),
-    DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
-    DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
-    DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
+    MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
+    MULTI_EXT_CFG_BOOL("Zifencei", ext_ifencei, true),
+    MULTI_EXT_CFG_BOOL("Zicsr", ext_icsr, true),
+    MULTI_EXT_CFG_BOOL("Zihintpause", ext_zihintpause, true),
+    MULTI_EXT_CFG_BOOL("Zawrs", ext_zawrs, true),
+    MULTI_EXT_CFG_BOOL("Zfa", ext_zfa, true),
+    MULTI_EXT_CFG_BOOL("Zfh", ext_zfh, false),
+    MULTI_EXT_CFG_BOOL("Zfhmin", ext_zfhmin, false),
+    MULTI_EXT_CFG_BOOL("Zve32f", ext_zve32f, false),
+    MULTI_EXT_CFG_BOOL("Zve64f", ext_zve64f, false),
+    MULTI_EXT_CFG_BOOL("Zve64d", ext_zve64d, false),
+    MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
+
+    MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
+    MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
+    MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
+    MULTI_EXT_CFG_BOOL("svnapot", ext_svnapot, false),
+    MULTI_EXT_CFG_BOOL("svpbmt", ext_svpbmt, false),
+
+    MULTI_EXT_CFG_BOOL("zba", ext_zba, true),
+    MULTI_EXT_CFG_BOOL("zbb", ext_zbb, true),
+    MULTI_EXT_CFG_BOOL("zbc", ext_zbc, true),
+    MULTI_EXT_CFG_BOOL("zbkb", ext_zbkb, false),
+    MULTI_EXT_CFG_BOOL("zbkc", ext_zbkc, false),
+    MULTI_EXT_CFG_BOOL("zbkx", ext_zbkx, false),
+    MULTI_EXT_CFG_BOOL("zbs", ext_zbs, true),
+    MULTI_EXT_CFG_BOOL("zk", ext_zk, false),
+    MULTI_EXT_CFG_BOOL("zkn", ext_zkn, false),
+    MULTI_EXT_CFG_BOOL("zknd", ext_zknd, false),
+    MULTI_EXT_CFG_BOOL("zkne", ext_zkne, false),
+    MULTI_EXT_CFG_BOOL("zknh", ext_zknh, false),
+    MULTI_EXT_CFG_BOOL("zkr", ext_zkr, false),
+    MULTI_EXT_CFG_BOOL("zks", ext_zks, false),
+    MULTI_EXT_CFG_BOOL("zksed", ext_zksed, false),
+    MULTI_EXT_CFG_BOOL("zksh", ext_zksh, false),
+    MULTI_EXT_CFG_BOOL("zkt", ext_zkt, false),
+
+    MULTI_EXT_CFG_BOOL("zdinx", ext_zdinx, false),
+    MULTI_EXT_CFG_BOOL("zfinx", ext_zfinx, false),
+    MULTI_EXT_CFG_BOOL("zhinx", ext_zhinx, false),
+    MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
+
+    MULTI_EXT_CFG_BOOL("zicbom", ext_icbom, true),
+    MULTI_EXT_CFG_BOOL("zicboz", ext_icboz, true),
+
+    MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
+
+    MULTI_EXT_CFG_BOOL("zca", ext_zca, false),
+    MULTI_EXT_CFG_BOOL("zcb", ext_zcb, false),
+    MULTI_EXT_CFG_BOOL("zcd", ext_zcd, false),
+    MULTI_EXT_CFG_BOOL("zce", ext_zce, false),
+    MULTI_EXT_CFG_BOOL("zcf", ext_zcf, false),
+    MULTI_EXT_CFG_BOOL("zcmp", ext_zcmp, false),
+    MULTI_EXT_CFG_BOOL("zcmt", ext_zcmt, false),
 };
 
-static Property riscv_cpu_vendor_exts[] = {
-    DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
-    DEFINE_PROP_BOOL("xtheadbb", RISCVCPU, cfg.ext_xtheadbb, false),
-    DEFINE_PROP_BOOL("xtheadbs", RISCVCPU, cfg.ext_xtheadbs, false),
-    DEFINE_PROP_BOOL("xtheadcmo", RISCVCPU, cfg.ext_xtheadcmo, false),
-    DEFINE_PROP_BOOL("xtheadcondmov", RISCVCPU, cfg.ext_xtheadcondmov, false),
-    DEFINE_PROP_BOOL("xtheadfmemidx", RISCVCPU, cfg.ext_xtheadfmemidx, false),
-    DEFINE_PROP_BOOL("xtheadfmv", RISCVCPU, cfg.ext_xtheadfmv, false),
-    DEFINE_PROP_BOOL("xtheadmac", RISCVCPU, cfg.ext_xtheadmac, false),
-    DEFINE_PROP_BOOL("xtheadmemidx", RISCVCPU, cfg.ext_xtheadmemidx, false),
-    DEFINE_PROP_BOOL("xtheadmempair", RISCVCPU, cfg.ext_xtheadmempair, false),
-    DEFINE_PROP_BOOL("xtheadsync", RISCVCPU, cfg.ext_xtheadsync, false),
-    DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
+static RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
+    MULTI_EXT_CFG_BOOL("xtheadba", ext_xtheadba, false),
+    MULTI_EXT_CFG_BOOL("xtheadbb", ext_xtheadbb, false),
+    MULTI_EXT_CFG_BOOL("xtheadbs", ext_xtheadbs, false),
+    MULTI_EXT_CFG_BOOL("xtheadcmo", ext_xtheadcmo, false),
+    MULTI_EXT_CFG_BOOL("xtheadcondmov", ext_xtheadcondmov, false),
+    MULTI_EXT_CFG_BOOL("xtheadfmemidx", ext_xtheadfmemidx, false),
+    MULTI_EXT_CFG_BOOL("xtheadfmv", ext_xtheadfmv, false),
+    MULTI_EXT_CFG_BOOL("xtheadmac", ext_xtheadmac, false),
+    MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
+    MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
+    MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
+    MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
 };
 
 /* These are experimental so mark with 'x-' */
-static Property riscv_cpu_experimental_exts[] = {
-    DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
+static RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
+    MULTI_EXT_CFG_BOOL("x-zicond", ext_zicond, false),
 
     /* ePMP 0.9.3 */
-    DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
-    DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
-    DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
+    MULTI_EXT_CFG_BOOL("x-epmp", epmp, false),
+    MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
+    MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
 
-    DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
-    DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
+    MULTI_EXT_CFG_BOOL("x-zvfh", ext_zvfh, false),
+    MULTI_EXT_CFG_BOOL("x-zvfhmin", ext_zvfhmin, false),
 
-    DEFINE_PROP_BOOL("x-zfbfmin", RISCVCPU, cfg.ext_zfbfmin, false),
-    DEFINE_PROP_BOOL("x-zvfbfmin", RISCVCPU, cfg.ext_zvfbfmin, false),
-    DEFINE_PROP_BOOL("x-zvfbfwma", RISCVCPU, cfg.ext_zvfbfwma, false),
+    MULTI_EXT_CFG_BOOL("x-zfbfmin", ext_zfbfmin, false),
+    MULTI_EXT_CFG_BOOL("x-zvfbfmin", ext_zvfbfmin, false),
+    MULTI_EXT_CFG_BOOL("x-zvfbfwma", ext_zvfbfwma, false),
 };
 
 static Property riscv_cpu_options[] = {
@@ -1925,6 +1940,49 @@ static Property riscv_cpu_options[] = {
     DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
 };
 
+static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
+    bool value;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    isa_ext_update_enabled(RISCV_CPU(obj), multi_ext_cfg->offset, value);
+
+    g_hash_table_insert(multi_ext_user_opts,
+                        GUINT_TO_POINTER(multi_ext_cfg->offset),
+                        (gpointer)value);
+}
+
+static void cpu_get_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
+    bool value = isa_ext_is_enabled(RISCV_CPU(obj), multi_ext_cfg->offset);
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void cpu_add_multi_ext_prop(Object *cpu_obj,
+                                   RISCVCPUMultiExtConfig *multi_cfg)
+{
+    object_property_add(cpu_obj, multi_cfg->name, "bool",
+                        cpu_get_multi_ext_cfg,
+                        cpu_set_multi_ext_cfg,
+                        NULL, (void *)multi_cfg);
+
+    /*
+     * Set def val directly instead of using
+     * object_property_set_bool() to save the set()
+     * callback hash for user inputs.
+     */
+    isa_ext_update_enabled(RISCV_CPU(cpu_obj), multi_cfg->offset,
+                           multi_cfg->enabled);
+}
+
 #ifndef CONFIG_USER_ONLY
 static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
                                     const char *name,
@@ -1944,10 +2002,10 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
 }
 #endif
 
-#define ADD_CPU_QDEV_PROPERTIES_ARRAY(_dev, _array) \
+#define ADD_CPU_MULTIEXT_PROPS_ARRAY(_obj, _array) \
     do { \
         for (int i = 0; i < ARRAY_SIZE(_array); i++) { \
-            qdev_property_add_static(_dev, &_array[i]); \
+            cpu_add_multi_ext_prop(_obj, &_array[i]); \
         } \
     } while (0)
 
@@ -2005,8 +2063,6 @@ static void riscv_cpu_add_kvm_properties(Object *obj)
  */
 static void riscv_cpu_add_user_properties(Object *obj)
 {
-    DeviceState *dev = DEVICE(obj);
-
 #ifndef CONFIG_USER_ONLY
     riscv_add_satp_mode_properties(obj);
 
@@ -2018,10 +2074,13 @@ static void riscv_cpu_add_user_properties(Object *obj)
 
     riscv_cpu_add_misa_properties(obj);
 
-    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_extensions);
-    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_options);
-    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_vendor_exts);
-    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_experimental_exts);
+    ADD_CPU_MULTIEXT_PROPS_ARRAY(obj, riscv_cpu_extensions);
+    ADD_CPU_MULTIEXT_PROPS_ARRAY(obj, riscv_cpu_vendor_exts);
+    ADD_CPU_MULTIEXT_PROPS_ARRAY(obj, riscv_cpu_experimental_exts);
+
+    for (int i = 0; i < ARRAY_SIZE(riscv_cpu_options); i++) {
+        qdev_property_add_static(DEVICE(obj), &riscv_cpu_options[i]);
+    }
 }
 
 /*
-- 
2.41.0

Re: [PATCH 5/8] target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig

[Alistair Francis]

On Fri, Jul 28, 2023 at 10:06 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> If we want to make better decisions when auto-enabling extensions during
> realize() we need a way to tell if an user set an extension manually.
> The RISC-V KVM driver has its own solution via a KVMCPUConfig struct
> that has an 'user_set' flag that is set during the Property set()
> callback. The set() callback also does init() time validations based on
> the current KVM driver capabilities.
>
> For TCG we would want a 'user_set' mechanic too, but we would look
> ad-hoc via cpu_cfg_ext_auto_update() if a certain extension was user set
> or not. If we copy what was made in the KVM side we would look for
> 'user_set' for one into 60+ extension structs spreaded in 3 arrays
> (riscv_cpu_extensions, riscv_cpu_experimental_exts,
> riscv_cpu_vendor_exts).
>
> We'll still need an extension struct but we won't be using the
> 'user_set' flag:
>
> - 'RISCVCPUMultiExtConfig' will be our specialized structure, similar to what
> we're already doing with the MISA extensions in 'RISCVCPUMisaExtConfig'.
> DEFINE_PROP_BOOL() for all 3 extensions arrays were replaced by
> MULTI_EXT_CFG_BOOL(), a macro that will init our specialized struct;
>
> - the 'multi_ext_user_opts' hash will be used to store the offset of each
> extension that the user set via the set() callback, cpu_set_multi_ext_cfg().
> For now we're just initializing and populating it - next patch will use
> it to determine if a certain extension was user set;
>
> - cpu_add_multi_ext_prop() is a new helper that will replace the
> qdev_property_add_static() calls that our macros are doing to populate
> user properties. The macro was renamed to ADD_CPU_MULTIEXT_PROPS_ARRAY()
> for clarity. Note that the non-extension properties in
> riscv_cpu_options[] still need to be declared via qdev().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 231 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 145 insertions(+), 86 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 88b263e830..b588f6969f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -153,6 +153,9 @@ static const struct isa_ext_data isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, ext_XVentanaCondOps),
>  };
>
> +/* Hash that stores user set extensions */
> +static GHashTable *multi_ext_user_opts;
> +
>  static bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset)
>  {
>      bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
> @@ -1668,6 +1671,8 @@ static void riscv_cpu_init(Object *obj)
>      qdev_init_gpio_in(DEVICE(cpu), riscv_cpu_set_irq,
>                        IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
>  #endif /* CONFIG_USER_ONLY */
> +
> +    multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>  }
>
>  typedef struct RISCVCPUMisaExtConfig {
> @@ -1819,94 +1824,104 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>      }
>  }
>
> -static Property riscv_cpu_extensions[] = {
> +typedef struct RISCVCPUMultiExtConfig {
> +    const char *name;
> +    uint32_t offset;
> +    bool enabled;
> +} RISCVCPUMultiExtConfig;
> +
> +#define MULTI_EXT_CFG_BOOL(_name, _prop, _defval) \
> +    {.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
> +     .enabled = _defval}
> +
> +static RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      /* Defaults for standard extensions */
> -    DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
> -    DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> -    DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> -    DEFINE_PROP_BOOL("Zihintpause", RISCVCPU, cfg.ext_zihintpause, true),
> -    DEFINE_PROP_BOOL("Zawrs", RISCVCPU, cfg.ext_zawrs, true),
> -    DEFINE_PROP_BOOL("Zfa", RISCVCPU, cfg.ext_zfa, true),
> -    DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> -    DEFINE_PROP_BOOL("Zfhmin", RISCVCPU, cfg.ext_zfhmin, false),
> -    DEFINE_PROP_BOOL("Zve32f", RISCVCPU, cfg.ext_zve32f, false),
> -    DEFINE_PROP_BOOL("Zve64f", RISCVCPU, cfg.ext_zve64f, false),
> -    DEFINE_PROP_BOOL("Zve64d", RISCVCPU, cfg.ext_zve64d, false),
> -    DEFINE_PROP_BOOL("sstc", RISCVCPU, cfg.ext_sstc, true),
> -
> -    DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
> -    DEFINE_PROP_BOOL("svadu", RISCVCPU, cfg.ext_svadu, true),
> -    DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
> -    DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
> -    DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
> -
> -    DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
> -    DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
> -    DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> -    DEFINE_PROP_BOOL("zbkb", RISCVCPU, cfg.ext_zbkb, false),
> -    DEFINE_PROP_BOOL("zbkc", RISCVCPU, cfg.ext_zbkc, false),
> -    DEFINE_PROP_BOOL("zbkx", RISCVCPU, cfg.ext_zbkx, false),
> -    DEFINE_PROP_BOOL("zbs", RISCVCPU, cfg.ext_zbs, true),
> -    DEFINE_PROP_BOOL("zk", RISCVCPU, cfg.ext_zk, false),
> -    DEFINE_PROP_BOOL("zkn", RISCVCPU, cfg.ext_zkn, false),
> -    DEFINE_PROP_BOOL("zknd", RISCVCPU, cfg.ext_zknd, false),
> -    DEFINE_PROP_BOOL("zkne", RISCVCPU, cfg.ext_zkne, false),
> -    DEFINE_PROP_BOOL("zknh", RISCVCPU, cfg.ext_zknh, false),
> -    DEFINE_PROP_BOOL("zkr", RISCVCPU, cfg.ext_zkr, false),
> -    DEFINE_PROP_BOOL("zks", RISCVCPU, cfg.ext_zks, false),
> -    DEFINE_PROP_BOOL("zksed", RISCVCPU, cfg.ext_zksed, false),
> -    DEFINE_PROP_BOOL("zksh", RISCVCPU, cfg.ext_zksh, false),
> -    DEFINE_PROP_BOOL("zkt", RISCVCPU, cfg.ext_zkt, false),
> -
> -    DEFINE_PROP_BOOL("zdinx", RISCVCPU, cfg.ext_zdinx, false),
> -    DEFINE_PROP_BOOL("zfinx", RISCVCPU, cfg.ext_zfinx, false),
> -    DEFINE_PROP_BOOL("zhinx", RISCVCPU, cfg.ext_zhinx, false),
> -    DEFINE_PROP_BOOL("zhinxmin", RISCVCPU, cfg.ext_zhinxmin, false),
> -
> -    DEFINE_PROP_BOOL("zicbom", RISCVCPU, cfg.ext_icbom, true),
> -    DEFINE_PROP_BOOL("zicboz", RISCVCPU, cfg.ext_icboz, true),
> -
> -    DEFINE_PROP_BOOL("zmmul", RISCVCPU, cfg.ext_zmmul, false),
> -
> -    DEFINE_PROP_BOOL("zca", RISCVCPU, cfg.ext_zca, false),
> -    DEFINE_PROP_BOOL("zcb", RISCVCPU, cfg.ext_zcb, false),
> -    DEFINE_PROP_BOOL("zcd", RISCVCPU, cfg.ext_zcd, false),
> -    DEFINE_PROP_BOOL("zce", RISCVCPU, cfg.ext_zce, false),
> -    DEFINE_PROP_BOOL("zcf", RISCVCPU, cfg.ext_zcf, false),
> -    DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
> -    DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
> +    MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
> +    MULTI_EXT_CFG_BOOL("Zifencei", ext_ifencei, true),
> +    MULTI_EXT_CFG_BOOL("Zicsr", ext_icsr, true),
> +    MULTI_EXT_CFG_BOOL("Zihintpause", ext_zihintpause, true),
> +    MULTI_EXT_CFG_BOOL("Zawrs", ext_zawrs, true),
> +    MULTI_EXT_CFG_BOOL("Zfa", ext_zfa, true),
> +    MULTI_EXT_CFG_BOOL("Zfh", ext_zfh, false),
> +    MULTI_EXT_CFG_BOOL("Zfhmin", ext_zfhmin, false),
> +    MULTI_EXT_CFG_BOOL("Zve32f", ext_zve32f, false),
> +    MULTI_EXT_CFG_BOOL("Zve64f", ext_zve64f, false),
> +    MULTI_EXT_CFG_BOOL("Zve64d", ext_zve64d, false),
> +    MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
> +
> +    MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> +    MULTI_EXT_CFG_BOOL("svadu", ext_svadu, true),
> +    MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
> +    MULTI_EXT_CFG_BOOL("svnapot", ext_svnapot, false),
> +    MULTI_EXT_CFG_BOOL("svpbmt", ext_svpbmt, false),
> +
> +    MULTI_EXT_CFG_BOOL("zba", ext_zba, true),
> +    MULTI_EXT_CFG_BOOL("zbb", ext_zbb, true),
> +    MULTI_EXT_CFG_BOOL("zbc", ext_zbc, true),
> +    MULTI_EXT_CFG_BOOL("zbkb", ext_zbkb, false),
> +    MULTI_EXT_CFG_BOOL("zbkc", ext_zbkc, false),
> +    MULTI_EXT_CFG_BOOL("zbkx", ext_zbkx, false),
> +    MULTI_EXT_CFG_BOOL("zbs", ext_zbs, true),
> +    MULTI_EXT_CFG_BOOL("zk", ext_zk, false),
> +    MULTI_EXT_CFG_BOOL("zkn", ext_zkn, false),
> +    MULTI_EXT_CFG_BOOL("zknd", ext_zknd, false),
> +    MULTI_EXT_CFG_BOOL("zkne", ext_zkne, false),
> +    MULTI_EXT_CFG_BOOL("zknh", ext_zknh, false),
> +    MULTI_EXT_CFG_BOOL("zkr", ext_zkr, false),
> +    MULTI_EXT_CFG_BOOL("zks", ext_zks, false),
> +    MULTI_EXT_CFG_BOOL("zksed", ext_zksed, false),
> +    MULTI_EXT_CFG_BOOL("zksh", ext_zksh, false),
> +    MULTI_EXT_CFG_BOOL("zkt", ext_zkt, false),
> +
> +    MULTI_EXT_CFG_BOOL("zdinx", ext_zdinx, false),
> +    MULTI_EXT_CFG_BOOL("zfinx", ext_zfinx, false),
> +    MULTI_EXT_CFG_BOOL("zhinx", ext_zhinx, false),
> +    MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
> +
> +    MULTI_EXT_CFG_BOOL("zicbom", ext_icbom, true),
> +    MULTI_EXT_CFG_BOOL("zicboz", ext_icboz, true),
> +
> +    MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
> +
> +    MULTI_EXT_CFG_BOOL("zca", ext_zca, false),
> +    MULTI_EXT_CFG_BOOL("zcb", ext_zcb, false),
> +    MULTI_EXT_CFG_BOOL("zcd", ext_zcd, false),
> +    MULTI_EXT_CFG_BOOL("zce", ext_zce, false),
> +    MULTI_EXT_CFG_BOOL("zcf", ext_zcf, false),
> +    MULTI_EXT_CFG_BOOL("zcmp", ext_zcmp, false),
> +    MULTI_EXT_CFG_BOOL("zcmt", ext_zcmt, false),
>  };
>
> -static Property riscv_cpu_vendor_exts[] = {
> -    DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
> -    DEFINE_PROP_BOOL("xtheadbb", RISCVCPU, cfg.ext_xtheadbb, false),
> -    DEFINE_PROP_BOOL("xtheadbs", RISCVCPU, cfg.ext_xtheadbs, false),
> -    DEFINE_PROP_BOOL("xtheadcmo", RISCVCPU, cfg.ext_xtheadcmo, false),
> -    DEFINE_PROP_BOOL("xtheadcondmov", RISCVCPU, cfg.ext_xtheadcondmov, false),
> -    DEFINE_PROP_BOOL("xtheadfmemidx", RISCVCPU, cfg.ext_xtheadfmemidx, false),
> -    DEFINE_PROP_BOOL("xtheadfmv", RISCVCPU, cfg.ext_xtheadfmv, false),
> -    DEFINE_PROP_BOOL("xtheadmac", RISCVCPU, cfg.ext_xtheadmac, false),
> -    DEFINE_PROP_BOOL("xtheadmemidx", RISCVCPU, cfg.ext_xtheadmemidx, false),
> -    DEFINE_PROP_BOOL("xtheadmempair", RISCVCPU, cfg.ext_xtheadmempair, false),
> -    DEFINE_PROP_BOOL("xtheadsync", RISCVCPU, cfg.ext_xtheadsync, false),
> -    DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps, false),
> +static RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[] = {
> +    MULTI_EXT_CFG_BOOL("xtheadba", ext_xtheadba, false),
> +    MULTI_EXT_CFG_BOOL("xtheadbb", ext_xtheadbb, false),
> +    MULTI_EXT_CFG_BOOL("xtheadbs", ext_xtheadbs, false),
> +    MULTI_EXT_CFG_BOOL("xtheadcmo", ext_xtheadcmo, false),
> +    MULTI_EXT_CFG_BOOL("xtheadcondmov", ext_xtheadcondmov, false),
> +    MULTI_EXT_CFG_BOOL("xtheadfmemidx", ext_xtheadfmemidx, false),
> +    MULTI_EXT_CFG_BOOL("xtheadfmv", ext_xtheadfmv, false),
> +    MULTI_EXT_CFG_BOOL("xtheadmac", ext_xtheadmac, false),
> +    MULTI_EXT_CFG_BOOL("xtheadmemidx", ext_xtheadmemidx, false),
> +    MULTI_EXT_CFG_BOOL("xtheadmempair", ext_xtheadmempair, false),
> +    MULTI_EXT_CFG_BOOL("xtheadsync", ext_xtheadsync, false),
> +    MULTI_EXT_CFG_BOOL("xventanacondops", ext_XVentanaCondOps, false),
>  };
>
>  /* These are experimental so mark with 'x-' */
> -static Property riscv_cpu_experimental_exts[] = {
> -    DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
> +static RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[] = {
> +    MULTI_EXT_CFG_BOOL("x-zicond", ext_zicond, false),
>
>      /* ePMP 0.9.3 */
> -    DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
> -    DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
> -    DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
> +    MULTI_EXT_CFG_BOOL("x-epmp", epmp, false),
> +    MULTI_EXT_CFG_BOOL("x-smaia", ext_smaia, false),
> +    MULTI_EXT_CFG_BOOL("x-ssaia", ext_ssaia, false),
>
> -    DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
> -    DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
> +    MULTI_EXT_CFG_BOOL("x-zvfh", ext_zvfh, false),
> +    MULTI_EXT_CFG_BOOL("x-zvfhmin", ext_zvfhmin, false),
>
> -    DEFINE_PROP_BOOL("x-zfbfmin", RISCVCPU, cfg.ext_zfbfmin, false),
> -    DEFINE_PROP_BOOL("x-zvfbfmin", RISCVCPU, cfg.ext_zvfbfmin, false),
> -    DEFINE_PROP_BOOL("x-zvfbfwma", RISCVCPU, cfg.ext_zvfbfwma, false),
> +    MULTI_EXT_CFG_BOOL("x-zfbfmin", ext_zfbfmin, false),
> +    MULTI_EXT_CFG_BOOL("x-zvfbfmin", ext_zvfbfmin, false),
> +    MULTI_EXT_CFG_BOOL("x-zvfbfwma", ext_zvfbfwma, false),
>  };
>
>  static Property riscv_cpu_options[] = {
> @@ -1925,6 +1940,49 @@ static Property riscv_cpu_options[] = {
>      DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>  };
>
> +static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
> +    bool value;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    isa_ext_update_enabled(RISCV_CPU(obj), multi_ext_cfg->offset, value);
> +
> +    g_hash_table_insert(multi_ext_user_opts,
> +                        GUINT_TO_POINTER(multi_ext_cfg->offset),
> +                        (gpointer)value);
> +}
> +
> +static void cpu_get_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
> +    bool value = isa_ext_is_enabled(RISCV_CPU(obj), multi_ext_cfg->offset);
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void cpu_add_multi_ext_prop(Object *cpu_obj,
> +                                   RISCVCPUMultiExtConfig *multi_cfg)
> +{
> +    object_property_add(cpu_obj, multi_cfg->name, "bool",
> +                        cpu_get_multi_ext_cfg,
> +                        cpu_set_multi_ext_cfg,
> +                        NULL, (void *)multi_cfg);
> +
> +    /*
> +     * Set def val directly instead of using
> +     * object_property_set_bool() to save the set()
> +     * callback hash for user inputs.
> +     */
> +    isa_ext_update_enabled(RISCV_CPU(cpu_obj), multi_cfg->offset,
> +                           multi_cfg->enabled);
> +}
> +
>  #ifndef CONFIG_USER_ONLY
>  static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>                                      const char *name,
> @@ -1944,10 +2002,10 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
>  }
>  #endif
>
> -#define ADD_CPU_QDEV_PROPERTIES_ARRAY(_dev, _array) \
> +#define ADD_CPU_MULTIEXT_PROPS_ARRAY(_obj, _array) \
>      do { \
>          for (int i = 0; i < ARRAY_SIZE(_array); i++) { \
> -            qdev_property_add_static(_dev, &_array[i]); \
> +            cpu_add_multi_ext_prop(_obj, &_array[i]); \
>          } \
>      } while (0)
>
> @@ -2005,8 +2063,6 @@ static void riscv_cpu_add_kvm_properties(Object *obj)
>   */
>  static void riscv_cpu_add_user_properties(Object *obj)
>  {
> -    DeviceState *dev = DEVICE(obj);
> -
>  #ifndef CONFIG_USER_ONLY
>      riscv_add_satp_mode_properties(obj);
>
> @@ -2018,10 +2074,13 @@ static void riscv_cpu_add_user_properties(Object *obj)
>
>      riscv_cpu_add_misa_properties(obj);
>
> -    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_extensions);
> -    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_options);
> -    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_vendor_exts);
> -    ADD_CPU_QDEV_PROPERTIES_ARRAY(dev, riscv_cpu_experimental_exts);
> +    ADD_CPU_MULTIEXT_PROPS_ARRAY(obj, riscv_cpu_extensions);
> +    ADD_CPU_MULTIEXT_PROPS_ARRAY(obj, riscv_cpu_vendor_exts);
> +    ADD_CPU_MULTIEXT_PROPS_ARRAY(obj, riscv_cpu_experimental_exts);
> +
> +    for (int i = 0; i < ARRAY_SIZE(riscv_cpu_options); i++) {
> +        qdev_property_add_static(DEVICE(obj), &riscv_cpu_options[i]);
> +    }
>  }
>
>  /*
> --
> 2.41.0
>
>

[PATCH 6/8] target/riscv: use isa_ext_update_enabled() in init_max_cpu_extensions()

[Daniel Henrique Barboza]

Before adding support to detect if an extension was user set we need to
handle how we're enabling extensions in riscv_init_max_cpu_extensions().
object_property_set_bool() calls the set() callback for the property,
and we're going to use this callback to set the 'multi_ext_user_opts'
hash.

This means that, as is today, all extensions we're setting for the 'max'
CPU will be seen as user set in the future. Let's change set_bool() to
isa_ext_update_enabled() that will just enable/disable the flag on a
certain offset.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b588f6969f..a40dc865a0 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -2096,25 +2096,24 @@ static void riscv_init_max_cpu_extensions(Object *obj)
     set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
 
     for (int i = 0; i < ARRAY_SIZE(riscv_cpu_extensions); i++) {
-        object_property_set_bool(obj, riscv_cpu_extensions[i].name,
-                                 true, NULL);
+        isa_ext_update_enabled(cpu, riscv_cpu_extensions[i].offset, true);
     }
 
     /* set vector version */
     env->vext_ver = VEXT_VERSION_1_00_0;
 
     /* Zfinx is not compatible with F. Disable it */
-    object_property_set_bool(obj, "zfinx", false, NULL);
-    object_property_set_bool(obj, "zdinx", false, NULL);
-    object_property_set_bool(obj, "zhinx", false, NULL);
-    object_property_set_bool(obj, "zhinxmin", false, NULL);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zfinx), false);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zdinx), false);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zhinx), false);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zhinxmin), false);
 
-    object_property_set_bool(obj, "zce", false, NULL);
-    object_property_set_bool(obj, "zcmp", false, NULL);
-    object_property_set_bool(obj, "zcmt", false, NULL);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zce), false);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcmp), false);
+    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcmt), false);
 
     if (env->misa_mxl != MXL_RV32) {
-        object_property_set_bool(obj, "zcf", false, NULL);
+        isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcf), false);
     }
 }
 
-- 
2.41.0

Re: [PATCH 6/8] target/riscv: use isa_ext_update_enabled() in init_max_cpu_extensions()

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:51 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Before adding support to detect if an extension was user set we need to
> handle how we're enabling extensions in riscv_init_max_cpu_extensions().
> object_property_set_bool() calls the set() callback for the property,
> and we're going to use this callback to set the 'multi_ext_user_opts'
> hash.
>
> This means that, as is today, all extensions we're setting for the 'max'
> CPU will be seen as user set in the future. Let's change set_bool() to
> isa_ext_update_enabled() that will just enable/disable the flag on a
> certain offset.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b588f6969f..a40dc865a0 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2096,25 +2096,24 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>      set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
>
>      for (int i = 0; i < ARRAY_SIZE(riscv_cpu_extensions); i++) {
> -        object_property_set_bool(obj, riscv_cpu_extensions[i].name,
> -                                 true, NULL);
> +        isa_ext_update_enabled(cpu, riscv_cpu_extensions[i].offset, true);
>      }
>
>      /* set vector version */
>      env->vext_ver = VEXT_VERSION_1_00_0;
>
>      /* Zfinx is not compatible with F. Disable it */
> -    object_property_set_bool(obj, "zfinx", false, NULL);
> -    object_property_set_bool(obj, "zdinx", false, NULL);
> -    object_property_set_bool(obj, "zhinx", false, NULL);
> -    object_property_set_bool(obj, "zhinxmin", false, NULL);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zfinx), false);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zdinx), false);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zhinx), false);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zhinxmin), false);
>
> -    object_property_set_bool(obj, "zce", false, NULL);
> -    object_property_set_bool(obj, "zcmp", false, NULL);
> -    object_property_set_bool(obj, "zcmt", false, NULL);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zce), false);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcmp), false);
> +    isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcmt), false);
>
>      if (env->misa_mxl != MXL_RV32) {
> -        object_property_set_bool(obj, "zcf", false, NULL);
> +        isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcf), false);
>      }
>  }
>
> --
> 2.41.0
>
>

[PATCH 7/8] target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update()

[Daniel Henrique Barboza]

Add a new cpu_cfg_ext_is_user_set() helper to check if an extension was
set by the user in the command line. Use it inside
cpu_cfg_ext_auto_update() to verify if the user set a certain extension
and, if that's the case, do not change its value.

This will make us honor user choice instead of overwriting the values.
Users will then be informed whether they're using an incompatible set of
extensions instead of QEMU setting a magic value that works.

For example, we'll now error out if the user explictly set 'zce' to true
and 'zca' to false:

$ ./build/qemu-system-riscv64 -M virt -cpu rv64,zce=true,zca=false -nographic
qemu-system-riscv64: Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca extension

This didn't happen before because we were enabling 'zca' if 'zce' was enabled
regardless if the user explictly set 'zca' to false.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index a40dc865a0..644d0fdad2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -187,6 +187,12 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
     return PRIV_VERSION_1_10_0;
 }
 
+static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
+{
+    return g_hash_table_contains(multi_ext_user_opts,
+                                 GUINT_TO_POINTER(ext_offset));
+}
+
 static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
                                     bool value)
 {
@@ -198,6 +204,10 @@ static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
         return;
     }
 
+    if (cpu_cfg_ext_is_user_set(ext_offset)) {
+        return;
+    }
+
     if (value && env->priv_ver != PRIV_VERSION_LATEST) {
         /* Do not enable it if priv_ver is older than min_version */
         min_version = cpu_cfg_ext_get_min_version(ext_offset);
-- 
2.41.0

Re: [PATCH 7/8] target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update()

[Alistair Francis]

On Fri, Jul 28, 2023 at 10:32 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Add a new cpu_cfg_ext_is_user_set() helper to check if an extension was
> set by the user in the command line. Use it inside
> cpu_cfg_ext_auto_update() to verify if the user set a certain extension
> and, if that's the case, do not change its value.
>
> This will make us honor user choice instead of overwriting the values.
> Users will then be informed whether they're using an incompatible set of
> extensions instead of QEMU setting a magic value that works.
>
> For example, we'll now error out if the user explictly set 'zce' to true
> and 'zca' to false:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu rv64,zce=true,zca=false -nographic
> qemu-system-riscv64: Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca extension
>
> This didn't happen before because we were enabling 'zca' if 'zce' was enabled
> regardless if the user explictly set 'zca' to false.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index a40dc865a0..644d0fdad2 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -187,6 +187,12 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
>      return PRIV_VERSION_1_10_0;
>  }
>
> +static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
> +{
> +    return g_hash_table_contains(multi_ext_user_opts,
> +                                 GUINT_TO_POINTER(ext_offset));
> +}
> +
>  static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
>                                      bool value)
>  {
> @@ -198,6 +204,10 @@ static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
>          return;
>      }
>
> +    if (cpu_cfg_ext_is_user_set(ext_offset)) {
> +        return;
> +    }
> +
>      if (value && env->priv_ver != PRIV_VERSION_LATEST) {
>          /* Do not enable it if priv_ver is older than min_version */
>          min_version = cpu_cfg_ext_get_min_version(ext_offset);
> --
> 2.41.0
>
>

[PATCH 8/8] target/riscv/cpu.c: consider user option with RVG

[Daniel Henrique Barboza]

Enabling RVG will enable a set of extensions that we're not checking if
the user was okay enabling or not. And in this case we want to error
out, instead of ignoring, otherwise we will be inconsistent enabling RVG
without all its extensions.

After this patch, disabling ifencei or icsr while enabling RVG will
result in error:

$ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic
qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 644d0fdad2..72a36b47ed 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1135,8 +1135,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
           riscv_has_ext(env, RVD) &&
           cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
         warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
-        cpu->cfg.ext_icsr = true;
-        cpu->cfg.ext_ifencei = true;
+
+        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_icsr)) &&
+            !cpu->cfg.ext_icsr) {
+            error_setg(errp, "RVG requires Zicsr but user set Zicsr to false");
+            return;
+        }
+
+        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_ifencei)) &&
+            !cpu->cfg.ext_ifencei) {
+            error_setg(errp, "RVG requires Zifencei but user set "
+                       "Zifencei to false");
+            return;
+        }
+
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
+        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
 
         env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
         env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
-- 
2.41.0

Re: [PATCH 8/8] target/riscv/cpu.c: consider user option with RVG

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:39 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Enabling RVG will enable a set of extensions that we're not checking if
> the user was okay enabling or not. And in this case we want to error
> out, instead of ignoring, otherwise we will be inconsistent enabling RVG
> without all its extensions.
>
> After this patch, disabling ifencei or icsr while enabling RVG will
> result in error:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 644d0fdad2..72a36b47ed 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1135,8 +1135,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>            riscv_has_ext(env, RVD) &&
>            cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
>          warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> -        cpu->cfg.ext_icsr = true;
> -        cpu->cfg.ext_ifencei = true;
> +
> +        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_icsr)) &&
> +            !cpu->cfg.ext_icsr) {
> +            error_setg(errp, "RVG requires Zicsr but user set Zicsr to false");
> +            return;
> +        }
> +
> +        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_ifencei)) &&
> +            !cpu->cfg.ext_ifencei) {
> +            error_setg(errp, "RVG requires Zifencei but user set "
> +                       "Zifencei to false");
> +            return;
> +        }
> +
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>          env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> --
> 2.41.0
>
>