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?
