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
