Re: [PATCH v2 47/49] KVM: x86: Drop superfluous host XSAVE check when adjusting guest XSAVES caps

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> 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@redhat.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.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?

Because that would result in VMX and SVM behavior diverging with respect to
whether guest_cpu_cap_has(X86_FEATURE_XSAVES).  E.g. for AMD it would be 100%
accurate, but for Intel it would be accurate if and only if XSAVE is supported.

In practice that isn't truly problematic, because checks on XSAVES from common
code are gated on guest CR4.OSXSAVE=1, i.e. implicitly check XSAVE support.  But
the potential danger of sublty divergent behavior between VMX and SVM isn't worth
making AVX vs. XSAVES consistent within VMX, especially since VMX vs. SVM would
still be inconsistent.

> AMD portion of this patch, on the other hand does makes sense, due to a lack
> of a separate XSAVES intercept.

FWIW, AMD also needs precise tracking in order to passthrough XSS for SEV-ES.