Re: [PATCH v5 04/13] vdpa: Read device configuration only if FEATURES_OK

Re: [PATCH v5 04/13] vdpa: Read device configuration only if FEATURES_OK

[Si-Wei Liu]

On 12/21/2021 9:19 AM, Eli Cohen wrote:
> Avoid reading device configuration during feature negotiation. Read
> device status and verify that VIRTIO_CONFIG_S_FEATURES_OK is set.
>
> Otherwise, return -EAGAIN.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 5134c83c4a22..3285cebd89c0 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -838,8 +838,18 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>   {
>   	u32 device_id;
>   	void *hdr;
> +	u8 status;
>   	int err;
>   
> +	mutex_lock(&vdev->cf_mutex);
> +	status = vdev->config->get_status(vdev);
Atomicity and data consistency not guaranteed against vdpa_get_config 
and get_features in vdpa_dev_net_config_fill(). Need to use coarse locking.

-Siwei
> +	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> +		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not completed");
> +		mutex_unlock(&vdev->cf_mutex);
> +		return -EAGAIN;
> +	}
> +	mutex_unlock(&vdev->cf_mutex);
> +
>   	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>   			  VDPA_CMD_DEV_CONFIG_GET);
>   	if (!hdr)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 04/13] vdpa: Read device configuration only if FEATURES_OK

[Si-Wei Liu]

On 12/21/2021 9:55 PM, Eli Cohen wrote:
> On Tue, Dec 21, 2021 at 05:55:56PM -0800, Si-Wei Liu wrote:
>>
>> On 12/21/2021 9:19 AM, Eli Cohen wrote:
>>> Avoid reading device configuration during feature negotiation. Read
>>> device status and verify that VIRTIO_CONFIG_S_FEATURES_OK is set.
>>>
>>> Otherwise, return -EAGAIN.
>>>
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c | 10 ++++++++++
>>>    1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 5134c83c4a22..3285cebd89c0 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -838,8 +838,18 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>>    {
>>>    	u32 device_id;
>>>    	void *hdr;
>>> +	u8 status;
>>>    	int err;
>>> +	mutex_lock(&vdev->cf_mutex);
>>> +	status = vdev->config->get_status(vdev);
>> Atomicity and data consistency not guaranteed against vdpa_get_config and
>> get_features in vdpa_dev_net_config_fill(). Need to use coarse locking.
> Your concern is that vdpa_get_config() is vdpa_dev_net_config_fill
> () is not being read under the lock?
Not exactly. vdpa_get_config() is already in the cf_mutex lock. However, 
the config data may become invalid getting out of the FEATURES_OK check, 
as theoretically it's possible the guest could change the config via 
set_config(), set_status(), or reset() in between. You'd need to use a 
single cf_mutex to protect almost the whole code block in 
vdpa_dev_config_fill() to ensure data consistency.

-Siwei
>
> I will put it under the lock.
>
>> -Siwei
>>> +	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>>> +		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not completed");
>>> +		mutex_unlock(&vdev->cf_mutex);
>>> +		return -EAGAIN;
>>> +	}
>>> +	mutex_unlock(&vdev->cf_mutex);
>>> +
>>>    	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>>    			  VDPA_CMD_DEV_CONFIG_GET);
>>>    	if (!hdr)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Si-Wei Liu]

On 12/21/2021 9:20 AM, Eli Cohen wrote:
> Add max_supported_vqs field to struct vdpa_mgmt_dev. Upstream drivers
> need to feel this value according to the device capabilities.
>
> This value is reported back in a netlink message when showing management
> devices.
>
> Example:
>
> $ vdpa dev show
s/dev/mgmtdev/

and,
> vdpa mgmtdev show
remove this line.
> auxiliary/mlx5_core.sf.1:
>    supported_classes net
>    max_supported_vqs 256
Not consistent with the example in patch #11 in the series.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c       | 3 +++
>   include/linux/vdpa.h      | 1 +
>   include/uapi/linux/vdpa.h | 1 +
>   3 files changed, 5 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index eb223bec5209..4b649125a038 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>   		err = -EMSGSIZE;
>   		goto msg_err;
>   	}
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
> +			mdev->max_supported_vqs))
It still needs a default value when the field is not explicitly filled 
in by the driver.

-Siwei

> +		goto msg_err;
>   
>   	genlmsg_end(msg, hdr);
>   	return 0;
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 47e2b780e4bc..b575f71fa5e7 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -447,6 +447,7 @@ struct vdpa_mgmt_dev {
>   	const struct vdpa_mgmtdev_ops *ops;
>   	const struct virtio_device_id *id_table;
>   	u64 config_attr_mask;
> +	u16 max_supported_vqs;
>   	struct list_head list;
>   };
>   
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index db3738ef3beb..995257c6bf2a 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -44,6 +44,7 @@ enum vdpa_attr {
>   	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
>   
>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> +	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u16 */
>   	/* new attributes must be added above here */
>   	VDPA_ATTR_MAX,
>   };

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Parav Pandit via Virtualization]

> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Wednesday, December 22, 2021 7:31 AM
> 
> On 12/21/2021 9:20 AM, Eli Cohen wrote:
> > Add max_supported_vqs field to struct vdpa_mgmt_dev. Upstream drivers
> > need to feel this value according to the device capabilities.
> >
> > This value is reported back in a netlink message when showing
> > management devices.
> >
> > Example:
> >
> > $ vdpa dev show
> s/dev/mgmtdev/
> 
> and,
> > vdpa mgmtdev show
> remove this line.
> > auxiliary/mlx5_core.sf.1:
> >    supported_classes net
> >    max_supported_vqs 256
It should be in same line.
Also please show the JSON output.

> Not consistent with the example in patch #11 in the series.
> >
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> >   drivers/vdpa/vdpa.c       | 3 +++
> >   include/linux/vdpa.h      | 1 +
> >   include/uapi/linux/vdpa.h | 1 +
> >   3 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> > eb223bec5209..4b649125a038 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
> vdpa_mgmt_dev *mdev, struct sk_buff *m
> >   		err = -EMSGSIZE;
> >   		goto msg_err;
> >   	}
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
> > +			mdev->max_supported_vqs))
> It still needs a default value when the field is not explicitly filled in by the
> driver.
> 
Unlikely. This can be optional field to help user decide device max limit.
When max_supported_vqs is set to zero. Vdpa should omit exposing it to user space.

> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -447,6 +447,7 @@ struct vdpa_mgmt_dev {
> >   	const struct vdpa_mgmtdev_ops *ops;
> >   	const struct virtio_device_id *id_table;
> >   	u64 config_attr_mask;
> > +	u16 max_supported_vqs;
This breaks the natural alignment and create holes in the struct.
Please move it at the last entry in the struct after list.

There has been some talk/patches of rdma virtio device.
I anticipate such device to support more than 64K queues by nature of rdma.
It is better to keep max_supported_vqs as u32.

> >   	struct list_head list;
> >   };
> >
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index db3738ef3beb..995257c6bf2a 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -44,6 +44,7 @@ enum vdpa_attr {
> >   	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
> >
> >   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> > +	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u16 */
> >   	/* new attributes must be added above here */
> >   	VDPA_ATTR_MAX,
> >   };

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Si-Wei Liu]

On 12/21/2021 9:06 PM, Parav Pandit wrote:
>
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Wednesday, December 22, 2021 7:31 AM
>>
>> On 12/21/2021 9:20 AM, Eli Cohen wrote:
>>> Add max_supported_vqs field to struct vdpa_mgmt_dev. Upstream drivers
>>> need to feel this value according to the device capabilities.
>>>
>>> This value is reported back in a netlink message when showing
>>> management devices.
>>>
>>> Example:
>>>
>>> $ vdpa dev show
>> s/dev/mgmtdev/
>>
>> and,
>>> vdpa mgmtdev show
>> remove this line.
>>> auxiliary/mlx5_core.sf.1:
>>>     supported_classes net
>>>     max_supported_vqs 256
> It should be in same line.
> Also please show the JSON output.
>
>> Not consistent with the example in patch #11 in the series.
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c       | 3 +++
>>>    include/linux/vdpa.h      | 1 +
>>>    include/uapi/linux/vdpa.h | 1 +
>>>    3 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>>> eb223bec5209..4b649125a038 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
>> vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>    		err = -EMSGSIZE;
>>>    		goto msg_err;
>>>    	}
>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
>>> +			mdev->max_supported_vqs))
>> It still needs a default value when the field is not explicitly filled in by the
>> driver.
>>
> Unlikely. This can be optional field to help user decide device max limit.
> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user space.
That'd be okay. I thought this field might also be useful for user to 
tell if the parent can support mq.

-Siwei

>
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -447,6 +447,7 @@ struct vdpa_mgmt_dev {
>>>    	const struct vdpa_mgmtdev_ops *ops;
>>>    	const struct virtio_device_id *id_table;
>>>    	u64 config_attr_mask;
>>> +	u16 max_supported_vqs;
> This breaks the natural alignment and create holes in the struct.
> Please move it at the last entry in the struct after list.
>
> There has been some talk/patches of rdma virtio device.
> I anticipate such device to support more than 64K queues by nature of rdma.
> It is better to keep max_supported_vqs as u32.
>
>>>    	struct list_head list;
>>>    };
>>>
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index db3738ef3beb..995257c6bf2a 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -44,6 +44,7 @@ enum vdpa_attr {
>>>    	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
>>>
>>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>> +	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u16 */
>>>    	/* new attributes must be added above here */
>>>    	VDPA_ATTR_MAX,
>>>    };

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Parav Pandit via Virtualization]

> From: Eli Cohen <elic@nvidia.com>
> Sent: Wednesday, December 22, 2021 12:17 PM
> 
> > > > --- a/drivers/vdpa/vdpa.c
> > > > +++ b/drivers/vdpa/vdpa.c
> > > > @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
> > > vdpa_mgmt_dev *mdev, struct sk_buff *m
> > > >   		err = -EMSGSIZE;
> > > >   		goto msg_err;
> > > >   	}
> > > > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
> > > > +			mdev->max_supported_vqs))
> > > It still needs a default value when the field is not explicitly
> > > filled in by the driver.
> > >
> > Unlikely. This can be optional field to help user decide device max limit.
> > When max_supported_vqs is set to zero. Vdpa should omit exposing it to user
> space.
> >
> 
> This is not about what you expose to userspace. It's about the number of VQs
> you want to create for a specific instance of vdpa.
This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N.
User will choose to create VQ with VQs <= N depending on its vcpu and other factors.

This is what is exposed to the user to decide the upper bound.
> > There has been some talk/patches of rdma virtio device.
> > I anticipate such device to support more than 64K queues by nature of rdma.
> > It is better to keep max_supported_vqs as u32.
> 
> Why not add it when we have it?
Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one.
Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Si-Wei Liu]

On 12/21/2021 11:10 PM, Eli Cohen wrote:
> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote:
>>
>>> From: Eli Cohen <elic@nvidia.com>
>>> Sent: Wednesday, December 22, 2021 12:17 PM
>>>
>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>>>>    		err = -EMSGSIZE;
>>>>>>    		goto msg_err;
>>>>>>    	}
>>>>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
>>>>>> +			mdev->max_supported_vqs))
>>>>> It still needs a default value when the field is not explicitly
>>>>> filled in by the driver.
>>>>>
>>>> Unlikely. This can be optional field to help user decide device max limit.
>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user
>>> space.
>>> This is not about what you expose to userspace. It's about the number of VQs
>>> you want to create for a specific instance of vdpa.
>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N.
>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors.
> You're right.
> So each vendor needs to put there their value.
If I understand Parav correctly, he was suggesting not to expose 
VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs 
== 0) from the driver.

But meanwhile, I do wonder how users tell apart multiqueue supporting 
parent from the single queue mgmtdev without getting the aid from this 
field. I hope the answer won't be to create a vdpa instance to try.

-Siwei

>
>> This is what is exposed to the user to decide the upper bound.
>>>> There has been some talk/patches of rdma virtio device.
>>>> I anticipate such device to support more than 64K queues by nature of rdma.
>>>> It is better to keep max_supported_vqs as u32.
>>> Why not add it when we have it?
>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one.
>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it.
> I can use u32 then.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Si-Wei Liu]

On 12/21/2021 11:54 PM, Eli Cohen wrote:
> On Tue, Dec 21, 2021 at 11:29:36PM -0800, Si-Wei Liu wrote:
>>
>> On 12/21/2021 11:10 PM, Eli Cohen wrote:
>>> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote:
>>>>> From: Eli Cohen <elic@nvidia.com>
>>>>> Sent: Wednesday, December 22, 2021 12:17 PM
>>>>>
>>>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
>>>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>>>>>>     		err = -EMSGSIZE;
>>>>>>>>     		goto msg_err;
>>>>>>>>     	}
>>>>>>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
>>>>>>>> +			mdev->max_supported_vqs))
>>>>>>> It still needs a default value when the field is not explicitly
>>>>>>> filled in by the driver.
>>>>>>>
>>>>>> Unlikely. This can be optional field to help user decide device max limit.
>>>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user
>>>>> space.
>>>>> This is not about what you expose to userspace. It's about the number of VQs
>>>>> you want to create for a specific instance of vdpa.
>>>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N.
>>>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors.
>>> You're right.
>>> So each vendor needs to put there their value.
>> If I understand Parav correctly, he was suggesting not to expose
>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs ==
>> 0) from the driver.
> I can see the reasoning, but maybe we should leave it as zero which
> means it was not reported. The user will then need to guess. I believe
> other vendors will follow with an update so this to a real value.
Unless you place a check in the vdpa core to enforce it on vdpa 
creation, otherwise it's very likely to get ignored by other vendors.

>
>> But meanwhile, I do wonder how users tell apart multiqueue supporting parent
>> from the single queue mgmtdev without getting the aid from this field. I
>> hope the answer won't be to create a vdpa instance to try.
>>
> Do you see a scenario that an admin decides to not instantiate vdpa just
> because it does not support MQ?
Yes, there is. If the hardware doesn't support MQ, the provisioning tool 
in the mgmt software will need to fallback to software vhost backend 
with mq=on. At the time the tool is checking out, it doesn't run with 
root privilege.

>
> And it the management device reports it does support, there's still no
> guarantee you'll end up with a MQ net device.
I'm not sure I follow. Do you mean it may be up to the guest feature 
negotiation? But the device itself is still MQ capable, isn't it?

Thanks,
-Siwei

>
>
>> -Siwei
>>
>>>> This is what is exposed to the user to decide the upper bound.
>>>>>> There has been some talk/patches of rdma virtio device.
>>>>>> I anticipate such device to support more than 64K queues by nature of rdma.
>>>>>> It is better to keep max_supported_vqs as u32.
>>>>> Why not add it when we have it?
>>>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one.
>>>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it.
>>> I can use u32 then.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Jason Wang]

On Thu, Dec 23, 2021 at 3:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 12/21/2021 11:54 PM, Eli Cohen wrote:
> > On Tue, Dec 21, 2021 at 11:29:36PM -0800, Si-Wei Liu wrote:
> >>
> >> On 12/21/2021 11:10 PM, Eli Cohen wrote:
> >>> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote:
> >>>>> From: Eli Cohen <elic@nvidia.com>
> >>>>> Sent: Wednesday, December 22, 2021 12:17 PM
> >>>>>
> >>>>>>>> --- a/drivers/vdpa/vdpa.c
> >>>>>>>> +++ b/drivers/vdpa/vdpa.c
> >>>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
> >>>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m
> >>>>>>>>                err = -EMSGSIZE;
> >>>>>>>>                goto msg_err;
> >>>>>>>>        }
> >>>>>>>> +      if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
> >>>>>>>> +                      mdev->max_supported_vqs))
> >>>>>>> It still needs a default value when the field is not explicitly
> >>>>>>> filled in by the driver.
> >>>>>>>
> >>>>>> Unlikely. This can be optional field to help user decide device max limit.
> >>>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user
> >>>>> space.
> >>>>> This is not about what you expose to userspace. It's about the number of VQs
> >>>>> you want to create for a specific instance of vdpa.
> >>>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N.
> >>>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors.
> >>> You're right.
> >>> So each vendor needs to put there their value.
> >> If I understand Parav correctly, he was suggesting not to expose
> >> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs ==
> >> 0) from the driver.
> > I can see the reasoning, but maybe we should leave it as zero which
> > means it was not reported. The user will then need to guess. I believe
> > other vendors will follow with an update so this to a real value.
> Unless you place a check in the vdpa core to enforce it on vdpa
> creation, otherwise it's very likely to get ignored by other vendors.
>
> >
> >> But meanwhile, I do wonder how users tell apart multiqueue supporting parent
> >> from the single queue mgmtdev without getting the aid from this field. I
> >> hope the answer won't be to create a vdpa instance to try.
> >>
> > Do you see a scenario that an admin decides to not instantiate vdpa just
> > because it does not support MQ?
> Yes, there is. If the hardware doesn't support MQ, the provisioning tool
> in the mgmt software will need to fallback to software vhost backend
> with mq=on. At the time the tool is checking out, it doesn't run with
> root privilege.
>
> >
> > And it the management device reports it does support, there's still no
> > guarantee you'll end up with a MQ net device.
> I'm not sure I follow. Do you mean it may be up to the guest feature
> negotiation? But the device itself is still MQ capable, isn't it?

I think we need to clarify the "device" here.

For compatibility reasons, there could be a case that mgmt doesn't
expect a mq capable vdpa device. So in this case, even if the parent
is MQ capable, the vdpa isn't.

Thanks

>
> Thanks,
> -Siwei
>
> >
> >
> >> -Siwei
> >>
> >>>> This is what is exposed to the user to decide the upper bound.
> >>>>>> There has been some talk/patches of rdma virtio device.
> >>>>>> I anticipate such device to support more than 64K queues by nature of rdma.
> >>>>>> It is better to keep max_supported_vqs as u32.
> >>>>> Why not add it when we have it?
> >>>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one.
> >>>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it.
> >>> I can use u32 then.
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Si-Wei Liu]

On 12/22/2021 6:27 PM, Jason Wang wrote:
> On Thu, Dec 23, 2021 at 3:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 12/21/2021 11:54 PM, Eli Cohen wrote:
>>> On Tue, Dec 21, 2021 at 11:29:36PM -0800, Si-Wei Liu wrote:
>>>> On 12/21/2021 11:10 PM, Eli Cohen wrote:
>>>>> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote:
>>>>>>> From: Eli Cohen <elic@nvidia.com>
>>>>>>> Sent: Wednesday, December 22, 2021 12:17 PM
>>>>>>>
>>>>>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
>>>>>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>>>>>>>>                 err = -EMSGSIZE;
>>>>>>>>>>                 goto msg_err;
>>>>>>>>>>         }
>>>>>>>>>> +      if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
>>>>>>>>>> +                      mdev->max_supported_vqs))
>>>>>>>>> It still needs a default value when the field is not explicitly
>>>>>>>>> filled in by the driver.
>>>>>>>>>
>>>>>>>> Unlikely. This can be optional field to help user decide device max limit.
>>>>>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user
>>>>>>> space.
>>>>>>> This is not about what you expose to userspace. It's about the number of VQs
>>>>>>> you want to create for a specific instance of vdpa.
>>>>>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N.
>>>>>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors.
>>>>> You're right.
>>>>> So each vendor needs to put there their value.
>>>> If I understand Parav correctly, he was suggesting not to expose
>>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs ==
>>>> 0) from the driver.
>>> I can see the reasoning, but maybe we should leave it as zero which
>>> means it was not reported. The user will then need to guess. I believe
>>> other vendors will follow with an update so this to a real value.
>> Unless you place a check in the vdpa core to enforce it on vdpa
>> creation, otherwise it's very likely to get ignored by other vendors.
>>
>>>> But meanwhile, I do wonder how users tell apart multiqueue supporting parent
>>>> from the single queue mgmtdev without getting the aid from this field. I
>>>> hope the answer won't be to create a vdpa instance to try.
>>>>
>>> Do you see a scenario that an admin decides to not instantiate vdpa just
>>> because it does not support MQ?
>> Yes, there is. If the hardware doesn't support MQ, the provisioning tool
>> in the mgmt software will need to fallback to software vhost backend
>> with mq=on. At the time the tool is checking out, it doesn't run with
>> root privilege.
>>
>>> And it the management device reports it does support, there's still no
>>> guarantee you'll end up with a MQ net device.
>> I'm not sure I follow. Do you mean it may be up to the guest feature
>> negotiation? But the device itself is still MQ capable, isn't it?
> I think we need to clarify the "device" here.
>
> For compatibility reasons, there could be a case that mgmt doesn't
> expect a mq capable vdpa device. So in this case, even if the parent
> is MQ capable, the vdpa isn't.
Right. The mgmt software is not necessarily libvirt. Perhaps I should be 
explicit to say the mgmt software we're building would definitely create 
MQ vdpa device in case on a MQ capable parent.

-Siwei

>
> Thanks
>
>> Thanks,
>> -Siwei
>>
>>>
>>>> -Siwei
>>>>
>>>>>> This is what is exposed to the user to decide the upper bound.
>>>>>>>> There has been some talk/patches of rdma virtio device.
>>>>>>>> I anticipate such device to support more than 64K queues by nature of rdma.
>>>>>>>> It is better to keep max_supported_vqs as u32.
>>>>>>> Why not add it when we have it?
>>>>>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one.
>>>>>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it.
>>>>> I can use u32 then.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 10/13] vdpa: Support reporting max device virtqueues

[Si-Wei Liu]

On 12/22/2021 9:39 PM, Eli Cohen wrote:
> On Wed, Dec 22, 2021 at 06:43:38PM -0800, Si-Wei Liu wrote:
>>
>> On 12/22/2021 6:27 PM, Jason Wang wrote:
>>> On Thu, Dec 23, 2021 at 3:25 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>
>>>> On 12/21/2021 11:54 PM, Eli Cohen wrote:
>>>>> On Tue, Dec 21, 2021 at 11:29:36PM -0800, Si-Wei Liu wrote:
>>>>>> On 12/21/2021 11:10 PM, Eli Cohen wrote:
>>>>>>> On Wed, Dec 22, 2021 at 09:03:37AM +0200, Parav Pandit wrote:
>>>>>>>>> From: Eli Cohen <elic@nvidia.com>
>>>>>>>>> Sent: Wednesday, December 22, 2021 12:17 PM
>>>>>>>>>
>>>>>>>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>>>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>>>>>>>> @@ -507,6 +507,9 @@ static int vdpa_mgmtdev_fill(const struct
>>>>>>>>>>> vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>>>>>>>>>>                  err = -EMSGSIZE;
>>>>>>>>>>>>                  goto msg_err;
>>>>>>>>>>>>          }
>>>>>>>>>>>> +      if (nla_put_u16(msg, VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,
>>>>>>>>>>>> +                      mdev->max_supported_vqs))
>>>>>>>>>>> It still needs a default value when the field is not explicitly
>>>>>>>>>>> filled in by the driver.
>>>>>>>>>>>
>>>>>>>>>> Unlikely. This can be optional field to help user decide device max limit.
>>>>>>>>>> When max_supported_vqs is set to zero. Vdpa should omit exposing it to user
>>>>>>>>> space.
>>>>>>>>> This is not about what you expose to userspace. It's about the number of VQs
>>>>>>>>> you want to create for a specific instance of vdpa.
>>>>>>>> This value on mgmtdev indicates that a given mgmt device supports creating a vdpa device who can have maximum VQs of N.
>>>>>>>> User will choose to create VQ with VQs <= N depending on its vcpu and other factors.
>>>>>>> You're right.
>>>>>>> So each vendor needs to put there their value.
>>>>>> If I understand Parav correctly, he was suggesting not to expose
>>>>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS to userspace if seeing (max_supported_vqs ==
>>>>>> 0) from the driver.
>>>>> I can see the reasoning, but maybe we should leave it as zero which
>>>>> means it was not reported. The user will then need to guess. I believe
>>>>> other vendors will follow with an update so this to a real value.
>>>> Unless you place a check in the vdpa core to enforce it on vdpa
>>>> creation, otherwise it's very likely to get ignored by other vendors.
>>>>
>>>>>> But meanwhile, I do wonder how users tell apart multiqueue supporting parent
>>>>>> from the single queue mgmtdev without getting the aid from this field. I
>>>>>> hope the answer won't be to create a vdpa instance to try.
>>>>>>
>>>>> Do you see a scenario that an admin decides to not instantiate vdpa just
>>>>> because it does not support MQ?
>>>> Yes, there is. If the hardware doesn't support MQ, the provisioning tool
>>>> in the mgmt software will need to fallback to software vhost backend
>>>> with mq=on. At the time the tool is checking out, it doesn't run with
>>>> root privilege.
>>>>
>>>>> And it the management device reports it does support, there's still no
>>>>> guarantee you'll end up with a MQ net device.
>>>> I'm not sure I follow. Do you mean it may be up to the guest feature
>>>> negotiation? But the device itself is still MQ capable, isn't it?
>>> I think we need to clarify the "device" here.
>>>
>>> For compatibility reasons, there could be a case that mgmt doesn't
>>> expect a mq capable vdpa device. So in this case, even if the parent
>>> is MQ capable, the vdpa isn't.
>> Right. The mgmt software is not necessarily libvirt. Perhaps I should be
>> explicit to say the mgmt software we're building would definitely create MQ
>> vdpa device in case on a MQ capable parent.
> OK, to recap:
>
> 1. I think waht you're asking for is to see what the parent device (e.g.
> mlx5_vdpa) is going to report to the virtio driver in the features, at
> the management device.
> So how about I add a features field to struct vdpa_mgmt_dev and return
> it in netlink. Userspace will present it as shown in patch 0008 in v6.
Yes, that's exactly what I want.

>
> 2. Si-Wei, you mentioned you want this information to be available to
> non privileged user. This is the case today.
Yep. As long as it doesn't need to involve creating a vdpa to check out....

Thanks!
-Siwei

>
>> -Siwei
>>
>>> Thanks
>>>
>>>> Thanks,
>>>> -Siwei
>>>>
>>>>>> -Siwei
>>>>>>
>>>>>>>> This is what is exposed to the user to decide the upper bound.
>>>>>>>>>> There has been some talk/patches of rdma virtio device.
>>>>>>>>>> I anticipate such device to support more than 64K queues by nature of rdma.
>>>>>>>>>> It is better to keep max_supported_vqs as u32.
>>>>>>>>> Why not add it when we have it?
>>>>>>>> Sure, with that approach we will end up adding two fields (current u16 and later another u32) due to smaller bit width of current one.
>>>>>>>> Either way is fine. Michael was suggesting similar higher bit-width in other patches, so bringing up here for this field on how he sees it.
>>>>>>> I can use u32 then.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 09/13] vdpa/mlx5: Restore cur_num_vqs in case of failure in change_num_qps()

[Si-Wei Liu]

On 12/21/2021 9:20 AM, Eli Cohen wrote:
> Restore ndev->cur_num_vqs to the original value in case change_num_qps()
> fails.
>
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>

Reviewed-by: Si-Wei Liu<si-wei.liu@oracle.com>

> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 848eabbb1831..72ed5efbe35d 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1536,9 +1536,11 @@ static int change_num_qps(struct mlx5_vdpa_dev *mvdev, int newqps)
>   	return 0;
>   
>   clean_added:
> -	for (--i; i >= cur_qps; --i)
> +	for (--i; i >= 2 * cur_qps; --i)
>   		teardown_vq(ndev, &ndev->vqs[i]);
>   
> +	ndev->cur_num_vqs = 2 * cur_qps;
> +
>   	return err;
>   }
>   

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 08/13] vdpa: Add support for returning device configuration information

[Si-Wei Liu]

On 12/21/2021 9:20 AM, Eli Cohen wrote:
> Add netlink attribute to store the negotiated features. This can be used
> by userspace to get the current state of the vdpa instance.
>
> Example:
>
> $ vdpa dev config show vdpa-a
> vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 4 \
>    mtu 1500 features c3182bc3182b
> CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ MQ \
>    CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
>   drivers/vdpa/vdpa.c       | 3 +++
>   include/uapi/linux/vdpa.h | 4 ++++
>   2 files changed, 7 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 45e4df4076da..eb223bec5209 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -839,6 +839,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>   		return -EMSGSIZE;
>   
>   	features = vdev->config->get_driver_features(vdev);
> +	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> +			      VDPA_ATTR_PAD))
Any reason why specify VDPA_ATTR_UNSPEC doesn't work here? ATTR_PAD for 
better naming?

Thanks,
-Siwei

> +		return -EMSGSIZE;
>   
>   	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
>   }
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index a252f06f9dfd..db3738ef3beb 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -23,6 +23,9 @@ enum vdpa_command {
>   enum vdpa_attr {
>   	VDPA_ATTR_UNSPEC,
>   
> +	/* Pad attribute for 64b alignment */
> +	VDPA_ATTR_PAD = VDPA_ATTR_UNSPEC,
> +
>   	/* bus name (optional) + dev name together make the parent device handle */
>   	VDPA_ATTR_MGMTDEV_BUS_NAME,		/* string */
>   	VDPA_ATTR_MGMTDEV_DEV_NAME,		/* string */
> @@ -40,6 +43,7 @@ enum vdpa_attr {
>   	VDPA_ATTR_DEV_NET_CFG_MAX_VQP,		/* u16 */
>   	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
>   
> +	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>   	/* new attributes must be added above here */
>   	VDPA_ATTR_MAX,
>   };

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 07/13] vdpa/mlx5: Support configuring max data virtqueue pairs

[Si-Wei Liu]

On 12/21/2021 9:20 AM, Eli Cohen wrote:
> Check whether the max number of data virtqueue pairs was provided when a
> adding a new device and verify the new value does not exceed device
> capabilities.
>
> In addition, change the arrays holding virtqueue and callback contexts
> to be dynamically allocated.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 43 +++++++++++++++++++++----------
>   1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 4a2149f70f1e..848eabbb1831 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
>   	struct mlx5_vq_restore_info ri;
>   };
>   
> -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
> - * provides for driver space allocation
> - */
> -#define MLX5_MAX_SUPPORTED_VQS 16
> -
>   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
>   {
>   	if (unlikely(idx > mvdev->max_idx))
> @@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
>   	struct mlx5_vdpa_dev mvdev;
>   	struct mlx5_vdpa_net_resources res;
>   	struct virtio_net_config config;
> -	struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
> -	struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
> +	struct mlx5_vdpa_virtqueue *vqs;
> +	struct vdpa_callback *event_cbs;
>   
>   	/* Serialize vq resources creation and destruction. This is required
>   	 * since memory map might change and we need to destroy and create
> @@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net *ndev)
>   {
>   	int i;
>   
> -	for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
> +	for (i = 0; i < ndev->mvdev.max_vqs; i++)
>   		suspend_vq(ndev, &ndev->vqs[i]);
>   }
>   
> @@ -1245,7 +1240,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>   	int i, j;
>   	int err;
>   
> -	max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
> +	max_rqt = min_t(int, roundup_pow_of_two(ndev->mvdev.max_vqs  / 2),
>   			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>   	if (max_rqt < 1)
>   		return -EOPNOTSUPP;
> @@ -2220,7 +2215,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>   	clear_vqs_ready(ndev);
>   	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   	ndev->mvdev.status = 0;
> -	memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
> +	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
>   	ndev->mvdev.actual_features = 0;
>   	++mvdev->generation;
>   	if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {
> @@ -2293,6 +2288,8 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
>   	}
>   	mlx5_vdpa_free_resources(&ndev->mvdev);
>   	mutex_destroy(&ndev->reslock);
> +	kfree(ndev->event_cbs);
> +	kfree(ndev->vqs);
>   }
>   
>   static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)
> @@ -2538,15 +2535,33 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   		return -EOPNOTSUPP;
>   	}
>   
> -	/* we save one virtqueue for control virtqueue should we require it */
>   	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> -	max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
> +	if (max_vqs < 2) {
> +		dev_warn(mdev->device,
> +			 "%d virtqueues are supported. At least 2 are required\n",
> +			 max_vqs);
> +		return -EAGAIN;
> +	}
> +
> +	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
> +		if (add_config->net.max_vq_pairs > max_vqs / 2)
> +			return -EINVAL;
> +		max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
> +	} else {
> +		max_vqs = 2;
> +	}
>   
>   	ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops,
>   				 name, false);
>   	if (IS_ERR(ndev))
>   		return PTR_ERR(ndev);
>   
> +	ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
> +	ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
> +	if (!ndev->vqs || !ndev->event_cbs) {
> +		err = -ENOMEM;
> +		goto err_alloc;
> +	}
>   	ndev->mvdev.max_vqs = max_vqs;
>   	mvdev = &ndev->mvdev;
>   	mvdev->mdev = mdev;
> @@ -2627,6 +2642,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   		mlx5_mpfs_del_mac(pfmdev, config->mac);
>   err_mtu:
>   	mutex_destroy(&ndev->reslock);
> +err_alloc:
>   	put_device(&mvdev->vdev.dev);
>   	return err;
>   }
> @@ -2669,7 +2685,8 @@ static int mlx5v_probe(struct auxiliary_device *adev,
>   	mgtdev->mgtdev.ops = &mdev_ops;
>   	mgtdev->mgtdev.device = mdev->device;
>   	mgtdev->mgtdev.id_table = id_table;
> -	mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> +	mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
> +					  BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>   	mgtdev->madev = madev;
>   
>   	err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 11/13] vdpa/mlx5: Configure max supported virtqueues

[Si-Wei Liu]

On 12/21/2021 9:20 AM, Eli Cohen wrote:
> Configure max supported virtqueues on the management device. This info
> can be retrieved using:
>
> $ vdpa mgmtdev show
> auxiliary/mlx5_core.sf.1:
>    supported_classes net
>    max_supported_vqs 257
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 72ed5efbe35d..67fa74cfaad8 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2689,6 +2689,8 @@ static int mlx5v_probe(struct auxiliary_device *adev,
>   	mgtdev->mgtdev.id_table = id_table;
>   	mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
>   					  BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> +	mgtdev->mgtdev.max_supported_vqs =
> +		MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues) + 1;
>   	mgtdev->madev = madev;
>   
>   	err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v5 13/13] vdpa: Use BIT_ULL for bit operations

[Si-Wei Liu]

On 12/21/2021 9:20 AM, Eli Cohen wrote:
> All masks in this file are 64 bits. Change BIT to BIT_ULL.
>
> Other occurences use (1 << val) which yields a 32 bit value. Change them
> to use BIT_ULL too.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/vdpa.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 4b649125a038..84ee11393349 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -576,9 +576,9 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
>   	return msg->len;
>   }
>   
> -#define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> -				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
> -				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
> +#define VDPA_DEV_NET_ATTRS_MASK (BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
> +				 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)     | \
> +				 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>   
>   static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
>   {
> @@ -597,12 +597,12 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>   	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
>   		macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
>   		memcpy(config.net.mac, macaddr, sizeof(config.net.mac));
> -		config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR);
> +		config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
>   	}
>   	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]) {
>   		config.net.mtu =
>   			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
> -		config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
> +		config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU);
>   	}
>   	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
>   		config.net.max_vq_pairs =
> @@ -814,7 +814,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
>   {
>   	u16 val_u16;
>   
> -	if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0)
> +	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
>   		return 0;
>   
>   	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization