On 6/08/2024 11:32 am, Williams, Dan J wrote:
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 global
metadata fields to a 'struct tdx_tdmr_sysinfo' for initializing the TDX
module, and the metadata reading code can only work with that structure.
Future changes will need to read other metadata fields that don't make
sense to populate to the "struct tdx_tdmr_sysinfo". It's essential to
provide a generic metadata read infrastructure which is not bound to any
specific structure.
To start providing such infrastructure, unbind the metadata reading code
with the 'struct tdx_tdmr_sysinfo'.
Note the kernel has a helper macro, TD_SYSINFO_MAP(), for marshaling the
metadata into the 'struct tdx_tdmr_sysinfo', and currently the macro
hardcodes the structure name. As part of unbinding the metadata reading
code with 'struct tdx_tdmr_sysinfo', it is extended to accept different
structures.
Unfortunately, this will result in the usage of TD_SYSINFO_MAP() for
populating 'struct tdx_tdmr_sysinfo' to be changed to use the structure
name explicitly for each structure member and make the code longer. Add
a wrapper macro which hides the 'struct tdx_tdmr_sysinfo' internally to
make the code shorter thus better readability.
Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
Reviewed-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
---
v1 -> v2:
- 'st_member' -> 'member'. (Nikolay)
---
arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index d8fa9325bf5e..2ce03c3ea017 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -272,9 +272,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
static int read_sys_metadata_field16(u64 field_id,
int offset,
- struct tdx_tdmr_sysinfo *ts)
+ void *stbuf)
The loss of all type-safety sticks out, and points to the fact that
@offset was awkward to pass in from the beginning. I would have expected
a calling convention like:
static int read_sys_metadata_field16(u64 field_id, u16 *val)
...and make the caller calculate the buffer in a type-safe way.
The problem with the current code is that it feels like it is planning
ahead for a dynamic metdata reading future, that is not coming, Instead
it could be leaning further into initializing all metadata once.
In other words what is the point of defining:
static const struct field_mapping fields[]
...only to throw away all type-safety and run it in a loop. Why not
unroll the loop, skip the array, and the runtime warning with something
like?
read_sys_metadata_field16(MD_FIELD_ID_MAX_TDMRS, &ts->max_tdmrs);
read_sys_metadata_field16(MD_FIELD_ID_MAX_RESERVED_PER_TDMR, &ts->max_reserved_per_tdmr);
...etc
The unrolled loop is the same amount of work as maintaining @fields.
Hi Dan,
Thanks for the feedback.
AFAICT Dave didn't like this way:
https://lore.kernel.org/lkml/cover.1699527082.git.kai.huang@xxxxxxxxx/T/#me6f615d7845215c278753b57a0bce1162960209d
