[RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[Eugenio Pérez]

At this moment the migration of net features that depends on CVQ is not
possible, as there is no reliable way to restore the device state like mac
address, number of enabled queues, etc to the destination.  This is mainly
caused because the device must only read CVQ, and process all the commands
before resuming the dataplane.

This series uses the VirtIO feature _F_RING_RESET to achieve it, adding an
alternative method to late vq enabling proposed in [1][2].  It expose SVQ to
the device until it process all the CVQ messages, and then replaces the vring
for the guest's one.

As an advantage, it uses a feature well deviced in the VirtIO standard.  As a
disadvantage, current HW already support the late enabling and it does not
support RING_RESET.

This patch must be applied on top of the series ("Enable vdpa net migration
with features depending on CVQ") [1][2].

The patch has been tested with vp_vdpa, but using high IOVA instead of a
sepparated ID for shadow CVQ and shadow temporal vrings.

[1] Message-id: <20230706191227.835526-1-eperezma@redhat.com>
[2] https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg01325.html

Eugenio Pérez (12):
  vhost: add vhost_reset_queue_op
  vhost: add vhost_restart_queue_op
  vhost_net: Use ops->vhost_restart_queue in vhost_net_virtqueue_restart
  vhost_net: Use ops->vhost_reset_queue in vhost_net_virtqueue_reset
  vdpa: add vhost_vdpa_set_vring_ready_internal
  vdpa: add vhost_vdpa_svq_stop
  vdpa: add vhost_vdpa_reset_queue
  vdpa: add vhost_vdpa_svq_start
  vdpa: add vhost_vdpa_restart_queue
  vdpa: enable all vqs if the device support RING_RESET feature
  vdpa: use SVQ to stall dataplane while NIC state is being restored
  vhost: Allow _F_RING_RESET with shadow virtqueue

 include/hw/virtio/vhost-backend.h  |   6 ++
 hw/net/vhost_net.c                 |  16 ++--
 hw/virtio/vhost-shadow-virtqueue.c |   1 +
 hw/virtio/vhost-vdpa.c             | 139 +++++++++++++++++++++--------
 net/vhost-vdpa.c                   |  55 ++++++++++--
 hw/virtio/trace-events             |   2 +-
 6 files changed, 171 insertions(+), 48 deletions(-)

-- 
2.39.3

[RFC PATCH 01/12] vhost: add vhost_reset_queue_op

[Eugenio Pérez]

So different vhost backends can perform custom actions at queue reset.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost-backend.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 31a251a9f5..37e05fa1f9 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -133,6 +133,8 @@ typedef int (*vhost_set_config_call_op)(struct vhost_dev *dev,
 
 typedef void (*vhost_reset_status_op)(struct vhost_dev *dev);
 
+typedef void (*vhost_reset_queue_op)(struct vhost_dev *dev, int idx);
+
 typedef struct VhostOps {
     VhostBackendType backend_type;
     vhost_backend_init vhost_backend_init;
@@ -181,6 +183,7 @@ typedef struct VhostOps {
     vhost_force_iommu_op vhost_force_iommu;
     vhost_set_config_call_op vhost_set_config_call;
     vhost_reset_status_op vhost_reset_status;
+    vhost_reset_queue_op vhost_reset_queue;
 } VhostOps;
 
 int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
-- 
2.39.3

[RFC PATCH 02/12] vhost: add vhost_restart_queue_op

[Eugenio Pérez]

So different vhost backends can perform custom actions at queue restart.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost-backend.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 37e05fa1f9..651402af10 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -135,6 +135,8 @@ typedef void (*vhost_reset_status_op)(struct vhost_dev *dev);
 
 typedef void (*vhost_reset_queue_op)(struct vhost_dev *dev, int idx);
 
+typedef int (*vhost_restart_queue_op)(struct vhost_dev *dev, int idx);
+
 typedef struct VhostOps {
     VhostBackendType backend_type;
     vhost_backend_init vhost_backend_init;
@@ -184,6 +186,7 @@ typedef struct VhostOps {
     vhost_set_config_call_op vhost_set_config_call;
     vhost_reset_status_op vhost_reset_status;
     vhost_reset_queue_op vhost_reset_queue;
+    vhost_restart_queue_op vhost_restart_queue;
 } VhostOps;
 
 int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
-- 
2.39.3

[RFC PATCH 03/12] vhost_net: Use ops->vhost_restart_queue in vhost_net_virtqueue_restart

[Eugenio Pérez]

Actually use vhost_restart_queue operation at restart.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/net/vhost_net.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 6b958d6363..416b7d8132 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -608,14 +608,16 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
         goto err_start;
     }
 
-    if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+    if (vhost_ops->vhost_restart_queue) {
+        r = vhost_ops->vhost_restart_queue(&net->dev, idx);
+    } else if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         file.index = idx;
         file.fd = net->backend;
         r = vhost_net_set_backend(&net->dev, &file);
-        if (r < 0) {
-            r = -errno;
-            goto err_start;
-        }
+    }
+    if (r < 0) {
+        r = -errno;
+        goto err_start;
     }
 
     return 0;
-- 
2.39.3

[RFC PATCH 04/12] vhost_net: Use ops->vhost_reset_queue in vhost_net_virtqueue_reset

[Eugenio Pérez]

Actually use vhost_reset_queue operation at queue reset.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/net/vhost_net.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 416b7d8132..5516b7a5aa 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -571,7 +571,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
 
     idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
 
-    if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
+    if (vhost_ops->vhost_reset_queue) {
+        vhost_ops->vhost_reset_queue(&net->dev, idx);
+    } else if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         file.index = idx;
         int r = vhost_net_set_backend(&net->dev, &file);
         assert(r >= 0);
-- 
2.39.3

[RFC PATCH 05/12] vdpa: add vhost_vdpa_set_vring_ready_internal

[Eugenio Pérez]

Reset a virtqueue reuses this call with .num = 0, so let's make it
possible to use that way.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 12 +++++++++---
 hw/virtio/trace-events |  2 +-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index f51f5d9969..e7ab69165c 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -528,19 +528,25 @@ int vhost_vdpa_get_iova_range(int fd, struct vhost_vdpa_iova_range *iova_range)
     return ret < 0 ? -errno : 0;
 }
 
-int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+static int vhost_vdpa_set_vring_ready_internal(struct vhost_vdpa *v,
+                                               unsigned idx, bool enable)
 {
     struct vhost_dev *dev = v->dev;
     struct vhost_vring_state state = {
         .index = idx,
-        .num = 1,
+        .num = enable,
     };
     int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
 
-    trace_vhost_vdpa_set_vring_ready(dev, idx, r);
+    trace_vhost_vdpa_set_vring_ready(dev, idx, enable, r);
     return r;
 }
 
+int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
+{
+    return vhost_vdpa_set_vring_ready_internal(v, idx, true);
+}
+
 /*
  * The use of this function is for requests that only need to be
  * applied once. Typically such request occurs at the beginning
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 4f6a6ba428..9b90da73af 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -46,7 +46,7 @@ vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRI
 vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32
 vhost_vdpa_reset_device(void *dev, uint8_t status) "dev: %p status: 0x%"PRIx8
 vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d"
-vhost_vdpa_set_vring_ready(void *dev, unsigned i, int r) "dev: %p, idx: %u, r: %d"
+vhost_vdpa_set_vring_ready(void *dev, unsigned i, bool e, int r) "dev: %p, idx: %u, num: %d, r: %d"
 vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s"
 vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32
 vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32
-- 
2.39.3

[RFC PATCH 06/12] vdpa: add vhost_vdpa_svq_stop

[Eugenio Pérez]

To split each SVQ stop in its own stop routine let's us to reset a VQ
individually, and to keep future vhost_vdpa_reset_queue symmetrical
with vhost_vdpa_reset_queue.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index e7ab69165c..6ae276ccde 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1263,6 +1263,18 @@ err:
     return false;
 }
 
+static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx)
+{
+    struct vhost_vdpa *v = dev->opaque;
+    VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx);
+
+    vhost_svq_stop(svq);
+    vhost_vdpa_svq_unmap_rings(dev, svq);
+
+    event_notifier_cleanup(&svq->hdev_kick);
+    event_notifier_cleanup(&svq->hdev_call);
+}
+
 static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
@@ -1272,13 +1284,7 @@ static void vhost_vdpa_svqs_stop(struct vhost_dev *dev)
     }
 
     for (unsigned i = 0; i < v->shadow_vqs->len; ++i) {
-        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
-
-        vhost_svq_stop(svq);
-        vhost_vdpa_svq_unmap_rings(dev, svq);
-
-        event_notifier_cleanup(&svq->hdev_kick);
-        event_notifier_cleanup(&svq->hdev_call);
+        vhost_vdpa_svq_stop(dev, i);
     }
 }
 
-- 
2.39.3

[RFC PATCH 07/12] vdpa: add vhost_vdpa_reset_queue

[Eugenio Pérez]

Split out vq reset operation in its own function, as it may be called
with ring reset.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 6ae276ccde..df2515a247 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -547,6 +547,21 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
     return vhost_vdpa_set_vring_ready_internal(v, idx, true);
 }
 
+/* TODO: Properly reorder static functions */
+static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx);
+static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx)
+{
+    struct vhost_vdpa *v = dev->opaque;
+
+    if (dev->features & VIRTIO_F_RING_RESET) {
+        vhost_vdpa_set_vring_ready_internal(v, idx, false);
+    }
+
+    if (v->shadow_vqs_enabled) {
+        vhost_vdpa_svq_stop(dev, idx - dev->vq_index);
+    }
+}
+
 /*
  * The use of this function is for requests that only need to be
  * applied once. Typically such request occurs at the beginning
@@ -1543,4 +1558,5 @@ const VhostOps vdpa_ops = {
         .vhost_force_iommu = vhost_vdpa_force_iommu,
         .vhost_set_config_call = vhost_vdpa_set_config_call,
         .vhost_reset_status = vhost_vdpa_reset_status,
+        .vhost_reset_queue = vhost_vdpa_reset_queue,
 };
-- 
2.39.3

[RFC PATCH 08/12] vdpa: add vhost_vdpa_svq_start

[Eugenio Pérez]

To split each SVQ in its own initialization routine let's us to restart
a VQ individually, and to keep future vhost_vdpa_restart_queue
symmetrical with vhost_vdpa_reset_queue.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 67 ++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index df2515a247..7248072989 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1223,6 +1223,46 @@ static bool vhost_vdpa_svq_setup(struct vhost_dev *dev,
     return r == 0;
 }
 
+static bool vhost_vdpa_svq_start(struct vhost_dev *dev, unsigned i,
+                                 Error **errp)
+{
+    struct vhost_vdpa *v = dev->opaque;
+    VirtQueue *vq = virtio_get_queue(dev->vdev, dev->vq_index + i);
+    VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
+    struct vhost_vring_addr addr = {
+        .index = dev->vq_index + i,
+    };
+    int r;
+    bool ok = vhost_vdpa_svq_setup(dev, svq, i, errp);
+    if (unlikely(!ok)) {
+        goto err;
+    }
+
+    vhost_svq_start(svq, dev->vdev, vq, v->iova_tree);
+    ok = vhost_vdpa_svq_map_rings(dev, svq, &addr, errp);
+    if (unlikely(!ok)) {
+        goto err_map;
+    }
+
+    /* Override vring GPA set by vhost subsystem */
+    r = vhost_vdpa_set_vring_dev_addr(dev, &addr);
+    if (unlikely(r != 0)) {
+        error_setg_errno(errp, -r, "Cannot set device address");
+        goto err_set_addr;
+    }
+
+    return true;
+
+err_set_addr:
+    vhost_vdpa_svq_unmap_rings(dev, g_ptr_array_index(v->shadow_vqs, i));
+
+err_map:
+    vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, i));
+
+err:
+    return false;
+}
+
 static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
@@ -1234,39 +1274,14 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
     }
 
     for (i = 0; i < v->shadow_vqs->len; ++i) {
-        VirtQueue *vq = virtio_get_queue(dev->vdev, dev->vq_index + i);
-        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, i);
-        struct vhost_vring_addr addr = {
-            .index = dev->vq_index + i,
-        };
-        int r;
-        bool ok = vhost_vdpa_svq_setup(dev, svq, i, &err);
+        bool ok = vhost_vdpa_svq_start(dev, i, &err);
         if (unlikely(!ok)) {
             goto err;
         }
-
-        vhost_svq_start(svq, dev->vdev, vq, v->iova_tree);
-        ok = vhost_vdpa_svq_map_rings(dev, svq, &addr, &err);
-        if (unlikely(!ok)) {
-            goto err_map;
-        }
-
-        /* Override vring GPA set by vhost subsystem */
-        r = vhost_vdpa_set_vring_dev_addr(dev, &addr);
-        if (unlikely(r != 0)) {
-            error_setg_errno(&err, -r, "Cannot set device address");
-            goto err_set_addr;
-        }
     }
 
     return true;
 
-err_set_addr:
-    vhost_vdpa_svq_unmap_rings(dev, g_ptr_array_index(v->shadow_vqs, i));
-
-err_map:
-    vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, i));
-
 err:
     error_reportf_err(err, "Cannot setup SVQ %u: ", i);
     for (unsigned j = 0; j < i; ++j) {
-- 
2.39.3

[RFC PATCH 09/12] vdpa: add vhost_vdpa_restart_queue

[Eugenio Pérez]

Split out vq restart operation in its own function, as it may be called
with ring reset.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7248072989..7b24fa3e12 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -562,6 +562,29 @@ static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx)
     }
 }
 
+/* TODO: Properly reorder static functions */
+static bool vhost_vdpa_svq_start(struct vhost_dev *dev, unsigned i,
+                                 Error **errp);
+static int vhost_vdpa_restart_queue(struct vhost_dev *dev, int idx)
+{
+    struct vhost_vdpa *v = dev->opaque;
+
+    if (v->shadow_vqs_enabled) {
+        Error *err = NULL;
+        bool ok = vhost_vdpa_svq_start(dev, idx, &err);
+        if (unlikely(!ok)) {
+            error_report_err(err);
+            return -1;
+        }
+    }
+
+    if (dev->features & VIRTIO_F_RING_RESET) {
+        return vhost_vdpa_set_vring_ready_internal(v, idx, true);
+    }
+
+    return 0;
+}
+
 /*
  * The use of this function is for requests that only need to be
  * applied once. Typically such request occurs at the beginning
@@ -1574,4 +1597,5 @@ const VhostOps vdpa_ops = {
         .vhost_set_config_call = vhost_vdpa_set_config_call,
         .vhost_reset_status = vhost_vdpa_reset_status,
         .vhost_reset_queue = vhost_vdpa_reset_queue,
+        .vhost_restart_queue = vhost_vdpa_restart_queue,
 };
-- 
2.39.3

[RFC PATCH 10/12] vdpa: enable all vqs if the device support RING_RESET feature

[Eugenio Pérez]

Prefer the ring reset approach against the late enable, as it is more
aligned with the standard.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 52415d7e0c..af83de92f8 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -840,8 +840,14 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
  */
 static bool vhost_vdpa_should_enable(const struct vhost_vdpa *v)
 {
+    VhostVDPAState *s = container_of(v, VhostVDPAState, vhost_vdpa);
     struct vhost_dev *dev = v->dev;
 
+    if (dev->features & VIRTIO_F_RING_RESET && !s->always_svq) {
+        /* Preventing dataplane processing exposing fake SVQ vring */
+        return true;
+    }
+
     if (!dev->vq_index_end % 2) {
         /* vDPA device does not have CVQ */
         return true;
-- 
2.39.3

[RFC PATCH 11/12] vdpa: use SVQ to stall dataplane while NIC state is being restored

[Eugenio Pérez]

Some dynamic state of a virtio-net vDPA devices is restored from CVQ in
the event of a live migration.  However, dataplane needs to be disabled
so the NIC does not receive buffers in the invalid ring.

As a default method to achieve it, let's offer a shadow vring with 0
avail idx.  As a fallback method, we will enable dataplane vqs later, as
proposed previously.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 49 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index af83de92f8..e14ae48f23 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -338,10 +338,25 @@ static int vhost_vdpa_net_data_start(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
     struct vhost_vdpa *v = &s->vhost_vdpa;
+    bool has_cvq = v->dev->vq_index_end % 2;
 
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
-    if (s->always_svq ||
+    if (has_cvq && (v->dev->features & VIRTIO_F_RING_RESET)) {
+        /*
+         * Offer a fake vring to the device while the state is restored
+         * through CVQ.  That way, the guest will not see packets in unexpected
+         * queues.
+         *
+         * This will be undone after loading all state through CVQ, at
+         * vhost_vdpa_net_load.
+         *
+         * TODO: Future optimizations may skip some SVQ setup and teardown,
+         * like set the right kick and call fd or doorbell maps directly, and
+         * the iova tree.
+         */
+        v->shadow_vqs_enabled = true;
+    } else if (s->always_svq ||
         migration_is_setup_or_active(migrate_get_current()->state)) {
         v->shadow_vqs_enabled = true;
         v->shadow_data = true;
@@ -738,10 +753,34 @@ static int vhost_vdpa_net_load(NetClientState *nc)
         return r;
     }
 
-    for (int i = 0; i < v->dev->vq_index; ++i) {
-        r = vhost_vdpa_set_vring_ready(v, i);
-        if (unlikely(r)) {
-            return r;
+    if (v->dev->features & VIRTIO_F_RING_RESET && !s->always_svq &&
+        !migration_is_setup_or_active(migrate_get_current()->state)) {
+        NICState *nic = qemu_get_nic(s->nc.peer);
+        int queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
+
+        for (int i = 0; i < queue_pairs; ++i) {
+            NetClientState *ncs = qemu_get_peer(nic->ncs, i);
+            VhostVDPAState *s_i = DO_UPCAST(VhostVDPAState, nc, ncs);
+
+            for (int j = 0; j < 2; ++j) {
+                vhost_net_virtqueue_reset(v->dev->vdev, ncs->peer, j);
+            }
+
+            s_i->vhost_vdpa.shadow_vqs_enabled = false;
+
+            for (int j = 0; j < 2; ++j) {
+                r = vhost_net_virtqueue_restart(v->dev->vdev, ncs->peer, j);
+                if (unlikely(r < 0)) {
+                    return r;
+                }
+            }
+        }
+    } else {
+        for (int i = 0; i < v->dev->vq_index; ++i) {
+            r = vhost_vdpa_set_vring_ready(v, i);
+            if (unlikely(r)) {
+                return r;
+            }
         }
     }
 
-- 
2.39.3

[RFC PATCH 12/12] vhost: Allow _F_RING_RESET with shadow virtqueue

[Eugenio Pérez]

This is needed to let CVQ restore at the beginning.

Not properly tested with actual guest ring reset, only with the reset
from qemu. For this to be included, more test is needed, but this series
is a good start for it.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 1b1d85306c..6490a98db7 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -34,6 +34,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp)
         switch (b) {
         case VIRTIO_F_ANY_LAYOUT:
         case VIRTIO_RING_F_EVENT_IDX:
+        case VIRTIO_F_RING_RESET:
             continue;
 
         case VIRTIO_F_ACCESS_PLATFORM:
-- 
2.39.3

Re: [RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[Eugenio Perez Martin]

On Thu, Jul 20, 2023 at 8:15 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> At this moment the migration of net features that depends on CVQ is not
> possible, as there is no reliable way to restore the device state like mac
> address, number of enabled queues, etc to the destination.  This is mainly
> caused because the device must only read CVQ, and process all the commands
> before resuming the dataplane.
>
> This series uses the VirtIO feature _F_RING_RESET to achieve it, adding an
> alternative method to late vq enabling proposed in [1][2].  It expose SVQ to
> the device until it process all the CVQ messages, and then replaces the vring
> for the guest's one.
>

A couple of things I forgot to add:
* Assuming the implementation of _F_RING_RESET in vdpa is calling
kernel vdpa ops .set_vq_ready(vq, false). I'm not sure if this is the
best implementation, but it is currently unused in the kernel. At the
same time, .set_vq_ready(vq, true) also enables the vq again.

> As an advantage, it uses a feature well deviced in the VirtIO standard.  As a
> disadvantage, current HW already support the late enabling and it does not
> support RING_RESET.
>
> This patch must be applied on top of the series ("Enable vdpa net migration
> with features depending on CVQ") [1][2].
>
> The patch has been tested with vp_vdpa, but using high IOVA instead of a
> sepparated ID for shadow CVQ and shadow temporal vrings.
>

And with _F_STATE implementation I sent long ago.

Based on this, my suggestion is:
* Leave the late enable for vDPA devices.
* Make them fail if the vDPA parent device does not support it. This
can be considered as a fix.
* Leave _F_RING_RESET to be added on top, as the semantics are not
implemented in vDPA at the moment.

Would that work?

> [1] Message-id: <20230706191227.835526-1-eperezma@redhat.com>
> [2] https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg01325.html
>
> Eugenio Pérez (12):
>   vhost: add vhost_reset_queue_op
>   vhost: add vhost_restart_queue_op
>   vhost_net: Use ops->vhost_restart_queue in vhost_net_virtqueue_restart
>   vhost_net: Use ops->vhost_reset_queue in vhost_net_virtqueue_reset
>   vdpa: add vhost_vdpa_set_vring_ready_internal
>   vdpa: add vhost_vdpa_svq_stop
>   vdpa: add vhost_vdpa_reset_queue
>   vdpa: add vhost_vdpa_svq_start
>   vdpa: add vhost_vdpa_restart_queue
>   vdpa: enable all vqs if the device support RING_RESET feature
>   vdpa: use SVQ to stall dataplane while NIC state is being restored
>   vhost: Allow _F_RING_RESET with shadow virtqueue
>
>  include/hw/virtio/vhost-backend.h  |   6 ++
>  hw/net/vhost_net.c                 |  16 ++--
>  hw/virtio/vhost-shadow-virtqueue.c |   1 +
>  hw/virtio/vhost-vdpa.c             | 139 +++++++++++++++++++++--------
>  net/vhost-vdpa.c                   |  55 ++++++++++--
>  hw/virtio/trace-events             |   2 +-
>  6 files changed, 171 insertions(+), 48 deletions(-)
>
> --
> 2.39.3
>
>

Re: [RFC PATCH 07/12] vdpa: add vhost_vdpa_reset_queue

[Si-Wei Liu]

On 7/20/2023 11:14 AM, Eugenio Pérez wrote:
> Split out vq reset operation in its own function, as it may be called
> with ring reset.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 6ae276ccde..df2515a247 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -547,6 +547,21 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
>       return vhost_vdpa_set_vring_ready_internal(v, idx, true);
>   }
>   
> +/* TODO: Properly reorder static functions */
> +static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx);
> +static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx)
> +{
> +    struct vhost_vdpa *v = dev->opaque;
> +
> +    if (dev->features & VIRTIO_F_RING_RESET) {
> +        vhost_vdpa_set_vring_ready_internal(v, idx, false);
I'm not sure I understand this patch - this is NOT the spec defined way 
to initiate RING_RESET? Quoting the spec diff from the original 
RING_RESET tex doc:

+The device MUST reset the queue when 1 is written to \field{queue_reset}, and
+present a 1 in \field{queue_reset} after the queue has been reset, until the
+driver re-enables the queue via \field{queue_enable} or the device is reset.
+The device MUST present consistent default values after queue reset.
+(see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).

Or you intend to rewrite it to be spec conforming later on?

-Siwei
> +    }
> +
> +    if (v->shadow_vqs_enabled) {
> +        vhost_vdpa_svq_stop(dev, idx - dev->vq_index);
> +    }
> +}
> +
>   /*
>    * The use of this function is for requests that only need to be
>    * applied once. Typically such request occurs at the beginning
> @@ -1543,4 +1558,5 @@ const VhostOps vdpa_ops = {
>           .vhost_force_iommu = vhost_vdpa_force_iommu,
>           .vhost_set_config_call = vhost_vdpa_set_config_call,
>           .vhost_reset_status = vhost_vdpa_reset_status,
> +        .vhost_reset_queue = vhost_vdpa_reset_queue,
>   };

Re: [RFC PATCH 11/12] vdpa: use SVQ to stall dataplane while NIC state is being restored

[Si-Wei Liu]

On 7/20/2023 11:14 AM, Eugenio Pérez wrote:
> Some dynamic state of a virtio-net vDPA devices is restored from CVQ in
> the event of a live migration.  However, dataplane needs to be disabled
> so the NIC does not receive buffers in the invalid ring.
>
> As a default method to achieve it, let's offer a shadow vring with 0
> avail idx.  As a fallback method, we will enable dataplane vqs later, as
> proposed previously.
Let's not jump to conclusion too early what will be the default v.s. 
fallback [1] - as this is on a latency sensitive path, I'm not fully 
convinced ring reset could perform better than or equally same as the 
deferred dataplane enablement approach on hardware. At this stage I 
think ring_reset has no adoption on vendors device, while it's 
definitely easier with lower hardware overhead for vendor to implement 
deferred dataplane enabling. If at some point vendor's device has to 
support RING_RESET for other use cases (MTU change propagation for ex., 
a prerequisite for GRO HW) than live migration, defaulting to RING_RESET 
on this SVQ path has no real benefit but adds complications needlessly 
to vendor's device.

[1] 
https://lore.kernel.org/virtualization/bf2164a9-1dfd-14d9-be2a-8bb7620a0619@oracle.com/T/#m15caca6fbb00ca9c00e2b33391297a2d8282ff89

Thanks,
-Siwei

>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   net/vhost-vdpa.c | 49 +++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index af83de92f8..e14ae48f23 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -338,10 +338,25 @@ static int vhost_vdpa_net_data_start(NetClientState *nc)
>   {
>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>       struct vhost_vdpa *v = &s->vhost_vdpa;
> +    bool has_cvq = v->dev->vq_index_end % 2;
>   
>       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>   
> -    if (s->always_svq ||
> +    if (has_cvq && (v->dev->features & VIRTIO_F_RING_RESET)) {
> +        /*
> +         * Offer a fake vring to the device while the state is restored
> +         * through CVQ.  That way, the guest will not see packets in unexpected
> +         * queues.
> +         *
> +         * This will be undone after loading all state through CVQ, at
> +         * vhost_vdpa_net_load.
> +         *
> +         * TODO: Future optimizations may skip some SVQ setup and teardown,
> +         * like set the right kick and call fd or doorbell maps directly, and
> +         * the iova tree.
> +         */
> +        v->shadow_vqs_enabled = true;
> +    } else if (s->always_svq ||
>           migration_is_setup_or_active(migrate_get_current()->state)) {
>           v->shadow_vqs_enabled = true;
>           v->shadow_data = true;
> @@ -738,10 +753,34 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>           return r;
>       }
>   
> -    for (int i = 0; i < v->dev->vq_index; ++i) {
> -        r = vhost_vdpa_set_vring_ready(v, i);
> -        if (unlikely(r)) {
> -            return r;
> +    if (v->dev->features & VIRTIO_F_RING_RESET && !s->always_svq &&
> +        !migration_is_setup_or_active(migrate_get_current()->state)) {
> +        NICState *nic = qemu_get_nic(s->nc.peer);
> +        int queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> +
> +        for (int i = 0; i < queue_pairs; ++i) {
> +            NetClientState *ncs = qemu_get_peer(nic->ncs, i);
> +            VhostVDPAState *s_i = DO_UPCAST(VhostVDPAState, nc, ncs);
> +
> +            for (int j = 0; j < 2; ++j) {
> +                vhost_net_virtqueue_reset(v->dev->vdev, ncs->peer, j);
> +            }
> +
> +            s_i->vhost_vdpa.shadow_vqs_enabled = false;
> +
> +            for (int j = 0; j < 2; ++j) {
> +                r = vhost_net_virtqueue_restart(v->dev->vdev, ncs->peer, j);
> +                if (unlikely(r < 0)) {
> +                    return r;
> +                }
> +            }
> +        }
> +    } else {
> +        for (int i = 0; i < v->dev->vq_index; ++i) {
> +            r = vhost_vdpa_set_vring_ready(v, i);
> +            if (unlikely(r)) {
> +                return r;
> +            }
>           }
>       }
>   

Re: [RFC PATCH 07/12] vdpa: add vhost_vdpa_reset_queue

[Eugenio Perez Martin]

On Fri, Jul 21, 2023 at 11:57 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 7/20/2023 11:14 AM, Eugenio Pérez wrote:
> > Split out vq reset operation in its own function, as it may be called
> > with ring reset.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-vdpa.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 6ae276ccde..df2515a247 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -547,6 +547,21 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx)
> >       return vhost_vdpa_set_vring_ready_internal(v, idx, true);
> >   }
> >
> > +/* TODO: Properly reorder static functions */
> > +static void vhost_vdpa_svq_stop(struct vhost_dev *dev, unsigned idx);
> > +static void vhost_vdpa_reset_queue(struct vhost_dev *dev, int idx)
> > +{
> > +    struct vhost_vdpa *v = dev->opaque;
> > +
> > +    if (dev->features & VIRTIO_F_RING_RESET) {
> > +        vhost_vdpa_set_vring_ready_internal(v, idx, false);
> I'm not sure I understand this patch - this is NOT the spec defined way
> to initiate RING_RESET? Quoting the spec diff from the original
> RING_RESET tex doc:
>
> +The device MUST reset the queue when 1 is written to \field{queue_reset}, and
> +present a 1 in \field{queue_reset} after the queue has been reset, until the
> +driver re-enables the queue via \field{queue_enable} or the device is reset.
> +The device MUST present consistent default values after queue reset.
> +(see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
>
> Or you intend to rewrite it to be spec conforming later on?
>

Sorry for the late notice, but yes, the plan would be either to
rewrite this piece of code or to make vDPA uAPI work that way
(unlikely). I didn't create a new ioctl for that.

Please see https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg04144.html

Thanks!

> -Siwei
> > +    }
> > +
> > +    if (v->shadow_vqs_enabled) {
> > +        vhost_vdpa_svq_stop(dev, idx - dev->vq_index);
> > +    }
> > +}
> > +
> >   /*
> >    * The use of this function is for requests that only need to be
> >    * applied once. Typically such request occurs at the beginning
> > @@ -1543,4 +1558,5 @@ const VhostOps vdpa_ops = {
> >           .vhost_force_iommu = vhost_vdpa_force_iommu,
> >           .vhost_set_config_call = vhost_vdpa_set_config_call,
> >           .vhost_reset_status = vhost_vdpa_reset_status,
> > +        .vhost_reset_queue = vhost_vdpa_reset_queue,
> >   };
>

Re: [RFC PATCH 11/12] vdpa: use SVQ to stall dataplane while NIC state is being restored

[Eugenio Perez Martin]

On Sat, Jul 22, 2023 at 12:59 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 7/20/2023 11:14 AM, Eugenio Pérez wrote:
> > Some dynamic state of a virtio-net vDPA devices is restored from CVQ in
> > the event of a live migration.  However, dataplane needs to be disabled
> > so the NIC does not receive buffers in the invalid ring.
> >
> > As a default method to achieve it, let's offer a shadow vring with 0
> > avail idx.  As a fallback method, we will enable dataplane vqs later, as
> > proposed previously.
> Let's not jump to conclusion too early what will be the default v.s.
> fallback [1] - as this is on a latency sensitive path, I'm not fully
> convinced ring reset could perform better than or equally same as the
> deferred dataplane enablement approach on hardware. At this stage I
> think ring_reset has no adoption on vendors device, while it's
> definitely easier with lower hardware overhead for vendor to implement
> deferred dataplane enabling. If at some point vendor's device has to
> support RING_RESET for other use cases (MTU change propagation for ex.,
> a prerequisite for GRO HW) than live migration, defaulting to RING_RESET
> on this SVQ path has no real benefit but adds complications needlessly
> to vendor's device.
>

I agree with that. Let's say "*This series* uses RING_RESET as the
default method, and late vq enablement as fallback".

Michael, given the current HW support, would it work to start merging
the late enable for vDPA after the feature freeze, and then add the
use of RING_RESET on top later?

Thanks!

> [1]
> https://lore.kernel.org/virtualization/bf2164a9-1dfd-14d9-be2a-8bb7620a0619@oracle.com/T/#m15caca6fbb00ca9c00e2b33391297a2d8282ff89
>
> Thanks,
> -Siwei
>
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   net/vhost-vdpa.c | 49 +++++++++++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 44 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index af83de92f8..e14ae48f23 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -338,10 +338,25 @@ static int vhost_vdpa_net_data_start(NetClientState *nc)
> >   {
> >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> >       struct vhost_vdpa *v = &s->vhost_vdpa;
> > +    bool has_cvq = v->dev->vq_index_end % 2;
> >
> >       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >
> > -    if (s->always_svq ||
> > +    if (has_cvq && (v->dev->features & VIRTIO_F_RING_RESET)) {
> > +        /*
> > +         * Offer a fake vring to the device while the state is restored
> > +         * through CVQ.  That way, the guest will not see packets in unexpected
> > +         * queues.
> > +         *
> > +         * This will be undone after loading all state through CVQ, at
> > +         * vhost_vdpa_net_load.
> > +         *
> > +         * TODO: Future optimizations may skip some SVQ setup and teardown,
> > +         * like set the right kick and call fd or doorbell maps directly, and
> > +         * the iova tree.
> > +         */
> > +        v->shadow_vqs_enabled = true;
> > +    } else if (s->always_svq ||
> >           migration_is_setup_or_active(migrate_get_current()->state)) {
> >           v->shadow_vqs_enabled = true;
> >           v->shadow_data = true;
> > @@ -738,10 +753,34 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> >           return r;
> >       }
> >
> > -    for (int i = 0; i < v->dev->vq_index; ++i) {
> > -        r = vhost_vdpa_set_vring_ready(v, i);
> > -        if (unlikely(r)) {
> > -            return r;
> > +    if (v->dev->features & VIRTIO_F_RING_RESET && !s->always_svq &&
> > +        !migration_is_setup_or_active(migrate_get_current()->state)) {
> > +        NICState *nic = qemu_get_nic(s->nc.peer);
> > +        int queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> > +
> > +        for (int i = 0; i < queue_pairs; ++i) {
> > +            NetClientState *ncs = qemu_get_peer(nic->ncs, i);
> > +            VhostVDPAState *s_i = DO_UPCAST(VhostVDPAState, nc, ncs);
> > +
> > +            for (int j = 0; j < 2; ++j) {
> > +                vhost_net_virtqueue_reset(v->dev->vdev, ncs->peer, j);
> > +            }
> > +
> > +            s_i->vhost_vdpa.shadow_vqs_enabled = false;
> > +
> > +            for (int j = 0; j < 2; ++j) {
> > +                r = vhost_net_virtqueue_restart(v->dev->vdev, ncs->peer, j);
> > +                if (unlikely(r < 0)) {
> > +                    return r;
> > +                }
> > +            }
> > +        }
> > +    } else {
> > +        for (int i = 0; i < v->dev->vq_index; ++i) {
> > +            r = vhost_vdpa_set_vring_ready(v, i);
> > +            if (unlikely(r)) {
> > +                return r;
> > +            }
> >           }
> >       }
> >
>

Re: [RFC PATCH 11/12] vdpa: use SVQ to stall dataplane while NIC state is being restored

[Jason Wang]

On Tue, Jul 25, 2023 at 3:59 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Sat, Jul 22, 2023 at 12:59 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >
> >
> >
> > On 7/20/2023 11:14 AM, Eugenio Pérez wrote:
> > > Some dynamic state of a virtio-net vDPA devices is restored from CVQ in
> > > the event of a live migration.  However, dataplane needs to be disabled
> > > so the NIC does not receive buffers in the invalid ring.
> > >
> > > As a default method to achieve it, let's offer a shadow vring with 0
> > > avail idx.  As a fallback method, we will enable dataplane vqs later, as
> > > proposed previously.
> > Let's not jump to conclusion too early what will be the default v.s.
> > fallback [1] - as this is on a latency sensitive path, I'm not fully
> > convinced ring reset could perform better than or equally same as the
> > deferred dataplane enablement approach on hardware. At this stage I
> > think ring_reset has no adoption on vendors device, while it's
> > definitely easier with lower hardware overhead for vendor to implement
> > deferred dataplane enabling.

That's my feeling as well.

> > If at some point vendor's device has to
> > support RING_RESET for other use cases (MTU change propagation for ex.,
> > a prerequisite for GRO HW) than live migration,

Currently, it is used for changing ring size, and it is planned for
AF_XDP. But I think neither of them requires low latency.

Thanks

> > defaulting to RING_RESET
> > on this SVQ path has no real benefit but adds complications needlessly
> > to vendor's device.
> >
>
> I agree with that. Let's say "*This series* uses RING_RESET as the
> default method, and late vq enablement as fallback".
>
> Michael, given the current HW support, would it work to start merging
> the late enable for vDPA after the feature freeze, and then add the
> use of RING_RESET on top later?
>
> Thanks!
>
> > [1]
> > https://lore.kernel.org/virtualization/bf2164a9-1dfd-14d9-be2a-8bb7620a0619@oracle.com/T/#m15caca6fbb00ca9c00e2b33391297a2d8282ff89
> >
> > Thanks,
> > -Siwei
> >
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   net/vhost-vdpa.c | 49 +++++++++++++++++++++++++++++++++++++++++++-----
> > >   1 file changed, 44 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index af83de92f8..e14ae48f23 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -338,10 +338,25 @@ static int vhost_vdpa_net_data_start(NetClientState *nc)
> > >   {
> > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > >       struct vhost_vdpa *v = &s->vhost_vdpa;
> > > +    bool has_cvq = v->dev->vq_index_end % 2;
> > >
> > >       assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > >
> > > -    if (s->always_svq ||
> > > +    if (has_cvq && (v->dev->features & VIRTIO_F_RING_RESET)) {
> > > +        /*
> > > +         * Offer a fake vring to the device while the state is restored
> > > +         * through CVQ.  That way, the guest will not see packets in unexpected
> > > +         * queues.
> > > +         *
> > > +         * This will be undone after loading all state through CVQ, at
> > > +         * vhost_vdpa_net_load.
> > > +         *
> > > +         * TODO: Future optimizations may skip some SVQ setup and teardown,
> > > +         * like set the right kick and call fd or doorbell maps directly, and
> > > +         * the iova tree.
> > > +         */
> > > +        v->shadow_vqs_enabled = true;
> > > +    } else if (s->always_svq ||
> > >           migration_is_setup_or_active(migrate_get_current()->state)) {
> > >           v->shadow_vqs_enabled = true;
> > >           v->shadow_data = true;
> > > @@ -738,10 +753,34 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> > >           return r;
> > >       }
> > >
> > > -    for (int i = 0; i < v->dev->vq_index; ++i) {
> > > -        r = vhost_vdpa_set_vring_ready(v, i);
> > > -        if (unlikely(r)) {
> > > -            return r;
> > > +    if (v->dev->features & VIRTIO_F_RING_RESET && !s->always_svq &&
> > > +        !migration_is_setup_or_active(migrate_get_current()->state)) {
> > > +        NICState *nic = qemu_get_nic(s->nc.peer);
> > > +        int queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> > > +
> > > +        for (int i = 0; i < queue_pairs; ++i) {
> > > +            NetClientState *ncs = qemu_get_peer(nic->ncs, i);
> > > +            VhostVDPAState *s_i = DO_UPCAST(VhostVDPAState, nc, ncs);
> > > +
> > > +            for (int j = 0; j < 2; ++j) {
> > > +                vhost_net_virtqueue_reset(v->dev->vdev, ncs->peer, j);
> > > +            }
> > > +
> > > +            s_i->vhost_vdpa.shadow_vqs_enabled = false;
> > > +
> > > +            for (int j = 0; j < 2; ++j) {
> > > +                r = vhost_net_virtqueue_restart(v->dev->vdev, ncs->peer, j);
> > > +                if (unlikely(r < 0)) {
> > > +                    return r;
> > > +                }
> > > +            }
> > > +        }
> > > +    } else {
> > > +        for (int i = 0; i < v->dev->vq_index; ++i) {
> > > +            r = vhost_vdpa_set_vring_ready(v, i);
> > > +            if (unlikely(r)) {
> > > +                return r;
> > > +            }
> > >           }
> > >       }
> > >
> >
>

Re: [RFC PATCH 03/12] vhost_net: Use ops->vhost_restart_queue in vhost_net_virtqueue_restart

[Jason Wang]

在 2023/7/21 02:14, Eugenio Pérez 写道:
> Actually use vhost_restart_queue operation at restart.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/net/vhost_net.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 6b958d6363..416b7d8132 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -608,14 +608,16 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
>           goto err_start;
>       }
>   
> -    if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> +    if (vhost_ops->vhost_restart_queue) {
> +        r = vhost_ops->vhost_restart_queue(&net->dev, idx);
> +    } else if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>           file.index = idx;
>           file.fd = net->backend;
>           r = vhost_net_set_backend(&net->dev, &file);


I would introduce a vhost-net specific reset routine and move the above 
logic there.

Thanks


> -        if (r < 0) {
> -            r = -errno;
> -            goto err_start;
> -        }
> +    }
> +    if (r < 0) {
> +        r = -errno;
> +        goto err_start;
>       }
>   
>       return 0;

Re: [RFC PATCH 04/12] vhost_net: Use ops->vhost_reset_queue in vhost_net_virtqueue_reset

[Jason Wang]

在 2023/7/21 02:14, Eugenio Pérez 写道:
> Actually use vhost_reset_queue operation at queue reset.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/net/vhost_net.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 416b7d8132..5516b7a5aa 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -571,7 +571,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
>   
>       idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
>   
> -    if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> +    if (vhost_ops->vhost_reset_queue) {
> +        vhost_ops->vhost_reset_queue(&net->dev, idx);
> +    } else if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>           file.index = idx;
>           int r = vhost_net_set_backend(&net->dev, &file);
>           assert(r >= 0);


Let's move the TAP specific logic to vhost-net specific reset_queue 
function.

Thanks

Re: [RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[Jason Wang]

在 2023/7/21 14:48, Eugenio Perez Martin 写道:
> On Thu, Jul 20, 2023 at 8:15 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>> At this moment the migration of net features that depends on CVQ is not
>> possible, as there is no reliable way to restore the device state like mac
>> address, number of enabled queues, etc to the destination.  This is mainly
>> caused because the device must only read CVQ, and process all the commands
>> before resuming the dataplane.
>>
>> This series uses the VirtIO feature _F_RING_RESET to achieve it, adding an
>> alternative method to late vq enabling proposed in [1][2].  It expose SVQ to
>> the device until it process all the CVQ messages, and then replaces the vring
>> for the guest's one.
>>
> A couple of things I forgot to add:
> * Assuming the implementation of _F_RING_RESET in vdpa is calling
> kernel vdpa ops .set_vq_ready(vq, false). I'm not sure if this is the
> best implementation, but it is currently unused in the kernel. At the
> same time, .set_vq_ready(vq, true) also enables the vq again.


I think we need another ops, as set_vq_ready() tends to be functional 
equivalent to queue_enable.

If we reuse set_vq_ready(vq, false), we would get conflict in the future 
when driver is allowed to stop/resume a specific virtqueue via setting 0 
to queue_enable. And that's also the reason why queue_enable is not 
resued to reset a virtqueue.


>
>> As an advantage, it uses a feature well deviced in the VirtIO standard.  As a
>> disadvantage, current HW already support the late enabling and it does not
>> support RING_RESET.
>>
>> This patch must be applied on top of the series ("Enable vdpa net migration
>> with features depending on CVQ") [1][2].
>>
>> The patch has been tested with vp_vdpa, but using high IOVA instead of a
>> sepparated ID for shadow CVQ and shadow temporal vrings.
>>
> And with _F_STATE implementation I sent long ago.
>
> Based on this, my suggestion is:
> * Leave the late enable for vDPA devices.
> * Make them fail if the vDPA parent device does not support it. This
> can be considered as a fix.
> * Leave _F_RING_RESET to be added on top, as the semantics are not
> implemented in vDPA at the moment.
>
> Would that work?


I think it can work, let's start from late enabling which seems 
lightweight than reset and see. We can leave the vp_vdpa to be done on 
top with another series.

Thanks


>
>> [1] Message-id: <20230706191227.835526-1-eperezma@redhat.com>
>> [2] https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg01325.html
>>
>> Eugenio Pérez (12):
>>    vhost: add vhost_reset_queue_op
>>    vhost: add vhost_restart_queue_op
>>    vhost_net: Use ops->vhost_restart_queue in vhost_net_virtqueue_restart
>>    vhost_net: Use ops->vhost_reset_queue in vhost_net_virtqueue_reset
>>    vdpa: add vhost_vdpa_set_vring_ready_internal
>>    vdpa: add vhost_vdpa_svq_stop
>>    vdpa: add vhost_vdpa_reset_queue
>>    vdpa: add vhost_vdpa_svq_start
>>    vdpa: add vhost_vdpa_restart_queue
>>    vdpa: enable all vqs if the device support RING_RESET feature
>>    vdpa: use SVQ to stall dataplane while NIC state is being restored
>>    vhost: Allow _F_RING_RESET with shadow virtqueue
>>
>>   include/hw/virtio/vhost-backend.h  |   6 ++
>>   hw/net/vhost_net.c                 |  16 ++--
>>   hw/virtio/vhost-shadow-virtqueue.c |   1 +
>>   hw/virtio/vhost-vdpa.c             | 139 +++++++++++++++++++++--------
>>   net/vhost-vdpa.c                   |  55 ++++++++++--
>>   hw/virtio/trace-events             |   2 +-
>>   6 files changed, 171 insertions(+), 48 deletions(-)
>>
>> --
>> 2.39.3
>>
>>

Re: [RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[Eugenio Perez Martin]

On Tue, Jul 25, 2023 at 9:15 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2023/7/21 14:48, Eugenio Perez Martin 写道:
> > On Thu, Jul 20, 2023 at 8:15 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >> At this moment the migration of net features that depends on CVQ is not
> >> possible, as there is no reliable way to restore the device state like mac
> >> address, number of enabled queues, etc to the destination.  This is mainly
> >> caused because the device must only read CVQ, and process all the commands
> >> before resuming the dataplane.
> >>
> >> This series uses the VirtIO feature _F_RING_RESET to achieve it, adding an
> >> alternative method to late vq enabling proposed in [1][2].  It expose SVQ to
> >> the device until it process all the CVQ messages, and then replaces the vring
> >> for the guest's one.
> >>
> > A couple of things I forgot to add:
> > * Assuming the implementation of _F_RING_RESET in vdpa is calling
> > kernel vdpa ops .set_vq_ready(vq, false). I'm not sure if this is the
> > best implementation, but it is currently unused in the kernel. At the
> > same time, .set_vq_ready(vq, true) also enables the vq again.
>
>
> I think we need another ops, as set_vq_ready() tends to be functional
> equivalent to queue_enable.
>
> If we reuse set_vq_ready(vq, false), we would get conflict in the future
> when driver is allowed to stop/resume a specific virtqueue via setting 0
> to queue_enable. And that's also the reason why queue_enable is not
> resued to reset a virtqueue.
>

Yes, I totally agree.

>
> >
> >> As an advantage, it uses a feature well deviced in the VirtIO standard.  As a
> >> disadvantage, current HW already support the late enabling and it does not
> >> support RING_RESET.
> >>
> >> This patch must be applied on top of the series ("Enable vdpa net migration
> >> with features depending on CVQ") [1][2].
> >>
> >> The patch has been tested with vp_vdpa, but using high IOVA instead of a
> >> sepparated ID for shadow CVQ and shadow temporal vrings.
> >>
> > And with _F_STATE implementation I sent long ago.
> >
> > Based on this, my suggestion is:
> > * Leave the late enable for vDPA devices.
> > * Make them fail if the vDPA parent device does not support it. This
> > can be considered as a fix.
> > * Leave _F_RING_RESET to be added on top, as the semantics are not
> > implemented in vDPA at the moment.
> >
> > Would that work?
>
>
> I think it can work, let's start from late enabling which seems
> lightweight than reset and see. We can leave the vp_vdpa to be done on
> top with another series.
>

great!

Thanks!


> Thanks
>
>
> >
> >> [1] Message-id: <20230706191227.835526-1-eperezma@redhat.com>
> >> [2] https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg01325.html
> >>
> >> Eugenio Pérez (12):
> >>    vhost: add vhost_reset_queue_op
> >>    vhost: add vhost_restart_queue_op
> >>    vhost_net: Use ops->vhost_restart_queue in vhost_net_virtqueue_restart
> >>    vhost_net: Use ops->vhost_reset_queue in vhost_net_virtqueue_reset
> >>    vdpa: add vhost_vdpa_set_vring_ready_internal
> >>    vdpa: add vhost_vdpa_svq_stop
> >>    vdpa: add vhost_vdpa_reset_queue
> >>    vdpa: add vhost_vdpa_svq_start
> >>    vdpa: add vhost_vdpa_restart_queue
> >>    vdpa: enable all vqs if the device support RING_RESET feature
> >>    vdpa: use SVQ to stall dataplane while NIC state is being restored
> >>    vhost: Allow _F_RING_RESET with shadow virtqueue
> >>
> >>   include/hw/virtio/vhost-backend.h  |   6 ++
> >>   hw/net/vhost_net.c                 |  16 ++--
> >>   hw/virtio/vhost-shadow-virtqueue.c |   1 +
> >>   hw/virtio/vhost-vdpa.c             | 139 +++++++++++++++++++++--------
> >>   net/vhost-vdpa.c                   |  55 ++++++++++--
> >>   hw/virtio/trace-events             |   2 +-
> >>   6 files changed, 171 insertions(+), 48 deletions(-)
> >>
> >> --
> >> 2.39.3
> >>
> >>
>

Re: [RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[Michael S. Tsirkin]

On Fri, Jul 21, 2023 at 08:48:02AM +0200, Eugenio Perez Martin wrote:
> * Leave _F_RING_RESET to be added on top, as the semantics are not
> implemented in vDPA at the moment.

We really need _F_RING_RESET in vdpa too though.
You did code it up already - why do you want to leave
it out?

-- 
MST

Re: [RFC PATCH 00/12] Prefer to use SVQ to stall dataplane at NIC state restore through CVQ

[Eugenio Perez Martin]

On Thu, Jul 27, 2023 at 3:06 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Jul 21, 2023 at 08:48:02AM +0200, Eugenio Perez Martin wrote:
> > * Leave _F_RING_RESET to be added on top, as the semantics are not
> > implemented in vDPA at the moment.
>
> We really need _F_RING_RESET in vdpa too though.
> You did code it up already - why do you want to leave
> it out?
>

I don't want to leave it out, sorry if it sounded that way.

I'd like to merge the late enable part first, as it has been already
validated and it works with current HW. The _F_RING_RESET part needs
more time regarding vhost vDPA API for Linux >=6.5, HW support, etc.

I can start working on the _F_RING_RESET kernel part needed changes
with a dedicated ioctl if this version looks good enough.

Thanks!