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

Dave Hansen
2025-11-18T18:33:09Z

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;