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

[Sean Christopherson <seanjc@google.com>]

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.