Re: [PATCH v2 02/10] x86/virt/tdx: Unbind global metadata read with 'struct tdx_tdmr_sysinfo'

[Adrian Hunter <adrian.hunter@intel.com>]

On 27/08/24 01:40, Huang, Kai wrote:
> On Mon, 2024-08-26 at 18:38 +0300, Adrian Hunter wrote:
>> On 7/08/24 15:09, Huang, Kai wrote:
>>> On Mon, 2024-08-05 at 18:13 -0700, Dan Williams wrote:
>>>> Huang, Kai wrote:
>>>> [..]
>>>>>> The unrolled loop is the same amount of work as maintaining @fields.
>>>>>
>>>>> Hi Dan,
>>>>>
>>>>> Thanks for the feedback.
>>>>>
>>>>> AFAICT Dave didn't like this way:
>>>>>
>>>>> https://lore.kernel.org/lkml/cover.1699527082.git.kai.huang@intel.com/T/#me6f615d7845215c278753b57a0bce1162960209d
>>>>
>>>> I agree with Dave that the original was unreadable. However, I also
>>>> think he glossed over the loss of type-safety and the silliness of
>>>> defining an array to precisely map fields only to turn around and do a
>>>> runtime check that the statically defined array was filled out
>>>> correctly. So I think lets solve the readability problem *and* make the
>>>> array definition identical in appearance to unrolled type-safe
>>>> execution, something like (UNTESTED!):
>>>>
>>>>
>>> [...]
>>>
>>>> +/*
>>>> + * Assumes locally defined @ret and @ts to convey the error code and the
>>>> + * 'struct tdx_tdmr_sysinfo' instance to fill out
>>>> + */
>>>> +#define TD_SYSINFO_MAP(_field_id, _offset)                              \
>>>> + ({                                                              \
>>>> +         if (ret == 0)                                           \
>>>> +                 ret = read_sys_metadata_field16(                \
>>>> +                         MD_FIELD_ID_##_field_id, &ts->_offset); \
>>>> + })
>>>> +
>>>
>>> We need to support u16/u32/u64 metadata field sizes, but not just u16.
>>>
>>> E.g.:
>>>
>>> struct tdx_sysinfo_module_info {
>>>         u32 sys_attributes;
>>>         u64 tdx_features0;
>>> };
>>>
>>> has both u32 and u64 in one structure.
>>>
>>> To achieve type-safety for all field sizes, I think we need one helper
>>> for each field size.  E.g.,
>>>
>>> #define READ_SYSMD_FIELD_FUNC(_size)                            \
>>> static inline int                                               \
>>> read_sys_metadata_field##_size(u64 field_id, u##_size *data)    \
>>> {                                                               \
>>>         u64 tmp;                                                \
>>>         int ret;                                                \
>>>                                                                 \
>>>         ret = read_sys_metadata_field(field_id, &tmp);          \
>>>         if (ret)                                                \
>>>                 return ret;                                     \
>>>                                                                 \
>>>         *data = tmp;                                            \
>>>         return 0;                                               \
>>> }
>>>
>>> /* For now only u16/u32/u64 are needed */
>>> READ_SYSMD_FIELD_FUNC(16)
>>> READ_SYSMD_FIELD_FUNC(32)
>>> READ_SYSMD_FIELD_FUNC(64)
>>>
>>> Is this what you were thinking?
>>>
>>> (Btw, I recall that I tried this before for internal review, but AFAICT
>>> Dave didn't like this.)
>>>
>>> For the build time check as you replied to the next patch, I agree it's
>>> better than the runtime warning check as done in the current code.
>>>
>>> If we still use the type-less 'void *stbuf' function to read metadata
>>> fields for all sizes, then I think we can do below:
>>>
>>> /*
>>>  * Read one global metadata field and store the data to a location of a
>>>  * given buffer specified by the offset and size (in bytes).
>>>  */
>>> static int stbuf_read_sysmd_field(u64 field_id, void *stbuf, int offset,
>>>                                   int size)
>>> {
>>>         void *member = stbuf + offset;
>>>         u64 tmp;
>>>         int ret;
>>>
>>>         ret = read_sys_metadata_field(field_id, &tmp);
>>>         if (ret)
>>>                 return ret;
>>>
>>>         memcpy(member, &tmp, size);
>>>
>>>         return 0;
>>> }
>>>
>>> /* Wrapper to read one metadata field to u8/u16/u32/u64 */
>>> #define stbuf_read_sysmd_single(_field_id, _pdata)      \
>>>         stbuf_read_sysmd_field(_field_id, _pdata, 0,        \
>>>             sizeof(typeof(*(_pdata))))
>>>
>>> #define CHECK_MD_FIELD_SIZE(_field_id, _st, _member)    \
>>>         BUILD_BUG_ON(MD_FIELD_ELE_SIZE(MD_FIELD_ID_##_field_id) != \
>>>                         sizeof(_st->_member))
>>>
>>> #define TD_SYSINFO_MAP_TEST(_field_id, _st, _member)                    \
>>>         ({                                                              \
>>>                 if (ret) {                                              \
>>>                         CHECK_MD_FIELD_SIZE(_field_id, _st, _member);   \
>>>                         ret = stbuf_read_sysmd_single(                  \
>>>                                         MD_FIELD_ID_##_field_id,        \
>>>                                         &_st->_member);                 \
>>>                 }                                                       \
>>>          })
>>>
>>> static int get_tdx_module_info(struct tdx_sysinfo_module_info *modinfo)
>>> {
>>>         int ret = 0;
>>>
>>> #define TD_SYSINFO_MAP_MOD_INFO(_field_id, _member)     \
>>>         TD_SYSINFO_MAP_TEST(_field_id, modinfo, _member)
>>>
>>>         TD_SYSINFO_MAP_MOD_INFO(SYS_ATTRIBUTES, sys_attributes);
>>>         TD_SYSINFO_MAP_MOD_INFO(TDX_FEATURES0,  tdx_features0);
>>>
>>>         return ret;
>>> }
>>>
>>> With the build time check above, I think it's OK to lose the type-safe
>>> inside the stbuf_read_sysmd_field(), and the code is simpler IMHO.
>>>
>>> Any comments?
>>
>> 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)))
>>
>>
> 
> Thanks for the comment, but I don't think this meets for our purpose.
> 
> We want a build time "error" when the "MD_FIELD_ELE_SIZE(_field_id) == _size"
> fails, but not "still initializing the size to 0".

FWIW, it isn't 0, it is void.  Assignment to void is an error.  Could use
anything that is correct syntax but would produce a compile-time error
e.g. (1 / 0).

> Otherwise, we might get
> some unexpected issue (due to size is 0) at runtime, which is worse IMHO than
> a runtime check as done in the current upstream code.
> 
> I have been trying to add a BUILD_BUG_ON() to the field_mapping structure
> initializer, but I haven't found a reliable way to do so.
> 
> For now I have completed the new version based on Dan's suggestion, but still
> need to work on changelog/coverletter etc, so I think I can send the new
> version out and see whether people like it.  We can revert back if that's not
> what people want.