[RFC 0/8] Dinamycally switch to vhost shadow virtqueues at vdpa net migration

[RFC 0/8] Dinamycally switch to vhost shadow virtqueues at vdpa net migration

[Eugenio Pérez]

It's possible to migrate vdpa net devices if they are shadowed from the
start. But to always shadow the dataplane is effectively break its host
passthrough, so its not convenient in vDPA scenarios.

This series enables dynamically switching to shadow mode only at migration
time. This allow full data virtqueues passthrough all the time qemu is not
migrating.

To do so it uses the VHOST_VDPA_SUSPEND ioctl, not merged in Linux at this time
[1]. Because of that, first patch is not signed and present a header
[NOTMERGE]. This series is also based on ASID one [2], not merged in qemu at the
moment.

Comments are welcome.

[1] https://lkml.org/lkml/2022/8/10/732
[2] https://lists.nongnu.org/archive/html/qemu-devel/2022-08/msg01047.html

Eugenio Pérez (8):
  [NOTMERGE] Update linux headers
  vdpa: Extract get_backend_features from vhost_vdpa_get_as_num
  vhost: expose memory listener priority
  vdpa: Add log_enabled to VhostVDPAState
  vdpa: Add vdpa memory listener
  vdpa: Negotiate _F_SUSPEND feature
  vdpa: Add feature_log member to vhost_vdpa
  vdpa: Conditionally expose _F_LOG in vhost_net devices

 include/hw/virtio/vhost-vdpa.h               |   1 +
 include/hw/virtio/vhost.h                    |   2 +
 include/standard-headers/linux/vhost_types.h |   3 +
 linux-headers/linux/vhost.h                  |   3 +
 hw/virtio/vhost-vdpa.c                       |   5 +-
 hw/virtio/vhost.c                            |   2 +-
 net/vhost-vdpa.c                             | 128 +++++++++++++++++--
 7 files changed, 132 insertions(+), 12 deletions(-)

-- 
2.31.1

[RFC 1/8] [NOTMERGE] Update linux headers

[Eugenio Pérez]

Add _F_SUSPEND and suspend ioctl.

TODO: This is still not merged in Linux upstream, so it may change.
---
 include/standard-headers/linux/vhost_types.h | 3 +++
 linux-headers/linux/vhost.h                  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/standard-headers/linux/vhost_types.h b/include/standard-headers/linux/vhost_types.h
index ce78551b0f..c93ed1b920 100644
--- a/include/standard-headers/linux/vhost_types.h
+++ b/include/standard-headers/linux/vhost_types.h
@@ -161,5 +161,8 @@ struct vhost_vdpa_iova_range {
  * message
  */
 #define VHOST_BACKEND_F_IOTLB_ASID  0x3
+/* Stop device from processing virtqueue buffers */
+#define VHOST_BACKEND_F_SUSPEND  0x4
+
 
 #endif
diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
index cab645d4a6..f3f5bea3cb 100644
--- a/linux-headers/linux/vhost.h
+++ b/linux-headers/linux/vhost.h
@@ -171,4 +171,7 @@
 #define VHOST_VDPA_SET_GROUP_ASID	_IOW(VHOST_VIRTIO, 0x7C, \
 					     struct vhost_vring_state)
 
+/* Stop or resume a device so it does not process virtqueue requests anymore */
+#define VHOST_VDPA_SUSPEND              _IO(VHOST_VIRTIO, 0x7D)
+
 #endif
-- 
2.31.1

[RFC 2/8] vdpa: Extract get_backend_features from vhost_vdpa_get_as_num

[Eugenio Pérez]

The series reuses it to check for SUSPEND feature bit.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 11241ebac4..85b10799bd 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -602,9 +602,17 @@ static const VhostShadowVirtqueueOps vhost_vdpa_net_svq_ops = {
     .avail_handler = vhost_vdpa_net_handle_ctrl_avail,
 };
 
-static uint32_t vhost_vdpa_get_as_num(int vdpa_device_fd)
+static uint64_t vhost_vdpa_get_backend_features(int fd)
 {
     uint64_t features;
+
+    /* No need to treat the error, only to know there is one */
+    int ret = ioctl(fd, VHOST_GET_BACKEND_FEATURES, &features);
+    return ret < 0 ? 0 : features;
+}
+
+static uint32_t vhost_vdpa_get_as_num(int vdpa_device_fd, uint64_t features)
+{
     unsigned num_as;
     int r;
 
@@ -733,7 +741,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                         NetClientState *peer, Error **errp)
 {
     const NetdevVhostVDPAOptions *opts;
-    uint64_t features;
+    uint64_t features, backend_features;
     int vdpa_device_fd;
     g_autofree NetClientState **ncs = NULL;
     g_autoptr(VhostIOVATree) iova_tree = NULL;
@@ -765,9 +773,10 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
         goto err;
     }
 
+    backend_features = vhost_vdpa_get_backend_features(vdpa_device_fd);
     svq_cvq = opts->x_svq;
     if (has_cvq && !opts->x_svq) {
-        num_as = vhost_vdpa_get_as_num(vdpa_device_fd);
+        num_as = vhost_vdpa_get_as_num(vdpa_device_fd, backend_features);
         svq_cvq = num_as > 1;
     }
 
-- 
2.31.1

[RFC 3/8] vhost: expose memory listener priority

[Eugenio Pérez]

We need to perform changes to vhost_vdpa devices before the memory
listener inform them about the migration. Otherwise, it will reach them
with no SVQ enabled and it cannot be guaranteed that it will be enabled
afterwards.

Expose the vhost memory listener priority so we can assign a lower one
to net/vhost-vdpa one.

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

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a346f23d13..ccd6cc5549 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -104,6 +104,8 @@ struct vhost_dev {
     const VhostDevConfigOps *config_ops;
 };
 
+#define VHOST_DEV_MEMORY_LISTENER_PRIORITY 10
+
 extern const VhostOps kernel_ops;
 extern const VhostOps user_ops;
 extern const VhostOps vdpa_ops;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0827d631c0..a1e822b871 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1411,7 +1411,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         .log_global_stop = vhost_log_global_stop,
         .eventfd_add = vhost_eventfd_add,
         .eventfd_del = vhost_eventfd_del,
-        .priority = 10
+        .priority = VHOST_DEV_MEMORY_LISTENER_PRIORITY
     };
 
     hdev->iommu_listener = (MemoryListener) {
-- 
2.31.1

[RFC 4/8] vdpa: Add log_enabled to VhostVDPAState

[Eugenio Pérez]

This enables VhostVDPAState to track the logging of the memory.

It cannot be merged with s->always_svq because always_svq is immutable
from the moment the device is parsed, and log_enabled must be enabled or
disabled depending on the log state.

Apart from that, they will affect the same to vhost vdpa device,
enabling the shadow virtqueue unconditionally.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 85b10799bd..a035c89c34 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -42,6 +42,10 @@ typedef struct VhostVDPAState {
 
     /* The device always have SVQ enabled */
     bool always_svq;
+
+    /* Device log enabled */
+    bool log_enabled;
+
     bool started;
 } VhostVDPAState;
 
@@ -346,15 +350,15 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
     cvq_group.index = v->dev->vq_index_end - 1;
 
     /* Default values */
-    v->listener_shadow_vq = s->always_svq;
-    v->shadow_vqs_enabled = s->always_svq;
+    v->listener_shadow_vq = s->always_svq || s->log_enabled;
+    v->shadow_vqs_enabled = s->always_svq || s->log_enabled;
     s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_PASSTHROUGH;
 
     if (s->address_space_num < 2) {
         return 0;
     }
 
-    if (s->always_svq) {
+    if (s->always_svq || s->log_enabled) {
         goto out;
     }
 
-- 
2.31.1

[RFC 5/8] vdpa: Add vdpa memory listener

[Eugenio Pérez]

This enable net/vdpa to restart the full device when a migration is
started or stopped.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index a035c89c34..4c6947feb8 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -21,6 +21,7 @@
 #include "qemu/memalign.h"
 #include "qemu/option.h"
 #include "qapi/error.h"
+#include "exec/address-spaces.h"
 #include <linux/vhost.h>
 #include <sys/ioctl.h>
 #include <err.h>
@@ -32,6 +33,8 @@
 typedef struct VhostVDPAState {
     NetClientState nc;
     struct vhost_vdpa vhost_vdpa;
+    MemoryListener memory_listener;
+
     VHostNetState *vhost_net;
 
     /* Control commands shadow buffers */
@@ -110,6 +113,16 @@ static const uint64_t vdpa_svq_device_features =
 #define VHOST_VDPA_NET_CVQ_PASSTHROUGH 0
 #define VHOST_VDPA_NET_CVQ_ASID 1
 
+/*
+ * Vdpa memory listener must run before vhost one, so vhost_vdpa does not get
+ * _F_LOG_ALL without SVQ.
+ */
+#define VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY \
+                                       (VHOST_DEV_MEMORY_LISTENER_PRIORITY - 1)
+/* Check for underflow */
+QEMU_BUILD_BUG_ON(VHOST_DEV_MEMORY_LISTENER_PRIORITY <
+                  VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY);
+
 VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -172,6 +185,9 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
 
     qemu_vfree(s->cvq_cmd_out_buffer);
     qemu_vfree(s->cvq_cmd_in_buffer);
+    if (dev->vq_index == 0) {
+        memory_listener_unregister(&s->memory_listener);
+    }
     if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
         g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
     }
@@ -224,6 +240,69 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc, const uint8_t *buf,
     return 0;
 }
 
+static void vhost_vdpa_net_log_global_enable(MemoryListener *listener,
+                                             bool enable)
+{
+    VhostVDPAState *s = container_of(listener, VhostVDPAState,
+                                     memory_listener);
+    struct vhost_vdpa *v = &s->vhost_vdpa;
+    VirtIONet *n;
+    VirtIODevice *vdev;
+    int data_queue_pairs, cvq, r;
+    NetClientState *peer;
+
+    if (s->always_svq || s->log_enabled == enable) {
+        return;
+    }
+
+    s->log_enabled = enable;
+    vdev = v->dev->vdev;
+    n = VIRTIO_NET(vdev);
+    if (!n->vhost_started) {
+        return;
+    }
+
+    if (enable) {
+        ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
+    }
+    data_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
+    cvq = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) ?
+                                  n->max_ncs - n->max_queue_pairs : 0;
+    vhost_net_stop(vdev, n->nic->ncs, data_queue_pairs, cvq);
+
+    peer = s->nc.peer;
+    for (int i = 0; i < data_queue_pairs + cvq; i++) {
+        VhostVDPAState *vdpa_state;
+        NetClientState *nc;
+
+        if (i < data_queue_pairs) {
+            nc = qemu_get_peer(peer, i);
+        } else {
+            nc = qemu_get_peer(peer, n->max_queue_pairs);
+        }
+
+        vdpa_state = DO_UPCAST(VhostVDPAState, nc, nc);
+        vdpa_state->vhost_vdpa.listener_shadow_vq = enable;
+        vdpa_state->vhost_vdpa.shadow_vqs_enabled = enable;
+        vdpa_state->log_enabled = enable;
+    }
+
+    r = vhost_net_start(vdev, n->nic->ncs, data_queue_pairs, cvq);
+    if (unlikely(r < 0)) {
+        error_report("unable to start vhost net: %s(%d)", g_strerror(-r), -r);
+    }
+}
+
+static void vhost_vdpa_net_log_global_start(MemoryListener *listener)
+{
+    vhost_vdpa_net_log_global_enable(listener, true);
+}
+
+static void vhost_vdpa_net_log_global_stop(MemoryListener *listener)
+{
+    vhost_vdpa_net_log_global_enable(listener, false);
+}
+
 static NetClientInfo net_vhost_vdpa_info = {
         .type = NET_CLIENT_DRIVER_VHOST_VDPA,
         .size = sizeof(VhostVDPAState),
@@ -413,6 +492,7 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
 
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
 
+    memory_listener_unregister(&s->memory_listener);
     if (s->vhost_vdpa.shadow_vqs_enabled) {
         vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
         vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_in_buffer);
@@ -671,6 +751,13 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     s->vhost_vdpa.shadow_vqs_enabled = svq;
     s->vhost_vdpa.listener_shadow_vq = svq;
     s->vhost_vdpa.iova_tree = iova_tree;
+    if (queue_pair_index == 0) {
+        s->memory_listener = (MemoryListener) {
+            .log_global_start = vhost_vdpa_net_log_global_start,
+            .log_global_stop = vhost_vdpa_net_log_global_stop,
+        };
+        memory_listener_register(&s->memory_listener, &address_space_memory);
+    }
     if (!is_datapath) {
         s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
                                             vhost_vdpa_net_cvq_cmd_page_len());
-- 
2.31.1

[RFC 6/8] vdpa: Negotiate _F_SUSPEND feature

[Eugenio Pérez]

This is needed for qemu to know it can suspend the device to retrieve
its status and enable SVQ with it, so all the process is transparent to
the guest.

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3607983422..d750d9cec1 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -679,7 +679,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
     uint64_t features;
     uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
         0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
-        0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
+        0x1ULL << VHOST_BACKEND_F_IOTLB_ASID |
+        0x1ULL << VHOST_BACKEND_F_SUSPEND;
     int r;
 
     if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
-- 
2.31.1

[RFC 7/8] vdpa: Add feature_log member to vhost_vdpa

[Eugenio Pérez]

This way device's vhost_vdpa can make the choice about exposing or not
the _F_LOG feature.

At the moment is always false.

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

diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
index 0c3ed2d69b..b09eae133a 100644
--- a/include/hw/virtio/vhost-vdpa.h
+++ b/include/hw/virtio/vhost-vdpa.h
@@ -33,6 +33,7 @@ typedef struct vhost_vdpa {
     MemoryListener listener;
     struct vhost_vdpa_iova_range iova_range;
     uint64_t acked_features;
+    bool feature_log;
     bool shadow_vqs_enabled;
     /* The listener must send iova tree addresses, not GPA */
     bool listener_shadow_vq;
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index d750d9cec1..17513ee820 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1246,7 +1246,7 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
     struct vhost_vdpa *v = dev->opaque;
     int ret = vhost_vdpa_get_dev_features(dev, features);
 
-    if (ret == 0 && v->shadow_vqs_enabled) {
+    if (ret == 0 && (v->shadow_vqs_enabled || v->feature_log)) {
         /* Add SVQ logging capabilities */
         *features |= BIT_ULL(VHOST_F_LOG_ALL);
     }
-- 
2.31.1

[RFC 8/8] vdpa: Conditionally expose _F_LOG in vhost_net devices

[Eugenio Pérez]

Vhost-vdpa networking devices need to met a few conditions to be
migratable. If SVQ is not enabled from the beginnig, to suspend the
device to retrieve the vq state is the first requirement.

However, qemu also needs to be able to intercept SVQ from the beginning.
To be able to do so, the vdpa device needs to expose certains features.

Expose _F_LOG only if all of these are met.

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4c6947feb8..73c27cd315 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -49,6 +49,9 @@ typedef struct VhostVDPAState {
     /* Device log enabled */
     bool log_enabled;
 
+    /* Device can suspend */
+    bool feature_suspend;
+
     bool started;
 } VhostVDPAState;
 
@@ -431,6 +434,7 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
     /* Default values */
     v->listener_shadow_vq = s->always_svq || s->log_enabled;
     v->shadow_vqs_enabled = s->always_svq || s->log_enabled;
+    v->feature_log = s->always_svq || s->log_enabled;
     s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_PASSTHROUGH;
 
     if (s->address_space_num < 2) {
@@ -455,6 +459,7 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
         if (unlikely(vq_group.num == cvq_group.num)) {
             warn_report("CVQ %u group is the same as VQ %u one (%u)",
                          cvq_group.index, vq_group.index, cvq_group.num);
+            v->feature_log = false;
             return 0;
         }
     }
@@ -464,6 +469,7 @@ static int vhost_vdpa_net_cvq_start(NetClientState *nc)
     if (r == 0) {
         v->shadow_vqs_enabled = true;
         s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
+        v->feature_log = s->feature_suspend;
     }
 
 out:
@@ -728,6 +734,7 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
                                            unsigned nas,
                                            bool is_datapath,
                                            bool svq,
+                                           bool feature_suspend,
                                            VhostIOVATree *iova_tree)
 {
     NetClientState *nc = NULL;
@@ -748,9 +755,11 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
     s->vhost_vdpa.device_fd = vdpa_device_fd;
     s->vhost_vdpa.index = queue_pair_index;
     s->always_svq = svq;
+    s->feature_suspend = feature_suspend;
     s->vhost_vdpa.shadow_vqs_enabled = svq;
     s->vhost_vdpa.listener_shadow_vq = svq;
     s->vhost_vdpa.iova_tree = iova_tree;
+    s->vhost_vdpa.feature_log = feature_suspend;
     if (queue_pair_index == 0) {
         s->memory_listener = (MemoryListener) {
             .log_global_start = vhost_vdpa_net_log_global_start,
@@ -839,7 +848,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     NetClientState *nc;
     int queue_pairs, r, i = 0, has_cvq = 0;
     unsigned num_as = 1;
-    bool svq_cvq;
+    bool svq_cvq, feature_suspend;
 
     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
     opts = &netdev->u.vhost_vdpa;
@@ -892,10 +901,11 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
 
     ncs = g_malloc0(sizeof(*ncs) * queue_pairs);
 
+    feature_suspend = backend_features & BIT_ULL(VHOST_BACKEND_F_SUSPEND);
     for (i = 0; i < queue_pairs; i++) {
         ncs[i] = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                      vdpa_device_fd, i, 2, num_as, true,
-                                     opts->x_svq, iova_tree);
+                                     opts->x_svq, feature_suspend, iova_tree);
         if (!ncs[i])
             goto err;
     }
@@ -903,7 +913,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     if (has_cvq) {
         nc = net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name,
                                  vdpa_device_fd, i, 1, num_as, false,
-                                 opts->x_svq, iova_tree);
+                                 opts->x_svq, feature_suspend, iova_tree);
         if (!nc)
             goto err;
     }
-- 
2.31.1

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Jason Wang]

On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> This enable net/vdpa to restart the full device when a migration is
> started or stopped.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

I have the following questions

1) any reason that we need to make this net specific? The dirty page
tracking via shadow virtqueue is pretty general. And the net specific
part was done via NetClientInfo anyhow.
2) any reason we can't re-use the vhost-vdpa listener?

(Anyway, it's better to explain the reason in detail in the changelog.)

Thanks

> ---
>  net/vhost-vdpa.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index a035c89c34..4c6947feb8 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -21,6 +21,7 @@
>  #include "qemu/memalign.h"
>  #include "qemu/option.h"
>  #include "qapi/error.h"
> +#include "exec/address-spaces.h"
>  #include <linux/vhost.h>
>  #include <sys/ioctl.h>
>  #include <err.h>
> @@ -32,6 +33,8 @@
>  typedef struct VhostVDPAState {
>      NetClientState nc;
>      struct vhost_vdpa vhost_vdpa;
> +    MemoryListener memory_listener;
> +
>      VHostNetState *vhost_net;
>
>      /* Control commands shadow buffers */
> @@ -110,6 +113,16 @@ static const uint64_t vdpa_svq_device_features =
>  #define VHOST_VDPA_NET_CVQ_PASSTHROUGH 0
>  #define VHOST_VDPA_NET_CVQ_ASID 1
>
> +/*
> + * Vdpa memory listener must run before vhost one, so vhost_vdpa does not get
> + * _F_LOG_ALL without SVQ.
> + */
> +#define VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY \
> +                                       (VHOST_DEV_MEMORY_LISTENER_PRIORITY - 1)
> +/* Check for underflow */
> +QEMU_BUILD_BUG_ON(VHOST_DEV_MEMORY_LISTENER_PRIORITY <
> +                  VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY);
> +
>  VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
>  {
>      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -172,6 +185,9 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
>
>      qemu_vfree(s->cvq_cmd_out_buffer);
>      qemu_vfree(s->cvq_cmd_in_buffer);
> +    if (dev->vq_index == 0) {
> +        memory_listener_unregister(&s->memory_listener);
> +    }
>      if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
>          g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
>      }
> @@ -224,6 +240,69 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc, const uint8_t *buf,
>      return 0;
>  }
>
> +static void vhost_vdpa_net_log_global_enable(MemoryListener *listener,
> +                                             bool enable)
> +{
> +    VhostVDPAState *s = container_of(listener, VhostVDPAState,
> +                                     memory_listener);
> +    struct vhost_vdpa *v = &s->vhost_vdpa;
> +    VirtIONet *n;
> +    VirtIODevice *vdev;
> +    int data_queue_pairs, cvq, r;
> +    NetClientState *peer;
> +
> +    if (s->always_svq || s->log_enabled == enable) {
> +        return;
> +    }
> +
> +    s->log_enabled = enable;
> +    vdev = v->dev->vdev;
> +    n = VIRTIO_NET(vdev);
> +    if (!n->vhost_started) {
> +        return;
> +    }
> +
> +    if (enable) {
> +        ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
> +    }
> +    data_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> +    cvq = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) ?
> +                                  n->max_ncs - n->max_queue_pairs : 0;
> +    vhost_net_stop(vdev, n->nic->ncs, data_queue_pairs, cvq);
> +
> +    peer = s->nc.peer;
> +    for (int i = 0; i < data_queue_pairs + cvq; i++) {
> +        VhostVDPAState *vdpa_state;
> +        NetClientState *nc;
> +
> +        if (i < data_queue_pairs) {
> +            nc = qemu_get_peer(peer, i);
> +        } else {
> +            nc = qemu_get_peer(peer, n->max_queue_pairs);
> +        }
> +
> +        vdpa_state = DO_UPCAST(VhostVDPAState, nc, nc);
> +        vdpa_state->vhost_vdpa.listener_shadow_vq = enable;
> +        vdpa_state->vhost_vdpa.shadow_vqs_enabled = enable;
> +        vdpa_state->log_enabled = enable;
> +    }
> +
> +    r = vhost_net_start(vdev, n->nic->ncs, data_queue_pairs, cvq);
> +    if (unlikely(r < 0)) {
> +        error_report("unable to start vhost net: %s(%d)", g_strerror(-r), -r);
> +    }
> +}
> +
> +static void vhost_vdpa_net_log_global_start(MemoryListener *listener)
> +{
> +    vhost_vdpa_net_log_global_enable(listener, true);
> +}
> +
> +static void vhost_vdpa_net_log_global_stop(MemoryListener *listener)
> +{
> +    vhost_vdpa_net_log_global_enable(listener, false);
> +}
> +
>  static NetClientInfo net_vhost_vdpa_info = {
>          .type = NET_CLIENT_DRIVER_VHOST_VDPA,
>          .size = sizeof(VhostVDPAState),
> @@ -413,6 +492,7 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
>
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>
> +    memory_listener_unregister(&s->memory_listener);
>      if (s->vhost_vdpa.shadow_vqs_enabled) {
>          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
>          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_in_buffer);
> @@ -671,6 +751,13 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>      s->vhost_vdpa.shadow_vqs_enabled = svq;
>      s->vhost_vdpa.listener_shadow_vq = svq;
>      s->vhost_vdpa.iova_tree = iova_tree;
> +    if (queue_pair_index == 0) {
> +        s->memory_listener = (MemoryListener) {
> +            .log_global_start = vhost_vdpa_net_log_global_start,
> +            .log_global_stop = vhost_vdpa_net_log_global_stop,
> +        };
> +        memory_listener_register(&s->memory_listener, &address_space_memory);
> +    }
>      if (!is_datapath) {
>          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
>                                              vhost_vdpa_net_cvq_cmd_page_len());
> --
> 2.31.1
>

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Eugenio Perez Martin]

On Fri, Aug 19, 2022 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > This enable net/vdpa to restart the full device when a migration is
> > started or stopped.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> I have the following questions
>
> 1) any reason that we need to make this net specific? The dirty page
> tracking via shadow virtqueue is pretty general. And the net specific
> part was done via NetClientInfo anyhow.

The listener is only used to know when migration is started / stopped,
no need for actual memory tracking. Maybe there is a better way to do
so?

It's net specific because we are restarting the whole vhost_vdpa
backend. We could do inside hw/virtio/vhost_vdpa.c (previous POCs did
that way), but it's a little more complicated in my opinion. To do it
that way, the setting of _F_LOG was detected and device were resetted
and setup there.


> 2) any reason we can't re-use the vhost-vdpa listener?
>

At this moment, all vhost_vdpa backend is restarted. That implies that
the listener will be unregistered and registered again.

If we use that listener, it needs either support to unregister itself,
or store all entries in the iova tree so we can iterate them, unmap
and map them again.

> (Anyway, it's better to explain the reason in detail in the changelog.)
>

Sure, I can expand with this.

Thanks!

> Thanks
>
> > ---
> >  net/vhost-vdpa.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index a035c89c34..4c6947feb8 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -21,6 +21,7 @@
> >  #include "qemu/memalign.h"
> >  #include "qemu/option.h"
> >  #include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> >  #include <linux/vhost.h>
> >  #include <sys/ioctl.h>
> >  #include <err.h>
> > @@ -32,6 +33,8 @@
> >  typedef struct VhostVDPAState {
> >      NetClientState nc;
> >      struct vhost_vdpa vhost_vdpa;
> > +    MemoryListener memory_listener;
> > +
> >      VHostNetState *vhost_net;
> >
> >      /* Control commands shadow buffers */
> > @@ -110,6 +113,16 @@ static const uint64_t vdpa_svq_device_features =
> >  #define VHOST_VDPA_NET_CVQ_PASSTHROUGH 0
> >  #define VHOST_VDPA_NET_CVQ_ASID 1
> >
> > +/*
> > + * Vdpa memory listener must run before vhost one, so vhost_vdpa does not get
> > + * _F_LOG_ALL without SVQ.
> > + */
> > +#define VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY \
> > +                                       (VHOST_DEV_MEMORY_LISTENER_PRIORITY - 1)
> > +/* Check for underflow */
> > +QEMU_BUILD_BUG_ON(VHOST_DEV_MEMORY_LISTENER_PRIORITY <
> > +                  VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY);
> > +
> >  VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> >  {
> >      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > @@ -172,6 +185,9 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
> >
> >      qemu_vfree(s->cvq_cmd_out_buffer);
> >      qemu_vfree(s->cvq_cmd_in_buffer);
> > +    if (dev->vq_index == 0) {
> > +        memory_listener_unregister(&s->memory_listener);
> > +    }
> >      if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
> >          g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
> >      }
> > @@ -224,6 +240,69 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc, const uint8_t *buf,
> >      return 0;
> >  }
> >
> > +static void vhost_vdpa_net_log_global_enable(MemoryListener *listener,
> > +                                             bool enable)
> > +{
> > +    VhostVDPAState *s = container_of(listener, VhostVDPAState,
> > +                                     memory_listener);
> > +    struct vhost_vdpa *v = &s->vhost_vdpa;
> > +    VirtIONet *n;
> > +    VirtIODevice *vdev;
> > +    int data_queue_pairs, cvq, r;
> > +    NetClientState *peer;
> > +
> > +    if (s->always_svq || s->log_enabled == enable) {
> > +        return;
> > +    }
> > +
> > +    s->log_enabled = enable;
> > +    vdev = v->dev->vdev;
> > +    n = VIRTIO_NET(vdev);
> > +    if (!n->vhost_started) {
> > +        return;
> > +    }
> > +
> > +    if (enable) {
> > +        ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
> > +    }
> > +    data_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> > +    cvq = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) ?
> > +                                  n->max_ncs - n->max_queue_pairs : 0;
> > +    vhost_net_stop(vdev, n->nic->ncs, data_queue_pairs, cvq);
> > +
> > +    peer = s->nc.peer;
> > +    for (int i = 0; i < data_queue_pairs + cvq; i++) {
> > +        VhostVDPAState *vdpa_state;
> > +        NetClientState *nc;
> > +
> > +        if (i < data_queue_pairs) {
> > +            nc = qemu_get_peer(peer, i);
> > +        } else {
> > +            nc = qemu_get_peer(peer, n->max_queue_pairs);
> > +        }
> > +
> > +        vdpa_state = DO_UPCAST(VhostVDPAState, nc, nc);
> > +        vdpa_state->vhost_vdpa.listener_shadow_vq = enable;
> > +        vdpa_state->vhost_vdpa.shadow_vqs_enabled = enable;
> > +        vdpa_state->log_enabled = enable;
> > +    }
> > +
> > +    r = vhost_net_start(vdev, n->nic->ncs, data_queue_pairs, cvq);
> > +    if (unlikely(r < 0)) {
> > +        error_report("unable to start vhost net: %s(%d)", g_strerror(-r), -r);
> > +    }
> > +}
> > +
> > +static void vhost_vdpa_net_log_global_start(MemoryListener *listener)
> > +{
> > +    vhost_vdpa_net_log_global_enable(listener, true);
> > +}
> > +
> > +static void vhost_vdpa_net_log_global_stop(MemoryListener *listener)
> > +{
> > +    vhost_vdpa_net_log_global_enable(listener, false);
> > +}
> > +
> >  static NetClientInfo net_vhost_vdpa_info = {
> >          .type = NET_CLIENT_DRIVER_VHOST_VDPA,
> >          .size = sizeof(VhostVDPAState),
> > @@ -413,6 +492,7 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
> >
> >      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> >
> > +    memory_listener_unregister(&s->memory_listener);
> >      if (s->vhost_vdpa.shadow_vqs_enabled) {
> >          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
> >          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_in_buffer);
> > @@ -671,6 +751,13 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> >      s->vhost_vdpa.listener_shadow_vq = svq;
> >      s->vhost_vdpa.iova_tree = iova_tree;
> > +    if (queue_pair_index == 0) {
> > +        s->memory_listener = (MemoryListener) {
> > +            .log_global_start = vhost_vdpa_net_log_global_start,
> > +            .log_global_stop = vhost_vdpa_net_log_global_stop,
> > +        };
> > +        memory_listener_register(&s->memory_listener, &address_space_memory);
> > +    }
> >      if (!is_datapath) {
> >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > --
> > 2.31.1
> >
>

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Jason Wang]

On Fri, Aug 19, 2022 at 4:30 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Fri, Aug 19, 2022 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > This enable net/vdpa to restart the full device when a migration is
> > > started or stopped.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >
> > I have the following questions
> >
> > 1) any reason that we need to make this net specific? The dirty page
> > tracking via shadow virtqueue is pretty general. And the net specific
> > part was done via NetClientInfo anyhow.
>
> The listener is only used to know when migration is started / stopped,
> no need for actual memory tracking. Maybe there is a better way to do
> so?

Not sure, SaveVMHandlers?

>
> It's net specific because we are restarting the whole vhost_vdpa
> backend. We could do inside hw/virtio/vhost_vdpa.c (previous POCs did
> that way), but it's a little more complicated in my opinion. To do it
> that way, the setting of _F_LOG was detected and device were resetted
> and setup there.

Can we still have a general vhost-vdpa one and introduce net specific
callbacks? Otherwise the block may have its own listener.

>
>
> > 2) any reason we can't re-use the vhost-vdpa listener?
> >
>
> At this moment, all vhost_vdpa backend is restarted. That implies that
> the listener will be unregistered and registered again.
>
> If we use that listener, it needs either support to unregister itself,
> or store all entries in the iova tree so we can iterate them, unmap
> and map them again.

Ok, but let's double check whether or not we have another choice.
Using a dedicated listener to know if migration is started or not
seems too heavyweight.

Thanks

>
> > (Anyway, it's better to explain the reason in detail in the changelog.)
> >
>
> Sure, I can expand with this.
>
> Thanks!
>
> > Thanks
> >
> > > ---
> > >  net/vhost-vdpa.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 87 insertions(+)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index a035c89c34..4c6947feb8 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -21,6 +21,7 @@
> > >  #include "qemu/memalign.h"
> > >  #include "qemu/option.h"
> > >  #include "qapi/error.h"
> > > +#include "exec/address-spaces.h"
> > >  #include <linux/vhost.h>
> > >  #include <sys/ioctl.h>
> > >  #include <err.h>
> > > @@ -32,6 +33,8 @@
> > >  typedef struct VhostVDPAState {
> > >      NetClientState nc;
> > >      struct vhost_vdpa vhost_vdpa;
> > > +    MemoryListener memory_listener;
> > > +
> > >      VHostNetState *vhost_net;
> > >
> > >      /* Control commands shadow buffers */
> > > @@ -110,6 +113,16 @@ static const uint64_t vdpa_svq_device_features =
> > >  #define VHOST_VDPA_NET_CVQ_PASSTHROUGH 0
> > >  #define VHOST_VDPA_NET_CVQ_ASID 1
> > >
> > > +/*
> > > + * Vdpa memory listener must run before vhost one, so vhost_vdpa does not get
> > > + * _F_LOG_ALL without SVQ.
> > > + */
> > > +#define VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY \
> > > +                                       (VHOST_DEV_MEMORY_LISTENER_PRIORITY - 1)
> > > +/* Check for underflow */
> > > +QEMU_BUILD_BUG_ON(VHOST_DEV_MEMORY_LISTENER_PRIORITY <
> > > +                  VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY);
> > > +
> > >  VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > >  {
> > >      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > @@ -172,6 +185,9 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
> > >
> > >      qemu_vfree(s->cvq_cmd_out_buffer);
> > >      qemu_vfree(s->cvq_cmd_in_buffer);
> > > +    if (dev->vq_index == 0) {
> > > +        memory_listener_unregister(&s->memory_listener);
> > > +    }
> > >      if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
> > >          g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
> > >      }
> > > @@ -224,6 +240,69 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc, const uint8_t *buf,
> > >      return 0;
> > >  }
> > >
> > > +static void vhost_vdpa_net_log_global_enable(MemoryListener *listener,
> > > +                                             bool enable)
> > > +{
> > > +    VhostVDPAState *s = container_of(listener, VhostVDPAState,
> > > +                                     memory_listener);
> > > +    struct vhost_vdpa *v = &s->vhost_vdpa;
> > > +    VirtIONet *n;
> > > +    VirtIODevice *vdev;
> > > +    int data_queue_pairs, cvq, r;
> > > +    NetClientState *peer;
> > > +
> > > +    if (s->always_svq || s->log_enabled == enable) {
> > > +        return;
> > > +    }
> > > +
> > > +    s->log_enabled = enable;
> > > +    vdev = v->dev->vdev;
> > > +    n = VIRTIO_NET(vdev);
> > > +    if (!n->vhost_started) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (enable) {
> > > +        ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
> > > +    }
> > > +    data_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> > > +    cvq = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) ?
> > > +                                  n->max_ncs - n->max_queue_pairs : 0;
> > > +    vhost_net_stop(vdev, n->nic->ncs, data_queue_pairs, cvq);
> > > +
> > > +    peer = s->nc.peer;
> > > +    for (int i = 0; i < data_queue_pairs + cvq; i++) {
> > > +        VhostVDPAState *vdpa_state;
> > > +        NetClientState *nc;
> > > +
> > > +        if (i < data_queue_pairs) {
> > > +            nc = qemu_get_peer(peer, i);
> > > +        } else {
> > > +            nc = qemu_get_peer(peer, n->max_queue_pairs);
> > > +        }
> > > +
> > > +        vdpa_state = DO_UPCAST(VhostVDPAState, nc, nc);
> > > +        vdpa_state->vhost_vdpa.listener_shadow_vq = enable;
> > > +        vdpa_state->vhost_vdpa.shadow_vqs_enabled = enable;
> > > +        vdpa_state->log_enabled = enable;
> > > +    }
> > > +
> > > +    r = vhost_net_start(vdev, n->nic->ncs, data_queue_pairs, cvq);
> > > +    if (unlikely(r < 0)) {
> > > +        error_report("unable to start vhost net: %s(%d)", g_strerror(-r), -r);
> > > +    }
> > > +}
> > > +
> > > +static void vhost_vdpa_net_log_global_start(MemoryListener *listener)
> > > +{
> > > +    vhost_vdpa_net_log_global_enable(listener, true);
> > > +}
> > > +
> > > +static void vhost_vdpa_net_log_global_stop(MemoryListener *listener)
> > > +{
> > > +    vhost_vdpa_net_log_global_enable(listener, false);
> > > +}
> > > +
> > >  static NetClientInfo net_vhost_vdpa_info = {
> > >          .type = NET_CLIENT_DRIVER_VHOST_VDPA,
> > >          .size = sizeof(VhostVDPAState),
> > > @@ -413,6 +492,7 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
> > >
> > >      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > >
> > > +    memory_listener_unregister(&s->memory_listener);
> > >      if (s->vhost_vdpa.shadow_vqs_enabled) {
> > >          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
> > >          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_in_buffer);
> > > @@ -671,6 +751,13 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > >      s->vhost_vdpa.listener_shadow_vq = svq;
> > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > +    if (queue_pair_index == 0) {
> > > +        s->memory_listener = (MemoryListener) {
> > > +            .log_global_start = vhost_vdpa_net_log_global_start,
> > > +            .log_global_stop = vhost_vdpa_net_log_global_stop,
> > > +        };
> > > +        memory_listener_register(&s->memory_listener, &address_space_memory);
> > > +    }
> > >      if (!is_datapath) {
> > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > --
> > > 2.31.1
> > >
> >
>

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Eugenio Perez Martin]

On Fri, Aug 19, 2022 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Aug 19, 2022 at 4:30 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Aug 19, 2022 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > This enable net/vdpa to restart the full device when a migration is
> > > > started or stopped.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >
> > > I have the following questions
> > >
> > > 1) any reason that we need to make this net specific? The dirty page
> > > tracking via shadow virtqueue is pretty general. And the net specific
> > > part was done via NetClientInfo anyhow.
> >
> > The listener is only used to know when migration is started / stopped,
> > no need for actual memory tracking. Maybe there is a better way to do
> > so?
>
> Not sure, SaveVMHandlers?
>

I'm fine with investigating this, but the only entry in the doc says
it's the "legacy way". I assume the "modern way" is through
VMStateDescription, which is in virtio-net.

The "pre_save" member already assumes the vhost backend is stopped, so
I'm not sure if this way is valid.

> >
> > It's net specific because we are restarting the whole vhost_vdpa
> > backend. We could do inside hw/virtio/vhost_vdpa.c (previous POCs did
> > that way), but it's a little more complicated in my opinion. To do it
> > that way, the setting of _F_LOG was detected and device were resetted
> > and setup there.
>
> Can we still have a general vhost-vdpa one and introduce net specific
> callbacks? Otherwise the block may have its own listener.
>

If we reuse the vhost-vdpa listener, we cannot unregister it and
register it again from its own log_global_start / stop. We need to
track all the memory always, so we can map it again using qemu's IOVA
to use SVQ. Also, we need to duplicate vhost_vdpa_dev_start / stop
without memory register related calls.

On the other hand, it will put the SVQ solution farther away from
being "a start parameter of vhost-vdpa backend". Instead of being an
argument of vhost_vdpa initialization, now it is a variable that can
switch state at any moment. That implies extra code in vhost-vdpa too.

> >
> >
> > > 2) any reason we can't re-use the vhost-vdpa listener?
> > >
> >
> > At this moment, all vhost_vdpa backend is restarted. That implies that
> > the listener will be unregistered and registered again.
> >
> > If we use that listener, it needs either support to unregister itself,
> > or store all entries in the iova tree so we can iterate them, unmap
> > and map them again.
>
> Ok, but let's double check whether or not we have another choice.

Sure!

> Using a dedicated listener to know if migration is started or not
> seems too heavyweight.
>

Take into account that memory.c does not call a callback that does not
exist. Although this new memory listener is registered per vdpa device
(not per queue pair), it's skipped when memory is added or removed.

In that regard, to register all hdev->memory_listener of each queue
pair + cvq is more expensive than adding this. And they have no use in
vdpa.

Thanks!

> Thanks
>
> >
> > > (Anyway, it's better to explain the reason in detail in the changelog.)
> > >
> >
> > Sure, I can expand with this.
> >
> > Thanks!
> >
> > > Thanks
> > >
> > > > ---
> > > >  net/vhost-vdpa.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 87 insertions(+)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index a035c89c34..4c6947feb8 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -21,6 +21,7 @@
> > > >  #include "qemu/memalign.h"
> > > >  #include "qemu/option.h"
> > > >  #include "qapi/error.h"
> > > > +#include "exec/address-spaces.h"
> > > >  #include <linux/vhost.h>
> > > >  #include <sys/ioctl.h>
> > > >  #include <err.h>
> > > > @@ -32,6 +33,8 @@
> > > >  typedef struct VhostVDPAState {
> > > >      NetClientState nc;
> > > >      struct vhost_vdpa vhost_vdpa;
> > > > +    MemoryListener memory_listener;
> > > > +
> > > >      VHostNetState *vhost_net;
> > > >
> > > >      /* Control commands shadow buffers */
> > > > @@ -110,6 +113,16 @@ static const uint64_t vdpa_svq_device_features =
> > > >  #define VHOST_VDPA_NET_CVQ_PASSTHROUGH 0
> > > >  #define VHOST_VDPA_NET_CVQ_ASID 1
> > > >
> > > > +/*
> > > > + * Vdpa memory listener must run before vhost one, so vhost_vdpa does not get
> > > > + * _F_LOG_ALL without SVQ.
> > > > + */
> > > > +#define VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY \
> > > > +                                       (VHOST_DEV_MEMORY_LISTENER_PRIORITY - 1)
> > > > +/* Check for underflow */
> > > > +QEMU_BUILD_BUG_ON(VHOST_DEV_MEMORY_LISTENER_PRIORITY <
> > > > +                  VHOST_VDPA_NET_MEMORY_LISTENER_PRIORITY);
> > > > +
> > > >  VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > >  {
> > > >      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > @@ -172,6 +185,9 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
> > > >
> > > >      qemu_vfree(s->cvq_cmd_out_buffer);
> > > >      qemu_vfree(s->cvq_cmd_in_buffer);
> > > > +    if (dev->vq_index == 0) {
> > > > +        memory_listener_unregister(&s->memory_listener);
> > > > +    }
> > > >      if (dev->vq_index + dev->nvqs == dev->vq_index_end) {
> > > >          g_clear_pointer(&s->vhost_vdpa.iova_tree, vhost_iova_tree_delete);
> > > >      }
> > > > @@ -224,6 +240,69 @@ static ssize_t vhost_vdpa_receive(NetClientState *nc, const uint8_t *buf,
> > > >      return 0;
> > > >  }
> > > >
> > > > +static void vhost_vdpa_net_log_global_enable(MemoryListener *listener,
> > > > +                                             bool enable)
> > > > +{
> > > > +    VhostVDPAState *s = container_of(listener, VhostVDPAState,
> > > > +                                     memory_listener);
> > > > +    struct vhost_vdpa *v = &s->vhost_vdpa;
> > > > +    VirtIONet *n;
> > > > +    VirtIODevice *vdev;
> > > > +    int data_queue_pairs, cvq, r;
> > > > +    NetClientState *peer;
> > > > +
> > > > +    if (s->always_svq || s->log_enabled == enable) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    s->log_enabled = enable;
> > > > +    vdev = v->dev->vdev;
> > > > +    n = VIRTIO_NET(vdev);
> > > > +    if (!n->vhost_started) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (enable) {
> > > > +        ioctl(v->device_fd, VHOST_VDPA_SUSPEND);
> > > > +    }
> > > > +    data_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
> > > > +    cvq = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) ?
> > > > +                                  n->max_ncs - n->max_queue_pairs : 0;
> > > > +    vhost_net_stop(vdev, n->nic->ncs, data_queue_pairs, cvq);
> > > > +
> > > > +    peer = s->nc.peer;
> > > > +    for (int i = 0; i < data_queue_pairs + cvq; i++) {
> > > > +        VhostVDPAState *vdpa_state;
> > > > +        NetClientState *nc;
> > > > +
> > > > +        if (i < data_queue_pairs) {
> > > > +            nc = qemu_get_peer(peer, i);
> > > > +        } else {
> > > > +            nc = qemu_get_peer(peer, n->max_queue_pairs);
> > > > +        }
> > > > +
> > > > +        vdpa_state = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > +        vdpa_state->vhost_vdpa.listener_shadow_vq = enable;
> > > > +        vdpa_state->vhost_vdpa.shadow_vqs_enabled = enable;
> > > > +        vdpa_state->log_enabled = enable;
> > > > +    }
> > > > +
> > > > +    r = vhost_net_start(vdev, n->nic->ncs, data_queue_pairs, cvq);
> > > > +    if (unlikely(r < 0)) {
> > > > +        error_report("unable to start vhost net: %s(%d)", g_strerror(-r), -r);
> > > > +    }
> > > > +}
> > > > +
> > > > +static void vhost_vdpa_net_log_global_start(MemoryListener *listener)
> > > > +{
> > > > +    vhost_vdpa_net_log_global_enable(listener, true);
> > > > +}
> > > > +
> > > > +static void vhost_vdpa_net_log_global_stop(MemoryListener *listener)
> > > > +{
> > > > +    vhost_vdpa_net_log_global_enable(listener, false);
> > > > +}
> > > > +
> > > >  static NetClientInfo net_vhost_vdpa_info = {
> > > >          .type = NET_CLIENT_DRIVER_VHOST_VDPA,
> > > >          .size = sizeof(VhostVDPAState),
> > > > @@ -413,6 +492,7 @@ static void vhost_vdpa_net_cvq_stop(NetClientState *nc)
> > > >
> > > >      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > > >
> > > > +    memory_listener_unregister(&s->memory_listener);
> > > >      if (s->vhost_vdpa.shadow_vqs_enabled) {
> > > >          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_out_buffer);
> > > >          vhost_vdpa_cvq_unmap_buf(&s->vhost_vdpa, s->cvq_cmd_in_buffer);
> > > > @@ -671,6 +751,13 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > > >      s->vhost_vdpa.listener_shadow_vq = svq;
> > > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > > +    if (queue_pair_index == 0) {
> > > > +        s->memory_listener = (MemoryListener) {
> > > > +            .log_global_start = vhost_vdpa_net_log_global_start,
> > > > +            .log_global_stop = vhost_vdpa_net_log_global_stop,
> > > > +        };
> > > > +        memory_listener_register(&s->memory_listener, &address_space_memory);
> > > > +    }
> > > >      if (!is_datapath) {
> > > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > > --
> > > > 2.31.1
> > > >
> > >
> >
>

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Jason Wang]

On Fri, Aug 19, 2022 at 6:35 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Fri, Aug 19, 2022 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, Aug 19, 2022 at 4:30 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Fri, Aug 19, 2022 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > This enable net/vdpa to restart the full device when a migration is
> > > > > started or stopped.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > >
> > > > I have the following questions
> > > >
> > > > 1) any reason that we need to make this net specific? The dirty page
> > > > tracking via shadow virtqueue is pretty general. And the net specific
> > > > part was done via NetClientInfo anyhow.
> > >
> > > The listener is only used to know when migration is started / stopped,
> > > no need for actual memory tracking. Maybe there is a better way to do
> > > so?
> >
> > Not sure, SaveVMHandlers?
> >
>
> I'm fine with investigating this, but the only entry in the doc says
> it's the "legacy way". I assume the "modern way" is through
> VMStateDescription, which is in virtio-net.

Right.

>
> The "pre_save" member already assumes the vhost backend is stopped, so
> I'm not sure if this way is valid.

I wonder if we can

1) new VhostOps
2) call that ops in vhost_log_gloabal_start/stop?

Thanks

Re: [RFC 5/8] vdpa: Add vdpa memory listener

[Eugenio Perez Martin]

On Tue, Aug 23, 2022 at 5:58 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Aug 19, 2022 at 6:35 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Aug 19, 2022 at 11:01 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Fri, Aug 19, 2022 at 4:30 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Fri, Aug 19, 2022 at 8:29 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Aug 11, 2022 at 2:42 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > This enable net/vdpa to restart the full device when a migration is
> > > > > > started or stopped.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > >
> > > > > I have the following questions
> > > > >
> > > > > 1) any reason that we need to make this net specific? The dirty page
> > > > > tracking via shadow virtqueue is pretty general. And the net specific
> > > > > part was done via NetClientInfo anyhow.
> > > >
> > > > The listener is only used to know when migration is started / stopped,
> > > > no need for actual memory tracking. Maybe there is a better way to do
> > > > so?
> > >
> > > Not sure, SaveVMHandlers?
> > >
> >
> > I'm fine with investigating this, but the only entry in the doc says
> > it's the "legacy way". I assume the "modern way" is through
> > VMStateDescription, which is in virtio-net.
>
> Right.
>
> >
> > The "pre_save" member already assumes the vhost backend is stopped, so
> > I'm not sure if this way is valid.
>
> I wonder if we can
>
> 1) new VhostOps
> 2) call that ops in vhost_log_gloabal_start/stop?
>

Bringing this thread up as I'm rebasing on top of the latest asid version.

We can detect that moment when set_features is called to set _F_LOG.
The problem is that it makes us duplicate the startup and teardown
code of the backend.

SVQ was proposed originally that way [1].

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg06049.html

Thanks!