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

[Huang, Kai]

On Fri, 2024-05-03 at 19:03 +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-05-03 at 12:52 +1200, Huang, Kai wrote:
> > "
> > 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.
> 
> How about:
> 
> The TDX module initialization code currently uses the metadata reading
> infrastructure to read several TDX module fields, and populate them all into the
> same kernel defined struct, "struct tdx_tdmr_sysinfo". So the helper macros for
> marshaling the data from the TDX module into the struct fields hardcode that
> struct name.
> 
> > 
> > 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.
> 
> Future changes will allow for other types of metadata to be read, that don't
> make sense to populate to that specific struct. To accommodate this the data
> marshaling macro, TD_SYSINFO_MAP, will be extended to take different structs.
> Unfortunately, it will result in the usage of TD_SYSINFO_MAP for populating
> struct tdx_tdmr_sysinfo to change to... [some undesirable situation].

I'll change to use your words, with some small tweaks to also mention the
function to read metadata field should also be relaxed to take a typeless
'void *' buffer.

Please see below.

> 
> Question for you:
> Is this just to make it shorter, or to avoid duplication of specifying the
> struct name? 
> 

The intention was to make it shorter, but I think both.

> Like is it a mitigation for exceeding 80 chars or 100?

Yes for not exceeding 100.

With this patch, the code actually exceeds 80 chars, but I found breaking
them to separate lines hurt the readability.

> 
> > 
> > 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.
> 
> Especially for TDX with all it's complexity and acronyms I think it helps to
> explain in simple terms. Like imagine if someone was working at their computer
> and you tapped on their shoulder, how would you introduce this change? If you
> start with "TDMR related global metadata fields" and "struct tdx_tdmr_sysinfo"
> they are going to have to struggle to context switch into it.
> 
> For each patch, if the connection is not clear, ease them into it. Of course
> everyone has the different preferences, so YMMV. But especially the tip folks
> seem to appreciate it.
> 
> > 
> > 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?
> > 
> 
> I don't think it is too long.

The new changelog based on your words:

The TDX module initialization code currently uses the metadata reading
infrastructure to read several TDX module fields, and populate them all
into the same kernel defined struct, "struct tdx_tdmr_sysinfo".  So the
function to read the metadata fields and the helper macros for marshaling
the data from the TDX module into the struct fields hardcode that struct
name.

Future changes will allow for other types of metadata to be read, that
don't make sense to populate to that specific struct.  To accommodate
this, change the metadata reading function to take a typeless 'void *'
buffer, and extend the data marshaling macro, TD_SYSINFO_MAP, to take
different structs.

Unfortunately, this will result in the usage of TD_SYSINFO_MAP for
populating 'struct tdx_tdmr_sysinfo' to be changed to use the struct name
explicitly for each struct member and make the code longer.  Define a
wrapper macro for reading TDMR related metadata fields to make the code
shorter, i.e., not exceeding the 100 characters limit while still keeping
the use of TDX_SYSINFO_MAP for each struct member in one line for better
readability.