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

[Andrea Bolognani]

On Thu, Oct 26, 2023 at 05:14:49PM +0200, Andrew Jones wrote:
> 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.

The code might be simpler, but the result is an additional burden on
the user, as the interactions between the various flags become much
more nuanced and less intuitive. I'm not convinced the trade-off is a
worthwhile one.

For the script override scenario, fair enough, but once again I feel
that we're making things much worse in the general case in order to
cater to a much narrower one. Script authors will naturally learn to
avoid hardcoding profile enablement once users have reported enough
failures resulting from that.

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

I understand the appeal, I just think that regular users should be
prevented from stumbling into this kind of expert-level,
intentionally-broken configuration by mistake.

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

I would be okay with something along these lines.

-- 
Andrea Bolognani / Red Hat / Virtualization