Re: [PATCH 4/5] x86/virt/tdx: Support global metadata read for all element sizes

From: "Huang, Kai" <kai.huang@intel.com>
To: "Yamahata, Isaku" <isaku.yamahata@intel.com>
Cc: "jgross@suse.com" <jgross@suse.com>,
	"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>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>,
	"mingo@redhat.com" <mingo@redhat.com>
Subject: Re: [PATCH 4/5] x86/virt/tdx: Support global metadata read for all element sizes
Date: Wed, 20 Mar 2024 12:24:54 +0000	[thread overview]
Message-ID: <a10578487c36a4530d880d4988734d9097e34979.camel@intel.com> (raw)
In-Reply-To: <20240316004945.GL1258280@ls.amr.corp.intel.com>

> > @@ -295,11 +307,30 @@ static int read_sys_metadata_field16(u64 field_id,
> >  struct field_mapping {
> >  	u64 field_id;
> >  	int offset;
> > +	int size;
> >  };
> >  
> >  #define TD_SYSINFO_MAP(_field_id, _struct, _member)	\
> >  	{ .field_id = MD_FIELD_ID_##_field_id,		\
> > -	  .offset   = offsetof(_struct, _member) }
> > +	  .offset   = offsetof(_struct, _member),	\
> > +	  .size     = sizeof(typeof(((_struct *)0)->_member)) }
> 
> Because we use compile time constant for _field_id mostly, can we add build
> time check? Something like this.
> 
> static inline metadata_size_check(u64 field_id, size_t size)
> {
>         BUILD_BUG_ON(get_metadata_field_bytes(field_id) != size);
> }
> 
> #define TD_SYSINFO_MAP(_field_id, _struct, _member)	\
> 	{ .field_id = MD_FIELD_ID_##_field_id,		\
> 	  .offset   = offsetof(_struct, _member),	\
> 	  .size     = \
> 		({ size_t s = sizeof(typeof(((_struct *)0)->_member)); \
>                 metadata_size_check(MD_FIELD_ID_##_field_id, s); \
>                 s; }) }
> 

Hmm.. The problem is "mostly" as you mentioned?

My understanding is BUILD_BUG_ON() relies on the "condition" to be compile-time
constant.  In your KVM TDX patchset there's code to do ...

	for (i = 0; i < NR_WHATEVER; i++) {
		const struct tdx_metadata_field_mapping fields = {
			TD_SYSINFO_MAP(FIELD_WHATEVERE + i, ...),
			...
		};

		...
	}

To be honest I am not exactly sure whether the compiler can determine
"FIELD_WHATEVER + i" as compile-time constant.

Btw, if there's any mismatch, the current code can already catch during runtime.
I think one purpose (perhaps the most important purpose) of BUILD_BUG_ON() is to
catch bug early if someone changed the constant (macros etc) in the "condition".
But in our case, once written no one is going to change the structure or the
metadata fields.  So I am not sure whether it's worth to do.