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

From: "Huang, Kai" <kai.huang@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
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 12:52:07 +1200	[thread overview]
Message-ID: <6940c326-bfca-4c67-badf-ab5c086bf492@intel.com> (raw)
In-Reply-To: <ebc3ef050ce889980c46275dac9eb21ab7289b8a.camel@intel.com>

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.