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

[Alistair Francis]

On Tue, Oct 31, 2023 at 3:19 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/30/23 10:28, Daniel Henrique Barboza wrote:
> >
> >
> > 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

I think that's the right approach

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

Urgh, that is odd.

> > 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().

Are the properties not just set in order automatically? So we end up
with the property being set by the last one?

> >
> > Let me think a bit about it.
>
> To be honest, all the debate around this feature is due to rv64 having default
> extensions and users doing non-orthodox stuff with the flag that will crop
> rv64 defaults, resulting in something that we don't know what this is.

Ok, just to summarise.

The idea is that having a CPU with nothing enabled makes it easy to
then enable features based on what extensions have been enabled. That
way if a profile is false, we can just ignore it. The result is the
features are disabled as that is the default state.

>
> And what aggravates the issue is that, ATM, we don't have any other CPU aside
> from rv64 (and max) to use profiles on.
>
> The RVA22U64 profile spec mentions: "RV64I is the mandatory base ISA for 
> RVA22U64".
> So we have a base. And we should base profiles around the base, not on a CPU 
> that
> has existing default values.

That is only a base for RVA22U64 though. Aren't there embedded
profiles that might use rv64e as a base? What about RV32 as well?

>
> I'll add the 'rv64i' CPU in v7. This will be a bare-bones CPU that will only 
> have
> RVI enabled by default. Profile support will be based on top of this CPU, 
> making
> enable/disable profiles a trivial matter since we have a fixed minimal base. 
> This
> will give users a stable way of using profiles.
>
> As long as we have the rv64i CPU I'll start not caring for what happens with
> '-cpu rv64,rva22u64=false' - users are free to use profiles in rv64 if they 
> want,

What does happen with -cpu rv64,rva22u64=false though?

I feel like this doesn't really address the problem

> but the feature is designed around rv64i.

One other thought it to create a CPU per extension. So the actual CPU
is rva22u64. That way it is easy for users to enable/disable features
on top of the extension and we don't have to worry about the complex
true/false operations for extensions

>
> I'll also throw a bone for all CPUs and add 'rva22u64' as an internal flag for
> everybody, including vendor CPUs, that will reflect whether the CPU complies 
> with
> the profile. query-cpu-model-expansion can expose this flag, allowing the 
> tooling
> to have an easier time verifying if a profile is implemented or not.

Great!

Alistair

>
>
> Thanks,
>
>
> Daniel
>
>
> >
> >
> > 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);
> >>       }
>