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

["Huang, Kai" <kai.huang@intel.com>]

On Thu, 2025-09-18 at 16:22 -0700, 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,
		    ^
		    more

> 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()).
	  ^
	  i.e. or e.g. ?
> 
> 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>
> 

[...]

> +#include <asm/trapnr.h>
> +
>  /* Upper 32 bit of the TDX error code encodes the status */
> -#define TDX_SEAMCALL_STATUS_MASK		0xFFFFFFFF00000000ULL
> +#define TDX_STATUS_MASK				0xFFFFFFFF00000000ULL

Nit: the whitespace/tab change here is a bit odd?

>  
>  /*
>   * 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_SW_ERROR(u64 err)
> +{
> +	return (err & TDX_SW_ERROR) == TDX_SW_ERROR;
> +}
> +
> 

[...]

> +#define DEFINE_TDX_ERRNO_HELPER(error)			\
> +	static inline bool IS_##error(u64 err)	\
> +	{						\
> +		return TDX_STATUS(err) == error;	\
> +	}

Given:

+#define TDX_ERROR			_BITULL(63)
+#define TDX_SW_ERROR			(TDX_ERROR | GENMASK_ULL(47, 40))

I think IS_TDX_SW_ERROR() can also use the DEFINE_TDX_ERRNO_HELPER() ?

> +
> +DEFINE_TDX_ERRNO_HELPER(TDX_SUCCESS);
> +DEFINE_TDX_ERRNO_HELPER(TDX_RND_NO_ENTROPY);
> +DEFINE_TDX_ERRNO_HELPER(TDX_OPERAND_INVALID);
> +DEFINE_TDX_ERRNO_HELPER(TDX_OPERAND_BUSY);
> +DEFINE_TDX_ERRNO_HELPER(TDX_VCPU_NOT_ASSOCIATED);
> +DEFINE_TDX_ERRNO_HELPER(TDX_FLUSHVP_NOT_DONE);
> +

[...]