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

[Dave Hansen]

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.

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

I don't *love* this. But, to be honest, I'd never even seen
FIELD_MODIFY() before.

Also, please double and triple check the assembly above. This does
depend pretty heavily on the rules around register naming and width, and
x86 is not exactly the most straightforward ISA in the world there.

I also wouldn't be shocked if there's a nicer way to do this that
someone else dreams up.