Re: [PATCH v2 04/21] x86/virt/tdx: Prepare to support P-SEAMLDR SEAMCALLs

[Chao Gao]

On Tue, Jan 13, 2026 at 05:48:56PM +0800, Xu Yilun wrote:
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index e872a411a359..7ad026618a23 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -32,6 +32,11 @@
>>  #define TDX_SUCCESS		0ULL
>>  #define TDX_RND_NO_ENTROPY	0x8000020300000000ULL
>>  
>> +/* P-SEAMLDR SEAMCALL leaf function error codes */
>> +#define SEAMLDR_RND_NO_ENTROPY	0x8000000000030001ULL
>> +
>> +#define SEAMLDR_SEAMCALL_MASK	_BITUL(63)
>
>If we do want Patch #3 [*], I think no need to expose this bit in
>public, define it in your newly added arch/x86/virt/vmx/tdx/seamcall.h

Makes sense.

>
>[*]: https://lore.kernel.org/all/20251001025442.427697-4-chao.gao@xxxxxxxxx/
>
>[...]
>
>> +static inline bool sc_need_retry(u64 fn, u64 error_code)
>> +{
>> +	if (is_seamldr_call(fn))
>> +		return error_code == SEAMLDR_RND_NO_ENTROPY;
>> +	else
>> +		return error_code == TDX_RND_NO_ENTROPY;
>> +}
>> +
>
>Maybe we can remove this single-use wrapper and integrate it in
>sc_retry?
>
>>  static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
>>  			   struct tdx_module_args *args)
>>  {
>
>	u64 retry_code = TDX_RND_NO_ENTROPY;
>
>	if (is_seamldr_call(fn))
>		retry_code = SEAMLDR_RND_NO_ENTROPY;
>
>> @@ -22,7 +35,7 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
>>  
>>  	do {
>>  		ret = func(fn, args);
>> -	} while (ret == TDX_RND_NO_ENTROPY && --retry);
>> +	} while (sc_need_retry(fn, ret) && --retry);
>
>	} while (ret == retry_code && --retry);
>
>Seems more straightforward to me.

I am ok with this change. Will do.

>
>>  
>>  	return ret;
>>  }
>> @@ -48,6 +61,17 @@ static inline void seamcall_err_ret(u64 fn, u64 err,
>>  			args->r9, args->r10, args->r11);
>>  }
>>  
>> +static inline void seamldr_err(u64 fn, u64 err, struct tdx_module_args *args)
>> +{
>> +	/*
>> +	 * Get the actual leaf number. No need to print the bit used to
>> +	 * differentiate between P-SEAMLDR and TDX module as the "P-SEAMLDR"
>> +	 * string in the error message already provides that information.
>> +	 */
>> +	fn &= ~SEAMLDR_SEAMCALL_MASK;
>
>Why we must strip the bit from leaf number? See from P-seamldr SPEC [*],
>all leaf definitions are with this bit, same for your C code:
>
>  +/* P-SEAMLDR SEAMCALL leaf function */
>  +#define P_SEAMLDR_INFO			0x8000000000000000
>
>So my feeling is, log readability reduces without this bit. We don't
>have to make all developers understand this bit, just expose the whole
>leaf as magic number.

Agree. Printing P-SEAMLDR leaf numbers in hex without removing bit 63 is
better, so the values shown in the logs can match code and spec.

>
>[*] https://cdrdv2.intel.com/v1/dl/getContent/733584
>
>> +	pr_err("P-SEAMLDR (%lld) failed: 0x%016llx\n", fn, err);
>
>I see no problem we keep both "P-SEAMLDR" string & SEAMLDR bit printed.
>
>
>And could we handle it the same as sc_retry(), something like:
>
> static inline void seamcall_err(u64 fn, u64 err, struct tdx_module_args *args)
> {
>-       pr_err("SEAMCALL (0x%016llx) failed: 0x%016llx\n", fn, err);
>+       char *call = "SEAMCALL";
>+
>+       if (is_seamldr_call(fn))
>+               call = "P-SEAMLDR";
>+
>+       pr_err("%s (0x%016llx) failed: 0x%016llx\n", call, fn, err);
> }

It's doable, but the conditional is unnecessary ...

>
>And the benifit is ...
>
>> +}
>> +
>>  static __always_inline int sc_retry_prerr(sc_func_t func,
>>  					  sc_err_func_t err_func,
>>  					  u64 fn, struct tdx_module_args *args)
>> @@ -76,4 +100,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))
>
>... we don't need this definition anymore, just use seamcall_prerr()

Having this seamldr_prerr() (and seamldr_err()) is to eliminate that conditional.