| Message ID | 20220701132826.8132-4-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ifcvf/vDPA: support query device config space through netlink | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> From: Zhu Lingshan <lingshan.zhu@intel.com> > Sent: Friday, July 1, 2022 9:28 AM > > This commit adds a new vDPA netlink attribution > VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features of vDPA devices through this new attr. > > Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) Missing the "" in the line. I reviewed the patches again. However, this is not the fix. A fix cannot add a new UAPI. Code is already considering negotiated driver features to return the device config space. Hence it is fine. This patch intents to provide device features to user space. First what vdpa device are capable of, are already returned by features attribute on the management device. This is done in commit [1]. The only reason to have it is, when one management device indicates that feature is supported, but device may end up not supporting this feature if such feature is shared with other devices on same physical device. For example all VFs may not be symmetric after large number of them are in use. In such case features bit of management device can differ (more features) than the vdpa device of this VF. Hence, showing on the device is useful. As mentioned before in V2, commit [1] has wrongly named the attribute to VDPA_ATTR_DEV_SUPPORTED_FEATURES. It should have been, VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. Because it is in UAPI, and since we don't want to break compilation of iproute2, It cannot be renamed anymore. Given that, we do not want to start trend of naming device attributes with additional _VDPA_ to it as done in this patch. Error in commit [1] was exception. Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return for device features too. Secondly, you need output example for showing device features in the commit log. 3rd, please drop the fixes tag as new capability is not a fix. [1] cd2629f6df1c ("vdpa: Support reporting max device capabilities ") > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/vdpa.c | 13 +++++++++---- > include/uapi/linux/vdpa.h | 1 + > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > ebf2f363fbe7..9b0e39b2f022 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct > vdpa_device *vdev, static int vdpa_dev_net_config_fill(struct vdpa_device > *vdev, struct sk_buff *msg) { > struct virtio_net_config config = {}; > - u64 features; > + u64 features_device, features_driver; > u16 val_u16; > > vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - > 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device > *vdev, struct sk_buff *ms > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > return -EMSGSIZE; > > - features = vdev->config->get_driver_features(vdev); > - if (nla_put_u64_64bit(msg, > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features, > + features_driver = vdev->config->get_driver_features(vdev); > + if (nla_put_u64_64bit(msg, > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > + VDPA_ATTR_PAD)) > + return -EMSGSIZE; > + > + features_device = vdev->config->get_device_features(vdev); > + if (nla_put_u64_64bit(msg, > VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, > +features_device, > VDPA_ATTR_PAD)) > return -EMSGSIZE; > > - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); > + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > +&config); > } > > static int > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index > 25c55cab3d7c..39f1c3d7c112 100644 > --- a/include/uapi/linux/vdpa.h > +++ b/include/uapi/linux/vdpa.h > @@ -47,6 +47,7 @@ enum vdpa_attr { > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > -- > 2.31.1
在 2022/7/2 06:02, Parav Pandit 写道: > >> From: Zhu Lingshan <lingshan.zhu@intel.com> >> Sent: Friday, July 1, 2022 9:28 AM >> >> This commit adds a new vDPA netlink attribution >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features of vDPA devices through this new attr. >> >> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) > Missing the "" in the line. > I reviewed the patches again. > > However, this is not the fix. > A fix cannot add a new UAPI. > > Code is already considering negotiated driver features to return the device config space. > Hence it is fine. > > This patch intents to provide device features to user space. > First what vdpa device are capable of, are already returned by features attribute on the management device. > This is done in commit [1]. > > The only reason to have it is, when one management device indicates that feature is supported, but device may end up not supporting this feature if such feature is shared with other devices on same physical device. > For example all VFs may not be symmetric after large number of them are in use. In such case features bit of management device can differ (more features) than the vdpa device of this VF. > Hence, showing on the device is useful. > > As mentioned before in V2, commit [1] has wrongly named the attribute to VDPA_ATTR_DEV_SUPPORTED_FEATURES. > It should have been, VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. > Because it is in UAPI, and since we don't want to break compilation of iproute2, > It cannot be renamed anymore. > > Given that, we do not want to start trend of naming device attributes with additional _VDPA_ to it as done in this patch. > Error in commit [1] was exception. > > Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return for device features too. This will probably break or confuse the existing userspace? Thanks > > Secondly, you need output example for showing device features in the commit log. > > 3rd, please drop the fixes tag as new capability is not a fix. > > [1] cd2629f6df1c ("vdpa: Support reporting max device capabilities ") > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 13 +++++++++---- >> include/uapi/linux/vdpa.h | 1 + >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >> ebf2f363fbe7..9b0e39b2f022 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct >> vdpa_device *vdev, static int vdpa_dev_net_config_fill(struct vdpa_device >> *vdev, struct sk_buff *msg) { >> struct virtio_net_config config = {}; >> - u64 features; >> + u64 features_device, features_driver; >> u16 val_u16; >> >> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - >> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device >> *vdev, struct sk_buff *ms >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >> return -EMSGSIZE; >> >> - features = vdev->config->get_driver_features(vdev); >> - if (nla_put_u64_64bit(msg, >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features, >> + features_driver = vdev->config->get_driver_features(vdev); >> + if (nla_put_u64_64bit(msg, >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >> + VDPA_ATTR_PAD)) >> + return -EMSGSIZE; >> + >> + features_device = vdev->config->get_device_features(vdev); >> + if (nla_put_u64_64bit(msg, >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, >> +features_device, >> VDPA_ATTR_PAD)) >> return -EMSGSIZE; >> >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >> +&config); >> } >> >> static int >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index >> 25c55cab3d7c..39f1c3d7c112 100644 >> --- a/include/uapi/linux/vdpa.h >> +++ b/include/uapi/linux/vdpa.h >> @@ -47,6 +47,7 @@ enum vdpa_attr { >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >> >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >> -- >> 2.31.1
> From: Jason Wang <jasowang@redhat.com> > Sent: Monday, July 4, 2022 12:47 AM > > > 在 2022/7/2 06:02, Parav Pandit 写道: > > > >> From: Zhu Lingshan <lingshan.zhu@intel.com> > >> Sent: Friday, July 1, 2022 9:28 AM > >> > >> This commit adds a new vDPA netlink attribution > >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features > >> of vDPA devices through this new attr. > >> > >> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) > > Missing the "" in the line. > > I reviewed the patches again. > > > > However, this is not the fix. > > A fix cannot add a new UAPI. > > > > Code is already considering negotiated driver features to return the device > config space. > > Hence it is fine. > > > > This patch intents to provide device features to user space. > > First what vdpa device are capable of, are already returned by features > attribute on the management device. > > This is done in commit [1]. > > > > The only reason to have it is, when one management device indicates that > feature is supported, but device may end up not supporting this feature if > such feature is shared with other devices on same physical device. > > For example all VFs may not be symmetric after large number of them are > in use. In such case features bit of management device can differ (more > features) than the vdpa device of this VF. > > Hence, showing on the device is useful. > > > > As mentioned before in V2, commit [1] has wrongly named the attribute to > VDPA_ATTR_DEV_SUPPORTED_FEATURES. > > It should have been, > VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. > > Because it is in UAPI, and since we don't want to break compilation of > > iproute2, It cannot be renamed anymore. > > > > Given that, we do not want to start trend of naming device attributes with > additional _VDPA_ to it as done in this patch. > > Error in commit [1] was exception. > > > > Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return > for device features too. > > > This will probably break or confuse the existing userspace? > It shouldn't break, because its new attribute on the device. All attributes are per command, so old one will not be confused either.
On 7/4/2022 8:53 PM, Parav Pandit wrote: >> From: Jason Wang <jasowang@redhat.com> >> Sent: Monday, July 4, 2022 12:47 AM >> >> >> 在 2022/7/2 06:02, Parav Pandit 写道: >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>> Sent: Friday, July 1, 2022 9:28 AM >>>> >>>> This commit adds a new vDPA netlink attribution >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features >>>> of vDPA devices through this new attr. >>>> >>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) >>> Missing the "" in the line. >>> I reviewed the patches again. >>> >>> However, this is not the fix. >>> A fix cannot add a new UAPI. >>> >>> Code is already considering negotiated driver features to return the device >> config space. >>> Hence it is fine. >>> >>> This patch intents to provide device features to user space. >>> First what vdpa device are capable of, are already returned by features >> attribute on the management device. >>> This is done in commit [1]. >>> >>> The only reason to have it is, when one management device indicates that >> feature is supported, but device may end up not supporting this feature if >> such feature is shared with other devices on same physical device. >>> For example all VFs may not be symmetric after large number of them are >> in use. In such case features bit of management device can differ (more >> features) than the vdpa device of this VF. >>> Hence, showing on the device is useful. >>> >>> As mentioned before in V2, commit [1] has wrongly named the attribute to >> VDPA_ATTR_DEV_SUPPORTED_FEATURES. >>> It should have been, >> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. >>> Because it is in UAPI, and since we don't want to break compilation of >>> iproute2, It cannot be renamed anymore. >>> >>> Given that, we do not want to start trend of naming device attributes with >> additional _VDPA_ to it as done in this patch. >>> Error in commit [1] was exception. >>> >>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return >> for device features too. >> >> >> This will probably break or confuse the existing userspace? >> > It shouldn't break, because its new attribute on the device. > All attributes are per command, so old one will not be confused either. A netlink attr should has its own and unique purpose, that's why we don't need locks for the attrs, only one consumer and only one producer. I am afraid re-using (for both management device and the vDPA device) the attr VDPA_ATTR_DEV_SUPPORTED_FEATURES would lead to new race condition. E.g., There are possibilities of querying FEATURES of a management device and a vDPA device simultaneously, or can there be a syncing issue in a tick? IMHO, I don't see any advantages of re-using this attr. Thanks, Zhu Lingshan
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Tuesday, July 5, 2022 3:59 AM > > > On 7/4/2022 8:53 PM, Parav Pandit wrote: > >> From: Jason Wang <jasowang@redhat.com> > >> Sent: Monday, July 4, 2022 12:47 AM > >> > >> > >> 在 2022/7/2 06:02, Parav Pandit 写道: > >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> > >>>> Sent: Friday, July 1, 2022 9:28 AM > >>>> > >>>> This commit adds a new vDPA netlink attribution > >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > >> features > >>>> of vDPA devices through this new attr. > >>>> > >>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver > >>>> feature) > >>> Missing the "" in the line. > >>> I reviewed the patches again. > >>> > >>> However, this is not the fix. > >>> A fix cannot add a new UAPI. > >>> > >>> Code is already considering negotiated driver features to return the > >>> device > >> config space. > >>> Hence it is fine. > >>> > >>> This patch intents to provide device features to user space. > >>> First what vdpa device are capable of, are already returned by > >>> features > >> attribute on the management device. > >>> This is done in commit [1]. > >>> > >>> The only reason to have it is, when one management device indicates > >>> that > >> feature is supported, but device may end up not supporting this > >> feature if such feature is shared with other devices on same physical device. > >>> For example all VFs may not be symmetric after large number of them > >>> are > >> in use. In such case features bit of management device can differ > >> (more > >> features) than the vdpa device of this VF. > >>> Hence, showing on the device is useful. > >>> > >>> As mentioned before in V2, commit [1] has wrongly named the > >>> attribute to > >> VDPA_ATTR_DEV_SUPPORTED_FEATURES. > >>> It should have been, > >> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. > >>> Because it is in UAPI, and since we don't want to break compilation > >>> of iproute2, It cannot be renamed anymore. > >>> > >>> Given that, we do not want to start trend of naming device > >>> attributes with > >> additional _VDPA_ to it as done in this patch. > >>> Error in commit [1] was exception. > >>> > >>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return > >> for device features too. > >> > >> > >> This will probably break or confuse the existing userspace? > >> > > It shouldn't break, because its new attribute on the device. > > All attributes are per command, so old one will not be confused either. > A netlink attr should has its own and unique purpose, that's why we don't need > locks for the attrs, only one consumer and only one producer. > I am afraid re-using (for both management device and the vDPA device) the attr > VDPA_ATTR_DEV_SUPPORTED_FEATURES would lead to new race condition. > E.g., There are possibilities of querying FEATURES of a management device and > a vDPA device simultaneously, or can there be a syncing issue in a tick? Both can be queried simultaneously. Each will return their own feature bits using same attribute. It wont lead to the race. > > IMHO, I don't see any advantages of re-using this attr. We don’t want to continue this mess of VDPA_DEV prefix for new attributes due to previous wrong naming.
On 7/5/2022 7:56 PM, Parav Pandit wrote: >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Tuesday, July 5, 2022 3:59 AM >> >> >> On 7/4/2022 8:53 PM, Parav Pandit wrote: >>>> From: Jason Wang <jasowang@redhat.com> >>>> Sent: Monday, July 4, 2022 12:47 AM >>>> >>>> >>>> 在 2022/7/2 06:02, Parav Pandit 写道: >>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> Sent: Friday, July 1, 2022 9:28 AM >>>>>> >>>>>> This commit adds a new vDPA netlink attribution >>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >>>> features >>>>>> of vDPA devices through this new attr. >>>>>> >>>>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver >>>>>> feature) >>>>> Missing the "" in the line. >>>>> I reviewed the patches again. >>>>> >>>>> However, this is not the fix. >>>>> A fix cannot add a new UAPI. >>>>> >>>>> Code is already considering negotiated driver features to return the >>>>> device >>>> config space. >>>>> Hence it is fine. >>>>> >>>>> This patch intents to provide device features to user space. >>>>> First what vdpa device are capable of, are already returned by >>>>> features >>>> attribute on the management device. >>>>> This is done in commit [1]. >>>>> >>>>> The only reason to have it is, when one management device indicates >>>>> that >>>> feature is supported, but device may end up not supporting this >>>> feature if such feature is shared with other devices on same physical device. >>>>> For example all VFs may not be symmetric after large number of them >>>>> are >>>> in use. In such case features bit of management device can differ >>>> (more >>>> features) than the vdpa device of this VF. >>>>> Hence, showing on the device is useful. >>>>> >>>>> As mentioned before in V2, commit [1] has wrongly named the >>>>> attribute to >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES. >>>>> It should have been, >>>> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. >>>>> Because it is in UAPI, and since we don't want to break compilation >>>>> of iproute2, It cannot be renamed anymore. >>>>> >>>>> Given that, we do not want to start trend of naming device >>>>> attributes with >>>> additional _VDPA_ to it as done in this patch. >>>>> Error in commit [1] was exception. >>>>> >>>>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return >>>> for device features too. >>>> >>>> >>>> This will probably break or confuse the existing userspace? >>>> >>> It shouldn't break, because its new attribute on the device. >>> All attributes are per command, so old one will not be confused either. >> A netlink attr should has its own and unique purpose, that's why we don't need >> locks for the attrs, only one consumer and only one producer. >> I am afraid re-using (for both management device and the vDPA device) the attr >> VDPA_ATTR_DEV_SUPPORTED_FEATURES would lead to new race condition. >> E.g., There are possibilities of querying FEATURES of a management device and >> a vDPA device simultaneously, or can there be a syncing issue in a tick? > Both can be queried simultaneously. Each will return their own feature bits using same attribute. > It wont lead to the race. How? It is just a piece of memory, xxxx[attr], do you see locks in nla_put_u64_64bit()? It is a typical race condition, data accessed by multiple producers / consumers. And re-use a netlink attr is really confusing. > >> IMHO, I don't see any advantages of re-using this attr. > We don’t want to continue this mess of VDPA_DEV prefix for new attributes due to previous wrong naming. as you point out before, is is a wrong naming, we can't re-nmme it because we don't want to break uAPI, so there needs a new attr, if you don't like the name VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, it is more than welcome to suggest a new one Thanks
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Tuesday, July 5, 2022 12:56 PM > > Both can be queried simultaneously. Each will return their own feature bits > using same attribute. > > It wont lead to the race. > How? It is just a piece of memory, xxxx[attr], do you see locks in > nla_put_u64_64bit()? It is a typical race condition, data accessed by multiple > producers / consumers. No. There is no race condition in here. And new attribute enum by no means avoid any race. Data put using nla_put cannot be accessed until they are transferred. > And re-use a netlink attr is really confusing. Please put comment for this variable explaining why it is shared for the exception. Before that lets start, can you share a real world example of when this feature bitmap will have different value than the mgmt. device bitmap value? > > > >> IMHO, I don't see any advantages of re-using this attr. > > We don’t want to continue this mess of VDPA_DEV prefix for new > attributes due to previous wrong naming. > as you point out before, is is a wrong naming, we can't re-nmme it because > we don't want to break uAPI, so there needs a new attr, if you don't like the > name VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, it is more than > welcome to suggest a new one > > Thanks
On 7/6/2022 1:01 AM, Parav Pandit wrote: >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Tuesday, July 5, 2022 12:56 PM >>> Both can be queried simultaneously. Each will return their own feature bits >> using same attribute. >>> It wont lead to the race. >> How? It is just a piece of memory, xxxx[attr], do you see locks in >> nla_put_u64_64bit()? It is a typical race condition, data accessed by multiple >> producers / consumers. > No. There is no race condition in here. > And new attribute enum by no means avoid any race. > > Data put using nla_put cannot be accessed until they are transferred. How this is guaranteed? Do you see errors when calling nla_put_xxx() twice? > >> And re-use a netlink attr is really confusing. > Please put comment for this variable explaining why it is shared for the exception. > > Before that lets start, can you share a real world example of when this feature bitmap will have different value than the mgmt. device bitmap value? For example, 1. When migrate the VM to a node which has a more resourceful device. If the source side device does not have MQ, RSS or TSO feature, the vDPA device assigned to the VM does not have MQ, RSS or TSO as well. When migrating to a node which has a device with MQ, RSS or TSO, to provide a consistent network device to the guest, to be transparent to the guest, we need to mask out MQ, RSS or TSO in the vDPA device when provisioning. This is an example that management device may have different feature bits than the vDPA device. 2.SIOV, if a virtio device is capable of managing SIOV devices, and it exposes this capability by a feature bit(Like what I am doing in the "transport virtqueue"), we don't want the SIOV ADIs have SIOV features, so the ADIs don't have SIOV feature bit. Thanks > >>>> IMHO, I don't see any advantages of re-using this attr. >>> We don’t want to continue this mess of VDPA_DEV prefix for new >> attributes due to previous wrong naming. >> as you point out before, is is a wrong naming, we can't re-nmme it because >> we don't want to break uAPI, so there needs a new attr, if you don't like the >> name VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, it is more than >> welcome to suggest a new one >> >> Thanks
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Tuesday, July 5, 2022 10:25 PM > 1. When migrate the VM to a node which has a more resourceful device. If > the source side device does not have MQ, RSS or TSO feature, the vDPA > device assigned to the VM does not have MQ, RSS or TSO as well. When > migrating to a node which has a device with MQ, RSS or TSO, to provide a > consistent network device to the guest, to be transparent to the guest, we > need to mask out MQ, RSS or TSO in the vDPA device when provisioning. > This is an example that management device may have different feature bits > than the vDPA device. Yes. Right.
On 7/2/2022 6:02 AM, Parav Pandit wrote: > >> From: Zhu Lingshan <lingshan.zhu@intel.com> >> Sent: Friday, July 1, 2022 9:28 AM >> >> This commit adds a new vDPA netlink attribution >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features of vDPA devices through this new attr. >> >> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) > Missing the "" in the line. will fix > I reviewed the patches again. > > However, this is not the fix. > A fix cannot add a new UAPI. I think we have discussed this, on why we can not re-name the existing wrong named attr, and why we can not re-use the attr. So are you suggesting remove this fixes tag? And why a fix can not add a new uAPI? > > Code is already considering negotiated driver features to return the device config space. > Hence it is fine. No, the spec says: The device MUST allow reading of any device-specific configuration field before FEATURES_OK is set by the driver. > > This patch intents to provide device features to user space. > First what vdpa device are capable of, are already returned by features attribute on the management device. > This is done in commit [1]. we have discussed this in another thread, vDPA device feature bits can be different from the management device feature bits. > > The only reason to have it is, when one management device indicates that feature is supported, but device may end up not supporting this feature if such feature is shared with other devices on same physical device. > For example all VFs may not be symmetric after large number of them are in use. In such case features bit of management device can differ (more features) than the vdpa device of this VF. > Hence, showing on the device is useful. > > As mentioned before in V2, commit [1] has wrongly named the attribute to VDPA_ATTR_DEV_SUPPORTED_FEATURES. > It should have been, VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. > Because it is in UAPI, and since we don't want to break compilation of iproute2, > It cannot be renamed anymore. Yes, rename it will break current uAPI, so I can not rename it. > > Given that, we do not want to start trend of naming device attributes with additional _VDPA_ to it as done in this patch. > Error in commit [1] was exception. > > Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return for device features too. > > Secondly, you need output example for showing device features in the commit log. > > 3rd, please drop the fixes tag as new capability is not a fix. > > [1] cd2629f6df1c ("vdpa: Support reporting max device capabilities ") > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 13 +++++++++---- >> include/uapi/linux/vdpa.h | 1 + >> 2 files changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >> ebf2f363fbe7..9b0e39b2f022 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct >> vdpa_device *vdev, static int vdpa_dev_net_config_fill(struct vdpa_device >> *vdev, struct sk_buff *msg) { >> struct virtio_net_config config = {}; >> - u64 features; >> + u64 features_device, features_driver; >> u16 val_u16; >> >> vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ - >> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device >> *vdev, struct sk_buff *ms >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >> return -EMSGSIZE; >> >> - features = vdev->config->get_driver_features(vdev); >> - if (nla_put_u64_64bit(msg, >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features, >> + features_driver = vdev->config->get_driver_features(vdev); >> + if (nla_put_u64_64bit(msg, >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >> + VDPA_ATTR_PAD)) >> + return -EMSGSIZE; >> + >> + features_device = vdev->config->get_device_features(vdev); >> + if (nla_put_u64_64bit(msg, >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, >> +features_device, >> VDPA_ATTR_PAD)) >> return -EMSGSIZE; >> >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >> +&config); >> } >> >> static int >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index >> 25c55cab3d7c..39f1c3d7c112 100644 >> --- a/include/uapi/linux/vdpa.h >> +++ b/include/uapi/linux/vdpa.h >> @@ -47,6 +47,7 @@ enum vdpa_attr { >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >> >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >> -- >> 2.31.1
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Friday, July 8, 2022 2:16 AM > > On 7/2/2022 6:02 AM, Parav Pandit wrote: > > > >> From: Zhu Lingshan <lingshan.zhu@intel.com> > >> Sent: Friday, July 1, 2022 9:28 AM > >> > >> This commit adds a new vDPA netlink attribution > >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query > features > >> of vDPA devices through this new attr. > >> > >> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) > > Missing the "" in the line. > will fix > > I reviewed the patches again. > > > > However, this is not the fix. > > A fix cannot add a new UAPI. > I think we have discussed this, on why we can not re-name the existing > wrong named attr, and why we can not re-use the attr. > So are you suggesting remove this fixes tag? > And why a fix can not add a new uAPI? Because a new attribute cannot fix any existing attribute. What is done in the patch is show current attributes of the vdpa device (which sometimes contains a different value than the mgmt. device). So it is a new functionality that cannot have fixes tag. > > > > Code is already considering negotiated driver features to return the device > config space. > > Hence it is fine. > No, the spec says: > The device MUST allow reading of any device-specific configuration field > before FEATURES_OK is set by the driver. > > > > This patch intents to provide device features to user space. > > First what vdpa device are capable of, are already returned by features > attribute on the management device. > > This is done in commit [1]. > we have discussed this in another thread, vDPA device feature bits can be > different from the management device feature bits. > > Yes. > > The only reason to have it is, when one management device indicates that > feature is supported, but device may end up not supporting this feature if > such feature is shared with other devices on same physical device. > > For example all VFs may not be symmetric after large number of them are > in use. In such case features bit of management device can differ (more > features) than the vdpa device of this VF. > > Hence, showing on the device is useful. > > > > As mentioned before in V2, commit [1] has wrongly named the attribute to > VDPA_ATTR_DEV_SUPPORTED_FEATURES. > > It should have been, > VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. > > Because it is in UAPI, and since we don't want to break compilation of > > iproute2, It cannot be renamed anymore. > Yes, rename it will break current uAPI, so I can not rename it. > > I know, which is why this patch needs to do following listed changes described in previous email. > > Given that, we do not want to start trend of naming device attributes with > additional _VDPA_ to it as done in this patch. > > Error in commit [1] was exception. > > > > Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return > for device features too. > > > > Secondly, you need output example for showing device features in the > commit log. > > > > 3rd, please drop the fixes tag as new capability is not a fix. > >
On 7/9/2022 12:13 AM, Parav Pandit wrote: > >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Friday, July 8, 2022 2:16 AM >> >> On 7/2/2022 6:02 AM, Parav Pandit wrote: >>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>> Sent: Friday, July 1, 2022 9:28 AM >>>> >>>> This commit adds a new vDPA netlink attribution >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >> features >>>> of vDPA devices through this new attr. >>>> >>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) >>> Missing the "" in the line. >> will fix >>> I reviewed the patches again. >>> >>> However, this is not the fix. >>> A fix cannot add a new UAPI. >> I think we have discussed this, on why we can not re-name the existing >> wrong named attr, and why we can not re-use the attr. >> So are you suggesting remove this fixes tag? >> And why a fix can not add a new uAPI? > Because a new attribute cannot fix any existing attribute. > > What is done in the patch is show current attributes of the vdpa device (which sometimes contains a different value than the mgmt. device). > So it is a new functionality that cannot have fixes tag. OK, I get the points now. > >>> Code is already considering negotiated driver features to return the device >> config space. >>> Hence it is fine. >> No, the spec says: >> The device MUST allow reading of any device-specific configuration field >> before FEATURES_OK is set by the driver. >>> This patch intents to provide device features to user space. >>> First what vdpa device are capable of, are already returned by features >> attribute on the management device. >>> This is done in commit [1]. >> we have discussed this in another thread, vDPA device feature bits can be >> different from the management device feature bits. > Yes. >>> The only reason to have it is, when one management device indicates that >> feature is supported, but device may end up not supporting this feature if >> such feature is shared with other devices on same physical device. >>> For example all VFs may not be symmetric after large number of them are >> in use. In such case features bit of management device can differ (more >> features) than the vdpa device of this VF. >>> Hence, showing on the device is useful. >>> >>> As mentioned before in V2, commit [1] has wrongly named the attribute to >> VDPA_ATTR_DEV_SUPPORTED_FEATURES. >>> It should have been, >> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. >>> Because it is in UAPI, and since we don't want to break compilation of >>> iproute2, It cannot be renamed anymore. >> Yes, rename it will break current uAPI, so I can not rename it. > I know, which is why this patch needs to do following listed changes described in previous email. > >>> Given that, we do not want to start trend of naming device attributes with >> additional _VDPA_ to it as done in this patch. >>> Error in commit [1] was exception. >>> >>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return >> for device features too. >>> Secondly, you need output example for showing device features in the >> commit log. >>> 3rd, please drop the fixes tag as new capability is not a fix. >>>
On 7/6/2022 10:25 AM, Zhu, Lingshan wrote: > > > On 7/6/2022 1:01 AM, Parav Pandit wrote: >>> From: Zhu, Lingshan <lingshan.zhu@intel.com> >>> Sent: Tuesday, July 5, 2022 12:56 PM >>>> Both can be queried simultaneously. Each will return their own >>>> feature bits >>> using same attribute. >>>> It wont lead to the race. >>> How? It is just a piece of memory, xxxx[attr], do you see locks in >>> nla_put_u64_64bit()? It is a typical race condition, data accessed >>> by multiple >>> producers / consumers. >> No. There is no race condition in here. >> And new attribute enum by no means avoid any race. >> >> Data put using nla_put cannot be accessed until they are transferred. > How this is guaranteed? Do you see errors when calling nla_put_xxx() > twice? Parav, did you miss this? >> >>> And re-use a netlink attr is really confusing. >> Please put comment for this variable explaining why it is shared for >> the exception. >> >> Before that lets start, can you share a real world example of when >> this feature bitmap will have different value than the mgmt. device >> bitmap value? > For example, > 1. When migrate the VM to a node which has a more resourceful device. > If the source side device does not have MQ, RSS or TSO feature, the > vDPA device assigned to the VM does not > have MQ, RSS or TSO as well. When migrating to a node which has a > device with MQ, RSS or TSO, to provide a consistent network device to > the guest, to be transparent to the guest, > we need to mask out MQ, RSS or TSO in the vDPA device when > provisioning. This is an example that management device may have > different feature bits than the vDPA device. > > 2.SIOV, if a virtio device is capable of managing SIOV devices, and it > exposes this capability by a feature bit(Like what I am doing in the > "transport virtqueue"), > we don't want the SIOV ADIs have SIOV features, so the ADIs don't have > SIOV feature bit. > > Thanks >> >>>>> IMHO, I don't see any advantages of re-using this attr. >>>> We don’t want to continue this mess of VDPA_DEV prefix for new >>> attributes due to previous wrong naming. >>> as you point out before, is is a wrong naming, we can't re-nmme it >>> because >>> we don't want to break uAPI, so there needs a new attr, if you don't >>> like the >>> name VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, it is more than >>> welcome to suggest a new one >>> >>> Thanks >
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Saturday, July 23, 2022 7:27 AM > > On 7/6/2022 10:25 AM, Zhu, Lingshan wrote: > > > > > > On 7/6/2022 1:01 AM, Parav Pandit wrote: > >>> From: Zhu, Lingshan <lingshan.zhu@intel.com> > >>> Sent: Tuesday, July 5, 2022 12:56 PM > >>>> Both can be queried simultaneously. Each will return their own > >>>> feature bits > >>> using same attribute. > >>>> It wont lead to the race. > >>> How? It is just a piece of memory, xxxx[attr], do you see locks in > >>> nla_put_u64_64bit()? It is a typical race condition, data accessed > >>> by multiple producers / consumers. > >> No. There is no race condition in here. > >> And new attribute enum by no means avoid any race. > >> > >> Data put using nla_put cannot be accessed until they are transferred. > > How this is guaranteed? Do you see errors when calling nla_put_xxx() > > twice? > Parav, did you miss this? It is not called twice and reading attribute and packing in nla message is not race condition.
On 7/5/2022 4:56 AM, Parav Pandit via Virtualization wrote: >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Tuesday, July 5, 2022 3:59 AM >> >> >> On 7/4/2022 8:53 PM, Parav Pandit wrote: >>>> From: Jason Wang <jasowang@redhat.com> >>>> Sent: Monday, July 4, 2022 12:47 AM >>>> >>>> >>>> 在 2022/7/2 06:02, Parav Pandit 写道: >>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> Sent: Friday, July 1, 2022 9:28 AM >>>>>> >>>>>> This commit adds a new vDPA netlink attribution >>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >>>> features >>>>>> of vDPA devices through this new attr. >>>>>> >>>>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver >>>>>> feature) >>>>> Missing the "" in the line. >>>>> I reviewed the patches again. >>>>> >>>>> However, this is not the fix. >>>>> A fix cannot add a new UAPI. >>>>> >>>>> Code is already considering negotiated driver features to return the >>>>> device >>>> config space. >>>>> Hence it is fine. >>>>> >>>>> This patch intents to provide device features to user space. >>>>> First what vdpa device are capable of, are already returned by >>>>> features >>>> attribute on the management device. >>>>> This is done in commit [1]. >>>>> >>>>> The only reason to have it is, when one management device indicates >>>>> that >>>> feature is supported, but device may end up not supporting this >>>> feature if such feature is shared with other devices on same physical device. >>>>> For example all VFs may not be symmetric after large number of them >>>>> are >>>> in use. In such case features bit of management device can differ >>>> (more >>>> features) than the vdpa device of this VF. >>>>> Hence, showing on the device is useful. >>>>> >>>>> As mentioned before in V2, commit [1] has wrongly named the >>>>> attribute to >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES. >>>>> It should have been, >>>> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. >>>>> Because it is in UAPI, and since we don't want to break compilation >>>>> of iproute2, It cannot be renamed anymore. >>>>> >>>>> Given that, we do not want to start trend of naming device >>>>> attributes with >>>> additional _VDPA_ to it as done in this patch. >>>>> Error in commit [1] was exception. >>>>> >>>>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return >>>> for device features too. >>>> >>>> >>>> This will probably break or confuse the existing userspace? >>>> >>> It shouldn't break, because its new attribute on the device. >>> All attributes are per command, so old one will not be confused either. >> A netlink attr should has its own and unique purpose, that's why we don't need >> locks for the attrs, only one consumer and only one producer. >> I am afraid re-using (for both management device and the vDPA device) the attr >> VDPA_ATTR_DEV_SUPPORTED_FEATURES would lead to new race condition. >> E.g., There are possibilities of querying FEATURES of a management device and >> a vDPA device simultaneously, or can there be a syncing issue in a tick? > Both can be queried simultaneously. Each will return their own feature bits using same attribute. > It wont lead to the race. Agreed. Multiple userspace callers would do recv() calls on different netlink sockets. Looks to me shouldn't involve any race. > >> IMHO, I don't see any advantages of re-using this attr. > We don’t want to continue this mess of VDPA_DEV prefix for new attributes due to previous wrong naming. Well, you can say it's a mess but since the attr name can be reused for different command, I didn't care that much while reviewing this. Actually, it was initially named this way to show the device features in "vdpa dev config ..." output, but later on it had been moved to mgmtdev to show parent's capability. -Siwei > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On 7/27/2022 4:15 PM, Si-Wei Liu wrote: > > > On 7/5/2022 4:56 AM, Parav Pandit via Virtualization wrote: >>> From: Zhu, Lingshan <lingshan.zhu@intel.com> >>> Sent: Tuesday, July 5, 2022 3:59 AM >>> >>> >>> On 7/4/2022 8:53 PM, Parav Pandit wrote: >>>>> From: Jason Wang <jasowang@redhat.com> >>>>> Sent: Monday, July 4, 2022 12:47 AM >>>>> >>>>> >>>>> 在 2022/7/2 06:02, Parav Pandit 写道: >>>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>>> Sent: Friday, July 1, 2022 9:28 AM >>>>>>> >>>>>>> This commit adds a new vDPA netlink attribution >>>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query >>>>> features >>>>>>> of vDPA devices through this new attr. >>>>>>> >>>>>>> Fixes: a64917bc2e9b vdpa: (Provide interface to read driver >>>>>>> feature) >>>>>> Missing the "" in the line. >>>>>> I reviewed the patches again. >>>>>> >>>>>> However, this is not the fix. >>>>>> A fix cannot add a new UAPI. >>>>>> >>>>>> Code is already considering negotiated driver features to return the >>>>>> device >>>>> config space. >>>>>> Hence it is fine. >>>>>> >>>>>> This patch intents to provide device features to user space. >>>>>> First what vdpa device are capable of, are already returned by >>>>>> features >>>>> attribute on the management device. >>>>>> This is done in commit [1]. >>>>>> >>>>>> The only reason to have it is, when one management device indicates >>>>>> that >>>>> feature is supported, but device may end up not supporting this >>>>> feature if such feature is shared with other devices on same >>>>> physical device. >>>>>> For example all VFs may not be symmetric after large number of them >>>>>> are >>>>> in use. In such case features bit of management device can differ >>>>> (more >>>>> features) than the vdpa device of this VF. >>>>>> Hence, showing on the device is useful. >>>>>> >>>>>> As mentioned before in V2, commit [1] has wrongly named the >>>>>> attribute to >>>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES. >>>>>> It should have been, >>>>> VDPA_ATTR_DEV_MGMTDEV_SUPPORTED_FEATURES. >>>>>> Because it is in UAPI, and since we don't want to break compilation >>>>>> of iproute2, It cannot be renamed anymore. >>>>>> >>>>>> Given that, we do not want to start trend of naming device >>>>>> attributes with >>>>> additional _VDPA_ to it as done in this patch. >>>>>> Error in commit [1] was exception. >>>>>> >>>>>> Hence, please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to return >>>>> for device features too. >>>>> >>>>> >>>>> This will probably break or confuse the existing userspace? >>>>> >>>> It shouldn't break, because its new attribute on the device. >>>> All attributes are per command, so old one will not be confused >>>> either. >>> A netlink attr should has its own and unique purpose, that's why we >>> don't need >>> locks for the attrs, only one consumer and only one producer. >>> I am afraid re-using (for both management device and the vDPA >>> device) the attr >>> VDPA_ATTR_DEV_SUPPORTED_FEATURES would lead to new race condition. >>> E.g., There are possibilities of querying FEATURES of a management >>> device and >>> a vDPA device simultaneously, or can there be a syncing issue in a >>> tick? >> Both can be queried simultaneously. Each will return their own >> feature bits using same attribute. >> It wont lead to the race. > Agreed. Multiple userspace callers would do recv() calls on different > netlink sockets. Looks to me shouldn't involve any race. oh yes, thanks for pointing this out, they are on different sockets belonging to different userspace programs. >> >>> IMHO, I don't see any advantages of re-using this attr. >> We don’t want to continue this mess of VDPA_DEV prefix for new >> attributes due to previous wrong naming. > Well, you can say it's a mess but since the attr name can be reused > for different command, I didn't care that much while reviewing this. > Actually, it was initially named this way to show the device features > in "vdpa dev config ..." output, but later on it had been moved to > mgmtdev to show parent's capability. yes there is a buggy commit, but we can not change it now, because we are not expected to break current uapi, so I think it is better to add a new attr, no benefits to reuse another attr. > > -Siwei >> _______________________________________________ >> Virtualization mailing list >> Virtualization@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index ebf2f363fbe7..9b0e39b2f022 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev, static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg) { struct virtio_net_config config = {}; - u64 features; + u64 features_device, features_driver; u16 val_u16; vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) return -EMSGSIZE; - features = vdev->config->get_driver_features(vdev); - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features, + features_driver = vdev->config->get_driver_features(vdev); + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, + VDPA_ATTR_PAD)) + return -EMSGSIZE; + + features_device = vdev->config->get_device_features(vdev); + if (nla_put_u64_64bit(msg, VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device, VDPA_ATTR_PAD)) return -EMSGSIZE; - return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config); + return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config); } static int diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index 25c55cab3d7c..39f1c3d7c112 100644 --- a/include/uapi/linux/vdpa.h +++ b/include/uapi/linux/vdpa.h @@ -47,6 +47,7 @@ enum vdpa_attr { VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
This commit adds a new vDPA netlink attribution VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query features of vDPA devices through this new attr. Fixes: a64917bc2e9b vdpa: (Provide interface to read driver feature) Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 13 +++++++++---- include/uapi/linux/vdpa.h | 1 + 2 files changed, 10 insertions(+), 4 deletions(-)