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

[Xu Yilun]

> > 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.

> 
> 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);

This is the scheme that is most aligned with ABI layout. But I assume we
don't really see it worth the massive code churn, is it?