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 <[email protected]>
Signed-off-by: Xiaoyao Li <[email protected]>
Signed-off-by: Tao Xu <[email protected]>
---
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
Hi,
On Thu, 20 Jun 2019 at 13:06, Tao Xu <[email protected]> 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 <[email protected]>
> Signed-off-by: Xiaoyao Li <[email protected]>
> Signed-off-by: Tao Xu <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
>> Signed-off-by: Xiaoyao Li <[email protected]>
>> Signed-off-by: Tao Xu <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]> 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