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@xxxxxxxxxx > > 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.
