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