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

在 2021/6/23 下午12:22, Parav Pandit 写道:

quoted

From: Jason Wang <redacted>
Sent: Wednesday, June 23, 2021 9:39 AM

在 2021/6/22 下午10:03, Parav Pandit 写道:

quoted

quoted

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.

quoted

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?

quoted

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.

quoted

Otherwise we need to implement another bitmask too to tell which fields

from the struct are valid and share with user space.

quoted

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.

quoted

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.

quoted

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