Re: [PATCH v3 02/16] x86/tdx: Add helpers to check return status codes

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

On 9/19/2025 7:22 AM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> 
> The TDX error code has a complex structure. The upper 32 bits encode the
> status code (higher level information), while the lower 32 bits provide
> clues about the error, such as operand ID, CPUID leaf, MSR index, etc.
> 
> In practice, the kernel logic cares mostly about the status code. Whereas
> the error details are more often dumped to warnings to be used as
> debugging breadcrumbs. This results in a lot of code that masks the status
> code and then checks the resulting value. Future code to support Dynamic
> PAMT will add yet mode SEAMCALL error code checking. To prepare for this,
> do some cleanup to reduce the boiler plate error code parsing.
> 
> Since the lower bits that contain details are needed for both error
> printing and a few cases where the logical code flow does depend on them,
> don’t reduce the boiler plate by masking the detail bits inside the
> SEAMCALL wrappers, returning only the status code. Instead, create some
> helpers to perform the needed masking and comparisons.
> 
> For the status code based checks, create a macro for generating the
> helpers based on the name. Name the helpers IS_TDX_FOO(), based on the
> discussion in the Link.
> 
> Many of the checks that consult the error details are only done in a
> single place. It could be argued that there is not any code savings by
> adding helpers for these checks. Add helpers for them anyway so that the
> checks look consistent when uses with checks that are used in multiple
> places (i.e. sc_retry_prerr()).
> 
> Finally, update the code that previously open coded the bit math to use
> the helpers.
> 
> Link: https://lore.kernel.org/kvm/aJNycTvk1GEWgK_Q@google.com/
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [Enhance log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> v3:
>   - Split from "x86/tdx: Consolidate TDX error handling" (Dave, Kai)
>   - Change name from IS_TDX_ERR_FOO() to IS_TDX_FOO() after the
>     conclusion to one of those naming debates. (Sean, Dave)
> ---
>   arch/x86/coco/tdx/tdx.c                 |  6 +--
>   arch/x86/include/asm/shared/tdx_errno.h | 54 ++++++++++++++++++++++++-
>   arch/x86/include/asm/tdx.h              |  2 +-
>   arch/x86/kvm/vmx/tdx.c                  | 44 +++++++++-----------
>   arch/x86/virt/vmx/tdx/tdx.c             |  8 ++--
>   5 files changed, 80 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 7b2833705d47..96554748adaa 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -129,9 +129,9 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>   
>   	ret = __tdcall(TDG_MR_REPORT, &args);
>   	if (ret) {
> -		if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> +		if (IS_TDX_OPERAND_INVALID(ret))
>   			return -ENXIO;
> -		else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
> +		else if (IS_TDX_OPERAND_BUSY(ret))
>   			return -EBUSY;
>   		return -EIO;
>   	}

There are TDCALL_RETURN_CODE() usages left in tdx_mcall_extend_rtmr().
Please clean them up as well, and the definitions of TDCALL_RETURN_CODE 
macro and friends can be removed totally:


   /* TDX Module call error codes */
   #define TDCALL_RETURN_CODE(a)	((a) >> 32)
   #define TDCALL_INVALID_OPERAND	0xc0000100
   #define TDCALL_OPERAND_BUSY	0x80000200

> @@ -316,7 +316,7 @@ static void reduce_unnecessary_ve(void)
>   {
>   	u64 err = tdg_vm_wr(TDCS_TD_CTLS, TD_CTLS_REDUCE_VE, TD_CTLS_REDUCE_VE);
>   
> -	if (err == TDX_SUCCESS)
> +	if (IS_TDX_SUCCESS(err))

I would expect a separate patch to change it first to

	if ((err & TDX_STATUS_MASK) == TDX_SUCCESS)

because it certainly changes the semantic of the check.

And this applies to some other places below, e.g.,

 > -	if (err == TDX_FLUSHVP_NOT_DONE)
 > +	if (IS_TDX_FLUSHVP_NOT_DONE(err))

 > -	if (err == TDX_RND_NO_ENTROPY) {
 > +	if (IS_TDX_RND_NO_ENTROPY(err)) {


>   		return;
>   
>   	/*
> diff --git a/arch/x86/include/asm/shared/tdx_errno.h b/arch/x86/include/asm/shared/tdx_errno.h
> index f98924fe5198..49ab7ecc7d54 100644
> --- a/arch/x86/include/asm/shared/tdx_errno.h
> +++ b/arch/x86/include/asm/shared/tdx_errno.h
> @@ -2,8 +2,10 @@
>   #ifndef _X86_SHARED_TDX_ERRNO_H
>   #define _X86_SHARED_TDX_ERRNO_H
>   
> +#include <asm/trapnr.h>
> +

This belongs to the previous patch, I think.

And in that patch, the <asm/trapnr.h> can be removed from
arch/x86/include/asm/tdx.h?

>   /* Upper 32 bit of the TDX error code encodes the status */
> -#define TDX_SEAMCALL_STATUS_MASK		0xFFFFFFFF00000000ULL
> +#define TDX_STATUS_MASK				0xFFFFFFFF00000000ULL
>   
>   /*
>    * TDX SEAMCALL Status Codes
> @@ -52,4 +54,54 @@
>   #define TDX_OPERAND_ID_SEPT			0x92
>   #define TDX_OPERAND_ID_TD_EPOCH			0xa9
>   
> +#ifndef __ASSEMBLER__
> +#include <linux/bits.h>
> +#include <linux/types.h>
> +
> +static inline u64 TDX_STATUS(u64 err)
> +{
> +	return err & TDX_STATUS_MASK;
> +}
> +
> +static inline bool IS_TDX_NON_RECOVERABLE(u64 err)
> +{
> +	return (err & TDX_NON_RECOVERABLE) == TDX_NON_RECOVERABLE;
> +}
> +
> +static inline bool IS_TDX_SW_ERROR(u64 err)
> +{
> +	return (err & TDX_SW_ERROR) == TDX_SW_ERROR;
> +}

Kai already catched that it can be defined with DEFINE_TDX_ERRNO_HELPER()

The background is that we wanted to use SEAMCALL return code to cover 
the #GP/#UD/VMFAILINVALID cases generally so that we asked TDX 
architecuts to reserve Class ID (0XFF) for software usage.

SW_ERROR is just a Linux defined status code (in the upper 32 bits), and 
details in the lower 32 bits to identify among #GP/#UD/VMFAILINVALID.

So ...

> +static inline bool IS_TDX_SEAMCALL_VMFAILINVALID(u64 err)
> +{
> +	return (err & TDX_SEAMCALL_VMFAILINVALID) ==
> +		TDX_SEAMCALL_VMFAILINVALID;
> +}
> +
> +static inline bool IS_TDX_SEAMCALL_GP(u64 err)
> +{
> +	return (err & TDX_SEAMCALL_GP) == TDX_SEAMCALL_GP;
> +}
> +
> +static inline bool IS_TDX_SEAMCALL_UD(u64 err)
> +{
> +	return (err & TDX_SEAMCALL_UD) == TDX_SEAMCALL_UD;
> +}

... TDX_SEAMCALL_{VMFAILINVALID,GP,UD} are full 64-bit return codes, not 
some masks. The check of

	(err & TDX_SEAMCALL_*) == TDX_SEAMCALL_*

isn't correct here and we need to check

	err == TDX_SEAMCALL_*;

e.g., The #UD is of number 6, which is 110b. If SEAMCALL could cause 
exception of vector 111b, 1110b, 1111b, they can pass the check of 
IS_TDX_SEAMCALL_UD()