linux-kernel - Re: [PATCH v1] KVM: x86: 0-initialize kvm_caps.supported

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] [day] [month] [year] [list]

Message-ID: <Zl5VAWlw5XfkqLC-@google.com>
Date: Mon, 3 Jun 2024 16:42:57 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Wei Wang <wei.w.wang@...el.com>
Cc: pbonzini@...hat.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] KVM: x86: 0-initialize kvm_caps.supported_xss on definition

On Fri, May 03, 2024, Wei Wang wrote:
> 0-initialize kvm_caps.supported_xss on definition, so that it doesn't
> need to be explicitly zero-ed either in the common x86 or VMX/SVM
> initialization paths. This simplifies the code and reduces LOCs.

Heh, nope, see commit 40269c03fdbf ("KVM: x86: Explicitly zero kvm_caps during
vendor module load").

I do agree the code in kvm_x86_vendor_init() in particular is a bit odd, but it's
there because KVM support for XSAVES could be cleared by ->hardware_setup()
(XSAVES has a VMX knob, XSAVE for XCR0 does not).

And while I agree it's also odd that vendor code explicitly zeros supported_xss,
that too serves a purpose.  It reduces the probability of advertising XSS support
in vendor code that doesn't yet fully support the feature, e.g. if KVM VMX supports
CET before KVM SVM, then KVM SVM "needs" to clear supported_xss.  In quotes because
common KVM should also do the clearing based on CET CPUID features.  But, it's
still a nice reminder that vendor code likely needs additional support.

> No functional changes intended.
> 
> Signed-off-by: Wei Wang <wei.w.wang@...el.com>
> ---
>  arch/x86/kvm/svm/svm.c | 1 -
>  arch/x86/kvm/vmx/vmx.c | 2 --
>  arch/x86/kvm/x86.c     | 4 +---
>  3 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 9aaf83c8d57d..8105e5383b62 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5092,7 +5092,6 @@ static __init void svm_set_cpu_caps(void)
>  	kvm_set_cpu_caps();
>  
>  	kvm_caps.supported_perf_cap = 0;
> -	kvm_caps.supported_xss = 0;
>  
>  	/* CPUID 0x80000001 and 0x8000000A (SVM features) */
>  	if (nested) {
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 22411f4aff53..495125723c15 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7952,8 +7952,6 @@ static __init void vmx_set_cpu_caps(void)
>  	if (vmx_umip_emulated())
>  		kvm_cpu_cap_set(X86_FEATURE_UMIP);
>  
> -	/* CPUID 0xD.1 */
> -	kvm_caps.supported_xss = 0;
>  	if (!cpu_has_vmx_xsaves())
>  		kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 91478b769af0..6a97592950ff 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -94,6 +94,7 @@
>  
>  struct kvm_caps kvm_caps __read_mostly = {
>  	.supported_mce_cap = MCG_CTL_P | MCG_SER_P,
> +	.supported_xss = 0,
>  };
>  EXPORT_SYMBOL_GPL(kvm_caps);
>  
> @@ -9795,9 +9796,6 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
>  
>  	kvm_register_perf_callbacks(ops->handle_intel_pt_intr);
>  
> -	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
> -		kvm_caps.supported_xss = 0;
> -
>  #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
>  	cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);
>  #undef __kvm_cpu_cap_has
> 
> base-commit: 16c20208b9c2fff73015ad4e609072feafbf81ad
> -- 
> 2.27.0
>