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

[Huang, Kai]

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.  :-)