[PATCH 0/3] KVM: VMX: Initialize TDX when loading KVM module

[PATCH 0/3] KVM: VMX: Initialize TDX when loading KVM module

[Kai Huang]

Hi Paolo/Sean,

This series contains patches to initialize TDX when loading KVM module.
This series is based on the discussion with Sean on the v19 patchset
[*], hoping it has addressed most (if not all) comments.

This series has been in our internal TDX tree for long time and has been
in kvm-coco-queue for some time thus it has been tested.

The main purpose for sending out is to have a review but this series can
also be applied to kvm/queue cleanly.

Thanks for your time!

[*]: https://lore.kernel.org/kvm/f028d43abeadaa3134297d28fb99f283445c0333.1708933498.git.isaku.yamahata@intel.com/

Kai Huang (3):
  KVM: VMX: Refactor VMX module init/exit functions
  KVM: Export hardware virtualization enabling/disabling functions
  KVM: VMX: Initialize TDX during KVM module load

 arch/x86/kvm/Makefile    |   1 +
 arch/x86/kvm/vmx/main.c  |  38 +++++++++++++
 arch/x86/kvm/vmx/tdx.c   | 115 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/tdx.h   |  12 ++++
 arch/x86/kvm/vmx/vmx.c   |  23 +-------
 arch/x86/kvm/vmx/vmx.h   |   3 +
 include/linux/kvm_host.h |   8 +++
 virt/kvm/kvm_main.c      |  18 ++----
 8 files changed, 183 insertions(+), 35 deletions(-)
 create mode 100644 arch/x86/kvm/vmx/tdx.c
 create mode 100644 arch/x86/kvm/vmx/tdx.h


base-commit: 5cb1659f412041e4780f2e8ee49b2e03728a2ba6
-- 
2.46.2

[PATCH 1/3] KVM: VMX: Refactor VMX module init/exit functions

[Kai Huang]

Add vt_init() and vt_exit() as the new module init/exit functions and
refactor existing vmx_init()/vmx_exit() as helper to make room for TDX
specific initialization and teardown.

To support TDX, KVM will need to enabling TDX during KVM module loading
time.  Enabling TDX requires enabling hardware virtualization first so
that all online CPUs (and the new CPU going online) are in post-VMXON
state.

Currently, the vmx_init() flow is:

 1) hv_init_evmcs(),
 2) kvm_x86_vendor_init(),
 3) Other VMX specific initialization,
 4) kvm_init()

The kvm_x86_vendor_init() invokes kvm_x86_init_ops::hardware_setup() to
do VMX specific hardware setup and calls kvm_update_ops() to initialize
kvm_x86_ops to VMX's version.

TDX will have its own version for most of kvm_x86_ops callbacks.  It
would be nice if kvm_x86_init_ops::hardware_setup() could also be used
for TDX, but in practice it cannot.  The reason is, as mentioned above,
TDX initialization requires hardware virtualization having been enabled,
which must happen after kvm_update_ops(), but hardware_setup() is done
before that.

Also, TDX is based on VMX, and it makes sense to only initialize TDX
after VMX has been initialized.  If VMX fails to initialize, TDX is
likely broken anyway.

So the new flow of KVM module init function will be:

 1) Current VMX initialization code in vmx_init() before kvm_init(),
 2) TDX initialization,
 3) kvm_init()

Split vmx_init() into two parts based above 1) and 3) so that TDX
initialization can fit in between.  Make part 1) as the new helper
vmx_init().  Introduce vt_init() as the new module init function which
calls vmx_init() and kvm_init().  TDX initialization will be added
later.

Do the same thing for vmx_exit()/vt_exit().

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/vmx/main.c | 32 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/vmx.c  | 23 ++---------------------
 arch/x86/kvm/vmx/vmx.h  |  3 +++
 3 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 7668e2fb8043..433ecbd90905 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -166,3 +166,35 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
 	.runtime_ops = &vt_x86_ops,
 	.pmu_ops = &intel_pmu_ops,
 };
+
+static void vt_exit(void)
+{
+	kvm_exit();
+	vmx_exit();
+}
+module_exit(vt_exit);
+
+static int __init vt_init(void)
+{
+	int r;
+
+	r = vmx_init();
+	if (r)
+		return r;
+
+	/*
+	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
+	 * exposed to userspace!
+	 */
+	r = kvm_init(sizeof(struct vcpu_vmx), __alignof__(struct vcpu_vmx),
+		     THIS_MODULE);
+	if (r)
+		goto err_kvm_init;
+
+	return 0;
+
+err_kvm_init:
+	vmx_exit();
+	return r;
+}
+module_init(vt_init);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index f6900bec4874..976fe6579f62 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8587,23 +8587,16 @@ static void vmx_cleanup_l1d_flush(void)
 	l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO;
 }
 
-static void __vmx_exit(void)
+void vmx_exit(void)
 {
 	allow_smaller_maxphyaddr = false;
 
 	vmx_cleanup_l1d_flush();
-}
 
-static void vmx_exit(void)
-{
-	kvm_exit();
-	__vmx_exit();
 	kvm_x86_vendor_exit();
-
 }
-module_exit(vmx_exit);
 
-static int __init vmx_init(void)
+int __init vmx_init(void)
 {
 	int r, cpu;
 
@@ -8647,21 +8640,9 @@ static int __init vmx_init(void)
 	if (!enable_ept)
 		allow_smaller_maxphyaddr = true;
 
-	/*
-	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
-	 * exposed to userspace!
-	 */
-	r = kvm_init(sizeof(struct vcpu_vmx), __alignof__(struct vcpu_vmx),
-		     THIS_MODULE);
-	if (r)
-		goto err_kvm_init;
-
 	return 0;
 
-err_kvm_init:
-	__vmx_exit();
 err_l1d_flush:
 	kvm_x86_vendor_exit();
 	return r;
 }
-module_init(vmx_init);
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 40303b43da6c..ad9efe41e691 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -755,4 +755,7 @@ static inline void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
 	vmx->segment_cache.bitmask = 0;
 }
 
+int vmx_init(void);
+void vmx_exit(void);
+
 #endif /* __KVM_X86_VMX_H */
-- 
2.46.2

[PATCH 2/3] KVM: Export hardware virtualization enabling/disabling functions

[Kai Huang]

To support TDX, KVM will need to enabling TDX during KVM module loading
time.  Enabling TDX requires enabling hardware virtualization first so
that all online CPUs (and the new CPU going online) are in post-VMXON
state.

KVM by default enables hardware virtualization but that is done in
kvm_init(), which must be the last step after all initialization is done
thus is too late for enabling TDX.

Export functions to enabling/disable hardware virtualization so that TDX
code can use them to handle hardware virtualization enabling before
kvm_init().

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 include/linux/kvm_host.h |  8 ++++++++
 virt/kvm/kvm_main.c      | 18 ++++--------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 02f0206fd2dc..024c91eef7db 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2557,4 +2557,12 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
 				    struct kvm_pre_fault_memory *range);
 #endif
 
+#ifdef CONFIG_KVM_GENERIC_HARDWARE_ENABLING
+int kvm_enable_virtualization(void);
+void kvm_disable_virtualization(void);
+#else
+static inline int kvm_enable_virtualization(void) { return 0; }
+static inline void kvm_disable_virtualization(void) { }
+#endif
+
 #endif
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b1b10dc408a0..a0117390ea7f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -143,8 +143,6 @@ static int kvm_no_compat_open(struct inode *inode, struct file *file)
 #define KVM_COMPAT(c)	.compat_ioctl	= kvm_no_compat_ioctl,	\
 			.open		= kvm_no_compat_open
 #endif
-static int kvm_enable_virtualization(void);
-static void kvm_disable_virtualization(void);
 
 static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
 
@@ -5523,7 +5521,7 @@ static struct syscore_ops kvm_syscore_ops = {
 	.shutdown = kvm_shutdown,
 };
 
-static int kvm_enable_virtualization(void)
+int kvm_enable_virtualization(void)
 {
 	int r;
 
@@ -5568,8 +5566,9 @@ static int kvm_enable_virtualization(void)
 	--kvm_usage_count;
 	return r;
 }
+EXPORT_SYMBOL_GPL(kvm_enable_virtualization);
 
-static void kvm_disable_virtualization(void)
+void kvm_disable_virtualization(void)
 {
 	guard(mutex)(&kvm_usage_lock);
 
@@ -5580,6 +5579,7 @@ static void kvm_disable_virtualization(void)
 	cpuhp_remove_state(CPUHP_AP_KVM_ONLINE);
 	kvm_arch_disable_virtualization();
 }
+EXPORT_SYMBOL_GPL(kvm_disable_virtualization);
 
 static int kvm_init_virtualization(void)
 {
@@ -5595,21 +5595,11 @@ static void kvm_uninit_virtualization(void)
 		kvm_disable_virtualization();
 }
 #else /* CONFIG_KVM_GENERIC_HARDWARE_ENABLING */
-static int kvm_enable_virtualization(void)
-{
-	return 0;
-}
-
 static int kvm_init_virtualization(void)
 {
 	return 0;
 }
 
-static void kvm_disable_virtualization(void)
-{
-
-}
-
 static void kvm_uninit_virtualization(void)
 {
 
-- 
2.46.2

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

[Kai Huang]

Before KVM can use TDX to create and run TDX guests, TDX needs to be
initialized from two perspectives: 1) TDX module must be initialized
properly to a working state; 2) A per-cpu TDX initialization, a.k.a the
TDH.SYS.LP.INIT SEAMCALL must be done on any logical cpu before it can
run any other TDX SEAMCALLs.

The TDX host core-kernel provides two functions to do above two
respectively: tdx_enable() and tdx_cpu_enable().

There are two options in terms of when to initialize TDX: initialize TDX
at KVM module loading time, or when creating the first TDX guest.

Choose to initialize TDX during KVM module loading time:

Initializing TDX module is both memory and CPU time consuming: 1) the
kernel needs to allocate a non-trivial size(~1/256) of system memory
as metadata used by TDX module to track each TDX-usable memory page's
status; 2) the TDX module needs to initialize this metadata, one entry
for each TDX-usable memory page.

Also, the kernel uses alloc_contig_pages() to allocate those metadata
chunks, because they are large and need to be physically contiguous.

alloc_contig_pages() can fail.  If initializing TDX when creating the
first TDX guest, then there's chance that KVM won't be able to run any
TDX guests albeit KVM _declares_ to be able to support TDX.

This isn't good to the user.  On the other hand, initializing TDX at KVM
module loading time can make sure KVM is providing a consistent view of
whether KVM can support TDX to the user.

Always only try to initialize TDX after VMX has been initialized.  TDX
is based on VMX, and if VMX fails to initialize then TDX is likely to be
broken anyway.  Also, in practice, supporting TDX will require part of
VMX and common x86 infrastructure in working order, so TDX cannot be
sololy w/o VMX support.

Specifically, initialize TDX after VMX has been initialized and before
kvm_init() in vt_init().  Don't fail the whole vt_init() if TDX fails to
initialize, since in this case KVM can still support normal VMX guests.

Because TDX costs additional memory, don't enable TDX by default.  Add a
new module parameter 'enable_tdx' to allow the user to opt-in.

Register a new TDX-specific cpuhp callback to run tdx_cpu_enable(), and
call tdx_enable() after that to ensure tdx_cpu_enable() has been done
for all online CPUs before making the tdx_enable().  Use a dynamic cpuhp
state for TDX so that KVM's cpuhp callback to enable VMX on new online
CPU can happen before tdx_cpu_enable().

Note, the name tdx_init() has already been taken by the early boot code.
Use tdx_bringup() for initializing TDX (and tdx_cleanup() since KVM
doesn't actually teardown TDX).  They don't match vt_init()/vt_exit(),
vmx_init()/vmx_exit() etc but it's not end of the world.

Also, once initialized, the TDX module cannot be disabled and enabled
again w/o the TDX module runtime update, which isn't supported by the
kernel.  After TDX is enabled, nothing needs to be done when KVM
disables hardware virtualization, e.g., when offlining CPU, or during
suspend/resume.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/Makefile   |   1 +
 arch/x86/kvm/vmx/main.c |   6 +++
 arch/x86/kvm/vmx/tdx.c  | 115 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/tdx.h  |  12 +++++
 4 files changed, 134 insertions(+)
 create mode 100644 arch/x86/kvm/vmx/tdx.c
 create mode 100644 arch/x86/kvm/vmx/tdx.h

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index f9dddb8cb466..fec803aff7ad 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -20,6 +20,7 @@ kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
 
 kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
 kvm-intel-$(CONFIG_KVM_HYPERV)	+= vmx/hyperv.o vmx/hyperv_evmcs.o
+kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o
 
 kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o
 
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 433ecbd90905..053294939eb1 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -6,6 +6,7 @@
 #include "nested.h"
 #include "pmu.h"
 #include "posted_intr.h"
+#include "tdx.h"
 
 #define VMX_REQUIRED_APICV_INHIBITS				\
 	(BIT(APICV_INHIBIT_REASON_DISABLED) |			\
@@ -170,6 +171,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
 static void vt_exit(void)
 {
 	kvm_exit();
+	tdx_cleanup();
 	vmx_exit();
 }
 module_exit(vt_exit);
@@ -182,6 +184,9 @@ static int __init vt_init(void)
 	if (r)
 		return r;
 
+	/* tdx_init() has been taken */
+	tdx_bringup();
+
 	/*
 	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
 	 * exposed to userspace!
@@ -194,6 +199,7 @@ static int __init vt_init(void)
 	return 0;
 
 err_kvm_init:
+	tdx_cleanup();
 	vmx_exit();
 	return r;
 }
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
new file mode 100644
index 000000000000..8651599822d5
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/cpu.h>
+#include <asm/tdx.h>
+#include "capabilities.h"
+#include "tdx.h"
+
+#undef pr_fmt
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+static bool enable_tdx __ro_after_init;
+module_param_named(tdx, enable_tdx, bool, 0444);
+
+static enum cpuhp_state tdx_cpuhp_state;
+
+static int tdx_online_cpu(unsigned int cpu)
+{
+	unsigned long flags;
+	int r;
+
+	/* Sanity check CPU is already in post-VMXON */
+	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
+
+	/* tdx_cpu_enable() must be called with IRQ disabled */
+	local_irq_save(flags);
+	r = tdx_cpu_enable();
+	local_irq_restore(flags);
+
+	return r;
+}
+
+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).  In fact the kernel doesn't support
+	 * disable (shut down) TDX module, so only need to remove the
+	 * cpuhp state.
+	 */
+	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;
+
+	/* tdx_enable() must be called with cpus_read_lock() */
+	r = tdx_enable();
+	if (r)
+		__do_tdx_cleanup();
+
+	return r;
+}
+
+static int __init __tdx_bringup(void)
+{
+	int r;
+
+	if (!enable_ept) {
+		pr_err("Cannot enable TDX with EPT disabled.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * 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();
+
+	if (r)
+		goto tdx_bringup_err;
+
+	/*
+	 * Leave hardware virtualization enabled after TDX is enabled
+	 * successfully.  TDX CPU hotplug depends on this.
+	 */
+	return 0;
+tdx_bringup_err:
+	kvm_disable_virtualization();
+	return r;
+}
+
+void tdx_cleanup(void)
+{
+	if (enable_tdx) {
+		__do_tdx_cleanup();
+		kvm_disable_virtualization();
+	}
+}
+
+void __init tdx_bringup(void)
+{
+	enable_tdx = enable_tdx && !__tdx_bringup();
+}
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
new file mode 100644
index 000000000000..766a6121f670
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -0,0 +1,12 @@
+#ifndef  __KVM_X86_VMX_TDX_H
+#define __KVM_X86_VMX_TDX_H
+
+#ifdef CONFIG_INTEL_TDX_HOST
+void tdx_bringup(void);
+void tdx_cleanup(void);
+#else
+static inline void tdx_bringup(void) {}
+static inline void tdx_cleanup(void) {}
+#endif
+
+#endif
-- 
2.46.2

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

[Sean Christopherson]

On Tue, Oct 29, 2024, Kai Huang wrote:
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index f9dddb8cb466..fec803aff7ad 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -20,6 +20,7 @@ kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
>  
>  kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
>  kvm-intel-$(CONFIG_KVM_HYPERV)	+= vmx/hyperv.o vmx/hyperv_evmcs.o
> +kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o

IMO, INTEL_TDX_HOST should be a KVM Kconfig, e.g. KVM_INTEL_TDX.  Forcing the user
to bounce between KVM's menu and the generic menu to enable KVM support for TDX is
kludgy.  Having INTEL_TDX_HOST exist before KVM support came along made sense, as
it allowed compile-testing a bunch of code, but I don't think it should be the end
state.

If others disagree, then we should adjust KVM_AMD_SEV in the opposite direction,
because doing different things for SEV vs. TDX is confusing and messy.

>  kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o
>  
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 433ecbd90905..053294939eb1 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -6,6 +6,7 @@
>  #include "nested.h"
>  #include "pmu.h"
>  #include "posted_intr.h"
> +#include "tdx.h"
>  
>  #define VMX_REQUIRED_APICV_INHIBITS				\
>  	(BIT(APICV_INHIBIT_REASON_DISABLED) |			\
> @@ -170,6 +171,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
>  static void vt_exit(void)
>  {
>  	kvm_exit();
> +	tdx_cleanup();
>  	vmx_exit();
>  }
>  module_exit(vt_exit);
> @@ -182,6 +184,9 @@ static int __init vt_init(void)
>  	if (r)
>  		return r;
>  
> +	/* tdx_init() has been taken */
> +	tdx_bringup();

tdx_module_init()?  And honestly, even though Linux doesn't currently support
unloading the TDX module, I think tdx_module_exit() is a perfectly fine name,
because not being able to unload the TDX module and reclaim all of that memory
is a flaw that should be addressed at some point.
> +static enum cpuhp_state tdx_cpuhp_state;
> +
> +static int tdx_online_cpu(unsigned int cpu)
> +{
> +	unsigned long flags;
> +	int r;
> +
> +	/* Sanity check CPU is already in post-VMXON */
> +	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
> +
> +	/* tdx_cpu_enable() must be called with IRQ disabled */

I don't find this comment helpfu.  If it explained _why_ tdx_cpu_enable() requires
IRQs to be disabled, then I'd feel differently, but as is, IMO it doesn't add value.

> +	local_irq_save(flags);
> +	r = tdx_cpu_enable();
> +	local_irq_restore(flags);
> +
> +	return r;
> +}
> +

...

> +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;
> +
> +	/* tdx_enable() must be called with cpus_read_lock() */

This comment is even less valuable, IMO.

> +	r = tdx_enable();
> +	if (r)
> +		__do_tdx_cleanup();
> +
> +	return r;
> +}
> +
> +static int __init __tdx_bringup(void)
> +{
> +	int r;
> +
> +	if (!enable_ept) {
> +		pr_err("Cannot enable TDX with EPT disabled.\n");

Why wait until now to check for EPT?  Force enable_tdx to false if enable_ept is
false, don't fail the module load.

> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * 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();
> +
> +	if (r)
> +		goto tdx_bringup_err;
> +
> +	/*
> +	 * Leave hardware virtualization enabled after TDX is enabled
> +	 * successfully.  TDX CPU hotplug depends on this.
> +	 */
> +	return 0;
> +tdx_bringup_err:
> +	kvm_disable_virtualization();
> +	return r;
> +}
> +
> +void tdx_cleanup(void)
> +{
> +	if (enable_tdx) {
> +		__do_tdx_cleanup();
> +		kvm_disable_virtualization();
> +	}
> +}
> +
> +void __init tdx_bringup(void)
> +{
> +	enable_tdx = enable_tdx && !__tdx_bringup();

Ah.  I don't love this approach because it mixes "failure" due to an unsupported
configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.

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

[Huang, Kai]

On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> On Tue, Oct 29, 2024, Kai Huang wrote:
> > diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> > index f9dddb8cb466..fec803aff7ad 100644
> > --- a/arch/x86/kvm/Makefile
> > +++ b/arch/x86/kvm/Makefile
> > @@ -20,6 +20,7 @@ kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
> >  
> >  kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
> >  kvm-intel-$(CONFIG_KVM_HYPERV)	+= vmx/hyperv.o vmx/hyperv_evmcs.o
> > +kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o
> 
> IMO, INTEL_TDX_HOST should be a KVM Kconfig, e.g. KVM_INTEL_TDX.  Forcing the user
> to bounce between KVM's menu and the generic menu to enable KVM support for TDX is
> kludgy.  Having INTEL_TDX_HOST exist before KVM support came along made sense, as
> it allowed compile-testing a bunch of code, but I don't think it should be the end
> state.
> 
> If others disagree, then we should adjust KVM_AMD_SEV in the opposite direction,
> because doing different things for SEV vs. TDX is confusing and messy.

+ Dave (and Dan for TDX Connect).

Agree SEV/TDX should be in similar way.  But also I find SEV has a dependency on
CRYPTO_DEV_SP_PSP, so perhaps it also reasonable to make an additional
KVM_INTEL_TDX and make it depend on INTEL_TDX_HOST?

We could remove INTEL_TDX_HOST but only keep KVM_INTEL_TDX.  But in the long
term, more kernel components will need to add TDX support (e.g., for TDX
Connect).  I think the question is whether we can safely disable TDX code in ALL
kernel components when KVM_INTEL_TDX is not enabled.

If the answer is yes (seems correct to me, because it seems meaningless to
enable TDX code in _ANY_ kernel components when it's even possible to run TDX 
guest), then I think we can just change the current INTEL_TDX_HOST to
KVM_INTEL_TDX and put it in arch/x86/kvm/Kconfig.

Hi Dave, Dan,

Do you have any comments?

> 
> >  kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o
> >  
> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 433ecbd90905..053294939eb1 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -6,6 +6,7 @@
> >  #include "nested.h"
> >  #include "pmu.h"
> >  #include "posted_intr.h"
> > +#include "tdx.h"
> >  
> >  #define VMX_REQUIRED_APICV_INHIBITS				\
> >  	(BIT(APICV_INHIBIT_REASON_DISABLED) |			\
> > @@ -170,6 +171,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
> >  static void vt_exit(void)
> >  {
> >  	kvm_exit();
> > +	tdx_cleanup();
> >  	vmx_exit();
> >  }
> >  module_exit(vt_exit);
> > @@ -182,6 +184,9 @@ static int __init vt_init(void)
> >  	if (r)
> >  		return r;
> >  
> > +	/* tdx_init() has been taken */
> > +	tdx_bringup();
> 
> tdx_module_init()?  And honestly, even though Linux doesn't currently support
> unloading the TDX module, I think tdx_module_exit() is a perfectly fine name,
> because not being able to unload the TDX module and reclaim all of that memory
> is a flaw that should be addressed at some point.

tdx_module_init()/exit() also work for me.

Or is vt_tdx_init()/exit() better?  We can rename vmx_init()/exit() to
vt_vmx_init()/exit() if needed.

> > +static enum cpuhp_state tdx_cpuhp_state;
> > +
> > +static int tdx_online_cpu(unsigned int cpu)
> > +{
> > +	unsigned long flags;
> > +	int r;
> > +
> > +	/* Sanity check CPU is already in post-VMXON */
> > +	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
> > +
> > +	/* tdx_cpu_enable() must be called with IRQ disabled */
> 
> I don't find this comment helpfu.  If it explained _why_ tdx_cpu_enable() requires
> IRQs to be disabled, then I'd feel differently, but as is, IMO it doesn't add value.

I'll remove the comment.

> 
> > +	local_irq_save(flags);
> > +	r = tdx_cpu_enable();
> > +	local_irq_restore(flags);
> > +
> > +	return r;
> > +}
> > +
> 
> ...
> 
> > +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;
> > +
> > +	/* tdx_enable() must be called with cpus_read_lock() */
> 
> This comment is even less valuable, IMO.

Will remove.

> 
> > +	r = tdx_enable();
> > +	if (r)
> > +		__do_tdx_cleanup();
> > +
> > +	return r;
> > +}
> > +
> > 

[...]

> > +void __init tdx_bringup(void)
> > +{
> > +	enable_tdx = enable_tdx && !__tdx_bringup();
> 
> Ah.  I don't love this approach because it mixes "failure" due to an unsupported
> configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
> fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.

Thanks for the comments.

I see your point.  However for "enabling virtualization failure" kvm_init() will
also try to do (default behaviour), so if it fails it will result in module
loading failure eventually.  So while I guess it would be slightly better to
make module loading fail if "enabling virtualization fails" in TDX, it is a nit
issue to me.

I think "enabling virtualization failure" is the only "unexpected issue" that
should result in module loading failure.  For any other TDX-specific
initialization failure (e.g., any memory allocation in future patches) it's
better to only disable TDX.

So I can change to "make loading KVM-the-module fail if enabling virtualization
fails in TDX", but I want to confirm this is what you want?

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

[Sean Christopherson]

On Thu, Oct 31, 2024, Kai Huang wrote:
> On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > > +void __init tdx_bringup(void)
> > > +{
> > > +	enable_tdx = enable_tdx && !__tdx_bringup();
> > 
> > Ah.  I don't love this approach because it mixes "failure" due to an unsupported
> > configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
> > fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
> 
> Thanks for the comments.
> 
> I see your point.  However for "enabling virtualization failure" kvm_init() will
> also try to do (default behaviour), so if it fails it will result in module
> loading failure eventually.  So while I guess it would be slightly better to
> make module loading fail if "enabling virtualization fails" in TDX, it is a nit
> issue to me.
> 
> I think "enabling virtualization failure" is the only "unexpected issue" that
> should result in module loading failure.  For any other TDX-specific
> initialization failure (e.g., any memory allocation in future patches) it's
> better to only disable TDX.

I disagree.  The platform owner wants TDX to be enabled, KVM shouldn't silently
disable TDX because of a transient, unrelated failure.

If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
disable, then that's different.  And that's the general pattern throughout KVM.
If a requested feature isn't supported, then KVM continues on updates the module
param accordingly.  But if something outright fails during setup, KVM aborts the
entire sequence.

> So I can change to "make loading KVM-the-module fail if enabling virtualization
> fails in TDX", but I want to confirm this is what you want?

I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
would normally succeed, fails.

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

[Huang, Kai]

On 1/11/2024 9:22 am, Sean Christopherson wrote:
> On Thu, Oct 31, 2024, Kai Huang wrote:
>> On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
>>>> +void __init tdx_bringup(void)
>>>> +{
>>>> +	enable_tdx = enable_tdx && !__tdx_bringup();
>>>
>>> Ah.  I don't love this approach because it mixes "failure" due to an unsupported
>>> configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
>>> fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
>>
>> Thanks for the comments.
>>
>> I see your point.  However for "enabling virtualization failure" kvm_init() will
>> also try to do (default behaviour), so if it fails it will result in module
>> loading failure eventually.  So while I guess it would be slightly better to
>> make module loading fail if "enabling virtualization fails" in TDX, it is a nit
>> issue to me.
>>
>> I think "enabling virtualization failure" is the only "unexpected issue" that
>> should result in module loading failure.  For any other TDX-specific
>> initialization failure (e.g., any memory allocation in future patches) it's
>> better to only disable TDX.
> 
> I disagree.  The platform owner wants TDX to be enabled, KVM shouldn't silently
> disable TDX because of a transient, unrelated failure.
> 
> If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
> disable, then that's different.  And that's the general pattern throughout KVM.
> If a requested feature isn't supported, then KVM continues on updates the module
> param accordingly.  But if something outright fails during setup, KVM aborts the
> entire sequence.
> 
>> So I can change to "make loading KVM-the-module fail if enabling virtualization
>> fails in TDX", but I want to confirm this is what you want?
> 
> I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
> would normally succeed, fails.

OK will change to what you suggested.  I'll need to take a deeper look 
though since later patches will add more checks.

Thanks for the comments!

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

[Edgecombe, Rick P]

Paolo,

On Fri, 2024-11-01 at 10:21 +1300, Huang, Kai wrote:
> > 
> > I would prefer the logic to be: reject loading kvm-intel.ko if an operation
> > that
> > would normally succeed, fails.
> 
> OK will change to what you suggested.  I'll need to take a deeper look 
> though since later patches will add more checks.
> 
> Thanks for the comments!

We had talked about you taking this series over, under the assumption that it
was mostly settled. Would it help for us to spin another branch with these
changes?

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

[Edgecombe, Rick P]

On Thu, 2024-10-31 at 14:29 -0700, Rick Edgecombe wrote:
> > Thanks for the comments!
> 
> We had talked about you taking this series over, under the assumption that it
> was mostly settled. Would it help for us to spin another branch with these
> changes?

Discussing on the PUCK call, Paolo said to send another version with the changes
incorporated.

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

[Huang, Kai]

On Thu, 2024-10-31 at 13:22 -0700, Sean Christopherson wrote:
> On Thu, Oct 31, 2024, Kai Huang wrote:
> > On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > > > +void __init tdx_bringup(void)
> > > > +{
> > > > +	enable_tdx = enable_tdx && !__tdx_bringup();
> > > 
> > > Ah.  I don't love this approach because it mixes "failure" due to an unsupported
> > > configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
> > > fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
> > 
> > Thanks for the comments.
> > 
> > I see your point.  However for "enabling virtualization failure" kvm_init() will
> > also try to do (default behaviour), so if it fails it will result in module
> > loading failure eventually.  So while I guess it would be slightly better to
> > make module loading fail if "enabling virtualization fails" in TDX, it is a nit
> > issue to me.
> > 
> > I think "enabling virtualization failure" is the only "unexpected issue" that
> > should result in module loading failure.  For any other TDX-specific
> > initialization failure (e.g., any memory allocation in future patches) it's
> > better to only disable TDX.
> 
> I disagree.  The platform owner wants TDX to be enabled, KVM shouldn't silently
> disable TDX because of a transient, unrelated failure.
> 
> If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
> disable, then that's different.  And that's the general pattern throughout KVM.
> If a requested feature isn't supported, then KVM continues on updates the module
> param accordingly.  But if something outright fails during setup, KVM aborts the
> entire sequence.
> 
> > So I can change to "make loading KVM-the-module fail if enabling virtualization
> > fails in TDX", but I want to confirm this is what you want?
> 
> I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
> would normally succeed, fails.

I looked at the final tdx.c that in our development branch [*], and below is the
list of the things that need to be done to init TDX (the code in
__tdx_bringup()), and my thinking of whether to fail loading the module or just
disable TDX:

1) Early dependency check fails.  Those include: tdp_mmu_enabled,
enable_mmio_caching, X86_FEATURE_MOVDIR64B check and check the presence of
TSX_CTL uret MSR.

For those we can disable TDX only but continue to load module.

2) Enable virtualization fails.

For this we fail to load module (as you suggested).

3) Fail to register TDX cpuhp to do tdx_cpu_enable() and handle cpu hotplug.

For this we only disable TDX but continue to load module.  The reason is I think
this is similar to enable a specific KVM feature but the hardware doesn't
support it.  We can go further to check the return value of tdx_cpu_enable() to
distinguish cases like "module not loaded" and "unexpected error", but I really
don't want to go that far.

4) tdx_enable() fails.

Ditto to 3).

5) tdx_get_sysinfo() fails.

This is a kernel bug since tdx_get_sysinfo() should always return valid TDX
sysinfo structure pointer after tdx_enable() is done successfully.  Currently we
just WARN() if the returned pointer is NULL and disable TDX only.  I think it's
also fine.

6) TDX global metadata check fails, e.g., MAX_VCPUS etc.

Ditto to 3).  For this we disable TDX only.

To summarize, if you agree with above, then only "enable virtualization failure"
results in module loading failure.

And I am not sure whether it's worth to distinguish those cases.  For the
concern of "KVM shouldn't silently disable TDX because of a transient, unrelated
failure", we can explicitly print error message to tell user why TDX is
disabled.

Any comments?

[*]
https://github.com/intel/tdx/blob/tdx_kvm_dev-2024-10-25.1-host-metadata-v6-rebase/arch/x86/kvm/vmx/tdx.c

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

[Sean Christopherson]

On Wed, Nov 06, 2024, Kai Huang wrote:
> On Thu, 2024-10-31 at 13:22 -0700, Sean Christopherson wrote:
> > On Thu, Oct 31, 2024, Kai Huang wrote:
> > > On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > > > > +void __init tdx_bringup(void)
> > > > > +{
> > > > > +	enable_tdx = enable_tdx && !__tdx_bringup();
> > > > 
> > > > Ah.  I don't love this approach because it mixes "failure" due to an unsupported
> > > > configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
> > > > fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
> > > 
> > > Thanks for the comments.
> > > 
> > > I see your point.  However for "enabling virtualization failure" kvm_init() will
> > > also try to do (default behaviour), so if it fails it will result in module
> > > loading failure eventually.  So while I guess it would be slightly better to
> > > make module loading fail if "enabling virtualization fails" in TDX, it is a nit
> > > issue to me.
> > > 
> > > I think "enabling virtualization failure" is the only "unexpected issue" that
> > > should result in module loading failure.  For any other TDX-specific
> > > initialization failure (e.g., any memory allocation in future patches) it's
> > > better to only disable TDX.
> > 
> > I disagree.  The platform owner wants TDX to be enabled, KVM shouldn't silently
> > disable TDX because of a transient, unrelated failure.
> > 
> > If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
> > disable, then that's different.  And that's the general pattern throughout KVM.
> > If a requested feature isn't supported, then KVM continues on updates the module
> > param accordingly.  But if something outright fails during setup, KVM aborts the
> > entire sequence.
> > 
> > > So I can change to "make loading KVM-the-module fail if enabling virtualization
> > > fails in TDX", but I want to confirm this is what you want?
> > 
> > I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
> > would normally succeed, fails.
> 
> I looked at the final tdx.c that in our development branch [*], and below is the
> list of the things that need to be done to init TDX (the code in
> __tdx_bringup()), and my thinking of whether to fail loading the module or just
> disable TDX:
> 
> 1) Early dependency check fails.  Those include: tdp_mmu_enabled,
> enable_mmio_caching, X86_FEATURE_MOVDIR64B check and check the presence of
> TSX_CTL uret MSR.
> 
> For those we can disable TDX only but continue to load module.
> 
> 2) Enable virtualization fails.
> 
> For this we fail to load module (as you suggested).
> 
> 3) Fail to register TDX cpuhp to do tdx_cpu_enable() and handle cpu hotplug.
> 
> For this we only disable TDX but continue to load module.  The reason is I think
> this is similar to enable a specific KVM feature but the hardware doesn't
> support it.  We can go further to check the return value of tdx_cpu_enable() to
> distinguish cases like "module not loaded" and "unexpected error", but I really
> don't want to go that far.

Hrm, tdx_cpu_enable() is a bit of a mess.  Ideally, there would be a separate
"probe" API so that KVM could detect if TDX is supported.  Though maybe it's the
TDX module itself is flawed, e.g. if TDH_SYS_INIT is literally the only way to
detect whether or not a module is loaded.

So, absent a way to clean up tdx_cpu_enable(), maybe disable the module param if
it returns -ENODEV, otherwise fail the module load?

> 4) tdx_enable() fails.
> 
> Ditto to 3).

No, this should fail the module load.  E.g. most of the error conditions are
-ENOMEM, which has nothing to do with host support for TDX.

> 5) tdx_get_sysinfo() fails.
> 
> This is a kernel bug since tdx_get_sysinfo() should always return valid TDX
> sysinfo structure pointer after tdx_enable() is done successfully.  Currently we
> just WARN() if the returned pointer is NULL and disable TDX only.  I think it's
> also fine.
> 
> 6) TDX global metadata check fails, e.g., MAX_VCPUS etc.
> 
> Ditto to 3).  For this we disable TDX only.

Where is this code?

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

[Huang, Kai]

On Wed, 2024-11-06 at 07:01 -0800, Sean Christopherson wrote:
> On Wed, Nov 06, 2024, Kai Huang wrote:
> > On Thu, 2024-10-31 at 13:22 -0700, Sean Christopherson wrote:
> > > On Thu, Oct 31, 2024, Kai Huang wrote:
> > > > On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > > > > > +void __init tdx_bringup(void)
> > > > > > +{
> > > > > > +	enable_tdx = enable_tdx && !__tdx_bringup();
> > > > > 
> > > > > Ah.  I don't love this approach because it mixes "failure" due to an unsupported
> > > > > configuration, with failure due to unexpected issues.  E.g. if enabling virtualization
> > > > > fails, loading KVM-the-module absolutely should fail too, not simply disable TDX.
> > > > 
> > > > Thanks for the comments.
> > > > 
> > > > I see your point.  However for "enabling virtualization failure" kvm_init() will
> > > > also try to do (default behaviour), so if it fails it will result in module
> > > > loading failure eventually.  So while I guess it would be slightly better to
> > > > make module loading fail if "enabling virtualization fails" in TDX, it is a nit
> > > > issue to me.
> > > > 
> > > > I think "enabling virtualization failure" is the only "unexpected issue" that
> > > > should result in module loading failure.  For any other TDX-specific
> > > > initialization failure (e.g., any memory allocation in future patches) it's
> > > > better to only disable TDX.
> > > 
> > > I disagree.  The platform owner wants TDX to be enabled, KVM shouldn't silently
> > > disable TDX because of a transient, unrelated failure.
> > > 
> > > If TDX _can't_ be supported, e.g. because EPT or MMIO SPTE caching was explicitly
> > > disable, then that's different.  And that's the general pattern throughout KVM.
> > > If a requested feature isn't supported, then KVM continues on updates the module
> > > param accordingly.  But if something outright fails during setup, KVM aborts the
> > > entire sequence.
> > > 
> > > > So I can change to "make loading KVM-the-module fail if enabling virtualization
> > > > fails in TDX", but I want to confirm this is what you want?
> > > 
> > > I would prefer the logic to be: reject loading kvm-intel.ko if an operation that
> > > would normally succeed, fails.
> > 
> > I looked at the final tdx.c that in our development branch [*], and below is the
> > list of the things that need to be done to init TDX (the code in
> > __tdx_bringup()), and my thinking of whether to fail loading the module or just
> > disable TDX:
> > 
> > 1) Early dependency check fails.  Those include: tdp_mmu_enabled,
> > enable_mmio_caching, X86_FEATURE_MOVDIR64B check and check the presence of
> > TSX_CTL uret MSR.
> > 
> > For those we can disable TDX only but continue to load module.
> > 
> > 2) Enable virtualization fails.
> > 
> > For this we fail to load module (as you suggested).
> > 
> > 3) Fail to register TDX cpuhp to do tdx_cpu_enable() and handle cpu hotplug.
> > 
> > For this we only disable TDX but continue to load module.  The reason is I think
> > this is similar to enable a specific KVM feature but the hardware doesn't
> > support it.  We can go further to check the return value of tdx_cpu_enable() to
> > distinguish cases like "module not loaded" and "unexpected error", but I really
> > don't want to go that far.
> 
> Hrm, tdx_cpu_enable() is a bit of a mess.  Ideally, there would be a separate
> "probe" API so that KVM could detect if TDX is supported.  Though maybe it's the
> TDX module itself is flawed, e.g. if TDH_SYS_INIT is literally the only way to
> detect whether or not a module is loaded.

We can also use P-SEAMLDR SEAMCALL to query, but I see no difference between
using TDH_SYS_INIT.  If you are asking whether there's CPUID or MSR to query
then no.

> 
> So, absent a way to clean up tdx_cpu_enable(), maybe disable the module param if
> it returns -ENODEV, otherwise fail the module load?

We can, but we need to assume cpuhp_setup_state_cpuslocked() itself will not
return -ENODEV (it is true now), otherwise we won't be able to distinguish
whether the -ENODEV was from cpuhp_setup_state_cpuslocked() or tdx_cpu_enable().

Unless we choose to do tdx_cpu_enable() via on_each_cpu() separately.

Btw tdx_cpu_enable() itself will print "module not loaded" in case of -ENODEV,
so the user will be aware anyway if we only disable TDX but not fail module
loading.

My concern is still the whole "different handling of error cases" seems over-
engineering.

> 
> > 4) tdx_enable() fails.
> > 
> > Ditto to 3).
> 
> No, this should fail the module load.  E.g. most of the error conditions are
> -ENOMEM, which has nothing to do with host support for TDX.
> 
> > 5) tdx_get_sysinfo() fails.
> > 
> > This is a kernel bug since tdx_get_sysinfo() should always return valid TDX
> > sysinfo structure pointer after tdx_enable() is done successfully.  Currently we
> > just WARN() if the returned pointer is NULL and disable TDX only.  I think it's
> > also fine.
> > 
> > 6) TDX global metadata check fails, e.g., MAX_VCPUS etc.
> > 
> > Ditto to 3).  For this we disable TDX only.
> 
> Where is this code?

Please check:

https://github.com/intel/tdx/blob/tdx_kvm_dev-2024-10-25.1-host-metadata-v6-rebase/arch/x86/kvm/vmx/tdx.c

.. starting at line 3320.

If you want individual commits, here's the list:


KVM: TDX: Get TDX global Information
https://github.com/intel/tdx/commit/6ae3ab1ddb51a4cf0f0810853a24d47d360abaea

KVM: TDX: Get system-wide info about TDX module on initialization
https://github.com/intel/tdx/commit/fd7947118b76f6d4256bc4228e03e73262e67ba2

KVM: TDX: Support per-VM KVM_CAP_MAX_VCPUS extension check
https://github.com/intel/tdx/commit/98162cf99ee728b97a0c9647bd2b39a254da6a4a

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

[Sean Christopherson]

On Wed, Nov 06, 2024, Kai Huang wrote:
> On Wed, 2024-11-06 at 07:01 -0800, Sean Christopherson wrote:
> > On Wed, Nov 06, 2024, Kai Huang wrote:
> > > For this we only disable TDX but continue to load module.  The reason is I think
> > > this is similar to enable a specific KVM feature but the hardware doesn't
> > > support it.  We can go further to check the return value of tdx_cpu_enable() to
> > > distinguish cases like "module not loaded" and "unexpected error", but I really
> > > don't want to go that far.
> > 
> > Hrm, tdx_cpu_enable() is a bit of a mess.  Ideally, there would be a separate
> > "probe" API so that KVM could detect if TDX is supported.  Though maybe it's the
> > TDX module itself is flawed, e.g. if TDH_SYS_INIT is literally the only way to
> > detect whether or not a module is loaded.
> 
> We can also use P-SEAMLDR SEAMCALL to query, but I see no difference between
> using TDH_SYS_INIT.  If you are asking whether there's CPUID or MSR to query
> then no.

Doesn't have to be a CPUID or MSR, anything idempotent would work.  Which begs
the question, is that P-SEAMLDR SEAMCALL query you have in mind idempotent? :-)

> > So, absent a way to clean up tdx_cpu_enable(), maybe disable the module param if
> > it returns -ENODEV, otherwise fail the module load?
> 
> We can, but we need to assume cpuhp_setup_state_cpuslocked() itself will not
> return -ENODEV (it is true now), otherwise we won't be able to distinguish
> whether the -ENODEV was from cpuhp_setup_state_cpuslocked() or tdx_cpu_enable().
> 
> Unless we choose to do tdx_cpu_enable() via on_each_cpu() separately.
> 
> Btw tdx_cpu_enable() itself will print "module not loaded" in case of -ENODEV,
> so the user will be aware anyway if we only disable TDX but not fail module
> loading.

That only helps if a human looks at dmesg before attempting to run a TDX VM on
the host, and parsing dmesg to treat that particular scenario as fatal isn't
something I want to recommend to end users.  E.g. if our platform configuration
screwed up and failed to load a TDX module, then I want that to be surfaced as
an alarm of sorts, not a silent "this platform doesn't support TDX" flag.

> My concern is still the whole "different handling of error cases" seems over-
> engineering.

IMO, that's a symptom of the TDX enabling code not cleanly separating "probe"
from "enable", and at a glance, that seems very solvable.  And I suspect that
cleaning things up will allow for additional hardening.  E.g. I assume the lack
of MOVDIR64B should be a WARN, but because KVM checks for MOVDIR64B before
checking for basic TDX support, it's an non-commitalpr_warn().

> > > 4) tdx_enable() fails.
> > > 
> > > Ditto to 3).
> > 
> > No, this should fail the module load.  E.g. most of the error conditions are
> > -ENOMEM, which has nothing to do with host support for TDX.
> > 
> > > 5) tdx_get_sysinfo() fails.
> > > 
> > > This is a kernel bug since tdx_get_sysinfo() should always return valid TDX
> > > sysinfo structure pointer after tdx_enable() is done successfully.  Currently we
> > > just WARN() if the returned pointer is NULL and disable TDX only.  I think it's
> > > also fine.
> > > 
> > > 6) TDX global metadata check fails, e.g., MAX_VCPUS etc.
> > > 
> > > Ditto to 3).  For this we disable TDX only.
> > 
> > Where is this code?
> 
> Please check:
> 
> https://github.com/intel/tdx/blob/tdx_kvm_dev-2024-10-25.1-host-metadata-v6-rebase/arch/x86/kvm/vmx/tdx.c
> 
> .. starting at line 3320.

Before I forget, that code has a bug.  This

	/* Check TDX module and KVM capabilities */
	if (!tdx_get_supported_attrs(&tdx_sysinfo->td_conf) ||
	    !tdx_get_supported_xfam(&tdx_sysinfo->td_conf))
		goto get_sysinfo_err;

will return '0' on error, instead of -EINVAL (or whatever it intends).

Back to the main discussion, these checks are obvious "probe" failures and so
should disable TDX without failing module load:

	if (!tdp_mmu_enabled || !enable_mmio_caching)
		return -EOPNOTSUPP;

	if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
		pr_warn("MOVDIR64B is reqiured for TDX\n");
		return -EOPNOTSUPP;
	}

A kvm_find_user_return_msr() error is obviously a KVM bug, i.e. should definitely
WARN and fail module module.  Ditto for kvm_enable_virtualization().

The boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM) that's buried in tdx_enable()
really belongs in KVM.  Having it in both is totally fine, but KVM shouldn't do
a bunch of work and _then_ check if all that work was pointless.

I am ok treating everything at or after tdx_get_sysinfo() as fatal to module load,
especially since, IIUC, TD_SYS_INIT can't be undone, i.e. KVM has crossed a point
of no return.

In short, assuming KVM can query if a TDX module is a loaded, I don't think it's
all that much work to do:

  static bool kvm_is_tdx_supported(void)
  {
	if (boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM))
		return false;

	if (!<is TDX module loaded>)
		return false;

	if (!tdp_mmu_enabled || !enable_mmio_caching)
		return false;

	if (WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)))
		return false;

	return true;
  }

  int __init tdx_bringup(void)
  {
	enable_tdx = enable_tdx && kvm_is_tdx_supported();
	if (!enable_tdx)
		return 0;

	return __tdx_bringup();
  }

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

[Huang, Kai]

On 8/11/2024 11:04 am, Sean Christopherson wrote:
> On Wed, Nov 06, 2024, Kai Huang wrote:
>> On Wed, 2024-11-06 at 07:01 -0800, Sean Christopherson wrote:
>>> On Wed, Nov 06, 2024, Kai Huang wrote:
>>>> For this we only disable TDX but continue to load module.  The reason is I think
>>>> this is similar to enable a specific KVM feature but the hardware doesn't
>>>> support it.  We can go further to check the return value of tdx_cpu_enable() to
>>>> distinguish cases like "module not loaded" and "unexpected error", but I really
>>>> don't want to go that far.
>>>
>>> Hrm, tdx_cpu_enable() is a bit of a mess.  Ideally, there would be a separate
>>> "probe" API so that KVM could detect if TDX is supported.  Though maybe it's the
>>> TDX module itself is flawed, e.g. if TDH_SYS_INIT is literally the only way to
>>> detect whether or not a module is loaded.
>>
>> We can also use P-SEAMLDR SEAMCALL to query, but I see no difference between
>> using TDH_SYS_INIT.  If you are asking whether there's CPUID or MSR to query
>> then no.
> 
> Doesn't have to be a CPUID or MSR, anything idempotent would work.  Which begs
> the question, is that P-SEAMLDR SEAMCALL query you have in mind idempotent? :-)

It is the SEAMLDR.INFO SEAMCALL, which writes bunch of information to a 
SEAMLDR_INFO structure.  There's one bit "SEAM_READY" which (when true) 
indicates the TDX module is ready for SEAMCALL, i.e., the module is loaded.

And yes it is idempotent I believe, even we consider TDX module runtime 
reload/update.

The problem is it is a SEAMCALL, and being a SEAMCALL requires all 
things like enabling virtualization first and adding another wrapper API 
and structure definition to do SEAMLDR.INFO (and we are still in 
discussion how to export SEAMCALLs to let KVM make).

 From this perspective, I don't see a big difference between using 
SEAMLDR.INFO and tdx_cpu_enable() for probing TDX.

I agree we can change to use SEAMLDR.INFO to detect in the long term 
after we move VMXON out of KVM, though, because we can get a lot more 
information with that besides whether module is loaded.  But before 
that, I see no big difference.

> 
>>> So, absent a way to clean up tdx_cpu_enable(), maybe disable the module param if
>>> it returns -ENODEV, otherwise fail the module load?
>>
>> We can, but we need to assume cpuhp_setup_state_cpuslocked() itself will not
>> return -ENODEV (it is true now), otherwise we won't be able to distinguish
>> whether the -ENODEV was from cpuhp_setup_state_cpuslocked() or tdx_cpu_enable().
>>
>> Unless we choose to do tdx_cpu_enable() via on_each_cpu() separately.
>>
>> Btw tdx_cpu_enable() itself will print "module not loaded" in case of -ENODEV,
>> so the user will be aware anyway if we only disable TDX but not fail module
>> loading.
> 
> That only helps if a human looks at dmesg before attempting to run a TDX VM on
> the host, and parsing dmesg to treat that particular scenario as fatal isn't
> something I want to recommend to end users.  E.g. if our platform configuration
> screwed up and failed to load a TDX module, then I want that to be surfaced as
> an alarm of sorts, not a silent "this platform doesn't support TDX" flag.
> 
>> My concern is still the whole "different handling of error cases" seems over-
>> engineering.
> 
> IMO, that's a symptom of the TDX enabling code not cleanly separating "probe"
> from "enable", and at a glance, that seems very solvable.  

I am not so sure about this at this stage, because you need to make a 
SEAMCALL anyway for that. :-)

I think we can document this imperfection for now and enhance after 
moving VMXON out of KVM.

Btw we are going to add P-SEAMLDR SEAMCALLs to support TDX runtime 
reload/update anyway, so we can add SEAMLDR.INFO there (or before that...).

> And I suspect that
> cleaning things up will allow for additional hardening.  E.g. I assume the lack
> of MOVDIR64B should be a WARN, but because KVM checks for MOVDIR64B before
> checking for basic TDX support, it's an non-commitalpr_warn().

Yeah if we check TDX_HOST_PLATFORM first then we can WARN() on MOVDIR64B.

> 
>>>> 4) tdx_enable() fails.
>>>>
>>>> Ditto to 3).
>>>
>>> No, this should fail the module load.  E.g. most of the error conditions are
>>> -ENOMEM, which has nothing to do with host support for TDX.
>>>
>>>> 5) tdx_get_sysinfo() fails.
>>>>
>>>> This is a kernel bug since tdx_get_sysinfo() should always return valid TDX
>>>> sysinfo structure pointer after tdx_enable() is done successfully.  Currently we
>>>> just WARN() if the returned pointer is NULL and disable TDX only.  I think it's
>>>> also fine.
>>>>
>>>> 6) TDX global metadata check fails, e.g., MAX_VCPUS etc.
>>>>
>>>> Ditto to 3).  For this we disable TDX only.
>>>
>>> Where is this code?
>>
>> Please check:
>>
>> https://github.com/intel/tdx/blob/tdx_kvm_dev-2024-10-25.1-host-metadata-v6-rebase/arch/x86/kvm/vmx/tdx.c
>>
>> .. starting at line 3320.
> 
> Before I forget, that code has a bug.  This
> 
> 	/* Check TDX module and KVM capabilities */
> 	if (!tdx_get_supported_attrs(&tdx_sysinfo->td_conf) ||
> 	    !tdx_get_supported_xfam(&tdx_sysinfo->td_conf))
> 		goto get_sysinfo_err;
> 
> will return '0' on error, instead of -EINVAL (or whatever it intends).

Indeed.  Thanks for catching this.  I'll report to Xiaoyao.

> 
> Back to the main discussion, these checks are obvious "probe" failures and so
> should disable TDX without failing module load:
> 
> 	if (!tdp_mmu_enabled || !enable_mmio_caching)
> 		return -EOPNOTSUPP;
> 
> 	if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
> 		pr_warn("MOVDIR64B is reqiured for TDX\n");
> 		return -EOPNOTSUPP;
> 	}

Yeah sure.

> 
> A kvm_find_user_return_msr() error is obviously a KVM bug, i.e. should definitely
> WARN and fail module module.  Ditto for kvm_enable_virtualization().

OK agreed.

> 
> The boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM) that's buried in tdx_enable()
> really belongs in KVM.  Having it in both is totally fine, but KVM shouldn't do
> a bunch of work and _then_ check if all that work was pointless.

Fine to me.

> 
> I am ok treating everything at or after tdx_get_sysinfo() as fatal to module load,
> especially since, IIUC, TD_SYS_INIT can't be undone, i.e. KVM has crossed a point
> of no return.
> 
> In short, assuming KVM can query if a TDX module is a loaded, I don't think it's
> all that much work to do:
> 
>    static bool kvm_is_tdx_supported(void)
>    {
> 	if (boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM))
> 		return false;
> 
> 	if (!<is TDX module loaded>)
> 		return false;
> 
> 	if (!tdp_mmu_enabled || !enable_mmio_caching)
> 		return false;
> 
> 	if (WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)))
> 		return false;
> 
> 	return true;
>    }
> 
>    int __init tdx_bringup(void)
>    {
> 	enable_tdx = enable_tdx && kvm_is_tdx_supported();
> 	if (!enable_tdx)
> 		return 0;
> 
> 	return __tdx_bringup();
>    }

Thanks for clarifying this.

As mentioned above, I would say we just use tdx_cpu_enable() to "probe" 
whether module is present before moving VMXON out of KVM.  We can 
document this imperfection for now and revisit later.

How about:

static bool kvm_can_support_tdx(void)
{
	if (boot_cpu_has(X86_FEATURE_TDX_HOST_PLATFORM))
		return false;

	if (!tdp_mmu_enabled || !enable_mmio_caching)
  		return false;

	if (WARN_ON_ONCE(!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)))
		return false;
}

int __init tdx_bringup(void)
{
	int r;

	enable_tdx = enable_tdx && kvm_can_support_tdx();

	if (!enable_tdx)
		return 0;

	/*
	 * Ideally KVM should probe whether TDX module has been loaded
	 * first and then try to bring it up, because KVM should treat
	 * them differently.  I.e., KVM should just disable TDX while
	 * still allow module to be loaded when TDX module is not
	 * loaded, but fail to load module at all when fail to bring up
	 * TDX.
	 *
	 * But unfortunately TDX needs to use SEAMCALL to probe whether
	 * the module is loaded (there is not CPUID or MSR for that),
	 * and making SEAMCALL requires enabling virtualization first (
	 * like the rest steps of bringing up TDX module).
	 *
	 * For simplicity, do the probing and bringing up together for
	 * now.
	 *
	 * Note the first SEAMCALL to bringing up TDX will return
	 * -ENODEV when module is not loaded, and this serves the probe
	 * albeit it is not perfect.
	 *
	 * Another option is using P-SEAMLDR's SEAMLDR.INFO SEAMCALL to
	 * probe, but it is still a SEAMCALL.  Currently kernel doesn't
	 * support P-SEAMLDR SEAMCALLs so don't bother to add it just
	 * for probing TDX module.
	 *
	 * Again, this is not perfect, and can be revisited once VMXON
	 * is moved to the core-kernel.
	 */
	r = __tdx_probe_and_bringup();
	if (r) {
		enable_tdx = 0;
		/*
		 * Disable TDX only but don't fail to load module when
		 * TDX module is not loaded.  No need to print error
		 * message since __tdx_probe_and_bringup() already did
		 * that in this case.
		 */
		if (r == -ENODEV)
			r = 0;
	}
	
	return r;
}

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

[Dan Williams]

Huang, Kai wrote:
> On Wed, 2024-10-30 at 08:19 -0700, Sean Christopherson wrote:
> > On Tue, Oct 29, 2024, Kai Huang wrote:
> > > diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> > > index f9dddb8cb466..fec803aff7ad 100644
> > > --- a/arch/x86/kvm/Makefile
> > > +++ b/arch/x86/kvm/Makefile
> > > @@ -20,6 +20,7 @@ kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
> > >  
> > >  kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
> > >  kvm-intel-$(CONFIG_KVM_HYPERV)	+= vmx/hyperv.o vmx/hyperv_evmcs.o
> > > +kvm-intel-$(CONFIG_INTEL_TDX_HOST)	+= vmx/tdx.o
> > 
> > IMO, INTEL_TDX_HOST should be a KVM Kconfig, e.g. KVM_INTEL_TDX.  Forcing the user
> > to bounce between KVM's menu and the generic menu to enable KVM support for TDX is
> > kludgy.  Having INTEL_TDX_HOST exist before KVM support came along made sense, as
> > it allowed compile-testing a bunch of code, but I don't think it should be the end
> > state.
> > 
> > If others disagree, then we should adjust KVM_AMD_SEV in the opposite direction,
> > because doing different things for SEV vs. TDX is confusing and messy.
> 
> + Dave (and Dan for TDX Connect).
> 
> Agree SEV/TDX should be in similar way.  But also I find SEV has a dependency on
> CRYPTO_DEV_SP_PSP, so perhaps it also reasonable to make an additional
> KVM_INTEL_TDX and make it depend on INTEL_TDX_HOST?
> 
> We could remove INTEL_TDX_HOST but only keep KVM_INTEL_TDX.  But in the long
> term, more kernel components will need to add TDX support (e.g., for TDX
> Connect).  I think the question is whether we can safely disable TDX code in ALL
> kernel components when KVM_INTEL_TDX is not enabled.
> 
> If the answer is yes (seems correct to me, because it seems meaningless to
> enable TDX code in _ANY_ kernel components when it's even possible to run TDX 
> guest), then I think we can just change the current INTEL_TDX_HOST to
> KVM_INTEL_TDX and put it in arch/x86/kvm/Kconfig.

I agree with Sean's later reply that kvm-intel.ko should fail if
anything that is expected to be there and not otherwise permanently
disabled fails setup.

However, I want to provide a counterpoint to this "_ANY_ kernel
component" dependency on being able to run a TDX guest. TDX Connect like
SEV-TIO offers device-security provisioning flows that are expected to
run before any confidential guest is being launched, and theoretically
may offer services independent of *ever* launching a guest (e.g. PCIe
link encrcyption without device assignment). So longer term, seamcalls
without kvm-intel.ko flexibility is useful, but in the near term a
coarse dependency on kvm-intel.ko is workable.

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

[Huang, Kai]

> However, I want to provide a counterpoint to this "_ANY_ kernel
> component" dependency on being able to run a TDX guest. TDX Connect like
> SEV-TIO offers device-security provisioning flows that are expected to
> run before any confidential guest is being launched, and theoretically
> may offer services independent of *ever* launching a guest (e.g. PCIe
> link encrcyption without device assignment). So longer term, seamcalls
> without kvm-intel.ko flexibility is useful, but in the near term a
> coarse dependency on kvm-intel.ko is workable.

Thanks for the info.

So it seems we should keep INTEL_TDX_HOST but add a new KVM_INTEL_TDX:

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index f09f13c01c6b..bcf4a1243013 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -126,6 +126,16 @@ config X86_SGX_KVM

           If unsure, say N.

+config KVM_INTEL_TDX
+       bool "Intel Trust Domain Extensions (TDX) support"
+       default y
+       depends on INTEL_TDX_HOST
+       help
+         Provides support for launching Intel Trust Domain Extensions
+         (TDX) confidential VMs on Intel processors.
+
+         If unsure, say N.
+
  config KVM_AMD
         tristate "KVM for AMD processors support"
         depends on KVM && (CPU_SUP_AMD || CPU_SUP_HYGON)
diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index fec803aff7ad..a5d362c7b504 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -20,7 +20,7 @@ kvm-intel-y           += vmx/vmx.o vmx/vmenter.o 
vmx/pmu_intel.o vmx/vmcs12.o \

  kvm-intel-$(CONFIG_X86_SGX_KVM)        += vmx/sgx.o
  kvm-intel-$(CONFIG_KVM_HYPERV) += vmx/hyperv.o vmx/hyperv_evmcs.o
-kvm-intel-$(CONFIG_INTEL_TDX_HOST)     += vmx/tdx.o
+kvm-intel-$(CONFIG_KVM_INTEL_TDX)      += vmx/tdx.o

  kvm-amd-y              += svm/svm.o svm/vmenter.o svm/pmu.o 
svm/nested.o svm/avic.o

One thing is currently INTEL_TDX_HOST depends on KVM_INTEL (with the 
reason that for now only KVM will use TDX), we can either remove this 
dependency together with the above diff, or we can have another patch in 
the future to remove that when TDX Connect comes near.

I think we can leave this part to the future.

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 007bab9f2a0e..acc5a14dfbbc 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1974,7 +1974,6 @@ config INTEL_TDX_HOST
         bool "Intel Trust Domain Extensions (TDX) host support"
         depends on CPU_SUP_INTEL
         depends on X86_64
-       depends on KVM_INTEL
         depends on X86_X2APIC
         select ARCH_KEEP_MEMBLOCK
         depends on CONTIG_ALLOC

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

[Dan Williams]

Huang, Kai wrote:
[..]
> One thing is currently INTEL_TDX_HOST depends on KVM_INTEL (with the 
> reason that for now only KVM will use TDX), we can either remove this 
> dependency together with the above diff, or we can have another patch in 
> the future to remove that when TDX Connect comes near.
> 
> I think we can leave this part to the future.

Agree, save that for later. There is latent effort to disconnect VTx
from KVM that TDX Connect is also eyeing, and I expect that work is what
allows INTEL_TDX_HOST to drop its KVM_INTEL dependency.

Re: [PATCH 0/3] KVM: VMX: Initialize TDX when loading KVM module

[Paolo Bonzini]

> This series contains patches to initialize TDX when loading KVM module.
> This series is based on the discussion with Sean on the v19 patchset
> [*], hoping it has addressed most (if not all) comments.
>
> This series has been in our internal TDX tree for long time and has been
> in kvm-coco-queue for some time thus it has been tested.
>
> The main purpose for sending out is to have a review but this series can
> also be applied to kvm/queue cleanly.

Thanks, I'll replace the commits in kvm-coco-queue so that it tracks
the posting to the mailing list.

Paolo

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

[Paolo Bonzini]

This is a slightly reviewed version of Kai's same-titled series.
The changes in patch 4 I include rewording the comments, adding
a check that enable_virt_at_load=1, and an error message for
!cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM).

Paolo

Kai Huang (3):
  KVM: Export hardware virtualization enabling/disabling functions
  KVM: VMX: Refactor VMX module init/exit functions
  KVM: VMX: Initialize TDX during KVM module load

 arch/x86/kvm/Kconfig     |  10 +++
 arch/x86/kvm/Makefile    |   1 +
 arch/x86/kvm/vmx/main.c  |  41 ++++++++++
 arch/x86/kvm/vmx/tdx.c   | 160 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/tdx.h   |  13 ++++
 arch/x86/kvm/vmx/vmx.c   |  23 +-----
 arch/x86/kvm/vmx/vmx.h   |   3 +
 include/linux/kvm_host.h |   9 +++
 virt/kvm/kvm_main.c      |  21 ++---
 9 files changed, 245 insertions(+), 36 deletions(-)
 create mode 100644 arch/x86/kvm/vmx/tdx.c
 create mode 100644 arch/x86/kvm/vmx/tdx.h

-- 
2.43.5

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

[Paolo Bonzini]

From: Kai Huang <kai.huang@intel.com>

Before KVM can use TDX to create and run TDX guests, TDX needs to be
initialized from two perspectives: 1) TDX module must be initialized
properly to a working state; 2) A per-cpu TDX initialization, a.k.a the
TDH.SYS.LP.INIT SEAMCALL must be done on any logical cpu before it can
run any other TDX SEAMCALLs.

The TDX host core-kernel provides two functions to do the above two
respectively: tdx_enable() and tdx_cpu_enable().

There are two options in terms of when to initialize TDX: initialize TDX
at KVM module loading time, or when creating the first TDX guest.

Choose to initialize TDX during KVM module loading time:

Initializing TDX module is both memory and CPU time consuming: 1) the
kernel needs to allocate a non-trivial size(~1/256) of system memory
as metadata used by TDX module to track each TDX-usable memory page's
status; 2) the TDX module needs to initialize this metadata, one entry
for each TDX-usable memory page.

Also, the kernel uses alloc_contig_pages() to allocate those metadata
chunks, because they are large and need to be physically contiguous.
alloc_contig_pages() can fail.  If initializing TDX when creating the
first TDX guest, then there's chance that KVM won't be able to run any
TDX guests albeit KVM _declares_ to be able to support TDX.

This isn't good for the user.

On the other hand, initializing TDX at KVM module loading time can make
sure KVM is providing a consistent view of whether KVM can support TDX
to the user.

Always only try to initialize TDX after VMX has been initialized.  TDX
is based on VMX, and if VMX fails to initialize then TDX is likely to be
broken anyway.  Also, in practice, supporting TDX will require part of
VMX and common x86 infrastructure in working order, so TDX cannot be
enabled alone w/o VMX support.

There are two cases that can result in failure to initialize TDX: 1) TDX
cannot be supported (e.g., because of TDX is not supported or enabled by
hardware, or module is not loaded, or missing some dependency in KVM's
configuration); 2) Any unexpected error during TDX bring-up.  For the
first case only mark TDX is disabled but still allow KVM module to be
loaded.  For the second case just fail to load the KVM module so that
the user can be aware.

Because TDX costs additional memory, don't enable TDX by default.  Add a
new module parameter 'enable_tdx' to allow the user to opt-in.

Note, the name tdx_init() has already been taken by the early boot code.
Use tdx_bringup() for initializing TDX (and tdx_cleanup() since KVM
doesn't actually teardown TDX).  They don't match vt_init()/vt_exit(),
vmx_init()/vmx_exit() etc but it's not end of the world.

Also, once initialized, the TDX module cannot be disabled and enabled
again w/o the TDX module runtime update, which isn't supported by the
kernel.  After TDX is enabled, nothing needs to be done when KVM
disables hardware virtualization, e.g., when offlining CPU, or during
suspend/resume.  TDX host core-kernel code internally tracks TDX status
and can handle "multiple enabling" scenario.

Similar to KVM_AMD_SEV, add a new KVM_INTEL_TDX Kconfig to guide KVM TDX
code.  Make it depend on INTEL_TDX_HOST but not replace INTEL_TDX_HOST
because in the longer term there's a use case that requires making
SEAMCALLs w/o KVM as mentioned by Dan [1].

Link: https://lore.kernel.org/6723fc2070a96_60c3294dc@dwillia2-mobl3.amr.corp.intel.com.notmuch/ [1]
Signed-off-by: Kai Huang <kai.huang@intel.com>
Message-ID: <162f9dee05c729203b9ad6688db1ca2960b4b502.1731664295.git.kai.huang@intel.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/Kconfig     |  10 +++
 arch/x86/kvm/Makefile    |   1 +
 arch/x86/kvm/vmx/main.c  |   9 +++
 arch/x86/kvm/vmx/tdx.c   | 160 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/vmx/tdx.h   |  13 ++++
 include/linux/kvm_host.h |   1 +
 virt/kvm/kvm_main.c      |   3 +-
 7 files changed, 196 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kvm/vmx/tdx.c
 create mode 100644 arch/x86/kvm/vmx/tdx.h

diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index ea2c4f21c1ca..fe8cbee6f614 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -128,6 +128,16 @@ config X86_SGX_KVM
 
 	  If unsure, say N.
 
+config KVM_INTEL_TDX
+	bool "Intel Trust Domain Extensions (TDX) support"
+	default y
+	depends on INTEL_TDX_HOST
+	help
+	  Provides support for launching Intel Trust Domain Extensions (TDX)
+	  confidential VMs on Intel processors.
+
+	  If unsure, say N.
+
 config KVM_AMD
 	tristate "KVM for AMD processors support"
 	depends on KVM && (CPU_SUP_AMD || CPU_SUP_HYGON)
diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index f9dddb8cb466..a5d362c7b504 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -20,6 +20,7 @@ kvm-intel-y		+= vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o vmx/vmcs12.o \
 
 kvm-intel-$(CONFIG_X86_SGX_KVM)	+= vmx/sgx.o
 kvm-intel-$(CONFIG_KVM_HYPERV)	+= vmx/hyperv.o vmx/hyperv_evmcs.o
+kvm-intel-$(CONFIG_KVM_INTEL_TDX)	+= vmx/tdx.o
 
 kvm-amd-y		+= svm/svm.o svm/vmenter.o svm/pmu.o svm/nested.o svm/avic.o
 
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 6772e560ac7b..bc690c63e511 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -6,6 +6,7 @@
 #include "nested.h"
 #include "pmu.h"
 #include "posted_intr.h"
+#include "tdx.h"
 
 #define VMX_REQUIRED_APICV_INHIBITS				\
 	(BIT(APICV_INHIBIT_REASON_DISABLED) |			\
@@ -171,6 +172,7 @@ struct kvm_x86_init_ops vt_init_ops __initdata = {
 static void vt_exit(void)
 {
 	kvm_exit();
+	tdx_cleanup();
 	vmx_exit();
 }
 module_exit(vt_exit);
@@ -183,6 +185,11 @@ static int __init vt_init(void)
 	if (r)
 		return r;
 
+	/* tdx_init() has been taken */
+	r = tdx_bringup();
+	if (r)
+		goto err_tdx_bringup;
+
 	/*
 	 * Common KVM initialization _must_ come last, after this, /dev/kvm is
 	 * exposed to userspace!
@@ -195,6 +202,8 @@ static int __init vt_init(void)
 	return 0;
 
 err_kvm_init:
+	tdx_cleanup();
+err_tdx_bringup:
 	vmx_exit();
 	return r;
 }
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
new file mode 100644
index 000000000000..0666dfbe0bc0
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/cpu.h>
+#include <asm/cpufeature.h>
+#include <asm/tdx.h>
+#include "capabilities.h"
+#include "tdx.h"
+
+#undef pr_fmt
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+static bool enable_tdx __ro_after_init;
+module_param_named(tdx, enable_tdx, bool, 0444);
+
+static enum cpuhp_state tdx_cpuhp_state;
+
+static int tdx_online_cpu(unsigned int cpu)
+{
+	unsigned long flags;
+	int r;
+
+	/* Sanity check CPU is already in post-VMXON */
+	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
+
+	local_irq_save(flags);
+	r = tdx_cpu_enable();
+	local_irq_restore(flags);
+
+	return r;
+}
+
+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 bool __init kvm_can_support_tdx(void)
+{
+	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);
+}
+
+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();
+
+	if (r)
+		goto tdx_bringup_err;
+
+	/*
+	 * Leave hardware virtualization enabled after TDX is enabled
+	 * successfully.  TDX CPU hotplug depends on this.
+	 */
+	return 0;
+tdx_bringup_err:
+	kvm_disable_virtualization();
+	return r;
+}
+
+void tdx_cleanup(void)
+{
+	if (enable_tdx) {
+		__do_tdx_cleanup();
+		kvm_disable_virtualization();
+	}
+}
+
+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;
+	}
+
+	/*
+	 * Ideally KVM should probe whether TDX module has been loaded
+	 * first and then try to bring it up.  But TDX needs to use SEAMCALL
+	 * to probe whether the module is loaded (there is no CPUID or MSR
+	 * for that), and making SEAMCALL requires enabling virtualization
+	 * first, just like the rest steps of bringing up TDX module.
+	 *
+	 * So, for simplicity do everything in __tdx_bringup(); the first
+	 * SEAMCALL will return -ENODEV when the module is not loaded.  The
+	 * only complication is having to make sure that initialization
+	 * SEAMCALLs don't return TDX_SEAMCALL_VMFAILINVALID in other
+	 * cases.
+	 */
+	r = __tdx_bringup();
+	if (r) {
+		/*
+		 * Disable TDX only but don't fail to load module if
+		 * the TDX module could not be loaded.  No need to print
+		 * message saying "module is not loaded" because it was
+		 * printed when the first SEAMCALL failed.
+		 */
+		if (r == -ENODEV)
+			goto success_disable_tdx;
+
+		enable_tdx = 0;
+	}
+
+	return r;
+
+success_disable_tdx:
+	enable_tdx = 0;
+	return 0;
+}
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
new file mode 100644
index 000000000000..8aee938a968f
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __KVM_X86_VMX_TDX_H
+#define __KVM_X86_VMX_TDX_H
+
+#ifdef CONFIG_INTEL_TDX_HOST
+int tdx_bringup(void);
+void tdx_cleanup(void);
+#else
+static inline int tdx_bringup(void) { return 0; }
+static inline void tdx_cleanup(void) {}
+#endif
+
+#endif
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3fcee4209120..0a141685872d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2272,6 +2272,7 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
 }
 
 #ifdef CONFIG_KVM_GENERIC_HARDWARE_ENABLING
+extern bool enable_virt_at_load;
 extern bool kvm_rebooting;
 #endif
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8214efa76a6f..265b687fc5d6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -5440,8 +5440,9 @@ static struct miscdevice kvm_dev = {
 };
 
 #ifdef CONFIG_KVM_GENERIC_HARDWARE_ENABLING
-static bool enable_virt_at_load = true;
+bool enable_virt_at_load = true;
 module_param(enable_virt_at_load, bool, 0444);
+EXPORT_SYMBOL_GPL(enable_virt_at_load);
 
 __visible bool kvm_rebooting;
 EXPORT_SYMBOL_GPL(kvm_rebooting);
-- 
2.43.5

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

[Chao Gao]

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

...

>+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();

The self deadlock issue isn't addressed.

>+
>+	return r;
>+}
>+
>+static bool __init kvm_can_support_tdx(void)
>+{
>+	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);

...

>+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");

The lack of X86_FEATURE_TDX_HOST_PLATFORM doesn't necessarily mean no TDX
private KeyIDs. In tdx_init(), X86_FEATURE_TDX_HOST_PLATFORM is not set if
hibernation is enabled.

>+		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;
>+	}
>+
>+	/*
>+	 * Ideally KVM should probe whether TDX module has been loaded
>+	 * first and then try to bring it up.  But TDX needs to use SEAMCALL
>+	 * to probe whether the module is loaded (there is no CPUID or MSR
>+	 * for that), and making SEAMCALL requires enabling virtualization
>+	 * first, just like the rest steps of bringing up TDX module.
>+	 *
>+	 * So, for simplicity do everything in __tdx_bringup(); the first
>+	 * SEAMCALL will return -ENODEV when the module is not loaded.  The
>+	 * only complication is having to make sure that initialization
>+	 * SEAMCALLs don't return TDX_SEAMCALL_VMFAILINVALID in other
>+	 * cases.
>+	 */
>+	r = __tdx_bringup();
>+	if (r) {
>+		/*
>+		 * Disable TDX only but don't fail to load module if
>+		 * the TDX module could not be loaded.  No need to print
>+		 * message saying "module is not loaded" because it was
>+		 * printed when the first SEAMCALL failed.
>+		 */
>+		if (r == -ENODEV)
>+			goto success_disable_tdx;

Given that two error messages are added above, I think it is worth adding one
more here for consistency. And, reloading KVM will not trigger the "module is
not loaded" error which is printed once.

>+
>+		enable_tdx = 0;
>+	}
>+
>+	return r;
>+
>+success_disable_tdx:
>+	enable_tdx = 0;
>+	return 0;
>+}

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

[Huang, Kai]

> >+	r = tdx_enable();
> >+	if (r)
> >+		__do_tdx_cleanup();
> 
> The self deadlock issue isn't addressed.
> 

I planned to send out a new version after merge window, but Paolo beats me.

I have fixed and now testing.  I'll send out the fixup soon.

Thanks Paolo!

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

[Huang, Kai]

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