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

Huang, Kai
2024-04-10T23:16:02Z

On 11/04/2024 3:29 am, Sean Christopherson wrote:
> On Wed, Apr 10, 2024, Kai Huang wrote:
>> On Fri, 2024-03-22 at 14:23 -0700, Isaku Yamahata wrote:
>>>>> + r = atomic_read(&enable.err);
>>>>> + if (!r)
>>>>> + r = tdx_module_setup();
>>>>> + else
>>>>> + r = -EIO;
>>>>> + on_each_cpu(vmx_off, &enable.enabled, true);
>>>>> + cpus_read_unlock();
>>>>> + free_cpumask_var(enable.enabled);
>>>>> +
>>>>> +out:
>>>>> + return r;
>>>>> +}
>>>>
>>>> At last, I think there's one problem here:
>>>>
>>>> KVM actually only registers CPU hotplug callback in kvm_init(), which happens
>>>> way after tdx_hardware_setup().
>>>>
>>>> What happens if any CPU goes online *BETWEEN* tdx_hardware_setup() and
>>>> kvm_init()?
>>>>
>>>> Looks we have two options:
>>>>
>>>> 1) move registering CPU hotplug callback before tdx_hardware_setup(), or
>>>> 2) we need to disable CPU hotplug until callbacks have been registered.
>
> This is all so dumb (not TDX, the current state of KVM). All of the hardware
> enabling crud is pointless complex inherited from misguided, decade old paranoia
> that led to the decision to enable VMX if and only if VMs are running. Enabling
> VMX doesn't make the system less secure, and the insane dances we are doing to
> do VMXON on-demand makes everything *more* fragile.
>
> And all of this complexity really was driven by VMX, enabling virtualization for
> every other vendor, including AMD/SVM, is completely uninteresting. Forcing other
> architectures/vendors to take on yet more complexity doesn't make any sense.

Ah, I actually preferred this solution, but I was trying to follow your
suggestion here:

https://lore.kernel.org/lkml/ZW6FRBnOwYV-UCkY@google.com/

form which I interpreted you didn't like always having VMX enabled when
KVM is present. :-)

>
> Barely tested, and other architectures would need to be converted, but I don't
> see any obvious reasons why we can't simply enable virtualization when the module
> is loaded.
>
> The diffstat pretty much says it all.

Thanks a lot for the code!

I can certainly follow up with this and generate a reviewable patchset
if I can confirm with you that this is what you want?