Re: [PATCH v3 7/8] x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find memory holes

[Huang, Kai]

On Fri, 2024-09-06 at 16:31 -0700, Dan Williams wrote:
> 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)
> > 
> [..]
> 
> I feel what I trimmed above is a lot of text just to say:
> 
>     "build_tdx_memlist() tries to fulfill the TDX module requirement it be
>     told about holes in the TDMR space <insert pointer / reference to
>     requirement>. It turns out that the kernel's view of memory holes is too
>     fine grained and sometimes exceeds the number of holes (16) that the TDX
>     module can track per TDMR <insert problematic memory map>. Thankfully
>     the module also lists memory that is potentially convertible in a list
>     of CMRs. That coarser grained CMR list tends to track usable memory in
>     the memory map even if it might be reserved for host usage like 'ACPI
>     data'. Use that list to relax what the kernel considers unusable
>     memory. If it falls in a CMR no need to instantiate a hole, and rely on
>     the fact that kernel will keep what it considers 'reserved' out of the
>     page allocator."
> 
> ...but don't spin the patch just to make the changelog more concise. It
> took me reading a few times to pull out those salient details, that is,
> if I understood it correctly?

Thanks.  Let me try to trim down the changelog since I need to send out another
version anyway.

> 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 0fb673dd43ed..fa335ab1ae92 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -331,6 +331,72 @@ static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version
> >  	return ret;
> >  }
> >  
> > +/* Update the @sysinfo_cmr->num_cmrs to trim tail empty CMRs */
> > +static void trim_empty_tail_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];
> > +
> > +		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));
> 
> Is it really required to let the TDX module taint and possibly crash the
> kernel if it provides misaligned CMRs? Shouldn't these be validated
> early and just turn off TDX support if the TDX module is this broken?

We can make this function return error code for those error cases, and fail to
initialize TDX module.

But CMRs are actually verified by the MCHECK.  If any WARN() above happens, TDX
won't get enabled in hardware.  I think maybe we can just remove the WARN()s?