Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

[Huang, Kai]

> 
> > +#define build_sysmd_read(_size)							\
> > +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val)	\
> > +{										\
> > +	u64 tmp;								\
> > +	int ret;								\
> > +										\
> > +	ret = tdh_sys_rd(field_id, &tmp);					\
> > +	if (ret)								\
> > +		return ret;							\
> > +										\
> > +	*val = tmp;								\
> > +										\
> > +	return 0;								\
> >   }
> >   
> > -#define read_sys_metadata_field16(_field_id, _val)		\
> > +build_sysmd_read(16)
> 
> nit: Generally the unwritten convention for this kind of macro 
> definition is to capitalize them and be of the from:
> 
> DEFINE_xxxxx - similar to how event classes are defined.
> 
> perhaps naming this macro:
> 
> DEFINE_TDX_METADATA_READER() ought to be more descriptive, also the
> "md" contraction of metadata also seems a bit quirky (at least to me).
> 
> It's not a deal breaker but if there is going to be another posting this 
> might be something to consider.
> 

Thanks for the comments.

I don't have opinion on this.  Dan said we can do something like
build_mmio_read() macro and this is where build_sysmd_read() came from.  Dan
used build_tdg_sys_rd() in his patch too:

https://lore.kernel.org/all/172618717675.516322.6087817418162288917.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/

Btw I actually agree that your DEFINE_xx() is more formal thus is better when
the macro is used by multiple C files.  But here only tdx.c uses it, and IMHO
it's also fine to have a informal but shorter name here.

Anyway let's see whether other people have anything to say.