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

[Jason Wang <jasowang@redhat.com>]

在 2021/6/28 下午6:56, Parav Pandit 写道:
>> 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.


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