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.
