lkml.org 
[lkml]   [2022]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 3/3] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022
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.

\
 
 \ /
  Last update: 2022-11-10 18:35    [W:0.059 / U:1.856 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site