Re: [PATCH v8 8.2/9] x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find memory holes

[Dave Hansen <dave.hansen@intel.com>]

On 12/8/24 22:50, Kai Huang wrote:
> A TDX module initialization failure was reported on an Emerald Rapids
> platform [*]:
> 
>   virt/tdx: initialization failed: TDMR [0x0, 0x80000000): reserved areas exhausted.
>   virt/tdx: module initialization failed (-28)
> 
> The kernel informs the TDX module of "TDX-usable memory regions" via the
> structure "TD Memory Region" (TDMR).  Each TDMR contains a limited
> number of "reserved areas" to inform the TDX module of the regions that
> cannot be used by TDX.
> 
> The kernel builds the list of "TDX-usable memory regions" from memblock
> (which reflects e820) and marks all memory holes as "reserved areas" in
> TDMRs.  It turns out on some large systems the holes in memblock can be
> too fine-grained [1] and exceed the number of reserved areas that the
> module can track per TDMR, resulting in the failure mentioned above.
> 
> The TDX module also reports TDX-capable memory as "Convertible Memory
> Regions" (CMRs).  CMRs tend to be coarser-grained [2] than the e820.
> Use CMRs to find memory holes when populating reserved areas to reduce
> their consumption.
> 
> Note the kernel does not prevent non-CMR memory from being added to
> "TDX-usable memory regions" but depends on the TDX module to catch in
> the TDH.SYS.CONFIG.  After switching to using CMRs to populate reserved
> areas this will no longer work.  To ensure no non-CMR memory is included
> in the TDMRs, verify that the memory region is truly TDX convertible
> before adding it as a TDX-usable memory region at early stage.

Thanks for trimming the changelog down.  But this changelog never
actually says what the fix is. It's also quite heavy on the "what" and
very light on the "why".

I think the "why" boils down to the fact that the kernel is treating RAM
-- as defined by the platform and TDX module -- as non-RAM.

> -	ret = tdmrs_populate_rsvd_areas_all(tdmr_list, tmb_list,
> +	/*
> +	 * On some large systems, the TDX memory blocks (which reflects
> +	 * e820) in the first 1GB can be too fine-grained.  Using them
> +	 * to populate reserved areas may result in reserved areas being
> +	 * exhausted.  CMRs are coarser-grained than e820.  Use CMRs to
> +	 * populate reserved areas to reduce their consumption.
> +	 */

I think there are still too many details here for a comment. This
comment is describing *highly* implementation and platform-specific
details particular to this bug you are fixing today. They will be
irrelevant to anyone reading this code tomorrow.

So in the end, I buy that the CMR's have something to offer here. But I
think that "why" I mentioned above casts doubt on whether
for_each_mem_pfn_range() is the right primitive on which to build the
TDX memblocks in the first place.

I suspect there's a much simpler solution that will emerge when
considering a deeper fix as opposed to adding CMRs as a band-aid.