> -----Original Message-----
> From: Jim Mattson <jmattson@xxxxxxxxxx>
> Sent: Thursday, October 3, 2019 11:15 AM
> To: Moger, Babu <Babu.Moger@xxxxxxx>
> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>; Aaron Lewis
> <aaronlewis@xxxxxxxxxx>; kvm list <kvm@xxxxxxxxxxxxxxx>; Natarajan,
> Janakarajan <Janakarajan.Natarajan@xxxxxxx>
> Subject: Re: [Patch] KVM: SVM: Fix svm_xsaves_supported
>
> On Thu, Oct 3, 2019 at 9:02 AM Moger, Babu <Babu.Moger@xxxxxxx>
> wrote:
> >
> >
> >
> > On 9/9/19 3:54 AM, Vitaly Kuznetsov wrote:
> > > Jim Mattson <jmattson@xxxxxxxxxx> writes:
> > >
> > >> On Wed, Sep 4, 2019 at 9:51 AM Vitaly Kuznetsov
> <vkuznets@xxxxxxxxxx> wrote:
> > >>
> > >>> Currently, VMX code only supports writing '0' to MSR_IA32_XSS:
> > >>>
> > >>> case MSR_IA32_XSS:
> > >>> if (!vmx_xsaves_supported() ||
> > >>> (!msr_info->host_initiated &&
> > >>> !(guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> > >>> guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))))
> > >>> return 1;
> > >>> /*
> > >>> * The only supported bit as of Skylake is bit 8, but
> > >>> * it is not supported on KVM.
> > >>> */
> > >>> if (data != 0)
> > >>> return 1;
> > >>>
> > >>>
> > >>> we will probably need the same limitation for SVM, however, I'd vote
> for
> > >>> creating separate kvm_x86_ops->set_xss() implementations.
> > >>
> > >> I hope separate implementations are unnecessary. The allowed
> IA32_XSS
> > >> bits should be derivable from guest_cpuid_has() in a
> > >> vendor-independent way. Otherwise, the CPU vendors have messed
> up. :-)
> > >>
> > >> At present, we use the MSR-load area to swap guest/host values of
> > >> IA32_XSS on Intel (when the host and guest values differ), but it
> > >> seems to me that IA32_XSS and %xcr0 should be swapped at the same
> > >> time, in kvm_load_guest_xcr0/kvm_put_guest_xcr0. This potentially
> adds
> > >> an additional L1 WRMSR VM-exit to every emulated VM-entry or VM-
> exit
> > >> for nVMX, but since the host currently sets IA32_XSS to 0 and we only
> > >> allow the guest to set IA32_XSS to 0, we can probably worry about this
> > >> later.
> > >
> > > Yea, I was suggesting to split implementations as a future proof but a
> > > comment like "This ought to be 0 for now" would also do)
> >
> > Hi, Anyone actively working on this?
> >
> > I was trying to expose xsaves on AMD qemu guest. Found out that we
> need to
> > get this above code working before I can expose xsaves on guest. I can
> > re-post these patches if it is ok.
> >
> > Sorry, I dont have the complete background. From what I understood, we
> > need to add the same check as Intel for MSR_IA32_XSS in get_msr and
> set_msr.
> >
> > static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > {
> > ..
> >
> > case MSR_IA32_XSS:
> > if (!vmx_xsaves_supported() ||
> > (!msr_info->host_initiated &&
> > !(guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> > guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))))
> > return 1;
> > msr_info->data = vcpu->arch.ia32_xss;
> > break;
> > ..
> > ..
> > }
> >
> > static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > {
> > ..
> > ..
> > case MSR_IA32_XSS:
> > if (!vmx_xsaves_supported() ||
> > (!msr_info->host_initiated &&
> > !(guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> > guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))))
> > return 1;
> > /*
> > * The only supported bit as of Skylake is bit 8, but
> > * it is not supported on KVM.
> > */
> > if (data != 0)
> > return 1;
> > vcpu->arch.ia32_xss = data;
> > if (vcpu->arch.ia32_xss != host_xss)
> > add_atomic_switch_msr(vmx, MSR_IA32_XSS,
> > vcpu->arch.ia32_xss, host_xss, false);
> > else
> > clear_atomic_switch_msr(vmx, MSR_IA32_XSS);
> > break;
> > ...
> > }
> >
> > We probably don't need last "if & else" check as we are setting it 0 now.
> > Does this look accurate?
>
> Aaron is working on it, and I think he's close to being ready to send
> out the next revision.
Ok. Thanks.
