Re: [PATCH 07/11] KVM: x86: Disable SMM for TDX

From: Sean Christopherson <seanjc@google.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.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>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, isaku.yamahata@intel.com,
	Kai Huang <kai.huang@intel.com>
Subject: Re: [PATCH 07/11] KVM: x86: Disable SMM for TDX
Date: Fri, 12 Nov 2021 18:35:28 +0000	[thread overview]
Message-ID: <YY6z8NoXyv4C2X9Z@google.com> (raw)
In-Reply-To: <YY6stGpsmZawyRy5@google.com>

On Fri, Nov 12, 2021, Sean Christopherson wrote:
> On Fri, Nov 12, 2021, Xiaoyao Li wrote:
> This patch neglects to disallow SMI via IPI.  Ditto for INIT+SIPI in the next
> patch.  And no small part of me thinks we shouldn't even bother handling the
> delivery mode in the MSI routing.  If we reject MSI configuration, then to be
> consistent we should also technically reject guest attempts to configure LVT
> entries.  Sadly, KVM doesn't handle any of that stuff correctly as there are
> assumptions left and right about how the guest will configure things like LVTT,
> but from an architctural perspective it is legal to configure LVT0, LVT1, LVTT,
> etc... to send SMI, NMI, INIT, etc...
> 
> The kvm_eoi_intercept_disallowed() part is a little different, since KVM can
> deliver the interrupt, it just can handle the backend correctly.  Dropping an
> event on the floor is a bit gross, but on the other hand I really don't want to
> sign up for a game of whack-a-mole for all the paths that can get to
> __apic_accept_irq().

...

> >  static int kvm_vcpu_ioctl_smi(struct kvm_vcpu *vcpu)
> >  {
> > +	if (kvm_smm_unsupported(vcpu->kvm))
> > +		return -EINVAL;
> > +

Oh, and despite saying we should "allow" the MSI routing, I do think we should
reject this ioctl().  The difference in my mind is that for the ioctl(), it's KVM's
"architecture" and we can define it as we see fit, whereas things like APIC delivery
mode are x86 architecture and we should do our best to avoid making things up.

We're obviously still making up behavior to some extent, but it's at least somewhat
aligned with hardware since I believe the architcture just says "undefined behavior"
for invalid modes/combinations, and AFAIK configuring invalid/reserved delivery modes
will "succeed" in the sense that the CPU won't explode on the write and the APIC
won't catch fire.