> > -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!
