[Qemu-devel] [PATCH v6 00/33] vhost-user reconnect fixes

[Qemu-devel] [PATCH v6 00/33] vhost-user reconnect fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Since 'vhost-user: simple reconnection support' has been merged, it is
possible to disconnect and reconnect a vhost-user backend. However,
many code paths in qemu may trigger assert() when the backend is
disconnected.

There are also code paths that are wrong, see "don't assume opaque is
a fd" patch for an example. Once those patches are reviewed & merged,
they are good candidates for stable too.

Some assert() could simply be replaced by error_report() or silently
fail since they are recoverable cases. Some missing error checks can
also help prevent later issues. The errors are reported up to vhost.c,
as the vhost-user backend alone doesn't handle disconnected state
transparently so far. There are still problematic code paths left
after this series, for example, starting a migration with a
disconnected backend will abort(). It is likely that other problematic
code path exists (vhost_scsi_start failure is fatal, but there are no
vhost-user backend that I know yet).

In many cases, the code assumes get_vhost_net() will be non-NULL after
a succesful connection, so I changed it to stay after a disconnect
until the new connection comes (as suggested by Michael).

Since there is feature checks on reconnection, qemu should wait for
the initial connection feature negotiation to complete. The test added
demonstrates this. Additionally, a regression was found during v2,
which could have been spotted with a multiqueue test, so add a basic
one that would have exhibited the issue.

For convenience, the series is also available on:
https://github.com/elmarco/qemu, branch vhost-user-reconnect

v6:
- fixes after Ilya Maximets review
- add "disconnect on HUP" for cases where peer disconnect doesn't
  trigger qemu disconnect event (reconnect won't work)
- add a flags mismatch on reconnect test

v5:
- rebased
- use a VHOST_OPS_DEBUG macro to print vhost_ops errors
- replace assert(foo != NULL) with assert(foo)
- add "RFC: vhost: do not update last avail idx"

v4:
- change notify_migration_done() patch to be VHOST_BACKEND_TYPE_USER
  specific, to avoid having to handle the case where the backend
  doesn't implement the callback
- change vhost_dev_cleanup() to assert on empty log, instead of
  adding a call to vhost_log_put()
- made "keep vhost_net after a disconnection" more robust, got rid of
  the acked_features field
- improve commit log, and some patch reorganization for clarity

v3:
- add vhost-user multiqueue test, which would have helped to find the
  following fix
- fix waiting on vhost-user connection with multiqueue (found by
  Yuanhan Liu)
- merge vhost_user_{read,write}() error checking patches
- add error reporting to vhost_user_read() (similar to
  vhost_user_write())
- add a vhost_net_set_backend() wrapper to help with potential crash
- some leak fixes

v2:
- some patch ordering: minor fix, close(fd) fix,
  assert/fprintf->error_report, check and return error,
  vhost_dev_cleanup() fixes, keep vhost_net after a disconnect, wait
  until connection is ready
- merge read/write error checks
- do not rely on link state to check vhost-user init completed

Marc-André Lureau (33):
  misc: indentation
  vhost-user: minor simplification
  vhost-user: disconnect on HUP
  vhost: don't assume opaque is a fd, use backend cleanup
  vhost: make vhost_log_put() idempotent
  vhost: assert the log was cleaned up
  vhost: fix cleanup on not fully initialized device
  vhost: make vhost_dev_cleanup() idempotent
  vhost-net: always call vhost_dev_cleanup() on failure
  vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()
  vhost: do not assert() on vhost_ops failure
  vhost: add missing VHOST_OPS_DEBUG
  vhost: use error_report() instead of fprintf(stderr,...)
  qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected
  vhost-user: call set_msgfds unconditionally
  vhost-user: check qemu_chr_fe_set_msgfds() return value
  vhost-user: check vhost_user_{read,write}() return value
  vhost-user: keep vhost_net after a disconnection
  vhost-user: add get_vhost_net() assertions
  Revert "vhost-net: do not crash if backend is not present"
  vhost-net: vhost_migration_done is vhost-user specific
  vhost: add assert() to check runtime behaviour
  char: add chr_wait_connected callback
  char: add and use tcp_chr_wait_connected
  vhost-user: wait until backend init is completed
  tests: plug some leaks in virtio-net-test
  tests: fix vhost-user-test leak
  tests: add /vhost-user/connect-fail test
  tests: add a simple /vhost-user/multiqueue test
  vhost-user: add error report in vhost_user_write()
  vhost: add vhost_net_set_backend()
  vhost-user-test: add flags mismatch test
  RFC: vhost: do not update last avail idx on get_vring_base() failure

 hw/net/vhost_net.c        |  34 +++-----
 hw/virtio/vhost-user.c    |  67 ++++++++++-----
 hw/virtio/vhost.c         | 162 +++++++++++++++++++++++-------------
 include/hw/virtio/vhost.h |   4 +
 include/sysemu/char.h     |   8 ++
 net/tap.c                 |   1 +
 net/vhost-user.c          |  65 +++++++++------
 qemu-char.c               |  82 ++++++++++++------
 tests/Makefile.include    |   2 +-
 tests/vhost-user-test.c   | 206 +++++++++++++++++++++++++++++++++++++++++++++-
 tests/virtio-net-test.c   |  12 ++-
 11 files changed, 486 insertions(+), 157 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v6 01/33] misc: indentation

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f92d3f8..3677a82 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -313,7 +313,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
          * properly.
          */
         if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
-                dev->use_guest_notifier_mask = false;
+            dev->use_guest_notifier_mask = false;
         }
      }
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 02/33] vhost-user: minor simplification

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Shorten the code and make it more clear by using the specialized
function g_str_has_prefix().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index c4d63e0..2af212b 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -316,7 +316,6 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
 {
     const char *name = opaque;
     const char *driver, *netdev;
-    const char virtio_name[] = "virtio-net-";
 
     driver = qemu_opt_get(opts, "driver");
     netdev = qemu_opt_get(opts, "netdev");
@@ -326,7 +325,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
     }
 
     if (strcmp(netdev, name) == 0 &&
-        strncmp(driver, virtio_name, strlen(virtio_name)) != 0) {
+        !g_str_has_prefix(driver, "virtio-net-")) {
         error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
         return -1;
     }
-- 
2.9.0

[Qemu-devel] [PATCH v6 03/33] vhost-user: disconnect on HUP

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

In some cases, qemu_chr_fe_read_all() on HUP event doesn't raise
CHR_EVENT_CLOSED because the read/recv function returns -1 on
disconnected peers (for example with tch_chr_recv, an ECONNRESET errno
overwritten as EIO).

It is simpler to explicitely disconnect on HUP, rising CHR_EVENT_CLOSED
if it wasn't disconnected already.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 2af212b..2cac32e 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -188,12 +188,8 @@ static gboolean net_vhost_user_watch(GIOChannel *chan, GIOCondition cond,
                                            void *opaque)
 {
     VhostUserState *s = opaque;
-    uint8_t buf[1];
 
-    /* We don't actually want to read anything, but CHR_EVENT_CLOSED will be
-     * raised as a side-effect of the read.
-     */
-    qemu_chr_fe_read_all(s->chr, buf, sizeof(buf));
+    qemu_chr_disconnect(s->chr);
 
     return FALSE;
 }
-- 
2.9.0

[Qemu-devel] [PATCH v6 04/33] vhost: don't assume opaque is a fd, use backend cleanup

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost-dev opaque isn't necessarily an fd, it can be a chardev when using
vhost-user. Goto fail, so vhost_backend_cleanup() is called to handle
backend cleanup appropriately.

vhost_set_backend_type() should never fail, use an assert().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ec3abda..429499a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1019,21 +1019,19 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     hdev->migration_blocker = NULL;
 
-    if (vhost_set_backend_type(hdev, backend_type) < 0) {
-        close((uintptr_t)opaque);
-        return -1;
-    }
+    r = vhost_set_backend_type(hdev, backend_type);
+    assert(r >= 0);
 
-    if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
-        close((uintptr_t)opaque);
-        return -errno;
+    r = hdev->vhost_ops->vhost_backend_init(hdev, opaque);
+    if (r < 0) {
+        goto fail;
     }
 
     if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
         fprintf(stderr, "vhost backend memory slots limit is less"
                 " than current number of present memory slots\n");
-        close((uintptr_t)opaque);
-        return -1;
+        r = -1;
+        goto fail;
     }
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 05/33] vhost: make vhost_log_put() idempotent

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Although not strictly required, it is nice to have vhost_log_put()
safely callable multiple times.

Clear dev->log* when calling vhost_log_put() to make the function
idempotent. This also simplifies a bit the caller work.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 429499a..9bac163 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -362,6 +362,8 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
     if (!log) {
         return;
     }
+    dev->log = NULL;
+    dev->log_size = 0;
 
     --log->refcnt;
     if (log->refcnt == 0) {
@@ -710,8 +712,6 @@ static int vhost_migration_log(MemoryListener *listener, int enable)
             return r;
         }
         vhost_log_put(dev, false);
-        dev->log = NULL;
-        dev->log_size = 0;
     } else {
         vhost_dev_log_resize(dev, vhost_get_log_size(dev));
         r = vhost_dev_set_log(dev, true);
@@ -1328,7 +1328,4 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
 
     vhost_log_put(hdev, true);
     hdev->started = false;
-    hdev->log = NULL;
-    hdev->log_size = 0;
 }
-
-- 
2.9.0

[Qemu-devel] [PATCH v6 06/33] vhost: assert the log was cleaned up

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Make sure the log was released on cleanup, or it will leak (the
alternative is to call vhost_log_put() unconditionally, but it may hide
some dev state issues).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9bac163..8a18f9b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1134,6 +1134,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     g_free(hdev->mem);
     g_free(hdev->mem_sections);
     hdev->vhost_ops->vhost_backend_cleanup(hdev);
+    assert(!hdev->log);
     QLIST_REMOVE(hdev, entry);
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 07/33] vhost: fix cleanup on not fully initialized device

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

If vhost_dev_init() failed, caller may still call vhost_dev_cleanup()
later. However, vhost_dev_cleanup() tries to remove the device from the
list even if it wasn't yet added, which may lead to crashes. Similarly
for the memory listener.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 8a18f9b..6b988e1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1033,7 +1033,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         r = -1;
         goto fail;
     }
-    QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
 
     r = hdev->vhost_ops->vhost_set_owner(hdev);
     if (r < 0) {
@@ -1103,6 +1102,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->started = false;
     hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
+    QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
     return 0;
 fail_busyloop:
     while (--i >= 0) {
@@ -1126,7 +1126,11 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev->vqs + i);
     }
-    memory_listener_unregister(&hdev->memory_listener);
+    if (hdev->mem) {
+        /* those are only safe after successful init */
+        memory_listener_unregister(&hdev->memory_listener);
+        QLIST_REMOVE(hdev, entry);
+    }
     if (hdev->migration_blocker) {
         migrate_del_blocker(hdev->migration_blocker);
         error_free(hdev->migration_blocker);
@@ -1135,7 +1139,6 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     g_free(hdev->mem_sections);
     hdev->vhost_ops->vhost_backend_cleanup(hdev);
     assert(!hdev->log);
-    QLIST_REMOVE(hdev, entry);
 }
 
 /* Stop processing guest IO notifications in qemu.
-- 
2.9.0

[Qemu-devel] [PATCH v6 08/33] vhost: make vhost_dev_cleanup() idempotent

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

It is called on multiple code path, so make it safe to call several
times (note: I don't remember a reproducer here, but a function called
'cleanup' should probably be idempotent in my book)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6b988e1..9400b47 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1123,6 +1123,7 @@ fail:
 void vhost_dev_cleanup(struct vhost_dev *hdev)
 {
     int i;
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev->vqs + i);
     }
@@ -1137,8 +1138,12 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     }
     g_free(hdev->mem);
     g_free(hdev->mem_sections);
-    hdev->vhost_ops->vhost_backend_cleanup(hdev);
+    if (hdev->vhost_ops) {
+        hdev->vhost_ops->vhost_backend_cleanup(hdev);
+    }
     assert(!hdev->log);
+
+    memset(hdev, 0, sizeof(struct vhost_dev));
 }
 
 /* Stop processing guest IO notifications in qemu.
-- 
2.9.0

[Qemu-devel] [PATCH v6 09/33] vhost-net: always call vhost_dev_cleanup() on failure

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost_dev_init(), calling vhost backend initialization, should be
cleaned up after failure too. Call vhost_dev_cleanup() in all failure
cases. First, it needs to zero-alloc the struct to avoid the initial
garbage.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 3677a82..c11f69c 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -140,7 +140,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
 {
     int r;
     bool backend_kernel = options->backend_type == VHOST_BACKEND_TYPE_KERNEL;
-    struct vhost_net *net = g_malloc(sizeof *net);
+    struct vhost_net *net = g_new0(struct vhost_net, 1);
     uint64_t features = 0;
 
     if (!options->net_backend) {
@@ -185,7 +185,6 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
             fprintf(stderr, "vhost lacks feature mask %" PRIu64
                    " for backend\n",
                    (uint64_t)(~net->dev.features & net->dev.backend_features));
-            vhost_dev_cleanup(&net->dev);
             goto fail;
         }
     }
@@ -197,7 +196,6 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
             fprintf(stderr, "vhost lacks feature mask %" PRIu64
                     " for backend\n",
                     (uint64_t)(~net->dev.features & features));
-            vhost_dev_cleanup(&net->dev);
             goto fail;
         }
     }
@@ -205,7 +203,9 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
     vhost_net_ack_features(net, features);
 
     return net;
+
 fail:
+    vhost_dev_cleanup(&net->dev);
     g_free(net);
     return NULL;
 }
-- 
2.9.0

[Qemu-devel] [PATCH v6 10/33] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost_net_init() calls vhost_dev_init() and in case of failure, calls
vhost_dev_cleanup() directly. However, the structure is already
partially cleaned on error. Calling vhost_dev_cleanup() again will call
vhost_virtqueue_cleanup() on already clean queues, and causing potential
double-close. Instead, adjust dev->nvqs and simplify vhost_dev_init()
code to not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 hw/virtio/vhost.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9400b47..e3091d1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1015,7 +1015,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
                    VhostBackendType backend_type, uint32_t busyloop_timeout)
 {
     uint64_t features;
-    int i, r;
+    int i, r, n_initialized_vqs = 0;
 
     hdev->migration_blocker = NULL;
 
@@ -1044,10 +1044,10 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         goto fail;
     }
 
-    for (i = 0; i < hdev->nvqs; ++i) {
+    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
         r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
         if (r < 0) {
-            goto fail_vq;
+            goto fail;
         }
     }
 
@@ -1104,19 +1104,14 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
     return 0;
+
 fail_busyloop:
     while (--i >= 0) {
         vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
     }
-    i = hdev->nvqs;
-fail_vq:
-    while (--i >= 0) {
-        vhost_virtqueue_cleanup(hdev->vqs + i);
-    }
 fail:
-    r = -errno;
-    hdev->vhost_ops->vhost_backend_cleanup(hdev);
-    QLIST_REMOVE(hdev, entry);
+    hdev->nvqs = n_initialized_vqs;
+    vhost_dev_cleanup(hdev);
     return r;
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 11/33] vhost: do not assert() on vhost_ops failure

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Calling a vhost operation may fail, for example with disconnected
vhost-user backend, but qemu shouldn't abort in this case.

Log an error instead, except on error and cleanup code paths where it
can be mostly ignored.

Let's use a VHOST_OPS_DEBUG macro to easily disable those messages once
disconnected backend stabilizes.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e3091d1..1ece1ef 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -27,6 +27,18 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/migration.h"
 
+/* enabled until disconnected backend stabilizes */
+#define _VHOST_DEBUG 1
+
+#ifdef _VHOST_DEBUG
+#define VHOST_OPS_DEBUG(fmt, ...) \
+    do { error_report(fmt ": %s (%d)", ## __VA_ARGS__, \
+                      strerror(errno), errno); } while (0)
+#else
+#define VHOST_OPS_DEBUG(fmt, ...) \
+    do { } while (0)
+#endif
+
 static struct vhost_log *vhost_log;
 static struct vhost_log *vhost_log_shm;
 
@@ -400,7 +412,10 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
     /* inform backend of log switching, this must be done before
        releasing the current log, to ensure no logging is lost */
     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
-    assert(r >= 0);
+    if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_log_base failed");
+    }
+
     vhost_log_put(dev, true);
     dev->log = log;
     dev->log_size = size;
@@ -567,7 +582,9 @@ static void vhost_commit(MemoryListener *listener)
 
     if (!dev->log_enabled) {
         r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
-        assert(r >= 0);
+        if (r < 0) {
+            VHOST_OPS_DEBUG("vhost_set_mem_table failed");
+        }
         dev->memory_changed = false;
         return;
     }
@@ -580,7 +597,9 @@ static void vhost_commit(MemoryListener *listener)
         vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
     }
     r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
-    assert(r >= 0);
+    if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_mem_table failed");
+    }
     /* To log less, can only decrease log size after table update. */
     if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
         vhost_dev_log_resize(dev, log_size);
@@ -667,7 +686,7 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
 
 static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
 {
-    int r, t, i, idx;
+    int r, i, idx;
     r = vhost_dev_set_features(dev, enable_log);
     if (r < 0) {
         goto err_features;
@@ -684,12 +703,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
 err_vq:
     for (; i >= 0; --i) {
         idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
-        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
-                                     dev->log_enabled);
-        assert(t >= 0);
+        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
+                                 dev->log_enabled);
     }
-    t = vhost_dev_set_features(dev, dev->log_enabled);
-    assert(t >= 0);
+    vhost_dev_set_features(dev, dev->log_enabled);
 err_features:
     return r;
 }
@@ -929,15 +946,11 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
      * native as legacy devices expect so by default.
      */
     if (vhost_needs_vring_endian(vdev)) {
-        r = vhost_virtqueue_set_vring_endian_legacy(dev,
-                                                    !virtio_is_big_endian(vdev),
-                                                    vhost_vq_index);
-        if (r < 0) {
-            error_report("failed to reset vring endianness");
-        }
+        vhost_virtqueue_set_vring_endian_legacy(dev,
+                                                !virtio_is_big_endian(vdev),
+                                                vhost_vq_index);
     }
 
-    assert (r >= 0);
     cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
                               0, virtio_queue_get_ring_size(vdev, idx));
     cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),
@@ -1228,7 +1241,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
 
     file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
     r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
-    assert(r >= 0);
+    if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_vring_call failed");
+    }
 }
 
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
-- 
2.9.0

[Qemu-devel] [PATCH v6 12/33] vhost: add missing VHOST_OPS_DEBUG

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add missing VHOST_OPS_DEBUG() logs, for completeness.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1ece1ef..5a29eb3 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -668,6 +668,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
     };
     int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_vring_addr failed");
         return -errno;
     }
     return 0;
@@ -681,6 +682,9 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
         features |= 0x1ULL << VHOST_F_LOG_ALL;
     }
     r = dev->vhost_ops->vhost_set_features(dev, features);
+    if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_features failed");
+    }
     return r < 0 ? -errno : 0;
 }
 
@@ -804,6 +808,7 @@ static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
         return 0;
     }
 
+    VHOST_OPS_DEBUG("vhost_set_vring_endian failed");
     if (errno == ENOTTY) {
         error_report("vhost does not support cross-endian");
         return -ENOSYS;
@@ -832,12 +837,14 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     vq->num = state.num = virtio_queue_get_num(vdev, idx);
     r = dev->vhost_ops->vhost_set_vring_num(dev, &state);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_num failed");
         return -errno;
     }
 
     state.num = virtio_queue_get_last_avail_idx(vdev, idx);
     r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_base failed");
         return -errno;
     }
 
@@ -889,6 +896,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     file.fd = event_notifier_get_fd(virtio_queue_get_host_notifier(vvq));
     r = dev->vhost_ops->vhost_set_vring_kick(dev, &file);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_kick failed");
         r = -errno;
         goto fail_kick;
     }
@@ -936,8 +944,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
 
     r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
     if (r < 0) {
-        fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx, r);
-        fflush(stderr);
+        VHOST_OPS_DEBUG("vhost VQ %d ring restore failed: %d", idx, r);
     }
     virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     virtio_queue_invalidate_signalled_used(vdev, idx);
@@ -989,6 +996,7 @@ static int vhost_virtqueue_set_busyloop_timeout(struct vhost_dev *dev,
 
     r = dev->vhost_ops->vhost_set_vring_busyloop_timeout(dev, &state);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_busyloop_timeout failed");
         return r;
     }
 
@@ -1010,6 +1018,7 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
     file.fd = event_notifier_get_fd(&vq->masked_notifier);
     r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_call failed");
         r = -errno;
         goto fail_call;
     }
@@ -1049,11 +1058,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     r = hdev->vhost_ops->vhost_set_owner(hdev);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_owner failed");
         goto fail;
     }
 
     r = hdev->vhost_ops->vhost_get_features(hdev, &features);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_get_features failed");
         goto fail;
     }
 
@@ -1286,6 +1297,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     }
     r = hdev->vhost_ops->vhost_set_mem_table(hdev, hdev->mem);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_mem_table failed");
         r = -errno;
         goto fail_mem;
     }
@@ -1310,6 +1322,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
                                                 hdev->log_size ? log_base : 0,
                                                 hdev->log);
         if (r < 0) {
+            VHOST_OPS_DEBUG("vhost_set_log_base failed");
             r = -errno;
             goto fail_log;
         }
-- 
2.9.0

[Qemu-devel] [PATCH v6 13/33] vhost: use error_report() instead of fprintf(stderr, ...)

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Let's use qemu proper error reporting API, this ensures the error is
reported at the right place (stderr or monitor), with a conventional
format.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5a29eb3..bb886f3 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -439,11 +439,11 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         l = vq->ring_size;
         p = cpu_physical_memory_map(vq->ring_phys, &l, 1);
         if (!p || l != vq->ring_size) {
-            fprintf(stderr, "Unable to map ring buffer for ring %d\n", i);
+            error_report("Unable to map ring buffer for ring %d", i);
             r = -ENOMEM;
         }
         if (p != vq->ring) {
-            fprintf(stderr, "Ring buffer relocated for ring %d\n", i);
+            error_report("Ring buffer relocated for ring %d", i);
             r = -EBUSY;
         }
         cpu_physical_memory_unmap(p, l, 0, 0);
@@ -1050,8 +1050,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     }
 
     if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
-        fprintf(stderr, "vhost backend memory slots limit is less"
-                " than current number of present memory slots\n");
+        error_report("vhost backend memory slots limit is less"
+                " than current number of present memory slots");
         r = -1;
         goto fail;
     }
@@ -1174,8 +1174,9 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     VirtioBusState *vbus = VIRTIO_BUS(qbus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
     int i, r, e;
+
     if (!k->ioeventfd_started) {
-        fprintf(stderr, "binding does not support host notifiers\n");
+        error_report("binding does not support host notifiers");
         r = -ENOSYS;
         goto fail;
     }
@@ -1184,7 +1185,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          true);
         if (r < 0) {
-            fprintf(stderr, "vhost VQ %d notifier binding failed: %d\n", i, -r);
+            error_report("vhost VQ %d notifier binding failed: %d", i, -r);
             goto fail_vq;
         }
     }
@@ -1195,8 +1196,7 @@ fail_vq:
         e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          false);
         if (e < 0) {
-            fprintf(stderr, "vhost VQ %d notifier cleanup error: %d\n", i, -r);
-            fflush(stderr);
+            error_report("vhost VQ %d notifier cleanup error: %d", i, -r);
         }
         assert (e >= 0);
     }
@@ -1218,8 +1218,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          false);
         if (r < 0) {
-            fprintf(stderr, "vhost VQ %d notifier cleanup failed: %d\n", i, -r);
-            fflush(stderr);
+            error_report("vhost VQ %d notifier cleanup failed: %d", i, -r);
         }
         assert (r >= 0);
     }
-- 
2.9.0

[Qemu-devel] [PATCH v6 14/33] qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Calling qemu_chr_fe_set_msgfds() on unconnected socket leads to crash
since s->ioc is NULL in this case. Return an error earlier instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e4b8448..1274f50 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2760,14 +2760,16 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num)
 {
     TCPCharDriver *s = chr->opaque;
 
-    if (!qio_channel_has_feature(s->ioc,
-                                 QIO_CHANNEL_FEATURE_FD_PASS)) {
-        return -1;
-    }
     /* clear old pending fd array */
     g_free(s->write_msgfds);
     s->write_msgfds = NULL;
 
+    if (!s->connected ||
+        !qio_channel_has_feature(s->ioc,
+                                 QIO_CHANNEL_FEATURE_FD_PASS)) {
+        return -1;
+    }
+
     if (num) {
         s->write_msgfds = g_new(int, num);
         memcpy(s->write_msgfds, fds, num * sizeof(int));
-- 
2.9.0

[Qemu-devel] [PATCH v6 15/33] vhost-user: call set_msgfds unconditionally

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

It is fine to call set_msgfds() with 0 fd, and ensures any previous fd
array is cleared.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..f01b92f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,9 +187,7 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
         return 0;
     }
 
-    if (fd_num) {
-        qemu_chr_fe_set_msgfds(chr, fds, fd_num);
-    }
+    qemu_chr_fe_set_msgfds(chr, fds, fd_num);
 
     return qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size) == size ?
             0 : -1;
-- 
2.9.0

[Qemu-devel] [PATCH v6 16/33] vhost-user: check qemu_chr_fe_set_msgfds() return value

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check qemu_chr_fe_set_msgfds() for errors, to make sure the message to
be sent is correct.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f01b92f..5dae496 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,7 +187,9 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
         return 0;
     }
 
-    qemu_chr_fe_set_msgfds(chr, fds, fd_num);
+    if (qemu_chr_fe_set_msgfds(chr, fds, fd_num) < 0) {
+        return -1;
+    }
 
     return qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size) == size ?
             0 : -1;
-- 
2.9.0

[Qemu-devel] [PATCH v6 17/33] vhost-user: check vhost_user_{read, write}() return value

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The vhost-user code is quite inconsistent with error handling. Instead
of ignoring some return values of read/write and silently going on with
invalid state (invalid read for example), break the code flow when the
error happened.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-user.c | 50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5dae496..819481d 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -214,12 +214,14 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
         fds[fd_num++] = log->fd;
     }
 
-    vhost_user_write(dev, &msg, fds, fd_num);
+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
+        return -1;
+    }
 
     if (shmfd) {
         msg.size = 0;
         if (vhost_user_read(dev, &msg) < 0) {
-            return 0;
+            return -1;
         }
 
         if (msg.request != VHOST_USER_SET_LOG_BASE) {
@@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     msg.size += sizeof(msg.payload.memory.padding);
     msg.size += fd_num * sizeof(VhostUserMemoryRegion);
 
-    vhost_user_write(dev, &msg, fds, fd_num);
+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
         .size = sizeof(msg.payload.addr),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev,
         .size = sizeof(msg.payload.state),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -360,10 +368,12 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
         .size = sizeof(msg.payload.state),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     if (vhost_user_read(dev, &msg) < 0) {
-        return 0;
+        return -1;
     }
 
     if (msg.request != VHOST_USER_GET_VRING_BASE) {
@@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
         msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
     }
 
-    vhost_user_write(dev, &msg, fds, fd_num);
+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
         .size = sizeof(msg.payload.u64),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -455,10 +469,12 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
         return 0;
     }
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     if (vhost_user_read(dev, &msg) < 0) {
-        return 0;
+        return -1;
     }
 
     if (msg.request != request) {
@@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
         .flags = VHOST_USER_VERSION,
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev *dev)
         .flags = VHOST_USER_VERSION,
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -588,7 +608,6 @@ static bool vhost_user_requires_shm_log(struct vhost_dev *dev)
 static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
 {
     VhostUserMsg msg = { 0 };
-    int err;
 
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
 
@@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
         memcpy((char *)&msg.payload.u64, mac_addr, 6);
         msg.size = sizeof(msg.payload.u64);
 
-        err = vhost_user_write(dev, &msg, NULL, 0);
-        return err;
+        return vhost_user_write(dev, &msg, NULL, 0);
     }
     return -1;
 }
-- 
2.9.0

[Qemu-devel] [PATCH v6 18/33] vhost-user: keep vhost_net after a disconnection

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Many code paths assume get_vhost_net() returns non-null.

Keep VhostUserState.vhost_net after a successful vhost_net_init(),
instead of freeing it in vhost_net_cleanup().

VhostUserState.vhost_net is thus freed before after being recreated or
on final vhost_user_cleanup() and there is no need to save the acked
features.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c |  1 -
 net/tap.c          |  1 +
 net/vhost-user.c   | 36 +++++++++++++++++++-----------------
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index c11f69c..7b76591 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -378,7 +378,6 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
 void vhost_net_cleanup(struct vhost_net *net)
 {
     vhost_dev_cleanup(&net->dev);
-    g_free(net);
 }
 
 int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
diff --git a/net/tap.c b/net/tap.c
index 40a8c74..6abb962 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -312,6 +312,7 @@ static void tap_cleanup(NetClientState *nc)
 
     if (s->vhost_net) {
         vhost_net_cleanup(s->vhost_net);
+        g_free(s->vhost_net);
         s->vhost_net = NULL;
     }
 
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 2cac32e..d2a984d 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -45,11 +45,6 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
     return s->acked_features;
 }
 
-static int vhost_user_running(VhostUserState *s)
-{
-    return (s->vhost_net) ? 1 : 0;
-}
-
 static void vhost_user_stop(int queues, NetClientState *ncs[])
 {
     VhostUserState *s;
@@ -59,15 +54,14 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
         assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
 
         s = DO_UPCAST(VhostUserState, nc, ncs[i]);
-        if (!vhost_user_running(s)) {
-            continue;
-        }
 
         if (s->vhost_net) {
             /* save acked features */
-            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
+            uint64_t features = vhost_net_get_acked_features(s->vhost_net);
+            if (features) {
+                s->acked_features = features;
+            }
             vhost_net_cleanup(s->vhost_net);
-            s->vhost_net = NULL;
         }
     }
 }
@@ -75,6 +69,7 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
 static int vhost_user_start(int queues, NetClientState *ncs[])
 {
     VhostNetOptions options;
+    struct vhost_net *net = NULL;
     VhostUserState *s;
     int max_queues;
     int i;
@@ -85,33 +80,39 @@ static int vhost_user_start(int queues, NetClientState *ncs[])
         assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
 
         s = DO_UPCAST(VhostUserState, nc, ncs[i]);
-        if (vhost_user_running(s)) {
-            continue;
-        }
 
         options.net_backend = ncs[i];
         options.opaque      = s->chr;
         options.busyloop_timeout = 0;
-        s->vhost_net = vhost_net_init(&options);
-        if (!s->vhost_net) {
+        net = vhost_net_init(&options);
+        if (!net) {
             error_report("failed to init vhost_net for queue %d", i);
             goto err;
         }
 
         if (i == 0) {
-            max_queues = vhost_net_get_max_queues(s->vhost_net);
+            max_queues = vhost_net_get_max_queues(net);
             if (queues > max_queues) {
                 error_report("you are asking more queues than supported: %d",
                              max_queues);
                 goto err;
             }
         }
+
+        if (s->vhost_net) {
+            vhost_net_cleanup(s->vhost_net);
+            g_free(s->vhost_net);
+        }
+        s->vhost_net = net;
     }
 
     return 0;
 
 err:
-    vhost_user_stop(i + 1, ncs);
+    if (net) {
+        vhost_net_cleanup(net);
+    }
+    vhost_user_stop(i, ncs);
     return -1;
 }
 
@@ -150,6 +151,7 @@ static void vhost_user_cleanup(NetClientState *nc)
 
     if (s->vhost_net) {
         vhost_net_cleanup(s->vhost_net);
+        g_free(s->vhost_net);
         s->vhost_net = NULL;
     }
     if (s->chr) {
-- 
2.9.0

[Qemu-devel] [PATCH v6 19/33] vhost-user: add get_vhost_net() assertions

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add a few assertions to be more explicit about the runtime behaviour
after the previous patch: get_vhost_net() is non-null after
net_vhost_user_init().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 1 +
 net/vhost-user.c   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 7b76591..4e6495e 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -417,6 +417,7 @@ VHostNetState *get_vhost_net(NetClientState *nc)
         break;
     case NET_CLIENT_DRIVER_VHOST_USER:
         vhost_net = vhost_user_get_vhost_net(nc);
+        assert(vhost_net);
         break;
     default:
         break;
diff --git a/net/vhost-user.c b/net/vhost-user.c
index d2a984d..39987a3 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -259,6 +259,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
 
     qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
 
+    assert(s->vhost_net);
+
     return 0;
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 20/33] Revert "vhost-net: do not crash if backend is not present"

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Now that get_vhost_net() returns non-null after a successful
vhost_net_init(), we no longer need to check this case.

This reverts commit ecd34898596c60f79886061618dd7e01001113ad.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 4e6495e..54cf015 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -429,15 +429,10 @@ VHostNetState *get_vhost_net(NetClientState *nc)
 int vhost_set_vring_enable(NetClientState *nc, int enable)
 {
     VHostNetState *net = get_vhost_net(nc);
-    const VhostOps *vhost_ops;
+    const VhostOps *vhost_ops = net->dev.vhost_ops;
 
     nc->vring_enable = enable;
 
-    if (!net) {
-        return 0;
-    }
-
-    vhost_ops = net->dev.vhost_ops;
     if (vhost_ops->vhost_set_vring_enable) {
         return vhost_ops->vhost_set_vring_enable(&net->dev, enable);
     }
-- 
2.9.0

[Qemu-devel] [PATCH v6 21/33] vhost-net: vhost_migration_done is vhost-user specific

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Either the callback is mandatory to implement, in which case an assert()
is more appropriate, or it's not and we can't tell much whether the
function should fail or not (given it's name, I guess it should silently
success by default). Instead, make the implementation mandatory and
vhost-user specific to be more clear about its usage.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 54cf015..dd41a8e 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -383,13 +383,11 @@ void vhost_net_cleanup(struct vhost_net *net)
 int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
 {
     const VhostOps *vhost_ops = net->dev.vhost_ops;
-    int r = -1;
 
-    if (vhost_ops->vhost_migration_done) {
-        r = vhost_ops->vhost_migration_done(&net->dev, mac_addr);
-    }
+    assert(vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
+    assert(vhost_ops->vhost_migration_done);
 
-    return r;
+    return vhost_ops->vhost_migration_done(&net->dev, mac_addr);
 }
 
 bool vhost_net_virtqueue_pending(VHostNetState *net, int idx)
-- 
2.9.0

[Qemu-devel] [PATCH v6 22/33] vhost: add assert() to check runtime behaviour

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

All these functions must be called only after the backend is connected.
They are called from virtio-net.c, after either virtio or link status
change.

The check for nc->peer->link_down should ensure vhost_net_{start,stop}()
are always called between vhost_user_{start,stop}().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index bb886f3..2d0d1d1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1242,6 +1242,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
     int r, index = n - hdev->vq_index;
     struct vhost_vring_file file;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops);
+
     if (mask) {
         assert(vdev->use_guest_notifier_mask);
         file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier);
@@ -1288,6 +1291,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i, r;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops);
+
     hdev->started = true;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);
@@ -1350,6 +1356,9 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops);
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_stop(hdev,
                              vdev,
-- 
2.9.0

[Qemu-devel] [PATCH v6 23/33] char: add chr_wait_connected callback

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

A function to wait on the backend to be connected, to be used in the
following patches.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/char.h | 8 ++++++++
 qemu-char.c           | 9 +++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0ea9eac..ee7e554 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -70,6 +70,7 @@ struct CharDriverState {
     int (*get_msgfds)(struct CharDriverState *s, int* fds, int num);
     int (*set_msgfds)(struct CharDriverState *s, int *fds, int num);
     int (*chr_add_client)(struct CharDriverState *chr, int fd);
+    int (*chr_wait_connected)(struct CharDriverState *chr, Error **errp);
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -158,6 +159,13 @@ void qemu_chr_disconnect(CharDriverState *chr);
  */
 void qemu_chr_cleanup(void);
 
+/**
+ * @qemu_chr_wait_connected:
+ *
+ * Wait for characted backend to be connected.
+ */
+int qemu_chr_wait_connected(CharDriverState *chr, Error **errp);
+
 /**
  * @qemu_chr_new_noreplay:
  *
diff --git a/qemu-char.c b/qemu-char.c
index 1274f50..6eba615 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3139,6 +3139,15 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
     return TRUE;
 }
 
+int qemu_chr_wait_connected(CharDriverState *chr, Error **errp)
+{
+    if (chr->chr_wait_connected) {
+        return chr->chr_wait_connected(chr, errp);
+    }
+
+    return 0;
+}
+
 static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
-- 
2.9.0

[Qemu-devel] [PATCH v6 24/33] char: add and use tcp_chr_wait_connected

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add a chr_wait_connected for the tcp backend, and use it in the
open_socket() function.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 6eba615..a50b8fb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3139,6 +3139,32 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
     return TRUE;
 }
 
+static int tcp_chr_wait_connected(CharDriverState *chr, Error **errp)
+{
+    TCPCharDriver *s = chr->opaque;
+    QIOChannelSocket *sioc;
+
+    while (!s->connected) {
+        if (s->is_listen) {
+            fprintf(stderr, "QEMU waiting for connection on: %s\n",
+                    chr->filename);
+            qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
+            tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
+            qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
+        } else {
+            sioc = qio_channel_socket_new();
+            if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+                object_unref(OBJECT(sioc));
+                return -1;
+            }
+            tcp_chr_new_client(chr, sioc);
+            object_unref(OBJECT(sioc));
+        }
+    }
+
+    return 0;
+}
+
 int qemu_chr_wait_connected(CharDriverState *chr, Error **errp)
 {
     if (chr->chr_wait_connected) {
@@ -4402,6 +4428,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
     s->addr = QAPI_CLONE(SocketAddress, sock->addr);
 
     chr->opaque = s;
+    chr->chr_wait_connected = tcp_chr_wait_connected;
     chr->chr_write = tcp_chr_write;
     chr->chr_sync_read = tcp_chr_sync_read;
     chr->chr_close = tcp_chr_close;
@@ -4425,32 +4452,30 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
         s->reconnect_time = reconnect;
     }
 
-    sioc = qio_channel_socket_new();
     if (s->reconnect_time) {
+        sioc = qio_channel_socket_new();
         qio_channel_socket_connect_async(sioc, s->addr,
                                          qemu_chr_socket_connected,
                                          chr, NULL);
-    } else if (s->is_listen) {
-        if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
-            goto error;
-        }
-        s->listen_ioc = sioc;
-        if (is_waitconnect) {
-            fprintf(stderr, "QEMU waiting for connection on: %s\n",
-                    chr->filename);
-            tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
-        }
-        qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
-        if (!s->ioc) {
-            s->listen_tag = qio_channel_add_watch(
-                QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
-        }
     } else {
-        if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+        if (s->is_listen) {
+            sioc = qio_channel_socket_new();
+            if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
+                goto error;
+            }
+            s->listen_ioc = sioc;
+            if (is_waitconnect &&
+                qemu_chr_wait_connected(chr, errp) < 0) {
+                goto error;
+            }
+            if (!s->ioc) {
+                s->listen_tag = qio_channel_add_watch(
+                    QIO_CHANNEL(s->listen_ioc), G_IO_IN,
+                    tcp_chr_accept, chr, NULL);
+            }
+        } else if (qemu_chr_wait_connected(chr, errp) < 0) {
             goto error;
         }
-        tcp_chr_new_client(chr, sioc);
-        object_unref(OBJECT(sioc));
     }
 
     return chr;
-- 
2.9.0

[Qemu-devel] [PATCH v6 25/33] vhost-user: wait until backend init is completed

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The chardev waits for an initial connection before starting qemu, and
vhost-user should wait for the backend negotiation to be completed
before starting qemu too.

vhost-user is started in the net_vhost_user_event callback, which is
synchronously called after the socket is connected. Use a
VhostUserState.started flag to indicate vhost-user init completed
successfully and qemu can be started.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 39987a3..b0595f8 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -24,6 +24,7 @@ typedef struct VhostUserState {
     VHostNetState *vhost_net;
     guint watch;
     uint64_t acked_features;
+    bool started;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -220,6 +221,7 @@ static void net_vhost_user_event(void *opaque, int event)
             return;
         }
         qmp_set_link(name, true, &err);
+        s->started = true;
         break;
     case CHR_EVENT_CLOSED:
         qmp_set_link(name, false, &err);
@@ -238,7 +240,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
                                const char *name, CharDriverState *chr,
                                int queues)
 {
-    NetClientState *nc;
+    NetClientState *nc, *nc0 = NULL;
     VhostUserState *s;
     int i;
 
@@ -247,6 +249,9 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
 
     for (i = 0; i < queues; i++) {
         nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
+        if (!nc0) {
+            nc0 = nc;
+        }
 
         snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
                  i, chr->label);
@@ -257,7 +262,16 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
         s->chr = chr;
     }
 
-    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
+    s = DO_UPCAST(VhostUserState, nc, nc0);
+    do {
+        Error *err = NULL;
+        if (qemu_chr_wait_connected(chr, &err) < 0) {
+            error_report_err(err);
+            return -1;
+        }
+        qemu_chr_add_handlers(chr, NULL, NULL,
+                              net_vhost_user_event, nc0->name);
+    } while (!s->started);
 
     assert(s->vhost_net);
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 26/33] tests: plug some leaks in virtio-net-test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Found thanks to valgrind.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/virtio-net-test.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index a34a939..361506f 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -149,6 +149,7 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     char test[] = "TEST";
     char buffer[64];
     int len = htonl(sizeof(test));
+    QDict *rsp;
     struct iovec iov[] = {
         {
             .iov_base = &len,
@@ -165,7 +166,8 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     free_head = qvirtqueue_add(vq, req_addr, 64, true, false);
     qvirtqueue_kick(bus, dev, vq, free_head);
 
-    qmp("{ 'execute' : 'stop'}");
+    rsp = qmp("{ 'execute' : 'stop'}");
+    QDECREF(rsp);
 
     ret = iov_send(socket, iov, 2, 0, sizeof(len) + sizeof(test));
     g_assert_cmpint(ret, ==, sizeof(test) + sizeof(len));
@@ -173,8 +175,10 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     /* We could check the status, but this command is more importantly to
      * ensure the packet data gets queued in QEMU, before we do 'cont'.
      */
-    qmp("{ 'execute' : 'query-status'}");
-    qmp("{ 'execute' : 'cont'}");
+    rsp = qmp("{ 'execute' : 'query-status'}");
+    QDECREF(rsp);
+    rsp = qmp("{ 'execute' : 'cont'}");
+    QDECREF(rsp);
 
     qvirtio_wait_queue_isr(bus, dev, vq, QVIRTIO_NET_TIMEOUT_US);
     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
@@ -230,8 +234,10 @@ static void pci_basic(gconstpointer data)
     /* End test */
     close(sv[0]);
     qvirtqueue_cleanup(&qvirtio_pci, &tx->vq, alloc);
+    qvirtqueue_cleanup(&qvirtio_pci, &rx->vq, alloc);
     pc_alloc_uninit(alloc);
     qvirtio_pci_device_disable(dev);
+    g_free(dev->pdev);
     g_free(dev);
     qpci_free_pc(bus);
     test_end();
-- 
2.9.0

[Qemu-devel] [PATCH v6 27/33] tests: fix vhost-user-test leak

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Spotted by valgrind.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 46d0588..27b10c1 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -683,6 +683,7 @@ static void test_reconnect(void)
                                   qtest_get_arch());
     g_test_trap_subprocess(path, 0, 0);
     g_test_trap_assert_passed();
+    g_free(path);
 }
 #endif
 
-- 
2.9.0

[Qemu-devel] [PATCH v6 28/33] tests: add /vhost-user/connect-fail test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check early connection failure and resume.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-test.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 27b10c1..2db8f3d 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -131,6 +131,7 @@ typedef struct TestServer {
     CompatGCond data_cond;
     int log_fd;
     uint64_t rings;
+    bool test_fail;
 } TestServer;
 
 static const char *tmpfs;
@@ -221,6 +222,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     uint8_t *p = (uint8_t *) &msg;
     int fd;
 
+    if (s->test_fail) {
+        qemu_chr_disconnect(chr);
+        /* now switch to non-failure */
+        s->test_fail = false;
+    }
+
     if (size != VHOST_USER_HDR_SIZE) {
         g_test_message("Wrong message size received %d\n", size);
         return;
@@ -685,6 +692,34 @@ static void test_reconnect(void)
     g_test_trap_assert_passed();
     g_free(path);
 }
+
+static void test_connect_fail_subprocess(void)
+{
+    TestServer *s = test_server_new("connect-fail");
+    char *cmd;
+
+    s->test_fail = true;
+    g_thread_new("connect", connect_thread, s);
+    cmd = GET_QEMU_CMDE(s, 2, ",server", "");
+    qtest_start(cmd);
+    g_free(cmd);
+
+    wait_for_fds(s);
+    wait_for_rings_started(s, 2);
+
+    qtest_end();
+    test_server_free(s);
+}
+
+static void test_connect_fail(void)
+{
+    gchar *path = g_strdup_printf("/%s/vhost-user/connect-fail/subprocess",
+                                  qtest_get_arch());
+    g_test_trap_subprocess(path, 0, 0);
+    g_test_trap_assert_passed();
+    g_free(path);
+}
+
 #endif
 
 int main(int argc, char **argv)
@@ -735,6 +770,9 @@ int main(int argc, char **argv)
     qtest_add_func("/vhost-user/reconnect/subprocess",
                    test_reconnect_subprocess);
     qtest_add_func("/vhost-user/reconnect", test_reconnect);
+    qtest_add_func("/vhost-user/connect-fail/subprocess",
+                   test_connect_fail_subprocess);
+    qtest_add_func("/vhost-user/connect-fail", test_connect_fail);
 #endif
 
     ret = g_test_run();
-- 
2.9.0

[Qemu-devel] [PATCH v6 29/33] tests: add a simple /vhost-user/multiqueue test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This test just checks that 2 virtio-net queues can be setup over
vhost-user and waits for them to be started.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/Makefile.include  |   2 +-
 tests/vhost-user-test.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 9286148..a6ee20a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -617,7 +617,7 @@ tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
 tests/postcopy-test$(EXESUF): tests/postcopy-test.o
-tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) $(test-io-obj-y)
+tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) $(test-io-obj-y) $(libqos-pc-obj-y) $(libqos-virtio-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 2db8f3d..9c1438d 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -17,6 +17,11 @@
 #include "sysemu/char.h"
 #include "sysemu/sysemu.h"
 
+#include "libqos/pci-pc.h"
+#include "libqos/virtio-pci.h"
+#include "libqos/malloc-pc.h"
+#include "hw/virtio/virtio-net.h"
+
 #include <linux/vhost.h>
 #include <sys/vfs.h>
 
@@ -46,6 +51,7 @@
 #define VHOST_MEMORY_MAX_NREGIONS    8
 
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_F_MQ 0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
 
 #define VHOST_LOG_PAGE 0x1000
@@ -68,6 +74,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_VRING_ERR = 14,
     VHOST_USER_GET_PROTOCOL_FEATURES = 15,
     VHOST_USER_SET_PROTOCOL_FEATURES = 16,
+    VHOST_USER_GET_QUEUE_NUM = 17,
     VHOST_USER_SET_VRING_ENABLE = 18,
     VHOST_USER_MAX
 } VhostUserRequest;
@@ -132,6 +139,7 @@ typedef struct TestServer {
     int log_fd;
     uint64_t rings;
     bool test_fail;
+    int queues;
 } TestServer;
 
 static const char *tmpfs;
@@ -253,6 +261,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         msg.size = sizeof(m.payload.u64);
         msg.payload.u64 = 0x1ULL << VHOST_F_LOG_ALL |
             0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+        if (s->queues > 1) {
+            msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
+        }
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
@@ -267,6 +278,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.payload.u64);
         msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
+        if (s->queues > 1) {
+            msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ;
+        }
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
@@ -279,7 +293,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
 
-        assert(msg.payload.state.index < 2);
+        assert(msg.payload.state.index < s->queues * 2);
         s->rings &= ~(0x1ULL << msg.payload.state.index);
         break;
 
@@ -319,10 +333,18 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         break;
 
     case VHOST_USER_SET_VRING_BASE:
-        assert(msg.payload.state.index < 2);
+        assert(msg.payload.state.index < s->queues * 2);
         s->rings |= 0x1ULL << msg.payload.state.index;
         break;
 
+    case VHOST_USER_GET_QUEUE_NUM:
+        msg.flags |= VHOST_USER_REPLY_MASK;
+        msg.size = sizeof(m.payload.u64);
+        msg.payload.u64 = s->queues;
+        p = (uint8_t *) &msg;
+        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+        break;
+
     default:
         break;
     }
@@ -369,6 +391,7 @@ static TestServer *test_server_new(const gchar *name)
     g_cond_init(&server->data_cond);
 
     server->log_fd = -1;
+    server->queues = 1;
 
     return server;
 }
@@ -722,6 +745,86 @@ static void test_connect_fail(void)
 
 #endif
 
+static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
+{
+    QVirtioPCIDevice *dev;
+
+    dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET);
+    g_assert(dev != NULL);
+    g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_NET);
+
+    qvirtio_pci_device_enable(dev);
+    qvirtio_reset(&qvirtio_pci, &dev->vdev);
+    qvirtio_set_acknowledge(&qvirtio_pci, &dev->vdev);
+    qvirtio_set_driver(&qvirtio_pci, &dev->vdev);
+
+    return dev;
+}
+
+static void driver_init(const QVirtioBus *bus, QVirtioDevice *dev)
+{
+    uint32_t features;
+
+    features = qvirtio_get_features(bus, dev);
+    features = features & ~(QVIRTIO_F_BAD_FEATURE |
+                            (1u << VIRTIO_RING_F_INDIRECT_DESC) |
+                            (1u << VIRTIO_RING_F_EVENT_IDX));
+    qvirtio_set_features(bus, dev, features);
+
+    qvirtio_set_driver_ok(bus, dev);
+}
+
+#define PCI_SLOT                0x04
+
+static void test_multiqueue(void)
+{
+    const int queues = 2;
+    TestServer *s = test_server_new("mq");
+    QVirtioPCIDevice *dev;
+    QPCIBus *bus;
+    QVirtQueuePCI *vq[queues * 2];
+    QGuestAllocator *alloc;
+    char *cmd;
+    int i;
+
+    s->queues = queues;
+    test_server_listen(s);
+
+    cmd = g_strdup_printf(QEMU_CMD_ACCEL QEMU_CMD_MEM QEMU_CMD_CHR
+                          QEMU_CMD_NETDEV ",queues=%d "
+                          "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d",
+                          512, 512, root, s->chr_name,
+                          s->socket_path, "", s->chr_name,
+                          queues, queues * 2 + 2);
+    qtest_start(cmd);
+    g_free(cmd);
+
+    bus = qpci_init_pc();
+    dev = virtio_net_pci_init(bus, PCI_SLOT);
+
+    alloc = pc_alloc_init();
+    for (i = 0; i < queues * 2; i++) {
+        vq[i] = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev,
+                                              alloc, i);
+    }
+
+    driver_init(&qvirtio_pci, &dev->vdev);
+    wait_for_rings_started(s, queues * 2);
+
+    /* End test */
+    for (i = 0; i < queues * 2; i++) {
+        qvirtqueue_cleanup(&qvirtio_pci, &vq[i]->vq, alloc);
+    }
+    pc_alloc_uninit(alloc);
+    qvirtio_pci_device_disable(dev);
+    g_free(dev->pdev);
+    g_free(dev);
+    qpci_free_pc(bus);
+    qtest_end();
+
+    test_server_free(s);
+}
+
 int main(int argc, char **argv)
 {
     QTestState *s = NULL;
@@ -766,6 +869,7 @@ int main(int argc, char **argv)
 
     qtest_add_data_func("/vhost-user/read-guest-mem", server, read_guest_mem);
     qtest_add_func("/vhost-user/migrate", test_migrate);
+    qtest_add_func("/vhost-user/multiqueue", test_multiqueue);
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
     qtest_add_func("/vhost-user/reconnect/subprocess",
                    test_reconnect_subprocess);
-- 
2.9.0

[Qemu-devel] [PATCH v6 30/33] vhost-user: add error report in vhost_user_write()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Similar to vhost_user_read() error report, it is useful to have early
error report.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-user.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 819481d..1995fd2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -176,7 +176,7 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
                             int *fds, int fd_num)
 {
     CharDriverState *chr = dev->opaque;
-    int size = VHOST_USER_HDR_SIZE + msg->size;
+    int ret, size = VHOST_USER_HDR_SIZE + msg->size;
 
     /*
      * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
@@ -188,11 +188,18 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
     }
 
     if (qemu_chr_fe_set_msgfds(chr, fds, fd_num) < 0) {
+        error_report("Failed to set msg fds.");
         return -1;
     }
 
-    return qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size) == size ?
-            0 : -1;
+    ret = qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size);
+    if (ret != size) {
+        error_report("Failed to write msg."
+                     " Wrote %d instead of %d.", ret, size);
+        return -1;
+    }
+
+    return 0;
 }
 
 static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
-- 
2.9.0

[Qemu-devel] [PATCH v6 31/33] vhost: add vhost_net_set_backend()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Not all vhost-user backends support ops->vhost_net_set_backend(). It is
a nicer to provide an assert/error than to crash trying to
call. Furthermore, it improves a bit the code by hiding vhost_ops
details.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c        |  9 +++------
 hw/virtio/vhost.c         | 10 ++++++++++
 include/hw/virtio/vhost.h |  4 ++++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index dd41a8e..dc61dc1 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -242,8 +242,7 @@ static int vhost_net_start_one(struct vhost_net *net,
         qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
         file.fd = net->backend;
         for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
+            r = vhost_net_set_backend(&net->dev, &file);
             if (r < 0) {
                 r = -errno;
                 goto fail;
@@ -255,8 +254,7 @@ fail:
     file.fd = -1;
     if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         while (file.index-- > 0) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            int r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
+            int r = vhost_net_set_backend(&net->dev, &file);
             assert(r >= 0);
         }
     }
@@ -277,8 +275,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
 
     if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            int r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
+            int r = vhost_net_set_backend(&net->dev, &file);
             assert(r >= 0);
         }
     }
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2d0d1d1..b0e8ecc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1369,3 +1369,13 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
     vhost_log_put(hdev, true);
     hdev->started = false;
 }
+
+int vhost_net_set_backend(struct vhost_dev *hdev,
+                          struct vhost_vring_file *file)
+{
+    if (hdev->vhost_ops->vhost_net_set_backend) {
+        return hdev->vhost_ops->vhost_net_set_backend(hdev, file);
+    }
+
+    return -1;
+}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 2106ed8..e433089 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -86,4 +86,8 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                         uint64_t features);
 bool vhost_has_free_slot(void);
+
+int vhost_net_set_backend(struct vhost_dev *hdev,
+                          struct vhost_vring_file *file);
+
 #endif
-- 
2.9.0

[Qemu-devel] [PATCH v6 32/33] vhost-user-test: add flags mismatch test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check that qemu disconnects the backend that doesn't have the previously
acked features.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-test.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 9c1438d..8e2b63c 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -126,6 +126,13 @@ static VhostUserMsg m __attribute__ ((unused));
 #define VHOST_USER_VERSION    (0x1)
 /*****************************************************************************/
 
+enum {
+    TEST_FLAGS_OK,
+    TEST_FLAGS_DISCONNECT,
+    TEST_FLAGS_BAD,
+    TEST_FLAGS_END,
+};
+
 typedef struct TestServer {
     gchar *socket_path;
     gchar *mig_path;
@@ -139,6 +146,7 @@ typedef struct TestServer {
     int log_fd;
     uint64_t rings;
     bool test_fail;
+    int test_flags;
     int queues;
 } TestServer;
 
@@ -264,6 +272,10 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         if (s->queues > 1) {
             msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
         }
+        if (s->test_flags >= TEST_FLAGS_BAD) {
+            msg.payload.u64 = 0;
+            s->test_flags = TEST_FLAGS_END;
+        }
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
@@ -271,6 +283,10 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     case VHOST_USER_SET_FEATURES:
 	g_assert_cmpint(msg.payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES),
 			!=, 0ULL);
+        if (s->test_flags == TEST_FLAGS_DISCONNECT) {
+            qemu_chr_disconnect(chr);
+            s->test_flags = TEST_FLAGS_BAD;
+        }
         break;
 
     case VHOST_USER_GET_PROTOCOL_FEATURES:
@@ -396,6 +412,16 @@ static TestServer *test_server_new(const gchar *name)
     return server;
 }
 
+static void chr_event(void *opaque, int event)
+{
+    TestServer *s = opaque;
+
+    if (s->test_flags == TEST_FLAGS_END &&
+        event == CHR_EVENT_CLOSED) {
+        s->test_flags = TEST_FLAGS_OK;
+    }
+}
+
 static void test_server_create_chr(TestServer *server, const gchar *opt)
 {
     gchar *chr_path;
@@ -404,7 +430,8 @@ static void test_server_create_chr(TestServer *server, const gchar *opt)
     server->chr = qemu_chr_new(server->chr_name, chr_path, NULL);
     g_free(chr_path);
 
-    qemu_chr_add_handlers(server->chr, chr_can_read, chr_read, NULL, server);
+    qemu_chr_add_handlers(server->chr, chr_can_read, chr_read,
+                          chr_event, server);
 }
 
 static void test_server_listen(TestServer *server)
@@ -743,6 +770,33 @@ static void test_connect_fail(void)
     g_free(path);
 }
 
+static void test_flags_mismatch_subprocess(void)
+{
+    TestServer *s = test_server_new("flags-mismatch");
+    char *cmd;
+
+    s->test_flags = TEST_FLAGS_DISCONNECT;
+    g_thread_new("connect", connect_thread, s);
+    cmd = GET_QEMU_CMDE(s, 2, ",server", "");
+    qtest_start(cmd);
+    g_free(cmd);
+
+    wait_for_fds(s);
+    wait_for_rings_started(s, 2);
+
+    qtest_end();
+    test_server_free(s);
+}
+
+static void test_flags_mismatch(void)
+{
+    gchar *path = g_strdup_printf("/%s/vhost-user/flags-mismatch/subprocess",
+                                  qtest_get_arch());
+    g_test_trap_subprocess(path, 0, 0);
+    g_test_trap_assert_passed();
+    g_free(path);
+}
+
 #endif
 
 static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
@@ -877,6 +931,9 @@ int main(int argc, char **argv)
     qtest_add_func("/vhost-user/connect-fail/subprocess",
                    test_connect_fail_subprocess);
     qtest_add_func("/vhost-user/connect-fail", test_connect_fail);
+    qtest_add_func("/vhost-user/flags-mismatch/subprocess",
+                   test_flags_mismatch_subprocess);
+    qtest_add_func("/vhost-user/flags-mismatch", test_flags_mismatch);
 #endif
 
     ret = g_test_run();
-- 
2.9.0

[Qemu-devel] [PATCH v6 33/33] RFC: vhost: do not update last avail idx on get_vring_base() failure

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The state.num value will probably be 0 in this case, but I guess that
doesn't make sense to update.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index b0e8ecc..3d0c807 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -945,8 +945,9 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
     r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
     if (r < 0) {
         VHOST_OPS_DEBUG("vhost VQ %d ring restore failed: %d", idx, r);
+    } else {
+        virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     }
-    virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     virtio_queue_invalidate_signalled_used(vdev, idx);
 
     /* In the cross-endian case, we need to reset the vring endianness to
-- 
2.9.0

Re: [Qemu-devel] [PATCH v6 29/33] tests: add a simple /vhost-user/multiqueue test

[Marc-André Lureau]

Hi

I just realized that the travis build failed, because of
wait_for_rings_started is under CONFIG_HAS_GLIB_SUBPROCESS_TESTS, the
fix in this patch is:

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 8e2b63c..82dfd0d 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -669,7 +669,6 @@ static void test_migrate(void)
     global_qtest = global;
 }

-#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
 static void wait_for_rings_started(TestServer *s, size_t count)
 {
     gint64 end_time;
@@ -687,6 +686,7 @@ static void wait_for_rings_started(TestServer *s,
size_t count)
     g_mutex_unlock(&s->data_mutex);
 }

+#ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
 static gboolean
 reconnect_cb(gpointer user_data)
 {

On Wed, Jul 27, 2016 at 1:15 AM,  <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This test just checks that 2 virtio-net queues can be setup over
> vhost-user and waits for them to be started.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/Makefile.include  |   2 +-
>  tests/vhost-user-test.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 107 insertions(+), 3 deletions(-)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 9286148..a6ee20a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -617,7 +617,7 @@ tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
>  tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
>  tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
>  tests/postcopy-test$(EXESUF): tests/postcopy-test.o
> -tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) $(test-io-obj-y)
> +tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) $(test-io-obj-y) $(libqos-pc-obj-y) $(libqos-virtio-obj-y)
>  tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
>  tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
>  tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y)
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 2db8f3d..9c1438d 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -17,6 +17,11 @@
>  #include "sysemu/char.h"
>  #include "sysemu/sysemu.h"
>
> +#include "libqos/pci-pc.h"
> +#include "libqos/virtio-pci.h"
> +#include "libqos/malloc-pc.h"
> +#include "hw/virtio/virtio-net.h"
> +
>  #include <linux/vhost.h>
>  #include <sys/vfs.h>
>
> @@ -46,6 +51,7 @@
>  #define VHOST_MEMORY_MAX_NREGIONS    8
>
>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
> +#define VHOST_USER_PROTOCOL_F_MQ 0
>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
>
>  #define VHOST_LOG_PAGE 0x1000
> @@ -68,6 +74,7 @@ typedef enum VhostUserRequest {
>      VHOST_USER_SET_VRING_ERR = 14,
>      VHOST_USER_GET_PROTOCOL_FEATURES = 15,
>      VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> +    VHOST_USER_GET_QUEUE_NUM = 17,
>      VHOST_USER_SET_VRING_ENABLE = 18,
>      VHOST_USER_MAX
>  } VhostUserRequest;
> @@ -132,6 +139,7 @@ typedef struct TestServer {
>      int log_fd;
>      uint64_t rings;
>      bool test_fail;
> +    int queues;
>  } TestServer;
>
>  static const char *tmpfs;
> @@ -253,6 +261,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
>          msg.size = sizeof(m.payload.u64);
>          msg.payload.u64 = 0x1ULL << VHOST_F_LOG_ALL |
>              0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> +        if (s->queues > 1) {
> +            msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
> +        }
>          p = (uint8_t *) &msg;
>          qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
>          break;
> @@ -267,6 +278,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
>          msg.flags |= VHOST_USER_REPLY_MASK;
>          msg.size = sizeof(m.payload.u64);
>          msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
> +        if (s->queues > 1) {
> +            msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ;
> +        }
>          p = (uint8_t *) &msg;
>          qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
>          break;
> @@ -279,7 +293,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
>          p = (uint8_t *) &msg;
>          qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
>
> -        assert(msg.payload.state.index < 2);
> +        assert(msg.payload.state.index < s->queues * 2);
>          s->rings &= ~(0x1ULL << msg.payload.state.index);
>          break;
>
> @@ -319,10 +333,18 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
>          break;
>
>      case VHOST_USER_SET_VRING_BASE:
> -        assert(msg.payload.state.index < 2);
> +        assert(msg.payload.state.index < s->queues * 2);
>          s->rings |= 0x1ULL << msg.payload.state.index;
>          break;
>
> +    case VHOST_USER_GET_QUEUE_NUM:
> +        msg.flags |= VHOST_USER_REPLY_MASK;
> +        msg.size = sizeof(m.payload.u64);
> +        msg.payload.u64 = s->queues;
> +        p = (uint8_t *) &msg;
> +        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
> +        break;
> +
>      default:
>          break;
>      }
> @@ -369,6 +391,7 @@ static TestServer *test_server_new(const gchar *name)
>      g_cond_init(&server->data_cond);
>
>      server->log_fd = -1;
> +    server->queues = 1;
>
>      return server;
>  }
> @@ -722,6 +745,86 @@ static void test_connect_fail(void)
>
>  #endif
>
> +static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
> +{
> +    QVirtioPCIDevice *dev;
> +
> +    dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET);
> +    g_assert(dev != NULL);
> +    g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_NET);
> +
> +    qvirtio_pci_device_enable(dev);
> +    qvirtio_reset(&qvirtio_pci, &dev->vdev);
> +    qvirtio_set_acknowledge(&qvirtio_pci, &dev->vdev);
> +    qvirtio_set_driver(&qvirtio_pci, &dev->vdev);
> +
> +    return dev;
> +}
> +
> +static void driver_init(const QVirtioBus *bus, QVirtioDevice *dev)
> +{
> +    uint32_t features;
> +
> +    features = qvirtio_get_features(bus, dev);
> +    features = features & ~(QVIRTIO_F_BAD_FEATURE |
> +                            (1u << VIRTIO_RING_F_INDIRECT_DESC) |
> +                            (1u << VIRTIO_RING_F_EVENT_IDX));
> +    qvirtio_set_features(bus, dev, features);
> +
> +    qvirtio_set_driver_ok(bus, dev);
> +}
> +
> +#define PCI_SLOT                0x04
> +
> +static void test_multiqueue(void)
> +{
> +    const int queues = 2;
> +    TestServer *s = test_server_new("mq");
> +    QVirtioPCIDevice *dev;
> +    QPCIBus *bus;
> +    QVirtQueuePCI *vq[queues * 2];
> +    QGuestAllocator *alloc;
> +    char *cmd;
> +    int i;
> +
> +    s->queues = queues;
> +    test_server_listen(s);
> +
> +    cmd = g_strdup_printf(QEMU_CMD_ACCEL QEMU_CMD_MEM QEMU_CMD_CHR
> +                          QEMU_CMD_NETDEV ",queues=%d "
> +                          "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d",
> +                          512, 512, root, s->chr_name,
> +                          s->socket_path, "", s->chr_name,
> +                          queues, queues * 2 + 2);
> +    qtest_start(cmd);
> +    g_free(cmd);
> +
> +    bus = qpci_init_pc();
> +    dev = virtio_net_pci_init(bus, PCI_SLOT);
> +
> +    alloc = pc_alloc_init();
> +    for (i = 0; i < queues * 2; i++) {
> +        vq[i] = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev,
> +                                              alloc, i);
> +    }
> +
> +    driver_init(&qvirtio_pci, &dev->vdev);
> +    wait_for_rings_started(s, queues * 2);
> +
> +    /* End test */
> +    for (i = 0; i < queues * 2; i++) {
> +        qvirtqueue_cleanup(&qvirtio_pci, &vq[i]->vq, alloc);
> +    }
> +    pc_alloc_uninit(alloc);
> +    qvirtio_pci_device_disable(dev);
> +    g_free(dev->pdev);
> +    g_free(dev);
> +    qpci_free_pc(bus);
> +    qtest_end();
> +
> +    test_server_free(s);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      QTestState *s = NULL;
> @@ -766,6 +869,7 @@ int main(int argc, char **argv)
>
>      qtest_add_data_func("/vhost-user/read-guest-mem", server, read_guest_mem);
>      qtest_add_func("/vhost-user/migrate", test_migrate);
> +    qtest_add_func("/vhost-user/multiqueue", test_multiqueue);
>  #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
>      qtest_add_func("/vhost-user/reconnect/subprocess",
>                     test_reconnect_subprocess);
> --
> 2.9.0
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v6 00/33] vhost-user reconnect fixes

[Michael S. Tsirkin]

On Wed, Jul 27, 2016 at 01:14:54AM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Since 'vhost-user: simple reconnection support' has been merged, it is
> possible to disconnect and reconnect a vhost-user backend. However,
> many code paths in qemu may trigger assert() when the backend is
> disconnected.
> 
> There are also code paths that are wrong, see "don't assume opaque is
> a fd" patch for an example. Once those patches are reviewed & merged,
> they are good candidates for stable too.

Thanks, I merged most of these except the new tests.

> Some assert() could simply be replaced by error_report() or silently
> fail since they are recoverable cases. Some missing error checks can
> also help prevent later issues. The errors are reported up to vhost.c,
> as the vhost-user backend alone doesn't handle disconnected state
> transparently so far. There are still problematic code paths left
> after this series, for example, starting a migration with a
> disconnected backend will abort(). It is likely that other problematic
> code path exists (vhost_scsi_start failure is fatal, but there are no
> vhost-user backend that I know yet).
> 
> In many cases, the code assumes get_vhost_net() will be non-NULL after
> a succesful connection, so I changed it to stay after a disconnect
> until the new connection comes (as suggested by Michael).
> 
> Since there is feature checks on reconnection, qemu should wait for
> the initial connection feature negotiation to complete. The test added
> demonstrates this. Additionally, a regression was found during v2,
> which could have been spotted with a multiqueue test, so add a basic
> one that would have exhibited the issue.
> 
> For convenience, the series is also available on:
> https://github.com/elmarco/qemu, branch vhost-user-reconnect
> 
> v6:
> - fixes after Ilya Maximets review
> - add "disconnect on HUP" for cases where peer disconnect doesn't
>   trigger qemu disconnect event (reconnect won't work)
> - add a flags mismatch on reconnect test
> 
> v5:
> - rebased
> - use a VHOST_OPS_DEBUG macro to print vhost_ops errors
> - replace assert(foo != NULL) with assert(foo)
> - add "RFC: vhost: do not update last avail idx"
> 
> v4:
> - change notify_migration_done() patch to be VHOST_BACKEND_TYPE_USER
>   specific, to avoid having to handle the case where the backend
>   doesn't implement the callback
> - change vhost_dev_cleanup() to assert on empty log, instead of
>   adding a call to vhost_log_put()
> - made "keep vhost_net after a disconnection" more robust, got rid of
>   the acked_features field
> - improve commit log, and some patch reorganization for clarity
> 
> v3:
> - add vhost-user multiqueue test, which would have helped to find the
>   following fix
> - fix waiting on vhost-user connection with multiqueue (found by
>   Yuanhan Liu)
> - merge vhost_user_{read,write}() error checking patches
> - add error reporting to vhost_user_read() (similar to
>   vhost_user_write())
> - add a vhost_net_set_backend() wrapper to help with potential crash
> - some leak fixes
> 
> v2:
> - some patch ordering: minor fix, close(fd) fix,
>   assert/fprintf->error_report, check and return error,
>   vhost_dev_cleanup() fixes, keep vhost_net after a disconnect, wait
>   until connection is ready
> - merge read/write error checks
> - do not rely on link state to check vhost-user init completed
> 
> Marc-André Lureau (33):
>   misc: indentation
>   vhost-user: minor simplification
>   vhost-user: disconnect on HUP
>   vhost: don't assume opaque is a fd, use backend cleanup
>   vhost: make vhost_log_put() idempotent
>   vhost: assert the log was cleaned up
>   vhost: fix cleanup on not fully initialized device
>   vhost: make vhost_dev_cleanup() idempotent
>   vhost-net: always call vhost_dev_cleanup() on failure
>   vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()
>   vhost: do not assert() on vhost_ops failure
>   vhost: add missing VHOST_OPS_DEBUG
>   vhost: use error_report() instead of fprintf(stderr,...)
>   qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected
>   vhost-user: call set_msgfds unconditionally
>   vhost-user: check qemu_chr_fe_set_msgfds() return value
>   vhost-user: check vhost_user_{read,write}() return value
>   vhost-user: keep vhost_net after a disconnection
>   vhost-user: add get_vhost_net() assertions
>   Revert "vhost-net: do not crash if backend is not present"
>   vhost-net: vhost_migration_done is vhost-user specific
>   vhost: add assert() to check runtime behaviour
>   char: add chr_wait_connected callback
>   char: add and use tcp_chr_wait_connected
>   vhost-user: wait until backend init is completed
>   tests: plug some leaks in virtio-net-test
>   tests: fix vhost-user-test leak
>   tests: add /vhost-user/connect-fail test
>   tests: add a simple /vhost-user/multiqueue test
>   vhost-user: add error report in vhost_user_write()
>   vhost: add vhost_net_set_backend()
>   vhost-user-test: add flags mismatch test
>   RFC: vhost: do not update last avail idx on get_vring_base() failure
> 
>  hw/net/vhost_net.c        |  34 +++-----
>  hw/virtio/vhost-user.c    |  67 ++++++++++-----
>  hw/virtio/vhost.c         | 162 +++++++++++++++++++++++-------------
>  include/hw/virtio/vhost.h |   4 +
>  include/sysemu/char.h     |   8 ++
>  net/tap.c                 |   1 +
>  net/vhost-user.c          |  65 +++++++++------
>  qemu-char.c               |  82 ++++++++++++------
>  tests/Makefile.include    |   2 +-
>  tests/vhost-user-test.c   | 206 +++++++++++++++++++++++++++++++++++++++++++++-
>  tests/virtio-net-test.c   |  12 ++-
>  11 files changed, 486 insertions(+), 157 deletions(-)
> 
> -- 
> 2.9.0