[RFC PATCH v6 00/28] libxl: Deprivilege qemu

[RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Ian Jackson]

This is a new version of Stefano Stabellini's series
  [PATCH v5 0/6] libxl: xs_restrict QEMU

I took Stefano's code as a spec for how to interact with qemu, and
have reworked the whole series.  In particular, I have
 - rebased onto staging
 - split up some of the larger patches
 - restructured the patches so that every intervening version should work
 - redone the async task handling
 - provided what seem to me to be appropriate configuration options
 - shaved a few yaks (although I tried to limit this!)
 - fixed the cross-version compatibility
 - reduced the new permission grant for the domain to only .../physmap

I have compiled this but I haven't tested it yet.

I think it would be very useful at this stage for others to read the
commit messages, internal docs, and important structural elements.

Detailed code review should probably wait until after testing which in
turn ought to wait unil we decide what (if any) design and interface
changes are needed.

And I don't expect to get any feedback until the new year :-).

Happy holidays all.

Regards,
Ian.

[PATCH 01/28] libxl: Move FILLZERO up in libxl_internal.h

[Ian Jackson]

This means that callers in libxl_internal.h can use it (eg, inline
functions).  No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_internal.h |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a556a38..e442996 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -121,6 +121,8 @@
 #define MB(_mb)     (_AC(_mb, ULL) << 20)
 #define GB(_gb)     (_AC(_gb, ULL) << 30)
 
+#define FILLZERO LIBXL_FILLZERO
+
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
 #define ROUNDUP(_val, _order)                                           \
@@ -3545,9 +3547,6 @@ _hidden void libxl__domain_suspend_callback(void *data);
     })
 
 
-#define FILLZERO LIBXL_FILLZERO
-
-
 /*
  * All of these assume (or define)
  *    libxl__gc *gc;
-- 
1.7.10.4

Re: [PATCH 01/28] libxl: Move FILLZERO up in libxl_internal.h

[Ian Campbell]

On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> This means that callers in libxl_internal.h can use it (eg, inline
> functions).  No functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>
(could go in now despite RFC-ness of the series as a whole)


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

[PATCH 02/28] libxl: libxl_types_internal.idl: Add Emacs mode comment

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_types_internal.idl |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 5e55685..4397141 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -1,3 +1,5 @@
+# -*- python -*-
+
 namespace("libxl__")
 hidden(True)
 
-- 
1.7.10.4

Re: [PATCH 02/28] libxl: libxl_types_internal.idl: Add Emacs mode comment

[Ian Campbell]

On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>
(could go in now despite RFC-ness of the series as a whole)

[PATCH 03/28] libxl: Provide libxl__dm_support_*

[Ian Jackson]

This allows code elsewhere in libxl to find out what options a device
model executable supports.  This is done by searching the usage
message for fixed strings.

Because this involves calling fork, callers must use the async
machinery.  To make this easier, and to avoid repeatedly invoking the
dm, we provide a cache within the libxl__ctx.

No call site yet.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: Completely rewritten.
    Cache is now in ctx only, not in xenstore.
    We use the proper libxl__ev_child machinery for fork.
---
 tools/libxl/libxl.c          |    4 +
 tools/libxl/libxl_dm.c       |  231 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |   76 ++++++++++++++
 3 files changed, 311 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9207621..d96189d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -87,6 +87,8 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
     ctx->sigchld_selfpipe[1] = -1;
     libxl__ev_fd_init(&ctx->sigchld_selfpipe_efd);
 
+    LIBXL_LIST_INIT(&ctx->dm_support_cache);
+
     /* The mutex is special because we can't idempotently destroy it */
 
     if (libxl__init_recursive_mutex(ctx, &ctx->lock) < 0) {
@@ -208,6 +210,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
     libxl__sigchld_notneeded(gc);
     libxl__pipe_close(ctx->sigchld_selfpipe);
 
+    libxl__dm_support_cache_destroy(gc);
+
     CTX_UNLOCK;
     pthread_mutex_destroy(&ctx->lock);
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0aaefd9..675e859 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2169,6 +2169,237 @@ out:
     return ret;
 }
 
+/*----- libxl__dm_support_* -----*/
+
+/* cache */
+
+static libxl__dm_support_cached *
+dm_support_cache_lookup(libxl__gc *gc, const char *dm)
+{
+    libxl__dm_support_cached *search;
+
+    LIBXL_LIST_FOREACH(search, &CTX->dm_support_cache, entry)
+        if (!strcmp(search->dm, dm))
+            return search;
+
+    return 0;
+}
+
+_hidden void libxl__dm_support_cache_destroy(libxl__gc *gc)
+{
+    libxl__dm_support_cached *iter, *next;
+
+    LIBXL_LIST_FOREACH_SAFE(iter, &CTX->dm_support_cache, entry, next) {
+        free(iter->dm);
+        free(iter);
+    }
+}
+
+_hidden bool libxl__dm_supported(libxl__gc *gc, const char *dm,
+                                 libxl__dm_support_check__index opt)
+{
+    libxl__dm_support_cached *cached = dm_support_cache_lookup(gc, dm);
+    assert(cached);
+    return !!(cached->bitmap & (1UL << opt));
+}
+
+/* `check', the long-running operation to check for support */
+
+static void dm_support_check_immed_cb(libxl__egc *egc, libxl__ev_time *ev,
+                                        const struct timeval *requested_abs,
+                                        int rc);
+static void dm_support_check_child_cb(libxl__egc *egc, libxl__ev_child*,
+                                      pid_t pid, int status);
+static void dm_support_check_done(libxl__egc *gc,
+                                  libxl__dm_support_check_state *ch,
+                                  int rc);
+
+_hidden void libxl__dm_support_check_init(libxl__dm_support_check_state *ch)
+{
+    FILLZERO(*ch);
+    libxl__ev_child_init(&ch->child);
+    libxl__ev_time_init(&ch->immediate);
+}
+
+static void dm_support_check_free(libxl__gc *gc,
+                                  libxl__dm_support_check_state *ch)
+{
+    if (ch->helpmsg) fclose(ch->helpmsg);
+    assert(!libxl__ev_child_inuse(&ch->child));
+    libxl__ev_time_deregister(gc, &ch->immediate);
+}
+
+_hidden int libxl__dm_support_check_start(libxl__dm_support_check_state *ch)
+{
+    int r, rc;
+
+    /* convenience aliases */
+    const char *const dm = ch->dm;
+    libxl__ao *const ao = ch->ao;
+    AO_GC;
+
+    libxl__dm_support_check_init(ch);
+
+    r = stat(ch->dm, &ch->stab);
+    if (r<0) {
+        LOGE(ERROR, "device model %s is not accessible", dm);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    libxl__dm_support_cached *cached = dm_support_cache_lookup(gc, dm);
+    if (cached &&
+        (ch->stab.st_mode  == cached->stab.st_mode  &&
+         ch->stab.st_dev   == cached->stab.st_dev   &&
+         ch->stab.st_ino   == cached->stab.st_ino   &&
+         ch->stab.st_mtime == cached->stab.st_mtime &&
+         ch->stab.st_ctime == cached->stab.st_ctime)) {
+
+        LOG(DEBUG, "returning cached support options for %s: %"PRIx32"",
+            dm, cached->bitmap);
+
+        rc = libxl__ev_time_register_rel(ao, &ch->immediate,
+                                         dm_support_check_immed_cb, 0);
+        if (rc) goto out;
+
+        /* we have queued a callback, our work in this function is done */
+        return 0;
+    }
+
+    ch->revalidate = cached;
+    if (cached) {
+        LOG(DEBUG, "revalidating cached support options for %s", dm);
+    }
+
+    ch->helpmsg = tmpfile();
+    if (!ch->helpmsg) {
+        LOGE(ERROR, "create tempfile for help message");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    pid_t pid = libxl__ev_child_fork(gc, &ch->child,
+                                     dm_support_check_child_cb);
+    if (pid < 0) {
+        LOGE(ERROR, "fork for %s -help", dm);
+        rc = pid;
+        goto out;
+    }
+
+    if (!pid) {
+        const char *const args[] = { dm, "--help", NULL };
+        int outfd = fileno(ch->helpmsg);
+        r = putenv("LC_ALL=C");
+        if (r) libxl__alloc_failed(CTX,__func__,1,0);
+        libxl__exec(gc, -1,outfd,-1, dm, (char**)args, NULL);
+    }
+    return 0;
+
+ out:
+    dm_support_check_free(gc,ch);
+    return rc;
+}
+
+static void dm_support_check_immed_cb(libxl__egc *egc, libxl__ev_time *ev,
+                                      const struct timeval *requested_abs,
+                                      int rc)
+{
+    libxl__dm_support_check_state *ch = CONTAINER_OF(ev, *ch, immediate);
+    dm_support_check_done(egc, ch, rc);
+}
+
+static void dm_support_check_lookfor(uint32_t *result_io,
+                                     const void *data, int datalen,
+                                     const char *str, int strlen,
+                                     int opt)
+{
+    if (memmem(data,datalen, str,strlen))
+        *result_io |= (1UL << opt);
+}
+
+void dm_support_check_child_cb(libxl__egc *egc, libxl__ev_child *child,
+                               pid_t pid, int status)
+{
+    libxl__dm_support_check_state *ch = CONTAINER_OF(child, *ch, child);
+    STATE_AO_GC(ch->ao);
+    int rc;
+
+    /* convenience aliases */
+    const char *const dm = ch->dm;
+
+    if (status) {
+        libxl_report_child_exitstatus(CTX, XTL_ERROR,
+                                      GCSPRINTF("%s -HELP", dm),
+                                      pid, status);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rewind(ch->helpmsg);
+
+    void *data;
+    int datalen;
+    rc = libxl_read_file_contents(CTX, "(temp file for dm help output)",
+                                  &data, &datalen);
+    if (rc) goto out;
+    libxl__ptr_add(gc, data);
+
+    if (!datalen) {
+        LOG(ERROR, "dm help output (from %s) was empty!", dm);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    ((char*)data)[datalen-1] = 0; /* a null-terminated string new */
+
+    uint32_t result = 0;
+
+#define DM_SUPPORT_CHECK_ENTRY(name, string)                    \
+    dm_support_check_lookfor(&result,                           \
+                             data, datalen,                     \
+                             string, sizeof(string)-1,          \
+                             libxl__dm_support_check__##name);
+    DM_SUPPORT_CHECK_LIST
+#undef DM_SUPPORT_CHECK_ENTRY
+
+    libxl__dm_support_cached *cached = ch->revalidate;
+    if (cached) {
+        if (cached->bitmap == result)
+            LOG(DEBUG, "confirmed cached support options for %s: %"PRIx32"",
+                dm, result);
+        else
+            LOG(DEBUG, "updating cached support options for %s:"
+                " %"PRIx32" -> %"PRIx32"", dm, cached->bitmap, result);
+    }        
+    if (!cached) {
+        LOG(DEBUG, "multiple in-parallel checks for %s", dm);
+        cached = dm_support_cache_lookup(gc, dm);
+        /* if already there, take the one which finished last, willy-nilly */
+    }
+    if (!cached) {
+        LOG(DEBUG, "caching support options for %s: %"PRIx32"", dm, result);
+        cached = libxl__zalloc(NOGC, sizeof(*cached));
+        cached->dm = libxl__strdup(NOGC, dm);
+        LIBXL_LIST_INSERT_HEAD(&CTX->dm_support_cache, cached, entry);
+    }
+
+    cached->stab = ch->stab;
+    cached->bitmap = result;
+
+    rc = 0;
+
+ out:
+    dm_support_check_done(egc, ch, rc);
+}
+
+static void dm_support_check_done(libxl__egc *egc,
+                                  libxl__dm_support_check_state *ch,
+                                  int rc)
+{
+    STATE_AO_GC(ch->ao);
+    dm_support_check_free(gc, ch);
+    ch->callback(egc, ch, rc);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e442996..f963ad6 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -181,6 +181,7 @@ typedef struct libxl__ao libxl__ao;
 typedef struct libxl__aop_occurred libxl__aop_occurred;
 typedef struct libxl__osevent_hook_nexus libxl__osevent_hook_nexus;
 typedef struct libxl__osevent_hook_nexi libxl__osevent_hook_nexi;
+typedef struct libxl__dm_support_cached libxl__dm_support_cached;
 
 _hidden void libxl__alloc_failed(libxl_ctx *, const char *func,
                          size_t nmemb, size_t size) __attribute__((noreturn));
@@ -398,6 +399,13 @@ struct libxl__poller {
     bool fds_changed;
 };
 
+struct libxl__dm_support_cached {
+    LIBXL_LIST_ENTRY(libxl__dm_support_cached) entry;
+    char *dm;
+    struct stat stab;
+    uint32_t bitmap;
+};
+
 struct libxl__gc {
     /* mini-GC */
     int alloc_maxsize; /* -1 means this is the dummy non-gc gc */
@@ -471,6 +479,8 @@ struct libxl__ctx {
     bool sigchld_user_registered;
     LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
 
+    LIBXL_LIST_HEAD(, libxl__dm_support_cached) dm_support_cache;
+
     libxl_version_info version_info;
 };
 
@@ -3198,6 +3208,72 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
  * If callback is passed rc==0, will have updated st->info appropriately */
 _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
 
+/*----- Device model support check -----*/
+
+/*
+ * This can be used to determine whether a particular qemu supports
+ * a particular option.  (Strictly, whether a particular literal
+ * string appears in the help text.)
+ *
+ * Callers should:
+ *   1. Some time it is convenient:
+ *         Set up a libxl__dm_support_check_state
+ *         Invoke libxl__dm_support_check_start
+ *         Await the callback and bomb if it fails
+ *   2. Any time after that, perhaps repeatedly
+ *         Call libxl__dm_supported
+ */
+
+typedef struct libxl__dm_support_check_state libxl__dm_support_check_state;
+
+#define DM_SUPPORT_CHECK_LIST                                 \
+    DM_SUPPORT_CHECK_ENTRY(emulator_id, "emulator_id")        \
+    DM_SUPPORT_CHECK_ENTRY(xsrestrict,  "xsrestrict")
+
+typedef enum {
+#define DM_SUPPORT_CHECK_ENTRY(name, string) \
+    libxl__dm_support_check__##name,
+DM_SUPPORT_CHECK_LIST
+#undef DM_SUPPORT_CHECK_ENTRY
+    libxl__dm_support_check__max
+} libxl__dm_support_check__index;
+
+typedef struct libxl__dm_support_check_state
+  libxl__dm_support_check_state;
+
+/* Cannot be called reentrantly */
+typedef void libxl__dm_support_check_callback(libxl__egc *egc,
+    libxl__dm_support_check_state *checking, int rc);
+
+_hidden void libxl__dm_support_cache_destroy(libxl__gc *gc);
+
+struct libxl__dm_support_check_state {
+    /* caller must fill these in, and they must all remain valid */
+    libxl__ao *ao;
+    const char *dm;
+    libxl__dm_support_check_callback *callback;
+    /* private */
+    FILE *helpmsg;
+    struct stat stab;
+    libxl__ev_child child;
+    libxl__ev_time immediate;
+    libxl__dm_support_cached *revalidate; /* points into dm_support_cache */
+};
+
+_hidden void libxl__dm_support_check_init(
+    libxl__dm_support_check_state *checking);
+
+_hidden int libxl__dm_support_check_start(
+    libxl__dm_support_check_state *checking);
+
+/*
+ * Eg
+ *  libxl__dm_supported(gc, my_dm, libxl__dm_support_check__xsrestrict);
+ * (opt is from DM_SUPPORT_CHECK_LIST, above).
+ */
+_hidden bool libxl__dm_supported(libxl__gc *gc,
+    const char *dm, libxl__dm_support_check__index opt);
+
 /*----- Domain destruction -----*/
 
 /* Domain destruction has been split into two functions:
-- 
1.7.10.4

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Ian Campbell]

On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> This allows code elsewhere in libxl to find out what options a device
> model executable supports.  This is done by searching the usage
> message for fixed strings.

Has anyone (ever, not necessarily a Xen person nor in this context)
approached upstream QEMU about a machine readable output of some sort?

I know libvirt does something similar to this, but they want to support
older versions, whereas we at least have the luxury of not caring about
versions before the point this code lands.

Ian.

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

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*"):
> On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> > This allows code elsewhere in libxl to find out what options a device
> > model executable supports.  This is done by searching the usage
> > message for fixed strings.
> 
> Has anyone (ever, not necessarily a Xen person nor in this context)
> approached upstream QEMU about a machine readable output of some sort?

Not as far as I'm aware.  Stefano, could you perhaps put out some
feelers ?

Ian.

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Jim Fehlig]

On 01/07/2016 10:13 AM, Ian Campbell wrote:
> On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
>> This allows code elsewhere in libxl to find out what options a device
>> model executable supports.  This is done by searching the usage
>> message for fixed strings.
> Has anyone (ever, not necessarily a Xen person nor in this context)
> approached upstream QEMU about a machine readable output of some sort?
>
> I know libvirt does something similar to this, but they want to support
> older versions, whereas we at least have the luxury of not caring about
> versions before the point this code lands.

Since qemu 1.2.0, libvirt has been using the various QMP commands to probe for
qemu capabilities, instead of parsing help output.

Regards,
Jim

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Ian Campbell]

On Mon, 2016-01-11 at 10:00 -0700, Jim Fehlig wrote:
> On 01/07/2016 10:13 AM, Ian Campbell wrote:
> > On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> > > This allows code elsewhere in libxl to find out what options a device
> > > model executable supports.  This is done by searching the usage
> > > message for fixed strings.
> > Has anyone (ever, not necessarily a Xen person nor in this context)
> > approached upstream QEMU about a machine readable output of some sort?
> > 
> > I know libvirt does something similar to this, but they want to support
> > older versions, whereas we at least have the luxury of not caring about
> > versions before the point this code lands.
> 
> Since qemu 1.2.0, libvirt has been using the various QMP commands to probe for
> qemu capabilities, instead of parsing help output.

As in it spawns a qemu specifically to ask the questions and then kills it
and starts what it needs _or_ it starts the qemu with minimal command line
cfg and then dynamically pokes in the full config via qmp?

Ian.

> 
> Regards,
> Jim
> 

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

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Jim Fehlig]

Ian Campbell wrote:
> On Mon, 2016-01-11 at 10:00 -0700, Jim Fehlig wrote:
>> On 01/07/2016 10:13 AM, Ian Campbell wrote:
>>> On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
>>>> This allows code elsewhere in libxl to find out what options a device
>>>> model executable supports.  This is done by searching the usage
>>>> message for fixed strings.
>>> Has anyone (ever, not necessarily a Xen person nor in this context)
>>> approached upstream QEMU about a machine readable output of some sort?
>>>
>>> I know libvirt does something similar to this, but they want to support
>>> older versions, whereas we at least have the luxury of not caring about
>>> versions before the point this code lands.
>> Since qemu 1.2.0, libvirt has been using the various QMP commands to probe for
>> qemu capabilities, instead of parsing help output.
> 
> As in it spawns a qemu specifically to ask the questions and then kills it
> and starts what it needs _or_ it starts the qemu with minimal command line
> cfg and then dynamically pokes in the full config via qmp?

The latter. When the qemu driver loads, it starts qemu, pokes it for
capabilities via qmp, caches what it finds, then terminates it. The cached
capabilities are used until the associated qemu binary is changed/updated.

If interested in peeking at the code, see virQEMUCapsInitQMP() and the functions
it calls in $libvirt_src/src/qemu/qemu_capabilities.c. E.g.

virQEMUCapsInitQMP()
  -> virQEMUCapsInitQMPMonitor()
    -> virQEMUCapsInitQMPBasic()

Regards,
Jim

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Ian Campbell]

On Thu, 2016-01-14 at 11:31 -0700, Jim Fehlig wrote:
> Ian Campbell wrote:
> > On Mon, 2016-01-11 at 10:00 -0700, Jim Fehlig wrote:
> > > On 01/07/2016 10:13 AM, Ian Campbell wrote:
> > > > On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> > > > > This allows code elsewhere in libxl to find out what options a
> > > > > device
> > > > > model executable supports.  This is done by searching the usage
> > > > > message for fixed strings.
> > > > Has anyone (ever, not necessarily a Xen person nor in this context)
> > > > approached upstream QEMU about a machine readable output of some
> > > > sort?
> > > > 
> > > > I know libvirt does something similar to this, but they want to
> > > > support
> > > > older versions, whereas we at least have the luxury of not caring
> > > > about
> > > > versions before the point this code lands.
> > > Since qemu 1.2.0, libvirt has been using the various QMP commands to
> > > probe for
> > > qemu capabilities, instead of parsing help output.
> > 
> > As in it spawns a qemu specifically to ask the questions and then kills
> > it
> > and starts what it needs _or_ it starts the qemu with minimal command
> > line
> > cfg and then dynamically pokes in the full config via qmp?
> 
> The latter.

From the description I think you meant former?

>  When the qemu driver loads, it starts qemu, pokes it for
> capabilities via qmp, caches what it finds, then terminates it. The cached
> capabilities are used until the associated qemu binary is changed/updated.
> 
> If interested in peeking at the code, see virQEMUCapsInitQMP() and the functions
> it calls in $libvirt_src/src/qemu/qemu_capabilities.c. E.g.
> 
> virQEMUCapsInitQMP()
>   -> virQEMUCapsInitQMPMonitor()
>     -> virQEMUCapsInitQMPBasic()

Thanks.

When I spoke to Ian J yesterday we decided doing this properly (via QMP as
above) would be nice it was out of scope for this series for now. It would
make a nice future bit of cleanup though for sure.

Ian.

> 
> Regards,
> Jim
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

Re: [PATCH 03/28] libxl: Provide libxl__dm_support_*

[Jim Fehlig]

On 01/15/2016 02:56 AM, Ian Campbell wrote:
> On Thu, 2016-01-14 at 11:31 -0700, Jim Fehlig wrote:
>> Ian Campbell wrote:
>>> On Mon, 2016-01-11 at 10:00 -0700, Jim Fehlig wrote:
>>>> On 01/07/2016 10:13 AM, Ian Campbell wrote:
>>>>> On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
>>>>>> This allows code elsewhere in libxl to find out what options a
>>>>>> device
>>>>>> model executable supports.  This is done by searching the usage
>>>>>> message for fixed strings.
>>>>> Has anyone (ever, not necessarily a Xen person nor in this context)
>>>>> approached upstream QEMU about a machine readable output of some
>>>>> sort?
>>>>>
>>>>> I know libvirt does something similar to this, but they want to
>>>>> support
>>>>> older versions, whereas we at least have the luxury of not caring
>>>>> about
>>>>> versions before the point this code lands.
>>>> Since qemu 1.2.0, libvirt has been using the various QMP commands to
>>>> probe for
>>>> qemu capabilities, instead of parsing help output.
>>> As in it spawns a qemu specifically to ask the questions and then kills
>>> it
>>> and starts what it needs _or_ it starts the qemu with minimal command
>>> line
>>> cfg and then dynamically pokes in the full config via qmp?
>> The latter.
> From the description I think you meant former?

Yes :-). Brain thought one way, fingers typed another...

Regards,
Jim

>
>>  When the qemu driver loads, it starts qemu, pokes it for
>> capabilities via qmp, caches what it finds, then terminates it. The cached
>> capabilities are used until the associated qemu binary is changed/updated.
>>
>> If interested in peeking at the code, see virQEMUCapsInitQMP() and the functions
>> it calls in $libvirt_src/src/qemu/qemu_capabilities.c. E.g.
>>
>> virQEMUCapsInitQMP()
>>   -> virQEMUCapsInitQMPMonitor()
>>     -> virQEMUCapsInitQMPBasic()
> Thanks.
>
> When I spoke to Ian J yesterday we decided doing this properly (via QMP as
> above) would be nice it was out of scope for this series for now. It would
> make a nice future bit of cleanup though for sure.
>
> Ian.
>
>> Regards,
>> Jim
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

[PATCH 04/28] libxl: Invoke libxl__dm_support_*

[Ian Jackson]

When creating a domain (including when restoring), invoke the
dm_support_check machinery on the principal qemu dm (if applicable).

This involves another introducing function callback boundary in the
domcreate control flow; but this is a straightforward one.

Now, libxl__dm_supported is available (for the principal qemu dm) in
the rest of the creation path.

No callers of libxl__dm_supported yet.  So in this patch we just
pointlessly run qemu, collect and scan its usage message, and cache
what we understand from the results.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_create.c   |   47 ++++++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_internal.h |    1 +
 2 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 261816a..1af7066 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -719,6 +719,8 @@ static void remus_checkpoint_stream_done(
  */
 
 /* Event callbacks, in this order: */
+static void domcreate_dm_support_checked(libxl__egc *egc,
+    libxl__dm_support_check_state *checking, int rc);
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc);
@@ -774,7 +776,7 @@ static void initiate_domain_create(libxl__egc *egc,
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
     libxl__domain_build_state *const state = &dcs->build_state;
-    const int restore_fd = dcs->restore_fd;
+    const libxl_domain_build_info *b_info = &d_config->b_info;
 
     domid = dcs->domid_soft_reset;
 
@@ -911,10 +913,6 @@ static void initiate_domain_create(libxl__egc *egc,
         }
     }
 
-    dcs->bl.ao = ao;
-    libxl_device_disk *bootdisk =
-        d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
-
     /*
      * The devid has to be set before launching the device model. For the
      * hotplug case this is done in libxl_device_nic_add but on domain
@@ -942,6 +940,41 @@ static void initiate_domain_create(libxl__egc *egc,
             d_config->nics[i].devid = ++last_devid;
     }
 
+    if (b_info->type != LIBXL_DOMAIN_TYPE_HVM ||
+        libxl_defbool_val(b_info->device_model_stubdomain)) {
+        domcreate_dm_support_checked(egc, &dcs->dm_support_check, 0);
+    } else {
+        dcs->dm_support_check.ao = ao;
+        dcs->dm_support_check.dm = libxl__domain_device_model(gc, b_info);
+        dcs->dm_support_check.callback = domcreate_dm_support_checked;
+        libxl__dm_support_check_start(&dcs->dm_support_check);
+    }
+
+error_out:
+    assert(ret);
+    domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_dm_support_checked(libxl__egc *egc,
+    libxl__dm_support_check_state *checking, int rc)
+{
+    libxl__domain_create_state *dcs =
+        CONTAINER_OF(checking, *dcs, dm_support_check);
+    STATE_AO_GC(dcs->ao);
+    
+    /* convenience aliases */
+    libxl_domain_config *const d_config = dcs->guest_config;
+    const int restore_fd = dcs->restore_fd;
+
+    if (rc) {
+        domcreate_complete(egc, dcs, rc);
+        return;
+    }
+
+    dcs->bl.ao = ao;
+    libxl_device_disk *bootdisk =
+        d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
+
     if (restore_fd >= 0 || dcs->domid_soft_reset != INVALID_DOMID) {
         LOG(DEBUG, "restoring, not running bootloader");
         domcreate_bootloader_done(egc, &dcs->bl, 0);
@@ -959,10 +992,6 @@ static void initiate_domain_create(libxl__egc *egc,
         libxl__bootloader_run(egc, &dcs->bl);
     }
     return;
-
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
 }
 
 static void domcreate_bootloader_console_available(libxl__egc *egc,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f963ad6..fd6ef61 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3510,6 +3510,7 @@ struct libxl__domain_create_state {
     /* private to domain_create */
     int guest_domid;
     libxl__domain_build_state build_state;
+    libxl__dm_support_check_state dm_support_check;
     libxl__bootloader_state bl;
     libxl__stub_dm_spawn_state dmss;
         /* If we're not doing stubdom, we use only dmss.dm,
-- 
1.7.10.4

[PATCH 05/28] libxl: libxl__spawn_stub_dm: Introduce `dmpath'

[Ian Jackson]

Combine two identical calls to libxl__device_model_xs_path, for
clarity.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_dm.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 675e859..6f5fe45 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1557,11 +1557,11 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     perm[1].perms = XS_PERM_READ;
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
-    xs_mkdir(ctx->xsh, t,
-             libxl__device_model_xs_path(gc, dm_domid, guest_domid, ""));
+    const char *dmpath = libxl__device_model_xs_path(gc,
+            dm_domid, guest_domid, "");
+    xs_mkdir(ctx->xsh, t, dmpath);
     xs_set_permissions(ctx->xsh, t,
-                       libxl__device_model_xs_path(gc, dm_domid,
-                                                   guest_domid, ""),
+                       dmpath,
                        perm, ARRAY_SIZE(perm));
     if (!xs_transaction_end(ctx->xsh, t, 0))
         if (errno == EAGAIN)
-- 
1.7.10.4

[PATCH 06/28] libxl: qemu_pci_*: Introduce DMPATH local macro, twice

[Ian Jackson]

This condenses a number of repetetive calls to
libxl__device_model_xs_path.  No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_pci.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index dc10cb7..2c7a2c6 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -966,10 +966,13 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     char *state, *vdevfn;
     uint32_t dm_domid;
 
+#define DMPATH(suffix) \
+    libxl__device_model_xs_path(gc, dm_domid, domid, (suffix))
+
     dm_domid = libxl_get_stubdom_id(CTX, domid);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = DMPATH("/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = DMPATH("/parameter");
     if (pcidev->vdevfn) {
         libxl__xs_printf(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
                          pcidev->domain, pcidev->bus, pcidev->dev,
@@ -984,9 +987,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     libxl__qemu_traditional_cmd(gc, domid, "pci-ins");
     rc = libxl__wait_for_device_model_deprecated(gc, domid, NULL, NULL,
                                       pci_ins_check, state);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = DMPATH("/parameter");
     vdevfn = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = DMPATH("/state");
     if ( rc < 0 )
         LOG(ERROR, "qemu refused to add device: %s", vdevfn);
     else if ( sscanf(vdevfn, "0x%x", &pcidev->vdevfn) != 1 ) {
@@ -995,6 +998,8 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     }
     xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
 
+#undef DMPATH
+
     return rc;
 }
 
@@ -1307,9 +1312,12 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
 
     dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+#define DMPATH(suffix) \
+    libxl__device_model_xs_path(gc, dm_domid, domid, suffix)
+
+    path = DMPATH("/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = DMPATH("/parameter");
     libxl__xs_printf(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
                      pcidev->bus, pcidev->dev, pcidev->func);
 
@@ -1327,9 +1335,11 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
             return ERROR_FAIL;
         }
     }
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = DMPATH("/state");
     xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
 
+#undef DMPATH
+
     return 0;
 }
 
-- 
1.7.10.4

[PATCH 07/28] libxl: libxl__device_model_xs_path: Add emulator_id parameter

[Ian Jackson]

We are going to want to talk to two different qemus for some domains.
This will involve a different xenstore path for the two qemus, so
libxl__device_model_xs_path will need to take a parameter to say which.

Most call sites will want to talk to the main (device model, `DM')
emulator.  So introduce that parameter now, currently always passing
EMUID_DM, which is defined in a dummy way that ensures the actual
argument is EMUID_DM.

No functional change.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v6: Largely rewritten, but in some sense (at least conceptually)
    based on a similar concept in v5.
---
 tools/libxl/libxl_device.c      |    3 ++-
 tools/libxl/libxl_dm.c          |   11 ++++++-----
 tools/libxl/libxl_dom.c         |   12 +++++++-----
 tools/libxl/libxl_dom_suspend.c |    3 ++-
 tools/libxl/libxl_internal.c    |    4 +++-
 tools/libxl/libxl_internal.h    |    9 ++++++---
 tools/libxl/libxl_pci.c         |    4 ++--
 7 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8bb5e93..22665e4 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1183,7 +1183,8 @@ int libxl__wait_for_device_model_deprecated(libxl__gc *gc,
     char *path;
     uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, dm_domid, domid,
+                                       EMUID_DM, "/state");
     return libxl__xenstore_child_wait_deprecated(gc, domid,
                                      LIBXL_DEVICE_MODEL_START_TIMEOUT,
                                      "Device Model", path, state, spawning,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 6f5fe45..ea82e11 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1558,7 +1558,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
     const char *dmpath = libxl__device_model_xs_path(gc,
-            dm_domid, guest_domid, "");
+            dm_domid, guest_domid, EMUID_DM, "");
     xs_mkdir(ctx->xsh, t, dmpath);
     xs_set_permissions(ctx->xsh, t,
                        dmpath,
@@ -1724,7 +1724,7 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
                                   dm_domid, sdss->dm.guest_domid);
     sdss->xswait.path =
         libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
-                                    "/state");
+                                    EMUID_DM, "/state");
     sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
     sdss->xswait.callback = stubdom_xswait_cb;
     rc = libxl__xswait_start(gc, &sdss->xswait);
@@ -1831,7 +1831,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         free(path);
     }
 
-    path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
+    path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
+                                       domid, EMUID_DM, "");
     xs_mkdir(ctx->xsh, XBT_NULL, path);
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
@@ -1886,7 +1887,7 @@ retry_transaction:
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
     spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                                domid, "/state");
+                                                domid, EMUID_DM, "/state");
     spawn->timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     spawn->pidpath = GCSPRINTF("%s/image/device-model-pid", dom_path);
     spawn->midproc_cb = libxl__spawn_record_pid;
@@ -2099,7 +2100,7 @@ out:
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
     char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                             domid, "");
+                                             domid, EMUID_DM, "");
     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. */
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 47971a9..6ded9c1 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1103,7 +1103,8 @@ int libxl__qemu_traditional_cmd(libxl__gc *gc, uint32_t domid,
 {
     char *path = NULL;
     uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/command");
+    path = libxl__device_model_xs_path(gc, dm_domid,
+                                       domid, EMUID_DM, "/command");
     return libxl__xs_printf(gc, XBT_NULL, path, "%s", cmd);
 }
 
@@ -1134,7 +1135,8 @@ int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
 
     const uint32_t domid = dcs->guest_domid;
     const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
-    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid, domid, "");
+    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid,
+                                                      domid, EMUID_DM, "");
 
     while (next < end) {
         key = next;
@@ -1225,9 +1227,9 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty
     if (!lds->cmd_path) {
         uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
         lds->cmd_path = libxl__device_model_xs_path(gc, dm_domid, domid,
-                                                    "/logdirty/cmd");
+                                              EMUID_DM, "/logdirty/cmd");
         lds->ret_path = libxl__device_model_xs_path(gc, dm_domid, domid,
-                                                    "/logdirty/ret");
+                                              EMUID_DM, "/logdirty/ret");
     }
     lds->cmd = enable ? "enable" : "disable";
 
@@ -1442,7 +1444,7 @@ int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
     const uint32_t domid = dss->domid;
     const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-    xs_root = libxl__device_model_xs_path(gc, dm_domid, domid, "");
+    xs_root = libxl__device_model_xs_path(gc, dm_domid, domid, EMUID_DM, "");
 
     entries = libxl__xs_directory(gc, 0, GCSPRINTF("%s/physmap", xs_root),
                                   &nr_entries);
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 3313ad1..d25fb59 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -383,7 +383,8 @@ int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid)
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
         uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-        path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+        path = libxl__device_model_xs_path(gc, dm_domid, domid,
+                                           EMUID_DM, "/state");
         state = libxl__xs_read(gc, XBT_NULL, path);
         if (state != NULL && !strcmp(state, "paused")) {
             libxl__qemu_traditional_cmd(gc, domid, "continue");
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 328046b..ec93797 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -554,7 +554,9 @@ void libxl__update_domain_configuration(libxl__gc *gc,
 }
 
 char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
-                                  uint32_t domid, const char *format,  ...)
+                                  uint32_t domid,
+                                  struct dummy_qemu_emulator_id emuid,
+                                  const char *format,  ...)
 {
     char *s, *fmt;
     va_list ap;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fd6ef61..ea56cda 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1955,9 +1955,12 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
-_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
-                                          uint32_t domid,
-                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);
+
+static const struct dummy_qemu_emulator_id { int x; } EMUID_DM;
+_hidden char *libxl__device_model_xs_path(libxl__gc *gc,
+        uint32_t dm_domid,
+        uint32_t domid, struct dummy_qemu_emulator_id,
+        const char *format, ...) PRINTF_ATTRIBUTE(5, 6);
 
 /*
  * Calling context and GC for event-generating functions:
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 2c7a2c6..86b8cf3 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -967,7 +967,7 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     uint32_t dm_domid;
 
 #define DMPATH(suffix) \
-    libxl__device_model_xs_path(gc, dm_domid, domid, (suffix))
+    libxl__device_model_xs_path(gc, dm_domid, domid, EMUID_DM, (suffix))
 
     dm_domid = libxl_get_stubdom_id(CTX, domid);
     path = DMPATH("/state");
@@ -1313,7 +1313,7 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
     dm_domid = libxl_get_stubdom_id(CTX, domid);
 
 #define DMPATH(suffix) \
-    libxl__device_model_xs_path(gc, dm_domid, domid, suffix)
+    libxl__device_model_xs_path(gc, dm_domid, domid, EMUID_DM, suffix)
 
     path = DMPATH("/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-- 
1.7.10.4

[PATCH 08/28] libxl: libxl__destroy_domid: Bring dm destruction together

[Ian Jackson]

Change the order of operations so that the dm determination and
destruction occur together.

The functional change is that the xenstore reads relating to the dm
determination now occur after the pci devices have been removed and
the domain has been paused.  This should not have any visible effect.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d96189d..1ac5d81 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1621,6 +1621,19 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         goto out;
     }
 
+    if (libxl__device_pci_destroy_all(gc, domid) < 0)
+        LOG(ERROR, "pci shutdown failed for domid %d", domid);
+    rc = xc_domain_pause(ctx->xch, domid);
+    if (rc < 0) {
+        LOGEV(ERROR, rc, "xc_domain_pause failed for %d", domid);
+    }
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    if (!dom_path) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     switch (libxl__domain_type(gc, domid)) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (libxl_get_stubdom_id(CTX, domid)) {
@@ -1639,18 +1652,6 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         abort();
     }
 
-    dom_path = libxl__xs_get_dompath(gc, domid);
-    if (!dom_path) {
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    if (libxl__device_pci_destroy_all(gc, domid) < 0)
-        LOG(ERROR, "pci shutdown failed for domid %d", domid);
-    rc = xc_domain_pause(ctx->xch, domid);
-    if (rc < 0) {
-        LOGEV(ERROR, rc, "xc_domain_pause failed for %d", domid);
-    }
     if (dm_present) {
         if (libxl__destroy_device_model(gc, domid) < 0)
             LOG(ERROR, "libxl__destroy_device_model failed for %d", domid);
-- 
1.7.10.4

[PATCH 09/28] libxl: Move some error handling and cleanup into libxl__destroy_device_model

[Ian Jackson]

Move
 - the error log message
 - the call to libxl__qmp_cleanup
into libxl__destroy_device_model from the one call site in
libxl__destroy_domid.

We are going to want to call libxl__destroy_device_model more than
once, and this code would otherwise need to be repeated for the other
call site(s).

The call site now completely ignores the error, sadly.  But I am not
intending in this series to try to undertake destruction
error-handling cleanup as well.

No functional change other than slight change to the error message.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl.c    |    4 +---
 tools/libxl/libxl_dm.c |   10 +++++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1ac5d81..5414649 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1653,10 +1653,8 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     }
 
     if (dm_present) {
-        if (libxl__destroy_device_model(gc, domid) < 0)
-            LOG(ERROR, "libxl__destroy_device_model failed for %d", domid);
+        libxl__destroy_device_model(gc, domid);
 
-        libxl__qmp_cleanup(gc, domid);
     }
     dis->drs.ao = ao;
     dis->drs.domid = domid;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ea82e11..b85b377 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2099,13 +2099,21 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    int rc;
+
     char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
                                              domid, EMUID_DM, "");
     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,
+    rc = kill_device_model(gc,
                 GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+    if (rc)
+        LOG(ERROR, "libxl__destroy_device_model failed for %d",
+            domid);
+
+    libxl__qmp_cleanup(gc, domid);
+    return rc;
 }
 
 int libxl__need_xenpv_qemu(libxl__gc *gc,
-- 
1.7.10.4

[PATCH 10/28] libxl: kill_device_model: Silently tolerate ENOENT on pid xs path

[Ian Jackson]

If the pid is absent in xenstore, this means there is no such DM.  Do
not complain about this, then.  This allows us to simplify call sites.
Do so for libxl__destroy_domid.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl.c    |    4 +---
 tools/libxl/libxl_dm.c |    7 ++++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5414649..2a0c092 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1606,7 +1606,6 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     libxl_ctx *ctx = CTX;
     uint32_t domid = dis->domid;
     char *dom_path;
-    char *pid;
     int rc, dm_present;
 
     libxl__ev_child_init(&dis->destroyer);
@@ -1642,8 +1641,7 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         }
         /* fall through */
     case LIBXL_DOMAIN_TYPE_PV:
-        pid = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
-        dm_present = (pid != NULL);
+        dm_present = 1;
         break;
     case LIBXL_DOMAIN_TYPE_INVALID:
         rc = ERROR_FAIL;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index b85b377..593f3e6 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2053,11 +2053,16 @@ static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
     int ret, pid;
 
     ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
-    if (ret || !xs_pid) {
+    if (ret) {
         LOG(ERROR, "unable to find device model pid in %s", xs_path_pid);
         ret = ret ? : ERROR_FAIL;
         goto out;
     }
+    if (!xs_pid) {
+        /* Jolly good */
+        ret = 0;
+        goto out;
+    }
     pid = atoi(xs_pid);
 
     ret = kill(pid, SIGHUP);
-- 
1.7.10.4

[PATCH 11/28] libxl: emuids: Pass correct emuid to internal functions

[Ian Jackson]

Provide EMUID_PV and use it appropriately.  We split our qemus into
two:

EMUID_DM does actual emulation.  EMUID_PV is for PV backends.  Most
places relate to emulation and can simply pass EMUID_DM.

Creation is a notable exception, where we pass the emuid as a new
parameter to the spawn functions.

There is no overall functional change, because the ultimate consumer,
libxl__device_model_xs_path, does not pay any attention to the
parameter yet, and because we still need to plumb the emuid through to
the qemu argument construction.

Also, domain destruction does not yet pass the right emuid.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com
---
v6: Largely rewritten, but in some sense (at least conceptually)
    based on a similar concept in v5.
---
 tools/libxl/libxl_create.c   |   11 ++++++-----
 tools/libxl/libxl_dm.c       |   16 +++++++++-------
 tools/libxl/libxl_internal.c |    3 +--
 tools/libxl/libxl_internal.h |   15 +++++++++++----
 4 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1af7066..41ece5a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1309,10 +1309,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl_device_vkb_dispose(&vkb);
 
         dcs->dmss.dm.guest_domid = domid;
-        if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
-            libxl__spawn_stub_dm(egc, &dcs->dmss);
-        else
-            libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+        if (libxl_defbool_val(d_config->b_info.device_model_stubdomain)) {
+            libxl__spawn_stub_dm(egc, &dcs->dmss, EMUID_DM);
+        } else {
+            libxl__spawn_local_dm(egc, &dcs->dmss.dm, EMUID_DM);
+        }
 
         /*
          * Handle the domain's (and the related stubdomain's) access to
@@ -1348,7 +1349,7 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
 
         if (need_qemu) {
             dcs->dmss.dm.guest_domid = domid;
-            libxl__spawn_local_dm(egc, &dcs->dmss.dm);
+            libxl__spawn_local_dm(egc, &dcs->dmss.dm, EMUID_PV);
             return;
         } else {
             assert(!dcs->dmss.dm.guest_domid);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 593f3e6..1e1dfa0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1434,7 +1434,8 @@ char *libxl__stub_dm_name(libxl__gc *gc, const char *guest_name)
     return GCSPRINTF("%s-dm", guest_name);
 }
 
-void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
+void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss,
+                          int emuid)
 {
     STATE_AO_GC(sdss->dm.spawn.ao);
     libxl_ctx *ctx = libxl__gc_owner(gc);
@@ -1558,7 +1559,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
     const char *dmpath = libxl__device_model_xs_path(gc,
-            dm_domid, guest_domid, EMUID_DM, "");
+            dm_domid, guest_domid, emuid, "");
     xs_mkdir(ctx->xsh, t, dmpath);
     xs_set_permissions(ctx->xsh, t,
                        dmpath,
@@ -1668,7 +1669,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     sdss->pvqemu.build_state = &sdss->dm_state;
     sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
 
-    libxl__spawn_local_dm(egc, &sdss->pvqemu);
+    libxl__spawn_local_dm(egc, &sdss->pvqemu, EMUID_PV);
 
     return;
 
@@ -1724,7 +1725,7 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
                                   dm_domid, sdss->dm.guest_domid);
     sdss->xswait.path =
         libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
-                                    EMUID_DM, "/state");
+                                    EMUID_PV, "/state");
     sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
     sdss->xswait.callback = stubdom_xswait_cb;
     rc = libxl__xswait_start(gc, &sdss->xswait);
@@ -1771,7 +1772,8 @@ static void device_model_spawn_outcome(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc);
 
-void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
+void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss,
+                           int emuid)
 {
     /* convenience aliases */
     const int domid = dmss->guest_domid;
@@ -1832,7 +1834,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     }
 
     path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                       domid, EMUID_DM, "");
+                                       domid, emuid, "");
     xs_mkdir(ctx->xsh, XBT_NULL, path);
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
@@ -1887,7 +1889,7 @@ retry_transaction:
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
     spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                                domid, EMUID_DM, "/state");
+                                                domid, emuid, "/state");
     spawn->timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     spawn->pidpath = GCSPRINTF("%s/image/device-model-pid", dom_path);
     spawn->midproc_cb = libxl__spawn_record_pid;
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ec93797..843fdbf 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -554,8 +554,7 @@ void libxl__update_domain_configuration(libxl__gc *gc,
 }
 
 char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
-                                  uint32_t domid,
-                                  struct dummy_qemu_emulator_id emuid,
+                                  uint32_t domid, int emuid,
                                   const char *format,  ...)
 {
     char *s, *fmt;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ea56cda..71c1e17 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1956,10 +1956,15 @@ _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
 
-static const struct dummy_qemu_emulator_id { int x; } EMUID_DM;
+enum {
+    EMUID_PV,
+    EMUID_DM,
+    /* NB stubdom and its PV service domain not recorded here */
+};
+
 _hidden char *libxl__device_model_xs_path(libxl__gc *gc,
         uint32_t dm_domid,
-        uint32_t domid, struct dummy_qemu_emulator_id,
+        uint32_t domid, int emuid,
         const char *format, ...) PRINTF_ATTRIBUTE(5, 6);
 
 /*
@@ -3399,7 +3404,8 @@ struct libxl__dm_spawn_state {
     libxl__dm_spawn_cb *callback;
 };
 
-_hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*);
+_hidden void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state*,
+                                   int emuid);
 
 /* Stubdom device models. */
 
@@ -3418,7 +3424,8 @@ typedef struct {
     libxl__xswait_state xswait;
 } libxl__stub_dm_spawn_state;
 
-_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
+_hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*,
+                                  int emuid);
 
 _hidden char *libxl__stub_dm_name(libxl__gc *gc, const char * guest_name);
 
-- 
1.7.10.4

[PATCH 12/28] libxl: Use libxl__device_model_xs_path in libxl__spawn_qdisk_backend

[Ian Jackson]

Rather than open-coding the answer.  We are going to change this path.

No functional change (other than to very unlikely error paths).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: Broken out from a portmanteau patch which was in v5, and fixed.
---
 tools/libxl/libxl_dm.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 1e1dfa0..916d12c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2018,7 +2018,8 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     dmss->build_state->saved_state = 0;
 
     dmss->spawn.what = GCSPRINTF("domain %u Qdisk backend", domid);
-    dmss->spawn.xspath = GCSPRINTF("device-model/%u/state", domid);
+    dmss->spawn.xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
+                                                     domid, EMUID_PV, "/state");
     dmss->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     /*
      * We cannot save Qemu pid anywhere in the xenstore guest dir,
-- 
1.7.10.4

[PATCH 13/28] libxl: emuids: Record which emuids we have started to create

[Ian Jackson]

Record in xenstore which emulator(s) we have started.  Right now this
information is not used.

The _get_bodgeerrors function is going to be needed in several call
sites where errors are currently not handled well.  I don't want to
try to combine an error handling rework with this series.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch - storing this information here is a new approach in v6.
---
 docs/misc/xenstore-paths.markdown |    5 +++
 tools/libxl/libxl_create.c        |    2 ++
 tools/libxl/libxl_dm.c            |   61 +++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h      |   14 +++++++++
 4 files changed, 82 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 197bb0f..208b64b 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -505,6 +505,11 @@ The guest's virtual time offset from UTC in seconds.
 
 The device model version for a domain.
 
+#### ~/libxl/$DOMID/dm-emuidmap = INTEGER
+
+Which emulator IDs are serving a domain.  This is a bitmask of
+`1u<<EMUID_*` values (see `libxl_internal.h`).
+
 #### /libxl/$DOMID/remus/netbuf/$DEVID/ifb = STRING [n,INTERNAL]
 
 ifb device used by Remus to buffer network output from the associated vif.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 41ece5a..71893c5 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -410,6 +410,8 @@ int libxl__domain_build(libxl__gc *gc,
     struct timeval start_time;
     int i, ret;
 
+    state->emuidmap = 0;
+
     ret = libxl__build_pre(gc, domid, d_config, state);
     if (ret)
         goto out;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 916d12c..2bd1eb0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1459,6 +1459,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss,
         goto out;
     }
 
+    ret = libxl__dm_emuidmap_add(gc, guest_domid, d_state, emuid);
+    if (ret) goto out;
+
     sdss->pvqemu.guest_domid = 0;
 
     libxl_domain_create_info_init(&dm_config->c_info);
@@ -1801,6 +1804,9 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss,
         abort();
     }
 
+    rc = libxl__dm_emuidmap_add(gc, domid, state, emuid);
+    if (rc) goto out;
+
     dm = libxl__domain_device_model(gc, b_info);
     if (!dm) {
         rc = ERROR_FAIL;
@@ -2417,6 +2423,61 @@ static void dm_support_check_done(libxl__egc *egc,
     ch->callback(egc, ch, rc);
 }
 
+/* emuid recording */
+
+static const char *emuidmap_path(libxl__gc *gc, int domid)
+{
+    return GCSPRINTF("libxl/%d/dm-emuidmap", domid);
+}
+
+int libxl__dm_emuidmap_add(libxl__gc *gc, int domid,
+                           libxl__domain_build_state *bstate, int emuid)
+{
+    unsigned newflag = 1u << emuid;
+
+    assert(!(bstate->emuidmap & newflag));
+    bstate->emuidmap |= newflag;
+    return libxl__xs_printf(gc, XBT_NULL, emuidmap_path(gc, domid),
+                            "%u", bstate->emuidmap);
+}
+
+int libxl__dm_emuidmap_get(libxl__gc *gc, int domid,
+                           unsigned *emuidmap_r)
+{
+    const char *s;
+    int rc;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL, emuidmap_path(gc, domid), &s);
+    if (rc) goto out;
+
+    if (!s) {
+        *emuidmap_r = 0;
+        return 0;
+    }
+
+    *emuidmap_r = atoi(s);
+    return 0;
+
+ out:
+    return rc;
+}
+
+void libxl__dm_emuidmap_get_bodgeerrors(libxl__gc *gc, int domid,
+                                        unsigned *emuidmap_r)
+{
+    int rc;
+
+    rc = libxl__dm_emuidmap_get(gc, domid, emuidmap_r);
+    if (rc) {
+        *emuidmap_r =
+            (1u << EMUID_DM) |
+            (1u << EMUID_PV);
+        LOG(ERROR,
+"Failed libxl_dm_emuidmap_get (rc=%d), using %#x (probably a wrong answer)",
+            rc, *emuidmap_r);
+    }
+}   
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 71c1e17..0194823 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1097,6 +1097,8 @@ typedef struct {
 
     char *saved_state;
 
+    unsigned emuidmap;
+
     libxl__file_reference pv_kernel;
     libxl__file_reference pv_ramdisk;
     const char * pv_cmdline;
@@ -1962,6 +1964,18 @@ enum {
     /* NB stubdom and its PV service domain not recorded here */
 };
 
+_hidden int libxl__dm_emuidmap_add(libxl__gc*, int domid,
+                                   libxl__domain_build_state*, int emuid);
+
+_hidden int libxl__dm_emuidmap_get(libxl__gc*, int domid,
+                                   unsigned *emuidmap_r);
+
+_hidden void libxl__dm_emuidmap_get_bodgeerrors(libxl__gc*, int domid,
+                                             unsigned *emuidmap_r);
+    /* *emuidmap_r is set to a default (but unreliable) value even
+     * if we get an error, which is logged.  Try to avoid calling
+     * this if you can! */
+
 _hidden char *libxl__device_model_xs_path(libxl__gc *gc,
         uint32_t dm_domid,
         uint32_t domid, int emuid,
-- 
1.7.10.4

[PATCH 14/28] libxl: emuids: Pass emuid to dm destruction

[Ian Jackson]

Rather than recomputing this with a switch() statement, etc., we rely
on the emuidmap to tell us which qemus to destroy.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.  This is a new approach in v6.

squash! libxl: Pass emuid to dm destruction
---
 tools/libxl/libxl.c          |   31 ++++++++++++-------------------
 tools/libxl/libxl_dm.c       |   13 +++++++++----
 tools/libxl/libxl_internal.h |    3 ++-
 3 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2a0c092..dff4478 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1606,7 +1606,8 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
     libxl_ctx *ctx = CTX;
     uint32_t domid = dis->domid;
     char *dom_path;
-    int rc, dm_present;
+    int rc;
+    unsigned emuidmap;
 
     libxl__ev_child_init(&dis->destroyer);
 
@@ -1633,27 +1634,19 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         goto out;
     }
 
-    switch (libxl__domain_type(gc, domid)) {
-    case LIBXL_DOMAIN_TYPE_HVM:
-        if (libxl_get_stubdom_id(CTX, domid)) {
-            dm_present = 0;
-            break;
-        }
-        /* fall through */
-    case LIBXL_DOMAIN_TYPE_PV:
-        dm_present = 1;
-        break;
-    case LIBXL_DOMAIN_TYPE_INVALID:
-        rc = ERROR_FAIL;
-        goto out;
-    default:
-        abort();
-    }
+    libxl__dm_emuidmap_get_bodgeerrors(gc, domid, &emuidmap);
 
-    if (dm_present) {
-        libxl__destroy_device_model(gc, domid);
+    libxl__destroy_device_model(gc, domid, emuidmap, EMUID_PV);
 
+    if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM &&
+        libxl_get_stubdom_id(CTX, domid)) {
+        /* This is destroyed in libxl__domain_destroy */
+    } else {
+        libxl__destroy_device_model(gc, domid, emuidmap, EMUID_DM);
     }
+
+    libxl__destroy_device_model(gc, domid, emuidmap, EMUID_PV);
+
     dis->drs.ao = ao;
     dis->drs.domid = domid;
     dis->drs.callback = devices_destroy_cb;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 2bd1eb0..bcae664 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2111,20 +2111,25 @@ out:
     return rc;
 }
 
-int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
+int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid,
+                                unsigned emuidmap, int emuid)
 {
     int rc;
 
+    if (!(emuidmap & (1u << emuid)))
+        return 0;
+
     char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                             domid, EMUID_DM, "");
+                                             domid, emuid, "");
     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. */
     rc = kill_device_model(gc,
                 GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
     if (rc)
-        LOG(ERROR, "libxl__destroy_device_model failed for %d",
-            domid);
+        LOG(ERROR, "libxl__destroy_device_model failed for %d (emuid=%d)",
+            domid, emuid);
 
     libxl__qmp_cleanup(gc, domid);
     return rc;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0194823..4a9003d 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1656,7 +1656,8 @@ _hidden int libxl__wait_for_device_model_deprecated(libxl__gc *gc,
                                                       void *userdata),
                                 void *check_callback_userdata);
 
-_hidden int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid);
+_hidden int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid,
+                                        unsigned emuidmap, int emuid);
 
 _hidden const libxl_vnc_info *libxl__dm_vnc(const libxl_domain_config *g_cfg);
 
-- 
1.7.10.4

[PATCH 15/28] libxl: emuids: Pass emuid to device model argument construction

[Ian Jackson]

No functional change yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_dm.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index bcae664..5b56047 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -752,6 +752,7 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
 
 static int libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
+                                        int emuid,
                                         const libxl_domain_config *guest_config,
                                         char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
@@ -1296,6 +1297,7 @@ end_search:
 
 static int libxl__build_device_model_args(libxl__gc *gc,
                                         const char *dm, int guest_domid,
+                                        int emuid,
                                         const libxl_domain_config *guest_config,
                                         char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
@@ -1313,7 +1315,8 @@ static int libxl__build_device_model_args(libxl__gc *gc,
         assert(dm_state_fd != NULL);
         assert(*dm_state_fd < 0);
         return libxl__build_device_model_args_new(gc, dm,
-                                                  guest_domid, guest_config,
+                                                  guest_domid, emuid,
+                                                  guest_config,
                                                   args, envs,
                                                   state, dm_state_fd);
     default:
@@ -1531,6 +1534,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss,
         goto out;
 
     ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
+                                         emuid,
                                          guest_config, &args, NULL,
                                          d_state, NULL);
     if (ret) {
@@ -1817,7 +1821,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss,
         rc = ERROR_FAIL;
         goto out;
     }
-    rc = libxl__build_device_model_args(gc, dm, domid, guest_config,
+    rc = libxl__build_device_model_args(gc, dm, domid, emuid, guest_config,
                                           &args, &envs, state,
                                           &dm_state_fd);
     if (rc)
-- 
1.7.10.4

[PATCH 16/28] libxl: emuids: Provide libxl__dm_xs_path_rel

[Ian Jackson]

This gives a relative path, suitable for driver domains.  It will be
used shortly.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch
---
 tools/libxl/libxl_internal.c |   10 ++++++++--
 tools/libxl/libxl_internal.h |    4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 843fdbf..d0c98b0 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -553,6 +553,12 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
+char *libxl__dm_xs_path_rel(libxl__gc *gc,
+                            uint32_t domid, int emuid)
+{
+    return GCSPRINTF("device-model/%u", domid);
+}
+
 char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
                                   uint32_t domid, int emuid,
                                   const char *format,  ...)
@@ -560,8 +566,8 @@ char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
     char *s, *fmt;
     va_list ap;
 
-    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
-                    domid, format);
+    fmt = GCSPRINTF("/local/domain/%u/%s%s", dm_domid,
+                    libxl__dm_xs_path_rel(gc, domid, emuid), format);
 
     va_start(ap, format);
     s = libxl__vsprintf(gc, fmt, ap);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 4a9003d..2248509 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1982,6 +1982,10 @@ _hidden char *libxl__device_model_xs_path(libxl__gc *gc,
         uint32_t domid, int emuid,
         const char *format, ...) PRINTF_ATTRIBUTE(5, 6);
 
+_hidden char *libxl__dm_xs_path_rel(libxl__gc *gc,
+                                    uint32_t domid, int emuid);
+    /* returns relative path (ie assuming we are the toolstack) */
+
 /*
  * Calling context and GC for event-generating functions:
  *
-- 
1.7.10.4

[PATCH 17/28] libxl: emuids: Do not open-code device-model/%u in libxl__destroy_qdisk_backend

[Ian Jackson]

This is used in a driver domain context and doesn't get the caller's
domid.  But that's OK, we can now use libxl__dm_xs_path_rel which
returns the same value as previously.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/libxl/libxl_dm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5b56047..3bf1317 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2109,7 +2109,7 @@ int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
 
     libxl__xs_rm_checked(gc, XBT_NULL, pid_path);
     libxl__xs_rm_checked(gc, XBT_NULL,
-                         GCSPRINTF("device-model/%u", domid));
+                         libxl__dm_xs_path_rel(gc, domid, EMUID_PV));
 
 out:
     return rc;
-- 
1.7.10.4

[PATCH 18/28] libxl: emuids: Change pid path in xenstore

[Ian Jackson]

Introduce libxl__dm_xs_pid_path and use it everywhere.

There is no overall functional change, although there is a change to
these internal paths.  Also, the paths used are now relative (ie to
/local/domain/<domid-containing-device-model>) rather than absolute.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: Rewritten, actual paths changed, patch title changed
v4: Update xenstore-paths.markdown
---
 docs/misc/xenstore-paths.markdown |   10 +++-------
 tools/libxl/libxl_dm.c            |   11 +++++------
 tools/libxl/libxl_internal.c      |    6 ++++++
 tools/libxl/libxl_internal.h      |    3 +++
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 208b64b..3bd31af 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -143,11 +143,6 @@ The guests name.
 
 The domain's own ID.
 
-#### ~/image/device-model-pid = INTEGER   [INTERNAL]
-
-The process ID of the device model associated with this domain, if it
-has one.
-
 #### ~/cpu/[0-9]+/availability = ("online"|"offline") [PV]
 
 One node for each virtual CPU up to the guest's configured
@@ -463,9 +458,10 @@ in the value supplied by the guest in this case).
 
 Contains the status of the device models running on the domain.
 
-#### ~/libxl/$DOMID/qdisk-backend-pid [w]
+#### ~/libxl/$DOMID/dm-pid-$EMUID [w] = INTEGER   [INTERNAL]
 
-Contains the PIDs of the device models running on the domain.
+Contains the PIDs of the device models running on this domain, to
+service $DOMID.
 
 ## Virtual Machine Paths
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3bf1317..6b0bbed 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1867,7 +1867,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss,
     }
 
     const char *dom_path = libxl__xs_get_dompath(gc, domid);
-    spawn->pidpath = GCSPRINTF("%s/%s", dom_path, "image/device-model-pid");
+    spawn->pidpath = libxl__dm_xs_pid_path(gc, domid, emuid);
 
     if (vnc && vnc->passwd) {
         /* This xenstore key will only be used by qemu-xen-traditionnal.
@@ -1901,7 +1901,7 @@ retry_transaction:
     spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
                                                 domid, emuid, "/state");
     spawn->timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
-    spawn->pidpath = GCSPRINTF("%s/image/device-model-pid", dom_path);
+    spawn->pidpath = libxl__dm_xs_pid_path(gc, domid, emuid);
     spawn->midproc_cb = libxl__spawn_record_pid;
     spawn->confirm_cb = device_model_confirm;
     spawn->failure_cb = device_model_startup_failed;
@@ -2036,7 +2036,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
      * because we will call this from unprivileged driver domains,
      * so save it in the current domain libxl private dir.
      */
-    dmss->spawn.pidpath = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    dmss->spawn.pidpath = libxl__dm_xs_pid_path(gc, domid, EMUID_PV);
     dmss->spawn.midproc_cb = libxl__spawn_record_pid;
     dmss->spawn.confirm_cb = device_model_confirm;
     dmss->spawn.failure_cb = device_model_startup_failed;
@@ -2101,7 +2101,7 @@ int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
     char *pid_path;
     int rc;
 
-    pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    pid_path = libxl__dm_xs_pid_path(gc, domid, EMUID_PV);
 
     rc = kill_device_model(gc, pid_path);
     if (rc)
@@ -2129,8 +2129,7 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid,
         LOG(ERROR, "xs_rm failed for %s", path);
 
     /* We should try to destroy the device model anyway. */
-    rc = kill_device_model(gc,
-                GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+    rc = kill_device_model(gc, libxl__dm_xs_pid_path(gc, domid, emuid));
     if (rc)
         LOG(ERROR, "libxl__destroy_device_model failed for %d (emuid=%d)",
             domid, emuid);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index d0c98b0..30271c9 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -576,6 +576,12 @@ char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
     return s;
 }
 
+char *libxl__dm_xs_pid_path(libxl__gc *gc, int guest_domid, int emuid)
+{
+    return GCSPRINTF("libxl/%d/dm-pid-%d", guest_domid, emuid);
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2248509..350ec29 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1986,6 +1986,9 @@ _hidden char *libxl__dm_xs_path_rel(libxl__gc *gc,
                                     uint32_t domid, int emuid);
     /* returns relative path (ie assuming we are the toolstack) */
 
+_hidden char *libxl__dm_xs_pid_path(libxl__gc *gc,
+                                    int guest_domid, int emuid);
+
 /*
  * Calling context and GC for event-generating functions:
  *
-- 
1.7.10.4

[PATCH 19/28] libxl: Improve libxl__destroy_device_model

[Ian Jackson]

Take some improvements from libxl__destroy_qdisk_backend:
* Use libxl__xs_rm_checked rather than xs_rm
* Remove the pid from xenstore

Then libxl__destroy_qdisk_backend can be made into a simple wrapper
around libxl__destroy_device_model.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_dm.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 6b0bbed..886ed9c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2098,21 +2098,12 @@ out:
 /* Helper to destroy a Qdisk backend */
 int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
 {
-    char *pid_path;
-    int rc;
-
-    pid_path = libxl__dm_xs_pid_path(gc, domid, EMUID_PV);
+    unsigned emuidmap;
 
-    rc = kill_device_model(gc, pid_path);
-    if (rc)
-        goto out;
+    libxl__dm_emuidmap_get(gc, domid, &emuidmap);
+    /* better to blunder on if this fails */
 
-    libxl__xs_rm_checked(gc, XBT_NULL, pid_path);
-    libxl__xs_rm_checked(gc, XBT_NULL,
-                         libxl__dm_xs_path_rel(gc, domid, EMUID_PV));
-
-out:
-    return rc;
+    return libxl__destroy_device_model(gc, domid, emuidmap, EMUID_PV);
 }
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid,
@@ -2123,16 +2114,16 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid,
     if (!(emuidmap & (1u << emuid)))
         return 0;
 
-    char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                             domid, emuid, "");
-    if (!xs_rm(CTX->xsh, XBT_NULL, path))
-        LOG(ERROR, "xs_rm failed for %s", path);
+    const char *control_path = libxl__dm_xs_path_rel(gc, domid, emuid);
+    libxl__xs_rm_checked(gc, XBT_NULL, control_path);
 
     /* We should try to destroy the device model anyway. */
-    rc = kill_device_model(gc, libxl__dm_xs_pid_path(gc, domid, emuid));
+    const char *pid_path = libxl__dm_xs_pid_path(gc, domid, emuid);
+    rc = kill_device_model(gc, pid_path);
     if (rc)
         LOG(ERROR, "libxl__destroy_device_model failed for %d (emuid=%d)",
             domid, emuid);
+    libxl__xs_rm_checked(gc, XBT_NULL, pid_path);
 
     libxl__qmp_cleanup(gc, domid);
     return rc;
-- 
1.7.10.4

[PATCH 20/28] libxl: domcreate_dm_support_checked: Introduce `goto out'

[Ian Jackson]

We are going to want this shortly.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_create.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 71893c5..b41c6bd 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -968,10 +968,7 @@ static void domcreate_dm_support_checked(libxl__egc *egc,
     libxl_domain_config *const d_config = dcs->guest_config;
     const int restore_fd = dcs->restore_fd;
 
-    if (rc) {
-        domcreate_complete(egc, dcs, rc);
-        return;
-    }
+    if (rc) goto out;
 
     dcs->bl.ao = ao;
     libxl_device_disk *bootdisk =
@@ -994,6 +991,10 @@ static void domcreate_dm_support_checked(libxl__egc *egc,
         libxl__bootloader_run(egc, &dcs->bl);
     }
     return;
+
+ out:
+    domcreate_complete(egc, dcs, rc);
+    return;
 }
 
 static void domcreate_bootloader_console_available(libxl__egc *egc,
-- 
1.7.10.4

[PATCH 21/28] libxl: dm user: Reject attempts to set user!=root with qemu trad

[Ian Jackson]

Previously this option would be silently ignored, which is a potential
security problem (introduced in 84f2fd1b "run QEMU as non-root" in
xen-unstable only).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl_dm.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 886ed9c..8232981 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -415,6 +415,14 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
     dm_args = flexarray_make(gc, 16, 1);
     dm_envs = flexarray_make(gc, 16, 1);
 
+    if (b_info->device_model_user && /* default is NULL if stubdom */
+        strcmp(b_info->device_model_user,"root")) {
+        LOG(ERROR,
+ "device_model_user != root (%s) not supported by qemu-xen-traditional",
+            b_info->device_model_user);
+        return ERROR_INVAL;
+    }
+
     flexarray_vappend(dm_args, dm,
                       "-d", GCSPRINTF("%d", domid), NULL);
 
-- 
1.7.10.4

Re: [PATCH 21/28] libxl: dm user: Reject attempts to set user!=root with qemu trad

[Ian Campbell]

On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> Previously this option would be silently ignored, which is a potential
> security problem (introduced in 84f2fd1b "run QEMU as non-root" in
> xen-unstable only).
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>
(could/should go in now despite RFC-ness of the series as a whole, assuming
it is as independent as it looks, we really don't want to forget this for
4.7 if the other 27 patches take longer to land)


> ---
> v6: New patch.
> ---
>  tools/libxl/libxl_dm.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 886ed9c..8232981 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -415,6 +415,14 @@ static int
> libxl__build_device_model_args_old(libxl__gc *gc,
>      dm_args = flexarray_make(gc, 16, 1);
>      dm_envs = flexarray_make(gc, 16, 1);
>  
> +    if (b_info->device_model_user && /* default is NULL if stubdom */
> +        strcmp(b_info->device_model_user,"root")) {
> +        LOG(ERROR,
> + "device_model_user != root (%s) not supported by qemu-xen-traditional",
> +            b_info->device_model_user);
> +        return ERROR_INVAL;
> +    }
> +
>      flexarray_vappend(dm_args, dm,
>                        "-d", GCSPRINTF("%d", domid), NULL);
>  

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

Re: [PATCH 21/28] libxl: dm user: Reject attempts to set user!=root with qemu trad

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 21/28] libxl: dm user: Reject attempts to set user!=root with qemu trad"):
> On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> > Previously this option would be silently ignored, which is a potential
> > security problem (introduced in 84f2fd1b "run QEMU as non-root" in
> > xen-unstable only).
> > 
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> (could/should go in now despite RFC-ness of the series as a whole, assuming
> it is as independent as it looks, we really don't want to forget this for
> 4.7 if the other 27 patches take longer to land)

Well, it moved easily to earlier in the series, so I have done that.
But there are other things wrong with 84f2fd1b which are sorted out
here, too.  I think if this series doesn't make 4.7 we may need to
revert 84f2fd1b, or at least consider other parts of this series to
cherry pick.

Thanks,
Ian.

Re: [PATCH 21/28] libxl: dm user: Reject attempts to set user!=root with qemu trad

[Ian Campbell]

On Fri, 2016-01-08 at 12:16 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 21/28] libxl: dm user: Reject attempts
> to set user!=root with qemu trad"):
> > On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> > > Previously this option would be silently ignored, which is a
> > > potential
> > > security problem (introduced in 84f2fd1b "run QEMU as non-root" in
> > > xen-unstable only).
> > > 
> > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > > CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > (could/should go in now despite RFC-ness of the series as a whole,
> > assuming
> > it is as independent as it looks, we really don't want to forget this
> > for
> > 4.7 if the other 27 patches take longer to land)
> 
> Well, it moved easily to earlier in the series, so I have done that.
> But there are other things wrong with 84f2fd1b which are sorted out
> here, too.  I think if this series doesn't make 4.7 we may need to
> revert 84f2fd1b, or at least consider other parts of this series to
> cherry pick.

Makes sense, I assume you'll either propose that revert of a suitable list
of cherry-picks if/when the time comes.

Ian.

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

[PATCH 22/28] libxl: dm user: Document the default

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v6: New patch
---
 docs/man/xl.cfg.pod.5 |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 8899f75..dd93725 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1839,7 +1839,8 @@ option to the device-model.
 =item B<device_model_user="username">
 
 Run the device model as user "username", instead of
-xen-qemudepriv-domid$domid or xen-qemudepriv-shared or root.
+xen-qemudepriv-domid$domid or xen-qemudepriv-shared or root.  The
+default is to use the first of these users which exists.
 
 =back
 
-- 
1.7.10.4

Re: [PATCH 22/28] libxl: dm user: Document the default

[Ian Campbell]

On Tue, 2015-12-22 at 18:44 +0000, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>
(could go in now despite RFC-ness of the series as a whole)

[PATCH 23/28] libxl: dm user: Move user choice earlier, and fill in config

[Ian Jackson]

Move the dm user defaulting from libxl__build_device_model_args_new
earlier in domain create.  We now do it just after the dm support
check has finished.

Convey the result to the dm args constructor in the b_info, ie in the
supplied config.  So, the application's supplied config is filled in
with the chosen user, as for other defaults.

Also:
 - slightly improve one of the warning messages
 - change the moved code to use `rc' (following CODING_STYLE)
   rather than `ret'
 - change the moved code to use `domid' rather than `guest_domid'
   like most of the rest of domain create
 - change the name of the (static) user selection function.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v6: New patch
---
 tools/libxl/libxl_create.c |   84 ++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dm.c     |   62 ++------------------------------
 2 files changed, 86 insertions(+), 60 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b41c6bd..be9bb7b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -20,6 +20,8 @@
 #include "libxl_internal.h"
 #include "libxl_arch.h"
 
+#include <pwd.h>
+
 #include <xc_dom.h>
 #include <xenguest.h>
 #include <xen/hvm/hvm_info_table.h>
@@ -388,6 +390,83 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     return 0;
 }
 
+/* return 1 if the user was found, 0 if it was not, -1 on error */
+static int dm_runas_helper(libxl__gc *gc, const char *username)
+{
+    struct passwd pwd, *user = NULL;
+    char *buf = NULL;
+    long buf_size;
+    int ret;
+
+    buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
+    if (buf_size < 0) {
+        LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
+                buf_size);
+        return ERROR_FAIL;
+    }
+
+    while (1) {
+        buf = libxl__realloc(gc, buf, buf_size);
+        ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
+        if (ret == ERANGE) {
+            buf_size += 128;
+            continue;
+        }
+        if (ret != 0)
+            return ERROR_FAIL;
+        if (user != NULL)
+            return 1;
+        return 0;
+    }
+}
+
+static int domcreate_setdefault_dm_user(libxl__gc *gc,
+                                        libxl__domain_create_state *dcs)
+{
+    /* convenience aliases */
+    libxl_domain_config *const d_config = dcs->guest_config;
+    libxl_domain_build_info *const b_info = &d_config->b_info;
+    const uint32_t domid = dcs->guest_domid;
+
+    int rc;
+    const char *user;
+
+    if (b_info->device_model_user)
+        /* already set, good-oh */
+        return 0;
+
+    if (!(b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
+          !libxl_defbool_val(b_info->device_model_stubdomain)))
+        /* we're not going to run it anyway */
+        return 0;
+
+    user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, domid);
+
+    rc = dm_runas_helper(gc, user);
+    if (rc < 0) goto error;
+    if (rc > 0) goto found;
+
+    user = LIBXL_QEMU_USER_SHARED;
+    rc = dm_runas_helper(gc, user);
+    if (rc < 0) goto error;
+    if (rc > 0) {
+        LOG(WARN, "Could not find user %s%d, falling back to %s",
+            LIBXL_QEMU_USER_BASE, domid, LIBXL_QEMU_USER_SHARED);
+        goto found;
+    }
+
+    LOG(WARN, "Could not find user %s%d or %s, starting QEMU as root",
+        LIBXL_QEMU_USER_BASE, domid, LIBXL_QEMU_USER_SHARED);
+    user = "root";
+
+ found:
+    b_info->device_model_user = libxl__strdup(NOGC, user);
+    return 0;
+
+ error:
+    return rc;
+}
+
 static void init_console_info(libxl__gc *gc,
                              libxl__device_console *console,
                              int dev_num)
@@ -970,6 +1049,11 @@ static void domcreate_dm_support_checked(libxl__egc *egc,
 
     if (rc) goto out;
 
+    /* config defaults which depend on dm support etc. */
+
+    rc = domcreate_setdefault_dm_user(gc, dcs);
+    if (rc) goto out;
+
     dcs->bl.ao = ao;
     libxl_device_disk *bootdisk =
         d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8232981..5e1a6cf 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -21,8 +21,6 @@
 
 #include <xc_dom.h>
 #include <xen/hvm/e820.h>
-#include <sys/types.h>
-#include <pwd.h>
 
 static const char *libxl_tapif_script(libxl__gc *gc)
 {
@@ -728,36 +726,6 @@ libxl__detect_gfx_passthru_kind(libxl__gc *gc,
     return LIBXL_GFX_PASSTHRU_KIND_DEFAULT;
 }
 
-/* return 1 if the user was found, 0 if it was not, -1 on error */
-static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
-{
-    struct passwd pwd, *user = NULL;
-    char *buf = NULL;
-    long buf_size;
-    int ret;
-
-    buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
-    if (buf_size < 0) {
-        LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
-                buf_size);
-        return ERROR_FAIL;
-    }
-
-    while (1) {
-        buf = libxl__realloc(gc, buf, buf_size);
-        ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
-        if (ret == ERANGE) {
-            buf_size += 128;
-            continue;
-        }
-        if (ret != 0)
-            return ERROR_FAIL;
-        if (user != NULL)
-            return 1;
-        return 0;
-    }
-}
-
 static int libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         int emuid,
@@ -777,10 +745,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     const char *keymap = dm_keymap(guest_config);
     char *machinearg;
     flexarray_t *dm_args, *dm_envs;
-    int i, connection, devid, ret;
+    int i, connection, devid;
     uint64_t ram_size;
     const char *path, *chardev;
-    char *user = NULL;
 
     dm_args = flexarray_make(gc, 16, 1);
     dm_envs = flexarray_make(gc, 16, 1);
@@ -1263,33 +1230,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             break;
         }
 
-        if (b_info->device_model_user) {
-            user = b_info->device_model_user;
-            goto end_search;
-        }
-
-        user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
-        ret = libxl__dm_runas_helper(gc, user);
-        if (ret < 0)
-            return ret;
-        if (ret > 0)
-            goto end_search;
-
-        user = LIBXL_QEMU_USER_SHARED;
-        ret = libxl__dm_runas_helper(gc, user);
-        if (ret < 0)
-            return ret;
-        if (ret > 0) {
-            LOG(WARN, "Could not find user %s%d, falling back to %s",
-                    LIBXL_QEMU_USER_BASE, guest_domid, LIBXL_QEMU_USER_SHARED);
-            goto end_search;
-        }
-
-        user = NULL;
-        LOG(WARN, "Could not find user %s, starting QEMU as root",
-            LIBXL_QEMU_USER_SHARED);
+        char *const user = b_info->device_model_user;
 
-end_search:
         if (user != NULL && strcmp(user, "root")) {
             flexarray_append(dm_args, "-runas");
             flexarray_append(dm_args, user);
-- 
1.7.10.4

[PATCH 24/28] libxl: dm spawn records rc in state struct rather than passing as argument

[Ian Jackson]

This is going to be much more convenient in a moment, when one of the
call sites is going to want to run two in parallel.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6: New patch.
---
 tools/libxl/libxl.c          |    4 ++--
 tools/libxl/libxl_create.c   |   14 +++++++-------
 tools/libxl/libxl_dm.c       |   22 +++++++++++++---------
 tools/libxl/libxl_internal.h |    5 +++--
 4 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index dff4478..3bb6d3b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4345,10 +4345,10 @@ static void device_complete(libxl__egc *egc, libxl__ao_device *aodev)
     libxl__nested_ao_free(aodev->ao);
 }
 
-static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss,
-                                int rc)
+static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss)
 {
     STATE_AO_GC(dmss->spawn.ao);
+    int rc = dmss->rc;
 
     LOG(DEBUG, "qdisk backend spawn for domain %u %s", dmss->guest_domid,
                rc ? "failed" : "succeed");
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index be9bb7b..e67e402 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -803,8 +803,7 @@ static void remus_checkpoint_stream_done(
 static void domcreate_dm_support_checked(libxl__egc *egc,
     libxl__dm_support_check_state *checking, int rc);
 static void domcreate_devmodel_started(libxl__egc *egc,
-                                       libxl__dm_spawn_state *dmss,
-                                       int rc);
+                                       libxl__dm_spawn_state *dmss);
 static void domcreate_bootloader_console_available(libxl__egc *egc,
                                                    libxl__bootloader_state *bl);
 static void domcreate_bootloader_done(libxl__egc *egc,
@@ -1387,8 +1386,8 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
 
         if (d_config->b_info.device_model_version ==
             LIBXL_DEVICE_MODEL_VERSION_NONE) {
-            domcreate_devmodel_started(egc, &dcs->dmss.dm, 0);
-            return;
+            dcs->dmss.dm.rc = 0;
+            domcreate_devmodel_started(egc, &dcs->dmss.dm);
         }
 
         libxl_device_vkb_init(&vkb);
@@ -1440,7 +1439,8 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
             return;
         } else {
             assert(!dcs->dmss.dm.guest_domid);
-            domcreate_devmodel_started(egc, &dcs->dmss.dm, 0);
+            dcs->dmss.dm.rc = 0;
+            domcreate_devmodel_started(egc, &dcs->dmss.dm);
             return;
         }
     }
@@ -1456,11 +1456,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
 }
 
 static void domcreate_devmodel_started(libxl__egc *egc,
-                                       libxl__dm_spawn_state *dmss,
-                                       int ret)
+                                       libxl__dm_spawn_state *dmss)
 {
     libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.dm);
     STATE_AO_GC(dmss->spawn.ao);
+    int ret = dmss->rc;
     int domid = dcs->guest_domid;
 
     /* convenience aliases */
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 5e1a6cf..58760e7 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1369,8 +1369,7 @@ retry_transaction:
 }
 
 static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
-                                libxl__dm_spawn_state *stubdom_dmss,
-                                int rc);
+                                libxl__dm_spawn_state *stubdom_dmss);
 
 static void spawn_stub_launch_dm(libxl__egc *egc,
                                  libxl__multidev *aodevs, int ret);
@@ -1534,7 +1533,8 @@ retry_transaction:
 
 out:
     assert(ret);
-    spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
+    sdss->pvqemu.rc = ret;
+    spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu);
 }
 
 static void spawn_stub_launch_dm(libxl__egc *egc,
@@ -1632,16 +1632,17 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
 
 out:
     assert(ret);
-    spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
+    sdss->pvqemu.rc = ret;
+    spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu);
 }
 
 static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
-                                libxl__dm_spawn_state *stubdom_dmss,
-                                int rc)
+                                libxl__dm_spawn_state *stubdom_dmss)
 {
     libxl__stub_dm_spawn_state *sdss =
         CONTAINER_OF(stubdom_dmss, *sdss, pvqemu);
     STATE_AO_GC(sdss->dm.spawn.ao);
+    int rc = stubdom_dmss->rc;
     uint32_t dm_domid = sdss->pvqemu.guest_domid;
     libxl_domain_config *d_config = stubdom_dmss->guest_config;
 
@@ -1712,7 +1713,8 @@ static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
         return;
  out:
     libxl__xswait_stop(gc, xswait);
-    sdss->callback(egc, &sdss->dm, rc);
+    sdss->dm.rc = rc;
+    sdss->callback(egc, &sdss->dm);
 }
 
 /* callbacks passed to libxl__spawn_spawn */
@@ -1929,7 +1931,8 @@ static void device_model_spawn_outcome(libxl__egc *egc,
     }
 
  out:
-    dmss->callback(egc, dmss, rc);
+    dmss->rc = rc;
+    dmss->callback(egc, dmss);
 }
 
 void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
@@ -2005,7 +2008,8 @@ error:
     assert(rc);
     if (logfile_w >= 0) close(logfile_w);
     if (null >= 0) close(null);
-    dmss->callback(egc, dmss, rc);
+    dmss->rc = rc;
+    dmss->callback(egc, dmss);
     return;
 }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 350ec29..82f264a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3413,12 +3413,13 @@ _hidden void libxl__add_vtpms(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
 
 typedef struct libxl__dm_spawn_state libxl__dm_spawn_state;
 
-typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*,
-                                int rc /* if !0, error was logged */);
+typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*);
 
 struct libxl__dm_spawn_state {
     /* mixed - spawn.ao must be initialised by user; rest is private: */
     libxl__spawn_state spawn;
+    /* filled in by dm spawn just before making callback */
+    int rc; /* if !0, error was logged */
     /* filled in by user, must remain valid: */
     uint32_t guest_domid; /* domain being served */
     libxl_domain_config *guest_config;
-- 
1.7.10.4

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

[Ian Jackson]

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.

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

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

[Ian Campbell]

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

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

[Ian Jackson]

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.

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

[Ian Campbell]

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.

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

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

[Ian Jackson]

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.

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

[Ian Campbell]

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.


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

[PATCH 26/28] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

We actually need to spawn a second QEMU if we want to run qemu as
non-root, because the pv backends ought to run as root (or device
hotplug may not work).

So in this case, start a second QEMU to provide PV backends in
userspace to HVM guests.  Use both dcs->dmss.pvqemu and dcs->dmss.dm
to keep track of the starting QEMUs.

Only proceed when both QEMUs have started.

And, we only default to running QEMU as non-root if we are going to be
able to run split qemus.  In particular, it is not safe to run split
qemus if they don't support the emulator_id option, because we need to
split the xenstore paths too.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v6: Split only if trying to run a qemu as non-root
    Reorganise changes to dm callbacks
    No more dcs in dmss
    Use min() to calculate worst rc
    Explicitly set unused fields of dmss.pvqemu to 0
    Change error handling

v3: use dcs->dmss.pvqemu to spawn the second QEMU
    keep track of the rc of both QEMUs before proceeding
---
 tools/libxl/libxl_create.c |   72 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e67e402..59dfcd67 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -431,6 +431,8 @@ static int domcreate_setdefault_dm_user(libxl__gc *gc,
     int rc;
     const char *user;
 
+    const char *dm = libxl__domain_device_model(gc, b_info);
+
     if (b_info->device_model_user)
         /* already set, good-oh */
         return 0;
@@ -440,6 +442,15 @@ static int domcreate_setdefault_dm_user(libxl__gc *gc,
         /* we're not going to run it anyway */
         return 0;
 
+    if (!libxl__dm_supported(gc, dm, libxl__dm_support_check__emulator_id)) {
+        /* we don't want to run the pv backends as non-root because
+         * device hotplug will no longer work. */
+        LOG(WARN,
+ "Device model does not support split PV backends, running it as root");
+        user = "root";
+        goto found;
+    }
+
     user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, domid);
 
     rc = dm_runas_helper(gc, user);
@@ -802,6 +813,14 @@ static void remus_checkpoint_stream_done(
 /* Event callbacks, in this order: */
 static void domcreate_dm_support_checked(libxl__egc *egc,
     libxl__dm_support_check_state *checking, int rc);
+
+static void domcreate_dm_local_split_pv_cb(libxl__egc *egc,
+                                           libxl__dm_spawn_state *dmss);
+static void domcreate_dm_local_split_dm_cb(libxl__egc *egc,
+                                           libxl__dm_spawn_state *dmss);
+static void domcreate_dm_local_split_cb(libxl__egc *egc,
+                                        libxl__domain_create_state *dcs);
+
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss);
 static void domcreate_bootloader_console_available(libxl__egc *egc,
@@ -1044,6 +1063,9 @@ static void domcreate_dm_support_checked(libxl__egc *egc,
     
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
+    const libxl_domain_build_info *b_info = &d_config->b_info;
+    libxl__domain_build_state *const b_state = &dcs->build_state;
+    const uint32_t domid = dcs->guest_domid;
     const int restore_fd = dcs->restore_fd;
 
     if (rc) goto out;
@@ -1053,6 +1075,15 @@ static void domcreate_dm_support_checked(libxl__egc *egc,
     rc = domcreate_setdefault_dm_user(gc, dcs);
     if (rc) goto out;
 
+    /* run two qemus? */
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
+        !libxl_defbool_val(d_config->b_info.device_model_stubdomain) &&
+        b_info->device_model_user &&
+        strcmp(b_info->device_model_user, "root")) {
+        rc = libxl__dm_emuidmap_add(gc, domid, b_state, EMUID_SPLIT);
+        if (rc) goto out;
+    }
+
     dcs->bl.ao = ao;
     libxl_device_disk *bootdisk =
         d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
@@ -1130,6 +1161,11 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->dmss.dm.callback = domcreate_devmodel_started;
     dcs->dmss.callback = domcreate_devmodel_started;
 
+    dcs->dmss.pvqemu.spawn.ao = ao;
+    dcs->dmss.pvqemu.guest_domid = domid;
+    dcs->dmss.pvqemu.guest_config = 0;
+    dcs->dmss.pvqemu.build_state = 0;
+
     if (restore_fd < 0 && dcs->domid_soft_reset == INVALID_DOMID) {
         rc = libxl__domain_build(gc, d_config, domid, state);
         domcreate_rebuild_done(egc, dcs, rc);
@@ -1398,6 +1434,16 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         if (libxl_defbool_val(d_config->b_info.device_model_stubdomain)) {
             libxl__spawn_stub_dm(egc, &dcs->dmss, EMUID_DM);
         } else {
+            if (state->emuidmap & (1u << EMUID_SPLIT)) {
+                dcs->dmss.dm.rc = 1;
+                dcs->dmss.dm.callback = domcreate_dm_local_split_dm_cb;
+
+                dcs->dmss.pvqemu.rc = 1; /* +ve means in progress */
+                dcs->dmss.pvqemu.callback = domcreate_dm_local_split_pv_cb;
+
+                libxl__spawn_qdisk_backend(egc, &dcs->dmss.pvqemu);
+            }
+
             libxl__spawn_local_dm(egc, &dcs->dmss.dm, EMUID_DM);
         }
 
@@ -1455,6 +1501,32 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     domcreate_complete(egc, dcs, ret);
 }
 
+static void domcreate_dm_local_split_pv_cb(libxl__egc *egc,
+                                           libxl__dm_spawn_state *dmss)
+{
+    libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.pvqemu);
+    domcreate_dm_local_split_cb(egc, dcs);
+}
+
+static void domcreate_dm_local_split_dm_cb(libxl__egc *egc,
+                                           libxl__dm_spawn_state *dmss)
+{
+    libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.dm);
+    domcreate_dm_local_split_cb(egc, dcs);
+}
+
+static void domcreate_dm_local_split_cb(libxl__egc *egc,
+                                        libxl__domain_create_state *dcs)
+{
+    if (dcs->dmss.dm.rc > 0 ||
+        dcs->dmss.pvqemu.rc > 0)
+        /* something is still in progress */
+        return;
+
+    dcs->dmss.dm.rc = min(dcs->dmss.dm.rc, dcs->dmss.pvqemu.rc);
+    domcreate_devmodel_started(egc, &dcs->dmss.dm);
+}
+
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss)
 {
-- 
1.7.10.4

Re: [PATCH 26/28] libxl: spawns two QEMUs for HVM guests

[Ian Campbell]

On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote:
> @@ -440,6 +442,15 @@ static int domcreate_setdefault_dm_user(libxl__gc
> *gc,
>          /* we're not going to run it anyway */
>          return 0;
>  
> +    if (!libxl__dm_supported(gc, dm,
> libxl__dm_support_check__emulator_id)) {
> +        /* we don't want to run the pv backends as non-root because
> +         * device hotplug will no longer work. */
> +        LOG(WARN,
> + "Device model does not support split PV backends, running it as root");
> +        user = "root";
> +        goto found;
> +    }

Should this be a fatal error, unless b_info->device_model_user == "root"?

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

Re: [PATCH 26/28] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 26/28] libxl: spawns two QEMUs for HVM guests"):
> On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote:
> > @@ -440,6 +442,15 @@ static int domcreate_setdefault_dm_user(libxl__gc
> > *gc,
> > +    if (!libxl__dm_supported(gc, dm,
> > libxl__dm_support_check__emulator_id)) {
> > +        /* we don't want to run the pv backends as non-root because
> > +         * device hotplug will no longer work. */
> > +        LOG(WARN,
> > + "Device model does not support split PV backends, running it as root");
> > +        user = "root";
> > +        goto found;
> > +    }
> 
> Should this be a fatal error, unless b_info->device_model_user == "root"?

This is in the code which sets the default value if the configuration
didn't specify one.

Ian.

Re: [PATCH 26/28] libxl: spawns two QEMUs for HVM guests

[Ian Campbell]

On Fri, 2016-01-08 at 14:35 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 26/28] libxl: spawns two QEMUs for HVM
> guests"):
> > On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote:
> > > @@ -440,6 +442,15 @@ static int
> > > domcreate_setdefault_dm_user(libxl__gc
> > > *gc,
> > > +    if (!libxl__dm_supported(gc, dm,
> > > libxl__dm_support_check__emulator_id)) {
> > > +        /* we don't want to run the pv backends as non-root because
> > > +         * device hotplug will no longer work. */
> > > +        LOG(WARN,
> > > + "Device model does not support split PV backends, running it as
> > > root");
> > > +        user = "root";
> > > +        goto found;
> > > +    }
> > 
> > Should this be a fatal error, unless b_info->device_model_user ==
> > "root"?
> 
> This is in the code which sets the default value if the configuration
> didn't specify one.

Oh yes, sorry for the noise.

> 
> Ian.

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

[PATCH 27/28] libxl: Limit qemu physmap entries

[Ian Jackson]

Add a maximum limit of physmap entries to save, so that when the guest
gets write access to physmap it cannot DOS the toolstack.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v6: Split out of xs permissions relaxation patch.
---
 tools/libxl/libxl_dom.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 6ded9c1..60e8f7f 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1431,6 +1431,8 @@ static void append_string(libxl__gc *gc, char **buf, uint32_t *len,
     *len += extralen;
 }
 
+#define MAX_PHYSMAP_ENTRIES 12
+
 int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
                                        char **callee_buf,
                                        uint32_t *callee_len)
@@ -1450,6 +1452,11 @@ int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
                                   &nr_entries);
     if (!entries || nr_entries == 0) { rc = 0; goto out; }
 
+    if (nr_entries > MAX_PHYSMAP_ENTRIES) {
+        LOG(ERROR, "Max physmap entries reached");
+        return ERROR_FAIL;
+    }
+
     for (i = 0; i < nr_entries; ++i) {
         static const char *const physmap_subkeys[] = {
             "start_addr", "size", "name"
-- 
1.7.10.4

Re: [PATCH 27/28] libxl: Limit qemu physmap entries

[Ian Campbell]

On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote:
> Add a maximum limit of physmap entries to save, so that when the guest
> gets write access to physmap it cannot DOS the toolstack.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Can we have a reference for where the number 12 comes from please.

With that I think this doesn't need to wait for the rest of the series?


> ---
> v6: Split out of xs permissions relaxation patch.
> ---
>  tools/libxl/libxl_dom.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 6ded9c1..60e8f7f 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1431,6 +1431,8 @@ static void append_string(libxl__gc *gc, char
> **buf, uint32_t *len,
>      *len += extralen;
>  }
>  
> +#define MAX_PHYSMAP_ENTRIES 12
> +
>  int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
>                                         char **callee_buf,
>                                         uint32_t *callee_len)
> @@ -1450,6 +1452,11 @@ int
> libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
>                                    &nr_entries);
>      if (!entries || nr_entries == 0) { rc = 0; goto out; }
>  
> +    if (nr_entries > MAX_PHYSMAP_ENTRIES) {
> +        LOG(ERROR, "Max physmap entries reached");
> +        return ERROR_FAIL;
> +    }
> +
>      for (i = 0; i < nr_entries; ++i) {
>          static const char *const physmap_subkeys[] = {
>              "start_addr", "size", "name"
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 28/28] libxl: xsrestrict QEMU

[Ian Jackson]

If QEMU supports xsrestrict, pass xsrestrict=on to it (by default).

XXX We need to do this only if xenstored supports it, and AFAICT there
is not a particularly easy way to test this.  Should we open a new
test xenstore connection to query this information ?  We could do this
once per libxl ctx.

Allow the user to specify that xsrestrict should be on, in which case
if it qemu cannot be depriv'd, we fail.

When qemu is depriv'd it still needs write access to the physmap
records in xenstore.  It will be running with the privilege of the
domain, so we need to allow the domain write access to
 /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID/physmap

It doesn't contain any information the guest doesn't already know.
However be aware that it still means relaxing a tools/guest
interface.

The guest physmap contains the memory layout of the guest.  The guest
is already in possession of this information.  A malicious guest could
modify the physmap entries causing QEMU to read wrong information at
restore time. QEMU would populate its XenPhysmap list with wrong
entries that wouldn't match its own device emulation addresses,
leading to a failure in restoring the domain.  But it could not
compromise the security of the system because the addresses are only
used for checks against QEMU's addresses.

Is it dangerous to let the guest write values that later QEMU and
libxl read back?  Let's dig a bit deeper into the code. QEMU and libxl
use very similar functions to parse the physmap entries.

Let's start checking libxl.  The function that reads the physmap is
libxl__toolstack_save.  It calls:

* libxl__xs_directory on /physmap
  This is safe.

* libxl__xs_read
  If the path is wrong (nothing is there), it returns NULL, and it is
  handled correctly.

* strtoll on the values read
  The values are under guest control but strtoll can handle bad
  formats.  strtoll always returns an unsigned long long.  In case of
  errors, it can return LLONG_MIN or LLONG_MAX.  libxl__toolstack_save
  doesn't check for conversion errors, but it never uses the returned
  values anywhere.  That's OK.  libxl__toolstack_restore writes back
  the values to xenstore.

So in case of errors:

1) libxl__toolstack_save could return early with an error, if the
xenstore paths are wrong (nothing is on xenstore)

2) libxl__toolstack_restore could write back to xenstore any unsigned
long long, including LLONG_MIN or LLONG_MAX

Either way libxl is fine.

>From the QEMU side, the code is very similar and uses strtoll to parse
the entries, that can deal with bad input values.  The values are used
to avoid memory allocations at restore time for memory that has
already been allocated (video memory).  If the values are wrong, QEMU
will attempt another memory allocation, failing because the maxmem
limit has been reached.

Either way QEMU is fine.

There are no other out-of-guest consumers of this information.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
v6: Merge the xsrestrict and the permissions update patches, so that
      the permission relaxation is done only when needed.
    Rip out qemu feature checking code, now in new (earlier) patch
    Rip out emulator_id, now in new (earlier) patch
    Provide and document a config option to control restriction.
    Make the default depend on specified dm user, and on stubdomness.
    Arrange that when we are going to xsrestrict, we will run two qemus,
      as required
    Move rwperm initialisaton higher up the relevant function to help
      avoid future changes using it uninitialised.
    Grant write access on to the physmap subdirectory, and drop
      discussion of the rest from the commit message, and adjust
      the xenstore doc accordingly.
    Move the physmap number of entries limit into its own patch.

v5 (permissions):
    improve commit message with security details

v4 (permissions):
    add error message in case count > MAX_PHYSMAP_ENTRIES
    add a note to xenstore-paths.markdown about the possible change in
     privilege level
    only change permissions if xsrestrict is supported

v4 (xsrestrict):
    update xenstore-paths.markdown

v3 (xsrestrict):
    add emulator_ids
    mark as WIP

v3 (permissions):
    use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
    update commit message with more info on why it is safe
    add a limit on the number of physmap entries to save and restore

v2 (permissioins):
    fix permissions to actually do what intended
    use LIBXL_TOOLSTACK_DOMID instead of 0
---
 docs/man/xl.cfg.pod.5             |   14 ++++++++++
 docs/misc/xenstore-paths.markdown |    7 +++++
 tools/libxl/libxl_create.c        |   51 +++++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_dm.c            |   30 ++++++++++++++++++++--
 tools/libxl/libxl_types.idl       |    1 +
 tools/libxl/xl_cmdimpl.c          |    2 ++
 6 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index dd93725..7c0bac3 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1842,6 +1842,20 @@ Run the device model as user "username", instead of
 xen-qemudepriv-domid$domid or xen-qemudepriv-shared or root.  The
 default is to use the first of these users which exists.
 
+=item B<device_model_restrict=BOOLEAN>
+
+Restrict the device model's access to hypervisor facilities (such as
+xenstore).  The default is true if device_model_user is not `root` and
+the qemu in use supports this option (ie, when the restriction is both
+possible and beneficial), or false otherwise.
+
+If the option is set to true (1) but the restriction is not possible
+or not effective, the domain configuration is rejected.
+
+This option is ignored when a stub domain device model is in use,
+because in that situation the device model is restricted by virtue of
+being in its own domain.
+
 =back
 
 =head2 Keymaps
diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 1dc54ac..27981de 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -339,6 +339,13 @@ 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/physmap [w]
+
+Information about an HVM domain's physical memory map.  This is
+written as well as read by device models which may run at the same
+privilege level of the guest domain.  When the device model ruus with
+system privilege, this path is INTERNAL.
+
 #### ~/device-model/$DOMID[/$EMULATOR_ID]/* [INTERNAL]
 
 Information relating to device models running in the domain. $DOMID is
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 59dfcd67..5182d2b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -478,6 +478,49 @@ static int domcreate_setdefault_dm_user(libxl__gc *gc,
     return rc;
 }
 
+static int domcreate_setdefault_dm_restrict(libxl__gc *gc,
+                                        libxl__domain_create_state *dcs)
+{
+    /* convenience aliases */
+    libxl_domain_config *const d_config = dcs->guest_config;
+    libxl_domain_build_info *b_info = &d_config->b_info;
+
+    if (!libxl_defbool_is_default(b_info->device_model_restrict) &&
+        !libxl_defbool_val(b_info->device_model_restrict))
+        /* explicitly false */
+        return 0;
+
+    if (b_info->type != LIBXL_DOMAIN_TYPE_HVM ||
+        libxl_defbool_val(b_info->device_model_stubdomain)) {
+        /* in theory we don't care, so default it to true for
+         * the benefit of future callers or in case of bugs */
+        libxl_defbool_setdefault(&b_info->device_model_restrict, 1);
+        return 0;
+    }
+
+    const char *dm = libxl__domain_device_model(gc, b_info);
+
+    const char *cannot_restrict =
+        !strcmp(b_info->device_model_user, "root")
+        ? "device model user is root" :
+        !libxl__dm_supported(gc, dm, libxl__dm_support_check__xsrestrict)
+        ? "device model does not support xs_restrict" :
+        !libxl__dm_supported(gc, dm, libxl__dm_support_check__emulator_id)
+        ? "device model does not support emulator_id" :
+        0;
+
+    libxl_defbool_setdefault(&b_info->device_model_restrict, !cannot_restrict);
+
+    if (libxl_defbool_val(b_info->device_model_restrict) &&
+        cannot_restrict) {
+        LOG(ERROR, "device model restriction desired but not possible: %s",
+            cannot_restrict);
+        return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
 static void init_console_info(libxl__gc *gc,
                              libxl__device_console *console,
                              int dev_num)
@@ -1075,11 +1118,15 @@ static void domcreate_dm_support_checked(libxl__egc *egc,
     rc = domcreate_setdefault_dm_user(gc, dcs);
     if (rc) goto out;
 
+    rc = domcreate_setdefault_dm_restrict(gc, dcs);
+    if (rc) goto out;
+
     /* run two qemus? */
     if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
         !libxl_defbool_val(d_config->b_info.device_model_stubdomain) &&
-        b_info->device_model_user &&
-        strcmp(b_info->device_model_user, "root")) {
+        ((b_info->device_model_user &&
+          strcmp(b_info->device_model_user, "root")) ||
+         libxl_defbool_val(d_config->b_info.device_model_restrict))) {
         rc = libxl__dm_emuidmap_add(gc, domid, b_state, EMUID_SPLIT);
         if (rc) goto out;
     }
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index d805800..0ee956f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -420,6 +420,14 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
             b_info->device_model_user);
         return ERROR_INVAL;
     }
+    if (libxl_defbool_val(b_info->device_model_restrict) &&
+        !libxl_defbool_val(b_info->device_model_stubdomain)) {
+        /* The dm support check ought to prevent this, but in case
+         * someone has done something odd, we double check. */
+        LOG(ERROR,
+            "device_model_restrict not supported by qemu-xen-traditional");
+        return ERROR_INVAL;
+    }
 
     flexarray_vappend(dm_args, dm,
                       "-d", GCSPRINTF("%d", domid), NULL);
@@ -1238,8 +1246,14 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
         if (state->emuidmap & (1u << EMUID_SPLIT)) {
             flexarray_append(dm_args, "-xenopts");
-            flexarray_append(dm_args,
-                             GCSPRINTF("emulator_id=%u", emuid));
+            flexarray_append(dm_args, GCSPRINTF("%semulator_id=%u",
+                    libxl_defbool_val(b_info->device_model_restrict)
+                    ? "xsrestrict=on," : "",
+                                                emuid));
+        } else {
+            /* xsrestrict only makes sense with split qemu because
+             * the pv devices need unrestricted access */
+            assert(!libxl_defbool_val(b_info->device_model_restrict));
         }
     }
     flexarray_append(dm_args, NULL);
@@ -1760,11 +1774,17 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss,
     char **pass_stuff;
     const char *dm;
     int dm_state_fd = -1;
+    struct xs_permissions rwperm[2];
 
     if (libxl_defbool_val(b_info->device_model_stubdomain)) {
         abort();
     }
 
+    rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
+    rwperm[0].perms = XS_PERM_NONE;
+    rwperm[1].id = domid;
+    rwperm[1].perms = XS_PERM_WRITE;
+
     rc = libxl__dm_emuidmap_add(gc, domid, state, emuid);
     if (rc) goto out;
 
@@ -1804,6 +1824,12 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss,
                                        domid, emuid, "");
     xs_mkdir(ctx->xsh, XBT_NULL, path);
 
+    if (libxl_defbool_val(b_info->device_model_restrict)) {
+        rc = libxl__xs_mknod(gc, XBT_NULL, GCSPRINTF("%s/physmap", path),
+                             rwperm, 2);
+        if (rc) goto out;
+    }
+
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
         b_info->device_model_version
         == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9658356..d5890a8 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -443,6 +443,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("device_model_ssidref", uint32),
     ("device_model_ssid_label", string),
     ("device_model_user", string),
+    ("device_model_restrict", libxl_defbool),
 
     # extra parameters pass directly to qemu, NULL terminated
     ("extra",            libxl_string_list),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f9933cb..bdef4dc 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2176,6 +2176,8 @@ skip_vfb:
         }
     }
 
+    xlu_cfg_get_defbool(config, "device_model_restrict",
+                        &b_info->device_model_restrict, 0);
 
     xlu_cfg_replace_string (config, "device_model_override",
                             &b_info->device_model, 0);
-- 
1.7.10.4

Re: [PATCH 28/28] libxl: xsrestrict QEMU

[Ian Campbell]

On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote:
> If QEMU supports xsrestrict, pass xsrestrict=on to it (by default).
> 
> XXX We need to do this only if xenstored supports it, and AFAICT there
> is not a particularly easy way to test this.  Should we open a new
> test xenstore connection to query this information ?  We could do this
> once per libxl ctx.

Is this because the support is in oxenstored ^ cxenstored?

Otherwise I would argue that the toolstack should expect the xenstored to
be at least as new as it is.

> Let's start checking libxl.  The function that reads the physmap is
> libxl__toolstack_save.  It calls:
> 
> * libxl__xs_directory on /physmap
>   This is safe.

Potentially very many entries and hence a big return array? Are the guest
quotas sufficient to not worry about this?

> Either way libxl is fine.
> 
[...]
> Either way QEMU is fine.

Perhaps we should add a code comment to each of these places noting that
the values are guest controlled, since it is a bit more unexpected in this
case than e.g. frontend dirs.

> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-
> paths.markdown
> index 1dc54ac..27981de 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -339,6 +339,13 @@ 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/physmap [w]
> +
> +Information about an HVM domain's physical memory map.  This is
> +written as well as read by device models which may run at the same
> +privilege level of the guest domain.  When the device model ruus with

"runs"

> +system privilege, this path is INTERNAL.


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

Re: [PATCH 28/28] libxl: xsrestrict QEMU

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 28/28] libxl: xsrestrict QEMU"):
> On Tue, 2015-12-22 at 18:45 +0000, Ian Jackson wrote:
> > If QEMU supports xsrestrict, pass xsrestrict=on to it (by default).
> > 
> > XXX We need to do this only if xenstored supports it, and AFAICT there
> > is not a particularly easy way to test this.  Should we open a new
> > test xenstore connection to query this information ?  We could do this
> > once per libxl ctx.
> 
> Is this because the support is in oxenstored ^ cxenstored?

Yes.

> Otherwise I would argue that the toolstack should expect the xenstored to
> be at least as new as it is.

True.

> > * libxl__xs_directory on /physmap
> >   This is safe.
> 
> Potentially very many entries and hence a big return array? Are the guest
> quotas sufficient to not worry about this?

Yes, the quota has to deal with that anyway (since the directory is in
core in xenstored, anyway).

> Perhaps we should add a code comment to each of these places noting that
> the values are guest controlled, since it is a bit more unexpected in this
> case than e.g. frontend dirs.

This is a good idea.

> > +Information about an HVM domain's physical memory map.  This is
> > +written as well as read by device models which may run at the same
> > +privilege level of the guest domain.  When the device model ruus with
> 
> "runs"

Fixed, thanks.

Ian.

Re: [PATCH 28/28] libxl: xsrestrict QEMU

[Stefano Stabellini]

On Tue, 22 Dec 2015, Ian Jackson wrote:
> If QEMU supports xsrestrict, pass xsrestrict=on to it (by default).
> 
> XXX We need to do this only if xenstored supports it, and AFAICT there
> is not a particularly easy way to test this.  Should we open a new
> test xenstore connection to query this information ?  We could do this
> once per libxl ctx.
> 
> Allow the user to specify that xsrestrict should be on, in which case
> if it qemu cannot be depriv'd, we fail.
> 
> When qemu is depriv'd it still needs write access to the physmap
> records in xenstore.  It will be running with the privilege of the
> domain, so we need to allow the domain write access to
>  /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID/physmap

QEMU also needs to be able to write to "state" under
/local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID

I guess theoretically it might be possible to restrict the xenstore
connection on the QEMU side only after switching to the "running" state,
however that's not how it was done in the early implementation at least:

http://marc.info/?i=1433173614-19716-2-git-send-email-stefano.stabellini%40eu.citrix.com

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

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Wei Liu]

On Tue, Dec 22, 2015 at 06:44:35PM +0000, Ian Jackson wrote:
> This is a new version of Stefano Stabellini's series
>   [PATCH v5 0/6] libxl: xs_restrict QEMU
> 
> I took Stefano's code as a spec for how to interact with qemu, and
> have reworked the whole series.  In particular, I have
>  - rebased onto staging
>  - split up some of the larger patches
>  - restructured the patches so that every intervening version should work
>  - redone the async task handling
>  - provided what seem to me to be appropriate configuration options
>  - shaved a few yaks (although I tried to limit this!)
>  - fixed the cross-version compatibility
>  - reduced the new permission grant for the domain to only .../physmap
> 

One aspect I don't see in this series is how save and restore for multiple
emulator context is handled.

Did I miss anything?

Wei.

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Stefano Stabellini]

On Thu, 7 Jan 2016, Wei Liu wrote:
> On Tue, Dec 22, 2015 at 06:44:35PM +0000, Ian Jackson wrote:
> > This is a new version of Stefano Stabellini's series
> >   [PATCH v5 0/6] libxl: xs_restrict QEMU
> > 
> > I took Stefano's code as a spec for how to interact with qemu, and
> > have reworked the whole series.  In particular, I have
> >  - rebased onto staging
> >  - split up some of the larger patches
> >  - restructured the patches so that every intervening version should work
> >  - redone the async task handling
> >  - provided what seem to me to be appropriate configuration options
> >  - shaved a few yaks (although I tried to limit this!)
> >  - fixed the cross-version compatibility
> >  - reduced the new permission grant for the domain to only .../physmap
> > 
> 
> One aspect I don't see in this series is how save and restore for multiple
> emulator context is handled.
> 
> Did I miss anything?

With the premise that I didn't have a chance to read the series yet, in
the original design there aren't multiple emulators: one qemu instance
is the emulator, the other only provides PV backends.

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Ian Jackson]

Stefano Stabellini writes ("Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu"):
> On Thu, 7 Jan 2016, Wei Liu wrote:
> > One aspect I don't see in this series is how save and restore for multiple
> > emulator context is handled.
> > 
> > Did I miss anything?
> 
> With the premise that I didn't have a chance to read the series yet, in
> the original design there aren't multiple emulators: one qemu instance
> is the emulator, the other only provides PV backends.

Right.  And the PV backends don't have any "emulator state" in that
sense.  They are simply torn down and recreated.

Ian.

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Stefano Stabellini]

I take that this series is going to miss 4.7 at this stage, right?

On Tue, 22 Dec 2015, Ian Jackson wrote:
> This is a new version of Stefano Stabellini's series
>   [PATCH v5 0/6] libxl: xs_restrict QEMU
> 
> I took Stefano's code as a spec for how to interact with qemu, and
> have reworked the whole series.  In particular, I have
>  - rebased onto staging
>  - split up some of the larger patches
>  - restructured the patches so that every intervening version should work
>  - redone the async task handling
>  - provided what seem to me to be appropriate configuration options
>  - shaved a few yaks (although I tried to limit this!)
>  - fixed the cross-version compatibility
>  - reduced the new permission grant for the domain to only .../physmap
> 
> I have compiled this but I haven't tested it yet.
> 
> I think it would be very useful at this stage for others to read the
> commit messages, internal docs, and important structural elements.
> 
> Detailed code review should probably wait until after testing which in
> turn ought to wait unil we decide what (if any) design and interface
> changes are needed.
> 
> And I don't expect to get any feedback until the new year :-).
> 
> Happy holidays all.
> 
> Regards,
> Ian.
> 

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

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Wei Liu]

On Sun, Apr 10, 2016 at 12:36:26PM -0700, Stefano Stabellini wrote:
> I take that this series is going to miss 4.7 at this stage, right?
> 

Correct.

Wei.

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

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Ian Jackson]

Stefano Stabellini writes ("Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu"):
> I take that this series is going to miss 4.7 at this stage, right?

I'm afraid so.  We concluded that a crucial piece - arranging for the
necessary access controls on privcmd - was not going to be in place
for 4.7.

Without that, this series does not offer much additional security.  It
is a shame that the code will rot a bit in the meantime as a result of
it missing 4.7.

Ian.

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

Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Ian Jackson]

Ian Jackson writes ("Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu"):
> Stefano Stabellini writes ("Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu"):
> > I take that this series is going to miss 4.7 at this stage, right?
> 
> I'm afraid so.  We concluded that a crucial piece - arranging for the
> necessary access controls on privcmd - was not going to be in place
> for 4.7.
> 
> Without that, this series does not offer much additional security.  It
> is a shame that the code will rot a bit in the meantime as a result of
> it missing 4.7.

FYI, the Citrix XenServer team is interested in work in this area.
I've been having some conversations with them, to see how we can best
work together.

Watch this space.  I'm hopeful that this work (or something
substantially similar) will be ready for Xen 4.8.

Ian.

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