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

[Huang, Kai]

> > +#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. :-)