Re: [PATCH 3/5] x86/virt/tdx: Unbind global metadata read with 'struct tdx_tdmr_sysinfo'

From: Huang, Kai
Date: Thu May 02 2024 - 20:52:46 EST




On 3/05/2024 12:12 pm, Edgecombe, Rick P wrote:
On Sat, 2024-03-02 at 00:20 +1300, Kai Huang wrote:
+#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)   \
+       TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
 static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 {
        /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
        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]),
+               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]),

The creation of TD_SYSINFO_MAP_TDMR_INFO part is not strictly needed, but makes
sense in the context of the signature change in read_sys_metadata_field16(). It
might be worth justifying it in the log.


I see your point. How about adding below paragraph to the end of this changelog?

"
The metadata reading code uses the TD_SYSINFO_MAP() macro to describe the mapping between the metadata fields and the members of the 'struct tdx_tdmr_sysinfo'. I.e., it hard-codes the 'struct tdx_tdmr_sysinfo' inside the macro.

As part of unbinding metadata read with 'struct tdx_tdmr_sysinfo', the TD_SYSINFO_MAP() macro needs to be changed to additionally take the structure as argument so it can accept any structure. That would make the current code to read TDMR related metadata fields longer if using TD_SYSINFO_MAP() directly.

Define a wrapper macro for reading TDMR related metadata fields to make the code shorter.
"

By typing, it reminds me that I kinda need to learn how to separate the "high level design" vs "low level implementation details". I think the latter can be seen easily in the code, and probably can be avoided in the changelog.

I am not sure whether adding the TD_SYSINFO_MAP_TDMR_INFO() macro belong to which category, especially when I needed a lot text to justify this change (thus I wonder whether it is worth to do).

Or any shorter version that you can suggest?

Thanks.