On Mon, Apr 29, 2024, Kai Huang wrote:
On Thu, 2024-04-25 at 15:43 -0700, Sean Christopherson wrote:
And the odd is currently the common SEAMCALL functions, a.k.a,
__seamcall() and seamcall() (the latter is a mocro actually), both return
u64, so if we want to have such CR4.VMX check code in the common code, we
need to invent a new error code for it.
Oh, I wasn't thinking that we'd check CR4.VMXE before *every* SEAMCALL, just
before the TDH.SYS.LP.INIT call, i.e. before the one that is most likely to fail
due to a software bug that results in the CPU not doing VMXON before enabling
TDX.
Again, my intent is to add a simple, cheap, and targeted sanity check to help
deal with potential failures in code that historically has been less than rock
solid, and in function that has a big fat assumption that the caller has done
VMXON on the CPU.
I see.
(To be fair, personally I don't recall that we ever had any bug due to
"cpu not in post-VMXON before SEAMCALL", but maybe it's just me. :-).)
But if tdx_enable() doesn't call tdx_cpu_enable() internally, then we will
have two functions need to handle.
Why? I assume there will be exactly one caller of TDH.SYS.LP.INIT.
For tdx_enable(), given it's still good idea to disable CPU hotplug around
it, we can still do some check for all online cpus at the beginning, like:
on_each_cpu(check_cr4_vmx(), &err, 1);
If it gets to that point, just omit the check. I really think you're making much
ado about nothing.
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.