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

[Kiryl Shutsemau <kas@kernel.org>]

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
> number" is just 16 bits, why is it a u64 in the API?
> 
> 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.
> 
> If "fn" is just the 16-bit leaf number it should be a u16 everywhere.
> Then there's no worry about the version number leaking in there
> somewhere. The type can't even _carry_ the version number.
> 
> As a general rule, I dislike doing things in assembly that can be done
> in C. But, in this case, we have some pretty darn simple assembly doing
> a pretty simple job: marshaling data out of 'tdx_module_args' and in to
> registers.

Ughh.. I think it is totally wrong direction.

The version is not a register operand, it is part of the function
number. And RAX carries more than leaf+version: bit 24 is
INTERRUPT_MODE (enumerated by TDX_FEATURES0 bit 62) and bit 63 selects
P-SEAMLDR. If the version gets a struct field marshaled in assembly,
do we add fields for these bits too? Composing 'fn' as u64 in the
caller is the honest representation of the ABI.

Rather than pushing the version all the way down to assembly, let's
look if we can salvage what was proposed initially[1].

I know you didn't like it, but the problem there was that the version
was invisible: TDH_SYS_UPDATE was silently v1 and the old one got
renamed to TDH_SYS_UPDATE_V0. With the version spelled out at the call
site, that objection goes away:

int tdx_module_run_update(void)
{
	struct tdx_module_args args = {};
	u64 seamcall_fn = TDH_SYS_UPDATE;
	int ret;

	if (tdx_addon_feature0) {
		seamcall_fn |= SEAMCALL_LEAF_VER(1);
		args.r9 = tdx_addon_feature0;
	}

	ret = seamcall_prerr(seamcall_fn, &args);
	if (ret)
		return ret;
	...
}

And tdh_vp_init() stays what it is in mainline today, just with the
macro instead of the open-coded shift:

	/* apicid requires version == 1. */
	return seamcall(TDH_VP_INIT | SEAMCALL_LEAF_VER(1), &args);

This also addresses all three complaints above at once: there is no
FIELD_MODIFY() to silently overwrite version bits in 'fn' as it is
exactly what the caller wrote, tdx_module_args stays a pure register
structure, and the assembly is untouched.

[1] https://lore.kernel.org/kvm/20260618081355.3253581-3-yilun.xu@linux.intel.com/

-- 
  Kiryl Shutsemau / Kirill A. Shutemov