Re: [PATCH v2 04/17] x86/virt/tdx: Add extra memory to TDX module for the extensions

[Xu Yilun]

> > TDH.EXT.MEM.ADD uses a new parameter type, HPA_LIST_INFO, to provide
> > this memory. This type represents a list of pages for TDX module to
> > access. It references an 'hpa_list page' which contains the list of
> > target HPAs. It collapses the HPA of the hpa_list page and the number
> > of valid target HPAs into a 64 bit raw value for SEAMCALL parameters.
> > The hpa_list page is always a medium, TDX module never keeps the
> > hpa_list page.
> 
> I think you are going to need to put the basic concepts up front. Don't lead
> with the acronyms. The TDX module needs to get a list of pages. It takes them in
> a PFN array that is encoded like (...). Create a helper for this, and name it
> the same after the TDX module term for this format, HPA_LIST_INFO.
> 
> Basically there are no exotic concepts here. So you can explain it in normal
> kernel terms.

OK. My refactored version:

    When adding the memory, the TDX module needs to get a list of pages.
    It accepts these target pages in a PFN array that is encoded into a
    single 64-bit SEAMCALL parameter: the PFN of a container page that
    holds the PFN array, and the number of the pages packed into a single
    value. Create a helper to encode this format and name it after the TDX
    module term: HPA_LIST_INFO.

[...]

> >  TDX module extensions consume tens of megabytes memory that
> > will never be returned to host. Use contiguous page allocation to
> > isolate these large blocks entirely, avoiding permanent memory
> > fragmentation and reducing buddy allocator efficiency. Print the
> > allocation amount on TDX module extensions initialization for
> > visibility.
> 
> Hmm. I'm not sure the user will care about the specific category of extension
> used memory.

Not sure if you suggest removing the print, but this is suggested by Dave:

https://lore.kernel.org/all/62bec236-4716-4326-8342-1863ad8a3f24@xxxxxxxxx/

[...]

> > +struct tdx_hpa_list {
> > +	u64 phys[PAGE_SIZE / sizeof(u64)];
> > +};
> > +
> > +static_assert(sizeof(struct tdx_hpa_list) == PAGE_SIZE);
> 
> What kind of change are we guarding against here?

I don't expect changes of the format, but this is to ensure the
container of the PFN array is a page, and also serves as the
documentation.

[...]

> > +	i = 0;
> > +	while (i < required_pages) {
> > +		unsigned int nents = min(required_pages - i,
> > +					 ARRAY_SIZE(hpa_list->phys));
> 
> I think this whole loop could be made clearer. nents is the number of pages we
> will add for an execution of the seamcall. That is not clear enough either.
> 
> > +		unsigned int j;
> > +
> > +		for (j = 0; j < nents; j++)
> > +			hpa_list->phys[j] = page_to_phys(page + i + j);
> > +
> > +		ret = tdx_ext_mem_add(virt_to_page(hpa_list), nents);
> > +		/*
> > +		 * No SEAMCALLs to reclaim the added pages. For simple error
> > +		 * handling, leak all pages.
> > +		 */
> > +		WARN(ret, "Fatal: TDX module rejected (%d) memory for
> > extensions, stranded all pages\n",
> > +		     ret);
> > +		if (ret)
> > +			break;
> 
> Can we combine the ret checks?

Yes.

> 
> > +
> > +		i += nents;
> 
> Maybe a full name for i?

Yes, I changed the name of i & nents.