Re: [PATCHv4 03/30] x86/tdx: Provide common base for SEAMCALL and TDCALL C wrappers

[Kirill A. Shutemov]

On Thu, Feb 24, 2022 at 08:35:32AM -0800, Dave Hansen wrote:
> ...
> >  arch/x86/include/asm/tdx.h    | 20 ++++++++
> >  arch/x86/kernel/asm-offsets.c |  9 ++++
> >  arch/x86/virt/tdxcall.S       | 91 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 120 insertions(+)
> >  create mode 100644 arch/x86/virt/tdxcall.S
> > 
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index ba8042ce61c2..2f8cb1e53e77 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -8,6 +8,25 @@
> >  #define TDX_CPUID_LEAF_ID	0x21
> >  #define TDX_IDENT		"IntelTDX    "
> >  
> > +#define TDX_SEAMCALL_VMFAILINVALID     0x8000FF00FFFF0000ULL
> 
> This needs a comment.  It's a completely software-defined value, right?

Right, bits 47:40 indicate "Status Code Class" and 0xFF reserved for SW
usage. (It is not reflected in the current public spec, but it will be in
the next.)

> > diff --git a/arch/x86/virt/tdxcall.S b/arch/x86/virt/tdxcall.S
> > new file mode 100644
> > index 000000000000..90569faedacc
> > --- /dev/null
> > +++ b/arch/x86/virt/tdxcall.S
> > @@ -0,0 +1,91 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#include <asm/asm-offsets.h>
> > +#include <asm/tdx.h>
> > +
> > +/*
> > + * TDX guests use the TDCALL instruction to make requests to the
> > + * TDX module and hypercalls to the VMM.
> > + *
> > + * TDX host user SEAMCALL instruction to make requests to TDX module.
> 
> 		^ uses??
> 
> 
> > + * They are supported in Binutils >= 2.36.
> > + */
> 
> While you're in there, could we make this a little more structured, please?
> 
> /*
>  * TDX_MODULE_CALL - common helper macro for both
>  *		     TDCALL and SEAMCALL instructions.
>  * 		
>  * TDCALL   - used by TDX guests to make requests to the
>  * 	      TDX module and hypercalls to the VMM.
>  * SEAMCALL - used by TDX hosts to make requests to the
>  *	      TDX module.
>  *
>  * Both instruction are supported in Binutils >= 2.36.
>  */

Sure.

 > +	/*
> > +	 * SEAMCALL instruction is essentially a VMExit from VMX root
> > +	 * mode to SEAM VMX root mode.  VMfailInvalid (CF=1) indicates
> > +	 * that the targeted SEAM firmware is not loaded or disabled,
> > +	 * or P-SEAMLDR is busy with another SEAMCALL.  %rax is not
> > +	 * changed in this case.
> > +	 *
> > +	 * Set %rax to TDX_SEAMCALL_VMFAILINVALID for VMfailInvalid.
> > +	 * This value will never be used as actual SEAMCALL error code.
> > +	 */
> 
> Why can TDX_SEAMCALL_VMFAILINVALID never be used as an actual SEAMCALL
> error code?  Would it be more accurate to say that today's P-SEAMLDR and
> TDX module never use TDX_SEAMCALL_VMFAILINVALID as an actual SEAMCALL
> error code?

No, see above. There's actual guarantee.

> There's an important distinction there.  It's possible that the SEAMCALL
> instruction architecture says or implies that it can never have
> TDX_SEAMCALL_VMFAILINVALID as a return value.  It's also possible that
> this "never" comes from a guess about what P-SEAMLDRs or TDX modules can do.
> 

...

> 
> That doesn't look too bad.  Please just send an updated version with the
> revised comments.

How about this: