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

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: <Z1uYihcPhJVRmrxh@intel.com>
Date: Fri, 13 Dec 2024 10:14:34 +0800
From: Chao Gao <chao.gao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov
	<vkuznets@...hat.com>, Jarkko Sakkinen <jarkko@...nel.org>,
	<kvm@...r.kernel.org>, <linux-sgx@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Maxim Levitsky <mlevitsk@...hat.com>, "Hou
 Wenlong" <houwenlong.hwl@...group.com>, Xiaoyao Li <xiaoyao.li@...el.com>,
	Kechen Lu <kechenl@...dia.com>, Oliver Upton <oliver.upton@...ux.dev>,
	"Binbin Wu" <binbin.wu@...ux.intel.com>, Yang Weijiang
	<weijiang.yang@...el.com>, Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v3 50/57] KVM: x86: Replace (almost) all guest CPUID
 feature queries with cpu_caps

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@...hat.com>
>Signed-off-by: Sean Christopherson <seanjc@...gle.com>
>---
> 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));