Re: [PATCH v4 31/43] KVM: arm64: Switch to table-driven FGU configuration

[Marc Zyngier <maz@kernel.org>]

On Thu, 08 May 2025 16:58:28 +0100,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Tue, May 06, 2025 at 05:43:36PM +0100, Marc Zyngier wrote:
> > Defining the FGU behaviour is extremely tedious. It relies on matching
> > each set of bits from FGT registers with am architectural feature, and
> > adding them to the FGU list if the corresponding feature isn't advertised
> > to the guest.
> > 
> > It is however relatively easy to dump most of that information from
> > the architecture JSON description, and use that to control the FGU bits.
> > 
> > Let's introduce a new set of tables descripbing the mapping between
> > FGT bits and features. Most of the time, this is only a lookup in
> > an idreg field, with a few more complex exceptions.
> > 
> > While this is obviously many more lines in a new file, this is
> > mostly generated, and is pretty easy to maintain.
> 
> I didn't review every single value in the maps (autogenerated as you said), but
> I did take a look at a few.
> 
> __compute_fixed_bits() is pretty confusing, it's doing multiple things:
>   - It returns a mask of bits that didn't match (aka RES0)
>   - or it returns a mask of bits that may have a fixed value and fills out a
>     pointer with what the fixed values are.
> 
> It has the __ prefix, so it's an implemetation-detail kinda function, not
> asking you to change anything, just pointing out the most confusing part of the
> patch!

I know. My OCDing self couldn't deal with having two helpers differing
only by the position of a single bit, hence the catch-all helper. But
I'm happy to revisit this in the future to make it more palatable.

It's just that I'm getting a bit fed-up with this series... :-/

> 
> Just one small whitespace nit below.
> 
> Reviewed-by: Joey Gouly <joey.gouly@arm.com>

Thank you!

> > +	NEEDS_FEAT(HFGRTR_EL2_ERXPFGCDN_EL1|
> > +		   HFGRTR_EL2_ERXPFGCTL_EL1|
> 
> Inconsistent |.

Ah, nice. Fixed.

> > +	NEEDS_FEAT_FLAG(HDFGRTR_EL2_OSDLR_EL1, NEVER_FGU,
> > +			FEAT_DoubleLock),
> 
> This confused me at first, but it's because OSDLR_EL1 is always accessible
> (with FEAT_AA64EL1), but can only be trapped with FEAT_DoubleLock.

Exactly. This is the conjunction of two architectural constraints
(selective output from my script):

- OSDLR_EL1: # Reg cond: IsFeatureImplemented(FEAT_AA64)
- HDFGRTR_EL2.OSDLR_EL1: # Field cond: IsFeatureImplemented(FEAT_DoubleLock)

The first implies that this can never UNDEF, while the second
indicates that the trap bit only exists under specific conditions.
It's one of these cases where the architecture is playing catch-up
with itself (v8.0 vs v8.6).

Thanks again,

	M.

-- 
Jazz isn't dead. It just smells funny.