Re: [PATCH v2 02/10] x86/virt/tdx: Unbind global metadata read with 'struct tdx_tdmr_sysinfo'

[Dan Williams]

Huang, Kai wrote:
[..]
> > 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

I agree with Dave that the original was unreadable. However, I also
think he glossed over the loss of type-safety and the silliness of
defining an array to precisely map fields only to turn around and do a
runtime check that the statically defined array was filled out
correctly. So I think lets solve the readability problem *and* make the
array definition identical in appearance to unrolled type-safe
execution, something like (UNTESTED!):


diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
index 4e2b2e2ac9f9..a177fb7332ae 100644
--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -270,60 +270,42 @@ 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_tdmr_sysinfo *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;
-
 	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_tdmr_sysinfo, _offset) }
-
-/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
-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]),
-};
-
-static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
+/*
+ * Assumes locally defined @ret and @ts to convey the error code and the
+ * 'struct tdx_tdmr_sysinfo' instance to fill out
+ */
+#define TD_SYSINFO_MAP(_field_id, _offset)                              \
+	({                                                              \
+		if (ret == 0)                                           \
+			ret = read_sys_metadata_field16(                \
+				MD_FIELD_ID_##_field_id, &ts->_offset); \
+	})
+
+static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *ts)
 {
-	int ret;
-	int i;
+	int ret = 0;
 
-	/* Populate 'tdmr_sysinfo' 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,
-						tdmr_sysinfo);
-		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 */