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. 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
