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

> From: Jason Wang <jasowang at 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_*
> 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.
Parsing fields require knowledge of features as well and application needs to
make multiple netlink calls to parse the config space.
I prefer to follow rest of the kernel style to return self contained
invidividual fields.

? 2021/6/23 ??12:22, Parav Pandit ??:
>
>> From: Jason Wang <jasowang at 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