TDX host kernel support

TDX supports 4K, 2M and 1G page sizes. The corresponding values are defined by the TDX module spec and used as TDX module ABI. Currently, they are used in try_accept_one() when the TDX guest tries to accept a page. However currently try_accept_one() uses hard-coded magic values.

Define TDX supported page sizes as macros and get rid of the hard-coded values in try_accept_one(). TDX host support will need to use them too.

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

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

+++ b/arch/x86/virt/vmx/tdx/tdx.h

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

@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H

Quoted 5 lines · Kai Huang

+++ b/arch/x86/virt/vmx/tdx/tdx.h

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

@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _X86_VIRT_TDX_H
+#define _X86_VIRT_TDX_H

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

Quoted 10 lines

arch/x86/virt/vmx/tdx/tdx.h new file +10 -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.
+ */
+
+/* MSR to report KeyID partitioning between MKTME and TDX */
+#define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087

The *VAST* majority of MSR definitions are in msr-index.h.

Why is this one different from the norm?

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.

The TDX module will be initialized in multi-steps defined by the TDX module and most of those steps involve a specific SEAMCALL:

1) Get the TDX module information and TDX-capable memory regions

    (TDH.SYS.INFO).
 2) Build the list of TDX-usable memory regions.
 3) Construct a list of "TD Memory Regions" (TDMRs) to cover all
    TDX-usable memory regions.
 4) Pick up one TDX private KeyID as the global KeyID.
 5) Configure the TDMRs and the global KeyID to the TDX module
    (TDH.SYS.CONFIG).

6) Configure the global KeyID on all packages (TDH.SYS.KEY.CONFIG).
7) Initialize all TDMRs (TDH.SYS.TDMR.INIT).

Quoted 12 lines · Kai Huang

The TDX module will be initialized in multi-steps defined by the TDX module and most of those steps involve a specific SEAMCALL:

1) Get the TDX module information and TDX-capable memory regions

    (TDH.SYS.INFO).
 2) Build the list of TDX-usable memory regions.
 3) Construct a list of "TD Memory Regions" (TDMRs) to cover all
    TDX-usable memory regions.
 4) Pick up one TDX private KeyID as the global KeyID.
 5) Configure the TDMRs and the global KeyID to the TDX module
    (TDH.SYS.CONFIG).

Quoted 2 lines

6) Configure the global KeyID on all packages (TDH.SYS.KEY.CONFIG).
7) Initialize all TDMRs (TDH.SYS.TDMR.INIT).

I don't think you really need this *AND* the "TODO" comments in init_tdx_module(). Just say:

Add a placeholder tdx_enable() to initialize the TDX module on demand. The TODO list will be pared down as functionality is added.

The subject here is a bit too specific. This patch isn't just "implementing functions". There are more than just functions here. The best subject is probably:

Add SEAMCALL infrastructure

But that's rather generic by necessity because this patch does several _different_ logical things:

* Wrap TDX_MODULE_CALL so it can be used for SEAMCALLs with host=1
* Add handling to TDX_MODULE_CALL to allow it to handle specifically host-side error conditions
* Add high-level seamcall() function with actual error handling

It's probably also worth noting that the code that allows "host=1" to be passed to TDX_MODULE_CALL is dead code in mainline right now. It probably shouldn't have been merged this way, but oh well.

I don't know that you really _need_ to split this up, but I'm just pointing out that mashing three different logical things together makes it hard to write a coherent Subject. But, I've seen worse.

Quoted 3 lines

I don't know that you really _need_ to split this up, but I'm just pointing out that mashing three different logical things together makes it hard to write a coherent Subject. But, I've seen worse.

Agreed.

To me seems "Add SEAMCALL infrastructure" is good enough, but I can split up if you want me to.

Quoted 3 lines · Kai

Quoted 3 lines

I don't know that you really _need_ to split this up, but I'm just pointing out that mashing three different logical things together makes it hard to write a coherent Subject. But, I've seen worse.

Quoted 4 lines

Agreed.

To me seems "Add SEAMCALL infrastructure" is good enough, but I can split up if you want me to.

Everything else being equal, I'd rather have them split up. But, I'm frankly not looking forward to the additional work on my part to review and rework three more patches and changelogs.

For now, both the TDX module information and CMRs are only used during the module initialization, so declare them as local. However, they are 1024 bytes and 512 bytes respectively. Putting them to the stack exceeds the default "stack frame size" that the kernel assumes as safe, and the compiler yields a warning about this. Add a kernel build flag to extend the safe stack size to 4K for tdx.c to silence the warning -- the initialization function is only called once so it's safe to have a 4K stack.

Note not all members in the 1024 bytes TDX module information are used (even by the KVM).

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

index 38d534f2c113..f8a40d15fdfc 100644
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
+CFLAGS_tdx.o += -Wframe-larger-than=4096
 obj-y += tdx.o seamcall.o

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

index b7cedf0589db..6fe505c32599 100644
@@ -13,6 +13,7 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/mutex.h>
+#include <asm/pgtable_types.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>
 #include "tdx.h"

@@ -107,9 +108,8 @@ bool platform_tdx_enabled(void)
  * 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)
+static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+		    u64 *seamcall_ret, struct tdx_module_output *out)
 {
 	u64 sret;
 

@@ -150,12 +150,85 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	}
 }
 
+static inline bool is_cmr_empty(struct cmr_info *cmr)
+{
+	return !cmr->size;
+}
+
+static void print_cmrs(struct cmr_info *cmr_array, int nr_cmrs)
+{
+	int i;
+
+	for (i = 0; i < nr_cmrs; i++) {
+		struct cmr_info *cmr = &cmr_array[i];
+
+		/*
+		 * The array of CMRs reported via TDH.SYS.INFO can
+		 * contain tail empty CMRs.  Don't print them.
+		 */
+		if (is_cmr_empty(cmr))
+			break;
+
+		pr_info("CMR: [0x%llx, 0x%llx)\n", cmr->base,
+				cmr->base + cmr->size);
+	}
+}
+
+/*
+ * Get the TDX module information (TDSYSINFO_STRUCT) and the array of
+ * CMRs, and save them to @sysinfo and @cmr_array, which come from the
+ * kernel stack.  @sysinfo must have been padded to have enough room
+ * to save the TDSYSINFO_STRUCT.
+ */
+static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
+			   struct cmr_info *cmr_array)
+{

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

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

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

+	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
+	int ret;
+
+	ret = tdx_get_sysinfo(sysinfo, cmr_array);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Get TDX module information and TDX-capable memory regions.
 	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.

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

 static int __tdx_enable(void)

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

index 884357a4133c..6d32f62e4182 100644
@@ -3,6 +3,8 @@
 #define _X86_VIRT_TDX_H
 
 #include <linux/types.h>
+#include <linux/stddef.h>
+#include <linux/compiler_attributes.h>
 
 /*
  * This file contains both macros and data structures defined by the TDX

@@ -14,6 +16,80 @@
 /* MSR to report KeyID partitioning between MKTME and TDX */
 #define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
 
+/*
+ * TDX module SEAMCALL leaf functions
+ */
+#define TDH_SYS_INFO		32
+
+struct cmr_info {
+	u64	base;
+	u64	size;
+} __packed;
+
+#define MAX_CMRS			32
+#define CMR_INFO_ARRAY_ALIGNMENT	512
+
+struct cpuid_config {
+	u32	leaf;
+	u32	sub_leaf;
+	u32	eax;
+	u32	ebx;
+	u32	ecx;
+	u32	edx;
+} __packed;

Quoted 8 lines · Kai Huang

For now, both the TDX module information and CMRs are only used during the module initialization, so declare them as local. However, they are 1024 bytes and 512 bytes respectively. Putting them to the stack exceeds the default "stack frame size" that the kernel assumes as safe, and the compiler yields a warning about this. Add a kernel build flag to extend the safe stack size to 4K for tdx.c to silence the warning -- the initialization function is only called once so it's safe to have a 4K stack.

Gah. This has gone off in a really odd direction.

The fact that this is called once really has nothing to do with how tolerant of a large stack we should be. If a function is called once from a deep call stack, it can't consume a lot of stack space. If it's called a billion times from a shallow stack depth, it can use all the stack it wants.

All I really wanted here was this:

static int init_tdx_module(void)
{

(quoted diff) +0 -1

-	struct cmr_info cmr_array[MAX_CMRS] ...;+	static struct cmr_info

cmr_array[MAX_CMRS] ...;

Just make the function variable static instead of having it be a global.
That's *IT*.

Quoted 3 lines

Note not all members in the 1024 bytes TDX module information are used (even by the KVM).

I'm not sure what this has to do with anything.

Quoted 3 lines

This is also a great place to mention that the tdsysinfo_struct contains a *lot* of gunk which will not be used for a bit or that may never get used.

Quoted 70 lines

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

index 38d534f2c113..f8a40d15fdfc 100644
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
+CFLAGS_tdx.o += -Wframe-larger-than=4096
 obj-y += tdx.o seamcall.o

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

index b7cedf0589db..6fe505c32599 100644
@@ -13,6 +13,7 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/mutex.h>
+#include <asm/pgtable_types.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>
 #include "tdx.h"

@@ -107,9 +108,8 @@ bool platform_tdx_enabled(void)
  * 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)
+static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
+		    u64 *seamcall_ret, struct tdx_module_output *out)
 {
 	u64 sret;
 

@@ -150,12 +150,85 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 	}
 }
 
+static inline bool is_cmr_empty(struct cmr_info *cmr)
+{
+	return !cmr->size;
+}
+
+static void print_cmrs(struct cmr_info *cmr_array, int nr_cmrs)
+{
+	int i;
+
+	for (i = 0; i < nr_cmrs; i++) {
+		struct cmr_info *cmr = &cmr_array[i];
+
+		/*
+		 * The array of CMRs reported via TDH.SYS.INFO can
+		 * contain tail empty CMRs.  Don't print them.
+		 */
+		if (is_cmr_empty(cmr))
+			break;
+
+		pr_info("CMR: [0x%llx, 0x%llx)\n", cmr->base,
+				cmr->base + cmr->size);
+	}
+}
+
+/*
+ * Get the TDX module information (TDSYSINFO_STRUCT) and the array of
+ * CMRs, and save them to @sysinfo and @cmr_array, which come from the
+ * kernel stack.  @sysinfo must have been padded to have enough room
+ * to save the TDSYSINFO_STRUCT.
+ */
+static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
+			   struct cmr_info *cmr_array)
+{

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

Quoted 10 lines

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

+	struct tdx_module_output out;
+	u64 sysinfo_pa, cmr_array_pa;
+	int ret;
+
+	/*
+	 * Cannot use __pa() directly as @sysinfo and @cmr_array
+	 * come from the kernel stack.
+	 */
+	sysinfo_pa = slow_virt_to_phys(sysinfo);
+	cmr_array_pa = slow_virt_to_phys(cmr_array);

Note: they won't be on the kernel stack if they're 'static'.

Quoted 13 lines

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

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

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

+	return 0;
+}
+
 static int init_tdx_module(void)
 {
+	/*
+	 * @tdsysinfo and @cmr_array are used in TDH.SYS.INFO SEAMCALL ABI.
+	 * They are 1024 bytes and 512 bytes respectively but it's fine to
+	 * keep them in the stack as this function is only called once.
+	 */

Again, more silliness about being called once.

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

Quoted 3 lines

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

+	DECLARE_PADDED_STRUCT(tdsysinfo_struct, tdsysinfo,
+			TDSYSINFO_STRUCT_SIZE, TDSYSINFO_STRUCT_ALIGNMENT);
+	struct cmr_info cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);

One more thing about being on the stack: These aren't implicitly zeroed.
They might have stack gunk from other calls in them. I _think_ that's
OK because of, for instance, the 'out.r9' that limits how many CMRs get read. But, not being zeroed is a potential source of bugs and it's also something that reviewers (and you) need to think about to make sure it doesn't have side-effects.

Quoted 20 lines

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

+	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
+	int ret;
+
+	ret = tdx_get_sysinfo(sysinfo, cmr_array);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Get TDX module information and TDX-capable memory regions.
 	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.

@@ -166,7 +239,9 @@ static int init_tdx_module(void)
 	 *
 	 *  Return error before all steps are done.
 	 */
-	return -EINVAL;

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 245

Quoted 4 lines

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

+	ret = -EINVAL;
+out:
+	return ret;
 }

I'm going to be lazy and not look into the future. But, you don't need the "out:" label here, yet. It doesn'serve any purpose like this, so why introduce it here?

Quoted 40 lines

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

 static int __tdx_enable(void)

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

index 884357a4133c..6d32f62e4182 100644
@@ -3,6 +3,8 @@
 #define _X86_VIRT_TDX_H
 
 #include <linux/types.h>
+#include <linux/stddef.h>
+#include <linux/compiler_attributes.h>
 
 /*
  * This file contains both macros and data structures defined by the TDX

@@ -14,6 +16,80 @@
 /* MSR to report KeyID partitioning between MKTME and TDX */
 #define MSR_IA32_MKTME_KEYID_PARTITIONING	0x00000087
 
+/*
+ * TDX module SEAMCALL leaf functions
+ */
+#define TDH_SYS_INFO		32
+
+struct cmr_info {
+	u64	base;
+	u64	size;
+} __packed;
+
+#define MAX_CMRS			32
+#define CMR_INFO_ARRAY_ALIGNMENT	512
+
+struct cpuid_config {
+	u32	leaf;
+	u32	sub_leaf;
+	u32	eax;
+	u32	ebx;
+	u32	ecx;
+	u32	edx;
+} __packed;

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

Quoted 10 lines

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

+
+#define DECLARE_PADDED_STRUCT(type, name, size, alignment)	\
+	struct type##_padded {					\
+		union {						\
+			struct type name;			\
+			u8 padding[size];			\
+		};						\
+	} name##_padded __aligned(alignment)
+
+#define PADDED_STRUCT(name)	(name##_padded.name)

These don't turn out looking _that_ nice in practice, but I do vastly prefer them to hard-coded padding.

<snip>

On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:

Quoted 4 lines

Quoted 2 lines

Note not all members in the 1024 bytes TDX module information are used (even by the KVM).

I'm not sure what this has to do with anything.

You mentioned in v7 that:

Quoted 1 line

Quoted 1 line

Quoted 1 line

This is also a great place to mention that the tdsysinfo_struct

Quoted 2 lines

Quoted 2 lines

a *lot* of gunk which will not be used for a bit or that may never get used.

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

Perhaps I misunderstood something but I was trying to address this.

Should I remove this sentence?

Quoted 19 lines

Quoted 19 lines

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

+	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
+	int ret;
+
+	ret = tdx_get_sysinfo(sysinfo, cmr_array);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Get TDX module information and TDX-capable memory regions.
 	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.

@@ -166,7 +239,9 @@ static int init_tdx_module(void)
 	 *
 	 *  Return error before all steps are done.
 	 */
-	return -EINVAL;

Quoted 8 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 245

Quoted 4 lines

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

+	ret = -EINVAL;
+out:
+	return ret;
 }

I'm going to be lazy and not look into the future. But, you don't need the "out:" label here, yet. It doesn'serve any purpose like this, so why introduce it here?

The 'out' label is here because of below code:

	ret = tdx_get_sysinfo(...);
	if (ret)
		goto out;

If I don't have 'out' label here in this patch, do you mean something below?

	ret = tdx_get_sysinfo(...);
	if (ret)
		return ret;

	/*
	 * TODO:
	 * ...
	 * Return error before all steps are done.
	 */
	return -EINVAL;

Quoted 1 line · Kai

On Fri, 2023-01-06 at 09:46 -0800, Dave Hansen wrote:

...

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Note not all members in the 1024 bytes TDX module information are used (even by the KVM).

I'm not sure what this has to do with anything.

You mentioned in v7 that:

Quoted 1 line

Quoted 1 line

Quoted 1 line

This is also a great place to mention that the tdsysinfo_struct

contains

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

a *lot* of gunk which will not be used for a bit or that may never get used.

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

Perhaps I misunderstood something but I was trying to address this.

Quoted 1 line

Should I remove this sentence?

If someone goes looking at this patch, the see tdsysinfo_struct with something like two dozen defined fields. But, very few of them get used in this patch. Why? Just saying that they are unused is a bit silly.

The 'tdsysinfo_struct' is fairly large (1k) and contains a lot of info about the TD. Fully define the entire structure, but

only use the fields necessary to build the PAMT and TDMRs and pr_info() some basics about the module.

The rest of the fields will get used... (by kvm? never??)

...

Quoted 36 lines

Quoted 19 lines

Quoted 19 lines

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

+	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
+	int ret;
+
+	ret = tdx_get_sysinfo(sysinfo, cmr_array);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Get TDX module information and TDX-capable memory regions.
 	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.

@@ -166,7 +239,9 @@ static int init_tdx_module(void)
 	 *
 	 *  Return error before all steps are done.
 	 */
-	return -EINVAL;

Quoted 8 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 245

Quoted 4 lines

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

+	ret = -EINVAL;
+out:
+	return ret;
 }

I'm going to be lazy and not look into the future. But, you don't need the "out:" label here, yet. It doesn'serve any purpose like this, so why introduce it here?

The 'out' label is here because of below code:

	ret = tdx_get_sysinfo(...);
	if (ret)
		goto out;

If I don't have 'out' label here in this patch, do you mean something below?

Quoted 10 lines

	ret = tdx_get_sysinfo(...);
	if (ret)
		return ret;

	/*
	 * TODO:
	 * ...
	 * Return error before all steps are done.
	 */
	return -EINVAL;

Yes, if you remove the 'out:' label like you've shown in your reply, it's actually _less_ code. The labels are really only necessary when you have common work to "undo" something before returning from the function. Here, you can just return.

Quoted 3 lines

The 'tdsysinfo_struct' is fairly large (1k) and contains a lot of info about the TD. Fully define the entire structure, but

^
should be: "about the TDX module"?

As a step of initializing the TDX module, the kernel needs to tell the TDX module which memory regions can be used by the TDX module as TDX guest memory.

TDX reports a list of "Convertible Memory Region" (CMR) to tell the kernel which memory is TDX compatible. The kernel needs to build a list of memory regions (out of CMRs) as "TDX-usable" memory and pass them to the TDX module. Once this is done, those "TDX-usable" memory regions are fixed during module's lifetime.

The initial support of TDX guests will only allocate TDX guest memory from the global page allocator. To keep things simple, just make sure all pages in the page allocator are TDX memory.

To guarantee that, stash off the memblock memory regions at the time of initializing the TDX module as TDX's own usable memory regions, and in the meantime, register a TDX memory notifier to reject to online any new memory in memory hotplug.

This approach works as in practice all boot-time present DIMMs are TDX convertible memory. However, if any non-TDX-convertible memory has been hot-added (i.e. CXL memory via kmem driver) before initializing the TDX module, the module initialization will fail.

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

arch/x86/Kconfig +1 -0

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

arch/x86/kernel/setup.c +2 -0

index 216fee7144ee..3a841a77fda4 100644
@@ -1174,6 +1174,8 @@ void __init setup_arch(char **cmdline_p)
 	 *
 	 * Moreover, on machines with SandyBridge graphics or in setups that use
 	 * crashkernel the entire 1M is reserved anyway.
+	 *
+	 * Note the host kernel TDX also requires the first 1MB being reserved.
 	 */
 	reserve_real_mode();
 

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

index 6fe505c32599..f010402f443d 100644
@@ -13,6 +13,13 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/memory.h>
+#include <linux/minmax.h>
+#include <linux/sizes.h>
+#include <linux/pfn.h>
 #include <asm/pgtable_types.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>

@@ -25,6 +32,12 @@ enum tdx_module_status_t {
 	TDX_MODULE_ERROR
 };
 
+struct tdx_memblock {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
 static u32 tdx_keyid_start __ro_after_init;
 static u32 nr_tdx_keyids __ro_after_init;
 

@@ -32,6 +45,9 @@ static enum tdx_module_status_t tdx_module_status;
 /* Prevent concurrent attempts on TDX detection and initialization */
 static DEFINE_MUTEX(tdx_module_lock);
 
+/* All TDX-usable memory regions */
+static LIST_HEAD(tdx_memlist);
+
 /*
  * tdx_keyid_start and nr_tdx_keyids indicate that TDX is uninitialized.
  * This is used in TDX initialization error paths to take it from

@@ -69,6 +85,50 @@ static int __init record_keyid_partitioning(void)
 	return 0;
 }
 
+static bool is_tdx_memory(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	/* Empty list means TDX isn't enabled. */
+	if (list_empty(&tdx_memlist))
+		return true;
+
+	list_for_each_entry(tmb, &tdx_memlist, list) {
+		/*
+		 * The new range is TDX memory if it is fully covered by
+		 * any TDX memory block.
+		 *

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

+	return is_tdx_memory(mn->start_pfn, mn->start_pfn + mn->nr_pages) ?
+		NOTIFY_OK : NOTIFY_BAD;
+}
+
+static struct notifier_block tdx_memory_nb = {
+	.notifier_call = tdx_memory_notifier,
+};
+
 static int __init tdx_init(void)
 {
 	int err;

@@ -89,6 +149,13 @@ static int __init tdx_init(void)
 		goto no_tdx;
 	}
 
+	err = register_memory_notifier(&tdx_memory_nb);
+	if (err) {
+		pr_info("initialization failed: register_memory_notifier() failed (%d)\n",
+				err);
+		goto no_tdx;
+	}
+
 	return 0;
 no_tdx:
 	clear_tdx();

@@ -209,6 +276,77 @@ static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
 	return 0;
 }
 
+/*
+ * Add a memory region as a TDX memory block.  The caller must make sure
+ * all memory regions are added in address ascending order and don't
+ * overlap.
+ */
+static int add_tdx_memblock(struct list_head *tmb_list, unsigned long start_pfn,
+			    unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	tmb = kmalloc(sizeof(*tmb), GFP_KERNEL);
+	if (!tmb)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&tmb->list);
+	tmb->start_pfn = start_pfn;
+	tmb->end_pfn = end_pfn;
+
+	list_add_tail(&tmb->list, tmb_list);
+	return 0;
+}
+

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

+static int build_tdx_memlist(struct list_head *tmb_list)
+{
+	unsigned long start_pfn, end_pfn;
+	int i, ret;
+
+	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, NULL) {
+		/*
+		 * The first 1MB is not reported as TDX convertible memory.
+		 * Although the first 1MB is always reserved and won't end up
+		 * to the page allocator, it is still in memblock's memory
+		 * regions.  Skip them manually to exclude them as TDX memory.
+		 */
+		start_pfn = max(start_pfn, PHYS_PFN(SZ_1M));
+		if (start_pfn >= end_pfn)
+			continue;
+
+		/*
+		 * Add the memory regions as TDX memory.  The regions in
+		 * memblock has already guaranteed they are in address
+		 * ascending order and don't overlap.
+		 */
+		ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	free_tdx_memlist(tmb_list);
+	return ret;
+}
+
 static int init_tdx_module(void)
 {
 	/*

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

+	ret = build_tdx_memlist(&tdx_memlist);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.
 	 *  - Pick up one TDX private KeyID as the global KeyID.

Quoted 15 lines · Kai Huang

Quoted 13 lines

Quoted 13 lines

As a step of initializing the TDX module, the kernel needs to tell the TDX module which memory regions can be used by the TDX module as TDX guest memory.

TDX reports a list of "Convertible Memory Region" (CMR) to tell the kernel which memory is TDX compatible. The kernel needs to build a list of memory regions (out of CMRs) as "TDX-usable" memory and pass them to the TDX module. Once this is done, those "TDX-usable" memory regions are fixed during module's lifetime.

The initial support of TDX guests will only allocate TDX guest memory from the global page allocator. To keep things simple, just make sure all pages in the page allocator are TDX memory.

Quoted 8 lines

It's hard to tell what "The initial support of TDX guests" means. I *think* you mean "this series". But, we try not to say "this blah" too much, so just say this:

To keep things simple, assume that all TDX-protected memory will comes page allocator. Make sure all pages in the page allocator *are* TDX-usable memory.

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

To guarantee that, stash off the memblock memory regions at the time of initializing the TDX module as TDX's own usable memory regions, and in the meantime, register a TDX memory notifier to reject to online any new memory in memory hotplug.

First, this is a run-on sentence. Second, it isn't really clear what memblocks have to do with this or why you need to stash them off. Please explain.

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This approach works as in practice all boot-time present DIMMs are TDX convertible memory. However, if any non-TDX-convertible memory has been hot-added (i.e. CXL memory via kmem driver) before initializing the TDX module, the module initialization will fail.

I really don't know what this is trying to say.

Quoted 3 lines

*How* and *why* does this module initialization failure occur?

Quoted 2 lines

How do
you implement it and why is it necessary?

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

Quoted 101 lines

Quoted 86 lines

Quoted 86 lines

arch/x86/Kconfig +1 -0

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

arch/x86/kernel/setup.c +2 -0

index 216fee7144ee..3a841a77fda4 100644
@@ -1174,6 +1174,8 @@ void __init setup_arch(char **cmdline_p)
 	 *
 	 * Moreover, on machines with SandyBridge graphics or in setups that use
 	 * crashkernel the entire 1M is reserved anyway.
+	 *
+	 * Note the host kernel TDX also requires the first 1MB being reserved.
 	 */
 	reserve_real_mode();
 

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

index 6fe505c32599..f010402f443d 100644
@@ -13,6 +13,13 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/memory.h>
+#include <linux/minmax.h>
+#include <linux/sizes.h>
+#include <linux/pfn.h>
 #include <asm/pgtable_types.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>

@@ -25,6 +32,12 @@ enum tdx_module_status_t {
 	TDX_MODULE_ERROR
 };
 
+struct tdx_memblock {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
 static u32 tdx_keyid_start __ro_after_init;
 static u32 nr_tdx_keyids __ro_after_init;
 

@@ -32,6 +45,9 @@ static enum tdx_module_status_t tdx_module_status;
 /* Prevent concurrent attempts on TDX detection and initialization */
 static DEFINE_MUTEX(tdx_module_lock);
 
+/* All TDX-usable memory regions */
+static LIST_HEAD(tdx_memlist);
+
 /*
  * tdx_keyid_start and nr_tdx_keyids indicate that TDX is uninitialized.
  * This is used in TDX initialization error paths to take it from

@@ -69,6 +85,50 @@ static int __init record_keyid_partitioning(void)
 	return 0;
 }
 
+static bool is_tdx_memory(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	/* Empty list means TDX isn't enabled. */
+	if (list_empty(&tdx_memlist))
+		return true;
+
+	list_for_each_entry(tmb, &tdx_memlist, list) {
+		/*
+		 * The new range is TDX memory if it is fully covered by
+		 * any TDX memory block.
+		 *
+		 * Note TDX memory blocks are originated from memblock
+		 * memory regions, which can only be contiguous when two

Quoted 8 lines

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

Quoted 8 lines

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

+		 * regions have different NUMA nodes or flags.  Therefore
+		 * the new range cannot cross multiple TDX memory blocks.
+		 */
+		if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+			return true;
+	}
+	return false;
+}

I don't really like that comment. It should first state its behavior and assumptions, like:

	This check assumes that the start_pfn<->end_pfn range does not
	cross multiple tdx_memlist entries.

Quoted 6 lines

Only then should it describe why that is OK:

A single memory hotplug even across mutliple memblocks (from which tdx_memlist entries are derived) is impossible. ... then actually explain

Quoted 18 lines

Quoted 14 lines

Quoted 4 lines

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

+static int tdx_memory_notifier(struct notifier_block *nb, unsigned long action,
+			       void *v)

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

Quoted 10 lines

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

+{
+	struct memory_notify *mn = v;
+
+	if (action != MEM_GOING_ONLINE)
+		return NOTIFY_OK;
+
+	/*
+	 * Not all memory is compatible with TDX.  Reject
+	 * to online any incompatible memory.
+	 */

This comment isn't quite right either. There might actually be totally TDX *compatible* memory here. It just wasn't configured for use with TDX.

Quoted 7 lines

Shouldn't this be something more like:

/*
* The TDX memory configuration is static and can not be
* changed. Reject onlining any memory which is outside
* of the static configuration whether it supports TDX or not. */

Quoted 65 lines

Quoted 54 lines

Quoted 54 lines

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

+	return is_tdx_memory(mn->start_pfn, mn->start_pfn + mn->nr_pages) ?
+		NOTIFY_OK : NOTIFY_BAD;
+}
+
+static struct notifier_block tdx_memory_nb = {
+	.notifier_call = tdx_memory_notifier,
+};
+
 static int __init tdx_init(void)
 {
 	int err;

@@ -89,6 +149,13 @@ static int __init tdx_init(void)
 		goto no_tdx;
 	}
 
+	err = register_memory_notifier(&tdx_memory_nb);
+	if (err) {
+		pr_info("initialization failed: register_memory_notifier() failed (%d)\n",
+				err);
+		goto no_tdx;
+	}
+
 	return 0;
 no_tdx:
 	clear_tdx();

@@ -209,6 +276,77 @@ static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
 	return 0;
 }
 
+/*
+ * Add a memory region as a TDX memory block.  The caller must make sure
+ * all memory regions are added in address ascending order and don't
+ * overlap.
+ */
+static int add_tdx_memblock(struct list_head *tmb_list, unsigned long start_pfn,
+			    unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	tmb = kmalloc(sizeof(*tmb), GFP_KERNEL);
+	if (!tmb)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&tmb->list);
+	tmb->start_pfn = start_pfn;
+	tmb->end_pfn = end_pfn;
+
+	list_add_tail(&tmb->list, tmb_list);
+	return 0;
+}
+
+static void free_tdx_memlist(struct list_head *tmb_list)
+{

Quoted 8 lines

arch/x86/virt/vmx/tdx/tdx.c static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo, · line 310

Quoted 8 lines

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

+	while (!list_empty(tmb_list)) {
+		struct tdx_memblock *tmb = list_first_entry(tmb_list,
+				struct tdx_memblock, list);
+
+		list_del(&tmb->list);
+		kfree(tmb);
+	}
+}

'tdx_memlist' is written only once at boot and then is read-only, right?

Quoted 2 lines

It might be nice to mention that so that the lack of locking doesn't look problematic.

Quoted 10 lines

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdx.c static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo, · line 317

Quoted 7 lines

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

+/*
+ * Ensure that all memblock memory regions are convertible to TDX
+ * memory.  Once this has been established, stash the memblock
+ * ranges off in a secondary structure because memblock is modified
+ * in memory hotplug while TDX memory regions are fixed.
+ */

Ahh, that's why we need to "shadow" the memblocks. Can you add a sentence on this to the changelog, please?

Quoted 40 lines

Quoted 40 lines

Quoted 40 lines

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

+static int build_tdx_memlist(struct list_head *tmb_list)
+{
+	unsigned long start_pfn, end_pfn;
+	int i, ret;
+
+	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, NULL) {
+		/*
+		 * The first 1MB is not reported as TDX convertible memory.
+		 * Although the first 1MB is always reserved and won't end up
+		 * to the page allocator, it is still in memblock's memory
+		 * regions.  Skip them manually to exclude them as TDX memory.
+		 */
+		start_pfn = max(start_pfn, PHYS_PFN(SZ_1M));
+		if (start_pfn >= end_pfn)
+			continue;
+
+		/*
+		 * Add the memory regions as TDX memory.  The regions in
+		 * memblock has already guaranteed they are in address
+		 * ascending order and don't overlap.
+		 */
+		ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	free_tdx_memlist(tmb_list);
+	return ret;
+}
+
 static int init_tdx_module(void)
 {
 	/*

@@ -226,10 +364,25 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out;
 

Quoted 8 lines

Quoted 4 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 370

Quoted 4 lines

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

+	/*
+	 * The initial support of TDX guests only allocates memory from
+	 * the global page allocator.  To keep things simple, just make
+	 * sure all pages in the page allocator are TDX memory.

I didn't like this in the changelog either. Try to make this "timeless" rather than refer to what the support is today. I gave you example text above.

Quoted 11 lines

Quoted 7 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 377

Quoted 7 lines

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

+	 * Build the list of "TDX-usable" memory regions which cover all
+	 * pages in the page allocator to guarantee that.  Do it while
+	 * holding mem_hotplug_lock read-lock as the memory hotplug code
+	 * path reads the @tdx_memlist to reject any new memory.
+	 */
+	get_online_mems();

Oh, it actually uses the memory hotplug locking for list protection. That's at least a bit subtle. Please document that somewhere in the functions that actually manipulate the list.

Quoted 10 lines

I think it's also worth saying something here about the high-level effects of what's going on:

Take a snapshot of the memory configuration (memblocks). This snapshot will be used to enable TDX support for *this* memory configuration only. Use a memory hotplug notifier to ensure that no other RAM can be added outside of this configuration.

That's it, right?

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

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

+	ret = build_tdx_memlist(&tdx_memlist);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.
 	 *  - Pick up one TDX private KeyID as the global KeyID.

@@ -241,6 +394,11 @@ static int init_tdx_module(void)
 	 */
 	ret = -EINVAL;
 out:
+	/*
+	 * @tdx_memlist is written here and read at memory hotplug time.
+	 * Lock out memory hotplug code while building it.
+	 */

Quoted 8 lines

Quoted 3 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 403

Quoted 3 lines

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

+	put_online_mems();
 	return ret;
 }

You would also be wise to have the folks who do a lot of memory hotplug work today look at this sooner rather than later. I _think_ what you have here is OK, but I'm really rusty on the code itself.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

This approach works as in practice all boot-time present DIMMs are TDX convertible memory. However, if any non-TDX-convertible memory has been hot-added (i.e. CXL memory via kmem driver) before initializing the TDX module, the module initialization will fail.

Quoted 1 line

Quoted 1 line

I really don't know what this is trying to say.

My intention is to explain and call out that such design (use all memory regions in memblock at the time of module initialization) works in practice, as long as non-CMR memory hasn't been added via memory hotplug.

Not sure if it is necessary, but I was thinking it may help reviewer to judge whether such design is acceptable.

Quoted 1 line

Quoted 1 line

*How* and *why* does this module initialization failure occur?

If we pass any non-CMR memory to the TDX module, the SEAMCALL (TDH.SYS.CONFIG) will fail.

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 22 lines

Quoted 22 lines

Quoted 7 lines

Quoted 7 lines

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

+       list_for_each_entry(tmb, &tdx_memlist, list) {
+               /*
+                * The new range is TDX memory if it is fully covered by
+                * any TDX memory block.
+                *
+                * Note TDX memory blocks are originated from memblock
+                * memory regions, which can only be contiguous when two

Quoted 8 lines

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

Quoted 8 lines

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

+                * regions have different NUMA nodes or flags.  Therefore
+                * the new range cannot cross multiple TDX memory blocks.
+                */
+               if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+                       return true;
+       }
+       return false;
+}

I don't really like that comment. It should first state its behavior and assumptions, like:

This check assumes that the start_pfn<->end_pfn range does not cross multiple tdx_memlist entries.

Quoted 6 lines

Quoted 6 lines

Only then should it describe why that is OK:

A single memory hotplug even across mutliple memblocks (from which tdx_memlist entries are derived) is impossible. ... then actually explain

How about below?

/*
* This check assumes that the start_pfn<->end_pfn range does not cross
* multiple tdx_memlist entries. A single memory hotplug event across
* multiple memblocks (from which tdx_memlist entries are derived) is
* impossible. That means start_pfn<->end_pfn range cannot exceed a
* tdx_memlist entry, and the new range is TDX memory if it is fully
* covered by any tdx_memlist entry.
*/

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 377

Quoted 6 lines

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

+        * Build the list of "TDX-usable" memory regions which cover all
+        * pages in the page allocator to guarantee that.  Do it while
+        * holding mem_hotplug_lock read-lock as the memory hotplug code
+        * path reads the @tdx_memlist to reject any new memory.
+        */
+       get_online_mems();

Oh, it actually uses the memory hotplug locking for list protection. That's at least a bit subtle. Please document that somewhere in the functions that actually manipulate the list.

add_tdx_memblock() and free_tdx_memlist() eventually calls list_add_tail() and list_del() to manipulate the list, but they actually takes 'struct list_head *tmb_list' as argument. 'tdx_memlist' is passed to build_tdx_memlist() as input.

Do you mean document the locking around the implementation of add_tdx_memblock() and free_tdx_memlist()?

Or should we just mention it around the 'tdx_memlist' variable?

/* All TDX-usable memory regions. Protected by memory hotplug locking. */ static LIST_HEAD(tdx_memlist);

Quoted 9 lines

Quoted 9 lines

I think it's also worth saying something here about the high-level effects of what's going on:

Take a snapshot of the memory configuration (memblocks). This snapshot will be used to enable TDX support for *this* memory configuration only. Use a memory hotplug notifier to ensure that no other RAM can be added outside of this configuration.

That's it, right?

Yes. I'll somehow include above into the comment around get_online_mems().

But should I move "Use a memory hotplug notifier ..." part to:

        err = register_memory_notifier(&tdx_memory_nb);

because this is where we actually use the memory hotplug notifier?

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

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

+       ret = build_tdx_memlist(&tdx_memlist);
+       if (ret)
+               goto out;
+
        /*
         * TODO:
         *
-        *  - Build the list of TDX-usable memory regions.
         *  - Construct a list of TDMRs to cover all TDX-usable memory
         *    regions.
         *  - Pick up one TDX private KeyID as the global KeyID.

@@ -241,6 +394,11 @@ static int init_tdx_module(void)
         */
        ret = -EINVAL;
 out:
+       /*
+        * @tdx_memlist is written here and read at memory hotplug time.
+        * Lock out memory hotplug code while building it.
+        */

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 403

Quoted 3 lines

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

+       put_online_mems();
        return ret;
 }

You would also be wise to have the folks who do a lot of memory hotplug work today look at this sooner rather than later. I _think_ what you have here is OK, but I'm really rusty on the code itself.

Thanks for advice. Will do.

On 1/9/23 03:48, Huang, Kai wrote:
...

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

This approach works as in practice all boot-time present DIMMs are TDX convertible memory. However, if any non-TDX-convertible memory has been hot-added (i.e. CXL memory via kmem driver) before initializing the TDX module, the module initialization will fail.

Quoted 8 lines

Quoted 1 line

Quoted 1 line

I really don't know what this is trying to say.

My intention is to explain and call out that such design (use all memory regions in memblock at the time of module initialization) works in practice, as long as non-CMR memory hasn't been added via memory hotplug.

Not sure if it is necessary, but I was thinking it may help reviewer to judge whether such design is acceptable.

This is yet another case where you've mechanically described the "what", but left out the implications or the underlying basis "why".

I'd take a more methodical approach to describe what is going on here. List the steps that must occur, or at least *one* example of those steps and how they intereact with the code in this patch. Then, explain the fallout.

I also don't think it's quite right to call out "CXL memory via kmem driver". If the CXL memory was "System RAM", it should get covered by a CMR and TDMR. The kmem driver can still go wild with it.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

*How* and *why* does this module initialization failure occur?

If we pass any non-CMR memory to the TDX module, the SEAMCALL (TDH.SYS.CONFIG) will fail.

I'm frankly lost now. Please go back and try to explain this better. Let me know if you want to iterate on this faster than resending this series five more times. I've got some ideas.

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

Quoted 1 line

But probably it is not necessarily to call out in the changelog?

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

...

Quoted 31 lines

Quoted 22 lines

Quoted 22 lines

Quoted 7 lines

Quoted 7 lines

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

+       list_for_each_entry(tmb, &tdx_memlist, list) {
+               /*
+                * The new range is TDX memory if it is fully covered by
+                * any TDX memory block.
+                *
+                * Note TDX memory blocks are originated from memblock
+                * memory regions, which can only be contiguous when two

Quoted 8 lines

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

Quoted 8 lines

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

+                * regions have different NUMA nodes or flags.  Therefore
+                * the new range cannot cross multiple TDX memory blocks.
+                */
+               if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+                       return true;
+       }
+       return false;
+}

I don't really like that comment. It should first state its behavior and assumptions, like:

This check assumes that the start_pfn<->end_pfn range does not cross multiple tdx_memlist entries.

Quoted 6 lines

Quoted 6 lines

Only then should it describe why that is OK:

A single memory hotplug even across mutliple memblocks (from which tdx_memlist entries are derived) is impossible. ... then actually explain

How about below?

Quoted 8 lines

/*
* This check assumes that the start_pfn<->end_pfn range does not cross
* multiple tdx_memlist entries. A single memory hotplug event across
* multiple memblocks (from which tdx_memlist entries are derived) is
* impossible. That means start_pfn<->end_pfn range cannot exceed a
* tdx_memlist entry, and the new range is TDX memory if it is fully
* covered by any tdx_memlist entry.
*/

I was hoping you would actually explain why it is impossible.

Is there something fundamental that keeps a memory area that spans two nodes from being removed and then a new area added that is comprised of a single node?
Boot time:

	| memblock  |  memblock |
	<--Node=0--> <--Node=1-->

Funky hotplug... nothing to see here, then:

	<--------Node=2-------->

I would believe that there is no current bare-metal TDX system that has an implementation like this. But, the comments above speak like it's fundamentally impossible. That should be clarified.

In other words, that comment talks about memblock attributes as being the core underlying reason that that simplified check is OK. Is that it, or is it really the reduced hotplug feature set on TDX systems?

...

Quoted 18 lines

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

arch/x86/virt/vmx/tdx/tdx.c static int init_tdx_module(void) · line 377

Quoted 6 lines

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

+        * Build the list of "TDX-usable" memory regions which cover all
+        * pages in the page allocator to guarantee that.  Do it while
+        * holding mem_hotplug_lock read-lock as the memory hotplug code
+        * path reads the @tdx_memlist to reject any new memory.
+        */
+       get_online_mems();

Oh, it actually uses the memory hotplug locking for list protection. That's at least a bit subtle. Please document that somewhere in the functions that actually manipulate the list.

add_tdx_memblock() and free_tdx_memlist() eventually calls list_add_tail() and list_del() to manipulate the list, but they actually takes 'struct list_head *tmb_list' as argument. 'tdx_memlist' is passed to build_tdx_memlist() as input.

Do you mean document the locking around the implementation of add_tdx_memblock() and free_tdx_memlist()?

Quoted 4 lines

Or should we just mention it around the 'tdx_memlist' variable?

/* All TDX-usable memory regions. Protected by memory hotplug locking. */ static LIST_HEAD(tdx_memlist);

I don't think I'd hate it being in all three spots. Also "protected by memory hotplug locking" is pretty generic. Please be more specific.

Quoted 13 lines

Quoted 10 lines

Quoted 10 lines

I think it's also worth saying something here about the high-level effects of what's going on:

Take a snapshot of the memory configuration (memblocks). This snapshot will be used to enable TDX support for *this* memory configuration only. Use a memory hotplug notifier to ensure that no other RAM can be added outside of this configuration.

That's it, right?

Yes. I'll somehow include above into the comment around get_online_mems().

Quoted 5 lines

But should I move "Use a memory hotplug notifier ..." part to:

        err = register_memory_notifier(&tdx_memory_nb);

because this is where we actually use the memory hotplug notifier?

I actually want that snippet in the changelog.

Quoted 19 lines · Dave Hansen

Quoted 17 lines · Kai

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 2 lines

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

Right. The old app will break w/o knowing the new ABI. One resolution, I think, is we don't introduce new userspace ABI, but hide "TDX-capable" and "non- TDX-capable" nodes in the kernel, and let kernel to enforce always allocating TDX guest memory from those "TDX-capable" nodes.

Quoted 10 lines

Is there something fundamental that keeps a memory area that spans two nodes from being removed and then a new area added that is comprised of a single node?
Boot time:

	| memblock  |  memblock |
	<--Node=0--> <--Node=1-->

Funky hotplug... nothing to see here, then:

Quoted 1 line

	<--------Node=2-------->

I must have missed something, but how can this happen?

I had memory that this cannot happen because the BIOS always allocates address ranges for all NUMA nodes during machine boot. Those address ranges don't necessarily need to have DIMM fully populated but they don't change during machine's runtime.

Quoted 20 lines · Kai

Quoted 19 lines · Dave Hansen

Quoted 17 lines · Kai

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 7 lines

Quoted 2 lines

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

Right. The old app will break w/o knowing the new ABI. One resolution, I think, is we don't introduce new userspace ABI, but hide "TDX-capable" and "non- TDX-capable" nodes in the kernel, and let kernel to enforce always allocating TDX guest memory from those "TDX-capable" nodes.

That doesn't actually hide all of the behavior from users. Let's say they do:

numactl --membind=6 qemu-kvm ...

In other words, take all of this guest's memory and put it on node 6. There lots of free memory on node 6 which is TDX-*IN*compatible. Then, they make it a TDX guest:

numactl --membind=6 qemu-kvm -tdx ...

What happens? Does the kernel silently ignore the --membind=6? Or does it return -ENOMEM somewhere and confuse the user who has *LOTS* of free memory on node 6.

In other words, I don't think the kernel can just enforce this internally and hide it from userspace.

Quoted 15 lines

Quoted 6 lines

Is there something fundamental that keeps a memory area that spans two nodes from being removed and then a new area added that is comprised of a single node?
Boot time:

Quoted 6 lines

	| memblock  |  memblock |
	<--Node=0--> <--Node=1-->

Funky hotplug... nothing to see here, then:

	<--------Node=2-------->

I must have missed something, but how can this happen?

Quoted 4 lines

I had memory that this cannot happen because the BIOS always allocates address ranges for all NUMA nodes during machine boot. Those address ranges don't necessarily need to have DIMM fully populated but they don't change during machine's runtime.

Is your memory correct? Is there evidence, or requirements in any specification to support your memory?

Quoted 38 lines · Dave

Quoted 20 lines · Kai

Quoted 19 lines · Dave Hansen

Quoted 17 lines · Kai

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 7 lines

Quoted 2 lines

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

Right. The old app will break w/o knowing the new ABI. One resolution, I think, is we don't introduce new userspace ABI, but hide "TDX-capable" and "non- TDX-capable" nodes in the kernel, and let kernel to enforce always allocating TDX guest memory from those "TDX-capable" nodes.

That doesn't actually hide all of the behavior from users. Let's say they do:

numactl --membind=6 qemu-kvm ...

In other words, take all of this guest's memory and put it on node 6. There lots of free memory on node 6 which is TDX-*IN*compatible. Then, they make it a TDX guest:

Quoted 8 lines

numactl --membind=6 qemu-kvm -tdx ...

What happens? Does the kernel silently ignore the --membind=6? Or does it return -ENOMEM somewhere and confuse the user who has *LOTS* of free memory on node 6.

In other words, I don't think the kernel can just enforce this internally and hide it from userspace.

IIUC, the kernel, for instance KVM who has knowledge the 'task_struct' is a TDX guest, can manually AND "TDX-capable" node masks to task's mempolicy, so that the memory will always be allocated from those "TDX-capable" nodes. KVM can refuse to create the TDX guest if it found task's mempolicy doesn't have any "TDX-capable" node, and print out a clear message to the userspace.

But I am new to the core-mm, so I might have some misunderstanding.

Quoted 54 lines · Kai

Quoted 38 lines · Dave

Quoted 20 lines · Kai

Quoted 19 lines · Dave Hansen

Quoted 17 lines · Kai

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 7 lines

Quoted 2 lines

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

Right. The old app will break w/o knowing the new ABI. One resolution, I think, is we don't introduce new userspace ABI, but hide "TDX-capable" and "non- TDX-capable" nodes in the kernel, and let kernel to enforce always allocating TDX guest memory from those "TDX-capable" nodes.

That doesn't actually hide all of the behavior from users. Let's say they do:

numactl --membind=6 qemu-kvm ...

In other words, take all of this guest's memory and put it on node 6. There lots of free memory on node 6 which is TDX-*IN*compatible. Then, they make it a TDX guest:

Quoted 8 lines

numactl --membind=6 qemu-kvm -tdx ...

What happens? Does the kernel silently ignore the --membind=6? Or does it return -ENOMEM somewhere and confuse the user who has *LOTS* of free memory on node 6.

In other words, I don't think the kernel can just enforce this internally and hide it from userspace.

IIUC, the kernel, for instance KVM who has knowledge the 'task_struct' is a TDX guest, can manually AND "TDX-capable" node masks to task's mempolicy, so that the memory will always be allocated from those "TDX-capable" nodes. KVM can refuse to create the TDX guest if it found task's mempolicy doesn't have any "TDX-capable" node, and print out a clear message to the userspace.

Quoted 1 line

But I am new to the core-mm, so I might have some misunderstanding.

KVM here means in-kernel KVM module? If so, KVM can only output some message in dmesg. Which isn't very good for users to digest. It's better for the user space QEMU to detect whether current configuration is usable and respond to users, via GUI, or syslog, etc.

Quoted 58 lines · Ying

Quoted 55 lines · Kai

Quoted 38 lines · Dave

Quoted 20 lines · Kai

Quoted 19 lines · Dave Hansen

Quoted 17 lines · Kai

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 7 lines

Quoted 2 lines

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

Right. The old app will break w/o knowing the new ABI. One resolution, I think, is we don't introduce new userspace ABI, but hide "TDX-capable" and "non- TDX-capable" nodes in the kernel, and let kernel to enforce always allocating TDX guest memory from those "TDX-capable" nodes.

That doesn't actually hide all of the behavior from users. Let's say they do:

numactl --membind=6 qemu-kvm ...

In other words, take all of this guest's memory and put it on node 6. There lots of free memory on node 6 which is TDX-*IN*compatible. Then, they make it a TDX guest:

Quoted 8 lines

numactl --membind=6 qemu-kvm -tdx ...

What happens? Does the kernel silently ignore the --membind=6? Or does it return -ENOMEM somewhere and confuse the user who has *LOTS* of free memory on node 6.

In other words, I don't think the kernel can just enforce this internally and hide it from userspace.

IIUC, the kernel, for instance KVM who has knowledge the 'task_struct' is a TDX guest, can manually AND "TDX-capable" node masks to task's mempolicy, so that the memory will always be allocated from those "TDX-capable" nodes. KVM can refuse to create the TDX guest if it found task's mempolicy doesn't have any "TDX-capable" node, and print out a clear message to the userspace.

But I am new to the core-mm, so I might have some misunderstanding.

Quoted 4 lines

KVM here means in-kernel KVM module? If so, KVM can only output some message in dmesg. Which isn't very good for users to digest. It's better for the user space QEMU to detect whether current configuration is usable and respond to users, via GUI, or syslog, etc.

I am not against this. For instance, maybe we can add some dedicated error code and let KVM return it to Qemu, but I don't want to speak for KVM guys. We can discuss this more when we have patches actually sent out to the community.

Quoted 64 lines · Kai

Quoted 62 lines · Ying

Quoted 48 lines · Kai

Quoted 38 lines · Dave

Quoted 20 lines · Kai

Quoted 19 lines · Dave Hansen

Quoted 17 lines · Kai

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

Why does it need to be enhanced? What's the problem?

The problem is after TDX module initialization, no more memory can be hot-added to the page allocator.

Kirill suggested this may not be ideal. With the existing NUMA ABIs we can actually have both TDX-capable and non-TDX-capable NUMA nodes online. We can bind TDX workloads to TDX-capable nodes while other non-TDX workloads can utilize all memory.

But probably it is not necessarily to call out in the changelog?

Quoted 7 lines

Quoted 2 lines

Let's say that we add this TDX-compatible-node ABI in the future. What will old code do that doesn't know about this ABI?

Right. The old app will break w/o knowing the new ABI. One resolution, I think, is we don't introduce new userspace ABI, but hide "TDX-capable" and "non- TDX-capable" nodes in the kernel, and let kernel to enforce always allocating TDX guest memory from those "TDX-capable" nodes.

That doesn't actually hide all of the behavior from users. Let's say they do:

numactl --membind=6 qemu-kvm ...

In other words, take all of this guest's memory and put it on node 6. There lots of free memory on node 6 which is TDX-*IN*compatible. Then, they make it a TDX guest:

Quoted 8 lines

numactl --membind=6 qemu-kvm -tdx ...

What happens? Does the kernel silently ignore the --membind=6? Or does it return -ENOMEM somewhere and confuse the user who has *LOTS* of free memory on node 6.

In other words, I don't think the kernel can just enforce this internally and hide it from userspace.

Quoted 7 lines

IIUC, the kernel, for instance KVM who has knowledge the 'task_struct' is a TDX guest, can manually AND "TDX-capable" node masks to task's mempolicy, so that the memory will always be allocated from those "TDX-capable" nodes. KVM can refuse to create the TDX guest if it found task's mempolicy doesn't have any "TDX-capable" node, and print out a clear message to the userspace.

But I am new to the core-mm, so I might have some misunderstanding.

KVM here means in-kernel KVM module? If so, KVM can only output some message in dmesg. Which isn't very good for users to digest. It's better for the user space QEMU to detect whether current configuration is usable and respond to users, via GUI, or syslog, etc.

Quoted 3 lines

I am not against this. For instance, maybe we can add some dedicated error code and let KVM return it to Qemu, but I don't want to speak for KVM guys. We can discuss this more when we have patches actually sent out to the community.

Error code is a kind of ABI too. :-)

Quoted 37 lines

Quoted 28 lines

Quoted 28 lines

Quoted 16 lines

Quoted 16 lines

+       list_for_each_entry(tmb, &tdx_memlist, list) {
+               /*
+                * The new range is TDX memory if it is fully covered by
+                * any TDX memory block.
+                *
+                * Note TDX memory blocks are originated from memblock
+                * memory regions, which can only be contiguous when two
+                * regions have different NUMA nodes or flags. Therefore
+                * the new range cannot cross multiple TDX memory blocks.
+                */
+               if (start_pfn >= tmb->start_pfn && end_pfn <= tmb-

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

end_pfn)

+                       return true;
+       }
+       return false;
+}

I don't really like that comment.  It should first state its behavior and assumptions, like:

     This check assumes that the start_pfn<->end_pfn range does not cross multiple tdx_memlist entries.

Quoted 6 lines

Quoted 6 lines

Only then should it describe why that is OK:

     A single memory hotplug even across mutliple memblocks (from which tdx_memlist entries are derived) is impossible.  ... then actually explain

How about below?

Quoted 12 lines

         /*
          * This check assumes that the start_pfn<->end_pfn range does not cross
          * multiple tdx_memlist entries. A single memory hotplug event across
          * multiple memblocks (from which tdx_memlist entries are derived) is
          * impossible. That means start_pfn<->end_pfn range cannot exceed a
          * tdx_memlist entry, and the new range is TDX memory if it is fully
          * covered by any tdx_memlist entry.
          */

I was hoping you would actually explain why it is impossible.

Is there something fundamental that keeps a memory area that spans two nodes from being removed and then a new area added that is comprised of a single node?

Boot time:

	| memblock  |  memblock |
	<--Node=0--> <--Node=1-->

Funky hotplug... nothing to see here, then:

	<--------Node=2-------->

I would believe that there is no current bare-metal TDX system that has an implementation like this.  But, the comments above speak like it's fundamentally impossible.  That should be clarified.

In other words, that comment talks about memblock attributes as being the core underlying reason that that simplified check is OK.  Is that it, or is it really the reduced hotplug feature set on TDX systems?

As a step of initializing the TDX module, the kernel needs to tell the TDX module which memory regions can be used by the TDX module as TDX guest memory.

TDX reports a list of "Convertible Memory Region" (CMR) to tell the kernel which memory is TDX compatible. The kernel needs to build a list of memory regions (out of CMRs) as "TDX-usable" memory and pass them to the TDX module. Once this is done, those "TDX-usable" memory regions are fixed during module's lifetime.

The initial support of TDX guests will only allocate TDX guest memory from the global page allocator. To keep things simple, just make sure all pages in the page allocator are TDX memory.

To guarantee that, stash off the memblock memory regions at the time of initializing the TDX module as TDX's own usable memory regions, and in the meantime, register a TDX memory notifier to reject to online any new memory in memory hotplug.

This approach works as in practice all boot-time present DIMMs are TDX convertible memory. However, if any non-TDX-convertible memory has been hot-added (i.e. CXL memory via kmem driver) before initializing the TDX module, the module initialization will fail.

This can also be enhanced in the future, i.e. by allowing adding non-TDX memory to a separate NUMA node. In this case, the "TDX-capable" nodes and the "non-TDX-capable" nodes can co-exist, but the kernel/userspace needs to guarantee memory pages for TDX guests are always allocated from the "TDX-capable" nodes.

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

---

v7 -> v8:
- Trimed down changelog (Dave).
- Changed to use PHYS_PFN() and PFN_PHYS() throughout this series (Ying).
- Moved memory hotplug handling from add_arch_memory() to memory_notifier (Dan/David).
- Removed 'nid' from 'struct tdx_memblock' to later patch (Dave).
- {build|free}_tdx_memory() -> {build|}free_tdx_memlist() (Dave).
- Removed pfn_covered_by_cmr() check as no code to trim CMRs now.
- Improve the comment around first 1MB (Dave).
- Added a comment around reserve_real_mode() to point out TDX code relies on first 1MB being reserved (Ying).
- Added comment to explain why the new online memory range cannot cross multiple TDX memory blocks (Dave).
- Improved other comments (Dave).

---
 arch/x86/Kconfig            |   1 +
 arch/x86/kernel/setup.c     |   2 +
 arch/x86/virt/vmx/tdx/tdx.c | 160 +++++++++++++++++++++++++++++++++++-
 3 files changed, 162 insertions(+), 1 deletion(-)

arch/x86/Kconfig +1 -0

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

arch/x86/kernel/setup.c +2 -0

index 216fee7144ee..3a841a77fda4 100644
@@ -1174,6 +1174,8 @@ void __init setup_arch(char **cmdline_p)
 	 *
 	 * Moreover, on machines with SandyBridge graphics or in setups that use
 	 * crashkernel the entire 1M is reserved anyway.
+	 *
+	 * Note the host kernel TDX also requires the first 1MB being reserved.
 	 */
 	reserve_real_mode();
 

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

index 6fe505c32599..f010402f443d 100644
@@ -13,6 +13,13 @@
 #include <linux/errno.h>
 #include <linux/printk.h>
 #include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/memory.h>
+#include <linux/minmax.h>
+#include <linux/sizes.h>
+#include <linux/pfn.h>
 #include <asm/pgtable_types.h>
 #include <asm/msr.h>
 #include <asm/tdx.h>

@@ -25,6 +32,12 @@ enum tdx_module_status_t {
 	TDX_MODULE_ERROR
 };
 
+struct tdx_memblock {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
 static u32 tdx_keyid_start __ro_after_init;
 static u32 nr_tdx_keyids __ro_after_init;
 

@@ -32,6 +45,9 @@ static enum tdx_module_status_t tdx_module_status;
 /* Prevent concurrent attempts on TDX detection and initialization */
 static DEFINE_MUTEX(tdx_module_lock);
 
+/* All TDX-usable memory regions */
+static LIST_HEAD(tdx_memlist);
+
 /*
  * tdx_keyid_start and nr_tdx_keyids indicate that TDX is uninitialized.
  * This is used in TDX initialization error paths to take it from

@@ -69,6 +85,50 @@ static int __init record_keyid_partitioning(void)
 	return 0;
 }
 
+static bool is_tdx_memory(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	/* Empty list means TDX isn't enabled. */
+	if (list_empty(&tdx_memlist))
+		return true;
+
+	list_for_each_entry(tmb, &tdx_memlist, list) {
+		/*
+		 * The new range is TDX memory if it is fully covered by
+		 * any TDX memory block.
+		 *
+		 * Note TDX memory blocks are originated from memblock
+		 * memory regions, which can only be contiguous when two
+		 * regions have different NUMA nodes or flags.  Therefore
+		 * the new range cannot cross multiple TDX memory blocks.
+		 */
+		if (start_pfn >= tmb->start_pfn && end_pfn <= tmb->end_pfn)
+			return true;
+	}
+	return false;
+}
+
+static int tdx_memory_notifier(struct notifier_block *nb, unsigned long action,
+			       void *v)
+{
+	struct memory_notify *mn = v;
+
+	if (action != MEM_GOING_ONLINE)
+		return NOTIFY_OK;
+
+	/*
+	 * Not all memory is compatible with TDX.  Reject
+	 * to online any incompatible memory.
+	 */
+	return is_tdx_memory(mn->start_pfn, mn->start_pfn + mn->nr_pages) ?
+		NOTIFY_OK : NOTIFY_BAD;
+}
+
+static struct notifier_block tdx_memory_nb = {
+	.notifier_call = tdx_memory_notifier,
+};
+
 static int __init tdx_init(void)
 {
 	int err;

@@ -89,6 +149,13 @@ static int __init tdx_init(void)
 		goto no_tdx;
 	}
 
+	err = register_memory_notifier(&tdx_memory_nb);
+	if (err) {
+		pr_info("initialization failed: register_memory_notifier() failed (%d)\n",
+				err);
+		goto no_tdx;
+	}
+
 	return 0;
 no_tdx:
 	clear_tdx();

@@ -209,6 +276,77 @@ static int tdx_get_sysinfo(struct tdsysinfo_struct *sysinfo,
 	return 0;
 }
 
+/*
+ * Add a memory region as a TDX memory block.  The caller must make sure
+ * all memory regions are added in address ascending order and don't
+ * overlap.
+ */
+static int add_tdx_memblock(struct list_head *tmb_list, unsigned long start_pfn,
+			    unsigned long end_pfn)
+{
+	struct tdx_memblock *tmb;
+
+	tmb = kmalloc(sizeof(*tmb), GFP_KERNEL);
+	if (!tmb)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&tmb->list);
+	tmb->start_pfn = start_pfn;
+	tmb->end_pfn = end_pfn;
+
+	list_add_tail(&tmb->list, tmb_list);
+	return 0;
+}
+
+static void free_tdx_memlist(struct list_head *tmb_list)
+{
+	while (!list_empty(tmb_list)) {
+		struct tdx_memblock *tmb = list_first_entry(tmb_list,
+				struct tdx_memblock, list);
+
+		list_del(&tmb->list);
+		kfree(tmb);
+	}
+}
+
+/*
+ * Ensure that all memblock memory regions are convertible to TDX
+ * memory.  Once this has been established, stash the memblock
+ * ranges off in a secondary structure because memblock is modified
+ * in memory hotplug while TDX memory regions are fixed.
+ */
+static int build_tdx_memlist(struct list_head *tmb_list)
+{
+	unsigned long start_pfn, end_pfn;
+	int i, ret;
+
+	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, NULL) {
+		/*
+		 * The first 1MB is not reported as TDX convertible memory.
+		 * Although the first 1MB is always reserved and won't end up
+		 * to the page allocator, it is still in memblock's memory
+		 * regions.  Skip them manually to exclude them as TDX memory.
+		 */
+		start_pfn = max(start_pfn, PHYS_PFN(SZ_1M));
+		if (start_pfn >= end_pfn)
+			continue;
+
+		/*
+		 * Add the memory regions as TDX memory.  The regions in
+		 * memblock has already guaranteed they are in address
+		 * ascending order and don't overlap.
+		 */
+		ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	free_tdx_memlist(tmb_list);
+	return ret;
+}
+
 static int init_tdx_module(void)
 {
 	/*

@@ -226,10 +364,25 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out;
 
+	/*
+	 * The initial support of TDX guests only allocates memory from
+	 * the global page allocator.  To keep things simple, just make
+	 * sure all pages in the page allocator are TDX memory.
+	 *
+	 * Build the list of "TDX-usable" memory regions which cover all
+	 * pages in the page allocator to guarantee that.  Do it while
+	 * holding mem_hotplug_lock read-lock as the memory hotplug code
+	 * path reads the @tdx_memlist to reject any new memory.
+	 */
+	get_online_mems();
+
+	ret = build_tdx_memlist(&tdx_memlist);
+	if (ret)
+		goto out;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Build the list of TDX-usable memory regions.
 	 *  - Construct a list of TDMRs to cover all TDX-usable memory
 	 *    regions.
 	 *  - Pick up one TDX private KeyID as the global KeyID.

@@ -241,6 +394,11 @@ static int init_tdx_module(void)
 	 */
 	ret = -EINVAL;
 out:
+	/*
+	 * @tdx_memlist is written here and read at memory hotplug time.
+	 * Lock out memory hotplug code while building it.
+	 */
+	put_online_mems();
 	return ret;
 }
 

+Dave, Oscar and Andrew.

Hi Memory hotplug maintainers,

Sorry to CC, but could you help to review this Intel TDX (Trusted Domain Extensions) patch, since it is related to memory hotplug (not modifying any common memory hotplug directly, though)? Dave suggested it's better to get memory hotplug guys to help to review sooner than later.

This whole series already has [email protected] in the CC list. Thanks for your time.

Quoted 9 lines · Kai

+Dave, Oscar and Andrew.

Hi Memory hotplug maintainers,

Sorry to CC, but could you help to review this Intel TDX (Trusted Domain Extensions) patch, since it is related to memory hotplug (not modifying any common memory hotplug directly, though)? Dave suggested it's better to get memory hotplug guys to help to review sooner than later.

Quoted 2 lines

This whole series already has [email protected] in the CC list. Thanks for your time.

Hi,

I remember discussing that part (notifier) already and it looked good to me. No objection from my side.

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

+/* Calculate the actual TDMR size */
+static int tdmr_size_single(u16 max_reserved_per_tdmr)
+{
+	int tdmr_sz;
+
+	/*
+	 * The actual size of TDMR depends on the maximum
+	 * number of reserved areas.
+	 */
+	tdmr_sz = sizeof(struct tdmr_info);
+	tdmr_sz += sizeof(struct tdmr_reserved_area) * max_reserved_per_tdmr;
+
+	return ALIGN(tdmr_sz, TDMR_INFO_ALIGNMENT);
+}
+
+static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
+			   struct tdsysinfo_struct *sysinfo)
+{
+	size_t tdmr_sz, tdmr_array_sz;
+	void *tdmr_array;
+
+	tdmr_sz = tdmr_size_single(sysinfo->max_reserved_per_tdmr);
+	tdmr_array_sz = tdmr_sz * sysinfo->max_tdmrs;
+
+	/*
+	 * To keep things simple, allocate all TDMRs together.

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

+	 * Keep the size of TDMR to find the target TDMR
+	 * at a given index in the TDMR list.
+	 */
+	tdmr_list->tdmr_sz = tdmr_sz;
+	tdmr_list->max_tdmrs = sysinfo->max_tdmrs;
+	tdmr_list->nr_tdmrs = 0;
+
+	return 0;
+}
+
+static void free_tdmr_list(struct tdmr_info_list *tdmr_list)
+{
+	free_pages_exact(tdmr_list->first_tdmr,
+			tdmr_list->max_tdmrs * tdmr_list->tdmr_sz);
+}
+
+/*
+ * Construct a list of TDMRs on the preallocated space in @tdmr_list
+ * to cover all TDX memory regions in @tmb_list based on the TDX module
+ * information in @sysinfo.
+ */
+static int construct_tdmrs(struct list_head *tmb_list,
+			   struct tdmr_info_list *tdmr_list,
+			   struct tdsysinfo_struct *sysinfo)
+{
+	/*
+	 * TODO:
+	 *
+	 *  - Fill out TDMRs to cover all TDX memory regions.
+	 *  - Allocate and set up PAMTs for each TDMR.
+	 *  - Designate reserved areas for each TDMR.
+	 *
+	 * Return -EINVAL until constructing TDMRs is done
+	 */
+	return -EINVAL;
+}
+
 static int init_tdx_module(void)
 {
 	/*

@@ -358,6 +439,7 @@ static int init_tdx_module(void)
 			TDSYSINFO_STRUCT_SIZE, TDSYSINFO_STRUCT_ALIGNMENT);
 	struct cmr_info cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
 	struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
+	struct tdmr_info_list tdmr_list;
 	int ret;
 
 	ret = tdx_get_sysinfo(sysinfo, cmr_array);

@@ -380,11 +462,19 @@ static int init_tdx_module(void)
 	if (ret)
 		goto out;
 
+	/* Allocate enough space for constructing TDMRs */
+	ret = alloc_tdmr_list(&tdmr_list, sysinfo);
+	if (ret)
+		goto out_free_tdx_mem;
+
+	/* Cover all TDX-usable memory regions in TDMRs */
+	ret = construct_tdmrs(&tdx_memlist, &tdmr_list, sysinfo);
+	if (ret)
+		goto out_free_tdmrs;
+
 	/*
 	 * TODO:
 	 *
-	 *  - Construct a list of TDMRs to cover all TDX-usable memory
-	 *    regions.
 	 *  - Pick up one TDX private KeyID as the global KeyID.
 	 *  - Configure the TDMRs and the global KeyID to the TDX module.
 	 *  - Configure the global KeyID on all packages.

@@ -393,6 +483,16 @@ static int init_tdx_module(void)
 	 *  Return error before all steps are done.
 	 */
 	ret = -EINVAL;
+out_free_tdmrs:
+	/*
+	 * Free the space for the TDMRs no matter the initialization is
+	 * successful or not.  They are not needed anymore after the
+	 * module initialization.
+	 */
+	free_tdmr_list(&tdmr_list);
+out_free_tdx_mem:
+	if (ret)
+		free_tdx_memlist(&tdx_memlist);
 out:
 	/*
 	 * @tdx_memlist is written here and read at memory hotplug time.

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

index 6d32f62e4182..d0c762f1a94c 100644
@@ -90,6 +90,29 @@ struct tdsysinfo_struct {
 	DECLARE_FLEX_ARRAY(struct cpuid_config, cpuid_configs);
 } __packed;
 
+struct tdmr_reserved_area {
+	u64 offset;
+	u64 size;
+} __packed;
+
+#define TDMR_INFO_ALIGNMENT	512
+
+struct tdmr_info {
+	u64 base;
+	u64 size;
+	u64 pamt_1g_base;
+	u64 pamt_1g_size;
+	u64 pamt_2m_base;
+	u64 pamt_2m_size;
+	u64 pamt_4k_base;
+	u64 pamt_4k_size;
+	/*
+	 * Actual number of reserved areas depends on
+	 * 'struct tdsysinfo_struct'::max_reserved_per_tdmr.
+	 */
+	DECLARE_FLEX_ARRAY(struct tdmr_reserved_area, reserved_areas);
+} __packed __aligned(TDMR_INFO_ALIGNMENT);
+
 /*
  * Do not put any hardware-defined TDX structure representations below
  * this comment!

arch/x86/virt/vmx/tdx/tdx.c static int build_tdx_memlist(struct list_head *tmb_list) · line 352

Quoted 2 lines

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

+struct tdmr_info_list {
+	struct tdmr_info *first_tdmr;

This is named badly. This is really a pointer to an array. While it _does_ of course point to the first member of the array, the naming should make it clear that there are multiple tdmr_infos here.

arch/x86/virt/vmx/tdx/tdx.c static int build_tdx_memlist(struct list_head *tmb_list) · line 356

Quoted 5 lines

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

+	int tdmr_sz;
+	int max_tdmrs;
+	int nr_tdmrs;	/* Actual number of TDMRs */
+};

This 'tdmr_info_list's is declared in an unfortunate place. I thought the tdmr_size_single() function below was related to it.

Also, tdmr_sz and max_tdmrs can both be derived from 'sysinfo'. Do they really need to be stored here?

max_reserved_per_tdmr):

If so, I think I'd probably do something
like this with the structure:

struct tdmr_info_list {
struct tdmr_info *tdmrs;
int nr_consumed_tdmrs; // How many @tdmrs are in use

/* Metadata for freeing this structure: */
int tdmr_sz; // Size of one 'tdmr_info' (has a flex array)
int max_tdmrs; // How many @tdmrs are allocated
};

Modulo whataver folks are doing for comments these days.

Quoted 32 lines

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

+/* Calculate the actual TDMR size */
+static int tdmr_size_single(u16 max_reserved_per_tdmr)
+{
+	int tdmr_sz;
+
+	/*
+	 * The actual size of TDMR depends on the maximum
+	 * number of reserved areas.
+	 */
+	tdmr_sz = sizeof(struct tdmr_info);
+	tdmr_sz += sizeof(struct tdmr_reserved_area) *

max_reserved_per_tdmr;

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

+
+	return ALIGN(tdmr_sz, TDMR_INFO_ALIGNMENT);
+}
+
+static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
+			   struct tdsysinfo_struct *sysinfo)
+{
+	size_t tdmr_sz, tdmr_array_sz;
+	void *tdmr_array;
+
+	tdmr_sz = tdmr_size_single(sysinfo->max_reserved_per_tdmr);
+	tdmr_array_sz = tdmr_sz * sysinfo->max_tdmrs;
+
+	/*
+	 * To keep things simple, allocate all TDMRs together.
+	 * The buffer needs to be physically contiguous to make
+	 * sure each TDMR is physically contiguous.
+	 */
+	tdmr_array = alloc_pages_exact(tdmr_array_sz,

arch/x86/virt/vmx/tdx/tdx.c static int build_tdx_memlist(struct list_head *tmb_list) · line 393

Quoted 6 lines

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

+			GFP_KERNEL | __GFP_ZERO);
+	if (!tdmr_array)
+		return -ENOMEM;
+
+	tdmr_list->first_tdmr = tdmr_array;
+	/*

^ probably missing whitepsace before the comment

Quoted 10 lines

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

+	 * Keep the size of TDMR to find the target TDMR
+	 * at a given index in the TDMR list.
+	 */
+	tdmr_list->tdmr_sz = tdmr_sz;
+	tdmr_list->max_tdmrs = sysinfo->max_tdmrs;
+	tdmr_list->nr_tdmrs = 0;
+
+	return 0;
+}
+

On Fri, 2023-01-06 at 11:24 -0800, Dave Hansen wrote:

Quoted 2 lines

Also, tdmr_sz and max_tdmrs can both be derived from 'sysinfo'. Do they really need to be stored here?

It's not mandatory to keep them here. I did it mainly because I want to avoid passing 'sysinfo' as argument for almost all functions related to constructing TDMRs.

Quoted 12 lines

If so, I think I'd probably do something
like this with the structure:

struct tdmr_info_list {
struct tdmr_info *tdmrs;
int nr_consumed_tdmrs; // How many @tdmrs are in use

/* Metadata for freeing this structure: */
int tdmr_sz; // Size of one 'tdmr_info' (has a flex array)
int max_tdmrs; // How many @tdmrs are allocated
};

Quoted 1 line

Modulo whataver folks are doing for comments these days.

Looks nice to me. Will use. A slight thing is 'tdmr_sz' is also used to get the TDMR at a given index, but not just freeing the structure.

Btw, is C++ style comment "//" OK in kernel code?

Quoted 1 line · Kai

On Fri, 2023-01-06 at 11:24 -0800, Dave Hansen wrote:

...

Quoted 3 lines

Quoted 2 lines

Also, tdmr_sz and max_tdmrs can both be derived from 'sysinfo'. Do they really need to be stored here?

Quoted 3 lines

It's not mandatory to keep them here. I did it mainly because I want to avoid passing 'sysinfo' as argument for almost all functions related to constructing TDMRs.

I don't think it hurts readability that much. On the contrary, it makes it more clear what data is needed for initialization.