Re: [RFC PATCH v2 10/10] x86/virt/tdx: Verify structure member sizes against metadata field IDs

[Edgecombe, Rick P]

On Fri, 2026-09-18 at 06:29 -0700, Chao Gao wrote:
> A metadata field ID encodes the size of a single element. TDX_SYSINFO_MAP()
> instead derives the copy size from the destination member, and nothing
> verifies that the two sizes agree.
> 
> A wrongly typed member is a kernel bug: declaring a u32 for an 8-byte
> metadata field would silently store only its low 4 bytes.
> 
> Add macros to extract the element size encoded in a field ID and verify it
> against the destination member size at build time.

The two things we could do are extract the field code and check it, or add it
into the field automatically from the struct size. In the later case the field
id's would be specified without the size bits already filled in. But since the
metadata docs have the field code already embedded when they are listed in the
docs, that is the most natural and easy thing to add to the field id code. It
makes it easy to search the docs too. So the checking design gives us some extra
safety, make it easier to add the code and search the docs.

I think probably you need to explain a bit more about what and why the fieldid
size bits exist, but I agree with the design.

> 
> BUILD_BUG_ON() cannot be used in a structure initializer, so use
> BUILD_BUG_ON_ZERO() and add its zero result to the .size initializer. This
> performs the build-time check without changing the stored size.

Neat. Might be controversial. I like it.

> 
> AI was used under supervision to review code and workshop logs. It
> suggested extracting TDX_MD_FIELD_SIZE_CHECK() instead of open coding the
> check in TDX_SYSINFO_MAP(), to keep the .size line from being too long.

Oh yea that is also a lot of bit math to cram in a struct member line too.

> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  arch/x86/virt/vmx/tdx/tdx.c | 12 +++++++++++-
>  arch/x86/virt/vmx/tdx/tdx.h | 15 +++++++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 9d5a3296d3c0..6dc328561009 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -424,11 +424,21 @@ static int __read_sys_metadata_table(const struct field_mapping *mappings,
>  	return 0;
>  }
>  
> +/*
> + * The size encoded in the field ID and the size of the destination C
> + * member must agree.
> + */
> +#define TDX_MD_FIELD_SIZE_CHECK(_field, _type, _member)	\
> +	BUILD_BUG_ON_ZERO(sizeof_field(_type, _member) !=	\
> +			  TDX_MD_FIELD_ELE_SIZE(TDX_MD_FIELD_ID_##_field))
> +
>  #define TDX_SYSINFO_MAP(_field, _type, _member)			\
>  {								\
>  	.field_id	= TDX_MD_FIELD_ID_##_field,		\
>  	.offset		= offsetof(_type, _member),		\
> -	.size		= sizeof_field(_type, _member),		\
> +	.size		= sizeof_field(_type, _member) +	\
> +			  TDX_MD_FIELD_SIZE_CHECK(		\
> +				_field, _type, _member),	\
>  }
>  
>  #define TDX_SYSINFO_MAP_VERSION(_field_id, _member) \
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 407aded3137a..17fdb682410e 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -99,6 +99,21 @@
>  /* Class "TDX Module Handoff" */
>  #define TDX_MD_FIELD_ID_MODULE_HV			0x8900000100000000ULL
>  
> +/*
> + * Sub-field definitions of TDX global metadata field IDs.
> + *
> + * See "Metadata Field Identifier" in the Intel TDX Module ABI
> + * Specification.
> + *
> + *  - Bit 33:32: ELEMENT_SIZE_CODE -- log2 of a single metadata
> + *                                    element's size in bytes
> + */
> +#define TDX_MD_FIELD_ELE_SIZE_CODE(field_id)	\
> +	(((field_id) & GENMASK_ULL(33, 32)) >> 32)
> +
> +#define TDX_MD_FIELD_ELE_SIZE(field_id)	\
> +	(1 << TDX_MD_FIELD_ELE_SIZE_CODE(field_id))
> +

I'd think these could be squashed together since only TDX_MD_FIELD_ELE_SIZE gets
used anywhere else.

>  /* TDX page types */
>  #define	PT_NDA		0x0
>  #define	PT_RSVD		0x1