Re: [PATCH v15 09/23] x86/virt/tdx: Get module global metadata for module initialization

[Dave Hansen <dave.hansen@xxxxxxxxx>]

On 11/9/23 03:55, Kai Huang wrote:
...> +	ret = read_sys_metadata_field16(MD_FIELD_ID_MAX_TDMRS,
> +			&tdmr_sysinfo->max_tdmrs);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_sys_metadata_field16(MD_FIELD_ID_MAX_RESERVED_PER_TDMR,
> +			&tdmr_sysinfo->max_reserved_per_tdmr);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_sys_metadata_field16(MD_FIELD_ID_PAMT_4K_ENTRY_SIZE,
> +			&tdmr_sysinfo->pamt_entry_size[TDX_PS_4K]);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_sys_metadata_field16(MD_FIELD_ID_PAMT_2M_ENTRY_SIZE,
> +			&tdmr_sysinfo->pamt_entry_size[TDX_PS_2M]);
> +	if (ret)
> +		return ret;
> +
> +	return read_sys_metadata_field16(MD_FIELD_ID_PAMT_1G_ENTRY_SIZE,
> +			&tdmr_sysinfo->pamt_entry_size[TDX_PS_1G]);
> +}

I kinda despise how this looks.  It's impossible to read.

I'd much rather do something like the attached where you just map the
field number to a structure member.  Note that this kind of structure
could also be converted to leverage the bulk metadata query in the future.

Any objections to doing something more like the attached completely
untested patch?


---

 b/arch/x86/virt/vmx/tdx/tdx.c |   59 ++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff -puN arch/x86/virt/vmx/tdx/tdx.c~cleaner-tdx-metadata-0 arch/x86/virt/vmx/tdx/tdx.c
--- a/arch/x86/virt/vmx/tdx/tdx.c~cleaner-tdx-metadata-0	2023-11-09 14:58:06.504531884 -0800
+++ b/arch/x86/virt/vmx/tdx/tdx.c	2023-11-09 15:22:46.895941908 -0800
@@ -256,50 +256,59 @@ static int read_sys_metadata_field(u64 f
 	return 0;
 }
 
-static int read_sys_metadata_field16(u64 field_id, u16 *data)
+static int read_sys_metadata_field16(u64 field_id,
+				     int offset,
+				     struct tdx_tdmr_sysinfo *ts)
 {
-	u64 _data;
+	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;
 
-	ret = read_sys_metadata_field(field_id, &_data);
+	ret = read_sys_metadata_field(field_id, &tmp);
 	if (ret)
 		return ret;
 
-	*data = (u16)_data;
+	*ts_member = 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_tdmr_sysinfo,_offset) }
+
+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]),
+};
+
 static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 {
 	int ret;
+	int i;
 
-	ret = read_sys_metadata_field16(MD_FIELD_ID_MAX_TDMRS,
-			&tdmr_sysinfo->max_tdmrs);
-	if (ret)
-		return ret;
-
-	ret = read_sys_metadata_field16(MD_FIELD_ID_MAX_RESERVED_PER_TDMR,
-			&tdmr_sysinfo->max_reserved_per_tdmr);
-	if (ret)
-		return ret;
-
-	ret = read_sys_metadata_field16(MD_FIELD_ID_PAMT_4K_ENTRY_SIZE,
-			&tdmr_sysinfo->pamt_entry_size[TDX_PS_4K]);
-	if (ret)
-		return ret;
+	for (i = 0; i < ARRAY_SIZE(fields); i++) {
+		ret = read_sys_metadata_field16(fields[i].field_id,
+						fields[i].offset,
+						tdmr_sysinfo);
+		if (ret)
+			return ret;
+	}
 
-	ret = read_sys_metadata_field16(MD_FIELD_ID_PAMT_2M_ENTRY_SIZE,
-			&tdmr_sysinfo->pamt_entry_size[TDX_PS_2M]);
-	if (ret)
-		return ret;
-
-	return read_sys_metadata_field16(MD_FIELD_ID_PAMT_1G_ENTRY_SIZE,
-			&tdmr_sysinfo->pamt_entry_size[TDX_PS_1G]);
+	return 0;
 }
 
 static int init_tdx_module(void)
_