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

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

On Sat, Oct 28, 2023 at 05:54:21AM -0300, 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'.

While this definition of profile=off looks appealing at first, it's really
just saying 'do not do anything', which means it's limiting the potential
of the property. But, I'll stop talking about this now, as I have another
design suggestion instead:

Since profiles are like G, and other shorthand extensions (just without an
official shorthand extension name), then I believe they should behave like
shorthand extensions. Also, since shorthand extensions can be derived from
their parts, then I think shorthand extensions should behave like
synthetic extensions. For example, zic64b starts 'false', but, at realize
time, if all its conditions are present, then it turns 'true'. Shorthand
extensions should be able to do that too by detecting that each of the
extensions they represent is present.

So, I think we should automatically turn G and profiles (and future
shorthand extensions) on when all their respective extensions are present.

Thanks,
drew

On Sat, Oct 28, 2023 at 8:44 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Sat, Oct 28, 2023 at 05:54:21AM -0300, 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'.
>
> While this definition of profile=off looks appealing at first, it's really
> just saying 'do not do anything', which means it's limiting the potential
> of the property. But, I'll stop talking about this now, as I have another

This seems like the way to go to me

> design suggestion instead:
>
> Since profiles are like G, and other shorthand extensions (just without an
> official shorthand extension name), then I believe they should behave like
> shorthand extensions. Also, since shorthand extensions can be derived from
> their parts, then I think shorthand extensions should behave like
> synthetic extensions. For example, zic64b starts 'false', but, at realize
> time, if all its conditions are present, then it turns 'true'. Shorthand
> extensions should be able to do that too by detecting that each of the
> extensions they represent is present.
>
> So, I think we should automatically turn G and profiles (and future
> shorthand extensions) on when all their respective extensions are present.

I think that's a good idea and something we should support

Alistair

>
> Thanks,
> drew
>

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);
>       }

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

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.

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.

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,
but the feature is designed around rv64i.

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.


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);
>>       }

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);
> >>       }
>

On 11/2/23 00:00, Alistair Francis wrote:
> 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?

For these profiles we would block them from running into RV64I because it's not
the right base ISA. We should add a RV64E CPU for them.

Same thing with any 32 bit profiles that we might add in the future - rva22u64
isn't compatible with any 32 bits ATM.

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

It doesn't, because it's an user problem. We're wasting too much time trying to
sandbag weird command lines. "-cpu rv64,rva22u64=false" is a nonsense command
line that will give a nonsense CPU.

A failsafe solution I see for it is to forbid profile support for rv64. But this
is too extreme because "-cpu rv64,rva22u64=true" is a valid use, and a way more
common use, that would be cropped out because we want to babysit a minority of
users doing nonsense.

IMO we should let users use profiles with any capable CPU and document properly
what it means to do stuff like "-cpu rv64,rva22u64=false".

And now that I thought more about it, there's no gain for the 'max' CPU to support
profiles (all extensions enabled by default) but -cpu max,rva22u64=false will still
be possible. I guess I'll remove profile support for 'max' CPU.

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

We can make aliases. '-cpu rva22u64' would be an alias for '-cpu rv64i,rva22u64=true'.
Same thing with every other profile we end up adding.


Thanks,


Daniel

> 
>>
>> 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);
>>>>        }
>>