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

Huang, Kai
2025-09-19T01:26:57Z

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);
> + …