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!
