Re: [PATCH v13 06/22] x86/virt/tdx: Add SEAMCALL error printing for module initialization

[Nikolay Borisov <nik.borisov@suse.com>]

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;