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

[Kiryl Shutsemau <kas@kernel.org>]

On Fri, Jul 10, 2026 at 09:48:22AM -0700, Dave Hansen wrote:
> On 7/10/26 09:21, Kiryl Shutsemau wrote:
> > The version is not a register operand, it is part of the function
> > number.
> 
> I'm not sure why that's important.
> 
> Sure, the arg structure is all full registers now. But it's not even all
> the registers because RAX isn't there. So let's say we made the data
> structure smarter:
> 
> struct tdx_module_args {
>         union {
>                 u64 rax;
>                 struct {
>                         u16 function_nr;
>                         u8  version
>                         u8  padding0;
>                         u32 flags;
>                 };
>         };
>         u64 rcx;
>         u64 rdx;
> 	...
> }

This union doesn't match the ABI it is supposed to represent.

RAX is:

  15:0	Leaf number
  23:16	Version number
  24	INTERRUPT_MODE
  62:25	Reserved
  63	P-SEAMLDR select

In your layout INTERRUPT_MODE ends up somewhere in padding0 and the
P-SEAMLDR bit is BIT(31) of 'flags'. Nothing lines up with the spec.

To describe the format properly the struct would need bitfields, and we
generally discourage bitfields for ABI-defined layouts. Without
bitfields it degrades into masks and shifts on a u64 -- which is
exactly what composing 'fn' with a macro is. The union doesn't add
anything on top of that, it only hides where the bits are.

> 
> All of the:
> 
>         struct tdx_module_args args = {};
> 
> instances would default to version=0. It would take a one-liner to get
> bit 63 set:
> 
>  static int seamldr_call(u64 fn, struct tdx_module_args *args)
>  {
>  ...
> +	args->flags |= SEAMLDR_BIT;
>         guard(raw_spinlock)(&seamldr_lock);
>         return seamcall_prerr(fn, args);
>  }
> 
> The truth of the matter is that 'fn' *IS* the RAX from the TDX ABI.
> We're carrying it around the kernel in that format, and it just doesn't
> work very well.
> 
> The alternative is to carry the logical pieces of RAX around the kernel
> and them assemble RAX out of them in one (or very few) places. *Not* to
> build RAX in the TDX module ABI early far from the TDX module ABI layer
> itself.

I don't see what delaying the assembly buys us.

The SEAMCALL helper is where we decide what we want from the call: the
leaf, the version, the operands. Nothing below it adds information --
sc_retry() and __seamcall_dirty_cache() only retry on entropy failure
and track cache state. There is no layer further down that is in a
better position to compose RAX than the helper itself.

And it is not "far from the TDX module ABI layer". The tdh_*() helpers
are the C representation of the ABI functions. They are the ABI layer.

We also don't carry the pieces "around the kernel" today. The
composition already happens in exactly one place -- a macro next to the
leaf defines. Call sites don't open-code shifts.

That said, I'm okay with RAX living in the struct as a plain u64
instead of a separate 'fn' argument, if you prefer the structure to
carry all the registers. But its composition should stay early, in the
helper, where the decisions about the call are made:

	args.rax = TDH_VP_INIT | SEAMCALL_LEAF_VER(1);

-- 
  Kiryl Shutsemau / Kirill A. Shutemov