Re: [PATCH 4/6] x86/virt/tdx: Add extra memory to TDX module for the extensions

[Xu Yilun <yilun.xu@linux.intel.com>]

> > The TDX module accepts the memory in the form of a PFN array. This array
> > is passed via a single 64-bit SEAMCALL leaf parameter, which encodes two
> > values: the PFN of the container page holding the array, and the number
> > of entries in the array. Create a helper to encode this format and name
> > it after the TDX module term: HPA_LIST_INFO.
> 
> The array entries are physical addresses, not PFNs. HPA_LIST_INFO encodes
> a PFN, the array does not.

OK. I'll change PFN array => HPA array

[...]

> > +#define TDX_HPA_LIST_MAX_NR_PAGES	(PAGE_SIZE / sizeof(u64))
> > +
> > +struct tdx_hpa_list {
> > +	u64 phys[TDX_HPA_LIST_MAX_NR_PAGES];
> > +};
> > +
> > +static_assert(sizeof(struct tdx_hpa_list) == PAGE_SIZE);

[...]

> > +	hpa_list = kzalloc_obj(*hpa_list);
> > +	if (!hpa_list)
> > +		return -ENOMEM;
> 
> to_hpa_list_info() expects hpa_list to be page-aligned. It happens to
> work with kmalloc for PAGE_SIZE allocation.

The struct tdx_hpa_list definition follows the TDX ABI and is guarenteed
to be PAGE_SIZE by:

	static_assert(sizeof(struct tdx_hpa_list) == PAGE_SIZE);

and kmalloc guarentees the page alignment.

	59bb47985c1d ("mm, sl[aou]b: guarantee natural alignment for kmalloc(power-of-two)")

So I think it's OK, not "happen to work".

> 
> Maybe it is better to allocate it with buddy allocator instead?

It can be, but then we need an extra variable to record the
struct page *, which seems redundant?

> 
> > +
> > +	page = alloc_contig_pages(required_pages, GFP_KERNEL, numa_mem_id(),
> > +				  &node_online_map);
> 
> Why contiguous? TDH.EXT.MEM.ADD takes a list of page addresses and the loop
> below writes every one of them out separately.
> 
> alloc_pages_bulk() fits the chunking that is already here, and a short
> return can be handled per chunk. alloc_contig_pages() isolates and migrates
> to get its range and fails TDX init outright when it cannot find one. PAMT

Yeah, this is not the ABI requirement, but the kernel's consideration. A
brief reasoning in the commit log: avoiding permanent memory fragmentation
and buddy allocator efficiency loss.

Also there is some discussion:

https://lore.kernel.org/all/167d9540-2d9a-4367-bc68-b96494bc4044@intel.com/

TL;DR
  - The memory will never return to the kernel.
  - There is chance that this tens of megabytes will fragment tens of
    gigabytes of memory forever.
  - The chance of fragmentation is actually low since at boot up, but
    let the buddy allocator take care of these never-returned memory
    is not necessary and lowers its efficiency.

> needs it because the TDMR ABI describes each PAMT as base+size. This does
> not.
> 
> > +	if (!page) {
> > +		ret = -ENOMEM;
> > +		goto out_free_hpa_list;
> > +	}
> > +
> > +	added_pages = 0;
> > +	while (added_pages < required_pages) {
> > +		unsigned int chunk_pages = min(required_pages - added_pages,
> > +					       TDX_HPA_LIST_MAX_NR_PAGES);
> > +		struct page *chunk = page + added_pages;
> > +		unsigned int i;
> > +
> > +		for (i = 0; i < chunk_pages; i++)
> > +			hpa_list->phys[i] = page_to_phys(chunk + i);
> > +
> > +		ret = tdx_ext_mem_add(hpa_list, chunk_pages);
> > +		if (ret) {
> > +			/*
> > +			 * This SEAMCALL leaf shouldn't fail, and if it does,
> > +			 * things are broken enough that complex error handling
> > +			 * isn't worth it. Intentionally leak all pages,
> > +			 * including un-added pages.
> > +			 */
> > +			WARN(1, "Fatal: TDX module rejected memory for extensions, stranded all pages\n");
> > +			break;
> 
> It supposed to be
> 			goto out_free_hpa_list;
> 
> No?

The difference is to print the memory amount or not. For simple error
handling, we stranded all pages, we let users know the cost even if the
initialization fails.

> 
> 
> > +		}
> > +
> > +		added_pages += chunk_pages;
> > +	}
> > +
> > +	/*
> > +	 * Memory for TDX module extensions is never reclaimed and can be tens
> > +	 * of megabytes. Print the amount so users know the cost.
> > +	 */
> > +	pr_info("%lu KB consumed for TDX module extensions\n",
> > +		required_pages * PAGE_SIZE / 1024);
> > +
> > +out_free_hpa_list:
> > +	kfree(hpa_list);
> > +
> > +	return ret;
> > +}
> > +

[...]

> > --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> > @@ -137,6 +137,12 @@ static __init int get_tdx_sys_info_ext(struct tdx_sys_info_ext *sysinfo_ext)
> >  	int ret;
> >  	u64 val;
> >  
> > +	ret = read_sys_metadata_field(0x3100000200000000, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	sysinfo_ext->memory_pool_required_pages = val;
> > +
> 
> Why above ext_required read?

I want to sort them in ascending order of the field ID, so reviewers can
seach them more easily.

> Is it even valid to read it in such case?

It is OK. The two ext metadata are both valid after TDX feature
configurations.

The ext_required == 0 && memory_pool_required_pages > 0 is highly
suspicious based on our current understanding, but that's more of a
module BUG, not caused by metadata reading order.

> 
> >  	ret = read_sys_metadata_field(0x3100000000000001, &val);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 2.25.1
> > 
> 
> -- 
>   Kiryl Shutsemau / Kirill A. Shutemov