linux-kernel - Re: [PATCH v2 07/14] KVM: arm64: Use a cpucap to determine if system supports FEAT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7YvkHpm3w__HnkO@linux.dev>
Date: Wed, 19 Feb 2025 11:22:56 -0800
From: Oliver Upton <oliver.upton@...ux.dev>
To: Marc Zyngier <maz@...nel.org>
Cc: kvmarm@...ts.linux.dev, Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Mingwei Zhang <mizhang@...gle.com>,
	Colton Lewis <coltonlewis@...gle.com>,
	Raghavendra Rao Ananta <rananta@...gle.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Janne Grunau <j@...nau.net>
Subject: Re: [PATCH v2 07/14] KVM: arm64: Use a cpucap to determine if system
 supports FEAT_PMUv3

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