Re: [PATCH v2 05/17] x86/virt/tdx: Make TDX module initialize the extensions

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 6/24/2026 1:03 AM, Xu Yilun wrote:
>>> +/* Initialize TDX module extensions for extension SEAMCALLs */
>>> +static __init int tdx_ext_init(void)
>>> +{
>>> +	struct tdx_module_args args = {};
>>> +	u64 r;
>>> +
>>> +	do {
>>> +		r = seamcall(TDH_EXT_INIT, &args);
>>> +	} while (r == TDX_INTERRUPTED_RESUMABLE);
>>> +
>>> +	if (r != TDX_SUCCESS)
>>> +		return -EFAULT;

As Chao caught in patch 04, we need to return -EIO here for consistence.

>>
>> [Severity: Medium]
>> Does this code silently discard the 64-bit hardware error code?
>>
>> When TDH_EXT_INIT fails, replacing the return value with -EFAULT means
>> the caller only logs a generic initialization failure without details.
>> Other TDX initialization functions use seamcall_prerr_ret() to preserve
>> and log the hardware error.
>>
>> Could this be updated to use seamcall_prerr_ret() to ensure the exact
>> firmware failure reason is logged?
> 
> No, it can't use seamcall_prerr_ret() directly. TDX_INTERRUPTED_RESUMABLE
> can't be handled in a unified way for all SEAMCALLs so can't embed in
> fundamental seamcall wrappers. I see 2 conventions:
> 
>    1. Host should set a resume=1 parameter on retry, such as,
>       TDH_PHYMEM_CACHE_WB, and several migration SEAMCALLs,
>       TDH.EXPORT.MEM, TDH.EXPORT.STATE.XXX
> 
>    2. Host just refills the previous args (may have been modified by seamcall
>       output) on retry. Much more examples, TDH_EXT_INIT, TDH_EXT_MEM_ADD,
>       TDH_QUOTE_INIT, TDH_QUOTE_GET...
> 
> So I think there is a need the callers have to:
> 
>    1. call seamcall() or seamcall wrappers (tdh_xxx)
>    2. specific handling of raw SEAMCALL return code
>    3. do a standard TDX error logging and Linux errno conversion.
> 
> seamcall_prerr_ret() does 1 & 2 & 3, but can't customize step 2.
> 
> I tried to add some TDX error logging & errno conversion helpers, most of
> the code are abstracted from seamcall_prerr_ret(), see below.
> 
> But I'm not sure if it worths the churn.

Though I have some comment, below refactor doesn't look so bad.

The only risk is existing SEAMCALL warppers is a mess and people may not 
want to add any more before re-work them. So the conservative option 
would to just add an ad-hoc pr_err() for !TDX_SUCCESS case?

> ----8<----
> 
> 
> diff --git a/arch/x86/virt/vmx/tdx/seamcall_internal.h b/arch/x86/virt/vmx/tdx/seamcall_internal.h
> index be5f446467df..f838715c9446 100644
> --- a/arch/x86/virt/vmx/tdx/seamcall_internal.h
> +++ b/arch/x86/virt/vmx/tdx/seamcall_internal.h
> @@ -78,12 +78,10 @@ static inline void seamcall_err_ret(u64 fn, u64 err,
>                          args->r9, args->r10, args->r11);
>   }
> 
> -static __always_inline int sc_retry_prerr(sc_func_t func,
> -                                         sc_err_func_t err_func,
> -                                         u64 fn, struct tdx_module_args *args)
> +static inline int sc_errno_prerr(sc_err_func_t err_func,
> +                                u64 fn, u64 sret,
> +                                struct tdx_module_args *args)
>   {
> -       u64 sret = sc_retry(func, fn, args);
> -
>          if (sret == TDX_SUCCESS)
>                  return 0;
> 
> @@ -100,10 +98,25 @@ static __always_inline int sc_retry_prerr(sc_func_t func,
>          return -EIO;
>   }
> 
> +static __always_inline int sc_retry_prerr(sc_func_t func,
> +                                         sc_err_func_t err_func,
> +                                         u64 fn, struct tdx_module_args *args)
> +{
> +       u64 sret = sc_retry(func, fn, args);
> +
> +       return sc_errno_prerr(err_func, fn, sret, args);
> +}
> +
>   #define seamcall_prerr(__fn, __args)                                           \
>          sc_retry_prerr(__seamcall, seamcall_err, (__fn), (__args))
> 
>   #define seamcall_prerr_ret(__fn, __args)                                       \
>          sc_retry_prerr(__seamcall_ret, seamcall_err_ret, (__fn), (__args))
> 
> +#define seamcall_errno_prerr(__fn, __sret, __args)                             \
> +       sc_errno_prerr(seamcall_err, (__fn), (__sret), (__args))
> +
> +#define seamcall_errno_prerr_ret(__fn, __sret, __args)                         \
> +       sc_errno_prerr(seamcall_err_ret, (__fn), (__sret), (__args))

We don't need these two wrappers.

The existing seamcall_prerr() and seamcall_prerr_ret() do two things:
1. call SEAMCALL;
2. handling the return code of SEAMCALL;

But the 2 new wrappers only do setp 2. So we can just call 
sc_errno_prerr() in the following cases ...

>   #endif /* _X86_VIRT_SEAMCALL_INTERNAL_H */
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 826504fbcdc7..637ff01ed0f1 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1403,10 +1403,7 @@ static int tdx_ext_init(void)
>                  r = seamcall(TDH_EXT_INIT, &args);
>          } while (r == TDX_INTERRUPTED_RESUMABLE);
> 
> -       if (r != TDX_SUCCESS)
> -               return -EFAULT;
> -
> -       return 0;
> +       return seamcall_errno_prerr(TDH_EXT_INIT, r, &args);

... return sc_errno_prerr(seamcall_err, TDH_EXT_INIT, r, &args);

and

>   }
> 
>   #define HPA_LIST_INFO_FIRST_ENTRY      GENMASK_U64(11, 3)
> @@ -1438,10 +1435,7 @@ static __init int tdx_ext_mem_add(struct page *hpa_list_page,
>                  r = seamcall_ret(TDH_EXT_MEM_ADD, &args);
>          } while (r == TDX_INTERRUPTED_RESUMABLE);
> 
> -       if (r != TDX_SUCCESS)
> -               return -EFAULT;
> -
> -       return 0;
> +       return seamcall_errno_prerr_ret(TDH_EXT_MEM_ADD, r, &args);

... return sc_errno_prerr(seamcall_err_ret, TDH_EXT_MEM_ADD, r, &args);

>   }