On Tue, 2024-07-09 at 12:15 -0700, Sean Christopherson wrote: > 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@xxxxxxxxxx> > > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> > > > --- > > > 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. This is a good justification, and IMHO it is worth a comment in the VMX code, so that this question I had won't be raised again. > 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. Makes sense too. > Best regards, Maxim Levitsky
