| Message ID | 20190307234302.4092-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: fix ARCH_CAPBILITIES emulation | expand |
On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > regardless of hardware support under the pretense that KVM fully > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > that it's emulated on AMD hosts. > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always supported") And this one should have: Cc: stable@vger.kernel.org > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > Cc: Jim Mattson <jmattson@google.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/vmx/vmx.c | 14 -------------- > arch/x86/kvm/vmx/vmx.h | 1 - > arch/x86/kvm/x86.c | 13 +++++++++++++ > 4 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 7712b4ed8aa1..3d10ff38cbdb 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > bool tpr_access_reporting; > u64 ia32_xss; > u64 microcode_version; > + u64 arch_capabilities; > > /* > * Paging state of the vcpu > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 2a86d296c90f..02cf8a551bd1 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > break; > - case MSR_IA32_ARCH_CAPABILITIES: > - if (!msr_info->host_initiated && > - !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) > - return 1; > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > - break; > case MSR_IA32_SYSENTER_CS: > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > break; > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD, > MSR_TYPE_W); > break; > - case MSR_IA32_ARCH_CAPABILITIES: > - if (!msr_info->host_initiated || > - (data & ~kvm_get_arch_capabilities())) > - return 1; > - vmx->arch_capabilities = data; > - break; > case MSR_IA32_CR_PAT: > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data)) > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > ++vmx->nmsrs; > } > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > - > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > /* 22.2.1, 20.8.1 */ > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 1554cb45b393..a1e00d0a2482 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -190,7 +190,6 @@ struct vcpu_vmx { > u64 msr_guest_kernel_gs_base; > #endif > > - u64 arch_capabilities; > u64 spec_ctrl; > > u32 vm_entry_controls_shadow; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index d90f011f3e32..7bfeebc482c4 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > if (msr_info->host_initiated) > vcpu->arch.microcode_version = data; > break; > + case MSR_IA32_ARCH_CAPABILITIES: > + if (!msr_info->host_initiated || > + (data & ~kvm_get_arch_capabilities())) > + return 1; > + vcpu->arch.arch_capabilities = data; > + break; > case MSR_EFER: > return set_efer(vcpu, data); > case MSR_K7_HWCR: > @@ -2747,6 +2753,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > case MSR_IA32_UCODE_REV: > msr_info->data = vcpu->arch.microcode_version; > break; > + case MSR_IA32_ARCH_CAPABILITIES: > + if (!msr_info->host_initiated && > + !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) > + return 1; > + msr_info->data = vcpu->arch.arch_capabilities; > + break; > case MSR_IA32_TSC: > msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset; > break; > @@ -8743,6 +8755,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > > int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) > { > + vcpu->arch.arch_capabilities = kvm_get_arch_capabilities(); > vcpu->arch.msr_platform_info = MSR_PLATFORM_INFO_CPUID_FAULT; > kvm_vcpu_mtrr_init(vcpu); > vcpu_load(vcpu); > -- > 2.21.0 >
On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > regardless of hardware support under the pretense that KVM fully > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > that it's emulated on AMD hosts. > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always supported") > > And this one should have: > > Cc: stable@vger.kernel.org > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > Cc: Jim Mattson <jmattson@google.com> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/include/asm/kvm_host.h | 1 + > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > arch/x86/kvm/vmx/vmx.h | 1 - > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > bool tpr_access_reporting; > > u64 ia32_xss; > > u64 microcode_version; > > + u64 arch_capabilities; > > > > /* > > * Paging state of the vcpu > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index 2a86d296c90f..02cf8a551bd1 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > break; > > - case MSR_IA32_ARCH_CAPABILITIES: > > - if (!msr_info->host_initiated && > > - !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) > > - return 1; > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > - break; > > case MSR_IA32_SYSENTER_CS: > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > break; > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD, > > MSR_TYPE_W); > > break; > > - case MSR_IA32_ARCH_CAPABILITIES: > > - if (!msr_info->host_initiated || > > - (data & ~kvm_get_arch_capabilities())) > > - return 1; > > - vmx->arch_capabilities = data; > > - break; > > case MSR_IA32_CR_PAT: > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data)) > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > ++vmx->nmsrs; > > } > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > - > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > /* 22.2.1, 20.8.1 */ > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > index 1554cb45b393..a1e00d0a2482 100644 > > --- a/arch/x86/kvm/vmx/vmx.h > > +++ b/arch/x86/kvm/vmx/vmx.h > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > u64 msr_guest_kernel_gs_base; > > #endif > > > > - u64 arch_capabilities; > > u64 spec_ctrl; > > > > u32 vm_entry_controls_shadow; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index d90f011f3e32..7bfeebc482c4 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > if (msr_info->host_initiated) > > vcpu->arch.microcode_version = data; > > break; > > + case MSR_IA32_ARCH_CAPABILITIES: > > + if (!msr_info->host_initiated || > > + (data & ~kvm_get_arch_capabilities())) > > + return 1; As I mentioned in the PATCH 1 thread, we'd still like to be able to set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not enumerated on the host. Aside from that, this patch looks good to me. Thanks for the fix. I'll try to be more cognizant of AMD in the future.
On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > > regardless of hardware support under the pretense that KVM fully > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > > that it's emulated on AMD hosts. > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always > > > supported") > > > > And this one should have: > > > > Cc: stable@vger.kernel.org > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > Cc: Jim Mattson <jmattson@google.com> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > arch/x86/include/asm/kvm_host.h | 1 + > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > b/arch/x86/include/asm/kvm_host.h > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > bool tpr_access_reporting; > > > u64 ia32_xss; > > > u64 microcode_version; > > > + u64 arch_capabilities; > > > > > > /* > > > * Paging state of the vcpu > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > --- a/arch/x86/kvm/vmx/vmx.c > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > > > struct msr_data *msr_info) > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > break; > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > - if (!msr_info->host_initiated && > > > - !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) > > > - return 1; > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > - break; > > > case MSR_IA32_SYSENTER_CS: > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > break; > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > > > struct msr_data *msr_info) > > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, > > > MSR_IA32_PRED_CMD, > > > MSR_TYPE_W); > > > break; > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > - if (!msr_info->host_initiated || > > > - (data & ~kvm_get_arch_capabilities())) > > > - return 1; > > > - vmx->arch_capabilities = data; > > > - break; > > > case MSR_IA32_CR_PAT: > > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data)) > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > > ++vmx->nmsrs; > > > } > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > - > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > /* 22.2.1, 20.8.1 */ > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > index 1554cb45b393..a1e00d0a2482 100644 > > > --- a/arch/x86/kvm/vmx/vmx.h > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > u64 msr_guest_kernel_gs_base; > > > #endif > > > > > > - u64 arch_capabilities; > > > u64 spec_ctrl; > > > > > > u32 vm_entry_controls_shadow; > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index d90f011f3e32..7bfeebc482c4 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > > > struct msr_data *msr_info) > > > if (msr_info->host_initiated) > > > vcpu->arch.microcode_version = data; > > > break; > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > + if (!msr_info->host_initiated || > > > + (data & ~kvm_get_arch_capabilities())) > > > + return 1; > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > enumerated on the host. > > Aside from that, this patch looks good to me. Thanks for the fix. I'll > try to be more cognizant of AMD in the future. Hi, Jim, It's clear now. It is your negligence that simply enforce this cpuid for all x86 arch. As a result, here comes some fixes patches, besides this series I have sent out another (kvm/x86: Move MSR_IA32_ARCH_CAPABILITIES to array emulated_msrs) https://patchwork.kernel.org/patch/10844303/ Hi, Paolo, Here is another thing that I need a decision or conclusion from you. MSR CORE_CPABILITY acts as a feature-enumerating MSR as same as MSR ARCH_CAPABILITIES. From my fix patch (kvm/x86/vmx: Make the emulation of MSR_IA32_ARCH_CAPABILITIES only for vmx) https://patchwork.kernel.org/patch/10842499/ , you said dropping CPUID flag is generally not a good idea, because it would change the guest ABI for AMD processors. I didn't do any test on AMD processor and I cannot tell anything. Anyway, as Jim's patch has merged into linus' tree for a while, using Sean's this series to fix it is better. However, when it comes to MSR CORE_CAPABILITIES, it faces the same situation. So we should make a conclusion that, when emulating a feature-enumeraing MSR should we expose it to vendor-specific or just expose to all x86 arch even though it may be the dead code for other vendor? Thanks, -Xiaoyao
On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> wrote: > > On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > > <sean.j.christopherson@intel.com> wrote: > > > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > > > regardless of hardware support under the pretense that KVM fully > > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > > > that it's emulated on AMD hosts. > > > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always > > > > supported") > > > > > > And this one should have: > > > > > > Cc: stable@vger.kernel.org > > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > Cc: Jim Mattson <jmattson@google.com> > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > --- > > > > arch/x86/include/asm/kvm_host.h | 1 + > > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > > b/arch/x86/include/asm/kvm_host.h > > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > > bool tpr_access_reporting; > > > > u64 ia32_xss; > > > > u64 microcode_version; > > > > + u64 arch_capabilities; > > > > > > > > /* > > > > * Paging state of the vcpu > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > > > > struct msr_data *msr_info) > > > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > > break; > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > - if (!msr_info->host_initiated && > > > > - !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) > > > > - return 1; > > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > > - break; > > > > case MSR_IA32_SYSENTER_CS: > > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > > break; > > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > > > > struct msr_data *msr_info) > > > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, > > > > MSR_IA32_PRED_CMD, > > > > MSR_TYPE_W); > > > > break; > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > - if (!msr_info->host_initiated || > > > > - (data & ~kvm_get_arch_capabilities())) > > > > - return 1; > > > > - vmx->arch_capabilities = data; > > > > - break; > > > > case MSR_IA32_CR_PAT: > > > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data)) > > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > > > ++vmx->nmsrs; > > > > } > > > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > > - > > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > > > /* 22.2.1, 20.8.1 */ > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > > index 1554cb45b393..a1e00d0a2482 100644 > > > > --- a/arch/x86/kvm/vmx/vmx.h > > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > > u64 msr_guest_kernel_gs_base; > > > > #endif > > > > > > > > - u64 arch_capabilities; > > > > u64 spec_ctrl; > > > > > > > > u32 vm_entry_controls_shadow; > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > index d90f011f3e32..7bfeebc482c4 100644 > > > > --- a/arch/x86/kvm/x86.c > > > > +++ b/arch/x86/kvm/x86.c > > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > > > > struct msr_data *msr_info) > > > > if (msr_info->host_initiated) > > > > vcpu->arch.microcode_version = data; > > > > break; > > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > > + if (!msr_info->host_initiated || > > > > + (data & ~kvm_get_arch_capabilities())) > > > > + return 1; > > > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > > enumerated on the host. > > > > Aside from that, this patch looks good to me. Thanks for the fix. I'll > > try to be more cognizant of AMD in the future. > > Hi, Jim, > > It's clear now. It is your negligence that simply enforce this cpuid for all x86 > arch. Ouch! That's harsh...and a bit supercilious. It didn't occur to me at the time, but kvm *should* emulate IA32_ARCH_CAPABILITIES for AMD as well as Intel, to support cross-vendor migration. If there are any Intel machines in the migration pool that are vulnerable to exploits of empty RSB conditions, userspace should be reporting IA32_ARCH_CAPABILITIES.RSBA regardless of virtual CPU vendor, and to do so, CPUID.(EAX=07H,ECX=00H):EDX[29] must be enumerated as well. Again, thanks for fixing this.
On Tue, Mar 12, 2019 at 11:45 AM Jim Mattson <jmattson@google.com> wrote: > > On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> wrote: > > > > On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > > > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > > > <sean.j.christopherson@intel.com> wrote: > > > > > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > > > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > > > > regardless of hardware support under the pretense that KVM fully > > > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > > > > that it's emulated on AMD hosts. > > > > > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always > > > > > supported") > > > > > > > > And this one should have: > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > > Cc: Jim Mattson <jmattson@google.com> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > --- > > > > > arch/x86/include/asm/kvm_host.h | 1 + > > > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > > > b/arch/x86/include/asm/kvm_host.h > > > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > > > bool tpr_access_reporting; > > > > > u64 ia32_xss; > > > > > u64 microcode_version; > > > > > + u64 arch_capabilities; > > > > > > > > > > /* > > > > > * Paging state of the vcpu > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > > > > > struct msr_data *msr_info) > > > > > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > > > break; > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > - if (!msr_info->host_initiated && > > > > > - !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) > > > > > - return 1; > > > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > > > - break; > > > > > case MSR_IA32_SYSENTER_CS: > > > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > > > break; > > > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > > > > > struct msr_data *msr_info) > > > > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, > > > > > MSR_IA32_PRED_CMD, > > > > > MSR_TYPE_W); > > > > > break; > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > - if (!msr_info->host_initiated || > > > > > - (data & ~kvm_get_arch_capabilities())) > > > > > - return 1; > > > > > - vmx->arch_capabilities = data; > > > > > - break; > > > > > case MSR_IA32_CR_PAT: > > > > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data)) > > > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > > > > ++vmx->nmsrs; > > > > > } > > > > > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > > > - > > > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > > > > > /* 22.2.1, 20.8.1 */ > > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > > > index 1554cb45b393..a1e00d0a2482 100644 > > > > > --- a/arch/x86/kvm/vmx/vmx.h > > > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > > > u64 msr_guest_kernel_gs_base; > > > > > #endif > > > > > > > > > > - u64 arch_capabilities; > > > > > u64 spec_ctrl; > > > > > > > > > > u32 vm_entry_controls_shadow; > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > > index d90f011f3e32..7bfeebc482c4 100644 > > > > > --- a/arch/x86/kvm/x86.c > > > > > +++ b/arch/x86/kvm/x86.c > > > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > > > > > struct msr_data *msr_info) > > > > > if (msr_info->host_initiated) > > > > > vcpu->arch.microcode_version = data; > > > > > break; > > > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > > > + if (!msr_info->host_initiated || > > > > > + (data & ~kvm_get_arch_capabilities())) > > > > > + return 1; > > > > > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > > > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > > > enumerated on the host. > > > > > > Aside from that, this patch looks good to me. Thanks for the fix. I'll > > > try to be more cognizant of AMD in the future. > > > > Hi, Jim, > > > > It's clear now. It is your negligence that simply enforce this cpuid for all x86 > > arch. > > Ouch! That's harsh...and a bit supercilious. > > It didn't occur to me at the time, but kvm *should* emulate > IA32_ARCH_CAPABILITIES for AMD as well as Intel, to support > cross-vendor migration. If there are any Intel machines in the > migration pool that are vulnerable to exploits of empty RSB > conditions, userspace should be reporting IA32_ARCH_CAPABILITIES.RSBA > regardless of virtual CPU vendor, and to do so, > CPUID.(EAX=07H,ECX=00H):EDX[29] must be enumerated as well. > > Again, thanks for fixing this. Note that you should probably also remove the 'case MSR_IA32_ARCH_CAPABILITIES' from svm_has_emulated_msr().
On Tue, 2019-03-12 at 12:02 -0700, Jim Mattson wrote: > On Tue, Mar 12, 2019 at 11:45 AM Jim Mattson <jmattson@google.com> wrote: > > > > On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> > > wrote: > > > > > > On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > > > > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > > > > <sean.j.christopherson@intel.com> wrote: > > > > > > > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > > > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > > > > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > > > > > regardless of hardware support under the pretense that KVM fully > > > > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > > > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > > > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > > > > > that it's emulated on AMD hosts. > > > > > > > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always > > > > > > supported") > > > > > > > > > > And this one should have: > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > > > Cc: Jim Mattson <jmattson@google.com> > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > --- > > > > > > arch/x86/include/asm/kvm_host.h | 1 + > > > > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > > > > b/arch/x86/include/asm/kvm_host.h > > > > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > > > > bool tpr_access_reporting; > > > > > > u64 ia32_xss; > > > > > > u64 microcode_version; > > > > > > + u64 arch_capabilities; > > > > > > > > > > > > /* > > > > > > * Paging state of the vcpu > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > > > > > > struct msr_data *msr_info) > > > > > > > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > > > > break; > > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > > - if (!msr_info->host_initiated && > > > > > > - !guest_cpuid_has(vcpu, > > > > > > X86_FEATURE_ARCH_CAPABILITIES)) > > > > > > - return 1; > > > > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > > > > - break; > > > > > > case MSR_IA32_SYSENTER_CS: > > > > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > > > > break; > > > > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > > > > > > struct msr_data *msr_info) > > > > > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, > > > > > > MSR_IA32_PRED_CMD, > > > > > > MSR_TYPE_W); > > > > > > break; > > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > > - if (!msr_info->host_initiated || > > > > > > - (data & ~kvm_get_arch_capabilities())) > > > > > > - return 1; > > > > > > - vmx->arch_capabilities = data; > > > > > > - break; > > > > > > case MSR_IA32_CR_PAT: > > > > > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) > > > > > > { > > > > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, > > > > > > data)) > > > > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx > > > > > > *vmx) > > > > > > ++vmx->nmsrs; > > > > > > } > > > > > > > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > > > > - > > > > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > > > > > > > /* 22.2.1, 20.8.1 */ > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > > > > index 1554cb45b393..a1e00d0a2482 100644 > > > > > > --- a/arch/x86/kvm/vmx/vmx.h > > > > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > > > > u64 msr_guest_kernel_gs_base; > > > > > > #endif > > > > > > > > > > > > - u64 arch_capabilities; > > > > > > u64 spec_ctrl; > > > > > > > > > > > > u32 vm_entry_controls_shadow; > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > > > index d90f011f3e32..7bfeebc482c4 100644 > > > > > > --- a/arch/x86/kvm/x86.c > > > > > > +++ b/arch/x86/kvm/x86.c > > > > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > > > > > > struct msr_data *msr_info) > > > > > > if (msr_info->host_initiated) > > > > > > vcpu->arch.microcode_version = data; > > > > > > break; > > > > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > > > > + if (!msr_info->host_initiated || > > > > > > + (data & ~kvm_get_arch_capabilities())) > > > > > > + return 1; > > > > > > > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > > > > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > > > > enumerated on the host. > > > > > > > > Aside from that, this patch looks good to me. Thanks for the fix. I'll > > > > try to be more cognizant of AMD in the future. > > > > > > Hi, Jim, > > > > > > It's clear now. It is your negligence that simply enforce this cpuid for > > > all x86 > > > arch. > > > > Ouch! That's harsh...and a bit supercilious. > > > > It didn't occur to me at the time, but kvm *should* emulate > > IA32_ARCH_CAPABILITIES for AMD as well as Intel, to support > > cross-vendor migration. If there are any Intel machines in the > > migration pool that are vulnerable to exploits of empty RSB > > conditions, userspace should be reporting IA32_ARCH_CAPABILITIES.RSBA > > regardless of virtual CPU vendor, and to do so, > > CPUID.(EAX=07H,ECX=00H):EDX[29] must be enumerated as well. > > > > Again, thanks for fixing this. > > Note that you should probably also remove the 'case > MSR_IA32_ARCH_CAPABILITIES' from svm_has_emulated_msr(). Thanks for your reminder. We don't need to remove the 'case MSR_IA32_ARCH_CAPABILITIES', because there doesn't have and svm_has_emulated_msr() return true by default.
On Tue, 2019-03-12 at 11:45 -0700, Jim Mattson wrote: > On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> wrote: > > > > On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > > > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > > > <sean.j.christopherson@intel.com> wrote: > > > > > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > > > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > > > > regardless of hardware support under the pretense that KVM fully > > > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > > > > that it's emulated on AMD hosts. > > > > > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always > > > > > supported") > > > > > > > > And this one should have: > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > > Cc: Jim Mattson <jmattson@google.com> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > --- > > > > > arch/x86/include/asm/kvm_host.h | 1 + > > > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > > > b/arch/x86/include/asm/kvm_host.h > > > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > > > bool tpr_access_reporting; > > > > > u64 ia32_xss; > > > > > u64 microcode_version; > > > > > + u64 arch_capabilities; > > > > > > > > > > /* > > > > > * Paging state of the vcpu > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > > > > > struct msr_data *msr_info) > > > > > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > > > break; > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > - if (!msr_info->host_initiated && > > > > > - !guest_cpuid_has(vcpu, > > > > > X86_FEATURE_ARCH_CAPABILITIES)) > > > > > - return 1; > > > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > > > - break; > > > > > case MSR_IA32_SYSENTER_CS: > > > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > > > break; > > > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > > > > > struct msr_data *msr_info) > > > > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, > > > > > MSR_IA32_PRED_CMD, > > > > > MSR_TYPE_W); > > > > > break; > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > - if (!msr_info->host_initiated || > > > > > - (data & ~kvm_get_arch_capabilities())) > > > > > - return 1; > > > > > - vmx->arch_capabilities = data; > > > > > - break; > > > > > case MSR_IA32_CR_PAT: > > > > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { > > > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, > > > > > data)) > > > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) > > > > > ++vmx->nmsrs; > > > > > } > > > > > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > > > - > > > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > > > > > /* 22.2.1, 20.8.1 */ > > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > > > index 1554cb45b393..a1e00d0a2482 100644 > > > > > --- a/arch/x86/kvm/vmx/vmx.h > > > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > > > u64 msr_guest_kernel_gs_base; > > > > > #endif > > > > > > > > > > - u64 arch_capabilities; > > > > > u64 spec_ctrl; > > > > > > > > > > u32 vm_entry_controls_shadow; > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > > index d90f011f3e32..7bfeebc482c4 100644 > > > > > --- a/arch/x86/kvm/x86.c > > > > > +++ b/arch/x86/kvm/x86.c > > > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > > > > > struct msr_data *msr_info) > > > > > if (msr_info->host_initiated) > > > > > vcpu->arch.microcode_version = data; > > > > > break; > > > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > > > + if (!msr_info->host_initiated || > > > > > + (data & ~kvm_get_arch_capabilities())) > > > > > + return 1; > > > > > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > > > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > > > enumerated on the host. > > > > > > Aside from that, this patch looks good to me. Thanks for the fix. I'll > > > try to be more cognizant of AMD in the future. > > > > Hi, Jim, > > > > It's clear now. It is your negligence that simply enforce this cpuid for all > > x86 > > arch. > > Ouch! That's harsh...and a bit supercilious. Sorry, no offense. > It didn't occur to me at the time, but kvm *should* emulate > IA32_ARCH_CAPABILITIES for AMD as well as Intel, to support > cross-vendor migration. If there are any Intel machines in the > migration pool that are vulnerable to exploits of empty RSB > conditions, userspace should be reporting IA32_ARCH_CAPABILITIES.RSBA > regardless of virtual CPU vendor, and to do so, > CPUID.(EAX=07H,ECX=00H):EDX[29] must be enumerated as well. > > Again, thanks for fixing this.
On Tue, Mar 12, 2019 at 7:02 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> wrote: > > On Tue, 2019-03-12 at 12:02 -0700, Jim Mattson wrote: > > On Tue, Mar 12, 2019 at 11:45 AM Jim Mattson <jmattson@google.com> wrote: > > > > > > On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > wrote: > > > > > > > > On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > > > > > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > > > > > <sean.j.christopherson@intel.com> wrote: > > > > > > > > > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson wrote: > > > > > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host > > > > > > > userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES > > > > > > > regardless of hardware support under the pretense that KVM fully > > > > > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts > > > > > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS > > > > > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > > > > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so > > > > > > > that it's emulated on AMD hosts. > > > > > > > > > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always > > > > > > > supported") > > > > > > > > > > > > And this one should have: > > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > > > > Cc: Jim Mattson <jmattson@google.com> > > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > > > --- > > > > > > > arch/x86/include/asm/kvm_host.h | 1 + > > > > > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > > > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > > > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > > > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > > > > > b/arch/x86/include/asm/kvm_host.h > > > > > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > > > > > bool tpr_access_reporting; > > > > > > > u64 ia32_xss; > > > > > > > u64 microcode_version; > > > > > > > + u64 arch_capabilities; > > > > > > > > > > > > > > /* > > > > > > > * Paging state of the vcpu > > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > > > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, > > > > > > > struct msr_data *msr_info) > > > > > > > > > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > > > > > break; > > > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > > > - if (!msr_info->host_initiated && > > > > > > > - !guest_cpuid_has(vcpu, > > > > > > > X86_FEATURE_ARCH_CAPABILITIES)) > > > > > > > - return 1; > > > > > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > > > > > - break; > > > > > > > case MSR_IA32_SYSENTER_CS: > > > > > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > > > > > break; > > > > > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, > > > > > > > struct msr_data *msr_info) > > > > > > > vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, > > > > > > > MSR_IA32_PRED_CMD, > > > > > > > MSR_TYPE_W); > > > > > > > break; > > > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > > > - if (!msr_info->host_initiated || > > > > > > > - (data & ~kvm_get_arch_capabilities())) > > > > > > > - return 1; > > > > > > > - vmx->arch_capabilities = data; > > > > > > > - break; > > > > > > > case MSR_IA32_CR_PAT: > > > > > > > if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) > > > > > > > { > > > > > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, > > > > > > > data)) > > > > > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx > > > > > > > *vmx) > > > > > > > ++vmx->nmsrs; > > > > > > > } > > > > > > > > > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > > > > > - > > > > > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > > > > > > > > > /* 22.2.1, 20.8.1 */ > > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > > > > > index 1554cb45b393..a1e00d0a2482 100644 > > > > > > > --- a/arch/x86/kvm/vmx/vmx.h > > > > > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > > > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > > > > > u64 msr_guest_kernel_gs_base; > > > > > > > #endif > > > > > > > > > > > > > > - u64 arch_capabilities; > > > > > > > u64 spec_ctrl; > > > > > > > > > > > > > > u32 vm_entry_controls_shadow; > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > > > > index d90f011f3e32..7bfeebc482c4 100644 > > > > > > > --- a/arch/x86/kvm/x86.c > > > > > > > +++ b/arch/x86/kvm/x86.c > > > > > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > > > > > > > struct msr_data *msr_info) > > > > > > > if (msr_info->host_initiated) > > > > > > > vcpu->arch.microcode_version = data; > > > > > > > break; > > > > > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > > > > > + if (!msr_info->host_initiated || > > > > > > > + (data & ~kvm_get_arch_capabilities())) > > > > > > > + return 1; > > > > > > > > > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > > > > > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > > > > > enumerated on the host. > > > > > > > > > > Aside from that, this patch looks good to me. Thanks for the fix. I'll > > > > > try to be more cognizant of AMD in the future. > > > > > > > > Hi, Jim, > > > > > > > > It's clear now. It is your negligence that simply enforce this cpuid for > > > > all x86 > > > > arch. > > > > > > Ouch! That's harsh...and a bit supercilious. > > > > > > It didn't occur to me at the time, but kvm *should* emulate > > > IA32_ARCH_CAPABILITIES for AMD as well as Intel, to support > > > cross-vendor migration. If there are any Intel machines in the > > > migration pool that are vulnerable to exploits of empty RSB > > > conditions, userspace should be reporting IA32_ARCH_CAPABILITIES.RSBA > > > regardless of virtual CPU vendor, and to do so, > > > CPUID.(EAX=07H,ECX=00H):EDX[29] must be enumerated as well. > > > > > > Again, thanks for fixing this. > > > > Note that you should probably also remove the 'case > > MSR_IA32_ARCH_CAPABILITIES' from svm_has_emulated_msr(). > > Thanks for your reminder. We don't need to remove the 'case > MSR_IA32_ARCH_CAPABILITIES', because there doesn't have and > svm_has_emulated_msr() return true by default. Ah. That's because Paolo never applied my changes to fix the AMD issue. See https://patchwork.kernel.org/patch/10784295/ and https://patchwork.kernel.org/patch/10784297/. The local branch I was looking at still has those changes.
On Tue, 2019-03-12 at 20:40 -0700, Jim Mattson wrote: > On Tue, Mar 12, 2019 at 7:02 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> wrote: > > > > On Tue, 2019-03-12 at 12:02 -0700, Jim Mattson wrote: > > > On Tue, Mar 12, 2019 at 11:45 AM Jim Mattson <jmattson@google.com> wrote: > > > > > > > > On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > wrote: > > > > > > > > > > On Mon, 2019-03-11 at 14:59 -0700, Jim Mattson wrote: > > > > > > On Fri, Mar 8, 2019 at 7:32 AM Sean Christopherson > > > > > > <sean.j.christopherson@intel.com> wrote: > > > > > > > > > > > > > > On Thu, Mar 07, 2019 at 03:43:02PM -0800, Sean Christopherson > > > > > > > wrote: > > > > > > > > The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to > > > > > > > > host > > > > > > > > userspace for all x86 hosts, i.e. KVM advertises > > > > > > > > ARCH_CAPABILITIES > > > > > > > > regardless of hardware support under the pretense that KVM fully > > > > > > > > emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX > > > > > > > > hosts > > > > > > > > handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite > > > > > > > > KVM_GET_MSRS > > > > > > > > also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). > > > > > > > > > > > > > > > > Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code > > > > > > > > so > > > > > > > > that it's emulated on AMD hosts. > > > > > > > > > > > > > > > > Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is > > > > > > > > always > > > > > > > > supported") > > > > > > > > > > > > > > And this one should have: > > > > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > > > > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> > > > > > > > > Cc: Jim Mattson <jmattson@google.com> > > > > > > > > Signed-off-by: Sean Christopherson < > > > > > > > > sean.j.christopherson@intel.com> > > > > > > > > --- > > > > > > > > arch/x86/include/asm/kvm_host.h | 1 + > > > > > > > > arch/x86/kvm/vmx/vmx.c | 14 -------------- > > > > > > > > arch/x86/kvm/vmx/vmx.h | 1 - > > > > > > > > arch/x86/kvm/x86.c | 13 +++++++++++++ > > > > > > > > 4 files changed, 14 insertions(+), 15 deletions(-) > > > > > > > > > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h > > > > > > > > b/arch/x86/include/asm/kvm_host.h > > > > > > > > index 7712b4ed8aa1..3d10ff38cbdb 100644 > > > > > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > > > > > @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { > > > > > > > > bool tpr_access_reporting; > > > > > > > > u64 ia32_xss; > > > > > > > > u64 microcode_version; > > > > > > > > + u64 arch_capabilities; > > > > > > > > > > > > > > > > /* > > > > > > > > * Paging state of the vcpu > > > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > > > > > > > index 2a86d296c90f..02cf8a551bd1 100644 > > > > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > > > > @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu > > > > > > > > *vcpu, > > > > > > > > struct msr_data *msr_info) > > > > > > > > > > > > > > > > msr_info->data = to_vmx(vcpu)->spec_ctrl; > > > > > > > > break; > > > > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > > > > - if (!msr_info->host_initiated && > > > > > > > > - !guest_cpuid_has(vcpu, > > > > > > > > X86_FEATURE_ARCH_CAPABILITIES)) > > > > > > > > - return 1; > > > > > > > > - msr_info->data = to_vmx(vcpu)->arch_capabilities; > > > > > > > > - break; > > > > > > > > case MSR_IA32_SYSENTER_CS: > > > > > > > > msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); > > > > > > > > break; > > > > > > > > @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu > > > > > > > > *vcpu, > > > > > > > > struct msr_data *msr_info) > > > > > > > > vmx_disable_intercept_for_msr(vmx- > > > > > > > > >vmcs01.msr_bitmap, > > > > > > > > MSR_IA32_PRED_CMD, > > > > > > > > MSR_TYPE_W); > > > > > > > > break; > > > > > > > > - case MSR_IA32_ARCH_CAPABILITIES: > > > > > > > > - if (!msr_info->host_initiated || > > > > > > > > - (data & ~kvm_get_arch_capabilities())) > > > > > > > > - return 1; > > > > > > > > - vmx->arch_capabilities = data; > > > > > > > > - break; > > > > > > > > case MSR_IA32_CR_PAT: > > > > > > > > if (vmcs_config.vmentry_ctrl & > > > > > > > > VM_ENTRY_LOAD_IA32_PAT) > > > > > > > > { > > > > > > > > if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, > > > > > > > > data)) > > > > > > > > @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx > > > > > > > > *vmx) > > > > > > > > ++vmx->nmsrs; > > > > > > > > } > > > > > > > > > > > > > > > > - vmx->arch_capabilities = kvm_get_arch_capabilities(); > > > > > > > > - > > > > > > > > vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); > > > > > > > > > > > > > > > > /* 22.2.1, 20.8.1 */ > > > > > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > > > > > > > > index 1554cb45b393..a1e00d0a2482 100644 > > > > > > > > --- a/arch/x86/kvm/vmx/vmx.h > > > > > > > > +++ b/arch/x86/kvm/vmx/vmx.h > > > > > > > > @@ -190,7 +190,6 @@ struct vcpu_vmx { > > > > > > > > u64 msr_guest_kernel_gs_base; > > > > > > > > #endif > > > > > > > > > > > > > > > > - u64 arch_capabilities; > > > > > > > > u64 spec_ctrl; > > > > > > > > > > > > > > > > u32 vm_entry_controls_shadow; > > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > > > > > index d90f011f3e32..7bfeebc482c4 100644 > > > > > > > > --- a/arch/x86/kvm/x86.c > > > > > > > > +++ b/arch/x86/kvm/x86.c > > > > > > > > @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu > > > > > > > > *vcpu, > > > > > > > > struct msr_data *msr_info) > > > > > > > > if (msr_info->host_initiated) > > > > > > > > vcpu->arch.microcode_version = data; > > > > > > > > break; > > > > > > > > + case MSR_IA32_ARCH_CAPABILITIES: > > > > > > > > + if (!msr_info->host_initiated || > > > > > > > > + (data & ~kvm_get_arch_capabilities())) > > > > > > > > + return 1; > > > > > > > > > > > > As I mentioned in the PATCH 1 thread, we'd still like to be able to > > > > > > set IA32_ARCH_CAPABILITIES.RSBA from userspace, even if it's not > > > > > > enumerated on the host. > > > > > > > > > > > > Aside from that, this patch looks good to me. Thanks for the fix. > > > > > > I'll > > > > > > try to be more cognizant of AMD in the future. > > > > > > > > > > Hi, Jim, > > > > > > > > > > It's clear now. It is your negligence that simply enforce this cpuid > > > > > for > > > > > all x86 > > > > > arch. > > > > > > > > Ouch! That's harsh...and a bit supercilious. > > > > > > > > It didn't occur to me at the time, but kvm *should* emulate > > > > IA32_ARCH_CAPABILITIES for AMD as well as Intel, to support > > > > cross-vendor migration. If there are any Intel machines in the > > > > migration pool that are vulnerable to exploits of empty RSB > > > > conditions, userspace should be reporting IA32_ARCH_CAPABILITIES.RSBA > > > > regardless of virtual CPU vendor, and to do so, > > > > CPUID.(EAX=07H,ECX=00H):EDX[29] must be enumerated as well. > > > > > > > > Again, thanks for fixing this. > > > > > > Note that you should probably also remove the 'case > > > MSR_IA32_ARCH_CAPABILITIES' from svm_has_emulated_msr(). > > > > Thanks for your reminder. We don't need to remove the 'case > > MSR_IA32_ARCH_CAPABILITIES', because there doesn't have and > > svm_has_emulated_msr() return true by default. > > Ah. That's because Paolo never applied my changes to fix the AMD > issue. See https://patchwork.kernel.org/patch/10784295/ and > https://patchwork.kernel.org/patch/10784297/. The local branch I was > looking at still has those changes. Yes, I found your fix patches in the maillist just now. I am so sorry about sending the fix patches without checking you have already sent out fix serires, and so sorry about the reply yesterday. I might use the wrong word, anyway, no offense. And now, as you said, kvm *should* emulate IA32_ARCH_CAPABILITIES for AMD as well as Intel. Sean's this patch fixes the most part. Also we need your patch https://patchwork.kernel.org/patch/10784297/. to move IA32_ARCH_CAPABILITIES from msrs_to_save[] to emulated_msrs[], while removing the 'case MSR_IA32_ARCH_CAPABILITIES' part. That's enough, right?
On Tue, Mar 12, 2019 at 9:13 PM Xiaoyao Li <xiaoyao.li@linux.intel.com> wrote: > Yes, I found your fix patches in the maillist just now. > I am so sorry about sending the fix patches without checking you have already > sent out fix serires, and so sorry about the reply yesterday. I might use the > wrong word, anyway, no offense. No problem. It's a word I imagine you're hearing a lot at Intel these days. :-) > And now, as you said, kvm *should* emulate IA32_ARCH_CAPABILITIES for AMD as > well as Intel. Sean's this patch fixes the most part. Also we need your patch > https://patchwork.kernel.org/patch/10784297/. to move IA32_ARCH_CAPABILITIES > from msrs_to_save[] to emulated_msrs[], while removing the 'case > MSR_IA32_ARCH_CAPABILITIES' part. > > That's enough, right? At the risk of beating a dead horse, yes, with the caveat that userspace should not be restricted from setting bits in the virtual MSR that aren't set in the physical MSR.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 7712b4ed8aa1..3d10ff38cbdb 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -568,6 +568,7 @@ struct kvm_vcpu_arch { bool tpr_access_reporting; u64 ia32_xss; u64 microcode_version; + u64 arch_capabilities; /* * Paging state of the vcpu diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 2a86d296c90f..02cf8a551bd1 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1682,12 +1682,6 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) msr_info->data = to_vmx(vcpu)->spec_ctrl; break; - case MSR_IA32_ARCH_CAPABILITIES: - if (!msr_info->host_initiated && - !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) - return 1; - msr_info->data = to_vmx(vcpu)->arch_capabilities; - break; case MSR_IA32_SYSENTER_CS: msr_info->data = vmcs_read32(GUEST_SYSENTER_CS); break; @@ -1894,12 +1888,6 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) vmx_disable_intercept_for_msr(vmx->vmcs01.msr_bitmap, MSR_IA32_PRED_CMD, MSR_TYPE_W); break; - case MSR_IA32_ARCH_CAPABILITIES: - if (!msr_info->host_initiated || - (data & ~kvm_get_arch_capabilities())) - return 1; - vmx->arch_capabilities = data; - break; case MSR_IA32_CR_PAT: if (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) { if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data)) @@ -4088,8 +4076,6 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx) ++vmx->nmsrs; } - vmx->arch_capabilities = kvm_get_arch_capabilities(); - vm_exit_controls_init(vmx, vmx_vmexit_ctrl()); /* 22.2.1, 20.8.1 */ diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index 1554cb45b393..a1e00d0a2482 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -190,7 +190,6 @@ struct vcpu_vmx { u64 msr_guest_kernel_gs_base; #endif - u64 arch_capabilities; u64 spec_ctrl; u32 vm_entry_controls_shadow; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index d90f011f3e32..7bfeebc482c4 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2443,6 +2443,12 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) if (msr_info->host_initiated) vcpu->arch.microcode_version = data; break; + case MSR_IA32_ARCH_CAPABILITIES: + if (!msr_info->host_initiated || + (data & ~kvm_get_arch_capabilities())) + return 1; + vcpu->arch.arch_capabilities = data; + break; case MSR_EFER: return set_efer(vcpu, data); case MSR_K7_HWCR: @@ -2747,6 +2753,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) case MSR_IA32_UCODE_REV: msr_info->data = vcpu->arch.microcode_version; break; + case MSR_IA32_ARCH_CAPABILITIES: + if (!msr_info->host_initiated && + !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES)) + return 1; + msr_info->data = vcpu->arch.arch_capabilities; + break; case MSR_IA32_TSC: msr_info->data = kvm_scale_tsc(vcpu, rdtsc()) + vcpu->arch.tsc_offset; break; @@ -8743,6 +8755,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu) { + vcpu->arch.arch_capabilities = kvm_get_arch_capabilities(); vcpu->arch.msr_platform_info = MSR_PLATFORM_INFO_CPUID_FAULT; kvm_vcpu_mtrr_init(vcpu); vcpu_load(vcpu);
The CPUID flag ARCH_CAPABILITIES is unconditioinally exposed to host userspace for all x86 hosts, i.e. KVM advertises ARCH_CAPABILITIES regardless of hardware support under the pretense that KVM fully emulates MSR_IA32_ARCH_CAPABILITIES. Unfortunately, only VMX hosts handle accesses to MSR_IA32_ARCH_CAPABILITIES (despite KVM_GET_MSRS also reporting MSR_IA32_ARCH_CAPABILITIES for all hosts). Move the MSR_IA32_ARCH_CAPABILITIES handling to common x86 code so that it's emulated on AMD hosts. Fixes: 1eaafe91a0df4 ("kvm: x86: IA32_ARCH_CAPABILITIES is always supported") Reported-by: Xiaoyao Li <xiaoyao.li@linux.intel.com> Cc: Jim Mattson <jmattson@google.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/vmx/vmx.c | 14 -------------- arch/x86/kvm/vmx/vmx.h | 1 - arch/x86/kvm/x86.c | 13 +++++++++++++ 4 files changed, 14 insertions(+), 15 deletions(-)