[PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen <redacted>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8e9d525d66c..a51b0f86afe2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
 		return;
 	}
 	mvq->avail_idx = attr.available_index;
+	mvq->used_idx = attr.used_index;
 }
 
 static void suspend_vqs(struct mlx5_vdpa_net *ndev)

@@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
 		return -EINVAL;
 	}
 
+	mvq->used_idx = state->avail_index;
 	mvq->avail_idx = state->avail_index;
 	return 0;
 }

@@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
 	 * that cares about emulating the index after vq is stopped.
 	 */
 	if (!mvq->initialized) {
-		state->avail_index = mvq->avail_idx;
+		state->avail_index = mvq->used_idx;
 		return 0;
 	}
 

@@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
 		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
 		return err;
 	}
-	state->avail_index = attr.available_index;
+	state->avail_index = attr.used_index;
 	return 0;
 }
 

@@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
 	}
 }
 
-static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
-{
-	int i;
-
-	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
-		ndev->vqs[i].avail_idx = 0;
-		ndev->vqs[i].used_idx = 0;
-	}
-}
-
 /* TODO: cross-endian support */
 static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
 {

@@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 	if (!status) {
 		mlx5_vdpa_info(mvdev, "performing device reset\n");
 		teardown_driver(ndev);
-		clear_virtqueues(ndev);
 		mlx5_vdpa_destroy_mr(&ndev->mvdev);
 		ndev->mvdev.status = 0;
 		++mvdev->generation;

-- 
2.29.2

On 2/16/2021 8:20 AM, Eli Cohen wrote:

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen <redacted>

Acked-by: Si-Wei Liu <redacted>

On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:

On 2/16/2021 8:20 AM, Eli Cohen wrote:

quoted

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen <redacted>

Acked-by: Si-Wei Liu <redacted>

Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?

quoted

---
  drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
  1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8e9d525d66c..a51b0f86afe2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
  		return;
  	}
  	mvq->avail_idx = attr.available_index;
+	mvq->used_idx = attr.used_index;
  }
  static void suspend_vqs(struct mlx5_vdpa_net *ndev)

@@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
  		return -EINVAL;
  	}
+	mvq->used_idx = state->avail_index;
  	mvq->avail_idx = state->avail_index;
  	return 0;
  }

@@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
  	 * that cares about emulating the index after vq is stopped.
  	 */
  	if (!mvq->initialized) {
-		state->avail_index = mvq->avail_idx;
+		state->avail_index = mvq->used_idx;
  		return 0;
  	}

@@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
  		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
  		return err;
  	}
-	state->avail_index = attr.available_index;
+	state->avail_index = attr.used_index;
  	return 0;
  }

@@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
  	}
  }
-static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
-{
-	int i;
-
-	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
-		ndev->vqs[i].avail_idx = 0;
-		ndev->vqs[i].used_idx = 0;
-	}
-}
-
  /* TODO: cross-endian support */
  static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
  {

@@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
  	if (!status) {
  		mlx5_vdpa_info(mvdev, "performing device reset\n");
  		teardown_driver(ndev);
-		clear_virtqueues(ndev);
  		mlx5_vdpa_destroy_mr(&ndev->mvdev);
  		ndev->mvdev.status = 0;
  		++mvdev->generation;

On 2/17/2021 1:20 PM, Michael S. Tsirkin wrote:

On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:

quoted

On 2/16/2021 8:20 AM, Eli Cohen wrote:

quoted

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen <redacted>

Acked-by: Si-Wei Liu <redacted>

Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?

I think so. It should have both "Fixes" tags.

-Siwei

quoted

quoted

---
   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
   1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8e9d525d66c..a51b0f86afe2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
   		return;
   	}
   	mvq->avail_idx = attr.available_index;
+	mvq->used_idx = attr.used_index;
   }
   static void suspend_vqs(struct mlx5_vdpa_net *ndev)

@@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
   		return -EINVAL;
   	}
+	mvq->used_idx = state->avail_index;
   	mvq->avail_idx = state->avail_index;
   	return 0;
   }

@@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
   	 * that cares about emulating the index after vq is stopped.
   	 */
   	if (!mvq->initialized) {
-		state->avail_index = mvq->avail_idx;
+		state->avail_index = mvq->used_idx;
   		return 0;
   	}

@@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
   		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
   		return err;
   	}
-	state->avail_index = attr.available_index;
+	state->avail_index = attr.used_index;
   	return 0;
   }

@@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
   	}
   }
-static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
-{
-	int i;
-
-	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
-		ndev->vqs[i].avail_idx = 0;
-		ndev->vqs[i].used_idx = 0;
-	}
-}
-
   /* TODO: cross-endian support */
   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
   {

@@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
   	if (!status) {
   		mlx5_vdpa_info(mvdev, "performing device reset\n");
   		teardown_driver(ndev);
-		clear_virtqueues(ndev);
   		mlx5_vdpa_destroy_mr(&ndev->mvdev);
   		ndev->mvdev.status = 0;
   		++mvdev->generation;

On Wed, Feb 17, 2021 at 04:20:14PM -0500, Michael S. Tsirkin wrote:

On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:

quoted

On 2/16/2021 8:20 AM, Eli Cohen wrote:

quoted

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen <redacted>

Acked-by: Si-Wei Liu <redacted>

Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?

Right.

quoted

quoted

---
  drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
  1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8e9d525d66c..a51b0f86afe2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
  		return;
  	}
  	mvq->avail_idx = attr.available_index;
+	mvq->used_idx = attr.used_index;
  }
  static void suspend_vqs(struct mlx5_vdpa_net *ndev)

@@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
  		return -EINVAL;
  	}
+	mvq->used_idx = state->avail_index;
  	mvq->avail_idx = state->avail_index;
  	return 0;
  }

@@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
  	 * that cares about emulating the index after vq is stopped.
  	 */
  	if (!mvq->initialized) {
-		state->avail_index = mvq->avail_idx;
+		state->avail_index = mvq->used_idx;
  		return 0;
  	}

@@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
  		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
  		return err;
  	}
-	state->avail_index = attr.available_index;
+	state->avail_index = attr.used_index;
  	return 0;
  }

@@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
  	}
  }
-static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
-{
-	int i;
-
-	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
-		ndev->vqs[i].avail_idx = 0;
-		ndev->vqs[i].used_idx = 0;
-	}
-}
-
  /* TODO: cross-endian support */
  static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
  {

@@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
  	if (!status) {
  		mlx5_vdpa_info(mvdev, "performing device reset\n");
  		teardown_driver(ndev);
-		clear_virtqueues(ndev);
  		mlx5_vdpa_destroy_mr(&ndev->mvdev);
  		ndev->mvdev.status = 0;
  		++mvdev->generation;

On 2/17/2021 11:42 AM, Si-Wei Liu wrote:

On 2/16/2021 8:20 AM, Eli Cohen wrote:

quoted

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 
devices")
Signed-off-by: Eli Cohen <redacted>

Acked-by: Si-Wei Liu <redacted>

I'd take it back for the moment, according to Jason's latest comment. 
Discussion still going.

quoted

---
  drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
  1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 

b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8e9d525d66c..a51b0f86afe2 100644

--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net 

*ndev, struct mlx5_vdpa_virtqueue *m
          return;
      }
      mvq->avail_idx = attr.available_index;
+    mvq->used_idx = attr.used_index;
  }
    static void suspend_vqs(struct mlx5_vdpa_net *ndev)

@@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct 

vdpa_device *vdev, u16 idx,
          return -EINVAL;
      }
  +    mvq->used_idx = state->avail_index;
      mvq->avail_idx = state->avail_index;

This is where things starts getting interesting. According to Jason, the 
original expectation of this API would be to restore the internal 
last_avail_index in the hardware. With Mellanox network vDPA hardware 
implementation, there's no such last_avail_index thing in the hardware 
but only a single last_used_index representing both indices, which 
should always be the same and in sync with each other. So from migration 
point of view, it appears logical to restore the saved last_avail_index 
to the last_used_index in the hardware, right? But what is the point to 
restore mvq->avail_idx?

Actually, this code path is being repurposed to address the index reset 
problem in the device reset scenario. Where the mvq->avail_idx and 
mvq->used_idx are both reset to 0 once device is reset. This is a bit 
crossing the boundary to me.

quoted

      return 0;
  }

@@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct 

vdpa_device *vdev, u16 idx, struct vdpa
       * that cares about emulating the index after vq is stopped.
       */
      if (!mvq->initialized) {
-        state->avail_index = mvq->avail_idx;
+        state->avail_index = mvq->used_idx;

This is where the last_used_index gets loaded from the hardware (when 
device is stopped).

quoted

          return 0;
      }
  @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct 
vdpa_device *vdev, u16 idx, struct vdpa
          mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
          return err;
      }
-    state->avail_index = attr.available_index;
+    state->avail_index = attr.used_index;

This code path never gets called from userspace (when device is running).

-Siwei

quoted

      return 0;
  }
  @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct 
mlx5_vdpa_net *ndev)
      }
  }
  -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
-{
-    int i;
-
-    for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
-        ndev->vqs[i].avail_idx = 0;
-        ndev->vqs[i].used_idx = 0;
-    }
-}
-
  /* TODO: cross-endian support */
  static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev 
*mvdev)
  {

@@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct 

vdpa_device *vdev, u8 status)
      if (!status) {
          mlx5_vdpa_info(mvdev, "performing device reset\n");
          teardown_driver(ndev);
-        clear_virtqueues(ndev);
          mlx5_vdpa_destroy_mr(&ndev->mvdev);
          ndev->mvdev.status = 0;
          ++mvdev->generation;