Re: [PATCH 8/9] x86/virt/tdx: Exclude memory region hole within CMR as TDMR's reserved area

[Nikolay Borisov]

On 16.06.24 г. 15:01 ч., Kai Huang wrote:
> A TDX module initialization failure was reported on a Emerald Rapids
> platform:
>
>    virt/tdx: initialization failed: TDMR [0x0, 0x80000000): reserved areas exhausted.
>    virt/tdx: module initialization failed (-28)
>
> As a step of initializing the TDX module, the kernel tells the TDX
> module all the "TDX-usable memory regions" via a set of TDX architecture
> defined structure "TD Memory Region" (TDMR).  Each TDMR must be in 1GB
> aligned and in 1GB granularity, and all "non-TDX-usable memory holes" in
> a given TDMR must be marked as a "reserved area".  Each TDMR only
> supports a maximum number of reserved areas reported by the TDX module.
>
> As shown above, the root cause of this failure is when the kernel tries
> to construct a TDMR to cover address range [0x0, 0x80000000), there
> are too many memory holes within that range and the number of memory
> holes exceeds the maximum number of reserved areas.
>
> The E820 table of that platform (see [1] below) reflects this: the
> number of memory holes among e820 "usable" entries exceeds 16, which is
> the maximum number of reserved areas TDX module supports in practice.
>
> === Fix ===
>
> There are two options to fix this: 1) put less memory holes as "reserved
> area" when constructing a TDMR; 2) reduce the TDMR's size to cover less
> memory regions, thus less memory holes.
>
> Option 1) is possible, and in fact is easier and preferable:
>
> TDX actually has a concept of "Convertible Memory Regions" (CMRs).  TDX
> reports a list of CMRs that meet TDX's security requirements on memory.
> TDX requires all the "TDX-usable memory regions" that the kernel passes
> to the module via TDMRs, a.k.a, all the "non-reserved regions in TDMRs",
> must be convertible memory.
>
> In other words, if a memory hole is indeed CMR, then it's not mandatory

So TDX requires all TDMR to be CMR, and CMR regions are reported by the 
BIOS, how did you arrive at the conclusion that if a hole is CMR there 
is no point in creating a TDMR for it?

> for the kernel to add it to the reserved areas.  The number of consumed
> reserved areas can be reduced if the kernel doesn't add those memory
> holes as reserved area.  Note this doesn't have security impact because
> the kernel is out of TDX's TCB anyway.
>
> This is feasible because in practice the CMRs just reflect the nature of
> whether the RAM can indeed be used by TDX, thus each CMR tends to be a
> large range w/o being split into small areas, e.g., in the way the e820
> table does to contain a lot "ACPI *" entries.  [2] below shows the CMRs
> reported on the problematic platform (using the off-tree TDX code).
>
> So for this particular module initialization failure, the memory holes
> that are within [0x0, 0x80000000) are mostly indeed CMR.  By not adding
> them to the reserved areas, the number of consumed reserved areas for
> the TDMR [0x0, 0x80000000) can be dramatically reduced.
>
> On the other hand, although option 2) is also theoretically feasible, it
> requires more complicated logic to handle around splitting TDMR into
> smaller ones.  E.g., today one memory region must be fully in one TDMR,
> while splitting TDMR will result in each TDMR only covering part of some
> memory region.  And this also increases the total number of TDMRs, which
> also cannot exceed a maximum value that TDX module supports.

<snip>

> Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> ---
>   arch/x86/virt/vmx/tdx/tdx.c | 149 ++++++++++++++++++++++++++++++++----
>   arch/x86/virt/vmx/tdx/tdx.h |  13 ++++
>   2 files changed, 146 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index ced40e3b516e..88a0c8b788b7 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -293,6 +293,10 @@ static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
>   	return 0;
>   }
>   
> +/* Wrapper to read one metadata field to u8/u16/u32/u64 */
> +#define stbuf_read_sysmd_single(_field_id, _pdata)	\
> +	stbuf_read_sysmd_field(_field_id, _pdata, 0, sizeof(typeof(*(_pdata))))

What value does adding yet another level of indirection bring here?

> +
>   struct field_mapping {
>   	u64 field_id;
>   	int offset;
> @@ -349,6 +353,76 @@ static int get_tdx_module_version(struct tdx_sysinfo_module_version *modver)
>   	return stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields), modver);
>   }
>   
> +/* Update the @cmr_info->num_cmrs to trim tail empty CMRs */
> +static void trim_empty_tail_cmrs(struct tdx_sysinfo_cmr_info *cmr_info)
> +{
> +	int i;
> +
> +	for (i = 0; i < cmr_info->num_cmrs; i++) {
> +		u64 cmr_base = cmr_info->cmr_base[i];
> +		u64 cmr_size = cmr_info->cmr_size[i];
> +
> +		if (!cmr_size) {
> +			WARN_ON_ONCE(cmr_base);
> +			break;
> +		}
> +
> +		/* TDX architecture: CMR must be 4KB aligned */
> +		WARN_ON_ONCE(!PAGE_ALIGNED(cmr_base) ||
> +				!PAGE_ALIGNED(cmr_size));
> +	}
> +
> +	cmr_info->num_cmrs = i;
> +}

That function is somewhat weird, on the one hand its name suggests it's 
doing some "optimisation" i.e removing empty cmrs, at the same time it 
will simply cap the number of CMRs until it meets the first empty CMR, 
what aif we have and will also WARN. In fact it could even crash the 
machine if panic_on_warn is enabled, furthermore the alignement checks 
suggest it's actually some sanity checking function. Furthermore if we 
have:"

ORDINARY_CMR,EMPTY_CMR,ORDINARY_CMR

(Is such a scenario even possible), in this case we'll ommit also the 
last ordinary cmr region?

> +
> +#define TD_SYSINFO_MAP_CMR_INFO(_field_id, _member)	\
> +	TD_SYSINFO_MAP(_field_id, struct tdx_sysinfo_cmr_info, _member)

nit: Again, no real value in introducing yet another level of 
indirection in this case.

> +
> +static int get_tdx_cmr_info(struct tdx_sysinfo_cmr_info *cmr_info)
> +{
> +	int i, ret;
> +
> +	ret = stbuf_read_sysmd_single(MD_FIELD_ID_NUM_CMRS,
> +			&cmr_info->num_cmrs);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < cmr_info->num_cmrs; i++) {
> +		const struct field_mapping fields[] = {
> +			TD_SYSINFO_MAP_CMR_INFO(CMR_BASE0 + i, cmr_base[i]),
> +			TD_SYSINFO_MAP_CMR_INFO(CMR_SIZE0 + i, cmr_size[i]),
> +		};
> +
> +		ret = stbuf_read_sysmd_multi(fields, ARRAY_SIZE(fields),
> +				cmr_info);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * The TDX module may just report the maximum number of CMRs that
> +	 * TDX architecturally supports as the actual number of CMRs,
> +	 * despite the latter is smaller.  In this case all the tail
> +	 * CMRs will be empty.  Trim them away.
> +	 */
> +	trim_empty_tail_cmrs(cmr_info);
> +
> +	return 0;
> +}
> +
> +static void print_cmr_info(struct tdx_sysinfo_cmr_info *cmr_info)
> +{
> +	int i;
> +
> +	for (i = 0; i < cmr_info->num_cmrs; i++) {
> +		u64 cmr_base = cmr_info->cmr_base[i];
> +		u64 cmr_size = cmr_info->cmr_size[i];
> +
> +		pr_info("CMR[%d]: [0x%llx, 0x%llx)\n", i, cmr_base,
> +				cmr_base + cmr_size);
> +	}
> +}

Do we really want to always print all CMR regions, won't that become way 
too spammy and isn't this really useful in debug scenarios? Perhaps gate 
this particular information behind a debug flag?

> +
>   static void print_basic_sysinfo(struct tdx_sysinfo *sysinfo)
>   {
>   	struct tdx_sysinfo_module_version *modver = &sysinfo->module_version;

<snip>