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

[Nikolay Borisov]

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