Re: [PATCH v2 3/3] KVM: VMX: Initialize TDX during KVM module load

["Huang, Kai" <kai.huang@intel.com>]

>> Shouldn't we make enable_tdx dependent on enable_virt_at_load? 
>> Otherwise, if
>> someone sets enable_tdx=1 and enable_virt_at_load=0, they will get 
>> hardware
>> virtualization enabled at load time while enable_virt_at_load still 
>> shows 0.
>> This behavior is a bit confusing to me.

Forgot to reply this ...

>>
>> I think a check against enable_virt_at_load in kvm_can_support_tdx() 
>> will work.
>>
>> The call of kvm_enable_virtualization() here effectively moves
>> kvm_init_virtualization() out of kvm_init() when enable_tdx=1. I 
>> wonder if it
>> makes more sense to refactor out kvm_init_virtualization() for non-TDX 
>> cases
>> as well, i.e.,
>>
>>    vmx_init();
>>    kvm_init_virtualization();
>>    tdx_init();
>>    kvm_init();
>>
>> I'm not sure if this was ever discussed. To me, this approach is 
>> better because
>> TDX code needn't handle virtualization enabling stuff. It can simply 
>> check that
>> enable_virt_at_load=1, assume virtualization is enabled and needn't 
>> disable
>> virtualization on errors.
> 
> I think this was briefly discussed here:
> 
> https://lore.kernel.org/all/ZrrFgBmoywk7eZYC@google.com/
> 
> The disadvantage of splitting out kvm_init_virtualization() is all other 
> ARCHs (all non-TDX cases actually) will need to explicitly call 
> kvm_init_virtualization() separately.
> 
>>
>> A bonus is that on non-TDX-capable systems, hardware virtualization 
>> won't be
>> toggled twice at KVM load time for no good reason.
> 
> I am fine with either way.
> 
> Sean, do you have any comments?
> 

... and yes I think we should make TDX depend on 'enable_virt_at_load'.

And by doing this, I think we can still do kvm_init_virtualization() 
inside kvm_init():

'enable_virt_at_load' still reflects its behaviour -- TDX just enables 
virt earlier than other cases, but it is still "enable virt at load".

It's perhaps not perfect, but it saves unneeded separate call of 
kvm_init_virtualization() for other ARCHs.