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

[Dave Hansen]

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.

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

It'd be nice if Peter Z or Andy L has a sec to look at this.  Seems like
the kind of thing they'd have good ideas about.