Re: [PATCH 02/15] x86/virt/tdx: Add extra memory to TDX Module for Extensions

[Xu Yilun]

> > +static int tdx_ext_mem_add(struct page *root, unsigned int nr_pages)
> > +{
> > +	struct tdx_module_args args = {
> > +		.rcx = to_hpa_list_info(root, nr_pages),
> > +	};
> > +	u64 r;
> > +
> > +	tdx_clflush_hpa_list(root, nr_pages);
> > +
> > +	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.
> > +		 */
> > +		r = seamcall_ret(TDH_EXT_MEM_ADD, &args);
> > +	} while (r == TDX_INTERRUPTED_RESUMABLE);
> 
> The retry loop compares the full return value against TDX_INTERRUPTED_RESUMABLE. Should
> it mask with TDX_SEAMCALL_STATUS_MASK first, in case the module sets any
> lower detail bits?

mm.. there is an existing case for TDX_INTERRUPTED_RESUMABLE which
doesn't do the mask:

  err = tdh_phymem_cache_wb(resume);
  switch (err) {
	case TDX_INTERRUPTED_RESUMABLE:
		continue;

I believe we don't mask it. TDX_INTERRUPTED_RESUMABLE should not carry
any lower bits according to its bit definition, if it does it's a
problem we should not skip.

> 
> Ditto for TDH.EXT.INIT in patch 3.
> 
> > +
> > +	if (r != TDX_SUCCESS)
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}
> > +
> > +static int tdx_ext_mem_setup(void)
> > +{
> > +	unsigned int nr_pages;
> > +	struct page *page;
> > +	u64 *root;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	nr_pages = tdx_sysinfo.ext.memory_pool_required_pages;
> > +	/*
> > +	 * memory_pool_required_pages == 0 means no need to add pages,
> > +	 * skip the memory setup.
> > +	 */
> > +	if (!nr_pages)
> > +		return 0;
> > +
> > +	root = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!root)
> > +		return -ENOMEM;
> > +
> > +	page = alloc_contig_pages(nr_pages, GFP_KERNEL, numa_mem_id(),
> > +				  &node_online_map);
> 
> The SEAMCALL takes a scatter list (HPA_LIST_INFO), so the module
> doesn't require contiguity. If the goal is just to avoid scattering
> pages across many 2MB regions, maybe dense, 2MB-aligned allocations should
> achieve that without a single pool-wide contiguous block.
> 
> > +	if (!page) {
> > +		ret = -ENOMEM;
> > +		goto out_free_root;
> > +	}
> > +
> > +	for (i = 0; i < nr_pages;) {
> > +		unsigned int nents = min(nr_pages - i,
> > +					 PAGE_SIZE / sizeof(*root));
> > +		int j;
> > +
> > +		for (j = 0; j < nents; j++)
> > +			root[j] = page_to_phys(page + i + j);
> 
> Would it be better to allocate per-batch (i.e. one root page's worth
> at a time) rather than the whole pool up front?
> 
> That way an intermediate TDH.EXT.MEM.ADD failure wouldn't leak
> all nr_pages. Also, a batch is up to 512 pages (= 2MB) and its allocation
> could be 2MB-aligned, addressing your fragmentation concern.

So IIUC allocating 2MB by 2MB has the pros:

  - Larger chance to get the memory.
  - Less memory waste when TDH.EXT.MEM.ADD failed.

and the cons:

  - Still fragment 4M & 1G memory region.


I think first of all we should focus on the normal path when Extension is
successfully initialized and memory is added, note these memory can
never be reclaimed in this case, so memory fragmentation becomes the
primary considration.

And in TDX platform, the TDH.EXT.MEM.ADD failure is not expected to
happen, which means the TDX module is buggy and from Confidential
Computing POV we should not continue, we should change to a new module
and reboot. So less memory waste doesn't matter much actually.

Then, the Extension initialization is done at bootup time. We can get
the memory in big chance. If we really can't, it is a signal that the
system is not well configured for TDX, and failing earlier isn't such
a bad thing to me.

So for now I still think alloc_contig_pages() is better than 2M-by-2M
allocation.

> 
> > +
> > +		ret = tdx_ext_mem_add(virt_to_page(root), nents);
> > +		/*
> > +		 * No SEAMCALLs to reclaim the added pages. For simple error
> > +		 * handling, leak all pages.
> > +		 */
> > +		WARN_ON_ONCE(ret);
> > +		if (ret)
> > +			break;
> > +
> > +		i += nents;
> > +	}
> > +
> > +	/*
> > +	 * Extensions memory can't be reclaimed once added, print out the
> > +	 * amount, stop tracking it and free the root page, no matter success
> > +	 * or failure.
> > +	 */
> > +	pr_info("%lu KB allocated for TDX Module Extensions\n",
> > +		nr_pages * PAGE_SIZE / 1024);
> > +
> > +out_free_root:
> > +	kfree(root);
> > +
> > +	return ret;
> > +}
> > +
> > +static int __maybe_unused init_tdx_ext(void)
> 
> Could this be named init_tdx_extensions() instead to disambiguate
> from tdx_ext_init() in patch 3?

Yes, good to me.

I'm changing all Extensions to Extension, cause the SPEC says "TDX
Module Extension". So I'll use init_tdx_extension().

Thanks,
Yilun