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

[Huang, Kai]

> > 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@xxxxxxxxxx/
>
> 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.