Re: [PATCH v3 50/57] KVM: x86: Replace (almost) all guest CPUID feature queries with cpu_caps

[Chao Gao]

On Wed, Nov 27, 2024 at 05:34:17PM -0800, Sean Christopherson wrote:
>Switch all queries (except XSAVES) of guest features from guest CPUID to
>guest capabilities, i.e. replace all calls to guest_cpuid_has() with calls
>to guest_cpu_cap_has().
>
>Keep guest_cpuid_has() around for XSAVES, but subsume its helper
>guest_cpuid_get_register() and add a compile-time assertion to prevent
>using guest_cpuid_has() for any other feature.  Add yet another comment
>for XSAVE to explain why KVM is allowed to query its raw guest CPUID.
>
>Opportunistically drop the unused guest_cpuid_clear(), as there should be
>no circumstance in which KVM needs to _clear_ a guest CPUID feature now
>that everything is tracked via cpu_caps.  E.g. KVM may need to _change_
>a feature to emulate dynamic CPUID flags, but KVM should never need to
>clear a feature in guest CPUID to prevent it from being used by the guest.
>
>Delete the last remnants of the governed features framework, as the lone
>holdout was vmx_adjust_secondary_exec_control()'s divergent behavior for
>governed vs. ungoverned features.
>
>Note, replacing guest_cpuid_has() checks with guest_cpu_cap_has() when
>computing reserved CR4 bits is a nop when viewed as a whole, as KVM's
>capabilities are already incorporated into the calculation, i.e. if a
>feature is present in guest CPUID but unsupported by KVM, its CR4 bit
>was already being marked as reserved, checking guest_cpu_cap_has() simply
>double-stamps that it's a reserved bit.

...

>
>Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
>Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>---
> arch/x86/kvm/cpuid.c             |  4 +-
> arch/x86/kvm/cpuid.h             | 76 ++++++++++++--------------------
> arch/x86/kvm/governed_features.h | 22 ---------
> arch/x86/kvm/hyperv.c            |  2 +-
> arch/x86/kvm/lapic.c             |  4 +-
> arch/x86/kvm/smm.c               | 10 ++---
> arch/x86/kvm/svm/pmu.c           |  8 ++--
> arch/x86/kvm/svm/sev.c           |  4 +-
> arch/x86/kvm/svm/svm.c           | 20 ++++-----
> arch/x86/kvm/vmx/hyperv.h        |  2 +-
> arch/x86/kvm/vmx/nested.c        | 12 ++---
> arch/x86/kvm/vmx/pmu_intel.c     |  4 +-
> arch/x86/kvm/vmx/sgx.c           | 14 +++---
> arch/x86/kvm/vmx/vmx.c           | 47 +++++++++-----------
> arch/x86/kvm/x86.c               | 66 +++++++++++++--------------
> 15 files changed, 124 insertions(+), 171 deletions(-)
> delete mode 100644 arch/x86/kvm/governed_features.h
>
>diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>index d3c3e1327ca1..8d088a888a0d 100644
>--- a/arch/x86/kvm/cpuid.c
>+++ b/arch/x86/kvm/cpuid.c
>@@ -416,7 +416,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> 	 * and can install smaller shadow pages if the host lacks 1GiB support.
> 	 */
> 	allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
>-				      guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
>+				      guest_cpu_cap_has(vcpu, X86_FEATURE_GBPAGES);
> 	guest_cpu_cap_change(vcpu, X86_FEATURE_GBPAGES, allow_gbpages);
> 
> 	best = kvm_find_cpuid_entry(vcpu, 1);
>@@ -441,7 +441,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> 
> #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
> 	vcpu->arch.cr4_guest_rsvd_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_) |
>-					 __cr4_reserved_bits(guest_cpuid_has, vcpu);
>+					 __cr4_reserved_bits(guest_cpu_cap_has, vcpu);

So, actually, __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_) can be dropped.
Is there any reason to keep it? It makes perfect sense to just look up the
guest cpu_caps given it already takes KVM caps into consideration.

> #undef __kvm_cpu_cap_has
> 
> 	kvm_hv_set_cpuid(vcpu, kvm_cpuid_has_hyperv(vcpu));