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

Edgecombe, Rick P
2025-09-26T21:27:36Z

On Fri, 2025-09-26 at 14:32 +0800, Xiaoyao Li wrote:
> On 9/19/2025 7:22 AM, Rick Edgecombe wrote:
> >    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:

Oh, good call.

>
>
>    /* 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.

I'm not sure. I see your point, but I'm not sure it's worth the extra
churn.

>
> 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?

Yep, on both accounts.

>
> >   /* 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.

Interesting history. The comment references this in an vague way, but I
can't find it in the official docs anywhere. Did I miss it?

>
> 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()

Right. Good catch.