[PATCH 0/2] riscv: add extension properties for all cpus

[PATCH 0/2] riscv: add extension properties for all cpus

[Daniel Henrique Barboza]

Hi,

At this moment we do not expose extension properties for vendor CPUs
because that would allow users to enable extensions in them. But that
comes at a cost: if we were to add an API that shows all CPU properties,
e.g. qmp-query-cpu-model-expansion, we won't be able to show the
extension state of vendor CPUs.

We're in a good spot to revisit this decision. We have the required
abstractions in place to be able to expose user properties for vendor
CPUs and, at the same time, forbid users to enable extensions for those
CPUs. As a bonus, we'll allow users to be able to disable extensions for
vendor CPUs, which can be useful for testing/debugging.

Patches based on Alistair's riscv-to-apply.next.

Daniel Henrique Barboza (2):
  target/riscv: add riscv_cpu_get_name()
  target/riscv/tcg-cpu.c: add extension properties for all cpus

 target/riscv/cpu.c         | 11 ++++++
 target/riscv/cpu.h         |  1 +
 target/riscv/tcg/tcg-cpu.c | 68 +++++++++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 15 deletions(-)

-- 
2.41.0

[PATCH 1/2] target/riscv: add riscv_cpu_get_name()

[Daniel Henrique Barboza]

We'll introduce generic errors that will output a CPU type name via its
RISCVCPU pointer. Create a helper for that.

Use the helper in tcg_cpu_realizefn() instead of hardcoding the 'host'
CPU name.

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

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index eeeb08a35a..521bb88538 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -643,6 +643,17 @@ static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
+char *riscv_cpu_get_name(RISCVCPU *cpu)
+{
+    RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
+    const char *typename = object_class_get_name(OBJECT_CLASS(rcc));
+
+    g_assert(g_str_has_suffix(typename, RISCV_CPU_TYPE_SUFFIX));
+
+    return g_strndup(typename,
+                     strlen(typename) - strlen(RISCV_CPU_TYPE_SUFFIX));
+}
+
 static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 219fe2e9b5..3f11e69223 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -730,6 +730,7 @@ typedef struct isa_ext_data {
     int ext_enable_offset;
 } RISCVIsaExtData;
 extern const RISCVIsaExtData isa_edata_arr[];
+char *riscv_cpu_get_name(RISCVCPU *cpu);
 
 void riscv_add_satp_mode_properties(Object *obj);
 
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 8c052d6fcd..f31aa9bcc4 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -563,7 +563,9 @@ static bool tcg_cpu_realizefn(CPUState *cs, Error **errp)
     Error *local_err = NULL;
 
     if (object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
-        error_setg(errp, "'host' CPU is not compatible with TCG acceleration");
+        g_autofree char *name = riscv_cpu_get_name(cpu);
+        error_setg(errp, "'%s' CPU is not compatible with TCG acceleration",
+                   name);
         return false;
     }
 
-- 
2.41.0

Re: [PATCH 1/2] target/riscv: add riscv_cpu_get_name()

[Alistair Francis]

On Wed, Sep 27, 2023 at 5:35 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> We'll introduce generic errors that will output a CPU type name via its
> RISCVCPU pointer. Create a helper for that.
>
> Use the helper in tcg_cpu_realizefn() instead of hardcoding the 'host'
> CPU name.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu.c         | 11 +++++++++++
>  target/riscv/cpu.h         |  1 +
>  target/riscv/tcg/tcg-cpu.c |  4 +++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index eeeb08a35a..521bb88538 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -643,6 +643,17 @@ static ObjectClass *riscv_cpu_class_by_name(const char *cpu_model)
>      return oc;
>  }
>
> +char *riscv_cpu_get_name(RISCVCPU *cpu)
> +{
> +    RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
> +    const char *typename = object_class_get_name(OBJECT_CLASS(rcc));
> +
> +    g_assert(g_str_has_suffix(typename, RISCV_CPU_TYPE_SUFFIX));
> +
> +    return g_strndup(typename,
> +                     strlen(typename) - strlen(RISCV_CPU_TYPE_SUFFIX));
> +}
> +
>  static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 219fe2e9b5..3f11e69223 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -730,6 +730,7 @@ typedef struct isa_ext_data {
>      int ext_enable_offset;
>  } RISCVIsaExtData;
>  extern const RISCVIsaExtData isa_edata_arr[];
> +char *riscv_cpu_get_name(RISCVCPU *cpu);
>
>  void riscv_add_satp_mode_properties(Object *obj);
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8c052d6fcd..f31aa9bcc4 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -563,7 +563,9 @@ static bool tcg_cpu_realizefn(CPUState *cs, Error **errp)
>      Error *local_err = NULL;
>
>      if (object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_CPU_HOST)) {
> -        error_setg(errp, "'host' CPU is not compatible with TCG acceleration");
> +        g_autofree char *name = riscv_cpu_get_name(cpu);
> +        error_setg(errp, "'%s' CPU is not compatible with TCG acceleration",
> +                   name);
>          return false;
>      }
>
> --
> 2.41.0
>
>

[PATCH 2/2] target/riscv/tcg-cpu.c: add extension properties for all cpus

[Daniel Henrique Barboza]

At this moment we do not expose extension properties for vendor CPUs
because that would allow users to change them via command line. The
drawback is that if we were to add an API that shows all CPU properties,
e.g. qmp-query-cpu-model-expansion, we won't be able to show extensions
state of vendor CPUs.

We have the required machinery to create extension properties for vendor
CPUs while not allowing users to enable extensions. Disabling existing
extensions is allowed since it can be useful for debugging.

Change the set() callback cpu_set_multi_ext_cfg() to allow enabling
extensions only for generic CPUs. In cpu_add_multi_ext_prop() let's not
set the default values for the properties if we're not dealing with
generic CPUs, otherwise the values set in cpu_init() of vendor CPUs will
be overwritten. And finally, in tcg_cpu_instance_init(), add cpu user
properties for all CPUs.

For the veyron-v1 CPU, we're now able to disable existing extensions
like smstateen:

$ ./build/qemu-system-riscv64 --nographic -M virt \
    -cpu veyron-v1,smstateen=false

But setting extensions that the CPU didn't set during cpu_init(), like
V, is not allowed:

$ ./build/qemu-system-riscv64 --nographic -M virt \
    -cpu veyron-v1,v=true
qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.v=true:
  'veyron-v1' CPU does not allow enabling extensions

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

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index f31aa9bcc4..a90ee63b06 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -549,6 +549,11 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
     riscv_cpu_disable_priv_spec_isa_exts(cpu);
 }
 
+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:
  *
@@ -632,13 +637,27 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
     target_ulong misa_bit = misa_ext_cfg->misa_bit;
     RISCVCPU *cpu = RISCV_CPU(obj);
     CPURISCVState *env = &cpu->env;
-    bool value;
+    bool generic_cpu = riscv_cpu_is_generic(obj);
+    bool prev_val, value;
 
     if (!visit_type_bool(v, name, &value, errp)) {
         return;
     }
 
+    prev_val = env->misa_ext & misa_bit;
+
+    if (value == prev_val) {
+        return;
+    }
+
     if (value) {
+        if (!generic_cpu) {
+            g_autofree char *cpuname = riscv_cpu_get_name(cpu);
+            error_setg(errp, "'%s' CPU does not allow enabling extensions",
+                       cpuname);
+            return;
+        }
+
         env->misa_ext |= misa_bit;
         env->misa_ext_mask |= misa_bit;
     } else {
@@ -688,6 +707,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
  */
 static void riscv_cpu_add_misa_properties(Object *cpu_obj)
 {
+    bool use_def_vals = riscv_cpu_is_generic(cpu_obj);
     int i;
 
     for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
@@ -706,7 +726,9 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
                             cpu_set_misa_ext_cfg,
                             NULL, (void *)misa_cfg);
         object_property_set_description(cpu_obj, name, desc);
-        object_property_set_bool(cpu_obj, name, misa_cfg->enabled, NULL);
+        if (use_def_vals) {
+            object_property_set_bool(cpu_obj, name, misa_cfg->enabled, NULL);
+        }
     }
 }
 
@@ -714,17 +736,32 @@ static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
                                   void *opaque, Error **errp)
 {
     const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
-    bool value;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+    bool generic_cpu = riscv_cpu_is_generic(obj);
+    bool prev_val, value;
 
     if (!visit_type_bool(v, name, &value, errp)) {
         return;
     }
 
-    isa_ext_update_enabled(RISCV_CPU(obj), multi_ext_cfg->offset, value);
-
     g_hash_table_insert(multi_ext_user_opts,
                         GUINT_TO_POINTER(multi_ext_cfg->offset),
                         (gpointer)value);
+
+    prev_val = isa_ext_is_enabled(cpu, multi_ext_cfg->offset);
+
+    if (value == prev_val) {
+        return;
+    }
+
+    if (value && !generic_cpu) {
+        g_autofree char *cpuname = riscv_cpu_get_name(cpu);
+        error_setg(errp, "'%s' CPU does not allow enabling extensions",
+                   cpuname);
+        return;
+    }
+
+    isa_ext_update_enabled(cpu, multi_ext_cfg->offset, value);
 }
 
 static void cpu_get_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
@@ -739,11 +776,17 @@ static void cpu_get_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
 static void cpu_add_multi_ext_prop(Object *cpu_obj,
                                    const RISCVCPUMultiExtConfig *multi_cfg)
 {
+    bool generic_cpu = riscv_cpu_is_generic(cpu_obj);
+
     object_property_add(cpu_obj, multi_cfg->name, "bool",
                         cpu_get_multi_ext_cfg,
                         cpu_set_multi_ext_cfg,
                         NULL, (void *)multi_cfg);
 
+    if (!generic_cpu) {
+        return;
+    }
+
     /*
      * Set def val directly instead of using
      * object_property_set_bool() to save the set()
@@ -828,20 +871,13 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
     return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
 }
 
-static bool riscv_cpu_has_user_properties(Object *cpu_obj)
-{
-    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
-}
-
 static void tcg_cpu_instance_init(CPUState *cs)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
     Object *obj = OBJECT(cpu);
 
-    if (riscv_cpu_has_user_properties(obj)) {
-        multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
-        riscv_cpu_add_user_properties(obj);
-    }
+    multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
+    riscv_cpu_add_user_properties(obj);
 
     if (riscv_cpu_has_max_extensions(obj)) {
         riscv_init_max_cpu_extensions(obj);
-- 
2.41.0

Re: [PATCH 2/2] target/riscv/tcg-cpu.c: add extension properties for all cpus

[Alistair Francis]

On Wed, Sep 27, 2023 at 4:32 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> At this moment we do not expose extension properties for vendor CPUs
> because that would allow users to change them via command line. The
> drawback is that if we were to add an API that shows all CPU properties,
> e.g. qmp-query-cpu-model-expansion, we won't be able to show extensions
> state of vendor CPUs.
>
> We have the required machinery to create extension properties for vendor
> CPUs while not allowing users to enable extensions. Disabling existing
> extensions is allowed since it can be useful for debugging.
>
> Change the set() callback cpu_set_multi_ext_cfg() to allow enabling
> extensions only for generic CPUs. In cpu_add_multi_ext_prop() let's not
> set the default values for the properties if we're not dealing with
> generic CPUs, otherwise the values set in cpu_init() of vendor CPUs will
> be overwritten. And finally, in tcg_cpu_instance_init(), add cpu user
> properties for all CPUs.
>
> For the veyron-v1 CPU, we're now able to disable existing extensions
> like smstateen:
>
> $ ./build/qemu-system-riscv64 --nographic -M virt \
>     -cpu veyron-v1,smstateen=false
>
> But setting extensions that the CPU didn't set during cpu_init(), like
> V, is not allowed:
>
> $ ./build/qemu-system-riscv64 --nographic -M virt \
>     -cpu veyron-v1,v=true
> qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.v=true:
>   'veyron-v1' CPU does not allow enabling extensions
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/tcg/tcg-cpu.c | 64 +++++++++++++++++++++++++++++---------
>  1 file changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index f31aa9bcc4..a90ee63b06 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -549,6 +549,11 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>      riscv_cpu_disable_priv_spec_isa_exts(cpu);
>  }
>
> +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:
>   *
> @@ -632,13 +637,27 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, const char *name,
>      target_ulong misa_bit = misa_ext_cfg->misa_bit;
>      RISCVCPU *cpu = RISCV_CPU(obj);
>      CPURISCVState *env = &cpu->env;
> -    bool value;
> +    bool generic_cpu = riscv_cpu_is_generic(obj);
> +    bool prev_val, value;
>
>      if (!visit_type_bool(v, name, &value, errp)) {
>          return;
>      }
>
> +    prev_val = env->misa_ext & misa_bit;
> +
> +    if (value == prev_val) {
> +        return;
> +    }
> +
>      if (value) {
> +        if (!generic_cpu) {
> +            g_autofree char *cpuname = riscv_cpu_get_name(cpu);
> +            error_setg(errp, "'%s' CPU does not allow enabling extensions",
> +                       cpuname);
> +            return;
> +        }
> +
>          env->misa_ext |= misa_bit;
>          env->misa_ext_mask |= misa_bit;
>      } else {
> @@ -688,6 +707,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>   */
>  static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>  {
> +    bool use_def_vals = riscv_cpu_is_generic(cpu_obj);
>      int i;
>
>      for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) {
> @@ -706,7 +726,9 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>                              cpu_set_misa_ext_cfg,
>                              NULL, (void *)misa_cfg);
>          object_property_set_description(cpu_obj, name, desc);
> -        object_property_set_bool(cpu_obj, name, misa_cfg->enabled, NULL);
> +        if (use_def_vals) {
> +            object_property_set_bool(cpu_obj, name, misa_cfg->enabled, NULL);
> +        }
>      }
>  }
>
> @@ -714,17 +736,32 @@ static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
>                                    void *opaque, Error **errp)
>  {
>      const RISCVCPUMultiExtConfig *multi_ext_cfg = opaque;
> -    bool value;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    bool generic_cpu = riscv_cpu_is_generic(obj);
> +    bool prev_val, value;
>
>      if (!visit_type_bool(v, name, &value, errp)) {
>          return;
>      }
>
> -    isa_ext_update_enabled(RISCV_CPU(obj), multi_ext_cfg->offset, value);
> -
>      g_hash_table_insert(multi_ext_user_opts,
>                          GUINT_TO_POINTER(multi_ext_cfg->offset),
>                          (gpointer)value);
> +
> +    prev_val = isa_ext_is_enabled(cpu, multi_ext_cfg->offset);
> +
> +    if (value == prev_val) {
> +        return;
> +    }
> +
> +    if (value && !generic_cpu) {
> +        g_autofree char *cpuname = riscv_cpu_get_name(cpu);
> +        error_setg(errp, "'%s' CPU does not allow enabling extensions",
> +                   cpuname);
> +        return;
> +    }
> +
> +    isa_ext_update_enabled(cpu, multi_ext_cfg->offset, value);
>  }
>
>  static void cpu_get_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
> @@ -739,11 +776,17 @@ static void cpu_get_multi_ext_cfg(Object *obj, Visitor *v, const char *name,
>  static void cpu_add_multi_ext_prop(Object *cpu_obj,
>                                     const RISCVCPUMultiExtConfig *multi_cfg)
>  {
> +    bool generic_cpu = riscv_cpu_is_generic(cpu_obj);
> +
>      object_property_add(cpu_obj, multi_cfg->name, "bool",
>                          cpu_get_multi_ext_cfg,
>                          cpu_set_multi_ext_cfg,
>                          NULL, (void *)multi_cfg);
>
> +    if (!generic_cpu) {
> +        return;
> +    }
> +
>      /*
>       * Set def val directly instead of using
>       * object_property_set_bool() to save the set()
> @@ -828,20 +871,13 @@ static bool riscv_cpu_has_max_extensions(Object *cpu_obj)
>      return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL;
>  }
>
> -static bool riscv_cpu_has_user_properties(Object *cpu_obj)
> -{
> -    return object_dynamic_cast(cpu_obj, TYPE_RISCV_DYNAMIC_CPU) != NULL;
> -}
> -
>  static void tcg_cpu_instance_init(CPUState *cs)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      Object *obj = OBJECT(cpu);
>
> -    if (riscv_cpu_has_user_properties(obj)) {
> -        multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
> -        riscv_cpu_add_user_properties(obj);
> -    }
> +    multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
> +    riscv_cpu_add_user_properties(obj);
>
>      if (riscv_cpu_has_max_extensions(obj)) {
>          riscv_init_max_cpu_extensions(obj);
> --
> 2.41.0
>
>

Re: [PATCH 2/2] target/riscv/tcg-cpu.c: add extension properties for all cpus

[Daniel P. Berrangé]

On Tue, Sep 26, 2023 at 03:31:09PM -0300, Daniel Henrique Barboza wrote:
> At this moment we do not expose extension properties for vendor CPUs
> because that would allow users to change them via command line. The
> drawback is that if we were to add an API that shows all CPU properties,
> e.g. qmp-query-cpu-model-expansion, we won't be able to show extensions
> state of vendor CPUs.
> 
> We have the required machinery to create extension properties for vendor
> CPUs while not allowing users to enable extensions. Disabling existing
> extensions is allowed since it can be useful for debugging.
> 
> Change the set() callback cpu_set_multi_ext_cfg() to allow enabling
> extensions only for generic CPUs. In cpu_add_multi_ext_prop() let's not
> set the default values for the properties if we're not dealing with
> generic CPUs, otherwise the values set in cpu_init() of vendor CPUs will
> be overwritten. And finally, in tcg_cpu_instance_init(), add cpu user
> properties for all CPUs.
> 
> For the veyron-v1 CPU, we're now able to disable existing extensions
> like smstateen:
> 
> $ ./build/qemu-system-riscv64 --nographic -M virt \
>     -cpu veyron-v1,smstateen=false
> 
> But setting extensions that the CPU didn't set during cpu_init(), like
> V, is not allowed:
> 
> $ ./build/qemu-system-riscv64 --nographic -M virt \
>     -cpu veyron-v1,v=true
> qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.v=true:
>   'veyron-v1' CPU does not allow enabling extensions

Why should we block the user if they want to enable an extra
feature, over and above what is built-in to the CPU model ?
Is there some technical reason that prevents this from working ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] target/riscv/tcg-cpu.c: add extension properties for all cpus

[Alistair Francis]

On Fri, Sep 29, 2023 at 8:39 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Sep 26, 2023 at 03:31:09PM -0300, Daniel Henrique Barboza wrote:
> > At this moment we do not expose extension properties for vendor CPUs
> > because that would allow users to change them via command line. The
> > drawback is that if we were to add an API that shows all CPU properties,
> > e.g. qmp-query-cpu-model-expansion, we won't be able to show extensions
> > state of vendor CPUs.
> >
> > We have the required machinery to create extension properties for vendor
> > CPUs while not allowing users to enable extensions. Disabling existing
> > extensions is allowed since it can be useful for debugging.
> >
> > Change the set() callback cpu_set_multi_ext_cfg() to allow enabling
> > extensions only for generic CPUs. In cpu_add_multi_ext_prop() let's not
> > set the default values for the properties if we're not dealing with
> > generic CPUs, otherwise the values set in cpu_init() of vendor CPUs will
> > be overwritten. And finally, in tcg_cpu_instance_init(), add cpu user
> > properties for all CPUs.
> >
> > For the veyron-v1 CPU, we're now able to disable existing extensions
> > like smstateen:
> >
> > $ ./build/qemu-system-riscv64 --nographic -M virt \
> >     -cpu veyron-v1,smstateen=false
> >
> > But setting extensions that the CPU didn't set during cpu_init(), like
> > V, is not allowed:
> >
> > $ ./build/qemu-system-riscv64 --nographic -M virt \
> >     -cpu veyron-v1,v=true
> > qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.v=true:
> >   'veyron-v1' CPU does not allow enabling extensions
>
> Why should we block the user if they want to enable an extra
> feature, over and above what is built-in to the CPU model ?

It ends up being tricky to maintain. On top of that we can report a
specific vendor CPU to guests, but then we don't correctly model it
(as extensions might be disabled).

Alistair

> Is there some technical reason that prevents this from working ?
>
> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
>

Re: [PATCH 0/2] riscv: add extension properties for all cpus

[Alistair Francis]

On Wed, Sep 27, 2023 at 4:32 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> At this moment we do not expose extension properties for vendor CPUs
> because that would allow users to enable extensions in them. But that
> comes at a cost: if we were to add an API that shows all CPU properties,
> e.g. qmp-query-cpu-model-expansion, we won't be able to show the
> extension state of vendor CPUs.
>
> We're in a good spot to revisit this decision. We have the required
> abstractions in place to be able to expose user properties for vendor
> CPUs and, at the same time, forbid users to enable extensions for those
> CPUs. As a bonus, we'll allow users to be able to disable extensions for
> vendor CPUs, which can be useful for testing/debugging.
>
> Patches based on Alistair's riscv-to-apply.next.
>
> Daniel Henrique Barboza (2):
>   target/riscv: add riscv_cpu_get_name()
>   target/riscv/tcg-cpu.c: add extension properties for all cpus

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu.c         | 11 ++++++
>  target/riscv/cpu.h         |  1 +
>  target/riscv/tcg/tcg-cpu.c | 68 +++++++++++++++++++++++++++++---------
>  3 files changed, 65 insertions(+), 15 deletions(-)
>
> --
> 2.41.0
>
>