Re: [PATCH] vdpa/mlx5: should not activate virtq object when suspended

From: Si-Wei Liu
Date: Mon Feb 13 2023 - 15:17:50 EST


+ Eugenio

On 2/11/2023 10:55 PM, Eli Cohen wrote:

On 11/02/2023 8:08, Si-Wei Liu wrote:
Otherwise the virtqueue object to instate could point to invalid address
that was unmapped from the MTT:

   mlx5_core 0000:41:04.2: mlx5_cmd_out_err:782:(pid 8321):
   CREATE_GENERAL_OBJECT(0xa00) op_mod(0xd) failed, status
   bad parameter(0x3), syndrome (0x5fa1c), err(-22)
I am familiar with this issue but I think it should be handled at the qemu level. This logic just hides the real problem. qemu provides the iova and the VQs' addresses so it should not shrink the map while such that VQ addresses are out of the iova range.

Here the whole device is already suspended, which should guarantee no further DMA memory access will be initiated by the device. What is the problem the mapping can't shrink in this case? Noted the code already allows shrinking for the reset case (~VIRTIO_CONFIG_S_DRIVER_OK status check), I see no essential difference the same couldn't be applied to the suspend case. On the other hand, map shrinking works well with platform IOMMU iommu_map/unmap() calls while vdpa device is suspended.

If your concern is that the device won't work with the shrunk map once the device is to be RESUME'd, I think I agree that's what QEMU has to guarantee, by either adding back the required mapping entries as needed, or change VQ addresses during suspend that points to mapped IOVA addresses. Or both can be done at the same time during suspend, with that we can build a fast path to switch svq mode with on-chip IOMMU e.g. mlx5_vdpa, rather than go through full device reset cycle.


While at it, add warning message to tell apart which object is
responsible for the CREATE_GENERAL_OBJECT command failure.

Fixes: cae15c2ed8e6 ("vdpa/mlx5: Implement susupend virtqueue callback")
Cc: Eli Cohen <elic@xxxxxxxxxx>
Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
---
  drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++++++++++++---
  1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 3a6dbbc6..c05c7f6 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -165,6 +165,7 @@ struct mlx5_vdpa_net {
      u32 cur_num_vqs;
      u32 rqt_size;
      bool nb_registered;
+    bool suspended;
      struct notifier_block nb;
      struct vdpa_callback config_cb;
      struct mlx5_vdpa_wq_ent cvq_ent;
@@ -1245,12 +1246,18 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
          goto err_connect;
        err = counter_set_alloc(ndev, mvq);
-    if (err)
+    if (err) {
+        mlx5_vdpa_warn(&ndev->mvdev, "failed to alloc counter on vq idx %d(%d)\n",
+                   idx, err);
Although you mention in the commit log, maybe it's better to have a separate patch for improving warning messages.
Alright, I will move the improved warning message out of this patch. This issue effectively blocks live migrating mlx5_vdpa device that has to get merged asap. While others can post patches to improve warnings on top.


Thanks,
-Siwei

          goto err_counter;
+    }
        err = create_virtqueue(ndev, mvq);
-    if (err)
+    if (err) {
+        mlx5_vdpa_warn(&ndev->mvdev, "failed to create virtqueue idx %d(%d)\n",
+                   idx, err);
          goto err_connect;
+    }
        if (mvq->ready) {
          err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
@@ -2411,7 +2418,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
      if (err)
          goto err_mr;
  -    if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
+    if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || ndev->suspended)
          goto err_mr;
        restore_channels_info(ndev);
@@ -2580,6 +2587,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
      mlx5_vdpa_destroy_mr(&ndev->mvdev);
      ndev->mvdev.status = 0;
      ndev->cur_num_vqs = 0;
+    ndev->suspended = false;
      ndev->mvdev.cvq.received_desc = 0;
      ndev->mvdev.cvq.completed_desc = 0;
      memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
@@ -2815,6 +2823,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
      struct mlx5_vdpa_virtqueue *mvq;
      int i;
  +    mlx5_vdpa_info(mvdev, "suspending device\n");
+
      down_write(&ndev->reslock);
      ndev->nb_registered = false;
      mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
@@ -2824,6 +2834,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
          suspend_vq(ndev, mvq);
      }
      mlx5_vdpa_cvq_suspend(mvdev);
+    ndev->suspended = true;
      up_write(&ndev->reslock);
      return 0;
  }