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

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

>+#define HPA_LIST_INFO_FIRST_ENTRY	GENMASK_U64(11, 3)
>+#define HPA_LIST_INFO_PFN		GENMASK_U64(51, 12)
>+#define HPA_LIST_INFO_LAST_ENTRY	GENMASK_U64(63, 55)
>+
>+static __init u64 to_hpa_list_info(struct page *hpa_list_page,
>+				   unsigned int nr_pages)
>+{
>+	return FIELD_PREP(HPA_LIST_INFO_FIRST_ENTRY, 0) |
>+	       FIELD_PREP(HPA_LIST_INFO_PFN, page_to_pfn(hpa_list_page)) |
>+	       FIELD_PREP(HPA_LIST_INFO_LAST_ENTRY, nr_pages - 1);
>+}
>+
>+static __init int tdx_ext_mem_add(struct page *hpa_list_page,
>+				  unsigned int nr_pages)
>+{
>+	struct tdx_module_args args = {
>+		.rcx = to_hpa_list_info(hpa_list_page, nr_pages),
>+	};
>+	u64 r;
>+
>+	do {
>+		/*
>+		 * TDH_EXT_MEM_ADD is designed to use output parameter RCX to
>+		 * override/update input parameter RCX, so the caller doesn't
>+		 * have to do manual parameter update on retry call.
>+		 */

I am not sure about the "manual parameter update" part.

how about:
		/*
		 * The TDX module overwrites RCX to track progress when
		 * this SEAMCALL is interrupted. Use seamcall_ret() to
		 * save and pass the updated value back on retry.
		 */

>+		r = seamcall_ret(TDH_EXT_MEM_ADD, &args);
>+	} while (r == TDX_INTERRUPTED_RESUMABLE);
>+
>+	if (r != TDX_SUCCESS)
>+		return -EFAULT;

why -EFAULT?

In sc_retry_prerr(), most SEAMCALL errors are mapped to EIO. Maybe
we should use EIO here.

>+
>+	return 0;
>+}
>+
>+struct tdx_hpa_list {
>+	u64 phys[PAGE_SIZE / sizeof(u64)];
>+};
>+
>+static_assert(sizeof(struct tdx_hpa_list) == PAGE_SIZE);
>+
>+static __init int tdx_ext_mem_setup(unsigned int required_pages)
>+{
>+	struct tdx_hpa_list *hpa_list;
>+	struct page *page;
>+	unsigned int i;
>+	int ret;
>+
>+	/*
>+	 * memory_pool_required_pages == 0 means no need to add pages,
>+	 * skip the memory setup.
>+	 */

There is no "memory_pool_required_pages" here.

>+	if (!required_pages)
>+		return 0;
>+
>+	hpa_list = kzalloc_obj(*hpa_list);
>+	if (!hpa_list)
>+		return -ENOMEM;
>+
>+	page = alloc_contig_pages(required_pages, GFP_KERNEL, numa_mem_id(),
>+				  &node_online_map);
>+	if (!page) {
>+		ret = -ENOMEM;
>+		goto out_free_hpa_list;
>+	}
>+
>+	i = 0;
>+	while (i < required_pages) {
>+		unsigned int nents = min(required_pages - i,
>+					 ARRAY_SIZE(hpa_list->phys));
>+		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);

Printing 'ret' is useless since it's always -EFAULT.

The real reason to WARN here isn't "no SEAMCALL to reclaim". It is "this
SEAMCALL shouldn't fail, and if it does, things are broken enough that
complex error handling isn't worth it".

>+		if (ret)
>+			break;
>+
>+		i += nents;
>+	}
>+
>+	/*
>+	 * Memory for extensions can't be reclaimed once added, print out the
>+	 * amount, stop tracking it and free the hpa_list page, no matter
>+	 * success or failure.
>+	 */

This doesn't explain why it should be print. How about:

	/*
	 * 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;
>+}