Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

Huang, Kai
2024-09-26T22:22:52Z

On 27/09/2024 3:47 am, Hansen, Dave wrote:
> On 9/24/24 04:28, Kai Huang wrote:
>> +#define build_sysmd_read(_size) \
>> +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val) \
>> +{ \
>> + u64 tmp; \
>> + int ret; \
>> + \
>> + ret = tdh_sys_rd(field_id, &tmp); \
>> + if (ret) \
>> + return ret; \
>> + \
>> + *val = tmp; \
>> + \
>> + return 0; \
>> }
>
> Why? What's so important about having the compiler do the copy?
>
>
> #define read_sys_metadata_field(id, val) \
> __read_sys_metadata_field(id, val, sizeof (*(val)))
>
> static int __read_sys_metadata_field(u64 field_id, void *ptr, int size)
> {
> ...
> memcpy(ptr, &tmp, size);
>
> return 0;
> }
>
> There's one simple #define there so that users don't have to do the
> sizeof and can't screw it up.

Yes we can do this. This is basically what I did in the previous version:

https://lore.kernel.org/kvm/0403cdb142b40b9838feeb222eb75a4831f6b46d.1724741926.git.kai.huang@intel.com/

But Dan commented using typeless 'void *' and 'size' is kinda a step
backwards and we should do something similar to build_mmio_read():

https://lore.kernel.org/kvm/66db75497a213_22a2294b@dwillia2-xfh.jf.intel.com.notmuch/

Hi Dan,

I think what Dave suggested makes sense. If the concern is using
__read_sys_metadata_field() directly isn't typesafe, we can add a
comment to it saying callers should not use it directly and use
read_sys_metadata_field() instead.

Dave's approach also makes the LoC slightly shorter, and cleaner from
the perspective that we don't need to explicitly specify the '16/32/64'
in the READ_SYS_INFO() macro anymore as shown in here:

https://lore.kernel.org/kvm/79c256b8978310803bb4de48cd81dd373330cbc2.1727173372.git.kai.huang@intel.com/

Please let me know your comment?