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

[Dan Williams]

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

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]

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

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

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);			\
	})