On Mon, Mar 14, 2022, Isaku Yamahata wrote:
> On Sun, Mar 13, 2022 at 03:03:40PM +0100,
> Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
> > On 3/4/22 20:48, isaku.yamahata@xxxxxxxxx wrote:
> > > +
> > > + if (!tdx_module_initialized) {
> > > + if (enable_tdx) {
> > > + ret = __tdx_module_setup();
> > > + if (ret)
> > > + enable_tdx = false;
> >
> > "enable_tdx = false" isn't great to do only when a VM is created. Does it
> > make sense to anticipate this to the point when the kvm_intel.ko module is
> > loaded?
>
> It's possible. I have the following two reasons to chose to defer TDX module
> initialization until creating first TD. Given those reasons, do you still want
> the initialization at loading kvm_intel.ko module? If yes, I'll change it.
Yes, TDX module setup needs to be done at load time. The loss of memory is
unfortunate, e.g. if the host is part of a pool that _might_ run TDX guests, but
the alternatives are worse. If TDX fails to initialize, e.g. due to low mem,
then the host will be unable to run TDX guests despite saying "I support TDX".
Or this gem :-)
/*
* TDH.SYS.KEY.CONFIG may fail with entropy error (which is
* a recoverable error). Assume this is exceedingly rare and
* just return error if encountered instead of retrying.
*/
The CPU overhead of initializing the TDX module is also non-trivial, and it
doesn't affect just this CPU, e.g. all CPUs need to do certain SEAMCALLs and at
least one WBINVD. The can cause noisy neighbor problems.
> - memory over head: The initialization of TDX module requires to allocate
> physically contiguous memory whose size is about 0.43% of the system memory.
> If user don't use TD, it will be wasted.
>
> - VMXON on all pCPUs: The TDX module initialization requires to enable VMX
> (VMXON) on all present pCPUs. vmx_hardware_enable() which is called on creating
> guest does it. It naturally fits with the TDX module initialization at creating
> first TD. I wanted to avoid code to enable VMXON on loading the kvm_intel.ko.
That's a solvable problem, though making it work without exporting hardware_enable_all()
could get messy.
