TDX host kernel support

TDX memory has integrity and confidentiality protections. Violations of this integrity protection are supposed to only affect TDX operations and are never supposed to affect the host kernel itself. In other words, the host kernel should never, itself, see machine checks induced by the TDX integrity hardware.

Alas, the first few generations of TDX hardware have an erratum. A partial write to a TDX private memory cacheline will silently "poison" the line. Subsequent reads will consume the poison and generate a machine check. According to the TDX hardware spec, neither of these things should have happened.

Virtually all kernel memory accesses operations happen in full cachelines. In practice, writing a "byte" of memory usually reads a 64 byte cacheline of memory, modifies it, then writes the whole line back. Those operations do not trigger this problem.

This problem is triggered by "partial" writes where a write transaction of less than cacheline lands at the memory controller. The CPU does these via non-temporal write instructions (like MOVNTI), or through UC/WC memory mappings. The issue can also be triggered away from the CPU by devices doing partial writes via DMA.

With this erratum, there are additional things need to be done. Similar to other CPU bugs, use a CPU bug bit to indicate this erratum, and detect this erratum during early boot. Note this bug reflects the hardware thus it is detected regardless of whether the kernel is built with TDX support or not.

Signed-off-by Kai Huang <[email protected]>

Reviewed-by Kirill A. Shutemov <[email protected]>

---

arch/x86/virt/vmx/tdx/tdx.c +26 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;
+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */
+		return 0;

Quoted 26 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +26 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;
+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */
+		return 0;

arch/x86/virt/vmx/tdx/tdx.c line 60

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	case TDX_SEAMCALL_VMFAILINVALID:
+		pr_err_once("module is not loaded.\n");
+		return -ENODEV;
+	default:
+		pr_err_once("SEAMCALL failed: CPU %d: leaf %llu, error 0x%llx.\n",
+				cpu, fn, sret);
+		if (out)
+			pr_err_once("additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n",
+					out->rcx, out->rdx, out->r8,
+					out->r9, out->r10, out->r11);

This look excessively noisy.

Don't we have SEAMCALL leafs that can fail in normal situation? Like TDX_OPERAND_BUSY error code that indicate that operation likely will succeed on retry.

Or is that wrapper only used for never-fail SEAMCALLs? If so, please document it.

Quoted 3 lines

Or is that wrapper only used for never-fail SEAMCALLs? If so, please document it.

How about adding below?

Use __seamcall() directly in cases that printing error message isn't desired, e.g., when SEAMCALL can legally fail with BUSY and the caller wants to retry.

Quoted 40 lines · On Tue, 2023-06-27 at 12:48 +0300, [email protected] wrote:

Quoted 26 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +26 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;
+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */
+		return 0;

arch/x86/virt/vmx/tdx/tdx.c line 60

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	case TDX_SEAMCALL_VMFAILINVALID:
+		pr_err_once("module is not loaded.\n");
+		return -ENODEV;
+	default:
+		pr_err_once("SEAMCALL failed: CPU %d: leaf %llu, error 0x%llx.\n",
+				cpu, fn, sret);
+		if (out)
+			pr_err_once("additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n",
+					out->rcx, out->rdx, out->r8,
+					out->r9, out->r10, out->r11);

This look excessively noisy.

Quoted 3 lines

Don't we have SEAMCALL leafs that can fail in normal situation? Like TDX_OPERAND_BUSY error code that indicate that operation likely will succeed on retry.

For TDX module initialization TDX_OPERAND_BUSY cannot happen. KVM may have legal cases that BUSY can happen, e.g., KVM's TDP MMU supports handling faults concurrently on different cpus, but that is still under discussion. Also KVM tends to use __seamcall() directly:

https://lore.kernel.org/lkml/3c2c142e14a04a833b47f77faecaa91899b472cd.1678643052.git.isaku.yamahata@intel.com/

I guess KVM doesn't want to print message in all cases as you said, but for module initialization is fine. Those error messages are useful in case something goes wrong, and printing them in seamcall() helps to reduce the code to print in all callers.

arch/x86/virt/vmx/tdx/tdx.c +11 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;

arch/x86/virt/vmx/tdx/tdx.c +1 -0

+		return 0;

Quoted 12 lines

(quoted diff) +7 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64

r9,

(quoted diff) +4 -0

+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;

Review

Quoted 10 lines

(quoted diff) +10 -0

+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;

Hi Kai,

All callers in this series pass NULL for seamcall_ret. I am no sure if you keep it intentionally.

Review

Quoted 5 lines

(quoted diff) +4 -0

+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */

Nit: if you add

#define TDX_SUCCESS 0

and do

	case TDX_SUCCESS:
		return 0;

then the code becomes self-explanatory. i.e., you can drop the comment.

Quoted 25 lines · Chao Gao

Quoted 12 lines

(quoted diff) +7 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64

r9,

(quoted diff) +4 -0

+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;

Review

Quoted 10 lines

(quoted diff) +10 -0

+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;

Hi Kai,

Quoted 2 lines

All callers in this series pass NULL for seamcall_ret. I am no sure if you keep it intentionally.

In this series all the callers doesn't need seamcall_ret.

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */

Nit: if you add

Quoted 8 lines

#define TDX_SUCCESS 0

and do

	case TDX_SUCCESS:
		return 0;

then the code becomes self-explanatory. i.e., you can drop the comment.

If using this, I ended up with below:

arch/x86/include/asm/tdx.h +2 -0

@@ -23,6 +23,8 @@
 #define TDX_SEAMCALL_GP                        (TDX_SW_ERROR | X86_TRAP_GP)
 #define TDX_SEAMCALL_UD                        (TDX_SW_ERROR | X86_TRAP_UD)
 
+#define TDX_SUCCESS           0
+
 
 Hi Kirill/Dave/David,
 

Are you happy with this?

Quoted 26 lines · Kai

Quoted 25 lines · Chao Gao

Quoted 12 lines

(quoted diff) +7 -0

+/*
+ * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
+ * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
+ * leaf function return code and the additional output respectively if
+ * not NULL.
+ */
+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64

r9,

(quoted diff) +4 -0

+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)
+{
+	u64 sret;

Review

Quoted 10 lines

(quoted diff) +10 -0

+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;

Hi Kai,

Quoted 4 lines

Quoted 2 lines

All callers in this series pass NULL for seamcall_ret. I am no sure if you keep it intentionally.

In this series all the callers doesn't need seamcall_ret.

I'm fine keeping it if it is needed by KVM TDX enabling. Otherwise, just drop it.

Quoted 1 line

then the code becomes self-explanatory. i.e., you can drop the comment.

If using this, I ended up with below:

arch/x86/include/asm/tdx.h +2 -0

@@ -23,6 +23,8 @@
  #define TDX_SEAMCALL_GP                        (TDX_SW_ERROR | X86_TRAP_GP)
  #define TDX_SEAMCALL_UD                        (TDX_SW_ERROR | X86_TRAP_UD)
 
+#define TDX_SUCCESS           0
+
 
 Hi Kirill/Dave/David,
 

Are you happy with this?

+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,

arch/x86/virt/vmx/tdx/tdx.c +11 -0

+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */
+		return 0;

arch/x86/virt/vmx/tdx/tdx.c +3 -0

+		return -EIO;
+	}
+}

Can someone explain to me why __tdx_hypercall() is sane (per the above) but then we grew __tdx_module_call() as an absolute abomination and are apparently using that for seam too?

Quoted 3 lines · Peter Zijlstra

Can someone explain to me why __tdx_hypercall() is sane (per the above) but then we grew __tdx_module_call() as an absolute abomination and are apparently using that for seam too?

That is, why do we have two different TDCALL wrappers? Makes no sense.

Quoted 3 lines · Peter Zijlstra

Can someone explain to me why __tdx_hypercall() is sane (per the above) but then we grew __tdx_module_call() as an absolute abomination and are apparently using that for seam too?

That is, why do we have two different TDCALL wrappers? Makes no sense.

Quoted 1 line · Kai Huang

+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,

__always_inline perhaps? __always_unused seems wrong, worse it's still there at the end of the series:

$ quilt diff --combine - | grep seamcall
...

(quoted diff) +7 -0

+static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
...
+       ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+       ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
+       ret = seamcall(TDH_SYS_INFO, sysinfo_pa, TDSYSINFO_STRUCT_SIZE,
+       ret = seamcall(TDH_SYS_CONFIG, __pa(tdmr_pa_array),
+       return seamcall(TDH_SYS_KEY_CONFIG, 0, 0, 0, 0, NULL, NULL);
+               ret = seamcall(TDH_SYS_TDMR_INIT, tdmr->base, 0, 0, 0, NULL,
...

Definitely not unused.

arch/x86/virt/vmx/tdx/tdx.c line 33

Quoted 3 lines

arch/x86/virt/vmx/tdx/tdx.c +2 -0

+				    u64 *seamcall_ret,
+				    struct tdx_module_output *out)

This interface is atrocious :/ Why have these two ret values? Why can't that live in a single space -- /me looks throught the callers, and finds seamcall_ret is unused :-(

Worse, the input (c,d,8,9) is a strict subset of the output (c,d,8,9,10,11) so why isn't that a single thing used for both input and output.

struct tdx_call {
	u64 rcx, rdx, r8, r9, r10, r11;
};

static int __always_inline seamcall(u64 fn, struct tdx_call *regs) {
}

	struct tdx_regs regs = { };
	ret = seamcall(THD_SYS_INIT, &regs);



	struct tdx_regs regs = {
		.rcx = sysinfo_pa,	.rdx = TDXSYSINFO_STRUCT_SIZE,
		.r8  = cmr_array_pa,	.r9  = MAX_CMRS,
	};
	ret = seamcall(THD_SYS_INFO, &regs);
	if (ret)
		return ret;

	print_cmrs(cmr_array, regs.r9);

/me looks more at this stuff and ... WTF!?!?

Can someone explain to me why __tdx_hypercall() is sane (per the above) but then we grew __tdx_module_call() as an absolute abomination and are apparently using that for seam too?

arch/x86/virt/vmx/tdx/tdx.c line 39

Quoted 8 lines

arch/x86/virt/vmx/tdx/tdx.c +6 -0

+{
+	u64 sret;
+	int cpu;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();

And that's important because?

Does having preemption off across the
seamcall make sense? Does it still make sense when you add a loop later?

Quoted 12 lines

arch/x86/virt/vmx/tdx/tdx.c +11 -0

+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	put_cpu();
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	switch (sret) {
+	case 0:
+		/* SEAMCALL was successful */
+		return 0;

arch/x86/virt/vmx/tdx/tdx.c line 60

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	case TDX_SEAMCALL_VMFAILINVALID:
+		pr_err_once("module is not loaded.\n");
+		return -ENODEV;
+	default:
+		pr_err_once("SEAMCALL failed: CPU %d: leaf %llu, error 0x%llx.\n",
+				cpu, fn, sret);
+		if (out)
+			pr_err_once("additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n",
+					out->rcx, out->rdx, out->r8,
+					out->r9, out->r10, out->r11);

At the very least this lacks { }, but it is quite horrendous coding style.

Why switch() at all, would not:

	if (!rset)
		return 0;

	if (sret == TDX_SEAMCALL_VMFAILINVALID) {
		pr_nonsense();
		return -ENODEV;
	}

	if (sret == TDX_SEAMCALL_GP) {
		pr_nonsense();
		return -ENODEV;
	}

	if (sret == TDX_SEAMCALL_UD) {
		pr_nonsense();
		return -EINVAL;
	}

	pr_nonsense();
	return -EIO;

be much clearer and have less horrific indenting issues?

Quoted 5 lines

arch/x86/virt/vmx/tdx/tdx.c line 39

Quoted 2 lines

arch/x86/virt/vmx/tdx/tdx.c +2 -0

+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();

And that's important because?

I want to have a stable cpu for error message printing.

arch/x86/virt/vmx/tdx/tdx.c +5 -0

+	 */
+	do {
+		sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	} while (sret == TDX_RND_NO_ENTROPY && --retry);
+
 	put_cpu();

arch/x86/virt/vmx/tdx/tdx.c static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, · line 47

Quoted 9 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +7 -1

 	cpu = get_cpu();
-	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+
+	/*
+	 * Certain SEAMCALL leaf functions may return error due to
+	 * running out of entropy, in which case the SEAMCALL should
+	 * be retried.  Handle this in SEAMCALL common function.
+	 *
+	 * Mimic rdrand_long() retry behavior.

Yeah, except that doesn't have preemption disabled.. you do.

Quoted 6 lines

arch/x86/virt/vmx/tdx/tdx.c +5 -0

+	 */
+	do {
+		sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	} while (sret == TDX_RND_NO_ENTROPY && --retry);
+
 	put_cpu();

To enable TDX the kernel needs to initialize TDX from two perspectives:
1) Do a set of SEAMCALLs to initialize the TDX module to make it ready to create and run TDX guests; 2) Do the per-cpu initialization SEAMCALL on one logical cpu before the kernel wants to make any other SEAMCALLs on that cpu (including those involved during module initialization and running TDX guests).

The TDX module can be initialized only once in its lifetime. Instead of always initializing it at boot time, this implementation chooses an "on demand" approach to initialize TDX until there is a real need (e.g when requested by KVM). This approach has below pros:

1) It avoids consuming the memory that must be allocated by kernel and given to the TDX module as metadata (~1/256th of the TDX-usable memory), and also saves the CPU cycles of initializing the TDX module (and the metadata) when TDX is not used at all.

2) The TDX module design allows it to be updated while the system is running. The update procedure shares quite a few steps with this "on demand" initialization mechanism. The hope is that much of "on demand" mechanism can be shared with a future "update" mechanism. A boot-time TDX module implementation would not be able to share much code with the update mechanism.

3) Making SEAMCALL requires VMX to be enabled. Currently, only the KVM code mucks with VMX enabling. If the TDX module were to be initialized separately from KVM (like at boot), the boot code would need to be taught how to muck with VMX enabling and KVM would need to be taught how to cope with that. Making KVM itself responsible for TDX initialization lets the rest of the kernel stay blissfully unaware of VMX.

Similar to module initialization, also make the per-cpu initialization "on demand" as it also depends on VMX being enabled.

Add two functions, tdx_enable() and tdx_cpu_enable(), to enable the TDX module and enable TDX on local cpu respectively. For now tdx_enable() is a placeholder. The TODO list will be pared down as functionality is added.

Export both tdx_cpu_enable() and tdx_enable() for KVM use.

In tdx_enable() use a state machine protected by mutex to make sure the initialization will only be done once, as tdx_enable() can be called multiple times (i.e. KVM module can be reloaded) and may be called concurrently by other kernel components in the future.

The per-cpu initialization on each cpu can only be done once during the module's life time. Use a per-cpu variable to track its status to make sure it is only done once in tdx_cpu_enable().

Also, a SEAMCALL to do TDX module global initialization must be done once on any logical cpu before any per-cpu initialization SEAMCALL. Do it inside tdx_cpu_enable() too (if hasn't been done).

tdx_enable() can potentially invoke SEAMCALLs on any online cpus. The per-cpu initialization must be done before those SEAMCALLs are invoked on some cpu. To keep things simple, in tdx_cpu_enable(), always do the per-cpu initialization regardless of whether the TDX module has been initialized or not. And in tdx_enable(), don't call tdx_cpu_enable() but assume the caller has disabled CPU hotplug, done VMXON and tdx_cpu_enable() on all online cpus before calling tdx_enable().

Signed-off-by Kai Huang <[email protected]>

---

v11 -> v12:
- Simplified TDX module global init and lp init status tracking (David).
- Added comment around try_init_module_global() for using raw_spin_lock() (Dave).
- Added one sentence to changelog to explain why to expose tdx_enable() and tdx_cpu_enable() (Dave).
- Simplifed comments around tdx_enable() and tdx_cpu_enable() to use lockdep_assert_*() instead. (Dave)
- Removed redundent "TDX" in error message (Dave).

v10 -> v11:
- Return -NODEV instead of -EINVAL when CONFIG_INTEL_TDX_HOST is off.
- Return the actual error code for tdx_enable() instead of -EINVAL.
- Added Isaku's Reviewed-by.

v9 -> v10:
- Merged the patch to handle per-cpu initialization to this patch to tell the story better.
- Changed how to handle the per-cpu initialization to only provide a tdx_cpu_enable() function to let the user of TDX to do it when the user wants to run TDX code on a certain cpu.
- Changed tdx_enable() to not call cpus_read_lock() explicitly, but call lockdep_assert_cpus_held() to assume the caller has done that.
- Improved comments around tdx_enable() and tdx_cpu_enable().
- Improved changelog to tell the story better accordingly.

v8 -> v9:
- Removed detailed TODO list in the changelog (Dave).
- Added back steps to do module global initialization and per-cpu initialization in the TODO list comment.
- Moved the 'enum tdx_module_status_t' from tdx.c to local tdx.h

v7 -> v8:
- Refined changelog (Dave).
- Removed "all BIOS-enabled cpus" related code (Peter/Thomas/Dave).
- Add a "TODO list" comment in init_tdx_module() to list all steps of initializing the TDX Module to tell the story (Dave).
- Made tdx_enable() unverisally return -EINVAL, and removed nonsense comments (Dave).
- Simplified __tdx_enable() to only handle success or failure.
- TDX_MODULE_SHUTDOWN -> TDX_MODULE_ERROR
- Removed TDX_MODULE_NONE (not loaded) as it is not necessary.
- Improved comments (Dave).
- Pointed out 'tdx_module_status' is software thing (Dave).

v6 -> v7:
- No change.

v5 -> v6:
- Added code to set status to TDX_MODULE_NONE if TDX module is not loaded (Chao)
- Added Chao's Reviewed-by.
- Improved comments around cpus_read_lock().

- v3->v5 (no feedback on v4):
- Removed the check that SEAMRR and TDX KeyID have been detected on all present cpus.
- Removed tdx_detect().
- Added num_online_cpus() to MADT-enabled CPUs check within the CPU hotplug lock and return early with error message.
- Improved dmesg printing for TDX module detection and initialization.

---
 arch/x86/include/asm/tdx.h  |   4 +
 arch/x86/virt/vmx/tdx/tdx.c | 162 ++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h |  13 +++
 3 files changed, 179 insertions(+)

arch/x86/include/asm/tdx.h +4 -0

index 4dfe2e794411..d8226a50c58c 100644
@@ -97,8 +97,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 
 #ifdef CONFIG_INTEL_TDX_HOST
 bool platform_tdx_enabled(void);
+int tdx_cpu_enable(void);
+int tdx_enable(void);
 #else	/* !CONFIG_INTEL_TDX_HOST */
 static inline bool platform_tdx_enabled(void) { return false; }
+static inline int tdx_cpu_enable(void) { return -ENODEV; }
+static inline int tdx_enable(void)  { return -ENODEV; }
 #endif	/* CONFIG_INTEL_TDX_HOST */
 
 #endif /* !__ASSEMBLY__ */

arch/x86/virt/vmx/tdx/tdx.c +4 -0

index 141d12376c4d..29ca18f66d61 100644
@@ -13,6 +13,10 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/smp.h>
+#include <linux/cpu.h>
+#include <linux/spinlock.h>
+#include <linux/percpu-defs.h>
+#include <linux/mutex.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/archrandom.h>

arch/x86/virt/vmx/tdx/tdx.c +12 -0

+
 /*
  * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
  * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL

@@ -74,6 +85,157 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	}
 }
 
+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */
+static int try_init_module_global(void)
+{
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * The TDX module global initialization only needs to be done

arch/x86/virt/vmx/tdx/tdx.c +130 -0

+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (!ret)
+		tdx_global_initialized = true;
+out:
+	raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags);
+
+	return ret;
+}
+
+/**
+ * tdx_cpu_enable - Enable TDX on local cpu
+ *
+ * Do one-time TDX module per-cpu initialization SEAMCALL (and TDX module
+ * global initialization SEAMCALL if not done) on local cpu to make this
+ * cpu be ready to run any other SEAMCALLs.
+ *
+ * Call this function with preemption disabled.
+ *
+ * Return 0 on success, otherwise errors.
+ */
+int tdx_cpu_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;
+
+	lockdep_assert_preemption_disabled();
+
+	/* Already done */
+	if (__this_cpu_read(tdx_lp_initialized))
+		return 0;
+
+	/*
+	 * The TDX module global initialization is the very first step
+	 * to enable TDX.  Need to do it first (if hasn't been done)
+	 * before the per-cpu initialization.
+	 */
+	ret = try_init_module_global();
+	if (ret)
+		return ret;
+
+	/* All '0's are just unused parameters */
+	ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (ret)
+		return ret;
+
+	__this_cpu_write(tdx_lp_initialized, true);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_cpu_enable);
+
+static int init_tdx_module(void)
+{
+	/*
+	 * TODO:
+	 *
+	 *  - Get TDX module information and TDX-capable memory regions.
+	 *  - Build the list of TDX-usable memory regions.
+	 *  - Construct a list of "TD Memory Regions" (TDMRs) to cover
+	 *    all TDX-usable memory regions.
+	 *  - Configure the TDMRs and the global KeyID to the TDX module.
+	 *  - Configure the global KeyID on all packages.
+	 *  - Initialize all TDMRs.
+	 *
+	 *  Return error before all steps are done.
+	 */
+	return -EINVAL;
+}
+
+static int __tdx_enable(void)
+{
+	int ret;
+
+	ret = init_tdx_module();
+	if (ret) {
+		pr_err("module initialization failed (%d)\n", ret);
+		tdx_module_status = TDX_MODULE_ERROR;
+		return ret;
+	}
+
+	pr_info("module initialized.\n");
+	tdx_module_status = TDX_MODULE_INITIALIZED;
+
+	return 0;
+}
+
+/**
+ * tdx_enable - Enable TDX module to make it ready to run TDX guests
+ *
+ * This function assumes the caller has: 1) held read lock of CPU hotplug
+ * lock to prevent any new cpu from becoming online; 2) done both VMXON
+ * and tdx_cpu_enable() on all online cpus.
+ *
+ * This function can be called in parallel by multiple callers.
+ *
+ * Return 0 if TDX is enabled successfully, otherwise error.
+ */
+int tdx_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;
+
+	lockdep_assert_cpus_held();
+
+	mutex_lock(&tdx_module_lock);
+
+	switch (tdx_module_status) {
+	case TDX_MODULE_UNKNOWN:
+		ret = __tdx_enable();
+		break;
+	case TDX_MODULE_INITIALIZED:
+		/* Already initialized, great, tell the caller. */
+		ret = 0;
+		break;
+	default:
+		/* Failed to initialize in the previous attempts */
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&tdx_module_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tdx_enable);
+
 static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
 					    u32 *nr_tdx_keyids)
 {

arch/x86/virt/vmx/tdx/tdx.h +13 -0

index 55dbb1b8c971..9fb46033c852 100644
@@ -16,11 +16,24 @@
  */
 #define TDX_RND_NO_ENTROPY	0x8000020300000000ULL
 
+/*
+ * TDX module SEAMCALL leaf functions
+ */
+#define TDH_SYS_INIT		33
+#define TDH_SYS_LP_INIT		35
+
 /*
  * Do not put any hardware-defined TDX structure representations below
  * this comment!
  */
 
+/* Kernel defined TDX module status during module initialization. */
+enum tdx_module_status_t {
+	TDX_MODULE_UNKNOWN,
+	TDX_MODULE_INITIALIZED,
+	TDX_MODULE_ERROR
+};
+
 struct tdx_module_output;
 u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	       struct tdx_module_output *out);

arch/x86/virt/vmx/tdx/tdx.c static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, · line 108

Quoted 1 line

arch/x86/virt/vmx/tdx/tdx.c +1 -0

+	/* All '0's are just unused parameters. */

I have noticed that you add the above comment whenever you call seamcall() with
0 as parameters. Is this a ask from the maintainer? If not, I think you can skip
it. Just explaining the parameters in seamcall function definition is good enough.

arch/x86/virt/vmx/tdx/tdx.c static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, · line 101

Quoted 5 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +5 -0

+	/*
+	 * The TDX module global initialization only needs to be done
+	 * once on any cpu.
+	 */
+	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);

I don't understand how the comment justifies using raw spin lock.

Quoted 9 lines · On Tue, 2023-06-27 at 12:50 +0300, [email protected] wrote:

arch/x86/virt/vmx/tdx/tdx.c static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, · line 101

Quoted 5 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +5 -0

+	/*
+	 * The TDX module global initialization only needs to be done
+	 * once on any cpu.
+	 */
+	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);

I don't understand how the comment justifies using raw spin lock.

This comment is for using lock in general. The reason to use raw_ version is because this function gets called in IRQ context, and for PREEMPT_RT kernel the normal spinlock is converted to sleeping lock.

Dave suggested to comment on the function rather than comment on the raw_spin_lock directly, e.g., no other kernel code does that:

https://lore.kernel.org/linux-mm/[email protected]/

So I commented the function in this version:

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

Quoted 5 lines

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

If interrupts are always disabled why do you need _irqsave()?

Quoted 6 lines

Quoted 5 lines

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

Quoted 2 lines

If interrupts are always disabled why do you need _irqsave()?

I'll remove the _irqsave().

AFAICT Isaku preferred this for additional security, but this is not necessary.

Quoted 6 lines · Kai

Quoted 6 lines

Quoted 5 lines

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

Quoted 9 lines

Quoted 2 lines

If interrupts are always disabled why do you need _irqsave()?

I'll remove the _irqsave().

AFAICT Isaku preferred this for additional security, but this is not necessary.

Damn. I think we can change the comment to say this function is called with preemption being disabled, but _can_ be called with interrupt disabled. And we keep using the _irqsave() version.

/*
* Do the module global initialization if not done yet. It's always
* called with preemption disabled and can be called with interrupts
* disabled.
*/

This allows a use case that the caller simply wants to call some SEAMCALL on local cpu, e.g., IOMMU code may just use below to get some TDX-IO information:

	preempt_disable();
	vmxon();
	tdx_cpu_enable();
	SEAMCALL;
	vmxoff();
	preempt_enable();

Are you OK with this?

Quoted 6 lines · Kai

Quoted 6 lines

Quoted 5 lines

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

Quoted 9 lines

Quoted 2 lines

If interrupts are always disabled why do you need _irqsave()?

I'll remove the _irqsave().

AFAICT Isaku preferred this for additional security, but this is not necessary.

Damn. I think we can change the comment to say this function is called with preemption being disabled, but _can_ be called with interrupt disabled. And we keep using the _irqsave() version.

/*
* Do the module global initialization if not done yet. It's always
* called with preemption disabled and can be called with interrupts
* disabled.
*/

Quoted 21 lines · Kai

Quoted 6 lines · Kai

Quoted 6 lines

Quoted 5 lines

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

Quoted 9 lines

Quoted 2 lines

If interrupts are always disabled why do you need _irqsave()?

I'll remove the _irqsave().

AFAICT Isaku preferred this for additional security, but this is not necessary.

Damn. I think we can change the comment to say this function is called with preemption being disabled, but _can_ be called with interrupt disabled. And we keep using the _irqsave() version.

Quoted 5 lines

/*
* Do the module global initialization if not done yet. It's always
* called with preemption disabled and can be called with interrupts
* disabled.
*/

That's still not explaining *why*, what you want to say is:

Can be called locally or through an IPI function call.

Quoted 2 lines

Can be called locally or through an IPI function call.

Thanks. As in another reply, if using spinlock is OK, then I think we can say it will be called either locally or through an IPI function call. Otherwise, we do via a new separate function tdx_global_init() and no lock is needed in that function. The caller should call it properly.

Quoted 3 lines · Kai

Quoted 2 lines

Can be called locally or through an IPI function call.

Quoted 4 lines

Thanks. As in another reply, if using spinlock is OK, then I think we can say it will be called either locally or through an IPI function call. Otherwise, we do via a new separate function tdx_global_init() and no lock is needed in that function. The caller should call it properly.

IPI must use raw_spinlock_t. I'm ok with using raw_spinlock_t if there's actual need for that, but the code as presented didn't -- in comments or otherwise -- make it clear why it was as it was.

TDX not specifying time constraints on the various TD/SEAM-CALLs is ofcourse sad, but alas.

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Kai

Quoted 2 lines

Can be called locally or through an IPI function call.

Thanks. As in another reply, if using spinlock is OK, then I think we can say it will be called either locally or through an IPI function call. Otherwise, we do via a new separate function tdx_global_init() and no lock is needed in that function. The caller should call it properly.

Quoted 3 lines

IPI must use raw_spinlock_t. I'm ok with using raw_spinlock_t if there's actual need for that, but the code as presented didn't -- in comments or otherwise -- make it clear why it was as it was.

There's no hard requirement as I replied in another email.

Presumably you prefer the option to have a dedicated tdx_global_init() so we can avoid the raw_spinlock_t?

Quoted 10 lines · Peter Zijlstra

Quoted 7 lines · Kai

Quoted 2 lines

Can be called locally or through an IPI function call.

Thanks. As in another reply, if using spinlock is OK, then I think we can say it will be called either locally or through an IPI function call. Otherwise, we do via a new separate function tdx_global_init() and no lock is needed in that function. The caller should call it properly.

Quoted 3 lines

IPI must use raw_spinlock_t. I'm ok with using raw_spinlock_t if there's actual need for that, but the code as presented didn't -- in comments or otherwise -- make it clear why it was as it was.

There's no hard requirement as I replied in another email.

Presumably you prefer the option to have a dedicated tdx_global_init() so we can avoid the raw_spinlock_t?

Quoted 6 lines

Quoted 5 lines

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */

Quoted 2 lines

If interrupts are always disabled why do you need _irqsave()?

I'll remove the _irqsave().

AFAICT Isaku preferred this for additional security, but this is not necessary.

arch/x86/virt/vmx/tdx/tdx.c +15 -0

+static int try_init_module_global(void)
+{
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * The TDX module global initialization only needs to be done
+	 * once on any cpu.
+	 */
+	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);
+
+	if (tdx_global_initialized) {
+		ret = 0;
+		goto out;
+	}

Quoted 20 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +20 -0

+static int try_init_module_global(void)
+{
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * The TDX module global initialization only needs to be done
+	 * once on any cpu.
+	 */
+	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);
+
+	if (tdx_global_initialized) {
+		ret = 0;
+		goto out;
+	}
+
+	/* All '0's are just unused parameters. */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (!ret)
+		tdx_global_initialized = true;

arch/x86/virt/vmx/tdx/tdx.c static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, · line 116

Quoted 5 lines

arch/x86/virt/vmx/tdx/tdx.c +5 -0

+out:
+	raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags);
+
+	return ret;
+}

How long does that TDX_SYS_INIT take and why is a raw_spinlock with IRQs disabled the right way to serialize this?

The spec says it doesn't have a latency requirement, so theoretically it could be long. SEAMCALL is a VMEXIT so it would at least cost thousands of cycles.

If raw_spinlock isn't desired, I think I can introduce another function to do this and let the caller to call it before calling tdx_cpu_enable(). E.g., we can have below functions:

1) tdx_global_init() -> TDH_SYS_INIT
2) tdx_cpu_init() -> TDH_SYS_LP_INIT
3) tdx_enable() -> actual module initialization

How does this sound?

Quoted 2 lines · Kai

The spec says it doesn't have a latency requirement, so theoretically it could be long. SEAMCALL is a VMEXIT so it would at least cost thousands of cycles.

:-(

Quoted 8 lines

If raw_spinlock isn't desired, I think I can introduce another function to do this and let the caller to call it before calling tdx_cpu_enable(). E.g., we can have below functions:

1) tdx_global_init() -> TDH_SYS_INIT
2) tdx_cpu_init() -> TDH_SYS_LP_INIT
3) tdx_enable() -> actual module initialization

Quoted 1 line

How does this sound?

Ah, wait, I hadn't had enough wake-up juice, it's tdx_global_init() that did the raw_spinlock_t, but that isn't the IPI thing.

Then perhaps just use a mutex to serialize things?

arch/x86/virt/vmx/tdx/tdx.c +57 -0

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */
+static int try_init_module_global(void)
+{
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * The TDX module global initialization only needs to be done
+	 * once on any cpu.
+	 */
+	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);
+
+	if (tdx_global_initialized) {
+		ret = 0;
+		goto out;
+	}
+
+	/* All '0's are just unused parameters. */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (!ret)
+		tdx_global_initialized = true;
+out:
+	raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags);
+
+	return ret;
+}
+
+/**
+ * tdx_cpu_enable - Enable TDX on local cpu
+ *
+ * Do one-time TDX module per-cpu initialization SEAMCALL (and TDX module
+ * global initialization SEAMCALL if not done) on local cpu to make this
+ * cpu be ready to run any other SEAMCALLs.
+ *
+ * Call this function with preemption disabled.
+ *
+ * Return 0 on success, otherwise errors.
+ */
+int tdx_cpu_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;
+
+	lockdep_assert_preemption_disabled();
+
+	/* Already done */
+	if (__this_cpu_read(tdx_lp_initialized))
+		return 0;
+
+	/*
+	 * The TDX module global initialization is the very first step
+	 * to enable TDX.  Need to do it first (if hasn't been done)

arch/x86/virt/vmx/tdx/tdx.c +5 -0

+
+	__this_cpu_write(tdx_lp_initialized, true);
+
+	return 0;
+}

Quoted 57 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +57 -0

+/*
+ * Do the module global initialization if not done yet.
+ * It's always called with interrupts and preemption disabled.
+ */
+static int try_init_module_global(void)
+{
+	unsigned long flags;
+	int ret;
+
+	/*
+	 * The TDX module global initialization only needs to be done
+	 * once on any cpu.
+	 */
+	raw_spin_lock_irqsave(&tdx_global_init_lock, flags);
+
+	if (tdx_global_initialized) {
+		ret = 0;
+		goto out;
+	}
+
+	/* All '0's are just unused parameters. */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (!ret)
+		tdx_global_initialized = true;
+out:
+	raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags);
+
+	return ret;
+}
+
+/**
+ * tdx_cpu_enable - Enable TDX on local cpu
+ *
+ * Do one-time TDX module per-cpu initialization SEAMCALL (and TDX module
+ * global initialization SEAMCALL if not done) on local cpu to make this
+ * cpu be ready to run any other SEAMCALLs.
+ *
+ * Call this function with preemption disabled.
+ *
+ * Return 0 on success, otherwise errors.
+ */
+int tdx_cpu_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;
+
+	lockdep_assert_preemption_disabled();
+
+	/* Already done */
+	if (__this_cpu_read(tdx_lp_initialized))
+		return 0;
+
+	/*
+	 * The TDX module global initialization is the very first step
+	 * to enable TDX.  Need to do it first (if hasn't been done)

arch/x86/virt/vmx/tdx/tdx.c static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, · line 154

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	 * before the per-cpu initialization.
+	 */
+	ret = try_init_module_global();
+	if (ret)
+		return ret;
+
+	/* All '0's are just unused parameters */
+	ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
+	if (ret)
+		return ret;

And here you do *NOT* have IRQs disabled... so an IRQ can come in here and do the above again.

I suspect that's a completely insane thing to have happen, but the way the code is written does not tell me this and might even suggest I should worry about it, per the above thing actually disabling IRQs.

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

Quoted 1 line · Kai Huang

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

I can't find a single caller of this.. why is this exported?

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Kai Huang

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

I can't find a single caller of this.. why is this exported?

It's for KVM TDX patch to use, which isn't in this series.

I'll remove the export. KVM TDX series can export it.

Quoted 8 lines · Kai

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Kai Huang

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

I can't find a single caller of this.. why is this exported?

It's for KVM TDX patch to use, which isn't in this series.

Quoted 1 line

I'll remove the export. KVM TDX series can export it.

Fair enough; where will the KVM TDX series call this? Earlier there was talk about doing it at kvm module load time -- but I objected (and still do object) to that.

What's the current plan?

Quoted 15 lines · Peter Zijlstra

Quoted 9 lines · Kai

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Kai Huang

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

I can't find a single caller of this.. why is this exported?

It's for KVM TDX patch to use, which isn't in this series.

I'll remove the export. KVM TDX series can export it.

Fair enough; where will the KVM TDX series call this? Earlier there was talk about doing it at kvm module load time -- but I objected (and still do object) to that.

Quoted 2 lines

What's the current plan?

The direction is still doing it during module load (not my series anyway). But this can be a separate discussion with KVM maintainers involved.

I understand you have concern that you don't want to have the memory & cpu time wasted on enabling TDX by default. For that we can have a kernel command line to disable TDX once for all (we can even make it default).

Also, KVM will have a module parameter 'enable_tdx'. I am hoping this could reduce your concern too.

Quoted 16 lines · Kai

Quoted 15 lines · Peter Zijlstra

Quoted 9 lines · Kai

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Kai Huang

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

I can't find a single caller of this.. why is this exported?

It's for KVM TDX patch to use, which isn't in this series.

I'll remove the export. KVM TDX series can export it.

Fair enough; where will the KVM TDX series call this? Earlier there was talk about doing it at kvm module load time -- but I objected (and still do object) to that.

Quoted 5 lines

Quoted 2 lines

What's the current plan?

The direction is still doing it during module load (not my series anyway). But this can be a separate discussion with KVM maintainers involved.

They all on Cc afaict.

Quoted 3 lines

I understand you have concern that you don't want to have the memory & cpu time wasted on enabling TDX by default. For that we can have a kernel command line to disable TDX once for all (we can even make it default).

That's insane, I don't want to totally disable it. I want it done at guard creation. Do the whole TDX setup the moment you actually create a TDX guast.

Totally killing TDX is stupid, just about as stupid as doing it on module load (which equates to always doing it).

Quoted 2 lines

Also, KVM will have a module parameter 'enable_tdx'. I am hoping this could reduce your concern too.

I don't get this obsession with doing at module load time :/

Quoted 29 lines · Peter Zijlstra

Quoted 16 lines · Kai

Quoted 15 lines · Peter Zijlstra

Quoted 9 lines · Kai

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Kai Huang

+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

I can't find a single caller of this.. why is this exported?

It's for KVM TDX patch to use, which isn't in this series.

I'll remove the export. KVM TDX series can export it.

Fair enough; where will the KVM TDX series call this? Earlier there was talk about doing it at kvm module load time -- but I objected (and still do object) to that.

Quoted 5 lines

Quoted 2 lines

What's the current plan?

The direction is still doing it during module load (not my series anyway). But this can be a separate discussion with KVM maintainers involved.

They all on Cc afaict.

Quoted 3 lines

I understand you have concern that you don't want to have the memory & cpu time wasted on enabling TDX by default. For that we can have a kernel command line to disable TDX once for all (we can even make it default).

Quoted 5 lines

That's insane, I don't want to totally disable it. I want it done at guard creation. Do the whole TDX setup the moment you actually create a TDX guast.

Totally killing TDX is stupid,

Quoted 7 lines

just about as stupid as doing it on module load (which equates to always doing it).

Quoted 2 lines

Also, KVM will have a module parameter 'enable_tdx'. I am hoping this could reduce your concern too.

I don't get this obsession with doing at module load time :/

The KVM maintainers prefer the initialization on kvm_intel.ko loading time. [1]

I can change enable_tdx parameter for kvm_intel.ko instead of boolean. Something like

enable_tdx
ondemand: on-demand initialization when creating the first TDX guest
onload: initialize TDX module when loading kvm_intel.ko

disable: disable TDX support

Quoted 2 lines · Isaku Yamahata

On Fri, Jun 30, 2023 at 08:30:20PM +0200,
Peter Zijlstra <[email protected]> wrote:

On the flip side, limited hardware availability (unless Intel has changed its tune) and the amount of enabling that's required in BIOS and whatnot makes it highly unlikely that random Linux users are going to unknowingly boot with TDX enabled.

That said, if this is a sticking point, let's just make enable_tdx off by default, i.e. force userspace to opt-in. Deployments that *know* they may want to schedule TDX VMs on the host can simply force the module param. And for everyone else, since KVM is typically configured as a module by distros, KVM can be unloaded and reload if the user realizes they want TDX well after the system is up and running.

That said, if this is a sticking point, let's just make enable_tdx off by default, i.e. force userspace to opt-in. Deployments that *know* they may want to schedule TDX VMs on the host can simply force the module param. And for everyone else, since KVM is typically configured as a module by distros, KVM can be unloaded and reload if the user realizes they want TDX well after the system is up and running.

I honestly don't care all that much either way. There's an escape hatch at runtime (reload kvm_intel.ko) no matter what we do.

I dunno about that, *totally* killing TDX would make my life a lot simpler ;-)

Quoted 1 line

Quoted 1 line

I don't get this obsession with doing at module load time :/

Waiting until userspace attempts to create the first TDX guest adds complexity and limits what KVM can do to harden itself. Currently, all feature support in KVM is effectively frozen at module load. E.g. most of the setup code is contained in __init functions, many module-scoped variables are effectively RO after init (though they can't be marked as such until we smush kvm-intel.ko and kvm-amd.ko into kvm.ko, which is tentatively the long-term plan). All of those patterns would get tossed aside if KVM waits until userspace attempts to create the first guest.

The userspace experience would also be poor, as KVM can't know whether or TDX is actually supported until the TDX module is fully loaded and configured.

There are also latency and noisy neighbor concerns, e.g. we *really* don't want to end up in a situation where creating a TDX guest for a customer can observe arbitrary latency *and* potentially be disruptive to VMs already running on the host.

Userspace can workaround the second and third issues by spawning a dummy TDX guest as early as possible, but that adds complexity to userspace, especially if there's any desire for it to be race free, e.g. with respect to reporting system capabilities to the control plan.

On the flip side, limited hardware availability (unless Intel has changed its tune) and the amount of enabling that's required in BIOS and whatnot makes it highly unlikely that random Linux users are going to unknowingly boot with TDX enabled.

That said, if this is a sticking point, let's just make enable_tdx off by default,

Quoted 4 lines

There are also latency and noisy neighbor concerns, e.g. we *really* don't want to end up in a situation where creating a TDX guest for a customer can observe arbitrary latency *and* potentially be disruptive to VMs already running on the host.

Well, that's a quality of implementation issue with the whole TDX crapola. Sounds like we want to impose latency constraints on the various TDX calls. Allowing it to consume arbitrary amounts of CPU time is unacceptable in any case.

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

There are also latency and noisy neighbor concerns, e.g. we *really* don't want to end up in a situation where creating a TDX guest for a customer can observe arbitrary latency *and* potentially be disruptive to VMs already running on the host.

Well, that's a quality of implementation issue with the whole TDX crapola. Sounds like we want to impose latency constraints on the various TDX calls. Allowing it to consume arbitrary amounts of CPU time is unacceptable in any case.

For what it's worth, everybody knew that calling into the TDX module was going to be a black hole and that consuming large amounts of CPU at random times would drive people bat guano crazy.

The TDX Module ABI spec does have "Leaf Function Latency" warnings for some of the module calls. But, it's basically a binary thing. A call is either normal or "longer than most".

The majority of the "longer than most" cases are for initialization. The _most_ obscene runtime ones are chunked up and can return partial progress to limit latency spikes. But I don't think folks tried as hard on the initialization calls since they're only called once which actually seems pretty reasonable to me.

Maybe we need three classes of "Leaf Function Latency":
1. Sane
2. "Longer than most"
3. Better turn the NMI watchdog off before calling this. :)

Would that help?

Quoted 24 lines · Dave Hansen

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

There are also latency and noisy neighbor concerns, e.g. we *really* don't want to end up in a situation where creating a TDX guest for a customer can observe arbitrary latency *and* potentially be disruptive to VMs already running on the host.

Well, that's a quality of implementation issue with the whole TDX crapola. Sounds like we want to impose latency constraints on the various TDX calls. Allowing it to consume arbitrary amounts of CPU time is unacceptable in any case.

For what it's worth, everybody knew that calling into the TDX module was going to be a black hole and that consuming large amounts of CPU at random times would drive people bat guano crazy.

The TDX Module ABI spec does have "Leaf Function Latency" warnings for some of the module calls. But, it's basically a binary thing. A call is either normal or "longer than most".

The majority of the "longer than most" cases are for initialization. The _most_ obscene runtime ones are chunked up and can return partial progress to limit latency spikes. But I don't think folks tried as hard on the initialization calls since they're only called once which actually seems pretty reasonable to me.

Quoted 6 lines

Maybe we need three classes of "Leaf Function Latency":
1. Sane
2. "Longer than most"
3. Better turn the NMI watchdog off before calling this. :)

Would that help?

I'm thikning we want something along the lines of the Xen preemptible hypercalls, except less crazy. Where the caller does:

	for (;;) {
		ret = tdcall(fn, args);
		if (ret == -EAGAIN) {
			cond_resched();
			continue;
		}
		break;
	}

And then the TDX black box provides a guarantee that any one tdcall (or seamcall or whatever) never takes more than X ns (possibly even configurable) and we get to raise a bug report if we can prove it actually takes longer.

Quoted 24 lines · Dave Hansen

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

There are also latency and noisy neighbor concerns, e.g. we *really* don't want to end up in a situation where creating a TDX guest for a customer can observe arbitrary latency *and* potentially be disruptive to VMs already running on the host.

Well, that's a quality of implementation issue with the whole TDX crapola. Sounds like we want to impose latency constraints on the various TDX calls. Allowing it to consume arbitrary amounts of CPU time is unacceptable in any case.

For what it's worth, everybody knew that calling into the TDX module was going to be a black hole and that consuming large amounts of CPU at random times would drive people bat guano crazy.

The TDX Module ABI spec does have "Leaf Function Latency" warnings for some of the module calls. But, it's basically a binary thing. A call is either normal or "longer than most".

The majority of the "longer than most" cases are for initialization. The _most_ obscene runtime ones are chunked up and can return partial progress to limit latency spikes. But I don't think folks tried as hard on the initialization calls since they're only called once which actually seems pretty reasonable to me.

Quoted 6 lines

Maybe we need three classes of "Leaf Function Latency":
1. Sane
2. "Longer than most"
3. Better turn the NMI watchdog off before calling this. :)

Would that help?

I'm thikning we want something along the lines of the Xen preemptible hypercalls, except less crazy. Where the caller does:

	for (;;) {
		ret = tdcall(fn, args);
		if (ret == -EAGAIN) {
			cond_resched();
			continue;
		}
		break;
	}

And then the TDX black box provides a guarantee that any one tdcall (or seamcall or whatever) never takes more than X ns (possibly even configurable) and we get to raise a bug report if we can prove it actually takes longer.

Quoted 12 lines

I'm thikning we want something along the lines of the Xen preemptible hypercalls, except less crazy. Where the caller does:

	for (;;) {
		ret = tdcall(fn, args);
		if (ret == -EAGAIN) {
			cond_resched();
			continue;
		}
		break;
	}

Quoted 4 lines

And then the TDX black box provides a guarantee that any one tdcall (or seamcall or whatever) never takes more than X ns (possibly even configurable) and we get to raise a bug report if we can prove it actually takes longer.

TDG.VP.VMCALL TDCALL can take arbitrary amount of time as it handles over control to the host/VMM.

But I'm not quite follow how it is different from the host stopping scheduling vCPU on a random instruction. It can happen at any point and TDCALL is not special from this PoV.

Quoted 44 lines · Peter Zijlstra

Quoted 24 lines · Dave Hansen

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

There are also latency and noisy neighbor concerns, e.g. we *really* don't want to end up in a situation where creating a TDX guest for a customer can observe arbitrary latency *and* potentially be disruptive to VMs already running on the host.

Well, that's a quality of implementation issue with the whole TDX crapola. Sounds like we want to impose latency constraints on the various TDX calls. Allowing it to consume arbitrary amounts of CPU time is unacceptable in any case.

For what it's worth, everybody knew that calling into the TDX module was going to be a black hole and that consuming large amounts of CPU at random times would drive people bat guano crazy.

The TDX Module ABI spec does have "Leaf Function Latency" warnings for some of the module calls. But, it's basically a binary thing. A call is either normal or "longer than most".

The majority of the "longer than most" cases are for initialization. The _most_ obscene runtime ones are chunked up and can return partial progress to limit latency spikes. But I don't think folks tried as hard on the initialization calls since they're only called once which actually seems pretty reasonable to me.

Quoted 6 lines

Maybe we need three classes of "Leaf Function Latency":
1. Sane
2. "Longer than most"
3. Better turn the NMI watchdog off before calling this. :)

Would that help?

I'm thikning we want something along the lines of the Xen preemptible hypercalls, except less crazy. Where the caller does:

	for (;;) {
		ret = tdcall(fn, args);
		if (ret == -EAGAIN) {
			cond_resched();
			continue;
		}
		break;
	}

Quoted 4 lines

And then the TDX black box provides a guarantee that any one tdcall (or seamcall or whatever) never takes more than X ns (possibly even configurable) and we get to raise a bug report if we can prove it actually takes longer.

TDG.VP.VMCALL TDCALL can take arbitrary amount of time as it handles over control to the host/VMM.

But I'm not quite follow how it is different from the host stopping scheduling vCPU on a random instruction. It can happen at any point and TDCALL is not special from this PoV.

Waiting until userspace attempts to create the first TDX guest adds complexity and limits what KVM can do to harden itself. Currently, all feature support in KVM is effectively frozen at module load. E.g. most of the setup code is contained in __init functions, many module-scoped variables are effectively RO after init (though they can't be marked as such until we smush kvm-intel.ko and kvm-amd.ko into kvm.ko, which is tentatively the long-term plan). All of those patterns would get tossed aside if KVM waits until userspace attempts to create the first guest.

Quoted 8 lines · Sean Christopherson

Waiting until userspace attempts to create the first TDX guest adds complexity and limits what KVM can do to harden itself. Currently, all feature support in KVM is effectively frozen at module load. E.g. most of the setup code is contained in __init functions, many module-scoped variables are effectively RO after init (though they can't be marked as such until we smush kvm-intel.ko and kvm-amd.ko into kvm.ko, which is tentatively the long-term plan). All of those patterns would get tossed aside if KVM waits until userspace attempts to create the first guest.

....

People got poked and the following was suggested:

On boot do:

TDH.SYS.INIT
TDH.SYS.LP.INIT
TDH.SYS.CONFIG
TDH.SYS.KEY.CONFIG

This should get TDX mostly sorted, but doesn't consume much resources. Then later, when starting the first TDX guest, do the whole

TDH.TDMR.INIT

dance to set up the PAMT array -- which is what gobbles up memory. From what I understand the TDH.TDMR.INIT thing is not one of those excessively long calls.

If we have concerns about allocating the PAMT array, can't we use CMA for this? Allocate the whole thing at boot as CMA such that when not used for TDX it can be used for regular things like userspace and filecache pages?

Those TDH.SYS calls should be enough to ensure TDX is actually working, no?

Quoted 27 lines · Peter Zijlstra

Quoted 8 lines · Sean Christopherson

Waiting until userspace attempts to create the first TDX guest adds complexity and limits what KVM can do to harden itself. Currently, all feature support in KVM is effectively frozen at module load. E.g. most of the setup code is contained in __init functions, many module-scoped variables are effectively RO after init (though they can't be marked as such until we smush kvm-intel.ko and kvm-amd.ko into kvm.ko, which is tentatively the long-term plan). All of those patterns would get tossed aside if KVM waits until userspace attempts to create the first guest.

....

People got poked and the following was suggested:

On boot do:

TDH.SYS.INIT
TDH.SYS.LP.INIT
TDH.SYS.CONFIG
TDH.SYS.KEY.CONFIG

This should get TDX mostly sorted, but doesn't consume much resources. Then later, when starting the first TDX guest, do the whole

TDH.TDMR.INIT

Quoted 3 lines

dance to set up the PAMT array -- which is what gobbles up memory. From what I understand the TDH.TDMR.INIT thing is not one of those excessively long calls.

The TDH.TDMR.INIT itself has it's own latency requirement implemented in the TDX module, thus it only initializes a small chunk (1M I guess) in each call. Therefore we need a loop to do bunch of TDH.TDMR.INIT in order to initialize all PAMT entries for all TDX-usable memory, which can be time-consuming.

Quoted 32 lines · Kai

Quoted 30 lines · Peter Zijlstra

Quoted 8 lines · Sean Christopherson

Waiting until userspace attempts to create the first TDX guest adds complexity and limits what KVM can do to harden itself. Currently, all feature support in KVM is effectively frozen at module load. E.g. most of the setup code is contained in __init functions, many module-scoped variables are effectively RO after init (though they can't be marked as such until we smush kvm-intel.ko and kvm-amd.ko into kvm.ko, which is tentatively the long-term plan). All of those patterns would get tossed aside if KVM waits until userspace attempts to create the first guest.

....

People got poked and the following was suggested:

On boot do:

TDH.SYS.INIT
TDH.SYS.LP.INIT
TDH.SYS.CONFIG
TDH.SYS.KEY.CONFIG

This should get TDX mostly sorted, but doesn't consume much resources. Then later, when starting the first TDX guest, do the whole

TDH.TDMR.INIT

dance to set up the PAMT array -- which is what gobbles up memory. From what I understand the TDH.TDMR.INIT thing is not one of those excessively long calls.

Quoted 4 lines

The TDH.TDMR.INIT itself has it's own latency requirement implemented in the TDX module, thus it only initializes a small chunk (1M I guess) in each call. Therefore we need a loop to do bunch of TDH.TDMR.INIT in order to initialize all PAMT entries for all TDX-usable memory, which can be time-consuming.

Yeah, so you can put a cond_resched() in that loop and all is well, you do not negatively affect other tasks. Because *that* was the concern raised.

If we have concerns about allocating the PAMT array, can't we use CMA for this? Allocate the whole thing at boot as CMA such that when not used for TDX it can be used for regular things like userspace and filecache pages?

Quoted 4 lines · Peter Zijlstra

If we have concerns about allocating the PAMT array, can't we use CMA for this? Allocate the whole thing at boot as CMA such that when not used for TDX it can be used for regular things like userspace and filecache pages?

I never thought of CMA as being super reliable. Maybe it's improved over the years.

KVM also has a rather nasty habit of pinning pages, like for device passthrough. I suspect that means that we'll have one of two scenarios:

1. CMA works great, but the TDX/CMA area is unusable for KVM because it's pinning all its pages and they just get moved out of the CMA area immediately. The CMA area is effectively wasted.
2. CMA sucks, and users get sporadic TDX failures when they wait a long time to run a TDX guest after boot. Users just work around the CMA support by starting up TDX guests at boot or demanding a module parameter be set. Hacking in CMA support was a waste.

Am I just too much of a pessimist?

Quoted 19 lines · Dave Hansen

Quoted 4 lines · Peter Zijlstra

If we have concerns about allocating the PAMT array, can't we use CMA for this? Allocate the whole thing at boot as CMA such that when not used for TDX it can be used for regular things like userspace and filecache pages?

I never thought of CMA as being super reliable. Maybe it's improved over the years.

KVM also has a rather nasty habit of pinning pages, like for device passthrough. I suspect that means that we'll have one of two scenarios:

1. CMA works great, but the TDX/CMA area is unusable for KVM because it's pinning all its pages and they just get moved out of the CMA area immediately. The CMA area is effectively wasted.
2. CMA sucks, and users get sporadic TDX failures when they wait a long time to run a TDX guest after boot. Users just work around the CMA support by starting up TDX guests at boot or demanding a module parameter be set. Hacking in CMA support was a waste.

Quoted 1 line

Am I just too much of a pessimist?

Well, if CMA still sucks, then that needs fixing. If CMA works, but we have a circular fail in that KVM needs to long-term pin the PAMT pages but long-term pin is evicted from CMA (the whole point of long-term pin, after all), then surely we can break that cycle somehow, since in this case the purpose of the CMA is being able to grab that memory chunk when we needs it.

That is, either way around is just a matter of a little code, no?

Quoted 21 lines · Peter Zijlstra

Quoted 19 lines · Dave Hansen

Quoted 4 lines · Peter Zijlstra

If we have concerns about allocating the PAMT array, can't we use CMA for this? Allocate the whole thing at boot as CMA such that when not used for TDX it can be used for regular things like userspace and filecache pages?

I never thought of CMA as being super reliable. Maybe it's improved over the years.

KVM also has a rather nasty habit of pinning pages, like for device passthrough. I suspect that means that we'll have one of two scenarios:

1. CMA works great, but the TDX/CMA area is unusable for KVM because it's pinning all its pages and they just get moved out of the CMA area immediately. The CMA area is effectively wasted.
2. CMA sucks, and users get sporadic TDX failures when they wait a long time to run a TDX guest after boot. Users just work around the CMA support by starting up TDX guests at boot or demanding a module parameter be set. Hacking in CMA support was a waste.

Quoted 1 line

Am I just too much of a pessimist?

Quoted 8 lines

Well, if CMA still sucks, then that needs fixing. If CMA works, but we have a circular fail in that KVM needs to long-term pin the PAMT pages but long-term pin is evicted from CMA (the whole point of long-term pin, after all), then surely we can break that cycle somehow, since in this case the purpose of the CMA is being able to grab that memory chunk when we needs it.

That is, either way around is just a matter of a little code, no?

It's not a circular dependency, it's conflicting requirements.

CMA makes memory more available, but only in the face of unpinned pages.

KVM can pin lots of pages, even outside of TDX-based VMs.

So we either need to change how CMA works fundamentally or stop KVM from pinning pages.

To enable TDX the kernel needs to initialize TDX from two perspectives:
1) Do a set of SEAMCALLs to initialize the TDX module to make it ready to create and run TDX guests; 2) Do the per-cpu initialization SEAMCALL on one logical cpu before the kernel wants to make any other SEAMCALLs on that cpu (including those involved during module initialization and running TDX guests).

The TDX module can be initialized only once in its lifetime. Instead of always initializing it at boot time, this implementation chooses an "on demand" approach to initialize TDX until there is a real need (e.g when requested by KVM). This approach has below pros:

1) It avoids consuming the memory that must be allocated by kernel and given to the TDX module as metadata (~1/256th of the TDX-usable memory), and also saves the CPU cycles of initializing the TDX module (and the metadata) when TDX is not used at all.

2) The TDX module design allows it to be updated while the system is running. The update procedure shares quite a few steps with this "on demand" initialization mechanism. The hope is that much of "on demand" mechanism can be shared with a future "update" mechanism. A boot-time TDX module implementation would not be able to share much code with the update mechanism.

3) Making SEAMCALL requires VMX to be enabled. Currently, only the KVM code mucks with VMX enabling. If the TDX module were to be initialized separately from KVM (like at boot), the boot code would need to be taught how to muck with VMX enabling and KVM would need to be taught how to cope with that. Making KVM itself responsible for TDX initialization lets the rest of the kernel stay blissfully unaware of VMX.

Similar to module initialization, also make the per-cpu initialization "on demand" as it also depends on VMX being enabled.

Add two functions, tdx_enable() and tdx_cpu_enable(), to enable the TDX module and enable TDX on local cpu respectively. For now tdx_enable() is a placeholder. The TODO list will be pared down as functionality is added.

Export both tdx_cpu_enable() and tdx_enable() for KVM use.

In tdx_enable() use a state machine protected by mutex to make sure the initialization will only be done once, as tdx_enable() can be called multiple times (i.e. KVM module can be reloaded) and may be called concurrently by other kernel components in the future.

The per-cpu initialization on each cpu can only be done once during the module's life time. Use a per-cpu variable to track its status to make sure it is only done once in tdx_cpu_enable().

Also, a SEAMCALL to do TDX module global initialization must be done once on any logical cpu before any per-cpu initialization SEAMCALL. Do it inside tdx_cpu_enable() too (if hasn't been done).

tdx_enable() can potentially invoke SEAMCALLs on any online cpus. The per-cpu initialization must be done before those SEAMCALLs are invoked on some cpu. To keep things simple, in tdx_cpu_enable(), always do the per-cpu initialization regardless of whether the TDX module has been initialized or not. And in tdx_enable(), don't call tdx_cpu_enable() but assume the caller has disabled CPU hotplug, done VMXON and tdx_cpu_enable() on all online cpus before calling tdx_enable().

Signed-off-by Kai Huang <[email protected]>

---

v11 -> v12:
- Simplified TDX module global init and lp init status tracking (David).
- Added comment around try_init_module_global() for using

    raw_spin_lock() (Dave).

- Added one sentence to changelog to explain why to expose tdx_enable() and tdx_cpu_enable() (Dave).
- Simplifed comments around tdx_enable() and tdx_cpu_enable() to use lockdep_assert_*() instead. (Dave)
- Removed redundent "TDX" in error message (Dave).

v10 -> v11:
- Return -NODEV instead of -EINVAL when CONFIG_INTEL_TDX_HOST is off.
- Return the actual error code for tdx_enable() instead of -EINVAL.
- Added Isaku's Reviewed-by.

v9 -> v10:
- Merged the patch to handle per-cpu initialization to this patch to tell the story better.
- Changed how to handle the per-cpu initialization to only provide a tdx_cpu_enable() function to let the user of TDX to do it when the user wants to run TDX code on a certain cpu.
- Changed tdx_enable() to not call cpus_read_lock() explicitly, but call lockdep_assert_cpus_held() to assume the caller has done that.
- Improved comments around tdx_enable() and tdx_cpu_enable().
- Improved changelog to tell the story better accordingly.

v8 -> v9:
- Removed detailed TODO list in the changelog (Dave).
- Added back steps to do module global initialization and per-cpu initialization in the TODO list comment.
- Moved the 'enum tdx_module_status_t' from tdx.c to local tdx.h

v7 -> v8:
- Refined changelog (Dave).
- Removed "all BIOS-enabled cpus" related code (Peter/Thomas/Dave).
- Add a "TODO list" comment in init_tdx_module() to list all steps of initializing the TDX Module to tell the story (Dave).
- Made tdx_enable() unverisally return -EINVAL, and removed nonsense comments (Dave).
- Simplified __tdx_enable() to only handle success or failure.
- TDX_MODULE_SHUTDOWN -> TDX_MODULE_ERROR
- Removed TDX_MODULE_NONE (not loaded) as it is not necessary.
- Improved comments (Dave).
- Pointed out 'tdx_module_status' is software thing (Dave).

v6 -> v7:
- No change.

v5 -> v6:
- Added code to set status to TDX_MODULE_NONE if TDX module is not loaded (Chao)
- Added Chao's Reviewed-by.
- Improved comments around cpus_read_lock().

- v3->v5 (no feedback on v4):
- Removed the check that SEAMRR and TDX KeyID have been detected on all present cpus.
- Removed tdx_detect().
- Added num_online_cpus() to MADT-enabled CPUs check within the CPU hotplug lock and return early with error message.
- Improved dmesg printing for TDX module detection and initialization.

---
  arch/x86/include/asm/tdx.h  |   4 +
  arch/x86/virt/vmx/tdx/tdx.c | 162 ++++++++++++++++++++++++++++++++++++
  arch/x86/virt/vmx/tdx/tdx.h |  13 +++
  3 files changed, 179 insertions(+)

arch/x86/include/asm/tdx.h +4 -0

index 4dfe2e794411..d8226a50c58c 100644
@@ -97,8 +97,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 
  #ifdef CONFIG_INTEL_TDX_HOST
  bool platform_tdx_enabled(void);
+int tdx_cpu_enable(void);
+int tdx_enable(void);
  #else	/* !CONFIG_INTEL_TDX_HOST */
  static inline bool platform_tdx_enabled(void) { return false; }
+static inline int tdx_cpu_enable(void) { return -ENODEV; }
+static inline int tdx_enable(void)  { return -ENODEV; }
  #endif	/* CONFIG_INTEL_TDX_HOST */
 
  #endif /* !__ASSEMBLY__ */

arch/x86/virt/vmx/tdx/tdx.c +4 -0

index 141d12376c4d..29ca18f66d61 100644
@@ -13,6 +13,10 @@
  #include <linux/errno.h>
  #include <linux/printk.h>
  #include <linux/smp.h>
+#include <linux/cpu.h>
+#include <linux/spinlock.h>
+#include <linux/percpu-defs.h>
+#include <linux/mutex.h>
  #include <asm/msr-index.h>
  #include <asm/msr.h>
  #include <asm/archrandom.h>

Quoted 2 lines

+static enum tdx_module_status_t tdx_module_status;

Why can't you switch to a simple bool here as well?

It's either initialized or uninitialized. If uninitialized and you get an error, leave it uninitialized. The next caller will try again and fail again.

static int init_tdx_module(void)
{

arch/x86/virt/vmx/tdx/tdx.c +9 -0

+	struct tdsysinfo_struct *sysinfo;
+	struct cmr_info *cmr_array;
+	int ret;
+
+	/*
+	 * Get the TDSYSINFO_STRUCT and CMRs from the TDX module.
+	 *
+	 * The buffers of the TDSYSINFO_STRUCT and the CMR array passed
+	 * to the TDX module must be 1024-bytes and 512-bytes aligned

Quoted 11 lines · Kai Huang

static int init_tdx_module(void)
{

arch/x86/virt/vmx/tdx/tdx.c +9 -0

+	struct tdsysinfo_struct *sysinfo;
+	struct cmr_info *cmr_array;
+	int ret;
+
+	/*
+	 * Get the TDSYSINFO_STRUCT and CMRs from the TDX module.
+	 *
+	 * The buffers of the TDSYSINFO_STRUCT and the CMR array passed
+	 * to the TDX module must be 1024-bytes and 512-bytes aligned

arch/x86/virt/vmx/tdx/tdx.c int tdx_cpu_enable(void) · line 227

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	 * respectively.  Allocate one page to accommodate them both and
+	 * also meet those alignment requirements.
+	 */
+	sysinfo = (struct tdsysinfo_struct *)__get_free_page(GFP_KERNEL);
+	if (!sysinfo)
+		return -ENOMEM;
+	cmr_array = (struct cmr_info *)((unsigned long)sysinfo + PAGE_SIZE / 2);
+
+	BUILD_BUG_ON(PAGE_SIZE / 2 < TDSYSINFO_STRUCT_SIZE);
+	BUILD_BUG_ON(PAGE_SIZE / 2 < sizeof(struct cmr_info) * MAX_CMRS);

This works, but why not just use slab for this? kmalloc has 512 and 1024 pools already and you won't waste memory for rounding up.

Something like this:

        sysinfo = kmalloc(TDSYSINFO_STRUCT_SIZE, GFP_KERNEL);
        if (!sysinfo)
                return -ENOMEM;

        cmr_array_size = sizeof(struct cmr_info) * MAX_CMRS;

        /* CMR array has to be 512-aligned */
        cmr_array_size = round_up(cmr_array_size, 512);

        cmr_array = kmalloc(cmr_array_size, GFP_KERNEL);
        if (!cmr_array) {
                kfree(sysinfo);
                return -ENOMEM;
        }

?

Quoted 28 lines · On Tue, 2023-06-27 at 12:51 +0300, [email protected] wrote:

Quoted 11 lines · Kai Huang

static int init_tdx_module(void)
{

arch/x86/virt/vmx/tdx/tdx.c +9 -0

+	struct tdsysinfo_struct *sysinfo;
+	struct cmr_info *cmr_array;
+	int ret;
+
+	/*
+	 * Get the TDSYSINFO_STRUCT and CMRs from the TDX module.
+	 *
+	 * The buffers of the TDSYSINFO_STRUCT and the CMR array passed
+	 * to the TDX module must be 1024-bytes and 512-bytes aligned

arch/x86/virt/vmx/tdx/tdx.c int tdx_cpu_enable(void) · line 227

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	 * respectively.  Allocate one page to accommodate them both and
+	 * also meet those alignment requirements.
+	 */
+	sysinfo = (struct tdsysinfo_struct *)__get_free_page(GFP_KERNEL);
+	if (!sysinfo)
+		return -ENOMEM;
+	cmr_array = (struct cmr_info *)((unsigned long)sysinfo + PAGE_SIZE / 2);
+
+	BUILD_BUG_ON(PAGE_SIZE / 2 < TDSYSINFO_STRUCT_SIZE);
+	BUILD_BUG_ON(PAGE_SIZE / 2 < sizeof(struct cmr_info) * MAX_CMRS);

This works, but why not just use slab for this? kmalloc has 512 and 1024 pools already and you won't waste memory for rounding up.

Something like this:

Quoted 8 lines

        sysinfo = kmalloc(TDSYSINFO_STRUCT_SIZE, GFP_KERNEL);
        if (!sysinfo)
                return -ENOMEM;

        cmr_array_size = sizeof(struct cmr_info) * MAX_CMRS;

        /* CMR array has to be 512-aligned */
        cmr_array_size = round_up(cmr_array_size, 512);

Should we define a macro for 512

+#define CMR_INFO_ARRAY_ALIGNMENT 512

And get rid of this comment? AFAICT Dave didn't like such comment mentioning 512-bytes aligned if we have a macro for that.

Quoted 6 lines

        cmr_array = kmalloc(cmr_array_size, GFP_KERNEL);
        if (!cmr_array) {
                kfree(sysinfo);
                return -ENOMEM;
        }

Quoted 2 lines

?

I confess the reason I used __get_free_page() was to avoid having to allocate twice, and in case of failure, I need to handle additional memory free. But I can do if you think it's clearer?

I wouldn't worry about wasting memory. The buffer is freed anyway for now. Long-termly it's just 4K.

Quoted 38 lines · Kai

Quoted 28 lines · On Tue, 2023-06-27 at 12:51 +0300, [email protected] wrote:

Quoted 11 lines · Kai Huang

static int init_tdx_module(void)
{

arch/x86/virt/vmx/tdx/tdx.c +9 -0

+	struct tdsysinfo_struct *sysinfo;
+	struct cmr_info *cmr_array;
+	int ret;
+
+	/*
+	 * Get the TDSYSINFO_STRUCT and CMRs from the TDX module.
+	 *
+	 * The buffers of the TDSYSINFO_STRUCT and the CMR array passed
+	 * to the TDX module must be 1024-bytes and 512-bytes aligned

arch/x86/virt/vmx/tdx/tdx.c int tdx_cpu_enable(void) · line 227

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+	 * respectively.  Allocate one page to accommodate them both and
+	 * also meet those alignment requirements.
+	 */
+	sysinfo = (struct tdsysinfo_struct *)__get_free_page(GFP_KERNEL);
+	if (!sysinfo)
+		return -ENOMEM;
+	cmr_array = (struct cmr_info *)((unsigned long)sysinfo + PAGE_SIZE / 2);
+
+	BUILD_BUG_ON(PAGE_SIZE / 2 < TDSYSINFO_STRUCT_SIZE);
+	BUILD_BUG_ON(PAGE_SIZE / 2 < sizeof(struct cmr_info) * MAX_CMRS);

This works, but why not just use slab for this? kmalloc has 512 and 1024 pools already and you won't waste memory for rounding up.

Something like this:

Quoted 8 lines

        sysinfo = kmalloc(TDSYSINFO_STRUCT_SIZE, GFP_KERNEL);
        if (!sysinfo)
                return -ENOMEM;

        cmr_array_size = sizeof(struct cmr_info) * MAX_CMRS;

        /* CMR array has to be 512-aligned */
        cmr_array_size = round_up(cmr_array_size, 512);

Quoted 6 lines

Should we define a macro for 512

+#define CMR_INFO_ARRAY_ALIGNMENT 512

And get rid of this comment? AFAICT Dave didn't like such comment mentioning 512-bytes aligned if we have a macro for that.

Good idea.

Quoted 6 lines

Quoted 6 lines

        cmr_array = kmalloc(cmr_array_size, GFP_KERNEL);
        if (!cmr_array) {
                kfree(sysinfo);
                return -ENOMEM;
        }

Quoted 6 lines

Quoted 2 lines

?

I confess the reason I used __get_free_page() was to avoid having to allocate twice, and in case of failure, I need to handle additional memory free. But I can do if you think it's clearer?

Less trickery is always cleaner. Especially if the trick is not justified.

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
+			   struct cmr_info *cmr_array)
+{
+	struct tdx_module_output out;
+	u64 sysinfo_pa, cmr_array_pa;
+	int ret;
+
+	sysinfo_pa = __pa(sysinfo);
+	cmr_array_pa = __pa(cmr_array);
+	ret = seamcall(TDH_SYS_INFO, sysinfo_pa, TDSYSINFO_STRUCT_SIZE,

arch/x86/virt/vmx/tdx/tdx.c +4 -0

+	print_cmrs(cmr_array, out.r9);
+
+	return 0;
+}

Quoted 10 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
+			   struct cmr_info *cmr_array)
+{
+	struct tdx_module_output out;
+	u64 sysinfo_pa, cmr_array_pa;
+	int ret;
+
+	sysinfo_pa = __pa(sysinfo);
+	cmr_array_pa = __pa(cmr_array);
+	ret = seamcall(TDH_SYS_INFO, sysinfo_pa, TDSYSINFO_STRUCT_SIZE,

arch/x86/virt/vmx/tdx/tdx.c int tdx_cpu_enable(void) · line 201

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.c +10 -0

+			cmr_array_pa, MAX_CMRS, NULL, &out);
+	if (ret)
+		return ret;
+
+	pr_info("TDX module: attributes 0x%x, vendor_id 0x%x, major_version %u, minor_version %u, build_date %u, build_num %u",
+		sysinfo->attributes,	sysinfo->vendor_id,
+		sysinfo->major_version, sysinfo->minor_version,
+		sysinfo->build_date,	sysinfo->build_num);
+
+	/* R9 contains the actual entries written to the CMR array. */

So I'm vexed by this comment; it's either not enough or too much.

I mean, as given you assume we all know about the magic parameters to TDH_SYS_INFO but then somehow need an explanation for how %r9 is changed from the array size to the number of used entries.

Either describe the whole thing or none of it.

Me, I would prefer all of it, because I've no idea where to begin looking for any of this,

SDM doesn't seem to be the place. That doesn't
even list TDCALL/SEAMCALL in Volume 2 :-( Let alone describe the magic values.

Sure. How about below?

(quoted diff) +11 -0

+       /*
+        * TDH.SYS.INFO writes the TDSYSINFO_STRUCT and the CMR array
+        * to the buffers provided by the kernel (via RCX and R8
+        * respectively).  The buffer size of the TDSYSINFO_STRUCT
+        * (via RDX) and the maximum entries of the CMR array (via R9)
+        * passed to this SEAMCALL must be at least the size of
+        * TDSYSINFO_STRUCT and MAX_CMRS respectively.
+        *
+        * Upon a successful return, R9 contains the actual entries
+        * written to the CMR array.
+        */
        sysinfo_pa = __pa(sysinfo);
        cmr_array_pa = __pa(cmr_array);
        ret = seamcall(TDH_SYS_INFO, sysinfo_pa, TDSYSINFO_STRUCT_SIZE,

Or should I just repeat the spec like below?

(quoted diff) +10 -0

+       /*
+        * TDH.SYS.INFO writes the TDSYSINFO_STRUCT and the CMR array
+        * to the buffers provided by the kernel:
+        *
+        * Input:
+        *  - RCX: The buffer of TDSYSINFO_STRUCT
+        *  - RDX: The size of the TDSYSINFO_STRUCT buffer, must be at
+        *         at least the size of TDSYSINFO_STRUCT
+        *  - R8: The buffer of the CMR array
+        *  - R9: The entry number of the array, must be at least