Virtualization - Aug 2022 - [PATCH V4 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

> From: Zhu Lingshan <lingshan.zhu at 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 at 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 at 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 at 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