[PATCH v3 0/5] Fix QEMU startup protocol

[PATCH v3 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          | 56 +++++++++++++++++++++++++---------------
 tools/libxl/libxl_internal.c     | 40 +++++++++++++++++++++++-----
 tools/libxl/libxl_internal.h     |  6 +++++
 tools/libxl/libxl_pci.c          | 22 +++++++++-------
 xen/arch/x86/hvm/hvm.c           | 21 ---------------
 xen/include/asm-x86/hvm/domain.h |  1 -
 9 files changed, 144 insertions(+), 67 deletions(-)

-- 
1.9.1

[PATCH v3 1/5] libxl: introduce libxl__device_model_xs_path

[Wei Liu]

Factor out libxl__vsprintf. Use it to implement
libxl__device_model_xs_patch helper to return xenstore path for device
model to avoid handcoded paths.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v3:
1. Factor out libxl__vsprintf.
---
 tools/libxl/libxl_internal.c | 40 ++++++++++++++++++++++++++++++++++------
 tools/libxl/libxl_internal.h |  4 ++++
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ddc68ab..c2c67bd 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -127,21 +127,33 @@ void *libxl__realloc(libxl__gc *gc, void *ptr, size_t new_size)
     return new_ptr;
 }
 
-char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
+char *libxl__vsprintf(libxl__gc *gc, const char *fmt, va_list ap)
 {
     char *s;
-    va_list ap;
+    va_list aq;
     int ret;
 
-    va_start(ap, fmt);
-    ret = vsnprintf(NULL, 0, fmt, ap);
-    va_end(ap);
+    va_copy(aq, ap);
+    ret = vsnprintf(NULL, 0, fmt, aq);
+    va_end(aq);
 
     assert(ret >= 0);
 
     s = libxl__zalloc(gc, ret + 1);
+    va_copy(aq, ap);
+    ret = vsnprintf(s, ret + 1, fmt, aq);
+    va_end(aq);
+
+    return s;
+}
+
+char *libxl__sprintf(libxl__gc *gc, const char *fmt, ...)
+{
+    char *s;
+    va_list ap;
+
     va_start(ap, fmt);
-    ret = vsnprintf(s, ret + 1, fmt, ap);
+    s = libxl__vsprintf(gc, fmt, ap);
     va_end(ap);
 
     return s;
@@ -555,6 +567,22 @@ 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;
+
+    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
+                    domid, format);
+
+    va_start(ap, format);
+    s = libxl__vsprintf(gc, 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..2f952d4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -521,6 +521,7 @@ _hidden void *libxl__realloc(libxl__gc *gc_opt, void *ptr, size_t new_size) NN1;
 /* print @fmt into an allocated string large enoughto contain the result.
  * (similar to gc'd asprintf(3)). */
 _hidden char *libxl__sprintf(libxl__gc *gc_opt, const char *fmt, ...) PRINTF_ATTRIBUTE(2, 3) NN1;
+_hidden char *libxl__vsprintf(libxl__gc *gc, const char *format, va_list ap);
 /* duplicate the string @c (similar to a gc'd strdup(3)). */
 _hidden char *libxl__strdup(libxl__gc *gc_opt, const char *c) NN1;
 /* duplicate at most @n bytes of string @c (similar to a gc'd strndup(3)). */
@@ -1794,6 +1795,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

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

[Ian Campbell]

On Fri, 2015-03-20 at 16:19 +0000, Wei Liu wrote:
> +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> +                                          uint32_t domid,
> +                                          const char *format,  ...);

This should have the PRINTF_ATTRIBUTE thing and not the double space
before "...".

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

Since this is pretty minor if you want to resend just this one patch, or
even send an incremental fixup to be folded in I can take care of it on
commit.

Ian.

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

[Wei Liu]

On Tue, Mar 24, 2015 at 02:32:42PM +0000, Ian Campbell wrote:
> On Fri, 2015-03-20 at 16:19 +0000, Wei Liu wrote:
> > +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> > +                                          uint32_t domid,
> > +                                          const char *format,  ...);
> 
> This should have the PRINTF_ATTRIBUTE thing and not the double space
> before "...".
> 
> With that fixed: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Since this is pretty minor if you want to resend just this one patch, or
> even send an incremental fixup to be folded in I can take care of it on
> commit.
> 

Here is an incremental fix.

---8<---
>From fe43ae63bc0bc9e43af6957566e51c2f5a9edd29 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 24 Mar 2015 14:47:51 +0000
Subject: [PATCH] libxl__device_model_xs_path fixup

---
 tools/libxl/libxl_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 1e9ee59..d6daf62 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1798,7 +1798,7 @@ _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
 _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,  ...);
+                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
-- 
1.9.1

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

[Ian Campbell]

On Tue, 2015-03-24 at 14:49 +0000, Wei Liu wrote:
> On Tue, Mar 24, 2015 at 02:32:42PM +0000, Ian Campbell wrote:
> > On Fri, 2015-03-20 at 16:19 +0000, Wei Liu wrote:
> > > +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> > > +                                          uint32_t domid,
> > > +                                          const char *format,  ...);
> > 
> > This should have the PRINTF_ATTRIBUTE thing and not the double space
> > before "...".
> > 
> > With that fixed: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Since this is pretty minor if you want to resend just this one patch, or
> > even send an incremental fixup to be folded in I can take care of it on
> > commit.
> > 
> 
> Here is an incremental fix.

Thanks, I folded that in and pushed the first 4 patches in this series.

The final patch (Revert "x86/hvm: wait for at least one ioreq server to
be enabled") needs an x86 ack which it currently lacks so I've left this
for now (Jan, I guess you'll pick it up or ask me to do so once you've
acked it)

Ian.

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

[Ian Campbell]

On Tue, 2015-03-24 at 16:53 +0000, Ian Campbell wrote:
> On Tue, 2015-03-24 at 14:49 +0000, Wei Liu wrote:
> > On Tue, Mar 24, 2015 at 02:32:42PM +0000, Ian Campbell wrote:
> > > On Fri, 2015-03-20 at 16:19 +0000, Wei Liu wrote:
> > > > +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> > > > +                                          uint32_t domid,
> > > > +                                          const char *format,  ...);
> > > 
> > > This should have the PRINTF_ATTRIBUTE thing and not the double space
> > > before "...".
> > > 
> > > With that fixed: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > Since this is pretty minor if you want to resend just this one patch, or
> > > even send an incremental fixup to be folded in I can take care of it on
> > > commit.
> > > 
> > 
> > Here is an incremental fix.
> 
> Thanks, I folded that in and pushed the first 4 patches in this series.
> 
> The final patch (Revert "x86/hvm: wait for at least one ioreq server to
> be enabled") needs an x86 ack which it currently lacks so I've left this
> for now (Jan, I guess you'll pick it up or ask me to do so once you've
> acked it)

Jan: Note that patch 4/5 got reverted, so patch 5 (the revert quoted
above) shouldn't go in just yet.

Ian.

[PATCH v3 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>
Acked-by: Ian Campbell <ian.campbell@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 94b4d59..75f1457 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 a8b08f2..6d34cb9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1642,6 +1642,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 v3 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>
Acked-by: Ian Campbell <ian.campbell@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 6d34cb9..e0bbb39 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1388,7 +1388,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 &&
@@ -1438,6 +1438,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

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

[Olaf Hering]

On Fri, Mar 20, Wei Liu wrote:

> +++ b/tools/libxl/libxl_dm.c
> @@ -1438,6 +1438,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");

This assigns xspath twice.

Olaf

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

[Wei Liu]

On Fri, Apr 24, 2015 at 10:49:36AM +0200, Olaf Hering wrote:
> On Fri, Mar 20, Wei Liu wrote:
> 
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1438,6 +1438,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");
> 
> This assigns xspath twice.
> 

Good spot. I will send a patch to fix this. Luckily this doesn't cause
practical problem except for wasting some memory. :-)

Wei.

> Olaf

[PATCH v3 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>
Acked-by: Ian Campbell <ian.campbell@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 v3:
1. Fix a spot that didn't use new helper.

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      | 56 +++++++++++++++++++++++++++-----------------
 tools/libxl/libxl_internal.h |  2 ++
 tools/libxl/libxl_pci.c      | 22 +++++++++--------
 6 files changed, 98 insertions(+), 36 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 75f1457..b8625b0 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 e0bbb39..ca34439 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1001,6 +1001,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)
 {
@@ -1130,9 +1132,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)
@@ -1278,6 +1281,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;
@@ -1286,10 +1291,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..d18d52a 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);
+    entries = libxl__xs_directory(gc, 0,
+                libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"),
+                &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 2f952d4..1e9ee59 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"
@@ -3048,6 +3049,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

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

[Ian Campbell]

On Fri, 2015-03-20 at 16:19 +0000, Wei Liu wrote:
> 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>

I knew about this but completely forgot when it came to applying.

Ian J has some questions about the qemu side patch so in the meantime I
have reverted this one patch and I'll assume Ian will apply along with
the qemu change once he is happy with it.

Ian.

[PATCH v3 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>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <ian.campbell@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 v3 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled"

[Jan Beulich]

>>> On 20.03.15 at 17:19, <wei.liu2@citrix.com> 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>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'd actually like to make my ack dependent on Paul's, considering
that he was the one who put in that workaround (and hence
should have been Cc-ed on a revert thereof).

Jan

> ---
>  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 v3 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled"

[Ian Campbell]

On Fri, 2015-03-20 at 16:43 +0000, Jan Beulich wrote:
> >>> On 20.03.15 at 17:19, <wei.liu2@citrix.com> 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>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I'd actually like to make my ack dependent on Paul's, considering
> that he was the one who put in that workaround (and hence
> should have been Cc-ed on a revert thereof).

FWIW in response to the first posting Paul said "Excellent :-) Glad to
hear it.", which isn't quite an ack I suppose ;)