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

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

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