[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <46361f0c834a25ad0a45ca2f1813ade603d29201.camel@redhat.com>
Date: Thu, 04 Jul 2024 22:36:50 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
<pbonzini@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Hou Wenlong
<houwenlong.hwl@...group.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 v2 47/49] KVM: x86: Drop superfluous host XSAVE check
when adjusting guest XSAVES caps
On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> Drop the manual boot_cpu_has() checks on XSAVE when adjusting the guest's
> XSAVES capabilities now that guest cpu_caps incorporates KVM's support.
> The guest's cpu_caps are initialized from kvm_cpu_caps, which are in turn
> initialized from boot_cpu_data, i.e. checking guest_cpu_cap_has() also
> checks host/KVM capabilities (which is the entire point of cpu_caps).
>
> Cc: Maxim Levitsky <mlevitsk@...hat.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
> arch/x86/kvm/svm/svm.c | 1 -
> arch/x86/kvm/vmx/vmx.c | 3 +--
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 06770b60c0ba..4aaffbf22531 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4340,7 +4340,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> * the guest read/write access to the host's XSS.
> */
> guest_cpu_cap_change(vcpu, X86_FEATURE_XSAVES,
> - boot_cpu_has(X86_FEATURE_XSAVE) &&
> boot_cpu_has(X86_FEATURE_XSAVES) &&
> guest_cpu_cap_has(vcpu, X86_FEATURE_XSAVE));
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 741961a1edcc..6fbdf520c58b 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7833,8 +7833,7 @@ void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> * to the guest. XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be
> * set if and only if XSAVE is supported.
> */
> - if (!boot_cpu_has(X86_FEATURE_XSAVE) ||
> - !guest_cpu_cap_has(vcpu, X86_FEATURE_XSAVE))
> + if (!guest_cpu_cap_has(vcpu, X86_FEATURE_XSAVE))
> guest_cpu_cap_clear(vcpu, X86_FEATURE_XSAVES);
Hi,
I have a question about this code, even before the patch was applied:
While it is obviously correct to disable XSAVES when XSAVE not supported, I wonder:
There are a lot more cases like that and KVM explicitly doesn't bother checking them,
e.g all of the AVX family also depends on XSAVE due to XCR0.
What makes XSAVES/XSAVE dependency special here? Maybe we can remove this code to be consistent?
AMD portion of this patch, on the other hand does makes sense,
due to a lack of a separate XSAVES intercept.
Best regards,
Maxim Levitsky
>
> vmx_setup_uret_msrs(vmx);
Powered by blists - more mailing lists