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.
