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

[Nikolay Borisov]

On 7/17/26 07:31, Xu Yilun wrote:
> > > 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
>
> I agree that to describe each part of RAX clearly we need bitfields, and
> bitfields is not good for args.rax.  So args.rax is not generally good
> to me, worse than a separate u64.
>
> > 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.
>
> [..]
>
> > > 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.
>
> I wanna advance the progress, so if I have to choose I sort of like
> *args.version*, or even args.interrupt_mode in future. True this does
> not exactly match the registers' layout of TDX module ABI, but I also
> don't see big problems. Using fn instead of args.rax at the first place
> already shows we don't have to be too strictly aligned to ABI. We are
> defining SW APIs.
>
> To me the args.version scheme clearly defines 3 components that we want
> from SW POV:
>
> 	the leaf	fn
> 	the version	args.version
> 	the operands	args.rcx	
> 			...
> 			args.r15
>
> The TDX module SPEC tells us each value of these components, we simply
> fill them in. No extra bit-or.

Just my .2 cent FWIW:

At the end of the day the layout of args is the software API that is 
presented to the rest of the system, the ABI is a low level detail 
handled in TDX_MODULE_CALL. Let's bury all the ugly handling in the asm 
and present a simpler interface to the rest of the kernel

Exposing a raw representation of RAX is somewhat cumbersome because of 
the reasons already listed so "converting" that to 3 variables in the 
structure seems same to me.


<snip>