[PATCH v2 0/5] libxl: xs_restrict QEMU

[PATCH v2 0/5] libxl: xs_restrict QEMU

[Stefano Stabellini]

Hi all,

this patch series changes libxl to start QEMU as device model with the
new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
It also starts a second QEMU to provide PV backends in userspace (qdisk)
to HVM guests.


Changes in v2:

- fix xs permissions to actually do what intended
- use LIBXL_TOOLSTACK_DOMID instead of 0
- add "libxl: xsrestrict QEMU"
- add "libxl: change xs path for pv qemu"
- add "libxl: spawns two QEMUs for HVM guests"


Stefano Stabellini (5):
      libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID
      libxl: do not add a vkb backend to hvm guests
      libxl: xsrestrict QEMU
      libxl: change xs path for pv qemu
      libxl: spawns two QEMUs for HVM guests

 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_create.c   |   23 +++++++---
 tools/libxl/libxl_device.c   |    2 +-
 tools/libxl/libxl_dm.c       |   99 +++++++++++++++++++++++++++++++++++++-----
 tools/libxl/libxl_dom.c      |   12 ++---
 tools/libxl/libxl_internal.c |   19 ++++++--
 tools/libxl/libxl_internal.h |    8 ++--
 tools/libxl/libxl_pci.c      |   14 +++---
 tools/libxl/libxl_utils.c    |   10 +++++
 9 files changed, 152 insertions(+), 37 deletions(-)

Cheers,

Stefano

[PATCH v2 1/5] libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

The device model is going to restrict its xenstore connection to $DOMID
level. Let it access /local/domain/0/device-model/$DOMID, as it is
required by QEMU to read/write the physmap. It doesn't contain any
information the guest is not already fully aware of.

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

---
Changes in v2:
- fix permissions to actually do what intended
- use LIBXL_TOOLSTACK_DOMID instead of 0
---
 tools/libxl/libxl_dm.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 79a9a22..f4f104f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1461,6 +1461,7 @@ 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();
@@ -1503,7 +1504,11 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     }
 
     path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
-    xs_mkdir(ctx->xsh, XBT_NULL, path);
+    rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
+    rwperm[0].perms = XS_PERM_NONE;
+    rwperm[1].id = domid;
+    rwperm[1].perms = XS_PERM_WRITE;
+    libxl__xs_mkdir(gc, XBT_NULL, path, rwperm, 2); 
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
         b_info->device_model_version
-- 
1.7.10.4

Re: [PATCH v2 1/5] libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID

[Wei Liu]

On Thu, Jun 04, 2015 at 12:28:15PM +0100, Stefano Stabellini wrote:
> The device model is going to restrict its xenstore connection to $DOMID
> level. Let it access /local/domain/0/device-model/$DOMID, as it is
> required by QEMU to read/write the physmap. It doesn't contain any
> information the guest is not already fully aware of.

The "0" in commit message and title need to be changed to
LIBXL_TOOLSTACK_DOMID as well.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> Changes in v2:
> - fix permissions to actually do what intended
> - use LIBXL_TOOLSTACK_DOMID instead of 0
> ---
>  tools/libxl/libxl_dm.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 79a9a22..f4f104f 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1461,6 +1461,7 @@ 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();
> @@ -1503,7 +1504,11 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>      }
>  
>      path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
> -    xs_mkdir(ctx->xsh, XBT_NULL, path);
> +    rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
> +    rwperm[0].perms = XS_PERM_NONE;
> +    rwperm[1].id = domid;
> +    rwperm[1].perms = XS_PERM_WRITE;
> +    libxl__xs_mkdir(gc, XBT_NULL, path, rwperm, 2); 
>  
>      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
>          b_info->device_model_version
> -- 
> 1.7.10.4

Re: [PATCH v2 1/5] libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

On Sun, 7 Jun 2015, Wei Liu wrote:
> On Thu, Jun 04, 2015 at 12:28:15PM +0100, Stefano Stabellini wrote:
> > The device model is going to restrict its xenstore connection to $DOMID
> > level. Let it access /local/domain/0/device-model/$DOMID, as it is
> > required by QEMU to read/write the physmap. It doesn't contain any
> > information the guest is not already fully aware of.
> 
> The "0" in commit message and title need to be changed to
> LIBXL_TOOLSTACK_DOMID as well.

OK


> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > Changes in v2:
> > - fix permissions to actually do what intended
> > - use LIBXL_TOOLSTACK_DOMID instead of 0
> > ---
> >  tools/libxl/libxl_dm.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 79a9a22..f4f104f 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1461,6 +1461,7 @@ 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();
> > @@ -1503,7 +1504,11 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
> >      }
> >  
> >      path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
> > -    xs_mkdir(ctx->xsh, XBT_NULL, path);
> > +    rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
> > +    rwperm[0].perms = XS_PERM_NONE;
> > +    rwperm[1].id = domid;
> > +    rwperm[1].perms = XS_PERM_WRITE;
> > +    libxl__xs_mkdir(gc, XBT_NULL, path, rwperm, 2); 
> >  
> >      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> >          b_info->device_model_version
> > -- 
> > 1.7.10.4
> 

Re: [PATCH v2 1/5] libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID

[Ian Jackson]

Stefano Stabellini writes ("[PATCH v2 1/5] libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID"):
> The device model is going to restrict its xenstore connection to $DOMID
> level. Let it access /local/domain/0/device-model/$DOMID, as it is
> required by QEMU to read/write the physmap. It doesn't contain any
> information the guest is not already fully aware of.

This permissions change needs to be accompanied, in its commit
message, with an argument explaining why it is safe.

In particular, we need to know that nothing uses information from this
path in an unsafe way (including in the case when the qemu is
privileged).

Ian.

[PATCH v2 2/5] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

When QEMU restricts its xenstore connection, it cannot provide PV
backends. A separate QEMU instance is required to provide PV backends in
userspace, such as qdisk. With two separate instances, it is not
possible to take advantage of vkb for mouse and keyboard, as the QEMU
that emulates the graphic card (the device model), would be separate
from the QEMU running the vkb backend (PV QEMU).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_create.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f0da7dc..a74b340 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     {
         libxl__device_console console;
         libxl__device device;
-        libxl_device_vkb vkb;
 
         init_console_info(gc, &console, 0);
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
-        libxl_device_vkb_init(&vkb);
-        libxl__device_vkb_add(gc, domid, &vkb);
-        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);
-- 
1.7.10.4

[PATCH v2 3/5] libxl: xsrestrict QEMU

[Stefano Stabellini]

Check whether QEMU supports the xsrestrict option, by parsing its --help
output. Store the result on xenstore for future reference on a per QEMU
binary basis, so that device_model_override still works fine with it.

Replace / with _ in the QEMU binary path before writing it to xenstore,
so that it doesn't get confused with xenstore paths.

If QEMU support xsrestrict, pass it as a command line option to it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_dm.c       |   63 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    3 ++
 tools/libxl/libxl_utils.c    |   10 +++++++
 3 files changed, 76 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f4f104f..b37a84a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -446,6 +446,65 @@ retry:
     return 0;
 }
 
+int libxl__check_qemu_supported(libxl__gc *gc, const char *dm, char *opt)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    pid_t pid;
+    int pipefd[2], status;
+    FILE *fp;
+    char *buf;
+    ssize_t buf_size = 512;
+    int ret = 0;
+    char *s;
+
+    s = libxl__strdup(gc, dm);
+    libxl__replace_chr(gc, s, '/', '_');
+    s = libxl__sprintf(gc, "libxl/%s/%s", s, opt);
+    buf = libxl__xs_read(gc, XBT_NULL, s);
+    if (buf != NULL)
+        return !strcmp(buf, "1");
+
+    if (access(dm, X_OK) < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "device model %s is not executable", dm);
+        return ERROR_FAIL;
+    }
+
+    if (libxl_pipe(ctx, pipefd) < 0)
+        return ERROR_FAIL;
+
+    pid = fork();
+    if (pid < 0)
+        return ERROR_FAIL;
+
+    /* child spawn QEMU */
+    if (!pid) {
+        char *args[] = {(char*)dm, "--help", NULL};
+        close(pipefd[0]);
+        libxl__exec(gc, -1, pipefd[1], pipefd[1], dm, args, NULL);
+        exit(1);
+    }
+
+    /* father parses the output */
+    close(pipefd[1]);
+    fp = fdopen(pipefd[0], "r");
+    buf = libxl__malloc(gc, buf_size);
+    while (fgets(buf, buf_size, fp) != NULL) {
+        if (strstr(buf, opt) != NULL) {
+            ret = 1;
+            goto out;
+        }
+    }
+out:
+    close(pipefd[0]);
+    waitpid(pid, &status, pid);
+    libxl_report_child_exitstatus(ctx, XTL_WARN, dm, pid, status);
+
+    ret = libxl__xs_write(gc, XBT_NULL, s, "%d", ret);
+
+    return ret;
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         const libxl_domain_config *guest_config,
@@ -931,6 +990,10 @@ end_search:
         if (user) {
             flexarray_append(dm_args, "-runas");
             flexarray_append(dm_args, user);
+            if (libxl__check_qemu_supported(gc, dm, "xsrestrict")) {
+                flexarray_append(dm_args, "-xenopts");
+                flexarray_append(dm_args, "xsrestrict=on");
+            }
         }
     }
     flexarray_append(dm_args, NULL);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7d0af40..b11297b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1505,6 +1505,7 @@ _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_vfbs, libxl_device_vfb *vfbs,
         int nr_disks, libxl_device_disk *disks,
         int nr_channels, libxl_device_channel *channels);
+_hidden int libxl__check_qemu_supported(libxl__gc *gc, const char *dm, char *opt);
 
 /*
  * This function will cause the whole libxl process to hang
@@ -3554,6 +3555,8 @@ int libxl__string_parse_json(libxl__gc *gc, const libxl__json_object *o,
                              char **p);
 
 int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len);
+/* replace all occurrences of old with new inside s */
+void libxl__replace_chr(libxl__gc *gc, char *s, char old, char new);
 
 /*
  * Compile time assertion
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 67c0b1c..ea08473 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1158,6 +1158,16 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len)
     return ret;
 }
 
+void libxl__replace_chr(libxl__gc *gc, char *s, char old, char new)
+{
+	int i = 0;
+
+	for (i = 0; s[i] != '\0'; i++) {
+		if (s[i] == old)
+			s[i] = new;
+	}
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

Re: [PATCH v2 3/5] libxl: xsrestrict QEMU

[Ian Jackson]

Stefano Stabellini writes ("[PATCH v2 3/5] libxl: xsrestrict QEMU"):
> Check whether QEMU supports the xsrestrict option, by parsing its --help
> output. Store the result on xenstore for future reference on a per QEMU
> binary basis, so that device_model_override still works fine with it.
...
> +    pid = fork();
> +    if (pid < 0)
> +        return ERROR_FAIL;

Sadly, direct use of fork is forbidden.  See the doc comment for
libxl__ev_child_fork.

I hereby volunteer to fix this.  But we should probably wait for this
series to settle down a bit first, as what you have done is suitable
for testing with xl, at least.

For now, please add a note to the commit message subject ("WIP" or
"RFC" or something).


And, I spotted this:

> +    /* father parses the output */

Normally we use "parent" rather than imputing gender to processes...


> +    s = libxl__strdup(gc, dm);
> +    libxl__replace_chr(gc, s, '/', '_');
> +    s = libxl__sprintf(gc, "libxl/%s/%s", s, opt);
> +    buf = libxl__xs_read(gc, XBT_NULL, s);
> +    if (buf != NULL)
> +        return !strcmp(buf, "1");

This cacheing mechanism is pretty nasty.  Wouldn't it be better to
save the qemu help output in a disk file, or something ?


Ian.

Re: [PATCH v2 3/5] libxl: xsrestrict QEMU

[Stefano Stabellini]

On Mon, 8 Jun 2015, Ian Jackson wrote:
> Stefano Stabellini writes ("[PATCH v2 3/5] libxl: xsrestrict QEMU"):
> > Check whether QEMU supports the xsrestrict option, by parsing its --help
> > output. Store the result on xenstore for future reference on a per QEMU
> > binary basis, so that device_model_override still works fine with it.
> ...
> > +    pid = fork();
> > +    if (pid < 0)
> > +        return ERROR_FAIL;
> 
> Sadly, direct use of fork is forbidden.  See the doc comment for
> libxl__ev_child_fork.
> 
> I hereby volunteer to fix this.

Thanks!!


> But we should probably wait for this
> series to settle down a bit first, as what you have done is suitable
> for testing with xl, at least.
>
> For now, please add a note to the commit message subject ("WIP" or
> "RFC" or something).

Sure, makes sense.


> And, I spotted this:
> 
> > +    /* father parses the output */
> 
> Normally we use "parent" rather than imputing gender to processes...

Good point

 
> > +    s = libxl__strdup(gc, dm);
> > +    libxl__replace_chr(gc, s, '/', '_');
> > +    s = libxl__sprintf(gc, "libxl/%s/%s", s, opt);
> > +    buf = libxl__xs_read(gc, XBT_NULL, s);
> > +    if (buf != NULL)
> > +        return !strcmp(buf, "1");
> 
> This cacheing mechanism is pretty nasty.  Wouldn't it be better to
> save the qemu help output in a disk file, or something ?

We could, but we would still have the same issue of having to store the
output of --help for each different qemu binary on different paths,
which I think is the nasty party.

[PATCH v2 4/5] libxl: change xs path for pv qemu

[Stefano Stabellini]

If QEMU is ran just to provide PV backends, change the xenstore path to
/local/domain/0/device-model/$DOMID/pv.

Add a parameter to libxl__device_model_xs_path to distinguish the device
model from the pv backends provider.

Store the device model binary path under
/local/domain/$DOMID/device-model on xenstore, so that we can fetch it
later and retrieve the list of supported options from
/local/domain/0/libxl/$device_model_binary, introduce in the previous
path.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_device.c   |    2 +-
 tools/libxl/libxl_dm.c       |   20 ++++++++++++--------
 tools/libxl/libxl_dom.c      |   12 ++++++------
 tools/libxl/libxl_internal.c |   19 +++++++++++++++----
 tools/libxl/libxl_internal.h |    5 ++---
 tools/libxl/libxl_pci.c      |   14 +++++++-------
 7 files changed, 44 insertions(+), 30 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 516713e..bca4c88 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1035,7 +1035,7 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
     if (type == LIBXL_DOMAIN_TYPE_HVM) {
         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, false, dm_domid, domid, "/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_device.c b/tools/libxl/libxl_device.c
index 93bb41e..aadcd08 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1190,7 +1190,7 @@ 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, false, dm_domid, domid, "/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 b37a84a..29ef8ae 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1267,9 +1267,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
     xs_mkdir(ctx->xsh, t,
-             libxl__device_model_xs_path(gc, dm_domid, guest_domid, ""));
+             libxl__device_model_xs_path(gc, false, dm_domid, guest_domid, ""));
     xs_set_permissions(ctx->xsh, t,
-                       libxl__device_model_xs_path(gc, dm_domid,
+                       libxl__device_model_xs_path(gc, false, dm_domid,
                                                    guest_domid, ""),
                        perm, ARRAY_SIZE(perm));
     if (!xs_transaction_end(ctx->xsh, t, 0))
@@ -1430,7 +1430,7 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
     sdss->xswait.what = GCSPRINTF("Stubdom %u for %u startup",
                                   dm_domid, sdss->dm.guest_domid);
     sdss->xswait.path =
-        libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
+        libxl__device_model_xs_path(gc, true, dm_domid, sdss->dm.guest_domid,
                                     "/state");
     sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
     sdss->xswait.callback = stubdom_xswait_cb;
@@ -1566,7 +1566,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, b_info->type == LIBXL_DOMAIN_TYPE_PV,
+			LIBXL_TOOLSTACK_DOMID, domid, "");
     rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
     rwperm[0].perms = XS_PERM_NONE;
     rwperm[1].id = domid;
@@ -1595,6 +1596,8 @@ 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");
 
+    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("%s/%s", dom_path, "device-model"), "%s", dm);
+
     if (vnc && vnc->passwd) {
         /* This xenstore key will only be used by qemu-xen-traditionnal.
          * The code to supply vncpasswd to qemu-xen is later. */
@@ -1619,8 +1622,9 @@ retry_transaction:
         LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
-    spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                                domid, "/state");
+    spawn->xspath = libxl__device_model_xs_path(gc,
+            b_info->type == LIBXL_DOMAIN_TYPE_PV, LIBXL_TOOLSTACK_DOMID,
+            domid, "/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;
@@ -1748,7 +1752,7 @@ 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, true, 0, domid, "/state");
     dmss->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     /*
      * We cannot save Qemu pid anywhere in the xenstore guest dir,
@@ -1831,7 +1835,7 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
-    char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
+    char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
                                              domid, "");
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOG(ERROR, "xs_rm failed for %s", path);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f408646..8e254a7 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1016,7 +1016,7 @@ 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, false, dm_domid, domid, "/command");
     return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
 }
 
@@ -1034,7 +1034,7 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
                                    uint32_t domid,
                                    uint64_t phys_offset, char *node)
 {
-    return libxl__device_model_xs_path(gc, dm_domid, domid,
+    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                        "/physmap/%"PRIx64"/%s",
                                        phys_offset, node);
 }
@@ -1146,9 +1146,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,
+        lds->cmd_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                                     "/logdirty/cmd");
-        lds->ret_path = libxl__device_model_xs_path(gc, dm_domid, domid,
+        lds->ret_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                                     "/logdirty/ret");
     }
     lds->cmd = enable ? "enable" : "disable";
@@ -1672,7 +1672,7 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
                                  uint32_t domid,
                                  char *phys_offset, char *node)
 {
-    return libxl__device_model_xs_path(gc, dm_domid, domid,
+    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                        "/physmap/%s/%s",
                                        phys_offset, node);
 }
@@ -1694,7 +1694,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
     dm_domid = libxl_get_stubdom_id(CTX, domid);
 
     entries = libxl__xs_directory(gc, 0,
-                libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"),
+                libxl__device_model_xs_path(gc, false, dm_domid, domid, "/physmap"),
                 &num);
     count = num;
 
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index c2c67bd..f8a59d2 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -567,14 +567,25 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
-char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
+char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
                                   uint32_t domid, const char *format,  ...)
 {
     char *s, *fmt;
     va_list ap;
-
-    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
-                    domid, format);
+    char *dm;
+    int xsrestrict = 0;
+
+    dm = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid));
+    if (dm)
+        xsrestrict = libxl__check_qemu_supported(gc, dm, "xsrestrict");
+
+    if (!xsrestrict || !pvqemu) {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
+                domid, format);
+    } else {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u/pv%s", dm_domid,
+                domid, 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 b11297b..c154827 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1826,9 +1826,8 @@ _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);
+_hidden char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
+                                  uint32_t domid, const char *format,  ...);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index e0743f8..07f6209 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -853,9 +853,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     uint32_t dm_domid;
 
     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, false, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
     if (pcidev->vdevfn) {
         libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
                         pcidev->domain, pcidev->bus, pcidev->dev,
@@ -870,9 +870,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 = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
     vdevfn = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     if ( rc < 0 )
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                    "qemu refused to add device: %s", vdevfn);
@@ -1178,9 +1178,9 @@ 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");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
     libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
                     pcidev->bus, pcidev->dev, pcidev->func);
 
@@ -1198,7 +1198,7 @@ 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 = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
 
     return 0;
-- 
1.7.10.4

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Wei Liu]

On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> If QEMU is ran just to provide PV backends, change the xenstore path to
> /local/domain/0/device-model/$DOMID/pv.
> 
> Add a parameter to libxl__device_model_xs_path to distinguish the device
> model from the pv backends provider.
> 
> Store the device model binary path under
> /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> later and retrieve the list of supported options from
> /local/domain/0/libxl/$device_model_binary, introduce in the previous
> path.
> 

TBH this protocol works, but it is not very extensible.

I envisaged we need to assign $emulator_id to different device models
when I fixed stubdom, but I never got to that since there weren't need
for multiple emulators.

That is, as an example:

/local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx

That way we can:

1. Have something like multidev in libxl to wait for several device
   models to be ready without writing tedious code for every single one.
2. Fit into libxl migration stream, which naturally uses $emulator_id to
   distinguish different emulators. 

The downside of this is we need to add an extra option to QEMU to accept
the emulator id assigned by toolstack.

Just my two cents.

Wei.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl.c          |    2 +-
>  tools/libxl/libxl_device.c   |    2 +-
>  tools/libxl/libxl_dm.c       |   20 ++++++++++++--------
>  tools/libxl/libxl_dom.c      |   12 ++++++------
>  tools/libxl/libxl_internal.c |   19 +++++++++++++++----
>  tools/libxl/libxl_internal.h |    5 ++---
>  tools/libxl/libxl_pci.c      |   14 +++++++-------
>  7 files changed, 44 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 516713e..bca4c88 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1035,7 +1035,7 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
>      if (type == LIBXL_DOMAIN_TYPE_HVM) {
>          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, false, dm_domid, domid, "/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_device.c b/tools/libxl/libxl_device.c
> index 93bb41e..aadcd08 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -1190,7 +1190,7 @@ 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, false, dm_domid, domid, "/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 b37a84a..29ef8ae 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1267,9 +1267,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>  retry_transaction:
>      t = xs_transaction_start(ctx->xsh);
>      xs_mkdir(ctx->xsh, t,
> -             libxl__device_model_xs_path(gc, dm_domid, guest_domid, ""));
> +             libxl__device_model_xs_path(gc, false, dm_domid, guest_domid, ""));
>      xs_set_permissions(ctx->xsh, t,
> -                       libxl__device_model_xs_path(gc, dm_domid,
> +                       libxl__device_model_xs_path(gc, false, dm_domid,
>                                                     guest_domid, ""),
>                         perm, ARRAY_SIZE(perm));
>      if (!xs_transaction_end(ctx->xsh, t, 0))
> @@ -1430,7 +1430,7 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
>      sdss->xswait.what = GCSPRINTF("Stubdom %u for %u startup",
>                                    dm_domid, sdss->dm.guest_domid);
>      sdss->xswait.path =
> -        libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
> +        libxl__device_model_xs_path(gc, true, dm_domid, sdss->dm.guest_domid,
>                                      "/state");
>      sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
>      sdss->xswait.callback = stubdom_xswait_cb;
> @@ -1566,7 +1566,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, b_info->type == LIBXL_DOMAIN_TYPE_PV,
> +			LIBXL_TOOLSTACK_DOMID, domid, "");
>      rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
>      rwperm[0].perms = XS_PERM_NONE;
>      rwperm[1].id = domid;
> @@ -1595,6 +1596,8 @@ 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");
>  
> +    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("%s/%s", dom_path, "device-model"), "%s", dm);
> +
>      if (vnc && vnc->passwd) {
>          /* This xenstore key will only be used by qemu-xen-traditionnal.
>           * The code to supply vncpasswd to qemu-xen is later. */
> @@ -1619,8 +1622,9 @@ retry_transaction:
>          LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
>  
>      spawn->what = GCSPRINTF("domain %d device model", domid);
> -    spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
> -                                                domid, "/state");
> +    spawn->xspath = libxl__device_model_xs_path(gc,
> +            b_info->type == LIBXL_DOMAIN_TYPE_PV, LIBXL_TOOLSTACK_DOMID,
> +            domid, "/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;
> @@ -1748,7 +1752,7 @@ 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, true, 0, domid, "/state");
>      dmss->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
>      /*
>       * We cannot save Qemu pid anywhere in the xenstore guest dir,
> @@ -1831,7 +1835,7 @@ out:
>  
>  int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
>  {
> -    char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
> +    char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
>                                               domid, "");
>      if (!xs_rm(CTX->xsh, XBT_NULL, path))
>          LOG(ERROR, "xs_rm failed for %s", path);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index f408646..8e254a7 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1016,7 +1016,7 @@ 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, false, dm_domid, domid, "/command");
>      return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
>  }
>  
> @@ -1034,7 +1034,7 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
>                                     uint32_t domid,
>                                     uint64_t phys_offset, char *node)
>  {
> -    return libxl__device_model_xs_path(gc, dm_domid, domid,
> +    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
>                                         "/physmap/%"PRIx64"/%s",
>                                         phys_offset, node);
>  }
> @@ -1146,9 +1146,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,
> +        lds->cmd_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
>                                                      "/logdirty/cmd");
> -        lds->ret_path = libxl__device_model_xs_path(gc, dm_domid, domid,
> +        lds->ret_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
>                                                      "/logdirty/ret");
>      }
>      lds->cmd = enable ? "enable" : "disable";
> @@ -1672,7 +1672,7 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
>                                   uint32_t domid,
>                                   char *phys_offset, char *node)
>  {
> -    return libxl__device_model_xs_path(gc, dm_domid, domid,
> +    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
>                                         "/physmap/%s/%s",
>                                         phys_offset, node);
>  }
> @@ -1694,7 +1694,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
>      dm_domid = libxl_get_stubdom_id(CTX, domid);
>  
>      entries = libxl__xs_directory(gc, 0,
> -                libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"),
> +                libxl__device_model_xs_path(gc, false, dm_domid, domid, "/physmap"),
>                  &num);
>      count = num;
>  
> diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> index c2c67bd..f8a59d2 100644
> --- a/tools/libxl/libxl_internal.c
> +++ b/tools/libxl/libxl_internal.c
> @@ -567,14 +567,25 @@ void libxl__update_domain_configuration(libxl__gc *gc,
>      dst->b_info.video_memkb = src->b_info.video_memkb;
>  }
>  
> -char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> +char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
>                                    uint32_t domid, const char *format,  ...)
>  {
>      char *s, *fmt;
>      va_list ap;
> -
> -    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
> -                    domid, format);
> +    char *dm;
> +    int xsrestrict = 0;
> +
> +    dm = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid));
> +    if (dm)
> +        xsrestrict = libxl__check_qemu_supported(gc, dm, "xsrestrict");
> +
> +    if (!xsrestrict || !pvqemu) {
> +        fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
> +                domid, format);
> +    } else {
> +        fmt = GCSPRINTF("/local/domain/%u/device-model/%u/pv%s", dm_domid,
> +                domid, 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 b11297b..c154827 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1826,9 +1826,8 @@ _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);
> +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
> +                                  uint32_t domid, const char *format,  ...);
>  
>  /* Check how executes hotplug script currently */
>  int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index e0743f8..07f6209 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -853,9 +853,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
>      uint32_t dm_domid;
>  
>      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, false, dm_domid, domid, "/state");
>      state = libxl__xs_read(gc, XBT_NULL, path);
> -    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
> +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
>      if (pcidev->vdevfn) {
>          libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
>                          pcidev->domain, pcidev->bus, pcidev->dev,
> @@ -870,9 +870,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 = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
>      vdevfn = libxl__xs_read(gc, XBT_NULL, path);
> -    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
> +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
>      if ( rc < 0 )
>          LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
>                     "qemu refused to add device: %s", vdevfn);
> @@ -1178,9 +1178,9 @@ 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");
> +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
>      state = libxl__xs_read(gc, XBT_NULL, path);
> -    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
> +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
>      libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
>                      pcidev->bus, pcidev->dev, pcidev->func);
>  
> @@ -1198,7 +1198,7 @@ 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 = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
>      xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
>  
>      return 0;
> -- 
> 1.7.10.4

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Andrew Cooper]

On 08/06/15 11:25, Wei Liu wrote:
> On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
>> If QEMU is ran just to provide PV backends, change the xenstore path to
>> /local/domain/0/device-model/$DOMID/pv.
>>
>> Add a parameter to libxl__device_model_xs_path to distinguish the device
>> model from the pv backends provider.
>>
>> Store the device model binary path under
>> /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
>> later and retrieve the list of supported options from
>> /local/domain/0/libxl/$device_model_binary, introduce in the previous
>> path.
>>
> TBH this protocol works, but it is not very extensible.
>
> I envisaged we need to assign $emulator_id to different device models
> when I fixed stubdom, but I never got to that since there weren't need
> for multiple emulators.
>
> That is, as an example:
>
> /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
>
> That way we can:
>
> 1. Have something like multidev in libxl to wait for several device
>    models to be ready without writing tedious code for every single one.
> 2. Fit into libxl migration stream, which naturally uses $emulator_id to
>    distinguish different emulators. 
>
> The downside of this is we need to add an extra option to QEMU to accept
> the emulator id assigned by toolstack.
>
> Just my two cents.
>
> Wei.

>From the XenServer point of view, we already use multiple device models
in certain cirumstances, and libxl support is one of the many tasks on
the xenopsd/libxl integration todo list.

If a change is going to be made, lets go the full way to getting
multiple emulators working properly.

FWIW, Wei's design looks to be sufficient.

~Andrew

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Stefano Stabellini]

On Mon, 8 Jun 2015, Wei Liu wrote:
> On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> > If QEMU is ran just to provide PV backends, change the xenstore path to
> > /local/domain/0/device-model/$DOMID/pv.
> > 
> > Add a parameter to libxl__device_model_xs_path to distinguish the device
> > model from the pv backends provider.
> > 
> > Store the device model binary path under
> > /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> > later and retrieve the list of supported options from
> > /local/domain/0/libxl/$device_model_binary, introduce in the previous
> > path.
> > 
> 
> TBH this protocol works, but it is not very extensible.
> 
> I envisaged we need to assign $emulator_id to different device models
> when I fixed stubdom, but I never got to that since there weren't need
> for multiple emulators.
> 
> That is, as an example:
> 
> /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
> 
> That way we can:
> 
> 1. Have something like multidev in libxl to wait for several device
>    models to be ready without writing tedious code for every single one.
> 2. Fit into libxl migration stream, which naturally uses $emulator_id to
>    distinguish different emulators. 
> 
> The downside of this is we need to add an extra option to QEMU to accept
> the emulator id assigned by toolstack.
> 
> Just my two cents.

Actually QEMU already retrieves the ioservid by calling
xc_hvm_create_ioreq_server. I could easily use that as id. I am not sure
how I could get that from libxl though, except by assuming that is going
to be 0, because libxl only knows how to create one QEMU emulator.

As there is no ioservid for pv QEMU, I could still use
/local/domain/$backend_domid/device-model/$domid/pv.

What do you think?



 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  tools/libxl/libxl.c          |    2 +-
> >  tools/libxl/libxl_device.c   |    2 +-
> >  tools/libxl/libxl_dm.c       |   20 ++++++++++++--------
> >  tools/libxl/libxl_dom.c      |   12 ++++++------
> >  tools/libxl/libxl_internal.c |   19 +++++++++++++++----
> >  tools/libxl/libxl_internal.h |    5 ++---
> >  tools/libxl/libxl_pci.c      |   14 +++++++-------
> >  7 files changed, 44 insertions(+), 30 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 516713e..bca4c88 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1035,7 +1035,7 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
> >      if (type == LIBXL_DOMAIN_TYPE_HVM) {
> >          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, false, dm_domid, domid, "/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_device.c b/tools/libxl/libxl_device.c
> > index 93bb41e..aadcd08 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -1190,7 +1190,7 @@ 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, false, dm_domid, domid, "/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 b37a84a..29ef8ae 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1267,9 +1267,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> >  retry_transaction:
> >      t = xs_transaction_start(ctx->xsh);
> >      xs_mkdir(ctx->xsh, t,
> > -             libxl__device_model_xs_path(gc, dm_domid, guest_domid, ""));
> > +             libxl__device_model_xs_path(gc, false, dm_domid, guest_domid, ""));
> >      xs_set_permissions(ctx->xsh, t,
> > -                       libxl__device_model_xs_path(gc, dm_domid,
> > +                       libxl__device_model_xs_path(gc, false, dm_domid,
> >                                                     guest_domid, ""),
> >                         perm, ARRAY_SIZE(perm));
> >      if (!xs_transaction_end(ctx->xsh, t, 0))
> > @@ -1430,7 +1430,7 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
> >      sdss->xswait.what = GCSPRINTF("Stubdom %u for %u startup",
> >                                    dm_domid, sdss->dm.guest_domid);
> >      sdss->xswait.path =
> > -        libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
> > +        libxl__device_model_xs_path(gc, true, dm_domid, sdss->dm.guest_domid,
> >                                      "/state");
> >      sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
> >      sdss->xswait.callback = stubdom_xswait_cb;
> > @@ -1566,7 +1566,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, b_info->type == LIBXL_DOMAIN_TYPE_PV,
> > +			LIBXL_TOOLSTACK_DOMID, domid, "");
> >      rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
> >      rwperm[0].perms = XS_PERM_NONE;
> >      rwperm[1].id = domid;
> > @@ -1595,6 +1596,8 @@ 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");
> >  
> > +    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("%s/%s", dom_path, "device-model"), "%s", dm);
> > +
> >      if (vnc && vnc->passwd) {
> >          /* This xenstore key will only be used by qemu-xen-traditionnal.
> >           * The code to supply vncpasswd to qemu-xen is later. */
> > @@ -1619,8 +1622,9 @@ retry_transaction:
> >          LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
> >  
> >      spawn->what = GCSPRINTF("domain %d device model", domid);
> > -    spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
> > -                                                domid, "/state");
> > +    spawn->xspath = libxl__device_model_xs_path(gc,
> > +            b_info->type == LIBXL_DOMAIN_TYPE_PV, LIBXL_TOOLSTACK_DOMID,
> > +            domid, "/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;
> > @@ -1748,7 +1752,7 @@ 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, true, 0, domid, "/state");
> >      dmss->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
> >      /*
> >       * We cannot save Qemu pid anywhere in the xenstore guest dir,
> > @@ -1831,7 +1835,7 @@ out:
> >  
> >  int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
> >  {
> > -    char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
> > +    char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
> >                                               domid, "");
> >      if (!xs_rm(CTX->xsh, XBT_NULL, path))
> >          LOG(ERROR, "xs_rm failed for %s", path);
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index f408646..8e254a7 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -1016,7 +1016,7 @@ 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, false, dm_domid, domid, "/command");
> >      return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
> >  }
> >  
> > @@ -1034,7 +1034,7 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
> >                                     uint32_t domid,
> >                                     uint64_t phys_offset, char *node)
> >  {
> > -    return libxl__device_model_xs_path(gc, dm_domid, domid,
> > +    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
> >                                         "/physmap/%"PRIx64"/%s",
> >                                         phys_offset, node);
> >  }
> > @@ -1146,9 +1146,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,
> > +        lds->cmd_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
> >                                                      "/logdirty/cmd");
> > -        lds->ret_path = libxl__device_model_xs_path(gc, dm_domid, domid,
> > +        lds->ret_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
> >                                                      "/logdirty/ret");
> >      }
> >      lds->cmd = enable ? "enable" : "disable";
> > @@ -1672,7 +1672,7 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
> >                                   uint32_t domid,
> >                                   char *phys_offset, char *node)
> >  {
> > -    return libxl__device_model_xs_path(gc, dm_domid, domid,
> > +    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
> >                                         "/physmap/%s/%s",
> >                                         phys_offset, node);
> >  }
> > @@ -1694,7 +1694,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
> >      dm_domid = libxl_get_stubdom_id(CTX, domid);
> >  
> >      entries = libxl__xs_directory(gc, 0,
> > -                libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"),
> > +                libxl__device_model_xs_path(gc, false, dm_domid, domid, "/physmap"),
> >                  &num);
> >      count = num;
> >  
> > diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
> > index c2c67bd..f8a59d2 100644
> > --- a/tools/libxl/libxl_internal.c
> > +++ b/tools/libxl/libxl_internal.c
> > @@ -567,14 +567,25 @@ void libxl__update_domain_configuration(libxl__gc *gc,
> >      dst->b_info.video_memkb = src->b_info.video_memkb;
> >  }
> >  
> > -char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
> > +char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
> >                                    uint32_t domid, const char *format,  ...)
> >  {
> >      char *s, *fmt;
> >      va_list ap;
> > -
> > -    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
> > -                    domid, format);
> > +    char *dm;
> > +    int xsrestrict = 0;
> > +
> > +    dm = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid));
> > +    if (dm)
> > +        xsrestrict = libxl__check_qemu_supported(gc, dm, "xsrestrict");
> > +
> > +    if (!xsrestrict || !pvqemu) {
> > +        fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
> > +                domid, format);
> > +    } else {
> > +        fmt = GCSPRINTF("/local/domain/%u/device-model/%u/pv%s", dm_domid,
> > +                domid, 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 b11297b..c154827 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -1826,9 +1826,8 @@ _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);
> > +_hidden char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
> > +                                  uint32_t domid, const char *format,  ...);
> >  
> >  /* Check how executes hotplug script currently */
> >  int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index e0743f8..07f6209 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -853,9 +853,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
> >      uint32_t dm_domid;
> >  
> >      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, false, dm_domid, domid, "/state");
> >      state = libxl__xs_read(gc, XBT_NULL, path);
> > -    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
> > +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
> >      if (pcidev->vdevfn) {
> >          libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
> >                          pcidev->domain, pcidev->bus, pcidev->dev,
> > @@ -870,9 +870,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 = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
> >      vdevfn = libxl__xs_read(gc, XBT_NULL, path);
> > -    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
> > +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
> >      if ( rc < 0 )
> >          LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> >                     "qemu refused to add device: %s", vdevfn);
> > @@ -1178,9 +1178,9 @@ 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");
> > +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
> >      state = libxl__xs_read(gc, XBT_NULL, path);
> > -    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
> > +    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
> >      libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
> >                      pcidev->bus, pcidev->dev, pcidev->func);
> >  
> > @@ -1198,7 +1198,7 @@ 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 = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
> >      xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
> >  
> >      return 0;
> > -- 
> > 1.7.10.4
> 

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Wei Liu]

On Mon, Jun 08, 2015 at 04:27:26PM +0100, Stefano Stabellini wrote:
> On Mon, 8 Jun 2015, Wei Liu wrote:
> > On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> > > If QEMU is ran just to provide PV backends, change the xenstore path to
> > > /local/domain/0/device-model/$DOMID/pv.
> > > 
> > > Add a parameter to libxl__device_model_xs_path to distinguish the device
> > > model from the pv backends provider.
> > > 
> > > Store the device model binary path under
> > > /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> > > later and retrieve the list of supported options from
> > > /local/domain/0/libxl/$device_model_binary, introduce in the previous
> > > path.
> > > 
> > 
> > TBH this protocol works, but it is not very extensible.
> > 
> > I envisaged we need to assign $emulator_id to different device models
> > when I fixed stubdom, but I never got to that since there weren't need
> > for multiple emulators.
> > 
> > That is, as an example:
> > 
> > /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
> > 
> > That way we can:
> > 
> > 1. Have something like multidev in libxl to wait for several device
> >    models to be ready without writing tedious code for every single one.
> > 2. Fit into libxl migration stream, which naturally uses $emulator_id to
> >    distinguish different emulators. 
> > 
> > The downside of this is we need to add an extra option to QEMU to accept
> > the emulator id assigned by toolstack.
> > 
> > Just my two cents.
> 
> Actually QEMU already retrieves the ioservid by calling
> xc_hvm_create_ioreq_server. I could easily use that as id. I am not sure
> how I could get that from libxl though, except by assuming that is going
> to be 0, because libxl only knows how to create one QEMU emulator.
> 

I don't think it's good to use that id directly because as you said
there is no way to allocate and get that from libxl. Libxl needs to
arrange xenstore directory before QEMU starts up, i.e. before QEMU is
able to allocate such id from hypervisor.

Assigning an emulator id would be easy in libxl.

> As there is no ioservid for pv QEMU, I could still use
> /local/domain/$backend_domid/device-model/$domid/pv.
> 
> What do you think?
> 

Do you envisage we would need to use several PV qemu? At that point we
would face the same problem again. We might as well solve it now.

Wei.

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Stefano Stabellini]

On Tue, 9 Jun 2015, Wei Liu wrote:
> On Mon, Jun 08, 2015 at 04:27:26PM +0100, Stefano Stabellini wrote:
> > On Mon, 8 Jun 2015, Wei Liu wrote:
> > > On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> > > > If QEMU is ran just to provide PV backends, change the xenstore path to
> > > > /local/domain/0/device-model/$DOMID/pv.
> > > > 
> > > > Add a parameter to libxl__device_model_xs_path to distinguish the device
> > > > model from the pv backends provider.
> > > > 
> > > > Store the device model binary path under
> > > > /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> > > > later and retrieve the list of supported options from
> > > > /local/domain/0/libxl/$device_model_binary, introduce in the previous
> > > > path.
> > > > 
> > > 
> > > TBH this protocol works, but it is not very extensible.
> > > 
> > > I envisaged we need to assign $emulator_id to different device models
> > > when I fixed stubdom, but I never got to that since there weren't need
> > > for multiple emulators.
> > > 
> > > That is, as an example:
> > > 
> > > /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
> > > 
> > > That way we can:
> > > 
> > > 1. Have something like multidev in libxl to wait for several device
> > >    models to be ready without writing tedious code for every single one.
> > > 2. Fit into libxl migration stream, which naturally uses $emulator_id to
> > >    distinguish different emulators. 
> > > 
> > > The downside of this is we need to add an extra option to QEMU to accept
> > > the emulator id assigned by toolstack.
> > > 
> > > Just my two cents.
> > 
> > Actually QEMU already retrieves the ioservid by calling
> > xc_hvm_create_ioreq_server. I could easily use that as id. I am not sure
> > how I could get that from libxl though, except by assuming that is going
> > to be 0, because libxl only knows how to create one QEMU emulator.
> > 
> 
> I don't think it's good to use that id directly because as you said
> there is no way to allocate and get that from libxl. Libxl needs to
> arrange xenstore directory before QEMU starts up, i.e. before QEMU is
> able to allocate such id from hypervisor.
> 
> Assigning an emulator id would be easy in libxl.

Adding another option to QEMU is easy. However it is hard to figure out
which one is the right path from libxl__device_model_xs_path, see:

char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
                                          uint32_t domid, const char
                                          *format,  ...)

As you can see from this patch, libxl__device_model_xs_path is called in
many places. Some of them have very little knowledge about QEMU (see for
example libxl__toolstack_save and libxl_domain_unpause), so it would be
difficult to find the right emulator_id, unless we assume that the
device model is emulator_id=0 and pv qemu emulator_id=1. 


> > As there is no ioservid for pv QEMU, I could still use
> > /local/domain/$backend_domid/device-model/$domid/pv.
> > 
> > What do you think?
> > 
> 
> Do you envisage we would need to use several PV qemu? At that point we
> would face the same problem again. We might as well solve it now.

Theoretically we could, but in practice is difficult because there is no
protocol to decide which QEMU process should handle a particular
backend: all pv QEMU instances would want to handle all PV backends,
conflicting with each others.

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Stefano Stabellini]

On Tue, 9 Jun 2015, Stefano Stabellini wrote:
> On Tue, 9 Jun 2015, Wei Liu wrote:
> > On Mon, Jun 08, 2015 at 04:27:26PM +0100, Stefano Stabellini wrote:
> > > On Mon, 8 Jun 2015, Wei Liu wrote:
> > > > On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> > > > > If QEMU is ran just to provide PV backends, change the xenstore path to
> > > > > /local/domain/0/device-model/$DOMID/pv.
> > > > > 
> > > > > Add a parameter to libxl__device_model_xs_path to distinguish the device
> > > > > model from the pv backends provider.
> > > > > 
> > > > > Store the device model binary path under
> > > > > /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> > > > > later and retrieve the list of supported options from
> > > > > /local/domain/0/libxl/$device_model_binary, introduce in the previous
> > > > > path.
> > > > > 
> > > > 
> > > > TBH this protocol works, but it is not very extensible.
> > > > 
> > > > I envisaged we need to assign $emulator_id to different device models
> > > > when I fixed stubdom, but I never got to that since there weren't need
> > > > for multiple emulators.
> > > > 
> > > > That is, as an example:
> > > > 
> > > > /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
> > > > 
> > > > That way we can:
> > > > 
> > > > 1. Have something like multidev in libxl to wait for several device
> > > >    models to be ready without writing tedious code for every single one.
> > > > 2. Fit into libxl migration stream, which naturally uses $emulator_id to
> > > >    distinguish different emulators. 
> > > > 
> > > > The downside of this is we need to add an extra option to QEMU to accept
> > > > the emulator id assigned by toolstack.
> > > > 
> > > > Just my two cents.
> > > 
> > > Actually QEMU already retrieves the ioservid by calling
> > > xc_hvm_create_ioreq_server. I could easily use that as id. I am not sure
> > > how I could get that from libxl though, except by assuming that is going
> > > to be 0, because libxl only knows how to create one QEMU emulator.
> > > 
> > 
> > I don't think it's good to use that id directly because as you said
> > there is no way to allocate and get that from libxl. Libxl needs to
> > arrange xenstore directory before QEMU starts up, i.e. before QEMU is
> > able to allocate such id from hypervisor.
> > 
> > Assigning an emulator id would be easy in libxl.
> 
> Adding another option to QEMU is easy. However it is hard to figure out
> which one is the right path from libxl__device_model_xs_path, see:
> 
> char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
>                                           uint32_t domid, const char
>                                           *format,  ...)
> 
> As you can see from this patch, libxl__device_model_xs_path is called in
> many places. Some of them have very little knowledge about QEMU (see for
> example libxl__toolstack_save and libxl_domain_unpause), so it would be
> difficult to find the right emulator_id, unless we assume that the
> device model is emulator_id=0 and pv qemu emulator_id=1. 

To be clear, changing the xenstore protocol to what you suggested is a
good idea and I am happy to do so. Changing the libxl internals to
support multiple emulator_ids is difficult. Unless we do something hacky
like the following:

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b11297b..7b0c989 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -106,6 +106,9 @@
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
 #define DOMID_XS_PATH "domid"
+/* For the moment assume only 2 QEMU are present */
+#define QEMU_XEN_DEVICE_MODEL_ID 0
+#define QEMU_XEN_PV_QEMU_ID 1
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index c2c67bd..2780fd8 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -567,14 +567,25 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
-char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
+char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
                                   uint32_t domid, const char *format,  ...)
 {
     char *s, *fmt;
     va_list ap;
-
-    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
-                    domid, format);
+    char *dm;
+    int emulator_id = 0;
+
+    dm = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid));
+    if (dm)
+        emulator_id = libxl__check_qemu_supported(gc, dm, "emulator_id");
+
+    if (!emulator_id) {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
+                domid, format);
+    } else {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u/%u%s", dm_domid,
+                domid, pvqemu ? QEMU_XEN_PV_QEMU_ID : QEMU_XEN_DEVICE_MODEL_ID, format);
+    }
 
     va_start(ap, format);
     s = libxl__vsprintf(gc, fmt, ap); 

Re: [PATCH v2 4/5] libxl: change xs path for pv qemu

[Wei Liu]

On Tue, Jun 09, 2015 at 12:24:35PM +0100, Stefano Stabellini wrote:
> On Tue, 9 Jun 2015, Wei Liu wrote:
> > On Mon, Jun 08, 2015 at 04:27:26PM +0100, Stefano Stabellini wrote:
> > > On Mon, 8 Jun 2015, Wei Liu wrote:
> > > > On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> > > > > If QEMU is ran just to provide PV backends, change the xenstore path to
> > > > > /local/domain/0/device-model/$DOMID/pv.
> > > > > 
> > > > > Add a parameter to libxl__device_model_xs_path to distinguish the device
> > > > > model from the pv backends provider.
> > > > > 
> > > > > Store the device model binary path under
> > > > > /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> > > > > later and retrieve the list of supported options from
> > > > > /local/domain/0/libxl/$device_model_binary, introduce in the previous
> > > > > path.
> > > > > 
> > > > 
> > > > TBH this protocol works, but it is not very extensible.
> > > > 
> > > > I envisaged we need to assign $emulator_id to different device models
> > > > when I fixed stubdom, but I never got to that since there weren't need
> > > > for multiple emulators.
> > > > 
> > > > That is, as an example:
> > > > 
> > > > /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
> > > > 
> > > > That way we can:
> > > > 
> > > > 1. Have something like multidev in libxl to wait for several device
> > > >    models to be ready without writing tedious code for every single one.
> > > > 2. Fit into libxl migration stream, which naturally uses $emulator_id to
> > > >    distinguish different emulators. 
> > > > 
> > > > The downside of this is we need to add an extra option to QEMU to accept
> > > > the emulator id assigned by toolstack.
> > > > 
> > > > Just my two cents.
> > > 
> > > Actually QEMU already retrieves the ioservid by calling
> > > xc_hvm_create_ioreq_server. I could easily use that as id. I am not sure
> > > how I could get that from libxl though, except by assuming that is going
> > > to be 0, because libxl only knows how to create one QEMU emulator.
> > > 
> > 
> > I don't think it's good to use that id directly because as you said
> > there is no way to allocate and get that from libxl. Libxl needs to
> > arrange xenstore directory before QEMU starts up, i.e. before QEMU is
> > able to allocate such id from hypervisor.
> > 
> > Assigning an emulator id would be easy in libxl.
> 
> Adding another option to QEMU is easy. However it is hard to figure out
> which one is the right path from libxl__device_model_xs_path, see:
> 
> char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
>                                           uint32_t domid, const char
>                                           *format,  ...)
> 
> As you can see from this patch, libxl__device_model_xs_path is called in
> many places. Some of them have very little knowledge about QEMU (see for
> example libxl__toolstack_save and libxl_domain_unpause), so it would be

What libxl_domain_unpause does is to unpause all emulators then unpause
the guest, i.e. it should traverse all emulator ids to unpause them if
necessary. The set of emulator ids can be retrieved from xenstore - a
uniform path can make it easier.

And libxl__toolstack_save proves the need for having emulator ids in
libxl because otherwise you won't know which one is which. It would be
trivial to implement v2 format with my patch.

<1433523702-4540-1-git-send-email-wei.liu2@citrix.com>

> difficult to find the right emulator_id, unless we assume that the
> device model is emulator_id=0 and pv qemu emulator_id=1. 

If this is inside libxl level I think it's fine as long as these two ids
are documented as reserved.

Wei.

> 
> 
> > > As there is no ioservid for pv QEMU, I could still use
> > > /local/domain/$backend_domid/device-model/$domid/pv.
> > > 
> > > What do you think?
> > > 
> > 
> > Do you envisage we would need to use several PV qemu? At that point we
> > would face the same problem again. We might as well solve it now.
> 
> Theoretically we could, but in practice is difficult because there is no
> protocol to decide which QEMU process should handle a particular
> backend: all pv QEMU instances would want to handle all PV backends,
> conflicting with each others.

[PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

Starts a second QEMU to provide PV backends in userspace to HVM guests.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_create.c |   18 ++++++++++++++++++
 tools/libxl/libxl_dm.c     |    9 ++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a74b340..925738f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -981,6 +981,15 @@ static void domcreate_console_available(libxl__egc *egc,
                                         dcs->aop_console_how.for_event));
 }
 
+static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss,
+                                int rc)
+{
+    STATE_AO_GC(dmss->spawn.ao);
+
+    LOG(DEBUG, "qdisk backend spawn for domain %u %s", dmss->guest_domid,
+               rc ? "failed" : "succeed");
+}
+
 static void domcreate_bootloader_done(libxl__egc *egc,
                                       libxl__bootloader_state *bl,
                                       int rc)
@@ -1016,6 +1025,15 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->dmss.dm.callback = domcreate_devmodel_started;
     dcs->dmss.callback = domcreate_devmodel_started;
 
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        libxl__dm_spawn_state *dmss2;
+        GCNEW(dmss2);
+        dmss2->guest_domid = domid;
+        dmss2->spawn.ao = ao;
+        dmss2->callback = qdisk_spawn_outcome;
+        libxl__spawn_qdisk_backend(egc, dmss2);
+    }
+
     if ( restore_fd < 0 ) {
         rc = libxl__domain_build(gc, d_config, domid, state);
         domcreate_rebuild_done(egc, dcs, rc);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 29ef8ae..08087ea 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1835,13 +1835,20 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    int rc;
     char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
                                              domid, "");
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOG(ERROR, "xs_rm failed for %s", path);
+
+    kill_device_model(gc,
+            GCSPRINTF("libxl/%u/qdisk-backend-pid", domid));
+
     /* 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));
+
+    return rc;
 }
 
 int libxl__need_xenpv_qemu(libxl__gc *gc,
-- 
1.7.10.4

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Wei Liu]

On Thu, Jun 04, 2015 at 12:28:19PM +0100, Stefano Stabellini wrote:
> Starts a second QEMU to provide PV backends in userspace to HVM guests.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  tools/libxl/libxl_create.c |   18 ++++++++++++++++++
>  tools/libxl/libxl_dm.c     |    9 ++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index a74b340..925738f 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -981,6 +981,15 @@ static void domcreate_console_available(libxl__egc *egc,
>                                          dcs->aop_console_how.for_event));
>  }
>  
> +static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss,
> +                                int rc)
> +{
> +    STATE_AO_GC(dmss->spawn.ao);
> +
> +    LOG(DEBUG, "qdisk backend spawn for domain %u %s", dmss->guest_domid,
> +               rc ? "failed" : "succeed");
> +}
> +

This fails to propagate rc. If qdisk backend spawn failed we should just
fail guest creation, right?

Also it looks very similar to the function with the same name in
libxl.c. I'm not sure why that doesn't propagate rc either...

Wei.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

On Sun, 7 Jun 2015, Wei Liu wrote:
> On Thu, Jun 04, 2015 at 12:28:19PM +0100, Stefano Stabellini wrote:
> > Starts a second QEMU to provide PV backends in userspace to HVM guests.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  tools/libxl/libxl_create.c |   18 ++++++++++++++++++
> >  tools/libxl/libxl_dm.c     |    9 ++++++++-
> >  2 files changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index a74b340..925738f 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -981,6 +981,15 @@ static void domcreate_console_available(libxl__egc *egc,
> >                                          dcs->aop_console_how.for_event));
> >  }
> >  
> > +static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss,
> > +                                int rc)
> > +{
> > +    STATE_AO_GC(dmss->spawn.ao);
> > +
> > +    LOG(DEBUG, "qdisk backend spawn for domain %u %s", dmss->guest_domid,
> > +               rc ? "failed" : "succeed");
> > +}
> > +
> 
> This fails to propagate rc. If qdisk backend spawn failed we should just
> fail guest creation, right?

I guess so, even though the guest could still run just fine only with
the emulated devices.

But how do I propagate the rc? I am not sure how: I cannot use
CONTAINER_OF to retrieve libxl__domain_create_state.



> Also it looks very similar to the function with the same name in
> libxl.c. I'm not sure why that doesn't propagate rc either...

Right, I based this code on that.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> On Sun, 7 Jun 2015, Wei Liu wrote:
> > Also it looks very similar to the function with the same name in
> > libxl.c. I'm not sure why that doesn't propagate rc either...

I think that's a bug.

> Right, I based this code on that.

qdisk_spawn_outcome in libxl.c is part of the driver domain backend
service, which is in a special situation.  I still think something
ought to be done with the error, there, but it would have to be
communicated to the toolstack somehow (via xenstore, probably) and I'm
not sure the existing protocol between toolstack and driver domain
permits that.

Ian.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

On Mon, 8 Jun 2015, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> > On Sun, 7 Jun 2015, Wei Liu wrote:
> > > Also it looks very similar to the function with the same name in
> > > libxl.c. I'm not sure why that doesn't propagate rc either...
> 
> I think that's a bug.
> 
> > Right, I based this code on that.
> 
> qdisk_spawn_outcome in libxl.c is part of the driver domain backend
> service, which is in a special situation.  I still think something
> ought to be done with the error, there, but it would have to be
> communicated to the toolstack somehow (via xenstore, probably) and I'm
> not sure the existing protocol between toolstack and driver domain
> permits that.
 
Do you have any suggestions on how I could fix this function for the
purpose of this series? I would be quite happy to leave fixing
qdisk_spawn_outcome in libxl.c to you :-)

TBH as I wrote earlier, the failure of starting pv QEMU is not fatal
from the guest POV, so we could also leave it as is.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> Do you have any suggestions on how I could fix this function for the
> purpose of this series? I would be quite happy to leave fixing
> qdisk_spawn_outcome in libxl.c to you :-)

Sure.

> TBH as I wrote earlier, the failure of starting pv QEMU is not fatal
> from the guest POV, so we could also leave it as is.

You need to collect the result anyway to avoid potentially leaking a
pending callback and tearing down a still-active ao.

I think the right answer is probably to start all the qemus in
parallel, and count (or otherwise record) the spawn outcomes.

Ian.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

On Mon, 8 Jun 2015, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> > Do you have any suggestions on how I could fix this function for the
> > purpose of this series? I would be quite happy to leave fixing
> > qdisk_spawn_outcome in libxl.c to you :-)
> 
> Sure.
> 
> > TBH as I wrote earlier, the failure of starting pv QEMU is not fatal
> > from the guest POV, so we could also leave it as is.
> 
> You need to collect the result anyway to avoid potentially leaking a
> pending callback and tearing down a still-active ao.
>
> I think the right answer is probably to start all the qemus in
> parallel, and count (or otherwise record) the spawn outcomes.

They are already started in parallel with this patch.
Where should libxl_create.c:qdisk_spawn_outcome store the outcome?
Somewhere in dcs->build_state? Who would check for that?

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> On Mon, 8 Jun 2015, Ian Jackson wrote:
> > I think the right answer is probably to start all the qemus in
> > parallel, and count (or otherwise record) the spawn outcomes.
> 
> They are already started in parallel with this patch.
> Where should libxl_create.c:qdisk_spawn_outcome store the outcome?
> Somewhere in dcs->build_state? Who would check for that?

Each qemu already needs a record in the dcs, or hanging off it, for
the libxl__ev_child.  You can store the exit status (or an rc derived
from it) there.

The spawn_outcome function does something like this:

       int worst_rc = 0;

       libxl_report_child_exitstatus(...)
       dcs->qemus[myself]->rc = status ? ERROR_QEMU_DIED : 0;

       for (i=0; i<n_qemus; i++) {
          if (libxl__ev_child_inuse(&dcs->qemus[i].childw))
              return;
          if (dcs->qemus[i].rc IS WORSE THAN worst_rc)
              worst_rc = dcs->qemus[i].rc;
       }
       /* all qemus have completed */
       if (worst_rc)
           domcreate_complete(egc, dcs, worst_rc)
       else
          domain_create_do_next_thing(egc, dcs, ...);


Ian.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> The spawn_outcome function does something like this:
> 
>        int worst_rc = 0;
> 
>        libxl_report_child_exitstatus(...)
>        dcs->qemus[myself]->rc = status ? ERROR_QEMU_DIED : 0;
> 
>        for (i=0; i<n_qemus; i++) {
>           if (libxl__ev_child_inuse(&dcs->qemus[i].childw))
>               return;
>           if (dcs->qemus[i].rc IS WORSE THAN worst_rc)
>               worst_rc = dcs->qemus[i].rc;
>        }
>        /* all qemus have completed */
>        if (worst_rc)
>            domcreate_complete(egc, dcs, worst_rc)
>        else
>           domain_create_do_next_thing(egc, dcs, ...);

... except you're using libxl__spawn, not libxl__ev_child, so you
don't get status but rather rc and you don't need to call
libxl_report_child_exitstatus, and instead of a childw you probably
have a dmss.  But the principles are the same.

Ian.

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

On Tue, 9 Jun 2015, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> > The spawn_outcome function does something like this:
> > 
> >        int worst_rc = 0;
> > 
> >        libxl_report_child_exitstatus(...)
> >        dcs->qemus[myself]->rc = status ? ERROR_QEMU_DIED : 0;
> > 
> >        for (i=0; i<n_qemus; i++) {
> >           if (libxl__ev_child_inuse(&dcs->qemus[i].childw))
> >               return;
> >           if (dcs->qemus[i].rc IS WORSE THAN worst_rc)
> >               worst_rc = dcs->qemus[i].rc;
> >        }
> >        /* all qemus have completed */
> >        if (worst_rc)
> >            domcreate_complete(egc, dcs, worst_rc)
> >        else
> >           domain_create_do_next_thing(egc, dcs, ...);
> 
> ... except you're using libxl__spawn, not libxl__ev_child, so you
> don't get status but rather rc and you don't need to call
> libxl_report_child_exitstatus, and instead of a childw you probably
> have a dmss.  But the principles are the same.

That is of great help, but this code is quite complex and I don't
understand it. I tried to complete the missing bits but I think I ended
up with something quite far from what you had in mind. At least it
compiles but it segfaults. See below:



diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a74b340..ad5191d 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -981,6 +981,25 @@ static void domcreate_console_available(libxl__egc *egc,
                                         dcs->aop_console_how.for_event));
 }
 
+static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss,
+                                int rc)
+{
+    STATE_AO_GC(dmss->spawn.ao);
+    int worst_rc = 0, i;
+
+    dmss->qemus[dmss->emulator_id].rc = rc;
+
+    for (i=0; i<dmss->num_qemus; i++) {
+       if (libxl__spawn_inuse(&dmss->qemus[i].dmss->spawn))
+           return;
+       if (dmss->qemus[i].rc < worst_rc)
+           worst_rc = dmss->qemus[i].rc;
+    }
+    /* all qemus have completed */
+    if (worst_rc)
+        domcreate_complete(egc, dmss->dcs, worst_rc);
+}
+
 static void domcreate_bootloader_done(libxl__egc *egc,
                                       libxl__bootloader_state *bl,
                                       int rc)
@@ -1014,8 +1033,27 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->dmss.dm.guest_config = dcs->guest_config;
     dcs->dmss.dm.build_state = &dcs->build_state;
     dcs->dmss.dm.callback = domcreate_devmodel_started;
+    dcs->dmss.dm.emulator_id = QEMU_XEN_DEVICE_MODEL_ID;
+    dcs->dmss.dm.qemus = libxl__malloc(gc, sizeof(struct qemu_spawn_state));
+    dcs->dmss.dm.qemus[QEMU_XEN_DEVICE_MODEL_ID].dmss = &dcs->dmss.dm;
+    dcs->dmss.dm.dcs = dcs;
+    dcs->dmss.dm.num_qemus = 2;
     dcs->dmss.callback = domcreate_devmodel_started;
 
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        libxl__dm_spawn_state *dmss2;
+        GCNEW(dmss2);
+        dmss2->guest_domid = domid;
+        dmss2->spawn.ao = ao;
+        dmss2->emulator_id = QEMU_XEN_PV_ID;
+        dmss2->qemus = dcs->dmss.dm.qemus;
+        dmss2->qemus[QEMU_XEN_PV_ID].dmss = dmss2;
+        dmss2->callback = qdisk_spawn_outcome;
+        dmss2->num_qemus = 2;
+        dmss2->dcs = dcs;
+        libxl__spawn_qdisk_backend(egc, dmss2);
+    }
+
     if ( restore_fd < 0 ) {
         rc = libxl__domain_build(gc, d_config, domid, state);
         domcreate_rebuild_done(egc, dcs, rc);
@@ -1348,6 +1386,7 @@ static void domcreate_devmodel_started(libxl__egc *egc,
     STATE_AO_GC(dmss->spawn.ao);
     libxl_ctx *ctx = CTX;
     int domid = dcs->guest_domid;
+    int i;
 
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
@@ -1380,6 +1419,10 @@ static void domcreate_devmodel_started(libxl__egc *egc,
 
 error_out:
     assert(ret);
+    for (i=0; i<dcs->dmss.dm.num_qemus; i++) {
+        if (libxl__spawn_inuse(&dmss->qemus[i].dmss->spawn))
+            return;
+    }
     domcreate_complete(egc, dcs, ret);
 }
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3a9819b..bf02280 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1848,13 +1848,20 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    int rc;
     char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
                                              domid, "");
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOG(ERROR, "xs_rm failed for %s", path);
+
+    kill_device_model(gc,
+            GCSPRINTF("libxl/%u/qdisk-backend-pid", domid));
+
     /* 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));
+
+    return rc;
 }
 
 int libxl__need_xenpv_qemu(libxl__gc *gc,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e15cdc7..b0350bc 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3062,9 +3062,18 @@ 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 struct libxl__domain_create_state libxl__domain_create_state;
+struct qemu_spawn_state {
+    int rc;
+    struct libxl__dm_spawn_state *dmss;
+};
 struct libxl__dm_spawn_state {
     /* mixed - spawn.ao must be initialised by user; rest is private: */
     libxl__spawn_state spawn;
+    uint32_t emulator_id;
+    struct qemu_spawn_state *qemus;
+    libxl__domain_create_state *dcs;
+    int num_qemus;
     /* filled in by user, must remain valid: */
     uint32_t guest_domid; /* domain being served */
     libxl_domain_config *guest_config;

Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v2 5/5] libxl: spawns two QEMUs for HVM guests"):
> That is of great help, but this code is quite complex and I don't
> understand it. I tried to complete the missing bits but I think I ended
> up with something quite far from what you had in mind. At least it
> compiles but it segfaults. See below:
...
> +static void qdisk_spawn_outcome(libxl__egc *egc, libxl__dm_spawn_state *dmss,
> +                                int rc)
> +{
> +    STATE_AO_GC(dmss->spawn.ao);
> +    int worst_rc = 0, i;
> +
> +    dmss->qemus[dmss->emulator_id].rc = rc;
> +
> +    for (i=0; i<dmss->num_qemus; i++) {
> +       if (libxl__spawn_inuse(&dmss->qemus[i].dmss->spawn))
> +           return;
> +       if (dmss->qemus[i].rc < worst_rc)
> +           worst_rc = dmss->qemus[i].rc;
> +    }
> +    /* all qemus have completed */
> +    if (worst_rc)
> +        domcreate_complete(egc, dmss->dcs, worst_rc);
> +}

You definitely need to do something in the case where worst_rc is 0.
Otherwise when all your qemus have spawned libxl will just sit there.

You need to do something like what currently happens in
domcreate_devmodel_started: on failure, call domcreate_complete,
otherwise initiate the next step.

My suggested spawn_outcome function needs to be called by _both_
spawns, and only do the stuff in domcreate_devmodel_started when both
are done.

>  static void domcreate_bootloader_done(libxl__egc *egc,
>                                        libxl__bootloader_state *bl,
>                                        int rc)
> @@ -1014,8 +1033,27 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>      dcs->dmss.dm.guest_config = dcs->guest_config;
>      dcs->dmss.dm.build_state = &dcs->build_state;
>      dcs->dmss.dm.callback = domcreate_devmodel_started;
> +    dcs->dmss.dm.emulator_id = QEMU_XEN_DEVICE_MODEL_ID;
> +    dcs->dmss.dm.qemus = libxl__malloc(gc, sizeof(struct qemu_spawn_state));
> +    dcs->dmss.dm.qemus[QEMU_XEN_DEVICE_MODEL_ID].dmss = &dcs->dmss.dm;
> +    dcs->dmss.dm.dcs = dcs;
> +    dcs->dmss.dm.num_qemus = 2;
>      dcs->dmss.callback = domcreate_devmodel_started;
>  
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
> +        libxl__dm_spawn_state *dmss2;
> +        GCNEW(dmss2);
> +        dmss2->guest_domid = domid;
> +        dmss2->spawn.ao = ao;
> +        dmss2->emulator_id = QEMU_XEN_PV_ID;
> +        dmss2->qemus = dcs->dmss.dm.qemus;
> +        dmss2->qemus[QEMU_XEN_PV_ID].dmss = dmss2;
> +        dmss2->callback = qdisk_spawn_outcome;
> +        dmss2->num_qemus = 2;
> +        dmss2->dcs = dcs;
> +        libxl__spawn_qdisk_backend(egc, dmss2);
> +    }

Here you have two arrays qemus[], one in dmss and one in dmss2.  One
callback will happen for the spawn which uses dmss and the other for
the spawn which uses dmss2.  I don't think that can be what you want.

I think you should just have a single array of dmss's.

If you (the programmer) know that you are always going to want zero,
one or two qemus, you can maybe allocate that array "statically" as
part of the domain_create_state.

Regardless of how you do the memory allocation, you need to be able to
recover the dcs from the dmss, since the spawn outcome callback gets
the dmss, so you may need to add a dcs* to the dmss.

>  int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
>  {
> +    int rc;
>      char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
>                                               domid, "");
>      if (!xs_rm(CTX->xsh, XBT_NULL, path))
>          LOG(ERROR, "xs_rm failed for %s", path);
> +
> +    kill_device_model(gc,
> +            GCSPRINTF("libxl/%u/qdisk-backend-pid", domid));
> +
>      /* 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));

This should be a loop perhaps ?

I agree with Wei that the xenstore paths should be made regular.

> +typedef struct libxl__domain_create_state libxl__domain_create_state;
> +struct qemu_spawn_state {
> +    int rc;
> +    struct libxl__dm_spawn_state *dmss;
> +};

It might be convenient to put the rc in the dmss instead of inventing
a new wrapper.

Ian.

Re: [PATCH v2 0/5] libxl: xs_restrict QEMU

[Wei Liu]

On Thu, Jun 04, 2015 at 12:27:12PM +0100, Stefano Stabellini wrote:
> Hi all,
> 
> this patch series changes libxl to start QEMU as device model with the
> new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
> It also starts a second QEMU to provide PV backends in userspace (qdisk)
> to HVM guests.
> 

The main problem I can think of is that now we have two emulators for
the same domain, we need a new xenstore protocol -- could be as easy as
having $EMULATOR_ID in xenstore path.

Currently both emulators will write to the same xenstore path to
indicate readiness, this is going to cause problem.

Wei.

> 
> Changes in v2:
> 
> - fix xs permissions to actually do what intended
> - use LIBXL_TOOLSTACK_DOMID instead of 0
> - add "libxl: xsrestrict QEMU"
> - add "libxl: change xs path for pv qemu"
> - add "libxl: spawns two QEMUs for HVM guests"
> 
> 
> Stefano Stabellini (5):
>       libxl: allow /local/domain/0/device-model/$DOMID to be written by $DOMID
>       libxl: do not add a vkb backend to hvm guests
>       libxl: xsrestrict QEMU
>       libxl: change xs path for pv qemu
>       libxl: spawns two QEMUs for HVM guests
> 
>  tools/libxl/libxl.c          |    2 +-
>  tools/libxl/libxl_create.c   |   23 +++++++---
>  tools/libxl/libxl_device.c   |    2 +-
>  tools/libxl/libxl_dm.c       |   99 +++++++++++++++++++++++++++++++++++++-----
>  tools/libxl/libxl_dom.c      |   12 ++---
>  tools/libxl/libxl_internal.c |   19 ++++++--
>  tools/libxl/libxl_internal.h |    8 ++--
>  tools/libxl/libxl_pci.c      |   14 +++---
>  tools/libxl/libxl_utils.c    |   10 +++++
>  9 files changed, 152 insertions(+), 37 deletions(-)
> 
> Cheers,
> 
> Stefano

Re: [PATCH v2 0/5] libxl: xs_restrict QEMU

[Wei Liu]

On Sun, Jun 07, 2015 at 12:38:38PM +0100, Wei Liu wrote:
> On Thu, Jun 04, 2015 at 12:27:12PM +0100, Stefano Stabellini wrote:
> > Hi all,
> > 
> > this patch series changes libxl to start QEMU as device model with the
> > new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
> > It also starts a second QEMU to provide PV backends in userspace (qdisk)
> > to HVM guests.
> > 
> 
> The main problem I can think of is that now we have two emulators for
> the same domain, we need a new xenstore protocol -- could be as easy as
> having $EMULATOR_ID in xenstore path.
> 
> Currently both emulators will write to the same xenstore path to
> indicate readiness, this is going to cause problem.
> 

Ignore this, I missed that patch that adds in different path.

Wei.