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

Nikolay Borisov
2024-09-26T06:27:04Z

On 24.09.24 г. 14:28 ч., Kai Huang wrote:
> The TDX module provides a set of "Global Metadata Fields". They report
> things like TDX module version, supported features, and fields related
> to create/run TDX guests and so on. TDX supports 8/16/32/64 bits
> metadata field element sizes. For a given metadata field, the element
> size is encoded in the metadata field ID.
>
> For now the kernel only reads "TD Memory Region" (TDMR) related metadata
> fields and they are all 16-bit. Thus the kernel only has one primitive
> __read_sys_metadata_field16() to read 16-bit metadata field and the
> macro, read_sys_metadata_field16(), which does additional build-time
> check of the field ID makes sure the field is indeed 16-bit.
>
> Future changes will need to read more metadata fields with different
> element sizes. Choose to provide one primitive for each element size to
> support that. Similar to the build_mmio_read() macro, reimplement the
> body of __read_sys_metadata_field16() as a macro build_sysmd_read(_size)
> in size-agnostic way, so it can be used to generate one primitive for
> each element size:
>
> build_sysmd_read(8)
> build_sysmd_read(16)
> ..
>
> Also extend read_sys_metadata_field16() take the '_size' as argument
> (and rename it to read_sys_metadata_field() to make it size-agnostic) to
> allow the READ_SYS_INFO() macro to choose which primitive to use.
>
> Signed-off-by: Kai Huang <kai.huang@intel.com>

<snip>

> +#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.

<snip>