On 30/04/2024 8:06 am, Sean Christopherson wrote:
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.
Right, it's only done in tdx_cpu_enable().
I was thinking "the one that is most likely to fail" isn't just
TDH.SYS.LP.INIT in this case, but also could be any SEAMCALL that is
firstly run on any online cpu inside tdx_enable().
Or perhaps you were thinking once tdx_cpu_enable() is called on one cpu,
then we can safely assume that cpu must be in post-VMXON, despite we
have two separate functions: tdx_cpu_enable() and tdx_enable().
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.
Yeah.
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.
Anyway, I don't think adding such check or not matters a lot at this
stage, and I don't want to jeopardize any more time from you on this. :-)
