On Tue, Mar 12, 2019 at 7:02 PM Xiaoyao Li <xiaoyao.li@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Mar 11, 2019 at 8:12 PM Xiaoyao Li <xiaoyao.li@xxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > > > > >
> > > > > > > Reported-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxxxxxxxx>
> > > > > > > Cc: Jim Mattson <jmattson@xxxxxxxxxx>
> > > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > > > > > ---
> > > > > > > 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.
