Re: [PATCH v2 01/17] x86/virt/tdx: Embed version info in SEAMCALL leaf function definitions

[Dave Hansen]

On 6/18/26 01:13, Xu Yilun wrote:
> Embed version information in SEAMCALL leaf function definitions rather
> than let the caller open code them. For now, only TDH.VP.INIT is
> involved.

This is jumping the gun a bit in the changelog.

What is a SEAMCALL leaf function?

How does the version fit in?

> Don't bother the caller to choose the SEAMCALL version if unnecessary.

I think I see what you are trying to say here but it's more than that.

The question is whether there should be a base seamcall() API that takes
an explicit version or whether the version should be passed in by callers.

One wrinkle is that the naming of all of these things is around
"function", "func" and "fn":

u64 __seamcall(u64 fn, struct tdx_module_args *args);

A "function" is TDH.SYS.INIT or TDH.SYS.INFO, not 'TDH.SYS.INFO v123'.

But the low-level calls could be:

	u64 __seamcall(u64 fn, u64 version, ...);
	
or

	u64 __seamcall(u64 fn, ...);

Where 'fn' encodes the function *and* version.

> The old TDX modules that only support TDH.VP.INIT v0 are all deprecated,
> so only provide the latest (v1) definition.

No, this isn't how this is going to work.

What do we *NEED* from v1? Why churn the code if we don't *NEED*
something from v1 and can live with v0? It has *ZERO* to do with the TDX
module being deprecated or whatever.

Linux stays on the old interface until we need a new interface. We are
*not* going to bump version numbers just because.


>  /*
>   * TDX module SEAMCALL leaf functions
>   */
> @@ -31,7 +44,7 @@
>  #define TDH_VP_CREATE			10
>  #define TDH_MNG_KEY_FREEID		20
>  #define TDH_MNG_INIT			21
> -#define TDH_VP_INIT			22
> +#define TDH_VP_INIT			SEAMCALL_LEAF_VER(22, 1)
>  #define TDH_PHYMEM_PAGE_RDMD		24
>  #define TDH_VP_RD			26
>  #define TDH_PHYMEM_PAGE_RECLAIM		28
> @@ -50,14 +63,6 @@
>  #define TDH_SYS_UPDATE			53
>  #define TDH_SYS_DISABLE			69

That is unreadable and patterns can't be seen. This is better:

#define TDH_MNG_INIT			SEAMCALL_LEAF_VER(21, 0)
#define TDH_VP_INIT			SEAMCALL_LEAF_VER(22, 1)
#define TDH_PHYMEM_PAGE_RDMD		SEAMCALL_LEAF_VER(24, 0)

> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1903,8 +1903,7 @@ u64 tdh_vp_init(struct tdx_vp *vp, u64 initial_rcx, u32 x2apicid)
>  		.r8 = x2apicid,
>  	};
>  
> -	/* apicid requires version == 1. */
> -	return seamcall(TDH_VP_INIT | (1ULL << TDX_VERSION_SHIFT), &args);
> +	return seamcall(TDH_VP_INIT, &args);
>  }
>  EXPORT_SYMBOL_FOR_KVM(tdh_vp_init);

But that whole scheme falls apart the first time the kernel needs
functionality from v2. You'll need:

#define TDH_VP_INIT_V0			SEAMCALL_LEAF_VER(22, 0)
#define TDH_VP_INIT_V1			SEAMCALL_LEAF_VER(22, 1)

and then the calls will do:

	if (foo)
		return seamcall(TDH_VP_INIT_V0, &args);
	else
		return seamcall(TDH_VP_INIT_V1, &args);

So this 100% goes down the road of needing #defines for *EACH* version.
That's the real implication here and the real choice.

That said, I don't particularly like:

	if (foo)
		return seamcall(TDH_VP_INIT, 0, &args);
	else
		return seamcall(TDH_VP_INIT, 1, &args);

all that much either because of the seemingly magic numbers.

The whole seamcall RAX thing is one step too clever. I think Linux did
the right thing:

5	common	open				sys_open
288	common	openat				sys_openat
437	common	openat2				sys_openat2

New ABI gets a new base number. No need for the other end of the ABI to
know that 288 is arguably a later version of 5.

Ugh. But why is this patch even in here in the first place? Why is this
even *ASSOCIATED* with DICE-based attestation? Isn't this completely
orthogonal?