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?
