Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022

From: Sean Christopherson <seanjc@google.com>
To: Like Xu <like.xu.linux@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Jim Mattson <jmattson@google.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Sandipan Das <sandipan.das@amd.com>
Subject: Re: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
Date: Thu, 10 Nov 2022 17:34:21 +0000	[thread overview]
Message-ID: <Y202HcmVLa0woaCF@google.com> (raw)
In-Reply-To: <948ec6a5-3f30-e8c2-9629-12235f1e1367@gmail.com>

On Thu, Nov 10, 2022, Like Xu wrote:
> On 28/10/2022 6:37 am, Sean Christopherson wrote:
> > I'm not a fan of perf's unions, but I at least understand the value added for
> > CPUID entries that are a bunch of multi-bit values.  However, this leaf appears
> > to be a pure features leaf.  In which case a union just makes life painful.
> > 
> > Please add a CPUID_8000_0022_EAX kvm_only_cpuid_leafs entry (details in link[*]
> > below) so that KVM can write sane code like
> > 
> > 	guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)
> > 
> > and cpuid_entry_override() instead of manually filling in information.
> > 
> > where appropriate.
> > 
> > [*] https://lore.kernel.org/all/Y1AQX3RfM+awULlE@google.com
> 
> When someone is selling syntactic sugar in the kernel space, extra attention
> needs to be paid to runtime performance (union) and memory footprint
> (reverse_cpuid).

No.  Just no.

First off, this is more than syntactic sugar.  KVM has had multiple bugs in the
past due to manually querying/filling CPUID entries.  The reverse-CPUID infrastructure
guards against some of those bugs by limiting potential bugs to the leaf definition
and the feature definition.  I.e. we only need to get the cpuid_leafs+X86_FEATURE_*
definitions correct.

Second, this code is not remotely performance sensitive, and the memory footprint
of the reverse_cpuid table is laughably small.  It's literally 8 bytes per entry
FOR THE ENTIRE KERNEL.  And that's ignoring the fact that the table might even be
optimized away entirely since it's really just a switch statement that doesn't
use a helper function.

> > With a proper CPUID_8000_0022_EAX, this becomes:
> > 
> > 		entry->ecx = entry->edx = 0;
> > 		if (!enable_pmu || !kvm_cpu_cap_has(X86_FEATURE_AMD_PMU_V2)) {
> > 			entry->eax = entry->ebx;
> > 			break;
> > 		}
> > 
> > 		cpuid_entry_override(entry, CPUID_8000_0022_EAX);
> > 
> > 		...
> 
> Then in this code block, we will have:
> 
> 	/* AMD PerfMon is only supported up to V2 in the KVM. */
> 	entry->eax |= BIT(0);

I can't tell exactly what you're suggesting, but if you're implying that you don't
want to add CPUID_8000_0022_EAX, then NAK.  Open coding CPUID feature bit
manipulations in KVM is not acceptable.

If I'm misunderstanding and there's something that isn't handled by
cpuid_entry_override(), then the correct way to force a CPUID feature bit is:

	cpuid_entry_set(entry, X86_FEATURE_AMD_PMU_V2);

> to cover AMD Perfmon V3+, any better move ?

Huh?  If/when V3+ comes along, the above

	cpuid_entry_override(entry, CPUID_8000_0022_EAX);

will continue to do the right thing because KVM will (a) advertise V2 if it's
supported in hardware and (b) NOT advertise V3+ because the relevant CPUID bit(s)
will not be set in kvm_cpu_caps until KVM gains the necessary support.