Re: [PATCH v6 06/12] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/28/23 05:54, Daniel Henrique Barboza wrote:
> The TCG emulation implements all the extensions described in the
> RVA22U64 profile, both mandatory and optional. The mandatory extensions
> will be enabled via the profile flag. We'll leave the optional
> extensions to be enabled by hand.
>
> Given that this is the first profile we're implementing in TCG we'll
> need some ground work first:
>
> - all profiles declared in riscv_profiles[] will be exposed to users.
> TCG is the main accelerator we're considering when adding profile
> support in QEMU, so for now it's safe to assume that all profiles in
> riscv_profiles[] will be relevant to TCG;
>
> - we'll not support user profile settings for vendor CPUs. The flags
> will still be exposed but users won't be able to change them. The idea
> is that vendor CPUs in the future can enable profiles internally in
> their cpu_init() functions, showing to the external world that the CPU
> supports a certain profile. But users won't be able to enable/disable
> it;
>
> - Setting a profile to 'true' means 'enable all mandatory extensions of
> this profile, setting it to 'false' means 'do not enable all mandatory
> extensions for this profile'. This is the same semantics used by RVG.
> Regular left-to-right option order will determine the resulting CPU
> configuration, i.e. the following QEMU command line:
>
> -cpu rv64,zicbom=false,zifencei=false,rva22u64=true
>
> Enables all rva22u64 mandatory extensions, including 'zicbom' and
> 'zifencei', while this other command line:
>
> -cpu rv64,rva22u64=true,zicbom=false,zifencei=false
>
> Enables all mandatory rva22u64 extensions, and then disable both zicbom
> and zifencei.
>
> For now we'll handle multi-letter extensions only. MISA extensions need
> additional steps that we'll take care later.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>   target/riscv/tcg/tcg-cpu.c | 63 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 65d59bc984..5fdec8f04e 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -783,6 +783,67 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
>       }
>   }
>   
> +static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    RISCVCPUProfile *profile = opaque;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +    bool value;
> +    int i, ext_offset;
> +
> +    if (object_dynamic_cast(obj, TYPE_RISCV_DYNAMIC_CPU) == NULL) {
> +        error_setg(errp, "Profile %s only available for generic CPUs",
> +                   profile->name);
> +        return;
> +    }
> +
> +    if (cpu->env.misa_mxl != MXL_RV64) {
> +        error_setg(errp, "Profile %s only available for 64 bit CPUs",
> +                   profile->name);
> +        return;
> +    }
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    profile->user_set = true;
> +    profile->enabled = value;
> +
> +    if (!profile->enabled) {
> +        return;
> +    }

My idea here was to prevent the following from disabling default rv64
extensions:

-cpu rv64,rva22u64=false

And yeah, this is a niche (I'll refrain from using the word I really wanted) use
of the profile extension, but we should keep in mind that setting a default 
'false'
option to 'false' shouldn't make changes in the CPU.

But now if I do this:

-cpu rv64,rva22u64=true (...) -cpu rv64,rva22u64=false

This will enable the profile in rv64 regardless of setting it to 'false' later
on. Which is also a weird mechanic. One way of solving this would be to postpone
profile enable/disable to realize()/finalize(), but then we're back to the 
problem
we had in v2 where, for multiple profiles, we can't tell the order in which the
user enabled/disabled them in the command line during realize().

Let me think a bit about it.


Daniel

> +
> +    for (i = 0; profile->ext_offsets[i] != RISCV_PROFILE_EXT_LIST_END; i++) {
> +        ext_offset = profile->ext_offsets[i];
> +
> +        g_hash_table_insert(multi_ext_user_opts,
> +                            GUINT_TO_POINTER(ext_offset),
> +                            (gpointer)profile->enabled);
> +        isa_ext_update_enabled(cpu, ext_offset, profile->enabled);
> +    }
> +}
> +
> +static void cpu_get_profile(Object *obj, Visitor *v, const char *name,
> +                            void *opaque, Error **errp)
> +{
> +    RISCVCPUProfile *profile = opaque;
> +    bool value = profile->enabled;
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void riscv_cpu_add_profiles(Object *cpu_obj)
> +{
> +    for (int i = 0; riscv_profiles[i] != NULL; i++) {
> +        const RISCVCPUProfile *profile = riscv_profiles[i];
> +
> +        object_property_add(cpu_obj, profile->name, "bool",
> +                            cpu_get_profile, cpu_set_profile,
> +                            NULL, (void *)profile);
> +    }
> +}
> +
>   static bool cpu_ext_is_deprecated(const char *ext_name)
>   {
>       return isupper(ext_name[0]);
> @@ -906,6 +967,8 @@ static void riscv_cpu_add_user_properties(Object *obj)
>   
>       riscv_cpu_add_multiext_prop_array(obj, riscv_cpu_deprecated_exts);
>   
> +    riscv_cpu_add_profiles(obj);
> +
>       for (Property *prop = riscv_cpu_options; prop && prop->name; prop++) {
>           qdev_property_add_static(DEVICE(obj), prop);
>       }