[PATCH 0/5] Fix QEMU startup protocol

[PATCH 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.

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: remove DM path in libxl__device_model_destroy
  libxl: use LIBXL_TOOLSTACK_DOMID
  libxl: use new QEMU xenstore protocol
  libxl: wait for stubdom to be ready
  Revert "x86/hvm: wait for at least one ioreq server to be enabled"

 tools/libxl/libxl.c              |  7 +++--
 tools/libxl/libxl_device.c       |  5 +++-
 tools/libxl/libxl_dm.c           | 65 +++++++++++++++++++++++++++++++++-------
 tools/libxl/libxl_dom.c          | 51 ++++++++++++++++++++-----------
 tools/libxl/libxl_internal.h     |  2 ++
 tools/libxl/libxl_pci.c          | 29 +++++++++++-------
 xen/arch/x86/hvm/hvm.c           | 21 -------------
 xen/include/asm-x86/hvm/domain.h |  1 -
 8 files changed, 117 insertions(+), 64 deletions(-)

-- 
1.9.1

[PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy

[Wei Liu]

... because it is the right place to clean up device model stuffs.

And the path should use LIBXL_TOOLSTACK_DOMID instead of hardcoded 0.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c    | 2 --
 tools/libxl/libxl_dm.c | 6 ++++++
 2 files changed, 6 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..0fd5ffa 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1629,6 +1629,12 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    char *path = GCSPRINTF("/local/domain/%d/device-model/%d",
+                           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 2/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.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0fd5ffa..3dedad4 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1375,7 +1375,8 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d",
+                          LIBXL_TOOLSTACK_DOMID, domid);
     xs_mkdir(ctx->xsh, XBT_NULL, path);
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
@@ -1424,7 +1425,8 @@ retry_transaction:
         LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
-    spawn->xspath = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
+    spawn->xspath = GCSPRINTF("/local/domain/%d/device-model/%d/state",
+                              LIBXL_TOOLSTACK_DOMID, domid);
     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 3/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.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c        |  5 ++++-
 tools/libxl/libxl_device.c |  5 ++++-
 tools/libxl/libxl_dm.c     | 14 ++++++-------
 tools/libxl/libxl_dom.c    | 51 ++++++++++++++++++++++++++++++----------------
 tools/libxl/libxl_pci.c    | 29 +++++++++++++++++---------
 5 files changed, 67 insertions(+), 37 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 46a43d5..bccb6cb 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1033,7 +1033,10 @@ 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 = GCSPRINTF("/local/domain/%d/device-model/%d/state",
+                         dm_domid, domid);
         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..b5a30b4 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1188,7 +1188,10 @@ 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 = GCSPRINTF("/local/domain/%d/device-model/%d/state",
+                     dm_domid, domid);
     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 3dedad4..4a38455 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -998,7 +998,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     libxl_device_vfb *vfb;
     libxl_device_vkb *vkb;
     char **args;
-    struct xs_permissions perm[2];
+    struct xs_permissions perm[1];
     xs_transaction_t t;
 
     /* convenience aliases */
@@ -1106,16 +1106,16 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     }
     xs_set_target(ctx->xsh, dm_domid, guest_domid);
 
-    perm[0].id = dm_domid;
-    perm[0].perms = XS_PERM_NONE;
-    perm[1].id = guest_domid;
-    perm[1].perms = XS_PERM_READ;
+    perm[0].id = guest_domid;
+    perm[0].perms = XS_PERM_READ;
 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__sprintf(gc, "/local/domain/%d/device-model/%d",
+                       dm_domid, guest_domid));
     xs_set_permissions(ctx->xsh, t,
-        libxl__sprintf(gc, "/local/domain/0/device-model/%d", guest_domid),
+        libxl__sprintf(gc, "/local/domain/%d/device-model/%d",
+                       dm_domid, guest_domid),
                        perm, ARRAY_SIZE(perm));
     if (!xs_transaction_end(ctx->xsh, t, 0))
         if (errno == EAGAIN)
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a16d4a1..1e2d145 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -846,7 +846,9 @@ 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 = GCSPRINTF("/local/domain/%d/device-model/%d/command",
+                     dm_domid, domid);
     return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
 }
 
@@ -860,11 +862,12 @@ 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 GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%"PRIx64"/%s",
+                     dm_domid, domid, 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,13 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty
     const char *got;
 
     if (!lds->cmd_path) {
+        uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
         lds->cmd_path = GCSPRINTF(
-                   "/local/domain/0/device-model/%u/logdirty/cmd", domid);
+                   "/local/domain/%u/device-model/%u/logdirty/cmd",
+                   dm_domid, domid);
         lds->ret_path = GCSPRINTF(
-                   "/local/domain/0/device-model/%u/logdirty/ret", domid);
+                   "/local/domain/%u/device-model/%u/logdirty/ret",
+                   dm_domid, domid);
     }
     lds->cmd = enable ? "enable" : "disable";
 
@@ -1491,11 +1501,12 @@ 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 GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%s/%s",
+                     dm_domid, domid, phys_offset, node);
 }
 
 int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
@@ -1510,9 +1521,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 +1550,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_pci.c b/tools/libxl/libxl_pci.c
index f3ae132..4aeb2bb 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -850,11 +850,14 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d/state",
+                          dm_domid, domid);
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/parameter",
-                          domid);
+    path = libxl__sprintf(gc, "/local/domain/%d/device-model/%d/parameter",
+                          dm_domid, domid);
     if (pcidev->vdevfn) {
         libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
                         pcidev->domain, pcidev->bus, pcidev->dev,
@@ -869,11 +872,11 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d/parameter",
+                          dm_domid, domid);
     vdevfn = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state",
-                          domid);
+    path = libxl__sprintf(gc, "/local/domain/%d/device-model/%d/state",
+                          dm_domid, domid);
     if ( rc < 0 )
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                    "qemu refused to add device: %s", vdevfn);
@@ -1175,10 +1178,15 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d/state",
+                          dm_domid, domid);
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/parameter", domid);
+    path = libxl__sprintf(gc, "/local/domain/%d/device-model/%d/parameter",
+                          dm_domid, domid);
     libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
                     pcidev->bus, pcidev->dev, pcidev->func);
 
@@ -1196,7 +1204,8 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d/state",
+                          dm_domid, domid);
     xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
 
     return 0;
-- 
1.9.1

[PATCH 4/5] libxl: wait for stubdom to be ready

[Wei Liu]

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>
---
 tools/libxl/libxl_dm.c       | 39 ++++++++++++++++++++++++++++++++++++++-
 tools/libxl/libxl_internal.h |  2 ++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 4a38455..ad2ef41 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)
 {
@@ -1273,16 +1275,51 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
     rc = libxl_domain_unpause(CTX, dm_domid);
     if (rc) goto out;
 
+    libxl__xswait_init(&sdss->xswait);
+    sdss->xswait.ao = ao;
+    sdss->xswait.what = GCSPRINTF("Stubdom %d startup", dm_domid);
+    sdss->xswait.path = GCSPRINTF("/local/domain/%u/device-model/%u/state",
+                                  dm_domid, sdss->dm.guest_domid);
+    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:
     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);
+            libxl__xswait_stop(gc, xswait);
             return;
         }
     }
+    libxl__xswait_stop(gc, xswait);
     sdss->callback(egc, &sdss->dm, rc);
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 934465a..3e06514 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"
@@ -3044,6 +3045,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*);
-- 
1.9.1

[PATCH 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: Paul Durrant <paul.durrant@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

[PATCH] xenstore: use relative path for device-model node

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xenstore.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/xenstore.c b/xenstore.c
index b0d6f77..09319c7 100644
--- a/xenstore.c
+++ b/xenstore.c
@@ -711,7 +711,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 
 
     /* Set a watch for log-dirty commands from the migration tools */
-    if (pasprintf(&buf, "/local/domain/0/device-model/%u/logdirty/cmd",
+    if (pasprintf(&buf, "device-model/%u/logdirty/cmd",
                   domid) != -1) {
         xs_watch(xsh, buf, "logdirty");
         fprintf(logfile, "Watching %s\n", buf);
@@ -719,7 +719,7 @@ void xenstore_parse_domain_config(int hvm_domid)
 
     /* Set a watch for suspend requests from the migration tools */
     if (pasprintf(&buf, 
-                  "/local/domain/0/device-model/%u/command", domid) != -1) {
+                  "device-model/%u/command", domid) != -1) {
         xs_watch(xsh, buf, "dm-command");
         fprintf(logfile, "Watching %s\n", buf);
     }
@@ -777,7 +777,7 @@ int xenstore_parse_disable_pf_config ()
     int disable_pf = 0;
     unsigned int len;
 
-    if (pasprintf(&buf, "/local/domain/0/device-model/%u/disable_pf",domid) == -1)
+    if (pasprintf(&buf, "device-model/%u/disable_pf",domid) == -1)
         goto out;
 
     params = xs_read(xsh, XBT_NULL, buf, &len);
@@ -808,13 +808,13 @@ static void xenstore_process_logdirty_event(void)
 
     /* Remember the paths for the command and response entries */
     if (pasprintf(&ret_path,
-                "/local/domain/0/device-model/%u/logdirty/ret",
+                "device-model/%u/logdirty/ret",
                 domid) == -1) {
         fprintf(logfile, "Log-dirty: out of memory\n");
         exit(1);
     }
     if (pasprintf(&cmd_path,
-                "/local/domain/0/device-model/%u/logdirty/cmd",
+                "device-model/%u/logdirty/cmd",
                 domid) == -1) {
         fprintf(logfile, "Log-dirty: out of memory\n");
         exit(1);
@@ -855,7 +855,7 @@ static void xenstore_process_dm_command_event(void)
     unsigned int len;
 
     if (pasprintf(&path, 
-                  "/local/domain/0/device-model/%u/command", domid) == -1) {
+                  "device-model/%u/command", domid) == -1) {
         fprintf(logfile, "out of memory reading dm command\n");
         goto out;
     }
@@ -875,7 +875,7 @@ static void xenstore_process_dm_command_event(void)
     } else if (!strncmp(command, "usb-add", len)) {
         fprintf(logfile, "dm-command: usb-add a usb device\n");
         if (pasprintf(&path,
-                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -889,7 +889,7 @@ static void xenstore_process_dm_command_event(void)
     } else if (!strncmp(command, "usb-del", len)) {
         fprintf(logfile, "dm-command: usb-del a usb device\n");
         if (pasprintf(&path,
-                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -905,7 +905,7 @@ static void xenstore_process_dm_command_event(void)
         fprintf(logfile, "dm-command: hot remove pass-through pci dev \n");
 
         if (pasprintf(&path, 
-                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                      "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -919,7 +919,7 @@ static void xenstore_process_dm_command_event(void)
         fprintf(logfile, "dm-command: hot insert pass-through pci dev \n");
 
         if (pasprintf(&path, 
-                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
+                      "device-model/%u/parameter", domid) == -1) {
             fprintf(logfile, "out of memory reading dm command parameter\n");
             goto out;
         }
@@ -944,7 +944,7 @@ void xenstore_record_dm(const char *subpath, const char *state)
     char *path = NULL;
 
     if (pasprintf(&path, 
-                  "/local/domain/0/device-model/%u/%s", domid, subpath) == -1) {
+                  "device-model/%u/%s", domid, subpath) == -1) {
         fprintf(logfile, "out of memory recording dm \n");
         goto out;
     }
@@ -1521,7 +1521,7 @@ char *xenstore_device_model_read(int domid, const char *key, unsigned int *len)
 {
     char *path = NULL, *value = NULL;
 
-    if (pasprintf(&path, "/local/domain/0/device-model/%d/%s", domid, key) == -1)
+    if (pasprintf(&path, "device-model/%d/%s", domid, key) == -1)
         return NULL;
 
     value = xs_read(xsh, XBT_NULL, path, len);
-- 
1.9.1

Re: [PATCH] xenstore: use relative path for device-model node

[Ian Campbell]

On Fri, 2015-03-13 at 10:38 +0000, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xenstore.c | 24 ++++++++++++------------

Is this a qemu path? Trad or upstream?

>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/xenstore.c b/xenstore.c
> index b0d6f77..09319c7 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -711,7 +711,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
> 
>      /* Set a watch for log-dirty commands from the migration tools */
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/logdirty/cmd",
> +    if (pasprintf(&buf, "device-model/%u/logdirty/cmd",
>                    domid) != -1) {
>          xs_watch(xsh, buf, "logdirty");
>          fprintf(logfile, "Watching %s\n", buf);
> @@ -719,7 +719,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
>      /* Set a watch for suspend requests from the migration tools */
>      if (pasprintf(&buf, 
> -                  "/local/domain/0/device-model/%u/command", domid) != -1) {
> +                  "device-model/%u/command", domid) != -1) {
>          xs_watch(xsh, buf, "dm-command");
>          fprintf(logfile, "Watching %s\n", buf);
>      }
> @@ -777,7 +777,7 @@ int xenstore_parse_disable_pf_config ()
>      int disable_pf = 0;
>      unsigned int len;
>  
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/disable_pf",domid) == -1)
> +    if (pasprintf(&buf, "device-model/%u/disable_pf",domid) == -1)
>          goto out;
>  
>      params = xs_read(xsh, XBT_NULL, buf, &len);
> @@ -808,13 +808,13 @@ static void xenstore_process_logdirty_event(void)
>  
>      /* Remember the paths for the command and response entries */
>      if (pasprintf(&ret_path,
> -                "/local/domain/0/device-model/%u/logdirty/ret",
> +                "device-model/%u/logdirty/ret",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
>      }
>      if (pasprintf(&cmd_path,
> -                "/local/domain/0/device-model/%u/logdirty/cmd",
> +                "device-model/%u/logdirty/cmd",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
> @@ -855,7 +855,7 @@ static void xenstore_process_dm_command_event(void)
>      unsigned int len;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/command", domid) == -1) {
> +                  "device-model/%u/command", domid) == -1) {
>          fprintf(logfile, "out of memory reading dm command\n");
>          goto out;
>      }
> @@ -875,7 +875,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-add", len)) {
>          fprintf(logfile, "dm-command: usb-add a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -889,7 +889,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-del", len)) {
>          fprintf(logfile, "dm-command: usb-del a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -905,7 +905,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot remove pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -919,7 +919,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot insert pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -944,7 +944,7 @@ void xenstore_record_dm(const char *subpath, const char *state)
>      char *path = NULL;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/%s", domid, subpath) == -1) {
> +                  "device-model/%u/%s", domid, subpath) == -1) {
>          fprintf(logfile, "out of memory recording dm \n");
>          goto out;
>      }
> @@ -1521,7 +1521,7 @@ char *xenstore_device_model_read(int domid, const char *key, unsigned int *len)
>  {
>      char *path = NULL, *value = NULL;
>  
> -    if (pasprintf(&path, "/local/domain/0/device-model/%d/%s", domid, key) == -1)
> +    if (pasprintf(&path, "device-model/%d/%s", domid, key) == -1)
>          return NULL;
>  
>      value = xs_read(xsh, XBT_NULL, path, len);

Re: [PATCH] xenstore: use relative path for device-model node

[Wei Liu]

On Fri, Mar 13, 2015 at 10:46:30AM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 10:38 +0000, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xenstore.c | 24 ++++++++++++------------
> 
> Is this a qemu path? Trad or upstream?
> 

This is a QEMU trad patch. Sorry it's not clear enough in the subject
line.

Though I have a similar patch for upstream QEMU, there is no need to
send it now because it's not affected by this protocol change.

Wei.

Re: [PATCH] xenstore: use relative path for device-model node

[Stefano Stabellini]

Sounds good to me. It should have been written like this since the
beginning.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


On Fri, 13 Mar 2015, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xenstore.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/xenstore.c b/xenstore.c
> index b0d6f77..09319c7 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -711,7 +711,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
>  
>      /* Set a watch for log-dirty commands from the migration tools */
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/logdirty/cmd",
> +    if (pasprintf(&buf, "device-model/%u/logdirty/cmd",
>                    domid) != -1) {
>          xs_watch(xsh, buf, "logdirty");
>          fprintf(logfile, "Watching %s\n", buf);
> @@ -719,7 +719,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
>      /* Set a watch for suspend requests from the migration tools */
>      if (pasprintf(&buf, 
> -                  "/local/domain/0/device-model/%u/command", domid) != -1) {
> +                  "device-model/%u/command", domid) != -1) {
>          xs_watch(xsh, buf, "dm-command");
>          fprintf(logfile, "Watching %s\n", buf);
>      }
> @@ -777,7 +777,7 @@ int xenstore_parse_disable_pf_config ()
>      int disable_pf = 0;
>      unsigned int len;
>  
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/disable_pf",domid) == -1)
> +    if (pasprintf(&buf, "device-model/%u/disable_pf",domid) == -1)
>          goto out;
>  
>      params = xs_read(xsh, XBT_NULL, buf, &len);
> @@ -808,13 +808,13 @@ static void xenstore_process_logdirty_event(void)
>  
>      /* Remember the paths for the command and response entries */
>      if (pasprintf(&ret_path,
> -                "/local/domain/0/device-model/%u/logdirty/ret",
> +                "device-model/%u/logdirty/ret",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
>      }
>      if (pasprintf(&cmd_path,
> -                "/local/domain/0/device-model/%u/logdirty/cmd",
> +                "device-model/%u/logdirty/cmd",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
> @@ -855,7 +855,7 @@ static void xenstore_process_dm_command_event(void)
>      unsigned int len;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/command", domid) == -1) {
> +                  "device-model/%u/command", domid) == -1) {
>          fprintf(logfile, "out of memory reading dm command\n");
>          goto out;
>      }
> @@ -875,7 +875,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-add", len)) {
>          fprintf(logfile, "dm-command: usb-add a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -889,7 +889,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-del", len)) {
>          fprintf(logfile, "dm-command: usb-del a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -905,7 +905,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot remove pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -919,7 +919,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot insert pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -944,7 +944,7 @@ void xenstore_record_dm(const char *subpath, const char *state)
>      char *path = NULL;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/%s", domid, subpath) == -1) {
> +                  "device-model/%u/%s", domid, subpath) == -1) {
>          fprintf(logfile, "out of memory recording dm \n");
>          goto out;
>      }
> @@ -1521,7 +1521,7 @@ char *xenstore_device_model_read(int domid, const char *key, unsigned int *len)
>  {
>      char *path = NULL, *value = NULL;
>  
> -    if (pasprintf(&path, "/local/domain/0/device-model/%d/%s", domid, key) == -1)
> +    if (pasprintf(&path, "device-model/%d/%s", domid, key) == -1)
>          return NULL;
>  
>      value = xs_read(xsh, XBT_NULL, path, len);
> -- 
> 1.9.1
> 

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

[Paul Durrant]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 13 March 2015 10:35
> To: xen-devel@lists.xen.org
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; Anthony Perard; Wei Liu;
> Paul Durrant
> Subject: [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to
> be enabled"
> 
> This reverts commit dd748d128d86996592afafea02e578cc7d4e6d42.
> 
> We don't need this workaround anymore since we have fixed the toolstack
> interlock problem that affects stubdom.
> 

Excellent :-) Glad to hear it.

  Paul

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Paul Durrant <paul.durrant@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 4/5] libxl: wait for stubdom to be ready

[Wei Liu]

On Fri, Mar 13, 2015 at 10:34:34AM +0000, Wei Liu wrote:
> 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>
> ---
>  tools/libxl/libxl_dm.c       | 39 ++++++++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_internal.h |  2 ++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 4a38455..ad2ef41 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)
>  {
> @@ -1273,16 +1275,51 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
>      rc = libxl_domain_unpause(CTX, dm_domid);
>      if (rc) goto out;
>  
> +    libxl__xswait_init(&sdss->xswait);

I just discovered a bug.

This line should be moved to the beginning of this function, otherwise
the error path will cause client program to segfault.

Wei.

Re: [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy

[Ian Campbell]

On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> ... because it is the right place to clean up device model stuffs.

... and not devices_destroy_cb because it is the right ...

(also "stuff").

> And the path should use LIBXL_TOOLSTACK_DOMID instead of hardcoded 0.

Between this and what is in the next patch I think you probably should
refactor into a helper function to get the correct path for a given
domid.

FWIW I think you might also be able to do:
        GCSPRINTF("/local/domain/" #LIBXL_TOOLSTACK_DOMID "/device-model/%d",
                  domid);

If you want, although perhaps the intention is for it to eventually not
be a hard-define of 0 and become e.g. a function call or a global
variable reference, in which case best not?

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c    | 2 --
>  tools/libxl/libxl_dm.c | 6 ++++++
>  2 files changed, 6 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..0fd5ffa 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1629,6 +1629,12 @@ out:
>  
>  int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
>  {
> +    char *path = GCSPRINTF("/local/domain/%d/device-model/%d",
> +                           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));
>  }

Re: [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID

[Ian Campbell]

On Fri, 2015-03-13 at 10:34 +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.

As I mentioned on the previous patch, path should come from a helper
rather than repeated everywhere.

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_dm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 0fd5ffa..3dedad4 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1375,7 +1375,8 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d",
> +                          LIBXL_TOOLSTACK_DOMID, domid);
>      xs_mkdir(ctx->xsh, XBT_NULL, path);
>  
>      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> @@ -1424,7 +1425,8 @@ retry_transaction:
>          LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
>  
>      spawn->what = GCSPRINTF("domain %d device model", domid);
> -    spawn->xspath = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
> +    spawn->xspath = GCSPRINTF("/local/domain/%d/device-model/%d/state",
> +                              LIBXL_TOOLSTACK_DOMID, domid);
>      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;

Re: [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy

[Ian Campbell]

On Wed, 2015-03-18 at 13:21 +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> > ... because it is the right place to clean up device model stuffs.
> 
> ... and not devices_destroy_cb because it is the right ...
> 
> (also "stuff").
> 
> > And the path should use LIBXL_TOOLSTACK_DOMID instead of hardcoded 0.
> 
> Between this and what is in the next patch I think you probably should
> refactor into a helper function to get the correct path for a given
> domid.
> 
> FWIW I think you might also be able to do:
>         GCSPRINTF("/local/domain/" #LIBXL_TOOLSTACK_DOMID "/device-model/%d",
>                   domid);
> 
> If you want, although perhaps the intention is for it to eventually not
> be a hard-define of 0 and become e.g. a function call or a global
> variable reference, in which case best not?

Having reread patch #0 more carefully I suppose the helper would need to
take the dm_domid too, which would then rule out that cpp trick.

Ian.

Re: [PATCH] xenstore: use relative path for device-model node

[Ian Campbell]

On Fri, 2015-03-13 at 10:38 +0000, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

I suppose that somewhere in the libxl series that
QEMU_TRADITIONAL_REVISION will need updating to incorporate this. It
might be useful to note where in that series this can safely be done..

> ---
>  xenstore.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/xenstore.c b/xenstore.c
> index b0d6f77..09319c7 100644
> --- a/xenstore.c
> +++ b/xenstore.c
> @@ -711,7 +711,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
> 
>      /* Set a watch for log-dirty commands from the migration tools */
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/logdirty/cmd",
> +    if (pasprintf(&buf, "device-model/%u/logdirty/cmd",
>                    domid) != -1) {
>          xs_watch(xsh, buf, "logdirty");
>          fprintf(logfile, "Watching %s\n", buf);
> @@ -719,7 +719,7 @@ void xenstore_parse_domain_config(int hvm_domid)
>  
>      /* Set a watch for suspend requests from the migration tools */
>      if (pasprintf(&buf, 
> -                  "/local/domain/0/device-model/%u/command", domid) != -1) {
> +                  "device-model/%u/command", domid) != -1) {
>          xs_watch(xsh, buf, "dm-command");
>          fprintf(logfile, "Watching %s\n", buf);
>      }
> @@ -777,7 +777,7 @@ int xenstore_parse_disable_pf_config ()
>      int disable_pf = 0;
>      unsigned int len;
>  
> -    if (pasprintf(&buf, "/local/domain/0/device-model/%u/disable_pf",domid) == -1)
> +    if (pasprintf(&buf, "device-model/%u/disable_pf",domid) == -1)
>          goto out;
>  
>      params = xs_read(xsh, XBT_NULL, buf, &len);
> @@ -808,13 +808,13 @@ static void xenstore_process_logdirty_event(void)
>  
>      /* Remember the paths for the command and response entries */
>      if (pasprintf(&ret_path,
> -                "/local/domain/0/device-model/%u/logdirty/ret",
> +                "device-model/%u/logdirty/ret",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
>      }
>      if (pasprintf(&cmd_path,
> -                "/local/domain/0/device-model/%u/logdirty/cmd",
> +                "device-model/%u/logdirty/cmd",
>                  domid) == -1) {
>          fprintf(logfile, "Log-dirty: out of memory\n");
>          exit(1);
> @@ -855,7 +855,7 @@ static void xenstore_process_dm_command_event(void)
>      unsigned int len;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/command", domid) == -1) {
> +                  "device-model/%u/command", domid) == -1) {
>          fprintf(logfile, "out of memory reading dm command\n");
>          goto out;
>      }
> @@ -875,7 +875,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-add", len)) {
>          fprintf(logfile, "dm-command: usb-add a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -889,7 +889,7 @@ static void xenstore_process_dm_command_event(void)
>      } else if (!strncmp(command, "usb-del", len)) {
>          fprintf(logfile, "dm-command: usb-del a usb device\n");
>          if (pasprintf(&path,
> -                "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -905,7 +905,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot remove pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -919,7 +919,7 @@ static void xenstore_process_dm_command_event(void)
>          fprintf(logfile, "dm-command: hot insert pass-through pci dev \n");
>  
>          if (pasprintf(&path, 
> -                      "/local/domain/0/device-model/%u/parameter", domid) == -1) {
> +                      "device-model/%u/parameter", domid) == -1) {
>              fprintf(logfile, "out of memory reading dm command parameter\n");
>              goto out;
>          }
> @@ -944,7 +944,7 @@ void xenstore_record_dm(const char *subpath, const char *state)
>      char *path = NULL;
>  
>      if (pasprintf(&path, 
> -                  "/local/domain/0/device-model/%u/%s", domid, subpath) == -1) {
> +                  "device-model/%u/%s", domid, subpath) == -1) {
>          fprintf(logfile, "out of memory recording dm \n");
>          goto out;
>      }
> @@ -1521,7 +1521,7 @@ char *xenstore_device_model_read(int domid, const char *key, unsigned int *len)
>  {
>      char *path = NULL, *value = NULL;
>  
> -    if (pasprintf(&path, "/local/domain/0/device-model/%d/%s", domid, key) == -1)
> +    if (pasprintf(&path, "device-model/%d/%s", domid, key) == -1)
>          return NULL;
>  
>      value = xs_read(xsh, XBT_NULL, path, len);

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

[Ian Campbell]

On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3dedad4..4a38455 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -998,7 +998,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>      libxl_device_vfb *vfb;
>      libxl_device_vkb *vkb;
>      char **args;
> -    struct xs_permissions perm[2];
> +    struct xs_permissions perm[1];
>      xs_transaction_t t;
>  
>      /* convenience aliases */
> @@ -1106,16 +1106,16 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>      }
>      xs_set_target(ctx->xsh, dm_domid, guest_domid);
>  
> -    perm[0].id = dm_domid;
> -    perm[0].perms = XS_PERM_NONE;
> -    perm[1].id = guest_domid;
> -    perm[1].perms = XS_PERM_READ;
> +    perm[0].id = guest_domid;
> +    perm[0].perms = XS_PERM_READ;

This will make the node owned by the guest (and hence r/w to the guest)
and set the perms for everyone else to r/o. I don't think this is what
you meant?

(The way perms are specified is a bit confusing, check out
http://wiki.xen.org/wiki/XenBus#Permissions )


> -    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%s/%s",
> -            domid, phys_offset, node);
> +    return GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%s/%s",
> +                     dm_domid, domid, phys_offset, node);

This sort of thing might imply that the helper takes the tail of the
path?
> SPRINTF(
> -                "/local/domain/0/device-model/%d/physmap", domid), &num);
> +                "/local/domain/%d/device-model/%d/physmap",

I only just noticed, but I think you want %u not %d (since dm_domid is
unsigned), although given the existing domid one is wrong too maybe you
don't want to bother...

> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index f3ae132..4aeb2bb 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -850,11 +850,14 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d/state",
> +                          dm_domid, domid);

Maybe (up to you) switch thing to GCSPRINTF as you touch them?

Ian.

Re: [PATCH 4/5] libxl: wait for stubdom to be ready

[Ian Campbell]

On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> 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>
> ---
>  tools/libxl/libxl_dm.c       | 39 ++++++++++++++++++++++++++++++++++++++-
>  tools/libxl/libxl_internal.h |  2 ++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 4a38455..ad2ef41 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)
>  {
> @@ -1273,16 +1275,51 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
>      rc = libxl_domain_unpause(CTX, dm_domid);
>      if (rc) goto out;
>  
> +    libxl__xswait_init(&sdss->xswait);
> +    sdss->xswait.ao = ao;
> +    sdss->xswait.what = GCSPRINTF("Stubdom %d startup", dm_domid);

Maybe include the domid too, e.g. "Stubdom %d for dom %d startup"?

(And probably %u throughout?)

> +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:
>      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);
> +            libxl__xswait_stop(gc, xswait);

Can you do this once befire the if (rc)?

Ian.

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

[Wei Liu]

On Wed, Mar 18, 2015 at 01:30:40PM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 3dedad4..4a38455 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -998,7 +998,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> >      libxl_device_vfb *vfb;
> >      libxl_device_vkb *vkb;
> >      char **args;
> > -    struct xs_permissions perm[2];
> > +    struct xs_permissions perm[1];
> >      xs_transaction_t t;
> >  
> >      /* convenience aliases */
> > @@ -1106,16 +1106,16 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> >      }
> >      xs_set_target(ctx->xsh, dm_domid, guest_domid);
> >  
> > -    perm[0].id = dm_domid;
> > -    perm[0].perms = XS_PERM_NONE;
> > -    perm[1].id = guest_domid;
> > -    perm[1].perms = XS_PERM_READ;
> > +    perm[0].id = guest_domid;
> > +    perm[0].perms = XS_PERM_READ;
> 
> This will make the node owned by the guest (and hence r/w to the guest)
> and set the perms for everyone else to r/o. I don't think this is what
> you meant?
> 
> (The way perms are specified is a bit confusing, check out
> http://wiki.xen.org/wiki/XenBus#Permissions )
> 

Oh man! I completely misunderstood the semantics. After reading that
wiki page, I don't think I need to touch this permission bit because the
original setting does what I need.

The content of that wiki page section should really be transferred to
the comment before xs_set_permissions (which at the moment is extremely
terse). I will send a patch to do that.

> 
> > -    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%s/%s",
> > -            domid, phys_offset, node);
> > +    return GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%s/%s",
> > +                     dm_domid, domid, phys_offset, node);
> 
> This sort of thing might imply that the helper takes the tail of the
> path?

What do you mean? Sorry I don't follow.

> > SPRINTF(
> > -                "/local/domain/0/device-model/%d/physmap", domid), &num);
> > +                "/local/domain/%d/device-model/%d/physmap",
> 
> I only just noticed, but I think you want %u not %d (since dm_domid is
> unsigned), although given the existing domid one is wrong too maybe you
> don't want to bother...
> 

Yes, I noticed but didn't bother to do that. "%d" is all over the place
that it should be fixed all in one go...

> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index f3ae132..4aeb2bb 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -850,11 +850,14 @@ 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__sprintf(gc, "/local/domain/%d/device-model/%d/state",
> > +                          dm_domid, domid);
> 
> Maybe (up to you) switch thing to GCSPRINTF as you touch them?
> 

Sure.

Wei.

> Ian.

Re: [PATCH 4/5] libxl: wait for stubdom to be ready

[Wei Liu]

On Wed, Mar 18, 2015 at 01:35:15PM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> > 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>
> > ---
> >  tools/libxl/libxl_dm.c       | 39 ++++++++++++++++++++++++++++++++++++++-
> >  tools/libxl/libxl_internal.h |  2 ++
> >  2 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 4a38455..ad2ef41 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)
> >  {
> > @@ -1273,16 +1275,51 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
> >      rc = libxl_domain_unpause(CTX, dm_domid);
> >      if (rc) goto out;
> >  
> > +    libxl__xswait_init(&sdss->xswait);
> > +    sdss->xswait.ao = ao;
> > +    sdss->xswait.what = GCSPRINTF("Stubdom %d startup", dm_domid);
> 
> Maybe include the domid too, e.g. "Stubdom %d for dom %d startup"?
> 
> (And probably %u throughout?)
> 

Ack.

> > +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:
> >      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);
> > +            libxl__xswait_stop(gc, xswait);
> 
> Can you do this once befire the if (rc)?
> 

I think so.

Wei.

> Ian.
> 

Re: [PATCH] xenstore: use relative path for device-model node

[Wei Liu]

On Wed, Mar 18, 2015 at 01:24:44PM +0000, Ian Campbell wrote:
> On Fri, 2015-03-13 at 10:38 +0000, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> I suppose that somewhere in the libxl series that
> QEMU_TRADITIONAL_REVISION will need updating to incorporate this. It
> might be useful to note where in that series this can safely be done..
> 

Yes, we should update Config.mk in "libxl: use new QEMU xenstore
protocol". I will write this down in my cover letter.

Wei.

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

[Ian Campbell]

On Thu, 2015-03-19 at 10:36 +0000, Wei Liu wrote:
> On Wed, Mar 18, 2015 at 01:30:40PM +0000, Ian Campbell wrote:
> > On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > index 3dedad4..4a38455 100644
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -998,7 +998,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> > >      libxl_device_vfb *vfb;
> > >      libxl_device_vkb *vkb;
> > >      char **args;
> > > -    struct xs_permissions perm[2];
> > > +    struct xs_permissions perm[1];
> > >      xs_transaction_t t;
> > >  
> > >      /* convenience aliases */
> > > @@ -1106,16 +1106,16 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> > >      }
> > >      xs_set_target(ctx->xsh, dm_domid, guest_domid);
> > >  
> > > -    perm[0].id = dm_domid;
> > > -    perm[0].perms = XS_PERM_NONE;
> > > -    perm[1].id = guest_domid;
> > > -    perm[1].perms = XS_PERM_READ;
> > > +    perm[0].id = guest_domid;
> > > +    perm[0].perms = XS_PERM_READ;
> > 
> > This will make the node owned by the guest (and hence r/w to the guest)
> > and set the perms for everyone else to r/o. I don't think this is what
> > you meant?
> > 
> > (The way perms are specified is a bit confusing, check out
> > http://wiki.xen.org/wiki/XenBus#Permissions )
> > 
> 
> Oh man! I completely misunderstood the semantics.

In fairness, they are insanely confusing, at least when encoded as an
array...

>  After reading that
> wiki page, I don't think I need to touch this permission bit because the
> original setting does what I need.
> 
> The content of that wiki page section should really be transferred to
> the comment before xs_set_permissions (which at the moment is extremely
> terse). I will send a patch to do that.

Awesome, thanks.

> > > -    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%s/%s",
> > > -            domid, phys_offset, node);
> > > +    return GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%s/%s",
> > > +                     dm_domid, domid, phys_offset, node);
> > 
> > This sort of thing might imply that the helper takes the tail of the
> > path?
> 
> What do you mean? Sorry I don't follow.

I suggested before having a helper to return
"/local/domain/0/device-model/%d/", this hunk made me wonder if perhaps
that helper should take a "const char *fmt, ..." which it appends, so
you would call it as:

     foo(dm_domid, domid, "physmap/%s/%s", phys_offset, node)

or if you just want the base path for some reason
     foo(dm_domid, domid, "") (or NULL as the last parameter).

(I'm unclear if dm_domid and domid are both needed or if the funcution
can call get_stubdom_id(domid) internally, I'm sure you know...).

> > > SPRINTF(
> > > -                "/local/domain/0/device-model/%d/physmap", domid), &num);
> > > +                "/local/domain/%d/device-model/%d/physmap",
> > 
> > I only just noticed, but I think you want %u not %d (since dm_domid is
> > unsigned), although given the existing domid one is wrong too maybe you
> > don't want to bother...
> > 
> 
> Yes, I noticed but didn't bother to do that. "%d" is all over the place
> that it should be fixed all in one go...

Ack.

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

[Wei Liu]

On Thu, Mar 19, 2015 at 10:55:51AM +0000, Ian Campbell wrote:
[...]
> > > > -    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%s/%s",
> > > > -            domid, phys_offset, node);
> > > > +    return GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%s/%s",
> > > > +                     dm_domid, domid, phys_offset, node);
> > > 
> > > This sort of thing might imply that the helper takes the tail of the
> > > path?
> > 
> > What do you mean? Sorry I don't follow.
> 
> I suggested before having a helper to return
> "/local/domain/0/device-model/%d/", this hunk made me wonder if perhaps
> that helper should take a "const char *fmt, ..." which it appends, so
> you would call it as:
> 
>      foo(dm_domid, domid, "physmap/%s/%s", phys_offset, node)
> 
> or if you just want the base path for some reason
>      foo(dm_domid, domid, "") (or NULL as the last parameter).
> 

OK. I get your idea.

> (I'm unclear if dm_domid and domid are both needed or if the funcution
> can call get_stubdom_id(domid) internally, I'm sure you know...).
> 

dm_domid is still needed.

Wei.

Re: [PATCH] xenstore: use relative path for device-model node

[Ian Campbell]

On Thu, 2015-03-19 at 10:54 +0000, Wei Liu wrote:
> On Wed, Mar 18, 2015 at 01:24:44PM +0000, Ian Campbell wrote:
> > On Fri, 2015-03-13 at 10:38 +0000, Wei Liu wrote:
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > I suppose that somewhere in the libxl series that
> > QEMU_TRADITIONAL_REVISION will need updating to incorporate this. It
> > might be useful to note where in that series this can safely be done..
> > 
> 
> Yes, we should update Config.mk in "libxl: use new QEMU xenstore
> protocol". I will write this down in my cover letter.

Could you note it after the --- in the patch too please.

> 
> Wei.

[PATCH] xenstore: use relative path for device-model node

[Ian Jackson]

Wei Liu writes ("[PATCH] xenstore: use relative path for device-model node"):
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

This commit message needs to contain an explanation IMO for what
appears like an incompatible change.  (Perhaps you could copy some of
the text from "libxl: use new QEMU xenstore protocol".)

The code changes are fine.

Thanks,
Ian.