Re: [PATCH v3 2/8] x86/virt/tdx: Remove 'struct field_mapping' and implement TD_SYSINFO_MAP() macro

[Huang, Kai]

On Fri, 2024-09-06 at 13:21 -0700, Dan Williams wrote:
> I think the subject buries the lead on what this patch does which is
> more like:
> 
> x86/virt/tdx: Rework TD_SYSINFO_MAP to support build-time verification

Yes this looks better.  Thanks.

> 
> Kai Huang wrote:
> > TL;DR: Remove the 'struct field_mapping' structure and use another way
> 
> I would drop the TL;DR: and just make the changelog more concise,
> because as it stands now it requires the reader to fully appreciate the
> direction of the v1 approach which this new proposal abandons:
> 
> Something like:
> 
>     Dan noticed [1] that read_sys_metadata_field16() has a runtime warning
>     to validate that the metadata field size matches the passed in buffer
>     size. In turns out that all the information to perform that validation
>     is available at build time. Rework TD_SYSINFO_MAP() to stop providing
>     runtime data to read_sys_metadata_field16() and instead just pass typed
>     fields to read_sys_metadata_field16() and let the compiler catch any
>     mismatches.
>     
>     The new TD_SYSINFO_MAP() has a couple quirks for readability.  It
>     requires the function that uses it to define a local variable @ret to
>     carry the error code and set the initial value to 0.  It also hard-codes
>     the variable name of the structure pointer used in the function, but it
>     is less code, build-time verfiable, and the same readability as the
>     former 'struct field_mapping' approach.
>     
>     Link: http://lore.kernel.org/66b16121c48f4_4fc729424@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch [1]

Thanks.  Will do.

Btw, Adrian suggested to rename TD_SYSINFO_MAP() to READ_SYS_INFO(), which is
reasonable to me, so I will also add "rename TD_SYSINFO_MAP() to
READ_SYS_INFO()" to your above text.  Please let know if you have any comments.

> 
> [..]
> > Link: https://lore.kernel.org/kvm/a107b067-861d-43f4-86b5-29271cb93dad@xxxxxxxxx/T/#m7cfb3c146214d94b24e978eeb8708d92c0b14ac6 [1]
> 
> The expectation for lore links is to capture the message-id. Note the
> differences with the "Link:" format above.

Oh I did not know this.  What's the difference between message-id and a normal
link that I got by "google <patch name> + open that lore link"?

> 
> > v2 -> v3:
> >  - Remove 'struct field_mapping' and reimplement TD_SYSINFO_MAP().
> > 
> > ---
> >  arch/x86/virt/vmx/tdx/tdx.c | 57 ++++++++++++++-----------------------
> >  1 file changed, 21 insertions(+), 36 deletions(-)
> > 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index e979bf442929..7e75c1b10838 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -270,60 +270,45 @@ 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_sys_info_tdmr *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;
> > +	BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> > +			MD_FIELD_ID_ELE_SIZE_16BIT);
> 
> Perhaps just move this to TD_SYSINFO_MAP() directly?

Sure will do.  It gets moved to TD_SYSINFO_MAP() in the next patch anyway.

> 
> Something like:
> 
> #define TD_SYSINFO_MAP(_field_id, _member, _size)					\
> 	({										\
> 		BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=			\
> 				MD_FIELD_ID_ELE_SIZE_##_size##BIT);			\
> 		if (!ret)								\
> 			ret = read_sys_metadata_field##_size(MD_FIELD_ID_##_field_id,	\
> 					&sysinfo_tdmr->_member);			\
> 	})

Will do.

Btw this patch doesn't extend to support other sizes but only 16-bits, so I'll
defer the support of "_size" to the next patch.