| Message ID | 20220722115309.82746-6-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| 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 22, 2022 7:53 AM > > If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so > when userspace querying queue pair numbers, it should return mq=1 than > zero. > > Function vdpa_dev_net_config_fill() fills the attributions of the vDPA > devices, so that it should call vdpa_dev_net_mq_config_fill() so the > parameter in vdpa_dev_net_mq_config_fill() should be feature_device than > feature_driver for the vDPA devices themselves > > Before this change, when MQ = 0, iproute2 output: > $vdpa dev config show vdpa0 > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 > mtu 1500 > > After applying this commit, when MQ = 0, iproute2 output: > $vdpa dev config show vdpa0 > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 > mtu 1500 > No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. Please drop this patch. Nack on this patch. > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/vdpa.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > d76b22b2f7ae..846dd37f3549 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct > vdpa_device *vdev, > u16 val_u16; > > if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) > - return 0; > + val_u16 = 1; > + else > + val_u16 = __virtio16_to_cpu(true, config- > >max_virtqueue_pairs); > > - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); > return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > val_u16); } > > @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct > vdpa_device *vdev, struct sk_buff *ms > VDPA_ATTR_PAD)) > return -EMSGSIZE; > > - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > &config); > + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, > +&config); > } > > static int > -- > 2.31.1
On 7/22/2022 9:14 PM, Parav Pandit wrote: > >> From: Zhu Lingshan <lingshan.zhu@intel.com> >> Sent: Friday, July 22, 2022 7:53 AM >> >> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so >> when userspace querying queue pair numbers, it should return mq=1 than >> zero. >> >> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA >> devices, so that it should call vdpa_dev_net_mq_config_fill() so the >> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than >> feature_driver for the vDPA devices themselves >> >> Before this change, when MQ = 0, iproute2 output: >> $vdpa dev config show vdpa0 >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 >> mtu 1500 >> >> After applying this commit, when MQ = 0, iproute2 output: >> $vdpa dev config show vdpa0 >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 >> mtu 1500 >> > No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. > Please drop this patch. > Nack on this patch. I believe MST has replied to you in the V3 thread, did you miss that? > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >> d76b22b2f7ae..846dd37f3549 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct >> vdpa_device *vdev, >> u16 val_u16; >> >> if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) >> - return 0; >> + val_u16 = 1; >> + else >> + val_u16 = __virtio16_to_cpu(true, config- >>> max_virtqueue_pairs); >> - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); >> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >> val_u16); } >> >> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct >> vdpa_device *vdev, struct sk_buff *ms >> VDPA_ATTR_PAD)) >> return -EMSGSIZE; >> >> - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >> &config); >> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, >> +&config); >> } >> >> static int >> -- >> 2.31.1
On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: > > > > From: Zhu Lingshan <lingshan.zhu@intel.com> > > Sent: Friday, July 22, 2022 7:53 AM > > > > If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so > > when userspace querying queue pair numbers, it should return mq=1 than > > zero. > > > > Function vdpa_dev_net_config_fill() fills the attributions of the vDPA > > devices, so that it should call vdpa_dev_net_mq_config_fill() so the > > parameter in vdpa_dev_net_mq_config_fill() should be feature_device than > > feature_driver for the vDPA devices themselves > > > > Before this change, when MQ = 0, iproute2 output: > > $vdpa dev config show vdpa0 > > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 > > mtu 1500 > > > > After applying this commit, when MQ = 0, iproute2 output: > > $vdpa dev config show vdpa0 > > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 > > mtu 1500 > > > No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. > Please drop this patch. > Nack on this patch. Wrt this patch as far as I'm concerned you guys are bikeshedding. Parav generally don't send nacks please they are not helpful. Zhu Lingshan please always address comments in some way. E.g. refer to them in the commit log explaining the motivation and the alternatives. I know you don't agree with Parav but ghosting isn't nice. I still feel what we should have done is not return a value, as long as we do return it we might as well return something reasonable, not 0. And I like it that this fixes sparse warning actually: max_virtqueue_pairs it tagged as __virtio, not __le. However, I am worried there is another bug here: this is checking driver features. But really max vqs should not depend on that, it depends on device features, no? > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > > --- > > drivers/vdpa/vdpa.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > > d76b22b2f7ae..846dd37f3549 100644 > > --- a/drivers/vdpa/vdpa.c > > +++ b/drivers/vdpa/vdpa.c > > @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct > > vdpa_device *vdev, > > u16 val_u16; > > > > if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) > > - return 0; > > + val_u16 = 1; > > + else > > + val_u16 = __virtio16_to_cpu(true, config- > > >max_virtqueue_pairs); > > > > - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); > > return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > > val_u16); } > > > > @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct > > vdpa_device *vdev, struct sk_buff *ms > > VDPA_ATTR_PAD)) > > return -EMSGSIZE; > > > > - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > > &config); > > + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, > > +&config); > > } > > > > static int > > -- > > 2.31.1
> From: Michael S. Tsirkin <mst@redhat.com> > Sent: Tuesday, August 9, 2022 3:36 PM > > > After applying this commit, when MQ = 0, iproute2 output: > > > $vdpa dev config show vdpa0 > > > vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false > > > max_vq_pairs 1 mtu 1500 > > > > > No. We do not want to diverge returning values of config space for fields > which are not present as discussed in previous versions. > > Please drop this patch. > > Nack on this patch. > > Wrt this patch as far as I'm concerned you guys are bikeshedding. > > Parav generally don't send nacks please they are not helpful. Ok. I explained the technical reasoning that we don't diverge in fields. All are linked to the respective feature bits uniformly. This I explained repeatedly in almost v1 to v3. And reporting 1 is mis-leading, because it says _MQ is not negotiated, how come this device tells its config space has max_qp = 1. But if you want to proceed to diverge kernel on feature bits go ahead. It requires inspection feature but feature. I just don't see how this can be well maintained. Commit log doesn't even describe the weird use case that says "as user space, I do not want to read device feature bits and just want to read config space to decide...". Odd..
On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote: > On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: >> >>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>> Sent: Friday, July 22, 2022 7:53 AM >>> >>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so >>> when userspace querying queue pair numbers, it should return mq=1 than >>> zero. >>> >>> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA >>> devices, so that it should call vdpa_dev_net_mq_config_fill() so the >>> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than >>> feature_driver for the vDPA devices themselves >>> >>> Before this change, when MQ = 0, iproute2 output: >>> $vdpa dev config show vdpa0 >>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 >>> mtu 1500 >>> >>> After applying this commit, when MQ = 0, iproute2 output: >>> $vdpa dev config show vdpa0 >>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 >>> mtu 1500 >>> >> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. >> Please drop this patch. >> Nack on this patch. > Wrt this patch as far as I'm concerned you guys are bikeshedding. > > Parav generally don't send nacks please they are not helpful. > > Zhu Lingshan please always address comments in some way. E.g. refer to > them in the commit log explaining the motivation and the alternatives. > I know you don't agree with Parav but ghosting isn't nice. > > I still feel what we should have done is > not return a value, as long as we do return it we might > as well return something reasonable, not 0. Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply not there, but userspace (iproute) mistakenly puts a zero value there. This is a pattern every tool in iproute follows consistently by large. I don't get why kernel has to return something without seeing a very convincing use case? Not to mention spec doesn't give us explicit definition for when the field in config space becomes valid and/or the default value at first sights as part of feature negotiation. If we want to stick to the model Lingshan proposed, maybe fix the spec first then get back on the details? -Siwei > > And I like it that this fixes sparse warning actually: > max_virtqueue_pairs it tagged as __virtio, not __le. > > However, I am worried there is another bug here: > this is checking driver features. But really max vqs > should not depend on that, it depends on device > features, no? > > > >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>> --- >>> drivers/vdpa/vdpa.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>> d76b22b2f7ae..846dd37f3549 100644 >>> --- a/drivers/vdpa/vdpa.c >>> +++ b/drivers/vdpa/vdpa.c >>> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct >>> vdpa_device *vdev, >>> u16 val_u16; >>> >>> if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) >>> - return 0; >>> + val_u16 = 1; >>> + else >>> + val_u16 = __virtio16_to_cpu(true, config- >>>> max_virtqueue_pairs); >>> - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); >>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >>> val_u16); } >>> >>> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct >>> vdpa_device *vdev, struct sk_buff *ms >>> VDPA_ATTR_PAD)) >>> return -EMSGSIZE; >>> >>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >>> &config); >>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, >>> +&config); >>> } >>> >>> static int >>> -- >>> 2.31.1 > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Wed, Aug 10, 2022 at 8:54 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: > > > > On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote: > > On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: > >> > >>> From: Zhu Lingshan <lingshan.zhu@intel.com> > >>> Sent: Friday, July 22, 2022 7:53 AM > >>> > >>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so > >>> when userspace querying queue pair numbers, it should return mq=1 than > >>> zero. > >>> > >>> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA > >>> devices, so that it should call vdpa_dev_net_mq_config_fill() so the > >>> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than > >>> feature_driver for the vDPA devices themselves > >>> > >>> Before this change, when MQ = 0, iproute2 output: > >>> $vdpa dev config show vdpa0 > >>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 > >>> mtu 1500 > >>> > >>> After applying this commit, when MQ = 0, iproute2 output: > >>> $vdpa dev config show vdpa0 > >>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 > >>> mtu 1500 > >>> > >> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. > >> Please drop this patch. > >> Nack on this patch. > > Wrt this patch as far as I'm concerned you guys are bikeshedding. > > > > Parav generally don't send nacks please they are not helpful. > > > > Zhu Lingshan please always address comments in some way. E.g. refer to > > them in the commit log explaining the motivation and the alternatives. > > I know you don't agree with Parav but ghosting isn't nice. > > > > I still feel what we should have done is > > not return a value, as long as we do return it we might > > as well return something reasonable, not 0. > Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is > not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply > not there, but userspace (iproute) mistakenly puts a zero value there. > This is a pattern every tool in iproute follows consistently by large. I > don't get why kernel has to return something without seeing a very > convincing use case? > > Not to mention spec doesn't give us explicit definition for when the > field in config space becomes valid and/or the default value at first > sights as part of feature negotiation. If we want to stick to the model > Lingshan proposed, maybe fix the spec first then get back on the details? So spec said " The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. " My understanding is that the field is always valid if the device offers the feature. Btw, even if the spec is unclear, it would be very hard to "fix" it without introducing a new feature bit, it means we still need to deal with device without the new feature. Thanks > > -Siwei > > > > > And I like it that this fixes sparse warning actually: > > max_virtqueue_pairs it tagged as __virtio, not __le. > > > > However, I am worried there is another bug here: > > this is checking driver features. But really max vqs > > should not depend on that, it depends on device > > features, no? > > > > > > > >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >>> --- > >>> drivers/vdpa/vdpa.c | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > >>> d76b22b2f7ae..846dd37f3549 100644 > >>> --- a/drivers/vdpa/vdpa.c > >>> +++ b/drivers/vdpa/vdpa.c > >>> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct > >>> vdpa_device *vdev, > >>> u16 val_u16; > >>> > >>> if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) > >>> - return 0; > >>> + val_u16 = 1; > >>> + else > >>> + val_u16 = __virtio16_to_cpu(true, config- > >>>> max_virtqueue_pairs); > >>> - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); > >>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > >>> val_u16); } > >>> > >>> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct > >>> vdpa_device *vdev, struct sk_buff *ms > >>> VDPA_ATTR_PAD)) > >>> return -EMSGSIZE; > >>> > >>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, > >>> &config); > >>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, > >>> +&config); > >>> } > >>> > >>> static int > >>> -- > >>> 2.31.1 > > _______________________________________________ > > Virtualization mailing list > > Virtualization@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization >
On 8/10/2022 3:36 AM, Michael S. Tsirkin wrote: > On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: >> >>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>> Sent: Friday, July 22, 2022 7:53 AM >>> >>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so >>> when userspace querying queue pair numbers, it should return mq=1 than >>> zero. >>> >>> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA >>> devices, so that it should call vdpa_dev_net_mq_config_fill() so the >>> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than >>> feature_driver for the vDPA devices themselves >>> >>> Before this change, when MQ = 0, iproute2 output: >>> $vdpa dev config show vdpa0 >>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 >>> mtu 1500 >>> >>> After applying this commit, when MQ = 0, iproute2 output: >>> $vdpa dev config show vdpa0 >>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 >>> mtu 1500 >>> >> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. >> Please drop this patch. >> Nack on this patch. > Wrt this patch as far as I'm concerned you guys are bikeshedding. > > Parav generally don't send nacks please they are not helpful. > > Zhu Lingshan please always address comments in some way. E.g. refer to > them in the commit log explaining the motivation and the alternatives. > I know you don't agree with Parav but ghosting isn't nice. I can work out a better commit message, currently I have an example on how to process MQ = 0. I will handle all conditional fields(MTC, MAC...) here. So I will explain them both in general and details for every field. Thanks, Zhu Lingshan > > I still feel what we should have done is > not return a value, as long as we do return it we might > as well return something reasonable, not 0. > > And I like it that this fixes sparse warning actually: > max_virtqueue_pairs it tagged as __virtio, not __le. > > However, I am worried there is another bug here: > this is checking driver features. But really max vqs > should not depend on that, it depends on device > features, no? We have this fix in this patch below: return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config); this checks device features. Thanks, Zhu Lingshan > > > >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>> --- >>> drivers/vdpa/vdpa.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>> d76b22b2f7ae..846dd37f3549 100644 >>> --- a/drivers/vdpa/vdpa.c >>> +++ b/drivers/vdpa/vdpa.c >>> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct >>> vdpa_device *vdev, >>> u16 val_u16; >>> >>> if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) >>> - return 0; >>> + val_u16 = 1; >>> + else >>> + val_u16 = __virtio16_to_cpu(true, config- >>>> max_virtqueue_pairs); >>> - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); >>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >>> val_u16); } >>> >>> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct >>> vdpa_device *vdev, struct sk_buff *ms >>> VDPA_ATTR_PAD)) >>> return -EMSGSIZE; >>> >>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >>> &config); >>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, >>> +&config); >>> } >>> >>> static int >>> -- >>> 2.31.1
On 8/9/2022 6:09 PM, Jason Wang wrote: > On Wed, Aug 10, 2022 at 8:54 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote: >> >> >> On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote: >>> On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote: >>>>> From: Zhu Lingshan <lingshan.zhu@intel.com> >>>>> Sent: Friday, July 22, 2022 7:53 AM >>>>> >>>>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so >>>>> when userspace querying queue pair numbers, it should return mq=1 than >>>>> zero. >>>>> >>>>> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA >>>>> devices, so that it should call vdpa_dev_net_mq_config_fill() so the >>>>> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than >>>>> feature_driver for the vDPA devices themselves >>>>> >>>>> Before this change, when MQ = 0, iproute2 output: >>>>> $vdpa dev config show vdpa0 >>>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 >>>>> mtu 1500 >>>>> >>>>> After applying this commit, when MQ = 0, iproute2 output: >>>>> $vdpa dev config show vdpa0 >>>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 >>>>> mtu 1500 >>>>> >>>> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions. >>>> Please drop this patch. >>>> Nack on this patch. >>> Wrt this patch as far as I'm concerned you guys are bikeshedding. >>> >>> Parav generally don't send nacks please they are not helpful. >>> >>> Zhu Lingshan please always address comments in some way. E.g. refer to >>> them in the commit log explaining the motivation and the alternatives. >>> I know you don't agree with Parav but ghosting isn't nice. >>> >>> I still feel what we should have done is >>> not return a value, as long as we do return it we might >>> as well return something reasonable, not 0. >> Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is >> not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply >> not there, but userspace (iproute) mistakenly puts a zero value there. >> This is a pattern every tool in iproute follows consistently by large. I >> don't get why kernel has to return something without seeing a very >> convincing use case? >> >> Not to mention spec doesn't give us explicit definition for when the >> field in config space becomes valid and/or the default value at first >> sights as part of feature negotiation. If we want to stick to the model >> Lingshan proposed, maybe fix the spec first then get back on the details? > So spec said > > " > The following driver-read-only field, max_virtqueue_pairs only exists > if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. > " > > My understanding is that the field is always valid if the device > offers the feature. The tricky part is to deal with VERSION_1 on transitional device that determines the endianness of field. I know we don't support !VERSION_1 vdpa provider for now, but the tool should be made independent of this assumption. For the most of config fields there's no actual valid "default" value during feature negotiation until it can be determined after negotiation is done. I wonder what is the administrative value if presenting those random value to the end user? And there's even special feature like VIRTIO_BLK_F_CONFIG_WCE that only present valid feature value after negotiation. I'm afraid it may further confuse end user, or it would require them to read and understand all of details in spec, which apparently contradict to the goal of showing meaningful queue-pair value without requiring user to read the spec details. -Siwei > > Btw, even if the spec is unclear, it would be very hard to "fix" it > without introducing a new feature bit, it means we still need to deal > with device without the new feature. > > Thanks > >> -Siwei >> >>> And I like it that this fixes sparse warning actually: >>> max_virtqueue_pairs it tagged as __virtio, not __le. >>> >>> However, I am worried there is another bug here: >>> this is checking driver features. But really max vqs >>> should not depend on that, it depends on device >>> features, no? >>> >>> >>> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>> --- >>>>> drivers/vdpa/vdpa.c | 7 ++++--- >>>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index >>>>> d76b22b2f7ae..846dd37f3549 100644 >>>>> --- a/drivers/vdpa/vdpa.c >>>>> +++ b/drivers/vdpa/vdpa.c >>>>> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct >>>>> vdpa_device *vdev, >>>>> u16 val_u16; >>>>> >>>>> if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) >>>>> - return 0; >>>>> + val_u16 = 1; >>>>> + else >>>>> + val_u16 = __virtio16_to_cpu(true, config- >>>>>> max_virtqueue_pairs); >>>>> - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); >>>>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, >>>>> val_u16); } >>>>> >>>>> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct >>>>> vdpa_device *vdev, struct sk_buff *ms >>>>> VDPA_ATTR_PAD)) >>>>> return -EMSGSIZE; >>>>> >>>>> - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, >>>>> &config); >>>>> + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, >>>>> +&config); >>>>> } >>>>> >>>>> static int >>>>> -- >>>>> 2.31.1 >>> _______________________________________________ >>> Virtualization mailing list >>> Virtualization@lists.linux-foundation.org >>> https://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NE42b1rl66ElGUzHr3b9xXGYCs2Vpb5dkhF0fPXnAyyFYzZZyzsY9NV_Qbf2AZCI3XxC13_nlWfSVN52yIM$ >> _______________________________________________ >> Virtualization mailing list >> Virtualization@lists.linux-foundation.org >> https://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NE42b1rl66ElGUzHr3b9xXGYCs2Vpb5dkhF0fPXnAyyFYzZZyzsY9NV_Qbf2AZCI3XxC13_nlWfSVN52yIM$ >>
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index d76b22b2f7ae..846dd37f3549 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev, u16 val_u16; if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0) - return 0; + val_u16 = 1; + else + val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs); - val_u16 = le16_to_cpu(config->max_virtqueue_pairs); return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16); } @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms VDPA_ATTR_PAD)) return -EMSGSIZE; - return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config); + return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config); } static int
If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so when userspace querying queue pair numbers, it should return mq=1 than zero. Function vdpa_dev_net_config_fill() fills the attributions of the vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so the parameter in vdpa_dev_net_mq_config_fill() should be feature_device than feature_driver for the vDPA devices themselves Before this change, when MQ = 0, iproute2 output: $vdpa dev config show vdpa0 vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0 mtu 1500 After applying this commit, when MQ = 0, iproute2 output: $vdpa dev config show vdpa0 vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1 mtu 1500 Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)