Re: [PATCH v3 3/6] target/riscv/tcg: add user flag for profile support

[Daniel Henrique Barboza]

On 10/23/23 05:16, Andrew Jones wrote:
> On Fri, Oct 20, 2023 at 07:39:48PM -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 disabling all its mandatory
> > extensions. Disabling a profile is discouraged for regular use and will
> > issue an user warning. User choices for individual extensions will take
> > precedence, i.e. enabling a profile will not enable extensions that the
> > user set to 'false', and vice-versa. This will make us independent of
> > left-to-right ordering in the QEMU command line, i.e. the following QEMU
> > command lines:
> >
> > -cpu rv64,zicbom=false,rva22u64=true,Zifencei=false
> > -cpu rv64,zicbom=false,Zifencei=false,rva22u64=true
> > -cpu rv64,rva22u64=true,zicbom=false,Zifencei=false
> >
> > They mean the same thing: "enable all mandatory extensions of the
> > rva22u64 profile while keeping zicbom and Zifencei disabled".
>
> Hmm, I'm not sure I agree with special-casing profiles like this. I think
> the left-to-right processing should be consistent for all. I'm also not
> sure we should always warn when disabling a profile. For example, if a
> user does
>
>   -cpu rv64,rva22u64=true,rva22u64=false
>
> then they'll get a warning, even though all they're doing is restoring the
> cpu model. While that looks like an odd thing to do, a script may be
> adding the rva22u64=true and the rva22u64=false is the user input which
> undoes what the script did.

QEMU options do not work with a "the user enabled then disabled the same option,
thus it'll count as nothing happened" logic. The last instance of the option 
will
overwrite all previous instances. In the example you mentioned above the user 
would
disable all mandatory extensions of rva22u64 in the CPU, doesn't matter if the
same profile was enabled beforehand.

Sure, the can put code in place to make this happen, but then this would make
profiles act different than regular extensions. "-cpu rv64,zicbom=true -cpu 
rv64,zicbom=false"
will disable zicbom, it will not preserve the original 'zicbom' rv64 default. If
we're going to keep left-to-right ordering consistent, this behavior should also
be consistent as well.


As for warnings, I agree that we'll throw warnings even when nothing of notice 
happened.
For example:

-cpu rv64,rva22u64=false -cpu rv64,rva22u64=true

This will throw a warning even though the user ended up enabling the extension
in the end.


We can fix it by postponing warnings to realize().


> As far as warnings go, it'd be nice to warn when mandatory profile
> extensions are disabled from an enabled profile. Doing that might be
> useful for debug, but users which do it without being aware they're
> "breaking" the profile may learn from that warning. Note, the warning
> should only come when the profile is actually enabled and when the
> extension would actually be disabled, i.e.
>
>   -cpu rv64,rva22u64=true,c=off
>
> should warn
>
>   -cpu rv64,c=off,rva22u64=true
>
> should not warn (rva22u64 overrides c=off since it's to the right)
>
>   -cpu rv64,rva22u64=true,rva22u64=false,c=off
>
> should not warn (rva22u64 is not enabled)

Ack for all the above.

> And,
>
>   -cpu rv64,rva22u64=true,rva24u64=false
>
> should warn for each extension which is mandatory in both profiles.

The way I'm imagining this happening is to cycle through all profiles during 
realize(),
see which ones are enabled, and then warn if the user disabled their mandatory
extensions. In this example we would warn for all rva22 mandatory extensions
that were disabled because we disabled rva24, but we won't emit any warnings for
rva24 mandatory extensions given that the profile is marked as disabled.



Thanks,

Daniel


> Thanks,
> drew