| Message ID | 20201203151516.14441-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: reinstate vendor-agnostic check on SPEC_CTRL cpuid bits | expand |
On Thu, Dec 03, 2020, Paolo Bonzini wrote: > Until commit e7c587da1252 ("x86/speculation: Use synthetic bits for IBRS/IBPB/STIBP", > 2018-05-17), KVM was testing both Intel and AMD CPUID bits before allowing the > guest to write MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD. Testing only Intel bits > on VMX processors, or only AMD bits on SVM processors, fails if the guests are > created with the "opposite" vendor as the host. > > While at it, also tweak the host CPU check to use the vendor-agnostic feature bit > X86_FEATURE_IBPB, since we only care about the availability of the MSR on the host > here and not about specific CPUID bits. > > Fixes: e7c587da1252 ("x86/speculation: Use synthetic bits for IBRS/IBPB/STIBP") > Cc: stable@vger.kernel.org > Reported-by: Denis V. Lunev <den@openvz.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/svm/svm.c | 3 ++- > arch/x86/kvm/vmx/vmx.c | 10 +++++++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 62390fbc9233..0b4aa60b2754 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -2686,12 +2686,13 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > break; > case MSR_IA32_PRED_CMD: > if (!msr->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBPB)) > return 1; > > if (data & ~PRED_CMD_IBPB) > return 1; > - if (!boot_cpu_has(X86_FEATURE_AMD_IBPB)) > + if (!boot_cpu_has(X86_FEATURE_IBPB)) > return 1; > if (!data) > break; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index c3441e7e5a87..b74d2105ced7 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2028,7 +2028,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) Doesn't vmx_get_msr() also need to be changed for SPEC_CTRL? Assuming that's the case, adding helpers in cpuid.h to detect guest support for SPEC_CTRL (and maybe for PRED_CMD?) would be helpful. It'd reduce duplicate code and document that KVM allows cross-vendor emulation. The condition for SPEC_CTRL support is especially long. > break; > case MSR_IA32_SPEC_CTRL: > if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_STIBP) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) Whitespace is messed up. ERROR: code indent should use tabs where possible #54: FILE: arch/x86/kvm/vmx/vmx.c:2031: + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) &&$ WARNING: please, no spaces at the start of a line #54: FILE: arch/x86/kvm/vmx/vmx.c:2031: + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) &&$ ERROR: code indent should use tabs where possible #55: FILE: arch/x86/kvm/vmx/vmx.c:2032: + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_STIBP) &&$ WARNING: please, no spaces at the start of a line #55: FILE: arch/x86/kvm/vmx/vmx.c:2032: + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_STIBP) &&$ ERROR: code indent should use tabs where possible #56: FILE: arch/x86/kvm/vmx/vmx.c:2033: + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) &&$ WARNING: please, no spaces at the start of a line #56: FILE: arch/x86/kvm/vmx/vmx.c:2033: + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) &&$ ERROR: code indent should use tabs where possible #57: FILE: arch/x86/kvm/vmx/vmx.c:2034: + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD))$ WARNING: please, no spaces at the start of a line #57: FILE: arch/x86/kvm/vmx/vmx.c:2034: + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD))$ > return 1; > > if (kvm_spec_ctrl_test_value(data)) > @@ -2063,12 +2066,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > goto find_uret_msr; > case MSR_IA32_PRED_CMD: > if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBPB)) > return 1; > > if (data & ~PRED_CMD_IBPB) > return 1; > - if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) > + if (!boot_cpu_has(X86_FEATURE_IBPB)) > return 1; > if (!data) > break; > -- > 2.26.2 >
On 04/12/20 18:12, Sean Christopherson wrote: > Assuming that's > the case, adding helpers in cpuid.h to detect guest support for SPEC_CTRL (and > maybe for PRED_CMD?) would be helpful. It'd reduce duplicate code and document > that KVM allows cross-vendor emulation. The condition for SPEC_CTRL support is > especially long. That's a great idea nevertheless. Paolo
On 12/3/20 6:15 PM, Paolo Bonzini wrote: > Until commit e7c587da1252 ("x86/speculation: Use synthetic bits for IBRS/IBPB/STIBP", > 2018-05-17), KVM was testing both Intel and AMD CPUID bits before allowing the > guest to write MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD. Testing only Intel bits > on VMX processors, or only AMD bits on SVM processors, fails if the guests are > created with the "opposite" vendor as the host. > > While at it, also tweak the host CPU check to use the vendor-agnostic feature bit > X86_FEATURE_IBPB, since we only care about the availability of the MSR on the host > here and not about specific CPUID bits. > > Fixes: e7c587da1252 ("x86/speculation: Use synthetic bits for IBRS/IBPB/STIBP") > Cc: stable@vger.kernel.org > Reported-by: Denis V. Lunev <den@openvz.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/svm/svm.c | 3 ++- > arch/x86/kvm/vmx/vmx.c | 10 +++++++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 62390fbc9233..0b4aa60b2754 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -2686,12 +2686,13 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > break; > case MSR_IA32_PRED_CMD: > if (!msr->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBPB)) > return 1; > > if (data & ~PRED_CMD_IBPB) > return 1; > - if (!boot_cpu_has(X86_FEATURE_AMD_IBPB)) > + if (!boot_cpu_has(X86_FEATURE_IBPB)) > return 1; > if (!data) > break; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index c3441e7e5a87..b74d2105ced7 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2028,7 +2028,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > break; > case MSR_IA32_SPEC_CTRL: > if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_STIBP) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) > return 1; > > if (kvm_spec_ctrl_test_value(data)) > @@ -2063,12 +2066,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > goto find_uret_msr; > case MSR_IA32_PRED_CMD: > if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && > + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBPB)) > return 1; > > if (data & ~PRED_CMD_IBPB) > return 1; > - if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) > + if (!boot_cpu_has(X86_FEATURE_IBPB)) > return 1; > if (!data) > break; Reviewed-by: Denis V. Lunev <den@openvz.org>
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 62390fbc9233..0b4aa60b2754 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -2686,12 +2686,13 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) break; case MSR_IA32_PRED_CMD: if (!msr->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBPB)) return 1; if (data & ~PRED_CMD_IBPB) return 1; - if (!boot_cpu_has(X86_FEATURE_AMD_IBPB)) + if (!boot_cpu_has(X86_FEATURE_IBPB)) return 1; if (!data) break; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index c3441e7e5a87..b74d2105ced7 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2028,7 +2028,10 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) break; case MSR_IA32_SPEC_CTRL: if (!msr_info->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_STIBP) && + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBRS) && + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_SSBD)) return 1; if (kvm_spec_ctrl_test_value(data)) @@ -2063,12 +2066,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) goto find_uret_msr; case MSR_IA32_PRED_CMD: if (!msr_info->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) + !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL) && + !guest_cpuid_has(vcpu, X86_FEATURE_AMD_IBPB)) return 1; if (data & ~PRED_CMD_IBPB) return 1; - if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL)) + if (!boot_cpu_has(X86_FEATURE_IBPB)) return 1; if (!data) break;
Until commit e7c587da1252 ("x86/speculation: Use synthetic bits for IBRS/IBPB/STIBP", 2018-05-17), KVM was testing both Intel and AMD CPUID bits before allowing the guest to write MSR_IA32_SPEC_CTRL and MSR_IA32_PRED_CMD. Testing only Intel bits on VMX processors, or only AMD bits on SVM processors, fails if the guests are created with the "opposite" vendor as the host. While at it, also tweak the host CPU check to use the vendor-agnostic feature bit X86_FEATURE_IBPB, since we only care about the availability of the MSR on the host here and not about specific CPUID bits. Fixes: e7c587da1252 ("x86/speculation: Use synthetic bits for IBRS/IBPB/STIBP") Cc: stable@vger.kernel.org Reported-by: Denis V. Lunev <den@openvz.org> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/svm/svm.c | 3 ++- arch/x86/kvm/vmx/vmx.c | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-)