On 27/08/24 10:14, Kai Huang wrote:
Subject could be "Simplify the reading of Global Metadata Fields"
> 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.
>
> The TDX module provides a set of "global metadata fields". They report
Global Metadata Fields
> 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.
>
> 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/
> 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
>
> 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
>
> - return 0;
> + return ret;
> }
>
> /* Calculate the actual TDMR size */
