[Qemu-devel] [PATCH v3 00/16] vhost-user: add migration log support (for 2.5)

[Qemu-devel] [PATCH v3 00/16] vhost-user: add migration log support (for 2.5)

[marcandre.lureau]

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

Hi,

The following series implement shareable log for vhost-user to support
memory tracking during live migration. On qemu-side, the solution is
fairly straightfoward since vhost already supports the dirty log, only
vhost-user couldn't access the log memory until then.

The series is based on top of "protocol feature negotiation" series
proposed earlier by Michael S. Tsirkinm and "vhost user: Add live
migration" series from Thibaut Collet .

Since v2:
- changed some patch summary
- added migration tests
- added a patch to replace error message with a trace

The development branch I used is:
https://github.com/elmarco/qemu branch "vhost-user"

More comments welcome!

Marc-André Lureau (16):
  configure: probe for memfd
  util: add linux-only memfd fallback
  util: add memfd helpers
  vhost: alloc shareable log
  vhost: document log resizing
  vhost: use variable arguments for vhost_call()
  vhost-user: start and end the va_list
  vhost-user: send log shm fd along with log_base
  vhost-user: document migration log
  net: add trace_vhost_user_event
  vhost-user-test: move wait_for_fds() out
  vhost-user-test: remove useless static check
  vhost-user-test: wrap server in TestServer struct
  vhost-user-test: learn to tweak various qemu arguments
  vhost-user-test: add live-migration test
  vhost-user-test: check ownership during migration

 configure                         |  19 ++
 docs/specs/vhost-user.txt         |  42 +++++
 hw/virtio/vhost-backend.c         |   4 +-
 hw/virtio/vhost-user.c            |  52 ++++--
 hw/virtio/vhost.c                 |  45 ++++-
 include/hw/virtio/vhost-backend.h |   6 +-
 include/hw/virtio/vhost.h         |   3 +-
 include/qemu/memfd.h              |  24 +++
 net/vhost-user.c                  |   5 +-
 tests/vhost-user-test.c           | 366 +++++++++++++++++++++++++++++++-------
 trace-events                      |   3 +
 util/Makefile.objs                |   2 +-
 util/memfd.c                      | 126 +++++++++++++
 13 files changed, 602 insertions(+), 95 deletions(-)
 create mode 100644 include/qemu/memfd.h
 create mode 100644 util/memfd.c

-- 
2.4.3

[Qemu-devel] [PATCH v3 01/16] configure: probe for memfd

[marcandre.lureau]

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

Check if memfd_create() is part of system libc.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/configure b/configure
index cd219d8..3dcbd3e 100755
--- a/configure
+++ b/configure
@@ -3405,6 +3405,22 @@ if compile_prog "" "" ; then
   eventfd=yes
 fi
 
+# check if memfd is supported
+memfd=no
+cat > $TMPC << EOF
+#include <sys/memfd.h>
+
+int main(void)
+{
+    return memfd_create("foo", MFD_ALLOW_SEALING);
+}
+EOF
+if compile_prog "" "" ; then
+  memfd=yes
+fi
+
+
+
 # check for fallocate
 fallocate=no
 cat > $TMPC << EOF
@@ -4785,6 +4801,9 @@ fi
 if test "$eventfd" = "yes" ; then
   echo "CONFIG_EVENTFD=y" >> $config_host_mak
 fi
+if test "$memfd" = "yes" ; then
+  echo "CONFIG_MEMFD=y" >> $config_host_mak
+fi
 if test "$fallocate" = "yes" ; then
   echo "CONFIG_FALLOCATE=y" >> $config_host_mak
 fi
-- 
2.4.3

[Qemu-devel] [PATCH v3 02/16] util: add linux-only memfd fallback

[marcandre.lureau]

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

Implement memfd_create() fallback if not available in system libc.
memfd_create() is still not included in glibc today, atlhough it's been
available since Linux 3.17 in Oct 2014.

memfd has numerous advantages over traditional shm/mmap for ipc memory
sharing with fd handler, which we are going to make use of for
vhost-user logging memory in following patches.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/memfd.h | 20 +++++++++++++++++++
 util/Makefile.objs   |  2 +-
 util/memfd.c         | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 include/qemu/memfd.h
 create mode 100644 util/memfd.c

diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h
new file mode 100644
index 0000000..8b1fe6a
--- /dev/null
+++ b/include/qemu/memfd.h
@@ -0,0 +1,20 @@
+#ifndef QEMU_MEMFD_H
+#define QEMU_MEMFD_H
+
+#include "config-host.h"
+
+#ifndef F_LINUX_SPECIFIC_BASE
+#define F_LINUX_SPECIFIC_BASE 1024
+#endif
+
+#ifndef F_ADD_SEALS
+#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
+#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
+
+#define F_SEAL_SEAL     0x0001  /* prevent further seals from being set */
+#define F_SEAL_SHRINK   0x0002  /* prevent file from shrinking */
+#define F_SEAL_GROW     0x0004  /* prevent file from growing */
+#define F_SEAL_WRITE    0x0008  /* prevent writes */
+#endif
+
+#endif /* QEMU_MEMFD_H */
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 114d657..84c5485 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -1,6 +1,6 @@
 util-obj-y = osdep.o cutils.o unicode.o qemu-timer-common.o
 util-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o event_notifier-win32.o
-util-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o event_notifier-posix.o qemu-openpty.o
+util-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-thread-posix.o event_notifier-posix.o qemu-openpty.o memfd.o
 util-obj-y += envlist.o path.o module.o
 util-obj-$(call lnot,$(CONFIG_INT128)) += host-utils.o
 util-obj-y += bitmap.o bitops.o hbitmap.o
diff --git a/util/memfd.c b/util/memfd.c
new file mode 100644
index 0000000..a98d57e
--- /dev/null
+++ b/util/memfd.c
@@ -0,0 +1,56 @@
+/*
+ * memfd.c
+ *
+ * Copyright (c) 2015 Red Hat, Inc.
+ *
+ * QEMU library functions on POSIX which are shared between QEMU and
+ * the QEMU tools.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "config-host.h"
+
+#include "qemu/memfd.h"
+
+#ifdef CONFIG_MEMFD
+#include <sys/memfd.h>
+#elif defined CONFIG_LINUX
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <asm/unistd.h>
+
+#ifndef MFD_CLOEXEC
+#define MFD_CLOEXEC 0x0001U
+#endif
+
+#ifndef MFD_ALLOW_SEALING
+#define MFD_ALLOW_SEALING 0x0002U
+#endif
+
+static inline int memfd_create(const char *name, unsigned int flags)
+{
+    return syscall(__NR_memfd_create, name, flags);
+}
+#else /* !LINUX */
+static inline int memfd_create(const char *name, unsigned int flags)
+{
+    return -1;
+}
+#endif
-- 
2.4.3

[Qemu-devel] [PATCH v3 03/16] util: add memfd helpers

[marcandre.lureau]

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

Add qemu_memfd_alloc/free() helpers.

The function helps to allocate and seal a memfd, and implements an
open/unlink/mmap fallback for system that do not support memfd.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/memfd.h |  4 +++
 util/memfd.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h
index 8b1fe6a..950fb88 100644
--- a/include/qemu/memfd.h
+++ b/include/qemu/memfd.h
@@ -17,4 +17,8 @@
 #define F_SEAL_WRITE    0x0008  /* prevent writes */
 #endif
 
+void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
+                       int *fd);
+void qemu_memfd_free(void *ptr, size_t size, int fd);
+
 #endif /* QEMU_MEMFD_H */
diff --git a/util/memfd.c b/util/memfd.c
index a98d57e..8b2b785 100644
--- a/util/memfd.c
+++ b/util/memfd.c
@@ -27,6 +27,14 @@
 
 #include "config-host.h"
 
+#include <glib.h>
+#include <glib/gprintf.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+
 #include "qemu/memfd.h"
 
 #ifdef CONFIG_MEMFD
@@ -44,13 +52,75 @@
 #define MFD_ALLOW_SEALING 0x0002U
 #endif
 
-static inline int memfd_create(const char *name, unsigned int flags)
+static int memfd_create(const char *name, unsigned int flags)
 {
     return syscall(__NR_memfd_create, name, flags);
 }
 #else /* !LINUX */
-static inline int memfd_create(const char *name, unsigned int flags)
+static int memfd_create(const char *name, unsigned int flags)
 {
     return -1;
 }
 #endif
+
+void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
+                       int *fd)
+{
+    void *ptr;
+    int mfd;
+
+    mfd = memfd_create(name, MFD_ALLOW_SEALING|MFD_CLOEXEC);
+    if (mfd != -1) {
+        if (ftruncate(mfd, size) == -1) {
+            perror("ftruncate");
+            close(mfd);
+            return NULL;
+        }
+
+        if (fcntl(mfd, F_ADD_SEALS, seals) == -1) {
+            perror("fcntl");
+            close(mfd);
+            return NULL;
+        }
+    } else {
+        const char *tmpdir = getenv("TMPDIR");
+        gchar *fname;
+
+        tmpdir = tmpdir ? tmpdir : "/tmp";
+
+        fname = g_strdup_printf("%s/memfd-XXXXXX", tmpdir);
+        mfd = mkstemp(fname);
+        unlink(fname);
+        g_free(fname);
+
+        if (mfd == -1) {
+            perror("mkstemp");
+            return NULL;
+        }
+
+        if (ftruncate(mfd, size) == -1) {
+            perror("ftruncate");
+            close(mfd);
+            return NULL;
+        }
+    }
+
+    ptr = mmap(0, size, PROT_READ|PROT_WRITE, MAP_SHARED, mfd, 0);
+    if (ptr == MAP_FAILED) {
+        perror("mmap");
+        close(mfd);
+        return NULL;
+    }
+
+    *fd = mfd;
+    return ptr;
+}
+
+void qemu_memfd_free(void *ptr, size_t size, int fd)
+{
+    if (ptr) {
+        munmap(ptr, size);
+    }
+
+    close(fd);
+}
-- 
2.4.3

[Qemu-devel] [PATCH v3 04/16] vhost: alloc shareable log

[marcandre.lureau]

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

If the backend is of type VHOST_BACKEND_TYPE_USER, allocate
shareable memory.

Note: vhost_log_get() can use a global "vhost_log" that can be shared by
several vhost devices. We may want instead a common shareable log and a
common non-shareable one.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c         | 38 +++++++++++++++++++++++++++++++-------
 include/hw/virtio/vhost.h |  3 ++-
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2712c6f..862e786 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -18,6 +18,7 @@
 #include "qemu/atomic.h"
 #include "qemu/range.h"
 #include "qemu/error-report.h"
+#include "qemu/memfd.h"
 #include <linux/vhost.h>
 #include "exec/address-spaces.h"
 #include "hw/virtio/virtio-bus.h"
@@ -286,20 +287,34 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
     }
     return log_size;
 }
-static struct vhost_log *vhost_log_alloc(uint64_t size)
+
+static struct vhost_log *vhost_log_alloc(uint64_t size, bool share)
 {
-    struct vhost_log *log = g_malloc0(sizeof *log + size * sizeof(*(log->log)));
+    struct vhost_log *log;
+    uint64_t logsize = size * sizeof(*(log->log));
+    int fd = -1;
+
+    log = g_new0(struct vhost_log, 1);
+    if (share) {
+        log->log = qemu_memfd_alloc("vhost-log", logsize,
+                                    F_SEAL_GROW|F_SEAL_SHRINK|F_SEAL_SEAL, &fd);
+        memset(log->log, 0, logsize);
+    } else {
+        log->log = g_malloc0(logsize);
+    }
 
     log->size = size;
     log->refcnt = 1;
+    log->fd = fd;
 
     return log;
 }
 
-static struct vhost_log *vhost_log_get(uint64_t size)
+static struct vhost_log *vhost_log_get(uint64_t size, bool share)
 {
-    if (!vhost_log || vhost_log->size != size) {
-        vhost_log = vhost_log_alloc(size);
+    if (!vhost_log || vhost_log->size != size ||
+        (share && vhost_log->fd == -1)) {
+        vhost_log = vhost_log_alloc(size, share);
     } else {
         ++vhost_log->refcnt;
     }
@@ -324,13 +339,21 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
         if (vhost_log == log) {
             vhost_log = NULL;
         }
+
+        if (log->fd == -1) {
+            g_free(log->log);
+        } else {
+            qemu_memfd_free(log->log, log->size * sizeof(*(log->log)),
+                            log->fd);
+        }
         g_free(log);
     }
 }
 
 static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
 {
-    struct vhost_log *log = vhost_log_get(size);
+    bool share = dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
+    struct vhost_log *log = vhost_log_get(size, share);
     uint64_t log_base = (uintptr_t)log->log;
     int r;
 
@@ -1136,9 +1159,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 
     if (hdev->log_enabled) {
         uint64_t log_base;
+        bool share = hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
 
         hdev->log_size = vhost_get_log_size(hdev);
-        hdev->log = vhost_log_get(hdev->log_size);
+        hdev->log = vhost_log_get(hdev->log_size, share);
         log_base = (uintptr_t)hdev->log->log;
         r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
                                         hdev->log_size ? &log_base : NULL);
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 6467c73..ab1dcac 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -31,7 +31,8 @@ typedef unsigned long vhost_log_chunk_t;
 struct vhost_log {
     unsigned long long size;
     int refcnt;
-    vhost_log_chunk_t log[0];
+    int fd;
+    vhost_log_chunk_t *log;
 };
 
 struct vhost_memory;
-- 
2.4.3

[Qemu-devel] [PATCH v3 05/16] vhost: document log resizing

[marcandre.lureau]

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

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 862e786..057d548 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -357,6 +357,8 @@ static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
     uint64_t log_base = (uintptr_t)log->log;
     int r;
 
+    /* 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_call(dev, VHOST_SET_LOG_BASE, &log_base);
     assert(r >= 0);
     vhost_log_put(dev, true);
-- 
2.4.3

[Qemu-devel] [PATCH v3 06/16] vhost: use variable arguments for vhost_call()

[marcandre.lureau]

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

It is useful to pass extra arguments to the funtions, for
various backend needs.

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

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 4d68a27..7255089 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -14,8 +14,8 @@
 
 #include <sys/ioctl.h>
 
-static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
-                             void *arg)
+static int vhost_kernel_call(struct vhost_dev *dev,
+                             unsigned long int request, void *arg, ...)
 {
     int fd = (uintptr_t) dev->opaque;
 
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4993b63..8b6d7e7 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -190,8 +190,8 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
             0 : -1;
 }
 
-static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
-        void *arg)
+static int vhost_user_call(struct vhost_dev *dev,
+                           unsigned long int request, void *arg, ...)
 {
     VhostUserMsg msg;
     VhostUserRequest msg_request;
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index e472f29..36fa0f7 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -11,6 +11,8 @@
 #ifndef VHOST_BACKEND_H_
 #define VHOST_BACKEND_H_
 
+#include <stdarg.h>
+
 typedef enum VhostBackendType {
     VHOST_BACKEND_TYPE_NONE = 0,
     VHOST_BACKEND_TYPE_KERNEL = 1,
@@ -20,8 +22,8 @@ typedef enum VhostBackendType {
 
 struct vhost_dev;
 
-typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
-             void *arg);
+typedef int (*vhost_call)(struct vhost_dev *dev,
+                          unsigned long int request, void *arg, ...);
 typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
 typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
 
-- 
2.4.3

[Qemu-devel] [PATCH v3 07/16] vhost-user: start and end the va_list

[marcandre.lureau]

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

Use a common start and exit branch to open and cleanup the va_list.

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

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8b6d7e7..21ecbcd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -198,11 +198,13 @@ static int vhost_user_call(struct vhost_dev *dev,
     struct vhost_vring_file *file = 0;
     int need_reply = 0;
     int fds[VHOST_MEMORY_MAX_NREGIONS];
-    int i, fd;
+    int i, fd, ret = 0;
     size_t fd_num = 0;
+    va_list ap;
 
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
 
+    va_start(ap, arg);
     msg_request = vhost_user_request_translate(request);
     msg.request = msg_request;
     msg.flags = VHOST_USER_VERSION;
@@ -247,7 +249,8 @@ static int vhost_user_call(struct vhost_dev *dev,
         if (!fd_num) {
             error_report("Failed initializing vhost-user memory map, "
                     "consider using -object memory-backend-file share=on");
-            return -1;
+            ret = -1;
+            goto end;
         }
 
         msg.size = sizeof(m.memory.nregions);
@@ -291,48 +294,53 @@ static int vhost_user_call(struct vhost_dev *dev,
         break;
     default:
         error_report("vhost-user trying to send unhandled ioctl");
-        return -1;
-        break;
+        ret = -1;
+        goto end;
     }
 
     if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
-        return 0;
+        goto end;
     }
 
     if (need_reply) {
         if (vhost_user_read(dev, &msg) < 0) {
-            return 0;
+            goto end;
         }
 
         if (msg_request != msg.request) {
             error_report("Received unexpected msg type."
                     " Expected %d received %d", msg_request, msg.request);
-            return -1;
+            ret = -1;
+            goto end;
         }
 
         switch (msg_request) {
         case VHOST_USER_GET_FEATURES:
             if (msg.size != sizeof(m.u64)) {
                 error_report("Received bad msg size.");
-                return -1;
+                ret = -1;
+                goto end;
             }
             *((__u64 *) arg) = msg.u64;
             break;
         case VHOST_USER_GET_VRING_BASE:
             if (msg.size != sizeof(m.state)) {
                 error_report("Received bad msg size.");
-                return -1;
+                ret = -1;
+                goto end;
             }
             memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
             break;
         default:
             error_report("Received unexpected msg type.");
-            return -1;
-            break;
+            ret = -1;
+            goto end;
         }
     }
 
-    return 0;
+end:
+    va_end(ap);
+    return ret;
 }
 
 static int vhost_user_init(struct vhost_dev *dev, void *opaque)
-- 
2.4.3

[Qemu-devel] [PATCH v3 08/16] vhost-user: send log shm fd along with log_base

[marcandre.lureau]

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

Send the shm for the dirty pages logging if the backend support
VHOST_USER_PROTOCOL_F_LOG_SHMFD.

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

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 21ecbcd..b2f46a9 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -26,7 +26,9 @@
 #define VHOST_MEMORY_MAX_NREGIONS    8
 
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
-#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
+
+#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
+#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
 
 typedef enum VhostUserRequest {
     VHOST_USER_NONE = 0,
@@ -215,8 +217,18 @@ static int vhost_user_call(struct vhost_dev *dev,
         need_reply = 1;
         break;
 
+    case VHOST_USER_SET_LOG_BASE: {
+        struct vhost_log *log = va_arg(ap, struct vhost_log *);
+
+        if (__virtio_has_feature(dev->protocol_features,
+                                 VHOST_USER_PROTOCOL_F_LOG_SHMFD) &&
+            log->fd != -1) {
+            fds[fd_num++] = log->fd;
+        }
+    }
+        /* fall through */
+
     case VHOST_USER_SET_FEATURES:
-    case VHOST_USER_SET_LOG_BASE:
         msg.u64 = *((__u64 *) arg);
         msg.size = sizeof(m.u64);
         break;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 057d548..ed8b1a5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -359,7 +359,7 @@ 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_call(dev, VHOST_SET_LOG_BASE, &log_base);
+    r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base, log);
     assert(r >= 0);
     vhost_log_put(dev, true);
     dev->log = log;
@@ -1167,7 +1167,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         hdev->log = vhost_log_get(hdev->log_size, share);
         log_base = (uintptr_t)hdev->log->log;
         r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
-                                        hdev->log_size ? &log_base : NULL);
+                                        hdev->log_size ? &log_base : NULL,
+                                        hdev->log);
         if (r < 0) {
             r = -errno;
             goto fail_log;
-- 
2.4.3

[Qemu-devel] [PATCH v3 09/16] vhost-user: document migration log

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 docs/specs/vhost-user.txt | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 0062baa..0322bcf 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -120,6 +120,7 @@ There are several messages that the master sends with file descriptors passed
 in the ancillary data:
 
  * VHOST_SET_MEM_TABLE
+ * VHOST_SET_LOG_BASE (if VHOST_USER_PROTOCOL_F_LOG_SHMFD)
  * VHOST_SET_LOG_FD
  * VHOST_SET_VRING_KICK
  * VHOST_SET_VRING_CALL
@@ -135,6 +136,11 @@ As older slaves don't support negotiating protocol features,
 a feature bit was dedicated for this purpose:
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
 
+Protocol features
+-----------------
+
+#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
+
 Message types
 -------------
 
@@ -301,3 +307,39 @@ Message types
       Bits (0-7) of the payload contain the vring index. Bit 8 is the
       invalid FD flag. This flag is set when there is no file descriptor
       in the ancillary data.
+
+Migration
+---------
+
+During live migration, the master may need to track the modifications
+the slave makes to the memory mapped regions. The client should mark
+the dirty pages in a log. Once it complies to this logging, it may
+declare the VHOST_F_LOG_ALL vhost feature.
+
+All the modifications to memory pointed by vring "descriptor" should
+be marked. Modifications to "used" vring should be marked if
+VHOST_VRING_F_LOG is part of ring's features.
+
+Dirty pages are of size:
+#define VHOST_LOG_PAGE 0x1000
+
+The log memory fd is provided in the ancillary data of
+VHOST_USER_SET_LOG_BASE message when the slave has
+VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol feature.
+
+The size of the log may be computed by using all the known guest
+addresses. The log covers from address 0 to the maximum of guest
+regions. In pseudo-code, to mark page at "addr" as dirty:
+
+page = addr / VHOST_LOG_PAGE
+log[page / 8] |= 1 << page % 8
+
+Use atomic operations, as the log may be concurrently manipulated.
+
+VHOST_USER_SET_LOG_FD is an optional message with an eventfd in
+ancillary data, it may be used to inform the master that the log has
+been modified.
+
+Once the source has finished migration, VHOST_USER_RESET_OWNER message
+will be sent by the source. No further update must be done before the
+destination takes over with new regions & rings.
-- 
2.4.3

[Qemu-devel] [PATCH v3 10/16] net: add trace_vhost_user_event

[marcandre.lureau]

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

Replace error_report() and use tracing instead.

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

diff --git a/net/vhost-user.c b/net/vhost-user.c
index dc0aa4c..5657992 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -14,6 +14,7 @@
 #include "sysemu/char.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
+#include "trace.h"
 
 typedef struct VhostUserState {
     NetClientState nc;
@@ -123,16 +124,16 @@ static void net_vhost_user_event(void *opaque, int event)
 {
     VhostUserState *s = opaque;
 
+    trace_vhost_user_event(s->chr->label, event);
+
     switch (event) {
     case CHR_EVENT_OPENED:
         vhost_user_start(s);
         net_vhost_link_down(s, false);
-        error_report("chardev \"%s\" went up", s->chr->label);
         break;
     case CHR_EVENT_CLOSED:
         net_vhost_link_down(s, true);
         vhost_user_stop(s);
-        error_report("chardev \"%s\" went down", s->chr->label);
         break;
     }
 }
diff --git a/trace-events b/trace-events
index 94bf3bb..a07b715 100644
--- a/trace-events
+++ b/trace-events
@@ -1654,3 +1654,6 @@ alsa_no_frames(int state) "No frames available and ALSA state is %d"
 # audio/ossaudio.c
 oss_version(int version) "OSS version = %#x"
 oss_invalid_available_size(int size, int bufsize) "Invalid available size, size=%d bufsize=%d"
+
+# net/vhost-user.c
+vhost_user_event(const char *chr, int event) "chr: %s got event: %d"
-- 
2.4.3

[Qemu-devel] [PATCH v3 11/16] vhost-user-test: move wait_for_fds() out

[marcandre.lureau]

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

This function is a precondition for most vhost-user tests.

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 228acb6..551a664 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -205,15 +205,11 @@ static GThread *_thread_new(const gchar *name, GThreadFunc func, gpointer data)
     return thread;
 }
 
-static void read_guest_mem(void)
+static void wait_for_fds(void)
 {
-    uint32_t *guest_mem;
     gint64 end_time;
-    int i, j;
-    size_t size;
 
     g_mutex_lock(data_mutex);
-
     end_time = _get_time() + 5 * G_TIME_SPAN_SECOND;
     while (!fds_num) {
         if (!_cond_wait_until(data_cond, data_mutex, end_time)) {
@@ -227,6 +223,17 @@ static void read_guest_mem(void)
     g_assert_cmpint(fds_num, >, 0);
     g_assert_cmpint(fds_num, ==, memory.nregions);
 
+    g_mutex_unlock(data_mutex);
+}
+
+static void read_guest_mem(void)
+{
+    uint32_t *guest_mem;
+    int i, j;
+    size_t size;
+
+    wait_for_fds();
+
     /* iterate all regions */
     for (i = 0; i < fds_num; i++) {
 
-- 
2.4.3

[Qemu-devel] [PATCH v3 12/16] vhost-user-test: remove useless static check

[marcandre.lureau]

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

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 551a664..68badf9 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -262,7 +262,6 @@ static void read_guest_mem(void)
         munmap(guest_mem, memory.regions[i].memory_size);
     }
 
-    g_assert_cmpint(1, ==, 1);
     g_mutex_unlock(data_mutex);
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v3 13/16] vhost-user-test: wrap server in TestServer struct

[marcandre.lureau]

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

In the coming patches, a test will use several servers
simultaneously. Wrap the server in a struct, out of the global scope.

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 68badf9..6605fba 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -115,10 +115,18 @@ static VhostUserMsg m __attribute__ ((unused));
 #define VHOST_USER_VERSION    (0x1)
 /*****************************************************************************/
 
-int fds_num = 0, fds[VHOST_MEMORY_MAX_NREGIONS];
-static VhostUserMemory memory;
-static GMutex *data_mutex;
-static GCond *data_cond;
+typedef struct TestServer {
+    gchar *socket_path;
+    gchar *chr_name;
+    CharDriverState *chr;
+    int fds_num;
+    int fds[VHOST_MEMORY_MAX_NREGIONS];
+    VhostUserMemory memory;
+    GMutex *data_mutex;
+    GCond *data_cond;
+} TestServer;
+
+static const char *hugefs;
 
 static gint64 _get_time(void)
 {
@@ -205,64 +213,63 @@ static GThread *_thread_new(const gchar *name, GThreadFunc func, gpointer data)
     return thread;
 }
 
-static void wait_for_fds(void)
+static void wait_for_fds(TestServer *s)
 {
     gint64 end_time;
 
-    g_mutex_lock(data_mutex);
+    g_mutex_lock(s->data_mutex);
     end_time = _get_time() + 5 * G_TIME_SPAN_SECOND;
-    while (!fds_num) {
-        if (!_cond_wait_until(data_cond, data_mutex, end_time)) {
+    while (!s->fds_num) {
+        if (!_cond_wait_until(s->data_cond, s->data_mutex, end_time)) {
             /* timeout has passed */
-            g_assert(fds_num);
+            g_assert(s->fds_num);
             break;
         }
     }
 
     /* check for sanity */
-    g_assert_cmpint(fds_num, >, 0);
-    g_assert_cmpint(fds_num, ==, memory.nregions);
+    g_assert_cmpint(s->fds_num, >, 0);
+    g_assert_cmpint(s->fds_num, ==, s->memory.nregions);
 
-    g_mutex_unlock(data_mutex);
+    g_mutex_unlock(s->data_mutex);
 }
 
-static void read_guest_mem(void)
+static void read_guest_mem(TestServer *s)
 {
     uint32_t *guest_mem;
     int i, j;
     size_t size;
 
-    wait_for_fds();
+    wait_for_fds(s);
 
     /* iterate all regions */
-    for (i = 0; i < fds_num; i++) {
+    for (i = 0; i < s->fds_num; i++) {
 
         /* We'll check only the region statring at 0x0*/
-        if (memory.regions[i].guest_phys_addr != 0x0) {
+        if (s->memory.regions[i].guest_phys_addr != 0x0) {
             continue;
         }
 
-        g_assert_cmpint(memory.regions[i].memory_size, >, 1024);
+        g_assert_cmpint(s->memory.regions[i].memory_size, >, 1024);
 
-        size =  memory.regions[i].memory_size + memory.regions[i].mmap_offset;
+        size = s->memory.regions[i].memory_size +
+            s->memory.regions[i].mmap_offset;
 
         guest_mem = mmap(0, size, PROT_READ | PROT_WRITE,
-                         MAP_SHARED, fds[i], 0);
+                         MAP_SHARED, s->fds[i], 0);
 
         g_assert(guest_mem != MAP_FAILED);
-        guest_mem += (memory.regions[i].mmap_offset / sizeof(*guest_mem));
+        guest_mem += (s->memory.regions[i].mmap_offset / sizeof(*guest_mem));
 
         for (j = 0; j < 256; j++) {
-            uint32_t a = readl(memory.regions[i].guest_phys_addr + j*4);
+            uint32_t a = readl(s->memory.regions[i].guest_phys_addr + j*4);
             uint32_t b = guest_mem[j];
 
             g_assert_cmpint(a, ==, b);
         }
 
-        munmap(guest_mem, memory.regions[i].memory_size);
+        munmap(guest_mem, s->memory.regions[i].memory_size);
     }
-
-    g_mutex_unlock(data_mutex);
 }
 
 static void *thread_function(void *data)
@@ -280,7 +287,8 @@ static int chr_can_read(void *opaque)
 
 static void chr_read(void *opaque, const uint8_t *buf, int size)
 {
-    CharDriverState *chr = opaque;
+    TestServer *s = opaque;
+    CharDriverState *chr = s->chr;
     VhostUserMsg msg;
     uint8_t *p = (uint8_t *) &msg;
     int fd;
@@ -290,12 +298,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    g_mutex_lock(data_mutex);
+    g_mutex_lock(s->data_mutex);
     memcpy(p, buf, VHOST_USER_HDR_SIZE);
 
     if (msg.size) {
         p += VHOST_USER_HDR_SIZE;
-        qemu_chr_fe_read_all(chr, p, msg.size);
+        g_assert_cmpint(qemu_chr_fe_read_all(chr, p, msg.size), ==, msg.size);
     }
 
     switch (msg.request) {
@@ -334,11 +342,11 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
 
     case VHOST_USER_SET_MEM_TABLE:
         /* received the mem table */
-        memcpy(&memory, &msg.memory, sizeof(msg.memory));
-        fds_num = qemu_chr_fe_get_msgfds(chr, fds, sizeof(fds) / sizeof(int));
+        memcpy(&s->memory, &msg.memory, sizeof(msg.memory));
+        s->fds_num = qemu_chr_fe_get_msgfds(chr, s->fds, G_N_ELEMENTS(s->fds));
 
         /* signal the test that it can continue */
-        g_cond_signal(data_cond);
+        g_cond_signal(s->data_cond);
         break;
 
     case VHOST_USER_SET_VRING_KICK:
@@ -355,7 +363,53 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     default:
         break;
     }
-    g_mutex_unlock(data_mutex);
+
+    g_mutex_unlock(s->data_mutex);
+}
+
+static TestServer *test_server_new(const gchar *name)
+{
+    TestServer *server = g_new0(TestServer, 1);
+    gchar *chr_path;
+
+    server->socket_path = g_strdup_printf("/tmp/vhost-%s-%d.sock",
+                                          name, getpid());
+
+    chr_path = g_strdup_printf("unix:%s,server,nowait", server->socket_path);
+    server->chr_name = g_strdup_printf("chr-%s", name);
+    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);
+
+    /* run the main loop thread so the chardev may operate */
+    server->data_mutex = _mutex_new();
+    server->data_cond = _cond_new();
+
+    return server;
+}
+
+#define GET_QEMU_CMD(s)                                         \
+    g_strdup_printf(QEMU_CMD, (hugefs), (s)->socket_path)
+
+
+static void test_server_free(TestServer *server)
+{
+    int i;
+
+    qemu_chr_delete(server->chr);
+
+    for (i = 0; i < server->fds_num; i++) {
+        close(server->fds[i]);
+    }
+
+    unlink(server->socket_path);
+    g_free(server->socket_path);
+
+    _cond_free(server->data_cond);
+    _mutex_free(server->data_mutex);
+
+    g_free(server);
 }
 
 static const char *init_hugepagefs(void)
@@ -394,41 +448,28 @@ static const char *init_hugepagefs(void)
 int main(int argc, char **argv)
 {
     QTestState *s = NULL;
-    CharDriverState *chr = NULL;
-    const char *hugefs = 0;
-    char *socket_path = 0;
-    char *qemu_cmd = 0;
-    char *chr_path = 0;
+    TestServer *server = NULL;
+    char *qemu_cmd;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
     module_call_init(MODULE_INIT_QOM);
+    qemu_add_opts(&qemu_chardev_opts);
 
     hugefs = init_hugepagefs();
     if (!hugefs) {
         return 0;
     }
 
-    socket_path = g_strdup_printf("/tmp/vhost-%d.sock", getpid());
-
-    /* create char dev and add read handlers */
-    qemu_add_opts(&qemu_chardev_opts);
-    chr_path = g_strdup_printf("unix:%s,server,nowait", socket_path);
-    chr = qemu_chr_new("chr0", chr_path, NULL);
-    g_free(chr_path);
-    qemu_chr_add_handlers(chr, chr_can_read, chr_read, NULL, chr);
-
-    /* run the main loop thread so the chardev may operate */
-    data_mutex = _mutex_new();
-    data_cond = _cond_new();
     _thread_new(NULL, thread_function, NULL);
 
-    qemu_cmd = g_strdup_printf(QEMU_CMD, hugefs, socket_path);
+    server = test_server_new("test");
+    qemu_cmd = GET_QEMU_CMD(server);
     s = qtest_start(qemu_cmd);
     g_free(qemu_cmd);
 
-    qtest_add_func("/vhost-user/read-guest-mem", read_guest_mem);
+    qtest_add_data_func("/vhost-user/read-guest-mem", server, read_guest_mem);
 
     ret = g_test_run();
 
@@ -437,10 +478,7 @@ int main(int argc, char **argv)
     }
 
     /* cleanup */
-    unlink(socket_path);
-    g_free(socket_path);
-    _cond_free(data_cond);
-    _mutex_free(data_mutex);
+    test_server_free(server);
 
     return ret;
 }
-- 
2.4.3

[Qemu-devel] [PATCH v3 14/16] vhost-user-test: learn to tweak various qemu arguments

[marcandre.lureau]

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

Add a new macro to make the qemu command line with other
values of memory size, and specific chardev id.

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 6605fba..4ef2d6c 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -37,10 +37,10 @@
 #endif
 
 #define QEMU_CMD_ACCEL  " -machine accel=tcg"
-#define QEMU_CMD_MEM    " -m 512 -object memory-backend-file,id=mem,size=512M,"\
+#define QEMU_CMD_MEM    " -m %d -object memory-backend-file,id=mem,size=%dM,"\
                         "mem-path=%s,share=on -numa node,memdev=mem"
-#define QEMU_CMD_CHR    " -chardev socket,id=chr0,path=%s"
-#define QEMU_CMD_NETDEV " -netdev vhost-user,id=net0,chardev=chr0,vhostforce"
+#define QEMU_CMD_CHR    " -chardev socket,id=%s,path=%s"
+#define QEMU_CMD_NETDEV " -netdev vhost-user,id=net0,chardev=%s,vhostforce"
 #define QEMU_CMD_NET    " -device virtio-net-pci,netdev=net0 "
 #define QEMU_CMD_ROM    " -option-rom ../pc-bios/pxe-virtio.rom"
 
@@ -389,9 +389,13 @@ static TestServer *test_server_new(const gchar *name)
     return server;
 }
 
-#define GET_QEMU_CMD(s)                                         \
-    g_strdup_printf(QEMU_CMD, (hugefs), (s)->socket_path)
+#define GET_QEMU_CMD(s)                                 \
+    g_strdup_printf(QEMU_CMD, 512, 512, (hugefs),       \
+        (s)->chr_name, (s)->socket_path, (s)->chr_name)
 
+#define GET_QEMU_CMDE(s, mem, extra, ...)                               \
+    g_strdup_printf(QEMU_CMD extra, (mem), (mem), (hugefs),             \
+        (s)->chr_name, (s)->socket_path, (s)->chr_name, ##__VA_ARGS__)
 
 static void test_server_free(TestServer *server)
 {
-- 
2.4.3

[Qemu-devel] [PATCH v3 15/16] vhost-user-test: add live-migration test

[marcandre.lureau]

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

This test checks that the log fd is given to the migration source, and
mark dirty pages during migration.

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 4ef2d6c..dcc14bf 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -13,6 +13,7 @@
 
 #include "libqtest.h"
 #include "qemu/option.h"
+#include "qemu/range.h"
 #include "sysemu/char.h"
 #include "sysemu/sysemu.h"
 
@@ -54,6 +55,9 @@
 #define VHOST_MEMORY_MAX_NREGIONS    8
 
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
+
+#define VHOST_LOG_PAGE 0x1000
 
 typedef enum VhostUserRequest {
     VHOST_USER_NONE = 0,
@@ -124,6 +128,7 @@ typedef struct TestServer {
     VhostUserMemory memory;
     GMutex *data_mutex;
     GCond *data_cond;
+    int log_fd;
 } TestServer;
 
 static const char *hugefs;
@@ -311,7 +316,8 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         /* send back features to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.u64);
-        msg.u64 = 0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+        msg.u64 = 0x1ULL << VHOST_F_LOG_ALL |
+            0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
@@ -325,12 +331,11 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         /* send back features to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.u64);
-        msg.u64 = 0;
+        msg.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
 
-
     case VHOST_USER_GET_VRING_BASE:
         /* send back vring base to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
@@ -360,6 +365,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
          */
         qemu_set_nonblock(fd);
         break;
+
+    case VHOST_USER_SET_LOG_BASE:
+        qemu_chr_fe_get_msgfds(chr, &s->log_fd, 1);
+        g_cond_signal(s->data_cond);
+        break;
+
     default:
         break;
     }
@@ -386,6 +397,8 @@ static TestServer *test_server_new(const gchar *name)
     server->data_mutex = _mutex_new();
     server->data_cond = _cond_new();
 
+    server->log_fd = -1;
+
     return server;
 }
 
@@ -407,15 +420,157 @@ static void test_server_free(TestServer *server)
         close(server->fds[i]);
     }
 
+    if (server->log_fd != -1) {
+        close(server->log_fd);
+    }
+
     unlink(server->socket_path);
     g_free(server->socket_path);
 
     _cond_free(server->data_cond);
     _mutex_free(server->data_mutex);
 
+    g_free(server->chr_name);
     g_free(server);
 }
 
+static void wait_for_log_fd(TestServer *s)
+{
+    gint64 end_time;
+
+    g_mutex_lock(s->data_mutex);
+    end_time = _get_time() + 5 * G_TIME_SPAN_SECOND;
+    while (s->log_fd == -1) {
+        if (!_cond_wait_until(s->data_cond, s->data_mutex, end_time)) {
+            /* timeout has passed */
+            g_assert(s->log_fd != -1);
+            break;
+        }
+    }
+
+    g_mutex_unlock(s->data_mutex);
+}
+
+static void write_guest_mem(TestServer *s, uint32 seed)
+{
+    uint32_t *guest_mem;
+    int i, j;
+    size_t size;
+
+    wait_for_fds(s);
+
+    /* iterate all regions */
+    for (i = 0; i < s->fds_num; i++) {
+
+        /* We'll write only the region statring at 0x0 */
+        if (s->memory.regions[i].guest_phys_addr != 0x0) {
+            continue;
+        }
+
+        g_assert_cmpint(s->memory.regions[i].memory_size, >, 1024);
+
+        size = s->memory.regions[i].memory_size +
+            s->memory.regions[i].mmap_offset;
+
+        guest_mem = mmap(0, size, PROT_READ | PROT_WRITE,
+                         MAP_SHARED, s->fds[i], 0);
+
+        g_assert(guest_mem != MAP_FAILED);
+        guest_mem += (s->memory.regions[i].mmap_offset / sizeof(*guest_mem));
+
+        for (j = 0; j < 256; j++) {
+            guest_mem[j] = seed + j;
+        }
+
+        munmap(guest_mem, s->memory.regions[i].memory_size);
+        break;
+    }
+}
+
+static guint64 get_log_size(TestServer *s)
+{
+    guint64 log_size = 0;
+    int i;
+
+    for (i = 0; i < s->memory.nregions; ++i) {
+        VhostUserMemoryRegion *reg = &s->memory.regions[i];
+        guint64 last = range_get_last(reg->guest_phys_addr,
+                                       reg->memory_size);
+        log_size = MAX(log_size, last / (8 * VHOST_LOG_PAGE) + 1);
+    }
+
+    return log_size;
+}
+
+static void test_migrate(void)
+{
+    TestServer *s = test_server_new("src");
+    TestServer *dest = test_server_new("dest");
+    const char *uri = "tcp:127.0.0.1:1234";
+    QTestState *global = global_qtest, *from, *to;
+    gchar *cmd;
+    QDict *rsp;
+    guint8 *log;
+    guint64 size;
+
+    cmd = GET_QEMU_CMDE(s, 2, "");
+    from = qtest_start(cmd);
+    g_free(cmd);
+
+    wait_for_fds(s);
+    size = get_log_size(s);
+    g_assert_cmpint(size, ==, (2 * 1024 * 1024) / (VHOST_LOG_PAGE * 8));
+
+    cmd = GET_QEMU_CMDE(dest, 2, " -incoming %s", uri);
+    to = qtest_init(cmd);
+    g_free(cmd);
+
+    /* slow down migration to have time to fiddle with log */
+    /* TODO: qtest could learn to break on some places */
+    rsp = qmp("{ 'execute': 'migrate_set_speed',"
+              "'arguments': { 'value': 10 } }");
+    g_assert(qdict_haskey(rsp, "return"));
+    QDECREF(rsp);
+
+    cmd = g_strdup_printf("{ 'execute': 'migrate',"
+                          "'arguments': { 'uri': '%s' } }",
+                          uri);
+    rsp = qmp(cmd);
+    g_free(cmd);
+    g_assert(qdict_haskey(rsp, "return"));
+    QDECREF(rsp);
+
+    wait_for_log_fd(s);
+
+    log = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, s->log_fd, 0);
+    g_assert(log != MAP_FAILED);
+
+    /* modify first page */
+    write_guest_mem(s, 0x42);
+    log[0] = 1;
+    munmap(log, size);
+
+    /* speed things up */
+    rsp = qmp("{ 'execute': 'migrate_set_speed',"
+              "'arguments': { 'value': 0 } }");
+    g_assert(qdict_haskey(rsp, "return"));
+    QDECREF(rsp);
+
+    qmp_eventwait("STOP");
+
+    global_qtest = to;
+    qmp_eventwait("RESUME");
+
+    read_guest_mem(dest);
+
+    qtest_quit(to);
+    test_server_free(dest);
+    qtest_quit(from);
+    test_server_free(s);
+
+    global_qtest = global;
+}
+
 static const char *init_hugepagefs(void)
 {
     const char *path;
@@ -474,6 +629,7 @@ int main(int argc, char **argv)
     g_free(qemu_cmd);
 
     qtest_add_data_func("/vhost-user/read-guest-mem", server, read_guest_mem);
+    qtest_add_func("/vhost-user/migrate", test_migrate);
 
     ret = g_test_run();
 
-- 
2.4.3

[Qemu-devel] [PATCH v3 16/16] vhost-user-test: check ownership during migration

[marcandre.lureau]

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

Check that backend source and destination do not have simultaneous
ownership during migration.

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 dcc14bf..f78483d 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -371,6 +371,10 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         g_cond_signal(s->data_cond);
         break;
 
+    case VHOST_USER_RESET_OWNER:
+        s->fds_num = 0;
+        break;
+
     default:
         break;
     }
@@ -502,12 +506,37 @@ static guint64 get_log_size(TestServer *s)
     return log_size;
 }
 
+typedef struct TestMigrateSource {
+    GSource source;
+    TestServer *src;
+    TestServer *dest;
+} TestMigrateSource;
+
+static gboolean
+test_migrate_source_check(GSource *source)
+{
+    TestMigrateSource *t = (TestMigrateSource *)source;
+    gboolean overlap = t->src->fds_num > 0 && t->dest->fds_num > 0;
+
+    g_assert(!overlap);
+
+    return FALSE;
+}
+
+GSourceFuncs test_migrate_source_funcs = {
+    NULL,
+    test_migrate_source_check,
+    NULL,
+    NULL
+};
+
 static void test_migrate(void)
 {
     TestServer *s = test_server_new("src");
     TestServer *dest = test_server_new("dest");
     const char *uri = "tcp:127.0.0.1:1234";
     QTestState *global = global_qtest, *from, *to;
+    GSource *source;
     gchar *cmd;
     QDict *rsp;
     guint8 *log;
@@ -525,6 +554,12 @@ static void test_migrate(void)
     to = qtest_init(cmd);
     g_free(cmd);
 
+    source = g_source_new(&test_migrate_source_funcs,
+                          sizeof(TestMigrateSource));
+    ((TestMigrateSource *)source)->src = s;
+    ((TestMigrateSource *)source)->dest = dest;
+    g_source_attach(source, NULL);
+
     /* slow down migration to have time to fiddle with log */
     /* TODO: qtest could learn to break on some places */
     rsp = qmp("{ 'execute': 'migrate_set_speed',"
@@ -563,6 +598,9 @@ static void test_migrate(void)
 
     read_guest_mem(dest);
 
+    g_source_destroy(source);
+    g_source_unref(source);
+
     qtest_quit(to);
     test_server_free(dest);
     qtest_quit(from);
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 06/16] vhost: use variable arguments for vhost_call()

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 02:40:42PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> It is useful to pass extra arguments to the funtions, for
> various backend needs.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Let's not do this.
It's probably time to replace vhost_call with an ops struct of functions.
This will solve this in a clean way.


> ---
>  hw/virtio/vhost-backend.c         | 4 ++--
>  hw/virtio/vhost-user.c            | 4 ++--
>  include/hw/virtio/vhost-backend.h | 6 ++++--
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 4d68a27..7255089 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -14,8 +14,8 @@
>  
>  #include <sys/ioctl.h>
>  
> -static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
> -                             void *arg)
> +static int vhost_kernel_call(struct vhost_dev *dev,
> +                             unsigned long int request, void *arg, ...)
>  {
>      int fd = (uintptr_t) dev->opaque;
>  
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 4993b63..8b6d7e7 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -190,8 +190,8 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
>              0 : -1;
>  }
>  
> -static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> -        void *arg)
> +static int vhost_user_call(struct vhost_dev *dev,
> +                           unsigned long int request, void *arg, ...)
>  {
>      VhostUserMsg msg;
>      VhostUserRequest msg_request;
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index e472f29..36fa0f7 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -11,6 +11,8 @@
>  #ifndef VHOST_BACKEND_H_
>  #define VHOST_BACKEND_H_
>  
> +#include <stdarg.h>
> +
>  typedef enum VhostBackendType {
>      VHOST_BACKEND_TYPE_NONE = 0,
>      VHOST_BACKEND_TYPE_KERNEL = 1,
> @@ -20,8 +22,8 @@ typedef enum VhostBackendType {
>  
>  struct vhost_dev;
>  
> -typedef int (*vhost_call)(struct vhost_dev *dev, unsigned long int request,
> -             void *arg);
> +typedef int (*vhost_call)(struct vhost_dev *dev,
> +                          unsigned long int request, void *arg, ...);
>  typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
>  typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
>  
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v3 00/16] vhost-user: add migration log support (for 2.5)

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 02:40:36PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> The following series implement shareable log for vhost-user to support
> memory tracking during live migration. On qemu-side, the solution is
> fairly straightfoward since vhost already supports the dirty log, only
> vhost-user couldn't access the log memory until then.
> 
> The series is based on top of "protocol feature negotiation" series
> proposed earlier by Michael S. Tsirkinm and "vhost user: Add live
> migration" series from Thibaut Collet .

I'd prefer to see the whole series included in the post.

This looks pretty clean, I have some minor comments.


> Since v2:
> - changed some patch summary
> - added migration tests
> - added a patch to replace error message with a trace
> 
> The development branch I used is:
> https://github.com/elmarco/qemu branch "vhost-user"
> 
> More comments welcome!
> 
> Marc-André Lureau (16):
>   configure: probe for memfd
>   util: add linux-only memfd fallback
>   util: add memfd helpers
>   vhost: alloc shareable log
>   vhost: document log resizing
>   vhost: use variable arguments for vhost_call()
>   vhost-user: start and end the va_list
>   vhost-user: send log shm fd along with log_base
>   vhost-user: document migration log
>   net: add trace_vhost_user_event
>   vhost-user-test: move wait_for_fds() out
>   vhost-user-test: remove useless static check
>   vhost-user-test: wrap server in TestServer struct
>   vhost-user-test: learn to tweak various qemu arguments
>   vhost-user-test: add live-migration test
>   vhost-user-test: check ownership during migration
> 
>  configure                         |  19 ++
>  docs/specs/vhost-user.txt         |  42 +++++
>  hw/virtio/vhost-backend.c         |   4 +-
>  hw/virtio/vhost-user.c            |  52 ++++--
>  hw/virtio/vhost.c                 |  45 ++++-
>  include/hw/virtio/vhost-backend.h |   6 +-
>  include/hw/virtio/vhost.h         |   3 +-
>  include/qemu/memfd.h              |  24 +++
>  net/vhost-user.c                  |   5 +-
>  tests/vhost-user-test.c           | 366 +++++++++++++++++++++++++++++++-------
>  trace-events                      |   3 +
>  util/Makefile.objs                |   2 +-
>  util/memfd.c                      | 126 +++++++++++++
>  13 files changed, 602 insertions(+), 95 deletions(-)
>  create mode 100644 include/qemu/memfd.h
>  create mode 100644 util/memfd.c
> 
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v3 04/16] vhost: alloc shareable log

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 02:40:40PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> If the backend is of type VHOST_BACKEND_TYPE_USER, allocate
> shareable memory.
> 
> Note: vhost_log_get() can use a global "vhost_log" that can be shared by
> several vhost devices. We may want instead a common shareable log and a
> common non-shareable one.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Shouldn't this be limited to when the appropriate feature flag
is negotiated?


> ---
>  hw/virtio/vhost.c         | 38 +++++++++++++++++++++++++++++++-------
>  include/hw/virtio/vhost.h |  3 ++-
>  2 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 2712c6f..862e786 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -18,6 +18,7 @@
>  #include "qemu/atomic.h"
>  #include "qemu/range.h"
>  #include "qemu/error-report.h"
> +#include "qemu/memfd.h"
>  #include <linux/vhost.h>
>  #include "exec/address-spaces.h"
>  #include "hw/virtio/virtio-bus.h"
> @@ -286,20 +287,34 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
>      }
>      return log_size;
>  }
> -static struct vhost_log *vhost_log_alloc(uint64_t size)
> +
> +static struct vhost_log *vhost_log_alloc(uint64_t size, bool share)
>  {
> -    struct vhost_log *log = g_malloc0(sizeof *log + size * sizeof(*(log->log)));
> +    struct vhost_log *log;
> +    uint64_t logsize = size * sizeof(*(log->log));
> +    int fd = -1;
> +
> +    log = g_new0(struct vhost_log, 1);
> +    if (share) {
> +        log->log = qemu_memfd_alloc("vhost-log", logsize,
> +                                    F_SEAL_GROW|F_SEAL_SHRINK|F_SEAL_SEAL, &fd);
> +        memset(log->log, 0, logsize);
> +    } else {
> +        log->log = g_malloc0(logsize);
> +    }
>  
>      log->size = size;
>      log->refcnt = 1;
> +    log->fd = fd;
>  
>      return log;
>  }
>  
> -static struct vhost_log *vhost_log_get(uint64_t size)
> +static struct vhost_log *vhost_log_get(uint64_t size, bool share)
>  {
> -    if (!vhost_log || vhost_log->size != size) {
> -        vhost_log = vhost_log_alloc(size);
> +    if (!vhost_log || vhost_log->size != size ||
> +        (share && vhost_log->fd == -1)) {
> +        vhost_log = vhost_log_alloc(size, share);
>      } else {
>          ++vhost_log->refcnt;
>      }
> @@ -324,13 +339,21 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
>          if (vhost_log == log) {
>              vhost_log = NULL;
>          }
> +
> +        if (log->fd == -1) {
> +            g_free(log->log);
> +        } else {
> +            qemu_memfd_free(log->log, log->size * sizeof(*(log->log)),
> +                            log->fd);
> +        }
>          g_free(log);
>      }
>  }
>  
>  static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t size)
>  {
> -    struct vhost_log *log = vhost_log_get(size);
> +    bool share = dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
> +    struct vhost_log *log = vhost_log_get(size, share);
>      uint64_t log_base = (uintptr_t)log->log;
>      int r;
>  
> @@ -1136,9 +1159,10 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>  
>      if (hdev->log_enabled) {
>          uint64_t log_base;
> +        bool share = hdev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER;
>  
>          hdev->log_size = vhost_get_log_size(hdev);
> -        hdev->log = vhost_log_get(hdev->log_size);
> +        hdev->log = vhost_log_get(hdev->log_size, share);
>          log_base = (uintptr_t)hdev->log->log;
>          r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
>                                          hdev->log_size ? &log_base : NULL);
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 6467c73..ab1dcac 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -31,7 +31,8 @@ typedef unsigned long vhost_log_chunk_t;
>  struct vhost_log {
>      unsigned long long size;
>      int refcnt;
> -    vhost_log_chunk_t log[0];
> +    int fd;
> +    vhost_log_chunk_t *log;
>  };
>  
>  struct vhost_memory;
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v3 08/16] vhost-user: send log shm fd along with log_base

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 02:40:44PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Send the shm for the dirty pages logging if the backend support

supports

> VHOST_USER_PROTOCOL_F_LOG_SHMFD.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/virtio/vhost-user.c | 16 ++++++++++++++--
>  hw/virtio/vhost.c      |  5 +++--
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 21ecbcd..b2f46a9 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -26,7 +26,9 @@
>  #define VHOST_MEMORY_MAX_NREGIONS    8
>  
>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
> -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
> +
> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
> +#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
>  
>  typedef enum VhostUserRequest {
>      VHOST_USER_NONE = 0,
> @@ -215,8 +217,18 @@ static int vhost_user_call(struct vhost_dev *dev,
>          need_reply = 1;
>          break;
>  
> +    case VHOST_USER_SET_LOG_BASE: {
> +        struct vhost_log *log = va_arg(ap, struct vhost_log *);
> +
> +        if (__virtio_has_feature(dev->protocol_features,
> +                                 VHOST_USER_PROTOCOL_F_LOG_SHMFD) &&
> +            log->fd != -1) {
> +            fds[fd_num++] = log->fd;
> +        }
> +    }
> +        /* fall through */
> +

Don't add {} like that please. Just move declarations to the top level.


>      case VHOST_USER_SET_FEATURES:
> -    case VHOST_USER_SET_LOG_BASE:
>          msg.u64 = *((__u64 *) arg);
>          msg.size = sizeof(m.u64);
>          break;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 057d548..ed8b1a5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -359,7 +359,7 @@ 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_call(dev, VHOST_SET_LOG_BASE, &log_base);
> +    r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base, log);
>      assert(r >= 0);
>      vhost_log_put(dev, true);
>      dev->log = log;
> @@ -1167,7 +1167,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
>          hdev->log = vhost_log_get(hdev->log_size, share);
>          log_base = (uintptr_t)hdev->log->log;
>          r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> -                                        hdev->log_size ? &log_base : NULL);
> +                                        hdev->log_size ? &log_base : NULL,
> +                                        hdev->log);
>          if (r < 0) {
>              r = -errno;
>              goto fail_log;
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v3 00/16] vhost-user: add migration log support (for 2.5)

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 02:40:36PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> The following series implement shareable log for vhost-user to support
> memory tracking during live migration. On qemu-side, the solution is
> fairly straightfoward since vhost already supports the dirty log, only
> vhost-user couldn't access the log memory until then.
> 
> The series is based on top of "protocol feature negotiation" series
> proposed earlier by Michael S. Tsirkinm and "vhost user: Add live
> migration" series from Thibaut Collet .

I've been thinking about this. I wonder whether an alternative
approach would make sense.
Specifically, write protect the pages we want to track in host
PTEs; on a page fault, log the change (in kernel) and restart.

This could work for all of vhost, userspace qemu, vhost-user
transparently (so we don't need to teach dpdk to do
dirty logging).
It would be more robust since it removes the ability for
a buggy userspace to change memory without logging it.

And it would be faster since log is only set on the 1st
write, afterwards page is writeable - so no fault.

It's a kernel patch so more work.

I guess we can support both approaches (e.g. for old kernels)
but I thought I'd through this out there.


> Since v2:
> - changed some patch summary
> - added migration tests
> - added a patch to replace error message with a trace
> 
> The development branch I used is:
> https://github.com/elmarco/qemu branch "vhost-user"
> 
> More comments welcome!
> 
> Marc-André Lureau (16):
>   configure: probe for memfd
>   util: add linux-only memfd fallback
>   util: add memfd helpers
>   vhost: alloc shareable log
>   vhost: document log resizing
>   vhost: use variable arguments for vhost_call()
>   vhost-user: start and end the va_list
>   vhost-user: send log shm fd along with log_base
>   vhost-user: document migration log
>   net: add trace_vhost_user_event
>   vhost-user-test: move wait_for_fds() out
>   vhost-user-test: remove useless static check
>   vhost-user-test: wrap server in TestServer struct
>   vhost-user-test: learn to tweak various qemu arguments
>   vhost-user-test: add live-migration test
>   vhost-user-test: check ownership during migration
> 
>  configure                         |  19 ++
>  docs/specs/vhost-user.txt         |  42 +++++
>  hw/virtio/vhost-backend.c         |   4 +-
>  hw/virtio/vhost-user.c            |  52 ++++--
>  hw/virtio/vhost.c                 |  45 ++++-
>  include/hw/virtio/vhost-backend.h |   6 +-
>  include/hw/virtio/vhost.h         |   3 +-
>  include/qemu/memfd.h              |  24 +++
>  net/vhost-user.c                  |   5 +-
>  tests/vhost-user-test.c           | 366 +++++++++++++++++++++++++++++++-------
>  trace-events                      |   3 +
>  util/Makefile.objs                |   2 +-
>  util/memfd.c                      | 126 +++++++++++++
>  13 files changed, 602 insertions(+), 95 deletions(-)
>  create mode 100644 include/qemu/memfd.h
>  create mode 100644 util/memfd.c
> 
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v3 06/16] vhost: use variable arguments for vhost_call()

[Marc-André Lureau]

On Wed, Sep 16, 2015 at 4:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Let's not do this.
> It's probably time to replace vhost_call with an ops struct of functions.
> This will solve this in a clean way.


done

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 08/16] vhost-user: send log shm fd along with log_base

[Marc-André Lureau]

On Wed, Sep 16, 2015 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> supports
>

ok

>> VHOST_USER_PROTOCOL_F_LOG_SHMFD.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  hw/virtio/vhost-user.c | 16 ++++++++++++++--
>>  hw/virtio/vhost.c      |  5 +++--
>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 21ecbcd..b2f46a9 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -26,7 +26,9 @@
>>  #define VHOST_MEMORY_MAX_NREGIONS    8
>>
>>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
>> -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x0ULL
>> +
>> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
>> +#define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
>>
>>  typedef enum VhostUserRequest {
>>      VHOST_USER_NONE = 0,
>> @@ -215,8 +217,18 @@ static int vhost_user_call(struct vhost_dev *dev,
>>          need_reply = 1;
>>          break;
>>
>> +    case VHOST_USER_SET_LOG_BASE: {
>> +        struct vhost_log *log = va_arg(ap, struct vhost_log *);
>> +
>> +        if (__virtio_has_feature(dev->protocol_features,
>> +                                 VHOST_USER_PROTOCOL_F_LOG_SHMFD) &&
>> +            log->fd != -1) {
>> +            fds[fd_num++] = log->fd;
>> +        }
>> +    }
>> +        /* fall through */
>> +
>
> Don't add {} like that please. Just move declarations to the top level.

Done with the vhost_call() refactoring


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 04/16] vhost: alloc shareable log

[Marc-André Lureau]

Hi

On Wed, Sep 16, 2015 at 4:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Shouldn't this be limited to when the appropriate feature flag
> is negotiated?


That's doable, I'll add it after the feature protocol is added (not in
this commit)

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 03/16] util: add memfd helpers

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 02:40:39PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add qemu_memfd_alloc/free() helpers.
> 
> The function helps to allocate and seal a memfd, and implements an
> open/unlink/mmap fallback for system that do not support memfd.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/memfd.h |  4 +++
>  util/memfd.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 76 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h
> index 8b1fe6a..950fb88 100644
> --- a/include/qemu/memfd.h
> +++ b/include/qemu/memfd.h
> @@ -17,4 +17,8 @@
>  #define F_SEAL_WRITE    0x0008  /* prevent writes */
>  #endif
>  
> +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
> +                       int *fd);
> +void qemu_memfd_free(void *ptr, size_t size, int fd);
> +
>  #endif /* QEMU_MEMFD_H */
> diff --git a/util/memfd.c b/util/memfd.c
> index a98d57e..8b2b785 100644
> --- a/util/memfd.c
> +++ b/util/memfd.c
> @@ -27,6 +27,14 @@
>  
>  #include "config-host.h"
>  
> +#include <glib.h>
> +#include <glib/gprintf.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
> +#include <sys/mman.h>
> +
>  #include "qemu/memfd.h"
>  
>  #ifdef CONFIG_MEMFD
> @@ -44,13 +52,75 @@
>  #define MFD_ALLOW_SEALING 0x0002U
>  #endif
>  
> -static inline int memfd_create(const char *name, unsigned int flags)
> +static int memfd_create(const char *name, unsigned int flags)
>  {
>      return syscall(__NR_memfd_create, name, flags);
>  }
>  #else /* !LINUX */
> -static inline int memfd_create(const char *name, unsigned int flags)
> +static int memfd_create(const char *name, unsigned int flags)
>  {
>      return -1;
>  }
>  #endif
> +
> +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
> +                       int *fd)
> +{
> +    void *ptr;
> +    int mfd;
> +
> +    mfd = memfd_create(name, MFD_ALLOW_SEALING|MFD_CLOEXEC);


Hmm. Does this interact correctly with the -mem-prealloc flag?

> +    if (mfd != -1) {
> +        if (ftruncate(mfd, size) == -1) {

Any limitations on size?

> +            perror("ftruncate");
> +            close(mfd);
> +            return NULL;
> +        }
> +
> +        if (fcntl(mfd, F_ADD_SEALS, seals) == -1) {
> +            perror("fcntl");
> +            close(mfd);
> +            return NULL;
> +        }
> +    } else {
> +        const char *tmpdir = getenv("TMPDIR");
> +        gchar *fname;
> +
> +        tmpdir = tmpdir ? tmpdir : "/tmp";
> +
> +        fname = g_strdup_printf("%s/memfd-XXXXXX", tmpdir);

This means there's now work to be done to set up selinux
to allow QEMU creating memfd under /tmp.

Maybe it's better to just fail gracefully for now.

> +        mfd = mkstemp(fname);
> +        unlink(fname);
> +        g_free(fname);
> +
> +        if (mfd == -1) {
> +            perror("mkstemp");
> +            return NULL;
> +        }
> +
> +        if (ftruncate(mfd, size) == -1) {
> +            perror("ftruncate");
> +            close(mfd);
> +            return NULL;
> +        }
> +    }
> +
> +    ptr = mmap(0, size, PROT_READ|PROT_WRITE, MAP_SHARED, mfd, 0);

Pls add space around | here and elsewhere.


> +    if (ptr == MAP_FAILED) {
> +        perror("mmap");
> +        close(mfd);
> +        return NULL;
> +    }
> +
> +    *fd = mfd;
> +    return ptr;
> +}
> +
> +void qemu_memfd_free(void *ptr, size_t size, int fd)
> +{
> +    if (ptr) {
> +        munmap(ptr, size);
> +    }
> +
> +    close(fd);

I notice you close fd unconditionally, but it's only returned
on success above. So this will close an uninitialized one?

> +}
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH v3 03/16] util: add memfd helpers

[Marc-André Lureau]

----- Original Message -----
> On Thu, Aug 06, 2015 at 02:40:39PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Add qemu_memfd_alloc/free() helpers.
> > 
> > The function helps to allocate and seal a memfd, and implements an
> > open/unlink/mmap fallback for system that do not support memfd.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/qemu/memfd.h |  4 +++
> >  util/memfd.c         | 74
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 76 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h
> > index 8b1fe6a..950fb88 100644
> > --- a/include/qemu/memfd.h
> > +++ b/include/qemu/memfd.h
> > @@ -17,4 +17,8 @@
> >  #define F_SEAL_WRITE    0x0008  /* prevent writes */
> >  #endif
> >  
> > +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
> > +                       int *fd);
> > +void qemu_memfd_free(void *ptr, size_t size, int fd);
> > +
> >  #endif /* QEMU_MEMFD_H */
> > diff --git a/util/memfd.c b/util/memfd.c
> > index a98d57e..8b2b785 100644
> > --- a/util/memfd.c
> > +++ b/util/memfd.c
> > @@ -27,6 +27,14 @@
> >  
> >  #include "config-host.h"
> >  
> > +#include <glib.h>
> > +#include <glib/gprintf.h>
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <fcntl.h>
> > +#include <sys/mman.h>
> > +
> >  #include "qemu/memfd.h"
> >  
> >  #ifdef CONFIG_MEMFD
> > @@ -44,13 +52,75 @@
> >  #define MFD_ALLOW_SEALING 0x0002U
> >  #endif
> >  
> > -static inline int memfd_create(const char *name, unsigned int flags)
> > +static int memfd_create(const char *name, unsigned int flags)
> >  {
> >      return syscall(__NR_memfd_create, name, flags);
> >  }
> >  #else /* !LINUX */
> > -static inline int memfd_create(const char *name, unsigned int flags)
> > +static int memfd_create(const char *name, unsigned int flags)
> >  {
> >      return -1;
> >  }
> >  #endif
> > +
> > +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
> > +                       int *fd)
> > +{
> > +    void *ptr;
> > +    int mfd;
> > +
> > +    mfd = memfd_create(name, MFD_ALLOW_SEALING|MFD_CLOEXEC);
> 
> 
> Hmm. Does this interact correctly with the -mem-prealloc flag?

It's unrelated imho. It's helper here.

In the rest of the series, it's used at runtime when migrating with variable size (today code doesn't prealloc that either)

> 
> > +    if (mfd != -1) {
> > +        if (ftruncate(mfd, size) == -1) {
> 
> Any limitations on size?

not that I know (reading memfd_create)

> 
> > +            perror("ftruncate");
> > +            close(mfd);
> > +            return NULL;
> > +        }
> > +
> > +        if (fcntl(mfd, F_ADD_SEALS, seals) == -1) {
> > +            perror("fcntl");
> > +            close(mfd);
> > +            return NULL;
> > +        }
> > +    } else {
> > +        const char *tmpdir = getenv("TMPDIR");
> > +        gchar *fname;
> > +
> > +        tmpdir = tmpdir ? tmpdir : "/tmp";
> > +
> > +        fname = g_strdup_printf("%s/memfd-XXXXXX", tmpdir);
> 
> This means there's now work to be done to set up selinux
> to allow QEMU creating memfd under /tmp.

doesn't sound unreasonable to me

> 
> Maybe it's better to just fail gracefully for now.

it's a fallback, but sure we can remove it and add it back later if needed

> > +        mfd = mkstemp(fname);
> > +        unlink(fname);
> > +        g_free(fname);
> > +
> > +        if (mfd == -1) {
> > +            perror("mkstemp");
> > +            return NULL;
> > +        }
> > +
> > +        if (ftruncate(mfd, size) == -1) {
> > +            perror("ftruncate");
> > +            close(mfd);
> > +            return NULL;
> > +        }
> > +    }
> > +
> > +    ptr = mmap(0, size, PROT_READ|PROT_WRITE, MAP_SHARED, mfd, 0);
> 
> Pls add space around | here and elsewhere.
> 

ok
> 
> > +    if (ptr == MAP_FAILED) {
> > +        perror("mmap");
> > +        close(mfd);
> > +        return NULL;
> > +    }
> > +
> > +    *fd = mfd;
> > +    return ptr;
> > +}
> > +
> > +void qemu_memfd_free(void *ptr, size_t size, int fd)
> > +{
> > +    if (ptr) {
> > +        munmap(ptr, size);
> > +    }
> > +
> > +    close(fd);
> 
> I notice you close fd unconditionally, but it's only returned
> on success above. So this will close an uninitialized one?

Ok, I'll add a -1 check

Re: [Qemu-devel] [PATCH v3 03/16] util: add memfd helpers

[Michael S. Tsirkin]

On Tue, Sep 29, 2015 at 11:25:04AM -0400, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
> > On Thu, Aug 06, 2015 at 02:40:39PM +0200, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > Add qemu_memfd_alloc/free() helpers.
> > > 
> > > The function helps to allocate and seal a memfd, and implements an
> > > open/unlink/mmap fallback for system that do not support memfd.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  include/qemu/memfd.h |  4 +++
> > >  util/memfd.c         | 74
> > >  ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 76 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/qemu/memfd.h b/include/qemu/memfd.h
> > > index 8b1fe6a..950fb88 100644
> > > --- a/include/qemu/memfd.h
> > > +++ b/include/qemu/memfd.h
> > > @@ -17,4 +17,8 @@
> > >  #define F_SEAL_WRITE    0x0008  /* prevent writes */
> > >  #endif
> > >  
> > > +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
> > > +                       int *fd);
> > > +void qemu_memfd_free(void *ptr, size_t size, int fd);
> > > +
> > >  #endif /* QEMU_MEMFD_H */
> > > diff --git a/util/memfd.c b/util/memfd.c
> > > index a98d57e..8b2b785 100644
> > > --- a/util/memfd.c
> > > +++ b/util/memfd.c
> > > @@ -27,6 +27,14 @@
> > >  
> > >  #include "config-host.h"
> > >  
> > > +#include <glib.h>
> > > +#include <glib/gprintf.h>
> > > +
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <fcntl.h>
> > > +#include <sys/mman.h>
> > > +
> > >  #include "qemu/memfd.h"
> > >  
> > >  #ifdef CONFIG_MEMFD
> > > @@ -44,13 +52,75 @@
> > >  #define MFD_ALLOW_SEALING 0x0002U
> > >  #endif
> > >  
> > > -static inline int memfd_create(const char *name, unsigned int flags)
> > > +static int memfd_create(const char *name, unsigned int flags)
> > >  {
> > >      return syscall(__NR_memfd_create, name, flags);
> > >  }
> > >  #else /* !LINUX */
> > > -static inline int memfd_create(const char *name, unsigned int flags)
> > > +static int memfd_create(const char *name, unsigned int flags)
> > >  {
> > >      return -1;
> > >  }
> > >  #endif
> > > +
> > > +void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals,
> > > +                       int *fd)
> > > +{
> > > +    void *ptr;
> > > +    int mfd;
> > > +
> > > +    mfd = memfd_create(name, MFD_ALLOW_SEALING|MFD_CLOEXEC);
> > 
> > 
> > Hmm. Does this interact correctly with the -mem-prealloc flag?
> 
> It's unrelated imho. It's helper here.
> 
> In the rest of the series, it's used at runtime when migrating with variable size (today code doesn't prealloc that either)

Yes but I think this means it will fault on access even if user
requested -realtime mlock=on.

> > 
> > > +    if (mfd != -1) {
> > > +        if (ftruncate(mfd, size) == -1) {
> > 
> > Any limitations on size?
> 
> not that I know (reading memfd_create)
> 
> > 
> > > +            perror("ftruncate");
> > > +            close(mfd);
> > > +            return NULL;
> > > +        }
> > > +
> > > +        if (fcntl(mfd, F_ADD_SEALS, seals) == -1) {
> > > +            perror("fcntl");
> > > +            close(mfd);
> > > +            return NULL;
> > > +        }
> > > +    } else {
> > > +        const char *tmpdir = getenv("TMPDIR");
> > > +        gchar *fname;
> > > +
> > > +        tmpdir = tmpdir ? tmpdir : "/tmp";
> > > +
> > > +        fname = g_strdup_printf("%s/memfd-XXXXXX", tmpdir);
> > 
> > This means there's now work to be done to set up selinux
> > to allow QEMU creating memfd under /tmp.
> 
> doesn't sound unreasonable to me
> 
> > 
> > Maybe it's better to just fail gracefully for now.
> 
> it's a fallback, but sure we can remove it and add it back later if needed
> 
> > > +        mfd = mkstemp(fname);
> > > +        unlink(fname);
> > > +        g_free(fname);
> > > +
> > > +        if (mfd == -1) {
> > > +            perror("mkstemp");
> > > +            return NULL;
> > > +        }
> > > +
> > > +        if (ftruncate(mfd, size) == -1) {
> > > +            perror("ftruncate");
> > > +            close(mfd);
> > > +            return NULL;
> > > +        }
> > > +    }
> > > +
> > > +    ptr = mmap(0, size, PROT_READ|PROT_WRITE, MAP_SHARED, mfd, 0);
> > 
> > Pls add space around | here and elsewhere.
> > 
> 
> ok
> > 
> > > +    if (ptr == MAP_FAILED) {
> > > +        perror("mmap");
> > > +        close(mfd);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    *fd = mfd;
> > > +    return ptr;
> > > +}
> > > +
> > > +void qemu_memfd_free(void *ptr, size_t size, int fd)
> > > +{
> > > +    if (ptr) {
> > > +        munmap(ptr, size);
> > > +    }
> > > +
> > > +    close(fd);
> > 
> > I notice you close fd unconditionally, but it's only returned
> > on success above. So this will close an uninitialized one?
> 
> Ok, I'll add a -1 check

Will only work well if you set *fd to -1 on error above ...

-- 
MST