| Message ID | 20190620050301.1149-1-tao3.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: vmx: Fix the broken usage of vmx_xsaves_supported | expand |
Hi, On Thu, 20 Jun 2019 at 13:06, Tao Xu <tao3.xu@intel.com> wrote: > > The helper vmx_xsaves_supported() returns the bit value of > SECONDARY_EXEC_XSAVES in vmcs_config.cpu_based_2nd_exec_ctrl, which > remains unchanged true if vmcs supports 1-setting of this bit after > setup_vmcs_config(). It should check the guest's cpuid not this > unchanged value when get/set msr. > > Besides, vmx_compute_secondary_exec_control() adjusts > SECONDARY_EXEC_XSAVES bit based on guest cpuid's X86_FEATURE_XSAVE > and X86_FEATURE_XSAVES, it should use updated value to decide whether > set XSS_EXIT_BITMAP. > > Co-developed-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > Signed-off-by: Tao Xu <tao3.xu@intel.com> > --- > arch/x86/kvm/vmx/vmx.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index b93e36ddee5e..935cf72439a9 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1721,7 +1721,8 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > return vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index, > &msr_info->data); > case MSR_IA32_XSS: > - if (!vmx_xsaves_supported()) > + if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) || > + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) > return 1; > msr_info->data = vcpu->arch.ia32_xss; > break; > @@ -1935,7 +1936,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > return 1; > return vmx_set_vmx_msr(vcpu, msr_index, data); > case MSR_IA32_XSS: > - if (!vmx_xsaves_supported()) > + if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) || > + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) > return 1; Not complete true. > /* > * The only supported bit as of Skylake is bit 8, but > @@ -4094,7 +4096,7 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > set_cr4_guest_host_mask(vmx); > > - if (vmx_xsaves_supported()) > + if (vmx->secondary_exec_control & SECONDARY_EXEC_XSAVES) > vmcs_write64(XSS_EXIT_BITMAP, VMX_XSS_EXIT_BITMAP); This is not true. SDM 24.6.20: On processors that support the 1-setting of the “enable XSAVES/XRSTORS” VM-execution control, the VM-execution control fields include a 64-bit XSS-exiting bitmap. It depends on whether or not processors support the 1-setting instead of “enable XSAVES/XRSTORS” is 1 in VM-exection control field. Anyway, I will send a patch to fix the msr read/write for commit 203000993de5(kvm: vmx: add MSR logic for XSAVES), thanks for the report. Regards, Wanpeng Li
On 6/20/2019 2:40 PM, Wanpeng Li wrote: > Hi, > On Thu, 20 Jun 2019 at 13:06, Tao Xu <tao3.xu@intel.com> wrote: >> >> The helper vmx_xsaves_supported() returns the bit value of >> SECONDARY_EXEC_XSAVES in vmcs_config.cpu_based_2nd_exec_ctrl, which >> remains unchanged true if vmcs supports 1-setting of this bit after >> setup_vmcs_config(). It should check the guest's cpuid not this >> unchanged value when get/set msr. >> >> Besides, vmx_compute_secondary_exec_control() adjusts >> SECONDARY_EXEC_XSAVES bit based on guest cpuid's X86_FEATURE_XSAVE >> and X86_FEATURE_XSAVES, it should use updated value to decide whether >> set XSS_EXIT_BITMAP. >> >> Co-developed-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> >> Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> >> Signed-off-by: Tao Xu <tao3.xu@intel.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index b93e36ddee5e..935cf72439a9 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -1721,7 +1721,8 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> return vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index, >> &msr_info->data); >> case MSR_IA32_XSS: >> - if (!vmx_xsaves_supported()) >> + if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) || >> + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) >> return 1; >> msr_info->data = vcpu->arch.ia32_xss; >> break; >> @@ -1935,7 +1936,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> return 1; >> return vmx_set_vmx_msr(vcpu, msr_index, data); >> case MSR_IA32_XSS: >> - if (!vmx_xsaves_supported()) >> + if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) || >> + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) >> return 1; > > Not complete true. > >> /* >> * The only supported bit as of Skylake is bit 8, but >> @@ -4094,7 +4096,7 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) >> >> set_cr4_guest_host_mask(vmx); >> >> - if (vmx_xsaves_supported()) >> + if (vmx->secondary_exec_control & SECONDARY_EXEC_XSAVES) >> vmcs_write64(XSS_EXIT_BITMAP, VMX_XSS_EXIT_BITMAP); > > This is not true. > > SDM 24.6.20: > On processors that support the 1-setting of the “enable > XSAVES/XRSTORS” VM-execution control, the VM-execution control fields > include a 64-bit XSS-exiting bitmap. > > It depends on whether or not processors support the 1-setting instead > of “enable XSAVES/XRSTORS” is 1 in VM-exection control field. Anyway, Yes, whether this field exist or not depends on whether processors support the 1-setting. But if "enable XSAVES/XRSTORS" is clear to 0, XSS_EXIT_BITMAP doesn't work. I think in this case, there is no need to set this vmcs field? > I will send a patch to fix the msr read/write for commit > 203000993de5(kvm: vmx: add MSR logic for XSAVES), thanks for the > report. > > Regards, > Wanpeng Li >
On 20/06/19 08:46, Xiaoyao Li wrote: >> >> It depends on whether or not processors support the 1-setting instead >> of “enable XSAVES/XRSTORS” is 1 in VM-exection control field. Anyway, > > Yes, whether this field exist or not depends on whether processors > support the 1-setting. > > But if "enable XSAVES/XRSTORS" is clear to 0, XSS_EXIT_BITMAP doesn't > work. I think in this case, there is no need to set this vmcs field? vmx->secondary_exec_control can change; you are making the code more complex by relying on the value of the field at the point of vmx_vcpu_setup. I do _think_ your version is incorrect, because at this point CPUID has not been initialized yet and therefore vmx_compute_secondary_exec_control has not set SECONDARY_EXEC_XSAVES. However I may be wrong because I didn't review the code very closely: the old code is obvious and so there is no point in changing it. Paolo
On Thu, 20 Jun 2019 at 16:17, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 20/06/19 08:46, Xiaoyao Li wrote: > >> > >> It depends on whether or not processors support the 1-setting instead > >> of “enable XSAVES/XRSTORS” is 1 in VM-exection control field. Anyway, > > > > Yes, whether this field exist or not depends on whether processors > > support the 1-setting. > > > > But if "enable XSAVES/XRSTORS" is clear to 0, XSS_EXIT_BITMAP doesn't > > work. I think in this case, there is no need to set this vmcs field? > > vmx->secondary_exec_control can change; you are making the code more > complex by relying on the value of the field at the point of vmx_vcpu_setup. > > I do _think_ your version is incorrect, because at this point CPUID has > not been initialized yet and therefore > vmx_compute_secondary_exec_control has not set SECONDARY_EXEC_XSAVES. > However I may be wrong because I didn't review the code very closely: > the old code is obvious and so there is no point in changing it. Agreed, in addition, guest can enable/disable cpuid bits by grub parameter, should we call kvm_x86_ops->cpuid_update() in kvm_vcpu_reset() path to reflect the new guest cpuid influence to exec_control? e.g. the first boot guest disable xsaves in grub, kvm disables xsaves in exec_control; then guest reboot w/ xsaves enabled, it still get an #UD when executing. Regards, Wanpeng Li
On 20/06/19 10:27, Wanpeng Li wrote: > Agreed, in addition, guest can enable/disable cpuid bits by grub > parameter Through what path? Guest can disable X86_FEATURE_* but that's purely a Linux feature, the few CPUID bits that can change at runtime already call kvm_x86_ops->cpuid_update(). Paolo > , should we call kvm_x86_ops->cpuid_update() in > kvm_vcpu_reset() path to reflect the new guest cpuid influence to > exec_control? e.g. the first boot guest disable xsaves in grub, kvm > disables xsaves in exec_control; then guest reboot w/ xsaves enabled, > it still get an #UD when executing.
On 6/20/2019 4:17 PM, Paolo Bonzini wrote: > On 20/06/19 08:46, Xiaoyao Li wrote: >>> >>> It depends on whether or not processors support the 1-setting instead >>> of “enable XSAVES/XRSTORS” is 1 in VM-exection control field. Anyway, >> >> Yes, whether this field exist or not depends on whether processors >> support the 1-setting. >> >> But if "enable XSAVES/XRSTORS" is clear to 0, XSS_EXIT_BITMAP doesn't >> work. I think in this case, there is no need to set this vmcs field? > > vmx->secondary_exec_control can change; you are making the code more > complex by relying on the value of the field at the point of vmx_vcpu_setup. > At this point. Agreed. It's harmless to set a default value. > I do _think_ your version is incorrect, because at this point CPUID has > not been initialized yet and therefore > vmx_compute_secondary_exec_control has not set SECONDARY_EXEC_XSAVES. SECONDARY_EXEC_XSAVES is in the opt when setup_vmcs_config, and vmx_compute_secondary_exec_control() is to clear SECONDARY_EXEC_XSAVES based on guest cpuid. > However I may be wrong because I didn't review the code very closely: > the old code is obvious and so there is no point in changing it. you mean this part about XSS_EXIT_BITMAP? how about the other part in vmx_set/get_msr() in this patch? > Paolo >
On 20/06/19 10:55, Xiaoyao Li wrote: > >> However I may be wrong because I didn't review the code very closely: >> the old code is obvious and so there is no point in changing it. > > you mean this part about XSS_EXIT_BITMAP? how about the other part in > vmx_set/get_msr() in this patch? Yes, only the XSS_EXIT_BITMAP part. The other is a bugfix, I didn't understand Wanpeng's objection very well. Paolo
On Thu, 20 Jun 2019 at 16:59, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 20/06/19 10:55, Xiaoyao Li wrote: > > > >> However I may be wrong because I didn't review the code very closely: > >> the old code is obvious and so there is no point in changing it. > > > > you mean this part about XSS_EXIT_BITMAP? how about the other part in > > vmx_set/get_msr() in this patch? > > Yes, only the XSS_EXIT_BITMAP part. The other is a bugfix, I didn't > understand Wanpeng's objection very well. https://lkml.org/lkml/2019/6/20/227 A more complete one. Regards, Wanpeng Li
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index b93e36ddee5e..935cf72439a9 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1721,7 +1721,8 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) return vmx_get_vmx_msr(&vmx->nested.msrs, msr_info->index, &msr_info->data); case MSR_IA32_XSS: - if (!vmx_xsaves_supported()) + if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) || + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) return 1; msr_info->data = vcpu->arch.ia32_xss; break; @@ -1935,7 +1936,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) return 1; return vmx_set_vmx_msr(vcpu, msr_index, data); case MSR_IA32_XSS: - if (!vmx_xsaves_supported()) + if (!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 @@ -4094,7 +4096,7 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) set_cr4_guest_host_mask(vmx); - if (vmx_xsaves_supported()) + if (vmx->secondary_exec_control & SECONDARY_EXEC_XSAVES) vmcs_write64(XSS_EXIT_BITMAP, VMX_XSS_EXIT_BITMAP); if (enable_pml) {