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

Huang, Kai
2024-05-01T02:56:45Z

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.