Re: [PATCH v5 2/8] x86/virt/tdx: Rework TD_SYSINFO_MAP to support build-time verification

[Huang, Kai]

On Mon, 2024-10-14 at 12:13 -0700, Dan Williams wrote:
> Dave Hansen wrote:
> > On 10/14/24 04:31, Kai Huang wrote:
> > > +#define READ_SYS_INFO(_field_id, _member)				\
> > > +	ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id,	\
> > > +					&sysinfo_tdmr->_member)
> > >  
> > > -	return 0;
> > > +	READ_SYS_INFO(MAX_TDMRS,	     max_tdmrs);
> > > +	READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> > > +	READ_SYS_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
> > > +	READ_SYS_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
> > > +	READ_SYS_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
> > 
> > I know what Dan asked for here, but I dislike how this ended up.
> > 
> > The existing stuff *has* type safety, despite the void*.  It at least
> > checks the size, which is the biggest problem.
> > 
> > Also, this isn't really an unrolled loop.  It still effectively has
> > gotos, just like the for loop did.  It just buries the goto in the "ret
> > = ret ?: " construct.  It hides the control flow logic.
> > 
> > Logically, this whole function is
> > 
> > 	ret = read_something1();
> > 	if (ret)
> > 		goto out;
> > 
> > 	ret = read_something2();
> > 	if (ret)
> > 		goto out;
> > 
> > 	...
> > 
> > I'd *much* rather have that goto be:
> > 
> > 	for () {
> > 		ret = read_something();
> > 		if (ret)
> > 			break; // aka. goto out
> > 	}
> > 
> > Than have something *look* like straight control flow when it isn't.

Yeah understood.  Thanks for letting me know.

The 'for() loop' approach would need the original 'struct field_mapping' to hold
the mapping between field ID and the offset/size info, though.

> 
> Yeah, the hiding of the control flow was the weakest part of the
> suggestion. My main gripe was runtime validation of details that could
> be validated at compile time.

I am looking into how to do build-time verification while still using the
original 'struct field_mapping' approach.  If we can do this, I hope this can
address your concern about doing runtime check instead of build-time?

Adrian provided one suggestion [*] that we can use __builtin_choose_expr() to
achieve this:

"
BUILD_BUG_ON() requires a function, but it is still
be possible to add a build time check in TD_SYSINFO_MAP
e.g.

#define TD_SYSINFO_CHECK_SIZE(_field_id, _size)			\
	__builtin_choose_expr(MD_FIELD_ELE_SIZE(_field_id) == _size, _size,
(void)0)

#define _TD_SYSINFO_MAP(_field_id, _offset, _size)		\
	{ .field_id = _field_id,				\
	  .offset   = _offset,					\
	  .size	    = TD_SYSINFO_CHECK_SIZE(_field_id, _size) }

#define TD_SYSINFO_MAP(_field_id, _struct, _member)		\
	_TD_SYSINFO_MAP(MD_FIELD_ID_##_field_id,		\
			offsetof(_struct, _member),		\
			sizeof(typeof(((_struct *)0)->_member)))
"

I tried this, and it worked for most cases where the field ID is a simple
integer constant, but I got build error for the CMRs:

	for (u16 i = 0; i < cmr_info->num_cmrs; i++) {
		const struct field_mapping fields[] = {
			TD_SYSINFO_CMRINFO_MAP(CMR_BASE0 + i, cmr_base[i]),
			TD_SYSINFO_CMRINFO_MAP(CMR_SIZE0 + i, cmr_size[i]),
		};
		...
	}

.. where field ID for CMR[i] is calculated by CMR0.

The MD_FIELD_ELE_SIZE(_field_id) works for 'CMR_BASE0 + i' for BUILD_BUG_ON(),
but somehow the compiler fails to determine the 'MD_FIELD_ELE_SIZE(_field_id) ==
_size' as a constant_express and caused build failure.  I am still looking into
this.

[*]
https://lore.kernel.org/kvm/cover.1721186590.git.kai.huang@xxxxxxxxx/T/#m379ce041f025dc20e7b58fa6dbdc484c2ce53af4

> There is no real need for control flow at all, i.e. early exit is not
> needed as these are not resources that need to be unwound. It simply
> needs to count whether all of the reads happened, so something like this
> is sufficient:
> 
>     success += READ_SYS_INFO(MAX_TDMRS,             max_tdmrs);
>     success += READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
>     success += READ_SYS_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]);
>     success += READ_SYS_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]);
>     success += READ_SYS_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]);
>     
>     if (success != 5)
>     	return false;
> 

If we go with this approach, it seems we can even get rid of the @success.

	int ret = 0;

#define READ_SYS_INFO(_field_id, _member)	\
	read_sys_metadata_field(MD_FIELD_ID_##_field_id, \
	                                     &sysinfo_tdmr->_member)

	ret |= READ_SYS_INFO(MAX_TDMRS, max_tdmrs);
	...
#undef READ_SYS_INFO

	return ret;

The tdh_sys_rd() always return -EIO when TDH.SYS.RD fails, so the above either
return 0 when all reads were successful or -EIO when there's any failed.

I can also go with route if Dave is fine?