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.
