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

[Dave Hansen <dave.hansen@intel.com>]

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

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.

> In preparation for adding support for P-SEAMLDR SEAMCALLs, do the two
> following changes to SEAMCALL low-level helpers:
> 
> 1) Tweak sc_retry() to retry on "lack of entropy" errors reported by
>    P-SEAMLDR because it uses a different error code.
> 
> 2) Add seamldr_err() to log error messages on P-SEAMLDR SEAMCALL failures.



> diff --git a/arch/x86/virt/vmx/tdx/seamcall.h b/arch/x86/virt/vmx/tdx/seamcall.h
> index 0912e03fabfe..256f71d6ca70 100644
> --- a/arch/x86/virt/vmx/tdx/seamcall.h
> +++ b/arch/x86/virt/vmx/tdx/seamcall.h
> @@ -34,15 +34,28 @@ static __always_inline u64 __seamcall_dirty_cache(sc_func_t func, u64 fn,
>  	return func(fn, args);
>  }
>  
> +#define SEAMLDR_RND_NO_ENTROPY	0x8000000000030001ULL

<sigh>

#define TDX_RND_NO_ENTROPY      0x8000020300000000ULL

So they're not even close values. They're not consistent or even a bit
off or anything.

Honestly, this needs a justification for why this was done this way. Why
can't "SEAM mode" be a monolithic thing from the kernel's perspective?

> +#define SEAMLDR_SEAMCALL_MASK	_BITUL(63)
> +
> +static inline bool is_seamldr_call(u64 fn)
> +{
> +	return fn & SEAMLDR_SEAMCALL_MASK;
> +}
> +
>  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?

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;

or even:

	retry_code = TDX_RND_NO_ENTROPY;
	if (is_seamldr_call(fn))
		retry_code = SEAMLDR_RND_NO_ENTROPY;

That makes it trivial that 'retry_code' can only have two values. It's
nitpicky because the original initialization is so close.

>  	do {
>  		ret = func(fn, args);
> -	} while (ret == TDX_RND_NO_ENTROPY && --retry);
> +	} while (ret == retry_code && --retry);
>  
>  	return ret;
>  }
> @@ -68,6 +81,16 @@ 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)
> +{
> +	/*
> +	 * 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?

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

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))
> +
>  #endif

So, honestly, for me, it's a NAK for this whole patch.

Go change the P-SEAMLDR to use the same error code as the TDX module,
and fix the documentation. No kernel changes, please.