[PATCH v3 0/6] libxl: xs_restrict QEMU

[PATCH v3 0/6] 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 v3:
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- update commit message with more info on why it is safe
- add a limit on the number of physmap entries to save and restore
- add emulator_ids
- mark patch #3 as WIP
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- change xs path to include the emulator_id
- change qdisk-backend-pid path on xenstore
- use dcs->dmss.pvqemu to spawn the second QEMU
- keep track of the rc of both QEMUs before proceeding


Stefano Stabellini (6):
      libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID
      libxl: do not add a vkb backend to hvm guests
      [WIP] libxl: xsrestrict QEMU
      libxl: change xs path for QEMU
      libxl: change qdisk-backend-pid path on xenstore
      libxl: spawns two QEMUs for HVM guests

 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_create.c   |   58 ++++++++++++++-------
 tools/libxl/libxl_device.c   |    2 +-
 tools/libxl/libxl_dm.c       |  114 +++++++++++++++++++++++++++++++++++++-----
 tools/libxl/libxl_dom.c      |   17 ++++---
 tools/libxl/libxl_internal.c |   19 +++++--
 tools/libxl/libxl_internal.h |   15 ++++--
 tools/libxl/libxl_pci.c      |   14 +++---
 tools/libxl/libxl_utils.c    |   10 ++++
 9 files changed, 198 insertions(+), 53 deletions(-)

Cheers,

Stefano

[PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

The device model is going to restrict its xenstore connection to $DOMID
level. Let qemu-xen access
/local/domain/$LIBXL_TOOLSTACK_DOMID/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.

Add a maximum limit of physmap entries to save, so that the guest cannot
DOS the toolstack.

Information under
/local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID include:

- the device model status, which is updated by the device model before
the guest is unpaused, then ignored by both QEMU and libxl

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


In the case of qemu-traditional, the state node is used to issue
commands to the device model, so avoid to change permissions in that
case.

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


---

In this version I added a limit to the number of xenstore entries to
save, because with this patch they become guest writeable. The guest
could fill xenstore with entries that libxl would need to save during
domain migration.

I didn't worry about the guest filling up xenstore itself, because the
guest could still do it under /local/domain/$DOMID/data or any of the
other nodes owned by the guest. Xenstore needs to protect itself against
this anyway.

---

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

Changes in v2:
- fix permissions to actually do what intended
- use LIBXL_TOOLSTACK_DOMID instead of 0
---
 tools/libxl/libxl_dm.c  |   11 ++++++++++-
 tools/libxl/libxl_dom.c |    5 +++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 79a9a22..2809ba0 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,15 @@ 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);
+    if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        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);
+    } else {
+        xs_mkdir(ctx->xsh, XBT_NULL, path);
+    }
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
         b_info->device_model_version
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f408646..34b3c54 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1677,6 +1677,8 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
                                        phys_offset, node);
 }
 
+#define MAX_PHYSMAP_ENTRIES 12
+
 int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
         uint32_t *len, void *dss_void)
 {
@@ -1698,6 +1700,9 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
                 &num);
     count = num;
 
+    if (count > MAX_PHYSMAP_ENTRIES)
+        return -1;
+
     *len = sizeof(version) + sizeof(count);
     *buf = calloc(1, *len);
     ptr = *buf;
-- 
1.7.10.4

[PATCH v3 2/6] 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 v3 3/6] [WIP] 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 supports xsrestrict and emulator_id, pass xsrestrict=on to it.
Statically reserve two emulator_ids, one for device models and another
for pv qemus. Use the emulator_ids appropriately.

WIP: direct use of fork is forbidden in libxl

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

---
Changes in v3:
- add emulator_ids
- mark as WIP
---
 tools/libxl/libxl_dm.c       |   72 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    7 ++++
 tools/libxl/libxl_utils.c    |   10 ++++++
 3 files changed, 89 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 2809ba0..bf77f50 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);
+    }
+
+    /* parent 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,14 @@ end_search:
         if (user) {
             flexarray_append(dm_args, "-runas");
             flexarray_append(dm_args, user);
+            if (libxl__check_qemu_supported(gc, dm, "xsrestrict") &&
+                libxl__check_qemu_supported(gc, dm, "emulator_id")) {
+                flexarray_append(dm_args, "-xenopts");
+                flexarray_append(dm_args,
+                        GCSPRINTF("xsrestrict=on,emulator_id=%u",
+                            (b_info->type == LIBXL_DOMAIN_TYPE_PV) ?
+                            QEMU_XEN_PV_ID : QEMU_XEN_DEVICE_MODEL_ID));
+            }
         }
     }
     flexarray_append(dm_args, NULL);
@@ -1666,6 +1733,11 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL);
     flexarray_vappend(dm_args, "-serial", "/dev/null", NULL);
     flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL);
+    if (libxl__check_qemu_supported(gc, dm, "emulator_id")) {
+        flexarray_append(dm_args, "-xenopts");
+        flexarray_append(dm_args,
+                GCSPRINTF("emulator_id=%u", QEMU_XEN_PV_ID));
+    }
     flexarray_append(dm_args, NULL);
     args = (char **) flexarray_contents(dm_args);
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7d0af40..b4bae2f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -106,6 +106,10 @@
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
 #define DOMID_XS_PATH "domid"
+/* Reserved QEMU emulator_ids. For the moment assume max two QEMUs: one
+ * device model and one PV backends provider. */
+#define QEMU_XEN_DEVICE_MODEL_ID  0
+#define QEMU_XEN_PV_ID            1
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
@@ -1505,6 +1509,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 +3559,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

[PATCH v3 4/6] libxl: change xs path for QEMU

[Stefano Stabellini]

Change the QEMU xenstore watch path to
/local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID/$EMULATOR_ID.
Currently two emulator_ids are statically allocated: one for device models and
one for pv qemus.

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/$LIBXL_TOOLSTACK_DOMID/libxl/$device_model_binary,
introduce in the previous path.

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

---

Changes in v3:
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- change xs path to include the emulator_id
---
 tools/libxl/libxl.c          |    2 +-
 tools/libxl/libxl_create.c   |    3 +++
 tools/libxl/libxl_device.c   |    2 +-
 tools/libxl/libxl_dm.c       |   18 ++++++++++--------
 tools/libxl/libxl_dom.c      |   12 ++++++------
 tools/libxl/libxl_internal.c |   19 +++++++++++++++----
 tools/libxl/libxl_internal.h |    5 ++---
 tools/libxl/libxl_pci.c      |   14 +++++++-------
 8 files changed, 45 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_create.c b/tools/libxl/libxl_create.c
index a74b340..df946e2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1002,6 +1002,9 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         return;
     }
 
+    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid),
+            "%s", libxl__domain_device_model(gc, info));
+
     /* consume bootloader outputs. state->pv_{kernel,ramdisk} have
      * been initialised by the bootloader already.
      */
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 bf77f50..7015729 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1271,9 +1271,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))
@@ -1434,7 +1434,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;
@@ -1570,7 +1570,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, "");
     if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
         rwperm[0].perms = XS_PERM_NONE;
@@ -1627,8 +1628,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;
@@ -1761,7 +1763,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,
@@ -1844,7 +1846,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 34b3c54..0f15a90 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);
 }
@@ -1696,7 +1696,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..5836742 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_ID : QEMU_XEN_DEVICE_MODEL_ID, 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 b4bae2f..e15cdc7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1830,9 +1830,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

[PATCH v3 5/6] libxl: change qdisk-backend-pid path on xenstore

[Stefano Stabellini]

Change the qdisk-backend-pid path on xenstore from
libxl/$DOMID/qdisk-backend-pid to /local/domain/$DOMID/image/pvqemu-pid to be
more similar to the device-model path.

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

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 7015729..fdd4d93 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1770,7 +1770,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
      * because we will call this from unprivileged driver domains,
      * so save it in the current domain libxl private dir.
      */
-    dmss->spawn.pidpath = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    dmss->spawn.pidpath = GCSPRINTF("/local/domain/%d/image/pvqemu-pid", domid);
     dmss->spawn.midproc_cb = libxl__spawn_record_pid;
     dmss->spawn.confirm_cb = device_model_confirm;
     dmss->spawn.failure_cb = device_model_startup_failed;
@@ -1830,7 +1830,7 @@ int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
     char *pid_path;
     int rc;
 
-    pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    pid_path = GCSPRINTF("/local/domain/%d/image/pvqemu-pid", domid);
 
     rc = kill_device_model(gc, pid_path);
     if (rc)
-- 
1.7.10.4

[PATCH v3 6/6] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

Starts a second QEMU to provide PV backends in userspace to HVM guests.
Use both dcs->dmss.pvqemu and dcs->dmss.dm to keep track of the starting
QEMUs. Introduce two new fields to struct libxl__dm_spawn_state: dcs to
store the pointer to libxl__domain_create_state, and rc to store the
return code.

Only proceed when both QEMUs have started.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
Changes in v3:
- use dcs->dmss.pvqemu to spawn the second QEMU
- keep track of the rc of both QEMUs before proceeding
---
 tools/libxl/libxl_create.c   |   50 ++++++++++++++++++++++++++++++------------
 tools/libxl/libxl_dm.c       |    9 +++++++-
 tools/libxl/libxl_internal.h |    3 +++
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index df946e2..94dd52c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -751,6 +751,9 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc);
+static void domcreate_devmodel_callback(libxl__egc *egc,
+                                       libxl__dm_spawn_state *dmss,
+                                       int ret);
 static void domcreate_bootloader_console_available(libxl__egc *egc,
                                                    libxl__bootloader_state *bl);
 static void domcreate_bootloader_done(libxl__egc *egc,
@@ -1016,8 +1019,17 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->dmss.dm.spawn.ao = ao;
     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.callback = domcreate_devmodel_started;
+    dcs->dmss.dm.callback = domcreate_devmodel_callback;
+    dcs->dmss.dm.dcs = dcs;
+    dcs->dmss.callback = domcreate_devmodel_callback;
+
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        dcs->dmss.pvqemu.guest_domid = domid;
+        dcs->dmss.pvqemu.spawn.ao = ao;
+        dcs->dmss.pvqemu.callback = domcreate_devmodel_callback;
+        dcs->dmss.pvqemu.dcs = dcs;
+        libxl__spawn_qdisk_backend(egc, &dcs->dmss.pvqemu);
+    }
 
     if ( restore_fd < 0 ) {
         rc = libxl__domain_build(gc, d_config, domid, state);
@@ -1347,20 +1359,13 @@ static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int ret)
 {
-    libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.dm);
+    libxl__domain_create_state *dcs = dmss->dcs;
     STATE_AO_GC(dmss->spawn.ao);
-    libxl_ctx *ctx = CTX;
     int domid = dcs->guest_domid;
 
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
 
-    if (ret) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
-                   "device model did not start: %d", ret);
-        goto error_out;
-    }
-
     if (dcs->dmss.dm.guest_domid) {
         if (d_config->b_info.device_model_version
             == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
@@ -1379,11 +1384,28 @@ static void domcreate_devmodel_started(libxl__egc *egc,
     }
 
     domcreate_attach_vtpms(egc, &dcs->multidev, 0);
-    return;
+}
 
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
+static void domcreate_devmodel_callback(libxl__egc *egc,
+                                       libxl__dm_spawn_state *dmss,
+                                       int ret)
+{
+    libxl__domain_create_state *dcs = dmss->dcs;
+    STATE_AO_GC(dmss->spawn.ao);
+    int worst_rc = 0;
+
+    dmss->rc = ret;
+
+    if (libxl__spawn_inuse(&dcs->dmss.dm.spawn) ||
+        libxl__spawn_inuse(&dcs->dmss.pvqemu.spawn))
+           return;
+    worst_rc = (dcs->dmss.dm.rc < dcs->dmss.pvqemu.rc) ? dcs->dmss.dm.rc : dcs->dmss.pvqemu.rc;
+
+    /* all qemus have completed */
+    if (worst_rc)
+        domcreate_complete(egc, dcs, worst_rc);
+    else
+        domcreate_devmodel_started(egc, dmss, 0);
 }
 
 static void domcreate_attach_vtpms(libxl__egc *egc,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fdd4d93..7423ef9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1846,13 +1846,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("/local/domain/%d/image/pvqemu-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..977ccca 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3062,9 +3062,12 @@ 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 libxl__dm_spawn_state {
     /* mixed - spawn.ao must be initialised by user; rest is private: */
     libxl__spawn_state spawn;
+    int rc;
+    libxl__domain_create_state *dcs;
     /* filled in by user, must remain valid: */
     uint32_t guest_domid; /* domain being served */
     libxl_domain_config *guest_config;
-- 
1.7.10.4

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Wei Liu]

On Wed, Jun 10, 2015 at 11:09:49AM +0100, Stefano Stabellini wrote:
> The device model is going to restrict its xenstore connection to $DOMID
> level. Let qemu-xen access
> /local/domain/$LIBXL_TOOLSTACK_DOMID/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.
> 
> Add a maximum limit of physmap entries to save, so that the guest cannot
> DOS the toolstack.
> 
> Information under
> /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID include:
> 
> - the device model status, which is updated by the device model before
> the guest is unpaused, then ignored by both QEMU and libxl
> 
> - the guest physmap, which contains the memory layout of the guest. The
> guest is already in possession of this information. A malicious guest
> could modify the physmap entries causing QEMU to read wrong information
> at restore time. QEMU would populate its XenPhysmap list with wrong
> entries that wouldn't match its own device emulation addresses, leading
> to a failure in restoring the domain. But it could not compromise the
> security of the system because the addresses are only used for checks
> against QEMU's addresses.
> 

I will leave this to Ian. FWIW they look reasonable to me.

> 
> In the case of qemu-traditional, the state node is used to issue
> commands to the device model, so avoid to change permissions in that
> case.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> 
> ---
> 
> In this version I added a limit to the number of xenstore entries to
> save, because with this patch they become guest writeable. The guest
> could fill xenstore with entries that libxl would need to save during
> domain migration.
> 
> I didn't worry about the guest filling up xenstore itself, because the
> guest could still do it under /local/domain/$DOMID/data or any of the
> other nodes owned by the guest. Xenstore needs to protect itself against
> this anyway.
> 
> ---
> 
> Changes in v3:
> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> - update commit message with more info on why it is safe
> - add a limit on the number of physmap entries to save and restore
> 
> Changes in v2:
> - fix permissions to actually do what intended
> - use LIBXL_TOOLSTACK_DOMID instead of 0
> ---
>  tools/libxl/libxl_dm.c  |   11 ++++++++++-
>  tools/libxl/libxl_dom.c |    5 +++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 79a9a22..2809ba0 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,15 @@ 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);
> +    if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +        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);

This now grants write permission to $domid regardless whether QEMU will
be actually started with xsrestrict option. Can you move this patch
later in this series after having checked QEMU supported xsrestrict and
only grant access when QEMU is capable of xsrestrict?

Wei.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Wei Liu]

On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> 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).
> 

The question is that how would this affect the non-split setup.

But do I understand correctly we are moving towards a split setup
regardlessly whether QEMU supports xsrestrict or not? I.e. there is no
non-split setup in the future.

Wei.

> 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

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Tue, 16 Jun 2015, Wei Liu wrote:
> On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > 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).
> > 
> 
> The question is that how would this affect the non-split setup.

vkb is useful because emulating usb forces QEMU to wake up more often.
However there is no way around it.


> But do I understand correctly we are moving towards a split setup
> regardlessly whether QEMU supports xsrestrict or not? I.e. there is no
> non-split setup in the future.

Yes, that is my intention.


> > 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
> 

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Ian Campbell]

On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> The device model is going to restrict its xenstore connection to $DOMID
> level.

Am I correct in concluding that only oxenstored supports XS_RESTRICT? I
don't see it in C xenstored at all.

>  Let qemu-xen access
> /local/domain/$LIBXL_TOOLSTACK_DOMID/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.

docs/misc/xenstore-paths.markdown should also be updated to reflect the
changes here (and document any keys which are missing, since you seem to
list more here than the doc contains).

> Add a maximum limit of physmap entries to save, so that the guest cannot
> DOS the toolstack.

How will we cope when the limit needs to be increased in the future for
some reason?

There is an interesting general issue here which is that we have
XS_RESTRICT which changes a connection to be treated as having the
permissions of a given target domain instead of the originating
privileged domain.

This means that in order to reduce the privileges of one thing we have
to increase the privilege of the guest itself (by granting access to
those paths), which seems rather counter-intuitive.

What we really want is a new privilege type which is "read/write to
connections which are _privileged_ over $domid, but not $domid itself"
and for XS_RESTRICT to imply that.

Retrofitting something like that to xenstored would be tricky I suspect.

When the physmap stuff was added doing it via xenstore was convenient
because we weren't concerning ourselves with this deprivileging. How
that we are though perhaps we should think about whether this is still
appropriate and consider using a QMP command to request the list instead
for example.

> @@ -1698,6 +1700,9 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
>                  &num);
>      count = num;
>  
> +    if (count > MAX_PHYSMAP_ENTRIES)
> +        return -1;

Probably worth logging some sort of clue here.

Ian.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Ian Campbell]

On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> On Tue, 16 Jun 2015, Wei Liu wrote:
> > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > 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).
> > > 
> > 
> > The question is that how would this affect the non-split setup.
> 
> vkb is useful because emulating usb forces QEMU to wake up more often.
> However there is no way around it.

Does pvfb+vkb continue to work due to code somewhere else?

Do we think anyone will actually be using emulated VGA + PV input
devices?

Re: [PATCH v3 4/6] libxl: change xs path for QEMU

[Ian Campbell]

On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> Change the QEMU xenstore watch path to
> /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID/$EMULATOR_ID.

This and the next patch should both be changing the xenstore-paths doc
too.


> Currently two emulator_ids are statically allocated: one for device models and
> one for pv qemus.
> 
> 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/$LIBXL_TOOLSTACK_DOMID/libxl/$device_model_binary,
> introduce in the previous path.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v3:
> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> - change xs path to include the emulator_id
> ---
>  tools/libxl/libxl.c          |    2 +-
>  tools/libxl/libxl_create.c   |    3 +++
>  tools/libxl/libxl_device.c   |    2 +-
>  tools/libxl/libxl_dm.c       |   18 ++++++++++--------
>  tools/libxl/libxl_dom.c      |   12 ++++++------
>  tools/libxl/libxl_internal.c |   19 +++++++++++++++----
>  tools/libxl/libxl_internal.h |    5 ++---
>  tools/libxl/libxl_pci.c      |   14 +++++++-------
>  8 files changed, 45 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_create.c b/tools/libxl/libxl_create.c
> index a74b340..df946e2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1002,6 +1002,9 @@ static void domcreate_bootloader_done(libxl__egc *egc,
>          return;
>      }
>  
> +    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid),
> +            "%s", libxl__domain_device_model(gc, info));
> +
>      /* consume bootloader outputs. state->pv_{kernel,ramdisk} have
>       * been initialised by the bootloader already.
>       */
> 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 bf77f50..7015729 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1271,9 +1271,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))
> @@ -1434,7 +1434,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;
> @@ -1570,7 +1570,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, "");
>      if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
>          rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
>          rwperm[0].perms = XS_PERM_NONE;
> @@ -1627,8 +1628,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;
> @@ -1761,7 +1763,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,
> @@ -1844,7 +1846,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 34b3c54..0f15a90 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);
>  }
> @@ -1696,7 +1696,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..5836742 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_ID : QEMU_XEN_DEVICE_MODEL_ID, 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 b4bae2f..e15cdc7 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1830,9 +1830,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;

Re: [PATCH v3 3/6] [WIP] libxl: xsrestrict QEMU

[Ian Campbell]

On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> 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.

Is there some way we could avoid needing to do this, e.g. by doing the
restrict later on via a qmp request, before the guest is unpaused of
course.

> 
> Replace / with _ in the QEMU binary path before writing it to xenstore,
> so that it doesn't get confused with xenstore paths.
> 
> If QEMU supports xsrestrict and emulator_id, pass xsrestrict=on to it.
> Statically reserve two emulator_ids, one for device models and another
> for pv qemus. Use the emulator_ids appropriately.
> 
> WIP: direct use of fork is forbidden in libxl
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> Changes in v3:
> - add emulator_ids
> - mark as WIP
> ---
>  tools/libxl/libxl_dm.c       |   72 ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |    7 ++++
>  tools/libxl/libxl_utils.c    |   10 ++++++
>  3 files changed, 89 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 2809ba0..bf77f50 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);
> +    }
> +
> +    /* parent 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,14 @@ end_search:
>          if (user) {
>              flexarray_append(dm_args, "-runas");
>              flexarray_append(dm_args, user);
> +            if (libxl__check_qemu_supported(gc, dm, "xsrestrict") &&
> +                libxl__check_qemu_supported(gc, dm, "emulator_id")) {
> +                flexarray_append(dm_args, "-xenopts");
> +                flexarray_append(dm_args,
> +                        GCSPRINTF("xsrestrict=on,emulator_id=%u",
> +                            (b_info->type == LIBXL_DOMAIN_TYPE_PV) ?
> +                            QEMU_XEN_PV_ID : QEMU_XEN_DEVICE_MODEL_ID));
> +            }
>          }
>      }
>      flexarray_append(dm_args, NULL);
> @@ -1666,6 +1733,11 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>      flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL);
>      flexarray_vappend(dm_args, "-serial", "/dev/null", NULL);
>      flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL);
> +    if (libxl__check_qemu_supported(gc, dm, "emulator_id")) {
> +        flexarray_append(dm_args, "-xenopts");
> +        flexarray_append(dm_args,
> +                GCSPRINTF("emulator_id=%u", QEMU_XEN_PV_ID));
> +    }
>      flexarray_append(dm_args, NULL);
>      args = (char **) flexarray_contents(dm_args);
>  
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 7d0af40..b4bae2f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -106,6 +106,10 @@
>  #define TAP_DEVICE_SUFFIX "-emu"
>  #define DISABLE_UDEV_PATH "libxl/disable_udev"
>  #define DOMID_XS_PATH "domid"
> +/* Reserved QEMU emulator_ids. For the moment assume max two QEMUs: one
> + * device model and one PV backends provider. */
> +#define QEMU_XEN_DEVICE_MODEL_ID  0
> +#define QEMU_XEN_PV_ID            1
>  
>  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>  
> @@ -1505,6 +1509,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 +3559,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

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

On Tue, 16 Jun 2015, Wei Liu wrote:
> On Wed, Jun 10, 2015 at 11:09:49AM +0100, Stefano Stabellini wrote:
> > The device model is going to restrict its xenstore connection to $DOMID
> > level. Let qemu-xen access
> > /local/domain/$LIBXL_TOOLSTACK_DOMID/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.
> > 
> > Add a maximum limit of physmap entries to save, so that the guest cannot
> > DOS the toolstack.
> > 
> > Information under
> > /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID include:
> > 
> > - the device model status, which is updated by the device model before
> > the guest is unpaused, then ignored by both QEMU and libxl
> > 
> > - the guest physmap, which contains the memory layout of the guest. The
> > guest is already in possession of this information. A malicious guest
> > could modify the physmap entries causing QEMU to read wrong information
> > at restore time. QEMU would populate its XenPhysmap list with wrong
> > entries that wouldn't match its own device emulation addresses, leading
> > to a failure in restoring the domain. But it could not compromise the
> > security of the system because the addresses are only used for checks
> > against QEMU's addresses.
> > 
> 
> I will leave this to Ian. FWIW they look reasonable to me.
> 
> > 
> > In the case of qemu-traditional, the state node is used to issue
> > commands to the device model, so avoid to change permissions in that
> > case.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > 
> > ---
> > 
> > In this version I added a limit to the number of xenstore entries to
> > save, because with this patch they become guest writeable. The guest
> > could fill xenstore with entries that libxl would need to save during
> > domain migration.
> > 
> > I didn't worry about the guest filling up xenstore itself, because the
> > guest could still do it under /local/domain/$DOMID/data or any of the
> > other nodes owned by the guest. Xenstore needs to protect itself against
> > this anyway.
> > 
> > ---
> > 
> > Changes in v3:
> > - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> > - update commit message with more info on why it is safe
> > - add a limit on the number of physmap entries to save and restore
> > 
> > Changes in v2:
> > - fix permissions to actually do what intended
> > - use LIBXL_TOOLSTACK_DOMID instead of 0
> > ---
> >  tools/libxl/libxl_dm.c  |   11 ++++++++++-
> >  tools/libxl/libxl_dom.c |    5 +++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 79a9a22..2809ba0 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,15 @@ 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);
> > +    if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> > +        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);
> 
> This now grants write permission to $domid regardless whether QEMU will
> be actually started with xsrestrict option. Can you move this patch
> later in this series after having checked QEMU supported xsrestrict and
> only grant access when QEMU is capable of xsrestrict?

That's a great idea, I'll do that!

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

On Thu, 25 Jun 2015, Ian Campbell wrote:
> On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> > The device model is going to restrict its xenstore connection to $DOMID
> > level.
> 
> Am I correct in concluding that only oxenstored supports XS_RESTRICT? I
> don't see it in C xenstored at all.

That is correct. C xenstored ignores it.


> >  Let qemu-xen access
> > /local/domain/$LIBXL_TOOLSTACK_DOMID/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.
> 
> docs/misc/xenstore-paths.markdown should also be updated to reflect the
> changes here (and document any keys which are missing, since you seem to
> list more here than the doc contains).

Actually docs/misc/xenstore-paths.markdown already specifies
~/device-model/$DOMID/*, I don't think there is any need to go into the
possible sub-directories there.  But I'll add a note on the possible
privilege level change.


> > Add a maximum limit of physmap entries to save, so that the guest cannot
> > DOS the toolstack.
> 
> How will we cope when the limit needs to be increased in the future for
> some reason?

The current limit, 12, is far higher than the actual number of entries
that is usually just 1 (vga). The limit is not likely to increase unless
the devices being modelled actually change significantly. In that case
we'll have a way to detect it in libxl.

The reason why I feel comfortable having such an high limit compared to
the actual usage, is that it is only meant to prevent the guest from
filling up dom0's disk, as this information is saved to file. As
xenstore entries have a size limit themselves, we should be pretty safe.

 
> There is an interesting general issue here which is that we have
> XS_RESTRICT which changes a connection to be treated as having the
> permissions of a given target domain instead of the originating
> privileged domain.
> 
> This means that in order to reduce the privileges of one thing we have
> to increase the privilege of the guest itself (by granting access to
> those paths), which seems rather counter-intuitive.
> 
> What we really want is a new privilege type which is "read/write to
> connections which are _privileged_ over $domid, but not $domid itself"
> and for XS_RESTRICT to imply that.
> 
> Retrofitting something like that to xenstored would be tricky I suspect.
> 
> When the physmap stuff was added doing it via xenstore was convenient
> because we weren't concerning ourselves with this deprivileging. How
> that we are though perhaps we should think about whether this is still
> appropriate and consider using a QMP command to request the list instead
> for example.

That is true, however this doesn't mean we cannot also add a QMP command
in the future too, especially if we follow Wei's suggestion and we only
change the permissions if xs_restrict. We could also implemented a
different XS_RESTRICT. In other words, I don't think we have to come up
with the best possible solution right now, we just need to improve over
what we already have. I think that this is an improvement.


> > @@ -1698,6 +1700,9 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
> >                  &num);
> >      count = num;
> >  
> > +    if (count > MAX_PHYSMAP_ENTRIES)
> > +        return -1;
> 
> Probably worth logging some sort of clue here.

OK

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Thu, 25 Jun 2015, Ian Campbell wrote:
> On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > 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).
> > > > 
> > > 
> > > The question is that how would this affect the non-split setup.
> > 
> > vkb is useful because emulating usb forces QEMU to wake up more often.
> > However there is no way around it.
> 
> Does pvfb+vkb continue to work due to code somewhere else?

Yes, it continues to work as usual for PV guests.


> Do we think anyone will actually be using emulated VGA + PV input
> devices?

VGA + PV input only works with Linux and is only useful for power
efficiency, because if you disable usb emulation in QEMU, then QEMU
would be able to wake up less often. Given that usb emulation is still
on by default, I don't think that this change will have a big impact.

Re: [PATCH v3 3/6] [WIP] libxl: xsrestrict QEMU

[Stefano Stabellini]

On Thu, 25 Jun 2015, Ian Campbell wrote:
> On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> > 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.
> 
> Is there some way we could avoid needing to do this, e.g. by doing the
> restrict later on via a qmp request, before the guest is unpaused of
> course.

It would be tricky because it needs to be done very early at boot time
in QEMU. Also we would still need to know whether a specific device
model supports this option before actually spawning it. So we would
still have to resort to spawning a "temporary" QEMU beforehand.


> > Replace / with _ in the QEMU binary path before writing it to xenstore,
> > so that it doesn't get confused with xenstore paths.
> > 
> > If QEMU supports xsrestrict and emulator_id, pass xsrestrict=on to it.
> > Statically reserve two emulator_ids, one for device models and another
> > for pv qemus. Use the emulator_ids appropriately.
> > 
> > WIP: direct use of fork is forbidden in libxl
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > Changes in v3:
> > - add emulator_ids
> > - mark as WIP
> > ---
> >  tools/libxl/libxl_dm.c       |   72 ++++++++++++++++++++++++++++++++++++++++++
> >  tools/libxl/libxl_internal.h |    7 ++++
> >  tools/libxl/libxl_utils.c    |   10 ++++++
> >  3 files changed, 89 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 2809ba0..bf77f50 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);
> > +    }
> > +
> > +    /* parent 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,14 @@ end_search:
> >          if (user) {
> >              flexarray_append(dm_args, "-runas");
> >              flexarray_append(dm_args, user);
> > +            if (libxl__check_qemu_supported(gc, dm, "xsrestrict") &&
> > +                libxl__check_qemu_supported(gc, dm, "emulator_id")) {
> > +                flexarray_append(dm_args, "-xenopts");
> > +                flexarray_append(dm_args,
> > +                        GCSPRINTF("xsrestrict=on,emulator_id=%u",
> > +                            (b_info->type == LIBXL_DOMAIN_TYPE_PV) ?
> > +                            QEMU_XEN_PV_ID : QEMU_XEN_DEVICE_MODEL_ID));
> > +            }
> >          }
> >      }
> >      flexarray_append(dm_args, NULL);
> > @@ -1666,6 +1733,11 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
> >      flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL);
> >      flexarray_vappend(dm_args, "-serial", "/dev/null", NULL);
> >      flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL);
> > +    if (libxl__check_qemu_supported(gc, dm, "emulator_id")) {
> > +        flexarray_append(dm_args, "-xenopts");
> > +        flexarray_append(dm_args,
> > +                GCSPRINTF("emulator_id=%u", QEMU_XEN_PV_ID));
> > +    }
> >      flexarray_append(dm_args, NULL);
> >      args = (char **) flexarray_contents(dm_args);
> >  
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 7d0af40..b4bae2f 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -106,6 +106,10 @@
> >  #define TAP_DEVICE_SUFFIX "-emu"
> >  #define DISABLE_UDEV_PATH "libxl/disable_udev"
> >  #define DOMID_XS_PATH "domid"
> > +/* Reserved QEMU emulator_ids. For the moment assume max two QEMUs: one
> > + * device model and one PV backends provider. */
> > +#define QEMU_XEN_DEVICE_MODEL_ID  0
> > +#define QEMU_XEN_PV_ID            1
> >  
> >  #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> >  
> > @@ -1505,6 +1509,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 +3559,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
> 
> 

Re: [PATCH v3 4/6] libxl: change xs path for QEMU

[Stefano Stabellini]

On Thu, 25 Jun 2015, Ian Campbell wrote:
> On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> > Change the QEMU xenstore watch path to
> > /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID/$EMULATOR_ID.
> 
> This and the next patch should both be changing the xenstore-paths doc
> too.

OK

> 
> > Currently two emulator_ids are statically allocated: one for device models and
> > one for pv qemus.
> > 
> > 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/$LIBXL_TOOLSTACK_DOMID/libxl/$device_model_binary,
> > introduce in the previous path.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in v3:
> > - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> > - change xs path to include the emulator_id
> > ---
> >  tools/libxl/libxl.c          |    2 +-
> >  tools/libxl/libxl_create.c   |    3 +++
> >  tools/libxl/libxl_device.c   |    2 +-
> >  tools/libxl/libxl_dm.c       |   18 ++++++++++--------
> >  tools/libxl/libxl_dom.c      |   12 ++++++------
> >  tools/libxl/libxl_internal.c |   19 +++++++++++++++----
> >  tools/libxl/libxl_internal.h |    5 ++---
> >  tools/libxl/libxl_pci.c      |   14 +++++++-------
> >  8 files changed, 45 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_create.c b/tools/libxl/libxl_create.c
> > index a74b340..df946e2 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -1002,6 +1002,9 @@ static void domcreate_bootloader_done(libxl__egc *egc,
> >          return;
> >      }
> >  
> > +    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid),
> > +            "%s", libxl__domain_device_model(gc, info));
> > +
> >      /* consume bootloader outputs. state->pv_{kernel,ramdisk} have
> >       * been initialised by the bootloader already.
> >       */
> > 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 bf77f50..7015729 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1271,9 +1271,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))
> > @@ -1434,7 +1434,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;
> > @@ -1570,7 +1570,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, "");
> >      if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> >          rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
> >          rwperm[0].perms = XS_PERM_NONE;
> > @@ -1627,8 +1628,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;
> > @@ -1761,7 +1763,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,
> > @@ -1844,7 +1846,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 34b3c54..0f15a90 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);
> >  }
> > @@ -1696,7 +1696,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..5836742 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_ID : QEMU_XEN_DEVICE_MODEL_ID, 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 b4bae2f..e15cdc7 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -1830,9 +1830,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;
> 
> 

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Ian Campbell]

On Mon, 2015-06-29 at 18:52 +0100, Stefano Stabellini wrote:
> On Thu, 25 Jun 2015, Ian Campbell wrote:
> > On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> > > The device model is going to restrict its xenstore connection to $DOMID
> > > level.
> > 
> > Am I correct in concluding that only oxenstored supports XS_RESTRICT? I
> > don't see it in C xenstored at all.
> 
> That is correct. C xenstored ignores it.
> 
> 
> > >  Let qemu-xen access
> > > /local/domain/$LIBXL_TOOLSTACK_DOMID/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.
> > 
> > docs/misc/xenstore-paths.markdown should also be updated to reflect the
> > changes here (and document any keys which are missing, since you seem to
> > list more here than the doc contains).
> 
> Actually docs/misc/xenstore-paths.markdown already specifies
> ~/device-model/$DOMID/*,

Yes, but it is flagged as INTERNAL, it needs to be now marked guest
readable.

>  I don't think there is any need to go into the
> possible sub-directories there.  But I'll add a note on the possible
> privilege level change.
> 
> 
> > > Add a maximum limit of physmap entries to save, so that the guest cannot
> > > DOS the toolstack.
> > 
> > How will we cope when the limit needs to be increased in the future for
> > some reason?
> 
> The current limit, 12, is far higher than the actual number of entries
> that is usually just 1 (vga). The limit is not likely to increase unless
> the devices being modelled actually change significantly. In that case
> we'll have a way to detect it in libxl.

My concern was with some external change making it >=13. If you are
confident that this cannot occur and any such change will always be
under our control then I guess that's ok.

> The reason why I feel comfortable having such an high limit compared to
> the actual usage, is that it is only meant to prevent the guest from
> filling up dom0's disk, as this information is saved to file. As
> xenstore entries have a size limit themselves, we should be pretty safe.

I'm not bothered by how high or low the limit is, just the mechanisms
which we intend to use in order to cope when it changes, which are much
easier to put in place now than trying to retrofit such a thing while
retaining support for existing released versions of qemu. 

> > There is an interesting general issue here which is that we have
> > XS_RESTRICT which changes a connection to be treated as having the
> > permissions of a given target domain instead of the originating
> > privileged domain.
> > 
> > This means that in order to reduce the privileges of one thing we have
> > to increase the privilege of the guest itself (by granting access to
> > those paths), which seems rather counter-intuitive.
> > 
> > What we really want is a new privilege type which is "read/write to
> > connections which are _privileged_ over $domid, but not $domid itself"
> > and for XS_RESTRICT to imply that.
> > 
> > Retrofitting something like that to xenstored would be tricky I suspect.
> > 
> > When the physmap stuff was added doing it via xenstore was convenient
> > because we weren't concerning ourselves with this deprivileging. How
> > that we are though perhaps we should think about whether this is still
> > appropriate and consider using a QMP command to request the list instead
> > for example.
> 
> That is true, however this doesn't mean we cannot also add a QMP command
> in the future too, especially if we follow Wei's suggestion and we only
> change the permissions if xs_restrict. We could also implemented a
> different XS_RESTRICT. In other words, I don't think we have to come up
> with the best possible solution right now, we just need to improve over
> what we already have. I think that this is an improvement.

It's not a strict improvement though, it trading off giving the guest
access to stuff which it never previously had a need to and which the
toolstack consumes.

Doing something via qmp now would turn this into a no-brainer strict
improvement rather than something which requires careful consideration
of trade offs.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Ian Campbell]

On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> On Thu, 25 Jun 2015, Ian Campbell wrote:
> > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > 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).
> > > > > 
> > > > 
> > > > The question is that how would this affect the non-split setup.
> > > 
> > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > However there is no way around it.
> > 
> > Does pvfb+vkb continue to work due to code somewhere else?
> 
> Yes, it continues to work as usual for PV guests.
> 
> 
> > Do we think anyone will actually be using emulated VGA + PV input
> > devices?
> 
> VGA + PV input only works with Linux and is only useful for power
> efficiency, because if you disable usb emulation in QEMU, then QEMU
> would be able to wake up less often. Given that usb emulation is still
> on by default, I don't think that this change will have a big impact.

My question was whether we thought anyone would be using this
non-default configuration, not what the impact on the default is.

You gave a good reason why people might be using this facility, do you
think anyone is actually using it?

Ian.

Re: [PATCH v3 3/6] [WIP] libxl: xsrestrict QEMU

[Ian Campbell]

On Mon, 2015-06-29 at 19:07 +0100, Stefano Stabellini wrote:
> On Thu, 25 Jun 2015, Ian Campbell wrote:
> > On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> > > 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.
> > 
> > Is there some way we could avoid needing to do this, e.g. by doing the
> > restrict later on via a qmp request, before the guest is unpaused of
> > course.
> 
> It would be tricky because it needs to be done very early at boot time
> in QEMU. Also we would still need to know whether a specific device
> model supports this option before actually spawning it. So we would
> still have to resort to spawning a "temporary" QEMU beforehand.

I think via qmp we can query the qemu after it starts to ask it if it
has the "xs-restrict" property and then set it to a domid if so. I had
some code to do this for the PCI permissive property we added recently,
it wasn't hard.

If we can move all the restriction stuff in libxl until after we are in
a position to ask qemu this then we don't need any temporary qemu or
--help parsing.

Ian.

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID"):
> On Thu, 25 Jun 2015, Ian Campbell wrote:
> > Am I correct in concluding that only oxenstored supports XS_RESTRICT? I
> > don't see it in C xenstored at all.
> 
> That is correct. C xenstored ignores it.

You mean it rejects it.  This is, of course, not the same ...

Ian.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Tue, 30 Jun 2015, Ian Campbell wrote:
> On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > 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).
> > > > > > 
> > > > > 
> > > > > The question is that how would this affect the non-split setup.
> > > > 
> > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > However there is no way around it.
> > > 
> > > Does pvfb+vkb continue to work due to code somewhere else?
> > 
> > Yes, it continues to work as usual for PV guests.
> > 
> > 
> > > Do we think anyone will actually be using emulated VGA + PV input
> > > devices?
> > 
> > VGA + PV input only works with Linux and is only useful for power
> > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > would be able to wake up less often. Given that usb emulation is still
> > on by default, I don't think that this change will have a big impact.
> 
> My question was whether we thought anyone would be using this
> non-default configuration, not what the impact on the default is.
> 
> You gave a good reason why people might be using this facility, do you
> think anyone is actually using it?
 
I don't know of anybody using it. I don't think we made clear enough how
to use this non-default configuration and its advantages for users to go
out of their ways to use it. 

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Ian Campbell]

On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> On Tue, 30 Jun 2015, Ian Campbell wrote:
> > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > 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).
> > > > > > > 
> > > > > > 
> > > > > > The question is that how would this affect the non-split setup.
> > > > > 
> > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > However there is no way around it.
> > > > 
> > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > 
> > > Yes, it continues to work as usual for PV guests.
> > > 
> > > 
> > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > devices?
> > > 
> > > VGA + PV input only works with Linux and is only useful for power
> > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > would be able to wake up less often. Given that usb emulation is still
> > > on by default, I don't think that this change will have a big impact.
> > 
> > My question was whether we thought anyone would be using this
> > non-default configuration, not what the impact on the default is.
> > 
> > You gave a good reason why people might be using this facility, do you
> > think anyone is actually using it?
>  
> I don't know of anybody using it. I don't think we made clear enough how
> to use this non-default configuration and its advantages for users to go
> out of their ways to use it. 

That's good enough for me, thanks,.

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

On Tue, 30 Jun 2015, Ian Campbell wrote:
> >  I don't think there is any need to go into the
> > possible sub-directories there.  But I'll add a note on the possible
> > privilege level change.
> > 
> > 
> > > > Add a maximum limit of physmap entries to save, so that the guest cannot
> > > > DOS the toolstack.
> > > 
> > > How will we cope when the limit needs to be increased in the future for
> > > some reason?
> > 
> > The current limit, 12, is far higher than the actual number of entries
> > that is usually just 1 (vga). The limit is not likely to increase unless
> > the devices being modelled actually change significantly. In that case
> > we'll have a way to detect it in libxl.
> 
> My concern was with some external change making it >=13. If you are
> confident that this cannot occur and any such change will always be
> under our control then I guess that's ok.

Yes, that's right, it cannot change "behind our backs".


> > The reason why I feel comfortable having such an high limit compared to
> > the actual usage, is that it is only meant to prevent the guest from
> > filling up dom0's disk, as this information is saved to file. As
> > xenstore entries have a size limit themselves, we should be pretty safe.
> 
> I'm not bothered by how high or low the limit is, just the mechanisms
> which we intend to use in order to cope when it changes, which are much
> easier to put in place now than trying to retrofit such a thing while
> retaining support for existing released versions of qemu. 
> 
> > > There is an interesting general issue here which is that we have
> > > XS_RESTRICT which changes a connection to be treated as having the
> > > permissions of a given target domain instead of the originating
> > > privileged domain.
> > > 
> > > This means that in order to reduce the privileges of one thing we have
> > > to increase the privilege of the guest itself (by granting access to
> > > those paths), which seems rather counter-intuitive.
> > > 
> > > What we really want is a new privilege type which is "read/write to
> > > connections which are _privileged_ over $domid, but not $domid itself"
> > > and for XS_RESTRICT to imply that.
> > > 
> > > Retrofitting something like that to xenstored would be tricky I suspect.
> > > 
> > > When the physmap stuff was added doing it via xenstore was convenient
> > > because we weren't concerning ourselves with this deprivileging. How
> > > that we are though perhaps we should think about whether this is still
> > > appropriate and consider using a QMP command to request the list instead
> > > for example.
> > 
> > That is true, however this doesn't mean we cannot also add a QMP command
> > in the future too, especially if we follow Wei's suggestion and we only
> > change the permissions if xs_restrict. We could also implemented a
> > different XS_RESTRICT. In other words, I don't think we have to come up
> > with the best possible solution right now, we just need to improve over
> > what we already have. I think that this is an improvement.
> 
> It's not a strict improvement though, it trading off giving the guest
> access to stuff which it never previously had a need to and which the
> toolstack consumes.

I understand that strictly speaking what you wrote is true, however I
would like to point out that the information under device-model on
xenstore is already exposed to the guest by other means (pci config
space, etc.).


> Doing something via qmp now would turn this into a no-brainer strict
> improvement rather than something which requires careful consideration
> of trade offs.

Unfortunately QMP is not available early enough at restore time.  That's
why we went for storing the physmap on xenstore in the first place. Also
it would certainly push the series past the code freeze.

Re: [PATCH v3 3/6] [WIP] libxl: xsrestrict QEMU

[Stefano Stabellini]

On Tue, 30 Jun 2015, Ian Campbell wrote:
> On Mon, 2015-06-29 at 19:07 +0100, Stefano Stabellini wrote:
> > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> > > > 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.
> > > 
> > > Is there some way we could avoid needing to do this, e.g. by doing the
> > > restrict later on via a qmp request, before the guest is unpaused of
> > > course.
> > 
> > It would be tricky because it needs to be done very early at boot time
> > in QEMU. Also we would still need to know whether a specific device
> > model supports this option before actually spawning it. So we would
> > still have to resort to spawning a "temporary" QEMU beforehand.
> 
> I think via qmp we can query the qemu after it starts to ask it if it
> has the "xs-restrict" property and then set it to a domid if so. I had
> some code to do this for the PCI permissive property we added recently,
> it wasn't hard.
> 
> If we can move all the restriction stuff in libxl until after we are in
> a position to ask qemu this then we don't need any temporary qemu or
> --help parsing.

The problem is not adding a new QMP command. The issue is that on the
QEMU side, we need to know whether to "xsrestrict" before starting the
PV backends, that is done before QMP is up.

Even if we work around that, on the libxl side, we certainly need to
know the correct device-model path on xenstore
(libxl__device_model_xs_path) in many many different places, and that
depends on "xsrestrict". I don't think is possible to move all the
restriction stuff after QEMU can reply to QMP requests.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Tue, 30 Jun 2015, Ian Campbell wrote:
> On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > > 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).
> > > > > > > > 
> > > > > > > 
> > > > > > > The question is that how would this affect the non-split setup.
> > > > > > 
> > > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > > However there is no way around it.
> > > > > 
> > > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > > 
> > > > Yes, it continues to work as usual for PV guests.
> > > > 
> > > > 
> > > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > > devices?
> > > > 
> > > > VGA + PV input only works with Linux and is only useful for power
> > > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > > would be able to wake up less often. Given that usb emulation is still
> > > > on by default, I don't think that this change will have a big impact.
> > > 
> > > My question was whether we thought anyone would be using this
> > > non-default configuration, not what the impact on the default is.
> > > 
> > > You gave a good reason why people might be using this facility, do you
> > > think anyone is actually using it?
> >  
> > I don't know of anybody using it. I don't think we made clear enough how
> > to use this non-default configuration and its advantages for users to go
> > out of their ways to use it. 
> 
> That's good enough for me, thanks,.

Can I add your acked-by?

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Ian Campbell]

On Tue, 2015-06-30 at 14:49 +0100, Stefano Stabellini wrote:
> > > The reason why I feel comfortable having such an high limit compared to
> > > the actual usage, is that it is only meant to prevent the guest from
> > > filling up dom0's disk, as this information is saved to file. As
> > > xenstore entries have a size limit themselves, we should be pretty safe.
> > 
> > I'm not bothered by how high or low the limit is, just the mechanisms
> > which we intend to use in order to cope when it changes, which are much
> > easier to put in place now than trying to retrofit such a thing while
> > retaining support for existing released versions of qemu. 
> > 
> > > > There is an interesting general issue here which is that we have
> > > > XS_RESTRICT which changes a connection to be treated as having the
> > > > permissions of a given target domain instead of the originating
> > > > privileged domain.
> > > > 
> > > > This means that in order to reduce the privileges of one thing we have
> > > > to increase the privilege of the guest itself (by granting access to
> > > > those paths), which seems rather counter-intuitive.
> > > > 
> > > > What we really want is a new privilege type which is "read/write to
> > > > connections which are _privileged_ over $domid, but not $domid itself"
> > > > and for XS_RESTRICT to imply that.
> > > > 
> > > > Retrofitting something like that to xenstored would be tricky I suspect.
> > > > 
> > > > When the physmap stuff was added doing it via xenstore was convenient
> > > > because we weren't concerning ourselves with this deprivileging. How
> > > > that we are though perhaps we should think about whether this is still
> > > > appropriate and consider using a QMP command to request the list instead
> > > > for example.
> > > 
> > > That is true, however this doesn't mean we cannot also add a QMP command
> > > in the future too, especially if we follow Wei's suggestion and we only
> > > change the permissions if xs_restrict. We could also implemented a
> > > different XS_RESTRICT. In other words, I don't think we have to come up
> > > with the best possible solution right now, we just need to improve over
> > > what we already have. I think that this is an improvement.
> > 
> > It's not a strict improvement though, it trading off giving the guest
> > access to stuff which it never previously had a need to and which the
> > toolstack consumes.
> 
> I understand that strictly speaking what you wrote is true, however I
> would like to point out that the information under device-model on
> xenstore is already exposed to the guest by other means (pci config
> space, etc.).

It's the ability of the guest to change this info into fakery that the
restorer will consume which is new here and possibly dangerous. Has the
reader of this data (both on the save side moving it into the stream and
on the restore side taking it back out) been audited for safety now that
this data is untrusted?

> > Doing something via qmp now would turn this into a no-brainer strict
> > improvement rather than something which requires careful consideration
> > of trade offs.
> 
> Unfortunately QMP is not available early enough at restore time.  That's
> why we went for storing the physmap on xenstore in the first place.

That's a shame.

> Also it would certainly push the series past the code freeze.

It seems like its not an option, but if it were I wouldn't be inclined
to let that stop us doing the right technical thing in general. There's
always another release.

Ian.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Ian Campbell]

On Tue, 2015-06-30 at 15:02 +0100, Stefano Stabellini wrote:
> On Tue, 30 Jun 2015, Ian Campbell wrote:
> > On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > > > 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).
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The question is that how would this affect the non-split setup.
> > > > > > > 
> > > > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > > > However there is no way around it.
> > > > > > 
> > > > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > > > 
> > > > > Yes, it continues to work as usual for PV guests.
> > > > > 
> > > > > 
> > > > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > > > devices?
> > > > > 
> > > > > VGA + PV input only works with Linux and is only useful for power
> > > > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > > > would be able to wake up less often. Given that usb emulation is still
> > > > > on by default, I don't think that this change will have a big impact.
> > > > 
> > > > My question was whether we thought anyone would be using this
> > > > non-default configuration, not what the impact on the default is.
> > > > 
> > > > You gave a good reason why people might be using this facility, do you
> > > > think anyone is actually using it?
> > >  
> > > I don't know of anybody using it. I don't think we made clear enough how
> > > to use this non-default configuration and its advantages for users to go
> > > out of their ways to use it. 
> > 
> > That's good enough for me, thanks,.
> 
> Can I add your acked-by?

If you put some distillation of the reasoning given in this subthread
for why we think we can get away with it into the commit message then
yes.

Ian.

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

On Tue, 30 Jun 2015, Ian Campbell wrote:
> On Tue, 2015-06-30 at 14:49 +0100, Stefano Stabellini wrote:
> > > > The reason why I feel comfortable having such an high limit compared to
> > > > the actual usage, is that it is only meant to prevent the guest from
> > > > filling up dom0's disk, as this information is saved to file. As
> > > > xenstore entries have a size limit themselves, we should be pretty safe.
> > > 
> > > I'm not bothered by how high or low the limit is, just the mechanisms
> > > which we intend to use in order to cope when it changes, which are much
> > > easier to put in place now than trying to retrofit such a thing while
> > > retaining support for existing released versions of qemu. 
> > > 
> > > > > There is an interesting general issue here which is that we have
> > > > > XS_RESTRICT which changes a connection to be treated as having the
> > > > > permissions of a given target domain instead of the originating
> > > > > privileged domain.
> > > > > 
> > > > > This means that in order to reduce the privileges of one thing we have
> > > > > to increase the privilege of the guest itself (by granting access to
> > > > > those paths), which seems rather counter-intuitive.
> > > > > 
> > > > > What we really want is a new privilege type which is "read/write to
> > > > > connections which are _privileged_ over $domid, but not $domid itself"
> > > > > and for XS_RESTRICT to imply that.
> > > > > 
> > > > > Retrofitting something like that to xenstored would be tricky I suspect.
> > > > > 
> > > > > When the physmap stuff was added doing it via xenstore was convenient
> > > > > because we weren't concerning ourselves with this deprivileging. How
> > > > > that we are though perhaps we should think about whether this is still
> > > > > appropriate and consider using a QMP command to request the list instead
> > > > > for example.
> > > > 
> > > > That is true, however this doesn't mean we cannot also add a QMP command
> > > > in the future too, especially if we follow Wei's suggestion and we only
> > > > change the permissions if xs_restrict. We could also implemented a
> > > > different XS_RESTRICT. In other words, I don't think we have to come up
> > > > with the best possible solution right now, we just need to improve over
> > > > what we already have. I think that this is an improvement.
> > > 
> > > It's not a strict improvement though, it trading off giving the guest
> > > access to stuff which it never previously had a need to and which the
> > > toolstack consumes.
> > 
> > I understand that strictly speaking what you wrote is true, however I
> > would like to point out that the information under device-model on
> > xenstore is already exposed to the guest by other means (pci config
> > space, etc.).
> 
> It's the ability of the guest to change this info into fakery that the
> restorer will consume which is new here and possibly dangerous. Has the
> reader of this data (both on the save side moving it into the stream and
> on the restore side taking it back out) been audited for safety now that
> this data is untrusted?

When I made this change, I gave a careful look both at the libxl side
and the QEMU side. Indeed I would appreciate a second pair of eyes on
this. These are my observations:

tools/libxl/libxl_dom.c:libxl__toolstack_save is the crux.
It calls:

- libxl__xs_directory on /physmap
this is safe

- libxl__xs_read
if the path is wrong (nothing is there), it returns NULL, and it is
handled correctly

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

So in case of errors:

1) libxl__toolstack_save could return early with an error, if the
xenstore paths are wrong (nothing is on xenstore)
2) libxl__toolstack_restore could write back to xenstore any unsigned
long long, including LLONG_MIN or LLONG_MAX

Either way we are fine.


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

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Konrad Rzeszutek Wilk]

On Tue, Jun 30, 2015 at 03:13:53PM +0100, Ian Campbell wrote:
> On Tue, 2015-06-30 at 15:02 +0100, Stefano Stabellini wrote:
> > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> > > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > > > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > > > > 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).
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > The question is that how would this affect the non-split setup.
> > > > > > > > 
> > > > > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > > > > However there is no way around it.
> > > > > > > 
> > > > > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > > > > 
> > > > > > Yes, it continues to work as usual for PV guests.
> > > > > > 
> > > > > > 
> > > > > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > > > > devices?
> > > > > > 
> > > > > > VGA + PV input only works with Linux and is only useful for power
> > > > > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > > > > would be able to wake up less often. Given that usb emulation is still
> > > > > > on by default, I don't think that this change will have a big impact.
> > > > > 
> > > > > My question was whether we thought anyone would be using this
> > > > > non-default configuration, not what the impact on the default is.
> > > > > 
> > > > > You gave a good reason why people might be using this facility, do you
> > > > > think anyone is actually using it?
> > > >  
> > > > I don't know of anybody using it. I don't think we made clear enough how
> > > > to use this non-default configuration and its advantages for users to go
> > > > out of their ways to use it. 
> > > 
> > > That's good enough for me, thanks,.
> > 
> > Can I add your acked-by?
> 
> If you put some distillation of the reasoning given in this subthread
> for why we think we can get away with it into the commit message then
> yes.

Why don't we also make the Linux code not expose this driver for HVM guests?

I've had an go for this last year (can't find the link) as it would unduly
cause the Linux kernel to take an extra 30 seconds to boot. That is because
'xend' by default exposes the PV configuration even for HVM guests - and of
course there are no PV drivers (as the VGA in QEMU is enabled).

The only use case I had was for ARM - where there are no VGA - and the
patch I think I had just disabled the xen-fbfront under X86 HVM.

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

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Tue, 30 Jun 2015, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 30, 2015 at 03:13:53PM +0100, Ian Campbell wrote:
> > On Tue, 2015-06-30 at 15:02 +0100, Stefano Stabellini wrote:
> > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> > > > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > > > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > > > > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > > > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > > > > > 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).
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > The question is that how would this affect the non-split setup.
> > > > > > > > > 
> > > > > > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > > > > > However there is no way around it.
> > > > > > > > 
> > > > > > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > > > > > 
> > > > > > > Yes, it continues to work as usual for PV guests.
> > > > > > > 
> > > > > > > 
> > > > > > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > > > > > devices?
> > > > > > > 
> > > > > > > VGA + PV input only works with Linux and is only useful for power
> > > > > > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > > > > > would be able to wake up less often. Given that usb emulation is still
> > > > > > > on by default, I don't think that this change will have a big impact.
> > > > > > 
> > > > > > My question was whether we thought anyone would be using this
> > > > > > non-default configuration, not what the impact on the default is.
> > > > > > 
> > > > > > You gave a good reason why people might be using this facility, do you
> > > > > > think anyone is actually using it?
> > > > >  
> > > > > I don't know of anybody using it. I don't think we made clear enough how
> > > > > to use this non-default configuration and its advantages for users to go
> > > > > out of their ways to use it. 
> > > > 
> > > > That's good enough for me, thanks,.
> > > 
> > > Can I add your acked-by?
> > 
> > If you put some distillation of the reasoning given in this subthread
> > for why we think we can get away with it into the commit message then
> > yes.
> 
> Why don't we also make the Linux code not expose this driver for HVM guests?
> 
> I've had an go for this last year (can't find the link) as it would unduly
> cause the Linux kernel to take an extra 30 seconds to boot. That is because
> 'xend' by default exposes the PV configuration even for HVM guests - and of
> course there are no PV drivers (as the VGA in QEMU is enabled).

But even with xend it only happens with a vfb line is in the config
file, right? That's why we didn't fix it back when the issue was
reported, if I remember correctly.


> The only use case I had was for ARM - where there are no VGA - and the
> patch I think I had just disabled the xen-fbfront under X86 HVM.

Yeah, we need xen-fbfront for ARM.


Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
be opposed to stop the driver initialization in Linux on x86/HVM. Unless
Roger's work on HVMlite is going to need xen-fbfront again, but in that
case we'll be able to distinguish a regular HVM guest from an HVMlite
guest, I think.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Roger Pau Monné]

El 01/07/15 a les 12.29, Stefano Stabellini ha escrit:
> Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
> be opposed to stop the driver initialization in Linux on x86/HVM. Unless
> Roger's work on HVMlite is going to need xen-fbfront again, but in that
> case we'll be able to distinguish a regular HVM guest from an HVMlite
> guest, I think.

I haven't get to that point yet, but yes, it seems useful for HVMlite
guests because we won't have an emulated VGA card any more.

HVMlite guest can be easily identified because they set the device model
to none:

device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE

For HVMlite.

Roger.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On Wed, 1 Jul 2015, Roger Pau Monné wrote:
> El 01/07/15 a les 12.29, Stefano Stabellini ha escrit:
> > Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
> > be opposed to stop the driver initialization in Linux on x86/HVM. Unless
> > Roger's work on HVMlite is going to need xen-fbfront again, but in that
> > case we'll be able to distinguish a regular HVM guest from an HVMlite
> > guest, I think.
>
> I haven't get to that point yet, but yes, it seems useful for HVMlite
> guests because we won't have an emulated VGA card any more.
>
> HVMlite guest can be easily identified because they set the device model
> to none:
>
> device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE

What about the guest side? Are they going to be hvm guests from Linux
POV? Or something else?

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Fabio Fantoni]

Il 01/07/2015 12:55, Roger Pau Monné ha scritto:
> El 01/07/15 a les 12.29, Stefano Stabellini ha escrit:
>> Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
>> be opposed to stop the driver initialization in Linux on x86/HVM. Unless
>> Roger's work on HVMlite is going to need xen-fbfront again, but in that
>> case we'll be able to distinguish a regular HVM guest from an HVMlite
>> guest, I think.
> I haven't get to that point yet, but yes, it seems useful for HVMlite
> guests because we won't have an emulated VGA card any more.

If you don't want have emulated vga you can already do it fast in xl 
cfg: vga="none"
Note: emulated vga is used by spice and vnc of hvm domUs, without 
connecting with them you'll have black screen.

>
> HVMlite guest can be easily identified because they set the device model
> to none:
>
> device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE
>
> For HVMlite.
>
> Roger.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Roger Pau Monné]

El 01/07/15 a les 12.56, Stefano Stabellini ha escrit:
> On Wed, 1 Jul 2015, Roger Pau Monné wrote:
>> El 01/07/15 a les 12.29, Stefano Stabellini ha escrit:
>>> Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
>>> be opposed to stop the driver initialization in Linux on x86/HVM. Unless
>>> Roger's work on HVMlite is going to need xen-fbfront again, but in that
>>> case we'll be able to distinguish a regular HVM guest from an HVMlite
>>> guest, I think.
>>
>> I haven't get to that point yet, but yes, it seems useful for HVMlite
>> guests because we won't have an emulated VGA card any more.
>>
>> HVMlite guest can be easily identified because they set the device model
>> to none:
>>
>> device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE
> 
> What about the guest side? Are they going to be hvm guests from Linux
> POV? Or something else?

That's not set on stone, they could certainly be HVM guests depending on
the implementation that each OS has.

IMHO from a guest POV the interface exposed is very similar to PVH, so
in order to ease the implementation I think they should be considered
PVH guests from Linux POV, we can always move them to being HVM guests
later if needed.

Roger.


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

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Konrad Rzeszutek Wilk]

On Wed, Jul 01, 2015 at 11:29:46AM +0100, Stefano Stabellini wrote:
> On Tue, 30 Jun 2015, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jun 30, 2015 at 03:13:53PM +0100, Ian Campbell wrote:
> > > On Tue, 2015-06-30 at 15:02 +0100, Stefano Stabellini wrote:
> > > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > > On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> > > > > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > > > > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > > > > > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > > > > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > > > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > > > > > > 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).
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > The question is that how would this affect the non-split setup.
> > > > > > > > > > 
> > > > > > > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > > > > > > However there is no way around it.
> > > > > > > > > 
> > > > > > > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > > > > > > 
> > > > > > > > Yes, it continues to work as usual for PV guests.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > > > > > > devices?
> > > > > > > > 
> > > > > > > > VGA + PV input only works with Linux and is only useful for power
> > > > > > > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > > > > > > would be able to wake up less often. Given that usb emulation is still
> > > > > > > > on by default, I don't think that this change will have a big impact.
> > > > > > > 
> > > > > > > My question was whether we thought anyone would be using this
> > > > > > > non-default configuration, not what the impact on the default is.
> > > > > > > 
> > > > > > > You gave a good reason why people might be using this facility, do you
> > > > > > > think anyone is actually using it?
> > > > > >  
> > > > > > I don't know of anybody using it. I don't think we made clear enough how
> > > > > > to use this non-default configuration and its advantages for users to go
> > > > > > out of their ways to use it. 
> > > > > 
> > > > > That's good enough for me, thanks,.
> > > > 
> > > > Can I add your acked-by?
> > > 
> > > If you put some distillation of the reasoning given in this subthread
> > > for why we think we can get away with it into the commit message then
> > > yes.
> > 
> > Why don't we also make the Linux code not expose this driver for HVM guests?
> > 
> > I've had an go for this last year (can't find the link) as it would unduly

And the link:

http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg00160.html
> > cause the Linux kernel to take an extra 30 seconds to boot. That is because
> > 'xend' by default exposes the PV configuration even for HVM guests - and of
> > course there are no PV drivers (as the VGA in QEMU is enabled).
> 
> But even with xend it only happens with a vfb line is in the config
> file, right? That's why we didn't fix it back when the issue was
> reported, if I remember correctly.

Both. If you had 'vnc' or 'vfb' it would setup the 'vfb' key.
> 
> 
> > The only use case I had was for ARM - where there are no VGA - and the
> > patch I think I had just disabled the xen-fbfront under X86 HVM.
> 
> Yeah, we need xen-fbfront for ARM.
> 
> 
> Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
> be opposed to stop the driver initialization in Linux on x86/HVM. Unless
> Roger's work on HVMlite is going to need xen-fbfront again, but in that
> case we'll be able to distinguish a regular HVM guest from an HVMlite
> guest, I think.

Correct. Right now the 'xen_pvh_domain' is based on it being PV and auto-translate.
That whole thing will need some help.


But I am looking at the xen-fbfront.c driver and it might be that
I had already fixed this issue! (inadvertly it seems)

51c71a3bbaca868043cc45b3ad3786dd48a90235
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Tue Nov 26 15:05:40 2013 -0500

    xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v4).

..
   - if running in HVM, check if user wanted 'xen_emul_unplug=never',
       in which case bail out and don't load any PV drivers.
     - if running in HVM, and if PCI device 5853:0001 (xen_platform_pci)
       does not exist, then bail out and not load PV drivers.
     - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=ide-disks',
       then bail out for all PV devices _except_ the block one.
       Ditto for the network one ('nics').
     - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=unnecessary'
       then load block PV driver, and also setup the legacy IDE paths.
       In (v3) make it actually load PV drivers.

.. which means that if the driver does not use the 'xen_has_pv_XXX_devices'
but only the 'xen_has_pv_devices' then for a normal HVM guest it won't load
it.

And sure enough we have:

+       if (!xen_has_pv_devices())
+               return -ENODEV;

so we bail out and not load it under HVM.

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Wed, 1 Jul 2015, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 01, 2015 at 11:29:46AM +0100, Stefano Stabellini wrote:
> > On Tue, 30 Jun 2015, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Jun 30, 2015 at 03:13:53PM +0100, Ian Campbell wrote:
> > > > On Tue, 2015-06-30 at 15:02 +0100, Stefano Stabellini wrote:
> > > > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > > > On Tue, 2015-06-30 at 12:21 +0100, Stefano Stabellini wrote:
> > > > > > > On Tue, 30 Jun 2015, Ian Campbell wrote:
> > > > > > > > On Mon, 2015-06-29 at 18:59 +0100, Stefano Stabellini wrote:
> > > > > > > > > On Thu, 25 Jun 2015, Ian Campbell wrote:
> > > > > > > > > > On Tue, 2015-06-16 at 16:39 +0100, Stefano Stabellini wrote:
> > > > > > > > > > > On Tue, 16 Jun 2015, Wei Liu wrote:
> > > > > > > > > > > > On Wed, Jun 10, 2015 at 11:09:50AM +0100, Stefano Stabellini wrote:
> > > > > > > > > > > > > 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).
> > > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > The question is that how would this affect the non-split setup.
> > > > > > > > > > > 
> > > > > > > > > > > vkb is useful because emulating usb forces QEMU to wake up more often.
> > > > > > > > > > > However there is no way around it.
> > > > > > > > > > 
> > > > > > > > > > Does pvfb+vkb continue to work due to code somewhere else?
> > > > > > > > > 
> > > > > > > > > Yes, it continues to work as usual for PV guests.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > Do we think anyone will actually be using emulated VGA + PV input
> > > > > > > > > > devices?
> > > > > > > > > 
> > > > > > > > > VGA + PV input only works with Linux and is only useful for power
> > > > > > > > > efficiency, because if you disable usb emulation in QEMU, then QEMU
> > > > > > > > > would be able to wake up less often. Given that usb emulation is still
> > > > > > > > > on by default, I don't think that this change will have a big impact.
> > > > > > > > 
> > > > > > > > My question was whether we thought anyone would be using this
> > > > > > > > non-default configuration, not what the impact on the default is.
> > > > > > > > 
> > > > > > > > You gave a good reason why people might be using this facility, do you
> > > > > > > > think anyone is actually using it?
> > > > > > >  
> > > > > > > I don't know of anybody using it. I don't think we made clear enough how
> > > > > > > to use this non-default configuration and its advantages for users to go
> > > > > > > out of their ways to use it. 
> > > > > > 
> > > > > > That's good enough for me, thanks,.
> > > > > 
> > > > > Can I add your acked-by?
> > > > 
> > > > If you put some distillation of the reasoning given in this subthread
> > > > for why we think we can get away with it into the commit message then
> > > > yes.
> > > 
> > > Why don't we also make the Linux code not expose this driver for HVM guests?
> > > 
> > > I've had an go for this last year (can't find the link) as it would unduly
> 
> And the link:
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg00160.html
> > > cause the Linux kernel to take an extra 30 seconds to boot. That is because
> > > 'xend' by default exposes the PV configuration even for HVM guests - and of
> > > course there are no PV drivers (as the VGA in QEMU is enabled).
> > 
> > But even with xend it only happens with a vfb line is in the config
> > file, right? That's why we didn't fix it back when the issue was
> > reported, if I remember correctly.
> 
> Both. If you had 'vnc' or 'vfb' it would setup the 'vfb' key.
> > 
> > 
> > > The only use case I had was for ARM - where there are no VGA - and the
> > > patch I think I had just disabled the xen-fbfront under X86 HVM.
> > 
> > Yeah, we need xen-fbfront for ARM.
> > 
> > 
> > Given that xen-fbfront is likely to go away for HVM guests, I wouldn't
> > be opposed to stop the driver initialization in Linux on x86/HVM. Unless
> > Roger's work on HVMlite is going to need xen-fbfront again, but in that
> > case we'll be able to distinguish a regular HVM guest from an HVMlite
> > guest, I think.
> 
> Correct. Right now the 'xen_pvh_domain' is based on it being PV and auto-translate.
> That whole thing will need some help.
> 
> 
> But I am looking at the xen-fbfront.c driver and it might be that
> I had already fixed this issue! (inadvertly it seems)
> 
> 51c71a3bbaca868043cc45b3ad3786dd48a90235
> Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date:   Tue Nov 26 15:05:40 2013 -0500
> 
>     xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v4).
> 
> ..
>    - if running in HVM, check if user wanted 'xen_emul_unplug=never',
>        in which case bail out and don't load any PV drivers.
>      - if running in HVM, and if PCI device 5853:0001 (xen_platform_pci)
>        does not exist, then bail out and not load PV drivers.
>      - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=ide-disks',
>        then bail out for all PV devices _except_ the block one.
>        Ditto for the network one ('nics').
>      - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=unnecessary'
>        then load block PV driver, and also setup the legacy IDE paths.
>        In (v3) make it actually load PV drivers.
> 
> .. which means that if the driver does not use the 'xen_has_pv_XXX_devices'
> but only the 'xen_has_pv_devices' then for a normal HVM guest it won't load
> it.
> 
> And sure enough we have:
> 
> +       if (!xen_has_pv_devices())
> +               return -ENODEV;
> 
> so we bail out and not load it under HVM.
 
And at the same time it works on ARM because CONFIG_XEN_PVHVM is not
defined there, right?

Re: [PATCH v3 2/6] libxl: do not add a vkb backend to hvm guests

[Konrad Rzeszutek Wilk]

> > But I am looking at the xen-fbfront.c driver and it might be that
> > I had already fixed this issue! (inadvertly it seems)
> > 
> > 51c71a3bbaca868043cc45b3ad3786dd48a90235
> > Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date:   Tue Nov 26 15:05:40 2013 -0500
> > 
> >     xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v4).
> > 
> > ..
> >    - if running in HVM, check if user wanted 'xen_emul_unplug=never',
> >        in which case bail out and don't load any PV drivers.
> >      - if running in HVM, and if PCI device 5853:0001 (xen_platform_pci)
> >        does not exist, then bail out and not load PV drivers.
> >      - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=ide-disks',
> >        then bail out for all PV devices _except_ the block one.
> >        Ditto for the network one ('nics').
> >      - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=unnecessary'
> >        then load block PV driver, and also setup the legacy IDE paths.
> >        In (v3) make it actually load PV drivers.
> > 
> > .. which means that if the driver does not use the 'xen_has_pv_XXX_devices'
> > but only the 'xen_has_pv_devices' then for a normal HVM guest it won't load
> > it.
> > 
> > And sure enough we have:
> > 
> > +       if (!xen_has_pv_devices())
> > +               return -ENODEV;
> > 
> > so we bail out and not load it under HVM.
>  
> And at the same time it works on ARM because CONFIG_XEN_PVHVM is not
> defined there, right?

Yup, and it ends up doing:

static inline bool xen_has_pv_devices(void)                                     
{                                                                               
        return IS_ENABLED(CONFIG_XEN);                                          
}                      

which will return true if CONFIG_XEN is set.

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Ian Campbell]

On Tue, 2015-06-30 at 16:00 +0100, Stefano Stabellini wrote:
> When I made this change, I gave a careful look both at the libxl side
> and the QEMU side. Indeed I would appreciate a second pair of eyes on
> this. These are my observations:

I think the request for secondary close review and more importantly
these observations (which == a rationale about why this is safe) deserve
top billing in the commit message.

IMO any commit which deliberately relaxes some security cordon should
have as much information and analysis as possible about why this is OK
to do prominently in the logs. Both so that we can tell that due
diligence has been done and so that we can look back in six months and
see what we thought the security properties of this change were.

> tools/libxl/libxl_dom.c:libxl__toolstack_save is the crux.
> It calls:
> 
> - libxl__xs_directory on /physmap
> this is safe
> 
> - libxl__xs_read
> if the path is wrong (nothing is there), it returns NULL, and it is
> handled correctly
> 
> - strtoll on the values read
> The values are under guest control but strtoll can handle bad formats.
> strtoll always returns an unsigned long long. In case of errors, it can
> return LLONG_MIN or LLONG_MAX.  libxl__toolstack_save doesn't check for
> conversion errors, but it never uses the returned values anywhere.
> That's OK.  tools/libxl/libxl_dom.c:libxl__toolstack_restore writes back
> the values to xenstore.
> 
> So in case of errors:
> 
> 1) libxl__toolstack_save could return early with an error, if the
> xenstore paths are wrong (nothing is on xenstore)
> 2) libxl__toolstack_restore could write back to xenstore any unsigned
> long long, including LLONG_MIN or LLONG_MAX
> 
> Either way we are fine.
> 
> 
> From the QEMU side, the code is fairly similar and uses strtoll to parse
> the entries. The values are used to avoid memory allocations at restore
> time for memory that has already been allocated (video memory). If the
> values are wrong, QEMU will attempt another memory allocation, failing
> because the maxmem limit has been reached (the infamous maxmem increase
> commit has been reverted).

Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

On Fri, 3 Jul 2015, Ian Campbell wrote:
> On Tue, 2015-06-30 at 16:00 +0100, Stefano Stabellini wrote:
> > When I made this change, I gave a careful look both at the libxl side
> > and the QEMU side. Indeed I would appreciate a second pair of eyes on
> > this. These are my observations:
> 
> I think the request for secondary close review and more importantly
> these observations (which == a rationale about why this is safe) deserve
> top billing in the commit message.
> 
> IMO any commit which deliberately relaxes some security cordon should
> have as much information and analysis as possible about why this is OK
> to do prominently in the logs. Both so that we can tell that due
> diligence has been done and so that we can look back in six months and
> see what we thought the security properties of this change were.

You are right. I write all this down.


> > tools/libxl/libxl_dom.c:libxl__toolstack_save is the crux.
> > It calls:
> > 
> > - libxl__xs_directory on /physmap
> > this is safe
> > 
> > - libxl__xs_read
> > if the path is wrong (nothing is there), it returns NULL, and it is
> > handled correctly
> > 
> > - strtoll on the values read
> > The values are under guest control but strtoll can handle bad formats.
> > strtoll always returns an unsigned long long. In case of errors, it can
> > return LLONG_MIN or LLONG_MAX.  libxl__toolstack_save doesn't check for
> > conversion errors, but it never uses the returned values anywhere.
> > That's OK.  tools/libxl/libxl_dom.c:libxl__toolstack_restore writes back
> > the values to xenstore.
> > 
> > So in case of errors:
> > 
> > 1) libxl__toolstack_save could return early with an error, if the
> > xenstore paths are wrong (nothing is on xenstore)
> > 2) libxl__toolstack_restore could write back to xenstore any unsigned
> > long long, including LLONG_MIN or LLONG_MAX
> > 
> > Either way we are fine.
> > 
> > 
> > From the QEMU side, the code is fairly similar and uses strtoll to parse
> > the entries. The values are used to avoid memory allocations at restore
> > time for memory that has already been allocated (video memory). If the
> > values are wrong, QEMU will attempt another memory allocation, failing
> > because the maxmem limit has been reached (the infamous maxmem increase
> > commit has been reverted).
> 
>