Re: [PATCH v19 023/130] KVM: TDX: Initialize the TDX module when loading the KVM intel kernel module

[Huang, Kai]

On 1/05/2024 4:13 am, Sean Christopherson wrote:
> On Tue, Apr 30, 2024, Kai Huang wrote:
> > On 30/04/2024 8:06 am, Sean Christopherson wrote:
> > > My suggestion is essentially "throw in a CR4.VMXE check before
> > > TDH.SYS.LP.INIT if it's easy".  If it's not easy for some reason, then don't do
> > > it.
> >
> > I see.  The disconnection between us is I am not super clear why we should
> > treat TDH.SYS.LP.INIT as a special one that deserves a CR4.VMXE check but
> > not other SEAMCALLs.
>
> Because TDH.SYS.LP.INIT is done on all CPUs via an IPI function call, is a one-
> time thing, and is at the intersection of core TDX and KVM module code, e.g. the
> the core TDX code has an explicit assumption that:
>
>   * This function assumes the caller has: 1) held read lock of CPU hotplug
>   * lock to prevent any new cpu from becoming online; 2) done both VMXON
>   * and tdx_cpu_enable() on all online cpus.

Yeah but from this perspective, both tdx_cpu_enable() and tdx_enable() 
are "a one time thing" and "at the intersection of core TDX and KVM"  :-)

But from the perspective that tdx_cpu_enable() must be called in IRQ 
disabled context, and there's no possibility that other thread/code 
could potentially mess up VMX enabling after the CR4.VMXE check, so it's 
fine to add such check.

And looking again, in fact the comment of tdx_cpu_enable() doesn't 
explicitly call out it requires the caller to do VMXON first (although 
kinda implied by the comment of tdx_enable() as you quoted above).

I can add a patch to make it more clear by calling out in the comment of 
tdx_cpu_enable() that it requires caller to do VMXON and adding a 
WARN_ON_ONCE(!CR4.VMXE) check inside.  I just don't know whether it is 
worth to do at this stage given it's not something mandatory because it 
requires review time from maintainers.  I can include such patch in next 
KVM TDX patchset if you prefer so we can see how it goes.

> KVM can obviously screw up and attempt SEAMCALLs without being post-VMXON, but
> that's entirely a _KVM_ bug.  And the probability of getting all the way to
> something like TDH_MEM_SEPT_ADD without being post-VMXON is comically low, e.g.
> KVM and/or the kernel would likely crash long before that point.

Yeah fully agree SEAMCALLs managed by KVM shouldn't need to do CR4.VMXE 
check.  I was talking about those involved in tdx_enable(), i.e., 
TDH.SYS.xxx.