Re: [RFC PATCH v3 23/59] KVM: x86: Allow host-initiated WRMSR to set X2APIC regardless of CPUID

From: Paolo Bonzini <pbonzini@redhat.com>
To: Thomas Gleixner <tglx@linutronix.de>,
	isaku.yamahata@intel.com, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	erdemaktas@google.com, Connor Kuehl <ckuehl@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc: isaku.yamahata@gmail.com,
	Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [RFC PATCH v3 23/59] KVM: x86: Allow host-initiated WRMSR to set X2APIC regardless of CPUID
Date: Fri, 26 Nov 2021 09:18:59 +0100	[thread overview]
Message-ID: <d449a4c2-131d-5406-b7a2-7549bacc02f9@redhat.com> (raw)
In-Reply-To: <87fsrkja4j.ffs@tglx>

On 11/25/21 20:41, Thomas Gleixner wrote:
> On Wed, Nov 24 2021 at 16:20, isaku yamahata wrote:
>> Let userspace, or in the case of TDX, KVM itself, enable X2APIC even if
>> X2APIC is not reported as supported in the guest's CPU model.  KVM
>> generally does not force specific ordering between ioctls(), e.g. this
>> forces userspace to configure CPUID before MSRs.  And for TDX, vCPUs
>> will always run with X2APIC enabled, e.g. KVM will want/need to enable
>> X2APIC from time zero.
> 
> This is complete crap. Fix the broken user space and do not add
> horrible hacks to the kernel.

tl;dr: I agree that it's a userspace issue but "configure CPUID before 
MSR" is not the issue (in fact QEMU calls KVM_SET_CPUID2 before any call 
to KVM_SET_MSRS).

We have quite a few other cases in which KVM_GET/SET_MSR is allowed to 
get/set MSRs in ways that the guests are not allowed to do.

In general, there are several reasons for this:

- simplifying userspace so that it can use the same list of MSRs for all 
guests (likely, the list that KVM provides with KVM_GET_MSR_INDEX_LIST). 
  For example MSR_TSC_AUX is only exposed to the guest if RDTSCP or 
RDPID are available, but the host can always access it.  This is usually 
the reason why host accesses to MSRs override CPUID.

- simplifying userspace so that it does not have to go through the 
various steps of a state machine; for example, it's okay if userspace 
goes DISABLED->X2APIC instead of having to do DISABLED->XAPIC->X2APIC.

- allowing userspace to set a reset value, for example overriding the 
lock bit in MSR_IA32_FEAT_CTL.

- read-only MSRs that are really "CPUID-like", i.e. they give the guest 
information about processor features (for example the VMX feature MSRs)

- MSRs had some weird limitations that were lifted later by introducing 
additional MSRs; for example KVM always allows the host to write to the 
full-width MSR_IA32_PMC0 counters, because they are a saner version of 
MSR_IA32_PERFCTR0 and there's no reason for userspace to inflict 
MSR_IA32_PERFCTR0 on userspace.

So the host_initiated check doesn't _necessarily_ count as a horrible 
hack in the kernel.  However, in this case we have a trusted domain 
without X2APIC.  I'm not sure this configuration is clearly bogus.  One 
could imagine special-purpose VMs that don't need interrupts at all. 
For full guests such as the ones that QEMU runs, I agree with Thomas 
that userspace must be fixed to enforce x2apic for TDX guests.

Paolo