KVM: SVM: Fix svm_xsaves_supported
PATCH 1/1 KVM: SVM: Fix svm_xsaves_supported
AMD allows guests to execute XSAVES/XRSTORS if supported by the host. This is different than Intel as they have an additional control bit that determines if XSAVES/XRSTORS can be used by the guest. Intel also has intercept bits that might prevent the guest from intercepting the instruction as well. AMD has none of that, not even an Intercept mechanism. AMD simply allows XSAVES/XRSTORS to be executed by the guest if also supported by the host.
--- arch/x86/kvm/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/svm.c +1 -1
@@ -5985,7 +5985,7 @@ static bool svm_mpx_supported(void)static bool svm_xsaves_supported (void ) { -return false; +return boot_cpu_has (X86_FEATURE_XSAVES ); }static bool svm_umip_emulated (void )
Signature · 1 line
2.23.0.187.g17f5b7556c-goog
Quoted 2 lines · Aaron Lewis
AMD allows guests to execute XSAVES/XRSTORS if supported by the host. This is different than Intel as they have an additional control bit that determines if XSAVES/XRSTORS can be used by the guest. Intel also has intercept bits that might prevent the guest from intercepting the instruction as well. AMD has none of that, not even an Intercept mechanism. AMD simply allows XSAVES/XRSTORS to be executed by the guest if also supported by the host.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
Quoted 10 lines
Signed-off-by Aaron Lewis <[email protected]>--- arch/x86/kvm/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/kvm/svm.c +0
-0
arch/x86/kvm/svm.c static bool svm_mpx_supported(void) · line 5991
Quoted 8 lines
arch/x86/kvm/svm.c +1
-1@@ -5985,7 +5985,7 @@ static bool svm_mpx_supported(void)static bool svm_xsaves_supported (void ) { -return false; +return boot_cpu_has (X86_FEATURE_XSAVES ); }static bool svm_umip_emulated (void )
I had a similar patch in my stash when I tried to debug Hyper-V 2016 not being able to boot on KVM. I may have forgotten some important details, but if I'm not mistaken XSAVES comes paired with MSR_IA32_XSS and some OSes may actually try to write there, in particular I've observed Hyper-V 2016 trying to write '0'. Currently, we only support MSR_IA32_XSS in vmx code, this will need to be extended to SVM.
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.
Signature · 1 line
Vitaly
Quoted 50 lines · Vitaly Kuznetsov
Quoted 2 lines · Aaron Lewis
AMD allows guests to execute XSAVES/XRSTORS if supported by the host. This is different than Intel as they have an additional control bit that determines if XSAVES/XRSTORS can be used by the guest. Intel also has intercept bits that might prevent the guest from intercepting the instruction as well. AMD has none of that, not even an Intercept mechanism. AMD simply allows XSAVES/XRSTORS to be executed by the guest if also supported by the host.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
Quoted 10 lines
Signed-off-by Aaron Lewis <[email protected]>--- arch/x86/kvm/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/kvm/svm.c +0
-0arch/x86/kvm/svm.c static bool svm_mpx_supported(void) · line 5991
Quoted 8 lines
arch/x86/kvm/svm.c +1
-1@@ -5985,7 +5985,7 @@ static bool svm_mpx_supported(void)static bool svm_xsaves_supported (void ) { -return false; +return boot_cpu_has (X86_FEATURE_XSAVES ); }static bool svm_umip_emulated (void )I had a similar patch in my stash when I tried to debug Hyper-V 2016 not being able to boot on KVM. I may have forgotten some important details, but if I'm not mistaken XSAVES comes paired with
MSR_IA32_XSSand some OSes may actually try to write there, in particular I've observed Hyper-V 2016 trying to write '0'. Currently, we only supportMSR_IA32_XSSin vmx code, this will need to be extended to SVM.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;
Quoted 5 lines
we will probably need the same limitation for SVM, however, I'd vote for creating separate
kvm_x86_ops->set_xss()implementations.--
Vitaly
Fixed the unwrapped description in v2.
As for extending VMX behavior to SVM for MSR_IA_32_XSS; I will do this in a follow up patch. Thanks for calling this out.
Quoted 54 lines · Aaron Lewis
Quoted 53 lines · Vitaly Kuznetsov
Quoted 2 lines · Aaron Lewis
AMD allows guests to execute XSAVES/XRSTORS if supported by the host. This is different than Intel as they have an additional control bit that determines if XSAVES/XRSTORS can be used by the guest. Intel also has intercept bits that might prevent the guest from intercepting the instruction as well. AMD has none of that, not even an Intercept mechanism. AMD simply allows XSAVES/XRSTORS to be executed by the guest if also supported by the host.
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
Quoted 10 lines
Signed-off-by Aaron Lewis <[email protected]>--- arch/x86/kvm/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/x86/kvm/svm.c +0
-0arch/x86/kvm/svm.c static bool svm_mpx_supported(void) · line 5991
Quoted 8 lines
arch/x86/kvm/svm.c +1
-1@@ -5985,7 +5985,7 @@ static bool svm_mpx_supported(void)static bool svm_xsaves_supported (void ) { -return false; +return boot_cpu_has (X86_FEATURE_XSAVES ); }static bool svm_umip_emulated (void )I had a similar patch in my stash when I tried to debug Hyper-V 2016 not being able to boot on KVM. I may have forgotten some important details, but if I'm not mistaken XSAVES comes paired with
MSR_IA32_XSSand some OSes may actually try to write there, in particular I've observed Hyper-V 2016 trying to write '0'. Currently, we only supportMSR_IA32_XSSin vmx code, this will need to be extended to SVM.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.
Quoted 7 lines
Quoted 2 lines
--
VitalyFixed the unwrapped description in v2.
As for extending VMX behavior to SVM for
MSR_IA_32_XSS; I will do this in a follow up patch. Thanks for calling this out.
Doing this in a separate patch is fine, however, I think this patch should come before we start announcing XSAVES support on AMD: both MSR_IA_32_XSS and XSAVES/XRSTORS instructions are enumerated by CPUID.(EAX=0DH, ECX=1).EAX[bit 3] so an unprepared guest may try accessing MSR_IA_32_XSS and get #GP.
Signature · 1 line
Vitaly
Quoted 16 lines · Vitaly Kuznetsov
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;
Quoted 2 lines
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.
I have to say, this is an awful lot of effort for an MSR that's never used!
Quoted 25 lines · Jim Mattson
Quoted 16 lines · Vitaly Kuznetsov
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;Quoted 2 lines
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_XSSbits should be derivable fromguest_cpuid_has()in a vendor-independent way. Otherwise, the CPU vendors have messed up. :-)
Quoted 8 lines
At present, we use the MSR-load area to swap guest/host values of
IA32_XSSon Intel (when the host and guest values differ), but it seems to me thatIA32_XSSand %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 setsIA32_XSSto 0 and we only allow the guest to setIA32_XSSto 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)
Quoted 2 lines
I have to say, this is an awful lot of effort for an MSR that's never used!
Agreed :-)
Signature · 1 line
Vitaly
Quoted 36 lines · Vitaly Kuznetsov
Quoted 25 lines · Jim Mattson
Quoted 16 lines · Vitaly Kuznetsov
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;Quoted 2 lines
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_XSSbits should be derivable fromguest_cpuid_has()in a vendor-independent way. Otherwise, the CPU vendors have messed up. :-)Quoted 8 lines
At present, we use the MSR-load area to swap guest/host values of
IA32_XSSon Intel (when the host and guest values differ), but it seems to me thatIA32_XSSand %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 setsIA32_XSSto 0 and we only allow the guest to setIA32_XSSto 0, we can probably worry about this later.
Quoted 2 lines
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?
Quoted 91 lines · Babu
Quoted 36 lines · Vitaly Kuznetsov
Quoted 25 lines · Jim Mattson
Quoted 16 lines · Vitaly Kuznetsov
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;Quoted 2 lines
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_XSSbits should be derivable fromguest_cpuid_has()in a vendor-independent way. Otherwise, the CPU vendors have messed up. :-)Quoted 8 lines
At present, we use the MSR-load area to swap guest/host values of
IA32_XSSon Intel (when the host and guest values differ), but it seems to me thatIA32_XSSand %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 setsIA32_XSSto 0 and we only allow the guest to setIA32_XSSto 0, we can probably worry about this later.Quoted 2 lines
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_XSSinget_msrandset_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){
..
..
caseMSR_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;
Quoted 5 lines
...
}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.
Quoted 115 lines
-----Original Message-----
From: Jim Mattson <[email protected]>
Sent: Thursday, October 3, 2019 11:15 AM
To: Moger, Babu <[email protected]>
Cc: Vitaly Kuznetsov <[email protected]>; Aaron Lewis <[email protected]>; kvm list <[email protected]>; Natarajan, Janakarajan <[email protected]>
Subject: Re: [Patch] KVM: SVM: Fixsvm_xsaves_supportedQuoted 7 lines · Babu
Quoted 3 lines · Vitaly Kuznetsov
Quoted 1 line · Jim Mattson
On Wed, Sep 4, 2019 at 9:51 AM Vitaly Kuznetsov
Quoted 17 lines · <[email protected]> wrote:
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
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;Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
we will probably need the same limitation for SVM, however, I'd vote
for
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
creating separate
kvm_x86_ops->set_xss()implementations.I hope separate implementations are unnecessary. The allowed
IA32_XSSQuoted 2 lines
Quoted 2 lines
Quoted 2 lines
bits should be derivable from
guest_cpuid_has()in a vendor-independent way. Otherwise, the CPU vendors have messedup. :-)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
At present, we use the MSR-load area to swap guest/host values of
IA32_XSSon Intel (when the host and guest values differ), but it seems to me thatIA32_XSSand %xcr0 should be swapped at the same time, in kvm_load_guest_xcr0/kvm_put_guest_xcr0. This potentiallyadds
Quoted 1 line
Quoted 1 line
Quoted 1 line
an additional L1 WRMSR VM-exit to every emulated VM-entry or VM-
exit
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
for nVMX, but since the host currently sets
IA32_XSSto 0 and we only allow the guest to setIA32_XSSto 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
Quoted 5 lines
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_XSSinget_msrand
set_msr.Quoted 43 lines
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){
..
..
caseMSR_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;...
}
Quoted 5 lines
Quoted 2 lines
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.