[RFC PATCH 00/12] NIC vhost-vdpa state restore via Shadow CVQ

[RFC PATCH 00/12] NIC vhost-vdpa state restore via Shadow CVQ

[Eugenio Pérez]

CVQ of net vhost-vdpa devices can be intercepted since the work of [1]. The
virtio-net device model is updated. The migration was blocked because although
the state can be megrated between VMM it was not possible to restore on the
destination NIC.

This series add support for SVQ to inject external messages without the guest's
knowledge, so before the guest is resumed all the guest visible state is
restored. It is done using standard CVQ messages, so the vhost-vdpa device does
not need to learn how to restore it: As long as they have the feature, they
know how to handle it.

This series needs SVQ CVQ support [1] and fixes [2] to be applied.

Thanks!

[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg02808.html
[2] https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg02726.html

Eugenio Pérez (12):
  vhost: Get vring base from vq, not svq
  vhost: Move SVQ queue rewind to the destination
  vdpa: Small rename of error labels
  vdpa: delay set_vring_ready after DRIVER_OK
  vhost: stop transfer elem ownership in vhost_handle_guest_kick
  vhost: Use opaque data in SVQDescState
  vhost: Add VhostVDPAStartOp operation
  vdpa: Add vhost_vdpa_start_control_svq
  vdpa: Extract vhost_vdpa_net_svq_add from
    vhost_vdpa_net_handle_ctrl_avail
  vdpa: Make vhost_vdpa_net_cvq_map_elem accept any out sg
  vdpa: Add virtio-net mac address via CVQ at start
  vdpa: Delete CVQ migration blocker

 hw/virtio/vhost-shadow-virtqueue.h |   7 +-
 include/hw/virtio/vhost-vdpa.h     |   6 +-
 hw/virtio/vhost-shadow-virtqueue.c |  30 ++---
 hw/virtio/vhost-vdpa.c             |  70 ++++++-----
 net/vhost-vdpa.c                   | 184 ++++++++++++++++++++---------
 5 files changed, 193 insertions(+), 104 deletions(-)

-- 
2.31.1

[RFC PATCH 01/12] vhost: Get vring base from vq, not svq

[Eugenio Pérez]

The SVQ vring used idx usually match with the guest visible one, as long
as all the guest buffers (GPA) maps to exactly one buffer within qemu's
VA. However, as we can see in virtqueue_map_desc, a single guest buffer
could map to many buffers in SVQ vring.

The solution is to stop using the device's used idx and check for the
last avail idx. Since we cannot report in-flight descriptors with vdpa,
let's rewind all of them.

Fixes: 6d0b22266633 ("vdpa: Adapt vhost_vdpa_get_vring_base to SVQ")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 795ed5a049..18820498b3 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1194,11 +1194,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
                                        struct vhost_vring_state *ring)
 {
     struct vhost_vdpa *v = dev->opaque;
-    int vdpa_idx = ring->index - dev->vq_index;
     int ret;
 
     if (v->shadow_vqs_enabled) {
-        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx);
+        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
 
         /*
          * Setting base as last used idx, so destination will see as available
@@ -1208,7 +1207,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
          * TODO: This is ok for networking, but other kinds of devices might
          * have problems with these retransmissions.
          */
-        ring->num = svq->last_used_idx;
+        while (virtqueue_rewind(vq, 1)) {
+            continue;
+        }
+        ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
         return 0;
     }
 
-- 
2.31.1

[RFC PATCH 02/12] vhost: Move SVQ queue rewind to the destination

[Eugenio Pérez]

Migration with SVQ already migrate the inflight descriptors, so the
destination can perform the work.

This makes easier to migrate between backends or to recover them in
vhost devices that support set in flight descriptors.

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 18820498b3..4458c8d23e 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1178,7 +1178,18 @@ static int vhost_vdpa_set_vring_base(struct vhost_dev *dev,
                                        struct vhost_vring_state *ring)
 {
     struct vhost_vdpa *v = dev->opaque;
+    VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
 
+    /*
+     * vhost-vdpa devices does not support in-flight requests. Set all of them
+     * as available.
+     *
+     * TODO: This is ok for networking, but other kinds of devices might
+     * have problems with these retransmissions.
+     */
+    while (virtqueue_rewind(vq, 1)) {
+        continue;
+    }
     if (v->shadow_vqs_enabled) {
         /*
          * Device vring base was set at device start. SVQ base is handled by
@@ -1197,19 +1208,6 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
     int ret;
 
     if (v->shadow_vqs_enabled) {
-        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
-
-        /*
-         * Setting base as last used idx, so destination will see as available
-         * all the entries that the device did not use, including the in-flight
-         * processing ones.
-         *
-         * TODO: This is ok for networking, but other kinds of devices might
-         * have problems with these retransmissions.
-         */
-        while (virtqueue_rewind(vq, 1)) {
-            continue;
-        }
         ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
         return 0;
     }
-- 
2.31.1

[RFC PATCH 03/12] vdpa: Small rename of error labels

[Eugenio Pérez]

So later patches are cleaner

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 4458c8d23e..906c365036 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1039,7 +1039,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
         int r;
         bool ok = vhost_vdpa_svq_setup(dev, svq, i, &err);
         if (unlikely(!ok)) {
-            goto err;
+            goto err_svq_setup;
         }
 
         vhost_svq_start(svq, dev->vdev, vq);
@@ -1064,8 +1064,7 @@ err_set_addr:
 err_map:
     vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, i));
 
-err:
-    error_reportf_err(err, "Cannot setup SVQ %u: ", i);
+err_svq_setup:
     for (unsigned j = 0; j < i; ++j) {
         VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, j);
         vhost_vdpa_svq_unmap_rings(dev, svq);
-- 
2.31.1

[RFC PATCH 04/12] vdpa: delay set_vring_ready after DRIVER_OK

[Eugenio Pérez]

To restore the device in the destination of a live migration we send the
commands through control virtqueue. For a device to read CVQ it must
have received DRIVER_OK status bit.

However this open a window where the device could start receiving
packets in rx queue 0 before it receive the RSS configuration. To avoid
that, we will not send vring_enable until all configuration is used by
the device.

As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 906c365036..1d8829c619 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -730,13 +730,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
     return idx;
 }
 
+/**
+ * Set ready all vring of the device
+ *
+ * @dev: Vhost device
+ */
 static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
 {
     int i;
     trace_vhost_vdpa_set_vring_ready(dev);
-    for (i = 0; i < dev->nvqs; ++i) {
+    for (i = 0; i < dev->vq_index_end; ++i) {
         struct vhost_vring_state state = {
-            .index = dev->vq_index + i,
+            .index = i,
             .num = 1,
         };
         vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
@@ -1111,7 +1116,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
         if (unlikely(!ok)) {
             return -1;
         }
-        vhost_vdpa_set_vring_ready(dev);
     } else {
         ok = vhost_vdpa_svqs_stop(dev);
         if (unlikely(!ok)) {
@@ -1125,16 +1129,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
     }
 
     if (started) {
+        int r;
+
         memory_listener_register(&v->listener, &address_space_memory);
-        return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+        r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+        if (unlikely(r)) {
+            return r;
+        }
+        vhost_vdpa_set_vring_ready(dev);
     } else {
         vhost_vdpa_reset_device(dev);
         vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                                    VIRTIO_CONFIG_S_DRIVER);
         memory_listener_unregister(&v->listener);
-
-        return 0;
     }
+
+    return 0;
 }
 
 static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
-- 
2.31.1

[RFC PATCH 05/12] vhost: stop transfer elem ownership in vhost_handle_guest_kick

[Eugenio Pérez]

It was easier to allow vhost_svq_add to handle the memory. Now that we
will move SVQDesc to an opaque context, it's better to handle it in the
caller.

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

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index a21b0b1bf6..29633b7a29 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -233,9 +233,6 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
 /**
  * Add an element to a SVQ.
  *
- * The caller must check that there is enough slots for the new element. It
- * takes ownership of the element: In case of failure not ENOSPC, it is free.
- *
  * Return -EINVAL if element is invalid, -ENOSPC if dev queue is full
  */
 int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
@@ -252,7 +249,6 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
 
     ok = vhost_svq_add_split(svq, out_sg, out_num, in_sg, in_num, &qemu_head);
     if (unlikely(!ok)) {
-        g_free(elem);
         return -EINVAL;
     }
 
@@ -293,7 +289,7 @@ static void vhost_handle_guest_kick(VhostShadowVirtqueue *svq)
         virtio_queue_set_notification(svq->vq, false);
 
         while (true) {
-            VirtQueueElement *elem;
+            g_autofree VirtQueueElement *elem;
             int r;
 
             if (svq->next_guest_avail_elem) {
@@ -324,12 +320,14 @@ static void vhost_handle_guest_kick(VhostShadowVirtqueue *svq)
                      * queue the current guest descriptor and ignore kicks
                      * until some elements are used.
                      */
-                    svq->next_guest_avail_elem = elem;
+                    svq->next_guest_avail_elem = g_steal_pointer(&elem);
                 }
 
                 /* VQ is full or broken, just return and ignore kicks */
                 return;
             }
+            /* elem belongs to SVQ or external caller now */
+            elem = NULL;
         }
 
         virtio_queue_set_notification(svq->vq, true);
-- 
2.31.1

[RFC PATCH 06/12] vhost: Use opaque data in SVQDescState

[Eugenio Pérez]

Since we'll allow net/vhost-vdpa to add elements that don't come from
the guest, we need to store opaque data that makes sense to the caller.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.h |  4 ++--
 hw/virtio/vhost-shadow-virtqueue.c | 20 +++++++++++---------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
index d04c34a589..03eb7ff670 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -16,7 +16,7 @@
 #include "hw/virtio/vhost-iova-tree.h"
 
 typedef struct SVQDescState {
-    VirtQueueElement *elem;
+    void *data;
 
     /*
      * Number of descriptors exposed to the device. May or may not match
@@ -115,7 +115,7 @@ void vhost_svq_push_elem(VhostShadowVirtqueue *svq,
                          const VirtQueueElement *elem, uint32_t len);
 int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
                   size_t out_num, const struct iovec *in_sg, size_t in_num,
-                  VirtQueueElement *elem);
+                  void *data);
 size_t vhost_svq_poll(VhostShadowVirtqueue *svq);
 
 void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 29633b7a29..88e290d94b 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -237,7 +237,7 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
  */
 int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
                   size_t out_num, const struct iovec *in_sg, size_t in_num,
-                  VirtQueueElement *elem)
+                  void *data)
 {
     unsigned qemu_head;
     unsigned ndescs = in_num + out_num;
@@ -252,7 +252,7 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
         return -EINVAL;
     }
 
-    svq->desc_state[qemu_head].elem = elem;
+    svq->desc_state[qemu_head].data = data;
     svq->desc_state[qemu_head].ndescs = ndescs;
     vhost_svq_kick(svq);
     return 0;
@@ -389,8 +389,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const VhostShadowVirtqueue *svq,
     return i;
 }
 
-static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
-                                           uint32_t *len)
+static void *vhost_svq_get_buf(VhostShadowVirtqueue *svq, uint32_t *len)
 {
     const vring_used_t *used = svq->vring.used;
     vring_used_elem_t used_elem;
@@ -413,7 +412,7 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
         return NULL;
     }
 
-    if (unlikely(!svq->desc_state[used_elem.id].elem)) {
+    if (unlikely(!svq->desc_state[used_elem.id].data)) {
         qemu_log_mask(LOG_GUEST_ERROR,
             "Device %s says index %u is used, but it was not available",
             svq->vdev->name, used_elem.id);
@@ -426,7 +425,7 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
     svq->free_head = used_elem.id;
 
     *len = used_elem.len;
-    return g_steal_pointer(&svq->desc_state[used_elem.id].elem);
+    return g_steal_pointer(&svq->desc_state[used_elem.id].data);
 }
 
 /**
@@ -498,8 +497,7 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq)
 {
     do {
         uint32_t len;
-        VirtQueueElement *elem = vhost_svq_get_buf(svq, &len);
-        if (elem) {
+        if (vhost_svq_get_buf(svq, &len)) {
             return len;
         }
 
@@ -658,8 +656,12 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
     vhost_svq_flush(svq, false);
 
     for (unsigned i = 0; i < svq->vring.num; ++i) {
+        /*
+         * We know .data is an element because external callers of
+         * vhost_svq_add use active polling, not SVQ
+         */
         g_autofree VirtQueueElement *elem = NULL;
-        elem = g_steal_pointer(&svq->desc_state[i].elem);
+        elem = g_steal_pointer(&svq->desc_state[i].data);
         if (elem) {
             virtqueue_detach_element(svq->vq, elem, 0);
         }
-- 
2.31.1

[RFC PATCH 07/12] vhost: Add VhostVDPAStartOp operation

[Eugenio Pérez]

It allows to run commands at start of the device, before it have enabled
any queue.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.h | 3 +++
 include/hw/virtio/vhost-vdpa.h     | 5 +++++
 hw/virtio/vhost-vdpa.c             | 8 ++++++++
 3 files changed, 16 insertions(+)

diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
index 03eb7ff670..210fe393cd 100644
--- a/hw/virtio/vhost-shadow-virtqueue.h
+++ b/hw/virtio/vhost-shadow-virtqueue.h
@@ -26,6 +26,8 @@ typedef struct SVQDescState {
 } SVQDescState;
 
 typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
+typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
+                                    void *opaque);
 
 /**
  * Callback to handle an avail buffer.
@@ -43,6 +45,7 @@ typedef int (*VirtQueueAvailCallback)(VhostShadowVirtqueue *svq,
                                       void *vq_callback_opaque);
 
 typedef struct VhostShadowVirtqueueOps {
+    ShadowVirtQueueStart start;
     VirtQueueAvailCallback avail_handler;
 } VhostShadowVirtqueueOps;
 
diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index d10a89303e..b7d18b4e30 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -24,6 +24,10 @@ typedef struct VhostVDPAHostNotifier {
     void *addr;
 } VhostVDPAHostNotifier;
 
+struct vhost_vdpa;
+/* Called after send DRIVER_OK but after enabling vrings */
+typedef int (*VhostVDPAStartOp)(struct vhost_vdpa *v);
+
 typedef struct vhost_vdpa {
     int device_fd;
     int index;
@@ -39,6 +43,7 @@ typedef struct vhost_vdpa {
     GPtrArray *shadow_vqs;
     const VhostShadowVirtqueueOps *shadow_vq_ops;
     void *shadow_vq_ops_opaque;
+    VhostVDPAStartOp start_op;
     struct vhost_dev *dev;
     VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
 } VhostVDPA;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 1d8829c619..48f031b8c0 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1136,6 +1136,14 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
         if (unlikely(r)) {
             return r;
         }
+
+        if (v->start_op) {
+            r = v->start_op(v);
+            if (unlikely(r)) {
+                return r;
+            }
+        }
+
         vhost_vdpa_set_vring_ready(dev);
     } else {
         vhost_vdpa_reset_device(dev);
-- 
2.31.1

[RFC PATCH 08/12] vdpa: Add vhost_vdpa_start_control_svq

[Eugenio Pérez]

As a first step we only enable CVQ first than others. Future patches add
state restore.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 986e6414b4..211bd0468b 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -334,6 +334,18 @@ static bool vhost_vdpa_net_cvq_map_elem(VhostVDPAState *s,
     return true;
 }
 
+static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
+{
+    struct vhost_vring_state state = {
+        .index = v->dev->vq_index,
+        .num = 1,
+    };
+    int r;
+
+    r = ioctl(v->device_fd, VHOST_VDPA_SET_VRING_ENABLE, &state);
+    return r < 0 ? -errno : r;
+}
+
 /**
  * Do not forward commands not supported by SVQ. Otherwise, the device could
  * accept it and qemu would not know how to update the device model.
@@ -493,6 +505,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
                                             vhost_vdpa_net_cvq_cmd_page_len());
         memset(s->cvq_cmd_in_buffer, 0, vhost_vdpa_net_cvq_cmd_page_len());
 
+        s->vhost_vdpa.start_op = vhost_vdpa_start_control_svq;
         s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops;
         s->vhost_vdpa.shadow_vq_ops_opaque = s;
         error_setg(&s->vhost_vdpa.migration_blocker,
-- 
2.31.1

[RFC PATCH 09/12] vdpa: Extract vhost_vdpa_net_svq_add from vhost_vdpa_net_handle_ctrl_avail

[Eugenio Pérez]

So we can reuse to inject state messages.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 211bd0468b..aaae51a778 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -334,6 +334,54 @@ static bool vhost_vdpa_net_cvq_map_elem(VhostVDPAState *s,
     return true;
 }
 
+static virtio_net_ctrl_ack vhost_vdpa_net_svq_add(VhostShadowVirtqueue *svq,
+                                               const struct iovec *dev_buffers)
+{
+    /* in buffer used for device model */
+    virtio_net_ctrl_ack status;
+    const struct iovec in = {
+        .iov_base = &status,
+        .iov_len = sizeof(status),
+    };
+    size_t dev_written;
+    int r;
+    void *unused = (void *)1;
+
+    r = vhost_svq_add(svq, &dev_buffers[0], 1, &dev_buffers[1], 1, unused);
+    if (unlikely(r != 0)) {
+        if (unlikely(r == -ENOSPC)) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device queue\n",
+                          __func__);
+        }
+        return VIRTIO_NET_ERR;
+    }
+
+    /*
+     * We can poll here since we've had BQL from the time we sent the
+     * descriptor. Also, we need to take the answer before SVQ pulls by itself,
+     * when BQL is released
+     */
+    dev_written = vhost_svq_poll(svq);
+    if (unlikely(dev_written < sizeof(status))) {
+        error_report("Insufficient written data (%zu)", dev_written);
+        return VIRTIO_NET_ERR;
+    }
+
+    memcpy(&status, dev_buffers[1].iov_base, sizeof(status));
+    if (status != VIRTIO_NET_OK) {
+        return VIRTIO_NET_ERR;
+    }
+
+    status = VIRTIO_NET_ERR;
+    virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, dev_buffers, 1);
+    if (status != VIRTIO_NET_OK) {
+        error_report("Bad CVQ processing in model");
+        return VIRTIO_NET_ERR;
+    }
+
+    return VIRTIO_NET_OK;
+}
+
 static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
 {
     struct vhost_vring_state state = {
@@ -392,19 +440,13 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
                                             void *opaque)
 {
     VhostVDPAState *s = opaque;
-    size_t in_len, dev_written;
+    size_t in_len;
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     /* out and in buffers sent to the device */
     struct iovec dev_buffers[2] = {
         { .iov_base = s->cvq_cmd_out_buffer },
         { .iov_base = s->cvq_cmd_in_buffer },
     };
-    /* in buffer used for device model */
-    const struct iovec in = {
-        .iov_base = &status,
-        .iov_len = sizeof(status),
-    };
-    int r;
     bool ok;
 
     ok = vhost_vdpa_net_cvq_map_elem(s, elem, dev_buffers);
@@ -417,36 +459,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
         goto out;
     }
 
-    r = vhost_svq_add(svq, &dev_buffers[0], 1, &dev_buffers[1], 1, elem);
-    if (unlikely(r != 0)) {
-        if (unlikely(r == -ENOSPC)) {
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device queue\n",
-                          __func__);
-        }
-        goto out;
-    }
-
-    /*
-     * We can poll here since we've had BQL from the time we sent the
-     * descriptor. Also, we need to take the answer before SVQ pulls by itself,
-     * when BQL is released
-     */
-    dev_written = vhost_svq_poll(svq);
-    if (unlikely(dev_written < sizeof(status))) {
-        error_report("Insufficient written data (%zu)", dev_written);
-        goto out;
-    }
-
-    memcpy(&status, dev_buffers[1].iov_base, sizeof(status));
-    if (status != VIRTIO_NET_OK) {
-        goto out;
-    }
-
-    status = VIRTIO_NET_ERR;
-    virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, dev_buffers, 1);
-    if (status != VIRTIO_NET_OK) {
-        error_report("Bad CVQ processing in model");
-    }
+    status = vhost_vdpa_net_svq_add(svq, dev_buffers);
 
 out:
     in_len = iov_from_buf(elem->in_sg, elem->in_num, 0, &status,
@@ -462,7 +475,7 @@ out:
     if (dev_buffers[1].iov_base) {
         vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, dev_buffers[1].iov_base);
     }
-    return r;
+    return status == VIRTIO_NET_OK ? 0 : 1;
 }
 
 static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
-- 
2.31.1

[RFC PATCH 10/12] vdpa: Make vhost_vdpa_net_cvq_map_elem accept any out sg

[Eugenio Pérez]

So its generic enough to accept any out sg buffer and we can inject
NIC state messages.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index aaae51a778..0183fce353 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -302,35 +302,36 @@ dma_map_err:
 }
 
 /**
- * Copy the guest element into a dedicated buffer suitable to be sent to NIC
+ * Maps out sg and in buffer into dedicated buffers suitable to be sent to NIC
  *
- * @iov: [0] is the out buffer, [1] is the in one
+ * @dev_iov: [0] is the out buffer, [1] is the in one
  */
-static bool vhost_vdpa_net_cvq_map_elem(VhostVDPAState *s,
-                                        VirtQueueElement *elem,
-                                        struct iovec *iov)
+static bool vhost_vdpa_net_cvq_map_sg(VhostVDPAState *s,
+                                      const struct iovec *out, size_t out_num,
+                                      struct iovec *dev_iov)
 {
     size_t in_copied;
     bool ok;
 
-    iov[0].iov_base = s->cvq_cmd_out_buffer;
-    ok = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, elem->out_sg, elem->out_num,
-                                vhost_vdpa_net_cvq_cmd_len(), iov[0].iov_base,
-                                &iov[0].iov_len, false);
+    dev_iov[0].iov_base = s->cvq_cmd_out_buffer;
+    ok = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, out, out_num,
+                                vhost_vdpa_net_cvq_cmd_len(),
+                                dev_iov[0].iov_base, &dev_iov[0].iov_len,
+                                false);
     if (unlikely(!ok)) {
         return false;
     }
 
-    iov[1].iov_base = s->cvq_cmd_in_buffer;
+    dev_iov[1].iov_base = s->cvq_cmd_in_buffer;
     ok = vhost_vdpa_cvq_map_buf(&s->vhost_vdpa, NULL, 0,
-                                sizeof(virtio_net_ctrl_ack), iov[1].iov_base,
-                                &in_copied, true);
+                                sizeof(virtio_net_ctrl_ack),
+                                dev_iov[1].iov_base, &in_copied, true);
     if (unlikely(!ok)) {
         vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
         return false;
     }
 
-    iov[1].iov_len = sizeof(virtio_net_ctrl_ack);
+    dev_iov[1].iov_len = sizeof(virtio_net_ctrl_ack);
     return true;
 }
 
@@ -449,7 +450,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
     };
     bool ok;
 
-    ok = vhost_vdpa_net_cvq_map_elem(s, elem, dev_buffers);
+    ok = vhost_vdpa_net_cvq_map_sg(s, elem->out_sg, elem->out_num, dev_buffers);
     if (unlikely(!ok)) {
         goto out;
     }
-- 
2.31.1

[RFC PATCH 11/12] vdpa: Add virtio-net mac address via CVQ at start

[Eugenio Pérez]

This is needed so the destination vdpa device see the same state a the
guest set in the source.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 0183fce353..2873be2ba4 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -383,7 +383,7 @@ static virtio_net_ctrl_ack vhost_vdpa_net_svq_add(VhostShadowVirtqueue *svq,
     return VIRTIO_NET_OK;
 }
 
-static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
+static int vhost_vdpa_enable_control_svq(struct vhost_vdpa *v)
 {
     struct vhost_vring_state state = {
         .index = v->dev->vq_index,
@@ -395,6 +395,57 @@ static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
     return r < 0 ? -errno : r;
 }
 
+static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
+{
+
+    VirtIONet *n = VIRTIO_NET(v->dev->vdev);
+    uint64_t features = v->dev->vdev->host_features;
+    VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, 0);
+    VhostVDPAState *s = container_of(v, VhostVDPAState, vhost_vdpa);
+    int r;
+
+    r = vhost_vdpa_enable_control_svq(v);
+    if (unlikely(r < 0)) {
+        return r;
+    }
+
+    if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
+        const struct virtio_net_ctrl_hdr ctrl = {
+            .class = VIRTIO_NET_CTRL_MAC,
+            .cmd = VIRTIO_NET_CTRL_MAC_ADDR_SET,
+        };
+        uint8_t mac[6];
+        const struct iovec out[] = {
+            {
+                .iov_base = (void *)&ctrl,
+                .iov_len = sizeof(ctrl),
+            },{
+                .iov_base = mac,
+                .iov_len = sizeof(mac),
+            },
+        };
+        struct iovec dev_buffers[2] = {
+            { .iov_base = s->cvq_cmd_out_buffer },
+            { .iov_base = s->cvq_cmd_in_buffer },
+        };
+        bool ok;
+        virtio_net_ctrl_ack state;
+
+        ok = vhost_vdpa_net_cvq_map_sg(s, out, ARRAY_SIZE(out), dev_buffers);
+        if (unlikely(!ok)) {
+            return -1;
+        }
+
+        memcpy(mac, n->mac, sizeof(mac));
+        state = vhost_vdpa_net_svq_add(svq, dev_buffers);
+        vhost_vdpa_cvq_unmap_buf(v, dev_buffers[0].iov_base);
+        vhost_vdpa_cvq_unmap_buf(v, dev_buffers[1].iov_base);
+        return state == VIRTIO_NET_OK ? 0 : 1;
+    }
+
+    return 0;
+}
+
 /**
  * Do not forward commands not supported by SVQ. Otherwise, the device could
  * accept it and qemu would not know how to update the device model.
-- 
2.31.1

[RFC PATCH 12/12] vdpa: Delete CVQ migration blocker

[Eugenio Pérez]

We can restore the device state in the destination via CVQ now. Remove
the migration blocker.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 include/hw/virtio/vhost-vdpa.h |  1 -
 hw/virtio/vhost-vdpa.c         | 11 -----------
 net/vhost-vdpa.c               |  2 --
 3 files changed, 14 deletions(-)

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index b7d18b4e30..85b8a53052 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -39,7 +39,6 @@ typedef struct vhost_vdpa {
     bool shadow_vqs_enabled;
     /* IOVA mapping used by the Shadow Virtqueue */
     VhostIOVATree *iova_tree;
-    Error *migration_blocker;
     GPtrArray *shadow_vqs;
     const VhostShadowVirtqueueOps *shadow_vq_ops;
     void *shadow_vq_ops_opaque;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 48f031b8c0..80bf461cf8 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1028,13 +1028,6 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
         return true;
     }
 
-    if (v->migration_blocker) {
-        int r = migrate_add_blocker(v->migration_blocker, &err);
-        if (unlikely(r < 0)) {
-            goto err_migration_blocker;
-        }
-    }
-
     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);
@@ -1076,7 +1069,6 @@ err_svq_setup:
         vhost_svq_stop(svq);
     }
 
-err_migration_blocker:
     error_reportf_err(err, "Cannot setup SVQ %u: ", i);
 
     return false;
@@ -1098,9 +1090,6 @@ static bool vhost_vdpa_svqs_stop(struct vhost_dev *dev)
         }
     }
 
-    if (v->migration_blocker) {
-        migrate_del_blocker(v->migration_blocker);
-    }
     return true;
 }
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 2873be2ba4..80ea9a1412 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -573,8 +573,6 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
         s->vhost_vdpa.start_op = vhost_vdpa_start_control_svq;
         s->vhost_vdpa.shadow_vq_ops = &vhost_vdpa_net_svq_ops;
         s->vhost_vdpa.shadow_vq_ops_opaque = s;
-        error_setg(&s->vhost_vdpa.migration_blocker,
-                   "Migration disabled: vhost-vdpa uses CVQ.");
     }
     ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa, queue_pair_index, nvqs);
     if (ret) {
-- 
2.31.1

Re: [RFC PATCH 07/12] vhost: Add VhostVDPAStartOp operation

[Eugenio Perez Martin]

On Sat, Jul 16, 2022 at 1:44 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> It allows to run commands at start of the device, before it have enabled
> any queue.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.h | 3 +++
>  include/hw/virtio/vhost-vdpa.h     | 5 +++++
>  hw/virtio/vhost-vdpa.c             | 8 ++++++++
>  3 files changed, 16 insertions(+)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index 03eb7ff670..210fe393cd 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -26,6 +26,8 @@ typedef struct SVQDescState {
>  } SVQDescState;
>
>  typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> +typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
> +                                    void *opaque);
>
>  /**
>   * Callback to handle an avail buffer.
> @@ -43,6 +45,7 @@ typedef int (*VirtQueueAvailCallback)(VhostShadowVirtqueue *svq,
>                                        void *vq_callback_opaque);
>
>  typedef struct VhostShadowVirtqueueOps {
> +    ShadowVirtQueueStart start;
>      VirtQueueAvailCallback avail_handler;
>  } VhostShadowVirtqueueOps;
>

Changes above are leftovers, I'll remove them for the next version.

Re: [RFC PATCH 01/12] vhost: Get vring base from vq, not svq

[Jason Wang]

On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> The SVQ vring used idx usually match with the guest visible one, as long
> as all the guest buffers (GPA) maps to exactly one buffer within qemu's
> VA. However, as we can see in virtqueue_map_desc, a single guest buffer
> could map to many buffers in SVQ vring.
>
> The solution is to stop using the device's used idx and check for the
> last avail idx. Since we cannot report in-flight descriptors with vdpa,
> let's rewind all of them.
>
> Fixes: 6d0b22266633 ("vdpa: Adapt vhost_vdpa_get_vring_base to SVQ")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-vdpa.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 795ed5a049..18820498b3 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -1194,11 +1194,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
>                                         struct vhost_vring_state *ring)
>  {
>      struct vhost_vdpa *v = dev->opaque;
> -    int vdpa_idx = ring->index - dev->vq_index;
>      int ret;
>
>      if (v->shadow_vqs_enabled) {
> -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx);
> +        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
>
>          /*
>           * Setting base as last used idx, so destination will see as available
> @@ -1208,7 +1207,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
>           * TODO: This is ok for networking, but other kinds of devices might
>           * have problems with these retransmissions.
>           */
> -        ring->num = svq->last_used_idx;
> +        while (virtqueue_rewind(vq, 1)) {
> +            continue;
> +        }

Will this leak mapped VirtQueueElement?

Thanks

> +        ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
>          return 0;
>      }
>
> --
> 2.31.1
>

Re: [RFC PATCH 02/12] vhost: Move SVQ queue rewind to the destination

[Jason Wang]

On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Migration with SVQ already migrate the inflight descriptors,

How is this done?

> so the
> destination can perform the work.
>
> This makes easier to migrate between backends or to recover them in
> vhost devices that support set in flight descriptors.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-vdpa.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 18820498b3..4458c8d23e 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -1178,7 +1178,18 @@ static int vhost_vdpa_set_vring_base(struct vhost_dev *dev,
>                                         struct vhost_vring_state *ring)
>  {
>      struct vhost_vdpa *v = dev->opaque;
> +    VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
>
> +    /*
> +     * vhost-vdpa devices does not support in-flight requests. Set all of them
> +     * as available.
> +     *
> +     * TODO: This is ok for networking, but other kinds of devices might
> +     * have problems with these retransmissions.
> +     */
> +    while (virtqueue_rewind(vq, 1)) {
> +        continue;
> +    }
>      if (v->shadow_vqs_enabled) {
>          /*
>           * Device vring base was set at device start. SVQ base is handled by
> @@ -1197,19 +1208,6 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
>      int ret;
>
>      if (v->shadow_vqs_enabled) {
> -        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> -
> -        /*
> -         * Setting base as last used idx, so destination will see as available
> -         * all the entries that the device did not use, including the in-flight
> -         * processing ones.
> -         *
> -         * TODO: This is ok for networking, but other kinds of devices might
> -         * have problems with these retransmissions.
> -         */
> -        while (virtqueue_rewind(vq, 1)) {
> -            continue;
> -        }

I think it's not a good idea to partially revert the code that was
just introduced in the previous patch (unless it can be backported to
-stable independently).

We probably need to squash the changes.

Thanks

>          ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
>          return 0;
>      }
> --
> 2.31.1
>

Re: [RFC PATCH 03/12] vdpa: Small rename of error labels

[Jason Wang]

On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> So later patches are cleaner
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-vdpa.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 4458c8d23e..906c365036 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -1039,7 +1039,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
>          int r;
>          bool ok = vhost_vdpa_svq_setup(dev, svq, i, &err);
>          if (unlikely(!ok)) {
> -            goto err;
> +            goto err_svq_setup;
>          }
>
>          vhost_svq_start(svq, dev->vdev, vq);
> @@ -1064,8 +1064,7 @@ err_set_addr:
>  err_map:
>      vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, i));
>
> -err:
> -    error_reportf_err(err, "Cannot setup SVQ %u: ", i);

This does not look like a simple rename.

Thanks

> +err_svq_setup:
>      for (unsigned j = 0; j < i; ++j) {
>          VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, j);
>          vhost_vdpa_svq_unmap_rings(dev, svq);
> --
> 2.31.1
>

Re: [RFC PATCH 04/12] vdpa: delay set_vring_ready after DRIVER_OK

[Jason Wang]

On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> To restore the device in the destination of a live migration we send the
> commands through control virtqueue. For a device to read CVQ it must
> have received DRIVER_OK status bit.
>
> However this open a window where the device could start receiving
> packets in rx queue 0 before it receive the RSS configuration.

A note here is that, after chatting with Michael, the device should
not start processing the buffer until a kick. So I wonder if it's more
than enough to not kick the data vq paris until we've recovered the
state via cvq?


> To avoid
> that, we will not send vring_enable until all configuration is used by
> the device.
>
> As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 906c365036..1d8829c619 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -730,13 +730,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
>      return idx;
>  }
>
> +/**
> + * Set ready all vring of the device

It should be better to mention, for device we mean virtio device
instead of vhost device (which we may have multiple ones if mq is
enabled).

Thanks

> + *
> + * @dev: Vhost device
> + */
>  static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
>  {
>      int i;
>      trace_vhost_vdpa_set_vring_ready(dev);
> -    for (i = 0; i < dev->nvqs; ++i) {
> +    for (i = 0; i < dev->vq_index_end; ++i) {
>          struct vhost_vring_state state = {
> -            .index = dev->vq_index + i,
> +            .index = i,
>              .num = 1,
>          };
>          vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> @@ -1111,7 +1116,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>          if (unlikely(!ok)) {
>              return -1;
>          }
> -        vhost_vdpa_set_vring_ready(dev);
>      } else {
>          ok = vhost_vdpa_svqs_stop(dev);
>          if (unlikely(!ok)) {
> @@ -1125,16 +1129,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>      }
>
>      if (started) {
> +        int r;
> +
>          memory_listener_register(&v->listener, &address_space_memory);
> -        return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> +        r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> +        if (unlikely(r)) {
> +            return r;
> +        }
> +        vhost_vdpa_set_vring_ready(dev);
>      } else {
>          vhost_vdpa_reset_device(dev);
>          vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                     VIRTIO_CONFIG_S_DRIVER);
>          memory_listener_unregister(&v->listener);
> -
> -        return 0;
>      }
> +
> +    return 0;
>  }
>
>  static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> --
> 2.31.1
>

Re: [RFC PATCH 04/12] vdpa: delay set_vring_ready after DRIVER_OK

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 8:34 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > To restore the device in the destination of a live migration we send the
> > commands through control virtqueue. For a device to read CVQ it must
> > have received DRIVER_OK status bit.
> >
> > However this open a window where the device could start receiving
> > packets in rx queue 0 before it receive the RSS configuration.
>
> A note here is that, after chatting with Michael, the device should
> not start processing the buffer until a kick. So I wonder if it's more
> than enough to not kick the data vq paris until we've recovered the
> state via cvq?
>

Oh, I see I misunderstood the mail now :). You both have a very good point.

Take into account the set of the state will also happen when
transitioning from passthrough mode to SVQ mode. In that switch we're
not pausing the VM (AFAIK), so guests are free to kick dvqs.

But it is totally right that it is not strictly needed for this
particular series, as:
* We're in full control of the device with SVQ.
* Guest still has not seen the DRIVER_OK bit set.

So I'll delay that part for the one able to do the transition. It's
totally safe that the guest kicks before DRIVER_OK at this moment (as
I explained in the other mail).

Thanks!

>
> > To avoid
> > that, we will not send vring_enable until all configuration is used by
> > the device.
> >
> > As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 906c365036..1d8829c619 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -730,13 +730,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> >      return idx;
> >  }
> >
> > +/**
> > + * Set ready all vring of the device
>
> It should be better to mention, for device we mean virtio device
> instead of vhost device (which we may have multiple ones if mq is
> enabled).
>
> Thanks
>
> > + *
> > + * @dev: Vhost device
> > + */
> >  static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> >  {
> >      int i;
> >      trace_vhost_vdpa_set_vring_ready(dev);
> > -    for (i = 0; i < dev->nvqs; ++i) {
> > +    for (i = 0; i < dev->vq_index_end; ++i) {
> >          struct vhost_vring_state state = {
> > -            .index = dev->vq_index + i,
> > +            .index = i,
> >              .num = 1,
> >          };
> >          vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> > @@ -1111,7 +1116,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >          if (unlikely(!ok)) {
> >              return -1;
> >          }
> > -        vhost_vdpa_set_vring_ready(dev);
> >      } else {
> >          ok = vhost_vdpa_svqs_stop(dev);
> >          if (unlikely(!ok)) {
> > @@ -1125,16 +1129,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >      }
> >
> >      if (started) {
> > +        int r;
> > +
> >          memory_listener_register(&v->listener, &address_space_memory);
> > -        return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > +        r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > +        if (unlikely(r)) {
> > +            return r;
> > +        }
> > +        vhost_vdpa_set_vring_ready(dev);
> >      } else {
> >          vhost_vdpa_reset_device(dev);
> >          vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >                                     VIRTIO_CONFIG_S_DRIVER);
> >          memory_listener_unregister(&v->listener);
> > -
> > -        return 0;
> >      }
> > +
> > +    return 0;
> >  }
> >
> >  static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH 01/12] vhost: Get vring base from vq, not svq

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 7:48 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > The SVQ vring used idx usually match with the guest visible one, as long
> > as all the guest buffers (GPA) maps to exactly one buffer within qemu's
> > VA. However, as we can see in virtqueue_map_desc, a single guest buffer
> > could map to many buffers in SVQ vring.
> >
> > The solution is to stop using the device's used idx and check for the
> > last avail idx. Since we cannot report in-flight descriptors with vdpa,
> > let's rewind all of them.
> >
> > Fixes: 6d0b22266633 ("vdpa: Adapt vhost_vdpa_get_vring_base to SVQ")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 795ed5a049..18820498b3 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1194,11 +1194,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> >                                         struct vhost_vring_state *ring)
> >  {
> >      struct vhost_vdpa *v = dev->opaque;
> > -    int vdpa_idx = ring->index - dev->vq_index;
> >      int ret;
> >
> >      if (v->shadow_vqs_enabled) {
> > -        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, vdpa_idx);
> > +        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> >
> >          /*
> >           * Setting base as last used idx, so destination will see as available
> > @@ -1208,7 +1207,10 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> >           * TODO: This is ok for networking, but other kinds of devices might
> >           * have problems with these retransmissions.
> >           */
> > -        ring->num = svq->last_used_idx;
> > +        while (virtqueue_rewind(vq, 1)) {
> > +            continue;
> > +        }
>
> Will this leak mapped VirtQueueElement?
>

vhost_get_vring_base op is called only on the device's teardown path,
so they should be free by vhost_svq_stop.

However, you have a point that maybe vhost_get_vring_base should not
trust in that cleaning, even for -stable.

So I think it should be better to squash this and the next one as the
same fix. But it's doing two things at the same time: One of them is
to use the right state (as vring_base), and another one is not
reverting in-flight descriptors.

Thanks!

> Thanks
>
> > +        ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
> >          return 0;
> >      }
> >
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH 02/12] vhost: Move SVQ queue rewind to the destination

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 7:50 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Migration with SVQ already migrate the inflight descriptors,
>
> How is this done?
>

Migration between vhost-vdpa with x-svq=on maintain the guest's
visible state in VirtQueues, and they already have all the protocols
to migrate them.

We cannot migrate if we cannot restore the state / inflight in the
destination, but since we need x-svq=on at this point, we can restore
both of them. Extra care will be needed when ASID is introduced.

> > so the
> > destination can perform the work.
> >
> > This makes easier to migrate between backends or to recover them in
> > vhost devices that support set in flight descriptors.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 18820498b3..4458c8d23e 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1178,7 +1178,18 @@ static int vhost_vdpa_set_vring_base(struct vhost_dev *dev,
> >                                         struct vhost_vring_state *ring)
> >  {
> >      struct vhost_vdpa *v = dev->opaque;
> > +    VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> >
> > +    /*
> > +     * vhost-vdpa devices does not support in-flight requests. Set all of them
> > +     * as available.
> > +     *
> > +     * TODO: This is ok for networking, but other kinds of devices might
> > +     * have problems with these retransmissions.
> > +     */
> > +    while (virtqueue_rewind(vq, 1)) {
> > +        continue;
> > +    }
> >      if (v->shadow_vqs_enabled) {
> >          /*
> >           * Device vring base was set at device start. SVQ base is handled by
> > @@ -1197,19 +1208,6 @@ static int vhost_vdpa_get_vring_base(struct vhost_dev *dev,
> >      int ret;
> >
> >      if (v->shadow_vqs_enabled) {
> > -        VirtQueue *vq = virtio_get_queue(dev->vdev, ring->index);
> > -
> > -        /*
> > -         * Setting base as last used idx, so destination will see as available
> > -         * all the entries that the device did not use, including the in-flight
> > -         * processing ones.
> > -         *
> > -         * TODO: This is ok for networking, but other kinds of devices might
> > -         * have problems with these retransmissions.
> > -         */
> > -        while (virtqueue_rewind(vq, 1)) {
> > -            continue;
> > -        }
>
> I think it's not a good idea to partially revert the code that was
> just introduced in the previous patch (unless it can be backported to
> -stable independently).
>
> We probably need to squash the changes.
>

You definitely have a point. Maybe it's better to remove the "Fixes"
tag? -stable version cannot enable SVQ anyway (it doesn't have the
parameter).

I'll send a v2 when we have all the comments sorted out. Maybe a
better alternative is to delay the x-svq parameter to the top of both
series if both of them go in?

Thanks!

> Thanks
>
> >          ring->num = virtio_queue_get_last_avail_idx(dev->vdev, ring->index);
> >          return 0;
> >      }
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH 03/12] vdpa: Small rename of error labels

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 7:50 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > So later patches are cleaner
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 4458c8d23e..906c365036 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1039,7 +1039,7 @@ static bool vhost_vdpa_svqs_start(struct vhost_dev *dev)
> >          int r;
> >          bool ok = vhost_vdpa_svq_setup(dev, svq, i, &err);
> >          if (unlikely(!ok)) {
> > -            goto err;
> > +            goto err_svq_setup;
> >          }
> >
> >          vhost_svq_start(svq, dev->vdev, vq);
> > @@ -1064,8 +1064,7 @@ err_set_addr:
> >  err_map:
> >      vhost_svq_stop(g_ptr_array_index(v->shadow_vqs, i));
> >
> > -err:
> > -    error_reportf_err(err, "Cannot setup SVQ %u: ", i);
>
> This does not look like a simple rename.
>

That's right.

I'll drop his patch from the series, it's not really useful and only adds noise.

Thanks!

> Thanks
>
> > +err_svq_setup:
> >      for (unsigned j = 0; j < i; ++j) {
> >          VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, j);
> >          vhost_vdpa_svq_unmap_rings(dev, svq);
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH 07/12] vhost: Add VhostVDPAStartOp operation

[Jason Wang]

On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> It allows to run commands at start of the device, before it have enabled
> any queue.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.h | 3 +++
>  include/hw/virtio/vhost-vdpa.h     | 5 +++++
>  hw/virtio/vhost-vdpa.c             | 8 ++++++++
>  3 files changed, 16 insertions(+)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index 03eb7ff670..210fe393cd 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -26,6 +26,8 @@ typedef struct SVQDescState {
>  } SVQDescState;
>
>  typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> +typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
> +                                    void *opaque);
>
>  /**
>   * Callback to handle an avail buffer.
> @@ -43,6 +45,7 @@ typedef int (*VirtQueueAvailCallback)(VhostShadowVirtqueue *svq,
>                                        void *vq_callback_opaque);
>
>  typedef struct VhostShadowVirtqueueOps {
> +    ShadowVirtQueueStart start;

What's the difference between this and start_op?

Thanks

>      VirtQueueAvailCallback avail_handler;
>  } VhostShadowVirtqueueOps;
>
> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> index d10a89303e..b7d18b4e30 100644
> --- a/include/hw/virtio/vhost-vdpa.h
> +++ b/include/hw/virtio/vhost-vdpa.h
> @@ -24,6 +24,10 @@ typedef struct VhostVDPAHostNotifier {
>      void *addr;
>  } VhostVDPAHostNotifier;
>
> +struct vhost_vdpa;
> +/* Called after send DRIVER_OK but after enabling vrings */
> +typedef int (*VhostVDPAStartOp)(struct vhost_vdpa *v);
> +
>  typedef struct vhost_vdpa {
>      int device_fd;
>      int index;
> @@ -39,6 +43,7 @@ typedef struct vhost_vdpa {
>      GPtrArray *shadow_vqs;
>      const VhostShadowVirtqueueOps *shadow_vq_ops;
>      void *shadow_vq_ops_opaque;
> +    VhostVDPAStartOp start_op;
>      struct vhost_dev *dev;
>      VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
>  } VhostVDPA;
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 1d8829c619..48f031b8c0 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -1136,6 +1136,14 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>          if (unlikely(r)) {
>              return r;
>          }
> +
> +        if (v->start_op) {
> +            r = v->start_op(v);
> +            if (unlikely(r)) {
> +                return r;
> +            }
> +        }
> +
>          vhost_vdpa_set_vring_ready(dev);
>      } else {
>          vhost_vdpa_reset_device(dev);
> --
> 2.31.1
>

Re: [RFC PATCH 09/12] vdpa: Extract vhost_vdpa_net_svq_add from vhost_vdpa_net_handle_ctrl_avail

[Jason Wang]

在 2022/7/16 19:34, Eugenio Pérez 写道:
> So we can reuse to inject state messages.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   net/vhost-vdpa.c | 89 +++++++++++++++++++++++++++---------------------
>   1 file changed, 51 insertions(+), 38 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 211bd0468b..aaae51a778 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -334,6 +334,54 @@ static bool vhost_vdpa_net_cvq_map_elem(VhostVDPAState *s,
>       return true;
>   }
>   
> +static virtio_net_ctrl_ack vhost_vdpa_net_svq_add(VhostShadowVirtqueue *svq,
> +                                               const struct iovec *dev_buffers)


The name should be tweaked since it is used only for cvq.


> +{
> +    /* in buffer used for device model */
> +    virtio_net_ctrl_ack status;
> +    const struct iovec in = {
> +        .iov_base = &status,
> +        .iov_len = sizeof(status),
> +    };
> +    size_t dev_written;
> +    int r;
> +    void *unused = (void *)1;
> +
> +    r = vhost_svq_add(svq, &dev_buffers[0], 1, &dev_buffers[1], 1, unused);
> +    if (unlikely(r != 0)) {
> +        if (unlikely(r == -ENOSPC)) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device queue\n",
> +                          __func__);
> +        }
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    /*
> +     * We can poll here since we've had BQL from the time we sent the
> +     * descriptor. Also, we need to take the answer before SVQ pulls by itself,
> +     * when BQL is released
> +     */


This reminds me that, do we need a upper limit of the time on the 
polling here. (Avoid taking BQL for too long time).

Thanks


> +    dev_written = vhost_svq_poll(svq);
> +    if (unlikely(dev_written < sizeof(status))) {
> +        error_report("Insufficient written data (%zu)", dev_written);
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    memcpy(&status, dev_buffers[1].iov_base, sizeof(status));
> +    if (status != VIRTIO_NET_OK) {
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    status = VIRTIO_NET_ERR;
> +    virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, dev_buffers, 1);
> +    if (status != VIRTIO_NET_OK) {
> +        error_report("Bad CVQ processing in model");
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    return VIRTIO_NET_OK;
> +}
> +
>   static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
>   {
>       struct vhost_vring_state state = {
> @@ -392,19 +440,13 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>                                               void *opaque)
>   {
>       VhostVDPAState *s = opaque;
> -    size_t in_len, dev_written;
> +    size_t in_len;
>       virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>       /* out and in buffers sent to the device */
>       struct iovec dev_buffers[2] = {
>           { .iov_base = s->cvq_cmd_out_buffer },
>           { .iov_base = s->cvq_cmd_in_buffer },
>       };
> -    /* in buffer used for device model */
> -    const struct iovec in = {
> -        .iov_base = &status,
> -        .iov_len = sizeof(status),
> -    };
> -    int r;
>       bool ok;
>   
>       ok = vhost_vdpa_net_cvq_map_elem(s, elem, dev_buffers);
> @@ -417,36 +459,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>           goto out;
>       }
>   
> -    r = vhost_svq_add(svq, &dev_buffers[0], 1, &dev_buffers[1], 1, elem);
> -    if (unlikely(r != 0)) {
> -        if (unlikely(r == -ENOSPC)) {
> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device queue\n",
> -                          __func__);
> -        }
> -        goto out;
> -    }
> -
> -    /*
> -     * We can poll here since we've had BQL from the time we sent the
> -     * descriptor. Also, we need to take the answer before SVQ pulls by itself,
> -     * when BQL is released
> -     */
> -    dev_written = vhost_svq_poll(svq);
> -    if (unlikely(dev_written < sizeof(status))) {
> -        error_report("Insufficient written data (%zu)", dev_written);
> -        goto out;
> -    }
> -
> -    memcpy(&status, dev_buffers[1].iov_base, sizeof(status));
> -    if (status != VIRTIO_NET_OK) {
> -        goto out;
> -    }
> -
> -    status = VIRTIO_NET_ERR;
> -    virtio_net_handle_ctrl_iov(svq->vdev, &in, 1, dev_buffers, 1);
> -    if (status != VIRTIO_NET_OK) {
> -        error_report("Bad CVQ processing in model");
> -    }
> +    status = vhost_vdpa_net_svq_add(svq, dev_buffers);
>   
>   out:
>       in_len = iov_from_buf(elem->in_sg, elem->in_num, 0, &status,
> @@ -462,7 +475,7 @@ out:
>       if (dev_buffers[1].iov_base) {
>           vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, dev_buffers[1].iov_base);
>       }
> -    return r;
> +    return status == VIRTIO_NET_OK ? 0 : 1;
>   }
>   
>   static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {

Re: [RFC PATCH 11/12] vdpa: Add virtio-net mac address via CVQ at start

[Jason Wang]

在 2022/7/16 19:34, Eugenio Pérez 写道:
> This is needed so the destination vdpa device see the same state a the
> guest set in the source.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   net/vhost-vdpa.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 0183fce353..2873be2ba4 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -383,7 +383,7 @@ static virtio_net_ctrl_ack vhost_vdpa_net_svq_add(VhostShadowVirtqueue *svq,
>       return VIRTIO_NET_OK;
>   }
>   
> -static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
> +static int vhost_vdpa_enable_control_svq(struct vhost_vdpa *v)
>   {
>       struct vhost_vring_state state = {
>           .index = v->dev->vq_index,
> @@ -395,6 +395,57 @@ static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
>       return r < 0 ? -errno : r;
>   }
>   
> +static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
> +{
> +
> +    VirtIONet *n = VIRTIO_NET(v->dev->vdev);
> +    uint64_t features = v->dev->vdev->host_features;
> +    VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, 0);
> +    VhostVDPAState *s = container_of(v, VhostVDPAState, vhost_vdpa);
> +    int r;
> +
> +    r = vhost_vdpa_enable_control_svq(v);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
> +
> +    if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> +        const struct virtio_net_ctrl_hdr ctrl = {
> +            .class = VIRTIO_NET_CTRL_MAC,
> +            .cmd = VIRTIO_NET_CTRL_MAC_ADDR_SET,
> +        };
> +        uint8_t mac[6];
> +        const struct iovec out[] = {
> +            {
> +                .iov_base = (void *)&ctrl,
> +                .iov_len = sizeof(ctrl),
> +            },{
> +                .iov_base = mac,
> +                .iov_len = sizeof(mac),
> +            },
> +        };
> +        struct iovec dev_buffers[2] = {
> +            { .iov_base = s->cvq_cmd_out_buffer },
> +            { .iov_base = s->cvq_cmd_in_buffer },
> +        };
> +        bool ok;
> +        virtio_net_ctrl_ack state;
> +
> +        ok = vhost_vdpa_net_cvq_map_sg(s, out, ARRAY_SIZE(out), dev_buffers);
> +        if (unlikely(!ok)) {
> +            return -1;
> +        }
> +
> +        memcpy(mac, n->mac, sizeof(mac));
> +        state = vhost_vdpa_net_svq_add(svq, dev_buffers);
> +        vhost_vdpa_cvq_unmap_buf(v, dev_buffers[0].iov_base);
> +        vhost_vdpa_cvq_unmap_buf(v, dev_buffers[1].iov_base);


Any reason we do per buffer unmap instead of the sg unmap here?

Thanks


> +        return state == VIRTIO_NET_OK ? 0 : 1;
> +    }
> +
> +    return 0;
> +}
> +
>   /**
>    * Do not forward commands not supported by SVQ. Otherwise, the device could
>    * accept it and qemu would not know how to update the device model.

Re: [RFC PATCH 11/12] vdpa: Add virtio-net mac address via CVQ at start

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 10:55 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/7/16 19:34, Eugenio Pérez 写道:
> > This is needed so the destination vdpa device see the same state a the
> > guest set in the source.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   net/vhost-vdpa.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 0183fce353..2873be2ba4 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -383,7 +383,7 @@ static virtio_net_ctrl_ack vhost_vdpa_net_svq_add(VhostShadowVirtqueue *svq,
> >       return VIRTIO_NET_OK;
> >   }
> >
> > -static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
> > +static int vhost_vdpa_enable_control_svq(struct vhost_vdpa *v)
> >   {
> >       struct vhost_vring_state state = {
> >           .index = v->dev->vq_index,
> > @@ -395,6 +395,57 @@ static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
> >       return r < 0 ? -errno : r;
> >   }
> >
> > +static int vhost_vdpa_start_control_svq(struct vhost_vdpa *v)
> > +{
> > +
> > +    VirtIONet *n = VIRTIO_NET(v->dev->vdev);
> > +    uint64_t features = v->dev->vdev->host_features;
> > +    VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, 0);
> > +    VhostVDPAState *s = container_of(v, VhostVDPAState, vhost_vdpa);
> > +    int r;
> > +
> > +    r = vhost_vdpa_enable_control_svq(v);
> > +    if (unlikely(r < 0)) {
> > +        return r;
> > +    }
> > +
> > +    if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> > +        const struct virtio_net_ctrl_hdr ctrl = {
> > +            .class = VIRTIO_NET_CTRL_MAC,
> > +            .cmd = VIRTIO_NET_CTRL_MAC_ADDR_SET,
> > +        };
> > +        uint8_t mac[6];
> > +        const struct iovec out[] = {
> > +            {
> > +                .iov_base = (void *)&ctrl,
> > +                .iov_len = sizeof(ctrl),
> > +            },{
> > +                .iov_base = mac,
> > +                .iov_len = sizeof(mac),
> > +            },
> > +        };
> > +        struct iovec dev_buffers[2] = {
> > +            { .iov_base = s->cvq_cmd_out_buffer },
> > +            { .iov_base = s->cvq_cmd_in_buffer },
> > +        };
> > +        bool ok;
> > +        virtio_net_ctrl_ack state;
> > +
> > +        ok = vhost_vdpa_net_cvq_map_sg(s, out, ARRAY_SIZE(out), dev_buffers);
> > +        if (unlikely(!ok)) {
> > +            return -1;
> > +        }
> > +
> > +        memcpy(mac, n->mac, sizeof(mac));
> > +        state = vhost_vdpa_net_svq_add(svq, dev_buffers);
> > +        vhost_vdpa_cvq_unmap_buf(v, dev_buffers[0].iov_base);
> > +        vhost_vdpa_cvq_unmap_buf(v, dev_buffers[1].iov_base);
>
>
> Any reason we do per buffer unmap instead of the sg unmap here?
>

I think I don't get this comment.

vhost_vdpa_net_handle_ctrl_avail also unmap each buffer individually,
and I need a function to unmap one of them at a time. I could create a
function to unmap a whole sg, but I'm not sure how much value it adds.

Thanks!

> Thanks
>
>
> > +        return state == VIRTIO_NET_OK ? 0 : 1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   /**
> >    * Do not forward commands not supported by SVQ. Otherwise, the device could
> >    * accept it and qemu would not know how to update the device model.
>

Re: [RFC PATCH 07/12] vhost: Add VhostVDPAStartOp operation

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 10:50 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > It allows to run commands at start of the device, before it have enabled
> > any queue.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-shadow-virtqueue.h | 3 +++
> >  include/hw/virtio/vhost-vdpa.h     | 5 +++++
> >  hw/virtio/vhost-vdpa.c             | 8 ++++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> > index 03eb7ff670..210fe393cd 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -26,6 +26,8 @@ typedef struct SVQDescState {
> >  } SVQDescState;
> >
> >  typedef struct VhostShadowVirtqueue VhostShadowVirtqueue;
> > +typedef int (*ShadowVirtQueueStart)(VhostShadowVirtqueue *svq,
> > +                                    void *opaque);
> >
> >  /**
> >   * Callback to handle an avail buffer.
> > @@ -43,6 +45,7 @@ typedef int (*VirtQueueAvailCallback)(VhostShadowVirtqueue *svq,
> >                                        void *vq_callback_opaque);
> >
> >  typedef struct VhostShadowVirtqueueOps {
> > +    ShadowVirtQueueStart start;
>
> What's the difference between this and start_op?
>

This is a leftover, I'll delete for the next series.

Thanks!

> Thanks
>
> >      VirtQueueAvailCallback avail_handler;
> >  } VhostShadowVirtqueueOps;
> >
> > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > index d10a89303e..b7d18b4e30 100644
> > --- a/include/hw/virtio/vhost-vdpa.h
> > +++ b/include/hw/virtio/vhost-vdpa.h
> > @@ -24,6 +24,10 @@ typedef struct VhostVDPAHostNotifier {
> >      void *addr;
> >  } VhostVDPAHostNotifier;
> >
> > +struct vhost_vdpa;
> > +/* Called after send DRIVER_OK but after enabling vrings */
> > +typedef int (*VhostVDPAStartOp)(struct vhost_vdpa *v);
> > +
> >  typedef struct vhost_vdpa {
> >      int device_fd;
> >      int index;
> > @@ -39,6 +43,7 @@ typedef struct vhost_vdpa {
> >      GPtrArray *shadow_vqs;
> >      const VhostShadowVirtqueueOps *shadow_vq_ops;
> >      void *shadow_vq_ops_opaque;
> > +    VhostVDPAStartOp start_op;
> >      struct vhost_dev *dev;
> >      VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX];
> >  } VhostVDPA;
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 1d8829c619..48f031b8c0 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -1136,6 +1136,14 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >          if (unlikely(r)) {
> >              return r;
> >          }
> > +
> > +        if (v->start_op) {
> > +            r = v->start_op(v);
> > +            if (unlikely(r)) {
> > +                return r;
> > +            }
> > +        }
> > +
> >          vhost_vdpa_set_vring_ready(dev);
> >      } else {
> >          vhost_vdpa_reset_device(dev);
> > --
> > 2.31.1
> >
>

Re: [RFC PATCH 09/12] vdpa: Extract vhost_vdpa_net_svq_add from vhost_vdpa_net_handle_ctrl_avail

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 10:53 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/7/16 19:34, Eugenio Pérez 写道:
> > So we can reuse to inject state messages.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   net/vhost-vdpa.c | 89 +++++++++++++++++++++++++++---------------------
> >   1 file changed, 51 insertions(+), 38 deletions(-)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 211bd0468b..aaae51a778 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -334,6 +334,54 @@ static bool vhost_vdpa_net_cvq_map_elem(VhostVDPAState *s,
> >       return true;
> >   }
> >
> > +static virtio_net_ctrl_ack vhost_vdpa_net_svq_add(VhostShadowVirtqueue *svq,
> > +                                               const struct iovec *dev_buffers)
>
>
> The name should be tweaked since it is used only for cvq.
>

Right, I'll change.

>
> > +{
> > +    /* in buffer used for device model */
> > +    virtio_net_ctrl_ack status;
> > +    const struct iovec in = {
> > +        .iov_base = &status,
> > +        .iov_len = sizeof(status),
> > +    };
> > +    size_t dev_written;
> > +    int r;
> > +    void *unused = (void *)1;
> > +
> > +    r = vhost_svq_add(svq, &dev_buffers[0], 1, &dev_buffers[1], 1, unused);
> > +    if (unlikely(r != 0)) {
> > +        if (unlikely(r == -ENOSPC)) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: No space on device queue\n",
> > +                          __func__);
> > +        }
> > +        return VIRTIO_NET_ERR;
> > +    }
> > +
> > +    /*
> > +     * We can poll here since we've had BQL from the time we sent the
> > +     * descriptor. Also, we need to take the answer before SVQ pulls by itself,
> > +     * when BQL is released
> > +     */
>
>
> This reminds me that, do we need a upper limit of the time on the
> polling here. (Avoid taking BQL for too long time).
>

Sending a new version of rx filters here.

But we have other parts where we can have BQL forever because we trust
the device, like vring enable syscalls for example.

Thanks!