| Message ID | 3b9ddfb377a944393b2a93f963cd902232a5ee33.1692962263.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host kernel support | expand |
On 25.08.23 г. 15:14 ч., Kai Huang wrote: > TDX module initialization is essentially to make a set of SEAMCALL leafs > to complete a state machine involving multiple states. These SEAMCALLs > are not expected to fail. In fact, they are not expected to return any > non-zero code (except the "running out of entropy error", which can be > handled internally already). > > Add yet another layer of SEAMCALL wrappers, which treats all non-zero > return code as error, to support printing SEAMCALL error upon failure > for module initialization. > > Other SEAMCALLs may treat some specific error codes as legal (e.g., some > can return BUSY legally and expect the caller to retry). The caller can > use the wrappers w/o error printing for those cases. The new wrappers > can also be improved to suit those cases. Leave this as future work. > > SEAMCALL can also return kernel defined error codes for three special > cases: 1) TDX isn't enabled by the BIOS; 2) TDX module isn't loaded; 3) > CPU isn't in VMX operation. The first case isn't expected (unless BIOS > bug, etc) because SEAMCALL is only expected to be made when the kernel > detects TDX is enabled. The second case is only expected to be legal > for the very first SEAMCALL during module initialization. The third > case can be legal for any SEAMCALL leaf because VMX can be disabled due > to emergency reboot. > > Also add wrappers to convert the SEAMCALL error code to the kernel error > code so that each caller doesn't have to repeat. Blindly print error > for the above special cases to save the effort to optimize them. > > TDX module can only be initialized once during its life cycle, but the > module can be runtime updated by the kernel (not yet supported). After > module runtime update, the kernel needs to initialize it again. Use > pr_err() to print SEAMCALL error for module initialization, because if > using pr_err_once() the SEAMCALL error during module initialization > won't be printed after module runtime update. > > At last, for now implement those wrappers in tdx.c but they can be moved > to <asm/tdx.h> when needed. They are implemented with intention to be > shared by other kernel components. After all, in most cases, SEAMCALL > failure is unexpected and the caller just wants to print. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > > v12 -> v13: > - New implementation due to TDCALL assembly series. > > --- > arch/x86/include/asm/tdx.h | 1 + > arch/x86/virt/vmx/tdx/tdx.c | 84 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 85 insertions(+) > > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h > index cfae8b31a2e9..3b248c94a4a4 100644 > --- a/arch/x86/include/asm/tdx.h > +++ b/arch/x86/include/asm/tdx.h > @@ -27,6 +27,7 @@ > /* > * TDX module SEAMCALL leaf function error codes > */ > +#define TDX_SUCCESS 0ULL > #define TDX_RND_NO_ENTROPY 0x8000020300000000ULL > > #ifndef __ASSEMBLY__ > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 908590e85749..bb63cb7361c8 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -16,6 +16,90 @@ > #include <asm/msr.h> > #include <asm/tdx.h> > > +#define seamcall_err(__fn, __err, __args, __prerr_func) \ > + __prerr_func("SEAMCALL (0x%llx) failed: 0x%llx\n", \ > + ((u64)__fn), ((u64)__err)) > + > +#define SEAMCALL_REGS_FMT \ > + "RCX 0x%llx RDX 0x%llx R8 0x%llx R9 0x%llx R10 0x%llx R11 0x%llx\n" > + > +#define seamcall_err_ret(__fn, __err, __args, __prerr_func) \ > +({ \ > + seamcall_err((__fn), (__err), (__args), __prerr_func); \ > + __prerr_func(SEAMCALL_REGS_FMT, \ > + (__args)->rcx, (__args)->rdx, (__args)->r8, \ > + (__args)->r9, (__args)->r10, (__args)->r11); \ > +}) > + > +#define SEAMCALL_EXTRA_REGS_FMT \ > + "RBX 0x%llx RDI 0x%llx RSI 0x%llx R12 0x%llx R13 0x%llx R14 0x%llx R15 0x%llx" > + > +#define seamcall_err_saved_ret(__fn, __err, __args, __prerr_func) \ > +({ \ > + seamcall_err_ret(__fn, __err, __args, __prerr_func); \ > + __prerr_func(SEAMCALL_EXTRA_REGS_FMT, \ > + (__args)->rbx, (__args)->rdi, (__args)->rsi, \ > + (__args)->r12, (__args)->r13, (__args)->r14, \ > + (__args)->r15); \ > +}) > + > +static __always_inline bool seamcall_err_is_kernel_defined(u64 err) > +{ > + /* All kernel defined SEAMCALL error code have TDX_SW_ERROR set */ > + return (err & TDX_SW_ERROR) == TDX_SW_ERROR; > +} > + > +#define __SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func, \ > + __prerr_func) \ > +({ \ > + u64 ___sret = __seamcall_func((__fn), (__args)); \ > + \ > + /* Kernel defined error code has special meaning, leave to caller */ \ > + if (!seamcall_err_is_kernel_defined((___sret)) && \ > + ___sret != TDX_SUCCESS) \ > + __seamcall_err_func((__fn), (___sret), (__args), __prerr_func); \ > + \ > + ___sret; \ > +}) > + > +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ > +({ \ > + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ > + __seamcall_err_func, pr_err); __SEAMCALL_PRERR seems to only ever be called with pr_err for as the error function, can you just kill off that argument and always call pr_err. \ > + int ___ret; \ > + \ > + switch (___sret) { \ > + case TDX_SUCCESS: \ > + ___ret = 0; \ > + break; \ > + case TDX_SEAMCALL_VMFAILINVALID: \ > + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ > + ___ret = -ENODEV; \ > + break; \ > + case TDX_SEAMCALL_GP: \ > + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ > + ___ret = -EOPNOTSUPP; \ > + break; \ > + case TDX_SEAMCALL_UD: \ > + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ > + ___ret = -EACCES; \ > + break; \ > + default: \ > + ___ret = -EIO; \ > + } \ > + ___ret; \ > +}) > + > +#define seamcall_prerr(__fn, __args) \ > + SEAMCALL_PRERR(seamcall, (__fn), (__args), seamcall_err) > + > +#define seamcall_prerr_ret(__fn, __args) \ > + SEAMCALL_PRERR(seamcall_ret, (__fn), (__args), seamcall_err_ret) > + > +#define seamcall_prerr_saved_ret(__fn, __args) \ > + SEAMCALL_PRERR(seamcall_saved_ret, (__fn), (__args), \ > + seamcall_err_saved_ret) The level of indirection which you add with those seamcal_err* function is just mind boggling: SEAMCALL_PRERR -> __SEAMCALL_PRERR -> __seamcall_err_func -> __prerr_func and all of this so you can have a standardized string printing. I see no value in having __SEAMCALL_PRERR as a separate macro, simply inline it into SEAMCALL_PRERR, replace the prerr_func argument with a direct call to pr_err. > + > static u32 tdx_global_keyid __ro_after_init; > static u32 tdx_guest_keyid_start __ro_after_init; > static u32 tdx_nr_guest_keyids __ro_after_init;
> > +#define seamcall_err(__fn, __err, __args, __prerr_func) \ > > + __prerr_func("SEAMCALL (0x%llx) failed: 0x%llx\n", \ > > + ((u64)__fn), ((u64)__err)) > > + > > +#define SEAMCALL_REGS_FMT \ > > + "RCX 0x%llx RDX 0x%llx R8 0x%llx R9 0x%llx R10 0x%llx R11 0x%llx\n" > > + > > +#define seamcall_err_ret(__fn, __err, __args, __prerr_func) \ > > +({ \ > > + seamcall_err((__fn), (__err), (__args), __prerr_func); \ > > + __prerr_func(SEAMCALL_REGS_FMT, \ > > + (__args)->rcx, (__args)->rdx, (__args)->r8, \ > > + (__args)->r9, (__args)->r10, (__args)->r11); \ > > +}) > > + > > +#define SEAMCALL_EXTRA_REGS_FMT \ > > + "RBX 0x%llx RDI 0x%llx RSI 0x%llx R12 0x%llx R13 0x%llx R14 0x%llx R15 0x%llx" > > + > > +#define seamcall_err_saved_ret(__fn, __err, __args, __prerr_func) \ > > +({ \ > > + seamcall_err_ret(__fn, __err, __args, __prerr_func); \ > > + __prerr_func(SEAMCALL_EXTRA_REGS_FMT, \ > > + (__args)->rbx, (__args)->rdi, (__args)->rsi, \ > > + (__args)->r12, (__args)->r13, (__args)->r14, \ > > + (__args)->r15); \ > > +}) > > + > > +static __always_inline bool seamcall_err_is_kernel_defined(u64 err) > > +{ > > + /* All kernel defined SEAMCALL error code have TDX_SW_ERROR set */ > > + return (err & TDX_SW_ERROR) == TDX_SW_ERROR; > > +} > > + > > +#define __SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func, \ > > + __prerr_func) \ > > +({ \ > > + u64 ___sret = __seamcall_func((__fn), (__args)); \ > > + \ > > + /* Kernel defined error code has special meaning, leave to caller */ \ > > + if (!seamcall_err_is_kernel_defined((___sret)) && \ > > + ___sret != TDX_SUCCESS) \ > > + __seamcall_err_func((__fn), (___sret), (__args), __prerr_func); \ > > + \ > > + ___sret; \ > > +}) > > + > > +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ > > +({ \ > > + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ > > + __seamcall_err_func, pr_err); > > __SEAMCALL_PRERR seems to only ever be called with pr_err for as the > error function, can you just kill off that argument and always call pr_err. Please see below. > \ > > + int ___ret; \ > > + \ > > + switch (___sret) { \ > > + case TDX_SUCCESS: \ > > + ___ret = 0; \ > > + break; \ > > + case TDX_SEAMCALL_VMFAILINVALID: \ > > + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ > > + ___ret = -ENODEV; \ > > + break; \ > > + case TDX_SEAMCALL_GP: \ > > + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ > > + ___ret = -EOPNOTSUPP; \ > > + break; \ > > + case TDX_SEAMCALL_UD: \ > > + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ > > + ___ret = -EACCES; \ > > + break; \ > > + default: \ > > + ___ret = -EIO; \ > > + } \ > > + ___ret; \ > > +}) > > + > > +#define seamcall_prerr(__fn, __args) \ > > + SEAMCALL_PRERR(seamcall, (__fn), (__args), seamcall_err) > > + > > +#define seamcall_prerr_ret(__fn, __args) \ > > + SEAMCALL_PRERR(seamcall_ret, (__fn), (__args), seamcall_err_ret) > > + > > +#define seamcall_prerr_saved_ret(__fn, __args) \ > > + SEAMCALL_PRERR(seamcall_saved_ret, (__fn), (__args), \ > > + seamcall_err_saved_ret) > > > The level of indirection which you add with those seamcal_err* function > is just mind boggling: > > > SEAMCALL_PRERR -> __SEAMCALL_PRERR -> __seamcall_err_func -> > __prerr_func and all of this so you can have a standardized string > printing. I see no value in having __SEAMCALL_PRERR as a separate macro, > simply inline it into SEAMCALL_PRERR, replace the prerr_func argument > with a direct call to pr_err. Thanks for comments! I was hoping __SEAMCALL_PRERR() can be used by KVM code but I guess I was over- thinking. I can remove __SEAMCALL_PRERR() unless Isaku thinks it is useful to KVM. However maybe it's better to keep __prerr_func in seamcall_err*() as KVM TDX patches use pr_err_ratelimited(). I am hoping KVM can use those to avoid duplication at some level. Also, IMHO having __prerr_func in seamcall_err*() would make SEAMCALL_PRERR() more understandable because we can immediately see pr_err() is used by just looking at SEAMCALL_PRERR(). Anyway I am eager to hear comments from others too. :-)
On 8.09.23 г. 13:33 ч., Huang, Kai wrote: > >>> +#define seamcall_err(__fn, __err, __args, __prerr_func) \ >>> + __prerr_func("SEAMCALL (0x%llx) failed: 0x%llx\n", \ >>> + ((u64)__fn), ((u64)__err)) >>> + >>> +#define SEAMCALL_REGS_FMT \ >>> + "RCX 0x%llx RDX 0x%llx R8 0x%llx R9 0x%llx R10 0x%llx R11 0x%llx\n" >>> + >>> +#define seamcall_err_ret(__fn, __err, __args, __prerr_func) \ >>> +({ \ >>> + seamcall_err((__fn), (__err), (__args), __prerr_func); \ >>> + __prerr_func(SEAMCALL_REGS_FMT, \ >>> + (__args)->rcx, (__args)->rdx, (__args)->r8, \ >>> + (__args)->r9, (__args)->r10, (__args)->r11); \ >>> +}) >>> + >>> +#define SEAMCALL_EXTRA_REGS_FMT \ >>> + "RBX 0x%llx RDI 0x%llx RSI 0x%llx R12 0x%llx R13 0x%llx R14 0x%llx R15 0x%llx" >>> + >>> +#define seamcall_err_saved_ret(__fn, __err, __args, __prerr_func) \ >>> +({ \ >>> + seamcall_err_ret(__fn, __err, __args, __prerr_func); \ >>> + __prerr_func(SEAMCALL_EXTRA_REGS_FMT, \ >>> + (__args)->rbx, (__args)->rdi, (__args)->rsi, \ >>> + (__args)->r12, (__args)->r13, (__args)->r14, \ >>> + (__args)->r15); \ >>> +}) >>> + >>> +static __always_inline bool seamcall_err_is_kernel_defined(u64 err) >>> +{ >>> + /* All kernel defined SEAMCALL error code have TDX_SW_ERROR set */ >>> + return (err & TDX_SW_ERROR) == TDX_SW_ERROR; >>> +} >>> + >>> +#define __SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func, \ >>> + __prerr_func) \ >>> +({ \ >>> + u64 ___sret = __seamcall_func((__fn), (__args)); \ >>> + \ >>> + /* Kernel defined error code has special meaning, leave to caller */ \ >>> + if (!seamcall_err_is_kernel_defined((___sret)) && \ >>> + ___sret != TDX_SUCCESS) \ >>> + __seamcall_err_func((__fn), (___sret), (__args), __prerr_func); \ >>> + \ >>> + ___sret; \ >>> +}) >>> + >>> +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ >>> +({ \ >>> + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ >>> + __seamcall_err_func, pr_err); >> >> __SEAMCALL_PRERR seems to only ever be called with pr_err for as the >> error function, can you just kill off that argument and always call pr_err. > > Please see below. > >> \ >>> + int ___ret; \ >>> + \ >>> + switch (___sret) { \ >>> + case TDX_SUCCESS: \ >>> + ___ret = 0; \ >>> + break; \ >>> + case TDX_SEAMCALL_VMFAILINVALID: \ >>> + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ >>> + ___ret = -ENODEV; \ >>> + break; \ >>> + case TDX_SEAMCALL_GP: \ >>> + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ >>> + ___ret = -EOPNOTSUPP; \ >>> + break; \ >>> + case TDX_SEAMCALL_UD: \ >>> + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ >>> + ___ret = -EACCES; \ >>> + break; \ >>> + default: \ >>> + ___ret = -EIO; \ >>> + } \ >>> + ___ret; \ >>> +}) >>> + >>> +#define seamcall_prerr(__fn, __args) \ >>> + SEAMCALL_PRERR(seamcall, (__fn), (__args), seamcall_err) >>> + >>> +#define seamcall_prerr_ret(__fn, __args) \ >>> + SEAMCALL_PRERR(seamcall_ret, (__fn), (__args), seamcall_err_ret) >>> + >>> +#define seamcall_prerr_saved_ret(__fn, __args) \ >>> + SEAMCALL_PRERR(seamcall_saved_ret, (__fn), (__args), \ >>> + seamcall_err_saved_ret) >> >> >> The level of indirection which you add with those seamcal_err* function >> is just mind boggling: >> >> >> SEAMCALL_PRERR -> __SEAMCALL_PRERR -> __seamcall_err_func -> >> __prerr_func and all of this so you can have a standardized string >> printing. I see no value in having __SEAMCALL_PRERR as a separate macro, >> simply inline it into SEAMCALL_PRERR, replace the prerr_func argument >> with a direct call to pr_err. > > Thanks for comments! > > I was hoping __SEAMCALL_PRERR() can be used by KVM code but I guess I was over- > thinking. I can remove __SEAMCALL_PRERR() unless Isaku thinks it is useful to > KVM. Be that as it may, I think it warrants at least some mentioning in the changelog. Alternatively in the first iteration of those patches this can be omitted and then it can be introduced at the time the first users shows up. In any case, let's wait for the KVM people to comment. > > However maybe it's better to keep __prerr_func in seamcall_err*() as KVM TDX > patches use pr_err_ratelimited(). I am hoping KVM can use those to avoid > duplication at some level. Also, IMHO having __prerr_func in seamcall_err*() > would make SEAMCALL_PRERR() more understandable because we can immediately see > pr_err() is used by just looking at SEAMCALL_PRERR(). > > Anyway I am eager to hear comments from others too. :-) >
On Fri, 2023-09-08 at 13:38 +0300, Nikolay Borisov wrote: > > On 8.09.23 г. 13:33 ч., Huang, Kai wrote: > > > > > > +#define seamcall_err(__fn, __err, __args, __prerr_func) \ > > > > + __prerr_func("SEAMCALL (0x%llx) failed: 0x%llx\n", \ > > > > + ((u64)__fn), ((u64)__err)) > > > > + > > > > +#define SEAMCALL_REGS_FMT \ > > > > + "RCX 0x%llx RDX 0x%llx R8 0x%llx R9 0x%llx R10 0x%llx R11 0x%llx\n" > > > > + > > > > +#define seamcall_err_ret(__fn, __err, __args, __prerr_func) \ > > > > +({ \ > > > > + seamcall_err((__fn), (__err), (__args), __prerr_func); \ > > > > + __prerr_func(SEAMCALL_REGS_FMT, \ > > > > + (__args)->rcx, (__args)->rdx, (__args)->r8, \ > > > > + (__args)->r9, (__args)->r10, (__args)->r11); \ > > > > +}) > > > > + > > > > +#define SEAMCALL_EXTRA_REGS_FMT \ > > > > + "RBX 0x%llx RDI 0x%llx RSI 0x%llx R12 0x%llx R13 0x%llx R14 0x%llx R15 0x%llx" > > > > + > > > > +#define seamcall_err_saved_ret(__fn, __err, __args, __prerr_func) \ > > > > +({ \ > > > > + seamcall_err_ret(__fn, __err, __args, __prerr_func); \ > > > > + __prerr_func(SEAMCALL_EXTRA_REGS_FMT, \ > > > > + (__args)->rbx, (__args)->rdi, (__args)->rsi, \ > > > > + (__args)->r12, (__args)->r13, (__args)->r14, \ > > > > + (__args)->r15); \ > > > > +}) > > > > + > > > > +static __always_inline bool seamcall_err_is_kernel_defined(u64 err) > > > > +{ > > > > + /* All kernel defined SEAMCALL error code have TDX_SW_ERROR set */ > > > > + return (err & TDX_SW_ERROR) == TDX_SW_ERROR; > > > > +} > > > > + > > > > +#define __SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func, \ > > > > + __prerr_func) \ > > > > +({ \ > > > > + u64 ___sret = __seamcall_func((__fn), (__args)); \ > > > > + \ > > > > + /* Kernel defined error code has special meaning, leave to caller */ \ > > > > + if (!seamcall_err_is_kernel_defined((___sret)) && \ > > > > + ___sret != TDX_SUCCESS) \ > > > > + __seamcall_err_func((__fn), (___sret), (__args), __prerr_func); \ > > > > + \ > > > > + ___sret; \ > > > > +}) > > > > + > > > > +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ > > > > +({ \ > > > > + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ > > > > + __seamcall_err_func, pr_err); > > > > > > __SEAMCALL_PRERR seems to only ever be called with pr_err for as the > > > error function, can you just kill off that argument and always call pr_err. > > > > Please see below. > > > > > \ > > > > + int ___ret; \ > > > > + \ > > > > + switch (___sret) { \ > > > > + case TDX_SUCCESS: \ > > > > + ___ret = 0; \ > > > > + break; \ > > > > + case TDX_SEAMCALL_VMFAILINVALID: \ > > > > + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ > > > > + ___ret = -ENODEV; \ > > > > + break; \ > > > > + case TDX_SEAMCALL_GP: \ > > > > + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ > > > > + ___ret = -EOPNOTSUPP; \ > > > > + break; \ > > > > + case TDX_SEAMCALL_UD: \ > > > > + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ > > > > + ___ret = -EACCES; \ > > > > + break; \ > > > > + default: \ > > > > + ___ret = -EIO; \ > > > > + } \ > > > > + ___ret; \ > > > > +}) > > > > + > > > > +#define seamcall_prerr(__fn, __args) \ > > > > + SEAMCALL_PRERR(seamcall, (__fn), (__args), seamcall_err) > > > > + > > > > +#define seamcall_prerr_ret(__fn, __args) \ > > > > + SEAMCALL_PRERR(seamcall_ret, (__fn), (__args), seamcall_err_ret) > > > > + > > > > +#define seamcall_prerr_saved_ret(__fn, __args) \ > > > > + SEAMCALL_PRERR(seamcall_saved_ret, (__fn), (__args), \ > > > > + seamcall_err_saved_ret) > > > > > > > > > The level of indirection which you add with those seamcal_err* function > > > is just mind boggling: > > > > > > > > > SEAMCALL_PRERR -> __SEAMCALL_PRERR -> __seamcall_err_func -> > > > __prerr_func and all of this so you can have a standardized string > > > printing. I see no value in having __SEAMCALL_PRERR as a separate macro, > > > simply inline it into SEAMCALL_PRERR, replace the prerr_func argument > > > with a direct call to pr_err. > > > > Thanks for comments! > > > > I was hoping __SEAMCALL_PRERR() can be used by KVM code but I guess I was over- > > thinking. I can remove __SEAMCALL_PRERR() unless Isaku thinks it is useful to > > KVM. > > Be that as it may, I think it warrants at least some mentioning in the > changelog. Alternatively in the first iteration of those patches this > can be omitted and then it can be introduced at the time the first users > shows up. In any case, let's wait for the KVM people to comment. Yeah agreed. I had below in the changelog but perhaps it's too vague: "At last, for now implement those wrappers in tdx.c but they can be moved to <asm/tdx.h> when needed. They are implemented with intention to be shared by other kernel components."
On 8/25/23 05:14, Kai Huang wrote: > +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ > +({ \ > + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ > + __seamcall_err_func, pr_err); \ > + int ___ret; \ > + \ > + switch (___sret) { \ > + case TDX_SUCCESS: \ > + ___ret = 0; \ > + break; \ > + case TDX_SEAMCALL_VMFAILINVALID: \ > + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ > + ___ret = -ENODEV; \ > + break; \ > + case TDX_SEAMCALL_GP: \ > + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ > + ___ret = -EOPNOTSUPP; \ > + break; \ > + case TDX_SEAMCALL_UD: \ > + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ > + ___ret = -EACCES; \ > + break; \ > + default: \ > + ___ret = -EIO; \ > + } \ > + ___ret; \ > +}) I have no clue where all of this came from or why it is necessary or why it has to be macros. I'm just utterly confused. I was really hoping to be able to run through this set and get it ready to be merged. But it seems to still be seeing a *LOT* of change. Should I wait another few weeks for this to settle down again?
On Fri, 2023-09-08 at 09:31 -0700, Dave Hansen wrote: > On 8/25/23 05:14, Kai Huang wrote: > > +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ > > +({ \ > > + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ > > + __seamcall_err_func, pr_err); \ > > + int ___ret; \ > > + \ > > + switch (___sret) { \ > > + case TDX_SUCCESS: \ > > + ___ret = 0; \ > > + break; \ > > + case TDX_SEAMCALL_VMFAILINVALID: \ > > + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ > > + ___ret = -ENODEV; \ > > + break; \ > > + case TDX_SEAMCALL_GP: \ > > + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ > > + ___ret = -EOPNOTSUPP; \ > > + break; \ > > + case TDX_SEAMCALL_UD: \ > > + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ > > + ___ret = -EACCES; \ > > + break; \ > > + default: \ > > + ___ret = -EIO; \ > > + } \ > > + ___ret; \ > > +}) > > I have no clue where all of this came from or why it is necessary or why > it has to be macros. I'm just utterly confused. > > I was really hoping to be able to run through this set and get it ready > to be merged. But it seems to still be seeing a *LOT* of change. > Should I wait another few weeks for this to settle down again? Those changes are due to SEAMCALL API change from the TDCALL/VMCALL/SEAMCALL assembly change patchset. I'll work internally to make this stable asap (I tried before but was suggested to sent out to community for feedback). Also I would appreciate if you could take a look at patch 18/19 (which are separated small patches for better review) and patch 20 (reset PAMT in kexec). Thanks in advance!
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h index cfae8b31a2e9..3b248c94a4a4 100644 --- a/arch/x86/include/asm/tdx.h +++ b/arch/x86/include/asm/tdx.h @@ -27,6 +27,7 @@ /* * TDX module SEAMCALL leaf function error codes */ +#define TDX_SUCCESS 0ULL #define TDX_RND_NO_ENTROPY 0x8000020300000000ULL #ifndef __ASSEMBLY__ diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 908590e85749..bb63cb7361c8 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -16,6 +16,90 @@ #include <asm/msr.h> #include <asm/tdx.h> +#define seamcall_err(__fn, __err, __args, __prerr_func) \ + __prerr_func("SEAMCALL (0x%llx) failed: 0x%llx\n", \ + ((u64)__fn), ((u64)__err)) + +#define SEAMCALL_REGS_FMT \ + "RCX 0x%llx RDX 0x%llx R8 0x%llx R9 0x%llx R10 0x%llx R11 0x%llx\n" + +#define seamcall_err_ret(__fn, __err, __args, __prerr_func) \ +({ \ + seamcall_err((__fn), (__err), (__args), __prerr_func); \ + __prerr_func(SEAMCALL_REGS_FMT, \ + (__args)->rcx, (__args)->rdx, (__args)->r8, \ + (__args)->r9, (__args)->r10, (__args)->r11); \ +}) + +#define SEAMCALL_EXTRA_REGS_FMT \ + "RBX 0x%llx RDI 0x%llx RSI 0x%llx R12 0x%llx R13 0x%llx R14 0x%llx R15 0x%llx" + +#define seamcall_err_saved_ret(__fn, __err, __args, __prerr_func) \ +({ \ + seamcall_err_ret(__fn, __err, __args, __prerr_func); \ + __prerr_func(SEAMCALL_EXTRA_REGS_FMT, \ + (__args)->rbx, (__args)->rdi, (__args)->rsi, \ + (__args)->r12, (__args)->r13, (__args)->r14, \ + (__args)->r15); \ +}) + +static __always_inline bool seamcall_err_is_kernel_defined(u64 err) +{ + /* All kernel defined SEAMCALL error code have TDX_SW_ERROR set */ + return (err & TDX_SW_ERROR) == TDX_SW_ERROR; +} + +#define __SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func, \ + __prerr_func) \ +({ \ + u64 ___sret = __seamcall_func((__fn), (__args)); \ + \ + /* Kernel defined error code has special meaning, leave to caller */ \ + if (!seamcall_err_is_kernel_defined((___sret)) && \ + ___sret != TDX_SUCCESS) \ + __seamcall_err_func((__fn), (___sret), (__args), __prerr_func); \ + \ + ___sret; \ +}) + +#define SEAMCALL_PRERR(__seamcall_func, __fn, __args, __seamcall_err_func) \ +({ \ + u64 ___sret = __SEAMCALL_PRERR(__seamcall_func, __fn, __args, \ + __seamcall_err_func, pr_err); \ + int ___ret; \ + \ + switch (___sret) { \ + case TDX_SUCCESS: \ + ___ret = 0; \ + break; \ + case TDX_SEAMCALL_VMFAILINVALID: \ + pr_err("SEAMCALL failed: TDX module not loaded.\n"); \ + ___ret = -ENODEV; \ + break; \ + case TDX_SEAMCALL_GP: \ + pr_err("SEAMCALL failed: TDX disabled by BIOS.\n"); \ + ___ret = -EOPNOTSUPP; \ + break; \ + case TDX_SEAMCALL_UD: \ + pr_err("SEAMCALL failed: CPU not in VMX operation.\n"); \ + ___ret = -EACCES; \ + break; \ + default: \ + ___ret = -EIO; \ + } \ + ___ret; \ +}) + +#define seamcall_prerr(__fn, __args) \ + SEAMCALL_PRERR(seamcall, (__fn), (__args), seamcall_err) + +#define seamcall_prerr_ret(__fn, __args) \ + SEAMCALL_PRERR(seamcall_ret, (__fn), (__args), seamcall_err_ret) + +#define seamcall_prerr_saved_ret(__fn, __args) \ + SEAMCALL_PRERR(seamcall_saved_ret, (__fn), (__args), \ + seamcall_err_saved_ret) + static u32 tdx_global_keyid __ro_after_init; static u32 tdx_guest_keyid_start __ro_after_init; static u32 tdx_nr_guest_keyids __ro_after_init;
TDX module initialization is essentially to make a set of SEAMCALL leafs to complete a state machine involving multiple states. These SEAMCALLs are not expected to fail. In fact, they are not expected to return any non-zero code (except the "running out of entropy error", which can be handled internally already). Add yet another layer of SEAMCALL wrappers, which treats all non-zero return code as error, to support printing SEAMCALL error upon failure for module initialization. Other SEAMCALLs may treat some specific error codes as legal (e.g., some can return BUSY legally and expect the caller to retry). The caller can use the wrappers w/o error printing for those cases. The new wrappers can also be improved to suit those cases. Leave this as future work. SEAMCALL can also return kernel defined error codes for three special cases: 1) TDX isn't enabled by the BIOS; 2) TDX module isn't loaded; 3) CPU isn't in VMX operation. The first case isn't expected (unless BIOS bug, etc) because SEAMCALL is only expected to be made when the kernel detects TDX is enabled. The second case is only expected to be legal for the very first SEAMCALL during module initialization. The third case can be legal for any SEAMCALL leaf because VMX can be disabled due to emergency reboot. Also add wrappers to convert the SEAMCALL error code to the kernel error code so that each caller doesn't have to repeat. Blindly print error for the above special cases to save the effort to optimize them. TDX module can only be initialized once during its life cycle, but the module can be runtime updated by the kernel (not yet supported). After module runtime update, the kernel needs to initialize it again. Use pr_err() to print SEAMCALL error for module initialization, because if using pr_err_once() the SEAMCALL error during module initialization won't be printed after module runtime update. At last, for now implement those wrappers in tdx.c but they can be moved to <asm/tdx.h> when needed. They are implemented with intention to be shared by other kernel components. After all, in most cases, SEAMCALL failure is unexpected and the caller just wants to print. Signed-off-by: Kai Huang <kai.huang@intel.com> --- v12 -> v13: - New implementation due to TDCALL assembly series. --- arch/x86/include/asm/tdx.h | 1 + arch/x86/virt/vmx/tdx/tdx.c | 84 +++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+)