[PATCH v3 00/16] target/riscv: deprecate riscv_cpu_options[]

[PATCH v3 00/16] target/riscv: deprecate riscv_cpu_options[]

[Daniel Henrique Barboza]

Hi,

In v2, after Vladimir's feedback, the following major changes were made:

- add default values in riscv_cpu_init() instead of doing gimmicks with
  setters and .set_default_value callbacks. The cost of having defaults
  for non-boolean properties in the parent class is smaller than having
  to deal with duplicated 'if cfg.prop == 0' checks in all setters;

- all properties are now being registered in the 'general_user_opts'
  hash. We were registering only the properties that KVM checks,
  Vladimir pointed out that this isn't consistent, we're making it
  consistent now.

Other smaller changes were made in patches 1, 2, 3 and 7.

Vladimir, due to the amount of changes I didn't add your "Tested-by" in
the patches. You're welcome to give this another try and add a tested-by
again :)

Patches based on riscv-to-apply.next. 

Changes from v2:
- patch 1:
  - remove 'bext_ver' from cpu.h
- patch 2 and 3: switched places
- patch 3 (former 2):
  - added hash implementation from patch 8
  - added pmu_mask setter()
  - pmu-mask and pmu-num can't be changed for vendor CPUs
- patches 4, 5, 6:
  - properties are now being added in the hash
- patch 7:
  - fixed string check in prop_vext_spec_set()
  - vext_spec is now being added in the hash
- patches 8, 9, 11, 12:
  - default values added in riscv_cpu_init()
  - no longer checking for "value == 0" in setters
- v2 link: https://lore.kernel.org/qemu-riscv/20231222122235.545235-1-dbarboza@ventanamicro.com/

Daniel Henrique Barboza (16):
  target/riscv/cpu_cfg.h: remove unused fields
  target/riscv: make riscv_cpu_is_generic() public
  target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]
  target/riscv: move 'mmu' to riscv_cpu_properties[]
  target/riscv: move 'pmp' to riscv_cpu_properties[]
  target/riscv: rework 'priv_spec'
  target/riscv: rework 'vext_spec'
  target/riscv: move 'vlen' to riscv_cpu_properties[]
  target/riscv: move 'elen' to riscv_cpu_properties[]
  target/riscv: create finalize_features() for KVM
  target/riscv: move 'cbom_blocksize' to riscv_cpu_properties[]
  target/riscv: move 'cboz_blocksize' to riscv_cpu_properties[]
  target/riscv: remove riscv_cpu_options[]
  target/riscv/cpu.c: move 'mvendorid' to riscv_cpu_properties[]
  target/riscv/cpu.c: move 'mimpid' to riscv_cpu_properties[]
  target/riscv/cpu.c: move 'marchid' to riscv_cpu_properties[]

 target/riscv/cpu.c           | 614 ++++++++++++++++++++++++++++++-----
 target/riscv/cpu.h           |   8 +-
 target/riscv/cpu_cfg.h       |   4 -
 target/riscv/kvm/kvm-cpu.c   |  94 +++---
 target/riscv/kvm/kvm_riscv.h |   1 +
 target/riscv/tcg/tcg-cpu.c   |  63 ----
 6 files changed, 590 insertions(+), 194 deletions(-)

-- 
2.43.0

[PATCH v3 01/16] target/riscv/cpu_cfg.h: remove unused fields

[Daniel Henrique Barboza]

user_spec, bext_spec and bext_ver aren't being used.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.h     | 1 -
 target/riscv/cpu_cfg.h | 2 --
 2 files changed, 3 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index d74b361be6..40c96a32cc 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -164,7 +164,6 @@ struct CPUArchState {
     target_ulong guest_phys_fault_addr;
 
     target_ulong priv_ver;
-    target_ulong bext_ver;
     target_ulong vext_ver;
 
     /* RISCVMXL, but uint32_t for vmstate migration */
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index f4605fb190..c67a8731d3 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -136,8 +136,6 @@ struct RISCVCPUConfig {
 
     uint32_t pmu_mask;
     char *priv_spec;
-    char *user_spec;
-    char *bext_spec;
     char *vext_spec;
     uint16_t vlen;
     uint16_t elen;
-- 
2.43.0

Re: [PATCH v3 01/16] target/riscv/cpu_cfg.h: remove unused fields

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:46 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> user_spec, bext_spec and bext_ver aren't being used.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.h     | 1 -
>  target/riscv/cpu_cfg.h | 2 --
>  2 files changed, 3 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index d74b361be6..40c96a32cc 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -164,7 +164,6 @@ struct CPUArchState {
>      target_ulong guest_phys_fault_addr;
>
>      target_ulong priv_ver;
> -    target_ulong bext_ver;
>      target_ulong vext_ver;
>
>      /* RISCVMXL, but uint32_t for vmstate migration */
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index f4605fb190..c67a8731d3 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -136,8 +136,6 @@ struct RISCVCPUConfig {
>
>      uint32_t pmu_mask;
>      char *priv_spec;
> -    char *user_spec;
> -    char *bext_spec;
>      char *vext_spec;
>      uint16_t vlen;
>      uint16_t elen;
> --
> 2.43.0
>
>

[PATCH v3 02/16] target/riscv: make riscv_cpu_is_generic() public

[Daniel Henrique Barboza]

We'll use this function in target/riscv/cpu.c to implement setters that
won't allow vendor CPU options to be changed.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 70bf10aa7c..65cfa6c740 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -183,6 +183,11 @@ void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en)
     *ext_enabled = en;
 }
 
+bool riscv_cpu_is_generic(Object *cpu_obj)
+{
+    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
+}
+
 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",
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 40c96a32cc..bf69cb9a27 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -756,6 +756,7 @@ enum riscv_pmu_event_idx {
 void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en);
 bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset);
 void riscv_cpu_set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
+bool riscv_cpu_is_generic(Object *cpu_obj);
 
 typedef struct RISCVCPUMultiExtConfig {
     const char *name;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 8a35683a34..a09300e908 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -658,11 +658,6 @@ bool riscv_cpu_tcg_compatible(RISCVCPU *cpu)
     return object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST) == NULL;
 }
 
-static bool riscv_cpu_is_generic(Object *cpu_obj)
-{
-    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
-}
-
 /*
  * We'll get here via the following path:
  *
-- 
2.43.0

Re: [PATCH v3 02/16] target/riscv: make riscv_cpu_is_generic() public

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:42 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We'll use this function in target/riscv/cpu.c to implement setters that
> won't allow vendor CPU options to be changed.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 5 +++++
>  target/riscv/cpu.h         | 1 +
>  target/riscv/tcg/tcg-cpu.c | 5 -----
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 70bf10aa7c..65cfa6c740 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -183,6 +183,11 @@ void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en)
>      *ext_enabled = en;
>  }
>
> +bool riscv_cpu_is_generic(Object *cpu_obj)
> +{
> +    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
> +}
> +
>  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",
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 40c96a32cc..bf69cb9a27 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -756,6 +756,7 @@ enum riscv_pmu_event_idx {
>  void isa_ext_update_enabled(RISCVCPU *cpu, uint32_t ext_offset, bool en);
>  bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset);
>  void riscv_cpu_set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
> +bool riscv_cpu_is_generic(Object *cpu_obj);
>
>  typedef struct RISCVCPUMultiExtConfig {
>      const char *name;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8a35683a34..a09300e908 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -658,11 +658,6 @@ bool riscv_cpu_tcg_compatible(RISCVCPU *cpu)
>      return object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST) == NULL;
>  }
>
> -static bool riscv_cpu_is_generic(Object *cpu_obj)
> -{
> -    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
> -}
> -
>  /*
>   * We'll get here via the following path:
>   *
> --
> 2.43.0
>
>

[PATCH v3 03/16] target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Every property in riscv_cpu_options[] will be migrated to
riscv_cpu_properties[]. This will make their default values init
earlier, allowing cpu_init() functions to overwrite them. We'll also
implement common getters and setters that both accelerators will use,
allowing them to share validations that TCG is doing.

At the same time, some options (namely 'vlen', 'elen' and the cache
blocksizes) need a way of tracking if the user set a value for them.
This is benign for TCG since the cost of always validating these values
are small, but for KVM we need syscalls to read the host values to make
the validations, thus knowing whether the user didn't touch the values
makes a difference.

We'll track user setting for these properties using a hash, like we do
in the TCG driver. All riscv cpu options will update this hash in case
the user sets it. The KVM driver will use this hash to minimize the
amount of syscalls done.

For now, both 'pmu-mask' and 'pmu-num' shouldn't be changed for vendor
CPUs. The existing setter for 'pmu-num' is changed to add this
restriction. New getters and setters are required for 'pmu-mask'

While we're at it, add a 'static' modifier to 'prop_pmu_num' since we're
not exporting it.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 65cfa6c740..e90b70c0a7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -53,6 +53,15 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
 #define BYTE(x)   (x)
 #endif
 
+/* Hash that stores general user set numeric options */
+static GHashTable *general_user_opts;
+
+static void cpu_option_add_user_setting(const char *optname, uint32_t value)
+{
+    g_hash_table_insert(general_user_opts, (gpointer)optname,
+                        GUINT_TO_POINTER(value));
+}
+
 #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
     {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
 
@@ -1218,11 +1227,15 @@ static void riscv_cpu_post_init(Object *obj)
 
 static void riscv_cpu_init(Object *obj)
 {
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
 #ifndef CONFIG_USER_ONLY
     qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
                       IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
 #endif /* CONFIG_USER_ONLY */
 
+    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
+
     /*
      * The timer and performance counters extensions were supported
      * in QEMU before they were added as discrete extensions in the
@@ -1232,6 +1245,9 @@ static void riscv_cpu_init(Object *obj)
      */
     RISCV_CPU(obj)->cfg.ext_zicntr = true;
     RISCV_CPU(obj)->cfg.ext_zihpm = true;
+
+    /* Default values for non-bool cpu properties */
+    cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
 }
 
 typedef struct misa_ext_info {
@@ -1431,26 +1447,51 @@ const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static bool riscv_cpu_is_vendor(Object *obj)
+{
+    return !riscv_cpu_is_generic(obj);
+}
+
+static void cpu_set_prop_err(RISCVCPU *cpu, const char *propname,
+                             Error **errp)
+{
+    g_autofree char *cpuname = riscv_cpu_get_name(cpu);
+    error_setg(errp, "CPU '%s' does not allow changing the value of '%s'",
+               cpuname, propname);
+}
+
 static void prop_pmu_num_set(Object *obj, Visitor *v, const char *name,
                              void *opaque, Error **errp)
 {
     RISCVCPU *cpu = RISCV_CPU(obj);
-    uint8_t pmu_num;
+    uint8_t pmu_num, curr_pmu_num;
+    uint32_t pmu_mask;
 
     visit_type_uint8(v, name, &pmu_num, errp);
 
+    curr_pmu_num = ctpop32(cpu->cfg.pmu_mask);
+
+    if (pmu_num != curr_pmu_num && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %u\n",
+                          name, curr_pmu_num);
+        return;
+    }
+
     if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
         error_setg(errp, "Number of counters exceeds maximum available");
         return;
     }
 
     if (pmu_num == 0) {
-        cpu->cfg.pmu_mask = 0;
+        pmu_mask = 0;
     } else {
-        cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
+        pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
     }
 
     warn_report("\"pmu-num\" property is deprecated; use \"pmu-mask\"");
+    cpu->cfg.pmu_mask = pmu_mask;
+    cpu_option_add_user_setting("pmu-mask", pmu_mask);
 }
 
 static void prop_pmu_num_get(Object *obj, Visitor *v, const char *name,
@@ -1462,16 +1503,54 @@ static void prop_pmu_num_get(Object *obj, Visitor *v, const char *name,
     visit_type_uint8(v, name, &pmu_num, errp);
 }
 
-const PropertyInfo prop_pmu_num = {
+static const PropertyInfo prop_pmu_num = {
     .name = "pmu-num",
     .get = prop_pmu_num_get,
     .set = prop_pmu_num_set,
 };
 
-Property riscv_cpu_options[] = {
-    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, MAKE_64BIT_MASK(3, 16)),
-    {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
+static void prop_pmu_mask_set(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint32_t value;
+    uint8_t pmu_num;
+
+    visit_type_uint32(v, name, &value, errp);
+
+    if (value != cpu->cfg.pmu_mask && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %x\n",
+                          name, cpu->cfg.pmu_mask);
+        return;
+    }
+
+    pmu_num = ctpop32(value);
+
+    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
+        error_setg(errp, "Number of counters exceeds maximum available");
+        return;
+    }
 
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.pmu_mask = value;
+}
+
+static void prop_pmu_mask_get(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    uint8_t pmu_mask = RISCV_CPU(obj)->cfg.pmu_mask;
+
+    visit_type_uint8(v, name, &pmu_mask, errp);
+}
+
+static const PropertyInfo prop_pmu_mask = {
+    .name = "pmu-mask",
+    .get = prop_pmu_mask_get,
+    .set = prop_pmu_mask_set,
+};
+
+Property riscv_cpu_options[] = {
     DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
     DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
 
@@ -1490,6 +1569,9 @@ Property riscv_cpu_options[] = {
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
+    {.name = "pmu-mask", .info = &prop_pmu_mask},
+    {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
+
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
-- 
2.43.0

Re: [PATCH v3 03/16] target/riscv: move 'pmu-mask' and 'pmu-num' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 5:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Every property in riscv_cpu_options[] will be migrated to
> riscv_cpu_properties[]. This will make their default values init
> earlier, allowing cpu_init() functions to overwrite them. We'll also
> implement common getters and setters that both accelerators will use,
> allowing them to share validations that TCG is doing.
>
> At the same time, some options (namely 'vlen', 'elen' and the cache
> blocksizes) need a way of tracking if the user set a value for them.
> This is benign for TCG since the cost of always validating these values
> are small, but for KVM we need syscalls to read the host values to make
> the validations, thus knowing whether the user didn't touch the values
> makes a difference.
>
> We'll track user setting for these properties using a hash, like we do
> in the TCG driver. All riscv cpu options will update this hash in case
> the user sets it. The KVM driver will use this hash to minimize the
> amount of syscalls done.
>
> For now, both 'pmu-mask' and 'pmu-num' shouldn't be changed for vendor
> CPUs. The existing setter for 'pmu-num' is changed to add this
> restriction. New getters and setters are required for 'pmu-mask'
>
> While we're at it, add a 'static' modifier to 'prop_pmu_num' since we're
> not exporting it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 96 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 89 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 65cfa6c740..e90b70c0a7 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -53,6 +53,15 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
>  #define BYTE(x)   (x)
>  #endif
>
> +/* Hash that stores general user set numeric options */
> +static GHashTable *general_user_opts;
> +
> +static void cpu_option_add_user_setting(const char *optname, uint32_t value)
> +{
> +    g_hash_table_insert(general_user_opts, (gpointer)optname,
> +                        GUINT_TO_POINTER(value));
> +}
> +
>  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
>      {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
>
> @@ -1218,11 +1227,15 @@ static void riscv_cpu_post_init(Object *obj)
>
>  static void riscv_cpu_init(Object *obj)
>  {
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>  #ifndef CONFIG_USER_ONLY
>      qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
>                        IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
>  #endif /* CONFIG_USER_ONLY */
>
> +    general_user_opts = g_hash_table_new(g_str_hash, g_str_equal);
> +
>      /*
>       * The timer and performance counters extensions were supported
>       * in QEMU before they were added as discrete extensions in the
> @@ -1232,6 +1245,9 @@ static void riscv_cpu_init(Object *obj)
>       */
>      RISCV_CPU(obj)->cfg.ext_zicntr = true;
>      RISCV_CPU(obj)->cfg.ext_zihpm = true;
> +
> +    /* Default values for non-bool cpu properties */
> +    cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
>  }
>
>  typedef struct misa_ext_info {
> @@ -1431,26 +1447,51 @@ const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static bool riscv_cpu_is_vendor(Object *obj)
> +{
> +    return !riscv_cpu_is_generic(obj);
> +}
> +
> +static void cpu_set_prop_err(RISCVCPU *cpu, const char *propname,
> +                             Error **errp)
> +{
> +    g_autofree char *cpuname = riscv_cpu_get_name(cpu);
> +    error_setg(errp, "CPU '%s' does not allow changing the value of '%s'",
> +               cpuname, propname);
> +}
> +
>  static void prop_pmu_num_set(Object *obj, Visitor *v, const char *name,
>                               void *opaque, Error **errp)
>  {
>      RISCVCPU *cpu = RISCV_CPU(obj);
> -    uint8_t pmu_num;
> +    uint8_t pmu_num, curr_pmu_num;
> +    uint32_t pmu_mask;
>
>      visit_type_uint8(v, name, &pmu_num, errp);
>
> +    curr_pmu_num = ctpop32(cpu->cfg.pmu_mask);
> +
> +    if (pmu_num != curr_pmu_num && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %u\n",
> +                          name, curr_pmu_num);
> +        return;
> +    }
> +
>      if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
>          error_setg(errp, "Number of counters exceeds maximum available");
>          return;
>      }
>
>      if (pmu_num == 0) {
> -        cpu->cfg.pmu_mask = 0;
> +        pmu_mask = 0;
>      } else {
> -        cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
> +        pmu_mask = MAKE_64BIT_MASK(3, pmu_num);
>      }
>
>      warn_report("\"pmu-num\" property is deprecated; use \"pmu-mask\"");
> +    cpu->cfg.pmu_mask = pmu_mask;
> +    cpu_option_add_user_setting("pmu-mask", pmu_mask);
>  }
>
>  static void prop_pmu_num_get(Object *obj, Visitor *v, const char *name,
> @@ -1462,16 +1503,54 @@ static void prop_pmu_num_get(Object *obj, Visitor *v, const char *name,
>      visit_type_uint8(v, name, &pmu_num, errp);
>  }
>
> -const PropertyInfo prop_pmu_num = {
> +static const PropertyInfo prop_pmu_num = {
>      .name = "pmu-num",
>      .get = prop_pmu_num_get,
>      .set = prop_pmu_num_set,
>  };
>
> -Property riscv_cpu_options[] = {
> -    DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, MAKE_64BIT_MASK(3, 16)),
> -    {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
> +static void prop_pmu_mask_set(Object *obj, Visitor *v, const char *name,
> +                             void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint32_t value;
> +    uint8_t pmu_num;
> +
> +    visit_type_uint32(v, name, &value, errp);
> +
> +    if (value != cpu->cfg.pmu_mask && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %x\n",
> +                          name, cpu->cfg.pmu_mask);
> +        return;
> +    }
> +
> +    pmu_num = ctpop32(value);
> +
> +    if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> +        error_setg(errp, "Number of counters exceeds maximum available");
> +        return;
> +    }
>
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.pmu_mask = value;
> +}
> +
> +static void prop_pmu_mask_get(Object *obj, Visitor *v, const char *name,
> +                             void *opaque, Error **errp)
> +{
> +    uint8_t pmu_mask = RISCV_CPU(obj)->cfg.pmu_mask;
> +
> +    visit_type_uint8(v, name, &pmu_mask, errp);
> +}
> +
> +static const PropertyInfo prop_pmu_mask = {
> +    .name = "pmu-mask",
> +    .get = prop_pmu_mask_get,
> +    .set = prop_pmu_mask_set,
> +};
> +
> +Property riscv_cpu_options[] = {
>      DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
>      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
>
> @@ -1490,6 +1569,9 @@ Property riscv_cpu_options[] = {
>  static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>
> +    {.name = "pmu-mask", .info = &prop_pmu_mask},
> +    {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> --
> 2.43.0
>
>

[PATCH v3 04/16] target/riscv: move 'mmu' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Commit 7f0bdfb5bfc ("target/riscv/cpu.c: remove cfg setup from
riscv_cpu_init()") already did some of the work by making some
cpu_init() functions to explictly enable their own 'mmu' default.

The generic CPUs didn't get update by that commit, so they are still
relying on the defaults set by the 'mmu' option. But having 'mmu' and
'pmp' being default=true will force CPUs that doesn't implement these
options to set them to 'false' in their cpu_init(), which isn't ideal.

We'll move 'mmu' to riscv_cpu_properties[] without any defaults, i.e.
the default will be 'false'. Compensate it by manually setting 'mmu =
true' to the generic CPUs that requires it.

Implement a setter for it to forbid the 'mmu' setting to be changed for
vendor CPUs. This will allow the option to exist for all CPUs and, at
the same time, protect vendor CPUs from undesired changes:

$ ./build/qemu-system-riscv64 -M virt -cpu sifive-e51,mmu=true
qemu-system-riscv64: can't apply global sifive-e51-riscv-cpu.mmu=true:
   CPU 'sifive-e51' does not allow changing the value of 'mmu'

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e90b70c0a7..9f1407b73f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -419,6 +419,8 @@ static void riscv_max_cpu_init(Object *obj)
     CPURISCVState *env = &cpu->env;
     RISCVMXL mlx = MXL_RV64;
 
+    cpu->cfg.mmu = true;
+
 #ifdef TARGET_RISCV32
     mlx = MXL_RV32;
 #endif
@@ -433,7 +435,11 @@ static void riscv_max_cpu_init(Object *obj)
 #if defined(TARGET_RISCV64)
 static void rv64_base_cpu_init(Object *obj)
 {
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
+
+    cpu->cfg.mmu = true;
+
     /* We set this in the realise function */
     riscv_cpu_set_misa(env, MXL_RV64, 0);
     /* Set latest version of privileged specification */
@@ -551,13 +557,18 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
 
 static void rv128_base_cpu_init(Object *obj)
 {
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
+
     if (qemu_tcg_mttcg_enabled()) {
         /* Missing 128-bit aligned atomics */
         error_report("128-bit RISC-V currently does not work with Multi "
                      "Threaded TCG. Please use: -accel tcg,thread=single");
         exit(EXIT_FAILURE);
     }
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
+
+    cpu->cfg.mmu = true;
+
     /* We set this in the realise function */
     riscv_cpu_set_misa(env, MXL_RV128, 0);
     /* Set latest version of privileged specification */
@@ -569,7 +580,11 @@ static void rv128_base_cpu_init(Object *obj)
 #else
 static void rv32_base_cpu_init(Object *obj)
 {
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    CPURISCVState *env = &cpu->env;
+
+    cpu->cfg.mmu = true;
+
     /* We set this in the realise function */
     riscv_cpu_set_misa(env, MXL_RV32, 0);
     /* Set latest version of privileged specification */
@@ -1550,8 +1565,38 @@ static const PropertyInfo prop_pmu_mask = {
     .set = prop_pmu_mask_set,
 };
 
+static void prop_mmu_set(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool value;
+
+    visit_type_bool(v, name, &value, errp);
+
+    if (cpu->cfg.mmu != value && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, "mmu", errp);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.mmu = value;
+}
+
+static void prop_mmu_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    bool value = RISCV_CPU(obj)->cfg.mmu;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_mmu = {
+    .name = "mmu",
+    .get = prop_mmu_get,
+    .set = prop_mmu_set,
+};
+
 Property riscv_cpu_options[] = {
-    DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
     DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
 
     DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
@@ -1572,6 +1617,8 @@ static Property riscv_cpu_properties[] = {
     {.name = "pmu-mask", .info = &prop_pmu_mask},
     {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
 
+    {.name = "mmu", .info = &prop_mmu},
+
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
-- 
2.43.0

Re: [PATCH v3 04/16] target/riscv: move 'mmu' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:43 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Commit 7f0bdfb5bfc ("target/riscv/cpu.c: remove cfg setup from
> riscv_cpu_init()") already did some of the work by making some
> cpu_init() functions to explictly enable their own 'mmu' default.
>
> The generic CPUs didn't get update by that commit, so they are still
> relying on the defaults set by the 'mmu' option. But having 'mmu' and
> 'pmp' being default=true will force CPUs that doesn't implement these
> options to set them to 'false' in their cpu_init(), which isn't ideal.
>
> We'll move 'mmu' to riscv_cpu_properties[] without any defaults, i.e.
> the default will be 'false'. Compensate it by manually setting 'mmu =
> true' to the generic CPUs that requires it.
>
> Implement a setter for it to forbid the 'mmu' setting to be changed for
> vendor CPUs. This will allow the option to exist for all CPUs and, at
> the same time, protect vendor CPUs from undesired changes:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu sifive-e51,mmu=true
> qemu-system-riscv64: can't apply global sifive-e51-riscv-cpu.mmu=true:
>    CPU 'sifive-e51' does not allow changing the value of 'mmu'
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 55 ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e90b70c0a7..9f1407b73f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -419,6 +419,8 @@ static void riscv_max_cpu_init(Object *obj)
>      CPURISCVState *env = &cpu->env;
>      RISCVMXL mlx = MXL_RV64;
>
> +    cpu->cfg.mmu = true;
> +
>  #ifdef TARGET_RISCV32
>      mlx = MXL_RV32;
>  #endif
> @@ -433,7 +435,11 @@ static void riscv_max_cpu_init(Object *obj)
>  #if defined(TARGET_RISCV64)
>  static void rv64_base_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +
> +    cpu->cfg.mmu = true;
> +
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV64, 0);
>      /* Set latest version of privileged specification */
> @@ -551,13 +557,18 @@ static void rv64_veyron_v1_cpu_init(Object *obj)
>
>  static void rv128_base_cpu_init(Object *obj)
>  {
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +
>      if (qemu_tcg_mttcg_enabled()) {
>          /* Missing 128-bit aligned atomics */
>          error_report("128-bit RISC-V currently does not work with Multi "
>                       "Threaded TCG. Please use: -accel tcg,thread=single");
>          exit(EXIT_FAILURE);
>      }
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +
> +    cpu->cfg.mmu = true;
> +
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV128, 0);
>      /* Set latest version of privileged specification */
> @@ -569,7 +580,11 @@ static void rv128_base_cpu_init(Object *obj)
>  #else
>  static void rv32_base_cpu_init(Object *obj)
>  {
> -    CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    CPURISCVState *env = &cpu->env;
> +
> +    cpu->cfg.mmu = true;
> +
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV32, 0);
>      /* Set latest version of privileged specification */
> @@ -1550,8 +1565,38 @@ static const PropertyInfo prop_pmu_mask = {
>      .set = prop_pmu_mask_set,
>  };
>
> +static void prop_mmu_set(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    bool value;
> +
> +    visit_type_bool(v, name, &value, errp);
> +
> +    if (cpu->cfg.mmu != value && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, "mmu", errp);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.mmu = value;
> +}
> +
> +static void prop_mmu_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    bool value = RISCV_CPU(obj)->cfg.mmu;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_mmu = {
> +    .name = "mmu",
> +    .get = prop_mmu_get,
> +    .set = prop_mmu_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -    DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
>      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
>
>      DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
> @@ -1572,6 +1617,8 @@ static Property riscv_cpu_properties[] = {
>      {.name = "pmu-mask", .info = &prop_pmu_mask},
>      {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
>
> +    {.name = "mmu", .info = &prop_mmu},
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> --
> 2.43.0
>
>

[PATCH v3 05/16] target/riscv: move 'pmp' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Move 'pmp' to riscv_cpu_properties[], creating a new setter() for it
that forbids 'pmp' to be changed in vendor CPUs, like we did with the
'mmu' option.

We'll also have to manually set 'pmp = true' to generic CPUs that were
still relying on the previous default to set it.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9f1407b73f..01b3b57cee 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -420,6 +420,7 @@ static void riscv_max_cpu_init(Object *obj)
     RISCVMXL mlx = MXL_RV64;
 
     cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 
 #ifdef TARGET_RISCV32
     mlx = MXL_RV32;
@@ -439,6 +440,7 @@ static void rv64_base_cpu_init(Object *obj)
     CPURISCVState *env = &cpu->env;
 
     cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 
     /* We set this in the realise function */
     riscv_cpu_set_misa(env, MXL_RV64, 0);
@@ -568,6 +570,7 @@ static void rv128_base_cpu_init(Object *obj)
     }
 
     cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 
     /* We set this in the realise function */
     riscv_cpu_set_misa(env, MXL_RV128, 0);
@@ -584,6 +587,7 @@ static void rv32_base_cpu_init(Object *obj)
     CPURISCVState *env = &cpu->env;
 
     cpu->cfg.mmu = true;
+    cpu->cfg.pmp = true;
 
     /* We set this in the realise function */
     riscv_cpu_set_misa(env, MXL_RV32, 0);
@@ -1596,9 +1600,38 @@ static const PropertyInfo prop_mmu = {
     .set = prop_mmu_set,
 };
 
-Property riscv_cpu_options[] = {
-    DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
+static void prop_pmp_set(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool value;
+
+    visit_type_bool(v, name, &value, errp);
 
+    if (cpu->cfg.pmp != value && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.pmp = value;
+}
+
+static void prop_pmp_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    bool value = RISCV_CPU(obj)->cfg.pmp;
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_pmp = {
+    .name = "pmp",
+    .get = prop_pmp_get,
+    .set = prop_pmp_set,
+};
+
+Property riscv_cpu_options[] = {
     DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
     DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
 
@@ -1618,6 +1651,7 @@ static Property riscv_cpu_properties[] = {
     {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
 
     {.name = "mmu", .info = &prop_mmu},
+    {.name = "pmp", .info = &prop_pmp},
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
-- 
2.43.0

Re: [PATCH v3 05/16] target/riscv: move 'pmp' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 4:53 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Move 'pmp' to riscv_cpu_properties[], creating a new setter() for it
> that forbids 'pmp' to be changed in vendor CPUs, like we did with the
> 'mmu' option.
>
> We'll also have to manually set 'pmp = true' to generic CPUs that were
> still relying on the previous default to set it.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 9f1407b73f..01b3b57cee 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -420,6 +420,7 @@ static void riscv_max_cpu_init(Object *obj)
>      RISCVMXL mlx = MXL_RV64;
>
>      cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>
>  #ifdef TARGET_RISCV32
>      mlx = MXL_RV32;
> @@ -439,6 +440,7 @@ static void rv64_base_cpu_init(Object *obj)
>      CPURISCVState *env = &cpu->env;
>
>      cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV64, 0);
> @@ -568,6 +570,7 @@ static void rv128_base_cpu_init(Object *obj)
>      }
>
>      cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV128, 0);
> @@ -584,6 +587,7 @@ static void rv32_base_cpu_init(Object *obj)
>      CPURISCVState *env = &cpu->env;
>
>      cpu->cfg.mmu = true;
> +    cpu->cfg.pmp = true;
>
>      /* We set this in the realise function */
>      riscv_cpu_set_misa(env, MXL_RV32, 0);
> @@ -1596,9 +1600,38 @@ static const PropertyInfo prop_mmu = {
>      .set = prop_mmu_set,
>  };
>
> -Property riscv_cpu_options[] = {
> -    DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> +static void prop_pmp_set(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    bool value;
> +
> +    visit_type_bool(v, name, &value, errp);
>
> +    if (cpu->cfg.pmp != value && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.pmp = value;
> +}
> +
> +static void prop_pmp_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    bool value = RISCV_CPU(obj)->cfg.pmp;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_pmp = {
> +    .name = "pmp",
> +    .get = prop_pmp_get,
> +    .set = prop_pmp_set,
> +};
> +
> +Property riscv_cpu_options[] = {
>      DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
>      DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
>
> @@ -1618,6 +1651,7 @@ static Property riscv_cpu_properties[] = {
>      {.name = "pmu-num", .info = &prop_pmu_num}, /* Deprecated */
>
>      {.name = "mmu", .info = &prop_mmu},
> +    {.name = "pmp", .info = &prop_pmp},
>
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> --
> 2.43.0
>
>

[PATCH v3 06/16] target/riscv: rework 'priv_spec'

[Daniel Henrique Barboza]

'priv_spec' and 'vext_spec' are two string options used as a fancy way
of setting integers in the CPU state (cpu->env.priv_ver and
cpu->env.vext_ver). It requires us to deal with string parsing and to
store them in cpu_cfg.

We must support these string options, but we don't need to store them.
We have a precedence for this kind of arrangement in target/ppc/compat.c,
ppc_compat_prop_get|set, getters and setters used for the
'max-cpu-compat' class property of the pseries ppc64 machine. We'll do
the same with both 'priv_spec' and 'vext_spec'.

For 'priv_spec', the validation from riscv_cpu_validate_priv_spec() will
be done by the prop_priv_spec_set() setter, while also preventing it to
be changed for vendor CPUs. Add two helpers that converts env->priv_ver
back and forth to its string representation. These helpers allow us to
get a string and set 'env->priv_ver' and return a string giving the
current env->priv_ver value. In other words, make the cpu->cfg.priv_spec
string obsolete.

Last but not the least, move the reworked 'priv_spec' option to
riscv_cpu_properties[].

After all said and done, we don't need to store the 'priv_spec' string in
the CPU state, and we're now protecting vendor CPUs from priv_ver
changes:

$ ./build/qemu-system-riscv64 -M virt -cpu sifive-e51,priv_spec="v1.12.0"
qemu-system-riscv64: can't apply global sifive-e51-riscv-cpu.priv_spec=v1.12.0:
    CPU 'sifive-e51' does not allow changing the value of 'priv_spec'
Current 'priv_spec' val: v1.10.0
$

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 73 +++++++++++++++++++++++++++++++++++++-
 target/riscv/cpu.h         |  3 ++
 target/riscv/cpu_cfg.h     |  1 -
 target/riscv/tcg/tcg-cpu.c | 29 ---------------
 4 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 01b3b57cee..657569d8a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1631,8 +1631,77 @@ static const PropertyInfo prop_pmp = {
     .set = prop_pmp_set,
 };
 
+static int priv_spec_from_str(const char *priv_spec_str)
+{
+    int priv_version = -1;
+
+    if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
+        priv_version = PRIV_VERSION_1_12_0;
+    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
+        priv_version = PRIV_VERSION_1_11_0;
+    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_10_0_STR)) {
+        priv_version = PRIV_VERSION_1_10_0;
+    }
+
+    return priv_version;
+}
+
+static const char *priv_spec_to_str(int priv_version)
+{
+    switch (priv_version) {
+    case PRIV_VERSION_1_10_0:
+        return PRIV_VER_1_10_0_STR;
+    case PRIV_VERSION_1_11_0:
+        return PRIV_VER_1_11_0_STR;
+    case PRIV_VERSION_1_12_0:
+        return PRIV_VER_1_12_0_STR;
+    default:
+        return NULL;
+    }
+}
+
+static void prop_priv_spec_set(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    g_autofree char *value = NULL;
+    int priv_version = -1;
+
+    visit_type_str(v, name, &value, errp);
+
+    priv_version = priv_spec_from_str(value);
+    if (priv_version < 0) {
+        error_setg(errp, "Unsupported privilege spec version '%s'", value);
+        return;
+    }
+
+    if (priv_version != cpu->env.priv_ver && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %s\n", name,
+                          object_property_get_str(obj, name, NULL));
+        return;
+    }
+
+    cpu_option_add_user_setting(name, priv_version);
+    cpu->env.priv_ver = priv_version;
+}
+
+static void prop_priv_spec_get(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    const char *value = priv_spec_to_str(cpu->env.priv_ver);
+
+    visit_type_str(v, name, (char **)&value, errp);
+}
+
+static const PropertyInfo prop_priv_spec = {
+    .name = "priv_spec",
+    .get = prop_priv_spec_get,
+    .set = prop_priv_spec_set,
+};
+
 Property riscv_cpu_options[] = {
-    DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
     DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
 
     DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
@@ -1653,6 +1722,8 @@ static Property riscv_cpu_properties[] = {
     {.name = "mmu", .info = &prop_mmu},
     {.name = "pmp", .info = &prop_pmp},
 
+    {.name = "priv_spec", .info = &prop_priv_spec},
+
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index bf69cb9a27..aa3d3372e3 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -77,6 +77,9 @@ const char *riscv_get_misa_ext_description(uint32_t bit);
 #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
 
 /* Privileged specification version */
+#define PRIV_VER_1_10_0_STR "v1.10.0"
+#define PRIV_VER_1_11_0_STR "v1.11.0"
+#define PRIV_VER_1_12_0_STR "v1.12.0"
 enum {
     PRIV_VERSION_1_10_0 = 0,
     PRIV_VERSION_1_11_0,
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index c67a8731d3..2dba1f0007 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -135,7 +135,6 @@ struct RISCVCPUConfig {
     bool ext_XVentanaCondOps;
 
     uint32_t pmu_mask;
-    char *priv_spec;
     char *vext_spec;
     uint16_t vlen;
     uint16_t elen;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index a09300e908..4d67b72d9e 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -175,29 +175,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
     }
 }
 
-static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
-{
-    CPURISCVState *env = &cpu->env;
-    int priv_version = -1;
-
-    if (cpu->cfg.priv_spec) {
-        if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
-            priv_version = PRIV_VERSION_1_12_0;
-        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
-            priv_version = PRIV_VERSION_1_11_0;
-        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
-            priv_version = PRIV_VERSION_1_10_0;
-        } else {
-            error_setg(errp,
-                       "Unsupported privilege spec version '%s'",
-                       cpu->cfg.priv_spec);
-            return;
-        }
-
-        env->priv_ver = priv_version;
-    }
-}
-
 static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
                                  Error **errp)
 {
@@ -625,12 +602,6 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
     CPURISCVState *env = &cpu->env;
     Error *local_err = NULL;
 
-    riscv_cpu_validate_priv_spec(cpu, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     riscv_cpu_validate_misa_priv(env, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
-- 
2.43.0

Re: [PATCH v3 06/16] target/riscv: rework 'priv_spec'

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:41 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 'priv_spec' and 'vext_spec' are two string options used as a fancy way
> of setting integers in the CPU state (cpu->env.priv_ver and
> cpu->env.vext_ver). It requires us to deal with string parsing and to
> store them in cpu_cfg.
>
> We must support these string options, but we don't need to store them.
> We have a precedence for this kind of arrangement in target/ppc/compat.c,
> ppc_compat_prop_get|set, getters and setters used for the
> 'max-cpu-compat' class property of the pseries ppc64 machine. We'll do
> the same with both 'priv_spec' and 'vext_spec'.
>
> For 'priv_spec', the validation from riscv_cpu_validate_priv_spec() will
> be done by the prop_priv_spec_set() setter, while also preventing it to
> be changed for vendor CPUs. Add two helpers that converts env->priv_ver
> back and forth to its string representation. These helpers allow us to
> get a string and set 'env->priv_ver' and return a string giving the
> current env->priv_ver value. In other words, make the cpu->cfg.priv_spec
> string obsolete.
>
> Last but not the least, move the reworked 'priv_spec' option to
> riscv_cpu_properties[].
>
> After all said and done, we don't need to store the 'priv_spec' string in
> the CPU state, and we're now protecting vendor CPUs from priv_ver
> changes:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu sifive-e51,priv_spec="v1.12.0"
> qemu-system-riscv64: can't apply global sifive-e51-riscv-cpu.priv_spec=v1.12.0:
>     CPU 'sifive-e51' does not allow changing the value of 'priv_spec'
> Current 'priv_spec' val: v1.10.0
> $
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 73 +++++++++++++++++++++++++++++++++++++-
>  target/riscv/cpu.h         |  3 ++
>  target/riscv/cpu_cfg.h     |  1 -
>  target/riscv/tcg/tcg-cpu.c | 29 ---------------
>  4 files changed, 75 insertions(+), 31 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 01b3b57cee..657569d8a6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1631,8 +1631,77 @@ static const PropertyInfo prop_pmp = {
>      .set = prop_pmp_set,
>  };
>
> +static int priv_spec_from_str(const char *priv_spec_str)
> +{
> +    int priv_version = -1;
> +
> +    if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> +        priv_version = PRIV_VERSION_1_12_0;
> +    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
> +        priv_version = PRIV_VERSION_1_11_0;
> +    } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_10_0_STR)) {
> +        priv_version = PRIV_VERSION_1_10_0;
> +    }
> +
> +    return priv_version;
> +}
> +
> +static const char *priv_spec_to_str(int priv_version)
> +{
> +    switch (priv_version) {
> +    case PRIV_VERSION_1_10_0:
> +        return PRIV_VER_1_10_0_STR;
> +    case PRIV_VERSION_1_11_0:
> +        return PRIV_VER_1_11_0_STR;
> +    case PRIV_VERSION_1_12_0:
> +        return PRIV_VER_1_12_0_STR;
> +    default:
> +        return NULL;
> +    }
> +}
> +
> +static void prop_priv_spec_set(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    g_autofree char *value = NULL;
> +    int priv_version = -1;
> +
> +    visit_type_str(v, name, &value, errp);
> +
> +    priv_version = priv_spec_from_str(value);
> +    if (priv_version < 0) {
> +        error_setg(errp, "Unsupported privilege spec version '%s'", value);
> +        return;
> +    }
> +
> +    if (priv_version != cpu->env.priv_ver && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %s\n", name,
> +                          object_property_get_str(obj, name, NULL));
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, priv_version);
> +    cpu->env.priv_ver = priv_version;
> +}
> +
> +static void prop_priv_spec_get(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    const char *value = priv_spec_to_str(cpu->env.priv_ver);
> +
> +    visit_type_str(v, name, (char **)&value, errp);
> +}
> +
> +static const PropertyInfo prop_priv_spec = {
> +    .name = "priv_spec",
> +    .get = prop_priv_spec_get,
> +    .set = prop_priv_spec_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -    DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
>      DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
>
>      DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
> @@ -1653,6 +1722,8 @@ static Property riscv_cpu_properties[] = {
>      {.name = "mmu", .info = &prop_mmu},
>      {.name = "pmp", .info = &prop_pmp},
>
> +    {.name = "priv_spec", .info = &prop_priv_spec},
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index bf69cb9a27..aa3d3372e3 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -77,6 +77,9 @@ const char *riscv_get_misa_ext_description(uint32_t bit);
>  #define CPU_CFG_OFFSET(_prop) offsetof(struct RISCVCPUConfig, _prop)
>
>  /* Privileged specification version */
> +#define PRIV_VER_1_10_0_STR "v1.10.0"
> +#define PRIV_VER_1_11_0_STR "v1.11.0"
> +#define PRIV_VER_1_12_0_STR "v1.12.0"
>  enum {
>      PRIV_VERSION_1_10_0 = 0,
>      PRIV_VERSION_1_11_0,
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index c67a8731d3..2dba1f0007 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -135,7 +135,6 @@ struct RISCVCPUConfig {
>      bool ext_XVentanaCondOps;
>
>      uint32_t pmu_mask;
> -    char *priv_spec;
>      char *vext_spec;
>      uint16_t vlen;
>      uint16_t elen;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index a09300e908..4d67b72d9e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -175,29 +175,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>      }
>  }
>
> -static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
> -{
> -    CPURISCVState *env = &cpu->env;
> -    int priv_version = -1;
> -
> -    if (cpu->cfg.priv_spec) {
> -        if (!g_strcmp0(cpu->cfg.priv_spec, "v1.12.0")) {
> -            priv_version = PRIV_VERSION_1_12_0;
> -        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.11.0")) {
> -            priv_version = PRIV_VERSION_1_11_0;
> -        } else if (!g_strcmp0(cpu->cfg.priv_spec, "v1.10.0")) {
> -            priv_version = PRIV_VERSION_1_10_0;
> -        } else {
> -            error_setg(errp,
> -                       "Unsupported privilege spec version '%s'",
> -                       cpu->cfg.priv_spec);
> -            return;
> -        }
> -
> -        env->priv_ver = priv_version;
> -    }
> -}
> -
>  static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
>                                   Error **errp)
>  {
> @@ -625,12 +602,6 @@ void riscv_tcg_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>      CPURISCVState *env = &cpu->env;
>      Error *local_err = NULL;
>
> -    riscv_cpu_validate_priv_spec(cpu, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
>      riscv_cpu_validate_misa_priv(env, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> --
> 2.43.0
>
>

[PATCH v3 07/16] target/riscv: rework 'vext_spec'

[Daniel Henrique Barboza]

The same rework did in 'priv_spec' is done for 'vext_spec'. This time is
simpler, since we only accept one value ("v1.0") and we'll always have
env->vext_ver set to VEXT_VERSION_1_00_0, thus we don't need helpers to
convert string to 'vext_ver' back and forth like we needed for
'priv_spec'.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 35 +++++++++++++++++++++++++++++++++--
 target/riscv/cpu.h         |  1 +
 target/riscv/cpu_cfg.h     |  1 -
 target/riscv/tcg/tcg-cpu.c | 15 ---------------
 4 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 657569d8a6..c39da44f51 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1267,6 +1267,7 @@ static void riscv_cpu_init(Object *obj)
 
     /* Default values for non-bool cpu properties */
     cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
+    cpu->env.vext_ver = VEXT_VERSION_1_00_0;
 }
 
 typedef struct misa_ext_info {
@@ -1701,9 +1702,38 @@ static const PropertyInfo prop_priv_spec = {
     .set = prop_priv_spec_set,
 };
 
-Property riscv_cpu_options[] = {
-    DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
+static void prop_vext_spec_set(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    g_autofree char *value = NULL;
 
+    visit_type_str(v, name, &value, errp);
+
+    if (g_strcmp0(value, VEXT_VER_1_00_0_STR) != 0) {
+        error_setg(errp, "Unsupported vector spec version '%s'", value);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, VEXT_VERSION_1_00_0);
+    cpu->env.vext_ver = VEXT_VERSION_1_00_0;
+}
+
+static void prop_vext_spec_get(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    const char *value = VEXT_VER_1_00_0_STR;
+
+    visit_type_str(v, name, (char **)&value, errp);
+}
+
+static const PropertyInfo prop_vext_spec = {
+    .name = "vext_spec",
+    .get = prop_vext_spec_get,
+    .set = prop_vext_spec_set,
+};
+
+Property riscv_cpu_options[] = {
     DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
@@ -1723,6 +1753,7 @@ static Property riscv_cpu_properties[] = {
     {.name = "pmp", .info = &prop_pmp},
 
     {.name = "priv_spec", .info = &prop_priv_spec},
+    {.name = "vext_spec", .info = &prop_vext_spec},
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index aa3d3372e3..f1715d9d31 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -89,6 +89,7 @@ enum {
 };
 
 #define VEXT_VERSION_1_00_0 0x00010000
+#define VEXT_VER_1_00_0_STR "v1.0"
 
 enum {
     TRANSLATE_SUCCESS,
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index 2dba1f0007..7112af6c4c 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -135,7 +135,6 @@ struct RISCVCPUConfig {
     bool ext_XVentanaCondOps;
 
     uint32_t pmu_mask;
-    char *vext_spec;
     uint16_t vlen;
     uint16_t elen;
     uint16_t cbom_blocksize;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 4d67b72d9e..6501c29d8e 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -201,21 +201,6 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
                    "in the range [8, 64]");
         return;
     }
-
-    if (cfg->vext_spec) {
-        if (!g_strcmp0(cfg->vext_spec, "v1.0")) {
-            env->vext_ver = VEXT_VERSION_1_00_0;
-        } else {
-            error_setg(errp, "Unsupported vector spec version '%s'",
-                       cfg->vext_spec);
-            return;
-        }
-    } else if (env->vext_ver == 0) {
-        qemu_log("vector version is not specified, "
-                 "use the default value v1.0\n");
-
-        env->vext_ver = VEXT_VERSION_1_00_0;
-    }
 }
 
 static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
-- 
2.43.0

Re: [PATCH v3 07/16] target/riscv: rework 'vext_spec'

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:47 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The same rework did in 'priv_spec' is done for 'vext_spec'. This time is
> simpler, since we only accept one value ("v1.0") and we'll always have
> env->vext_ver set to VEXT_VERSION_1_00_0, thus we don't need helpers to
> convert string to 'vext_ver' back and forth like we needed for
> 'priv_spec'.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 35 +++++++++++++++++++++++++++++++++--
>  target/riscv/cpu.h         |  1 +
>  target/riscv/cpu_cfg.h     |  1 -
>  target/riscv/tcg/tcg-cpu.c | 15 ---------------
>  4 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 657569d8a6..c39da44f51 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1267,6 +1267,7 @@ static void riscv_cpu_init(Object *obj)
>
>      /* Default values for non-bool cpu properties */
>      cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
> +    cpu->env.vext_ver = VEXT_VERSION_1_00_0;
>  }
>
>  typedef struct misa_ext_info {
> @@ -1701,9 +1702,38 @@ static const PropertyInfo prop_priv_spec = {
>      .set = prop_priv_spec_set,
>  };
>
> -Property riscv_cpu_options[] = {
> -    DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
> +static void prop_vext_spec_set(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    g_autofree char *value = NULL;
>
> +    visit_type_str(v, name, &value, errp);
> +
> +    if (g_strcmp0(value, VEXT_VER_1_00_0_STR) != 0) {
> +        error_setg(errp, "Unsupported vector spec version '%s'", value);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, VEXT_VERSION_1_00_0);
> +    cpu->env.vext_ver = VEXT_VERSION_1_00_0;
> +}
> +
> +static void prop_vext_spec_get(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    const char *value = VEXT_VER_1_00_0_STR;
> +
> +    visit_type_str(v, name, (char **)&value, errp);
> +}
> +
> +static const PropertyInfo prop_vext_spec = {
> +    .name = "vext_spec",
> +    .get = prop_vext_spec_get,
> +    .set = prop_vext_spec_set,
> +};
> +
> +Property riscv_cpu_options[] = {
>      DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
>      DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>
> @@ -1723,6 +1753,7 @@ static Property riscv_cpu_properties[] = {
>      {.name = "pmp", .info = &prop_pmp},
>
>      {.name = "priv_spec", .info = &prop_priv_spec},
> +    {.name = "vext_spec", .info = &prop_vext_spec},
>
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index aa3d3372e3..f1715d9d31 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -89,6 +89,7 @@ enum {
>  };
>
>  #define VEXT_VERSION_1_00_0 0x00010000
> +#define VEXT_VER_1_00_0_STR "v1.0"
>
>  enum {
>      TRANSLATE_SUCCESS,
> diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> index 2dba1f0007..7112af6c4c 100644
> --- a/target/riscv/cpu_cfg.h
> +++ b/target/riscv/cpu_cfg.h
> @@ -135,7 +135,6 @@ struct RISCVCPUConfig {
>      bool ext_XVentanaCondOps;
>
>      uint32_t pmu_mask;
> -    char *vext_spec;
>      uint16_t vlen;
>      uint16_t elen;
>      uint16_t cbom_blocksize;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 4d67b72d9e..6501c29d8e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -201,21 +201,6 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
>                     "in the range [8, 64]");
>          return;
>      }
> -
> -    if (cfg->vext_spec) {
> -        if (!g_strcmp0(cfg->vext_spec, "v1.0")) {
> -            env->vext_ver = VEXT_VERSION_1_00_0;
> -        } else {
> -            error_setg(errp, "Unsupported vector spec version '%s'",
> -                       cfg->vext_spec);
> -            return;
> -        }
> -    } else if (env->vext_ver == 0) {
> -        qemu_log("vector version is not specified, "
> -                 "use the default value v1.0\n");
> -
> -        env->vext_ver = VEXT_VERSION_1_00_0;
> -    }
>  }
>
>  static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
> --
> 2.43.0
>
>

[PATCH v3 08/16] target/riscv: move 'vlen' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Turning 'vlen' into a class property will allow its default value to be
overwritten by cpu_init() later on, solving the issue we have now where
CPU specific settings are getting overwritten by the default.

Common validation bits are moved from riscv_cpu_validate_v() to
prop_vlen_set() to be shared with KVM.

And, as done with every option we migrated to riscv_cpu_properties[],
vendor CPUs can't have their 'vlen' value changed.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 45 +++++++++++++++++++++++++++++++++++++-
 target/riscv/tcg/tcg-cpu.c |  5 -----
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c39da44f51..557874a017 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -29,6 +29,7 @@
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
 #include "hw/qdev-properties.h"
+#include "hw/core/qdev-prop-internal.h"
 #include "migration/vmstate.h"
 #include "fpu/softfloat-helpers.h"
 #include "sysemu/kvm.h"
@@ -1267,6 +1268,7 @@ static void riscv_cpu_init(Object *obj)
 
     /* Default values for non-bool cpu properties */
     cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
+    cpu->cfg.vlen = 128;
     cpu->env.vext_ver = VEXT_VERSION_1_00_0;
 }
 
@@ -1733,8 +1735,47 @@ static const PropertyInfo prop_vext_spec = {
     .set = prop_vext_spec_set,
 };
 
+static void prop_vlen_set(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint16_t value;
+
+    if (!visit_type_uint16(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!is_power_of_2(value)) {
+        error_setg(errp, "Vector extension VLEN must be power of 2");
+        return;
+    }
+
+    if (value != cpu->cfg.vlen && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %u\n",
+                          name, cpu->cfg.vlen);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.vlen = value;
+}
+
+static void prop_vlen_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    uint16_t value = RISCV_CPU(obj)->cfg.vlen;
+
+    visit_type_uint16(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_vlen = {
+    .name = "vlen",
+    .get = prop_vlen_get,
+    .set = prop_vlen_set,
+};
+
 Property riscv_cpu_options[] = {
-    DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
     DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
 
     DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
@@ -1755,6 +1796,8 @@ static Property riscv_cpu_properties[] = {
     {.name = "priv_spec", .info = &prop_priv_spec},
     {.name = "vext_spec", .info = &prop_vext_spec},
 
+    {.name = "vlen", .info = &prop_vlen},
+
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 6501c29d8e..8ec858e096 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -178,11 +178,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
 static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
                                  Error **errp)
 {
-    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 "
-- 
2.43.0

Re: [PATCH v3 08/16] target/riscv: move 'vlen' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:48 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Turning 'vlen' into a class property will allow its default value to be
> overwritten by cpu_init() later on, solving the issue we have now where
> CPU specific settings are getting overwritten by the default.
>
> Common validation bits are moved from riscv_cpu_validate_v() to
> prop_vlen_set() to be shared with KVM.
>
> And, as done with every option we migrated to riscv_cpu_properties[],
> vendor CPUs can't have their 'vlen' value changed.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 45 +++++++++++++++++++++++++++++++++++++-
>  target/riscv/tcg/tcg-cpu.c |  5 -----
>  2 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index c39da44f51..557874a017 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -29,6 +29,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/qdev-properties.h"
> +#include "hw/core/qdev-prop-internal.h"
>  #include "migration/vmstate.h"
>  #include "fpu/softfloat-helpers.h"
>  #include "sysemu/kvm.h"
> @@ -1267,6 +1268,7 @@ static void riscv_cpu_init(Object *obj)
>
>      /* Default values for non-bool cpu properties */
>      cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
> +    cpu->cfg.vlen = 128;
>      cpu->env.vext_ver = VEXT_VERSION_1_00_0;
>  }
>
> @@ -1733,8 +1735,47 @@ static const PropertyInfo prop_vext_spec = {
>      .set = prop_vext_spec_set,
>  };
>
> +static void prop_vlen_set(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint16_t value;
> +
> +    if (!visit_type_uint16(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (!is_power_of_2(value)) {
> +        error_setg(errp, "Vector extension VLEN must be power of 2");
> +        return;
> +    }
> +
> +    if (value != cpu->cfg.vlen && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %u\n",
> +                          name, cpu->cfg.vlen);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.vlen = value;
> +}
> +
> +static void prop_vlen_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    uint16_t value = RISCV_CPU(obj)->cfg.vlen;
> +
> +    visit_type_uint16(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_vlen = {
> +    .name = "vlen",
> +    .get = prop_vlen_get,
> +    .set = prop_vlen_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -    DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
>      DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
>
>      DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
> @@ -1755,6 +1796,8 @@ static Property riscv_cpu_properties[] = {
>      {.name = "priv_spec", .info = &prop_priv_spec},
>      {.name = "vext_spec", .info = &prop_vext_spec},
>
> +    {.name = "vlen", .info = &prop_vlen},
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 6501c29d8e..8ec858e096 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -178,11 +178,6 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>  static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
>                                   Error **errp)
>  {
> -    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 "
> --
> 2.43.0
>
>

[PATCH v3 09/16] target/riscv: move 'elen' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Do the same thing we did with 'vlen' in the previous patch with 'elen'.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 44 ++++++++++++++++++++++++++++++++++++--
 target/riscv/tcg/tcg-cpu.c |  5 -----
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 557874a017..da432e4c1e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1269,6 +1269,7 @@ static void riscv_cpu_init(Object *obj)
     /* Default values for non-bool cpu properties */
     cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
     cpu->cfg.vlen = 128;
+    cpu->cfg.elen = 64;
     cpu->env.vext_ver = VEXT_VERSION_1_00_0;
 }
 
@@ -1775,9 +1776,47 @@ static const PropertyInfo prop_vlen = {
     .set = prop_vlen_set,
 };
 
-Property riscv_cpu_options[] = {
-    DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
+static void prop_elen_set(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint16_t value;
+
+    if (!visit_type_uint16(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!is_power_of_2(value)) {
+        error_setg(errp, "Vector extension ELEN must be power of 2");
+        return;
+    }
+
+    if (value != cpu->cfg.elen && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %u\n",
+                          name, cpu->cfg.elen);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.elen = value;
+}
 
+static void prop_elen_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    uint16_t value = RISCV_CPU(obj)->cfg.elen;
+
+    visit_type_uint16(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_elen = {
+    .name = "elen",
+    .get = prop_elen_get,
+    .set = prop_elen_set,
+};
+
+Property riscv_cpu_options[] = {
     DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
     DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
 
@@ -1797,6 +1836,7 @@ static Property riscv_cpu_properties[] = {
     {.name = "vext_spec", .info = &prop_vext_spec},
 
     {.name = "vlen", .info = &prop_vlen},
+    {.name = "elen", .info = &prop_elen},
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 8ec858e096..84064ef7e0 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -185,11 +185,6 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
         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 "
-- 
2.43.0

Re: [PATCH v3 09/16] target/riscv: move 'elen' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:49 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Do the same thing we did with 'vlen' in the previous patch with 'elen'.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 44 ++++++++++++++++++++++++++++++++++++--
>  target/riscv/tcg/tcg-cpu.c |  5 -----
>  2 files changed, 42 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 557874a017..da432e4c1e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1269,6 +1269,7 @@ static void riscv_cpu_init(Object *obj)
>      /* Default values for non-bool cpu properties */
>      cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
>      cpu->cfg.vlen = 128;
> +    cpu->cfg.elen = 64;
>      cpu->env.vext_ver = VEXT_VERSION_1_00_0;
>  }
>
> @@ -1775,9 +1776,47 @@ static const PropertyInfo prop_vlen = {
>      .set = prop_vlen_set,
>  };
>
> -Property riscv_cpu_options[] = {
> -    DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
> +static void prop_elen_set(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint16_t value;
> +
> +    if (!visit_type_uint16(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (!is_power_of_2(value)) {
> +        error_setg(errp, "Vector extension ELEN must be power of 2");
> +        return;
> +    }
> +
> +    if (value != cpu->cfg.elen && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %u\n",
> +                          name, cpu->cfg.elen);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.elen = value;
> +}
>
> +static void prop_elen_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    uint16_t value = RISCV_CPU(obj)->cfg.elen;
> +
> +    visit_type_uint16(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_elen = {
> +    .name = "elen",
> +    .get = prop_elen_get,
> +    .set = prop_elen_set,
> +};
> +
> +Property riscv_cpu_options[] = {
>      DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
>      DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>
> @@ -1797,6 +1836,7 @@ static Property riscv_cpu_properties[] = {
>      {.name = "vext_spec", .info = &prop_vext_spec},
>
>      {.name = "vlen", .info = &prop_vlen},
> +    {.name = "elen", .info = &prop_elen},
>
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8ec858e096..84064ef7e0 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -185,11 +185,6 @@ static void riscv_cpu_validate_v(CPURISCVState *env, RISCVCPUConfig *cfg,
>          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 "
> --
> 2.43.0
>
>

[PATCH v3 10/16] target/riscv: create finalize_features() for KVM

[Daniel Henrique Barboza]

To turn cbom_blocksize and cboz_blocksize into class properties we need
KVM specific changes.

KVM is creating its own version of these options with a customized
setter() that prevents users from picking an invalid value during init()
time. This comes at the cost of duplicating each option that KVM
supports. This will keep happening for each new shared option KVM
implements in the future.

We can avoid that by using the same property TCG uses and adding
specific KVM handling during finalize() time, like TCG already does with
riscv_tcg_cpu_finalize_features(). To do that, the common CPU property
offers a way of knowing if an option was user set or not, sparing us
from doing unneeded syscalls.

riscv_kvm_cpu_finalize_features() is then created using the same
KVMScratch CPU we already use during init() time, since finalize() time
is still too early to use the official KVM CPU for it. cbom_blocksize
and cboz_blocksize are then handled during finalize() in the same way
they're handled by their KVM specific setter.

With this change we can proceed with the blocksize changes in the common
code without breaking the KVM driver.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index da432e4c1e..92b4881e9c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -63,6 +63,11 @@ static void cpu_option_add_user_setting(const char *optname, uint32_t value)
                         GUINT_TO_POINTER(value));
 }
 
+bool riscv_cpu_option_set(const char *optname)
+{
+    return g_hash_table_contains(general_user_opts, optname);
+}
+
 #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
     {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
 
@@ -1056,17 +1061,18 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
 {
     Error *local_err = NULL;
 
-    /*
-     * KVM accel does not have a specialized finalize()
-     * callback because its extensions are validated
-     * in the get()/set() callbacks of each property.
-     */
     if (tcg_enabled()) {
         riscv_tcg_cpu_finalize_features(cpu, &local_err);
         if (local_err != NULL) {
             error_propagate(errp, local_err);
             return;
         }
+    } else if (kvm_enabled()) {
+        riscv_kvm_cpu_finalize_features(cpu, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f1715d9d31..484c32e607 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -494,6 +494,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         bool probe, uintptr_t retaddr);
 char *riscv_isa_string(RISCVCPU *cpu);
 void riscv_cpu_list(void);
+bool riscv_cpu_option_set(const char *optname);
 
 #define cpu_list riscv_cpu_list
 #define cpu_mmu_index riscv_cpu_mmu_index
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 62a1e51f0a..70fb075846 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1490,6 +1490,65 @@ static void kvm_cpu_instance_init(CPUState *cs)
     }
 }
 
+void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
+{
+    CPURISCVState *env = &cpu->env;
+    KVMScratchCPU kvmcpu;
+    struct kvm_one_reg reg;
+    uint64_t val;
+    int ret;
+
+    /* short-circuit without spinning the scratch CPU */
+    if (!cpu->cfg.ext_zicbom && !cpu->cfg.ext_zicboz) {
+        return;
+    }
+
+    if (!kvm_riscv_create_scratch_vcpu(&kvmcpu)) {
+        error_setg(errp, "Unable to create scratch KVM cpu");
+        return;
+    }
+
+    if (cpu->cfg.ext_zicbom &&
+        riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
+
+        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+                                        kvm_cbom_blocksize.kvm_reg_id);
+        reg.addr = (uint64_t)&val;
+        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
+        if (ret != 0) {
+            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
+            return;
+        }
+
+        if (cpu->cfg.cbom_blocksize != val) {
+            error_setg(errp, "Unable to set cbom_blocksize to a different "
+                       "value than the host (%lu)", val);
+            return;
+        }
+    }
+
+    if (cpu->cfg.ext_zicboz &&
+        riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
+
+        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
+                                        kvm_cboz_blocksize.kvm_reg_id);
+        reg.addr = (uint64_t)&val;
+        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
+        if (ret != 0) {
+            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
+            return;
+        }
+
+        if (cpu->cfg.cboz_blocksize != val) {
+            error_setg(errp, "Unable to set cboz_blocksize to a different "
+                       "value than the host (%lu)", val);
+            return;
+        }
+    }
+
+    kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
+}
+
 static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
diff --git a/target/riscv/kvm/kvm_riscv.h b/target/riscv/kvm/kvm_riscv.h
index 8329cfab82..4bd98fddc7 100644
--- a/target/riscv/kvm/kvm_riscv.h
+++ b/target/riscv/kvm/kvm_riscv.h
@@ -27,5 +27,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
                           uint64_t guest_num);
 void riscv_kvm_aplic_request(void *opaque, int irq, int level);
 int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state);
+void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
 
 #endif
-- 
2.43.0

Re: [PATCH v3 10/16] target/riscv: create finalize_features() for KVM

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:46 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> To turn cbom_blocksize and cboz_blocksize into class properties we need
> KVM specific changes.
>
> KVM is creating its own version of these options with a customized
> setter() that prevents users from picking an invalid value during init()
> time. This comes at the cost of duplicating each option that KVM
> supports. This will keep happening for each new shared option KVM
> implements in the future.
>
> We can avoid that by using the same property TCG uses and adding
> specific KVM handling during finalize() time, like TCG already does with
> riscv_tcg_cpu_finalize_features(). To do that, the common CPU property
> offers a way of knowing if an option was user set or not, sparing us
> from doing unneeded syscalls.
>
> riscv_kvm_cpu_finalize_features() is then created using the same
> KVMScratch CPU we already use during init() time, since finalize() time
> is still too early to use the official KVM CPU for it. cbom_blocksize
> and cboz_blocksize are then handled during finalize() in the same way
> they're handled by their KVM specific setter.
>
> With this change we can proceed with the blocksize changes in the common
> code without breaking the KVM driver.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c           | 16 +++++++---
>  target/riscv/cpu.h           |  1 +
>  target/riscv/kvm/kvm-cpu.c   | 59 ++++++++++++++++++++++++++++++++++++
>  target/riscv/kvm/kvm_riscv.h |  1 +
>  4 files changed, 72 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index da432e4c1e..92b4881e9c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -63,6 +63,11 @@ static void cpu_option_add_user_setting(const char *optname, uint32_t value)
>                          GUINT_TO_POINTER(value));
>  }
>
> +bool riscv_cpu_option_set(const char *optname)
> +{
> +    return g_hash_table_contains(general_user_opts, optname);
> +}
> +
>  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
>      {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}
>
> @@ -1056,17 +1061,18 @@ void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
>  {
>      Error *local_err = NULL;
>
> -    /*
> -     * KVM accel does not have a specialized finalize()
> -     * callback because its extensions are validated
> -     * in the get()/set() callbacks of each property.
> -     */
>      if (tcg_enabled()) {
>          riscv_tcg_cpu_finalize_features(cpu, &local_err);
>          if (local_err != NULL) {
>              error_propagate(errp, local_err);
>              return;
>          }
> +    } else if (kvm_enabled()) {
> +        riscv_kvm_cpu_finalize_features(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
>      }
>
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f1715d9d31..484c32e607 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -494,6 +494,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                          bool probe, uintptr_t retaddr);
>  char *riscv_isa_string(RISCVCPU *cpu);
>  void riscv_cpu_list(void);
> +bool riscv_cpu_option_set(const char *optname);
>
>  #define cpu_list riscv_cpu_list
>  #define cpu_mmu_index riscv_cpu_mmu_index
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 62a1e51f0a..70fb075846 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1490,6 +1490,65 @@ static void kvm_cpu_instance_init(CPUState *cs)
>      }
>  }
>
> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> +{
> +    CPURISCVState *env = &cpu->env;
> +    KVMScratchCPU kvmcpu;
> +    struct kvm_one_reg reg;
> +    uint64_t val;
> +    int ret;
> +
> +    /* short-circuit without spinning the scratch CPU */
> +    if (!cpu->cfg.ext_zicbom && !cpu->cfg.ext_zicboz) {
> +        return;
> +    }
> +
> +    if (!kvm_riscv_create_scratch_vcpu(&kvmcpu)) {
> +        error_setg(errp, "Unable to create scratch KVM cpu");
> +        return;
> +    }
> +
> +    if (cpu->cfg.ext_zicbom &&
> +        riscv_cpu_option_set(kvm_cbom_blocksize.name)) {
> +
> +        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
> +                                        kvm_cbom_blocksize.kvm_reg_id);
> +        reg.addr = (uint64_t)&val;
> +        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
> +        if (ret != 0) {
> +            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
> +            return;
> +        }
> +
> +        if (cpu->cfg.cbom_blocksize != val) {
> +            error_setg(errp, "Unable to set cbom_blocksize to a different "
> +                       "value than the host (%lu)", val);
> +            return;
> +        }
> +    }
> +
> +    if (cpu->cfg.ext_zicboz &&
> +        riscv_cpu_option_set(kvm_cboz_blocksize.name)) {
> +
> +        reg.id = kvm_riscv_reg_id_ulong(env, KVM_REG_RISCV_CONFIG,
> +                                        kvm_cboz_blocksize.kvm_reg_id);
> +        reg.addr = (uint64_t)&val;
> +        ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
> +        if (ret != 0) {
> +            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);

cboz_blocksize in the message

Alistair

> +            return;
> +        }
> +
> +        if (cpu->cfg.cboz_blocksize != val) {
> +            error_setg(errp, "Unable to set cboz_blocksize to a different "
> +                       "value than the host (%lu)", val);
> +            return;
> +        }
> +    }
> +
> +    kvm_riscv_destroy_scratch_vcpu(&kvmcpu);
> +}
> +
>  static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
>  {
>      AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
> diff --git a/target/riscv/kvm/kvm_riscv.h b/target/riscv/kvm/kvm_riscv.h
> index 8329cfab82..4bd98fddc7 100644
> --- a/target/riscv/kvm/kvm_riscv.h
> +++ b/target/riscv/kvm/kvm_riscv.h
> @@ -27,5 +27,6 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>                            uint64_t guest_num);
>  void riscv_kvm_aplic_request(void *opaque, int irq, int level);
>  int kvm_riscv_sync_mpstate_to_kvm(RISCVCPU *cpu, int state);
> +void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp);
>
>  #endif
> --
> 2.43.0
>
>

[PATCH v3 11/16] target/riscv: move 'cbom_blocksize' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

After adding a KVM finalize() implementation, turn cbom_blocksize into a
class property. Follow the same design we used with 'vlen' and 'elen'.

The duplicated 'cbom_blocksize' KVM property can be removed from
kvm_riscv_add_cpu_user_properties().

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 92b4881e9c..b510cb94fc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1276,6 +1276,7 @@ static void riscv_cpu_init(Object *obj)
     cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
     cpu->cfg.vlen = 128;
     cpu->cfg.elen = 64;
+    cpu->cfg.cbom_blocksize = 64;
     cpu->env.vext_ver = VEXT_VERSION_1_00_0;
 }
 
@@ -1822,8 +1823,42 @@ static const PropertyInfo prop_elen = {
     .set = prop_elen_set,
 };
 
+static void prop_cbom_blksize_set(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint16_t value;
+
+    if (!visit_type_uint16(v, name, &value, errp)) {
+        return;
+    }
+
+    if (value != cpu->cfg.cbom_blocksize && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %u\n",
+                          name, cpu->cfg.cbom_blocksize);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.cbom_blocksize = value;
+}
+
+static void prop_cbom_blksize_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    uint16_t value = RISCV_CPU(obj)->cfg.cbom_blocksize;
+
+    visit_type_uint16(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_cbom_blksize = {
+    .name = "cbom_blocksize",
+    .get = prop_cbom_blksize_get,
+    .set = prop_cbom_blksize_set,
+};
+
 Property riscv_cpu_options[] = {
-    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
     DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
 
     DEFINE_PROP_END_OF_LIST(),
@@ -1844,6 +1879,8 @@ static Property riscv_cpu_properties[] = {
     {.name = "vlen", .info = &prop_vlen},
     {.name = "elen", .info = &prop_elen},
 
+    {.name = "cbom_blocksize", .info = &prop_cbom_blksize},
+
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 70fb075846..1866b56913 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -484,10 +484,6 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
                             NULL, multi_cfg);
     }
 
-    object_property_add(cpu_obj, "cbom_blocksize", "uint16",
-                        NULL, kvm_cpu_set_cbomz_blksize,
-                        NULL, &kvm_cbom_blocksize);
-
     object_property_add(cpu_obj, "cboz_blocksize", "uint16",
                         NULL, kvm_cpu_set_cbomz_blksize,
                         NULL, &kvm_cboz_blocksize);
-- 
2.43.0

Re: [PATCH v3 11/16] target/riscv: move 'cbom_blocksize' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:44 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> After adding a KVM finalize() implementation, turn cbom_blocksize into a
> class property. Follow the same design we used with 'vlen' and 'elen'.
>
> The duplicated 'cbom_blocksize' KVM property can be removed from
> kvm_riscv_add_cpu_user_properties().
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 39 +++++++++++++++++++++++++++++++++++++-
>  target/riscv/kvm/kvm-cpu.c |  4 ----
>  2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 92b4881e9c..b510cb94fc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1276,6 +1276,7 @@ static void riscv_cpu_init(Object *obj)
>      cpu->cfg.pmu_mask = MAKE_64BIT_MASK(3, 16);
>      cpu->cfg.vlen = 128;
>      cpu->cfg.elen = 64;
> +    cpu->cfg.cbom_blocksize = 64;
>      cpu->env.vext_ver = VEXT_VERSION_1_00_0;
>  }
>
> @@ -1822,8 +1823,42 @@ static const PropertyInfo prop_elen = {
>      .set = prop_elen_set,
>  };
>
> +static void prop_cbom_blksize_set(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint16_t value;
> +
> +    if (!visit_type_uint16(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (value != cpu->cfg.cbom_blocksize && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %u\n",
> +                          name, cpu->cfg.cbom_blocksize);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.cbom_blocksize = value;
> +}
> +
> +static void prop_cbom_blksize_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    uint16_t value = RISCV_CPU(obj)->cfg.cbom_blocksize;
> +
> +    visit_type_uint16(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_cbom_blksize = {
> +    .name = "cbom_blocksize",
> +    .get = prop_cbom_blksize_get,
> +    .set = prop_cbom_blksize_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -    DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
>      DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>
>      DEFINE_PROP_END_OF_LIST(),
> @@ -1844,6 +1879,8 @@ static Property riscv_cpu_properties[] = {
>      {.name = "vlen", .info = &prop_vlen},
>      {.name = "elen", .info = &prop_elen},
>
> +    {.name = "cbom_blocksize", .info = &prop_cbom_blksize},
> +
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
>  #endif
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 70fb075846..1866b56913 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -484,10 +484,6 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>                              NULL, multi_cfg);
>      }
>
> -    object_property_add(cpu_obj, "cbom_blocksize", "uint16",
> -                        NULL, kvm_cpu_set_cbomz_blksize,
> -                        NULL, &kvm_cbom_blocksize);
> -
>      object_property_add(cpu_obj, "cboz_blocksize", "uint16",
>                          NULL, kvm_cpu_set_cbomz_blksize,
>                          NULL, &kvm_cboz_blocksize);
> --
> 2.43.0
>
>

[PATCH v3 12/16] target/riscv: move 'cboz_blocksize' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Do the same we did with 'cbom_blocksize' in the previous patch.

Remove the now unused kvm_cpu_set_cbomz_blksize() setter.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 38 +++++++++++++++++++++++++++++++++++++-
 target/riscv/kvm/kvm-cpu.c | 28 ----------------------------
 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b510cb94fc..1e35b73e40 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1277,6 +1277,7 @@ static void riscv_cpu_init(Object *obj)
     cpu->cfg.vlen = 128;
     cpu->cfg.elen = 64;
     cpu->cfg.cbom_blocksize = 64;
+    cpu->cfg.cboz_blocksize = 64;
     cpu->env.vext_ver = VEXT_VERSION_1_00_0;
 }
 
@@ -1858,8 +1859,42 @@ static const PropertyInfo prop_cbom_blksize = {
     .set = prop_cbom_blksize_set,
 };
 
+static void prop_cboz_blksize_set(Object *obj, Visitor *v, const char *name,
+                                  void *opaque, Error **errp)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint16_t value;
+
+    if (!visit_type_uint16(v, name, &value, errp)) {
+        return;
+    }
+
+    if (value != cpu->cfg.cboz_blocksize && riscv_cpu_is_vendor(obj)) {
+        cpu_set_prop_err(cpu, name, errp);
+        error_append_hint(errp, "Current '%s' val: %u\n",
+                          name, cpu->cfg.cboz_blocksize);
+        return;
+    }
+
+    cpu_option_add_user_setting(name, value);
+    cpu->cfg.cboz_blocksize = value;
+}
+
+static void prop_cboz_blksize_get(Object *obj, Visitor *v, const char *name,
+                         void *opaque, Error **errp)
+{
+    uint16_t value = RISCV_CPU(obj)->cfg.cboz_blocksize;
+
+    visit_type_uint16(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_cboz_blksize = {
+    .name = "cboz_blocksize",
+    .get = prop_cboz_blksize_get,
+    .set = prop_cboz_blksize_set,
+};
+
 Property riscv_cpu_options[] = {
-    DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
 
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -1880,6 +1915,7 @@ static Property riscv_cpu_properties[] = {
     {.name = "elen", .info = &prop_elen},
 
     {.name = "cbom_blocksize", .info = &prop_cbom_blksize},
+    {.name = "cboz_blocksize", .info = &prop_cboz_blksize},
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 1866b56913..137a8ab2bb 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -343,30 +343,6 @@ static KVMCPUConfig kvm_cboz_blocksize = {
     .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicboz_block_size)
 };
 
-static void kvm_cpu_set_cbomz_blksize(Object *obj, Visitor *v,
-                                      const char *name,
-                                      void *opaque, Error **errp)
-{
-    KVMCPUConfig *cbomz_cfg = opaque;
-    RISCVCPU *cpu = RISCV_CPU(obj);
-    uint16_t value, *host_val;
-
-    if (!visit_type_uint16(v, name, &value, errp)) {
-        return;
-    }
-
-    host_val = kvmconfig_get_cfg_addr(cpu, cbomz_cfg);
-
-    if (value != *host_val) {
-        error_report("Unable to set %s to a different value than "
-                     "the host (%u)",
-                     cbomz_cfg->name, *host_val);
-        exit(EXIT_FAILURE);
-    }
-
-    cbomz_cfg->user_set = true;
-}
-
 static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
 {
     CPURISCVState *env = &cpu->env;
@@ -484,10 +460,6 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
                             NULL, multi_cfg);
     }
 
-    object_property_add(cpu_obj, "cboz_blocksize", "uint16",
-                        NULL, kvm_cpu_set_cbomz_blksize,
-                        NULL, &kvm_cboz_blocksize);
-
     riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_extensions);
     riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_vendor_exts);
     riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_experimental_exts);
-- 
2.43.0

Re: [PATCH v3 12/16] target/riscv: move 'cboz_blocksize' to riscv_cpu_properties[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 4:56 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Do the same we did with 'cbom_blocksize' in the previous patch.
>
> Remove the now unused kvm_cpu_set_cbomz_blksize() setter.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 38 +++++++++++++++++++++++++++++++++++++-
>  target/riscv/kvm/kvm-cpu.c | 28 ----------------------------
>  2 files changed, 37 insertions(+), 29 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b510cb94fc..1e35b73e40 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1277,6 +1277,7 @@ static void riscv_cpu_init(Object *obj)
>      cpu->cfg.vlen = 128;
>      cpu->cfg.elen = 64;
>      cpu->cfg.cbom_blocksize = 64;
> +    cpu->cfg.cboz_blocksize = 64;
>      cpu->env.vext_ver = VEXT_VERSION_1_00_0;
>  }
>
> @@ -1858,8 +1859,42 @@ static const PropertyInfo prop_cbom_blksize = {
>      .set = prop_cbom_blksize_set,
>  };
>
> +static void prop_cboz_blksize_set(Object *obj, Visitor *v, const char *name,
> +                                  void *opaque, Error **errp)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    uint16_t value;
> +
> +    if (!visit_type_uint16(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    if (value != cpu->cfg.cboz_blocksize && riscv_cpu_is_vendor(obj)) {
> +        cpu_set_prop_err(cpu, name, errp);
> +        error_append_hint(errp, "Current '%s' val: %u\n",
> +                          name, cpu->cfg.cboz_blocksize);
> +        return;
> +    }
> +
> +    cpu_option_add_user_setting(name, value);
> +    cpu->cfg.cboz_blocksize = value;
> +}
> +
> +static void prop_cboz_blksize_get(Object *obj, Visitor *v, const char *name,
> +                         void *opaque, Error **errp)
> +{
> +    uint16_t value = RISCV_CPU(obj)->cfg.cboz_blocksize;
> +
> +    visit_type_uint16(v, name, &value, errp);
> +}
> +
> +static const PropertyInfo prop_cboz_blksize = {
> +    .name = "cboz_blocksize",
> +    .get = prop_cboz_blksize_get,
> +    .set = prop_cboz_blksize_set,
> +};
> +
>  Property riscv_cpu_options[] = {
> -    DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
>
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -1880,6 +1915,7 @@ static Property riscv_cpu_properties[] = {
>      {.name = "elen", .info = &prop_elen},
>
>      {.name = "cbom_blocksize", .info = &prop_cbom_blksize},
> +    {.name = "cboz_blocksize", .info = &prop_cboz_blksize},
>
>  #ifndef CONFIG_USER_ONLY
>      DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 1866b56913..137a8ab2bb 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -343,30 +343,6 @@ static KVMCPUConfig kvm_cboz_blocksize = {
>      .kvm_reg_id = KVM_REG_RISCV_CONFIG_REG(zicboz_block_size)
>  };
>
> -static void kvm_cpu_set_cbomz_blksize(Object *obj, Visitor *v,
> -                                      const char *name,
> -                                      void *opaque, Error **errp)
> -{
> -    KVMCPUConfig *cbomz_cfg = opaque;
> -    RISCVCPU *cpu = RISCV_CPU(obj);
> -    uint16_t value, *host_val;
> -
> -    if (!visit_type_uint16(v, name, &value, errp)) {
> -        return;
> -    }
> -
> -    host_val = kvmconfig_get_cfg_addr(cpu, cbomz_cfg);
> -
> -    if (value != *host_val) {
> -        error_report("Unable to set %s to a different value than "
> -                     "the host (%u)",
> -                     cbomz_cfg->name, *host_val);
> -        exit(EXIT_FAILURE);
> -    }
> -
> -    cbomz_cfg->user_set = true;
> -}
> -
>  static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
>  {
>      CPURISCVState *env = &cpu->env;
> @@ -484,10 +460,6 @@ static void kvm_riscv_add_cpu_user_properties(Object *cpu_obj)
>                              NULL, multi_cfg);
>      }
>
> -    object_property_add(cpu_obj, "cboz_blocksize", "uint16",
> -                        NULL, kvm_cpu_set_cbomz_blksize,
> -                        NULL, &kvm_cboz_blocksize);
> -
>      riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_extensions);
>      riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_vendor_exts);
>      riscv_cpu_add_kvm_unavail_prop_array(cpu_obj, riscv_cpu_experimental_exts);
> --
> 2.43.0
>
>

[PATCH v3 13/16] target/riscv: remove riscv_cpu_options[]

[Daniel Henrique Barboza]

The array is empty and can be removed.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c         | 5 -----
 target/riscv/cpu.h         | 1 -
 target/riscv/kvm/kvm-cpu.c | 9 ---------
 target/riscv/tcg/tcg-cpu.c | 4 ----
 4 files changed, 19 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e35b73e40..0d0197a8ef 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1894,11 +1894,6 @@ static const PropertyInfo prop_cboz_blksize = {
     .set = prop_cboz_blksize_set,
 };
 
-Property riscv_cpu_options[] = {
-
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 484c32e607..59e23708d1 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -773,7 +773,6 @@ extern const RISCVCPUMultiExtConfig riscv_cpu_extensions[];
 extern const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[];
 extern const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[];
 extern const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[];
-extern Property riscv_cpu_options[];
 
 typedef struct isa_ext_data {
     const char *name;
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 137a8ab2bb..5800abc9c6 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1443,19 +1443,10 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
 static void kvm_cpu_instance_init(CPUState *cs)
 {
     Object *obj = OBJECT(RISCV_CPU(cs));
-    DeviceState *dev = DEVICE(obj);
 
     riscv_init_kvm_registers(obj);
 
     kvm_riscv_add_cpu_user_properties(obj);
-
-    for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
-        /* Check if we have a specific KVM handler for the option */
-        if (object_property_find(obj, prop->name)) {
-            continue;
-        }
-        qdev_property_add_static(dev, prop);
-    }
 }
 
 void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 84064ef7e0..d3eeedc758 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -889,10 +889,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_experimental_exts);
 
     riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
-
-    for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
-        qdev_property_add_static(DEVICE(obj), prop);
-    }
 }
 
 /*
-- 
2.43.0

Re: [PATCH v3 13/16] target/riscv: remove riscv_cpu_options[]

[Alistair Francis]

On Thu, Jan 4, 2024 at 3:45 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> The array is empty and can be removed.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 5 -----
>  target/riscv/cpu.h         | 1 -
>  target/riscv/kvm/kvm-cpu.c | 9 ---------
>  target/riscv/tcg/tcg-cpu.c | 4 ----
>  4 files changed, 19 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 1e35b73e40..0d0197a8ef 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1894,11 +1894,6 @@ static const PropertyInfo prop_cboz_blksize = {
>      .set = prop_cboz_blksize_set,
>  };
>
> -Property riscv_cpu_options[] = {
> -
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static Property riscv_cpu_properties[] = {
>      DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 484c32e607..59e23708d1 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -773,7 +773,6 @@ extern const RISCVCPUMultiExtConfig riscv_cpu_extensions[];
>  extern const RISCVCPUMultiExtConfig riscv_cpu_vendor_exts[];
>  extern const RISCVCPUMultiExtConfig riscv_cpu_experimental_exts[];
>  extern const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[];
> -extern Property riscv_cpu_options[];
>
>  typedef struct isa_ext_data {
>      const char *name;
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 137a8ab2bb..5800abc9c6 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1443,19 +1443,10 @@ void kvm_riscv_aia_create(MachineState *machine, uint64_t group_shift,
>  static void kvm_cpu_instance_init(CPUState *cs)
>  {
>      Object *obj = OBJECT(RISCV_CPU(cs));
> -    DeviceState *dev = DEVICE(obj);
>
>      riscv_init_kvm_registers(obj);
>
>      kvm_riscv_add_cpu_user_properties(obj);
> -
> -    for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
> -        /* Check if we have a specific KVM handler for the option */
> -        if (object_property_find(obj, prop->name)) {
> -            continue;
> -        }
> -        qdev_property_add_static(dev, prop);
> -    }
>  }
>
>  void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 84064ef7e0..d3eeedc758 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -889,10 +889,6 @@ static void riscv_cpu_add_user_properties(Object *obj)
>      riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_experimental_exts);
>
>      riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
> -
> -    for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
> -        qdev_property_add_static(DEVICE(obj), prop);
> -    }
>  }
>
>  /*
> --
> 2.43.0
>
>

[PATCH v3 14/16] target/riscv/cpu.c: move 'mvendorid' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Keep all class properties in riscv_cpu_properties[].

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 0d0197a8ef..2fe282d0af 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1894,6 +1894,41 @@ static const PropertyInfo prop_cboz_blksize = {
     .set = prop_cboz_blksize_set,
 };
 
+static void prop_mvendorid_set(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint32_t prev_val = cpu->cfg.mvendorid;
+    uint32_t value;
+
+    if (!visit_type_uint32(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!dynamic_cpu && prev_val != value) {
+        error_setg(errp, "Unable to change %s mvendorid (0x%x)",
+                   object_get_typename(obj), prev_val);
+        return;
+    }
+
+    cpu->cfg.mvendorid = value;
+}
+
+static void prop_mvendorid_get(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    uint32_t value = RISCV_CPU(obj)->cfg.mvendorid;
+
+    visit_type_uint32(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_mvendorid = {
+    .name = "mvendorid",
+    .get = prop_mvendorid_get,
+    .set = prop_mvendorid_set,
+};
+
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
@@ -1912,6 +1947,8 @@ static Property riscv_cpu_properties[] = {
     {.name = "cbom_blocksize", .info = &prop_cbom_blksize},
     {.name = "cboz_blocksize", .info = &prop_cboz_blksize},
 
+     {.name = "mvendorid", .info = &prop_mvendorid},
+
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
 #endif
@@ -1976,35 +2013,6 @@ static const struct SysemuCPUOps riscv_sysemu_ops = {
 };
 #endif
 
-static void cpu_set_mvendorid(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
-{
-    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
-    RISCVCPU *cpu = RISCV_CPU(obj);
-    uint32_t prev_val = cpu->cfg.mvendorid;
-    uint32_t value;
-
-    if (!visit_type_uint32(v, name, &value, errp)) {
-        return;
-    }
-
-    if (!dynamic_cpu && prev_val != value) {
-        error_setg(errp, "Unable to change %s mvendorid (0x%x)",
-                   object_get_typename(obj), prev_val);
-        return;
-    }
-
-    cpu->cfg.mvendorid = value;
-}
-
-static void cpu_get_mvendorid(Object *obj, Visitor *v, const char *name,
-                              void *opaque, Error **errp)
-{
-    uint32_t value = RISCV_CPU(obj)->cfg.mvendorid;
-
-    visit_type_uint32(v, name, &value, errp);
-}
-
 static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
                            void *opaque, Error **errp)
 {
@@ -2114,9 +2122,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_arch_name = riscv_gdb_arch_name;
     cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
 
-    object_class_property_add(c, "mvendorid", "uint32", cpu_get_mvendorid,
-                              cpu_set_mvendorid, NULL, NULL);
-
     object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
                               cpu_set_mimpid, NULL, NULL);
 
-- 
2.43.0

[PATCH v3 15/16] target/riscv/cpu.c: move 'mimpid' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Keep all class properties in riscv_cpu_properties[].

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2fe282d0af..3ea823f5cf 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1929,6 +1929,41 @@ static const PropertyInfo prop_mvendorid = {
     .set = prop_mvendorid_set,
 };
 
+static void prop_mimpid_set(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint64_t prev_val = cpu->cfg.mimpid;
+    uint64_t value;
+
+    if (!visit_type_uint64(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!dynamic_cpu && prev_val != value) {
+        error_setg(errp, "Unable to change %s mimpid (0x%" PRIu64 ")",
+                   object_get_typename(obj), prev_val);
+        return;
+    }
+
+    cpu->cfg.mimpid = value;
+}
+
+static void prop_mimpid_get(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    uint64_t value = RISCV_CPU(obj)->cfg.mimpid;
+
+    visit_type_uint64(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_mimpid = {
+    .name = "mimpid",
+    .get = prop_mimpid_get,
+    .set = prop_mimpid_set,
+};
+
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
@@ -1948,6 +1983,7 @@ static Property riscv_cpu_properties[] = {
     {.name = "cboz_blocksize", .info = &prop_cboz_blksize},
 
      {.name = "mvendorid", .info = &prop_mvendorid},
+     {.name = "mimpid", .info = &prop_mimpid},
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
@@ -2013,35 +2049,6 @@ static const struct SysemuCPUOps riscv_sysemu_ops = {
 };
 #endif
 
-static void cpu_set_mimpid(Object *obj, Visitor *v, const char *name,
-                           void *opaque, Error **errp)
-{
-    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
-    RISCVCPU *cpu = RISCV_CPU(obj);
-    uint64_t prev_val = cpu->cfg.mimpid;
-    uint64_t value;
-
-    if (!visit_type_uint64(v, name, &value, errp)) {
-        return;
-    }
-
-    if (!dynamic_cpu && prev_val != value) {
-        error_setg(errp, "Unable to change %s mimpid (0x%" PRIu64 ")",
-                   object_get_typename(obj), prev_val);
-        return;
-    }
-
-    cpu->cfg.mimpid = value;
-}
-
-static void cpu_get_mimpid(Object *obj, Visitor *v, const char *name,
-                           void *opaque, Error **errp)
-{
-    uint64_t value = RISCV_CPU(obj)->cfg.mimpid;
-
-    visit_type_uint64(v, name, &value, errp);
-}
-
 static void cpu_set_marchid(Object *obj, Visitor *v, const char *name,
                             void *opaque, Error **errp)
 {
@@ -2122,9 +2129,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_arch_name = riscv_gdb_arch_name;
     cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
 
-    object_class_property_add(c, "mimpid", "uint64", cpu_get_mimpid,
-                              cpu_set_mimpid, NULL, NULL);
-
     object_class_property_add(c, "marchid", "uint64", cpu_get_marchid,
                               cpu_set_marchid, NULL, NULL);
 
-- 
2.43.0

[PATCH v3 16/16] target/riscv/cpu.c: move 'marchid' to riscv_cpu_properties[]

[Daniel Henrique Barboza]

Keep all class properties in riscv_cpu_properties[].

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3ea823f5cf..c67741ffa7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1964,6 +1964,62 @@ static const PropertyInfo prop_mimpid = {
     .set = prop_mimpid_set,
 };
 
+static void prop_marchid_set(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    uint64_t prev_val = cpu->cfg.marchid;
+    uint64_t value, invalid_val;
+    uint32_t mxlen = 0;
+
+    if (!visit_type_uint64(v, name, &value, errp)) {
+        return;
+    }
+
+    if (!dynamic_cpu && prev_val != value) {
+        error_setg(errp, "Unable to change %s marchid (0x%" PRIu64 ")",
+                   object_get_typename(obj), prev_val);
+        return;
+    }
+
+    switch (riscv_cpu_mxl(&cpu->env)) {
+    case MXL_RV32:
+        mxlen = 32;
+        break;
+    case MXL_RV64:
+    case MXL_RV128:
+        mxlen = 64;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    invalid_val = 1LL << (mxlen - 1);
+
+    if (value == invalid_val) {
+        error_setg(errp, "Unable to set marchid with MSB (%u) bit set "
+                         "and the remaining bits zero", mxlen);
+        return;
+    }
+
+    cpu->cfg.marchid = value;
+}
+
+static void prop_marchid_get(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    uint64_t value = RISCV_CPU(obj)->cfg.marchid;
+
+    visit_type_uint64(v, name, &value, errp);
+}
+
+static const PropertyInfo prop_marchid = {
+    .name = "marchid",
+    .get = prop_marchid_get,
+    .set = prop_marchid_set,
+};
+
 static Property riscv_cpu_properties[] = {
     DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
 
@@ -1984,6 +2040,7 @@ static Property riscv_cpu_properties[] = {
 
      {.name = "mvendorid", .info = &prop_mvendorid},
      {.name = "mimpid", .info = &prop_mimpid},
+     {.name = "marchid", .info = &prop_marchid},
 
 #ifndef CONFIG_USER_ONLY
     DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, DEFAULT_RSTVEC),
@@ -2049,56 +2106,6 @@ static const struct SysemuCPUOps riscv_sysemu_ops = {
 };
 #endif
 
-static void cpu_set_marchid(Object *obj, Visitor *v, const char *name,
-                            void *opaque, Error **errp)
-{
-    bool dynamic_cpu = riscv_cpu_is_dynamic(obj);
-    RISCVCPU *cpu = RISCV_CPU(obj);
-    uint64_t prev_val = cpu->cfg.marchid;
-    uint64_t value, invalid_val;
-    uint32_t mxlen = 0;
-
-    if (!visit_type_uint64(v, name, &value, errp)) {
-        return;
-    }
-
-    if (!dynamic_cpu && prev_val != value) {
-        error_setg(errp, "Unable to change %s marchid (0x%" PRIu64 ")",
-                   object_get_typename(obj), prev_val);
-        return;
-    }
-
-    switch (riscv_cpu_mxl(&cpu->env)) {
-    case MXL_RV32:
-        mxlen = 32;
-        break;
-    case MXL_RV64:
-    case MXL_RV128:
-        mxlen = 64;
-        break;
-    default:
-        g_assert_not_reached();
-    }
-
-    invalid_val = 1LL << (mxlen - 1);
-
-    if (value == invalid_val) {
-        error_setg(errp, "Unable to set marchid with MSB (%u) bit set "
-                         "and the remaining bits zero", mxlen);
-        return;
-    }
-
-    cpu->cfg.marchid = value;
-}
-
-static void cpu_get_marchid(Object *obj, Visitor *v, const char *name,
-                           void *opaque, Error **errp)
-{
-    uint64_t value = RISCV_CPU(obj)->cfg.marchid;
-
-    visit_type_uint64(v, name, &value, errp);
-}
-
 static void riscv_cpu_class_init(ObjectClass *c, void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
@@ -2129,9 +2136,6 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_arch_name = riscv_gdb_arch_name;
     cc->gdb_get_dynamic_xml = riscv_gdb_get_dynamic_xml;
 
-    object_class_property_add(c, "marchid", "uint64", cpu_get_marchid,
-                              cpu_set_marchid, NULL, NULL);
-
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
-- 
2.43.0