Re: [PATCH 25/28] libxl: emuids: Perhaps change dm xs control path

From: Ian Campbell <ian.campbell@citrix.com>
To: Ian Jackson <ian.jackson@eu.citrix.com>, xen-devel@lists.xensource.com
Cc: Wei Liu <wei.liu2@citrix.com>,
	Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH 25/28] libxl: emuids: Perhaps change dm xs control path
Date: Thu, 7 Jan 2016 17:26:30 +0000	[thread overview]
Message-ID: <1452187590.21055.269.camel@citrix.com> (raw)
In-Reply-To: <1450809903-3393-26-git-send-email-ian.jackson@eu.citrix.com>

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 */
>  };
>  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel