[PATCH RESEND V2 1/3] vdpa: mlx5: prevent cvq work from hogging CPU

[PATCH RESEND V2 1/3] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

A userspace triggerable infinite loop could happen in
mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
cvq requests.

Fixing this by introducing a quota and re-queue the work if we're out
of the budget (currently the implicit budget is one) . While at it,
using a per device work struct to avoid on demand memory allocation
for cvq.

Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- Using 1 as the budget
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index d0f91078600e..b2afd2b6fbca 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
 	u32 cur_num_vqs;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
+	struct mlx5_vdpa_wq_ent cvq_ent;
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -1616,10 +1617,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 	cvq = &mvdev->cvq;
 	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
-		goto out;
+		return;
 
 	if (!cvq->ready)
-		goto out;
+		return;
 
 	while (true) {
 		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1653,9 +1654,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 
 		if (vringh_need_notify_iotlb(&cvq->vring))
 			vringh_notify(&cvq->vring);
+
+		queue_work(mvdev->wq, &wqent->work);
+		break;
 	}
-out:
-	kfree(wqent);
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -1663,7 +1665,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	struct mlx5_vdpa_virtqueue *mvq;
-	struct mlx5_vdpa_wq_ent *wqent;
 
 	if (!is_index_valid(mvdev, idx))
 		return;
@@ -1672,13 +1673,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
 		if (!mvdev->cvq.ready)
 			return;
 
-		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
-		if (!wqent)
-			return;
-
-		wqent->mvdev = mvdev;
-		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
-		queue_work(mvdev->wq, &wqent->work);
+		queue_work(mvdev->wq, &ndev->cvq_ent.work);
 		return;
 	}
 
@@ -2668,6 +2663,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	if (err)
 		goto err_mr;
 
+	ndev->cvq_ent.mvdev = mvdev;
+	INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
 	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
 	if (!mvdev->wq) {
 		err = -ENOMEM;
-- 
2.18.1

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

[PATCH RESEND V2 1/3] vdpa: mlx5: prevent cvq work from hogging CPU

[Jason Wang]

A userspace triggerable infinite loop could happen in
mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
cvq requests.

Fixing this by introducing a quota and re-queue the work if we're out
of the budget (currently the implicit budget is one) . While at it,
using a per device work struct to avoid on demand memory allocation
for cvq.

Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- Using 1 as the budget
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index d0f91078600e..b2afd2b6fbca 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
 	u32 cur_num_vqs;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
+	struct mlx5_vdpa_wq_ent cvq_ent;
 };
 
 static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -1616,10 +1617,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 	cvq = &mvdev->cvq;
 	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
-		goto out;
+		return;
 
 	if (!cvq->ready)
-		goto out;
+		return;
 
 	while (true) {
 		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1653,9 +1654,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 
 		if (vringh_need_notify_iotlb(&cvq->vring))
 			vringh_notify(&cvq->vring);
+
+		queue_work(mvdev->wq, &wqent->work);
+		break;
 	}
-out:
-	kfree(wqent);
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -1663,7 +1665,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	struct mlx5_vdpa_virtqueue *mvq;
-	struct mlx5_vdpa_wq_ent *wqent;
 
 	if (!is_index_valid(mvdev, idx))
 		return;
@@ -1672,13 +1673,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
 		if (!mvdev->cvq.ready)
 			return;
 
-		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
-		if (!wqent)
-			return;
-
-		wqent->mvdev = mvdev;
-		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
-		queue_work(mvdev->wq, &wqent->work);
+		queue_work(mvdev->wq, &ndev->cvq_ent.work);
 		return;
 	}
 
@@ -2668,6 +2663,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	if (err)
 		goto err_mr;
 
+	ndev->cvq_ent.mvdev = mvdev;
+	INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
 	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
 	if (!mvdev->wq) {
 		err = -ENOMEM;
-- 
2.18.1

[PATCH RESEND V2 2/3] vdpa: mlx5: synchronize driver status with CVQ

[Jason Wang]

Currently, CVQ doesn't have any synchronization with the driver
status. Then CVQ emulation code run in the middle of:

1) device reset
2) device status changed
3) map updating

The will lead several unexpected issue like trying to execute CVQ
command after the driver has been teared down.

Fixing this by using reslock to synchronize CVQ emulation code with
the driver status changing:

- protect the whole device reset, status changing and set_map()
  updating with reslock
- protect the CVQ handler with the reslock and check
  VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler

This will guarantee that:

1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
2) CVQ handler will see a consistent state of the driver instead of
   the partial one when it is running in the middle of the
   teardown_driver() or setup_driver().

Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- document the lock requirement
- protect the whole .set_map()
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b2afd2b6fbca..53b8c1a68f90 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1616,11 +1616,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 	mvdev = wqent->mvdev;
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 	cvq = &mvdev->cvq;
+
+	mutex_lock(&ndev->reslock);
+
+	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
+		goto out;
+
 	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
-		return;
+		goto out;
 
 	if (!cvq->ready)
-		return;
+		goto out;
 
 	while (true) {
 		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1658,6 +1664,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 		queue_work(mvdev->wq, &wqent->work);
 		break;
 	}
+
+out:
+	mutex_unlock(&ndev->reslock);
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -2132,7 +2141,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
 		goto err_mr;
 
 	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
-		return 0;
+		goto err_mr;
 
 	restore_channels_info(ndev);
 	err = setup_driver(mvdev);
@@ -2147,12 +2156,14 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
 	return err;
 }
 
+/* reslock must be held for this function */
 static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 {
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	int err;
 
-	mutex_lock(&ndev->reslock);
+	WARN_ON(!mutex_is_locked(&ndev->reslock));
+
 	if (ndev->setup) {
 		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
 		err = 0;
@@ -2182,7 +2193,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 		goto err_fwd;
 	}
 	ndev->setup = true;
-	mutex_unlock(&ndev->reslock);
 
 	return 0;
 
@@ -2193,23 +2203,23 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 err_rqt:
 	teardown_virtqueues(ndev);
 out:
-	mutex_unlock(&ndev->reslock);
 	return err;
 }
 
+/* reslock must be held for this function */
 static void teardown_driver(struct mlx5_vdpa_net *ndev)
 {
-	mutex_lock(&ndev->reslock);
+
+	WARN_ON(!mutex_is_locked(&ndev->reslock));
+
 	if (!ndev->setup)
-		goto out;
+		return;
 
 	remove_fwd_to_tir(ndev);
 	destroy_tir(ndev);
 	destroy_rqt(ndev);
 	teardown_virtqueues(ndev);
 	ndev->setup = false;
-out:
-	mutex_unlock(&ndev->reslock);
 }
 
 static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
@@ -2230,6 +2240,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 
 	print_status(mvdev, status, true);
 
+	mutex_lock(&ndev->reslock);
+
 	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
 		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
 			err = setup_driver(mvdev);
@@ -2239,16 +2251,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 			}
 		} else {
 			mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
-			return;
+			goto err_clear;
 		}
 	}
 
 	ndev->mvdev.status = status;
+	mutex_unlock(&ndev->reslock);
 	return;
 
 err_setup:
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
 	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
+err_clear:
+	mutex_unlock(&ndev->reslock);
 }
 
 static int mlx5_vdpa_reset(struct vdpa_device *vdev)
@@ -2258,6 +2273,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 
 	print_status(mvdev, 0, true);
 	mlx5_vdpa_info(mvdev, "performing device reset\n");
+
+	mutex_lock(&ndev->reslock);
 	teardown_driver(ndev);
 	clear_vqs_ready(ndev);
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
@@ -2270,6 +2287,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 		if (mlx5_vdpa_create_mr(mvdev, NULL))
 			mlx5_vdpa_warn(mvdev, "create MR failed\n");
 	}
+	mutex_unlock(&ndev->reslock);
 
 	return 0;
 }
@@ -2305,19 +2323,24 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
 static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb)
 {
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	bool change_map;
 	int err;
 
+	mutex_lock(&ndev->reslock);
+
 	err = mlx5_vdpa_handle_set_map(mvdev, iotlb, &change_map);
 	if (err) {
 		mlx5_vdpa_warn(mvdev, "set map failed(%d)\n", err);
-		return err;
+		goto err;
 	}
 
 	if (change_map)
-		return mlx5_vdpa_change_map(mvdev, iotlb);
+		err = mlx5_vdpa_change_map(mvdev, iotlb);
 
-	return 0;
+err:
+	mutex_unlock(&ndev->reslock);
+	return err;
 }
 
 static void mlx5_vdpa_free(struct vdpa_device *vdev)
-- 
2.18.1

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

[PATCH RESEND V2 2/3] vdpa: mlx5: synchronize driver status with CVQ

[Jason Wang]

Currently, CVQ doesn't have any synchronization with the driver
status. Then CVQ emulation code run in the middle of:

1) device reset
2) device status changed
3) map updating

The will lead several unexpected issue like trying to execute CVQ
command after the driver has been teared down.

Fixing this by using reslock to synchronize CVQ emulation code with
the driver status changing:

- protect the whole device reset, status changing and set_map()
  updating with reslock
- protect the CVQ handler with the reslock and check
  VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler

This will guarantee that:

1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
2) CVQ handler will see a consistent state of the driver instead of
   the partial one when it is running in the middle of the
   teardown_driver() or setup_driver().

Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- document the lock requirement
- protect the whole .set_map()
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b2afd2b6fbca..53b8c1a68f90 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1616,11 +1616,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 	mvdev = wqent->mvdev;
 	ndev = to_mlx5_vdpa_ndev(mvdev);
 	cvq = &mvdev->cvq;
+
+	mutex_lock(&ndev->reslock);
+
+	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
+		goto out;
+
 	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
-		return;
+		goto out;
 
 	if (!cvq->ready)
-		return;
+		goto out;
 
 	while (true) {
 		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1658,6 +1664,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
 		queue_work(mvdev->wq, &wqent->work);
 		break;
 	}
+
+out:
+	mutex_unlock(&ndev->reslock);
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -2132,7 +2141,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
 		goto err_mr;
 
 	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
-		return 0;
+		goto err_mr;
 
 	restore_channels_info(ndev);
 	err = setup_driver(mvdev);
@@ -2147,12 +2156,14 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
 	return err;
 }
 
+/* reslock must be held for this function */
 static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 {
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	int err;
 
-	mutex_lock(&ndev->reslock);
+	WARN_ON(!mutex_is_locked(&ndev->reslock));
+
 	if (ndev->setup) {
 		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
 		err = 0;
@@ -2182,7 +2193,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 		goto err_fwd;
 	}
 	ndev->setup = true;
-	mutex_unlock(&ndev->reslock);
 
 	return 0;
 
@@ -2193,23 +2203,23 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
 err_rqt:
 	teardown_virtqueues(ndev);
 out:
-	mutex_unlock(&ndev->reslock);
 	return err;
 }
 
+/* reslock must be held for this function */
 static void teardown_driver(struct mlx5_vdpa_net *ndev)
 {
-	mutex_lock(&ndev->reslock);
+
+	WARN_ON(!mutex_is_locked(&ndev->reslock));
+
 	if (!ndev->setup)
-		goto out;
+		return;
 
 	remove_fwd_to_tir(ndev);
 	destroy_tir(ndev);
 	destroy_rqt(ndev);
 	teardown_virtqueues(ndev);
 	ndev->setup = false;
-out:
-	mutex_unlock(&ndev->reslock);
 }
 
 static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
@@ -2230,6 +2240,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 
 	print_status(mvdev, status, true);
 
+	mutex_lock(&ndev->reslock);
+
 	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
 		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
 			err = setup_driver(mvdev);
@@ -2239,16 +2251,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 			}
 		} else {
 			mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
-			return;
+			goto err_clear;
 		}
 	}
 
 	ndev->mvdev.status = status;
+	mutex_unlock(&ndev->reslock);
 	return;
 
 err_setup:
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
 	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
+err_clear:
+	mutex_unlock(&ndev->reslock);
 }
 
 static int mlx5_vdpa_reset(struct vdpa_device *vdev)
@@ -2258,6 +2273,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 
 	print_status(mvdev, 0, true);
 	mlx5_vdpa_info(mvdev, "performing device reset\n");
+
+	mutex_lock(&ndev->reslock);
 	teardown_driver(ndev);
 	clear_vqs_ready(ndev);
 	mlx5_vdpa_destroy_mr(&ndev->mvdev);
@@ -2270,6 +2287,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
 		if (mlx5_vdpa_create_mr(mvdev, NULL))
 			mlx5_vdpa_warn(mvdev, "create MR failed\n");
 	}
+	mutex_unlock(&ndev->reslock);
 
 	return 0;
 }
@@ -2305,19 +2323,24 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
 static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb)
 {
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 	bool change_map;
 	int err;
 
+	mutex_lock(&ndev->reslock);
+
 	err = mlx5_vdpa_handle_set_map(mvdev, iotlb, &change_map);
 	if (err) {
 		mlx5_vdpa_warn(mvdev, "set map failed(%d)\n", err);
-		return err;
+		goto err;
 	}
 
 	if (change_map)
-		return mlx5_vdpa_change_map(mvdev, iotlb);
+		err = mlx5_vdpa_change_map(mvdev, iotlb);
 
-	return 0;
+err:
+	mutex_unlock(&ndev->reslock);
+	return err;
 }
 
 static void mlx5_vdpa_free(struct vdpa_device *vdev)
-- 
2.18.1

RE: [PATCH RESEND V2 2/3] vdpa: mlx5: synchronize driver status with CVQ

[Eli Cohen]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, March 29, 2022 7:21 AM
> To: jasowang@redhat.com; mst@redhat.com
> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: [PATCH RESEND V2 2/3] vdpa: mlx5: synchronize driver status with CVQ
> 
> Currently, CVQ doesn't have any synchronization with the driver
> status. Then CVQ emulation code run in the middle of:
> 
> 1) device reset
> 2) device status changed
> 3) map updating
> 
> The will lead several unexpected issue like trying to execute CVQ
> command after the driver has been teared down.
> 
> Fixing this by using reslock to synchronize CVQ emulation code with
> the driver status changing:
> 
> - protect the whole device reset, status changing and set_map()
>   updating with reslock
> - protect the CVQ handler with the reslock and check
>   VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler
> 
> This will guarantee that:
> 
> 1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
> 2) CVQ handler will see a consistent state of the driver instead of
>    the partial one when it is running in the middle of the
>    teardown_driver() or setup_driver().
> 
> Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Eli Cohen <elic@nvidia.com>

> ---
> Changes since V1:
> - document the lock requirement
> - protect the whole .set_map()
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b2afd2b6fbca..53b8c1a68f90 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1616,11 +1616,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  	mvdev = wqent->mvdev;
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
>  	cvq = &mvdev->cvq;
> +
> +	mutex_lock(&ndev->reslock);
> +
> +	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> +		goto out;
> +
>  	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> -		return;
> +		goto out;
> 
>  	if (!cvq->ready)
> -		return;
> +		goto out;
> 
>  	while (true) {
>  		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1658,6 +1664,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  		queue_work(mvdev->wq, &wqent->work);
>  		break;
>  	}
> +
> +out:
> +	mutex_unlock(&ndev->reslock);
>  }
> 
>  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -2132,7 +2141,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>  		goto err_mr;
> 
>  	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> -		return 0;
> +		goto err_mr;
> 
>  	restore_channels_info(ndev);
>  	err = setup_driver(mvdev);
> @@ -2147,12 +2156,14 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
>  	return err;
>  }
> 
> +/* reslock must be held for this function */
>  static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  {
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	int err;
> 
> -	mutex_lock(&ndev->reslock);
> +	WARN_ON(!mutex_is_locked(&ndev->reslock));
> +
>  	if (ndev->setup) {
>  		mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
>  		err = 0;
> @@ -2182,7 +2193,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  		goto err_fwd;
>  	}
>  	ndev->setup = true;
> -	mutex_unlock(&ndev->reslock);
> 
>  	return 0;
> 
> @@ -2193,23 +2203,23 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
>  err_rqt:
>  	teardown_virtqueues(ndev);
>  out:
> -	mutex_unlock(&ndev->reslock);
>  	return err;
>  }
> 
> +/* reslock must be held for this function */
>  static void teardown_driver(struct mlx5_vdpa_net *ndev)
>  {
> -	mutex_lock(&ndev->reslock);
> +
> +	WARN_ON(!mutex_is_locked(&ndev->reslock));
> +
>  	if (!ndev->setup)
> -		goto out;
> +		return;
> 
>  	remove_fwd_to_tir(ndev);
>  	destroy_tir(ndev);
>  	destroy_rqt(ndev);
>  	teardown_virtqueues(ndev);
>  	ndev->setup = false;
> -out:
> -	mutex_unlock(&ndev->reslock);
>  }
> 
>  static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> @@ -2230,6 +2240,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> 
>  	print_status(mvdev, status, true);
> 
> +	mutex_lock(&ndev->reslock);
> +
>  	if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
>  		if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>  			err = setup_driver(mvdev);
> @@ -2239,16 +2251,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>  			}
>  		} else {
>  			mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
> -			return;
> +			goto err_clear;
>  		}
>  	}
> 
>  	ndev->mvdev.status = status;
> +	mutex_unlock(&ndev->reslock);
>  	return;
> 
>  err_setup:
>  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>  	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> +err_clear:
> +	mutex_unlock(&ndev->reslock);
>  }
> 
>  static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> @@ -2258,6 +2273,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> 
>  	print_status(mvdev, 0, true);
>  	mlx5_vdpa_info(mvdev, "performing device reset\n");
> +
> +	mutex_lock(&ndev->reslock);
>  	teardown_driver(ndev);
>  	clear_vqs_ready(ndev);
>  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2270,6 +2287,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>  		if (mlx5_vdpa_create_mr(mvdev, NULL))
>  			mlx5_vdpa_warn(mvdev, "create MR failed\n");
>  	}
> +	mutex_unlock(&ndev->reslock);
> 
>  	return 0;
>  }
> @@ -2305,19 +2323,24 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>  static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb)
>  {
>  	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> +	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	bool change_map;
>  	int err;
> 
> +	mutex_lock(&ndev->reslock);
> +
>  	err = mlx5_vdpa_handle_set_map(mvdev, iotlb, &change_map);
>  	if (err) {
>  		mlx5_vdpa_warn(mvdev, "set map failed(%d)\n", err);
> -		return err;
> +		goto err;
>  	}
> 
>  	if (change_map)
> -		return mlx5_vdpa_change_map(mvdev, iotlb);
> +		err = mlx5_vdpa_change_map(mvdev, iotlb);
> 
> -	return 0;
> +err:
> +	mutex_unlock(&ndev->reslock);
> +	return err;
>  }
> 
>  static void mlx5_vdpa_free(struct vdpa_device *vdev)
> --
> 2.18.1

[PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Jason Wang]

From: Eli Cohen <elic@nvidia.com>

The current code evaluates RQT size based on the configured number of
virtqueues. This can raise an issue in the following scenario:

Assume MQ was negotiated.
1. mlx5_vdpa_set_map() gets called.
2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
   than the configured max VQs.
3. A second set_map gets called, but now a smaller number of VQs is used
   to evaluate the size of the RQT.
4. handle_ctrl_mq() is called with a value larger than what the RQT can
   hold. This will emit errors and the driver state is compromised.

To fix this, we use a new field in struct mlx5_vdpa_net to hold the
required number of entries in the RQT. This value is evaluated in
mlx5_vdpa_set_driver_features() where we have the negotiated features
all set up.

In addtion to that, we take into consideration the max capability of RQT
entries early when the device is added so we don't need to take consider
it when creating the RQT.

Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
max_vas / 2 and make the code clearer.

Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
 1 file changed, 21 insertions(+), 40 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 53b8c1a68f90..61bec1ed0bc9 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
 	struct mlx5_flow_handle *rx_rule_mcast;
 	bool setup;
 	u32 cur_num_vqs;
+	u32 rqt_size;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
 	struct mlx5_vdpa_wq_ent cvq_ent;
@@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
 	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
 }
 
-static inline u32 mlx5_vdpa_max_qps(int max_vqs)
-{
-	return max_vqs / 2;
-}
-
 static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
 {
 	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
 		return 2;
 
-	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
+	return mvdev->max_vqs;
 }
 
 static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
@@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
 static int create_rqt(struct mlx5_vdpa_net *ndev)
 {
 	__be32 *list;
-	int max_rqt;
 	void *rqtc;
 	int inlen;
 	void *in;
 	int i, j;
 	int err;
-	int num;
-
-	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
-		num = 1;
-	else
-		num = ndev->cur_num_vqs / 2;
 
-	max_rqt = min_t(int, roundup_pow_of_two(num),
-			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
-	if (max_rqt < 1)
-		return -EOPNOTSUPP;
-
-	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
+	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
 	in = kzalloc(inlen, GFP_KERNEL);
 	if (!in)
 		return -ENOMEM;
@@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
 	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
 
 	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
-	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
+	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
 	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
-	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
+	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
+		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
 
-	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
+	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
 	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
 	kfree(in);
 	if (err)
@@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
 static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
 {
 	__be32 *list;
-	int max_rqt;
 	void *rqtc;
 	int inlen;
 	void *in;
 	int i, j;
 	int err;
 
-	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
-			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
-	if (max_rqt < 1)
-		return -EOPNOTSUPP;
-
-	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
+	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
 	in = kzalloc(inlen, GFP_KERNEL);
 	if (!in)
 		return -ENOMEM;
@@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
 	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
 
 	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
-	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
+	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
 		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
 
-	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
+	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
 	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
 	kfree(in);
 	if (err)
@@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
 		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
-		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
+		    newqps > ndev->rqt_size)
 			break;
 
 		if (ndev->cur_num_vqs == 2 * newqps) {
@@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
 	int err;
 	int i;
 
-	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
+	for (i = 0; i < mvdev->max_vqs; i++) {
 		err = setup_vq(ndev, &ndev->vqs[i]);
 		if (err)
 			goto err_vq;
@@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
 
 	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
 	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
-		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
+		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
 	else
-		ndev->cur_num_vqs = 2;
+		ndev->rqt_size = 1;
+
+	ndev->cur_num_vqs = 2 * ndev->rqt_size;
 
 	update_cvq_info(mvdev);
 	return err;
@@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
 	struct mlx5_vdpa_virtqueue *mvq;
 	int i;
 
-	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
+	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
 		mvq = &ndev->vqs[i];
 		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
 		mvq->index = i;
@@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 		return -EOPNOTSUPP;
 	}
 
-	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
+	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
+			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
 	if (max_vqs < 2) {
 		dev_warn(mdev->device,
 			 "%d virtqueues are supported. At least 2 are required\n",
@@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
 	}
 
-	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
+	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
 	mvdev->vdev.dma_dev = &mdev->pdev->dev;
 	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
 	if (err)
@@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	ndev->nb.notifier_call = event_handler;
 	mlx5_notifier_register(mdev, &ndev->nb);
 	mvdev->vdev.mdev = &mgtdev->mgtdev;
-	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
+	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
 	if (err)
 		goto err_reg;
 
-- 
2.18.1

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

[PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Jason Wang]

From: Eli Cohen <elic@nvidia.com>

The current code evaluates RQT size based on the configured number of
virtqueues. This can raise an issue in the following scenario:

Assume MQ was negotiated.
1. mlx5_vdpa_set_map() gets called.
2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
   than the configured max VQs.
3. A second set_map gets called, but now a smaller number of VQs is used
   to evaluate the size of the RQT.
4. handle_ctrl_mq() is called with a value larger than what the RQT can
   hold. This will emit errors and the driver state is compromised.

To fix this, we use a new field in struct mlx5_vdpa_net to hold the
required number of entries in the RQT. This value is evaluated in
mlx5_vdpa_set_driver_features() where we have the negotiated features
all set up.

In addtion to that, we take into consideration the max capability of RQT
entries early when the device is added so we don't need to take consider
it when creating the RQT.

Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
max_vas / 2 and make the code clearer.

Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
 1 file changed, 21 insertions(+), 40 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 53b8c1a68f90..61bec1ed0bc9 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
 	struct mlx5_flow_handle *rx_rule_mcast;
 	bool setup;
 	u32 cur_num_vqs;
+	u32 rqt_size;
 	struct notifier_block nb;
 	struct vdpa_callback config_cb;
 	struct mlx5_vdpa_wq_ent cvq_ent;
@@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
 	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
 }
 
-static inline u32 mlx5_vdpa_max_qps(int max_vqs)
-{
-	return max_vqs / 2;
-}
-
 static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
 {
 	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
 		return 2;
 
-	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
+	return mvdev->max_vqs;
 }
 
 static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
@@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
 static int create_rqt(struct mlx5_vdpa_net *ndev)
 {
 	__be32 *list;
-	int max_rqt;
 	void *rqtc;
 	int inlen;
 	void *in;
 	int i, j;
 	int err;
-	int num;
-
-	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
-		num = 1;
-	else
-		num = ndev->cur_num_vqs / 2;
 
-	max_rqt = min_t(int, roundup_pow_of_two(num),
-			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
-	if (max_rqt < 1)
-		return -EOPNOTSUPP;
-
-	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
+	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
 	in = kzalloc(inlen, GFP_KERNEL);
 	if (!in)
 		return -ENOMEM;
@@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
 	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
 
 	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
-	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
+	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
 	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
-	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
+	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
+		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
 
-	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
+	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
 	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
 	kfree(in);
 	if (err)
@@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
 static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
 {
 	__be32 *list;
-	int max_rqt;
 	void *rqtc;
 	int inlen;
 	void *in;
 	int i, j;
 	int err;
 
-	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
-			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
-	if (max_rqt < 1)
-		return -EOPNOTSUPP;
-
-	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
+	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
 	in = kzalloc(inlen, GFP_KERNEL);
 	if (!in)
 		return -ENOMEM;
@@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
 	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
 
 	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
-	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
+	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
 		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
 
-	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
+	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
 	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
 	kfree(in);
 	if (err)
@@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
 		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
-		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
+		    newqps > ndev->rqt_size)
 			break;
 
 		if (ndev->cur_num_vqs == 2 * newqps) {
@@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
 	int err;
 	int i;
 
-	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
+	for (i = 0; i < mvdev->max_vqs; i++) {
 		err = setup_vq(ndev, &ndev->vqs[i]);
 		if (err)
 			goto err_vq;
@@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
 
 	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
 	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
-		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
+		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
 	else
-		ndev->cur_num_vqs = 2;
+		ndev->rqt_size = 1;
+
+	ndev->cur_num_vqs = 2 * ndev->rqt_size;
 
 	update_cvq_info(mvdev);
 	return err;
@@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
 	struct mlx5_vdpa_virtqueue *mvq;
 	int i;
 
-	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
+	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
 		mvq = &ndev->vqs[i];
 		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
 		mvq->index = i;
@@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 		return -EOPNOTSUPP;
 	}
 
-	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
+	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
+			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
 	if (max_vqs < 2) {
 		dev_warn(mdev->device,
 			 "%d virtqueues are supported. At least 2 are required\n",
@@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
 	}
 
-	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
+	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
 	mvdev->vdev.dma_dev = &mdev->pdev->dev;
 	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
 	if (err)
@@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	ndev->nb.notifier_call = event_handler;
 	mlx5_notifier_register(mdev, &ndev->nb);
 	mvdev->vdev.mdev = &mgtdev->mgtdev;
-	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
+	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
 	if (err)
 		goto err_reg;
 
-- 
2.18.1

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Michael S. Tsirkin]

On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> From: Eli Cohen <elic@nvidia.com>
> 
> The current code evaluates RQT size based on the configured number of
> virtqueues. This can raise an issue in the following scenario:
> 
> Assume MQ was negotiated.
> 1. mlx5_vdpa_set_map() gets called.
> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
>    than the configured max VQs.
> 3. A second set_map gets called, but now a smaller number of VQs is used
>    to evaluate the size of the RQT.
> 4. handle_ctrl_mq() is called with a value larger than what the RQT can
>    hold. This will emit errors and the driver state is compromised.
> 
> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> required number of entries in the RQT. This value is evaluated in
> mlx5_vdpa_set_driver_features() where we have the negotiated features
> all set up.
> 
> In addtion

addition?

> to that, we take into consideration the max capability of RQT
> entries early when the device is added so we don't need to take consider
> it when creating the RQT.
> 
> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> max_vas / 2 and make the code clearer.
> 
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Signed-off-by: Eli Cohen <elic@nvidia.com>

Jason I don't have your ack or S.O.B on this one.


> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
>  1 file changed, 21 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 53b8c1a68f90..61bec1ed0bc9 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
>  	struct mlx5_flow_handle *rx_rule_mcast;
>  	bool setup;
>  	u32 cur_num_vqs;
> +	u32 rqt_size;
>  	struct notifier_block nb;
>  	struct vdpa_callback config_cb;
>  	struct mlx5_vdpa_wq_ent cvq_ent;
> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>  }
>  
> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> -{
> -	return max_vqs / 2;
> -}
> -
>  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>  {
>  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>  		return 2;
>  
> -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> +	return mvdev->max_vqs;
>  }
>  
>  static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>  static int create_rqt(struct mlx5_vdpa_net *ndev)
>  {
>  	__be32 *list;
> -	int max_rqt;
>  	void *rqtc;
>  	int inlen;
>  	void *in;
>  	int i, j;
>  	int err;
> -	int num;
> -
> -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> -		num = 1;
> -	else
> -		num = ndev->cur_num_vqs / 2;
>  
> -	max_rqt = min_t(int, roundup_pow_of_two(num),
> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> -	if (max_rqt < 1)
> -		return -EOPNOTSUPP;
> -
> -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>  	in = kzalloc(inlen, GFP_KERNEL);
>  	if (!in)
>  		return -ENOMEM;
> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>  	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>  
>  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
>  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
>  
> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>  	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
>  	kfree(in);
>  	if (err)
> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>  static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>  {
>  	__be32 *list;
> -	int max_rqt;
>  	void *rqtc;
>  	int inlen;
>  	void *in;
>  	int i, j;
>  	int err;
>  
> -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> -	if (max_rqt < 1)
> -		return -EOPNOTSUPP;
> -
> -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>  	in = kzalloc(inlen, GFP_KERNEL);
>  	if (!in)
>  		return -ENOMEM;
> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>  
>  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>  		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>  
> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>  	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
>  	kfree(in);
>  	if (err)
> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>  
>  		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>  		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> +		    newqps > ndev->rqt_size)
>  			break;
>  
>  		if (ndev->cur_num_vqs == 2 * newqps) {
> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
>  	int err;
>  	int i;
>  
> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> +	for (i = 0; i < mvdev->max_vqs; i++) {
>  		err = setup_vq(ndev, &ndev->vqs[i]);
>  		if (err)
>  			goto err_vq;
> @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>  
>  	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
>  	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>  	else
> -		ndev->cur_num_vqs = 2;
> +		ndev->rqt_size = 1;
> +
> +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
>  
>  	update_cvq_info(mvdev);
>  	return err;
> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
>  	struct mlx5_vdpa_virtqueue *mvq;
>  	int i;
>  
> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
> +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
>  		mvq = &ndev->vqs[i];
>  		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
>  		mvq->index = i;
> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
> +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
>  	if (max_vqs < 2) {
>  		dev_warn(mdev->device,
>  			 "%d virtqueues are supported. At least 2 are required\n",
> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>  	}
>  
> -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
> +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
>  	mvdev->vdev.dma_dev = &mdev->pdev->dev;
>  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>  	if (err)
> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	ndev->nb.notifier_call = event_handler;
>  	mlx5_notifier_register(mdev, &ndev->nb);
>  	mvdev->vdev.mdev = &mgtdev->mgtdev;
> -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>  	if (err)
>  		goto err_reg;
>  
> -- 
> 2.18.1

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

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Michael S. Tsirkin]

On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> From: Eli Cohen <elic@nvidia.com>
> 
> The current code evaluates RQT size based on the configured number of
> virtqueues. This can raise an issue in the following scenario:
> 
> Assume MQ was negotiated.
> 1. mlx5_vdpa_set_map() gets called.
> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
>    than the configured max VQs.
> 3. A second set_map gets called, but now a smaller number of VQs is used
>    to evaluate the size of the RQT.
> 4. handle_ctrl_mq() is called with a value larger than what the RQT can
>    hold. This will emit errors and the driver state is compromised.
> 
> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> required number of entries in the RQT. This value is evaluated in
> mlx5_vdpa_set_driver_features() where we have the negotiated features
> all set up.
> 
> In addtion

addition?

> to that, we take into consideration the max capability of RQT
> entries early when the device is added so we don't need to take consider
> it when creating the RQT.
> 
> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> max_vas / 2 and make the code clearer.
> 
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Signed-off-by: Eli Cohen <elic@nvidia.com>

Jason I don't have your ack or S.O.B on this one.


> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
>  1 file changed, 21 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 53b8c1a68f90..61bec1ed0bc9 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
>  	struct mlx5_flow_handle *rx_rule_mcast;
>  	bool setup;
>  	u32 cur_num_vqs;
> +	u32 rqt_size;
>  	struct notifier_block nb;
>  	struct vdpa_callback config_cb;
>  	struct mlx5_vdpa_wq_ent cvq_ent;
> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>  }
>  
> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> -{
> -	return max_vqs / 2;
> -}
> -
>  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>  {
>  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>  		return 2;
>  
> -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> +	return mvdev->max_vqs;
>  }
>  
>  static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>  static int create_rqt(struct mlx5_vdpa_net *ndev)
>  {
>  	__be32 *list;
> -	int max_rqt;
>  	void *rqtc;
>  	int inlen;
>  	void *in;
>  	int i, j;
>  	int err;
> -	int num;
> -
> -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> -		num = 1;
> -	else
> -		num = ndev->cur_num_vqs / 2;
>  
> -	max_rqt = min_t(int, roundup_pow_of_two(num),
> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> -	if (max_rqt < 1)
> -		return -EOPNOTSUPP;
> -
> -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>  	in = kzalloc(inlen, GFP_KERNEL);
>  	if (!in)
>  		return -ENOMEM;
> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>  	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>  
>  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
>  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
>  
> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>  	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
>  	kfree(in);
>  	if (err)
> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>  static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>  {
>  	__be32 *list;
> -	int max_rqt;
>  	void *rqtc;
>  	int inlen;
>  	void *in;
>  	int i, j;
>  	int err;
>  
> -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> -	if (max_rqt < 1)
> -		return -EOPNOTSUPP;
> -
> -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>  	in = kzalloc(inlen, GFP_KERNEL);
>  	if (!in)
>  		return -ENOMEM;
> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>  
>  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>  		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>  
> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>  	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
>  	kfree(in);
>  	if (err)
> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>  
>  		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>  		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> +		    newqps > ndev->rqt_size)
>  			break;
>  
>  		if (ndev->cur_num_vqs == 2 * newqps) {
> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
>  	int err;
>  	int i;
>  
> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> +	for (i = 0; i < mvdev->max_vqs; i++) {
>  		err = setup_vq(ndev, &ndev->vqs[i]);
>  		if (err)
>  			goto err_vq;
> @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>  
>  	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
>  	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>  	else
> -		ndev->cur_num_vqs = 2;
> +		ndev->rqt_size = 1;
> +
> +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
>  
>  	update_cvq_info(mvdev);
>  	return err;
> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
>  	struct mlx5_vdpa_virtqueue *mvq;
>  	int i;
>  
> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
> +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
>  		mvq = &ndev->vqs[i];
>  		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
>  		mvq->index = i;
> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
> +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
>  	if (max_vqs < 2) {
>  		dev_warn(mdev->device,
>  			 "%d virtqueues are supported. At least 2 are required\n",
> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>  	}
>  
> -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
> +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
>  	mvdev->vdev.dma_dev = &mdev->pdev->dev;
>  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>  	if (err)
> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	ndev->nb.notifier_call = event_handler;
>  	mlx5_notifier_register(mdev, &ndev->nb);
>  	mvdev->vdev.mdev = &mgtdev->mgtdev;
> -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>  	if (err)
>  		goto err_reg;
>  
> -- 
> 2.18.1

RE: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Eli Cohen]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, April 4, 2022 1:35 PM
> To: Jason Wang <jasowang@redhat.com>
> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> 
> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> > From: Eli Cohen <elic@nvidia.com>
> >
> > The current code evaluates RQT size based on the configured number of
> > virtqueues. This can raise an issue in the following scenario:
> >
> > Assume MQ was negotiated.
> > 1. mlx5_vdpa_set_map() gets called.
> > 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
> >    than the configured max VQs.
> > 3. A second set_map gets called, but now a smaller number of VQs is used
> >    to evaluate the size of the RQT.
> > 4. handle_ctrl_mq() is called with a value larger than what the RQT can
> >    hold. This will emit errors and the driver state is compromised.
> >
> > To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> > required number of entries in the RQT. This value is evaluated in
> > mlx5_vdpa_set_driver_features() where we have the negotiated features
> > all set up.
> >
> > In addtion
> 
> addition?

Do you need me to send another version?
If so, let's wait for Jason's reply.

> 
> > to that, we take into consideration the max capability of RQT
> > entries early when the device is added so we don't need to take consider
> > it when creating the RQT.
> >
> > Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> > max_vas / 2 and make the code clearer.
> >
> > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> 
> Jason I don't have your ack or S.O.B on this one.
> 
> 
> > ---
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
> >  1 file changed, 21 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 53b8c1a68f90..61bec1ed0bc9 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> >  	struct mlx5_flow_handle *rx_rule_mcast;
> >  	bool setup;
> >  	u32 cur_num_vqs;
> > +	u32 rqt_size;
> >  	struct notifier_block nb;
> >  	struct vdpa_callback config_cb;
> >  	struct mlx5_vdpa_wq_ent cvq_ent;
> > @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
> >  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> >  }
> >
> > -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> > -{
> > -	return max_vqs / 2;
> > -}
> > -
> >  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> >  {
> >  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >  		return 2;
> >
> > -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> > +	return mvdev->max_vqs;
> >  }
> >
> >  static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> > @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> >  static int create_rqt(struct mlx5_vdpa_net *ndev)
> >  {
> >  	__be32 *list;
> > -	int max_rqt;
> >  	void *rqtc;
> >  	int inlen;
> >  	void *in;
> >  	int i, j;
> >  	int err;
> > -	int num;
> > -
> > -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > -		num = 1;
> > -	else
> > -		num = ndev->cur_num_vqs / 2;
> >
> > -	max_rqt = min_t(int, roundup_pow_of_two(num),
> > -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > -	if (max_rqt < 1)
> > -		return -EOPNOTSUPP;
> > -
> > -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> > +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
> >  	in = kzalloc(inlen, GFP_KERNEL);
> >  	if (!in)
> >  		return -ENOMEM;
> > @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> >  	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
> >
> >  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> > -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> > +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> >  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> > +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
> >
> > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> >  	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
> >  	kfree(in);
> >  	if (err)
> > @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> >  static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> >  {
> >  	__be32 *list;
> > -	int max_rqt;
> >  	void *rqtc;
> >  	int inlen;
> >  	void *in;
> >  	int i, j;
> >  	int err;
> >
> > -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> > -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > -	if (max_rqt < 1)
> > -		return -EOPNOTSUPP;
> > -
> > -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> > +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
> >  	in = kzalloc(inlen, GFP_KERNEL);
> >  	if (!in)
> >  		return -ENOMEM;
> > @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> >  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> >
> >  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> >  		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
> >
> > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> >  	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
> >  	kfree(in);
> >  	if (err)
> > @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >
> >  		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> >  		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> > -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> > +		    newqps > ndev->rqt_size)
> >  			break;
> >
> >  		if (ndev->cur_num_vqs == 2 * newqps) {
> > @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
> >  	int err;
> >  	int i;
> >
> > -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> > +	for (i = 0; i < mvdev->max_vqs; i++) {
> >  		err = setup_vq(ndev, &ndev->vqs[i]);
> >  		if (err)
> >  			goto err_vq;
> > @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> >
> >  	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> >  	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> > -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> > +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> >  	else
> > -		ndev->cur_num_vqs = 2;
> > +		ndev->rqt_size = 1;
> > +
> > +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
> >
> >  	update_cvq_info(mvdev);
> >  	return err;
> > @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> >  	struct mlx5_vdpa_virtqueue *mvq;
> >  	int i;
> >
> > -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
> > +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
> >  		mvq = &ndev->vqs[i];
> >  		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
> >  		mvq->index = i;
> > @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >  		return -EOPNOTSUPP;
> >  	}
> >
> > -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> > +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
> > +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
> >  	if (max_vqs < 2) {
> >  		dev_warn(mdev->device,
> >  			 "%d virtqueues are supported. At least 2 are required\n",
> > @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >  		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
> >  	}
> >
> > -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
> > +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
> >  	mvdev->vdev.dma_dev = &mdev->pdev->dev;
> >  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> >  	if (err)
> > @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> >  	ndev->nb.notifier_call = event_handler;
> >  	mlx5_notifier_register(mdev, &ndev->nb);
> >  	mvdev->vdev.mdev = &mgtdev->mgtdev;
> > -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> > +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> >  	if (err)
> >  		goto err_reg;
> >
> > --
> > 2.18.1

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Michael S. Tsirkin]

On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, April 4, 2022 1:35 PM
> > To: Jason Wang <jasowang@redhat.com>
> > Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> > 
> > On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> > > From: Eli Cohen <elic@nvidia.com>
> > >
> > > The current code evaluates RQT size based on the configured number of
> > > virtqueues. This can raise an issue in the following scenario:
> > >
> > > Assume MQ was negotiated.
> > > 1. mlx5_vdpa_set_map() gets called.
> > > 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
> > >    than the configured max VQs.
> > > 3. A second set_map gets called, but now a smaller number of VQs is used
> > >    to evaluate the size of the RQT.
> > > 4. handle_ctrl_mq() is called with a value larger than what the RQT can
> > >    hold. This will emit errors and the driver state is compromised.
> > >
> > > To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> > > required number of entries in the RQT. This value is evaluated in
> > > mlx5_vdpa_set_driver_features() where we have the negotiated features
> > > all set up.
> > >
> > > In addtion
> > 
> > addition?
> 
> Do you need me to send another version?

It's a bit easier that way but I can handle it manually too.

> If so, let's wait for Jason's reply.

Right.

> > 
> > > to that, we take into consideration the max capability of RQT
> > > entries early when the device is added so we don't need to take consider
> > > it when creating the RQT.
> > >
> > > Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> > > max_vas / 2 and make the code clearer.
> > >
> > > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > 
> > Jason I don't have your ack or S.O.B on this one.
> > 
> > 
> > > ---
> > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
> > >  1 file changed, 21 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 53b8c1a68f90..61bec1ed0bc9 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> > >  	struct mlx5_flow_handle *rx_rule_mcast;
> > >  	bool setup;
> > >  	u32 cur_num_vqs;
> > > +	u32 rqt_size;
> > >  	struct notifier_block nb;
> > >  	struct vdpa_callback config_cb;
> > >  	struct mlx5_vdpa_wq_ent cvq_ent;
> > > @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
> > >  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> > >  }
> > >
> > > -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> > > -{
> > > -	return max_vqs / 2;
> > > -}
> > > -
> > >  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> > >  {
> > >  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > >  		return 2;
> > >
> > > -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > +	return mvdev->max_vqs;
> > >  }
> > >
> > >  static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> > > @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> > >  static int create_rqt(struct mlx5_vdpa_net *ndev)
> > >  {
> > >  	__be32 *list;
> > > -	int max_rqt;
> > >  	void *rqtc;
> > >  	int inlen;
> > >  	void *in;
> > >  	int i, j;
> > >  	int err;
> > > -	int num;
> > > -
> > > -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > > -		num = 1;
> > > -	else
> > > -		num = ndev->cur_num_vqs / 2;
> > >
> > > -	max_rqt = min_t(int, roundup_pow_of_two(num),
> > > -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > > -	if (max_rqt < 1)
> > > -		return -EOPNOTSUPP;
> > > -
> > > -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> > > +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
> > >  	in = kzalloc(inlen, GFP_KERNEL);
> > >  	if (!in)
> > >  		return -ENOMEM;
> > > @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> > >  	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
> > >
> > >  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> > > -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> > > +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> > >  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > > -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > > -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> > > +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > > +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
> > >
> > > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> > > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> > >  	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
> > >  	kfree(in);
> > >  	if (err)
> > > @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> > >  static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> > >  {
> > >  	__be32 *list;
> > > -	int max_rqt;
> > >  	void *rqtc;
> > >  	int inlen;
> > >  	void *in;
> > >  	int i, j;
> > >  	int err;
> > >
> > > -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> > > -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > > -	if (max_rqt < 1)
> > > -		return -EOPNOTSUPP;
> > > -
> > > -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> > > +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
> > >  	in = kzalloc(inlen, GFP_KERNEL);
> > >  	if (!in)
> > >  		return -ENOMEM;
> > > @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> > >  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> > >
> > >  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > > -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > > +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > >  		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
> > >
> > > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> > > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> > >  	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
> > >  	kfree(in);
> > >  	if (err)
> > > @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> > >
> > >  		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> > >  		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> > > -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> > > +		    newqps > ndev->rqt_size)
> > >  			break;
> > >
> > >  		if (ndev->cur_num_vqs == 2 * newqps) {
> > > @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
> > >  	int err;
> > >  	int i;
> > >
> > > -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> > > +	for (i = 0; i < mvdev->max_vqs; i++) {
> > >  		err = setup_vq(ndev, &ndev->vqs[i]);
> > >  		if (err)
> > >  			goto err_vq;
> > > @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > >
> > >  	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> > >  	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> > > +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> > >  	else
> > > -		ndev->cur_num_vqs = 2;
> > > +		ndev->rqt_size = 1;
> > > +
> > > +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
> > >
> > >  	update_cvq_info(mvdev);
> > >  	return err;
> > > @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > >  	struct mlx5_vdpa_virtqueue *mvq;
> > >  	int i;
> > >
> > > -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
> > > +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
> > >  		mvq = &ndev->vqs[i];
> > >  		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
> > >  		mvq->index = i;
> > > @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >  		return -EOPNOTSUPP;
> > >  	}
> > >
> > > -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> > > +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
> > > +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
> > >  	if (max_vqs < 2) {
> > >  		dev_warn(mdev->device,
> > >  			 "%d virtqueues are supported. At least 2 are required\n",
> > > @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >  		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
> > >  	}
> > >
> > > -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
> > > +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
> > >  	mvdev->vdev.dma_dev = &mdev->pdev->dev;
> > >  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > >  	if (err)
> > > @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >  	ndev->nb.notifier_call = event_handler;
> > >  	mlx5_notifier_register(mdev, &ndev->nb);
> > >  	mvdev->vdev.mdev = &mgtdev->mgtdev;
> > > -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> > > +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> > >  	if (err)
> > >  		goto err_reg;
> > >
> > > --
> > > 2.18.1

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

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Michael S. Tsirkin]

On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, April 4, 2022 1:35 PM
> > To: Jason Wang <jasowang@redhat.com>
> > Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> > 
> > On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> > > From: Eli Cohen <elic@nvidia.com>
> > >
> > > The current code evaluates RQT size based on the configured number of
> > > virtqueues. This can raise an issue in the following scenario:
> > >
> > > Assume MQ was negotiated.
> > > 1. mlx5_vdpa_set_map() gets called.
> > > 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
> > >    than the configured max VQs.
> > > 3. A second set_map gets called, but now a smaller number of VQs is used
> > >    to evaluate the size of the RQT.
> > > 4. handle_ctrl_mq() is called with a value larger than what the RQT can
> > >    hold. This will emit errors and the driver state is compromised.
> > >
> > > To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> > > required number of entries in the RQT. This value is evaluated in
> > > mlx5_vdpa_set_driver_features() where we have the negotiated features
> > > all set up.
> > >
> > > In addtion
> > 
> > addition?
> 
> Do you need me to send another version?

It's a bit easier that way but I can handle it manually too.

> If so, let's wait for Jason's reply.

Right.

> > 
> > > to that, we take into consideration the max capability of RQT
> > > entries early when the device is added so we don't need to take consider
> > > it when creating the RQT.
> > >
> > > Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> > > max_vas / 2 and make the code clearer.
> > >
> > > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > 
> > Jason I don't have your ack or S.O.B on this one.
> > 
> > 
> > > ---
> > >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
> > >  1 file changed, 21 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 53b8c1a68f90..61bec1ed0bc9 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> > >  	struct mlx5_flow_handle *rx_rule_mcast;
> > >  	bool setup;
> > >  	u32 cur_num_vqs;
> > > +	u32 rqt_size;
> > >  	struct notifier_block nb;
> > >  	struct vdpa_callback config_cb;
> > >  	struct mlx5_vdpa_wq_ent cvq_ent;
> > > @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
> > >  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
> > >  }
> > >
> > > -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> > > -{
> > > -	return max_vqs / 2;
> > > -}
> > > -
> > >  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> > >  {
> > >  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > >  		return 2;
> > >
> > > -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> > > +	return mvdev->max_vqs;
> > >  }
> > >
> > >  static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> > > @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> > >  static int create_rqt(struct mlx5_vdpa_net *ndev)
> > >  {
> > >  	__be32 *list;
> > > -	int max_rqt;
> > >  	void *rqtc;
> > >  	int inlen;
> > >  	void *in;
> > >  	int i, j;
> > >  	int err;
> > > -	int num;
> > > -
> > > -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> > > -		num = 1;
> > > -	else
> > > -		num = ndev->cur_num_vqs / 2;
> > >
> > > -	max_rqt = min_t(int, roundup_pow_of_two(num),
> > > -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > > -	if (max_rqt < 1)
> > > -		return -EOPNOTSUPP;
> > > -
> > > -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> > > +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
> > >  	in = kzalloc(inlen, GFP_KERNEL);
> > >  	if (!in)
> > >  		return -ENOMEM;
> > > @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> > >  	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
> > >
> > >  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> > > -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> > > +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> > >  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > > -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > > -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> > > +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > > +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
> > >
> > > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> > > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> > >  	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
> > >  	kfree(in);
> > >  	if (err)
> > > @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
> > >  static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> > >  {
> > >  	__be32 *list;
> > > -	int max_rqt;
> > >  	void *rqtc;
> > >  	int inlen;
> > >  	void *in;
> > >  	int i, j;
> > >  	int err;
> > >
> > > -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> > > -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> > > -	if (max_rqt < 1)
> > > -		return -EOPNOTSUPP;
> > > -
> > > -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
> > > +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
> > >  	in = kzalloc(inlen, GFP_KERNEL);
> > >  	if (!in)
> > >  		return -ENOMEM;
> > > @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> > >  	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> > >
> > >  	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> > > -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> > > +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> > >  		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
> > >
> > > -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> > > +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> > >  	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
> > >  	kfree(in);
> > >  	if (err)
> > > @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> > >
> > >  		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> > >  		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> > > -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> > > +		    newqps > ndev->rqt_size)
> > >  			break;
> > >
> > >  		if (ndev->cur_num_vqs == 2 * newqps) {
> > > @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
> > >  	int err;
> > >  	int i;
> > >
> > > -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> > > +	for (i = 0; i < mvdev->max_vqs; i++) {
> > >  		err = setup_vq(ndev, &ndev->vqs[i]);
> > >  		if (err)
> > >  			goto err_vq;
> > > @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > >
> > >  	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> > >  	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> > > +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> > >  	else
> > > -		ndev->cur_num_vqs = 2;
> > > +		ndev->rqt_size = 1;
> > > +
> > > +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
> > >
> > >  	update_cvq_info(mvdev);
> > >  	return err;
> > > @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
> > >  	struct mlx5_vdpa_virtqueue *mvq;
> > >  	int i;
> > >
> > > -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
> > > +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
> > >  		mvq = &ndev->vqs[i];
> > >  		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
> > >  		mvq->index = i;
> > > @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >  		return -EOPNOTSUPP;
> > >  	}
> > >
> > > -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
> > > +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
> > > +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
> > >  	if (max_vqs < 2) {
> > >  		dev_warn(mdev->device,
> > >  			 "%d virtqueues are supported. At least 2 are required\n",
> > > @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >  		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
> > >  	}
> > >
> > > -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
> > > +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
> > >  	mvdev->vdev.dma_dev = &mdev->pdev->dev;
> > >  	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > >  	if (err)
> > > @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > >  	ndev->nb.notifier_call = event_handler;
> > >  	mlx5_notifier_register(mdev, &ndev->nb);
> > >  	mvdev->vdev.mdev = &mgtdev->mgtdev;
> > > -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
> > > +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> > >  	if (err)
> > >  		goto err_reg;
> > >
> > > --
> > > 2.18.1

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Jason Wang]

在 2022/4/4 下午7:24, Michael S. Tsirkin 写道:
> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
>>> From: Michael S. Tsirkin <mst@redhat.com>
>>> Sent: Monday, April 4, 2022 1:35 PM
>>> To: Jason Wang <jasowang@redhat.com>
>>> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
>>>
>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
>>>> From: Eli Cohen <elic@nvidia.com>
>>>>
>>>> The current code evaluates RQT size based on the configured number of
>>>> virtqueues. This can raise an issue in the following scenario:
>>>>
>>>> Assume MQ was negotiated.
>>>> 1. mlx5_vdpa_set_map() gets called.
>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
>>>>     than the configured max VQs.
>>>> 3. A second set_map gets called, but now a smaller number of VQs is used
>>>>     to evaluate the size of the RQT.
>>>> 4. handle_ctrl_mq() is called with a value larger than what the RQT can
>>>>     hold. This will emit errors and the driver state is compromised.
>>>>
>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
>>>> required number of entries in the RQT. This value is evaluated in
>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features
>>>> all set up.
>>>>
>>>> In addtion
>>> addition?
>> Do you need me to send another version?
> It's a bit easier that way but I can handle it manually too.


Let me send a new version with this fixed.


>
>> If so, let's wait for Jason's reply.
> Right.
>
>>>> to that, we take into consideration the max capability of RQT
>>>> entries early when the device is added so we don't need to take consider
>>>> it when creating the RQT.
>>>>
>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
>>>> max_vas / 2 and make the code clearer.
>>>>
>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> Jason I don't have your ack or S.O.B on this one.


My bad, for some reason, I miss that.

Will fix.

Thanks


>>>
>>>
>>>> ---
>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
>>>>   1 file changed, 21 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> index 53b8c1a68f90..61bec1ed0bc9 100644
>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
>>>>   	struct mlx5_flow_handle *rx_rule_mcast;
>>>>   	bool setup;
>>>>   	u32 cur_num_vqs;
>>>> +	u32 rqt_size;
>>>>   	struct notifier_block nb;
>>>>   	struct vdpa_callback config_cb;
>>>>   	struct mlx5_vdpa_wq_ent cvq_ent;
>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>>>>   	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>>>>   }
>>>>
>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
>>>> -{
>>>> -	return max_vqs / 2;
>>>> -}
>>>> -
>>>>   static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>>>>   {
>>>>   	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>>   		return 2;
>>>>
>>>> -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>> +	return mvdev->max_vqs;
>>>>   }
>>>>
>>>>   static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>>>>   static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>   {
>>>>   	__be32 *list;
>>>> -	int max_rqt;
>>>>   	void *rqtc;
>>>>   	int inlen;
>>>>   	void *in;
>>>>   	int i, j;
>>>>   	int err;
>>>> -	int num;
>>>> -
>>>> -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>> -		num = 1;
>>>> -	else
>>>> -		num = ndev->cur_num_vqs / 2;
>>>>
>>>> -	max_rqt = min_t(int, roundup_pow_of_two(num),
>>>> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>> -	if (max_rqt < 1)
>>>> -		return -EOPNOTSUPP;
>>>> -
>>>> -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
>>>> +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>>>>   	in = kzalloc(inlen, GFP_KERNEL);
>>>>   	if (!in)
>>>>   		return -ENOMEM;
>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>   	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>>>>
>>>>   	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>> -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
>>>> +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
>>>>   	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>> -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
>>>> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>> +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
>>>>
>>>> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>   	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
>>>>   	kfree(in);
>>>>   	if (err)
>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>>>   {
>>>>   	__be32 *list;
>>>> -	int max_rqt;
>>>>   	void *rqtc;
>>>>   	int inlen;
>>>>   	void *in;
>>>>   	int i, j;
>>>>   	int err;
>>>>
>>>> -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
>>>> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>> -	if (max_rqt < 1)
>>>> -		return -EOPNOTSUPP;
>>>> -
>>>> -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
>>>> +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>>>>   	in = kzalloc(inlen, GFP_KERNEL);
>>>>   	if (!in)
>>>>   		return -ENOMEM;
>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>>>   	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>>
>>>>   	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>>   		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>>>>
>>>> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>   	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
>>>>   	kfree(in);
>>>>   	if (err)
>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>>>
>>>>   		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>>>>   		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>>>> -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
>>>> +		    newqps > ndev->rqt_size)
>>>>   			break;
>>>>
>>>>   		if (ndev->cur_num_vqs == 2 * newqps) {
>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
>>>>   	int err;
>>>>   	int i;
>>>>
>>>> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
>>>> +	for (i = 0; i < mvdev->max_vqs; i++) {
>>>>   		err = setup_vq(ndev, &ndev->vqs[i]);
>>>>   		if (err)
>>>>   			goto err_vq;
>>>> @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>>
>>>>   	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
>>>>   	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>>>> -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>>>> +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>>>>   	else
>>>> -		ndev->cur_num_vqs = 2;
>>>> +		ndev->rqt_size = 1;
>>>> +
>>>> +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
>>>>
>>>>   	update_cvq_info(mvdev);
>>>>   	return err;
>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
>>>>   	struct mlx5_vdpa_virtqueue *mvq;
>>>>   	int i;
>>>>
>>>> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
>>>> +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
>>>>   		mvq = &ndev->vqs[i];
>>>>   		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
>>>>   		mvq->index = i;
>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>>   		return -EOPNOTSUPP;
>>>>   	}
>>>>
>>>> -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
>>>> +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
>>>> +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
>>>>   	if (max_vqs < 2) {
>>>>   		dev_warn(mdev->device,
>>>>   			 "%d virtqueues are supported. At least 2 are required\n",
>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>>   		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>>>>   	}
>>>>
>>>> -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
>>>> +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
>>>>   	mvdev->vdev.dma_dev = &mdev->pdev->dev;
>>>>   	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>   	if (err)
>>>> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>>   	ndev->nb.notifier_call = event_handler;
>>>>   	mlx5_notifier_register(mdev, &ndev->nb);
>>>>   	mvdev->vdev.mdev = &mgtdev->mgtdev;
>>>> -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
>>>> +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>>>>   	if (err)
>>>>   		goto err_reg;
>>>>
>>>> --
>>>> 2.18.1

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

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Jason Wang]

在 2022/4/4 下午7:24, Michael S. Tsirkin 写道:
> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
>>> From: Michael S. Tsirkin <mst@redhat.com>
>>> Sent: Monday, April 4, 2022 1:35 PM
>>> To: Jason Wang <jasowang@redhat.com>
>>> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
>>>
>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
>>>> From: Eli Cohen <elic@nvidia.com>
>>>>
>>>> The current code evaluates RQT size based on the configured number of
>>>> virtqueues. This can raise an issue in the following scenario:
>>>>
>>>> Assume MQ was negotiated.
>>>> 1. mlx5_vdpa_set_map() gets called.
>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, lower
>>>>     than the configured max VQs.
>>>> 3. A second set_map gets called, but now a smaller number of VQs is used
>>>>     to evaluate the size of the RQT.
>>>> 4. handle_ctrl_mq() is called with a value larger than what the RQT can
>>>>     hold. This will emit errors and the driver state is compromised.
>>>>
>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
>>>> required number of entries in the RQT. This value is evaluated in
>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features
>>>> all set up.
>>>>
>>>> In addtion
>>> addition?
>> Do you need me to send another version?
> It's a bit easier that way but I can handle it manually too.


Let me send a new version with this fixed.


>
>> If so, let's wait for Jason's reply.
> Right.
>
>>>> to that, we take into consideration the max capability of RQT
>>>> entries early when the device is added so we don't need to take consider
>>>> it when creating the RQT.
>>>>
>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
>>>> max_vas / 2 and make the code clearer.
>>>>
>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>> Jason I don't have your ack or S.O.B on this one.


My bad, for some reason, I miss that.

Will fix.

Thanks


>>>
>>>
>>>> ---
>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 +++++++++++--------------------
>>>>   1 file changed, 21 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> index 53b8c1a68f90..61bec1ed0bc9 100644
>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
>>>>   	struct mlx5_flow_handle *rx_rule_mcast;
>>>>   	bool setup;
>>>>   	u32 cur_num_vqs;
>>>> +	u32 rqt_size;
>>>>   	struct notifier_block nb;
>>>>   	struct vdpa_callback config_cb;
>>>>   	struct mlx5_vdpa_wq_ent cvq_ent;
>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>>>>   	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>>>>   }
>>>>
>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
>>>> -{
>>>> -	return max_vqs / 2;
>>>> -}
>>>> -
>>>>   static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>>>>   {
>>>>   	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>>   		return 2;
>>>>
>>>> -	return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>> +	return mvdev->max_vqs;
>>>>   }
>>>>
>>>>   static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>>>>   static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>   {
>>>>   	__be32 *list;
>>>> -	int max_rqt;
>>>>   	void *rqtc;
>>>>   	int inlen;
>>>>   	void *in;
>>>>   	int i, j;
>>>>   	int err;
>>>> -	int num;
>>>> -
>>>> -	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>> -		num = 1;
>>>> -	else
>>>> -		num = ndev->cur_num_vqs / 2;
>>>>
>>>> -	max_rqt = min_t(int, roundup_pow_of_two(num),
>>>> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>> -	if (max_rqt < 1)
>>>> -		return -EOPNOTSUPP;
>>>> -
>>>> -	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
>>>> +	inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>>>>   	in = kzalloc(inlen, GFP_KERNEL);
>>>>   	if (!in)
>>>>   		return -ENOMEM;
>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>   	rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>>>>
>>>>   	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>> -	MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
>>>> +	MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
>>>>   	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>> -		list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
>>>> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>> +		list[i] = cpu_to_be32(ndev->vqs[j % ndev->cur_num_vqs].virtq_id);
>>>>
>>>> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>   	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);
>>>>   	kfree(in);
>>>>   	if (err)
>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>>>   {
>>>>   	__be32 *list;
>>>> -	int max_rqt;
>>>>   	void *rqtc;
>>>>   	int inlen;
>>>>   	void *in;
>>>>   	int i, j;
>>>>   	int err;
>>>>
>>>> -	max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
>>>> -			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>> -	if (max_rqt < 1)
>>>> -		return -EOPNOTSUPP;
>>>> -
>>>> -	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * MLX5_ST_SZ_BYTES(rq_num);
>>>> +	inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * MLX5_ST_SZ_BYTES(rq_num);
>>>>   	in = kzalloc(inlen, GFP_KERNEL);
>>>>   	if (!in)
>>>>   		return -ENOMEM;
>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>>>   	MLX5_SET(rqtc, rqtc, list_q_type, MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>>
>>>>   	list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>> -	for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>> +	for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>>   		list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>>>>
>>>> -	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>> +	MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>   	err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, ndev->res.rqtn);
>>>>   	kfree(in);
>>>>   	if (err)
>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>>>
>>>>   		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>>>>   		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>>>> -		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
>>>> +		    newqps > ndev->rqt_size)
>>>>   			break;
>>>>
>>>>   		if (ndev->cur_num_vqs == 2 * newqps) {
>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct mlx5_vdpa_dev *mvdev)
>>>>   	int err;
>>>>   	int i;
>>>>
>>>> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
>>>> +	for (i = 0; i < mvdev->max_vqs; i++) {
>>>>   		err = setup_vq(ndev, &ndev->vqs[i]);
>>>>   		if (err)
>>>>   			goto err_vq;
>>>> @@ -2017,9 +1995,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>>
>>>>   	ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
>>>>   	if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>>>> -		ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>>>> +		ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
>>>>   	else
>>>> -		ndev->cur_num_vqs = 2;
>>>> +		ndev->rqt_size = 1;
>>>> +
>>>> +	ndev->cur_num_vqs = 2 * ndev->rqt_size;
>>>>
>>>>   	update_cvq_info(mvdev);
>>>>   	return err;
>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
>>>>   	struct mlx5_vdpa_virtqueue *mvq;
>>>>   	int i;
>>>>
>>>> -	for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); ++i) {
>>>> +	for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
>>>>   		mvq = &ndev->vqs[i];
>>>>   		memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
>>>>   		mvq->index = i;
>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>>   		return -EOPNOTSUPP;
>>>>   	}
>>>>
>>>> -	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
>>>> +	max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues),
>>>> +			1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
>>>>   	if (max_vqs < 2) {
>>>>   		dev_warn(mdev->device,
>>>>   			 "%d virtqueues are supported. At least 2 are required\n",
>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>>   		ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>>>>   	}
>>>>
>>>> -	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, mlx5_vdpa_max_qps(max_vqs));
>>>> +	config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
>>>>   	mvdev->vdev.dma_dev = &mdev->pdev->dev;
>>>>   	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>   	if (err)
>>>> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>>>   	ndev->nb.notifier_call = event_handler;
>>>>   	mlx5_notifier_register(mdev, &ndev->nb);
>>>>   	mvdev->vdev.mdev = &mgtdev->mgtdev;
>>>> -	err = _vdpa_register_device(&mvdev->vdev, 2 * mlx5_vdpa_max_qps(max_vqs) + 1);
>>>> +	err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>>>>   	if (err)
>>>>   		goto err_reg;
>>>>
>>>> --
>>>> 2.18.1

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Jason Wang]

在 2022/4/6 上午10:35, Jason Wang 写道:
>
> 在 2022/4/4 下午7:24, Michael S. Tsirkin 写道:
>> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Monday, April 4, 2022 1:35 PM
>>>> To: Jason Wang <jasowang@redhat.com>
>>>> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; 
>>>> virtualization@lists.linux-foundation.org; 
>>>> linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
>>>>
>>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
>>>>> From: Eli Cohen <elic@nvidia.com>
>>>>>
>>>>> The current code evaluates RQT size based on the configured number of
>>>>> virtqueues. This can raise an issue in the following scenario:
>>>>>
>>>>> Assume MQ was negotiated.
>>>>> 1. mlx5_vdpa_set_map() gets called.
>>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, 
>>>>> lower
>>>>>     than the configured max VQs.
>>>>> 3. A second set_map gets called, but now a smaller number of VQs 
>>>>> is used
>>>>>     to evaluate the size of the RQT.
>>>>> 4. handle_ctrl_mq() is called with a value larger than what the 
>>>>> RQT can
>>>>>     hold. This will emit errors and the driver state is compromised.
>>>>>
>>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
>>>>> required number of entries in the RQT. This value is evaluated in
>>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features
>>>>> all set up.
>>>>>
>>>>> In addtion
>>>> addition?
>>> Do you need me to send another version?
>> It's a bit easier that way but I can handle it manually too.
>
>
> Let me send a new version with this fixed.


Ok, it looks like if I use git-send-email when From:tag is not me. The 
patch will be sent as a attachment as spotted by Maxime.

Eli, would you please send a v3 with my acked-by? (Since I don't want to 
change the author)

Thanks


>
>
>>
>>> If so, let's wait for Jason's reply.
>> Right.
>>
>>>>> to that, we take into consideration the max capability of RQT
>>>>> entries early when the device is added so we don't need to take 
>>>>> consider
>>>>> it when creating the RQT.
>>>>>
>>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
>>>>> max_vas / 2 and make the code clearer.
>>>>>
>>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>> Jason I don't have your ack or S.O.B on this one.
>
>
> My bad, for some reason, I miss that.
>
> Will fix.
>
> Thanks
>
>
>>>>
>>>>
>>>>> ---
>>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 
>>>>> +++++++++++--------------------
>>>>>   1 file changed, 21 insertions(+), 40 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>>>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> index 53b8c1a68f90..61bec1ed0bc9 100644
>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
>>>>>       struct mlx5_flow_handle *rx_rule_mcast;
>>>>>       bool setup;
>>>>>       u32 cur_num_vqs;
>>>>> +    u32 rqt_size;
>>>>>       struct notifier_block nb;
>>>>>       struct vdpa_callback config_cb;
>>>>>       struct mlx5_vdpa_wq_ent cvq_ent;
>>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct 
>>>>> mlx5_vdpa_dev *mvdev, u16 val)
>>>>>       return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), 
>>>>> val);
>>>>>   }
>>>>>
>>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
>>>>> -{
>>>>> -    return max_vqs / 2;
>>>>> -}
>>>>> -
>>>>>   static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>>>>>   {
>>>>>       if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>>>           return 2;
>>>>>
>>>>> -    return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>> +    return mvdev->max_vqs;
>>>>>   }
>>>>>
>>>>>   static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
>>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct 
>>>>> mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>>>>>   static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>>   {
>>>>>       __be32 *list;
>>>>> -    int max_rqt;
>>>>>       void *rqtc;
>>>>>       int inlen;
>>>>>       void *in;
>>>>>       int i, j;
>>>>>       int err;
>>>>> -    int num;
>>>>> -
>>>>> -    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>>> -        num = 1;
>>>>> -    else
>>>>> -        num = ndev->cur_num_vqs / 2;
>>>>>
>>>>> -    max_rqt = min_t(int, roundup_pow_of_two(num),
>>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>>> -    if (max_rqt < 1)
>>>>> -        return -EOPNOTSUPP;
>>>>> -
>>>>> -    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>> +    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>>       in = kzalloc(inlen, GFP_KERNEL);
>>>>>       if (!in)
>>>>>           return -ENOMEM;
>>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net 
>>>>> *ndev)
>>>>>       rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>>>>>
>>>>>       MLX5_SET(rqtc, rqtc, list_q_type, 
>>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>>> -    MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
>>>>> +    MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
>>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>>> -        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
>>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>>> +        list[i] = cpu_to_be32(ndev->vqs[j % 
>>>>> ndev->cur_num_vqs].virtq_id);
>>>>>
>>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>>       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, 
>>>>> &ndev->res.rqtn);
>>>>>       kfree(in);
>>>>>       if (err)
>>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net 
>>>>> *ndev)
>>>>>   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>>>>   {
>>>>>       __be32 *list;
>>>>> -    int max_rqt;
>>>>>       void *rqtc;
>>>>>       int inlen;
>>>>>       void *in;
>>>>>       int i, j;
>>>>>       int err;
>>>>>
>>>>> -    max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
>>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>>> -    if (max_rqt < 1)
>>>>> -        return -EOPNOTSUPP;
>>>>> -
>>>>> -    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>> +    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>>       in = kzalloc(inlen, GFP_KERNEL);
>>>>>       if (!in)
>>>>>           return -ENOMEM;
>>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net 
>>>>> *ndev, int num)
>>>>>       MLX5_SET(rqtc, rqtc, list_q_type, 
>>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>>>
>>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>>>           list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>>>>>
>>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>>       err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, 
>>>>> ndev->res.rqtn);
>>>>>       kfree(in);
>>>>>       if (err)
>>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack 
>>>>> handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>>>>
>>>>>           newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>>>>>           if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>>>>> -            newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
>>>>> +            newqps > ndev->rqt_size)
>>>>>               break;
>>>>>
>>>>>           if (ndev->cur_num_vqs == 2 * newqps) {
>>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct 
>>>>> mlx5_vdpa_dev *mvdev)
>>>>>       int err;
>>>>>       int i;
>>>>>
>>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
>>>>> +    for (i = 0; i < mvdev->max_vqs; i++) {
>>>>>           err = setup_vq(ndev, &ndev->vqs[i]);
>>>>>           if (err)
>>>>>               goto err_vq;
>>>>> @@ -2017,9 +1995,11 @@ static int 
>>>>> mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>>>
>>>>>       ndev->mvdev.actual_features = features & 
>>>>> ndev->mvdev.mlx_features;
>>>>>       if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>>>>> -        ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, 
>>>>> ndev->config.max_virtqueue_pairs);
>>>>> +        ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, 
>>>>> ndev->config.max_virtqueue_pairs);
>>>>>       else
>>>>> -        ndev->cur_num_vqs = 2;
>>>>> +        ndev->rqt_size = 1;
>>>>> +
>>>>> +    ndev->cur_num_vqs = 2 * ndev->rqt_size;
>>>>>
>>>>>       update_cvq_info(mvdev);
>>>>>       return err;
>>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net 
>>>>> *ndev)
>>>>>       struct mlx5_vdpa_virtqueue *mvq;
>>>>>       int i;
>>>>>
>>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); 
>>>>> ++i) {
>>>>> +    for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
>>>>>           mvq = &ndev->vqs[i];
>>>>>           memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
>>>>>           mvq->index = i;
>>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct 
>>>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>>>           return -EOPNOTSUPP;
>>>>>       }
>>>>>
>>>>> -    max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, 
>>>>> max_num_virtio_queues);
>>>>> +    max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, 
>>>>> max_num_virtio_queues),
>>>>> +            1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
>>>>>       if (max_vqs < 2) {
>>>>>           dev_warn(mdev->device,
>>>>>                "%d virtqueues are supported. At least 2 are 
>>>>> required\n",
>>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct 
>>>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>>>           ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>>>>>       }
>>>>>
>>>>> -    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, 
>>>>> mlx5_vdpa_max_qps(max_vqs));
>>>>> +    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, 
>>>>> max_vqs / 2);
>>>>>       mvdev->vdev.dma_dev = &mdev->pdev->dev;
>>>>>       err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>>       if (err)
>>>>> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct 
>>>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>>>       ndev->nb.notifier_call = event_handler;
>>>>>       mlx5_notifier_register(mdev, &ndev->nb);
>>>>>       mvdev->vdev.mdev = &mgtdev->mgtdev;
>>>>> -    err = _vdpa_register_device(&mvdev->vdev, 2 * 
>>>>> mlx5_vdpa_max_qps(max_vqs) + 1);
>>>>> +    err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>>>>>       if (err)
>>>>>           goto err_reg;
>>>>>
>>>>> -- 
>>>>> 2.18.1

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

Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Jason Wang]

在 2022/4/6 上午10:35, Jason Wang 写道:
>
> 在 2022/4/4 下午7:24, Michael S. Tsirkin 写道:
>> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Monday, April 4, 2022 1:35 PM
>>>> To: Jason Wang <jasowang@redhat.com>
>>>> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; 
>>>> virtualization@lists.linux-foundation.org; 
>>>> linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
>>>>
>>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
>>>>> From: Eli Cohen <elic@nvidia.com>
>>>>>
>>>>> The current code evaluates RQT size based on the configured number of
>>>>> virtqueues. This can raise an issue in the following scenario:
>>>>>
>>>>> Assume MQ was negotiated.
>>>>> 1. mlx5_vdpa_set_map() gets called.
>>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value, 
>>>>> lower
>>>>>     than the configured max VQs.
>>>>> 3. A second set_map gets called, but now a smaller number of VQs 
>>>>> is used
>>>>>     to evaluate the size of the RQT.
>>>>> 4. handle_ctrl_mq() is called with a value larger than what the 
>>>>> RQT can
>>>>>     hold. This will emit errors and the driver state is compromised.
>>>>>
>>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
>>>>> required number of entries in the RQT. This value is evaluated in
>>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features
>>>>> all set up.
>>>>>
>>>>> In addtion
>>>> addition?
>>> Do you need me to send another version?
>> It's a bit easier that way but I can handle it manually too.
>
>
> Let me send a new version with this fixed.


Ok, it looks like if I use git-send-email when From:tag is not me. The 
patch will be sent as a attachment as spotted by Maxime.

Eli, would you please send a v3 with my acked-by? (Since I don't want to 
change the author)

Thanks


>
>
>>
>>> If so, let's wait for Jason's reply.
>> Right.
>>
>>>>> to that, we take into consideration the max capability of RQT
>>>>> entries early when the device is added so we don't need to take 
>>>>> consider
>>>>> it when creating the RQT.
>>>>>
>>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
>>>>> max_vas / 2 and make the code clearer.
>>>>>
>>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>> Jason I don't have your ack or S.O.B on this one.
>
>
> My bad, for some reason, I miss that.
>
> Will fix.
>
> Thanks
>
>
>>>>
>>>>
>>>>> ---
>>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 61 
>>>>> +++++++++++--------------------
>>>>>   1 file changed, 21 insertions(+), 40 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>>>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> index 53b8c1a68f90..61bec1ed0bc9 100644
>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
>>>>>       struct mlx5_flow_handle *rx_rule_mcast;
>>>>>       bool setup;
>>>>>       u32 cur_num_vqs;
>>>>> +    u32 rqt_size;
>>>>>       struct notifier_block nb;
>>>>>       struct vdpa_callback config_cb;
>>>>>       struct mlx5_vdpa_wq_ent cvq_ent;
>>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct 
>>>>> mlx5_vdpa_dev *mvdev, u16 val)
>>>>>       return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), 
>>>>> val);
>>>>>   }
>>>>>
>>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
>>>>> -{
>>>>> -    return max_vqs / 2;
>>>>> -}
>>>>> -
>>>>>   static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>>>>>   {
>>>>>       if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>>>           return 2;
>>>>>
>>>>> -    return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
>>>>> +    return mvdev->max_vqs;
>>>>>   }
>>>>>
>>>>>   static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
>>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct 
>>>>> mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
>>>>>   static int create_rqt(struct mlx5_vdpa_net *ndev)
>>>>>   {
>>>>>       __be32 *list;
>>>>> -    int max_rqt;
>>>>>       void *rqtc;
>>>>>       int inlen;
>>>>>       void *in;
>>>>>       int i, j;
>>>>>       int err;
>>>>> -    int num;
>>>>> -
>>>>> -    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>>>>> -        num = 1;
>>>>> -    else
>>>>> -        num = ndev->cur_num_vqs / 2;
>>>>>
>>>>> -    max_rqt = min_t(int, roundup_pow_of_two(num),
>>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>>> -    if (max_rqt < 1)
>>>>> -        return -EOPNOTSUPP;
>>>>> -
>>>>> -    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>> +    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>>       in = kzalloc(inlen, GFP_KERNEL);
>>>>>       if (!in)
>>>>>           return -ENOMEM;
>>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net 
>>>>> *ndev)
>>>>>       rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
>>>>>
>>>>>       MLX5_SET(rqtc, rqtc, list_q_type, 
>>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>>> -    MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
>>>>> +    MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
>>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>>> -        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
>>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>>> +        list[i] = cpu_to_be32(ndev->vqs[j % 
>>>>> ndev->cur_num_vqs].virtq_id);
>>>>>
>>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>>       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, 
>>>>> &ndev->res.rqtn);
>>>>>       kfree(in);
>>>>>       if (err)
>>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net 
>>>>> *ndev)
>>>>>   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
>>>>>   {
>>>>>       __be32 *list;
>>>>> -    int max_rqt;
>>>>>       void *rqtc;
>>>>>       int inlen;
>>>>>       void *in;
>>>>>       int i, j;
>>>>>       int err;
>>>>>
>>>>> -    max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
>>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
>>>>> -    if (max_rqt < 1)
>>>>> -        return -EOPNOTSUPP;
>>>>> -
>>>>> -    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>> +    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size * 
>>>>> MLX5_ST_SZ_BYTES(rq_num);
>>>>>       in = kzalloc(inlen, GFP_KERNEL);
>>>>>       if (!in)
>>>>>           return -ENOMEM;
>>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net 
>>>>> *ndev, int num)
>>>>>       MLX5_SET(rqtc, rqtc, list_q_type, 
>>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
>>>>>
>>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
>>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
>>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
>>>>>           list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
>>>>>
>>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
>>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
>>>>>       err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen, 
>>>>> ndev->res.rqtn);
>>>>>       kfree(in);
>>>>>       if (err)
>>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack 
>>>>> handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>>>>
>>>>>           newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>>>>>           if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>>>>> -            newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
>>>>> +            newqps > ndev->rqt_size)
>>>>>               break;
>>>>>
>>>>>           if (ndev->cur_num_vqs == 2 * newqps) {
>>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct 
>>>>> mlx5_vdpa_dev *mvdev)
>>>>>       int err;
>>>>>       int i;
>>>>>
>>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
>>>>> +    for (i = 0; i < mvdev->max_vqs; i++) {
>>>>>           err = setup_vq(ndev, &ndev->vqs[i]);
>>>>>           if (err)
>>>>>               goto err_vq;
>>>>> @@ -2017,9 +1995,11 @@ static int 
>>>>> mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>>>>
>>>>>       ndev->mvdev.actual_features = features & 
>>>>> ndev->mvdev.mlx_features;
>>>>>       if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
>>>>> -        ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev, 
>>>>> ndev->config.max_virtqueue_pairs);
>>>>> +        ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, 
>>>>> ndev->config.max_virtqueue_pairs);
>>>>>       else
>>>>> -        ndev->cur_num_vqs = 2;
>>>>> +        ndev->rqt_size = 1;
>>>>> +
>>>>> +    ndev->cur_num_vqs = 2 * ndev->rqt_size;
>>>>>
>>>>>       update_cvq_info(mvdev);
>>>>>       return err;
>>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net 
>>>>> *ndev)
>>>>>       struct mlx5_vdpa_virtqueue *mvq;
>>>>>       int i;
>>>>>
>>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs); 
>>>>> ++i) {
>>>>> +    for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
>>>>>           mvq = &ndev->vqs[i];
>>>>>           memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
>>>>>           mvq->index = i;
>>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct 
>>>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>>>           return -EOPNOTSUPP;
>>>>>       }
>>>>>
>>>>> -    max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, 
>>>>> max_num_virtio_queues);
>>>>> +    max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev, 
>>>>> max_num_virtio_queues),
>>>>> +            1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
>>>>>       if (max_vqs < 2) {
>>>>>           dev_warn(mdev->device,
>>>>>                "%d virtqueues are supported. At least 2 are 
>>>>> required\n",
>>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct 
>>>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>>>           ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>>>>>       }
>>>>>
>>>>> -    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, 
>>>>> mlx5_vdpa_max_qps(max_vqs));
>>>>> +    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, 
>>>>> max_vqs / 2);
>>>>>       mvdev->vdev.dma_dev = &mdev->pdev->dev;
>>>>>       err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>>       if (err)
>>>>> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct 
>>>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>>>       ndev->nb.notifier_call = event_handler;
>>>>>       mlx5_notifier_register(mdev, &ndev->nb);
>>>>>       mvdev->vdev.mdev = &mgtdev->mgtdev;
>>>>> -    err = _vdpa_register_device(&mvdev->vdev, 2 * 
>>>>> mlx5_vdpa_max_qps(max_vqs) + 1);
>>>>> +    err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
>>>>>       if (err)
>>>>>           goto err_reg;
>>>>>
>>>>> -- 
>>>>> 2.18.1

RE: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size

[Eli Cohen]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, April 6, 2022 11:26 AM
> To: Michael S. Tsirkin <mst@redhat.com>; Eli Cohen <elic@nvidia.com>
> Cc: hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> 
> 
> 在 2022/4/6 上午10:35, Jason Wang 写道:
> >
> > 在 2022/4/4 下午7:24, Michael S. Tsirkin 写道:
> >> On Mon, Apr 04, 2022 at 11:07:36AM +0000, Eli Cohen wrote:
> >>>> From: Michael S. Tsirkin <mst@redhat.com>
> >>>> Sent: Monday, April 4, 2022 1:35 PM
> >>>> To: Jason Wang <jasowang@redhat.com>
> >>>> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com;
> >>>> virtualization@lists.linux-foundation.org;
> >>>> linux-kernel@vger.kernel.org
> >>>> Subject: Re: [PATCH RESEND V2 3/3] vdpa/mlx5: Use consistent RQT size
> >>>>
> >>>> On Tue, Mar 29, 2022 at 12:21:09PM +0800, Jason Wang wrote:
> >>>>> From: Eli Cohen <elic@nvidia.com>
> >>>>>
> >>>>> The current code evaluates RQT size based on the configured number of
> >>>>> virtqueues. This can raise an issue in the following scenario:
> >>>>>
> >>>>> Assume MQ was negotiated.
> >>>>> 1. mlx5_vdpa_set_map() gets called.
> >>>>> 2. handle_ctrl_mq() is called setting cur_num_vqs to some value,
> >>>>> lower
> >>>>>     than the configured max VQs.
> >>>>> 3. A second set_map gets called, but now a smaller number of VQs
> >>>>> is used
> >>>>>     to evaluate the size of the RQT.
> >>>>> 4. handle_ctrl_mq() is called with a value larger than what the
> >>>>> RQT can
> >>>>>     hold. This will emit errors and the driver state is compromised.
> >>>>>
> >>>>> To fix this, we use a new field in struct mlx5_vdpa_net to hold the
> >>>>> required number of entries in the RQT. This value is evaluated in
> >>>>> mlx5_vdpa_set_driver_features() where we have the negotiated features
> >>>>> all set up.
> >>>>>
> >>>>> In addtion
> >>>> addition?
> >>> Do you need me to send another version?
> >> It's a bit easier that way but I can handle it manually too.
> >
> >
> > Let me send a new version with this fixed.
> 
> 
> Ok, it looks like if I use git-send-email when From:tag is not me. The
> patch will be sent as a attachment as spotted by Maxime.
> 
> Eli, would you please send a v3 with my acked-by? (Since I don't want to
> change the author)
> 

Sure.

> Thanks
> 
> 
> >
> >
> >>
> >>> If so, let's wait for Jason's reply.
> >> Right.
> >>
> >>>>> to that, we take into consideration the max capability of RQT
> >>>>> entries early when the device is added so we don't need to take
> >>>>> consider
> >>>>> it when creating the RQT.
> >>>>>
> >>>>> Last, we remove the use of mlx5_vdpa_max_qps() which just returns the
> >>>>> max_vas / 2 and make the code clearer.
> >>>>>
> >>>>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> >>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> >>>> Jason I don't have your ack or S.O.B on this one.
> >
> >
> > My bad, for some reason, I miss that.
> >
> > Will fix.
> >
> > Thanks
> >
> >
> >>>>
> >>>>
> >>>>> ---
> >>>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 61
> >>>>> +++++++++++--------------------
> >>>>>   1 file changed, 21 insertions(+), 40 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> index 53b8c1a68f90..61bec1ed0bc9 100644
> >>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>>>> @@ -161,6 +161,7 @@ struct mlx5_vdpa_net {
> >>>>>       struct mlx5_flow_handle *rx_rule_mcast;
> >>>>>       bool setup;
> >>>>>       u32 cur_num_vqs;
> >>>>> +    u32 rqt_size;
> >>>>>       struct notifier_block nb;
> >>>>>       struct vdpa_callback config_cb;
> >>>>>       struct mlx5_vdpa_wq_ent cvq_ent;
> >>>>> @@ -204,17 +205,12 @@ static __virtio16 cpu_to_mlx5vdpa16(struct
> >>>>> mlx5_vdpa_dev *mvdev, u16 val)
> >>>>>       return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev),
> >>>>> val);
> >>>>>   }
> >>>>>
> >>>>> -static inline u32 mlx5_vdpa_max_qps(int max_vqs)
> >>>>> -{
> >>>>> -    return max_vqs / 2;
> >>>>> -}
> >>>>> -
> >>>>>   static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
> >>>>>   {
> >>>>>       if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >>>>>           return 2;
> >>>>>
> >>>>> -    return 2 * mlx5_vdpa_max_qps(mvdev->max_vqs);
> >>>>> +    return mvdev->max_vqs;
> >>>>>   }
> >>>>>
> >>>>>   static bool is_ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev, u16 idx)
> >>>>> @@ -1236,25 +1232,13 @@ static void teardown_vq(struct
> >>>>> mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> >>>>>   static int create_rqt(struct mlx5_vdpa_net *ndev)
> >>>>>   {
> >>>>>       __be32 *list;
> >>>>> -    int max_rqt;
> >>>>>       void *rqtc;
> >>>>>       int inlen;
> >>>>>       void *in;
> >>>>>       int i, j;
> >>>>>       int err;
> >>>>> -    int num;
> >>>>> -
> >>>>> -    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >>>>> -        num = 1;
> >>>>> -    else
> >>>>> -        num = ndev->cur_num_vqs / 2;
> >>>>>
> >>>>> -    max_rqt = min_t(int, roundup_pow_of_two(num),
> >>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> >>>>> -    if (max_rqt < 1)
> >>>>> -        return -EOPNOTSUPP;
> >>>>> -
> >>>>> -    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + max_rqt *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>> +    inlen = MLX5_ST_SZ_BYTES(create_rqt_in) + ndev->rqt_size *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>>       in = kzalloc(inlen, GFP_KERNEL);
> >>>>>       if (!in)
> >>>>>           return -ENOMEM;
> >>>>> @@ -1263,12 +1247,12 @@ static int create_rqt(struct mlx5_vdpa_net
> >>>>> *ndev)
> >>>>>       rqtc = MLX5_ADDR_OF(create_rqt_in, in, rqt_context);
> >>>>>
> >>>>>       MLX5_SET(rqtc, rqtc, list_q_type,
> >>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> >>>>> -    MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
> >>>>> +    MLX5_SET(rqtc, rqtc, rqt_max_size, ndev->rqt_size);
> >>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> >>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> >>>>> -        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);
> >>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> >>>>> +        list[i] = cpu_to_be32(ndev->vqs[j %
> >>>>> ndev->cur_num_vqs].virtq_id);
> >>>>>
> >>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> >>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> >>>>>       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,
> >>>>> &ndev->res.rqtn);
> >>>>>       kfree(in);
> >>>>>       if (err)
> >>>>> @@ -1282,19 +1266,13 @@ static int create_rqt(struct mlx5_vdpa_net
> >>>>> *ndev)
> >>>>>   static int modify_rqt(struct mlx5_vdpa_net *ndev, int num)
> >>>>>   {
> >>>>>       __be32 *list;
> >>>>> -    int max_rqt;
> >>>>>       void *rqtc;
> >>>>>       int inlen;
> >>>>>       void *in;
> >>>>>       int i, j;
> >>>>>       int err;
> >>>>>
> >>>>> -    max_rqt = min_t(int, roundup_pow_of_two(ndev->cur_num_vqs / 2),
> >>>>> -            1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
> >>>>> -    if (max_rqt < 1)
> >>>>> -        return -EOPNOTSUPP;
> >>>>> -
> >>>>> -    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + max_rqt *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>> +    inlen = MLX5_ST_SZ_BYTES(modify_rqt_in) + ndev->rqt_size *
> >>>>> MLX5_ST_SZ_BYTES(rq_num);
> >>>>>       in = kzalloc(inlen, GFP_KERNEL);
> >>>>>       if (!in)
> >>>>>           return -ENOMEM;
> >>>>> @@ -1305,10 +1283,10 @@ static int modify_rqt(struct mlx5_vdpa_net
> >>>>> *ndev, int num)
> >>>>>       MLX5_SET(rqtc, rqtc, list_q_type,
> >>>>> MLX5_RQTC_LIST_Q_TYPE_VIRTIO_NET_Q);
> >>>>>
> >>>>>       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
> >>>>> -    for (i = 0, j = 0; i < max_rqt; i++, j += 2)
> >>>>> +    for (i = 0, j = 0; i < ndev->rqt_size; i++, j += 2)
> >>>>>           list[i] = cpu_to_be32(ndev->vqs[j % num].virtq_id);
> >>>>>
> >>>>> -    MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
> >>>>> +    MLX5_SET(rqtc, rqtc, rqt_actual_size, ndev->rqt_size);
> >>>>>       err = mlx5_vdpa_modify_rqt(&ndev->mvdev, in, inlen,
> >>>>> ndev->res.rqtn);
> >>>>>       kfree(in);
> >>>>>       if (err)
> >>>>> @@ -1582,7 +1560,7 @@ static virtio_net_ctrl_ack
> >>>>> handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
> >>>>>
> >>>>>           newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> >>>>>           if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> >>>>> -            newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> >>>>> +            newqps > ndev->rqt_size)
> >>>>>               break;
> >>>>>
> >>>>>           if (ndev->cur_num_vqs == 2 * newqps) {
> >>>>> @@ -1946,7 +1924,7 @@ static int setup_virtqueues(struct
> >>>>> mlx5_vdpa_dev *mvdev)
> >>>>>       int err;
> >>>>>       int i;
> >>>>>
> >>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(mvdev->max_vqs); i++) {
> >>>>> +    for (i = 0; i < mvdev->max_vqs; i++) {
> >>>>>           err = setup_vq(ndev, &ndev->vqs[i]);
> >>>>>           if (err)
> >>>>>               goto err_vq;
> >>>>> @@ -2017,9 +1995,11 @@ static int
> >>>>> mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> >>>>>
> >>>>>       ndev->mvdev.actual_features = features &
> >>>>> ndev->mvdev.mlx_features;
> >>>>>       if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> >>>>> -        ndev->cur_num_vqs = 2 * mlx5vdpa16_to_cpu(mvdev,
> >>>>> ndev->config.max_virtqueue_pairs);
> >>>>> +        ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev,
> >>>>> ndev->config.max_virtqueue_pairs);
> >>>>>       else
> >>>>> -        ndev->cur_num_vqs = 2;
> >>>>> +        ndev->rqt_size = 1;
> >>>>> +
> >>>>> +    ndev->cur_num_vqs = 2 * ndev->rqt_size;
> >>>>>
> >>>>>       update_cvq_info(mvdev);
> >>>>>       return err;
> >>>>> @@ -2486,7 +2466,7 @@ static void init_mvqs(struct mlx5_vdpa_net
> >>>>> *ndev)
> >>>>>       struct mlx5_vdpa_virtqueue *mvq;
> >>>>>       int i;
> >>>>>
> >>>>> -    for (i = 0; i < 2 * mlx5_vdpa_max_qps(ndev->mvdev.max_vqs);
> >>>>> ++i) {
> >>>>> +    for (i = 0; i < ndev->mvdev.max_vqs; ++i) {
> >>>>>           mvq = &ndev->vqs[i];
> >>>>>           memset(mvq, 0, offsetof(struct mlx5_vdpa_virtqueue, ri));
> >>>>>           mvq->index = i;
> >>>>> @@ -2606,7 +2586,8 @@ static int mlx5_vdpa_dev_add(struct
> >>>>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>>>           return -EOPNOTSUPP;
> >>>>>       }
> >>>>>
> >>>>> -    max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
> >>>>> max_num_virtio_queues);
> >>>>> +    max_vqs = min_t(int, MLX5_CAP_DEV_VDPA_EMULATION(mdev,
> >>>>> max_num_virtio_queues),
> >>>>> +            1 << MLX5_CAP_GEN(mdev, log_max_rqt_size));
> >>>>>       if (max_vqs < 2) {
> >>>>>           dev_warn(mdev->device,
> >>>>>                "%d virtqueues are supported. At least 2 are
> >>>>> required\n",
> >>>>> @@ -2670,7 +2651,7 @@ static int mlx5_vdpa_dev_add(struct
> >>>>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>>>           ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
> >>>>>       }
> >>>>>
> >>>>> -    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev,
> >>>>> mlx5_vdpa_max_qps(max_vqs));
> >>>>> +    config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev,
> >>>>> max_vqs / 2);
> >>>>>       mvdev->vdev.dma_dev = &mdev->pdev->dev;
> >>>>>       err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> >>>>>       if (err)
> >>>>> @@ -2697,7 +2678,7 @@ static int mlx5_vdpa_dev_add(struct
> >>>>> vdpa_mgmt_dev *v_mdev, const char *name,
> >>>>>       ndev->nb.notifier_call = event_handler;
> >>>>>       mlx5_notifier_register(mdev, &ndev->nb);
> >>>>>       mvdev->vdev.mdev = &mgtdev->mgtdev;
> >>>>> -    err = _vdpa_register_device(&mvdev->vdev, 2 *
> >>>>> mlx5_vdpa_max_qps(max_vqs) + 1);
> >>>>> +    err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> >>>>>       if (err)
> >>>>>           goto err_reg;
> >>>>>
> >>>>> --
> >>>>> 2.18.1

RE: [PATCH RESEND V2 1/3] vdpa: mlx5: prevent cvq work from hogging CPU

[Eli Cohen]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, March 29, 2022 7:21 AM
> To: jasowang@redhat.com; mst@redhat.com
> Cc: Eli Cohen <elic@nvidia.com>; hdanton@sina.com; virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> Subject: [PATCH RESEND V2 1/3] vdpa: mlx5: prevent cvq work from hogging CPU
> 
> A userspace triggerable infinite loop could happen in
> mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> cvq requests.
> 
> Fixing this by introducing a quota and re-queue the work if we're out
> of the budget (currently the implicit budget is one) . While at it,
> using a per device work struct to avoid on demand memory allocation
> for cvq.
> 
> Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Eli Cohen <elic@nvidia.com>

> ---
> Changes since V1:
> - Using 1 as the budget
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d0f91078600e..b2afd2b6fbca 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
>  	u32 cur_num_vqs;
>  	struct notifier_block nb;
>  	struct vdpa_callback config_cb;
> +	struct mlx5_vdpa_wq_ent cvq_ent;
>  };
> 
>  static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -1616,10 +1617,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>  	ndev = to_mlx5_vdpa_ndev(mvdev);
>  	cvq = &mvdev->cvq;
>  	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> -		goto out;
> +		return;
> 
>  	if (!cvq->ready)
> -		goto out;
> +		return;
> 
>  	while (true) {
>  		err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1653,9 +1654,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> 
>  		if (vringh_need_notify_iotlb(&cvq->vring))
>  			vringh_notify(&cvq->vring);
> +
> +		queue_work(mvdev->wq, &wqent->work);
> +		break;
>  	}
> -out:
> -	kfree(wqent);
>  }
> 
>  static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -1663,7 +1665,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>  	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>  	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>  	struct mlx5_vdpa_virtqueue *mvq;
> -	struct mlx5_vdpa_wq_ent *wqent;
> 
>  	if (!is_index_valid(mvdev, idx))
>  		return;
> @@ -1672,13 +1673,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
>  		if (!mvdev->cvq.ready)
>  			return;
> 
> -		wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> -		if (!wqent)
> -			return;
> -
> -		wqent->mvdev = mvdev;
> -		INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> -		queue_work(mvdev->wq, &wqent->work);
> +		queue_work(mvdev->wq, &ndev->cvq_ent.work);
>  		return;
>  	}
> 
> @@ -2668,6 +2663,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	if (err)
>  		goto err_mr;
> 
> +	ndev->cvq_ent.mvdev = mvdev;
> +	INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
>  	mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
>  	if (!mvdev->wq) {
>  		err = -ENOMEM;
> --
> 2.18.1