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

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Huang, Kai" <kai.huang@intel.com>
Cc: "Hansen, Dave" <dave.hansen@intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"bp@alien8.de" <bp@alien8.de>, "x86@kernel.org" <x86@kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"jgross@suse.com" <jgross@suse.com>,
	"Yamahata, Isaku" <isaku.yamahata@intel.com>
Subject: Re: [PATCH 3/5] x86/virt/tdx: Unbind global metadata read with 'struct tdx_tdmr_sysinfo'
Date: Fri, 3 May 2024 19:03:00 +0000	[thread overview]
Message-ID: <18f5114bd700f13fac5b36bd322745cb2ea2ab15.camel@intel.com> (raw)
In-Reply-To: <6940c326-bfca-4c67-badf-ab5c086bf492@intel.com>

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].

Question for you:
Is this just to make it shorter, or to avoid duplication of specifying the
struct name? Like is it a mitigation for exceeding 80 chars or 100?

> 
> 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.