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

[Binbin Wu <binbin.wu@linux.intel.com>]

On 11/10/2026 6:34 AM, 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".
> 
>>
>> 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.

Although the change log above has mentioned "For a module over 495 pages,
...", a 496-page module was compatible with the P-SEAMLDR without chaining
support, but now it requires P-SEAMLDR with chaining support.  Is it worth
calling out this change to userspace?

>>
>> Thanks to Yilun for the idea of allocating the chain pages together
>> with 'struct seamldr_params'.
> 
> I'm not sure what this means.
> 
>>
>> AI was used under supervision to review code and workshop logs.
> 
> Same comment as on the next patch.
> 
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> v2:
>>  - Make 'struct seamldr_params' a fixed 16 pages instead of sizing it
>>    to each module, capping modules at about 32MB. [Rick]
>>  - Drop the patch that moved the allocation after image validation. [Rick]
>> ---
>>  arch/x86/virt/vmx/tdx/seamldr.c | 32 +++++++++++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
>> index b1137ca6150d..087bdf10a9a7 100644
>> --- a/arch/x86/virt/vmx/tdx/seamldr.c
>> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
>> @@ -24,7 +24,18 @@
>>  #define P_SEAMLDR_INFO			0x8000000000000000
>>  #define P_SEAMLDR_INSTALL		0x8000000000000001
>>  
>> -#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?

My understanding is MODULE_PA_ENTRIES hold both addresses of module pages and
the pointers to the next page.  SEAMLDR_MAX_NR_MODULE_PAGES below is the page
number for the TDX module.

> 
>> +#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.

I was wondering why SEAMLDR_MAX_NR_MODULE_PAGES	is not
(SEAMLDR_NR_MODULE_PA_ENTRIES - SEAMLDR_PARAMS_NR_PAGES + 1), since I thought
the last page should all be the addresses for module pages.

But I find the code below always treats the last entry of a page as the pointer
to the next page, i.e. SEAMLDR_MAX_NR_MODULE_PAGES is indeed
(SEAMLDR_NR_MODULE_PA_ENTRIES - SEAMLDR_PARAMS_NR_PAGES), but I am not sure the
comment is clear enough.

>>   */
>>  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. */
> 
>> +		if (PAGE_ALIGNED(&pa_list[j + 1])) {
>> +			pa_list[j] = __pa(&pa_list[j + 1]);
>> +			j++;
>> +		}
>> +
>> +		pa_list[j] = pfn << PAGE_SHIFT;
>> +		j++;
>>  	}
>>  }
>>  
>