Re: [PATCH v2] x86/virt/tdx: Formalize SEAMCALL version encoding support

[Xu Yilun <yilun.xu@linux.intel.com>]

On Wed, Jul 08, 2026 at 11:03:14AM -0700, Dave Hansen wrote:
> On 7/8/26 10:03, Xu Yilun wrote:
> > +/*
> > + * SEAMCALL leaf:
> > + *
> > + * Bit 15:0	Leaf number
> > + * Bit 23:16	Version number
> > + */
> > +#define SEAMCALL_VERSION_MASK		GENMASK_U64(23, 16)
> > +
> >  static __always_inline u64 __seamcall_dirty_cache(sc_func_t func, u64 fn,
> >  						  struct tdx_module_args *args)
> >  {
> > @@ -39,6 +48,7 @@ static __always_inline u64 __seamcall_dirty_cache(sc_func_t func, u64 fn,
> >  	 */
> >  	this_cpu_write(cache_state_incoherent, true);
> >  
> > +	FIELD_MODIFY(SEAMCALL_VERSION_MASK, &fn, args->version);
> >  	return func(fn, args);
> >  }
> 
> This is really looking fragmented and inconsistent.
> 
> What if someone *does* set the version bits in 'fn'? Also, if the "leaf

Then these bits would be ignored. FIELD_MODIFY() would overwrite the
version bits with the value in args->version.

> number" is just 16 bits, why is it a u64 in the API?

Because the "leaf number" is actually "bit 15:0 + bit 63". Seamldr calls
also use this path. And their leaf definitions include bit 63, such as:

  /* P-SEAMLDR SEAMCALL leaf function */
  #define P_SEAMLDR_INFO                0x8000000000000000
  #define P_SEAMLDR_INSTALL             0x8000000000000001

Sorry I only moved existing "SEAMCALL leaf" comments here, but the full
definition in TDX module SPEC is:

  Bit 15:0      Leaf number
  Bit 23:16     Version number
  Bit 24        Interrupt mode /* Setting 1 causes irq-resume loop forever when irq disabled, Linux always sets 0 */
  Bit 62:25     Reserved, must be 0
  Bit 63        Invoke *P-SEAMLDR* calls

> 
> Additionally, look at this:
> 
> > /*
> >  * Used in __tdcall*() to gather the input/output registers' values of the
> >  * TDCALL instruction when requesting services from the TDX module. This is a
> >  * software only structure and not part of the TDX module/VMM ABI
> >  */
> > struct tdx_module_args {
> 
> "version" doesn't fit this comment, does it? It's not a register.

OK, I think I can add a sentence to the comment a bit:

  ...when requesting service from the TDX module. The 'version' is an exception,
  it is encoded in rax along with the Leaf number. This is a software...

[...]

> If we add a new argument to 'tdx_module_args' it seems like the most
> consistent thing to do would be to extend the assembly to marshal it too.
> 
> We already have:
> 
>         /* Move Leaf ID to RAX */
>         mov %rdi, %rax
> 
> and it wouldn't be rocket science to add two instructions to get
> ->version in to place:
> 
> 	/* Leaf ABI version -> RAX[23:16]. Zero rest of RAX. */
> 	movzbl	TDX_MODULE_version(%rsi), %eax
> 	shl	$16, %eax
> 	/* Leaf number arg -> RAX[15:0]; Preserve [23:16]. */
> 	mov	%di, %ax

If we want to keep seamldr call work as is, we can't lose bit 63:

	/* Leaf ABI version -> RAX[23:16]. Zero rest of RAX. */
	movzbl	TDX_MODULE_version(%rsi), %eax
	shl	$16, %eax
	/* Leaf number arg -> RCX[63] and RCX[15:0]. Zero rest of RCX */
	mov	%rdi, %rcx
	movabs	$0x800000000000ffff, %rdx
	and	%rdx, %rcx
	/* Combine Leaf number and version -> RAX */
	or	%rcx, %rax


Or, I prefer more to assert that the Leaf numbers only touch bit 63 & bit [15:0]
in C code, this catches buggy bits rather than ignore them, and simplifies
assembly code:

@@ -58,6 +58,8 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
        int retry = RDRAND_RETRY_LOOPS;
        u64 ret;

+       BUILD_BUG_ON(fn & ~0x800000000000ffffULL); /* TODO: proper Macros */

then:

	/* Leaf ABI version -> RAX[23:16]. Zero rest of RAX. */
        movzbl  TDX_MODULE_version(%rsi), %eax
        shl     $16, %eax
	/* Combine Leaf ID and ABI version to RAX, they don't overlap */
        or      %rdi, %rax