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