On 27.08.24 г. 10:14 ч., 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.
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.
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.
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:
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)
The type of 'size' should be size_t.
{
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)
nit: I guess its a personal preference but honestly I think the amount
of macro indirection (3 levels) here is crazy, despite each being rather
simple. Just use TD_SYSINFO_MAP directly, saving the typing of
"sysinfo_tdmr" doesn't seem like a big deal.
You can probably take it even a bit further and simply opencode
read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with
just it, no ? No other patch in this series uses read_sys_metadata_field
stand alone, if anything factoring it out could be deferred until the
first users gets introduced.
+
+ 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) \
That ELE seems a bit ambiguous, ELEM seems more natural and is in line
with other macros in the kernel.
+ (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))
struct tdmr_reserved_area {
u64 offset;
