Re: [PATCH v5 08/22] x86/virt/tdx: Shut down TDX module in case of error

[Kai Huang]

On Mon, 2022-06-27 at 15:56 -0700, Dave Hansen wrote:
> On 6/27/22 15:34, Kai Huang wrote:
> > On Mon, 2022-06-27 at 13:46 -0700, Dave Hansen wrote:
> > I think I can just use __always_unused for this purpose?
> > 
> > So I think we put seamcall() implementation to the patch which implements
> > __seamcall().  And we can inline for seamcall() and put it in either tdx.h or
> > tdx.c, or we can use __always_unused  (or the one you prefer) to get rid of the
> > warning.
> > 
> > What's your opinion?
> 
> A temporary __always_unused seems fine to me.

Thanks will do.

> 
> > > > Alternatively, we can always add EXTABLE to TDX_MODULE_CALL macro to handle #UD
> > > > and #GP by returning dedicated error codes (please also see my reply to previous
> > > > patch for the code needed to handle), in which case we don't need such check
> > > > here.
> > > > 
> > > > Always handling #UD in TDX_MODULE_CALL macro also has another advantage:  there
> > > > will be no Oops for #UD regardless the issue that "there's no way to check
> > > > whether VMXON has been done" in the above comment.
> > > > 
> > > > What's your opinion?
> > > 
> > > I think you should explore using the EXTABLE.  Let's see how it looks.
> > 
> > I tried to wrote the code before.  I didn't test but it should look like to
> > something below.  Any comments?
> > 
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index 4b75c930fa1b..4a97ca8eb14c 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -8,6 +8,7 @@
> >  #include <asm/ptrace.h>
> >  #include <asm/shared/tdx.h>
> > 
> > +#ifdef CONFIG_INTEL_TDX_HOST
> >  /*
> >   * SW-defined error codes.
> >   *
> > @@ -18,6 +19,21 @@
> >  #define TDX_SW_ERROR                   (TDX_ERROR | GENMASK_ULL(47, 40))
> >  #define TDX_SEAMCALL_VMFAILINVALID     (TDX_SW_ERROR | _UL(0xFFFF0000))
> > 
> > +/*
> > + * Special error codes to indicate SEAMCALL #GP and #UD.
> > + *
> > + * SEAMCALL causes #GP when SEAMRR is not properly enabled by BIOS, and
> > + * causes #UD when CPU is not in VMX operation.  Define two separate
> > + * error codes to distinguish the two cases so caller can be aware of
> > + * what caused the SEAMCALL to fail.
> > + *
> > + * Bits 61:48 are reserved bits which will never be set by the TDX
> > + * module.  Borrow 2 reserved bits to represent #GP and #UD.
> > + */
> > +#define TDX_SEAMCALL_GP                (TDX_ERROR | GENMASK_ULL(48, 48))
> > +#define TDX_SEAMCALL_UD                (TDX_ERROR | GENMASK_ULL(49, 49))
> > +#endif
> > +
> >  #ifndef __ASSEMBLY__
> > 
> >  /*
> > diff --git a/arch/x86/virt/vmx/tdx/tdxcall.S b/arch/x86/virt/vmx/tdx/tdxcall.S
> > index 49a54356ae99..7431c47258d9 100644
> > --- a/arch/x86/virt/vmx/tdx/tdxcall.S
> > +++ b/arch/x86/virt/vmx/tdx/tdxcall.S
> > @@ -1,6 +1,7 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> >  #include <asm/asm-offsets.h>
> >  #include <asm/tdx.h>
> > +#include <asm/asm.h>
> > 
> >  /*
> >   * TDCALL and SEAMCALL are supported in Binutils >= 2.36.
> > @@ -45,6 +46,7 @@
> >         /* Leave input param 2 in RDX */
> > 
> >         .if \host
> > +1:
> >         seamcall
> >         /*
> >          * SEAMCALL instruction is essentially a VMExit from VMX root
> > @@ -57,9 +59,25 @@
> >          * This value will never be used as actual SEAMCALL error code as
> >          * it is from the Reserved status code class.
> >          */
> > -       jnc .Lno_vmfailinvalid
> > +       jnc .Lseamcall_out
> >         mov $TDX_SEAMCALL_VMFAILINVALID, %rax
> > -.Lno_vmfailinvalid:
> > +       jmp .Lseamcall_out
> > +2:
> > +       /*
> > +        * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
> > +        * the trap number.  Check the trap number and set up the return
> > +        * value to %rax.
> > +        */
> > +       cmp $X86_TRAP_GP, %eax
> > +       je .Lseamcall_gp
> > +       mov $TDX_SEAMCALL_UD, %rax
> > +       jmp .Lseamcall_out
> > +.Lseamcall_gp:
> > +       mov $TDX_SEAMCALL_GP, %rax
> > +       jmp .Lseamcall_out
> > +
> > +       _ASM_EXTABLE_FAULT(1b, 2b)
> > +.Lseamcall_out
> 
> Not too bad, although the end of that is a bit ugly.  It would be nicer
> if you could just return the %rax value in the exception section instead
> of having to do the transform there.  Maybe have a TDX_ERROR code with
> enough bits to hold any X86_TRAP_FOO.

We already have declared bits 47:40 == 0xFF is never used by TDX module:

/*
 * SW-defined error codes.
 *
 * Bits 47:40 == 0xFF indicate Reserved status code class that never used by
 * TDX module.
 */
#define TDX_ERROR                       _BITUL(63)
#define TDX_SW_ERROR                    (TDX_ERROR | GENMASK_ULL(47, 40))
#define TDX_SEAMCALL_VMFAILINVALID      (TDX_SW_ERROR | _UL(0xFFFF0000))

So how about just putting the X86_TRAP_FOO to the last 32-bits?  We only have 32
traps, so 32-bits is more than enough.

#define TDX_SEAMCALL_GP		(TDX_SW_ERROR | X86_TRAP_GP)
#define TDX_SEAMCALL_UD		(TDX_SW_ERROR | X86_TRAP_UD)

If so,  in the assembly, I think we can just XOR TDX_SW_ERROR to the %rax and
return %rax:

2:
        /*
	 * SEAMCALL caused #GP or #UD.  By reaching here %eax contains
	 * the trap number.  Convert trap number to TDX error code by setting
	 * TDX_SW_ERROR to the high 32-bits of %rax.
	 */
	xorq	$TDX_SW_ERROR, %rax

How does this look?



-- 
Thanks,
-Kai