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

[Jason Wang <jasowang@redhat.com>]

在 2021/6/23 下午12:22, Parav Pandit 写道:
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Wednesday, June 23, 2021 9:39 AM
>>
>> 在 2021/6/22 下午10:03, Parav Pandit 写道:
>>>> Is it better to use a separate enum for net specific attributes?
>>>>
>>> Yes, because they are only net specific.
>>> I guess it is related to your below question.
>>>
>>>> Another question (sorry if it has been asked before). Can we simply
>>>> return the config (binary) to the userspace, then usespace can use
>>>> the existing uAPI like virtio_net_config plus the feature to explain the
>> config?
>>> We did discuss in v2.
>>> Usually returning the whole blob and parsing is not desired via netlink.
>>> Returning individual fields give the full flexibility to return only the valid
>> fields.
>>> Otherwise we need to implement another bitmask too to tell which fields
>> from the struct are valid and share with user space.
>>> Returning individual fields is the widely used approach.
>>
>> The main concerns are:
>>
>> 1) The blob will be self contained if it was passed with the negotiated
>> features, so we don't need bitmask.
> Which fields of the struct are valid is told by additional fields.
>> 2) Using individual fields means it must duplicate the config fields of every
>> virtio devices
>>
> Mostly no. if there are common config fields across two device types, they would be named as
> VDPA_ATTR_DEV_CFG_*
> Net specific will be,
> VDPA_ATTR_DEV_NET_CFG_*
> Block specific, will be,
> VDPA_ATTR_DEV_BLK_CFG_*


I meant it looks like VDPA_ATTR_DEV_NET will duplicate all the fields of:

struct virtio_net_config;

And VDPA_ATTR_DEV_BLOCK will duplicate all the fields of

struct virtio_blk_config; which has ~21 fields.

And we had a plenty of other types of virtio devices.

Consider we had a mature set of virtio specific uAPI for config space. 
It would be a burden if we need an unnecessary translation layer of 
netlink in the middle:

[vDPA parent (virtio_net_config)] <-> [netlink (VDPA_ATTR_DEV_NET_XX)] 
<-> [userspace (VDPA_ATTR_DEV_NET_XX)] <-> [ user (virtio_net_config)]

If we make netlink simply a transport, it would be much easier. And we 
had the chance to unify the logic of build_config() and set_config() in 
the driver.


>
>> And actually, it's not the binary blob since uapi clearly define the format (e.g
>> struct virtio_net_config), can we find a way to use that? E.g introduce
>> device/net specific command and passing the blob with length and
>> negotiated features.
> Length may change in future, mostly expand. And parsing based on length is not such a clean way.


Length is only for legal checking. The config is self contained with:

1) device id
2) features


> Parsing fields require knowledge of features as well and application needs to make multiple netlink calls to parse the config space.


I think we don't care about the performance in this case. It's about 
three netlink calls:

1) get config
2) get device id
3) get features

For build config, it's only one

1) build config


> I prefer to follow rest of the kernel style to return self contained invidividual fields.


But I saw a lot of kernel codes choose to use e.g nla_put() directly 
with module specific structure.

Thanks


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