Re: [PATCH 1/2] KVM: X86: Move ignore_msrs handling upper the stack

[Sean Christopherson]

On Fri, Jun 26, 2020 at 02:07:32PM -0400, Peter Xu wrote:
> On Thu, Jun 25, 2020 at 09:25:40AM -0700, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 5eb618dbf211..64322446e590 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -1013,9 +1013,9 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> >                 *ebx = entry->ebx;
> >                 *ecx = entry->ecx;
> >                 *edx = entry->edx;
> > -               if (function == 7 && index == 0) {
> > +               if (function == 7 && index == 0 && (*ebx | (F(RTM) | F(HLE))) {
> >                         u64 data;
> > -                       if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
> > +                       if (!kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data) &&
> >                             (data & TSX_CTRL_CPUID_CLEAR))
> >                                 *ebx &= ~(F(RTM) | F(HLE));
> >                 }
> > 
> > 
> > On VMX, MSR_IA32_TSX_CTRL will be added to the so called shared MSR array
> > regardless of whether or not it is being advertised to userspace (this is
> > a bug in its own right).  Using the host_initiated variant means KVM will
> > incorrectly bypass VMX's ARCH_CAP_TSX_CTRL_MSR check, i.e. incorrectly
> > clear the bits if userspace is being weird and stuffed MSR_IA32_TSX_CTRL
> > without advertising it to the guest.
> 
> Btw, would it be more staightforward to check "vcpu->arch.arch_capabilities &
> ARCH_CAP_TSX_CTRL_MSR" rather than "*ebx | (F(RTM) | F(HLE))" even if we want
> to have such a fix?

Not really, That ends up duplicating the check in vmx_get_msr().  From an
emulation perspective, this really is a "guest" access to the MSR, in the
sense that it the virtual CPU is in the guest domain, i.e. not a god-like
entity that gets to break the rules of emulation.

The other thing to consider is that SVM doesn't have any knowledge of any
of this, so arguably the "ignored msr" crud should get logged for SVM as
it's effectively a userspace bug if they've configured the VM to have RTM
or HLE on a system that can't possibly support either.