Re: [PATCH 12/30] KVM: VMX: Initialize TDX during KVM module load

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

[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]


> +
> +static void __do_tdx_cleanup(void)
> +{
> +	/*
> +	 * Once TDX module is initialized, it cannot be disabled and
> +	 * re-initialized again w/o runtime update (which isn't
> +	 * supported by kernel).  Only need to remove the cpuhp here.
> +	 * The TDX host core code tracks TDX status and can handle
> +	 * 'multiple enabling' scenario.
> +	 */
> +	WARN_ON_ONCE(!tdx_cpuhp_state);
> +	cpuhp_remove_state_nocalls(tdx_cpuhp_state);
> +	tdx_cpuhp_state = 0;
> +}
> +
> +static int __init __do_tdx_bringup(void)
> +{
> +	int r;
> +
> +	/*
> +	 * TDX-specific cpuhp callback to call tdx_cpu_enable() on all
> +	 * online CPUs before calling tdx_enable(), and on any new
> +	 * going-online CPU to make sure it is ready for TDX guest.
> +	 */
> +	r = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> +					 "kvm/cpu/tdx:online",
> +					 tdx_online_cpu, NULL);
> +	if (r < 0)
> +		return r;
> +
> +	tdx_cpuhp_state = r;
> +
> +	r = tdx_enable();
> +	if (r)
> +		__do_tdx_cleanup();
> +
> +	return r;
> +}
> 
[...]

> +static int __init __tdx_bringup(void)
> +{
> +	int r;
> +
> +	/*
> +	 * Enabling TDX requires enabling hardware virtualization first,
> +	 * as making SEAMCALLs requires CPU being in post-VMXON state.
> +	 */
> +	r = kvm_enable_virtualization();
> +	if (r)
> +		return r;
> +
> +	cpus_read_lock();
> +	r = __do_tdx_bringup();
> +	cpus_read_unlock();
> +

Hi Paolo,

This patch still doesn't address a bug Chao pointed out, that the
__do_tdx_cleanup() can be called from __do_tdx_bringup() with cpus_read_lock()
being hold, so we need to use cpuhp_remove_state_nocalls_cpuslocked() in
__do_tdx_cleanup().

I posted a diff to address here:

https://lore.kernel.org/lkml/46ea74bcd8eebe241a143e9280c65ca33cb8dcce.camel@intel.com/T/#m1e86328e69b27e6cc9978f90df923144d699c350

It would be great if you could squash to the kvm-coco-queue.  There will be some
minor rebase conflict to the rest patches, though, so if you want me to send out
fixup patch(es) for you to squash please do let me know.

Btw, the diff also moves the 'enable_virt_at_load' check to
kvm_can_support_tdx(), which isn't related to this issue.  Below is the diff
(also attached) w/o this code change but only to address the above bug if you
prefer.

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 0666dfbe0bc0..9115467f208d 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -38,10 +38,17 @@ static void __do_tdx_cleanup(void)
         * 'multiple enabling' scenario.
         */
        WARN_ON_ONCE(!tdx_cpuhp_state);
-       cpuhp_remove_state_nocalls(tdx_cpuhp_state);
+       cpuhp_remove_state_nocalls_cpuslocked(tdx_cpuhp_state);
        tdx_cpuhp_state = 0;
 }

+static void __tdx_cleanup(void)
+{
+       cpus_read_lock();
+       __do_tdx_cleanup();
+       cpus_read_unlock();
+}
+
 static int __init __do_tdx_bringup(void)
 {
        int r;
@@ -103,7 +110,7 @@ static int __init __tdx_bringup(void)
 void tdx_cleanup(void)
 {
        if (enable_tdx) {
-               __do_tdx_cleanup();
+               __tdx_cleanup();
                kvm_disable_virtualization();
        }
 }




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tdx-init-2.diff --]
[-- Type: text/x-patch; name="tdx-init-2.diff", Size: 769 bytes --]

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 0666dfbe0bc0..9115467f208d 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -38,10 +38,17 @@ static void __do_tdx_cleanup(void)
 	 * 'multiple enabling' scenario.
 	 */
 	WARN_ON_ONCE(!tdx_cpuhp_state);
-	cpuhp_remove_state_nocalls(tdx_cpuhp_state);
+	cpuhp_remove_state_nocalls_cpuslocked(tdx_cpuhp_state);
 	tdx_cpuhp_state = 0;
 }
 
+static void __tdx_cleanup(void)
+{
+	cpus_read_lock();
+	__do_tdx_cleanup();
+	cpus_read_unlock();
+}
+
 static int __init __do_tdx_bringup(void)
 {
 	int r;
@@ -103,7 +110,7 @@ static int __init __tdx_bringup(void)
 void tdx_cleanup(void)
 {
 	if (enable_tdx) {
-		__do_tdx_cleanup();
+		__tdx_cleanup();
 		kvm_disable_virtualization();
 	}
 }