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

["Huang, Kai" <kai.huang@intel.com>]

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?