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

[Andrew Jones]

On Thu, Oct 26, 2023 at 07:36:21AM -0700, Andrea Bolognani wrote:
> On Mon, Oct 23, 2023 at 07:35:16PM +0200, Andrew Jones wrote:
> > On Mon, Oct 23, 2023 at 02:00:00PM -0300, Daniel Henrique Barboza wrote:
> > > On 10/23/23 05:16, Andrew Jones wrote:
> > > > 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.
> >
> > Yup, I'm aware, but I keep thinking that we'll only be using profiles with
> > a base cpu type. If you start with nothing (a base) and then add a profile
> > and take the same one away, you shouldn't be taking away anything else. I
> > agree that if you use a profile on some cpu type that already enabled a
> > bunch of stuff itself, then disabling a profile would potentially remove
> > some of those too, but mixing cpu types that have their own extensions and
> > profiles seems like a great way to confuse oneself as to what extensions
> > will be present.  IOW, we should be adding a base cpu type at the same
> > time we're adding these profiles.
> 
> The question that keep bouncing around my head is: why would we even
> allow disabling profiles?
> 
> It seems to me that it only makes things more complicated, and I
> really can't see the use case for it.
> 
> Enabling additional features on top of a profile? There's obvious
> value in that, so that you can model hardware that implements
> optional and proprietary extensions. Enabling multiple profiles?
> You've convinced me that it's useful. But disabling profiles, I just
> don't see it. I believe Alistair was similarly unconvinced.

The only value I see in allowing a profile to be disabled is to undo the
enabling of the profile by specifying the profile as 'off' to the right of
it being specified as 'on'. That may seem pointless, but scripts take
advantage of being able to do that. Besides that one possible use case,
there isn't much use in disabling profiles, but treating profile
properties like every other boolean property makes the UI consistent and
should actually simplify the code.

> 
> > > > 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)
> 
> I think these should be hard errors, not warnings.
> 
> If you're enabling a profile and then disabling an extension that's
> mandatory for that profile, you've invalidated the profile. You've
> asked for a configuration that doesn't make any sense: you can't have
> a CPU that both implements a profile and lacks one of its mandatory
> extensions.
> 

Given a platform which implements a profile which mandates extension E and
a need to debug E or test behavior where E is [incorrectly] absent, you'll
need to expand the profile first, listing each of the other extensions
manually. It'd be much faster to specify the profile, take away the
extension, and ignore the warning.

> QEMU users could easily miss the warning. libvirt users won't see it
> at all. It's a user error and it needs to be treated as such IMO.

I do agree with the concern that warnings will be missed/ignored. Maybe
QEMU needs something like -Werror for stuff like this, i.e.

 -cpu rv64,error-on-extension-warnings=on,profile-A=on,extension-of-A=off

would error out, but, without the special property, just warn. Or, flip
the default behavior around with

 -cpu rv64,ignore-extension-errors=on,profile-A=on,extension-of-A=off

which would either silently proceed or just warn, but, without the
special property, error out. libvirt would default to the error out
case, whichever that one is, but also provide an element to turn off
erroring-out.

Thanks,
drew