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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5927eaf9-d9ac-4056-85af-39bb90eabed3@intel.com>
Date: Thu, 21 Nov 2024 08:28:50 +0800
From: "Huang, Kai" <kai.huang@...el.com>
To: Chao Gao <chao.gao@...el.com>
CC: <pbonzini@...hat.com>, <seanjc@...gle.com>, <kvm@...r.kernel.org>,
	<rick.p.edgecombe@...el.com>, <isaku.yamahata@...el.com>,
	<reinette.chatre@...el.com>, <binbin.wu@...ux.intel.com>,
	<xiaoyao.li@...el.com>, <yan.y.zhao@...el.com>, <adrian.hunter@...el.com>,
	<tony.lindgren@...el.com>, <kristen@...ux.intel.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] KVM: VMX: Initialize TDX during KVM module load


>> 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.