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

Dave Hansen
2025-11-21T15:15:14Z

On 11/21/25 04:54, Xu Yilun wrote:
...
> For now, TDX Module Extensions consume quite large amount of memory
> (12800 pages), print this readout value on TDX Module Extentions
> initialization.

Overall, the description is looking better, thanks!

A few more nits, though. Please don't talk about things in terms of
number of pages. Just give the usage in megabytes.

>>> --- 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?
>
> I assume you mean TDH_SYS_CONFIG & TDH_SYS_CONFIG_V1.

Sure. But I wasn't being that literal about it. My point was whether we
need two macros for two simultaneous uses of the same seamcall.

> If you want to enable optional features via this seamcall, you must use
> v1, otherwise v0 & v1 are all good. Mm... I suddenly don't see usecase
> they must co-exist. Unconditionally use v1 is fine. So does TDH_VP_INIT.
>
> Does that mean we don't have to keep versions, always use the latest is
> good? (Proper Macro to be used...)
>
> -#define TDH_SYS_CONFIG 45
> +#define TDH_SYS_CONFIG (45 | (1ULL << TDX_VERSION_SHIFT))

That's my theory: we don't need to keep versions.

>> Second, does it hurt to pass the version along with other calls, like
>> ... (naming a random one) ... TDH_PHYMEM_PAGE_WBINVD ?
>
> I see no runtime hurt, just an extra zero parameter passed around.
>
> And version change always goes with more parameters, if we add version
> parameter, it looks like:
>
> u64 tdh_phymem_page_wbinvd_tdr(int version, struct tdx_td *td, int new_param1, int new_param2);
>
> For readability, I prefer the following, they provide clear definitions:
>
> u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td);
> u64 tdh_phymem_page_wbinvd_tdr_1(struct tdx_td *td, int new_param1, int new_param2);
>
> But I hope eventually we don't have to keep versions, then we don't have to choose:
>
> u64 tdh_phymem_page_wbinvd_tdr(struct tdx_td *td, int new_param1, int new_param2);

Sure, but that's not happening today. So for TDX_FEATURES0_TDXCONNECT at
least, config_tdx_module() doesn't change.

> The TDX Module can only accept one root page (i.e. 512 HPAs at most), while
> struct tdx_page_array contains the whole EXT memory (12800 pages). So we
> can't populate all pages into one root page then tell TDX Module. We need to
> populate one batch, tell tdx module, then populate the next batch, tell
> tdx module...

That is, indeed, the information that I was looking for. Can you please
ensure that makes it into code comments?