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

[Dave Hansen <dave.hansen@intel.com>]

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?

...>> 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?

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

BTW... Do you see how I'm trimming context as I reply? Could you please
endeavor to do the same?

>>> @@ -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?



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

>> 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?

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;