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

[Xu Yilun <yilun.xu@linux.intel.com>]

> 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

[*]: https://lore.kernel.org/all/20251001025442.427697-4-chao.gao@intel.com/

[...]

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

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

[*] 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);
 }

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