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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux