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

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

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

Hi Paolo,

Thanks for doing this!

> +
> +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;
> +}

As Gao, Chao pointed out, there's bug here since it is also called by
__do_tdx_bringup() which is called with cpus_read_lock() hold.  We need the
_cpuslocked() version here.

I pasted the fixup at the bottom of this reply for your reference and please
merge if it is fine to you.  The diff is also attached if that's easier.


[...]

> +int __init tdx_bringup(void)
> +{
> +	int r;
> +
> +	if (!enable_tdx)
> +		return 0;
> +
> +	if (!kvm_can_support_tdx()) {
> +		pr_err("tdx: no TDX private KeyIDs available\n");
> +		goto success_disable_tdx;
> +	}
> +
> +	if (!enable_virt_at_load) {
> +		pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n");
> +		goto success_disable_tdx;
> +	}

The intention of kvm_can_support_tdx() is to put all dependency checks to it.  I
think we should just put the check of 'enable_virt_at_load' inside.  And there
will be more checks in the later patches such as checking 'tdp_mmu_enabled' and
'enable_mmio_caching' so it doesn't make sense to check 'enable_virt_at_load'
outside.


diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 0666dfbe0bc0..d8c008437e79 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;
@@ -68,7 +75,17 @@ static int __init __do_tdx_bringup(void)
 
 static bool __init kvm_can_support_tdx(void)
 {
-	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM)) {
+		pr_err("tdx: no TDX private KeyIDs available\n");
+		return false;
+	}
+
+	if (!enable_virt_at_load) {
+		pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n");
+		return false;
+	}
+
+	return true;
 }
 
 static int __init __tdx_bringup(void)
@@ -103,7 +120,7 @@ static int __init __tdx_bringup(void)
 void tdx_cleanup(void)
 {
 	if (enable_tdx) {
-		__do_tdx_cleanup();
+		__tdx_cleanup();
 		kvm_disable_virtualization();
 	}
 }
@@ -115,15 +132,8 @@ int __init tdx_bringup(void)
 	if (!enable_tdx)
 		return 0;
 
-	if (!kvm_can_support_tdx()) {
-		pr_err("tdx: no TDX private KeyIDs available\n");
-		goto success_disable_tdx;
-	}
-
-	if (!enable_virt_at_load) {
-		pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n");
+	if (!kvm_can_support_tdx())
 		goto success_disable_tdx;
-	}
 
 	/*
 	 * Ideally KVM should probe whether TDX module has been loaded


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

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 0666dfbe0bc0..d8c008437e79 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;
@@ -68,7 +75,17 @@ static int __init __do_tdx_bringup(void)
 
 static bool __init kvm_can_support_tdx(void)
 {
-	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM)) {
+		pr_err("tdx: no TDX private KeyIDs available\n");
+		return false;
+	}
+
+	if (!enable_virt_at_load) {
+		pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n");
+		return false;
+	}
+
+	return true;
 }
 
 static int __init __tdx_bringup(void)
@@ -103,7 +120,7 @@ static int __init __tdx_bringup(void)
 void tdx_cleanup(void)
 {
 	if (enable_tdx) {
-		__do_tdx_cleanup();
+		__tdx_cleanup();
 		kvm_disable_virtualization();
 	}
 }
@@ -115,15 +132,8 @@ int __init tdx_bringup(void)
 	if (!enable_tdx)
 		return 0;
 
-	if (!kvm_can_support_tdx()) {
-		pr_err("tdx: no TDX private KeyIDs available\n");
-		goto success_disable_tdx;
-	}
-
-	if (!enable_virt_at_load) {
-		pr_err("tdx: tdx requires kvm.enable_virt_at_load=1\n");
+	if (!kvm_can_support_tdx())
 		goto success_disable_tdx;
-	}
 
 	/*
 	 * Ideally KVM should probe whether TDX module has been loaded