* [PATCH 0/5] Fix QEMU startup protocol
@ 2015-03-13 10:34 Wei Liu
2015-03-13 10:34 ` [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy Wei Liu
` (5 more replies)
0 siblings, 6 replies; 25+ messages in thread
From: Wei Liu @ 2015-03-13 10:34 UTC (permalink / raw)
To: xen-devel
Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini
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
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu @ 2015-03-13 10:34 ` Wei Liu 2015-03-18 13:21 ` Ian Campbell 2015-03-13 10:34 ` [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID Wei Liu ` (4 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Wei Liu @ 2015-03-13 10:34 UTC (permalink / raw) To: xen-devel Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini ... 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy 2015-03-13 10:34 ` [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy Wei Liu @ 2015-03-18 13:21 ` Ian Campbell 2015-03-18 13:23 ` Ian Campbell 0 siblings, 1 reply; 25+ messages in thread From: Ian Campbell @ 2015-03-18 13:21 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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)); > } ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy 2015-03-18 13:21 ` Ian Campbell @ 2015-03-18 13:23 ` Ian Campbell 0 siblings, 0 replies; 25+ messages in thread From: Ian Campbell @ 2015-03-18 13:23 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu 2015-03-13 10:34 ` [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy Wei Liu @ 2015-03-13 10:34 ` Wei Liu 2015-03-18 13:21 ` Ian Campbell 2015-03-13 10:34 ` [PATCH 3/5] libxl: use new QEMU xenstore protocol Wei Liu ` (3 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Wei Liu @ 2015-03-13 10:34 UTC (permalink / raw) To: xen-devel Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID 2015-03-13 10:34 ` [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID Wei Liu @ 2015-03-18 13:21 ` Ian Campbell 0 siblings, 0 replies; 25+ messages in thread From: Ian Campbell @ 2015-03-18 13:21 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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; ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/5] libxl: use new QEMU xenstore protocol 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu 2015-03-13 10:34 ` [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy Wei Liu 2015-03-13 10:34 ` [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID Wei Liu @ 2015-03-13 10:34 ` Wei Liu 2015-03-18 13:30 ` Ian Campbell 2015-03-13 10:34 ` [PATCH 4/5] libxl: wait for stubdom to be ready Wei Liu ` (2 subsequent siblings) 5 siblings, 1 reply; 25+ messages in thread From: Wei Liu @ 2015-03-13 10:34 UTC (permalink / raw) To: xen-devel Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 3/5] libxl: use new QEMU xenstore protocol 2015-03-13 10:34 ` [PATCH 3/5] libxl: use new QEMU xenstore protocol Wei Liu @ 2015-03-18 13:30 ` Ian Campbell 2015-03-19 10:36 ` Wei Liu 0 siblings, 1 reply; 25+ messages in thread From: Ian Campbell @ 2015-03-18 13:30 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/5] libxl: use new QEMU xenstore protocol 2015-03-18 13:30 ` Ian Campbell @ 2015-03-19 10:36 ` Wei Liu 2015-03-19 10:55 ` Ian Campbell 0 siblings, 1 reply; 25+ messages in thread From: Wei Liu @ 2015-03-19 10:36 UTC (permalink / raw) To: Ian Campbell Cc: anthony.perard, ian.jackson, stefano.stabellini, Wei Liu, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/5] libxl: use new QEMU xenstore protocol 2015-03-19 10:36 ` Wei Liu @ 2015-03-19 10:55 ` Ian Campbell 2015-03-19 11:04 ` Wei Liu 0 siblings, 1 reply; 25+ messages in thread From: Ian Campbell @ 2015-03-19 10:55 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/5] libxl: use new QEMU xenstore protocol 2015-03-19 10:55 ` Ian Campbell @ 2015-03-19 11:04 ` Wei Liu 0 siblings, 0 replies; 25+ messages in thread From: Wei Liu @ 2015-03-19 11:04 UTC (permalink / raw) To: Ian Campbell Cc: anthony.perard, ian.jackson, stefano.stabellini, Wei Liu, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/5] libxl: wait for stubdom to be ready 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu ` (2 preceding siblings ...) 2015-03-13 10:34 ` [PATCH 3/5] libxl: use new QEMU xenstore protocol Wei Liu @ 2015-03-13 10:34 ` Wei Liu 2015-03-16 17:55 ` Wei Liu 2015-03-18 13:35 ` Ian Campbell 2015-03-13 10:34 ` [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled" Wei Liu 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu 5 siblings, 2 replies; 25+ messages in thread From: Wei Liu @ 2015-03-13 10:34 UTC (permalink / raw) To: xen-devel Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] libxl: wait for stubdom to be ready 2015-03-13 10:34 ` [PATCH 4/5] libxl: wait for stubdom to be ready Wei Liu @ 2015-03-16 17:55 ` Wei Liu 2015-03-18 13:35 ` Ian Campbell 1 sibling, 0 replies; 25+ messages in thread From: Wei Liu @ 2015-03-16 17:55 UTC (permalink / raw) To: xen-devel Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] libxl: wait for stubdom to be ready 2015-03-13 10:34 ` [PATCH 4/5] libxl: wait for stubdom to be ready Wei Liu 2015-03-16 17:55 ` Wei Liu @ 2015-03-18 13:35 ` Ian Campbell 2015-03-19 10:46 ` Wei Liu 1 sibling, 1 reply; 25+ messages in thread From: Ian Campbell @ 2015-03-18 13:35 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 4/5] libxl: wait for stubdom to be ready 2015-03-18 13:35 ` Ian Campbell @ 2015-03-19 10:46 ` Wei Liu 0 siblings, 0 replies; 25+ messages in thread From: Wei Liu @ 2015-03-19 10:46 UTC (permalink / raw) To: Ian Campbell Cc: anthony.perard, ian.jackson, stefano.stabellini, Wei Liu, xen-devel 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. > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled" 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu ` (3 preceding siblings ...) 2015-03-13 10:34 ` [PATCH 4/5] libxl: wait for stubdom to be ready Wei Liu @ 2015-03-13 10:34 ` Wei Liu 2015-03-13 11:32 ` Paul Durrant 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu 5 siblings, 1 reply; 25+ messages in thread From: Wei Liu @ 2015-03-13 10:34 UTC (permalink / raw) To: xen-devel Cc: Wei Liu, ian.campbell, stefano.stabellini, ian.jackson, Paul Durrant, anthony.perard 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled" 2015-03-13 10:34 ` [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled" Wei Liu @ 2015-03-13 11:32 ` Paul Durrant 0 siblings, 0 replies; 25+ messages in thread From: Paul Durrant @ 2015-03-13 11:32 UTC (permalink / raw) To: xen-devel Cc: Anthony Perard, Ian Jackson, Stefano Stabellini, Ian Campbell, Wei Liu > -----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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] xenstore: use relative path for device-model node 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu ` (4 preceding siblings ...) 2015-03-13 10:34 ` [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled" Wei Liu @ 2015-03-13 10:38 ` Wei Liu 2015-03-13 10:46 ` Ian Campbell ` (3 more replies) 5 siblings, 4 replies; 25+ messages in thread From: Wei Liu @ 2015-03-13 10:38 UTC (permalink / raw) To: xen-devel Cc: anthony.perard, Wei Liu, ian.jackson, ian.campbell, stefano.stabellini 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 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH] xenstore: use relative path for device-model node 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu @ 2015-03-13 10:46 ` Ian Campbell 2015-03-13 11:01 ` Wei Liu 2015-03-13 11:04 ` Stefano Stabellini ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Ian Campbell @ 2015-03-13 10:46 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] xenstore: use relative path for device-model node 2015-03-13 10:46 ` Ian Campbell @ 2015-03-13 11:01 ` Wei Liu 0 siblings, 0 replies; 25+ messages in thread From: Wei Liu @ 2015-03-13 11:01 UTC (permalink / raw) To: Ian Campbell Cc: anthony.perard, ian.jackson, stefano.stabellini, Wei Liu, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] xenstore: use relative path for device-model node 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu 2015-03-13 10:46 ` Ian Campbell @ 2015-03-13 11:04 ` Stefano Stabellini 2015-03-18 13:24 ` Ian Campbell 2015-03-24 17:16 ` Ian Jackson 3 siblings, 0 replies; 25+ messages in thread From: Stefano Stabellini @ 2015-03-13 11:04 UTC (permalink / raw) To: Wei Liu Cc: anthony.perard, stefano.stabellini, ian.jackson, ian.campbell, xen-devel 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 > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] xenstore: use relative path for device-model node 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu 2015-03-13 10:46 ` Ian Campbell 2015-03-13 11:04 ` Stefano Stabellini @ 2015-03-18 13:24 ` Ian Campbell 2015-03-19 10:54 ` Wei Liu 2015-03-24 17:16 ` Ian Jackson 3 siblings, 1 reply; 25+ messages in thread From: Ian Campbell @ 2015-03-18 13:24 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] xenstore: use relative path for device-model node 2015-03-18 13:24 ` Ian Campbell @ 2015-03-19 10:54 ` Wei Liu 2015-03-19 11:04 ` Ian Campbell 0 siblings, 1 reply; 25+ messages in thread From: Wei Liu @ 2015-03-19 10:54 UTC (permalink / raw) To: Ian Campbell Cc: anthony.perard, ian.jackson, stefano.stabellini, Wei Liu, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] xenstore: use relative path for device-model node 2015-03-19 10:54 ` Wei Liu @ 2015-03-19 11:04 ` Ian Campbell 0 siblings, 0 replies; 25+ messages in thread From: Ian Campbell @ 2015-03-19 11:04 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.jackson, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] xenstore: use relative path for device-model node 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu ` (2 preceding siblings ...) 2015-03-18 13:24 ` Ian Campbell @ 2015-03-24 17:16 ` Ian Jackson 3 siblings, 0 replies; 25+ messages in thread From: Ian Jackson @ 2015-03-24 17:16 UTC (permalink / raw) To: Wei Liu; +Cc: anthony.perard, stefano.stabellini, ian.campbell, xen-devel 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2015-03-24 17:16 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-13 10:34 [PATCH 0/5] Fix QEMU startup protocol Wei Liu 2015-03-13 10:34 ` [PATCH 1/5] libxl: remove DM path in libxl__device_model_destroy Wei Liu 2015-03-18 13:21 ` Ian Campbell 2015-03-18 13:23 ` Ian Campbell 2015-03-13 10:34 ` [PATCH 2/5] libxl: use LIBXL_TOOLSTACK_DOMID Wei Liu 2015-03-18 13:21 ` Ian Campbell 2015-03-13 10:34 ` [PATCH 3/5] libxl: use new QEMU xenstore protocol Wei Liu 2015-03-18 13:30 ` Ian Campbell 2015-03-19 10:36 ` Wei Liu 2015-03-19 10:55 ` Ian Campbell 2015-03-19 11:04 ` Wei Liu 2015-03-13 10:34 ` [PATCH 4/5] libxl: wait for stubdom to be ready Wei Liu 2015-03-16 17:55 ` Wei Liu 2015-03-18 13:35 ` Ian Campbell 2015-03-19 10:46 ` Wei Liu 2015-03-13 10:34 ` [PATCH 5/5] Revert "x86/hvm: wait for at least one ioreq server to be enabled" Wei Liu 2015-03-13 11:32 ` Paul Durrant 2015-03-13 10:38 ` [PATCH] xenstore: use relative path for device-model node Wei Liu 2015-03-13 10:46 ` Ian Campbell 2015-03-13 11:01 ` Wei Liu 2015-03-13 11:04 ` Stefano Stabellini 2015-03-18 13:24 ` Ian Campbell 2015-03-19 10:54 ` Wei Liu 2015-03-19 11:04 ` Ian Campbell 2015-03-24 17:16 ` Ian Jackson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.