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
>
>