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

[Chao Gao]

>> +/*
>> + * Like populate_pa_list(), but skip entries pointing to the next page.
>> + */
>> +static void populate_module_pa_list(struct seamldr_params *params,
>> +				    const u8 *vmalloc_addr, u32
>> vmalloc_len_pages)
>> +{
>> +	u64 *pa_list = params->module_pages_pa_list;
>> +	int i, j;
>> +
>> +	for (i = 0, j = 0; i < vmalloc_len_pages; i++, j++) {
>> +		unsigned long offset = i * PAGE_SIZE;
>> +		unsigned long pfn = vmalloc_to_pfn(&vmalloc_addr[offset]);
>> +
>> +		/*
>> +		 * Skip non-zero entries. They point to the next page and
>> +		 * are populated during allocation.
>> +		 */
>> +		if (pa_list[j])
>> +			j++;
>
>We know what position in the page this should be right? Can we do some modulus
>and skip it that way? Feels less hacky.

Yes, the link is always the last entry of a page.

how about:
		/*
		 * The last entry of each page points to the next chain
		 * page.
		 */
		if (PAGE_ALIGNED(&pa_list[j + 1]))
			j++;

>
>> +
>> +		pa_list[j] = pfn << PAGE_SHIFT;
>> +	}
>> +}
>> +
>> +static struct seamldr_params *alloc_seamldr_params(u32 nr_module_pages)
>> +{
>> +	struct seamldr_params *params;
>> +	u32 nr_chain_pages;
>> +	u32 nr_direct;
>> +	u64 *link;
>> +
>> +	/*
>> +	 * Number of module pages that fit directly in
>
>"fit directly" feels like adding jargon where there doesn't need to be.

[...]

>
>> +	 * module_pages_pa_list[]. Minus one as the last entry is
>> +	 * reserved to point to the first chain page.
>> +	 */
>> +	nr_direct = SEAMLDR_NR_MODULE_PA_ENTRIES - 1;
>> +
>> +	/* Calculate the number of extra chain pages needed. */
>> +	nr_chain_pages = 0;
>> +	if (nr_module_pages > nr_direct) {
>> +		u32 nr_left = nr_module_pages - nr_direct;
>> +
>> +		nr_chain_pages = DIV_ROUND_UP(nr_left,
>> SEAMLDR_NR_CHAIN_PA_ENTRIES);
>
>Left pages, module pages, direct pages and chain pages. It's a lot...

[...]

>
>> +	}
>> +
>> +	params = kzalloc_flex(*params, chain_pages, nr_chain_pages);
>> +	if (!params)
>> +		return NULL;
>> +
>> +	/* Link all the chain pages. */
>> +	link = &params->module_pages_pa_list[nr_direct];
>> +	for (int i = 0; i < nr_chain_pages; i++) {
>> +		struct chain_page *page = &params->chain_pages[i];
>> +
>> +		*link = __pa(page);
>> +		link = &page->next_page_pa;
>> +	}
>> +
>> +	return params;
>> +}
>
>This all seems like more than should be needed.

Most of the complexity came from sizing the allocation exactly. If we
over-allocate a little instead, the size calculation becomes much
simpler. Making module_pages_pa_list[] a flexible array also removes
the need to treat its entries differently from those in the chain
pages. And linking the pages while filling in the module page addresses
removes the separate linking loop.

We could even fold populate_module_pa_list() into populate_pa_list().
That works because sigstruct_pages_pa_list[] never reaches the end of a
page. It would save a function, but it looks a bit hacky for the
sigstruct pages, because the spec defines links only for the module page
list. I am not sure which is better. Do you have a preference?


diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
index b1137ca6150d..cd4a00a7b02e 100644
--- a/arch/x86/virt/vmx/tdx/seamldr.c
+++ b/arch/x86/virt/vmx/tdx/seamldr.c
 /*
@@ -48,11 +54,9 @@ 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[];
 } __packed;
 
-static_assert(sizeof(struct seamldr_params) == 4096);
-
 /*
  * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
  * interact with P-SEAMLDR simultaneously. Use raw version as the calls can
@@ -143,6 +147,55 @@ static void populate_pa_list(u64 *pa_list, const u8 *vmalloc_addr, u32 vmalloc_l
 	}
 }
 
+/*
+ * Like populate_pa_list(), but skip entries pointing to the next page.
+ */
+static void populate_module_pa_list(struct seamldr_params *params,
+				    const u8 *vmalloc_addr, u32 vmalloc_len_pages)
+{
+	u64 *pa_list = params->module_pages_pa_list;
+	int i, j;
+
+	for (i = 0, j = 0; i < vmalloc_len_pages; i++, j++) {
+		unsigned long offset = i * PAGE_SIZE;
+		unsigned long pfn = vmalloc_to_pfn(&vmalloc_addr[offset]);
+
+		/*
+		 * The last entry of each page points to the next chain
+		 * page.
+		 */
+		if (PAGE_ALIGNED(&pa_list[j + 1])) {
+			pa_list[j] = __pa(&pa_list[j + 1]);
+			j++;
+		}
+
+		pa_list[j] = pfn << PAGE_SHIFT;
+	}
+}
+
+static struct seamldr_params *alloc_seamldr_params(u32 nr_module_pages)
+{
+	struct seamldr_params *params;
+	u32 nr_pages;
+
+	if (nr_module_pages > SEAMLDR_MAX_NR_MODULE_PAGES)
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * One page for 'struct seamldr_params', plus chain pages that
+	 * each hold 511 module page addresses and a link to the next
+	 * page. Ignoring the addresses held in the first page
+	 * over-allocates a little.
+	 */
+	nr_pages = 1 + DIV_ROUND_UP(nr_module_pages, 511);
+
+	params = kzalloc(nr_pages * PAGE_SIZE, GFP_KERNEL);
+	if (!params)
+		return ERR_PTR(-ENOMEM);
+
+	return params;
+}
+