TDX host kernel support

+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Those values are part of the TDX module ABI. Do not change them.

Quoted 4 lines · Kai Huang

+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Those values are part of the TDX module ABI. Do not change them.

It would be better if you include specification version and section title.

Quoted 7 lines · Sathyanarayanan Kuppuswamy

Quoted 4 lines · Kai Huang

+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Those values are part of the TDX module ABI. Do not change them.

Quoted 3 lines

It would be better if you include specification version and section title.

Such as below?

"Those values are part of the TDX module ABI (section "Physical Page Size", TDX module 1.0 spec). Do not change them."

Btw, Dave mentioned we should not put the "section numbers" to the comment:

https://lore.kernel.org/lkml/[email protected]/

I was trying to follow.

Quoted 4 lines · Kai Huang

+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Those values are part of the TDX module ABI. Do not change them.

It would be better if you include specification version and section title.

arch/x86/include/asm/tdx.h line 30

Quoted 8 lines · Kai Huang

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

+/*
+ * TDX supported page sizes (4K/2M/1G).
+ *
+ * Those values are part of the TDX module ABI.  Do not change them.
+ */
+#define TDX_PS_4K	0
+#define TDX_PS_2M	1
+#define TDX_PS_1G	2

That comment can just be:

/* TDX supported page sizes from the TDX module ABI. */

I think folks understand that the kernel can't willy nilly change ABI values.

Intel Trust Domain Extensions (TDX) protects guest VMs from malicious host and certain physical attacks. A CPU-attested software module called 'the TDX module' runs inside a new isolated memory range as a trusted hypervisor to manage and run protected VMs.

Pre-TDX Intel hardware has support for a memory encryption architecture called MKTME. The memory encryption hardware underpinning MKTME is also used for Intel TDX. TDX ends up "stealing" some of the physical address space from the MKTME architecture for crypto-protection to VMs. The BIOS is responsible for partitioning the "KeyID" space between legacy MKTME and TDX. The KeyIDs reserved for TDX are called 'TDX private KeyIDs' or 'TDX KeyIDs' for short.

TDX doesn't trust the BIOS. During machine boot, TDX verifies the TDX private KeyIDs are consistently and correctly programmed by the BIOS across all CPU packages before it enables TDX on any CPU core. A valid TDX private KeyID range on BSP indicates TDX has been enabled by the BIOS, otherwise the BIOS is buggy.

The TDX module is expected to be loaded by the BIOS when it enables TDX, but the kernel needs to properly initialize it before it can be used to create and run any TDX guests. The TDX module will be initialized at runtime by the user (i.e. KVM) on demand.

Add a new early_initcall(tdx_init) to do TDX early boot initialization. Only detect TDX private KeyIDs for now. Some other early checks will follow up. Also add a new function to report whether TDX has been enabled by BIOS (TDX private KeyID range is valid). Kexec() will also need it to determine whether need to flush dirty cachelines that are associated with any TDX private KeyIDs before booting to the new kernel.

To start to support TDX, create a new arch/x86/virt/vmx/tdx/tdx.c for TDX host kernel support. Add a new Kconfig option CONFIG_INTEL_TDX_HOST to opt-in TDX host kernel support (to distinguish with TDX guest kernel support). So far only KVM is the only user of TDX. Make the new config option depend on KVM_INTEL.

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

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

---

v6 -> v7:
- No change.

v5 -> v6:
- Removed SEAMRR detection to make code simpler.
- Removed the 'default N' in the KVM_TDX_HOST Kconfig (Kirill).
- Changed to use 'obj-y' in arch/x86/virt/vmx/tdx/Makefile (Kirill).

---
 arch/x86/Kconfig               | 12 +++++
 arch/x86/Makefile              |  2 +
 arch/x86/include/asm/tdx.h     |  7 +++
 arch/x86/virt/Makefile         |  2 +
 arch/x86/virt/vmx/Makefile     |  2 +
 arch/x86/virt/vmx/tdx/Makefile |  2 +
 arch/x86/virt/vmx/tdx/tdx.c    | 95 ++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h    | 15 ++++++
 8 files changed, 137 insertions(+)
 create mode 100644 arch/x86/virt/Makefile
 create mode 100644 arch/x86/virt/vmx/Makefile
 create mode 100644 arch/x86/virt/vmx/tdx/Makefile
 create mode 100644 arch/x86/virt/vmx/tdx/tdx.c
 create mode 100644 arch/x86/virt/vmx/tdx/tdx.h

arch/x86/Kconfig +12 -0

index 67745ceab0db..cced4ef3bfb2 100644
@@ -1953,6 +1953,18 @@ config X86_SGX
 
 	  If unsure, say N.
 
+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
+	help
+	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
+	  host and certain physical attacks.  This option enables necessary TDX
+	  support in host kernel to run protected VMs.
+
+	  If unsure, say N.
+
 config EFI
 	bool "EFI runtime service support"
 	depends on ACPI

arch/x86/Makefile +2 -0

index 415a5d138de4..38d3e8addc5f 100644
@@ -246,6 +246,8 @@ archheaders:
 
 libs-y  += arch/x86/lib/
 
+core-y += arch/x86/virt/
+
 # drivers-y are linked after core-y
 drivers-$(CONFIG_MATH_EMULATION) += arch/x86/math-emu/
 drivers-$(CONFIG_PCI)            += arch/x86/pci/

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

index e9a3f4a6fba1..51c4222a13ae 100644
@@ -98,5 +98,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 	return -ENODEV;
 }
 #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
+
+#ifdef CONFIG_INTEL_TDX_HOST
+bool platform_tdx_enabled(void);
+#else	/* !CONFIG_INTEL_TDX_HOST */
+static inline bool platform_tdx_enabled(void) { return false; }
+#endif	/* CONFIG_INTEL_TDX_HOST */
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASM_X86_TDX_H */

arch/x86/virt/Makefile new file +2 -0

new file mode 100644
index 000000000000..1e36502cd738
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-y	+= vmx/

arch/x86/virt/vmx/Makefile new file +2 -0

new file mode 100644
index 000000000000..feebda21d793
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_INTEL_TDX_HOST)	+= tdx/

arch/x86/virt/vmx/tdx/Makefile new file +2 -0

new file mode 100644
index 000000000000..93ca8b73e1f1
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-y += tdx.o

arch/x86/virt/vmx/tdx/tdx.c new file +45 -0

new file mode 100644
index 000000000000..982d9c453b6b
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2022 Intel Corporation.
+ *
+ * Intel Trusted Domain Extensions (TDX) support
+ */
+
+#define pr_fmt(fmt)	"tdx: " fmt
+
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/printk.h>
+#include <asm/msr-index.h>
+#include <asm/msr.h>
+#include <asm/tdx.h>
+#include "tdx.h"
+
+static u32 tdx_keyid_start __ro_after_init;
+static u32 tdx_keyid_num __ro_after_init;
+
+/*
+ * Detect TDX private KeyIDs to see whether TDX has been enabled by the
+ * BIOS.  Both initializing the TDX module and running TDX guest require
+ * TDX private KeyID.
+ *
+ * TDX doesn't trust BIOS.  TDX verifies all configurations from BIOS
+ * are correct before enabling TDX on any core.  TDX requires the BIOS
+ * to correctly and consistently program TDX private KeyIDs on all CPU
+ * packages.  Unless there is a BIOS bug, detecting a valid TDX private
+ * KeyID range on BSP indicates TDX has been enabled by the BIOS.  If
+ * there's such BIOS bug, it will be caught later when initializing the
+ * TDX module.
+ */
+static int __init detect_tdx(void)
+{
+	int ret;
+
+	/*
+	 * IA32_MKTME_KEYID_PARTIONING:
+	 *   Bit [31:0]:	Number of MKTME KeyIDs.
+	 *   Bit [63:32]:	Number of TDX private KeyIDs.
+	 */
+	ret = rdmsr_safe(MSR_IA32_MKTME_KEYID_PARTITIONING, &tdx_keyid_start,
+			&tdx_keyid_num);
+	if (ret)

arch/x86/virt/vmx/tdx/tdx.c new file +40 -0

+
+	pr_info("TDX enabled by BIOS. TDX private KeyID range: [%u, %u)\n",
+			tdx_keyid_start, tdx_keyid_start + tdx_keyid_num);
+
+	return 0;
+}
+
+static void __init clear_tdx(void)
+{
+	tdx_keyid_start = tdx_keyid_num = 0;
+}
+
+static int __init tdx_init(void)
+{
+	if (detect_tdx())
+		return -ENODEV;
+
+	/*
+	 * Initializing the TDX module requires one TDX private KeyID.
+	 * If there's only one TDX KeyID then after module initialization
+	 * KVM won't be able to run any TDX guest, which makes the whole
+	 * thing worthless.  Just disable TDX in this case.
+	 */
+	if (tdx_keyid_num < 2) {
+		pr_info("Disable TDX as there's only one TDX private KeyID available.\n");
+		goto no_tdx;
+	}
+
+	return 0;
+no_tdx:
+	clear_tdx();
+	return -ENODEV;
+}
+early_initcall(tdx_init);
+
+/* Return whether the BIOS has enabled TDX */
+bool platform_tdx_enabled(void)
+{
+	return !!tdx_keyid_num;
+}

arch/x86/virt/vmx/tdx/tdx.h new file +15 -0

new file mode 100644
index 000000000000..d00074abcb20
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H
+
+/*
+ * This file contains both macros and data structures defined by the TDX
+ * architecture and Linux defined software data structures and functions.
+ * The two should not be mixed together for better readability.  The
+ * architectural definitions come first.
+ */
+
+/* MSR to report KeyID partitioning between MKTME and TDX */
+#define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
+
+#endif

Quoted 33 lines

arch/x86/virt/vmx/tdx/tdx.c new file +32 -0

+static u32 tdx_keyid_start __ro_after_init;
+static u32 tdx_keyid_num __ro_after_init;
+
+/*
+ * Detect TDX private KeyIDs to see whether TDX has been enabled by the
+ * BIOS.  Both initializing the TDX module and running TDX guest require
+ * TDX private KeyID.
+ *
+ * TDX doesn't trust BIOS.  TDX verifies all configurations from BIOS
+ * are correct before enabling TDX on any core.  TDX requires the BIOS
+ * to correctly and consistently program TDX private KeyIDs on all CPU
+ * packages.  Unless there is a BIOS bug, detecting a valid TDX private
+ * KeyID range on BSP indicates TDX has been enabled by the BIOS.  If
+ * there's such BIOS bug, it will be caught later when initializing the
+ * TDX module.
+ */
+static int __init detect_tdx(void)
+{
+	int ret;
+
+	/*
+	 * IA32_MKTME_KEYID_PARTIONING:
+	 *   Bit [31:0]:	Number of MKTME KeyIDs.
+	 *   Bit [63:32]:	Number of TDX private KeyIDs.
+	 */
+	ret = rdmsr_safe(MSR_IA32_MKTME_KEYID_PARTITIONING, &tdx_keyid_start,
+			&tdx_keyid_num);
+	if (ret)
+		return -ENODEV;
+
+	if (!tdx_keyid_num)
+		return -ENODEV;

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

Quoted 6 lines

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

+
+	/*
+	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
+	 * KeyIDs start after the last MKTME KeyID.
+	 */
+	tdx_keyid_start++;

It is not very clear why you increment tdx_keyid_start.

What we read from
MSR_IA32_MKTME_KEYID_PARTITIONING is not the correct start keyid?

Also why is this global variable? At least in this patch, there seems to be no use case.

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

Quoted 12 lines

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

+	/*
+	 * Initializing the TDX module requires one TDX private KeyID.
+	 * If there's only one TDX KeyID then after module initialization
+	 * KVM won't be able to run any TDX guest, which makes the whole
+	 * thing worthless.  Just disable TDX in this case.
+	 */
+	if (tdx_keyid_num < 2) {
+		pr_info("Disable TDX as there's only one TDX private KeyID available.\n");
+		goto no_tdx;
+	}
+

Quoted 2 lines

Also why is this global variable? At least in this patch, there seems to be no use case.

Platform_tdx_enabled() uses tdx_keyid_num to determine whether TDX is enabled by BIOS.

Also, in the changlog I can add "both initializing the TDX module and creating TDX guest will need to use TDX private KeyID".

But I also have a comment saying something similar around ...

Intel Trust Domain Extensions (TDX) protects guest VMs from malicious host and certain physical attacks. A CPU-attested software module called 'the TDX module' runs inside a new isolated memory range as a trusted hypervisor to manage and run protected VMs.

Pre-TDX Intel hardware has support for a memory encryption architecture called MKTME. The memory encryption hardware underpinning MKTME is also used for Intel TDX. TDX ends up "stealing" some of the physical address space from the MKTME architecture for crypto-protection to VMs. The BIOS is responsible for partitioning the "KeyID" space between legacy MKTME and TDX. The KeyIDs reserved for TDX are called 'TDX private KeyIDs' or 'TDX KeyIDs' for short.

TDX doesn't trust the BIOS. During machine boot, TDX verifies the TDX private KeyIDs are consistently and correctly programmed by the BIOS across all CPU packages before it enables TDX on any CPU core. A valid TDX private KeyID range on BSP indicates TDX has been enabled by the BIOS, otherwise the BIOS is buggy.

The TDX module is expected to be loaded by the BIOS when it enables TDX, but the kernel needs to properly initialize it before it can be used to create and run any TDX guests. The TDX module will be initialized at runtime by the user (i.e. KVM) on demand.

Add a new early_initcall(tdx_init) to do TDX early boot initialization. Only detect TDX private KeyIDs for now. Some other early checks will follow up.

Also add a new function to report whether TDX has been
enabled by BIOS (TDX private KeyID range is valid). Kexec() will also need it to determine whether need to flush dirty cachelines that are associated with any TDX private KeyIDs before booting to the new kernel.

To start to support TDX, create a new arch/x86/virt/vmx/tdx/tdx.c for TDX host kernel support. Add a new Kconfig option CONFIG_INTEL_TDX_HOST to opt-in TDX host kernel support (to distinguish with TDX guest kernel support). So far only KVM is the only user of TDX. Make the new config option depend on KVM_INTEL.

arch/x86/Kconfig +12 -0

+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
+	help
+	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
+	  host and certain physical attacks.  This option enables necessary TDX
+	  support in host kernel to run protected VMs.
+
+	  If unsure, say N.
+
 config EFI
 	bool "EFI runtime service support"
 	depends on ACPI

arch/x86/Makefile +2 -0

index 415a5d138de4..38d3e8addc5f 100644
@@ -246,6 +246,8 @@ archheaders:
 
 libs-y  += arch/x86/lib/
 
+core-y += arch/x86/virt/
+
 # drivers-y are linked after core-y
 drivers-$(CONFIG_MATH_EMULATION) += arch/x86/math-emu/
 drivers-$(CONFIG_PCI)            += arch/x86/pci/

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

index e9a3f4a6fba1..51c4222a13ae 100644
@@ -98,5 +98,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 	return -ENODEV;
 }
 #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
+
+#ifdef CONFIG_INTEL_TDX_HOST
+bool platform_tdx_enabled(void);
+#else	/* !CONFIG_INTEL_TDX_HOST */
+static inline bool platform_tdx_enabled(void) { return false; }
+#endif	/* CONFIG_INTEL_TDX_HOST */
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASM_X86_TDX_H */

arch/x86/virt/Makefile new file +2 -0

new file mode 100644
index 000000000000..1e36502cd738
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-y	+= vmx/

arch/x86/virt/vmx/Makefile new file +2 -0

new file mode 100644
index 000000000000..feebda21d793
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_INTEL_TDX_HOST)	+= tdx/

arch/x86/virt/vmx/tdx/Makefile new file +2 -0

new file mode 100644
index 000000000000..93ca8b73e1f1
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-y += tdx.o

arch/x86/virt/vmx/tdx/tdx.c new file +14 -0

new file mode 100644
index 000000000000..982d9c453b6b
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2022 Intel Corporation.
+ *
+ * Intel Trusted Domain Extensions (TDX) support
+ */
+
+#define pr_fmt(fmt)	"tdx: " fmt
+
+#include <linux/types.h>
+#include <linux/init.h>
+#include <linux/printk.h>
+#include <asm/msr-index.h>
+#include <asm/msr.h>

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

+	clear_tdx();
+	return -ENODEV;
+}
+early_initcall(tdx_init);
+
+/* Return whether the BIOS has enabled TDX */
+bool platform_tdx_enabled(void)
+{
+	return !!tdx_keyid_num;
+}

arch/x86/virt/vmx/tdx/tdx.h new file +15 -0

new file mode 100644
index 000000000000..d00074abcb20
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H
+
+/*
+ * This file contains both macros and data structures defined by the TDX
+ * architecture and Linux defined software data structures and functions.
+ * The two should not be mixed together for better readability.  The
+ * architectural definitions come first.
+ */
+
+/* MSR to report KeyID partitioning between MKTME and TDX */
+#define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
+
+#endif

Quoted 5 lines

The TDX module is expected to be loaded by the BIOS when it enables TDX, but the kernel needs to properly initialize it before it can be used to create and run any TDX guests. The TDX module will be initialized at runtime by the user (i.e. KVM) on demand.

Calling KVM "the user" is a stretch. How about we give actual user facts instead of filling this with i.e.'s when there's only one actual way it happens?

The TDX module will be initialized by the KVM subsystem when <insert actual trigger description here>.

Quoted 3 lines

Add a new early_initcall(tdx_init) to do TDX early boot initialization. Only detect TDX private KeyIDs for now. Some other early checks will follow up.

Just say what this patch is doing. Don't try to

Quoted 4 lines

Also add a new function to report whether TDX has been
enabled by BIOS (TDX private KeyID range is valid). Kexec() will also need it to determine whether need to flush dirty cachelines that are associated with any TDX private KeyIDs before booting to the new kernel.

That last sentence doesn't parse correctly.

Quoted 4 lines

+/*
+ * Detect TDX private KeyIDs to see whether TDX has been enabled by the
+ * BIOS. Both initializing the TDX module and running TDX guest require
+ * TDX private KeyID.

This comment is not right, sorry.

Talk about the function at a *HIGH* level. Don't talk about every little detailed facet of the function. That's what the code is there for.

Quoted 8 lines

+ * TDX doesn't trust BIOS. TDX verifies all configurations from BIOS
+ * are correct before enabling TDX on any core. TDX requires the BIOS
+ * to correctly and consistently program TDX private KeyIDs on all CPU
+ * packages. Unless there is a BIOS bug, detecting a valid TDX private
+ * KeyID range on BSP indicates TDX has been enabled by the BIOS. If
+ * there's such BIOS bug, it will be caught later when initializing the
+ * TDX module.
+ */

I have no idea what that comment is doing. Can it just be removed?

Review

Quoted 5 lines

(quoted diff) +4 -0

+	/*
+	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
+	 * KeyIDs start after the last MKTME KeyID.
+	 */

Is the TME key a "MKTME KeyID"?

Review

Quoted 5 lines

(quoted diff) +4 -0

+static void __init clear_tdx(void)
+{
+	tdx_keyid_start = tdx_keyid_num = 0;
+}

This is where a comment is needed and can actually help.

/*
* tdx_keyid_start/num indicate that TDX is uninitialized. This
* is used in TDX initialization error paths to take it from
* initialized -> uninitialized.
*/

Quoted 6 lines

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

Quoted 4 lines

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

+	/*
+	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
+	 * KeyIDs start after the last MKTME KeyID.
+	 */

Is the TME key a "MKTME KeyID"?

I don't think so. Hardware handles TME KeyID 0 differently from non-0 MKTME KeyIDs. And PCONFIG only accept non-0 KeyIDs.

Quoted 7 lines

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

Quoted 4 lines

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

+static void __init clear_tdx(void)
+{
+	tdx_keyid_start = tdx_keyid_num = 0;
+}

This is where a comment is needed and can actually help.

Quoted 5 lines

/*
* tdx_keyid_start/num indicate that TDX is uninitialized. This
* is used in TDX initialization error paths to take it from
* initialized -> uninitialized.
*/

Just want to point out after removing the !x2apic_enabled() check, the only thing need to do here is to detect/record the TDX KeyIDs.

And the purpose of this TDX boot-time initialization code is to provide platform_tdx_enabled() function so that kexec() can use.

To distinguish boot-time TDX initialization from runtime TDX module initialization, how about change the comment to below?

static void __init clear_tdx(void)
{
/*
* tdx_keyid_start and nr_tdx_keyids indicate that TDX is not
* enabled by the BIOS. This is used in TDX boot-time
* initializatiton error paths to take it from enabled to not
* enabled.
*/
tdx_keyid_start = nr_tdx_keyids = 0;
}

[...]

And below is the updated patch. How does it look to you?

arch/x86/virt/vmx/tdx/tdx.c new file +13 -0

+static u32 tdx_keyid_start __ro_after_init;
+static u32 nr_tdx_keyids __ro_after_init;
+
+static int __init record_keyid_partitioning(void)
+{
+       u32 nr_mktme_keyids;
+       int ret;
+
+       /*
+        * IA32_MKTME_KEYID_PARTIONING:
+        *   Bit [31:0]:        Number of MKTME KeyIDs.
+        *   Bit [63:32]:       Number of TDX private KeyIDs.
+        */

Quoted 9 lines · Kai

Quoted 6 lines

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

Quoted 4 lines

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

+	/*
+	 * KeyID 0 is for TME.  MKTME KeyIDs start from 1.  TDX private
+	 * KeyIDs start after the last MKTME KeyID.
+	 */

Is the TME key a "MKTME KeyID"?

I don't think so. Hardware handles TME KeyID 0 differently from non-0 MKTME KeyIDs. And PCONFIG only accept non-0 KeyIDs.

Let's say we have 4 MKTME hardware bits, we'd have:

0: TME Key
1->3: MKTME Keys
4->7: TDX Private Keys

First, the MSR values:

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

Quoted 3 lines

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

+        * IA32_MKTME_KEYID_PARTIONING:
+        *   Bit [31:0]:        Number of MKTME KeyIDs.
+        *   Bit [63:32]:       Number of TDX private KeyIDs.

These would be:

	Bit [ 31:0] = 3
	Bit [63:22] = 4

And in the end the variables:

tdx_keyid_start would be 4 and tdx_keyid_num would be 4.

Right?

That's a bit wonky for my brain because I guess I know too much about the internal implementation and how the key space is split up. I guess I (wrongly) expected Bit[31:0]==Bit[63:22].

Quoted 36 lines

Quoted 10 lines

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

Quoted 7 lines

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

+static void __init clear_tdx(void)
+{
+	tdx_keyid_start = tdx_keyid_num = 0;
+}

This is where a comment is needed and can actually help.

Quoted 5 lines

/*
* tdx_keyid_start/num indicate that TDX is uninitialized. This
* is used in TDX initialization error paths to take it from
* initialized -> uninitialized.
*/

Just want to point out after removing the !x2apic_enabled() check, the only thing need to do here is to detect/record the TDX KeyIDs.

And the purpose of this TDX boot-time initialization code is to provide platform_tdx_enabled() function so that kexec() can use.

To distinguish boot-time TDX initialization from runtime TDX module initialization, how about change the comment to below?

static void __init clear_tdx(void)
{
/*
* tdx_keyid_start and nr_tdx_keyids indicate that TDX is not
* enabled by the BIOS. This is used in TDX boot-time
* initializatiton error paths to take it from enabled to not
* enabled.
*/
tdx_keyid_start = nr_tdx_keyids = 0;
}

Quoted 1 line

[...]

I honestly have no idea what "boot-time TDX initialization" is versus "runtime TDX module initialization". This doesn't hel.

Quoted 2 lines

And below is the updated patch. How does it look to you?

Let's see...

...

Quoted 13 lines

arch/x86/virt/vmx/tdx/tdx.c new file +13 -0

+static u32 tdx_keyid_start __ro_after_init;
+static u32 nr_tdx_keyids __ro_after_init;
+
+static int __init record_keyid_partitioning(void)
+{
+       u32 nr_mktme_keyids;
+       int ret;
+
+       /*
+        * IA32_MKTME_KEYID_PARTIONING:
+        *   Bit [31:0]:        Number of MKTME KeyIDs.
+        *   Bit [63:32]:       Number of TDX private KeyIDs.
+        */

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

Quoted 10 lines

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

+       ret = rdmsr_safe(MSR_IA32_MKTME_KEYID_PARTITIONING, &nr_mktme_keyids,
+                       &nr_tdx_keyids);
+       if (ret)
+               return -ENODEV;
+
+       if (!nr_tdx_keyids)
+               return -ENODEV;
+
+       /* TDX KeyIDs start after the last MKTME KeyID. */
+       tdx_keyid_start++;

tdx_keyid_start is uniniitalized here. So, it'd be 0, then ++'d.

Kai, please take a moment and slow down. This isn't a race. I offered some replacement code here, which you've discarded, missed or ignored and in the process broken this code.

This approach just wastes reviewer time. It's not working for me.

I'm going to make a suggestion (aka. a demand): You can post these patches at most once a week. You get a whole week to (carefully) incorporate reviewer feedback, make the patch better, and post a new version. Need more time? Go ahead and take it. Take as much time as you want.

The MMIO/xAPIC interface has some problems, most notably the APIC LEAK

[1]. This bug allows an attacker to use the APIC MMIO interface to extract data from the SGX enclave.

TDX is not immune from this either. Early check X2APIC and disable TDX if X2APIC is not enabled, and make INTEL_TDX_HOST depend on X86_X2APIC.

[1]: https://aepicleak.com/aepicleak.pdf

Link https://lore.kernel.org/lkml/[email protected]/ open here

Link https://lore.kernel.org/lkml/[email protected]/ open here

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

---

v6 -> v7:
- Changed to use "Link" for the two lore links to get rid of checkpatch warning.

---
 arch/x86/Kconfig            |  1 +
 arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

arch/x86/Kconfig +1 -0

index cced4ef3bfb2..dd333b46fafb 100644
@@ -1958,6 +1958,7 @@ config INTEL_TDX_HOST
 	depends on CPU_SUP_INTEL
 	depends on X86_64
 	depends on KVM_INTEL
+	depends on X86_X2APIC
 	help
 	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
 	  host and certain physical attacks.  This option enables necessary TDX

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

index 982d9c453b6b..8d943bdc8335 100644
@@ -12,6 +12,7 @@
 #include <linux/printk.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
+#include <asm/apic.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 

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

+		goto no_tdx;
+	}
+
 	return 0;
 no_tdx:
 	clear_tdx();

Quoted 1 line · Kai Huang

The MMIO/xAPIC interface has some problems, most notably the APIC LEAK

"some problems" looks more generic. May be we can be specific here. Like it has security issues?

Quoted 51 lines

[1]. This bug allows an attacker to use the APIC MMIO interface to extract data from the SGX enclave.

TDX is not immune from this either. Early check X2APIC and disable TDX if X2APIC is not enabled, and make INTEL_TDX_HOST depend on X86_X2APIC.

[1]: https://aepicleak.com/aepicleak.pdf

Link https://lore.kernel.org/lkml/[email protected]/ open here

Link https://lore.kernel.org/lkml/[email protected]/ open here

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

---

v6 -> v7:
- Changed to use "Link" for the two lore links to get rid of checkpatch warning.

---
 arch/x86/Kconfig            |  1 +
 arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

arch/x86/Kconfig +1 -0

index cced4ef3bfb2..dd333b46fafb 100644
@@ -1958,6 +1958,7 @@ config INTEL_TDX_HOST
 	depends on CPU_SUP_INTEL
 	depends on X86_64
 	depends on KVM_INTEL
+	depends on X86_X2APIC
 	help
 	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
 	  host and certain physical attacks.  This option enables necessary TDX

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

index 982d9c453b6b..8d943bdc8335 100644
@@ -12,6 +12,7 @@
 #include <linux/printk.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
+#include <asm/apic.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 

@@ -81,6 +82,16 @@ static int __init tdx_init(void)
 		goto no_tdx;
 	}
 

arch/x86/virt/vmx/tdx/tdx.c static int __init tdx_init(void) · line 88

Quoted 4 lines

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

+	/*
+	 * TDX requires X2APIC being enabled to prevent potential data
+	 * leak via APIC MMIO registers.  Just disable TDX if not using
+	 * X2APIC.

Remove the double space.

arch/x86/virt/vmx/tdx/tdx.c static int __init tdx_init(void) · line 91

Quoted 4 lines

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

+	 */
+	if (!x2apic_enabled()) {
+		pr_info("Disable TDX as X2APIC is not enabled.\n");

pr_warn()?

Quoted 6 lines · Sathyanarayanan Kuppuswamy

Quoted 1 line · Kai Huang

The MMIO/xAPIC interface has some problems, most notably the APIC LEAK

"some problems" looks more generic. May be we can be specific here. Like it has security issues?

It was quoted from below upstream commit id (I only kept the one that I quoted to save space):

commit b8d1d163604bd1e600b062fb00de5dc42baa355f (tag: x86_apic_for_v6.1_rc1, tip/x86/apic)

Author: Daniel Sneddon <[email protected]>
Date:   Tue Aug 16 16:19:42 2022 -0700

x86/apic: Don't disable x2APIC if locked

....

The MMIO/xAPIC interface has some problems, most notably the APIC LEAK [1]. This bug allows an attacker to use the APIC MMIO interface to extract data from the SGX enclave.

....

[1]: https://aepicleak.com/aepicleak.pdf

Signed-off-by Daniel Sneddon <[email protected]>

Signed-off-by Dave Hansen <[email protected]>

Acked-by Dave Hansen <[email protected]>

Tested-by Neelima Krishnan <[email protected]>

Link:
https://lkml.kernel.org/r/[email protected]

Quoted 51 lines

Quoted 51 lines

[1]. This bug allows an attacker to use the APIC MMIO interface to extract data from the SGX enclave.

TDX is not immune from this either. Early check X2APIC and disable TDX if X2APIC is not enabled, and make INTEL_TDX_HOST depend on X86_X2APIC.

[1]: https://aepicleak.com/aepicleak.pdf

Link https://lore.kernel.org/lkml/[email protected]/ open here

Link https://lore.kernel.org/lkml/[email protected]/ open here

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

---

v6 -> v7:
- Changed to use "Link" for the two lore links to get rid of checkpatch warning.

---
 arch/x86/Kconfig            |  1 +
 arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

arch/x86/Kconfig +1 -0

index cced4ef3bfb2..dd333b46fafb 100644
@@ -1958,6 +1958,7 @@ config INTEL_TDX_HOST
 	depends on CPU_SUP_INTEL
 	depends on X86_64
 	depends on KVM_INTEL
+	depends on X86_X2APIC
 	help
 	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
 	  host and certain physical attacks.  This option enables necessary TDX

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

index 982d9c453b6b..8d943bdc8335 100644
@@ -12,6 +12,7 @@
 #include <linux/printk.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
+#include <asm/apic.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 

@@ -81,6 +82,16 @@ static int __init tdx_init(void)
 		goto no_tdx;
 	}
 

Quoted 6 lines

arch/x86/virt/vmx/tdx/tdx.c static int __init tdx_init(void) · line 88

Quoted 4 lines

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

+	/*
+	 * TDX requires X2APIC being enabled to prevent potential data
+	 * leak via APIC MMIO registers.  Just disable TDX if not using
+	 * X2APIC.

Remove the double space.

Sorry which "double space"?

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdx.c static int __init tdx_init(void) · line 91

Quoted 5 lines

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

+	 */
+	if (!x2apic_enabled()) {
+		pr_info("Disable TDX as X2APIC is not enabled.\n");

pr_warn()?

Why?

Quoted 9 lines

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdx.c static int __init tdx_init(void) · line 88

Quoted 5 lines

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

+	/*
+	 * TDX requires X2APIC being enabled to prevent potential data
+	 * leak via APIC MMIO registers.  Just disable TDX if not using
+	 * X2APIC.

Remove the double space.

Sorry which "double space"?

Before Just disable. It looked like double space. Is it not?

The MMIO/xAPIC interface has some problems, most notably the APIC LEAK [1]. This bug allows an attacker to use the APIC MMIO interface to extract data from the SGX enclave.

TDX is not immune from this either. Early check X2APIC and disable TDX if X2APIC is not enabled, and make INTEL_TDX_HOST depend on X86_X2APIC.

Quoted 6 lines · Kai Huang

The MMIO/xAPIC interface has some problems, most notably the APIC LEAK [1]. This bug allows an attacker to use the APIC MMIO interface to extract data from the SGX enclave.

TDX is not immune from this either. Early check X2APIC and disable TDX if X2APIC is not enabled, and make INTEL_TDX_HOST depend on X86_X2APIC.

This makes no sense.

This is TDX host code. TDX hosts are untrusted. Zero of the TDX security guarantees are provided by the host.

What is the benefit of disabling TDX from the host if x2APIC is not enabled? It can't be for security reasons since the host does not help provide TDX security guarantees. It also can't be for SGX because SGX doesn't depend on the OS doing anything in order to be secure.

So, this boils down to the most fundamental of questions you need to answer about every patch:

What does this code do?

What end-user-visible effect is there if this code is not present?

How about still having a patch to make INTEL_TDX_HOST depend on X86_X2APIC but with something below in the changelog?

"
TDX capable platforms are locked to X2APIC mode and cannot fall back to the
legacy xAPIC mode when TDX is enabled by the BIOS. It doesn't make sense to
turn on INTEL_TDX_HOST while X86_X2APIC is not enabled. Make INTEL_TDX_HOST depend on X86_X2APIC.

Quoted 4 lines · Kai

How about still having a patch to make INTEL_TDX_HOST depend on X86_X2APIC but with something below in the changelog?

Quoted 5 lines

"
TDX capable platforms are locked to X2APIC mode and cannot fall back to the
legacy xAPIC mode when TDX is enabled by the BIOS. It doesn't make sense to
turn on INTEL_TDX_HOST while X86_X2APIC is not enabled. Make INTEL_TDX_HOST depend on X86_X2APIC.

That's fine and it makes logical sense as a dependency. TDX host support requires x2APIC. Period.

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

+static int __tdx_enable(void)
+{
+	int ret;
+
+	/*
+	 * Initializing the TDX module requires doing SEAMCALL on all
+	 * boot-time present CPUs.  For simplicity temporarily disable
+	 * CPU hotplug to prevent any CPU from going offline during
+	 * the initialization.
+	 */
+	cpus_read_lock();
+
+	/*
+	 * Check whether all boot-time present CPUs are online and
+	 * return early with a message so the user can be aware.
+	 *
+	 * Note a non-buggy BIOS should never support physical (ACPI)
+	 * CPU hotplug when TDX is enabled, and all boot-time present
+	 * CPU should be enabled in MADT, so there should be no
+	 * disabled_cpus and num_processors won't change at runtime
+	 * either.
+	 */
+	if (disabled_cpus || num_online_cpus() != num_processors) {
+		pr_err("Unable to initialize the TDX module when there's offline CPU(s).\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = init_tdx_module();
+	if (ret == -ENODEV) {
+		pr_info("TDX module is not loaded.\n");
+		tdx_module_status = TDX_MODULE_NONE;
+		goto out;
+	}
+
+	/*
+	 * Shut down the TDX module in case of any error during the
+	 * initialization process.  It's meaningless to leave the TDX
+	 * module in any middle state of the initialization process.
+	 *
+	 * Shutting down the module also requires doing SEAMCALL on all
+	 * MADT-enabled CPUs.  Do it while CPU hotplug is disabled.
+	 *
+	 * Return all errors during the initialization as -EFAULT as the
+	 * module is always shut down.
+	 */
+	if (ret) {
+		pr_info("Failed to initialize TDX module. Shut it down.\n");
+		shutdown_tdx_module();
+		tdx_module_status = TDX_MODULE_SHUTDOWN;
+		ret = -EFAULT;

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 205

Quoted 4 lines · Kai Huang

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

+	cpus_read_unlock();
+
+	return ret;
+}

Uhm.. so if we've offlined all the SMT siblings because of some speculation fail or other, this TDX thing will fail to initialize?

Because as I understand it; this TDX initialization happens some random time after boot, when the first (TDX using) KVM instance gets created, long after the speculation mitigations are enforced.

Nothing in the TDX specs and docs mentions physical hotplug or a requirement for invoking seamcall on the world.

    if (tdx_global_data_ptr->num_of_init_lps < tdx_global_data_ptr->num_of_lps)
    {
        TDX_ERROR("Num of initialized lps %d is smaller than total num of lps %d\n",
                    tdx_global_data_ptr->num_of_init_lps, tdx_global_data_ptr->num_of_lps);
        retval = TDX_SYS_CONFIG_NOT_PENDING;
        goto EXIT;
    }

Quoted 2 lines · Thomas Gleixner

Nothing in the TDX specs and docs mentions physical hotplug or a requirement for invoking seamcall on the world.

The TDX module source is actually out there[1] for us to look at. It's in a lovely, convenient zip file, but you can read it if sufficiently motivated.

It has this lovely nugget in it:

WARNING!!! Proprietary License!! Avert your virgin eyes!!!

Quoted 7 lines

    if (tdx_global_data_ptr->num_of_init_lps < tdx_global_data_ptr->num_of_lps)
    {
        TDX_ERROR("Num of initialized lps %d is smaller than total num of lps %d\n",
                    tdx_global_data_ptr->num_of_init_lps, tdx_global_data_ptr->num_of_lps);
        retval = TDX_SYS_CONFIG_NOT_PENDING;
        goto EXIT;
    }

tdx_global_data_ptr->num_of_init_lps is incremented at TDH.SYS.INIT time. That if() is called at TDH.SYS.CONFIG time to help bring the module up.

So, I think you're right. I don't see the docs that actually *explain* this "you must seamcall all the things" requirement.

Quoted 7 lines · Dave Hansen

Quoted 2 lines · Thomas Gleixner

Nothing in the TDX specs and docs mentions physical hotplug or a requirement for invoking seamcall on the world.

The TDX module source is actually out there[1] for us to look at. It's in a lovely, convenient zip file, but you can read it if sufficiently motivated.

zip file? Version control from the last millenium?

The whole thing wants to be @github with a proper change history if Intel wants anyone to trust this and take it serious.

/me refrains from ranting about the outrageous license choice.

Quoted 11 lines

Quoted 10 lines

     if (tdx_global_data_ptr->num_of_init_lps < tdx_global_data_ptr-

Quoted 1 line

num_of_lps)

     {
         TDX_ERROR("Num of initialized lps %d is smaller than total num of lps %d\n",
                     tdx_global_data_ptr->num_of_init_lps, tdx_global_data_ptr->num_of_lps);
         retval = TDX_SYS_CONFIG_NOT_PENDING;
         goto EXIT;
     }

Quoted 6 lines

tdx_global_data_ptr->num_of_init_lps is incremented at TDH.SYS.INIT time.  That if() is called at TDH.SYS.CONFIG time to help bring the module up.

So, I think you're right.  I don't see the docs that actually *explain* this "you must seamcall all the things" requirement.

The code actually enforces this.

At TDH.SYS.INIT which is the first operation it gets the total number of LPs from the sysinfo table:

src/vmm_dispatcher/api_calls/tdh_sys_init.c:

    tdx_global_data_ptr->num_of_lps = sysinfo_table_ptr-

Quoted 1 line

mcheck_fields.tot_num_lps;

Then TDH.SYS.LP.INIT increments the count of initialized LPs.

src/vmm_dispatcher/api_calls/tdh_sys_lp_init.c:

    increment_num_of_lps(tdx_global_data_ptr) _lock_xadd_32b(&tdx_global_data_ptr->num_of_init_lps, 1);

Finally TDH.SYS.CONFIG checks whether _ALL_ LPs have been initialized.

src/vmm_dispatcher/api_calls/tdh_sys_config.c:

    if (tdx_global_data_ptr->num_of_init_lps < tdx_global_data_ptr-

Quoted 1 line

num_of_lps)

Clearly that's nowhere spelled out in the documentation, but I don't buy the 'architecturaly required' argument not at all. It's an implementation detail of the TDX module.

Technically there is IMO ZERO requirement to do so.

 1) The TDX module is global

 2) Seam-root and Seam-non-root operation are strictly a LP property.

    The only architectural prerequisite for using Seam on a LP is that obviously the encryption/decryption mechanics have been initialized on the package to which the LP belongs.

I can see why it might be complicated to add/remove an LP after initialization fact, but technically it should be possible.

TDX/Seam is not that special.

But what's absolutely annoying is that the documentation lacks any information about the choice of enforcement which has been hardcoded into the Seam module for whatever reasons.

Maybe I overlooked it, but then it's definitely well hidden.

Quoted 4 lines

Clearly that's nowhere spelled out in the documentation, but I don't buy the 'architecturaly required' argument not at all. It's an implementation detail of the TDX module.

Hi Thomas,

Thanks for review!

I agree on hardware level there shouldn't be such requirement (not 100% sure though), but I guess from kernel's perspective, "the implementation detail of the TDX module" is sort of "architectural requirement" -- at least Intel arch guys think so I guess.

Quoted 3 lines · Thomas Gleixner

Clearly that's nowhere spelled out in the documentation, but I don't buy the 'architecturaly required' argument not at all. It's an implementation detail of the TDX module.

I agree on hardware level there shouldn't be such requirement (not 100% sure though), but I guess from kernel's perspective, "the implementation detail of the TDX module" is sort of "architectural requirement"

-- at least Intel arch guys think so I guess.

Quoted 10 lines

Technically there is IMO ZERO requirement to do so.

 1) The TDX module is global

 2) Seam-root and Seam-non-root operation are strictly a LP property.

    The only architectural prerequisite for using Seam on a LP is that obviously the encryption/decryption mechanics have been initialized on the package to which the LP belongs.

Quoted 2 lines

I can see why it might be complicated to add/remove an LP after initialization fact, but technically it should be possible.

"kernel soft offline" actually isn't an issue. We can bring down a logical cpu after it gets initialized and then bring it up again.

Only add/removal of physical cpu will cause problem:

TDX MCHECK verifies all boot-time present cpus to make sure they are TDX- compatible before it enables TDX in hardware. MCHECK cannot run on hot-added CPU, so TDX cannot support physical CPU hotplug.

We tried to get it clarified in the specification, and below is what TDX/module arch guys agreed to put to the TDX module spec (just checked it's not in latest public spec yet, but they said it will be in next release):

"
4.1.3.2. CPU Configuration

During platform boot, MCHECK verifies all logical CPUs to ensure they meet TDX’s

security and certain functionality requirements, and MCHECK passes the following CPU configuration information to the NP-SEAMLDR, P-SEAMLDR and the TDX Module:

· Total number of logical processors in the platform.

· Total number of installed packages in the platform.

· A table of per-package CPU family, model and stepping etc.
identification, as enumerated by CPUID(1).EAX.
The above information is static and does not change after platform boot and MCHECK run.

Note: TDX doesn’t support adding or removing CPUs from TDX security
perimeter, as checked my MCHECK.

BIOS should prevent CPUs from being hot-added or hot-removed after platform boots.

The TDX module performs additional checks of the CPU’s configuration and supported features, by reading MSRs and CPUID information as described in the following sections.

Kai!

Quoted 8 lines · Kai Huang

Quoted 3 lines · Thomas Gleixner

Clearly that's nowhere spelled out in the documentation, but I don't buy the 'architecturaly required' argument not at all. It's an implementation detail of the TDX module.

I agree on hardware level there shouldn't be such requirement (not 100% sure though), but I guess from kernel's perspective, "the implementation detail of the TDX module" is sort of "architectural requirement"

Sure, but then it needs to be clearly documented so.

Quoted 1 line

-- at least Intel arch guys think so I guess.

Intel "arch" guys? You might look up the meanings of "arch" in a dictionary. LKML is not twatter.

Quoted 10 lines

Quoted 10 lines

Technically there is IMO ZERO requirement to do so.

 1) The TDX module is global

 2) Seam-root and Seam-non-root operation are strictly a LP property.

    The only architectural prerequisite for using Seam on a LP is that obviously the encryption/decryption mechanics have been initialized on the package to which the LP belongs.

Quoted 5 lines

Quoted 2 lines

I can see why it might be complicated to add/remove an LP after initialization fact, but technically it should be possible.

"kernel soft offline" actually isn't an issue. We can bring down a logical cpu after it gets initialized and then bring it up again.

That's the whole point where this discussion started: _AFTER_ it gets initialized.

Which means that, e.g. adding "nosmt" to the kernel command line will make TDX fail hard because at the point where TDX is initialized the hyperthreads are not 'soft' online and cannot respond to anything, but the BIOS already accounted them.

This is just wrong as we all know that "nosmt" is sadly one of the obvious counter measures for the never ending flood of speculation issues.

Quoted 2 lines

Only add/removal of physical cpu will cause problem:

You wish.

Quoted 3 lines

TDX MCHECK verifies all boot-time present cpus to make sure they are TDX- compatible before it enables TDX in hardware. MCHECK cannot run on hot-added CPU, so TDX cannot support physical CPU hotplug.

TDX can rightfully impose the limitation that it only executes on CPUs, which are known at boot/initialization time, and only utilizes "known" memory. That's it, but that does not enforce to prevent physical hotplug in general.

Quoted 8 lines

We tried to get it clarified in the specification, and below is what TDX/module arch guys agreed to put to the TDX module spec (just checked it's not in latest public spec yet, but they said it will be in next release):

"
4.1.3.2. CPU Configuration

Quoted 2 lines

During platform boot, MCHECK verifies all logical CPUs to ensure they meet TDX’s

That MCHECK falls into the category of security voodoo.

It needs to verify _ALL_ logical CPUs to ensure that Intel did not put different models and steppings into a package or what?

Quoted 5 lines

security and certain functionality requirements, and MCHECK passes the following CPU configuration information to the NP-SEAMLDR, P-SEAMLDR and the TDX Module:

· Total number of logical processors in the platform.

You surely need MCHECK for this

Quoted 1 line

· Total number of installed packages in the platform.

and for this...

Quoted 5 lines

· A table of per-package CPU family, model and stepping etc.
identification, as enumerated by CPUID(1).EAX.
The above information is static and does not change after platform boot and MCHECK run.

Quoted 2 lines

Note: TDX doesn’t support adding or removing CPUs from TDX security
perimeter, as checked my MCHECK.

More security voodoo. The TDX security perimeter has nothing to do with adding or removing CPUs on a system. If that'd be true then TDX is a complete fail.

Quoted 2 lines

BIOS should prevent CPUs from being hot-added or hot-removed after platform boots.

If the BIOS does not prevent it, then TDX and the Seam module will not even notice unless the OS tries to invoke seamcall() on a newly plugged CPU.

A newly added CPU and newly added memory should not have any impact on the TDX integrity of the already running system. If they have then again, TDX is broken by design.

Quoted 4 lines

The TDX module performs additional checks of the CPU’s configuration and supported features, by reading MSRs and CPUID information as described in the following sections.

to ensure that the MCHECK generated table is still valid at the point where TDX is initialized?

That said, this documentation is at least better than the existing void, but that does not make it technically correct.

Thanks,

tglx

2) It is more flexible to support TDX module runtime updating in the future (after updating the TDX module, it needs to be initialized again).

3) It avoids having to do a "temporary" solution to handle VMXON in the core (non-KVM) kernel for now. This is because SEAMCALL requires CPU being in VMX operation (VMXON is done), but currently only KVM handles VMXON. Adding VMXON support to the core kernel isn't trivial. More importantly, from long-term a reference-based approach is likely needed in the core kernel as more kernel components are likely needed to support TDX as well. Allow KVM to initialize the TDX module avoids having to handle VMXON during kernel boot for now.

Add a placeholder tdx_enable() to detect and initialize the TDX module on demand, with a state machine protected by mutex to support concurrent calls from multiple callers.

The TDX module will be initialized in multi-steps defined by the TDX module:

1) Global initialization;
2) Logical-CPU scope initialization;
3) Enumerate the TDX module capabilities and platform configuration;
4) Configure the TDX module about TDX usable memory ranges and global

     KeyID information;

5) Package-scope configuration for the global KeyID;
6) Initialize usable memory ranges based on 4).

The TDX module can also be shut down at any time during its lifetime. In case of any error during the initialization process, shut down the module. It's pointless to leave the module in any intermediate state during the initialization.

Both logical CPU scope initialization and shutting down the TDX module require calling SEAMCALL on all boot-time present CPUs. For simplicity just temporarily disable CPU hotplug during the module initialization.

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

index 8d943bdc8335..28c187b8726f 100644
@@ -10,15 +10,34 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/printk.h>
+#include <linux/mutex.h>
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 
+/* TDX module status during initialization */

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

 static u32 tdx_keyid_start __ro_after_init;
 static u32 tdx_keyid_num __ro_after_init;
 
+static enum tdx_module_status_t tdx_module_status;
+/* Prevent concurrent attempts on TDX detection and initialization */
+static DEFINE_MUTEX(tdx_module_lock);
+
 /*
  * Detect TDX private KeyIDs to see whether TDX has been enabled by the
  * BIOS.  Both initializing the TDX module and running TDX guest require

@@ -104,3 +123,134 @@ bool platform_tdx_enabled(void)
 {
 	return !!tdx_keyid_num;
 }
+
+/*
+ * Detect and initialize the TDX module.
+ *
+ * Return -ENODEV when the TDX module is not loaded, 0 when it
+ * is successfully initialized, or other error when it fails to
+ * initialize.
+ */
+static int init_tdx_module(void)
+{
+	/* The TDX module hasn't been detected */
+	return -ENODEV;
+}
+
+static void shutdown_tdx_module(void)
+{
+	/* TODO: Shut down the TDX module */
+}
+
+static int __tdx_enable(void)
+{
+	int ret;
+
+	/*
+	 * Initializing the TDX module requires doing SEAMCALL on all
+	 * boot-time present CPUs.  For simplicity temporarily disable
+	 * CPU hotplug to prevent any CPU from going offline during
+	 * the initialization.
+	 */
+	cpus_read_lock();
+

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

+		pr_info("TDX module is not loaded.\n");
+		tdx_module_status = TDX_MODULE_NONE;
+		goto out;
+	}
+
+	/*
+	 * Shut down the TDX module in case of any error during the
+	 * initialization process.  It's meaningless to leave the TDX

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

+		shutdown_tdx_module();
+		tdx_module_status = TDX_MODULE_SHUTDOWN;
+		ret = -EFAULT;
+		goto out;
+	}
+
+	pr_info("TDX module initialized.\n");
+	tdx_module_status = TDX_MODULE_INITIALIZED;
+out:
+	cpus_read_unlock();

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

+int tdx_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;

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

+	mutex_unlock(&tdx_module_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tdx_enable);

Quoted 3 lines · Kai Huang

2) It is more flexible to support TDX module runtime updating in the future (after updating the TDX module, it needs to be initialized again).

I hate this generic blabber about "more flexible". There's a *REASON* it's more flexible, so let's talk about the reasons, please.

It's really something like this, right?

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" loading mechanism. The hope is that much of this "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.

Quoted 10 lines

3) It avoids having to do a "temporary" solution to handle VMXON in the core (non-KVM) kernel for now. This is because SEAMCALL requires CPU being in VMX operation (VMXON is done), but currently only KVM handles VMXON. Adding VMXON support to the core kernel isn't trivial. More importantly, from long-term a reference-based approach is likely needed in the core kernel as more kernel components are likely needed to support TDX as well. Allow KVM to initialize the TDX module avoids having to handle VMXON during kernel boot for now.

There are a lot of words in there.

3) Loading the TDX module requires VMX to be enabled. Currently, only the kernel 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.

Quoted 4 lines

Add a placeholder tdx_enable() to detect and initialize the TDX module on demand, with a state machine protected by mutex to support concurrent calls from multiple callers.

As opposed to concurrent calls from one caller? ;)

Quoted 11 lines

The TDX module will be initialized in multi-steps defined by the TDX module:

1) Global initialization;
2) Logical-CPU scope initialization;
3) Enumerate the TDX module capabilities and platform configuration;
4) Configure the TDX module about TDX usable memory ranges and global

     KeyID information;

5) Package-scope configuration for the global KeyID;
6) Initialize usable memory ranges based on 4).

This would actually be a nice place to call out the SEAMCALL names and mention that each of these steps involves a set of SEAMCALLs.

Quoted 6 lines

The TDX module can also be shut down at any time during its lifetime. In case of any error during the initialization process, shut down the module. It's pointless to leave the module in any intermediate state during the initialization.

Quoted 3 lines

Both logical CPU scope initialization and shutting down the TDX module require calling SEAMCALL on all boot-time present CPUs. For simplicity just temporarily disable CPU hotplug during the module initialization.

You might want to more precisely define "boot-time present CPUs". The boot of *what*?

Quoted 18 lines

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

index 8d943bdc8335..28c187b8726f 100644
@@ -10,15 +10,34 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/printk.h>
+#include <linux/mutex.h>
+#include <linux/cpu.h>
+#include <linux/cpumask.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 

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

Quoted 11 lines

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

+/* TDX module status during initialization */
+enum tdx_module_status_t {
+	/* TDX module hasn't been detected and initialized */
+	TDX_MODULE_UNKNOWN,
+	/* TDX module is not loaded */
+	TDX_MODULE_NONE,
+	/* TDX module is initialized */
+	TDX_MODULE_INITIALIZED,
+	/* TDX module is shut down due to initialization error */
+	TDX_MODULE_SHUTDOWN,
+};

Are these part of the ABI or just a purely OS-side construct?

Quoted 46 lines

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

 static u32 tdx_keyid_start __ro_after_init;
 static u32 tdx_keyid_num __ro_after_init;
 
+static enum tdx_module_status_t tdx_module_status;
+/* Prevent concurrent attempts on TDX detection and initialization */
+static DEFINE_MUTEX(tdx_module_lock);
+
 /*
  * Detect TDX private KeyIDs to see whether TDX has been enabled by the
  * BIOS.  Both initializing the TDX module and running TDX guest require

@@ -104,3 +123,134 @@ bool platform_tdx_enabled(void)
 {
 	return !!tdx_keyid_num;
 }
+
+/*
+ * Detect and initialize the TDX module.
+ *
+ * Return -ENODEV when the TDX module is not loaded, 0 when it
+ * is successfully initialized, or other error when it fails to
+ * initialize.
+ */
+static int init_tdx_module(void)
+{
+	/* The TDX module hasn't been detected */
+	return -ENODEV;
+}
+
+static void shutdown_tdx_module(void)
+{
+	/* TODO: Shut down the TDX module */
+}
+
+static int __tdx_enable(void)
+{
+	int ret;
+
+	/*
+	 * Initializing the TDX module requires doing SEAMCALL on all
+	 * boot-time present CPUs.  For simplicity temporarily disable
+	 * CPU hotplug to prevent any CPU from going offline during
+	 * the initialization.
+	 */
+	cpus_read_lock();
+

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 166

Quoted 10 lines

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

+	/*
+	 * Check whether all boot-time present CPUs are online and
+	 * return early with a message so the user can be aware.
+	 *
+	 * Note a non-buggy BIOS should never support physical (ACPI)
+	 * CPU hotplug when TDX is enabled, and all boot-time present
+	 * CPU should be enabled in MADT, so there should be no
+	 * disabled_cpus and num_processors won't change at runtime
+	 * either.
+	 */

Again, there are a lot of words in that comment, but I'm not sure why it's here. Despite all the whinging about ACPI, doesn't it boil down to:

The TDX module itself establishes its own concept of how many logical CPUs there are in the system when it is loaded.

The
module will reject initialization attempts unless the kernel runs TDX initialization code on every last CPU.

Ensure that the kernel is able to run code on all known logical CPUs.

and these checks are just to see if the kernel has shot itself in the foot and is *KNOWS* that it is currently unable to run code on some logical CPU?

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 174

Quoted 9 lines

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

+	if (disabled_cpus || num_online_cpus() != num_processors) {
+		pr_err("Unable to initialize the TDX module when there's offline CPU(s).\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = init_tdx_module();
+	if (ret == -ENODEV) {

Why check for -ENODEV exclusively? Is there some other error nonzero code that indicates success?

Quoted 14 lines

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

+		pr_info("TDX module is not loaded.\n");
+		tdx_module_status = TDX_MODULE_NONE;
+		goto out;
+	}
+
+	/*
+	 * Shut down the TDX module in case of any error during the
+	 * initialization process.  It's meaningless to leave the TDX
+	 * module in any middle state of the initialization process.
+	 *
+	 * Shutting down the module also requires doing SEAMCALL on all
+	 * MADT-enabled CPUs.  Do it while CPU hotplug is disabled.
+	 *

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 192

Quoted 5 lines

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

+	 * Return all errors during the initialization as -EFAULT as the
+	 * module is always shut down.
+	 */
+	if (ret) {
+		pr_info("Failed to initialize TDX module. Shut it down.\n");

"Shut it down" seems wrong here. That could be interpreted as "I have already shut it down". "Shutting down" seems better.

Quoted 11 lines

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

+		shutdown_tdx_module();
+		tdx_module_status = TDX_MODULE_SHUTDOWN;
+		ret = -EFAULT;
+		goto out;
+	}
+
+	pr_info("TDX module initialized.\n");
+	tdx_module_status = TDX_MODULE_INITIALIZED;
+out:
+	cpus_read_unlock();

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 212

Quoted 10 lines

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

+
+	return ret;
+}
+
+/**
+ * tdx_enable - Enable TDX by initializing the TDX module
+ *
+ * Caller to make sure all CPUs are online and in VMX operation before
+ * calling this function.  CPU hotplug is temporarily disabled internally
+ * to prevent any cpu from going offline.

"cpu" or "CPU"?

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 226

Quoted 13 lines

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

+ * This function can be called in parallel by multiple callers.
+ *
+ * Return:
+ *
+ * * 0:		The TDX module has been successfully initialized.
+ * * -ENODEV:	The TDX module is not loaded, or TDX is not supported.
+ * * -EINVAL:	The TDX module cannot be initialized due to certain
+ *		conditions are not met (i.e. when not all MADT-enabled
+ *		CPUs are not online).
+ * * -EFAULT:	Other internal fatal errors, or the TDX module is in
+ *		shutdown mode due to it failed to initialize in previous
+ *		attempts.
+ */

I honestly don't think all these error codes mean anything. They're plumbed nowhere and the use of -EFAULT is just plain wrong.

Nobody can *DO* anything with these anyway.

Just give one error code and make sure that you have pr_info()'s around to make it clear what went wrong. Then just do -EINVAL universally. Remove all the nonsense comments.

Quoted 6 lines

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

+int tdx_enable(void)
+{
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -ENODEV;

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 242

Quoted 10 lines

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

+
+	mutex_lock(&tdx_module_lock);
+
+	switch (tdx_module_status) {
+	case TDX_MODULE_UNKNOWN:
+		ret = __tdx_enable();
+		break;
+	case TDX_MODULE_NONE:
+		ret = -ENODEV;
+		break;

TDX_MODULE_NONE should probably be called TDX_MODULE_NOT_LOADED. A comment would also be nice:

	/* The BIOS did not load the module.  No way to fix that. */

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 243

Quoted 1 line

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

+	case TDX_MODULE_INITIALIZED:

		/* Already initialized, great, tell the caller: */

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 250

Quoted 8 lines

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

+		ret = 0;
+		break;
+	default:
+		WARN_ON_ONCE(tdx_module_status != TDX_MODULE_SHUTDOWN);
+		ret = -EFAULT;
+		break;
+	}

I don't get what that default: is for or what it has to do with TDX_MODULE_SHUTDOWN.

Quoted 7 lines

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

+	mutex_unlock(&tdx_module_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tdx_enable);

Quoted 6 lines

Again, there are a lot of words in that comment, but I'm not sure why it's here. Despite all the whinging about ACPI, doesn't it boil down to:

The TDX module itself establishes its own concept of how many logical CPUs there are in the system when it is loaded.

This isn't accurate. TDX MCHECK records the total number of logical CPUs when the BIOS enables TDX. This happens before the TDX module is loaded. In fact the TDX module only gets this information from a secret location.

    //NUM_LPS
    tdx_global_data_ptr->num_of_lps = sysinfo_table_ptr->mcheck_fields.tot_num_lps;

Quoted 9 lines · Kai

Quoted 6 lines

Again, there are a lot of words in that comment, but I'm not sure why it's here. Despite all the whinging about ACPI, doesn't it boil down to:

The TDX module itself establishes its own concept of how many logical CPUs there are in the system when it is loaded.

This isn't accurate. TDX MCHECK records the total number of logical CPUs when the BIOS enables TDX. This happens before the TDX module is loaded. In fact the TDX module only gets this information from a secret location.

Kai, this is the point where I lose patience with the conversation around this series. I'll you paste you the line of code where the TDX module literally "establishes its own concept of how many logical CPUs there are in the system":

Quoted 2 lines

    //NUM_LPS
    tdx_global_data_ptr->num_of_lps = sysinfo_table_ptr->mcheck_fields.tot_num_lps;

Yes, this is derived directly from MCHECK. But, this concept is separate from MCHECK. We have seen zero actual facts from you or other folks at Intel that this is anything other than an arbitrary choice made for the convenience of the TDX module. It _might_ not be this way. I'm open to hearing those facts and changing my position on this.

But, please bring facts, not random references to "secret locations" that aren't that secret.

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 136

Quoted 10 lines · Kai Huang

arch/x86/virt/vmx/tdx/tdx.c +10 -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)
+{

What's the point of a 'static __always_unused' function again? Other than to test the DCE pass of a linker, that is?

TDX introduces a new CPU mode: Secure Arbitration Mode (SEAM). This mode runs only the TDX module itself or other code to load the TDX module.

The host kernel communicates with SEAM software via a new SEAMCALL instruction. This is conceptually similar to a guest->host hypercall, except it is made from the host to SEAM software instead.

The TDX module defines a set of SEAMCALL leaf functions to allow the host to initialize it, and to create and run protected VMs. SEAMCALL leaf functions use an ABI different from the x86-64 system-v ABI. Instead, they share the same ABI with the TDCALL leaf functions.

Implement a function __seamcall() to allow the host to make SEAMCALL to SEAM software using the TDX_MODULE_CALL macro which is the common assembly for both SEAMCALL and TDCALL.

SEAMCALL instruction causes #GP when SEAMRR isn't enabled, and #UD when CPU is not in VMX operation. The current TDX_MODULE_CALL macro doesn't handle any of them. There's no way to check whether the CPU is in VMX operation or not.

Initializing the TDX module is done at runtime on demand, and it depends on the caller to ensure CPU is in VMX operation before making SEAMCALL. To avoid getting Oops when the caller mistakenly tries to initialize the TDX module when CPU is not in VMX operation, extend the TDX_MODULE_CALL macro to handle #UD (and also #GP, which can theoretically still happen when TDX isn't actually enabled by the BIOS, i.e. due to BIOS bug).

Introduce two new TDX error codes for #UD and #GP respectively so the caller can distinguish. Also, Opportunistically put the new TDX error codes and the existing TDX_SEAMCALL_VMFAILINVALID into INTEL_TDX_HOST Kconfig option as they are only used when it is on.

As __seamcall() can potentially return multiple error codes, besides the actual SEAMCALL leaf function return code, also introduce a wrapper function seamcall() to convert the __seamcall() error code to the kernel error code, so the caller doesn't need to duplicate the code to check return value of __seamcall() and return kernel error code accordingly.

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

index 05fc89d9742a..d688228f3151 100644
@@ -8,6 +8,10 @@
 #include <asm/ptrace.h>
 #include <asm/shared/tdx.h>
 
+#ifdef CONFIG_INTEL_TDX_HOST
+
+#include <asm/trapnr.h>
+
 /*
  * SW-defined error codes.
  *

@@ -18,6 +22,11 @@
 #define TDX_SW_ERROR			(TDX_ERROR | GENMASK_ULL(47, 40))
 #define TDX_SEAMCALL_VMFAILINVALID	(TDX_SW_ERROR | _UL(0xFFFF0000))
 
+#define TDX_SEAMCALL_GP			(TDX_SW_ERROR | X86_TRAP_GP)
+#define TDX_SEAMCALL_UD			(TDX_SW_ERROR | X86_TRAP_UD)
+
+#endif
+
 #ifndef __ASSEMBLY__
 
 /*

arch/x86/virt/vmx/tdx/Makefile +1 -1

index 93ca8b73e1f1..38d534f2c113 100644
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y += tdx.o
+obj-y += tdx.o seamcall.o

arch/x86/virt/vmx/tdx/seamcall.S new file +52 -0

new file mode 100644
index 000000000000..f81be6b9c133
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/linkage.h>
+#include <asm/frame.h>
+
+#include "tdxcall.S"
+
+/*
+ * __seamcall() - Host-side interface functions to SEAM software module
+ *		  (the P-SEAMLDR or the TDX module).
+ *
+ * Transform function call register arguments into the SEAMCALL register
+ * ABI.  Return TDX_SEAMCALL_VMFAILINVALID if the SEAMCALL itself fails,
+ * or the completion status of the SEAMCALL leaf function.  Additional
+ * output operands are saved in @out (if it is provided by the caller).
+ *
+ *-------------------------------------------------------------------------
+ * SEAMCALL ABI:
+ *-------------------------------------------------------------------------
+ * Input Registers:
+ *
+ * RAX                 - SEAMCALL Leaf number.
+ * RCX,RDX,R8-R9       - SEAMCALL Leaf specific input registers.
+ *
+ * Output Registers:
+ *
+ * RAX                 - SEAMCALL completion status code.
+ * RCX,RDX,R8-R11      - SEAMCALL Leaf specific output registers.
+ *
+ *-------------------------------------------------------------------------
+ *
+ * __seamcall() function ABI:
+ *
+ * @fn  (RDI)          - SEAMCALL Leaf number, moved to RAX
+ * @rcx (RSI)          - Input parameter 1, moved to RCX
+ * @rdx (RDX)          - Input parameter 2, moved to RDX
+ * @r8  (RCX)          - Input parameter 3, moved to R8
+ * @r9  (R8)           - Input parameter 4, moved to R9
+ *
+ * @out (R9)           - struct tdx_module_output pointer
+ *			 stored temporarily in R12 (not
+ *			 used by the P-SEAMLDR or the TDX
+ *			 module). It can be NULL.
+ *
+ * Return (via RAX) the completion status of the SEAMCALL, or
+ * TDX_SEAMCALL_VMFAILINVALID.
+ */
+SYM_FUNC_START(__seamcall)
+	FRAME_BEGIN
+	TDX_MODULE_CALL host=1
+	FRAME_END
+	RET
+SYM_FUNC_END(__seamcall)

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

index 28c187b8726f..b06c1a2bc9cb 100644
@@ -124,6 +124,48 @@ bool platform_tdx_enabled(void)
 	return !!tdx_keyid_num;
 }
 
+/*
+ * 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;
+
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+
+	/* Save SEAMCALL return code if caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+

Quoted 4 lines · Kai Huang

TDX introduces a new CPU mode: Secure Arbitration Mode (SEAM). This mode runs only the TDX module itself or other code to load the TDX module.

Quoted 8 lines

The host kernel communicates with SEAM software via a new SEAMCALL instruction. This is conceptually similar to a guest->host hypercall, except it is made from the host to SEAM software instead.

The TDX module defines a set of SEAMCALL leaf functions to allow the host to initialize it, and to create and run protected VMs. SEAMCALL leaf functions use an ABI different from the x86-64 system-v ABI. Instead, they share the same ABI with the TDCALL leaf functions.

I may have suggested this along the way, but the mention of the sysv ABI is just confusing here. This is enough for a changelog:

The TDX module establishes a new SEAMCALL ABI which allows the host to initialize the module and to and to manage VMs.

Kill the rest.

Quoted 4 lines

Implement a function __seamcall() to allow the host to make SEAMCALL to SEAM software using the TDX_MODULE_CALL macro which is the common assembly for both SEAMCALL and TDCALL.

In general, I dislike mentioning function names in changelogs. Keep this high-level, like:

Add infrastructure to make SEAMCALLs. The SEAMCALL ABI is very similar to the TDCALL ABI and leverages much TDCALL infrastructure.

Quoted 5 lines

SEAMCALL instruction causes #GP when SEAMRR isn't enabled, and #UD when CPU is not in VMX operation. The current TDX_MODULE_CALL macro doesn't handle any of them. There's no way to check whether the CPU is in VMX operation or not.

What is SEAMRR?

Why even mention this behavior in the changelog. Is this a problem? Does it have a solution?

Quoted 7 lines

Initializing the TDX module is done at runtime on demand, and it depends on the caller to ensure CPU is in VMX operation before making SEAMCALL. To avoid getting Oops when the caller mistakenly tries to initialize the TDX module when CPU is not in VMX operation, extend the TDX_MODULE_CALL macro to handle #UD (and also #GP, which can theoretically still happen when TDX isn't actually enabled by the BIOS, i.e. due to BIOS bug).

I'm not completely sure this is worth it. If the BIOS lies, we oops. There are lots of ways that the BIOS lying can make the kernel oops. What's one more?

Quoted 11 lines

Introduce two new TDX error codes for #UD and #GP respectively so the caller can distinguish. Also, Opportunistically put the new TDX error codes and the existing TDX_SEAMCALL_VMFAILINVALID into INTEL_TDX_HOST Kconfig option as they are only used when it is on.

As __seamcall() can potentially return multiple error codes, besides the actual SEAMCALL leaf function return code, also introduce a wrapper function seamcall() to convert the __seamcall() error code to the kernel error code, so the caller doesn't need to duplicate the code to check return value of __seamcall() and return kernel error code accordingly.

Quoted 18 lines

arch/x86/virt/vmx/tdx/tdx.c +18 -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;
+
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+
+	/* Save SEAMCALL return code if caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 154

Quoted 10 lines

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

+	/* SEAMCALL was successful */
+	if (!sret)
+		return 0;
+
+	switch (sret) {
+	case TDX_SEAMCALL_GP:
+		/*
+		 * platform_tdx_enabled() is checked to be true
+		 * before making any SEAMCALL.
+		 */

This doesn't make any sense. "platform_tdx_enabled() is checked"???

Do you mean that it *should* be checked and probably wasn't which is what caused the error?

arch/x86/virt/vmx/tdx/tdx.c bool platform_tdx_enabled(void) · line 159

Quoted 6 lines

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

+		WARN_ON_ONCE(1);
+		fallthrough;
+	case TDX_SEAMCALL_VMFAILINVALID:
+		/* Return -ENODEV if the TDX module is not loaded. */
+		return -ENODEV;

Pro tip: you don't need to rewrite code in comments. If the code literally says, "return -ENODEV", there is very little value in writing virtually identical bytes "Return -ENODEV" in the comment.

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

+/*
+ * Data structure to make SEAMCALL on multiple CPUs concurrently.
+ * @err is set to -EFAULT when SEAMCALL fails on any cpu.
+ */
+struct seamcall_ctx {
+	u64 fn;
+	u64 rcx;
+	u64 rdx;
+	u64 r8;
+	u64 r9;
+	atomic_t err;
+};

@@ -166,6 +180,25 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	}
 }

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

Quoted 8 lines · Kai Huang

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

+/*
+ * Call the SEAMCALL on all online CPUs concurrently.  Caller to check
+ * @sc->err to determine whether any SEAMCALL failed on any cpu.
+ */
+static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
+{
+	on_each_cpu(seamcall_smp_call_function, sc, true);
+}

Suppose the user has NOHZ_FULL configured, and is already running userspace that will terminate on interrupt (this is desired feature for NOHZ_FULL), guess how happy they'll be if someone, on another parition, manages to tickle this TDX gunk?

Quoted 14 lines · Peter Zijlstra

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

Quoted 8 lines · Kai Huang

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

+/*
+ * Call the SEAMCALL on all online CPUs concurrently.  Caller to check
+ * @sc->err to determine whether any SEAMCALL failed on any cpu.
+ */
+static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
+{
+	on_each_cpu(seamcall_smp_call_function, sc, true);
+}

Suppose the user has NOHZ_FULL configured, and is already running userspace that will terminate on interrupt (this is desired feature for NOHZ_FULL), guess how happy they'll be if someone, on another parition, manages to tickle this TDX gunk?

Yeah, they'll be none too happy.

But, what do we do?

There are technical solutions like detecting if NOHZ_FULL is in play and refusing to initialize TDX. There are also non-technical solutions like telling folks in the documentation that they better modprobe kvm early if they want to do TDX, or their NOHZ_FULL apps will pay.

We could also force the TDX module to be loaded early in boot before NOHZ_FULL is in play, but that would waste memory on TDX metadata even if TDX is never used.

How do NOHZ_FULL folks deal with late microcode updates, for example? Those are roughly equally disruptive to all CPUs.

Quoted 16 lines · Dave Hansen

Quoted 14 lines · Peter Zijlstra

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

Quoted 8 lines · Kai Huang

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

+/*
+ * Call the SEAMCALL on all online CPUs concurrently.  Caller to check
+ * @sc->err to determine whether any SEAMCALL failed on any cpu.
+ */
+static void seamcall_on_each_cpu(struct seamcall_ctx *sc)
+{
+	on_each_cpu(seamcall_smp_call_function, sc, true);
+}

Suppose the user has NOHZ_FULL configured, and is already running userspace that will terminate on interrupt (this is desired feature for NOHZ_FULL), guess how happy they'll be if someone, on another parition, manages to tickle this TDX gunk?

Quoted 3 lines

Yeah, they'll be none too happy.

But, what do we do?

Not intialize TDX on busy NOHZ_FULL cpus and hard-limit the cpumask of all TDX using tasks.

Quoted 4 lines

There are technical solutions like detecting if NOHZ_FULL is in play and refusing to initialize TDX. There are also non-technical solutions like telling folks in the documentation that they better modprobe kvm early if they want to do TDX, or their NOHZ_FULL apps will pay.

Surely modprobe kvm isn't the point where TDX gets loaded? Because that's on boot for everybody due to all the auto-probing nonsense.

I was expecting TDX to not get initialized until the first TDX using KVM instance is created. Am I wrong?

https://lore.kernel.org/lkml/CAAhR5DFrwP+5K8MOxz5YK7jYShhaK4A+2h1Pi31U_9+Z+cz-0A@mail.gmail.com/T/

Quoted 3 lines

We could also force the TDX module to be loaded early in boot before NOHZ_FULL is in play, but that would waste memory on TDX metadata even if TDX is never used.

I'm thikning it makes sense to have a tdx={off,on-demand,force} toggle anyway.

Quoted 2 lines

How do NOHZ_FULL folks deal with late microcode updates, for example? Those are roughly equally disruptive to all CPUs.

I imagine they don't do that -- in fact I would recommend we make the whole late loading thing mutually exclusive with nohz_full; can't have both.

Quoted 2 lines

Not intialize TDX on busy NOHZ_FULL cpus and hard-limit the cpumask of all TDX using tasks.

I don't think that works. As I mentioned to Thomas elsewhere, you don't just need to initialize TDX on the CPUs where it is used. Before the module will start working you need to initialize it on *all* the CPUs it knows about. The module itself has a little counter where it tracks this and will refuse to start being useful until it gets called thoroughly enough.

Quoted 9 lines · Dave Hansen

Quoted 2 lines

Not intialize TDX on busy NOHZ_FULL cpus and hard-limit the cpumask of all TDX using tasks.

I don't think that works. As I mentioned to Thomas elsewhere, you don't just need to initialize TDX on the CPUs where it is used. Before the module will start working you need to initialize it on *all* the CPUs it knows about. The module itself has a little counter where it tracks this and will refuse to start being useful until it gets called thoroughly enough.

That's bloody terrible, that is. How are we going to make that work with the SMT mitigation crud that forces the SMT sibilng offline?

Then the counters don't match and TDX won't work.

Can we get this limitiation removed and simply let the module throw a wobbly (error) when someone tries and use TDX without that logical CPU having been properly initialized?

Quoted 8 lines · Dave Hansen

Quoted 2 lines

Not intialize TDX on busy NOHZ_FULL cpus and hard-limit the cpumask of all TDX using tasks.

I don't think that works. As I mentioned to Thomas elsewhere, you don't just need to initialize TDX on the CPUs where it is used. Before the module will start working you need to initialize it on *all* the CPUs it knows about. The module itself has a little counter where it tracks this and will refuse to start being useful until it gets called thoroughly enough.

That's bloody terrible, that is. How are we going to make that work with the SMT mitigation crud that forces the SMT sibilng offline?

Then the counters don't match and TDX won't work.

Can we get this limitiation removed and simply let the module throw a wobbly (error) when someone tries and use TDX without that logical CPU having been properly initialized?

Quoted 3 lines · Peter Zijlstra

Can we get this limitiation removed and simply let the module throw a wobbly (error) when someone tries and use TDX without that logical CPU having been properly initialized?

It sounds like we can at least punt the limitation away from the OS's purview.

There's actually a multi-step process to get a "real" TDX module loaded.
There's a fancy ACM (Authenticated Code Module) that's invoked via
GETSEC[ENTERACCS] and an intermediate module loader. That dance used to be done in the kernel, but we talked the BIOS guys into doing it instead.

I believe these per-logical-CPU checks _can_ also be punted out of the TDX module itself and delegated to one of these earlier module loading phases that the BIOS drives.

I'm still a _bit_ skeptical that the checks are needed in the first place. But, as long as they're hidden from the OS, I don't see a need to be too cranky about it.

In the end, we could just plain stop doing the TDH.SYS.LP.INIT code in the kernel.

Unless someone screams, I'll ask the BIOS and TDX module folks to look into this.