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

[Huang, Kai]

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@xxxxxxxxx/

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@xxxxxxxxxxxxxxxxxxxxxxxxx.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@xxxxxxxxx/

Please let me know your comment?