Re: [PATCH v3 4/8] KVM: Add a module param to allow enabling virtualization when KVM is loaded

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

On 13/08/2024 5:22 pm, Huang, Kai wrote:
> On Mon, 2024-08-12 at 19:31 -0700, Sean Christopherson wrote:
>> On Fri, Aug 02, 2024, Kai Huang wrote:
>>>
>>>> +static void kvm_uninit_virtualization(void)
>>>> +{
>>>> +	if (enable_virt_at_load)
>>>> +		kvm_disable_virtualization();
>>>> +
>>>> +	WARN_ON(kvm_usage_count);
>>>> +}
>>>>
>>>
>>> Hi Sean,
>>>
>>> The above "WARN_ON(kvm_usage_count);" assumes the
>>> kvm_uninit_virtualization() is the last call of
>>> kvm_disable_virtualization(), and it is called ...
>>>
>>>> @@ -6433,6 +6468,8 @@ void kvm_exit(void)
>>>>   	 */
>>>>   	misc_deregister(&kvm_dev);
>>>>   
>>>> +	kvm_uninit_virtualization();
>>>> +
>>>>
>>>
>>> ... from kvm_exit().
>>>
>>> Accordingly, kvm_init_virtualization() is called in kvm_init().
>>>
>>> For TDX, we want to "explicitly call kvm_enable_virtualization() +
>>> initializing TDX module" before kvm_init() in vt_init(), since kvm_init()
>>> is supposed to be the last step after initializing TDX.
>>>
>>> In the exit path, accordingly, for TDX we want to call kvm_exit() first,
>>> and then "do TDX cleanup staff + explicitly call
>>> kvm_disable_virtualizaation()".
>>>
>>> This will trigger the above "WARN_ON(kvm_usage_count);" when
>>> enable_virt_at_load is true, because kvm_uninit_virtualization() isn't
>>> the last call of kvm_disable_virtualization().
>>>
>>> To resolve, I think one way is we can move kvm_init_virtualization() out
>>> of kvm_init(), but I am not sure whether there's another common place
>>> that kvm_init_virtualization() can be called for all ARCHs.
>>>
>>> Do you have any comments?
>>
>> Drat.  That's my main coment, though not the exact word I used :-)
>>
>> I managed to completely forget about TDX needing to enable virtualization to do
>> its setup before creating /dev/kvm.  A few options jump to mind:
>>
>>   1. Expose kvm_enable_virtualization() to arch code and delete the WARN_ON().
>>
>>   2. Move kvm_init_virtualization() as you suggested.
>>
>>   3. Move the call to misc_register() out of kvm_init(), so that arch code can
>>      do additional setup between kvm_init() and kvm_register_dev_kvm() or whatever.
>>
>> I'm leaning towards #1.  IIRC, that was my original intent before going down the
>> "enable virtualization at module load" path.  And it's not mutually exclusive
>> with allowing virtualization to be forced on at module load.
>>
>> If #1 isn't a good option for whatever reason, I'd lean slightly for #3 over #2,
>> purely because it's less arbitrary (registering /dev/kvm is the only thing that
>> has strict ordering requirements).  But I don't know that having a separate
>> registration API would be a net positive, e.g. it's kinda nice that kvm_init()
>> needs to be last, because it helps ensure some amount of guaranteed ordering
>> between common KVM and arch code.
> 
> I agree with option 1).  We just allow arch code to do additional
> kvm_enable_virtualization() before kvm_init() and kvm_disable_virtualization()
> after kvm_exit().  I think it's kinda normal behaviour anyway.
> 
> And this is exactly what I am doing :-)
> 
> https://github.com/intel/tdx/commit/2f7cef685527a5ef952346ff5ab9adbb8bb6f371
> https://github.com/intel/tdx/commit/6c76ffa47a98ca370fad389271dc3cedf304df2d
> 

Hi Sean,

Forgot to ask:  I assume you will post v4 with that WARN_ON() in 
kvm_uninit_virtualizaiton() removed, correct?

I am not sure whether you will include the patch to export 
kvm_enable_virtualization() and kvm_disable_virtualization() but either 
way is fine to me.

I am thinking if we can get this patchset merged to kvm-coco-queue then 
we can actually start to review (and try to integrate) the "init TDX 
during KVM loading" patches.

I appreciate if you and Paolo could share some plan on this.  Thanks!