On Thu, 2024-08-29 at 10:20 +0300, Adrian Hunter wrote:
> On 27/08/24 10:14, Kai Huang wrote:
>
> Subject could be "Simplify the reading of Global Metadata Fields"
OK.
>
> > TL;DR: Remove the 'struct field_mapping' structure and use another way
> > to implement the TD_SYSINFO_MAP() macro to improve the current metadata
> > reading code, e.g., switching to use build-time check for metadata field
> > size over the existing runtime check.
>
> Perhaps:
>
> Remove 'struct field_mapping' and let read_sys_metadata_field16() accept
> the member variable address, simplifying the code in preparation for adding
> support for more metadata structures and field sizes.
Fine to me. I would like also to mention there are improvements in the new
code (as suggested by Dan), so I will say:
"..., simplifying and improving the code in preparation ...".
>
> >
> > The TDX module provides a set of "global metadata fields". They report
>
> Global Metadata Fields
OK.
>
> > things like TDX module version, supported features, and fields related
> > to create/run TDX guests and so on.
> >
> > For now the kernel only reads "TD Memory Region" (TDMR) related global
> > metadata fields, and all of those fields are organized in one structure.
>
> The patch is self-evidently simpler (21 insertions(+), 36 deletions(-))
> so there doesn't seem to be any need for further elaboration. Perhaps
> just round it off and stop there.
>
> ... and all of those fields are organized in one structure,
> but that will change in the near future.
I think the code change here is not only to reduce the LoC (i.e., simplify the
code), but also improve the code, so I would like to keep the things mentioned
by Dan in the changelog.
>
> >
> > The kernel currently uses 'struct field_mapping' to facilitate reading
> > multiple metadata fields into one structure. The 'struct field_mapping'
> > records the mapping between the field ID and the offset of the structure
> > to fill out. The kernel initializes an array of 'struct field_mapping'
> > for each structure member (using the TD_SYSINFO_MAP() macro) and then
> > reads all metadata fields in a loop using that array.
> >
> > Currently the kernel only reads TDMR related metadata fields into one
> > structure, and the function to read one metadata field takes the pointer
> > of that structure and the offset:
> >
> > static int read_sys_metadata_field16(u64 field_id,
> > int offset,
> > struct tdx_sys_info_tdmr *ts)
> > {...}
> >
> > Future changes will need to read more metadata fields into different
> > structures. To support this the above function will need to be changed
> > to take 'void *':
> >
> > static int read_sys_metadata_field16(u64 field_id,
> > int offset,
> > void *stbuf)
> > {...}
> >
> > This approach loses type-safety, as Dan suggested. A better way is to
> > make it be ..
> >
> > static int read_sys_metadata_field16(u64 field_id, u16 *val) {...}
> >
> > .. and let the caller calculate the buffer in a type-safe way [1].
> >
> > Also, the using of the 'struct field_mapping' loses the ability to be
> > able to do build-time check about the metadata field size (encoded in
> > the field ID) due to the field ID is "indirectly" initialized to the
> > 'struct field_mapping' and passed to the function to read. Thus the
> > current code uses runtime check instead.
> >
> > Dan suggested to remove the 'struct field_mapping', unroll the loop,
> > skip the array, and call the read_sys_metadata_field16() directly with
> > build-time check [1][2]. And to improve the readability, reimplement
> > the TD_SYSINFO_MAP() [3].
> >
> > The new TD_SYSINFO_MAP() isn't perfect. It requires the function that
> > uses it to define a local variable @ret to carry the error code and set
> > the initial value to 0. It also hard-codes the variable name of the
> > structure pointer used in the function. But overall the pros of this
> > approach beat the cons.
> >
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@xxxxxxxxx/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1]
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@xxxxxxxxx/T/#mbe65f0903ff7835bc418a907f0d02d7a9e0b78be [2]
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@xxxxxxxxx/T/#m80cde5e6504b3af74d933ea0cbfc3ca9d24697d3 [3]
>
> Probably just one link would suffice, say the permalink to Dan's
> comment:
>
> https://lore.kernel.org/kvm/66b16121c48f4_4fc729424@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch/
[1] and [2] are actually replies to different patches, so I would like to keep
them. [1] and [3] are replies to the same patch, so I could remove [3], but I
think keeping all of them is also fine since it's more easy to find the exact
comment that I want to address.
>
> > Suggested-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> > ---
> >
> > v2 -> v3:
> > - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP().
> >
> > ---
> > arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++-----------------------
> > 1 file changed, 21 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index e979bf442929..7e75c1b10838 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -270,60 +270,45 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
> > return 0;
> > }
> >
> > -static int read_sys_metadata_field16(u64 field_id,
> > - int offset,
> > - struct tdx_sys_info_tdmr *ts)
> > +static int read_sys_metadata_field16(u64 field_id, u16 *val)
> > {
> > - u16 *ts_member = ((void *)ts) + offset;
> > u64 tmp;
> > int ret;
> >
> > - if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > - MD_FIELD_ID_ELE_SIZE_16BIT))
> > - return -EINVAL;
> > + BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > + MD_FIELD_ID_ELE_SIZE_16BIT);
>
> This gets removed next patch, so why do it
This patch I didn't add the build-time check in the (new) TD_SYSINFO_MAP(), so
I changed the runtime check to build-time check here since we can actually do
it here. I think even for this middle state patch we should do the build-time
check. I can move it to the (new) TD_SYSINFO_MAP() though if that's better.
>
> >
> > ret = read_sys_metadata_field(field_id, &tmp);
> > if (ret)
> > return ret;
> >
> > - *ts_member = tmp;
> > + *val = tmp;
> >
> > return 0;
> > }
> >
> > -struct field_mapping {
> > - u64 field_id;
> > - int offset;
> > -};
> > -
> > -#define TD_SYSINFO_MAP(_field_id, _offset) \
> > - { .field_id = MD_FIELD_ID_##_field_id, \
> > - .offset = offsetof(struct tdx_sys_info_tdmr, _offset) }
> > -
> > -/* Map TD_SYSINFO fields into 'struct tdx_sys_info_tdmr': */
> > -static const struct field_mapping fields[] = {
> > - 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]),
> > -};
> > +/*
> > + * Assumes locally defined @ret and @sysinfo_tdmr to convey the error
> > + * code and the 'struct tdx_sys_info_tdmr' instance to fill out.
> > + */
> > +#define TD_SYSINFO_MAP(_field_id, _member) \
>
> "MAP" made sense when it was in a struct whereas
> now it is reading.
>
> > + ({ \
> > + if (!ret) \
> > + ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \
> > + &sysinfo_tdmr->_member); \
> > + })
> >
> > static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> > {
> > - int ret;
> > - int i;
> > + int ret = 0;
> >
> > - /* Populate 'sysinfo_tdmr' fields using the mapping structure above: */
> > - for (i = 0; i < ARRAY_SIZE(fields); i++) {
> > - ret = read_sys_metadata_field16(fields[i].field_id,
> > - fields[i].offset,
> > - sysinfo_tdmr);
> > - if (ret)
> > - return ret;
> > - }
> > + 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]);
>
> Another possibility is to put the macro at the invocation site:
>
> #define READ_SYS_INFO(_field_id, _member) \
> ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \
> &sysinfo_tdmr->_member)
>
> READ_SYS_INFO(MAX_TDMRS, max_tdmrs);
> READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> READ_SYS_INFO(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]);
> READ_SYS_INFO(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]);
> READ_SYS_INFO(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]);
>
> #undef READ_SYS_INFO
>
> And so on in later patches:
>
> #define READ_SYS_INFO(_field_id, _member) \
> ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id, \
> &sysinfo_version->_member)
>
> READ_SYS_INFO(MAJOR_VERSION, major);
> READ_SYS_INFO(MINOR_VERSION, minor);
> READ_SYS_INFO(UPDATE_VERSION, update);
> READ_SYS_INFO(INTERNAL_VERSION, internal);
> READ_SYS_INFO(BUILD_NUM, build_num);
> READ_SYS_INFO(BUILD_DATE, build_date);
>
> #undef READ_SYS_INFO
>
This is fine to me. But AFAICT Kirill doesn't like the "#undef" part.
Kirill, do you have comments?
Otherwise I will go with what Adrian suggested.
