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

[Daniel Henrique Barboza]

On 10/26/23 14:36, Andrea Bolognani wrote:
> 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.

I'm not thrilled about how we're able to disable profiles either. I'm
coping with it because (1) it was a feedback from the first version of
this work [1] and no one had strong opinions against it back then and
(2) I believe that users won't find much use in doing "-cpu rv64,profileA=false"
in a real world/common scenario, so we can get away with this kind of
weird functionality.

The profile flag is set to 'false' by default for all current CPUs. If
the user manually sets it to 'false', well, it doesn't change the internal
state of the CPU, does it? But then I need to be creative and interpret it
as 'it's not a default false, it's an user-set false, so I need to disable
extensions'. I can't think of many qemu options that behave like that, if
any.

We also have the example of RVG, a bit that is default set to 'false' that,
when enabled, causes IMAFD_zicsr_zifencei to be enabled. Today, if the user
set RVG to 'false', nothing happens - we're not disabling IMAFD_zicsr_zifencei.
In the latest version of this work there's a deliberate effort to make RVG
behave like a profile [2], but perhaps I should make profiles behave like RVG.

Last but not the least, I'm planning to add a couple of bare-bones CPUs (rv64i
and rv64e). Disabling profiles in these CPUs is a total waste of cycles since
the CPUs are already bare.

After writing all this stuff, and realizing that profile disablement creates a
lot of confusion and has no vocal fans, I had a change of heart. Profiles will
behave like RVG -> if set, mandatory extensions will be enabled (respecting user
choice on disabled extensions, of course). If disabled, nothing happens. Fans
of the current design are welcome to weight in the discussion, of course.

If we decide in the future that stripping extensions from a CPU model is 
desirable
we can come up with a 'bare' option, e.g. "-cpu rv64,bare=true" will strip all
extensions from rv64. This is a much cleaner way of doing what profile 
disablement
is currently doing.


Thanks,


Daniel


[1] https://lore.kernel.org/qemu-riscv/ZRarBuEeBi7WkS6K@redhat.com/
[2] 
20231025234459.581697-10-dbarboza@ventanamicro.com/">https://lore.kernel.org/qemu-riscv/20231025234459.581697-10-dbarboza@ventanamicro.com/






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