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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux