| Message ID | 20220909085712.46006-2-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Conditionally read fields in dev cfg space | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> 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. > > This commit invokes vdpa_config_ops.get_config() than > vdpa_get_config_unlocked() to read the device config > spcae, so no raeces in vdpa_set_features_unlocked() > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> It's better to share the userspace code as well. > --- > drivers/vdpa/vdpa.c | 19 ++++++++++++++----- > include/uapi/linux/vdpa.h | 4 ++++ > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index c06c02704461..798a02c7aa94 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 */ The code explains itself, there's no need for the comment. > if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, > mdev->supported_features, VDPA_ATTR_PAD)) { > err = -EMSGSIZE; > @@ -815,10 +817,10 @@ 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)); > + vdev->config->get_config(vdev, 0, &config, sizeof(config)); > > if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), > config.mac)) > @@ -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..97531b52dcbe 100644 > --- a/include/uapi/linux/vdpa.h > +++ b/include/uapi/linux/vdpa.h > @@ -46,12 +46,16 @@ 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 */ > > VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ > > + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ > + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES? Thanks > + > /* new attributes must be added above here */ > VDPA_ATTR_MAX, > }; > -- > 2.31.1 >
On 9/20/2022 10:02 AM, Jason Wang wrote: > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> 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. >> >> This commit invokes vdpa_config_ops.get_config() than >> vdpa_get_config_unlocked() to read the device config >> spcae, so no raeces in vdpa_set_features_unlocked() >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > It's better to share the userspace code as well. OK, will share it in V2. > >> --- >> drivers/vdpa/vdpa.c | 19 ++++++++++++++----- >> include/uapi/linux/vdpa.h | 4 ++++ >> 2 files changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >> index c06c02704461..798a02c7aa94 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 */ > The code explains itself, there's no need for the comment. these comments are required in other discussions > >> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, >> mdev->supported_features, VDPA_ATTR_PAD)) { >> err = -EMSGSIZE; >> @@ -815,10 +817,10 @@ 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)); >> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); >> >> if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), >> config.mac)) >> @@ -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..97531b52dcbe 100644 >> --- a/include/uapi/linux/vdpa.h >> +++ b/include/uapi/linux/vdpa.h >> @@ -46,12 +46,16 @@ 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 */ >> >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >> VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ >> >> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES? This is to report a vDPA device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES is used for reporting the management device features, we have a long discussion on this before. > > Thanks > >> + >> /* new attributes must be added above here */ >> VDPA_ATTR_MAX, >> }; >> -- >> 2.31.1 >>
On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: > > > > On 9/20/2022 10:02 AM, Jason Wang wrote: > > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> 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. > >> > >> This commit invokes vdpa_config_ops.get_config() than > >> vdpa_get_config_unlocked() to read the device config > >> spcae, so no raeces in vdpa_set_features_unlocked() > >> > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > > It's better to share the userspace code as well. > OK, will share it in V2. > > > >> --- > >> drivers/vdpa/vdpa.c | 19 ++++++++++++++----- > >> include/uapi/linux/vdpa.h | 4 ++++ > >> 2 files changed, 18 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > >> index c06c02704461..798a02c7aa94 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 */ > > The code explains itself, there's no need for the comment. > these comments are required in other discussions I think it's more than sufficient to clarify the semantic where it is defined. > > > >> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, > >> mdev->supported_features, VDPA_ATTR_PAD)) { > >> err = -EMSGSIZE; > >> @@ -815,10 +817,10 @@ 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)); > >> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); > >> > >> if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), > >> config.mac)) > >> @@ -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..97531b52dcbe 100644 > >> --- a/include/uapi/linux/vdpa.h > >> +++ b/include/uapi/linux/vdpa.h > >> @@ -46,12 +46,16 @@ 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 */ > >> > >> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > >> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > >> VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ > >> > >> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ > >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > > What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES? > This is to report a vDPA device features, and > VDPA_ATTR_DEV_SUPPORTED_FEATURES > is used for reporting the management device features, we have a long > discussion > on this before. Yes, but the comment is not clear in many ways: " features of a vDPA management device" sounds like features that is out of the scope of the virtio. And "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device. Thanks > > > > Thanks > > > >> + > >> /* new attributes must be added above here */ > >> VDPA_ATTR_MAX, > >> }; > >> -- > >> 2.31.1 > >> >
On 9/21/2022 10:17 AM, Jason Wang wrote: > On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: >> >> >> On 9/20/2022 10:02 AM, Jason Wang wrote: >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> 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. >>>> >>>> This commit invokes vdpa_config_ops.get_config() than >>>> vdpa_get_config_unlocked() to read the device config >>>> spcae, so no raeces in vdpa_set_features_unlocked() >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>> It's better to share the userspace code as well. >> OK, will share it in V2. >>>> --- >>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++----- >>>> include/uapi/linux/vdpa.h | 4 ++++ >>>> 2 files changed, 18 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>> index c06c02704461..798a02c7aa94 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 */ >>> The code explains itself, there's no need for the comment. >> these comments are required in other discussions > I think it's more than sufficient to clarify the semantic where it is defined. OK, then only comments in the header file > >>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, >>>> mdev->supported_features, VDPA_ATTR_PAD)) { >>>> err = -EMSGSIZE; >>>> @@ -815,10 +817,10 @@ 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)); >>>> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); >>>> >>>> if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), >>>> config.mac)) >>>> @@ -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..97531b52dcbe 100644 >>>> --- a/include/uapi/linux/vdpa.h >>>> +++ b/include/uapi/linux/vdpa.h >>>> @@ -46,12 +46,16 @@ 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 */ >>>> >>>> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >>>> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >>>> VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ >>>> >>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES? >> This is to report a vDPA device features, and >> VDPA_ATTR_DEV_SUPPORTED_FEATURES >> is used for reporting the management device features, we have a long >> discussion >> on this before. > Yes, but the comment is not clear in many ways: > > " features of a vDPA management device" sounds like features that is > out of the scope of the virtio. I think the term "vDPA device" implies that it is a virtio device. So how about: "virtio features of a vDPA management device" > > And > > "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device. will remove this example here. Thanks > > Thanks > >>> Thanks >>> >>>> + >>>> /* new attributes must be added above here */ >>>> VDPA_ATTR_MAX, >>>> }; >>>> -- >>>> 2.31.1 >>>>
On Wed, Sep 21, 2022 at 2:00 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: > > > > On 9/21/2022 10:17 AM, Jason Wang wrote: > > On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: > >> > >> > >> On 9/20/2022 10:02 AM, Jason Wang wrote: > >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> 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. > >>>> > >>>> This commit invokes vdpa_config_ops.get_config() than > >>>> vdpa_get_config_unlocked() to read the device config > >>>> spcae, so no raeces in vdpa_set_features_unlocked() > >>>> > >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >>> It's better to share the userspace code as well. > >> OK, will share it in V2. > >>>> --- > >>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++----- > >>>> include/uapi/linux/vdpa.h | 4 ++++ > >>>> 2 files changed, 18 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > >>>> index c06c02704461..798a02c7aa94 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 */ > >>> The code explains itself, there's no need for the comment. > >> these comments are required in other discussions > > I think it's more than sufficient to clarify the semantic where it is defined. > OK, then only comments in the header file > > > >>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, > >>>> mdev->supported_features, VDPA_ATTR_PAD)) { > >>>> err = -EMSGSIZE; > >>>> @@ -815,10 +817,10 @@ 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)); > >>>> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); > >>>> > >>>> if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), > >>>> config.mac)) > >>>> @@ -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..97531b52dcbe 100644 > >>>> --- a/include/uapi/linux/vdpa.h > >>>> +++ b/include/uapi/linux/vdpa.h > >>>> @@ -46,12 +46,16 @@ 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 */ > >>>> > >>>> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ > >>>> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ > >>>> VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ > >>>> > >>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ > >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ > >>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES? > >> This is to report a vDPA device features, and > >> VDPA_ATTR_DEV_SUPPORTED_FEATURES > >> is used for reporting the management device features, we have a long > >> discussion > >> on this before. > > Yes, but the comment is not clear in many ways: > > > > " features of a vDPA management device" sounds like features that is > > out of the scope of the virtio. > I think the term "vDPA device" implies that it is a virtio device. > So how about: "virtio features of a vDPA management device" Not a native speaker, but how about "virtio features that are supported by the vDPA management device?" Thanks > > > > And > > > > "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device. > will remove this example here. > > Thanks > > > > Thanks > > > >>> Thanks > >>> > >>>> + > >>>> /* new attributes must be added above here */ > >>>> VDPA_ATTR_MAX, > >>>> }; > >>>> -- > >>>> 2.31.1 > >>>> >
On 9/21/2022 3:44 PM, Jason Wang wrote: > On Wed, Sep 21, 2022 at 2:00 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: >> >> >> On 9/21/2022 10:17 AM, Jason Wang wrote: >>> On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: >>>> >>>> On 9/20/2022 10:02 AM, Jason Wang wrote: >>>>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> 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. >>>>>> >>>>>> This commit invokes vdpa_config_ops.get_config() than >>>>>> vdpa_get_config_unlocked() to read the device config >>>>>> spcae, so no raeces in vdpa_set_features_unlocked() >>>>>> >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>> It's better to share the userspace code as well. >>>> OK, will share it in V2. >>>>>> --- >>>>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++----- >>>>>> include/uapi/linux/vdpa.h | 4 ++++ >>>>>> 2 files changed, 18 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>>>> index c06c02704461..798a02c7aa94 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 */ >>>>> The code explains itself, there's no need for the comment. >>>> these comments are required in other discussions >>> I think it's more than sufficient to clarify the semantic where it is defined. >> OK, then only comments in the header file >>>>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, >>>>>> mdev->supported_features, VDPA_ATTR_PAD)) { >>>>>> err = -EMSGSIZE; >>>>>> @@ -815,10 +817,10 @@ 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)); >>>>>> + vdev->config->get_config(vdev, 0, &config, sizeof(config)); >>>>>> >>>>>> if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), >>>>>> config.mac)) >>>>>> @@ -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..97531b52dcbe 100644 >>>>>> --- a/include/uapi/linux/vdpa.h >>>>>> +++ b/include/uapi/linux/vdpa.h >>>>>> @@ -46,12 +46,16 @@ 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 */ >>>>>> >>>>>> VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ >>>>>> VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ >>>>>> VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ >>>>>> >>>>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ >>>>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ >>>>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES? >>>> This is to report a vDPA device features, and >>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES >>>> is used for reporting the management device features, we have a long >>>> discussion >>>> on this before. >>> Yes, but the comment is not clear in many ways: >>> >>> " features of a vDPA management device" sounds like features that is >>> out of the scope of the virtio. >> I think the term "vDPA device" implies that it is a virtio device. >> So how about: "virtio features of a vDPA management device" > Not a native speaker, but how about "virtio features that are > supported by the vDPA management device?" OK > > Thanks > >>> And >>> >>> "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device. >> will remove this example here. >> >> Thanks >>> Thanks >>> >>>>> Thanks >>>>> >>>>>> + >>>>>> /* new attributes must be added above here */ >>>>>> VDPA_ATTR_MAX, >>>>>> }; >>>>>> -- >>>>>> 2.31.1 >>>>>>
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index c06c02704461..798a02c7aa94 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,10 +817,10 @@ 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)); + vdev->config->get_config(vdev, 0, &config, sizeof(config)); if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), config.mac)) @@ -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..97531b52dcbe 100644 --- a/include/uapi/linux/vdpa.h +++ b/include/uapi/linux/vdpa.h @@ -46,12 +46,16 @@ 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 */ VDPA_ATTR_DEV_QUEUE_INDEX, /* u32 */ VDPA_ATTR_DEV_VENDOR_ATTR_NAME, /* string */ VDPA_ATTR_DEV_VENDOR_ATTR_VALUE, /* u64 */ + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */ + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */ + /* new attributes must be added above here */ VDPA_ATTR_MAX, };
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. This commit invokes vdpa_config_ops.get_config() than vdpa_get_config_unlocked() to read the device config spcae, so no raeces in vdpa_set_features_unlocked() Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 19 ++++++++++++++----- include/uapi/linux/vdpa.h | 4 ++++ 2 files changed, 18 insertions(+), 5 deletions(-)