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

[Andrew Jones]

On Fri, Oct 27, 2023 at 02:52:38PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> 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.

If we provided a mechanism to query the state of profiles, then they
should be 'true' when all their mandatory extensions are present. That
means that, despite defaulting to 'false', they should become true at
realize time after detecting all their mandatory extensions are present,
and then, if a user set the profile false, the user would be disabling
all the profile's extensions along with the profile.

Without a mechanism to query profiles, then it doesn't matter if they
remain false after realize, even if they should be true, but that's just
a coding decision (which probably deserves a code comment).

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

I would disable the extensions that G represents when G is explicitly
disabled, just as I suggest we do for profiles.

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

More extension shorthands are coming[1] and, IMO, they should be addressed
in the same way as G (whatever the final decision on how to address G is).
Since G collects such basic extensions, then it might be hard to reason as
to why it should be possible to disable it, but if, for example, you have
a CPU which supports the Zvkn extensions and you want to test/debug with
all of those extensions disabled, then it'd be nice to do zvkn=off and
expect all of them to be disabled.

[1] 20231026151828.754279-1-max.chou@sifive.com/">https://lore.kernel.org/all/20231026151828.754279-1-max.chou@sifive.com/

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

I've already pointed out a couple times that disabling a profile may be
useful for scripts, particularly for bare-bones CPUs, since you know that
disabling something previously enabled is implementing a clean 'undo'.

> 
> After writing all this stuff, and realizing that profile disablement creates a
> lot of confusion

I'm having trouble imaging this hypothetical user that knows how to
construction QEMU command lines, knows what profiles are meant to enable,
knows how boolean properties work, but would be surprised that
'profile=off' disabled the profile's mandatory extensions and not
surprised when 'profile=off' does nothing. So, for me, 'profile=off' doing
nothing looks like it'll introduce confusion, not the other way around.

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

I presume the user disabled extensions would need to come to the right of
the profiles on the command line to be respected.

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

Doesn't this just kick the can down the road? I suspect we'd end up
discussing what 'bare=false' means at that time.

Thanks,
drew