TDX host kernel support

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 by the KVM subsystem when KVM wants to use TDX.

Add a new early_initcall(tdx_init) to detect the TDX by detecting TDX private KeyIDs. Also add a function to report whether TDX is enabled by the BIOS. Similar to AMD SME, kexec() will use it to determine whether cache flush is needed.

The TDX module itself requires one TDX KeyID as the 'TDX global KeyID' to protect its metadata. Each TDX guest also needs a TDX KeyID for its own protection. Just use the first TDX KeyID as the global KeyID and leave the rest for TDX guests. If no TDX KeyID is left for TDX guests, disable TDX as initializing the TDX module alone is useless.

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 uses TDX. Make the new config option depend on KVM_INTEL.

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

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

---

v10 -> v11 (David):
- "host kernel" -> "the host kernel"
- "protected VM" -> "confidential VM".
- Moved setting tdx_global_keyid to the end of tdx_init().

v9 -> v10:
- No change.

v8 -> v9:
- Moved MSR macro from local tdx.h to <asm/msr-index.h> (Dave).
- Moved reserving the TDX global KeyID from later patch to here.
- Changed 'tdx_keyid_start' and 'nr_tdx_keyids' to 'tdx_guest_keyid_start' and 'tdx_nr_guest_keyids' to represent KeyIDs can be used by guest. (Dave)
- Slight changelog update according to above changes.

v7 -> v8: (address Dave's comments)
- Improved changelog:
- "KVM user" -> "The TDX module will be initialized by KVM when ..."
- Changed "tdx_int" part to "Just say what this patch is doing"
- Fixed the last sentence of "kexec()" paragraph
- detect_tdx() -> record_keyid_partitioning()
- Improved how to calculate tdx_keyid_start.
- tdx_keyid_num -> nr_tdx_keyids.
- Improved dmesg printing.
- Add comment to clear_tdx().

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/msr-index.h |  3 ++
 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      | 92 ++++++++++++++++++++++++++++++++
 8 files changed, 122 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

arch/x86/Kconfig +12 -0

index 53bab123a8ee..191587f75810 100644
@@ -1952,6 +1952,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 the host kernel to run confidential VMs.
+
+	  If unsure, say N.
+
 config EFI
 	bool "EFI runtime service support"
 	depends on ACPI

arch/x86/Makefile +2 -0

index b39975977c03..ec0e71d8fa30 100644
@@ -252,6 +252,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/msr-index.h +3 -0

index 3aedae61af4f..6d8f15b1552c 100644
@@ -523,6 +523,9 @@
 #define MSR_RELOAD_PMC0			0x000014c1
 #define MSR_RELOAD_FIXED_CTR0		0x00001309
 
+/* KeyID partitioning between MKTME and TDX */
+#define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
+
 /*
  * AMD64 MSRs. Not complete. See the architecture manual for a more
  * complete list.

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

index 25fd6070dc0b..4dfe2e794411 100644
@@ -94,5 +94,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 +61 -0

new file mode 100644
index 000000000000..2d91e7120c90
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2023 Intel Corporation.
+ *
+ * Intel Trusted Domain Extensions (TDX) support
+ */
+
+#define pr_fmt(fmt)	"tdx: " fmt
+
+#include <linux/types.h>
+#include <linux/cache.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <asm/msr-index.h>
+#include <asm/msr.h>
+#include <asm/tdx.h>
+
+static u32 tdx_global_keyid __ro_after_init;
+static u32 tdx_guest_keyid_start __ro_after_init;
+static u32 tdx_nr_guest_keyids __ro_after_init;
+
+static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
+					    u32 *nr_tdx_keyids)
+{
+	u32 _nr_mktme_keyids, _tdx_keyid_start, _nr_tdx_keyids;
+	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, &_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 = _nr_mktme_keyids + 1;
+
+	*tdx_keyid_start = _tdx_keyid_start;
+	*nr_tdx_keyids = _nr_tdx_keyids;
+
+	return 0;
+}
+
+static int __init tdx_init(void)
+{
+	u32 tdx_keyid_start, nr_tdx_keyids;
+	int err;
+
+	err = record_keyid_partitioning(&tdx_keyid_start, &nr_tdx_keyids);
+	if (err)
+		return err;
+
+	pr_info("BIOS enabled: private KeyID range [%u, %u)\n",
+			tdx_keyid_start, tdx_keyid_start + nr_tdx_keyids);

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

+	}
+
+	/*
+	 * Just use the first TDX KeyID as the 'global KeyID' and
+	 * leave the rest for TDX guests.
+	 */
+	tdx_global_keyid = tdx_keyid_start;
+	tdx_guest_keyid_start = ++tdx_keyid_start;
+	tdx_nr_guest_keyids = --nr_tdx_keyids;
+
+	return 0;
+no_tdx:
+	return -ENODEV;
+}
+early_initcall(tdx_init);
+
+/* Return whether the BIOS has enabled TDX */
+bool platform_tdx_enabled(void)
+{
+	return !!tdx_global_keyid;
+}

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

Quoted 4 lines

(quoted diff) +2 -0

+	if (nr_tdx_keyids < 2) {
+		pr_info("initialization failed: too few private KeyIDs

available.\n");

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

+		goto no_tdx;

I think you can return -ENODEV directly here. Maybe this goto is added to adapt to next
patches. But for this patch, I don't think you need it.

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

new file mode 100644
index 000000000000..2d91e7120c90
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2023 Intel Corporation.
+ *
+ * Intel Trusted Domain Extensions (TDX) support
+ */
+
+#define pr_fmt(fmt)	"tdx: " fmt
+
+#include <linux/types.h>
+#include <linux/cache.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <asm/msr-index.h>
+#include <asm/msr.h>
+#include <asm/tdx.h>
+
+static u32 tdx_global_keyid __ro_after_init;
+static u32 tdx_guest_keyid_start __ro_after_init;
+static u32 tdx_nr_guest_keyids __ro_after_init;
+
+static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
+					    u32 *nr_tdx_keyids)
+{
+	u32 _nr_mktme_keyids, _tdx_keyid_start, _nr_tdx_keyids;
+	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, &_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 = _nr_mktme_keyids + 1;
+
+	*tdx_keyid_start = _tdx_keyid_start;
+	*nr_tdx_keyids = _nr_tdx_keyids;
+
+	return 0;
+}
+
+static int __init tdx_init(void)
+{
+	u32 tdx_keyid_start, nr_tdx_keyids;
+	int err;
+
+	err = record_keyid_partitioning(&tdx_keyid_start, &nr_tdx_keyids);
+	if (err)
+		return err;
+
+	pr_info("BIOS enabled: private KeyID range [%u, %u)\n",

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 by the KVM subsystem when KVM wants to use TDX.

Add a new early_initcall(tdx_init) to detect the TDX by detecting TDX private KeyIDs. Also add a function to report whether TDX is enabled by the BIOS. Similar to AMD SME, kexec() will use it to determine whether cache flush is needed.

The TDX module itself requires one TDX KeyID as the 'TDX global KeyID' to protect its metadata. Each TDX guest also needs a TDX KeyID for its own protection. Just use the first TDX KeyID as the global KeyID and leave the rest for TDX guests. If no TDX KeyID is left for TDX guests, disable TDX as initializing the TDX module alone is useless.

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 uses TDX. Make the new config option depend on KVM_INTEL.

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

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

---

v10 -> v11 (David):
- "host kernel" -> "the host kernel"
- "protected VM" -> "confidential VM".
- Moved setting tdx_global_keyid to the end of tdx_init().

v9 -> v10:
- No change.

v8 -> v9:
- Moved MSR macro from local tdx.h to <asm/msr-index.h> (Dave).
- Moved reserving the TDX global KeyID from later patch to here.
- Changed 'tdx_keyid_start' and 'nr_tdx_keyids' to 'tdx_guest_keyid_start' and 'tdx_nr_guest_keyids' to represent KeyIDs can be used by guest. (Dave)
- Slight changelog update according to above changes.

v7 -> v8: (address Dave's comments)
- Improved changelog:
- "KVM user" -> "The TDX module will be initialized by KVM when ..."
- Changed "tdx_int" part to "Just say what this patch is doing"
- Fixed the last sentence of "kexec()" paragraph
- detect_tdx() -> record_keyid_partitioning()
- Improved how to calculate tdx_keyid_start.
- tdx_keyid_num -> nr_tdx_keyids.
- Improved dmesg printing.
- Add comment to clear_tdx().

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/msr-index.h |  3 ++
  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      | 92 ++++++++++++++++++++++++++++++++
  8 files changed, 122 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

arch/x86/Kconfig +12 -0

index 53bab123a8ee..191587f75810 100644
@@ -1952,6 +1952,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 the host kernel to run confidential VMs.
+
+	  If unsure, say N.
+
  config EFI
  	bool "EFI runtime service support"
  	depends on ACPI

arch/x86/Makefile +2 -0

index b39975977c03..ec0e71d8fa30 100644
@@ -252,6 +252,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/msr-index.h +3 -0

index 3aedae61af4f..6d8f15b1552c 100644
@@ -523,6 +523,9 @@
  #define MSR_RELOAD_PMC0			0x000014c1
  #define MSR_RELOAD_FIXED_CTR0		0x00001309
 
+/* KeyID partitioning between MKTME and TDX */
+#define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
+
  /*
   * AMD64 MSRs. Not complete. See the architecture manual for a more
   * complete list.

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

index 25fd6070dc0b..4dfe2e794411 100644
@@ -94,5 +94,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 +70 -0

new file mode 100644
index 000000000000..2d91e7120c90
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2023 Intel Corporation.
+ *
+ * Intel Trusted Domain Extensions (TDX) support
+ */
+
+#define pr_fmt(fmt)	"tdx: " fmt
+
+#include <linux/types.h>
+#include <linux/cache.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/printk.h>
+#include <asm/msr-index.h>
+#include <asm/msr.h>
+#include <asm/tdx.h>
+
+static u32 tdx_global_keyid __ro_after_init;
+static u32 tdx_guest_keyid_start __ro_after_init;
+static u32 tdx_nr_guest_keyids __ro_after_init;
+
+static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
+					    u32 *nr_tdx_keyids)
+{
+	u32 _nr_mktme_keyids, _tdx_keyid_start, _nr_tdx_keyids;
+	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, &_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 = _nr_mktme_keyids + 1;
+
+	*tdx_keyid_start = _tdx_keyid_start;
+	*nr_tdx_keyids = _nr_tdx_keyids;
+
+	return 0;
+}
+
+static int __init tdx_init(void)
+{
+	u32 tdx_keyid_start, nr_tdx_keyids;
+	int err;
+
+	err = record_keyid_partitioning(&tdx_keyid_start, &nr_tdx_keyids);
+	if (err)
+		return err;
+
+	pr_info("BIOS enabled: private KeyID range [%u, %u)\n",
+			tdx_keyid_start, tdx_keyid_start + nr_tdx_keyids);
+
+	/*
+	 * The TDX module itself requires one 'global KeyID' to protect
+	 * its metadata.  If there's only one TDX KeyID, there won't be
+	 * any left for TDX guests thus there's no point to enable TDX
+	 * at all.
+	 */
+	if (nr_tdx_keyids < 2) {
+		pr_info("initialization failed: too few private KeyIDs available.\n");

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

Quoted 7 lines

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

+	/*
+	 * Just use the first TDX KeyID as the 'global KeyID' and
+	 * leave the rest for TDX guests.
+	 */
+	tdx_global_keyid = tdx_keyid_start;
+	tdx_guest_keyid_start = ++tdx_keyid_start;
+	tdx_nr_guest_keyids = --nr_tdx_keyids;

tdx_guest_keyid_start = tdx_keyid_start + 1;
tdx_nr_guest_keyids = nr_tdx_keyids - 1;

Easier to get, because the modified values are unused.

I'd probably avoid the "tdx" terminology in the local variables ("keid_start", "nr_keyids") to give a better hint what the global variables are (tdx_*), but just a personal preference.

Apart from that,

Reviewed-by David Hildenbrand <[email protected]>

TDX capable platforms are locked to X2APIC mode and cannot fall back to the legacy xAPIC mode when TDX is enabled by the BIOS. TDX host support requires x2APIC. Make INTEL_TDX_HOST depend on X86_X2APIC.

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

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

Reviewed-by Dave Hansen <[email protected]>

Reviewed-by David Hildenbrand <[email protected]>

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 around machine check handler and kexec(), etc. 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]>

---

v10 -> v11:
- New patch

---
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/kernel/cpu/intel.c        | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

arch/x86/include/asm/cpufeatures.h +1 -0

index cb8ca46213be..dc8701f8d88b 100644
@@ -483,5 +483,6 @@
 #define X86_BUG_RETBLEED		X86_BUG(27) /* CPU is affected by RETBleed */
 #define X86_BUG_EIBRS_PBRSB		X86_BUG(28) /* EIBRS is vulnerable to Post Barrier RSB Predictions */
 #define X86_BUG_SMT_RSB			X86_BUG(29) /* CPU is vulnerable to Cross-Thread Return Address Predictions */
+#define X86_BUG_TDX_PW_MCE		X86_BUG(30) /* CPU may incur #MC if non-TD software does partial write to TDX private memory */
 
 #endif /* _ASM_X86_CPUFEATURES_H */

arch/x86/kernel/cpu/intel.c +11 -0

index 1c4639588ff9..251b333e53d2 100644
@@ -1552,3 +1552,24 @@ u8 get_this_hybrid_cpu_type(void)
 
 	return cpuid_eax(0x0000001a) >> X86_HYBRID_CPU_TYPE_ID_SHIFT;
 }
+
+/*
+ * These CPUs have an erratum.  A partial write from non-TD
+ * software (e.g. via MOVNTI variants or UC/WC mapping) to TDX
+ * private memory poisons that memory, and a subsequent read of
+ * that memory triggers #MC.
+ */
+static const struct x86_cpu_id tdx_pw_mce_cpu_ids[] __initconst = {
+	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, NULL),
+	{ }

Quoted 75 lines · Kai Huang

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 around machine check handler and kexec(), etc. 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]>

---

v10 -> v11:
- New patch

---
 arch/x86/include/asm/cpufeatures.h |  1 +
 arch/x86/kernel/cpu/intel.c        | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

arch/x86/include/asm/cpufeatures.h +1 -0

index cb8ca46213be..dc8701f8d88b 100644
@@ -483,5 +483,6 @@
 #define X86_BUG_RETBLEED		X86_BUG(27) /* CPU is affected by RETBleed */
 #define X86_BUG_EIBRS_PBRSB		X86_BUG(28) /* EIBRS is vulnerable to Post Barrier RSB Predictions */
 #define X86_BUG_SMT_RSB			X86_BUG(29) /* CPU is vulnerable to Cross-Thread Return Address Predictions */
+#define X86_BUG_TDX_PW_MCE		X86_BUG(30) /* CPU may incur #MC if non-TD software does partial write to TDX private memory */
 
 #endif /* _ASM_X86_CPUFEATURES_H */

arch/x86/kernel/cpu/intel.c +16 -0

index 1c4639588ff9..251b333e53d2 100644
@@ -1552,3 +1552,24 @@ u8 get_this_hybrid_cpu_type(void)
 
 	return cpuid_eax(0x0000001a) >> X86_HYBRID_CPU_TYPE_ID_SHIFT;
 }
+
+/*
+ * These CPUs have an erratum.  A partial write from non-TD
+ * software (e.g. via MOVNTI variants or UC/WC mapping) to TDX
+ * private memory poisons that memory, and a subsequent read of
+ * that memory triggers #MC.
+ */
+static const struct x86_cpu_id tdx_pw_mce_cpu_ids[] __initconst = {
+	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, NULL),
+	{ }
+};
+
+static int __init tdx_erratum_detect(void)
+{
+	if (x86_match_cpu(tdx_pw_mce_cpu_ids))

arch/x86/kernel/cpu/intel.c u8 get_this_hybrid_cpu_type(void) · line 1575

Quoted 5 lines

arch/x86/kernel/cpu/intel.c +5 -0

+		setup_force_cpu_bug(X86_BUG_TDX_PW_MCE);
+
+	return 0;
+}
+early_initcall(tdx_erratum_detect);

Initcall? Don't we already have a codepath to call it directly? Maybe cpu_set_bug_bits()?

So it seems there's no hard rule that all bugs need to be done in cpu_set_bug_bits().

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.

Quoted 5 lines · Kai Huang

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.

At the risk of patting myself on the back by acking a changelog that I wrote 95% of:

Reviewed-by Dave Hansen <[email protected]>

Quoted 7 lines · Dave

Quoted 5 lines · Kai Huang

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.

Quoted 5 lines

At the risk of patting myself on the back by acking a changelog that I wrote 95% of:

Reviewed-by Dave Hansen <[email protected]>

Thanks!

Please let me know for any comments?

Quoted 1 line · Kai

Please let me know for any comments?

Can you please go look at where most of the X86_BUG_* bits are set? Can you set yours near one of the existing sites instead of plopping a new one into the code?

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 around machine check handler and kexec(), etc. 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]>

---

v10 -> v11:
- New patch

---
  arch/x86/include/asm/cpufeatures.h |  1 +
  arch/x86/kernel/cpu/intel.c        | 21 +++++++++++++++++++++
  2 files changed, 22 insertions(+)

arch/x86/include/asm/cpufeatures.h +1 -0

index cb8ca46213be..dc8701f8d88b 100644
@@ -483,5 +483,6 @@
  #define X86_BUG_RETBLEED		X86_BUG(27) /* CPU is affected by RETBleed */
  #define X86_BUG_EIBRS_PBRSB		X86_BUG(28) /* EIBRS is vulnerable to Post Barrier RSB Predictions */
  #define X86_BUG_SMT_RSB			X86_BUG(29) /* CPU is vulnerable to Cross-Thread Return Address Predictions */
+#define X86_BUG_TDX_PW_MCE		X86_BUG(30) /* CPU may incur #MC if non-TD software does partial write to TDX private memory */
 
  #endif /* _ASM_X86_CPUFEATURES_H */

arch/x86/kernel/cpu/intel.c +21 -0

index 1c4639588ff9..251b333e53d2 100644
@@ -1552,3 +1552,24 @@ u8 get_this_hybrid_cpu_type(void)
 
  	return cpuid_eax(0x0000001a) >> X86_HYBRID_CPU_TYPE_ID_SHIFT;
  }
+
+/*
+ * These CPUs have an erratum.  A partial write from non-TD
+ * software (e.g. via MOVNTI variants or UC/WC mapping) to TDX
+ * private memory poisons that memory, and a subsequent read of
+ * that memory triggers #MC.
+ */
+static const struct x86_cpu_id tdx_pw_mce_cpu_ids[] __initconst = {
+	X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(EMERALDRAPIDS_X, NULL),
+	{ }
+};
+
+static int __init tdx_erratum_detect(void)
+{
+	if (x86_match_cpu(tdx_pw_mce_cpu_ids))
+		setup_force_cpu_bug(X86_BUG_TDX_PW_MCE);
+
+	return 0;
+}
+early_initcall(tdx_erratum_detect);

Quoted 12 lines · Kai Huang

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.

Quoted 4 lines

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.

So, ordinary writes to TD private memory are not a problem?

I thought
one motivation for the unmapped-guest-memory discussion was to prevent host (userspace) writes to such memory because it would trigger a MC and eventually crash the host.

I recall that this would happen easily (not just in some weird "partial" case and that the spec would allow for it)

1) Does that, in general, not happen anymore (was the hardware fixed?)?

2) Will new hardware prevent/"fix" that completely (was the spec updated?)?

So, ordinary writes to TD private memory are not a problem? I thought one motivation for the unmapped-guest-memory discussion was to prevent host (userspace) writes to such memory because it would trigger a MC and eventually crash the host.

Quoted 4 lines · David Hildenbrand

So, ordinary writes to TD private memory are not a problem? I thought one motivation for the unmapped-guest-memory discussion was to prevent host (userspace) writes to such memory because it would trigger a MC and eventually crash the host.

Those are two different problems.

Problem #1 (this patch): The host encounters poison when going about its normal business accessing normal memory. This happens when something in the host accidentally clobbers some TDX memory and *then* reads it. Only occurs with partial writes.

Problem #2 (addressed with unmapping): Host *userspace* intentionally and maliciously clobbers some TDX memory and then the TDX module or a TDX guest can't run because the memory integrity checks (checksum or TD bit) fail. This can also take the system down because #MC's are nasty.

Host userspace unmapping doesn't prevent problem #1 because it's the kernel who screwed up with the _kernel_ mapping.

Quoted 18 lines · Dave Hansen

Quoted 4 lines · David Hildenbrand

So, ordinary writes to TD private memory are not a problem? I thought one motivation for the unmapped-guest-memory discussion was to prevent host (userspace) writes to such memory because it would trigger a MC and eventually crash the host.

Those are two different problems.

Problem #1 (this patch): The host encounters poison when going about its normal business accessing normal memory.  This happens when something in the host accidentally clobbers some TDX memory and *then* reads it. Only occurs with partial writes.

Problem #2 (addressed with unmapping): Host *userspace* intentionally and maliciously clobbers some TDX memory and then the TDX module or a TDX guest can't run because the memory integrity checks (checksum or TD bit) fail.  This can also take the system down because #MC's are nasty.

Quoted 2 lines

Host userspace unmapping doesn't prevent problem #1 because it's the kernel who screwed up with the _kernel_ mapping.

Ahh, thanks for verifying. I was hoping that problem #2 would get fixed in HW as well (and treated like a BUG).

Because problem #2 also sounds like something that directly violates the first paragraph of this patch description "violations of this integrity protection are supposed to only affect TDX operations and are never supposed to affect the host kernel itself."

So I would expect the TDX guest to fail hard, but not other TDX guests (or the host kernel).

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 establishes a new SEAMCALL ABI which allows the host to initialize the module and to manage VMs.

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

SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD when CPU is not in VMX operation. Currently, only KVM code mocks with VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

Extend the TDX_MODULE_CALL macro to handle #UD and #GP to return error codes. Introduce two new TDX error codes for them respectively so the caller can distinguish.

Also add a wrapper function of SEAMCALL to convert SEAMCALL error code to the kernel error code, and print out SEAMCALL error code to help the user to understand what went wrong.

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

---

v10 -> v11:
- No update

v9 -> v10:
- Make the TDX_SEAMCALL_{GP|UD} error codes unconditional but doesn't define them when INTEL_TDX_HOST is enabled. (Dave)
- Slightly improved changelog to explain why add assembly code to handle #UD and #GP.

v8 -> v9:
- Changed patch title (Dave).
- Enhanced seamcall() to include the cpu id to the error message when SEAMCALL fails.

v7 -> v8:
- Improved changelog (Dave):
- Trim down some sentences (Dave).
- Removed __seamcall() and seamcall() function name and changed accordingly (Dave).
- Improved the sentence explaining why to handle #GP (Dave).
- Added code to print out error message in seamcall(), following the idea that tdx_enable() to return universal error and print out error message to make clear what's going wrong (Dave). Also mention this in changelog.

v6 -> v7:
- No change.

v5 -> v6:
- Added code to handle #UD and #GP (Dave).
- Moved the seamcall() wrapper function to this patch, and used a temporary __always_unused to avoid compile warning (Dave).

- v3 -> v5 (no feedback on v4):
- Explicitly tell TDX_SEAMCALL_VMFAILINVALID is returned if the SEAMCALL itself fails.
- Improve the changelog.

---
 arch/x86/include/asm/tdx.h       |  5 +++
 arch/x86/virt/vmx/tdx/Makefile   |  2 +-
 arch/x86/virt/vmx/tdx/seamcall.S | 52 +++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.c      | 56 ++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h      | 10 ++++++
 arch/x86/virt/vmx/tdx/tdxcall.S  | 19 +++++++++--
 6 files changed, 141 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/virt/vmx/tdx/seamcall.S
 create mode 100644 arch/x86/virt/vmx/tdx/tdx.h

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

index 4dfe2e794411..b489b5b9de5d 100644
@@ -8,6 +8,8 @@
 #include <asm/ptrace.h>
 #include <asm/shared/tdx.h>
 
+#include <asm/trapnr.h>
+
 /*
  * SW-defined error codes.
  *

@@ -18,6 +20,9 @@
 #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)
+
 #ifndef __ASSEMBLY__
 
 /* TDX supported page sizes from the TDX module ABI. */

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 +56 -0

index 2d91e7120c90..e82713dd5d54 100644
@@ -12,14 +12,70 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/printk.h>
+#include <linux/smp.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>
+#include "tdx.h"
 
 static u32 tdx_global_keyid __ro_after_init;
 static u32 tdx_guest_keyid_start __ro_after_init;
 static u32 tdx_nr_guest_keyids __ro_after_init;
 
+/*
+ * 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)
+{
+	int cpu, ret = 0;
+	u64 sret;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	/* SEAMCALL was successful */
+	if (!sret)
+		goto out;
+
+	switch (sret) {
+	case TDX_SEAMCALL_GP:
+		pr_err_once("[firmware bug]: TDX is not enabled by BIOS.\n");
+		ret = -ENODEV;
+		break;
+	case TDX_SEAMCALL_VMFAILINVALID:
+		pr_err_once("TDX module is not loaded.\n");
+		ret = -ENODEV;
+		break;
+	case TDX_SEAMCALL_UD:
+		pr_err_once("SEAMCALL failed: CPU %d is not in VMX operation.\n",
+				cpu);
+		ret = -EINVAL;
+		break;
+	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);
+		ret = -EIO;
+	}
+out:
+	put_cpu();
+	return ret;
+}
+
 static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
 					    u32 *nr_tdx_keyids)
 {

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

new file mode 100644
index 000000000000..48ad1a1ba737
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H
+
+#include <linux/types.h>
+
+struct tdx_module_output;
+u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+	       struct tdx_module_output *out);
+#endif

arch/x86/virt/vmx/tdx/tdxcall.S +17 -2

index 49a54356ae99..757b0c34be10 100644
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <asm/asm-offsets.h>
 #include <asm/tdx.h>
+#include <asm/asm.h>
 
 /*
  * TDCALL and SEAMCALL are supported in Binutils >= 2.36.

@@ -45,6 +46,7 @@
 	/* Leave input param 2 in RDX */
 
 	.if \host
+1:
 	seamcall
 	/*
 	 * SEAMCALL instruction is essentially a VMExit from VMX root

@@ -57,10 +59,23 @@
 	 * This value will never be used as actual SEAMCALL error code as
 	 * it is from the Reserved status code class.
 	 */
-	jnc .Lno_vmfailinvalid
+	jnc .Lseamcall_out
 	mov $TDX_SEAMCALL_VMFAILINVALID, %rax
-.Lno_vmfailinvalid:
+	jmp .Lseamcall_out
+2:
+	/*
+	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
+	 * the trap number.  Convert the trap number to the TDX error
+	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
+	 *
+	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
+	 * only accepts 32-bit immediate at most.
+	 */
+	mov $TDX_SW_ERROR, %r12
+	orq %r12, %rax
 
+	_ASM_EXTABLE_FAULT(1b, 2b)
+.Lseamcall_out:
 	.else
 	tdcall
 	.endif

Signature · 1 line

2.40.1

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 establishes a new SEAMCALL ABI which allows the host to initialize the module and to manage VMs.

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

SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD when CPU is not in VMX operation. Currently, only KVM code mocks with

VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

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

index 4dfe2e794411..b489b5b9de5d 100644
@@ -8,6 +8,8 @@
 #include <asm/ptrace.h>
 #include <asm/shared/tdx.h>
 
+#include <asm/trapnr.h>
+
 /*
  * SW-defined error codes.
  *

@@ -18,6 +20,9 @@
 #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)
+
 #ifndef __ASSEMBLY__
 
 /* TDX supported page sizes from the TDX module ABI. */

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 +45 -0

index 2d91e7120c90..e82713dd5d54 100644
@@ -12,14 +12,70 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/printk.h>
+#include <linux/smp.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>
+#include "tdx.h"
 
 static u32 tdx_global_keyid __ro_after_init;
 static u32 tdx_guest_keyid_start __ro_after_init;
 static u32 tdx_nr_guest_keyids __ro_after_init;
 
+/*
+ * 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)
+{
+	int cpu, ret = 0;
+	u64 sret;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+
+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	/* SEAMCALL was successful */
+	if (!sret)
+		goto out;
+
+	switch (sret) {
+	case TDX_SEAMCALL_GP:
+		pr_err_once("[firmware bug]: TDX is not enabled by BIOS.\n");
+		ret = -ENODEV;
+		break;
+	case TDX_SEAMCALL_VMFAILINVALID:
+		pr_err_once("TDX module is not loaded.\n");
+		ret = -ENODEV;
+		break;
+	case TDX_SEAMCALL_UD:
+		pr_err_once("SEAMCALL failed: CPU %d is not in VMX operation.\n",
+				cpu);
+		ret = -EINVAL;
+		break;
+	default:
+		pr_err_once("SEAMCALL failed: CPU %d: leaf %llu, error 0x%llx.\n",
+				cpu, fn, sret);

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

new file mode 100644
index 000000000000..48ad1a1ba737
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H
+
+#include <linux/types.h>
+
+struct tdx_module_output;
+u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+	       struct tdx_module_output *out);
+#endif

arch/x86/virt/vmx/tdx/tdxcall.S +5 -2

index 49a54356ae99..757b0c34be10 100644
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <asm/asm-offsets.h>
 #include <asm/tdx.h>
+#include <asm/asm.h>
 
 /*
  * TDCALL and SEAMCALL are supported in Binutils >= 2.36.

@@ -45,6 +46,7 @@
 	/* Leave input param 2 in RDX */
 
 	.if \host
+1:
 	seamcall
 	/*
 	 * SEAMCALL instruction is essentially a VMExit from VMX root

@@ -57,10 +59,23 @@
 	 * This value will never be used as actual SEAMCALL error code as
 	 * it is from the Reserved status code class.
 	 */
-	jnc .Lno_vmfailinvalid
+	jnc .Lseamcall_out
 	mov $TDX_SEAMCALL_VMFAILINVALID, %rax
-.Lno_vmfailinvalid:
+	jmp .Lseamcall_out
+2:

Quoted 10 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.

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 establishes a new SEAMCALL ABI which allows the host to initialize the module and to manage VMs.

Quoted 5 lines

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

SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD when CPU is not in VMX operation. Currently, only KVM code mocks with

"mocks"? Did you mean "mucks"?

Quoted 9 lines

VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

Quoted 12 lines

Quoted 9 lines

VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

Quoted 2 lines

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

TDX KVM + reboot can hit #UD. On reboot, VMX is disabled (VMXOFF) via syscore.shutdown callback. However, guest TD can be still running to issue SEAMCALL resulting in #UD.

Or we can postpone the change and make the TDX KVM patch series carry a patch for it.

Quoted 14 lines · Isaku Yamahata

Quoted 14 lines

Quoted 9 lines

VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

Quoted 6 lines

TDX KVM + reboot can hit #UD. On reboot, VMX is disabled (VMXOFF) via syscore.shutdown callback. However, guest TD can be still running to issue SEAMCALL resulting in #UD.

Or we can postpone the change and make the TDX KVM patch series carry a patch for it.

How does the existing KVM use of VMLAUNCH/VMRESUME avoid that problem?

Quoted 15 lines · Dave Hansen

Quoted 14 lines · Isaku Yamahata

Quoted 14 lines

Quoted 9 lines

VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

Quoted 8 lines

Quoted 6 lines

TDX KVM + reboot can hit #UD. On reboot, VMX is disabled (VMXOFF) via syscore.shutdown callback. However, guest TD can be still running to issue SEAMCALL resulting in #UD.

Or we can postpone the change and make the TDX KVM patch series carry a patch for it.

How does the existing KVM use of VMLAUNCH/VMRESUME avoid that problem?

extable. From arch/x86/kvm/vmx/vmenter.S

.Lvmresume:
vmresume
jmp .Lvmfail

.Lvmlaunch:
vmlaunch
jmp .Lvmfail

_ASM_EXTABLE(.Lvmresume, .Lfixup)
_ASM_EXTABLE(.Lvmlaunch, .Lfixup)

Quoted 23 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

Quoted 7 lines

Quoted 6 lines

TDX KVM + reboot can hit #UD. On reboot, VMX is disabled (VMXOFF) via syscore.shutdown callback. However, guest TD can be still running to issue SEAMCALL resulting in #UD.

Or we can postpone the change and make the TDX KVM patch series carry a patch for it.

How does the existing KVM use of VMLAUNCH/VMRESUME avoid that problem?

extable. From arch/x86/kvm/vmx/vmenter.S

.Lvmresume:
vmresume
jmp .Lvmfail

Quoted 6 lines

.Lvmlaunch:
vmlaunch
jmp .Lvmfail

_ASM_EXTABLE(.Lvmresume, .Lfixup)
_ASM_EXTABLE(.Lvmlaunch, .Lfixup)

More specifically, KVM eats faults on VMX and SVM instructions that occur after KVM forcefully disables VMX/SVM.

Quoted 23 lines · Sean Christopherson

Quoted 23 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

Quoted 7 lines

Quoted 6 lines

TDX KVM + reboot can hit #UD. On reboot, VMX is disabled (VMXOFF) via syscore.shutdown callback. However, guest TD can be still running to issue SEAMCALL resulting in #UD.

Or we can postpone the change and make the TDX KVM patch series carry a patch for it.

How does the existing KVM use of VMLAUNCH/VMRESUME avoid that problem?

extable. From arch/x86/kvm/vmx/vmenter.S

.Lvmresume:
vmresume
jmp .Lvmfail

Quoted 8 lines

Quoted 6 lines

.Lvmlaunch:
vmlaunch
jmp .Lvmfail

_ASM_EXTABLE(.Lvmresume, .Lfixup)
_ASM_EXTABLE(.Lvmlaunch, .Lfixup)

More specifically, KVM eats faults on VMX and SVM instructions that occur after KVM forcefully disables VMX/SVM.

<grumble> That's a *TOTALLY* different argument than the patch makes.

KVM is being a _bit_ nutty here, but I do respect it trying to honor the "-f". I have no objections to the SEAMCALL code being nutty in the same way.

Why do I get the feeling that code is being written without understanding _why_, despite this being v11?

How about I add below to the changelog?

"
The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made when TDX is enabled by the BIOS or when CPU isn't in VMX operation. For the former, the callers could check platform_tdx_enabled() first, although that doesn't rule out the buggy BIOS in which case the kernel could still get Oops. For the latter, the caller could check CR4.VMXE based on the fact that currently setting this bit and doing VMXON are done together when IRQ is disabled, although from hardware's perspective checking CR4.VMXE isn't enough.

However this could be problematic if SEAMCALL is called in the cases such as exception handler, NMI handler, etc, as disabling IRQ doesn't prevent any of them from happening.

To have a clean solution, just make the SEAMCALL always return error code by using EXTTABLE so the SEAMCALL can be safely called in any context. A later patch will need to use SEAMCALL in the machine check handler. There might be such use cases in the future too.
"

Quoted 10 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.

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 establishes a new SEAMCALL ABI which allows the host to initialize the module and to manage VMs.

Quoted 5 lines

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

SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD when CPU is not in VMX operation. Currently, only KVM code mocks with

"mocks"? Did you mean "mucks"?

Quoted 10 lines

VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

I'm not sure this is a great justification. If the BIOS is lying to the OS, we _should_ oops.

How else can this happen other than silly kernel bugs. It's OK to oops in the face of silly kernel bugs.

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdxcall.S +7 -0

+	/*
+	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
+	 * the trap number.  Convert the trap number to the TDX error
+	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
+	 *
+	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
+	 * only accepts 32-bit immediate at most.

arch/x86/virt/vmx/tdx/tdxcall.S line 75

Quoted 3 lines

arch/x86/virt/vmx/tdx/tdxcall.S +3 -0

+	 */
+	mov $TDX_SW_ERROR, %r12
+	orq %r12, %rax

I think the justification for doing the #UD/#GP handling is a bit weak. In the end, it gets us a nicer error message. Is that error message *REALLY* needed? Or is an oops OK in the very rare circumstance that the BIOS is totally buggy?

It's not just for the "BIOS buggy" case. The main purpose is to give an error message when the caller mistakenly calls tdx_enable().

Also, now the machine check handler improvement patch also calls SEAMCALL to get a given page's page type. It's totally legal that a machine check happens when the CPU isn't in VMX operation (e.g. KVM isn't loaded), and in fact we use the SEAMCALL return value to detect whether CPU is in VMX operation and handles such case accordingly.

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 establishes a new SEAMCALL ABI which allows the host to initialize the module and to manage VMs.

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

SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD when CPU is not in VMX operation. Currently, only KVM code mocks with VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

Extend the TDX_MODULE_CALL macro to handle #UD and #GP to return error codes. Introduce two new TDX error codes for them respectively so the caller can distinguish.

Also add a wrapper function of SEAMCALL to convert SEAMCALL error code to the kernel error code, and print out SEAMCALL error code to help the user to understand what went wrong.

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

---

arch/x86/virt/vmx/tdx/tdx.c +8 -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,

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

+
  static int __init record_keyid_partitioning(u32 *tdx_keyid_start,
  					    u32 *nr_tdx_keyids)
  {

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

new file mode 100644
index 000000000000..48ad1a1ba737
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H
+
+#include <linux/types.h>
+
+struct tdx_module_output;
+u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+	       struct tdx_module_output *out);
+#endif

arch/x86/virt/vmx/tdx/tdxcall.S +3 -1

index 49a54356ae99..757b0c34be10 100644
@@ -1,6 +1,7 @@
  /* SPDX-License-Identifier: GPL-2.0 */
  #include <asm/asm-offsets.h>
  #include <asm/tdx.h>
+#include <asm/asm.h>
 
  /*
   * TDCALL and SEAMCALL are supported in Binutils >= 2.36.

@@ -45,6 +46,7 @@
  	/* Leave input param 2 in RDX */
 
  	.if \host
+1:
  	seamcall
  	/*
  	 * SEAMCALL instruction is essentially a VMExit from VMX root

@@ -57,10 +59,23 @@
  	 * This value will never be used as actual SEAMCALL error code as
  	 * it is from the Reserved status code class.
  	 */
-	jnc .Lno_vmfailinvalid
+	jnc .Lseamcall_out
  	mov $TDX_SEAMCALL_VMFAILINVALID, %rax

arch/x86/virt/vmx/tdx/tdxcall.S +5 -0

+	 */
+	mov $TDX_SW_ERROR, %r12
+	orq %r12, %rax
 
+	_ASM_EXTABLE_FAULT(1b, 2b)
+.Lseamcall_out:
  	.else
  	tdcall
  	.endif

Quoted 33 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.

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 establishes a new SEAMCALL ABI which allows the host to initialize the module and to manage VMs.

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

SEAMCALL instruction causes #GP when TDX isn't BIOS enabled, and #UD when CPU is not in VMX operation. Currently, only KVM code mocks with VMX enabling, and KVM is the only user of TDX. This implementation chooses to make KVM itself responsible for enabling VMX before using TDX and let the rest of the kernel stay blissfully unaware of VMX.

The current TDX_MODULE_CALL macro handles neither #GP nor #UD. The kernel would hit Oops if SEAMCALL were mistakenly made w/o enabling VMX first. Architecturally, there is no CPU flag to check whether the CPU is in VMX operation. Also, if a BIOS were buggy, it could still report valid TDX private KeyIDs when TDX actually couldn't be enabled.

Extend the TDX_MODULE_CALL macro to handle #UD and #GP to return error codes. Introduce two new TDX error codes for them respectively so the caller can distinguish.

Also add a wrapper function of SEAMCALL to convert SEAMCALL error code to the kernel error code, and print out SEAMCALL error code to help the user to understand what went wrong.

Quoted 2 lines

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

---

I agree with Dave that a buggy bios is not a good motivation for this patch. The real strength of this infrastructure IMHO is central error handling and expressive error messages. Maybe it makes some corner cases (reboot -f) easier to handle. That would make a better justification than buggy bios -- and should be spelled out in the patch description.

[...]

Quoted 8 lines

arch/x86/virt/vmx/tdx/tdx.c +8 -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,

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

Quoted 10 lines

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

+				    struct tdx_module_output *out)
+{
+	int cpu, ret = 0;
+	u64 sret;
+
+	/* Need a stable CPU id for printing error message */
+	cpu = get_cpu();
+
+	sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+

Why not

cpu = get_cpu();
sret = __seamcall(fn, rcx, rdx, r8, r9, out);
put_cpu();

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

Quoted 9 lines

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

+	/* Save SEAMCALL return code if the caller wants it */
+	if (seamcall_ret)
+		*seamcall_ret = sret;
+
+	/* SEAMCALL was successful */
+	if (!sret)
+		goto out;

Why not move that into the switch statement below to avoid th goto? If you do the put_cpu() early, you can avoid "ret" as well.

switch (sret) {
case 0:

	/* SEAMCALL was successful */
	return 0;
case TDX_SEAMCALL_GP:
	pr_err_once("[firmware bug]: TDX is not enabled by BIOS.\n");
	return -ENODEV;

...
}

arch/x86/virt/vmx/tdx/tdxcall.S line 72

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdxcall.S +7 -0

+	/*
+	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
+	 * the trap number.  Convert the trap number to the TDX error
+	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
+	 *
+	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
+	 * only accepts 32-bit immediate at most.

Not sure if that comment is really helpful here. It's a common pattern for large immediates, no?

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdxcall.S +5 -0

+	 */
+	mov $TDX_SW_ERROR, %r12
+	orq %r12, %rax
 
+	_ASM_EXTABLE_FAULT(1b, 2b)
+.Lseamcall_out:
  	.else
  	tdcall
  	.endif

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdxcall.S line 72

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdxcall.S +7 -0

+	/*
+	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
+	 * the trap number.  Convert the trap number to the TDX error
+	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
+	 *
+	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
+	 * only accepts 32-bit immediate at most.

Not sure if that comment is really helpful here. It's a common pattern for large immediates, no?

I am not sure. I guess I am not expert of x86 assembly but only casual writer.

Hi Dave, Kirill,

Are you OK to remove it?

Quoted 13 lines · Kai

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdxcall.S line 72

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdxcall.S +7 -0

+	/*
+	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
+	 * the trap number.  Convert the trap number to the TDX error
+	 * code by setting TDX_SW_ERROR to the high 32-bits of %rax.
+	 *
+	 * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction
+	 * only accepts 32-bit immediate at most.

Not sure if that comment is really helpful here. It's a common pattern for large immediates, no?

I am not sure. I guess I am not expert of x86 assembly but only casual writer.

Quoted 3 lines

Hi Dave, Kirill,

Are you OK to remove it?

I would rather keep it. I wanted to ask why separate MOV is needed here, before I read the comment. Also size of $TDX_SW_ERROR is not visible here, so it contributes to possible confusion without the comment.

Quoted 7 lines

+    /*
+     * SEAMCALL caused #GP or #UD.  By reaching here %eax contains +     * the trap number.  Convert the trap number to the TDX error +     * code by setting TDX_SW_ERROR to the high 32-bits of %rax. +     *
+     * Note cannot OR TDX_SW_ERROR directly to %rax as OR instruction +     * only accepts 32-bit immediate at most.

Not sure if that comment is really helpful here. It's a common pattern for large immediates, no?

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

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

---

v10 -> v11:
- New patch

---
 arch/x86/virt/vmx/tdx/tdx.c | 15 ++++++++++++++-
 arch/x86/virt/vmx/tdx/tdx.h | 17 +++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

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

index e82713dd5d54..e62e978eba1b 100644
@@ -15,6 +15,7 @@
 #include <linux/smp.h>
 #include <asm/msr-index.h>
 #include <asm/msr.h>
+#include <asm/archrandom.h>
 #include <asm/tdx.h>
 #include "tdx.h"
 

@@ -33,12 +34,24 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 				    struct tdx_module_output *out)
 {
 	int cpu, ret = 0;
+	int retry;
 	u64 sret;
 
 	/* Need a stable CPU id for printing error message */
 	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 the existing rdrand_long() to retry
+	 * RDRAND_RETRY_LOOPS times.
+	 */
+	retry = RDRAND_RETRY_LOOPS;
+	do {
+		sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	} while (sret == TDX_RND_NO_ENTROPY && --retry);
 
 	/* Save SEAMCALL return code if the caller wants it */
 	if (seamcall_ret)

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

index 48ad1a1ba737..55dbb1b8c971 100644
@@ -4,6 +4,23 @@
 
 #include <linux/types.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.
+ */
+
+/*
+ * TDX SEAMCALL error codes
+ */
+#define TDX_RND_NO_ENTROPY	0x8000020300000000ULL
+
+/*
+ * Do not put any hardware-defined TDX structure representations below
+ * this comment!
+ */
+
 struct tdx_module_output;
 u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	       struct tdx_module_output *out);

Signature · 1 line

2.40.1

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

Quoted 6 lines · Kai Huang

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

... because who are we kidding? When the TDX module says it doesn't have enough entropy it means rdrand.

Reviewed-by Dave Hansen <[email protected]>

Quoted 5 lines · Dave

Quoted 4 lines · Kai Huang

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Quoted 5 lines

Quoted 2 lines

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

... because who are we kidding? When the TDX module says it doesn't have enough entropy it means rdrand.

The TDX spec says "e.g., RDRAND or RDSEED".

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

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

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

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

---

v10 -> v11:
- New patch

---
  arch/x86/virt/vmx/tdx/tdx.c | 15 ++++++++++++++-
  arch/x86/virt/vmx/tdx/tdx.h | 17 +++++++++++++++++
  2 files changed, 31 insertions(+), 1 deletion(-)

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

index 48ad1a1ba737..55dbb1b8c971 100644
@@ -4,6 +4,23 @@
 
  #include <linux/types.h>
 
+/*

Quoted 18 lines · Kai Huang

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

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

---

v10 -> v11:
- New patch

---
  arch/x86/virt/vmx/tdx/tdx.c | 15 ++++++++++++++-
  arch/x86/virt/vmx/tdx/tdx.h | 17 +++++++++++++++++
  2 files changed, 31 insertions(+), 1 deletion(-)

<snip>

Quoted 8 lines

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

index 48ad1a1ba737..55dbb1b8c971 100644
@@ -4,6 +4,23 @@
 
  #include <linux/types.h>
 

arch/x86/virt/vmx/tdx/tdx.h line 17

Quoted 11 lines

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

+/*
+ * 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.
+ */
+
+/*
+ * TDX SEAMCALL error codes
+ */
+#define TDX_RND_NO_ENTROPY	0x8000020300000000ULL

Where is this return value documented, in TDX module 1.0 spec there are
only: 8000020[123]00000000 specified and there's 80000800 (TDX_KEY_GENERATION_FAILED) and its description mentions the possible failure due to lack of entropy?

Certain SEAMCALL leaf functions may return error due to running out of entropy, in which case the SEAMCALL should be retried as suggested by the TDX spec.

Handle this case in SEAMCALL common function. Mimic the existing rdrand_long() to retry RDRAND_RETRY_LOOPS times.

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

---

v10 -> v11:
- New patch

---
  arch/x86/virt/vmx/tdx/tdx.c | 15 ++++++++++++++-
  arch/x86/virt/vmx/tdx/tdx.h | 17 +++++++++++++++++
  2 files changed, 31 insertions(+), 1 deletion(-)

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

index e82713dd5d54..e62e978eba1b 100644
@@ -15,6 +15,7 @@
  #include <linux/smp.h>
  #include <asm/msr-index.h>
  #include <asm/msr.h>
+#include <asm/archrandom.h>
  #include <asm/tdx.h>
  #include "tdx.h"
 

@@ -33,12 +34,24 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
  				    struct tdx_module_output *out)
  {
  	int cpu, ret = 0;
+	int retry;
  	u64 sret;
 
  	/* Need a stable CPU id for printing error message */
  	cpu = get_cpu();
 

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

+	do {
+		sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	} while (sret == TDX_RND_NO_ENTROPY && --retry);
 
  	/* Save SEAMCALL return code if the caller wants it */
  	if (seamcall_ret)

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

index 48ad1a1ba737..55dbb1b8c971 100644
@@ -4,6 +4,23 @@
 
  #include <linux/types.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.
+ */
+
+/*
+ * TDX SEAMCALL error codes
+ */

Quoted 11 lines

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

@@ -33,12 +34,24 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
  				    struct tdx_module_output *out)
  {
  	int cpu, ret = 0;
+	int retry;
  	u64 sret;
 
  	/* Need a stable CPU id for printing error message */
  	cpu = get_cpu();
 

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

Quoted 10 lines

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

-	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 the existing rdrand_long() to retry
+	 * RDRAND_RETRY_LOOPS times.
+	 */
+	retry = RDRAND_RETRY_LOOPS;

Nit: I'd just do a "int retry = RDRAND_RETRY_LOOPS" and simplify this comment to "Mimic rdrand_long() retry behavior."

Quoted 25 lines

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

+	do {
+		sret = __seamcall(fn, rcx, rdx, r8, r9, out);
+	} while (sret == TDX_RND_NO_ENTROPY && --retry);
 
  	/* Save SEAMCALL return code if the caller wants it */
  	if (seamcall_ret)

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

index 48ad1a1ba737..55dbb1b8c971 100644
@@ -4,6 +4,23 @@
 
  #include <linux/types.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.
+ */
+
+/*
+ * TDX SEAMCALL error codes
+ */

arch/x86/virt/vmx/tdx/tdx.h line 26

Quoted 10 lines

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

+#define TDX_RND_NO_ENTROPY	0x8000020300000000ULL
+
+/*
+ * Do not put any hardware-defined TDX structure representations below
+ * this comment!
+ */
+
  struct tdx_module_output;
  u64 __seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
  	       struct tdx_module_output *out);

In general, LGTM

Reviewed-by David Hildenbrand <[email protected]>

Quoted 11 lines

Quoted 11 lines

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

@@ -33,12 +34,24 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
   				    struct tdx_module_output *out)
   {
   	int cpu, ret = 0;
+	int retry;
   	u64 sret;
 
   	/* Need a stable CPU id for printing error message */
   	cpu = get_cpu();
 

Quoted 13 lines

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

Quoted 10 lines

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

-	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 the existing rdrand_long() to retry
+	 * RDRAND_RETRY_LOOPS times.
+	 */
+	retry = RDRAND_RETRY_LOOPS;

Nit: I'd just do a "int retry = RDRAND_RETRY_LOOPS" and simplify this comment to "Mimic rdrand_long() retry behavior."

OK will do.

But I think you are talking about replacing the second paragraph but not the entire comment?

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

index b489b5b9de5d..03f74851608f 100644
@@ -102,8 +102,12 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 
 #ifdef CONFIG_INTEL_TDX_HOST

Quoted 16 lines · Kai Huang

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

index b489b5b9de5d..03f74851608f 100644
@@ -102,8 +102,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 */
 

arch/x86/include/asm/tdx.h static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1, · line 113

Quoted 1 line

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

 #endif /* !__ASSEMBLY__ */

Please include errno.h.
In the case of !INTEL_TDX_HOST && INTEL_TDX_GUEST, drivers/virt/coco/tdx-guest/tdx-guest.c results in compile error.

Although there are other files that include asm/tdx.h, they seem to luckily include errno.h directly or indirectly.

I've changed to -ENODEV which doesn't cause build error in my test.

I agree it's better to include errno.h explicitly but IMHO it's a problem in the existing upstream code (tdx_kvm_hypercall() returns -ENODEV when INTEL_TDX_GUEST is off), thus should be addressed in a separate patch but not folded into this one.

Since now there's no build error, I hesitated to added such patch. I can do it separately if it should be addressed.

Quoted 7 lines · Kai

I've changed to -ENODEV which doesn't cause build error in my test.

I agree it's better to include errno.h explicitly but IMHO it's a problem in the existing upstream code (tdx_kvm_hypercall() returns -ENODEV when INTEL_TDX_GUEST is off), thus should be addressed in a separate patch but not folded into this one.

Quoted 2 lines

Since now there's no build error, I hesitated to added such patch. I can do it separately if it should be addressed.

Yes, it should be addressed.

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

+	if (tdx_global_init_status & TDX_GLOBAL_INIT_DONE) {
+		ret = tdx_global_init_status & TDX_GLOBAL_INIT_FAILED ?
+			-EINVAL : 0;
+		goto out;
+	}
+
+	/* All '0's are just unused parameters. */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+
+	tdx_global_init_status = TDX_GLOBAL_INIT_DONE;
+	if (ret)
+		tdx_global_init_status |= TDX_GLOBAL_INIT_FAILED;
+out:
+	raw_spin_unlock_irqrestore(&tdx_global_init_lock, flags);
+
+	return ret;
+}
+
+/**

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

+int tdx_cpu_enable(void)
+{
+	unsigned int lp_status;
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -EINVAL;
+
+	lp_status = __this_cpu_read(tdx_lp_init_status);
+
+	/* Already done */
+	if (lp_status & TDX_LP_INIT_DONE)
+		return lp_status & TDX_LP_INIT_FAILED ? -EINVAL : 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 doing the per-cpu initialization.
+	 */
+	ret = try_init_module_global();
+
+	/*
+	 * If the module global initialization failed, there's no point
+	 * to do the per-cpu initialization.  Just mark it as done but
+	 * failed.
+	 */
+	if (ret)
+		goto update_status;
+
+	/* All '0's are just unused parameters */
+	ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
+

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

+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.
+	 */

On 6/4/23 07:27, Kai Huang wrote:
...

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

Quoted 10 lines

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

Why is this "raw_"?

There's zero mention of it anywhere.

Quoted 25 lines

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

+	if (tdx_global_init_status & TDX_GLOBAL_INIT_DONE) {
+		ret = tdx_global_init_status & TDX_GLOBAL_INIT_FAILED ?
+			-EINVAL : 0;
+		goto out;
+	}
+
+	/* All '0's are just unused parameters. */
+	ret = seamcall(TDH_SYS_INIT, 0, 0, 0, 0, NULL, NULL);
+
+	tdx_global_init_status = TDX_GLOBAL_INIT_DONE;
+	if (ret)
+		tdx_global_init_status |= TDX_GLOBAL_INIT_FAILED;
+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.

Quoted 5 lines

+ *
+ * Note this function must be called when preemption is not possible
+ * (i.e. via SMP call or in per-cpu thread). It is not IRQ safe either
+ * (i.e. cannot be called in per-cpu thread and via SMP call from remote
+ * cpu simultaneously).

lockdep_assert_*() are your friends. Unlike comments, they will actually tell you if this goes wrong.

Quoted 38 lines

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

+int tdx_cpu_enable(void)
+{
+	unsigned int lp_status;
+	int ret;
+
+	if (!platform_tdx_enabled())
+		return -EINVAL;
+
+	lp_status = __this_cpu_read(tdx_lp_init_status);
+
+	/* Already done */
+	if (lp_status & TDX_LP_INIT_DONE)
+		return lp_status & TDX_LP_INIT_FAILED ? -EINVAL : 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 doing the per-cpu initialization.
+	 */
+	ret = try_init_module_global();
+
+	/*
+	 * If the module global initialization failed, there's no point
+	 * to do the per-cpu initialization.  Just mark it as done but
+	 * failed.
+	 */
+	if (ret)
+		goto update_status;
+
+	/* All '0's are just unused parameters */
+	ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
+
+update_status:
+	lp_status = TDX_LP_INIT_DONE;
+	if (ret)
+		lp_status |= TDX_LP_INIT_FAILED;
+

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

Quoted 5 lines

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

+	this_cpu_write(tdx_lp_init_status, lp_status);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

You danced around it in the changelog, but the reason for the exports is not clear.

Quoted 16 lines

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

+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.
+	 */

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

Quoted 10 lines

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

+	return -EINVAL;
+}
+
+static int __tdx_enable(void)
+{
+	int ret;
+
+	ret = init_tdx_module();
+	if (ret) {
+		pr_err("TDX module initialization failed (%d)\n", ret);

Have you actually gone any looked at how this pr_*()'s look?

Won't they say:

tdx: TDX module initialized

Isn't that a _bit_ silly? Why not just say:

	pr_info("module initialized.\n");

Quoted 15 lines · Dave Hansen

On 6/4/23 07:27, Kai Huang wrote:
...

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

Quoted 10 lines

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

Why is this "raw_"?

Quoted 1 line

There's zero mention of it anywhere.

Isaku pointed out the normal spinlock_t is converted to sleeping lock for PREEMPT_RT kernel. KVM calls this with IRQ disabled, thus requires a non- sleeping lock.

How about adding below comment here?

/*
* Normal spinlock_t is converted to sleeping lock in PREEMPT_RT
* kernel. Use raw_spinlock_t instead so this function can be called
* even when IRQ is disabled in any kernel configuration. */

Quoted 37 lines

Quoted 37 lines

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

+int tdx_cpu_enable(void)
+{
+   unsigned int lp_status;
+   int ret;
+
+   if (!platform_tdx_enabled())
+           return -EINVAL;
+
+   lp_status = __this_cpu_read(tdx_lp_init_status);
+
+   /* Already done */
+   if (lp_status & TDX_LP_INIT_DONE)
+           return lp_status & TDX_LP_INIT_FAILED ? -EINVAL : 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 doing the per-cpu initialization.
+    */
+   ret = try_init_module_global();
+
+   /*
+    * If the module global initialization failed, there's no point
+    * to do the per-cpu initialization.  Just mark it as done but
+    * failed.
+    */
+   if (ret)
+           goto update_status;
+
+   /* All '0's are just unused parameters */
+   ret = seamcall(TDH_SYS_LP_INIT, 0, 0, 0, 0, NULL, NULL);
+
+update_status:
+   lp_status = TDX_LP_INIT_DONE;
+   if (ret)
+           lp_status |= TDX_LP_INIT_FAILED;
+

Quoted 8 lines

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

Quoted 5 lines

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

+   this_cpu_write(tdx_lp_init_status, lp_status);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(tdx_cpu_enable);

You danced around it in the changelog, but the reason for the exports is not clear.