[PATCH v2 0/5] Fix QEMU startup protocol

[PATCH v2 0/5] Fix QEMU startup protocol

[Wei Liu]

This patch series tries to reason about various hardcoded "/local/domain/0" in
libxl and 1) replace them with something sensible, 2) fix QEMU startup
protocol.

Please apply a patch to QEMU traditional first, squash the update to Config.mk
into "libxl: use new QEMU xenstore protocol" to keep the tree bisectable.

The new protocol is introduced in "libxl: use new QEMU xenstore protocol".
Basically it replaces hardcoded "/local/domain/0" with
"/local/domain/$dm_domid".

For both QEMU upstream and traditional running in Dom0, the new protocol is
compatible with the old one, because in those cases $dm_domid is 0.

For QEMU traditional running in stubdom, this protocol is incompatible with
old one. However this is acceptable because we always ships QEMU traditional
with Xen. For Xen 4.5 we should backport Paul's workaround.

For QEMU upstream running in stubdom, there is no compatibility issue because
there is no QEMU upstream stubdom yet. This patch series is also a good start
to avoid making the same mistake in QEMU upstream stubdom.

Wei.

Wei Liu (5):
  libxl: introduce libxl__device_model_xs_path
  libxl: remove device model path in libxl__device_model_destroy
  libxl: use LIBXL_TOOLSTACK_DOMID
  libxl: use new QEMU xenstore protocol
  Revert "x86/hvm: wait for at least one ioreq server to be enabled"

 tools/libxl/libxl.c              |  6 ++---
 tools/libxl/libxl_device.c       |  4 ++-
 tools/libxl/libxl_dm.c           | 55 +++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_dom.c          | 54 ++++++++++++++++++++++++---------------
 tools/libxl/libxl_internal.c     | 22 ++++++++++++++++
 tools/libxl/libxl_internal.h     |  5 ++++
 tools/libxl/libxl_pci.c          | 22 ++++++++--------
 xen/arch/x86/hvm/hvm.c           | 21 ---------------
 xen/include/asm-x86/hvm/domain.h |  1 -
 9 files changed, 130 insertions(+), 60 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] libxl: introduce libxl__device_model_xs_path

[Wei Liu]

Introduce this helper to return xenstore path for device model to avoid
handcoded paths.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.c | 22 ++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  3 +++
 2 files changed, 25 insertions(+)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ddc68ab..8877288 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -555,6 +555,28 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
+char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
+                                  uint32_t domid, const char *format,  ...)
+{
+    char *s, *fmt;
+    va_list ap;
+    int ret;
+
+    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
+                    domid, format);
+
+    va_start(ap, format);
+    ret = vsnprintf(NULL, 0, fmt, ap);
+    va_end(ap);
+
+    s = libxl__zalloc(gc, ret + 1);
+    va_start(ap, format);
+    ret = vsnprintf(s, ret + 1, fmt, ap);
+    va_end(ap);
+
+    return s;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..9ef2ec6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1794,6 +1794,9 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
+_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
+                                          uint32_t domid,
+                                          const char *format,  ...);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
-- 
1.9.1

[PATCH v2 2/5] libxl: remove device model path in libxl__device_model_destroy

[Wei Liu]

... and not devices_destroy_cb because it is the right place to clean up
device model stuff.

And the path should use LIBXL_TOOLSTACK_DOMID instead of hardcoded 0.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v2:
1. Use helper function to generate xenstore path.
---
 tools/libxl/libxl.c    | 2 --
 tools/libxl/libxl_dm.c | 5 +++++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 088786e..46a43d5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1659,8 +1659,6 @@ static void devices_destroy_cb(libxl__egc *egc,
 
     xs_rm(ctx->xsh, XBT_NULL, libxl__xs_libxl_path(gc, domid));
     xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc,
-                                "/local/domain/0/device-model/%d", domid));
-    xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc,
                                 "/local/domain/%d/hvmloader", domid));
 
     /* This is async operation, we already hold CTX lock */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8599a6a..affd5ed 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1629,6 +1629,11 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
+                                             domid, "");
+    if (!xs_rm(CTX->xsh, XBT_NULL, path))
+        LOG(ERROR, "xs_rm failed for %s", path);
+    /* We should try to destroy the device model anyway. */
     return kill_device_model(gc,
                 GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
 }
-- 
1.9.1

[PATCH v2 3/5] libxl: use LIBXL_TOOLSTACK_DOMID

[Wei Liu]

The function in question is libxl__spawn_local_dm. We should use
LIBXL_TOOLSTACK_DOMID when constructing xenstore path.

Currently LIBXL_TOOLSTACK_DOMID is 0, so this patch introduces no
functional change.

Use helper function to generate xenstore path.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v2:
1. Use helper function to generate xenstore path.
---
 tools/libxl/libxl_dm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index affd5ed..42a893c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1375,7 +1375,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         free(path);
     }
 
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d", domid);
+    path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
     xs_mkdir(ctx->xsh, XBT_NULL, path);
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
@@ -1425,6 +1425,8 @@ retry_transaction:
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
     spawn->xspath = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
+    spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
+                                                domid, "/state");
     spawn->timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     spawn->pidpath = GCSPRINTF("%s/image/device-model-pid", dom_path);
     spawn->midproc_cb = libxl__spawn_record_pid;
-- 
1.9.1

[PATCH v2 4/5] libxl: use new QEMU xenstore protocol

[Wei Liu]

Originally both QEMU traditional and QEMU upstream used hardcoded
/local/domain/0 paths. This patch changes the protocol to use
/local/domain/$dm_domid path.

For QEMU traditional and upstream without stubdom, $dm_domid is 0 so
the path is in fact still /local/domain/0.

For QEMU traditional stubdom, this is incompatible protocol change.
However QEMU traditional is shipped with Xen so we are allowed to do
such change. This change needs to work with corresponding QEMU
traditional changeset.

There is no compatibility issue with QEMU upstream stubdom, because QEMU
upstream stubdom doesn't exist yet.

Watch /local/domain/$dm_domid/device-model/$domid/state, wait until
state turns "running" then unpause guest.

LIBXL_STUBDOM_START_TIMEOUT is the timeout used wait for stubdom to be
ready. My test on a very old machine (Core 2 6400) showed that it might
need more than 20s before the stubdom is ready to serve DomU.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Note:
Config.mk should be updated in this patch as well.
The patch to QEMU traditional can be found at
   <1426243119-24591-1-git-send-email-wei.liu2@citrix.com>

Changes in v2:
1. Use helper to generate xenstore path.
2. Fix xenstore permissions.
3. Squash patch to wait for stubdom here
  3.1. Move libxl__xswait_init to right place.
  3.2. Use helper to generate xenstore path.
  3.3. Use only one libxl__xswait_stop.
---
 tools/libxl/libxl.c          |  4 +++-
 tools/libxl/libxl_device.c   |  4 +++-
 tools/libxl/libxl_dm.c       | 46 ++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_dom.c      | 54 ++++++++++++++++++++++++++++----------------
 tools/libxl/libxl_internal.h |  2 ++
 tools/libxl/libxl_pci.c      | 22 ++++++++++--------
 6 files changed, 97 insertions(+), 35 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 46a43d5..65b0baa 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1033,7 +1033,9 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
     }
 
     if (type == LIBXL_DOMAIN_TYPE_HVM) {
-        path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+        uint32_t dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+        path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
         state = libxl__xs_read(gc, XBT_NULL, path);
         if (state != NULL && !strcmp(state, "paused")) {
             libxl__qemu_traditional_cmd(gc, domid, "continue");
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 0f50d04..0c06dc4 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1188,7 +1188,9 @@ int libxl__wait_for_device_model_deprecated(libxl__gc *gc,
                                  void *check_callback_userdata)
 {
     char *path;
-    path = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
+    uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
     return libxl__xenstore_child_wait_deprecated(gc, domid,
                                      LIBXL_DEVICE_MODEL_START_TIMEOUT,
                                      "Device Model", path, state, spawning,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 42a893c..33bcbd9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -984,6 +984,8 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
 static void spawn_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
                                             libxl__destroy_domid_state *dis,
                                             int rc);
+static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
+                              int rc, const char *p);
 
 char *libxl__stub_dm_name(libxl__gc *gc, const char *guest_name)
 {
@@ -1113,9 +1115,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
     xs_mkdir(ctx->xsh, t,
-        libxl__sprintf(gc, "/local/domain/0/device-model/%d", guest_domid));
+             libxl__device_model_xs_path(gc, dm_domid, guest_domid, ""));
     xs_set_permissions(ctx->xsh, t,
-        libxl__sprintf(gc, "/local/domain/0/device-model/%d", guest_domid),
+                       libxl__device_model_xs_path(gc, dm_domid,
+                                                   guest_domid, ""),
                        perm, ARRAY_SIZE(perm));
     if (!xs_transaction_end(ctx->xsh, t, 0))
         if (errno == EAGAIN)
@@ -1265,6 +1268,8 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
     STATE_AO_GC(sdss->dm.spawn.ao);
     uint32_t dm_domid = sdss->pvqemu.guest_domid;
 
+    libxl__xswait_init(&sdss->xswait);
+
     if (rc) {
         LOGE(ERROR, "error connecting nics devices");
         goto out;
@@ -1273,10 +1278,45 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
     rc = libxl_domain_unpause(CTX, dm_domid);
     if (rc) goto out;
 
+    sdss->xswait.ao = ao;
+    sdss->xswait.what = GCSPRINTF("Stubdom %u for %u startup",
+                                  dm_domid, sdss->dm.guest_domid);
+    sdss->xswait.path =
+        libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
+                                    "/state");
+    sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
+    sdss->xswait.callback = stubdom_xswait_cb;
+    rc = libxl__xswait_start(gc, &sdss->xswait);
+    if (rc) goto out;
+
+    return;
+
+ out:
+    stubdom_xswait_cb(egc, &sdss->xswait, rc, NULL);
+}
+
+static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
+                              int rc, const char *p)
+{
+    EGC_GC;
+    libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(xswait, *sdss, xswait);
+    uint32_t dm_domid = sdss->pvqemu.guest_domid;
+
+    if (rc) {
+        if (rc == ERROR_TIMEDOUT)
+            LOG(ERROR, "%s: startup timed out", xswait->what);
+        goto out;
+    }
+
+    if (!p) return;
+
+    if (strcmp(p, "running"))
+        return;
  out:
+    libxl__xswait_stop(gc, xswait);
     if (rc) {
         if (dm_domid) {
-            sdss->dis.ao = ao;
+            sdss->dis.ao = sdss->dm.spawn.ao;
             sdss->dis.domid = dm_domid;
             sdss->dis.callback = spawn_stubdom_pvqemu_destroy_cb;
             libxl__destroy_domid(egc, &sdss->dis);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a16d4a1..7c92646 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -846,7 +846,8 @@ int libxl__qemu_traditional_cmd(libxl__gc *gc, uint32_t domid,
                                 const char *cmd)
 {
     char *path = NULL;
-    path = GCSPRINTF("/local/domain/0/device-model/%d/command", domid);
+    uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/command");
     return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
 }
 
@@ -860,11 +861,13 @@ struct libxl__physmap_info {
 
 #define TOOLSTACK_SAVE_VERSION 1
 
-static inline char *restore_helper(libxl__gc *gc, uint32_t domid,
-        uint64_t phys_offset, char *node)
+static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
+                                   uint32_t domid,
+                                   uint64_t phys_offset, char *node)
 {
-    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%"PRIx64"/%s",
-            domid, phys_offset, node);
+    return libxl__device_model_xs_path(gc, dm_domid, domid,
+                                       "/physmap/%"PRIx64"/%s",
+                                       phys_offset, node);
 }
 
 int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
@@ -878,6 +881,7 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
     uint32_t count = 0, version = 0;
     struct libxl__physmap_info* pi;
     char *xs_path;
+    uint32_t dm_domid;
 
     LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size);
 
@@ -903,20 +907,23 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
         return -1;
     }
 
+    dm_domid = libxl_get_stubdom_id(CTX, domid);
     for (i = 0; i < count; i++) {
         pi = (struct libxl__physmap_info*) ptr;
         ptr += sizeof(struct libxl__physmap_info) + pi->namelen;
 
-        xs_path = restore_helper(gc, domid, pi->phys_offset, "start_addr");
+        xs_path = restore_helper(gc, dm_domid, domid,
+                                 pi->phys_offset, "start_addr");
         ret = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->start_addr);
         if (ret)
             return -1;
-        xs_path = restore_helper(gc, domid, pi->phys_offset, "size");
+        xs_path = restore_helper(gc, dm_domid, domid, pi->phys_offset, "size");
         ret = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->size);
         if (ret)
             return -1;
         if (pi->namelen > 0) {
-            xs_path = restore_helper(gc, domid, pi->phys_offset, "name");
+            xs_path = restore_helper(gc, dm_domid, domid,
+                                     pi->phys_offset, "name");
             ret = libxl__xs_write(gc, 0, xs_path, "%s", pi->name);
             if (ret)
                 return -1;
@@ -969,10 +976,11 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty
     const char *got;
 
     if (!lds->cmd_path) {
-        lds->cmd_path = GCSPRINTF(
-                   "/local/domain/0/device-model/%u/logdirty/cmd", domid);
-        lds->ret_path = GCSPRINTF(
-                   "/local/domain/0/device-model/%u/logdirty/ret", domid);
+        uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+        lds->cmd_path = libxl__device_model_xs_path(gc, dm_domid, domid,
+                                                    "/logdirty/cmd");
+        lds->ret_path = libxl__device_model_xs_path(gc, dm_domid, domid,
+                                                    "/logdirty/ret");
     }
     lds->cmd = enable ? "enable" : "disable";
 
@@ -1491,11 +1499,13 @@ static void domain_suspend_common_done(libxl__egc *egc,
     dss->callback_common_done(egc, dss, ok);
 }
 
-static inline char *physmap_path(libxl__gc *gc, uint32_t domid,
-        char *phys_offset, char *node)
+static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
+                                 uint32_t domid,
+                                 char *phys_offset, char *node)
 {
-    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%s/%s",
-            domid, phys_offset, node);
+    return libxl__device_model_xs_path(gc, dm_domid, domid,
+                                       "/physmap/%s/%s",
+                                       phys_offset, node);
 }
 
 int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
@@ -1510,9 +1520,13 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
     uint8_t *ptr = NULL;
     char **entries = NULL;
     struct libxl__physmap_info *pi;
+    uint32_t dm_domid;
+
+    dm_domid = libxl_get_stubdom_id(CTX, domid);
 
     entries = libxl__xs_directory(gc, 0, GCSPRINTF(
-                "/local/domain/0/device-model/%d/physmap", domid), &num);
+                "/local/domain/%d/device-model/%d/physmap",
+                dm_domid, domid), &num);
     count = num;
 
     *len = sizeof(version) + sizeof(count);
@@ -1535,21 +1549,21 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
             return -1;
         }
 
-        xs_path = physmap_path(gc, domid, phys_offset, "start_addr");
+        xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "start_addr");
         start_addr = libxl__xs_read(gc, 0, xs_path);
         if (start_addr == NULL) {
             LOG(ERROR, "%s is NULL", xs_path);
             return -1;
         }
 
-        xs_path = physmap_path(gc, domid, phys_offset, "size");
+        xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "size");
         size = libxl__xs_read(gc, 0, xs_path);
         if (size == NULL) {
             LOG(ERROR, "%s is NULL", xs_path);
             return -1;
         }
 
-        xs_path = physmap_path(gc, domid, phys_offset, "name");
+        xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "name");
         name = libxl__xs_read(gc, 0, xs_path);
         if (name == NULL)
             namelen = 0;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 9ef2ec6..deb74b9 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -86,6 +86,7 @@
 #define LIBXL_DESTROY_TIMEOUT 10
 #define LIBXL_HOTPLUG_TIMEOUT 10
 #define LIBXL_DEVICE_MODEL_START_TIMEOUT 10
+#define LIBXL_STUBDOM_START_TIMEOUT 30
 #define LIBXL_QEMU_BODGE_TIMEOUT 2
 #define LIBXL_XENCONSOLE_LIMIT 1048576
 #define LIBXL_XENCONSOLE_PROTOCOL "vt100"
@@ -3047,6 +3048,7 @@ typedef struct {
     libxl__dm_spawn_state pvqemu;
     libxl__destroy_domid_state dis;
     libxl__multidev multidev;
+    libxl__xswait_state xswait;
 } libxl__stub_dm_spawn_state;
 
 _hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..394f61c 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -850,11 +850,12 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     int rc = 0;
     char *path;
     char *state, *vdevfn;
+    uint32_t dm_domid;
 
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+    dm_domid = libxl_get_stubdom_id(CTX, domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/parameter",
-                          domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
     if (pcidev->vdevfn) {
         libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
                         pcidev->domain, pcidev->bus, pcidev->dev,
@@ -869,11 +870,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     libxl__qemu_traditional_cmd(gc, domid, "pci-ins");
     rc = libxl__wait_for_device_model_deprecated(gc, domid, NULL, NULL,
                                       pci_ins_check, state);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/parameter",
-                          domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
     vdevfn = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state",
-                          domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
     if ( rc < 0 )
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                    "qemu refused to add device: %s", vdevfn);
@@ -1175,10 +1174,13 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     char *state;
     char *path;
+    uint32_t dm_domid;
 
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+    dm_domid = libxl_get_stubdom_id(CTX, domid);
+
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/parameter", domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
     libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
                     pcidev->bus, pcidev->dev, pcidev->func);
 
@@ -1196,7 +1198,7 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
             return ERROR_FAIL;
         }
     }
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
     xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
 
     return 0;
-- 
1.9.1

[PATCH v2 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled"

[Wei Liu]

This reverts commit dd748d128d86996592afafea02e578cc7d4e6d42.

We don't need this workaround anymore since we have fixed the toolstack
interlock problem that affects stubdom.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/hvm/hvm.c           | 21 ---------------------
 xen/include/asm-x86/hvm/domain.h |  1 -
 2 files changed, 22 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4734d71..32905d0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -892,13 +892,6 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
 
   done:
     spin_unlock(&s->lock);
-
-    /* This check is protected by the domain ioreq server lock. */
-    if ( d->arch.hvm_domain.ioreq_server.waiting )
-    {
-        d->arch.hvm_domain.ioreq_server.waiting = 0;
-        domain_unpause(d);
-    }
 }
 
 static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
@@ -1450,20 +1443,6 @@ int hvm_domain_initialise(struct domain *d)
 
     spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
     INIT_LIST_HEAD(&d->arch.hvm_domain.ioreq_server.list);
-    
-    /*
-     * In the case where a stub domain is providing emulation for
-     * the guest, there is no interlock in the toolstack to prevent
-     * the guest from running before the stub domain is ready.
-     * Hence the domain must remain paused until at least one ioreq
-     * server is created and enabled.
-     */
-    if ( !is_pvh_domain(d) )
-    {
-        domain_pause(d);
-        d->arch.hvm_domain.ioreq_server.waiting = 1;
-    }
-
     spin_lock_init(&d->arch.hvm_domain.irq_lock);
     spin_lock_init(&d->arch.hvm_domain.uc_lock);
 
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 0702bf5..2757c7f 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -83,7 +83,6 @@ struct hvm_domain {
     struct {
         spinlock_t       lock;
         ioservid_t       id;
-        bool_t           waiting;
         struct list_head list;
     } ioreq_server;
     struct hvm_ioreq_server *default_ioreq_server;
-- 
1.9.1

Re: [PATCH v2 1/5] libxl: introduce libxl__device_model_xs_path

[Ian Campbell]

On Thu, 2015-03-19 at 13:18 +0000, Wei Liu wrote:
> Introduce this helper to return xenstore path for device model to avoid
> handcoded paths.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_internal.c | 22 ++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |  3 +++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index ddc68ab..8877288 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -555,6 +555,28 @@ void libxl__update_domain_configuration(libxl__gc *gc,
>      dst->b_info.video_memkb = src->b_info.video_memkb;
>  }
>  
> +char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> +                                  uint32_t domid, const char *format,  ...)
> +{
> +    char *s, *fmt;
> +    va_list ap;
> +    int ret;
> +
> +    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
> +                    domid, format);
> +
> +    va_start(ap, format);
> +    ret = vsnprintf(NULL, 0, fmt, ap);
> +    va_end(ap);
> +
> +    s = libxl__zalloc(gc, ret + 1);
> +    va_start(ap, format);
> +    ret = vsnprintf(s, ret + 1, fmt, ap);
> +    va_end(ap);

Please could you refactor the existing libxl__sprintf into a
libxl__vsprintf (i.e. which takes a va_list, and uses va_copy for the
two calls to vsnprintf). Then implement your new helper in terms of the
libxl__vsprintf.

> +
> +    return s;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 934465a..9ef2ec6 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1794,6 +1794,9 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
>  _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
>    /* Return the system-wide default device model */
>  _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
> +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> +                                          uint32_t domid,
> +                                          const char *format,  ...);
>  
>  /* Check how executes hotplug script currently */
>  int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);

Re: [PATCH v2 2/5] libxl: remove device model path in libxl__device_model_destroy

[Ian Campbell]

On Thu, 2015-03-19 at 13:18 +0000, Wei Liu wrote:
> ... and not devices_destroy_cb because it is the right place to clean up
> device model stuff.
> 
> And the path should use LIBXL_TOOLSTACK_DOMID instead of hardcoded 0.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 3/5] libxl: use LIBXL_TOOLSTACK_DOMID

[Ian Campbell]

On Thu, 2015-03-19 at 13:18 +0000, Wei Liu wrote:
> The function in question is libxl__spawn_local_dm. We should use
> LIBXL_TOOLSTACK_DOMID when constructing xenstore path.
> 
> Currently LIBXL_TOOLSTACK_DOMID is 0, so this patch introduces no
> functional change.
> 
> Use helper function to generate xenstore path.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 4/5] libxl: use new QEMU xenstore protocol

[Ian Campbell]

On Thu, 2015-03-19 at 13:18 +0000, Wei Liu wrote:
>      entries = libxl__xs_directory(gc, 0, GCSPRINTF(
> -                "/local/domain/0/device-model/%d/physmap", domid), &num);
> +                "/local/domain/%d/device-model/%d/physmap",
> +                dm_domid, domid), &num);

You've missed an opportunity to use your new helper, I think.

With that fixed: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v2 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled"

[Ian Campbell]

On Thu, 2015-03-19 at 13:18 +0000, Wei Liu wrote:
> This reverts commit dd748d128d86996592afafea02e578cc7d4e6d42.
> 
> We don't need this workaround anymore since we have fixed the toolstack
> interlock problem that affects stubdom.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

But, really this needs acks from the x86 guys. Cc Added.

> ---
>  xen/arch/x86/hvm/hvm.c           | 21 ---------------------
>  xen/include/asm-x86/hvm/domain.h |  1 -
>  2 files changed, 22 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 4734d71..32905d0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -892,13 +892,6 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
>  
>    done:
>      spin_unlock(&s->lock);
> -
> -    /* This check is protected by the domain ioreq server lock. */
> -    if ( d->arch.hvm_domain.ioreq_server.waiting )
> -    {
> -        d->arch.hvm_domain.ioreq_server.waiting = 0;
> -        domain_unpause(d);
> -    }
>  }
>  
>  static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
> @@ -1450,20 +1443,6 @@ int hvm_domain_initialise(struct domain *d)
>  
>      spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
>      INIT_LIST_HEAD(&d->arch.hvm_domain.ioreq_server.list);
> -    
> -    /*
> -     * In the case where a stub domain is providing emulation for
> -     * the guest, there is no interlock in the toolstack to prevent
> -     * the guest from running before the stub domain is ready.
> -     * Hence the domain must remain paused until at least one ioreq
> -     * server is created and enabled.
> -     */
> -    if ( !is_pvh_domain(d) )
> -    {
> -        domain_pause(d);
> -        d->arch.hvm_domain.ioreq_server.waiting = 1;
> -    }
> -
>      spin_lock_init(&d->arch.hvm_domain.irq_lock);
>      spin_lock_init(&d->arch.hvm_domain.uc_lock);
>  
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 0702bf5..2757c7f 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -83,7 +83,6 @@ struct hvm_domain {
>      struct {
>          spinlock_t       lock;
>          ioservid_t       id;
> -        bool_t           waiting;
>          struct list_head list;
>      } ioreq_server;
>      struct hvm_ioreq_server *default_ioreq_server;