Re: [RFC PATCH v2 1/2] x86/virt/seamldr: Support large TDX module binaries

[Chao Gao <chao.gao@intel.com>]

On Tue, Nov 10, 2026 at 06:34:29AM +0800, Edgecombe, Rick P wrote:
>On Fri, 2026-11-06 at 05:21 -0800, Chao Gao wrote:
>> The module binary is handed to P-SEAMLDR as a list of page physical
>> addresses in 'struct seamldr_params'. The list has 496 entries, which
>> caps a module at 496 pages.
>
>nit. change it to "...caps a module at the same number of pages".

Ack.

>
>> 
>> TDX modules are gaining features and are about to outgrow that limit.
>> 
>> The P-SEAMLDR ABI was extended to chain the list across 4KB pages. For
>> a module over 495 pages, the last of the 496 entries points to another
>> page of entries instead of to a module page. The last entry of that
>> page can point to yet another page, and so on, until every module page
>> is listed.
>> 
>> Grow module_pages_pa_list[] so that 'struct seamldr_params' spans
>> multiple pages (16 for now), and use the extra pages as the chained
>> pages. The last entry of each page then points to the entry right after
>> it, which starts the next page. These link entries do not hold module
>> pages, so leave them out of the maximum module page count.
>> 
>> The 16-page size is somewhat arbitrary. It fits modules of up to about
>> 32MB, far above the roughly 2MB of current modules. It wastes 60KB for
>> today's modules, which is fine because the memory is freed right after
>> the update. Sizing module_pages_pa_list[] per module would mean either
>> moving the allocation after the image header is validated, or allocating
>> from an unvalidated page count. Either way it is more code churn.
>> 
>> Do not query P-SEAMLDR for chaining support. Userspace selects the update
>> image and is responsible for ensuring the module is compatible with
>> P-SEAMLDR.
>> 
>> Thanks to Yilun for the idea of allocating the chain pages together
>> with 'struct seamldr_params'.
>
>I'm not sure what this means.

I previously allocated the chained pages separately, as many as the module
needed, and linked them from 'struct seamldr_params'. Yilun suggested
growing 'struct seamldr_params' to include the chained pages, so a single
allocation covers both.

I will credit Yilun with a Suggested-by tag.

>> -#define SEAMLDR_MAX_NR_MODULE_PAGES	496
>> +/*
>> + * module_pages_pa_list[] spans multiple pages. The last entry of each
>> + * page points to the next page and does not hold a module page, so the
>> + * maximum number of module pages is the array size minus one entry per
>> + * page.
>> + *
>> + * SEAMLDR_PARAMS_NR_PAGES is somewhat arbitrary. It allows modules of
>> + * up to about 32MB. Increase it if modules outgrow that.
>> + */
>> +#define SEAMLDR_PARAMS_NR_PAGES		16
>> +#define SEAMLDR_NR_MODULE_PA_ENTRIES	(496 + 512 * (SEAMLDR_PARAMS_NR_PAGES - 1))
>
>I think you should describe the math a little bit here. 496 is the space in the
>original format, but the last one is now a pa pointer, right? So can't be used
>as a page of module data. And then each successive page is 511 entries and then
>a pa pointer? So is the math actually right?

As Binbin explained, SEAMLDR_NR_MODULE_PA_ENTRIES counts all entries,
including the ones that point to the next page. That is 496 in the first
page and 512 in each following page ...

>
>> +#define SEAMLDR_MAX_NR_MODULE_PAGES	(SEAMLDR_NR_MODULE_PA_ENTRIES - SEAMLDR_PARAMS_NR_PAGES)

... this subtracts one pointer entry per page.

How about:

/*
 * 'struct seamldr_params' spans this many pages. The number is somewhat
 * arbitrary. Increase it if modules outgrow it.
 */
#define SEAMLDR_PARAMS_NR_PAGES         16

/*
 * module_pages_pa_list[] has 496 entries in the first page and 512 in
 * each following page. The last entry of each page is reserved to point
 * to the next page and cannot hold a module page.
 */
#define SEAMLDR_NR_MODULE_PA_ENTRIES    (496 + 512 * (SEAMLDR_PARAMS_NR_PAGES - 1))
#define SEAMLDR_MAX_NR_MODULE_PAGES     (SEAMLDR_NR_MODULE_PA_ENTRIES - SEAMLDR_PARAMS_NR_PAGES)

>>  #define SEAMLDR_MAX_NR_SIG_PAGES	1
>>  
>>  /*
>> @@ -48,10 +59,10 @@ struct seamldr_params {
>>  	u64	sigstruct_pages_pa_list[SEAMLDR_MAX_NR_SIG_PAGES];
>>  	u8	reserved[104];
>>  	u64	module_nr_pages;
>> -	u64	module_pages_pa_list[SEAMLDR_MAX_NR_MODULE_PAGES];
>> +	u64	module_pages_pa_list[SEAMLDR_NR_MODULE_PA_ENTRIES];
>>  } __packed;
>>  
>> -static_assert(sizeof(struct seamldr_params) == 4096);
>> +static_assert(sizeof(struct seamldr_params) == SEAMLDR_PARAMS_NR_PAGES * PAGE_SIZE);
>>  
>>  /*
>>   * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
>> @@ -129,17 +140,24 @@ struct tdx_image {
>>  /*
>>   * Given a vmalloc() allocation, write all of the backing physical
>>   * addresses to pa_list[]. Caller guarantees that the array is big
>> - * enough.
>> + * enough. If pa_list[] spans multiple pages, point the last entry
>> + * of each page to the next page.
>>   */
>>  static void populate_pa_list(u64 *pa_list, const u8 *vmalloc_addr, u32 vmalloc_len_pages)
>>  {
>> -	int i;
>> +	int i, j;
>>  
>> -	for (i = 0; i < vmalloc_len_pages; i++) {
>> +	for (i = 0, j = 0; i < vmalloc_len_pages; i++) {
>>  		unsigned long offset = i * PAGE_SIZE;
>>  		unsigned long pfn = vmalloc_to_pfn(&vmalloc_addr[offset]);
>>  
>> -		pa_list[i] = pfn << PAGE_SHIFT;
>
>How about a little:
>/* If the current entry is the last in the page. */

Sure. I reworded it slightly to also say what the hunk does:

		/* Point the last entry of a page to the next page. */

>
>> +		if (PAGE_ALIGNED(&pa_list[j + 1])) {
>> +			pa_list[j] = __pa(&pa_list[j + 1]);
>> +			j++;
>> +		}
>> +
>> +		pa_list[j] = pfn << PAGE_SHIFT;
>> +		j++;
>>  	}
>>  }
>>  
>