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

[Jason Wang <jasowang@redhat.com>]

在 2021/6/29 下午5:49, Parav Pandit 写道:
> Hi Jason,
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Tuesday, June 29, 2021 9:22 AM
>
>>>>> 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)
>>
> [..]
>
>> 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?
>>
>>
>>> 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];
>> };
> In this structure we need to add bi field flags to indicate which entry is valid.
> And when structure layout changes, we end up with similar typecast issues, length checks and more.
> Most of it is inbuild to the netlink.
>
> So I propose,
> (a) we pass config parameters during vdpa device create
> $ vdpa dev add name foo mgmtdev pci/0000:03:00.4 mac 00:11:22:33:44:55 maxq 10
>
> This results in adding two onenew netlink optional attributes as VDPA_DEV_NET_MAC.
> VDPA_ATTR_DEV_MAX_VQ_SIZE is already dfined for max queues.
> NLA_POLICY_ETH_ADDR takes care to validate length size when passed.
>
>> 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.
> Today it is only mac and max queues. Later on we may need to define rss hashing as hw/device advances.
> And structure size will change.
> Hence, I propose to have each as individual attribute that doesn’t need to cast in struct.


Ok, that should work. If Michael are fine with this, I'm also fine.

Just to clarify, if I understand this correctly, with the individual 
attribute, there's no need for the bit like xxx_is_valid?

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization