| Message ID | 20220815092638.504528-2-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | allow userspace to query device features | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 8/15/2022 2:26 AM, Zhu Lingshan wrote: > 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. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/vdpa.c | 17 +++++++++++++---- > include/uapi/linux/vdpa.h | 3 +++ > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index c06c02704461..efb55a06e961 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m > err = -EMSGSIZE; > goto msg_err; > } > + > + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */ > if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, > mdev->supported_features, VDPA_ATTR_PAD)) { > err = -EMSGSIZE; > @@ -815,7 +817,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 +834,19 @@ 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); > + > + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */ > + 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..d171b92ef522 100644 > --- a/include/uapi/linux/vdpa.h > +++ b/include/uapi/linux/vdpa.h > @@ -46,7 +46,10 @@ enum vdpa_attr { > > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > + /* features of a vDPA management device */ > VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ > + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ Append to the end, please. Otherwise it breaks userspace ABI. -Siwei > > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */
On 8/16/2022 2:15 AM, Si-Wei Liu wrote: > > > On 8/15/2022 2:26 AM, Zhu Lingshan wrote: >> 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. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 17 +++++++++++++---- >> include/uapi/linux/vdpa.h | 3 +++ >> 2 files changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >> index c06c02704461..efb55a06e961 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct >> vdpa_mgmt_dev *mdev, struct sk_buff *m >> err = -EMSGSIZE; >> goto msg_err; >> } >> + >> + /* report features of a vDPA management device through >> VDPA_ATTR_DEV_SUPPORTED_FEATURES */ >> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, >> mdev->supported_features, VDPA_ATTR_PAD)) { >> err = -EMSGSIZE; >> @@ -815,7 +817,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 +834,19 @@ 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); >> + >> + /* report features of a vDPA device through >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */ >> + 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..d171b92ef522 100644 >> --- a/include/uapi/linux/vdpa.h >> +++ b/include/uapi/linux/vdpa.h >> @@ -46,7 +46,10 @@ enum vdpa_attr { >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >> + /* features of a vDPA management device */ >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > Append to the end, please. Otherwise it breaks userspace ABI. OK, will fix it in V2 > > -Siwei > >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >
> From: Zhu, Lingshan <lingshan.zhu@intel.com> > Sent: Monday, August 15, 2022 9:49 PM > > On 8/16/2022 2:15 AM, Si-Wei Liu wrote: > > > > > > On 8/15/2022 2:26 AM, Zhu Lingshan wrote: > >> 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. > >> > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >> --- > >> drivers/vdpa/vdpa.c | 17 +++++++++++++---- > >> include/uapi/linux/vdpa.h | 3 +++ > >> 2 files changed, 16 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > >> c06c02704461..efb55a06e961 100644 > >> --- a/drivers/vdpa/vdpa.c > >> +++ b/drivers/vdpa/vdpa.c > >> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct > >> vdpa_mgmt_dev *mdev, struct sk_buff *m > >> err = -EMSGSIZE; > >> goto msg_err; > >> } > >> + > >> + /* report features of a vDPA management device through > >> VDPA_ATTR_DEV_SUPPORTED_FEATURES */ > >> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, > >> mdev->supported_features, VDPA_ATTR_PAD)) { > >> err = -EMSGSIZE; > >> @@ -815,7 +817,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 +834,19 @@ 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); > >> + > >> + /* report features of a vDPA device through > >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */ > >> + 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..d171b92ef522 100644 > >> --- a/include/uapi/linux/vdpa.h > >> +++ b/include/uapi/linux/vdpa.h > >> @@ -46,7 +46,10 @@ enum vdpa_attr { > >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ > >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ > >> + /* features of a vDPA management device */ Say comment as. /* Features of the vDPA management device matching the virtio feature bits 0 to 63 when queried on the mgmt. device. * When returned on the vdpa device, it indicates virtio feature bits 0 to 63 of the vdpa device */ > >> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ > >> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ > >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > > Append to the end, please. Otherwise it breaks userspace ABI. > OK, will fix it in V2 I have read Se-Wei comment in the v4. However I disagree, we don’t need to continue the past mistake done with the naming. Please add a comment that VDPA_ATTR_DEV_SUPPORTED_FEATURES like above about the exception. We established that there is no race condition either. So no need to add new UAPI for some past history.
On 8/16/2022 10:07 AM, Parav Pandit wrote: >> From: Zhu, Lingshan <lingshan.zhu@intel.com> >> Sent: Monday, August 15, 2022 9:49 PM >> >> On 8/16/2022 2:15 AM, Si-Wei Liu wrote: >>> >>> On 8/15/2022 2:26 AM, Zhu Lingshan wrote: >>>> 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. >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>> --- >>>> drivers/vdpa/vdpa.c | 17 +++++++++++++---- >>>> include/uapi/linux/vdpa.h | 3 +++ >>>> 2 files changed, 16 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>>> c06c02704461..efb55a06e961 100644 >>>> --- a/drivers/vdpa/vdpa.c >>>> +++ b/drivers/vdpa/vdpa.c >>>> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct >>>> vdpa_mgmt_dev *mdev, struct sk_buff *m >>>> err = -EMSGSIZE; >>>> goto msg_err; >>>> } >>>> + >>>> + /* report features of a vDPA management device through >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES */ >>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, >>>> mdev->supported_features, VDPA_ATTR_PAD)) { >>>> err = -EMSGSIZE; >>>> @@ -815,7 +817,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 +834,19 @@ 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); >>>> + >>>> + /* report features of a vDPA device through >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */ >>>> + 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..d171b92ef522 100644 >>>> --- a/include/uapi/linux/vdpa.h >>>> +++ b/include/uapi/linux/vdpa.h >>>> @@ -46,7 +46,10 @@ enum vdpa_attr { >>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ >>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ >>>> + /* features of a vDPA management device */ > Say comment as. > /* Features of the vDPA management device matching the virtio feature bits 0 to 63 when queried on the mgmt. device. > * When returned on the vdpa device, it indicates virtio feature bits 0 to 63 of the vdpa device > */ This still suggests to re-use the attr VDPA_ATTR_DEV_SUPPORTED_FEATURES, I think a new attr should be better and easier for others. > >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ >>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >>> Append to the end, please. Otherwise it breaks userspace ABI. >> OK, will fix it in V2 > I have read Se-Wei comment in the v4. > However I disagree, we don’t need to continue the past mistake done with the naming. > > Please add a comment that VDPA_ATTR_DEV_SUPPORTED_FEATURES like above about the exception. > We established that there is no race condition either. > So no need to add new UAPI for some past history. Yes, no race conditions, the thing is, it is not a good practice to re-use a netlink attr, every attr should has its own purpose, no need to confuse others, I think we had this discussion before. Thanks
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index c06c02704461..efb55a06e961 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m err = -EMSGSIZE; goto msg_err; } + + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */ if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, mdev->supported_features, VDPA_ATTR_PAD)) { err = -EMSGSIZE; @@ -815,7 +817,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 +834,19 @@ 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); + + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */ + 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..d171b92ef522 100644 --- a/include/uapi/linux/vdpa.h +++ b/include/uapi/linux/vdpa.h @@ -46,7 +46,10 @@ enum vdpa_attr { VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */ VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */ + /* features of a vDPA management device */ VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */ + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ + 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. Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 17 +++++++++++++---- include/uapi/linux/vdpa.h | 3 +++ 2 files changed, 16 insertions(+), 4 deletions(-)