| Message ID | 1450809903-3393-26-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote: > We are going to want to run two qemus, which will mean them having > different xs control paths. But sometimes we will run only one qemu > to do both jobs, in which case there has to be one xs control path, > because otherwise the single qemu will only see in xenstore either the > HVM DM work to do, or the PV (eg qdisk) backend work to do. > > So we need to record whether any particular domain has been set up > this way. Use a new emuid flag bit EMUID_SPLIT which just tells us > whether we are running two qemus in this way. So, do I understand correctly that the states (combinations of bits) are: 0 No QEMUs PV Single QEMU only providing PV backends DM Single QEMU only providing device emulation PV|DM Single QEMU providing PV backends and device emulation SPLIT|PV|DM A pair of QEMUs, one for PV b/e and one for device emu SPLIT|PV Nonsense SPLIT|DM Nonsense ? Describing SPLIT as an "ID" is a bit odd (it's a kind of meta thing) but I suppose I can see why it is done this way. An alternative might be to have a separate bit for the PV|DM case together, maybe you discarded that possibility though? Ian. > > If we do set this flag, we pass the emulator_id to qemu, so that qemu > and libxl agree on the xenstore path. Obviously this depends on qemu > understanding emulator_id, but we don't check that now, because: > > We do not actually ever set EMUID_SPLIT, because when we run split > qemus we also need to actually run two qemus which means a lot of > non-xs-path-related changes which are more readable in a separate > patch. > > So right now there is no overall functional change. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> > --- > v6: Rewritten. > --- > docs/misc/xenstore-paths.markdown | 11 +++++++++++ > tools/libxl/libxl_dm.c | 15 +++++++++++++++ > tools/libxl/libxl_internal.c | 16 +++++++++++++++- > tools/libxl/libxl_internal.h | 1 + > 4 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore- > paths.markdown > index 3bd31af..1dc54ac 100644 > --- a/docs/misc/xenstore-paths.markdown > +++ b/docs/misc/xenstore-paths.markdown > @@ -339,6 +339,17 @@ A PV console backend. Described in > [console.txt](console.txt) > Information relating to device models running in the domain. $DOMID is > the target domain of the device model. > > +#### ~/device-model/$DOMID[/$EMULATOR_ID]/* [INTERNAL] > + > +Information relating to device models running in the domain. $DOMID is > +the target domain of the device model. > + > +$EMULATOR_ID indentifies a specific device model instance (multiple > +device model instances for the same domain are possible). This path > +component is present only if there are (going to be) more than one > +device model for the domain. See `/libxl/$DOMID/dm-emuidmap` and > +`EMUID_SPLIT` in `libxl_internal.h`. > + > #### ~/libxl/disable_udev = ("1"|"0") [] > > Indicates whether device hotplug scripts in this domain should be run > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 58760e7..d805800 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -1236,6 +1236,11 @@ static int > libxl__build_device_model_args_new(libxl__gc *gc, > flexarray_append(dm_args, "-runas"); > flexarray_append(dm_args, user); > } > + if (state->emuidmap & (1u << EMUID_SPLIT)) { > + flexarray_append(dm_args, "-xenopts"); > + flexarray_append(dm_args, > + GCSPRINTF("emulator_id=%u", emuid)); > + } > } > flexarray_append(dm_args, NULL); > *args = (char **) flexarray_contents(dm_args); > @@ -1943,6 +1948,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, > libxl__dm_spawn_state *dmss) > const char *dm; > int logfile_w, null = -1, rc; > uint32_t domid = dmss->guest_domid; > + unsigned emuidmap; > > /* Always use qemu-xen as device model */ > dm = qemu_xen_path(gc); > @@ -1958,6 +1964,15 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, > libxl__dm_spawn_state *dmss) > flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL); > flexarray_vappend(dm_args, "-serial", "/dev/null", NULL); > flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL); > + > + rc = libxl__dm_emuidmap_get(gc, domid, &emuidmap); > + if (rc) goto error; > + > + if (emuidmap & (1u << EMUID_SPLIT)) { > + flexarray_append(dm_args, "-xenopts"); > + flexarray_append(dm_args, > + GCSPRINTF("emulator_id=%u", EMUID_PV)); > + } > flexarray_append(dm_args, NULL); > args = (char **) flexarray_contents(dm_args); > > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c > index 30271c9..8dc34ad 100644 > --- a/tools/libxl/libxl_internal.c > +++ b/tools/libxl/libxl_internal.c > @@ -556,7 +556,21 @@ void libxl__update_domain_configuration(libxl__gc > *gc, > char *libxl__dm_xs_path_rel(libxl__gc *gc, > uint32_t domid, int emuid) > { > - return GCSPRINTF("device-model/%u", domid); > + unsigned emuidmap; > + > + libxl__dm_emuidmap_get_bodgeerrors(gc, domid, &emuidmap); > + > + if (!(emuidmap & (1u << emuid))) { > + LOG(ERROR, > + "Trying to find path for emulator %d but map is %#x, probable BUG", > + emuid, emuidmap); > + } > + > + if (!(emuidmap & (1u << EMUID_SPLIT))) { > + return GCSPRINTF("device-model/%u", domid); > + } else { > + return GCSPRINTF("device-model/%u/%u", domid, emuid); > + } > } > > char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 82f264a..02c35e0 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -1962,6 +1962,7 @@ _hidden libxl_device_model_version > libxl__default_device_model(libxl__gc *gc); > enum { > EMUID_PV, > EMUID_DM, > + EMUID_SPLIT, /* we will run both PV and DM, put emuid in xs path */ > /* NB stubdom and its PV service domain not recorded here */ > }; >
Ian Campbell writes ("Re: [PATCH 25/28] libxl: emuids: Perhaps change dm xs control path"):
> So, do I understand correctly that the states (combinations of bits) are:
No.
> Describing SPLIT as an "ID" is a bit odd (it's a kind of meta thing) but I
> suppose I can see why it is done this way. An alternative might be to have
> a separate bit for the PV|DM case together, maybe you discarded that
> possibility though?
Perhaps things will be clearer with this comment, which I've just
added to add in this patch:
/*
* This EMUID enum is used for several overlapping purposes.
*
* Each qemu process has an EMUID, which is either EMUID_PV or
* EMUID_DM.
*
* Operations which talk to a qemu process need to know its emuid so
* that they talk to the right one. (Specifically, so that they find
* the right place in xenstore.) Likewise a qemu process needs to
* be told its emuid for the same reason.
*
* However, qemut and older versions of qemuu do not support emuids.
* In that case for an HVM domain, the same qemu process deals with
* both roles. In this situation the xenstore paths for PV backend
* control do not include the emuid, and therefore we can talk to the
* single qemu `via' `both' emuids.
*
* In xenstore we record which qemus a domain has. This is necessary
* for correct teardown. And, we also record whether the xenstore
* paths are unified, as discussed above, so that subsequent libxl
* operations can do the right thing. This is what the EMUID_SPLIT
* flag is for.
*
* Overall, the following scenarios are possible:
*
* dm-emuidmap libxl__device_model_xs_path
* can be passed control xs paths
* which emuids? contain what
* to distinguish
*
* 0 No QEMU processes PV -
* PV PV domain PV -
* PV HVM domain with stub dm PV; HVM stubdomid
* DM HVM domain, 1 QEMU PV; HVM -
* PV|DM|SPLIT HVM domain, 2 QEMUs privsep PV; HVM emuid
*
* The following scenarios would be handled correctly but are not set up
* by current libxl code, and/or are implausible for other reasons:
*
* 0 Stub dm only HVM stubdomid
* PV|SPLIT PV domain PV emuid
*/
Ian.
On Fri, 2016-01-08 at 14:12 +0000, Ian Jackson wrote: > Ian Campbell writes ("Re: [PATCH 25/28] libxl: emuids: Perhaps change dm > xs control path"): > > So, do I understand correctly that the states (combinations of bits) > > are: > > No. > > > Describing SPLIT as an "ID" is a bit odd (it's a kind of meta thing) > > but I > > suppose I can see why it is done this way. An alternative might be to > > have > > a separate bit for the PV|DM case together, maybe you discarded that > > possibility though? > > Perhaps things will be clearer with this comment, which I've just > added to add in this patch: Much thanks. One nit: > > * In xenstore we record which qemus a domain has. This is necessary > * for correct teardown. And, we also record whether the xenstore > * paths are unified, as discussed above, so that subsequent libxl > * operations can do the right thing. This is what the EMUID_SPLIT > * flag is for. EMUID_SPLIT actually records the inverse (i.e. that they are split, rather than unified as the previous text is discussing). I knew what you meant though, and clarifying it could well make it less obvious I suspect. Ian.
Ian Campbell writes ("Re: [PATCH 25/28] libxl: emuids: Perhaps change dm xs control path"):
> On Fri, 2016-01-08 at 14:12 +0000, Ian Jackson wrote:
> > * In xenstore we record which qemus a domain has. This is necessary
> > * for correct teardown. And, we also record whether the xenstore
> > * paths are unified, as discussed above, so that subsequent libxl
> > * operations can do the right thing. This is what the EMUID_SPLIT
> > * flag is for.
>
> EMUID_SPLIT actually records the inverse (i.e. that they are split, rather
> than unified as the previous text is discussing).
>
> I knew what you meant though, and clarifying it could well make it less
> obvious I suspect.
Yes. I could explain the reason for the sense of the flag, which is
to get the default right, but that seems obvious.
How about:
> > * In xenstore we record which qemus a domain has. This is
> > * necessary for correct teardown. And, we also record whether
> > * the xenstore paths are split or unified, as discussed above,
++++++++
> > * so that subsequent libxl operations can do the right
> > * thing. This is what the EMUID_SPLIT flag is for.
?
Ian.
On Fri, 2016-01-08 at 14:45 +0000, Ian Jackson wrote: > Ian Campbell writes ("Re: [PATCH 25/28] libxl: emuids: Perhaps change dm > xs control path"): > > On Fri, 2016-01-08 at 14:12 +0000, Ian Jackson wrote: > > > * In xenstore we record which qemus a domain has. This is > > > necessary > > > * for correct teardown. And, we also record whether the xenstore > > > * paths are unified, as discussed above, so that subsequent libxl > > > * operations can do the right thing. This is what the EMUID_SPLIT > > > * flag is for. > > > > EMUID_SPLIT actually records the inverse (i.e. that they are split, > > rather > > than unified as the previous text is discussing). > > > > I knew what you meant though, and clarifying it could well make it less > > obvious I suspect. > > Yes. I could explain the reason for the sense of the flag, which is > to get the default right, but that seems obvious. > > How about: > > > > * In xenstore we record which qemus a domain has. This is > > > * necessary for correct teardown. And, we also record whether > > > * the xenstore paths are split or unified, as discussed above, > ++++++++ > > > * so that subsequent libxl operations can do the right > > > * thing. This is what the EMUID_SPLIT flag is for. > > ? Looks good, thanks. Ian.
diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown index 3bd31af..1dc54ac 100644 --- a/docs/misc/xenstore-paths.markdown +++ b/docs/misc/xenstore-paths.markdown @@ -339,6 +339,17 @@ A PV console backend. Described in [console.txt](console.txt) Information relating to device models running in the domain. $DOMID is the target domain of the device model. +#### ~/device-model/$DOMID[/$EMULATOR_ID]/* [INTERNAL] + +Information relating to device models running in the domain. $DOMID is +the target domain of the device model. + +$EMULATOR_ID indentifies a specific device model instance (multiple +device model instances for the same domain are possible). This path +component is present only if there are (going to be) more than one +device model for the domain. See `/libxl/$DOMID/dm-emuidmap` and +`EMUID_SPLIT` in `libxl_internal.h`. + #### ~/libxl/disable_udev = ("1"|"0") [] Indicates whether device hotplug scripts in this domain should be run diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index 58760e7..d805800 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1236,6 +1236,11 @@ static int libxl__build_device_model_args_new(libxl__gc *gc, flexarray_append(dm_args, "-runas"); flexarray_append(dm_args, user); } + if (state->emuidmap & (1u << EMUID_SPLIT)) { + flexarray_append(dm_args, "-xenopts"); + flexarray_append(dm_args, + GCSPRINTF("emulator_id=%u", emuid)); + } } flexarray_append(dm_args, NULL); *args = (char **) flexarray_contents(dm_args); @@ -1943,6 +1948,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) const char *dm; int logfile_w, null = -1, rc; uint32_t domid = dmss->guest_domid; + unsigned emuidmap; /* Always use qemu-xen as device model */ dm = qemu_xen_path(gc); @@ -1958,6 +1964,15 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss) flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL); flexarray_vappend(dm_args, "-serial", "/dev/null", NULL); flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL); + + rc = libxl__dm_emuidmap_get(gc, domid, &emuidmap); + if (rc) goto error; + + if (emuidmap & (1u << EMUID_SPLIT)) { + flexarray_append(dm_args, "-xenopts"); + flexarray_append(dm_args, + GCSPRINTF("emulator_id=%u", EMUID_PV)); + } flexarray_append(dm_args, NULL); args = (char **) flexarray_contents(dm_args); diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c index 30271c9..8dc34ad 100644 --- a/tools/libxl/libxl_internal.c +++ b/tools/libxl/libxl_internal.c @@ -556,7 +556,21 @@ void libxl__update_domain_configuration(libxl__gc *gc, char *libxl__dm_xs_path_rel(libxl__gc *gc, uint32_t domid, int emuid) { - return GCSPRINTF("device-model/%u", domid); + unsigned emuidmap; + + libxl__dm_emuidmap_get_bodgeerrors(gc, domid, &emuidmap); + + if (!(emuidmap & (1u << emuid))) { + LOG(ERROR, + "Trying to find path for emulator %d but map is %#x, probable BUG", + emuid, emuidmap); + } + + if (!(emuidmap & (1u << EMUID_SPLIT))) { + return GCSPRINTF("device-model/%u", domid); + } else { + return GCSPRINTF("device-model/%u/%u", domid, emuid); + } } char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid, diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 82f264a..02c35e0 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1962,6 +1962,7 @@ _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc); enum { EMUID_PV, EMUID_DM, + EMUID_SPLIT, /* we will run both PV and DM, put emuid in xs path */ /* NB stubdom and its PV service domain not recorded here */ };