* [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
@ 2019-06-20 5:03 Tao Xu
2019-06-20 6:40 ` Wanpeng Li
0 siblings, 1 reply; 9+ messages in thread
From: Tao Xu @ 2019-06-20 5:03 UTC (permalink / raw)
To: pbonzini, rkrcmar, tglx, mingo, bp, hpa
Cc: kvm, linux-kernel, xiaoyao.li, tao3.xu
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;
/*
* 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) {
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 5:03 [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported Tao Xu
@ 2019-06-20 6:40 ` Wanpeng Li
2019-06-20 6:46 ` Xiaoyao Li
0 siblings, 1 reply; 9+ messages in thread
From: Wanpeng Li @ 2019-06-20 6:40 UTC (permalink / raw)
To: Tao Xu
Cc: Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, kvm, LKML, xiaoyao.li
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 6:40 ` Wanpeng Li
@ 2019-06-20 6:46 ` Xiaoyao Li
2019-06-20 8:17 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Xiaoyao Li @ 2019-06-20 6:46 UTC (permalink / raw)
To: Wanpeng Li, Tao Xu
Cc: Paolo Bonzini, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, kvm, LKML
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 6:46 ` Xiaoyao Li
@ 2019-06-20 8:17 ` Paolo Bonzini
2019-06-20 8:27 ` Wanpeng Li
2019-06-20 8:55 ` Xiaoyao Li
0 siblings, 2 replies; 9+ messages in thread
From: Paolo Bonzini @ 2019-06-20 8:17 UTC (permalink / raw)
To: Xiaoyao Li, Wanpeng Li, Tao Xu
Cc: Radim Krcmar, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, kvm, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 8:17 ` Paolo Bonzini
@ 2019-06-20 8:27 ` Wanpeng Li
2019-06-20 8:36 ` Paolo Bonzini
2019-06-20 8:55 ` Xiaoyao Li
1 sibling, 1 reply; 9+ messages in thread
From: Wanpeng Li @ 2019-06-20 8:27 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Xiaoyao Li, Tao Xu, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, kvm, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 8:27 ` Wanpeng Li
@ 2019-06-20 8:36 ` Paolo Bonzini
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2019-06-20 8:36 UTC (permalink / raw)
To: Wanpeng Li
Cc: Xiaoyao Li, Tao Xu, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, kvm, LKML
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 8:17 ` Paolo Bonzini
2019-06-20 8:27 ` Wanpeng Li
@ 2019-06-20 8:55 ` Xiaoyao Li
2019-06-20 8:59 ` Paolo Bonzini
1 sibling, 1 reply; 9+ messages in thread
From: Xiaoyao Li @ 2019-06-20 8:55 UTC (permalink / raw)
To: Paolo Bonzini, Wanpeng Li, Tao Xu
Cc: Radim Krcmar, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, kvm, LKML
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 8:55 ` Xiaoyao Li
@ 2019-06-20 8:59 ` Paolo Bonzini
2019-06-20 9:02 ` Wanpeng Li
0 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2019-06-20 8:59 UTC (permalink / raw)
To: Xiaoyao Li, Wanpeng Li, Tao Xu
Cc: Radim Krcmar, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, kvm, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported
2019-06-20 8:59 ` Paolo Bonzini
@ 2019-06-20 9:02 ` Wanpeng Li
0 siblings, 0 replies; 9+ messages in thread
From: Wanpeng Li @ 2019-06-20 9:02 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Xiaoyao Li, Tao Xu, Radim Krcmar, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, kvm, LKML
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-20 9:01 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-20 5:03 [PATCH] KVM: vmx: Fix the broken usage of vmx_xsaves_supported Tao Xu
2019-06-20 6:40 ` Wanpeng Li
2019-06-20 6:46 ` Xiaoyao Li
2019-06-20 8:17 ` Paolo Bonzini
2019-06-20 8:27 ` Wanpeng Li
2019-06-20 8:36 ` Paolo Bonzini
2019-06-20 8:55 ` Xiaoyao Li
2019-06-20 8:59 ` Paolo Bonzini
2019-06-20 9:02 ` Wanpeng Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox