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
