[v7,07/10] x86/virt/tdx: Trim away tail null CMRs

Commit Message

Huang, Kai Nov. 11, 2024, 10:39 a.m. UTC

TDX architecturally supports up to 32 CMRs.  The global metadata field
"NUM_CMRS" reports the number of CMR entries that can be read by the
kernel.  However, that field may just report the maximum number of CMRs
albeit the actual number of CMRs is smaller, in which case there are
tail null CMRs (size is 0).

Trim away those null CMRs, and print valid CMRs since they are useful
at least to developers.

More information about CMR can be found at "Intel TDX ISA Background:
Convertible Memory Ranges (CMRs)" in TDX 1.5 base spec [1], and
"CMR_INFO" in TDX 1.5 ABI spec [2].

Now get_tdx_sys_info() just reads kernel-needed global metadata to
kernel structure, and it is auto-generated.  Add a wrapper function
init_tdx_sys_info() to invoke get_tdx_sys_info() and provide room to do
additional things like dealing with CMRs.

Link: https://cdrdv2.intel.com/v1/dl/getContent/733575 [1]
Link: https://cdrdv2.intel.com/v1/dl/getContent/733579 [2]
Signed-off-by: Kai Huang <[e-mail]>
Reviewed-by: Dan Williams <[e-mail]>
---
 arch/x86/virt/vmx/tdx/tdx.c | 56 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

Comments

Nikolay Borisov Nov. 11, 2024, 4:32 p.m. UTC | #1

On 11.11.24 г. 12:39 ч., Kai Huang wrote:
> TDX architecturally supports up to 32 CMRs.  The global metadata field
> "NUM_CMRS" reports the number of CMR entries that can be read by the
> kernel.  However, that field may just report the maximum number of CMRs
> albeit the actual number of CMRs is smaller, in which case there are
> tail null CMRs (size is 0).

nit: Is it safe to assume that null CMRs are going to be sequential and 
always at the end? Nothing in the TDX module spec suggests this. I.e 
can't we have :


1. Valid CMR region
2. ZERO CMR
3. Valid CMR

Sure, it might be a dummy and pointless but nothing prevents such CMR 
records. In any case I think the mentioning of "tail" is a bit too much 
detail and adds to unnecessary mental overload. Simply say you trim 
empty CMR's and that such regions will be sequential (if that's the 
case) and be done with it.

Because having "tail null cmr" can be interpreted as also having  there 
might be "non-tail null CMR", which doesn't seem to be the case?

> 
> Trim away those null CMRs, and print valid CMRs since they are useful
> at least to developers.
> 
> More information about CMR can be found at "Intel TDX ISA Background:
> Convertible Memory Ranges (CMRs)" in TDX 1.5 base spec [1], and
> "CMR_INFO" in TDX 1.5 ABI spec [2].
> 
> Now get_tdx_sys_info() just reads kernel-needed global metadata to
> kernel structure, and it is auto-generated.  Add a wrapper function
> init_tdx_sys_info() to invoke get_tdx_sys_info() and provide room to do
> additional things like dealing with CMRs.
> 
> Link: https://cdrdv2.intel.com/v1/dl/getContent/733575 [1]
> Link: https://cdrdv2.intel.com/v1/dl/getContent/733579 [2]
> Signed-off-by: Kai Huang <[e-mail]>
> Reviewed-by: Dan Williams <[e-mail]>

Reviewed-by: Nikolay Borisov <[e-mail]>

Huang, Kai Nov. 11, 2024, 7:30 p.m. UTC | #2

On Mon, 2024-11-11 at 18:32 +0200, Nikolay Borisov wrote:
> 
> On 11.11.24 г. 12:39 ч., Kai Huang wrote:
> > TDX architecturally supports up to 32 CMRs.  The global metadata field
> > "NUM_CMRS" reports the number of CMR entries that can be read by the
> > kernel.  However, that field may just report the maximum number of CMRs
> > albeit the actual number of CMRs is smaller, in which case there are
> > tail null CMRs (size is 0).
> 
> nit: Is it safe to assume that null CMRs are going to be sequential and 
> always at the end? Nothing in the TDX module spec suggests this. I.e 
> can't we have :
> 
> 
> 1. Valid CMR region
> 2. ZERO CMR
> 3. Valid CMR
> 
> Sure, it might be a dummy and pointless but nothing prevents such CMR 
> records. In any case I think the mentioning of "tail" is a bit too much 
> detail and adds to unnecessary mental overload. Simply say you trim 
> empty CMR's and that such regions will be sequential (if that's the 
> case) and be done with it.
> 
> Because having "tail null cmr" can be interpreted as also having  there 
> might be "non-tail null CMR", which doesn't seem to be the case?

It's described in the comment in the code:

+	 * Note the CMRs are generated by the BIOS, but the MCHECK
+	 * verifies CMRs before enabling TDX on hardware.  Skip other
+	 * sanity checks (e.g., verify CMR is 4KB aligned) but trust
+	 * MCHECK to work properly.
+	 *
+	 * The spec doesn't say whether it's legal to have null CMRs
+	 * in the middle of valid CMRs.  For now assume no sane BIOS
+	 * would do that (even MCHECK allows).

I don't see why a sane BIOS would need to do that, and we have never seen such
case in reality.  IMO we don't need to be too skeptical now.  If we see this can
indeed happen in the future, we can always come up with a patch to fix.
 
[...]


> Reviewed-by: Nikolay Borisov <[e-mail]>
> 

Thanks!

Dave Hansen Nov. 11, 2024, 7:41 p.m. UTC | #3

On 11/11/24 02:39, Kai Huang wrote:
> TDX architecturally supports up to 32 CMRs.  The global metadata field
> "NUM_CMRS" reports the number of CMR entries that can be read by the
> kernel.  However, that field may just report the maximum number of CMRs
> albeit the actual number of CMRs is smaller, in which case there are
> tail null CMRs (size is 0).
> 
> Trim away those null CMRs, and print valid CMRs since they are useful
> at least to developers.
> 
> More information about CMR can be found at "Intel TDX ISA Background:
> Convertible Memory Ranges (CMRs)" in TDX 1.5 base spec [1], and
> "CMR_INFO" in TDX 1.5 ABI spec [2].
> 
> Now get_tdx_sys_info() just reads kernel-needed global metadata to
> kernel structure, and it is auto-generated.  Add a wrapper function
> init_tdx_sys_info() to invoke get_tdx_sys_info() and provide room to do
> additional things like dealing with CMRs.

I'm not sure I understand why this patch is here.

What does trimming buy us in the first place?

Huang, Kai Nov. 11, 2024, 8:22 p.m. UTC | #4

On Mon, 2024-11-11 at 11:41 -0800, Dave Hansen wrote:
> On 11/11/24 02:39, Kai Huang wrote:
> > TDX architecturally supports up to 32 CMRs.  The global metadata field
> > "NUM_CMRS" reports the number of CMR entries that can be read by the
> > kernel.  However, that field may just report the maximum number of CMRs
> > albeit the actual number of CMRs is smaller, in which case there are
> > tail null CMRs (size is 0).
> > 
> > Trim away those null CMRs, and print valid CMRs since they are useful
> > at least to developers.
> > 
> > More information about CMR can be found at "Intel TDX ISA Background:
> > Convertible Memory Ranges (CMRs)" in TDX 1.5 base spec [1], and
> > "CMR_INFO" in TDX 1.5 ABI spec [2].
> > 
> > Now get_tdx_sys_info() just reads kernel-needed global metadata to
> > kernel structure, and it is auto-generated.  Add a wrapper function
> > init_tdx_sys_info() to invoke get_tdx_sys_info() and provide room to do
> > additional things like dealing with CMRs.
> 
> I'm not sure I understand why this patch is here.
> 
> What does trimming buy us in the first place?

I think the global metadata provided by the core kernel should only reflect the
real valid CMRs via 'num_cmrs', so when the kernel uses CMRs it can just get
valid ones.

One immediate need is the next patch will need to loop over CMRs to set up
reserved areas for TDMRs.  If we don't trim here, we will need to explicitly
skip all null CMRs in each loop.  This will result in more duplicated code and
is not as clean as trimming at early IMO.

I should call this out here though.  

How about I clarify this in the changelog like below?

"
A future fix to a module initialization failure issue will need to loop over all
CMRs.  Trim away those null CMRs once for all here so that the kernel doesn't
need to explicitly skip them each time when it uses CMRs in later time.  Also
print valid CMRs since they are useful at least to developers.
"

Patch

diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 43ec56db5084..e81bdcfc20bf 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -272,6 +272,60 @@  static int read_sys_metadata_field(u64 field_id, u64 *data)
 
 #include "tdx_global_metadata.c"
 
+/* Update the @sysinfo_cmr->num_cmrs to trim tail null CMRs */
+static void trim_null_tail_cmrs(struct tdx_sys_info_cmr *sysinfo_cmr)
+{
+	int i;
+
+	/*
+	 * The TDX module may report the maximum number of CMRs that
+	 * TDX architecturally supports as the actual number of CMRs,
+	 * despite the latter is smaller.  In this case some tail
+	 * CMR(s) will be null (size is 0).  Trim them away.
+	 *
+	 * Note the CMRs are generated by the BIOS, but the MCHECK
+	 * verifies CMRs before enabling TDX on hardware.  Skip other
+	 * sanity checks (e.g., verify CMR is 4KB aligned) but trust
+	 * MCHECK to work properly.
+	 *
+	 * The spec doesn't say whether it's legal to have null CMRs
+	 * in the middle of valid CMRs.  For now assume no sane BIOS
+	 * would do that (even MCHECK allows).
+	 */
+	for (i = 0; i < sysinfo_cmr->num_cmrs; i++)
+		if (!sysinfo_cmr->cmr_size[i])
+			break;
+
+	sysinfo_cmr->num_cmrs = i;
+}
+
+static void print_cmrs(struct tdx_sys_info_cmr *sysinfo_cmr)
+{
+	int i;
+
+	for (i = 0; i < sysinfo_cmr->num_cmrs; i++) {
+		u64 cmr_base = sysinfo_cmr->cmr_base[i];
+		u64 cmr_size = sysinfo_cmr->cmr_size[i];
+
+		pr_info("CMR[%d]: [0x%llx, 0x%llx)\n", i, cmr_base,
+				cmr_base + cmr_size);
+	}
+}
+
+static int init_tdx_sys_info(struct tdx_sys_info *sysinfo)
+{
+	int ret;
+
+	ret = get_tdx_sys_info(sysinfo);
+	if (ret)
+		return ret;
+
+	trim_null_tail_cmrs(&sysinfo->cmr);
+	print_cmrs(&sysinfo->cmr);
+
+	return 0;
+}
+
 /* Calculate the actual TDMR size */
 static int tdmr_size_single(u16 max_reserved_per_tdmr)
 {
@@ -1051,7 +1105,7 @@  static int init_tdx_module(void)
 	struct tdx_sys_info sysinfo;
 	int ret;
 
-	ret = get_tdx_sys_info(&sysinfo);
+	ret = init_tdx_sys_info(&sysinfo);
 	if (ret)
 		return ret;