On 27/08/24 10:14, Kai Huang wrote:
> 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 fields
> for module initialization. There are both immediate needs to read more
> fields for module initialization and near-future needs for other kernel
> components like KVM to run TDX guests.
> will be organized in different structures depending on their meanings.
Line above seems lost
>
> For now the kernel only reads TDMR related fields. The TD_SYSINFO_MAP()
> macro hard-codes the 'struct tdx_sys_info_tdmr' instance name. To make
> it work with different instances of different structures, extend it to
> take the structure instance name as an argument.
>
> This also means the current code which uses TD_SYSINFO_MAP() must type
> 'struct tdx_sys_info_tdmr' instance name explicitly for each use. To
> make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO()
> which hides the instance name.
Note, were you to accept my suggestion for patch 2, TD_SYSINFO_MAP() would
have gone away, and no changes would be needed to get_tdx_sys_info_tdmr().
So the above 2 paragraphs could be dropped.
>
> TDX also support 8/16/32/64 bits metadata field element sizes. For now
> all TDMR related fields are 16-bit long thus the kernel only has one
> helper:
>
> static int read_sys_metadata_field16(u64 field_id, u16 *val) {}
>
> Future changes will need to read more metadata fields with different
> element sizes. To make the code short, extend the helper to take a
> 'void *' buffer and the buffer size so it can work with all element
> sizes.
>
> Note in this way the helper loses the type safety and the build-time
> check inside the helper cannot work anymore because the compiler cannot
> determine the exact value of the buffer size.
>
> To resolve those, add a wrapper of the helper which only works with
> u8/u16/u32/u64 directly and do build-time check, where the compiler
> can easily know both the element size (from field ID) and the buffer
> size(using sizeof()), before calling the helper.
>
> An alternative option is to provide one helper for each element size:
IMHO, it is not necessary to describe alternative options. Better to
explain why the actual code change is good, rather than why something
that wasn't done, wasn't so good. That discussion can stay on the
mailing list. For example, this commit message can just have:
Add a build-time check that the element size is correct, which
enables a common function to safely read data of different sizes.
Perhaps end up with:
TDX supports 8/16/32/64 bit metadata field element sizes. For now all
TDMR related fields are 16-bits long. Future changes will need to read
more metadata fields with different element sizes.
So add a build-time check that the element size is correct, which
enables a common function to safely read data of different sizes.
>
> static int read_sys_metadata_field8(u64 field_id, u8 *val) {}
> static int read_sys_metadata_field16(u64 field_id, u16 *val) {}
> ...
>
> But this will result in duplicated code given those helpers will look
> exactly the same except for the type of buffer pointer. It's feasible
> to define a macro for the body of the helper and define one entry for
> each element size to reduce the code, but it is a little bit
> over-engineering.
>
> Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> ---
>
> v2 -> v3:
> - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be
> used as the high level wrapper. Get rid of "stbuf_" prefix since
> people don't like it.
>
> - Rewrite after removing 'struct field_mapping' and reimplementing
> TD_SYSINFO_MAP().
>
> ---
> arch/x86/virt/vmx/tdx/tdx.c | 45 +++++++++++++++++++++----------------
> arch/x86/virt/vmx/tdx/tdx.h | 3 ++-
> 2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 7e75c1b10838..1cd9035c783f 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list)
> return ret;
> }
>
> -static int read_sys_metadata_field(u64 field_id, u64 *data)
> +static int tdh_sys_rd(u64 field_id, u64 *data)
> {
> struct tdx_module_args args = {};
> int ret;
> @@ -270,43 +270,50 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
> return 0;
> }
>
> -static int read_sys_metadata_field16(u64 field_id, u16 *val)
> +static int __read_sys_metadata_field(u64 field_id, void *val, int size)
> {
> 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)
> +
> + 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) \
> + (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))
>
> struct tdmr_reserved_area {
> u64 offset;
