linux-kernel - Re: [PATCH v3 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

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: <fd965db365b08bd88179318bef30c5de15542699.camel@intel.com>
Date: Fri, 30 Aug 2024 11:09:31 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "bp@...en8.de" <bp@...en8.de>, "peterz@...radead.org"
	<peterz@...radead.org>, "hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "Williams, Dan J"
	<dan.j.williams@...el.com>, "nik.borisov@...e.com" <nik.borisov@...e.com>
CC: "Gao, Chao" <chao.gao@...el.com>, "Edgecombe, Rick P"
	<rick.p.edgecombe@...el.com>, "x86@...nel.org" <x86@...nel.org>,
	"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Hunter,
 Adrian" <adrian.hunter@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH v3 3/8] x86/virt/tdx: Prepare to support reading other
 global metadata fields


> > -static int read_sys_metadata_field16(u64 field_id, u16 *val)
> > +static int __read_sys_metadata_field(u64 field_id, void *val, int size)
> 
> The type of 'size' should be size_t.

OK will do.

> 
> >   {
> >   	u64 tmp;
> >   	int ret;
> >   
> > -	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > -			MD_FIELD_ID_ELE_SIZE_16BIT);
> > -
> > -	ret = read_sys_metadata_field(field_id, &tmp);
> > +	ret = tdh_sys_rd(field_id, &tmp);
> >   	if (ret)
> >   		return ret;
> >   
> > -	*val = tmp;
> > +	memcpy(val, &tmp, size);
> >   
> >   	return 0;
> >   }
> >   
> > +/* Wrapper to read one global metadata to u8/u16/u32/u64 */
> > +#define read_sys_metadata_field(_field_id, _val)					\
> > +	({										\
> > +		BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val))));	\
> > +		__read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val))));	\
> > +	})
> > +
> >   /*
> > - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error
> > - * code and the 'struct tdx_sys_info_tdmr' instance to fill out.
> > + * Read one global metadata field to a member of a structure instance,
> > + * assuming locally defined @ret to convey the error code.
> >    */
> > -#define TD_SYSINFO_MAP(_field_id, _member)						\
> > -	({										\
> > -		if (!ret)								\
> > -			ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> > -					&sysinfo_tdmr->_member);			\
> > +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member)				\
> > +	({									\
> > +		if (!ret)							\
> > +			ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id,	\
> > +					&_stbuf->_member);			\
> >   	})
> >   
> >   static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> >   {
> >   	int ret = 0;
> >   
> > -	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs);
> > -	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> > -	TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> > -	TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> > -	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> > +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
> > +	TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member)
> 
> nit: I guess its a personal preference but honestly I think the amount 
> of macro indirection (3 levels) here is crazy, despite each being rather 
> simple. Just use TD_SYSINFO_MAP directly, saving the typing of 
> "sysinfo_tdmr" doesn't seem like a big deal.

We have a different interpretation of "crazy" :-)

> 
> You can probably take it even a bit further and simply opencode 
> read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with 
> just it, no ? No other patch in this series uses read_sys_metadata_field 
> stand alone, if anything factoring it out could be deferred until the 
> first users gets introduced.

Joke aside, anyway, with what Adrian suggested we just need the
TD_SYSINFO_MAP() but don't need those wrappers anymore.  I'll go with this if
no one says differently.

> 
> > +
> > +	TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS,	        max_tdmrs);
> > +	TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> > +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> > +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> > +	TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> >   
> >   	return ret;
> >   }
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> > index 148f9b4d1140..7458f6717873 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.h
> > +++ b/arch/x86/virt/vmx/tdx/tdx.h
> > @@ -53,7 +53,8 @@
> >   #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id)	\
> >   		(((_field_id) & GENMASK_ULL(33, 32)) >> 32)
> >   
> > -#define MD_FIELD_ID_ELE_SIZE_16BIT	1
> > +#define MD_FIELD_ELE_SIZE(_field_id)	\
> 
> That ELE seems a bit ambiguous, ELEM seems more natural and is in line 
> with other macros in the kernel.

OK will do.  Thanks for the review!