Re: [PATCH v3 06/26] x86/virt/tdx: Prepare to support P-SEAMLDR SEAMCALLs

[Chao Gao]

On Wed, Jan 28, 2026 at 03:03:14PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>> P-SEAMLDR is another component alongside the TDX module within the
>> protected SEAM range. P-SEAMLDR can update the TDX module at runtime.
>> Software can talk with P-SEAMLDR via SEAMCALLs with the bit 63 of RAX
>> (leaf number) set to 1 (a.k.a P-SEAMLDR SEAMCALLs).
>
>This text kinda bugs me. It's OK, but needs improvement.
>
>First, don't explain the ABI in the changelog. Nobody cares that it's
>bit 63.
>
>
>Background:
>
>	The TDX architecture uses the "SEAMCALL" instruction to
>	communicate with SEAM mode software. Right now, the only SEAM
>	mode software that the kernel communicates with is the TDX
>	module. But, there are actually some components that run in SEAM
>	mode but that are separate from the TDX module: that SEAM
>	loaders. Right now, the only component that communicates with
>	them is the BIOS which loads the TDX module itself at boot. But,
>	to support updating the TDX module, the kernel now needs to be
>	able to talk to one of the the SEAM loaders: the Persistent
>	loader or "P-SEAMLDR".

Thanks. This is much clearer than my version.

One tiny nit: NP-SEAMLDR isn't SEAM mode software. It is an authenticated code
module (ACM).

>
>Then do this part:
>
>> P-SEAMLDR SEAMCALLs differ from SEAMCALLs of the TDX module in terms of
>> error codes and the handling of the current VMCS.
>Except I don't even know how the TDX module handles the current VMCS.
>That probably needs to be in there. Or, it should be brought up in the
>patch itself that implements this. Or, uplifted to the cover letter.

My logic was:

1. The kernel communicates with P-SEAMLDR via SEAMCALL, just like with the TDX
   Module.
2. But P-SEAMLDR SEAMCALLs and TDX Module SEAMCALLs are slightly different.

So we need some tweaks to the low-level helpers to add separate wrappers for
P-SEAMLDR SEAMCALLs.

To me, without mentioning #2, these tweaks in this patch (for separate wrappers
in the next patch) aren't justified.

<snip>

>>  static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
>>  			   struct tdx_module_args *args)
>>  {
>> +	u64 retry_code = TDX_RND_NO_ENTROPY;
>>  	int retry = RDRAND_RETRY_LOOPS;
>>  	u64 ret;
>>  
>> +	if (unlikely(is_seamldr_call(fn)))
>> +		retry_code = SEAMLDR_RND_NO_ENTROPY;
>
>(un)likey() has two uses:
>
>1. It's in performance critical code and compilers have been
>   demonstrated to be generating bad code.
>2. It's in code where it's not obvious what the fast path is
>   and (un)likey() makes the code more readable.
>
>Which one is this?

I think #2 although I am happy to drop "unlikely".

>
>Second, this is nitpicky, but I'd rather this be:
>
>	if (is_seamldr_call(fn))
>		retry_code = SEAMLDR_RND_NO_ENTROPY;
>	else
>		retry_code = TDX_RND_NO_ENTROPY;

Will do.

<snip>

>> +static inline void seamldr_err(u64 fn, u64 err, struct tdx_module_args *args)
>> +{
>> +	/*
>> +	 * Note: P-SEAMLDR leaf numbers are printed in hex as they have
>> +	 * bit 63 set, making them hard to read and understand if printed
>> +	 * in decimal
>> +	 */
>> +	pr_err("P-SEAMLDR (%llx) failed: %#016llx\n", fn, err);
>> +}
>
>Oh, lovely.
>
>Didn't you just propose changing the module SEAMCALL leaf numbers in
>decimal? Isn't it a little crazy to do one in decimal and the other in hex?

Yes, that's crazy. I'll just reuse seamcall_err(), so leaf numbers will be
printed in hex for both the TDX Module and P-SEAMLDR

>
>I'd really rather just see the TDX documentation changed.

I'll submit a request for TDX documentation to display leaf numbers in both hex
and decimal.

>
>But, honestly, I'd probably just leave the thing in hex, drop this hunk,
>and go thwack someone that writes TDX module documentation instead.
>
>>  static __always_inline int sc_retry_prerr(sc_func_t func,
>>  					  sc_err_func_t err_func,
>>  					  u64 fn, struct tdx_module_args *args)
>> @@ -96,4 +119,7 @@ static __always_inline int sc_retry_prerr(sc_func_t func,
>>  #define seamcall_prerr_ret(__fn, __args)					\
>>  	sc_retry_prerr(__seamcall_ret, seamcall_err_ret, (__fn), (__args))
>>  
>> +#define seamldr_prerr(__fn, __args)						\
>> +	sc_retry_prerr(__seamcall, seamldr_err, (__fn), (__args))

This can be dropped if we don't need to add seamldr_err().