| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <24d2f165-f854-4996-89cf-28d644c592a3@intel.com>
Date: Fri, 26 Sep 2025 14:32:34 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>, kas@...nel.org,
bp@...en8.de, chao.gao@...el.com, dave.hansen@...ux.intel.com,
isaku.yamahata@...el.com, kai.huang@...el.com, kvm@...r.kernel.org,
linux-coco@...ts.linux.dev, linux-kernel@...r.kernel.org, mingo@...hat.com,
pbonzini@...hat.com, seanjc@...gle.com, tglx@...utronix.de, x86@...nel.org,
yan.y.zhao@...el.com, vannapurve@...gle.com
Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH v3 02/16] x86/tdx: Add helpers to check return status
codes
On 9/19/2025 7:22 AM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.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@...ux.intel.com>
> [Enhance log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.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()
Powered by blists - more mailing lists