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.
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.
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 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]
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);
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) \
+ ({ \
+ 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]);
- return 0;
+ return ret;
}
/* Calculate the actual TDMR size */
--
2.46.0
