在 2021/6/28 下午6:56, Parav Pandit 写道:
From: Jason Wang <jasowang@xxxxxxxxxx>
Sent: Monday, June 28, 2021 10:33 AM
[..]
I don't see why it needs typecast, virtio_net_config is also uABI,
you can deference the fields directly.
User wants set only the mac address of the config space. How do user
space tell this?
Good question, but we need first answer:
"Do we allow userspace space to modify one specific field of all the config?"
Even if we restrict to specify config params at creation time, question still remains open how to pass, either as whole struct + side_based info or as individual fields.
More below.
Right.
Pass the whole virtio_net_config and inform via side channel?
That could be a method.
I prefer the method to pass individual fields which has the clean code approach and full flexibility.
Clean code =
1. no typecasting based on length
2. self-describing fields, do not depends on feature bits parsing
3. proof against structure size increases in fully backward/forward compatibility without code changes
So I think I agree. But I think we'd better to that in the virito uAPI
(include/uapi/linux/virito_xxx.h)
Or vendor driver is expected to compare what fields changed from old
config space?
So I think we need solve them all, but netlink is probably the wrong
layer, we need to solve them at virtio level and let netlink a transport
for them virtio uAPI/ABI.
In spirit of using the virtio UAPI structure, we creating other side band fields, that results into code that’s not common to netlink method.
I think maybe we can start from inventing new virtio uAPI and see if it
has some contradict with netlink. Or maybe you can give me some example?
Ioctl() interface of QEMU/vhost didn't have any other choice with ioctl().
And we need to figure out if we want to allow the userspace to modify
the config after the device is created. If not, simply build the
virtio_net_config and pass it to the vDPA parent during device creation.
I like this idea to pass fields at creation time.
If not, invent new uAPI at virtio level to passing the config fields.
Virtio or vDPA core can provide the library to compare the difference.
My feeling is that, if we restrict to only support build the config
during the creation, it would simply a lot of things. And I didn't
notice a use case that we need to change the config fields in the middle
via the management API/tool.
Sure yes. Whichever config fields user wants to pass, user space passes it.
For virito_net_config, why not simply:
len = ops->get_config_len();
config = kmalloc(len, GFP_KERNEL);
ops->get_config(vdev, 0, config, len);
nla_put(skb, VIRTIO_CONFIG, config, len);
User space need to parse content based on this length as it can change in
future.
Length telling how to typecast is want I want to avoid here.
So there's no real difference, using xxx_is_valid, is just a implicit
length checking as what is done via config_len:
if (a_is_valid) {
/* dump a */
} else if (b_is_valid) {
/* dump b */
}
vs.
if (length < offsetof(struct virtio_net_config, next field of a)) {
/* dump a*/
+ the feature parsing code, for each field.
}
Actually, Qemu has solved the similar issues via the uAPI:
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/net/virtio-
net.c;h=bd7958b9f0eed2705e0d6a2feaeaefb5e63bd6a4;hb=HEAD#l92
If the current uAPI is not sufficient, let's tweak it.
I am unable to convince my self to build side bitmask for config fields, type casting code in spirit of using existing structure UAPI.
This creates messy code for future.
Just a quick thought, how about simply something like:
struct virtio_net_config_build {
__u8 mac[ETH_ALEN];
__virtio16 max_virtqueue_pairs;
__virtio16 reserved[3];
};
It looks to we don't need the rest of fields in the virtio_net_config to
build the config since they are all hardware attributes.
So it looks self-contained and can be transported via netlink.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
