Re: [PATCH v1 08/26] x86/virt/tdx: Add tdx_enable_ext() to enable of TDX Module Extensions

[Xu Yilun <yilun.xu@linux.intel.com>]

On Tue, Nov 18, 2025 at 10:32:13AM -0800, Dave Hansen wrote:
> On 11/18/25 09:14, Xu Yilun wrote:
> ....>>> The extension initialization uses the new TDH.EXT.MEM.ADD and
> >>> TDX.EXT.INIT seamcalls. TDH.EXT.MEM.ADD add pages to a shared memory
> >>> pool for extensions to consume.
> >>
> >> "Shared memory" is an exceedingly unfortunate term to use here. They're
> >> TDX private memory, right?
> > 
> > Sorry, they are indeed TDX private memory. Here 'shared' means the
> > memory in the pool will be consumed by multiple new features but this
> > is TDX Module internal details that I should not ramble, especially in
> > TDX context.
> ... and you'll find a better term in the next revision. Right?

Yes, I think just "memory pool" is enough.

> 
> ...>> How much memory does this consume?
> > 
> > 12800 pages.
> 
> Oof. That's more than I expected and it's also getting up to the amount
> that you don't want to just eat without saying seomthing about it.
> 
> Could you please at least dump a pr_info() out about how much memory
> this consumes?

Sure.

> 
> >>> TDH.EXT.MEM.ADD is the first user of tdx_page_array. It provides pages
> >>> to TDX Module as control (private) pages. A tdx_clflush_page_array()
> >>> helper is introduced to flush shared cache before SEAMCALL, to avoid
> >>> shared cache write back damages these private pages.
> >>
> >> First, this talks about "control pages". But I don't know what a control
> >> page is.
> > 
> > It refers to pages provided to TDX Module to hold all kinds of control
> > structures or metadata. E.g. TDR, TDCS, TDVPR... With TDX Connect we
> > have more, SPDM metadata, IDE metadata...
> 
> Please *say* that. Explain how existing TDX metadata consumes memory and
> how this new mechanism is different.

Yes.

Existing ways to provide an array of metadata pages to TDX Module
varies:

 1. Assign each HPA for each SEAMCALL register.
 2. Call the same seamcall multiple times.
 3. Assign the PA of HPA-array in one register and the page number in
    another register.

TDX Module defines new interfaces trying to unify the page array
provision. It is similar to the 3rd method. The new objects HPA_ARRAY_T
and HPA_LIST_INFO need a 'root page' which contains a list of HPAs.
They collapse the HPA of the root page and the number of valid HPAs
into a 64 bit raw value for one SEAMCALL parameter.

I think these words should be in:

  x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects
> 
> BTW... Do you see how I'm trimming context as I reply? Could you please
> endeavor to do the same?

Yes.

> 
> >>> @@ -1251,7 +1254,14 @@ static __init int config_tdx_module(struct tdmr_info_list *tdmr_list,
> >>>  	args.rcx = __pa(tdmr_pa_array);
> >>>  	args.rdx = tdmr_list->nr_consumed_tdmrs;
> >>>  	args.r8 = global_keyid;
> >>> -	ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> >>> +
> >>> +	if (tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_TDXCONNECT) {
> >>> +		args.r9 |= TDX_FEATURES0_TDXCONNECT;
> >>> +		args.r11 = ktime_get_real_seconds();
> >>> +		ret = seamcall_prerr(TDH_SYS_CONFIG | (1ULL << TDX_VERSION_SHIFT), &args);
> >>> +	} else {
> >>> +		ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> >>> +	}
> >>
> >> I'm in the first actual hunk of code and I'm lost. I don't have any idea
> >> what the "(1ULL << TDX_VERSION_SHIFT)" is doing.
> > 
> > TDX Module defines the version field in its leaf to specify updated
> > parameter set. The existing user is:
> > 
> > u64 tdh_vp_init(struct tdx_vp *vp, u64 initial_rcx, u32 x2apicid)
> > {
> > 	struct tdx_module_args args = {
> > 		.rcx = vp->tdvpr_pa,
> > 		.rdx = initial_rcx,
> > 		.r8 = x2apicid,
> > 	};
> > 
> > 	/* apicid requires version == 1. */
> > 	return seamcall(TDH_VP_INIT | (1ULL << TDX_VERSION_SHIFT), &args);
> > }
> 
> OK, so there's a single existing user with this thing open coded.
> 
> You're adding a second user, so you just copied and pasted the existing
> code. Is there a better way to do this? For instance, can we just pass
> the version number to *ALL* seamcall()s?

I think it may be too heavy. We have a hundred SEAMCALLs and I expect
few needs version 1. I actually think v2 is nothing different from a new
leaf. How about something like:

--- a/arch/x86/virt/vmx/tdx/tdx.h
+++ b/arch/x86/virt/vmx/tdx/tdx.h
@@ -46,6 +46,7 @@
 #define TDH_PHYMEM_PAGE_WBINVD         41
 #define TDH_VP_WR                      43
 #define TDH_SYS_CONFIG                 45
+#define TDH_SYS_CONFIG_V1              (TDH_SYS_CONFIG | (1ULL << TDX_VERSION_SHIFT))

And if a SEAMCALL needs export, add new tdh_foobar() helper. Anyway
the parameter list should be different.

> 
> 
> 
> ...>> This is really difficult to understand. It's not really filling a
> >> "root", it's populating an array. The structure of the loop is also
> > 
> > It is populating the root page with part (512 pages at most) of the array.
> > So is it better name the function tdx_page_array_populate_root()?
> 
> That's getting a bit verbose.

tdx_page_array_populate()

> 
> >> rather non-obvious. It's doing:
> >>
> >> 	while (1) {
> >> 		fill(&array);
> >> 		tell_tdx_module(&array);
> >> 	}
> > 
> > There is some explanation in Patch #6:
> 
> That doesn't really help me, or future reviewers.
> 
> >  4. Note the root page contains 512 HPAs at most, if more pages are
> >    required, refilling the tdx_page_array is needed.
> > 
> >  - struct tdx_page_array *array = tdx_page_array_alloc(nr_pages);
> >  - for each 512-page bulk
> >    - tdx_page_array_fill_root(array, offset);
> >    - seamcall(TDH_XXX_ADD, array, ...);
> 
> Great! That is useful information to have here, in the code.
> 
> >> Why can't it be:
> >>
> >> 	while (1)
> >> 		fill(&array);
> >> 	while (1)
> >> 		tell_tdx_module(&array);
> > 
> > The consideration is, no need to create as much supporting
> > structures (struct tdx_page_array *, struct page ** and root page) for
> > each 512-page bulk. Use one and re-populate it in loop is efficient.
> 
> Huh? What is it efficient for? Are you saving a few pages of _temporary_
> memory?

In this case yes, cause no way to reclaim TDX Module EXT required pages.
But when reclaimation is needed, will hold these supporting structures
long time.

Also I want the tdx_page_array object itself not been restricted by 512
pages, so tdx_page_array users don't have to manage an array of array.

> 
> I'm not following at all.
> 
> >>> +static int init_tdx_ext(void)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	if (!(tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_EXT))
> >>> +		return -EOPNOTSUPP;
> >>> +
> >>> +	struct tdx_page_array *mempool __free(tdx_ext_mempool_free) =
> >>> +		tdx_ext_mempool_setup();
> >>> +	/* Return NULL is OK, means no need to setup mempool */
> >>> +	if (IS_ERR(mempool))
> >>> +		return PTR_ERR(mempool);
> >>
> >> That's a somewhat odd comment to put above an if() that doesn't return NULL.
> > 
> > I meant to explain why using IS_ERR instead of IS_ERR_OR_NULL. I can
> > impove the comment.
> 
> I'd kinda rather the code was improved. Why cram everything into a
> pointer if you don't need to. This would be just fine, no?
> 
> 	ret = tdx_ext_mempool_setup(&mempool);
> 	if (ret)
> 		return ret;

It's good.

The usage of pointer is still about __free(). In order to auto-free
something, we need an object handler for something. I think this is a
more controversial usage of __free() than pure allocation. We setup
something and want auto-undo something on failure.

Thanks,
Yilun