lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <33169a3f272b631b497cd8fbf710236cdc3681d8.camel@intel.com>
Date:   Fri, 8 Sep 2023 11:00:21 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Raj, Ashok" <ashok.raj@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "david@...hat.com" <david@...hat.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Luck, Tony" <tony.luck@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v13 06/22] x86/virt/tdx: Add SEAMCALL error printing for
 module initialization

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."

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ