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

[Nikolay Borisov <nik.borisov@suse.com>]

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>