RE: [PATCH linux-next v3 2/6] vdpa: Introduce query of device config layout

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> 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.

> 
> > 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

> 
> 
> > 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.
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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization