Re: [PATCH v2 02/25] KVM: arm64: Add feature checking helpers

[Marc Zyngier <maz@kernel.org>]

On Fri, 02 Feb 2024 17:13:07 +0000,
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
> Hi Marc,
> 
> On 30/01/2024 20:45, Marc Zyngier wrote:
> > In order to make it easier to check whether a particular feature
> > is exposed to a guest, add a new set of helpers, with kvm_has_feat()
> > being the most useful.
> > 
> > Let's start making use of them in the PMU code (courtesy of Oliver).
> > Follow-up work will intricude additional use patterns.
> 
> I think there is a bit of inconsistency in the macros for signed
> and unsigned. The unsigned fields are extracted (i.e., as if they
> were shifted to bit 0). But the signed fields are not shifted
> completely to bit '0' (in fact to different positions) and eventually
> we compare wrong things.
> 
> Using ID_AA64PFR0_EL1, fld=EL2, val=IMP for unsigned and
> ID_AA64PFR0_EL1, AdvSIMD, NI for signed.
> 
> > 
> > Co-developed--by: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >   arch/arm64/include/asm/kvm_host.h | 53 +++++++++++++++++++++++++++++++
> >   arch/arm64/kvm/pmu-emul.c         | 11 ++++---
> >   arch/arm64/kvm/sys_regs.c         |  6 ++--
> >   include/kvm/arm_pmu.h             | 11 -------
> >   4 files changed, 61 insertions(+), 20 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 21c57b812569..c0cf9c5f5e8d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1233,4 +1233,57 @@ static inline void kvm_hyp_reserve(void) { }
> >   void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);
> >   bool kvm_arm_vcpu_stopped(struct kvm_vcpu *vcpu);
> >   +#define __expand_field_sign_unsigned(id, fld, val)
> > \
> > +	((u64)(id##_##fld##_##val))
> 
> For unsigned features we get the actual "field" value, not the value
> in position. e.g,: ID_AA64PFR0_EL1_EL2_IMP = (0x1)
> 
> > +
> > +#define __expand_field_sign_signed(id, fld, val)			\
> > +	({								\
> > +		s64 __val = id##_##fld##_##val;				\
> > +		__val <<= 64 - id##_##fld##_WIDTH;			\
> > +		__val >>= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
> 
> But for signed fields, we shift them back into the "position" as in
> the ID_REG. e.g.,
> 
> ID_AA64PFR0_EL1, AdvSIMD, NI we get:
> 
> 	__val = ID_AA64PFR0_EL1_AdvSIMD_NI; /* = 0xf */
> 	__val <<= 64 - 4;		/* 0xf0_00_00_00_00_00_00_00 */
> 	__val >>= 64 - 20 - 4;		/* 0xff_ff_ff_ff_ff_f0_00_00 */
> 
> I think the last line instead should be:
> 	__val >>= 64 - id##_##fld##_WIDTH;

Huh, you're absolutely right.

> 
> > +									\
> > +		__val;							\
> > +	})
> > +
> > +#define expand_field_sign(id, fld, val)					\
> > +	(id##_##fld##_SIGNED ?						\
> > +	 __expand_field_sign_signed(id, fld, val) :			\
> > +	 __expand_field_sign_unsigned(id, fld, val))
> > +
> > +#define get_idreg_field_unsigned(kvm, id, fld)				\
> > +	({								\
> > +		u64 __val = IDREG(kvm, SYS_##id);			\
> > +		__val &= id##_##fld##_MASK;				\
> > +		__val >>= id##_##fld##_SHIFT;				\
> > +									\
> 
> We extract the field value for unsigned field, i.e., shifted to bit"0"
> and that matches the expand_field_sign().
> 
> > +		__val;							\
> > +	})
> > +
> > +#define get_idreg_field_signed(kvm, id, fld)				\
> > +	({								\
> > +		s64 __val = IDREG(kvm, SYS_##id);			\
> > +		__val <<= 64 - id##_##fld##_SHIFT - id##_##fld##_WIDTH;	\
> > +		__val >>= id##_##fld##_SHIFT;				\
> 
> However, here we get (assuming value ID_AA64PFR0_EL1_ASIMD = 0xf, and
> all other fields are 0 for clarity)
> 	
> 	__val = IDREG(kvm, SYS_ID_AA64PFR0_EL1); = 0xf0_00_00; 	/* 0xf << 20 */
> 	__val <<= 64 - 20 - 4;	/* = 0xf0_00_00_00_00_00_00_00 */
> 	__val >>= 20;		/* = 0xff_ff_ff_00_00_00_00_00 */

Gah... again!

>
> Thus they don;t match. Instead the last line should be :
> 
> 	__val >>= id##_##fld##_WIDTH;

Shouldn't this be (64 - WIDTH) instead, since we want the value to be
shifted to bit 0? Otherwise, you get 0xff_00_00_00_00_00_00_00 (as per
your example).

Thanks a lot for spotting those, much appreciated.

	M.

-- 
Without deviation from the norm, progress is not possible.