| Message ID | 89fffc70cdbb74c80bb324364b712ec41e5f8b91.1655894131.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host kernel support | expand |
So, the last patch was called: Implement SEAMCALL function and yet, in this patch, we have a "seamcall()" function. That's a bit confusing and not covered at *all* in this subject. Further, seamcall() is the *ONLY* caller of __seamcall() that I see in this series. That makes its presence here even more odd. The seamcall() bits should either be in their own patch, or mashed in with __seamcall(). > +/* > + * Wrapper of __seamcall(). It additionally prints out the error > + * informationi if __seamcall() fails normally. It is useful during > + * the module initialization by providing more information to the user. > + */ > +static u64 seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > + struct tdx_module_output *out) > +{ > + u64 ret; > + > + ret = __seamcall(fn, rcx, rdx, r8, r9, out); > + if (ret == TDX_SEAMCALL_VMFAILINVALID || !ret) > + return ret; > + > + pr_err("SEAMCALL failed: leaf: 0x%llx, error: 0x%llx\n", fn, ret); > + if (out) > + pr_err("SEAMCALL additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n", > + out->rcx, out->rdx, out->r8, out->r9, out->r10, out->r11); > + > + return ret; > +} > + > +static void seamcall_smp_call_function(void *data) > +{ > + struct seamcall_ctx *sc = data; > + struct tdx_module_output out; > + u64 ret; > + > + ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9, &out); > + if (ret) > + atomic_set(&sc->err, -EFAULT); > +} > + > +/* > + * Call the SEAMCALL on all online CPUs concurrently. Caller to check > + * @sc->err to determine whether any SEAMCALL failed on any cpu. > + */ > +static void seamcall_on_each_cpu(struct seamcall_ctx *sc) > +{ > + on_each_cpu(seamcall_smp_call_function, sc, true); > +} You can get away with this three-liner seamcall_on_each_cpu() being in this patch, but seamcall() itself doesn't belong here. > /* > * Detect and initialize the TDX module. > * > @@ -138,7 +195,10 @@ static int init_tdx_module(void) > > static void shutdown_tdx_module(void) > { > - /* TODO: Shut down the TDX module */ > + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN }; > + > + seamcall_on_each_cpu(&sc); > + > tdx_module_status = TDX_MODULE_SHUTDOWN; > } > > @@ -221,6 +281,9 @@ bool platform_tdx_enabled(void) > * CPU hotplug is temporarily disabled internally to prevent any cpu > * from going offline. > * > + * Caller also needs to guarantee all CPUs are in VMX operation during > + * this function, otherwise Oops may be triggered. I would *MUCH* rather have this be a: if (!cpu_feature_enabled(X86_FEATURE_VMX)) WARN_ONCE("VMX should be on blah blah\n"); than just plain oops. Even a pr_err() that preceded the oops would be nicer than an oops that someone has to go decode and then grumble when their binutils is too old that it can't disassemble the TDCALL. > * This function can be called in parallel by multiple callers. > * > * Return: > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index f1a2dfb978b1..95d4eb884134 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -46,6 +46,11 @@ > #define TDX_KEYID_NUM(_keyid_part) ((u32)((_keyid_part) >> 32)) > > > +/* > + * TDX module SEAMCALL leaf functions > + */ > +#define TDH_SYS_LP_SHUTDOWN 44 > + > /* > * Do not put any hardware-defined TDX structure representations below this > * comment!
On Fri, 2022-06-24 at 11:50 -0700, Dave Hansen wrote: > So, the last patch was called: > > Implement SEAMCALL function > > and yet, in this patch, we have a "seamcall()" function. That's a bit > confusing and not covered at *all* in this subject. > > Further, seamcall() is the *ONLY* caller of __seamcall() that I see in > this series. That makes its presence here even more odd. > > The seamcall() bits should either be in their own patch, or mashed in > with __seamcall(). Right. The reason I didn't put the seamcall() into previous patch was it is only used in this tdx.c, so it should be static. But adding a static function w/o using it in previous patch will trigger a compile warning. So I introduced here where it is first used. One option is I can introduce seamcall() as a static inline function in tdx.h in previous patch so there won't be a warning. I'll change to use this way. Please let me know if you have any comments. > > > +/* > > + * Wrapper of __seamcall(). It additionally prints out the error > > + * informationi if __seamcall() fails normally. It is useful during > > + * the module initialization by providing more information to the user. > > + */ > > +static u64 seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > > + struct tdx_module_output *out) > > +{ > > + u64 ret; > > + > > + ret = __seamcall(fn, rcx, rdx, r8, r9, out); > > + if (ret == TDX_SEAMCALL_VMFAILINVALID || !ret) > > + return ret; > > + > > + pr_err("SEAMCALL failed: leaf: 0x%llx, error: 0x%llx\n", fn, ret); > > + if (out) > > + pr_err("SEAMCALL additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n", > > + out->rcx, out->rdx, out->r8, out->r9, out->r10, out->r11); > > + > > + return ret; > > +} > > + > > +static void seamcall_smp_call_function(void *data) > > +{ > > + struct seamcall_ctx *sc = data; > > + struct tdx_module_output out; > > + u64 ret; > > + > > + ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9, &out); > > + if (ret) > > + atomic_set(&sc->err, -EFAULT); > > +} > > + > > +/* > > + * Call the SEAMCALL on all online CPUs concurrently. Caller to check > > + * @sc->err to determine whether any SEAMCALL failed on any cpu. > > + */ > > +static void seamcall_on_each_cpu(struct seamcall_ctx *sc) > > +{ > > + on_each_cpu(seamcall_smp_call_function, sc, true); > > +} > > You can get away with this three-liner seamcall_on_each_cpu() being in > this patch, but seamcall() itself doesn't belong here. Right. Please see above reply. > > > /* > > * Detect and initialize the TDX module. > > * > > @@ -138,7 +195,10 @@ static int init_tdx_module(void) > > > > static void shutdown_tdx_module(void) > > { > > - /* TODO: Shut down the TDX module */ > > + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN }; > > + > > + seamcall_on_each_cpu(&sc); > > + > > tdx_module_status = TDX_MODULE_SHUTDOWN; > > } > > > > @@ -221,6 +281,9 @@ bool platform_tdx_enabled(void) > > * CPU hotplug is temporarily disabled internally to prevent any cpu > > * from going offline. > > * > > + * Caller also needs to guarantee all CPUs are in VMX operation during > > + * this function, otherwise Oops may be triggered. > > I would *MUCH* rather have this be a: > > if (!cpu_feature_enabled(X86_FEATURE_VMX)) > WARN_ONCE("VMX should be on blah blah\n"); > > than just plain oops. Even a pr_err() that preceded the oops would be > nicer than an oops that someone has to go decode and then grumble when > their binutils is too old that it can't disassemble the TDCALL. I can add this to seamcall(): /* * SEAMCALL requires CPU being in VMX operation otherwise it causes #UD. * Sanity check and return early to avoid Oops. Note cpu_vmx_enabled() * actually only checks whether VMX is enabled but doesn't check whether * CPU is in VMX operation (VMXON is done). There's no way to check * whether VMXON has been done, but currently enabling VMX and doing * VMXON are always done together. */ if (!cpu_vmx_enabled()) { WARN_ONCE("CPU is not in VMX operation before making SEAMCALL"); return -EINVAL; } The reason I didn't do is I'd like to make seamcall() simple, that it only returns TDX_SEAMCALL_VMFAILINVALID or the actual SEAMCALL leaf error. With above, this function also returns kernel error code, which isn't good. 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?
On 6/26/22 22:26, Kai Huang wrote: > On Fri, 2022-06-24 at 11:50 -0700, Dave Hansen wrote: >> So, the last patch was called: >> >> Implement SEAMCALL function >> >> and yet, in this patch, we have a "seamcall()" function. That's a bit >> confusing and not covered at *all* in this subject. >> >> Further, seamcall() is the *ONLY* caller of __seamcall() that I see in >> this series. That makes its presence here even more odd. >> >> The seamcall() bits should either be in their own patch, or mashed in >> with __seamcall(). > > Right. The reason I didn't put the seamcall() into previous patch was it is > only used in this tdx.c, so it should be static. But adding a static function > w/o using it in previous patch will trigger a compile warning. So I introduced > here where it is first used. > > One option is I can introduce seamcall() as a static inline function in tdx.h in > previous patch so there won't be a warning. I'll change to use this way. > Please let me know if you have any comments. Does a temporary __unused get rid of the warning? >>> /* >>> * Detect and initialize the TDX module. >>> * >>> @@ -138,7 +195,10 @@ static int init_tdx_module(void) >>> >>> static void shutdown_tdx_module(void) >>> { >>> - /* TODO: Shut down the TDX module */ >>> + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN }; >>> + >>> + seamcall_on_each_cpu(&sc); >>> + >>> tdx_module_status = TDX_MODULE_SHUTDOWN; >>> } >>> >>> @@ -221,6 +281,9 @@ bool platform_tdx_enabled(void) >>> * CPU hotplug is temporarily disabled internally to prevent any cpu >>> * from going offline. >>> * >>> + * Caller also needs to guarantee all CPUs are in VMX operation during >>> + * this function, otherwise Oops may be triggered. >> >> I would *MUCH* rather have this be a: >> >> if (!cpu_feature_enabled(X86_FEATURE_VMX)) >> WARN_ONCE("VMX should be on blah blah\n"); >> >> than just plain oops. Even a pr_err() that preceded the oops would be >> nicer than an oops that someone has to go decode and then grumble when >> their binutils is too old that it can't disassemble the TDCALL. > > I can add this to seamcall(): > > /* > * SEAMCALL requires CPU being in VMX operation otherwise it causes > #UD. > * Sanity check and return early to avoid Oops. Note cpu_vmx_enabled() > * actually only checks whether VMX is enabled but doesn't check > whether > * CPU is in VMX operation (VMXON is done). There's no way to check > * whether VMXON has been done, but currently enabling VMX and doing > * VMXON are always done together. > */ > if (!cpu_vmx_enabled()) { > WARN_ONCE("CPU is not in VMX operation before making > SEAMCALL"); > return -EINVAL; > } > > The reason I didn't do is I'd like to make seamcall() simple, that it only > returns TDX_SEAMCALL_VMFAILINVALID or the actual SEAMCALL leaf error. With > above, this function also returns kernel error code, which isn't good. I think you're missing the point. You wasted two lines of code on a *COMMENT* that doesn't actually help anyone decode an oops. You could have, instead, spent two lines on actual code that would have been just as good or better than a comment *AND* help folks looking at an oops. It's almost always better to do something actionable in code than to comment it, unless it's in some crazy fast path. > 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.
On Mon, 2022-06-27 at 13:46 -0700, Dave Hansen wrote: > On 6/26/22 22:26, Kai Huang wrote: > > On Fri, 2022-06-24 at 11:50 -0700, Dave Hansen wrote: > > > So, the last patch was called: > > > > > > Implement SEAMCALL function > > > > > > and yet, in this patch, we have a "seamcall()" function. That's a bit > > > confusing and not covered at *all* in this subject. > > > > > > Further, seamcall() is the *ONLY* caller of __seamcall() that I see in > > > this series. That makes its presence here even more odd. > > > > > > The seamcall() bits should either be in their own patch, or mashed in > > > with __seamcall(). > > > > Right. The reason I didn't put the seamcall() into previous patch was it is > > only used in this tdx.c, so it should be static. But adding a static function > > w/o using it in previous patch will trigger a compile warning. So I introduced > > here where it is first used. > > > > One option is I can introduce seamcall() as a static inline function in tdx.h in > > previous patch so there won't be a warning. I'll change to use this way. > > Please let me know if you have any comments. > > Does a temporary __unused get rid of the warning? Yes, and both __maybe_unused and __always_unused also git rid of the warning too. __unused is not defined in compiler_attributes.h, so we need to use __attribute__((__unused__)) explicitly, or have __unused defined to it as a macro. 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? > > > > > /* > > > > * Detect and initialize the TDX module. > > > > * > > > > @@ -138,7 +195,10 @@ static int init_tdx_module(void) > > > > > > > > static void shutdown_tdx_module(void) > > > > { > > > > - /* TODO: Shut down the TDX module */ > > > > + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN }; > > > > + > > > > + seamcall_on_each_cpu(&sc); > > > > + > > > > tdx_module_status = TDX_MODULE_SHUTDOWN; > > > > } > > > > > > > > @@ -221,6 +281,9 @@ bool platform_tdx_enabled(void) > > > > * CPU hotplug is temporarily disabled internally to prevent any cpu > > > > * from going offline. > > > > * > > > > + * Caller also needs to guarantee all CPUs are in VMX operation during > > > > + * this function, otherwise Oops may be triggered. > > > > > > I would *MUCH* rather have this be a: > > > > > > if (!cpu_feature_enabled(X86_FEATURE_VMX)) > > > WARN_ONCE("VMX should be on blah blah\n"); > > > > > > than just plain oops. Even a pr_err() that preceded the oops would be > > > nicer than an oops that someone has to go decode and then grumble when > > > their binutils is too old that it can't disassemble the TDCALL. > > > > I can add this to seamcall(): > > > > /* > > * SEAMCALL requires CPU being in VMX operation otherwise it causes > > #UD. > > * Sanity check and return early to avoid Oops. Note cpu_vmx_enabled() > > * actually only checks whether VMX is enabled but doesn't check > > whether > > * CPU is in VMX operation (VMXON is done). There's no way to check > > * whether VMXON has been done, but currently enabling VMX and doing > > * VMXON are always done together. > > */ > > if (!cpu_vmx_enabled()) { > > WARN_ONCE("CPU is not in VMX operation before making > > SEAMCALL"); > > return -EINVAL; > > } > > > > The reason I didn't do is I'd like to make seamcall() simple, that it only > > returns TDX_SEAMCALL_VMFAILINVALID or the actual SEAMCALL leaf error. With > > above, this function also returns kernel error code, which isn't good. > > I think you're missing the point. You wasted two lines of code on a > *COMMENT* that doesn't actually help anyone decode an oops. You could > have, instead, spent two lines on actual code that would have been just > as good or better than a comment *AND* help folks looking at an oops. > > It's almost always better to do something actionable in code than to > comment it, unless it's in some crazy fast path. Agreed. Thanks. > > > 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
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.
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?
On 6/27/22 16:59, Kai Huang wrote: > 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? I guess it doesn't matter if you know the things being masked together are padded correctly, but I probably would have done a straight OR, not XOR. Otherwise, I think that looks OK. Simplifies the assembly for sure.
On Mon, 2022-06-27 at 17:03 -0700, Dave Hansen wrote: > On 6/27/22 16:59, Kai Huang wrote: > > 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? > > I guess it doesn't matter if you know the things being masked together > are padded correctly, but I probably would have done a straight OR, not XOR. > > Otherwise, I think that looks OK. Simplifies the assembly for sure. Right straight OR is better. Thanks.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 1f9d8108eeea..31ce4522100a 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -13,6 +13,8 @@ #include <linux/mutex.h> #include <linux/cpu.h> #include <linux/cpumask.h> +#include <linux/smp.h> +#include <linux/atomic.h> #include <asm/cpufeatures.h> #include <asm/cpufeature.h> #include <asm/msr-index.h> @@ -123,6 +125,61 @@ static int __init tdx_early_detect(void) } early_initcall(tdx_early_detect); +/* + * Data structure to make SEAMCALL on multiple CPUs concurrently. + * @err is set to -EFAULT when SEAMCALL fails on any cpu. + */ +struct seamcall_ctx { + u64 fn; + u64 rcx; + u64 rdx; + u64 r8; + u64 r9; + atomic_t err; +}; + +/* + * Wrapper of __seamcall(). It additionally prints out the error + * informationi if __seamcall() fails normally. It is useful during + * the module initialization by providing more information to the user. + */ +static u64 seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, + struct tdx_module_output *out) +{ + u64 ret; + + ret = __seamcall(fn, rcx, rdx, r8, r9, out); + if (ret == TDX_SEAMCALL_VMFAILINVALID || !ret) + return ret; + + pr_err("SEAMCALL failed: leaf: 0x%llx, error: 0x%llx\n", fn, ret); + if (out) + pr_err("SEAMCALL additional output: rcx 0x%llx, rdx 0x%llx, r8 0x%llx, r9 0x%llx, r10 0x%llx, r11 0x%llx.\n", + out->rcx, out->rdx, out->r8, out->r9, out->r10, out->r11); + + return ret; +} + +static void seamcall_smp_call_function(void *data) +{ + struct seamcall_ctx *sc = data; + struct tdx_module_output out; + u64 ret; + + ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9, &out); + if (ret) + atomic_set(&sc->err, -EFAULT); +} + +/* + * Call the SEAMCALL on all online CPUs concurrently. Caller to check + * @sc->err to determine whether any SEAMCALL failed on any cpu. + */ +static void seamcall_on_each_cpu(struct seamcall_ctx *sc) +{ + on_each_cpu(seamcall_smp_call_function, sc, true); +} + /* * Detect and initialize the TDX module. * @@ -138,7 +195,10 @@ static int init_tdx_module(void) static void shutdown_tdx_module(void) { - /* TODO: Shut down the TDX module */ + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN }; + + seamcall_on_each_cpu(&sc); + tdx_module_status = TDX_MODULE_SHUTDOWN; } @@ -221,6 +281,9 @@ bool platform_tdx_enabled(void) * CPU hotplug is temporarily disabled internally to prevent any cpu * from going offline. * + * Caller also needs to guarantee all CPUs are in VMX operation during + * this function, otherwise Oops may be triggered. + * * This function can be called in parallel by multiple callers. * * Return: diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h index f1a2dfb978b1..95d4eb884134 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -46,6 +46,11 @@ #define TDX_KEYID_NUM(_keyid_part) ((u32)((_keyid_part) >> 32)) +/* + * TDX module SEAMCALL leaf functions + */ +#define TDH_SYS_LP_SHUTDOWN 44 + /* * Do not put any hardware-defined TDX structure representations below this * comment!
TDX supports shutting down the TDX module at any time during its lifetime. After the module is shut down, no further TDX module SEAMCALL leaf functions can be made to the module on any logical cpu. Shut down the TDX module in case of any error during the initialization process. It's pointless to leave the TDX module in some middle state. Shutting down the TDX module requires calling TDH.SYS.LP.SHUTDOWN on all BIOS-enabled CPUs, and the SEMACALL can run concurrently on different CPUs. Implement a mechanism to run SEAMCALL concurrently on all online CPUs and use it to shut down the module. Later logical-cpu scope module initialization will use it too. Also add a wrapper of __seamcall() which additionally prints out the error information if SEAMCALL fails. It will be useful during the TDX module initialization as it provides more error information to the user. SEAMCALL instruction causes #UD if CPU is not in VMX operation (VMXON has been done). So far only KVM supports VMXON. It guarantees all online CPUs are in VMX operation when there's any VM still exists. As so far KVM is also the only user of TDX, choose to just let the caller to guarantee all CPUs are in VMX operation during tdx_init(). Adding the support of VMXON/VMXOFF to the core kernel isn't trivial. In the long term, more kernel components will likely need to use TDX so a reference-based approach to do VMXON/VMXOFF will likely be needed. Signed-off-by: Kai Huang <kai.huang@intel.com> --- - v3 -> v5 (no feedback on v4): - Added a wrapper of __seamcall() to print error code if SEAMCALL fails. - Made the seamcall_on_each_cpu() void. - Removed 'seamcall_ret' and 'tdx_module_out' from 'struct seamcall_ctx', as they must be local variable. - Added the comments to tdx_init() and one paragraph to changelog to explain the caller should handle VMXON. - Called out after shut down, no "TDX module" SEAMCALL can be made. --- arch/x86/virt/vmx/tdx/tdx.c | 65 ++++++++++++++++++++++++++++++++++++- arch/x86/virt/vmx/tdx/tdx.h | 5 +++ 2 files changed, 69 insertions(+), 1 deletion(-)