Re: [PATCH v2 07/14] KVM: arm64: Use a cpucap to determine if system supports FEAT_PMUv3

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Feb 19, 2025 at 05:44:59PM +0000, Marc Zyngier wrote:
> > +static bool has_pmuv3(const struct arm64_cpu_capabilities *entry, int scope)
> > +{
> > +	u64 dfr0 = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> > +	unsigned int pmuver;
> > +
> > +	pmuver = cpuid_feature_extract_unsigned_field(dfr0,
> > +						      ID_AA64DFR0_EL1_PMUVer_SHIFT);
> > +	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> > +		return false;
> > +
> > +	return pmuver >= ID_AA64DFR0_EL1_PMUVer_IMP;
> 
> Given that PMUVer is a signed field, how about using
> cpuid_feature_extract_signed_field() and do a signed comparison instead?

I'm happy to include a comment, but the PMUVer field is not signed. Any value
other than 0xF is meant to be treated as an unsigned quantity.

DDI047L.a D24.1.3.2 is where this is coming from.

> > +}
> > +
> >  #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
> >  #define KPTI_NG_TEMP_VA		(-(1UL << PMD_SHIFT))
> >  
> > @@ -2999,6 +3012,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> >  		ARM64_CPUID_FIELDS(ID_AA64PFR1_EL1, GCS, IMP)
> >  	},
> >  #endif
> > +	{
> > +		.desc = "PMUv3",
> > +		.capability = ARM64_HAS_PMUV3,
> > +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > +		.matches = has_pmuv3,
> > +	},
> 
> This cap is probed unconditionally (without any configuration
> dependency)...
> 
> >  	{},
> >  };
> >  
> > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > index f838a45665f2..0edc7882bedb 100644
> > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > @@ -244,7 +244,7 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
> >  	 * counter, which could make a PMXEVCNTR_EL0 access UNDEF at
> >  	 * EL1 instead of being trapped to EL2.
> >  	 */
> > -	if (kvm_arm_support_pmu_v3()) {
> > +	if (system_supports_pmuv3()) {
> 
> ... but kvm_arm_support_pmu_v3() is conditional on
> CONFIG_HW_PERF_EVENTS.  Doesn't this create some sort of new code path
> that we didn't expect?

Yep. It ought to be benign, but pointless. I'll condition this correctly
next time around.

Thanks,
Oliver