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

Huang, Kai
2024-09-26T12:13:38Z

>
> > +#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@dwillia2-xfh.jf.intel.com/

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.