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/19/25 22:09, Xu Yilun wrote:
> On Tue, Nov 18, 2025 at 10:32:13AM -0800, Dave Hansen wrote:
...
>> 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

That's not quite what I was hoping for.

I want an overview of how this new memory fits into the overall scheme.
I'd argue that these "control pages" are most similar to the PAMT:
There's some back-and-forth with the module about how much memory it
needs, the kernel allocates it, hands it over, and never gets it back.

That's the level that this needs to be presented at: a high-level
logical overview.

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

I'd need quite a bit of convincing that this is the right way.

What is the scenario where there's a:

	TDH_SYS_CONFIG_V1
and
	TDH_SYS_CONFIG_V2

in the tree at the same time?

Second, does it hurt to pass the version along with other calls, like
... (naming a random one) ... TDH_PHYMEM_PAGE_WBINVD ?

Even if we did this, we wouldn't copy and paste "(1ULL <<
TDX_VERSION_SHIFT)" all over the place, right? We'd create a more
concise, cleaner macro and then use it everywhere. Right?
	
>>>> 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.

I asked in my last message, but perhaps it was missed: Could you please
start clearing irrelevant context in your replies. Like that hunk ^

>>>> 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 suspect we're not really talking about the same thing here.

In any case, I'm not a super big fan of how tdx_ext_mempool_setup() is
written. Can you please take another pass at it and try to simplify it
and make it easier to follow?

This would be a great opportunity to bring in some of your colleagues to
take a look. You can solicit them for suggestions.

>>>>> +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.
I'm not sure what you are trying to say here. By saying "It's good" are
you agreeing that my suggested structure is good and that you will use
it? Or are you saying that the original structure is good?

Second, what is an "object handler"? Are you talking about the function
that is pointed to by __free()?

Third, are you saying that the original code structure is somehow
connected to __free()? I thought that all of these were logically
equivalent:

	void *foo __free(foofree) = alloc_foo();

	void *foo __free(foofree) = NULL:
	foo = alloc_foo();

	void *foo __free(foofree) = NULL;
	populate_foo(&foo);

Is there something special about doing the variable assignment at the
variable declaration spot?