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

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

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.