[RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

General idea is to allow freely set device_model_version and
device_model_stubdomain_override and choose the right options based on this choice.
Also, allow to specific path to stubdomain kernel/ramdisk, for greater flexibility.
Right now when qemu-xen in stubdomain is selected, it is assumed it's
Linux-based and few decisions are based on it, specifically:
 - qemu args encoding (\x1b as separator, to allow spaces in arguments)
 - save/restore stream access (specific FDs passed to qemu by a wrapper script)
 - QMP access, if any

It may be a good idea to document "stubdomain API" somewhere. Is there such
document for MiniOS one? Here is an initial version for Linux one:

    Assumptions about Linux-based stubdomain for qemu-xen:
    * qemu command line is stored by toolstack in
      /vm/<target-uuid>/image/dmargs xenstore entry, arguments are separated
      with \x1b character
    * qemu can access saved state (if any) at its FD 3
    * qemu can write its state (for save/migration) to its FD 4
    * qemu expects backend for serial console at /dev/hvc3
    * disks configured for the target are available as /dev/xvd*, in
      configuration order
    * qemu can call /etc/qemu-ifup and /etc/qemu-ifdown to connect/disconnect
      network interface to appropriate network

Initial version has no QMP support - in initial patches it is completely
disabled, which means no suspend/restore. QMP normally would be used for PCI
passthrough setup, but it is worked around with MiniOS-like control protocol
over xenstore, which then is translated to QMP on stubdomain side.
Some option is to use separate console for that, but that require
xenstoled supporting multiple consoles per domain (the goal is to not have qemu
in dom0 at all). Also, it would be preferable to evaluate how libxl
handle potentially malicious QMP responses.
Another option is to use xenstore - either translate everything needed to
MiniOS-like thing, or write already json-formatted requests to xenstore and
watch there for responses. When using separate xenstore dir for that, with
per-command entries (command id as a key name?) that would solve concurrency
problem.

QMP support over separate console: patch "libxl: access QMP socket via console
for qemu-in-stubdomain" implements some early PoC of this.
Major limitation: only one connection at a time and no means of out of
band reset (and re-negotiate). I've tried adding very simple `qmp_reset`
command for in-band connection reset, but it is unreliable because of the
first limitation - xl process running in background hold this connection open
and every other xl instance interfere with it. On the other hand, for libvirt
use case one connection could be enough (leaving alone libvirtd restart).

Xenconsoled patches add support for multiple consoles in xenconsoled, to avoid the need
for qemu in dom0 for this to work. Multiple consoles for a stubdomain are used for:
 - logs (console 0)
 - save + restore (console 1, 2)
 - qmp (console 3)
 - serial terminal to target domU (console 4)
Xenconsoled patches are in fact a separate series, but I'm sending them here to
ease dependencies handling (latter libxl patches use that).

What qmp-libxenstat socket is for?

PCI passthrough support require some more love. Right now, libxl tries to setup
pcifront for both target HVM and stubdomain and the former fails (race
condition):
    xen-pciback pci-259-0: 22 Couldn't locate PCI device (0000:00:1b.0)! perhaps already in-use?
Fortunately it isn't needed. There is also a PCI related problem on
domain shutdown - it looks like first stubdomain is paused and then libxl waits
for pcifront there.
Also, MSI doesn't work (qemu output):

    [00:05.0] xen_pt_msgctrl_reg_write: setup MSI (register: 81).
    [00:05.0] msi_msix_setup: requested pirq 55 for MSI (vec: 0, entry: 0)
    [00:05.0] msi_msix_setup: Error: Mapping of MSI (err: 1, vec: 0, entry 0)
    [00:05.0] xen_pt_msgctrl_reg_write: Warning: Can not map MSI (register: 80)!
    [00:05.0] Write-back to unknown field 0x44 (partially) inhibited (0x00)
    [00:05.0] If the device doesn't work, try enabling permissive mode
    [00:05.0] (unsafe) and if it helps report the problem to xen-devel

The actual stubdomain implementation is here:

    https://github.com/marmarek/qubes-vmm-xen-stubdom-linux (branch for-upstream)

See readme there for build instructions.

Remaining parts for eliminating dom0's instance of qemu:
 - do not force QDISK backend for CDROM

This patch series is third (fourth?) attempt to get rid of limitation
"if you want to use stubdomain, you're stuck with qemu-traditional", done over
years, by many people.
I think it should be acceptable plan to gradually add features to
qemu-xen+stubdomain configuration - not necessary waiting with committing any
of those patches until full feature set of qemu-xen is supported. After all,
right now "feature set supported by qemu-xen+stubdom" is empty, so wouldn't be
worse.

Changes in v2:
 - apply review comments by Jason Andryuk

Cc: Simon Gaiser <simon@invisiblethingslab.com>
Cc: Eric Shelton <eshelton@pobox.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

Eric Shelton (3):
  libxl: Add "stubdomain_version" to domain_build_info.
  libxl: Handle Linux stubdomain specific QEMU options.
  libxl: Build the domain with a Linux based stubdomain

Marek Marczykowski-Górecki (13):
  libxl: fix qemu-trad cmdline for no sdl/vnc case
  libxl: create vkb device only for guests with graphics output
  libxl: add save/restore support for qemu-xen in stubdomain
  xl: add stubdomain related options to xl config parser
  libxl: use \x1b to separate qemu arguments for linux stubdomain
  xenconsoled: install xenstore watch for all supported consoles
  xenconsoled: add support for consoles using 'state' xenstore entry
  xenconsoled: make console_type->use_gnttab less confusing
  xenconsoled: add support for up to 3 secondary consoles
  xenconsoled: deduplicate error handling
  xenconsoled: add support for non-pty output
  libxl: access QMP socket via console for qemu-in-stubdomain
  libxl: use xenconsoled even for multiple stubdomain's consoles

Simon Gaiser (1):
  libxl: use xenstore for pci hotplug qemu-in-linux-stubdom commands

 docs/man/xl.cfg.pod.5.in        |  23 ++-
 tools/console/daemon/io.c       | 222 ++++++++++++++++++++++++++++-----
 tools/libxl/libxl_create.c      |  83 ++++++++++--
 tools/libxl/libxl_dm.c          | 190 +++++++++++++++++++---------
 tools/libxl/libxl_dom_suspend.c |  10 +-
 tools/libxl/libxl_internal.c    |  22 +++-
 tools/libxl/libxl_internal.h    |   9 +-
 tools/libxl/libxl_mem.c         |   6 +-
 tools/libxl/libxl_pci.c         |  22 ++-
 tools/libxl/libxl_qmp.c         |  52 +++++++-
 tools/libxl/libxl_types.idl     |  10 +-
 tools/xl/xl_parse.c             |   7 +-
 12 files changed, 546 insertions(+), 110 deletions(-)

base-commit: e752f28409678ce3ade49986b39309178fb2a6d6
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 01/17] libxl: fix qemu-trad cmdline for no sdl/vnc case

[Marek Marczykowski-Górecki]

When qemu is running in stubdomain, any attempt to initialize vnc/sdl
there will crash it (on failed attempt to load a keymap from a file). If
vfb is present, all those cases are skipped. But since
b053f0c4c9e533f3d97837cf897eb920b8355ed3 "libxl: do not start dom0 qemu
for stubdomain when not needed" it is possible to create a stubdomain
without vfb and contrary to the comment -vnc none do trigger VNC
initialization code (just skips exposing it externally).
Change the implicit SDL avoiding method to -nographics option, used when
none of SDL or VNC is enabled.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v2:
 - typo in qemu option
---
 tools/libxl/libxl_dm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fdd7fa3..ebe8e0c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -475,14 +475,14 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
         if (libxl_defbool_val(vnc->findunused)) {
             flexarray_append(dm_args, "-vncunused");
         }
-    } else
+    } else if (!sdl)
         /*
          * VNC is not enabled by default by qemu-xen-traditional,
-         * however passing -vnc none causes SDL to not be
-         * (unexpectedly) enabled by default. This is overridden by
-         * explicitly passing -sdl below as required.
+         * however skipping -vnc causes SDL to be
+         * (unexpectedly) enabled by default. If undesired, disable graphics at
+         * all.
          */
-        flexarray_append_pair(dm_args, "-vnc", "none");
+        flexarray_append(dm_args, "-nographic");
 
     if (sdl) {
         flexarray_append(dm_args, "-sdl");
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info.

[Marek Marczykowski-Górecki]

From: Eric Shelton <eshelton@pobox.com>

This enum gives the ability to select between a MiniOS-based QEMU
traditional stub domain and a Linux-based QEMU upstream stub domain.  To
use the Linux-based stubdomain, the following two lines should be
included in the appropriate xl.cfg file:

device_model_version="qemu-xen"
device_model_stubdomain_override=1

To use the MiniOS-based stubdomain, the following is used instead:

device_model_version="qemu-xen-traditional"
device_model_stubdomain_override=1

Signed-off-by: Eric Shelton <eshelton@pobox.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libxl/libxl_create.c  | 34 +++++++++++++++++++++++++++++-----
 tools/libxl/libxl_types.idl |  7 +++++++
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1ccb3e3..1d92b5f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -159,12 +159,36 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     }
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
-        b_info->device_model_version !=
-            LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
         libxl_defbool_val(b_info->device_model_stubdomain)) {
-        LOG(ERROR,
-            "device model stubdomains require \"qemu-xen-traditional\"");
-        return ERROR_INVAL;
+        if (!b_info->stubdomain_version) {
+            switch (b_info->device_model_version) {
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                b_info->stubdomain_version = LIBXL_STUBDOMAIN_VERSION_MINIOS;
+                break;
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                b_info->stubdomain_version = LIBXL_STUBDOMAIN_VERSION_LINUX;
+                break;
+            default: abort();
+            }
+        }
+
+        switch (b_info->device_model_version) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            if (b_info->stubdomain_version != LIBXL_STUBDOMAIN_VERSION_MINIOS) {
+                LOG(ERROR,
+                    "\"qemu-xen-traditional\" require \"minios\" as stubdomain");
+                return ERROR_INVAL;
+            }
+            break;
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            if (b_info->stubdomain_version != LIBXL_STUBDOMAIN_VERSION_LINUX) {
+                LOG(ERROR,
+                    "\"qemu-xen\" require \"linux\" as stubdomain");
+                return ERROR_INVAL;
+            }
+            break;
+        default: abort();
+        }
     }
 
     if (!b_info->max_vcpus)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 4a38580..847e2fe 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -101,6 +101,12 @@ libxl_device_model_version = Enumeration("device_model_version", [
     (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
     ])
 
+# Give the kernel running in the stub-domain
+libxl_stubdomain_version = Enumeration("stubdomain_version", [
+    (1, "MINIOS"),
+    (2, "LINUX"),
+    ])
+
 libxl_console_type = Enumeration("console_type", [
     (0, "UNKNOWN"),
     (1, "SERIAL"),
@@ -491,6 +497,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
+    ("stubdomain_version", libxl_stubdomain_version),
     # if you set device_model you must set device_model_version too
     ("device_model",     string),
     ("device_model_ssidref", uint32),
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Marek Marczykowski-Górecki]

From: Eric Shelton <eshelton@pobox.com>

This patch creates an appropriate command line for the QEMU instance
running in a Linux-based stubdomain.

NOTE: a number of items are not currently implemented for Linux-based
stubdomains, such as:
- save/restore
- QMP socket
- graphics output (e.g., VNC)

Signed-off-by: Eric Shelton <eshelton@pobox.com>

Simon:
 * fix disk path
 * fix cdrom path and "format"
 * pass downscript for network interfaces

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
[drop Qubes-specific parts]
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
 - fix serial specified with serial=[ ... ] syntax
 - error out on multiple consoles (incompatible with stubdom)
 - drop erroneous chunk about cdrom
---
 tools/libxl/libxl_dm.c | 114 +++++++++++++++++++++++++++++-------------
 1 file changed, 81 insertions(+), 33 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ebe8e0c..82ff490 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -25,9 +25,24 @@
 #include <pwd.h>
 #include <grp.h>
 
-static const char *libxl_tapif_script(libxl__gc *gc)
+static const char *libxl_tapif_script(libxl__gc *gc,
+                                      const libxl_domain_build_info *info)
 {
 #if defined(__linux__) || defined(__FreeBSD__)
+    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
+        return libxl__sprintf(gc, "/etc/qemu-ifup");
+    return libxl__strdup(gc, "no");
+#else
+    return GCSPRINTF("%s/qemu-ifup", libxl__xen_script_dir_path());
+#endif
+}
+
+static const char *libxl_tapif_downscript(libxl__gc *gc,
+                                          const libxl_domain_build_info *info)
+{
+#if defined(__linux__) || defined(__FreeBSD__)
+    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
+        return libxl__sprintf(gc, "/etc/qemu-ifdown");
     return libxl__strdup(gc, "no");
 #else
     return GCSPRINTF("%s/qemu-ifup", libxl__xen_script_dir_path());
@@ -616,8 +631,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
                                       "tap,vlan=%d,ifname=%s,bridge=%s,"
                                       "script=%s,downscript=%s",
                                       nics[i].devid, ifname, nics[i].bridge,
-                                      libxl_tapif_script(gc),
-                                      libxl_tapif_script(gc)),
+                                      libxl_tapif_script(gc, b_info),
+                                      libxl_tapif_downscript(gc, b_info)),
                                   NULL);
                 ioemu_nics++;
             }
@@ -933,6 +948,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     const char *path, *chardev;
     char *user = NULL;
     struct passwd *user_base, user_pwbuf;
+    bool is_stubdom = libxl_defbool_val(b_info->device_model_stubdomain);
 
     dm_args = flexarray_make(gc, 16, 1);
     dm_envs = flexarray_make(gc, 16, 1);
@@ -943,24 +959,27 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                       "-xen-domid",
                       GCSPRINTF("%d", guest_domid), NULL);
 
-    flexarray_append(dm_args, "-chardev");
-    flexarray_append(dm_args,
-                     GCSPRINTF("socket,id=libxl-cmd,"
-                                    "path=%s/qmp-libxl-%d,server,nowait",
-                                    libxl__run_dir_path(), guest_domid));
+    /* There is currently no way to access the QMP socket in the stubdom */
+    if (!is_stubdom) {
+        flexarray_append(dm_args, "-chardev");
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxl-cmd,"
+                                        "path=%s/qmp-libxl-%d,server,nowait",
+                                        libxl__run_dir_path(), guest_domid));
 
-    flexarray_append(dm_args, "-no-shutdown");
-    flexarray_append(dm_args, "-mon");
-    flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
+        flexarray_append(dm_args, "-no-shutdown");
+        flexarray_append(dm_args, "-mon");
+        flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
 
-    flexarray_append(dm_args, "-chardev");
-    flexarray_append(dm_args,
-                     GCSPRINTF("socket,id=libxenstat-cmd,"
-                                    "path=%s/qmp-libxenstat-%d,server,nowait",
-                                    libxl__run_dir_path(), guest_domid));
+        flexarray_append(dm_args, "-chardev");
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxenstat-cmd,"
+                                        "path=%s/qmp-libxenstat-%d,server,nowait",
+                                        libxl__run_dir_path(), guest_domid));
 
-    flexarray_append(dm_args, "-mon");
-    flexarray_append(dm_args, "chardev=libxenstat-cmd,mode=control");
+        flexarray_append(dm_args, "-mon");
+        flexarray_append(dm_args, "chardev=libxenstat-cmd,mode=control");
+    }
 
     for (i = 0; i < guest_config->num_channels; i++) {
         connection = guest_config->channels[i].connection;
@@ -1004,7 +1023,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         flexarray_vappend(dm_args, "-name", c_info->name, NULL);
     }
 
-    if (vnc) {
+    if (vnc && !is_stubdom) {
         char *vncarg = NULL;
 
         flexarray_append(dm_args, "-vnc");
@@ -1043,7 +1062,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
 
         flexarray_append(dm_args, vncarg);
-    } else
+    } else if (!is_stubdom)
         /*
          * Ensure that by default no vnc server is created.
          */
@@ -1055,7 +1074,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
      */
     flexarray_append_pair(dm_args, "-display", "none");
 
-    if (sdl) {
+    if (sdl && !is_stubdom) {
         flexarray_append(dm_args, "-sdl");
         if (sdl->display)
             flexarray_append_pair(dm_envs, "DISPLAY", sdl->display);
@@ -1099,10 +1118,31 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 return ERROR_INVAL;
             }
             if (b_info->u.hvm.serial) {
-                flexarray_vappend(dm_args,
-                                  "-serial", b_info->u.hvm.serial, NULL);
-            } else if (b_info->u.hvm.serial_list) {
+                if (is_stubdom) {
+                    flexarray_vappend(dm_args,
+                                      "-serial",
+                                      GCSPRINTF("/dev/hvc%d",
+                                                STUBDOM_CONSOLE_SERIAL),
+                                      NULL);
+                } else {
+                    flexarray_vappend(dm_args,
+                                      "-serial", b_info->u.hvm.serial, NULL);
+                }
+            } else if (b_info->u.hvm.serial_list &&
+                    b_info->u.hvm.serial_list[0]) {
                 char **p;
+                if (is_stubdom) {
+                    if (b_info->u.hvm.serial_list[1]) {
+                        LOGD(ERROR, guest_domid,
+                             "device model in stubdomain doesn't support multiple serial consoles");
+                        return ERROR_INVAL;
+                    }
+                    flexarray_vappend(dm_args,
+                                      "-serial",
+                                      GCSPRINTF("/dev/hvc%d",
+                                                STUBDOM_CONSOLE_SERIAL),
+                                      NULL);
+                }
                 for (p = b_info->u.hvm.serial_list;
                      *p;
                      p++) {
@@ -1117,7 +1157,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-nographic");
         }
 
-        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
+        if (libxl_defbool_val(b_info->u.hvm.spice.enable) && !is_stubdom) {
             const libxl_spice_info *spice = &b_info->u.hvm.spice;
             char *spiceoptions = dm_spice_options(gc, spice);
             if (!spiceoptions)
@@ -1256,8 +1296,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                                  GCSPRINTF("type=tap,id=net%d,ifname=%s,"
                                            "script=%s,downscript=%s",
                                            nics[i].devid, ifname,
-                                           libxl_tapif_script(gc),
-                                           libxl_tapif_script(gc)));
+                                           libxl_tapif_script(gc, b_info),
+                                           libxl_tapif_downscript(gc, b_info)));
 
                 /* Userspace COLO Proxy need this */
 #define APPEND_COLO_SOCK_SERVER(sock_id, sock_ip, sock_port) ({             \
@@ -1503,7 +1543,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
              * If qemu isn't doing the interpreting, the parameter is
              * always raw
              */
-            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
+            if (libxl_defbool_val(b_info->device_model_stubdomain))
+                format = "host_device";
+            else if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
                 format = libxl__qemu_disk_format_string(disks[i].format);
             else
                 format = libxl__qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
@@ -1514,6 +1556,8 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                          disks[i].vdev);
                     continue;
                 }
+            } else if (libxl_defbool_val(b_info->device_model_stubdomain)) {
+                target_path = GCSPRINTF("/dev/xvd%c", 'a' + disk);
             } else {
                 if (format == NULL) {
                     LOGD(WARN, guest_domid,
@@ -1727,7 +1771,7 @@ static int libxl__build_device_model_args(libxl__gc *gc,
                                         char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
                                         int *dm_state_fd)
-/* dm_state_fd may be NULL iff caller knows we are using old stubdom
+/* dm_state_fd may be NULL iff caller knows we are using stubdom
  * and therefore will be passing a filename rather than a fd. */
 {
     switch (guest_config->b_info.device_model_version) {
@@ -1737,8 +1781,10 @@ static int libxl__build_device_model_args(libxl__gc *gc,
                                                   args, envs,
                                                   state);
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-        assert(dm_state_fd != NULL);
-        assert(*dm_state_fd < 0);
+        if (!libxl_defbool_val(guest_config->b_info.device_model_stubdomain)) {
+            assert(dm_state_fd != NULL);
+            assert(*dm_state_fd < 0);
+	}
         return libxl__build_device_model_args_new(gc, dm,
                                                   guest_domid, guest_config,
                                                   args, envs,
@@ -1796,7 +1842,7 @@ static int libxl__vfb_and_vkb_from_hvm_guest_config(libxl__gc *gc,
 
 static int libxl__write_stub_dmargs(libxl__gc *gc,
                                     int dm_domid, int guest_domid,
-                                    char **args)
+                                    char **args, bool linux_stubdom)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int i;
@@ -1824,7 +1870,9 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
     i = 1;
     dmargs[0] = '\0';
     while (args[i] != NULL) {
-        if (strcmp(args[i], "-sdl") && strcmp(args[i], "-M") && strcmp(args[i], "xenfv")) {
+        if (linux_stubdom ||
+            (strcmp(args[i], "-sdl") &&
+             strcmp(args[i], "-M") && strcmp(args[i], "xenfv"))) {
             strcat(dmargs, " ");
             strcat(dmargs, args[i]);
         }
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 04/17] libxl: Build the domain with a Linux based stubdomain

[Marek Marczykowski-Górecki]

From: Eric Shelton <eshelton@pobox.com>

This will build a Linux-based stubdomain with QEMU upstream.

Signed-off-by: Eric Shelton <eshelton@pobox.com>

Simon:
 * use initramfs instead of disk with rootfs
 * don't initialize qmp (unused in Qubes)
 * Make libxl_domain_need_memory consistent with actual stubdoma build
   code (bugfix relevant also for non-linux case)
 * Make stubdomain memory size configurable
Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>

Marek:
 * Make kernel and ramdisk paths configurable.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libxl/libxl_create.c   | 75 +++++++++++++++++++++++++++----------
 tools/libxl/libxl_dm.c       | 48 ++++++++++++++++++------
 tools/libxl/libxl_internal.c | 22 +++++++++++-
 tools/libxl/libxl_internal.h |  4 ++-
 tools/libxl/libxl_mem.c      |  6 ++-
 tools/libxl/libxl_types.idl  |  3 +-
 6 files changed, 126 insertions(+), 32 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1d92b5f..ff27d30 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -162,32 +162,54 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_val(b_info->device_model_stubdomain)) {
         if (!b_info->stubdomain_version) {
             switch (b_info->device_model_version) {
+                case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                    b_info->stubdomain_version = LIBXL_STUBDOMAIN_VERSION_MINIOS;
+                    break;
+                case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    b_info->stubdomain_version = LIBXL_STUBDOMAIN_VERSION_LINUX;
+                    break;
+                default: abort();
+            }
+        }
+
+        switch (b_info->device_model_version) {
             case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
-                b_info->stubdomain_version = LIBXL_STUBDOMAIN_VERSION_MINIOS;
+                if (b_info->stubdomain_version != LIBXL_STUBDOMAIN_VERSION_MINIOS) {
+                    LOG(ERROR,
+                            "\"qemu-xen-traditional\" require \"minios\" as stubdomain");
+                    return ERROR_INVAL;
+                }
                 break;
             case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-                b_info->stubdomain_version = LIBXL_STUBDOMAIN_VERSION_LINUX;
+                if (b_info->stubdomain_version != LIBXL_STUBDOMAIN_VERSION_LINUX) {
+                    LOG(ERROR,
+                            "\"qemu-xen\" require \"linux\" as stubdomain");
+                    return ERROR_INVAL;
+                }
                 break;
             default: abort();
-            }
         }
 
-        switch (b_info->device_model_version) {
-        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
-            if (b_info->stubdomain_version != LIBXL_STUBDOMAIN_VERSION_MINIOS) {
-                LOG(ERROR,
-                    "\"qemu-xen-traditional\" require \"minios\" as stubdomain");
-                return ERROR_INVAL;
+        if (!b_info->stubdomain_kernel) {
+            switch (b_info->stubdomain_version) {
+                case LIBXL_STUBDOMAIN_VERSION_MINIOS:
+                    b_info->stubdomain_kernel =
+                        libxl__abs_path(NOGC, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
+                    b_info->stubdomain_ramdisk = NULL;
+                    break;
+                case LIBXL_STUBDOMAIN_VERSION_LINUX:
+                    b_info->stubdomain_kernel =
+                        libxl__abs_path(NOGC,
+                                "stubdom-linux-kernel",
+                                libxl__xenfirmwaredir_path());
+                    b_info->stubdomain_ramdisk =
+                        libxl__abs_path(NOGC,
+                                "stubdom-linux-rootfs",
+                                libxl__xenfirmwaredir_path());
+                    break;
+                default:
+                    abort();
             }
-            break;
-        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            if (b_info->stubdomain_version != LIBXL_STUBDOMAIN_VERSION_LINUX) {
-                LOG(ERROR,
-                    "\"qemu-xen\" require \"linux\" as stubdomain");
-                return ERROR_INVAL;
-            }
-            break;
-        default: abort();
         }
     }
 
@@ -226,6 +248,19 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT)
         b_info->target_memkb = b_info->max_memkb;
 
+    if (b_info->stubdomain_memkb == LIBXL_MEMKB_DEFAULT) {
+        switch (b_info->stubdomain_version) {
+            case LIBXL_STUBDOMAIN_VERSION_MINIOS:
+                b_info->stubdomain_memkb = 28 * 1024;
+                break;
+            case LIBXL_STUBDOMAIN_VERSION_LINUX:
+                b_info->stubdomain_memkb = LIBXL_LINUX_STUBDOM_MEM * 1024;;
+                break;
+            default:
+                b_info->stubdomain_memkb = 0; // no stubdomain
+        }
+    }
+
     libxl_defbool_setdefault(&b_info->claim_mode, false);
 
     libxl_defbool_setdefault(&b_info->localtime, false);
@@ -1594,7 +1629,9 @@ static void domcreate_devmodel_started(libxl__egc *egc,
     if (dcs->sdss.dm.guest_domid) {
         if (d_config->b_info.device_model_version
             == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
-            libxl__qmp_initializations(gc, domid, d_config);
+            if (!libxl_defbool_val(d_config->b_info.device_model_stubdomain)) {
+                libxl__qmp_initializations(gc, domid, d_config);
+            }
         }
     }
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 82ff490..167dad9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1891,6 +1891,16 @@ retry_transaction:
     return 0;
 }
 
+static int libxl__store_libxl_entry(libxl__gc *gc, uint32_t domid,
+                                    const char *name, const char *value)
+{
+    char *path = NULL;
+
+    path = libxl__xs_libxl_path(gc, domid);
+    path = libxl__sprintf(gc, "%s/%s", path, name);
+    return libxl__xs_printf(gc, XBT_NULL, path, "%s", value);
+}
+
 static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
                                 libxl__dm_spawn_state *stubdom_dmss,
                                 int rc);
@@ -1928,10 +1938,14 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     libxl__domain_build_state *const d_state = sdss->dm.build_state;
     libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
 
-    if (guest_config->b_info.device_model_version !=
-        LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
-        ret = ERROR_INVAL;
-        goto out;
+    assert(libxl_defbool_val(guest_config->b_info.device_model_stubdomain));
+
+    if (guest_config->b_info.stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX) {
+        if (d_state->saved_state) {
+            LOG(ERROR, "Save/Restore not supported yet with Linux Stubdom.");
+            ret = -1;
+            goto out;
+        }
     }
 
     sdss->pvqemu.guest_domid = 0;
@@ -1952,8 +1966,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     libxl_domain_build_info_init_type(&dm_config->b_info, LIBXL_DOMAIN_TYPE_PV);
 
     dm_config->b_info.max_vcpus = 1;
-    dm_config->b_info.max_memkb = 28 * 1024 +
-        guest_config->b_info.video_memkb;
+    dm_config->b_info.max_memkb = guest_config->b_info.stubdomain_memkb;
+    dm_config->b_info.max_memkb += guest_config->b_info.video_memkb;
     dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
 
     dm_config->b_info.max_grant_frames = guest_config->b_info.max_grant_frames;
@@ -1994,10 +2008,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
         dm_config->num_vkbs = 1;
     }
 
-    stubdom_state->pv_kernel.path
-        = libxl__abs_path(gc, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
-    stubdom_state->pv_cmdline = GCSPRINTF(" -d %d", guest_domid);
-    stubdom_state->pv_ramdisk.path = "";
+    stubdom_state->pv_kernel.path = guest_config->b_info.stubdomain_kernel;
+    stubdom_state->pv_ramdisk.path = guest_config->b_info.stubdomain_ramdisk;
 
     /* fixme: this function can leak the stubdom if it fails */
     ret = libxl__domain_make(gc, dm_config, stubdom_state,
@@ -2017,7 +2029,12 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
         goto out;
     }
 
-    libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
+    libxl__store_libxl_entry(gc, guest_domid, "dm-version",
+        libxl_device_model_version_to_string(dm_config->b_info.device_model_version));
+    libxl__store_libxl_entry(gc, dm_domid, "stubdom-version",
+        libxl_stubdomain_version_to_string(guest_config->b_info.stubdomain_version));
+    libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args,
+        guest_config->b_info.stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX);
     libxl__xs_printf(gc, XBT_NULL,
                      GCSPRINTF("%s/image/device-model-domid",
                                libxl__xs_get_dompath(gc, guest_domid)),
@@ -2026,6 +2043,15 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
                      GCSPRINTF("%s/target",
                                libxl__xs_get_dompath(gc, dm_domid)),
                      "%d", guest_domid);
+    if (guest_config->b_info.stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX) {
+        /* qemu-xen is used as a dm in the stubdomain, so we set the bios
+         * accroding to this */
+        libxl__xs_printf(gc, XBT_NULL,
+                        libxl__sprintf(gc, "%s/hvmloader/bios",
+                                       libxl__xs_get_dompath(gc, guest_domid)),
+                        "%s",
+                        libxl_bios_type_to_string(guest_config->b_info.u.hvm.bios));
+    }
     ret = xc_domain_set_target(ctx->xch, dm_domid, guest_domid);
     if (ret<0) {
         LOGED(ERROR, guest_domid, "setting target domain %d -> %d",
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index f492dae..c6b7465 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -377,6 +377,28 @@ out:
     return rc;
 }
 
+int libxl__stubdomain_version_running(libxl__gc *gc, uint32_t domid)
+{
+    char *path = NULL;
+    char *stub_version = NULL;
+    libxl_stubdomain_version value;
+
+    path = libxl__xs_libxl_path(gc, domid);
+    path = libxl__sprintf(gc, "%s/stubdom-version", path);
+    stub_version = libxl__xs_read(gc, XBT_NULL, path);
+    if (!stub_version) {
+        return LIBXL_STUBDOMAIN_VERSION_MINIOS;
+    }
+
+    if (libxl_stubdomain_version_from_string(stub_version, &value) < 0) {
+        libxl_ctx *ctx = libxl__gc_owner(gc);
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+                   "fatal: %s contain a wrong value (%s)", path, stub_version);
+        return -1;
+    }
+    return value;
+}
+
 int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid)
 {
     char *path = NULL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 843c625..fbcb0b7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -117,6 +117,7 @@
 #define STUBDOM_CONSOLE_RESTORE 2
 #define STUBDOM_CONSOLE_SERIAL 3
 #define STUBDOM_SPECIAL_CONSOLES 3
+#define LIBXL_LINUX_STUBDOM_MEM 128
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DOMID_XS_PATH "domid"
 #define INVALID_DOMID ~0
@@ -2041,6 +2042,9 @@ _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);
+  /* Based on /libxl/$domid/stubdom-version xenstore key
+   * default is minios */
+_hidden int libxl__stubdomain_version_running(libxl__gc *gc, uint32_t domid);
 
 #define DEVICE_MODEL_XS_PATH(gc, dm_domid, domid, fmt, _a...)              \
     libxl__sprintf(gc, "/local/domain/%u/device-model/%u" fmt, dm_domid,   \
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index e551e09..4cf6a73 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -463,8 +463,10 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
     case LIBXL_DOMAIN_TYPE_PVH:
     case LIBXL_DOMAIN_TYPE_HVM:
         *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
-        if (libxl_defbool_val(b_info->device_model_stubdomain))
-            *need_memkb += 32 * 1024;
+        if (libxl_defbool_val(b_info->device_model_stubdomain)) {
+            *need_memkb += b_info->stubdomain_memkb;
+            *need_memkb += b_info->video_memkb;
+        }
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 847e2fe..cdd9a70 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -498,6 +498,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
     ("stubdomain_version", libxl_stubdomain_version),
+    ("stubdomain_memkb",   MemKB),
+    ("stubdomain_kernel",  string),
+    ("stubdomain_ramdisk", string),
     # if you set device_model you must set device_model_version too
     ("device_model",     string),
     ("device_model_ssidref", uint32),
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 05/17] libxl: use xenstore for pci hotplug qemu-in-linux-stubdom commands

[Marek Marczykowski-Górecki]

From: Simon Gaiser <simon@invisiblethingslab.com>

There is no QMP socket access, re-use the same mechanism as for MiniOS
based stubdom.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_pci.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 4755a0c..311fad4 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -995,6 +995,7 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
     uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
     uint32_t domainid = domid;
     bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
+    uint32_t dm_domid;
 
     if (type == LIBXL_DOMAIN_TYPE_INVALID)
         return ERROR_FAIL;
@@ -1010,7 +1011,15 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcidev, i
                 rc = qemu_pci_add_xenstore(gc, domid, pcidev);
                 break;
             case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-                rc = libxl__qmp_pci_add(gc, domid, pcidev);
+                dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+                if (dm_domid != 0
+                    && libxl__stubdomain_version_running(gc, dm_domid) ==
+                         LIBXL_STUBDOMAIN_VERSION_LINUX) {
+                    rc = qemu_pci_add_xenstore(gc, domid, pcidev);
+                } else {
+                    rc = libxl__qmp_pci_add(gc, domid, pcidev);
+                }
                 break;
             default:
                 return ERROR_INVAL;
@@ -1362,7 +1371,7 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
     libxl_device_pci *assigned;
     libxl_domain_type type = libxl__domain_type(gc, domid);
     int hvm = 0, rc, num;
-    int stubdomid = 0;
+    int stubdomid = libxl_get_stubdom_id(ctx, domid);
     uint32_t domainid = domid;
     bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
 
@@ -1390,7 +1399,13 @@ static int do_pci_remove(libxl__gc *gc, uint32_t domid,
             rc = qemu_pci_remove_xenstore(gc, domid, pcidev, force);
             break;
         case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-            rc = libxl__qmp_pci_del(gc, domid, pcidev);
+            if (stubdomid != 0
+                && libxl__stubdomain_version_running(gc, stubdomid) ==
+                     LIBXL_STUBDOMAIN_VERSION_LINUX) {
+                rc = qemu_pci_remove_xenstore(gc, domid, pcidev, force);
+            } else {
+                rc = libxl__qmp_pci_del(gc, domid, pcidev);
+            }
             break;
         default:
             rc = ERROR_INVAL;
@@ -1470,7 +1485,6 @@ out:
             LOGED(ERROR, domainid, "xc_deassign_device failed");
     }
 
-    stubdomid = libxl_get_stubdom_id(ctx, domid);
     if (stubdomid != 0) {
         libxl_device_pci pcidev_s = *pcidev;
         libxl__device_pci_remove_common(gc, stubdomid, &pcidev_s, force);
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 06/17] libxl: create vkb device only for guests with graphics output

[Marek Marczykowski-Górecki]

The forced vkb device is meant for better performance of qemu access
(at least according to ebbd2561b4cefb299f0f68a88b2788504223de18 "libxl:
Add a vkbd frontend/backend pair for HVM guests"), which isn't used if
there is no configured channel to actually access that keyboard.

One can still add vkb device manually if needed.

This is continuation of b053f0c4c9e533f3d97837cf897eb920b8355ed3 "libxl: do
not start dom0 qemu for stubdomain when not needed".

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libxl/libxl_create.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index ff27d30..a0acd6b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1451,9 +1451,13 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
-        libxl_device_vkb_init(&vkb);
-        libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
-        libxl_device_vkb_dispose(&vkb);
+        if (libxl_defbool_val(d_config->b_info.u.hvm.vnc.enable)
+            || libxl_defbool_val(d_config->b_info.u.hvm.spice.enable)
+            || libxl_defbool_val(d_config->b_info.u.hvm.sdl.enable)) {
+            libxl_device_vkb_init(&vkb);
+            libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
+            libxl_device_vkb_dispose(&vkb);
+        }
 
         dcs->sdss.dm.guest_domid = domid;
         if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain

[Marek Marczykowski-Górecki]

Rely on a wrapper script in stubdomain to attach FD 3/4 of qemu to
relevant consoles.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libxl/libxl_dm.c          | 23 +++++++++++------------
 tools/libxl/libxl_dom_suspend.c | 10 ++++++++--
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 167dad9..330d552 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1454,10 +1454,17 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         flexarray_append(dm_args, "-xen-domid-restrict");
 
     if (state->saved_state) {
-        /* This file descriptor is meant to be used by QEMU */
-        *dm_state_fd = open(state->saved_state, O_RDONLY);
-        flexarray_append(dm_args, "-incoming");
-        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
+        if (is_stubdom) {
+            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
+             */
+            flexarray_append(dm_args, "-incoming");
+            flexarray_append(dm_args, "fd:3");
+        } else {
+            /* This file descriptor is meant to be used by QEMU */
+            *dm_state_fd = open(state->saved_state, O_RDONLY);
+            flexarray_append(dm_args, "-incoming");
+            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
+        }
     }
     for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
         flexarray_append(dm_args, b_info->extra[i]);
@@ -1940,14 +1947,6 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     assert(libxl_defbool_val(guest_config->b_info.device_model_stubdomain));
 
-    if (guest_config->b_info.stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX) {
-        if (d_state->saved_state) {
-            LOG(ERROR, "Save/Restore not supported yet with Linux Stubdom.");
-            ret = -1;
-            goto out;
-        }
-    }
-
     sdss->pvqemu.guest_domid = 0;
 
     libxl_domain_create_info_init(&dm_config->c_info);
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 1e904ba..8c8ae84 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -73,7 +73,8 @@ int libxl__domain_suspend_device_model(libxl__gc *gc,
 {
     int ret = 0;
     uint32_t const domid = dsps->domid;
-    const char *const filename = dsps->dm_savefile;
+    uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+    const char * filename = dsps->dm_savefile;
 
     switch (libxl__device_model_version_running(gc, domid)) {
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
@@ -86,8 +87,13 @@ int libxl__domain_suspend_device_model(libxl__gc *gc,
         if (libxl__qmp_stop(gc, domid))
             return ERROR_FAIL;
         /* Save DM state into filename */
+        if (dm_domid) {
+            /* if DM is in stubdomain, instruct it to use console, which is
+             * connected to a file pointed by filename */
+            filename = "/proc/self/fd/4";
+        }
         ret = libxl__qmp_save(gc, domid, filename, dsps->live);
-        if (ret)
+        if (ret && !dm_domid)
             unlink(filename);
         break;
     default:
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 08/17] xl: add stubdomain related options to xl config parser

[Marek Marczykowski-Górecki]

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
 docs/man/xl.cfg.pod.5.in | 23 +++++++++++++++++++----
 tools/xl/xl_parse.c      |  7 +++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index b727181..54a9585 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -2700,10 +2700,25 @@ model which they were installed with.
 
 =item B<device_model_override="PATH">
 
-Override the path to the binary to be used as the device-model. The
-binary provided here MUST be consistent with the
-B<device_model_version> which you have specified. You should not
-normally need to specify this option.
+Override the path to the binary to be used as the device-model running in
+toolstack domain. The binary provided here MUST be consistent with the
+B<device_model_version> which you have specified. You should not normally need
+to specify this option.
+
+=item B<stubdomain_kernel="PATH">
+
+Override the path to the kernel image used as device-model stubdomain.
+The binary provided here MUST be consistent with the
+B<device_model_version> which you have specified.
+In case of B<qemu-xen-traditional> it is expected to be MiniOS-based stubdomain
+image, in case of B<qemu-xen> it is expected to be Linux-based stubdomain
+kernel.
+
+=item B<stubdomain_ramdisk="PATH">
+
+Override the path to the ramdisk image used as device-model stubdomain.
+The binary provided here is to be used by a kernel pointed by B<stubdomain_kernel>.
+It is known to be used only by Linux-based stubdomain kernel.
 
 =item B<device_model_stubdomain_override=BOOLEAN>
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 971ec1b..a7cd8a7 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2496,6 +2496,13 @@ skip_usbdev:
     xlu_cfg_replace_string(config, "device_model_user",
                            &b_info->device_model_user, 0);
 
+    xlu_cfg_replace_string (config, "stubdomain_kernel",
+                            &b_info->stubdomain_kernel, 0);
+    xlu_cfg_replace_string (config, "stubdomain_ramdisk",
+                            &b_info->stubdomain_ramdisk, 0);
+    if (!xlu_cfg_get_long (config, "stubdomain_memory", &l, 0))
+        b_info->stubdomain_memkb = l * 1024;
+
 #define parse_extra_args(type)                                            \
     e = xlu_cfg_get_list_as_string_list(config, "device_model_args"#type, \
                                     &b_info->extra##type, 0);            \
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 09/17] libxl: use \x1b to separate qemu arguments for linux stubdomain

[Marek Marczykowski-Górecki]

This allows using arguments with spaces, like -append.
Stubdomain side of this require "xenstore-client: Add option for raw
in-/output" commit.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libxl/libxl_dm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 330d552..6eea377 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1855,6 +1855,7 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
     int i;
     char *vm_path;
     char *dmargs, *path;
+    const char arg_sep = linux_stubdom ? '\x1b' : ' ';
     int dmargs_size;
     struct xs_permissions roperm[2];
     xs_transaction_t t;
@@ -1880,8 +1881,9 @@ static int libxl__write_stub_dmargs(libxl__gc *gc,
         if (linux_stubdom ||
             (strcmp(args[i], "-sdl") &&
              strcmp(args[i], "-M") && strcmp(args[i], "xenfv"))) {
-            strcat(dmargs, " ");
             strcat(dmargs, args[i]);
+            if (args[i + 1] != NULL)
+                strncat(dmargs, &arg_sep, 1);
         }
         i++;
     }
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 10/17] xenconsoled: install xenstore watch for all supported consoles

[Marek Marczykowski-Górecki]

Not only for the primary one (/local/domain/<domid>/console path).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/console/daemon/io.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 8dac279..4d4e223 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -791,24 +791,24 @@ static int console_create_ring(struct console *con)
 	return err;
 }
 
-static bool watch_domain(struct domain *dom, bool watch)
+static int watch_domain(struct console *con, struct domain *dom, void **data)
 {
+	bool watch = data;
 	char domid_str[3 + MAX_STRLEN(dom->domid)];
 	bool success;
-	struct console *con = &dom->console[0];
 
 	snprintf(domid_str, sizeof(domid_str), "dom%u", dom->domid);
 	if (watch) {
 		success = xs_watch(xs, con->xspath, domid_str);
 		if (success)
-			console_iter_int_arg1(dom, console_create_ring);
+			console_create_ring(con);
 		else
 			xs_unwatch(xs, con->xspath, domid_str);
 	} else {
 		success = xs_unwatch(xs, con->xspath, domid_str);
 	}
 
-	return success;
+	return !success;
 }
 
 static int console_init(struct console *con, struct domain *dom, void **data)
@@ -878,7 +878,7 @@ static struct domain *create_domain(int domid)
 	if (console_iter_int_arg3(dom, console_init, (void **)&con_type))
 		goto out;
 
-	if (!watch_domain(dom, true))
+	if (console_iter_int_arg3(dom, watch_domain, (void**)true))
 		goto out;
 
 	dom->next = dom_head;
@@ -952,7 +952,7 @@ static void console_close_evtchn(struct console *con)
 static void shutdown_domain(struct domain *d)
 {
 	d->is_dead = true;
-	watch_domain(d, false);
+	console_iter_int_arg3(d, watch_domain, (void**)false);
 	console_iter_void_arg1(d, console_unmap_interface);
 	console_iter_void_arg1(d, console_close_evtchn);
 }
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry

[Marek Marczykowski-Górecki]

Add support for standard xenbus initialization protocol using 'state'
xenstore entry. It will be necessary for secondary consoles.
For consoles supporting it, read 'state' entry on the frontend and
proceed accordingly - either init console or close it. When closing,
make sure all the in-transit data is flushed (both from shared ring and
from local buffer), if possible. This is especially important for
MiniOS-based qemu stubdomain, which closes console just after writing
device model state to it.
For consoles without 'state' field behavior is unchanged - on any watch
event try to connect console, as long as domain is alive.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/console/daemon/io.c | 86 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 83 insertions(+), 3 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 4d4e223..82806b2 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -25,6 +25,7 @@
 #include <xengnttab.h>
 #include <xenstore.h>
 #include <xen/io/console.h>
+#include <xen/io/xenbus.h>
 #include <xen/grant_table.h>
 
 #include <stdlib.h>
@@ -110,6 +111,7 @@ struct console {
 	struct domain *d;
 	bool optional;
 	bool use_gnttab;
+	bool have_state;
 };
 
 struct console_type {
@@ -118,6 +120,7 @@ struct console_type {
 	char *log_suffix;
 	bool optional;
 	bool use_gnttab;
+	bool have_state;  // uses 'state' xenstore entry
 };
 
 static struct console_type console_type[] = {
@@ -157,6 +160,8 @@ typedef void (*VOID_ITER_FUNC_ARG2)(struct console *,  void *);
 typedef int (*INT_ITER_FUNC_ARG3)(struct console *,
 				  struct domain *dom, void **);
 
+static void handle_tty_write(struct console *con);
+
 static inline bool console_enabled(struct console *con)
 {
 	return con->local_port != -1;
@@ -672,6 +677,20 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
 	return ret;
 }
 
+static void set_backend_state(struct console *con, int state)
+{
+	char path[PATH_MAX], state_str[2], *be_path;
+
+	snprintf(state_str, sizeof(state_str), "%d", state);
+	snprintf(path, sizeof(path), "%s/backend", con->xspath);
+	be_path = xs_read(xs, XBT_NULL, path, NULL);
+	if (be_path) {
+		snprintf(path, sizeof(path), "%s/state", be_path);
+		xs_write(xs, XBT_NULL, path, state_str, 1);
+		free(be_path);
+	}
+}
+
 static void console_unmap_interface(struct console *con)
 {
 	if (con->interface == NULL)
@@ -683,7 +702,7 @@ static void console_unmap_interface(struct console *con)
 	con->interface = NULL;
 	con->ring_ref = -1;
 }
- 
+
 static int console_create_ring(struct console *con)
 {
 	int err, remote_port, ring_ref, rc;
@@ -787,10 +806,70 @@ static int console_create_ring(struct console *con)
 	if (log_guest && (con->log_fd == -1))
 		con->log_fd = create_console_log(con);
 
+	/* if everything ok, signal backend readiness, in backend tree */
+	set_backend_state(con, XenbusStateConnected);
+
  out:
 	return err;
 }
 
+/* gracefully close console */
+static int console_close(struct console *con) {
+
+	if (con->interface && con->master_fd != -1 && con->buffer.data) {
+		/* handle remaining data in buffers */
+		buffer_append(con);
+
+		/* write it out, if possible */
+		if (con->master_pollfd_idx != -1) {
+			if (fds[con->master_pollfd_idx].revents &
+					POLLOUT)
+				handle_tty_write(con);
+		}
+	}
+
+	console_close_tty(con);
+	console_unmap_interface(con);
+	set_backend_state(con, XenbusStateClosed);
+
+	return 0;
+}
+
+
+static int handle_console_state(struct console *con) {
+	int err, state;
+
+	if (!con->have_state)
+		return console_create_ring(con);
+
+	err = xs_gather(xs, con->xspath,
+			"state", "%u", &state,
+			NULL);
+	if (err)
+		/* no 'state' entry, assume removal */
+		state = XenbusStateClosed;
+
+	switch (state) {
+		case XenbusStateInitialising:
+		case XenbusStateInitWait:
+			/* wait for frontent init */
+			return 0;
+		case XenbusStateInitialised:
+		case XenbusStateConnected:
+			/* ok, init backend (also on restart) */
+			return console_create_ring(con);
+		case XenbusStateClosing:
+		case XenbusStateClosed:
+			/* close requested */
+			return console_close(con);
+		default:
+			dolog(LOG_ERR,
+			      "Invalid state %d set by console %s of domain %d\n",
+			      state, con->xspath, con->d->domid);
+			return 1;
+	}
+}
+
 static int watch_domain(struct console *con, struct domain *dom, void **data)
 {
 	bool watch = data;
@@ -801,7 +880,7 @@ static int watch_domain(struct console *con, struct domain *dom, void **data)
 	if (watch) {
 		success = xs_watch(xs, con->xspath, domid_str);
 		if (success)
-			console_create_ring(con);
+			handle_console_state(con);
 		else
 			xs_unwatch(xs, con->xspath, domid_str);
 	} else {
@@ -839,6 +918,7 @@ static int console_init(struct console *con, struct domain *dom, void **data)
 	con->log_suffix = (*con_type)->log_suffix;
 	con->optional = (*con_type)->optional;
 	con->use_gnttab = (*con_type)->use_gnttab;
+	con->have_state = (*con_type)->have_state;
 	xsname = (char *)(*con_type)->xsname;
 	xspath = xs_get_domain_path(xs, dom->domid);
 	s = realloc(xspath, strlen(xspath) +
@@ -1149,7 +1229,7 @@ static void handle_xs(void)
 		/* We may get watches firing for domains that have recently
 		   been removed, so dom may be NULL here. */
 		if (dom && dom->is_dead == false)
-			console_iter_int_arg1(dom, console_create_ring);
+			console_iter_int_arg1(dom, handle_console_state);
 	}
 
 	free(vec);
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 12/17] xenconsoled: make console_type->use_gnttab less confusing

[Marek Marczykowski-Górecki]

Before this commit 'use_gnttab' means xenconsoled should first try
special GNTTAB_RESERVED_CONSOLE entry, and only then fallback to
ring-ref xenstore entry (being gfn of actual ring).
In case of secondary consoles, ring-ref entry contains grant table
reference (not gfn of it), which makes the old meaning of use_gnttab
really confusing (should be false for such consoles).
To solve this, add new entry in console_type (and console) structures
named 'use_reserverd_gnttab' with the old meaning of 'use_gnttab', then
use 'ues_gnttab' for consoles where ring-ref holds grant table
reference.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/console/daemon/io.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 82806b2..3f2b9cb 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -111,6 +111,7 @@ struct console {
 	struct domain *d;
 	bool optional;
 	bool use_gnttab;
+	bool use_reserved_gnttab;
 	bool have_state;
 };
 
@@ -120,6 +121,7 @@ struct console_type {
 	char *log_suffix;
 	bool optional;
 	bool use_gnttab;
+	bool use_reserved_gnttab;
 	bool have_state;  // uses 'state' xenstore entry
 };
 
@@ -130,6 +132,7 @@ static struct console_type console_type[] = {
 		.log_suffix = "",
 		.optional = false,
 		.use_gnttab = true,
+		.use_reserved_gnttab = true,
 	},
 #if defined(CONFIG_ARM)
 	{
@@ -695,7 +698,7 @@ static void console_unmap_interface(struct console *con)
 {
 	if (con->interface == NULL)
 		return;
-	if (xgt_handle && con->ring_ref == -1)
+	if (xgt_handle && con->use_gnttab)
 		xengnttab_unmap(xgt_handle, con->interface, 1);
 	else
 		munmap(con->interface, XC_PAGE_SIZE);
@@ -739,12 +742,19 @@ static int console_create_ring(struct console *con)
 
 	if (!con->interface && xgt_handle && con->use_gnttab) {
 		/* Prefer using grant table */
-		con->interface = xengnttab_map_grant_ref(xgt_handle,
-			dom->domid, GNTTAB_RESERVED_CONSOLE,
-			PROT_READ|PROT_WRITE);
-		con->ring_ref = -1;
+		if (con->use_reserved_gnttab) {
+			con->interface = xengnttab_map_grant_ref(xgt_handle,
+					dom->domid, GNTTAB_RESERVED_CONSOLE,
+					PROT_READ|PROT_WRITE);
+			con->ring_ref = -1;
+		} else {
+			con->interface = xengnttab_map_grant_ref(xgt_handle,
+					dom->domid, ring_ref,
+					PROT_READ|PROT_WRITE);
+			con->ring_ref = ring_ref;
+		}
 	}
-	if (!con->interface) {
+	if (!con->interface && (!con->use_gnttab || con->use_reserved_gnttab)) {
 		/* Fall back to xc_map_foreign_range */
 		con->interface = xc_map_foreign_range(
 			xc, dom->domid, XC_PAGE_SIZE,
@@ -918,6 +928,7 @@ static int console_init(struct console *con, struct domain *dom, void **data)
 	con->log_suffix = (*con_type)->log_suffix;
 	con->optional = (*con_type)->optional;
 	con->use_gnttab = (*con_type)->use_gnttab;
+	con->use_reserved_gnttab = (*con_type)->use_reserved_gnttab;
 	con->have_state = (*con_type)->have_state;
 	xsname = (char *)(*con_type)->xsname;
 	xspath = xs_get_domain_path(xs, dom->domid);
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles

[Marek Marczykowski-Górecki]

Based on previous few commits, this adds basic support for multiple
consoles in xenconsoled. A static number of them - up to 3 (+ one
primary).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
I know this is awful, but everything else I can think of (real support
for multiple consoles, dynamically allocated) requires major restructure
of the code. Given I'm still not sure if multiple consoles are the way
to go with stubdomain, I'd rather not invest time in something that
could never by actually useful.
---
 tools/console/daemon/io.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 3f2b9cb..c2c37dc 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -134,6 +134,30 @@ static struct console_type console_type[] = {
 		.use_gnttab = true,
 		.use_reserved_gnttab = true,
 	},
+	{
+		.xsname = "/device/console/1",
+		.ttyname = "tty",
+		.log_suffix = "-con1",
+		.optional = true,
+		.use_gnttab = true,
+		.have_state = true,
+	},
+	{
+		.xsname = "/device/console/2",
+		.ttyname = "tty",
+		.log_suffix = "-con2",
+		.optional = true,
+		.use_gnttab = true,
+		.have_state = true,
+	},
+	{
+		.xsname = "/device/console/3",
+		.ttyname = "tty",
+		.log_suffix = "-con3",
+		.optional = true,
+		.use_gnttab = true,
+		.have_state = true,
+	},
 #if defined(CONFIG_ARM)
 	{
 		.xsname = "/vuart/0",
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 14/17] xenconsoled: deduplicate error handling

[Marek Marczykowski-Górecki]

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/console/daemon/io.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index c2c37dc..ea07442 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -819,9 +819,7 @@ static int console_create_ring(struct console *con)
 
 	if (rc == -1) {
 		err = errno;
-		xenevtchn_close(con->xce_handle);
-		con->xce_handle = NULL;
-		goto out;
+		goto err_xce;
 	}
 	con->local_port = rc;
 	con->remote_port = remote_port;
@@ -829,11 +827,7 @@ static int console_create_ring(struct console *con)
 	if (con->master_fd == -1) {
 		if (!console_create_tty(con)) {
 			err = errno;
-			xenevtchn_close(con->xce_handle);
-			con->xce_handle = NULL;
-			con->local_port = -1;
-			con->remote_port = -1;
-			goto out;
+			goto err_xce;
 		}
 	}
 
@@ -843,6 +837,13 @@ static int console_create_ring(struct console *con)
 	/* if everything ok, signal backend readiness, in backend tree */
 	set_backend_state(con, XenbusStateConnected);
 
+ err_xce:
+	if (err) {
+		xenevtchn_close(con->xce_handle);
+		con->xce_handle = NULL;
+		con->local_port = -1;
+		con->remote_port = -1;
+	}
  out:
 	return err;
 }
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 15/17] xenconsoled: add support for non-pty output

[Marek Marczykowski-Górecki]

Handle 'output' xenstore entry, as qemu does. Right now support only few
simple options:
 - "pty" (unchanged)
 - "file:path" (overwrite file)
 - "pipe:path" (read-write file/pipe)
 - "null"

Also, when ever read() returns 0, stop reading from that source, instead
of spinning in a loop.
For now, in case of reconnect, intentionally use pty (ignore 'output'
xenstore entry), as for normal files close+open can be harmful
(especially with O_TRUNC).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
According to qemu docs, "pipe" should behave differently - it should
open two pipes, with ".in" and ".out". According to actual qemu code, it
does that, but if it fails it fallbacks to just one file. And libxl
relies on that fallback, so I've implemented that version only.
If xenconsoled would have some man page, I'd add it there...
---
 tools/console/daemon/io.c | 62 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 5 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index ea07442..15deca4 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -96,6 +96,8 @@ struct console {
 	int master_fd;
 	int master_pollfd_idx;
 	int slave_fd;
+	bool master_fd_can_read;
+	bool master_fd_can_write;
 	int log_fd;
 	struct buffer buffer;
 	char *xspath;
@@ -730,6 +732,53 @@ static void console_unmap_interface(struct console *con)
 	con->ring_ref = -1;
 }
 
+static int create_console_output(struct console *con)
+{
+	int err;
+	char *output, *path;
+
+	if (asprintf(&path, "%s/%s", con->xspath, "output") == -1) {
+		err = ENOMEM;
+		goto out;
+	}
+
+	con->master_fd_can_read = true;
+	con->master_fd_can_write = true;
+
+	output = xs_read(xs, XBT_NULL, path, NULL);
+
+	if (!output || !strcmp(output, "pty")) {
+		if (!console_create_tty(con)) {
+			err = errno;
+			goto out;
+		}
+	} else if (!strncmp(output, "file:", 5)) {
+		con->master_fd = open(output+5, O_WRONLY | O_CREAT | O_TRUNC, 0600);
+		if (con->master_fd == -1) {
+			err = errno;
+			goto out;
+		}
+		/* this is write-only file */
+		con->master_fd_can_read = false;
+	} else if (!strncmp(output, "pipe:", 5)) {
+		con->master_fd = open(output+5, O_RDWR);
+		if (con->master_fd == -1) {
+			err = errno;
+			goto out;
+		}
+	} else if (!strcmp(output, "null")) {
+		con->master_fd = open("/dev/null", O_RDWR);
+		if (con->master_fd == -1) {
+			err = errno;
+			goto out;
+		}
+	}
+
+	err = 0;
+out:
+	return err;
+}
+
 static int console_create_ring(struct console *con)
 {
 	int err, remote_port, ring_ref, rc;
@@ -825,10 +874,9 @@ static int console_create_ring(struct console *con)
 	con->remote_port = remote_port;
 
 	if (con->master_fd == -1) {
-		if (!console_create_tty(con)) {
-			err = errno;
+		err = create_console_output(con);
+		if (err)
 			goto err_xce;
-		}
 	}
 
 	if (log_guest && (con->log_fd == -1))
@@ -941,6 +989,8 @@ static int console_init(struct console *con, struct domain *dom, void **data)
 
 	con->master_fd = -1;
 	con->master_pollfd_idx = -1;
+	con->master_fd_can_read = true;
+	con->master_fd_can_write = true;
 	con->slave_fd = -1;
 	con->log_fd = -1;
 	con->ring_ref = -1;
@@ -1152,6 +1202,8 @@ static void handle_tty_read(struct console *con)
 	 */
 	if (len < 0) {
 		console_handle_broken_tty(con, domain_is_valid(dom->domid));
+	} else if (len == 0) {
+		con->master_fd_can_read = false;
 	} else if (domain_is_valid(dom->domid)) {
 		prod = intf->in_prod;
 		for (i = 0; i < len; i++) {
@@ -1396,10 +1448,10 @@ static void maybe_add_console_tty_fd(struct console *con)
 {
 	if (con->master_fd != -1) {
 		short events = 0;
-		if (!con->d->is_dead && ring_free_bytes(con))
+		if (!con->d->is_dead && con->master_fd_can_read && ring_free_bytes(con))
 			events |= POLLIN;
 
-		if (!buffer_empty(&con->buffer))
+		if (con->master_fd_can_write && !buffer_empty(&con->buffer))
 			events |= POLLOUT;
 
 		if (events)
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 16/17] libxl: access QMP socket via console for qemu-in-stubdomain

[Marek Marczykowski-Górecki]

Add support for talking with qemu in stubdomain via QMP connected to a
console. Since a console doesn't have out of band connect/disconnect
signaling, use (new) qmp_reset command at every connect, to force
renegotiation.

This commit doesn't deal with multiple users accessing the same console.
For example all connected libxl users will see responses for commands
send by any user. If commands IDs would be unique (they aren't),
theoretically that wouldn't be a problem. But two instances sending
commands at once is still problematic.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_dm.c       | 18 ++++++++-----
 tools/libxl/libxl_internal.h |  5 ++--
 tools/libxl/libxl_qmp.c      | 52 +++++++++++++++++++++++++++++++++----
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 6eea377..ab851cc 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -959,18 +959,23 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                       "-xen-domid",
                       GCSPRINTF("%d", guest_domid), NULL);
 
-    /* There is currently no way to access the QMP socket in the stubdom */
-    if (!is_stubdom) {
-        flexarray_append(dm_args, "-chardev");
+    flexarray_append(dm_args, "-chardev");
+    /* stubdom + qemu-xen implies linux based stubdom */
+    if (is_stubdom)
+        flexarray_append(dm_args,
+                         GCSPRINTF("serial,id=libxl-cmd,path=/dev/hvc%d",
+                                   STUBDOM_CONSOLE_QMP));
+    else
         flexarray_append(dm_args,
                          GCSPRINTF("socket,id=libxl-cmd,"
                                         "path=%s/qmp-libxl-%d,server,nowait",
                                         libxl__run_dir_path(), guest_domid));
 
-        flexarray_append(dm_args, "-no-shutdown");
-        flexarray_append(dm_args, "-mon");
-        flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
+    flexarray_append(dm_args, "-no-shutdown");
+    flexarray_append(dm_args, "-mon");
+    flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
 
+    if (!is_stubdom) {
         flexarray_append(dm_args, "-chardev");
         flexarray_append(dm_args,
                          GCSPRINTF("socket,id=libxenstat-cmd,"
@@ -2143,6 +2148,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
         /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
          * STUBDOM_CONSOLE_SAVE (console 1) is for writing the save file
          * STUBDOM_CONSOLE_RESTORE (console 2) is for reading the save file
+         * STUBDOM_CONSOLE_QMP (console 3) is for accessing qmp socket (linux stubdom only)
          */
         switch (i) {
             char *filename;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fbcb0b7..8ef415e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -115,8 +115,9 @@
 #define STUBDOM_CONSOLE_LOGGING 0
 #define STUBDOM_CONSOLE_SAVE 1
 #define STUBDOM_CONSOLE_RESTORE 2
-#define STUBDOM_CONSOLE_SERIAL 3
-#define STUBDOM_SPECIAL_CONSOLES 3
+#define STUBDOM_CONSOLE_QMP 3
+#define STUBDOM_CONSOLE_SERIAL 4
+#define STUBDOM_SPECIAL_CONSOLES 4
 #define LIBXL_LINUX_STUBDOM_MEM 128
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DOMID_XS_PATH "domid"
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 987bf02..b39ff90 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -476,6 +476,16 @@ out:
     return ret;
 }
 
+static int qmp_open_pty(libxl__qmp_handler *qmp, const char *qmp_pty_path)
+{
+    qmp->qmp_fd = open(qmp_pty_path,
+                       O_RDWR | O_NOCTTY | O_NONBLOCK | O_CLOEXEC);
+    if (qmp->qmp_fd < 0) {
+        return -1;
+    }
+    return 0;
+}
+
 static void qmp_close(libxl__qmp_handler *qmp)
 {
     callback_id_pair *pp = NULL;
@@ -766,15 +776,47 @@ libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc, uint32_t domid)
     int ret = 0;
     libxl__qmp_handler *qmp = NULL;
     char *qmp_socket;
+    uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
 
     qmp = qmp_init_handler(gc, domid);
     if (!qmp) return NULL;
 
-    qmp_socket = GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
-    if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
-        LOGED(ERROR, domid, "Connection error");
-        qmp_free_handler(qmp);
-        return NULL;
+    if (dm_domid) {
+        /* sanity check */
+        if (libxl__stubdomain_version_running(gc, dm_domid) ==
+                LIBXL_STUBDOMAIN_VERSION_MINIOS) {
+            LOGED(ERROR, domid, "QMP socket unsupported for minios stubdomain");
+            qmp_free_handler(qmp);
+            return NULL;
+        }
+        ret = libxl_console_get_tty(CTX, dm_domid,
+                                    STUBDOM_CONSOLE_QMP,
+                                    LIBXL_CONSOLE_TYPE_PV,
+                                    &qmp_socket);
+        if (ret) {
+            LOGED(ERROR, domid, "Failed to get QMP tty path");
+            qmp_free_handler(qmp);
+            return NULL;
+        }
+        ret = qmp_open_pty(qmp, qmp_socket);
+        if (ret) {
+            LOGED(ERROR, domid, "Connection error");
+            qmp_free_handler(qmp);
+            return NULL;
+        }
+        ret = qmp_send(qmp, "qmp_reset", NULL, NULL, NULL, NULL);
+        if (ret < 0) {
+            LOGED(ERROR, domid, "Failed to send qmp_reset");
+            qmp_free_handler(qmp);
+            return NULL;
+        }
+    } else {
+        qmp_socket = GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
+        if ((ret = qmp_open(qmp, qmp_socket, QMP_SOCKET_CONNECT_TIMEOUT)) < 0) {
+            LOGED(ERROR, domid, "Connection error");
+            qmp_free_handler(qmp);
+            return NULL;
+        }
     }
 
     LOGD(DEBUG, domid, "connected to %s", qmp_socket);
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH v2 17/17] libxl: use xenconsoled even for multiple stubdomain's consoles

[Marek Marczykowski-Górecki]

Since multiple consoles support was added to xenconsoled, use it for
stubdomain. This makes it possible to have HVM without qemu in dom0 at
al. As long as no other feature requiring qemu in dom0 is used, like VNC
or qdisk.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_dm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ab851cc..582f122 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2144,7 +2144,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
 
     for (i = 0; i < num_console; i++) {
         console[i].devid = i;
-        console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+        /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if qemu
+         * will be in use */
+        console[i].consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
         /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
          * STUBDOM_CONSOLE_SAVE (console 1) is for writing the save file
          * STUBDOM_CONSOLE_RESTORE (console 2) is for reading the save file
@@ -2160,9 +2162,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
                 if (ret) goto out;
                 console[i].output = GCSPRINTF("file:%s", filename);
                 free(filename);
-                /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if qemu
-                 * will be in use */
-                console[i].consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
                 break;
             case STUBDOM_CONSOLE_SAVE:
                 console[i].output = GCSPRINTF("file:%s",
-- 
git-series 0.9.1

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 01/17] libxl: fix qemu-trad cmdline for no sdl/vnc case

[Wei Liu]

On Tue, Aug 07, 2018 at 04:16:06AM +0200, Marek Marczykowski-Górecki wrote:
> When qemu is running in stubdomain, any attempt to initialize vnc/sdl
> there will crash it (on failed attempt to load a keymap from a file). If
> vfb is present, all those cases are skipped. But since
> b053f0c4c9e533f3d97837cf897eb920b8355ed3 "libxl: do not start dom0 qemu
> for stubdomain when not needed" it is possible to create a stubdomain
> without vfb and contrary to the comment -vnc none do trigger VNC
> initialization code (just skips exposing it externally).
> Change the implicit SDL avoiding method to -nographics option, used when
> none of SDL or VNC is enabled.

Urgh...

> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info.

[Wei Liu]

On Tue, Aug 07, 2018 at 04:16:07AM +0200, Marek Marczykowski-Górecki wrote:
> From: Eric Shelton <eshelton@pobox.com>
> 
> This enum gives the ability to select between a MiniOS-based QEMU
> traditional stub domain and a Linux-based QEMU upstream stub domain.  To
> use the Linux-based stubdomain, the following two lines should be
> included in the appropriate xl.cfg file:
> 
> device_model_version="qemu-xen"
> device_model_stubdomain_override=1
> 
> To use the MiniOS-based stubdomain, the following is used instead:
> 
> device_model_version="qemu-xen-traditional"
> device_model_stubdomain_override=1


Where is stubomd_version= ? Don't you want to expose such option in xl.cfg?

I think the above runes are not very good. Too much inferring is needed.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Wei Liu]

On Tue, Aug 07, 2018 at 04:16:08AM +0200, Marek Marczykowski-Górecki wrote:
> From: Eric Shelton <eshelton@pobox.com>
> 
> This patch creates an appropriate command line for the QEMU instance
> running in a Linux-based stubdomain.
> 
> NOTE: a number of items are not currently implemented for Linux-based
> stubdomains, such as:
> - save/restore
> - QMP socket
> - graphics output (e.g., VNC)
> 
> Signed-off-by: Eric Shelton <eshelton@pobox.com>
> 
> Simon:
>  * fix disk path
>  * fix cdrom path and "format"
>  * pass downscript for network interfaces
> 
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> [drop Qubes-specific parts]
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Looks okay to me. But I need to review it more closely before giving my
ack.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Jason Andryuk]

On Mon, Aug 6, 2018 at 10:22 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> From: Eric Shelton <eshelton@pobox.com>
>
> This patch creates an appropriate command line for the QEMU instance
> running in a Linux-based stubdomain.
>
> NOTE: a number of items are not currently implemented for Linux-based
> stubdomains, such as:
> - save/restore
> - QMP socket
> - graphics output (e.g., VNC)
>
> Signed-off-by: Eric Shelton <eshelton@pobox.com>
>
> Simon:
>  * fix disk path
>  * fix cdrom path and "format"
>  * pass downscript for network interfaces
>
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> [drop Qubes-specific parts]
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v2:
>  - fix serial specified with serial=[ ... ] syntax
>  - error out on multiple consoles (incompatible with stubdom)
>  - drop erroneous chunk about cdrom

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1468 bytes --]

On Thu, Aug 09, 2018 at 10:25:42AM +0100, Wei Liu wrote:
> On Tue, Aug 07, 2018 at 04:16:07AM +0200, Marek Marczykowski-Górecki wrote:
> > From: Eric Shelton <eshelton@pobox.com>
> > 
> > This enum gives the ability to select between a MiniOS-based QEMU
> > traditional stub domain and a Linux-based QEMU upstream stub domain.  To
> > use the Linux-based stubdomain, the following two lines should be
> > included in the appropriate xl.cfg file:
> > 
> > device_model_version="qemu-xen"
> > device_model_stubdomain_override=1
> > 
> > To use the MiniOS-based stubdomain, the following is used instead:
> > 
> > device_model_version="qemu-xen-traditional"
> > device_model_stubdomain_override=1
> 
> 
> Where is stubomd_version= ? Don't you want to expose such option in xl.cfg?

Actually, with patch 04/17 you can set explicit stubdomain path, so
stubdomain_version is only another way (redundant to
device_model_version) to signal what protocol to communicate with
stubdomain should be used. Right now each qemu version have only one
stubdomain protocol:
 - qemu-xen-traditional - "mini os" one
 - qemu-xen - "linux" one - see cover letter

Anyway, I can add stubdomain_version to 08/17 "xl: add stubdomain
related options to xl config parser" patch.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 10/17] xenconsoled: install xenstore watch for all supported consoles

[Wei Liu]

On Tue, Aug 07, 2018 at 04:16:15AM +0200, Marek Marczykowski-Górecki wrote:
> Not only for the primary one (/local/domain/<domid>/console path).
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info."):
> Actually, with patch 04/17 you can set explicit stubdomain path, so
> stubdomain_version is only another way (redundant to
> device_model_version) to signal what protocol to communicate with
> stubdomain should be used. Right now each qemu version have only one
> stubdomain protocol:
>  - qemu-xen-traditional - "mini os" one
>  - qemu-xen - "linux" one - see cover letter

I'm not sure why you need introduce a stubdomain_version variable
internally, or the corresponding stubdomain_version config option, at
all.

Do we expect to have qemu trad on Linux ?  Seems unlikely.  We have
been trying to do modern qemu on minios or rump kernels or something
for years and gotten nowhere.

So maybe what we should have is an internal macro or inline function
called stubdomain_is_linux, so we can add new variants later
internally.

But I don't see the need for this to be in the public and stable libxl
ABI.  Unless I'm missing something.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> [stuff]

Thanks for this.  I'm very keen on this approach and keen on getting
it upstream.  Sorry for the delay reviewing it!  I have been away a
lot.

> General idea is to allow freely set device_model_version and
> device_model_stubdomain_override and choose the right options based on this choice.
> Also, allow to specific path to stubdomain kernel/ramdisk, for greater flexibility.
> Right now when qemu-xen in stubdomain is selected, it is assumed it's
> Linux-based and few decisions are based on it, specifically:

That sounds sensible.

>  - qemu args encoding (\x1b as separator, to allow spaces in arguments)

What if the arguments contain \x1b ?  I think you may need a quoting
scheme, or to use nul.

> It may be a good idea to document "stubdomain API" somewhere. Is there such
> document for MiniOS one?

No.

>  Here is an initial version for Linux one:
> 
>     Assumptions about Linux-based stubdomain for qemu-xen:
>     * qemu command line is stored by toolstack in
>       /vm/<target-uuid>/image/dmargs xenstore entry, arguments are separated
>       with \x1b character

Oh, xenstore.  From docs/misc/xenstore.txt:

| xenstore values should normally be 7-bit ASCII text strings
| containing bytes 0x20..0x7f only

I think you could have separate xenstore keys for the seperate
arguments, or you could encode it somehow.

>     * qemu can access saved state (if any) at its FD 3
>     * qemu can write its state (for save/migration) to its FD 4

That's a description of the protocol to *qemu*.  Is the toolstack then
responsible for the protocol across the domain boundary ?  I think it
would be nice to specify that here too.

>     * qemu expects backend for serial console at /dev/hvc3
>     * disks configured for the target are available as /dev/xvd*, in
>       configuration order
>     * qemu can call /etc/qemu-ifup and /etc/qemu-ifdown to connect/disconnect
>       network interface to appropriate network

Please can we put this in-tree and not in a 00/ cover letter :-).

> Initial version has no QMP support - in initial patches it is completely
> disabled, which means no suspend/restore. QMP normally would be used for PCI
> passthrough setup, but it is worked around with MiniOS-like control protocol
> over xenstore, which then is translated to QMP on stubdomain side.

How unpleasant.  But better than nothing.

> Some option is to use separate console for that, but that require
> xenstoled supporting multiple consoles per domain (the goal is to not have qemu
> in dom0 at all). Also, it would be preferable to evaluate how libxl
> handle potentially malicious QMP responses.

We are working on that latter point anyway.

> Another option is to use xenstore - either translate everything needed to
> MiniOS-like thing, or write already json-formatted requests to xenstore and
> watch there for responses. When using separate xenstore dir for that, with
> per-command entries (command id as a key name?) that would solve concurrency
> problem.

That would be fine too.

> QMP support over separate console: patch "libxl: access QMP socket via console
> for qemu-in-stubdomain" implements some early PoC of this.
> Major limitation: only one connection at a time and no means of out of
> band reset (and re-negotiate). I've tried adding very simple `qmp_reset`
> command for in-band connection reset, but it is unreliable because of the
> first limitation - xl process running in background hold this connection open
> and every other xl instance interfere with it. On the other hand, for libvirt
> use case one connection could be enough (leaving alone libvirtd restart).

This is awkward, isn't it.  The Xen PV console protocol has no reset
mechanism.  Can we use libvchan here and provide qmp vchans ?

> Xenconsoled patches add support for multiple consoles in xenconsoled, to avoid the need
> for qemu in dom0 for this to work. Multiple consoles for a stubdomain are used for:
>  - logs (console 0)
>  - save + restore (console 1, 2)
>  - qmp (console 3)
>  - serial terminal to target domU (console 4)
> Xenconsoled patches are in fact a separate series, but I'm sending them here to
> ease dependencies handling (latter libxl patches use that).

That sounds reasonable.

> What qmp-libxenstat socket is for?
> 
> PCI passthrough support require some more love. Right now, libxl tries to setup
> pcifront for both target HVM and stubdomain and the former fails (race
> condition):
>     xen-pciback pci-259-0: 22 Couldn't locate PCI device (0000:00:1b.0)! perhaps already in-use?
> Fortunately it isn't needed. There is also a PCI related problem on
> domain shutdown - it looks like first stubdomain is paused and then libxl waits
> for pcifront there.
> Also, MSI doesn't work (qemu output):
> 
>     [00:05.0] xen_pt_msgctrl_reg_write: setup MSI (register: 81).
>     [00:05.0] msi_msix_setup: requested pirq 55 for MSI (vec: 0, entry: 0)
>     [00:05.0] msi_msix_setup: Error: Mapping of MSI (err: 1, vec: 0, entry 0)
>     [00:05.0] xen_pt_msgctrl_reg_write: Warning: Can not map MSI (register: 80)!
>     [00:05.0] Write-back to unknown field 0x44 (partially) inhibited (0x00)
>     [00:05.0] If the device doesn't work, try enabling permissive mode
>     [00:05.0] (unsafe) and if it helps report the problem to xen-devel

I confess I don't understand PCI passthrough but it would be nice for
this to work.  I think a starting point might be to write down who is
supposed to do what...

> This patch series is third (fourth?) attempt to get rid of limitation
> "if you want to use stubdomain, you're stuck with qemu-traditional", done over
> years, by many people.
> I think it should be acceptable plan to gradually add features to
> qemu-xen+stubdomain configuration - not necessary waiting with committing any
> of those patches until full feature set of qemu-xen is supported. After all,
> right now "feature set supported by qemu-xen+stubdom" is empty, so wouldn't be
> worse.

Absolutely.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 01/17] libxl: fix qemu-trad cmdline for no sdl/vnc case

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 01/17] libxl: fix qemu-trad cmdline for no sdl/vnc case"):
> When qemu is running in stubdomain, any attempt to initialize vnc/sdl
> there will crash it (on failed attempt to load a keymap from a file). If
> vfb is present, all those cases are skipped. But since
> b053f0c4c9e533f3d97837cf897eb920b8355ed3 "libxl: do not start dom0 qemu
> for stubdomain when not needed" it is possible to create a stubdomain
> without vfb and contrary to the comment -vnc none do trigger VNC
> initialization code (just skips exposing it externally).
> Change the implicit SDL avoiding method to -nographics option, used when
> none of SDL or VNC is enabled.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index fdd7fa3..ebe8e0c 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -475,14 +475,14 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
>          if (libxl_defbool_val(vnc->findunused)) {
>              flexarray_append(dm_args, "-vncunused");
>          }
> -    } else
> +    } else if (!sdl)
>          /*
>           * VNC is not enabled by default by qemu-xen-traditional,
> -         * however passing -vnc none causes SDL to not be
> -         * (unexpectedly) enabled by default. This is overridden by
> -         * explicitly passing -sdl below as required.
> +         * however skipping -vnc causes SDL to be
> +         * (unexpectedly) enabled by default. If undesired, disable graphics at
> +         * all.
>           */
> -        flexarray_append_pair(dm_args, "-vnc", "none");
> +        flexarray_append(dm_args, "-nographic");
>  

I would welcome a patch to add the missing { } to this last if branch.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options."):
> From: Eric Shelton <eshelton@pobox.com>
> 
> This patch creates an appropriate command line for the QEMU instance
> running in a Linux-based stubdomain.
...
> -static const char *libxl_tapif_script(libxl__gc *gc)
> +static const char *libxl_tapif_script(libxl__gc *gc,
> +                                      const libxl_domain_build_info *info)
>  {
>  #if defined(__linux__) || defined(__FreeBSD__)
> +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> +        return libxl__sprintf(gc, "/etc/qemu-ifup");
> +    return libxl__strdup(gc, "no");
> +#else
> +    return GCSPRINTF("%s/qemu-ifup", libxl__xen_script_dir_path());
> +#endif
> +}
> +
> +static const char *libxl_tapif_downscript(libxl__gc *gc,
> +                                          const libxl_domain_build_info *info)
> +{
> +#if defined(__linux__) || defined(__FreeBSD__)
> +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> +        return libxl__sprintf(gc, "/etc/qemu-ifdown");

We should never have permitted this #ifdefery.  The resulting diff
here is almost incomprehensible due to the 3 levels of improper
nesting: diff, ifdef, and code.

Also we do not currently support any dom0's other than Linux and
FreeBSD anyway!  So the #ifdef is entirely redundant.  This wasn't 
noticed when 2b2ef0c54459722943db6166da28e098af12a9e6
  "libxl: don't use a qemu-ifup script on FreeBSD"
was prepared and accepted.

AFAICT this part of this patch is separating out the down and up
versions of libxl_tapif_script.  The resulting two functions are quite
similar though.

I suggest the following series of small changes:
   1. Drop the #if and all the code in the #else
   2. Add a libxl__device_action parameter to libxl_tapif_script
   3. Make your new code check for linux stubdom and if so
       pass    "qemu" + (action == add) ? "up" :
                        (action == remove) ? "down" : (abort(),0)
       or some such

What do you think ?

> @@ -1099,10 +1118,31 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>                  return ERROR_INVAL;
>              }
>              if (b_info->u.hvm.serial) {
> -                flexarray_vappend(dm_args,
> -                                  "-serial", b_info->u.hvm.serial, NULL);
> -            } else if (b_info->u.hvm.serial_list) {
> +                if (is_stubdom) {
> +                    flexarray_vappend(dm_args,
> +                                      "-serial",
> +                                      GCSPRINTF("/dev/hvc%d",
> +                                                STUBDOM_CONSOLE_SERIAL),
> +                                      NULL);
> +                } else {
> +                    flexarray_vappend(dm_args,
> +                                      "-serial", b_info->u.hvm.serial, NULL);
> +                }
> +            } else if (b_info->u.hvm.serial_list &&
> +                    b_info->u.hvm.serial_list[0]) {
>                  char **p;
> +                if (is_stubdom) {
> +                    if (b_info->u.hvm.serial_list[1]) {

This can't possibly be right.  The 2nd if is in the else of a
condition which will always catch all of its cases.

Also,

> +                    flexarray_vappend(dm_args,
> +                                      "-serial",
> +                                      GCSPRINTF("/dev/hvc%d",
> +                                                STUBDOM_CONSOLE_SERIAL),

it repeats some of the code.

> @@ -1503,7 +1543,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>               * If qemu isn't doing the interpreting, the parameter is
>               * always raw
>               */
> -            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
> +            if (libxl_defbool_val(b_info->device_model_stubdomain))
> +                format = "host_device";

So I infer that you have created in qemu a "block device format"
called "host_device" which is actually a pv frontend ?  Or are we
using Linux's blkfront here ?  In which case why not "raw" ?

> +            } else if (libxl_defbool_val(b_info->device_model_stubdomain)) {
> +                target_path = GCSPRINTF("/dev/xvd%c", 'a' + disk);

Needs an error check in case disk is too large.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 7452 bytes --]

On Tue, Oct 16, 2018 at 05:53:02PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > [stuff]
> 
> Thanks for this.  I'm very keen on this approach and keen on getting
> it upstream.  Sorry for the delay reviewing it!  I have been away a
> lot.
> 
> > General idea is to allow freely set device_model_version and
> > device_model_stubdomain_override and choose the right options based on this choice.
> > Also, allow to specific path to stubdomain kernel/ramdisk, for greater flexibility.
> > Right now when qemu-xen in stubdomain is selected, it is assumed it's
> > Linux-based and few decisions are based on it, specifically:
> 
> That sounds sensible.
> 
> >  - qemu args encoding (\x1b as separator, to allow spaces in arguments)
> 
> What if the arguments contain \x1b ? 

Well, here I've changed "What if the arguments contain a space" to "What
if the arguments contain \x1b" ;)

> I think you may need a quoting
> scheme, or to use nul.

The main reason for this over alternatives (including nul) is using
shell script on the stubdomain side to handle this. Handling nul char in
shell is major PITA.

> > It may be a good idea to document "stubdomain API" somewhere. Is there such
> > document for MiniOS one?
> 
> No.
> 
> >  Here is an initial version for Linux one:
> > 
> >     Assumptions about Linux-based stubdomain for qemu-xen:
> >     * qemu command line is stored by toolstack in
> >       /vm/<target-uuid>/image/dmargs xenstore entry, arguments are separated
> >       with \x1b character
> 
> Oh, xenstore.  From docs/misc/xenstore.txt:
> 
> | xenstore values should normally be 7-bit ASCII text strings
> | containing bytes 0x20..0x7f only
> 
> I think you could have separate xenstore keys for the seperate
> arguments, or you could encode it somehow.

What "normally" means in this context? For me it doesn't mean other
characters are prohibited.

> >     * qemu can access saved state (if any) at its FD 3
> >     * qemu can write its state (for save/migration) to its FD 4
> 
> That's a description of the protocol to *qemu*.  Is the toolstack then
> responsible for the protocol across the domain boundary ?  I think it
> would be nice to specify that here too.

Well, toolstack is responsible for qemu command line (and QMP), so it is
part of the stubdomain protocol...

> 
> >     * qemu expects backend for serial console at /dev/hvc3
> >     * disks configured for the target are available as /dev/xvd*, in
> >       configuration order
> >     * qemu can call /etc/qemu-ifup and /etc/qemu-ifdown to connect/disconnect
> >       network interface to appropriate network

Actually, after recent changes in our linux stubdomain, this last point
wouldn't be needed. It's handled internally by observing qmp events.

> Please can we put this in-tree and not in a 00/ cover letter :-).

Sure.

> > Initial version has no QMP support - in initial patches it is completely
> > disabled, which means no suspend/restore. QMP normally would be used for PCI
> > passthrough setup, but it is worked around with MiniOS-like control protocol
> > over xenstore, which then is translated to QMP on stubdomain side.
> 
> How unpleasant.  But better than nothing.
>
> > Some option is to use separate console for that, but that require
> > xenstoled supporting multiple consoles per domain (the goal is to not have qemu
> > in dom0 at all). Also, it would be preferable to evaluate how libxl
> > handle potentially malicious QMP responses.
> 
> We are working on that latter point anyway.
> 
> > Another option is to use xenstore - either translate everything needed to
> > MiniOS-like thing, or write already json-formatted requests to xenstore and
> > watch there for responses. When using separate xenstore dir for that, with
> > per-command entries (command id as a key name?) that would solve concurrency
> > problem.
> 
> That would be fine too.
> 
> > QMP support over separate console: patch "libxl: access QMP socket via console
> > for qemu-in-stubdomain" implements some early PoC of this.
> > Major limitation: only one connection at a time and no means of out of
> > band reset (and re-negotiate). I've tried adding very simple `qmp_reset`
> > command for in-band connection reset, but it is unreliable because of the
> > first limitation - xl process running in background hold this connection open
> > and every other xl instance interfere with it. On the other hand, for libvirt
> > use case one connection could be enough (leaving alone libvirtd restart).
> 
> This is awkward, isn't it.  The Xen PV console protocol has no reset
> mechanism.  Can we use libvchan here and provide qmp vchans ?

That would be an option too. Require (trivial) vchan->unix socket proxy.

> > Xenconsoled patches add support for multiple consoles in xenconsoled, to avoid the need
> > for qemu in dom0 for this to work. Multiple consoles for a stubdomain are used for:
> >  - logs (console 0)
> >  - save + restore (console 1, 2)
> >  - qmp (console 3)
> >  - serial terminal to target domU (console 4)
> > Xenconsoled patches are in fact a separate series, but I'm sending them here to
> > ease dependencies handling (latter libxl patches use that).
> 
> That sounds reasonable.
> 
> > What qmp-libxenstat socket is for?
> > 
> > PCI passthrough support require some more love. Right now, libxl tries to setup
> > pcifront for both target HVM and stubdomain and the former fails (race
> > condition):
> >     xen-pciback pci-259-0: 22 Couldn't locate PCI device (0000:00:1b.0)! perhaps already in-use?
> > Fortunately it isn't needed. There is also a PCI related problem on
> > domain shutdown - it looks like first stubdomain is paused and then libxl waits
> > for pcifront there.
> > Also, MSI doesn't work (qemu output):
> > 
> >     [00:05.0] xen_pt_msgctrl_reg_write: setup MSI (register: 81).
> >     [00:05.0] msi_msix_setup: requested pirq 55 for MSI (vec: 0, entry: 0)
> >     [00:05.0] msi_msix_setup: Error: Mapping of MSI (err: 1, vec: 0, entry 0)
> >     [00:05.0] xen_pt_msgctrl_reg_write: Warning: Can not map MSI (register: 80)!
> >     [00:05.0] Write-back to unknown field 0x44 (partially) inhibited (0x00)
> >     [00:05.0] If the device doesn't work, try enabling permissive mode
> >     [00:05.0] (unsafe) and if it helps report the problem to xen-devel
> 
> I confess I don't understand PCI passthrough but it would be nice for
> this to work.  I think a starting point might be to write down who is
> supposed to do what...
> 
> > This patch series is third (fourth?) attempt to get rid of limitation
> > "if you want to use stubdomain, you're stuck with qemu-traditional", done over
> > years, by many people.
> > I think it should be acceptable plan to gradually add features to
> > qemu-xen+stubdomain configuration - not necessary waiting with committing any
> > of those patches until full feature set of qemu-xen is supported. After all,
> > right now "feature set supported by qemu-xen+stubdom" is empty, so wouldn't be
> > worse.
> 
> Absolutely.
> 
> Ian.
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1781 bytes --]

On Tue, Oct 16, 2018 at 05:43:34PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 02/17] libxl: Add "stubdomain_version" to domain_build_info."):
> > Actually, with patch 04/17 you can set explicit stubdomain path, so
> > stubdomain_version is only another way (redundant to
> > device_model_version) to signal what protocol to communicate with
> > stubdomain should be used. Right now each qemu version have only one
> > stubdomain protocol:
> >  - qemu-xen-traditional - "mini os" one
> >  - qemu-xen - "linux" one - see cover letter
> 
> I'm not sure why you need introduce a stubdomain_version variable
> internally, or the corresponding stubdomain_version config option, at
> all.
> 
> Do we expect to have qemu trad on Linux ?  Seems unlikely.  We have
> been trying to do modern qemu on minios or rump kernels or something
> for years and gotten nowhere.
> 
> So maybe what we should have is an internal macro or inline function
> called stubdomain_is_linux, so we can add new variants later
> internally.
> 
> But I don't see the need for this to be in the public and stable libxl
> ABI.  Unless I'm missing something.

I imagine there may be more stubdomain versions in the future,
especially when the stubdomain path can be easily set in domain config.
But it's ok to add such field only if/when we'll actually have multiple
of them supported in upstream libxl. As long as the code will be clear
about what it "qemu-xen specific" and what is "linux stubdomain
specific". But stubdomain_is_linux macro should be enough.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 5442 bytes --]

On Tue, Oct 16, 2018 at 06:16:41PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options."):
> > From: Eric Shelton <eshelton@pobox.com>
> > 
> > This patch creates an appropriate command line for the QEMU instance
> > running in a Linux-based stubdomain.
> ...
> > -static const char *libxl_tapif_script(libxl__gc *gc)
> > +static const char *libxl_tapif_script(libxl__gc *gc,
> > +                                      const libxl_domain_build_info *info)
> >  {
> >  #if defined(__linux__) || defined(__FreeBSD__)
> > +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> > +        return libxl__sprintf(gc, "/etc/qemu-ifup");
> > +    return libxl__strdup(gc, "no");
> > +#else
> > +    return GCSPRINTF("%s/qemu-ifup", libxl__xen_script_dir_path());
> > +#endif
> > +}
> > +
> > +static const char *libxl_tapif_downscript(libxl__gc *gc,
> > +                                          const libxl_domain_build_info *info)
> > +{
> > +#if defined(__linux__) || defined(__FreeBSD__)
> > +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> > +        return libxl__sprintf(gc, "/etc/qemu-ifdown");
> 
> We should never have permitted this #ifdefery.  The resulting diff
> here is almost incomprehensible due to the 3 levels of improper
> nesting: diff, ifdef, and code.
> 
> Also we do not currently support any dom0's other than Linux and
> FreeBSD anyway!  So the #ifdef is entirely redundant.  This wasn't 
> noticed when 2b2ef0c54459722943db6166da28e098af12a9e6
>   "libxl: don't use a qemu-ifup script on FreeBSD"
> was prepared and accepted.
> 
> AFAICT this part of this patch is separating out the down and up
> versions of libxl_tapif_script.  The resulting two functions are quite
> similar though.
> 
> I suggest the following series of small changes:
>    1. Drop the #if and all the code in the #else
>    2. Add a libxl__device_action parameter to libxl_tapif_script
>    3. Make your new code check for linux stubdom and if so
>        pass    "qemu" + (action == add) ? "up" :
>                         (action == remove) ? "down" : (abort(),0)
>        or some such
> 
> What do you think ?

Given updated stubdomain doesn't need qemu-ifup/ifdown at all, this
whole chunk can be dropped.

> > @@ -1099,10 +1118,31 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >                  return ERROR_INVAL;
> >              }
> >              if (b_info->u.hvm.serial) {
> > -                flexarray_vappend(dm_args,
> > -                                  "-serial", b_info->u.hvm.serial, NULL);
> > -            } else if (b_info->u.hvm.serial_list) {
> > +                if (is_stubdom) {
> > +                    flexarray_vappend(dm_args,
> > +                                      "-serial",
> > +                                      GCSPRINTF("/dev/hvc%d",
> > +                                                STUBDOM_CONSOLE_SERIAL),
> > +                                      NULL);
> > +                } else {
> > +                    flexarray_vappend(dm_args,
> > +                                      "-serial", b_info->u.hvm.serial, NULL);
> > +                }
> > +            } else if (b_info->u.hvm.serial_list &&
> > +                    b_info->u.hvm.serial_list[0]) {
> >                  char **p;
> > +                if (is_stubdom) {
> > +                    if (b_info->u.hvm.serial_list[1]) {
> 
> This can't possibly be right.  The 2nd if is in the else of a
> condition which will always catch all of its cases.

Oh, indeed.

> Also,
> 
> > +                    flexarray_vappend(dm_args,
> > +                                      "-serial",
> > +                                      GCSPRINTF("/dev/hvc%d",
> > +                                                STUBDOM_CONSOLE_SERIAL),
> 
> it repeats some of the code.
> 
> > @@ -1503,7 +1543,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >               * If qemu isn't doing the interpreting, the parameter is
> >               * always raw
> >               */
> > -            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
> > +            if (libxl_defbool_val(b_info->device_model_stubdomain))
> > +                format = "host_device";
> 
> So I infer that you have created in qemu a "block device format"
> called "host_device" which is actually a pv frontend ?  Or are we
> using Linux's blkfront here ?  In which case why not "raw" ?

"format" is actually passed to "driver" option for -drive. And according
to qemu documentation, "raw" should be used only for regular file, not
block devices. And since qemu 3.0, "raw" driver rejects block devices[1].

> > +            } else if (libxl_defbool_val(b_info->device_model_stubdomain)) {
> > +                target_path = GCSPRINTF("/dev/xvd%c", 'a' + disk);
> 
> Needs an error check in case disk is too large.

Ok.

> Thanks,
> Ian.

[1] https://qemu.weilnetz.de/doc/qemu-doc.html#g_t_002ddrive-file_003djson_003a_007b_002e_002e_002e_007b_0027driver_0027_003a_0027file_0027_007d_007d-_0028since-3_002e0_0029

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> On Tue, Oct 16, 2018 at 05:53:02PM +0100, Ian Jackson wrote:
> > I think you may need a quoting
> > scheme, or to use nul.
> 
> The main reason for this over alternatives (including nul) is using
> shell script on the stubdomain side to handle this. Handling nul char in
> shell is major PITA.

Ah.  Yes.  You could handle multiple arguments in multiple xenstore
keys easily enough though, I think ?

Or you could pass a shell string.  That is, you could shell escape the
arguments in libxl.  Shell escaping is a bit fiddly but not too
hard.[1] Then in the stubdom you can just say sh -c.

[1] One algorithm would be
   1. replace all \ in each argument with '\\'
   2. replace all ' in each argument with '\''
   3. put ' ' around each argument
   4. concatenate with spaces in between

> > | xenstore values should normally be 7-bit ASCII text strings
> > | containing bytes 0x20..0x7f only
> > 
> > I think you could have separate xenstore keys for the seperate
> > arguments, or you could encode it somehow.
> 
> What "normally" means in this context? For me it doesn't mean other
> characters are prohibited.

The reasoning is explained in the surrounding text,  Basically, it
makes debugging (listing xenstore by hand, etc) very annoying.

> > >     * qemu can access saved state (if any) at its FD 3
> > >     * qemu can write its state (for save/migration) to its FD 4
> > 
> > That's a description of the protocol to *qemu*.  Is the toolstack then
> > responsible for the protocol across the domain boundary ?  I think it
> > would be nice to specify that here too.
> 
> Well, toolstack is responsible for qemu command line (and QMP), so it is
> part of the stubdomain protocol...

Err, I mean, you are specifying a protocol at the qemu boundary.  It
is good to write that down.  But it would be nice to also write down
the protocol at the stubdom boundary, even though both halves of it
are actually implemented by part of the toolstack (the stubdom side
being scripts passed in by the toolstack).

> > This is awkward, isn't it.  The Xen PV console protocol has no reset
> > mechanism.  Can we use libvchan here and provide qmp vchans ?
> 
> That would be an option too. Require (trivial) vchan->unix socket proxy.

Yes.  Or teaching qemu about libvchan.
Hey, we should teach socat about libvchan :-).

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 3333 bytes --]

On Tue, Oct 16, 2018 at 06:52:36PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > On Tue, Oct 16, 2018 at 05:53:02PM +0100, Ian Jackson wrote:
> > > I think you may need a quoting
> > > scheme, or to use nul.
> > 
> > The main reason for this over alternatives (including nul) is using
> > shell script on the stubdomain side to handle this. Handling nul char in
> > shell is major PITA.
> 
> Ah.  Yes.  You could handle multiple arguments in multiple xenstore
> keys easily enough though, I think ?
> 
> Or you could pass a shell string. 

From those two options I'd prefer multiple xenstore keys as much
cleaner. We do have them as an array in libxl already.

So, let image/dmargs be actually a directory with entries like:
 - image/dmargs/000 = -xen-domid
 - image/dmargs/001 = 123
 - image/dmargs/002 = -nodefaults
 ...

I wonder if there needs to be arguments count, or iterating until ENOENT
is enough?

> That is, you could shell escape the
> arguments in libxl.  Shell escaping is a bit fiddly but not too
> hard.[1] Then in the stubdom you can just say sh -c.
> 
> [1] One algorithm would be
>    1. replace all \ in each argument with '\\'
>    2. replace all ' in each argument with '\''
>    3. put ' ' around each argument
>    4. concatenate with spaces in between
> 
> > > | xenstore values should normally be 7-bit ASCII text strings
> > > | containing bytes 0x20..0x7f only
> > > 
> > > I think you could have separate xenstore keys for the seperate
> > > arguments, or you could encode it somehow.
> > 
> > What "normally" means in this context? For me it doesn't mean other
> > characters are prohibited.
> 
> The reasoning is explained in the surrounding text,  Basically, it
> makes debugging (listing xenstore by hand, etc) very annoying.
> 
> > > >     * qemu can access saved state (if any) at its FD 3
> > > >     * qemu can write its state (for save/migration) to its FD 4
> > > 
> > > That's a description of the protocol to *qemu*.  Is the toolstack then
> > > responsible for the protocol across the domain boundary ?  I think it
> > > would be nice to specify that here too.
> > 
> > Well, toolstack is responsible for qemu command line (and QMP), so it is
> > part of the stubdomain protocol...
> 
> Err, I mean, you are specifying a protocol at the qemu boundary.  It
> is good to write that down.  But it would be nice to also write down
> the protocol at the stubdom boundary, even though both halves of it
> are actually implemented by part of the toolstack (the stubdom side
> being scripts passed in by the toolstack).
> 
> > > This is awkward, isn't it.  The Xen PV console protocol has no reset
> > > mechanism.  Can we use libvchan here and provide qmp vchans ?
> > 
> > That would be an option too. Require (trivial) vchan->unix socket proxy.
> 
> Yes.  Or teaching qemu about libvchan.

That's also an option. I'll see how hard it would be to add this to
qemu.

> Hey, we should teach socat about libvchan :-).

:)

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> From those two options I'd prefer multiple xenstore keys as much
> cleaner. We do have them as an array in libxl already.
> 
> So, let image/dmargs be actually a directory with entries like:
>  - image/dmargs/000 = -xen-domid
>  - image/dmargs/001 = 123
>  - image/dmargs/002 = -nodefaults
>
> I wonder if there needs to be arguments count, or iterating until ENOENT
> is enough?

xenstore-read doesn't seem to provide an easy way for a shell script
to tell ENOENT apart from "everything is doomed".  Here is its
(frankly, very poor) error handling:

            char *val = xs_read(xsh, xth, argv[optind], &len);
            if (val == NULL) {
                warnx("couldn't read path %s", argv[optind]);
                return 1;
            }

It doesn't even print the errno value, let alone change the exit
status or provide a way to tolerate ENOENT without bulrbing to stderr.

If I were you I'd send a shell string but it's entirely up to you.

> > Yes.  Or teaching qemu about libvchan.
> 
> That's also an option. I'll see how hard it would be to add this to
> qemu.

Good luck.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> [stuff]

So, I only got a little way through this series, but it was a while
ago and you say things are done differently now.  I'm not sure if it
is useful for me to review the rest of it.

Would it be better for me to await a repost ?

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2292 bytes --]

On Wed, Oct 17, 2018 at 04:16:03PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > [stuff]
> 
> So, I only got a little way through this series, but it was a while
> ago and you say things are done differently now.  I'm not sure if it
> is useful for me to review the rest of it.
> 
> Would it be better for me to await a repost ?

All the xenconsoled stuff is unchanged (and unlikely to change), so it
should be safe to review it. Patches 06 and 07 also shouldn't change.

The thing that will change is qemu cmdline and qmp handling. TBH I'm not sure
if its better to touch qmp now, or after reworked qmp handling by
Anthony will be merged. There will definitely be some conflicts and it
may even affects what underlying mechanism is chosen for qmp transport.
Based on discussion here, and in libxl__ev_qmp_* thread, I see two
viable options:

1. libvchan
  pros:
   - out of band reset support, so qmp capabilities negotiation can be
     handled gracefully
  cons:
   - more work, require patching qemu (or adding vchan->socket proxy),
     adds dependency on libvchan to libxl (probably not a problem)
   - possibly more complex libxl__ev_qmp_* handling, or at least needs
     separate handling of send/receive for stubdomain case
2. pv console
  pros:
   - no qemu modifications
   - same read()/write() on libxl side
  cons:
   - no out of band reset, needs libxl handling for that (skipping
     negotiation)
   - possibly other problems from that (events filling up some buffers
     when no one is listening?)

In both cases, there is only one simultaneous connection to qemu, so
some locking will be needed at libxl side.
BTW Does libxl listed for qmp events?

There is also third option: xenstore, but that would probably require
totally separate libxl__ev_qmp_* implementation, so I'd rule it out.

If problems with pv console could be solved, I'd go this way. But
otherwise libvchan seems like a good alternative.

Adding Anthony.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options.

[Roger Pau Monné]

On Tue, Oct 16, 2018 at 06:16:41PM +0100, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 03/17] libxl: Handle Linux stubdomain specific QEMU options."):
> > From: Eric Shelton <eshelton@pobox.com>
> > 
> > This patch creates an appropriate command line for the QEMU instance
> > running in a Linux-based stubdomain.
> ...
> > -static const char *libxl_tapif_script(libxl__gc *gc)
> > +static const char *libxl_tapif_script(libxl__gc *gc,
> > +                                      const libxl_domain_build_info *info)
> >  {
> >  #if defined(__linux__) || defined(__FreeBSD__)
> > +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> > +        return libxl__sprintf(gc, "/etc/qemu-ifup");
> > +    return libxl__strdup(gc, "no");
> > +#else
> > +    return GCSPRINTF("%s/qemu-ifup", libxl__xen_script_dir_path());
> > +#endif
> > +}
> > +
> > +static const char *libxl_tapif_downscript(libxl__gc *gc,
> > +                                          const libxl_domain_build_info *info)
> > +{
> > +#if defined(__linux__) || defined(__FreeBSD__)
> > +    if (info->stubdomain_version == LIBXL_STUBDOMAIN_VERSION_LINUX)
> > +        return libxl__sprintf(gc, "/etc/qemu-ifdown");
> 
> We should never have permitted this #ifdefery.  The resulting diff
> here is almost incomprehensible due to the 3 levels of improper
> nesting: diff, ifdef, and code.
> 
> Also we do not currently support any dom0's other than Linux and
> FreeBSD anyway!

I'm not sure this is entirely true, IIRC NetBSD requires a QEMU nic
hotplug script and is still supported:

http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/sysutils/xentools48/

That's the reason why we require such ifdefery.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> All the xenconsoled stuff is unchanged (and unlikely to change), so it
> should be safe to review it. Patches 06 and 07 also shouldn't change.

Sorry, I missed this reply.  I will go on to review those.

> The thing that will change is qemu cmdline and qmp handling. TBH I'm not sure
> if its better to touch qmp now, or after reworked qmp handling by
> Anthony will be merged. There will definitely be some conflicts and it
> may even affects what underlying mechanism is chosen for qmp transport.
> Based on discussion here, and in libxl__ev_qmp_* thread, I see two
> viable options:
> 
> 1. libvchan
>   pros:
>    - out of band reset support, so qmp capabilities negotiation can be
>      handled gracefully
>   cons:
>    - more work, require patching qemu (or adding vchan->socket proxy),
>      adds dependency on libvchan to libxl (probably not a problem)
>    - possibly more complex libxl__ev_qmp_* handling, or at least needs
>      separate handling of send/receive for stubdomain case

I think that the changes to libxl__ev_qmp_* will be relatively
self-contained, particularly after Anthony's async rework.  There's
one place where the communication occurs.

Does libvchan offer asynchronous connection (ie, connect/disconnect
calls which cannot be stalled by the peer, but which instead allow
poll/select-based async) ?  I think it may not, in which case we need
a vchan to socket proxy anyway.

Aside from that the libxl dependency is untroublesome.

> 2. pv console
>   pros:
>    - no qemu modifications
>    - same read()/write() on libxl side
>   cons:
>    - no out of band reset, needs libxl handling for that (skipping
>      negotiation)

Doesn't this potentially mean that the qmp console connection can
become irrecoverably desynchronised ?  I don't know how you would
recover from the situation where another libxl process had got halfway
through some qmp stuff and been terminated (for whatever reason; maybe
the calling toolstack crashed).

>    - possibly other problems from that (events filling up some buffers
>      when no one is listening?)

xenconsole drops things in this situation.  I think that may result in
desynchronisation.

> BTW Does libxl listed for qmp events?

Not right now.  It may want to in future.  Anthony's qmp series
discards events.

> There is also third option: xenstore, but that would probably require
> totally separate libxl__ev_qmp_* implementation, so I'd rule it out.

That's not a terrible idea but I can't see it being popular with qemu
upstream, so you'd end up writing a kind of bidirectional
qmp<->xenstore proxy.  Urgh.

> If problems with pv console could be solved, I'd go this way. But
> otherwise libvchan seems like a good alternative.

Yes.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 06/17] libxl: create vkb device only for guests with graphics output

[Ian Jackson]

(CCing Stefano's new address.)

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 06/17] libxl: create vkb device only for guests with graphics output"):
> The forced vkb device is meant for better performance of qemu access
> (at least according to ebbd2561b4cefb299f0f68a88b2788504223de18 "libxl:
> Add a vkbd frontend/backend pair for HVM guests"), which isn't used if
> there is no configured channel to actually access that keyboard.

I think the background here is that the "usb keyboard/mouse" referred
to in ebbd2561 is supposedly used only for vnc ?

I think we still support SDL though, so maybe that needs to be checked
too.  I did `git-grep -i sdl tools/libxl' and there's a few
occurrences of something like this
  tools/libxl/libxl_dm.c:        if (!sdl && !vnc)
in particular, in one of these cases it passes -nographic to qemu.

Probably the situations where we shouldn't unconditionally provide a
vkb are those where -nographic is passed to qemu ?

Maybe we need a helper inline function for sdl || vnc.  If you decide
to introduce one, please do that in a patch before this one.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain"):
> Rely on a wrapper script in stubdomain to attach FD 3/4 of qemu to
> relevant consoles.
...
>      if (state->saved_state) {
> -        /* This file descriptor is meant to be used by QEMU */
> -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> -        flexarray_append(dm_args, "-incoming");
> -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> +        if (is_stubdom) {
> +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> +             */
> +            flexarray_append(dm_args, "-incoming");
> +            flexarray_append(dm_args, "fd:3");

I think this hardcoded fd is troublesome.  For example, we don't have
anywhere to write down the list of hardcoded fds being used like this.
I mean, libxl and the Linux qemu stubdom wrapper script are allowed to
cooperate, but at least this needs a clear comment in the wrapper
script, and a reference here to the in-tree location of the script.

I'm missing the code which is transfers the data from the
state->saved_state to the console.  Am I just being dim ?

> diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
...
>          /* Save DM state into filename */
> +        if (dm_domid) {
> +            /* if DM is in stubdomain, instruct it to use console, which is
> +             * connected to a file pointed by filename */
> +            filename = "/proc/self/fd/4";

Same comment (mutatis mutandi).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2157 bytes --]

On Thu, Nov 01, 2018 at 05:11:21PM +0000, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain"):
> > Rely on a wrapper script in stubdomain to attach FD 3/4 of qemu to
> > relevant consoles.
> ...
> >      if (state->saved_state) {
> > -        /* This file descriptor is meant to be used by QEMU */
> > -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> > -        flexarray_append(dm_args, "-incoming");
> > -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> > +        if (is_stubdom) {
> > +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> > +             */
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, "fd:3");
> 
> I think this hardcoded fd is troublesome.  For example, we don't have
> anywhere to write down the list of hardcoded fds being used like this.
> I mean, libxl and the Linux qemu stubdom wrapper script are allowed to
> cooperate, but at least this needs a clear comment in the wrapper
> script, and a reference here to the in-tree location of the script.

This is exactly what I'm writing about in cover letter. And indeed some
#define would be helpful here.

> I'm missing the code which is transfers the data from the
> state->saved_state to the console.  Am I just being dim ?

This is done by existing code by connecting STUBDOM_CONSOLE_RESTORE to
that file. See libxl_dm.c:spawn_stub_launch_dm.

> > diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
> ...
> >          /* Save DM state into filename */
> > +        if (dm_domid) {
> > +            /* if DM is in stubdomain, instruct it to use console, which is
> > +             * connected to a file pointed by filename */
> > +            filename = "/proc/self/fd/4";
> 
> Same comment (mutatis mutandi).
> 
> Ian.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry"):
> Add support for standard xenbus initialization protocol using 'state'
> xenstore entry. It will be necessary for secondary consoles.
> For consoles supporting it, read 'state' entry on the frontend and
> proceed accordingly - either init console or close it. When closing,
> make sure all the in-transit data is flushed (both from shared ring and
> from local buffer), if possible. This is especially important for
> MiniOS-based qemu stubdomain, which closes console just after writing
> device model state to it.
> For consoles without 'state' field behavior is unchanged - on any watch
> event try to connect console, as long as domain is alive.

I'm not opposed to the introduction of this state field.  The code
looks plausible.

But:

Firstly, you have put the protocol description in your commit
message (and it seems rather informal).  Can you please provide
a comprehensive protocol specification in-tree ?  You need to patch
   docs/misc/console.txt
I think.

I say `comprehensive' because your text is not particularly clearly
about who is supposed to `flush' which data exactly when.  Nor what
`flushing' means (does it ever mean discarding?)

Secondly: what about backwards compatibility ?  I think we need to at
least think about the possibility that there are some guest frontends
out there which may look for a `state' node and do something
undesirable with it.  I think this possibility is remote but it should
be mentioned in the commit message.

What about the possibility that one or the other end of the connection
may be replaced by a different implementation, so that the peer
appears to gain or lose support for `state' ?

I'll be able to review the code more effectively when there is a
formal protocol spec to compare it to...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 12/17] xenconsoled: make console_type->use_gnttab less confusing

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 12/17] xenconsoled: make console_type->use_gnttab less confusing"):
> Before this commit 'use_gnttab' means xenconsoled should first try
> special GNTTAB_RESERVED_CONSOLE entry, and only then fallback to
> ring-ref xenstore entry (being gfn of actual ring).
> In case of secondary consoles, ring-ref entry contains grant table
> reference (not gfn of it), which makes the old meaning of use_gnttab
> really confusing (should be false for such consoles).
> To solve this, add new entry in console_type (and console) structures
> named 'use_reserverd_gnttab' with the old meaning of 'use_gnttab', then
> use 'ues_gnttab' for consoles where ring-ref holds grant table
> reference.

I'm afraid I don't have the mental bandwidth to deal with this patch
properly right now but if you change the meaning of `use_gnttab' you
should certainly rename it.  Otherwise you risk failing to update some
part of the code for the new semantics.

Also, I feel I'm being dim, but I don't understand what combinations
of your (new) use_gnttab and use_reserved_gnttab are legitimate and
what their meanings are.  Maybe this wants to be one enum instead of
two booleans.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles"):
> Based on previous few commits, this adds basic support for multiple
> consoles in xenconsoled. A static number of them - up to 3 (+ one
> primary).

I'm confused.  I thought we already had support for multiple PV
consoles.  Is the problem that the backend is in qemu rather than
xenconsoled ?  I'm not sure how this patch interacts with the
qemu-provided extra PV consoles in docs/misc/console.txt.

I'm afraid (as you probably predicted) I don't think this patch is
suitable for upstream in its current form.  I can see that
restructuring xenconsoled to be more dynamic is some work but I may be
able to help there.

CCing Stefano, the author of docs/misc/console.txt, at his new
address.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 14/17] xenconsoled: deduplicate error handling

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 14/17] xenconsoled: deduplicate error handling"):
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 4707 bytes --]

On Thu, Nov 01, 2018 at 04:57:18PM +0000, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > All the xenconsoled stuff is unchanged (and unlikely to change), so it
> > should be safe to review it. Patches 06 and 07 also shouldn't change.
> 
> Sorry, I missed this reply.  I will go on to review those.
> 
> > The thing that will change is qemu cmdline and qmp handling. TBH I'm not sure
> > if its better to touch qmp now, or after reworked qmp handling by
> > Anthony will be merged. There will definitely be some conflicts and it
> > may even affects what underlying mechanism is chosen for qmp transport.
> > Based on discussion here, and in libxl__ev_qmp_* thread, I see two
> > viable options:
> > 
> > 1. libvchan
> >   pros:
> >    - out of band reset support, so qmp capabilities negotiation can be
> >      handled gracefully
> >   cons:
> >    - more work, require patching qemu (or adding vchan->socket proxy),
> >      adds dependency on libvchan to libxl (probably not a problem)
> >    - possibly more complex libxl__ev_qmp_* handling, or at least needs
> >      separate handling of send/receive for stubdomain case
> 
> I think that the changes to libxl__ev_qmp_* will be relatively
> self-contained, particularly after Anthony's async rework.  There's
> one place where the communication occurs.
> 
> Does libvchan offer asynchronous connection (ie, connect/disconnect
> calls which cannot be stalled by the peer, but which instead allow
> poll/select-based async) ?  I think it may not, in which case we need
> a vchan to socket proxy anyway.

libxenvchan_server_init is asynchronous. libxenvchan_client_init is too,
but it fails if called before server is ready. I have a
wrapper[1] around libxenvchan_client_init in Qubes code which solve this
problem with xs_watch. "libvchan: create xenstore entries in one
transaction" patch is related to that wrapper.

Maybe it should be also added to libxenvchan? Right now it only waits
(synchronously) for server to appear (using while(...) xs_read_watch()).
This is slightly more complex, as it also handle remote domain death
before establishing connection as well as save+restore local domain.
But it can be provided as a separate function like
libxenvchan_client_wait_for_server or such.

Providing a function that could be used in libxl would be more complex,
as it needs to integrate with libxl async API. Maybe it could use
good old trick with separate thread + pipe() for signaling readiness?
This way, the libxenvchan_client_wait_for_server would start separate
thread (using own xenstore connection) and return fd that libxl can wait
on. No need to convert libxenvchan_client_wait_for_server into callback
hell...

[1] https://github.com/QubesOS/qubes-core-vchan-xen/blob/master/vchan/init.c#L58-L168

> Aside from that the libxl dependency is untroublesome.
> 
> > 2. pv console
> >   pros:
> >    - no qemu modifications
> >    - same read()/write() on libxl side
> >   cons:
> >    - no out of band reset, needs libxl handling for that (skipping
> >      negotiation)
> 
> Doesn't this potentially mean that the qmp console connection can
> become irrecoverably desynchronised ?  I don't know how you would
> recover from the situation where another libxl process had got halfway
> through some qmp stuff and been terminated (for whatever reason; maybe
> the calling toolstack crashed).

That's right, it could result in irrecoverably desynchronised
connection. So, we need out of band reset.

> >    - possibly other problems from that (events filling up some buffers
> >      when no one is listening?)
> 
> xenconsole drops things in this situation.  I think that may result in
> desynchronisation.
> 
> > BTW Does libxl listed for qmp events?
> 
> Not right now.  It may want to in future.  Anthony's qmp series
> discards events.
> 
> > There is also third option: xenstore, but that would probably require
> > totally separate libxl__ev_qmp_* implementation, so I'd rule it out.
> 
> That's not a terrible idea but I can't see it being popular with qemu
> upstream, so you'd end up writing a kind of bidirectional
> qmp<->xenstore proxy.  Urgh.

Well, I do that already (for pci-ins). In bash.

> > If problems with pv console could be solved, I'd go this way. But
> > otherwise libvchan seems like a good alternative.
> 
> Yes.
> 
> Ian.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 15/17] xenconsoled: add support for non-pty output

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 15/17] xenconsoled: add support for non-pty output"):
> Handle 'output' xenstore entry, as qemu does. Right now support only few
> simple options:
>  - "pty" (unchanged)
>  - "file:path" (overwrite file)
>  - "pipe:path" (read-write file/pipe)
>  - "null"

I have always thought the qemu set of console things very awkward to
deal with.  pipe, in particular, is very awkward to use because pipes
have poor semantics for this.

Would it be useful if I implemented a facility for xenconsoled to make
an AF_UNIX listening socket for each console it handles ?  People who
wanted to talk to the console would connect() to it.

Multiple connections would be supported a la screen or tmux, although
of course for your application you'd use a lock to prevent multiple
access.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 16/17] libxl: access QMP socket via console for qemu-in-stubdomain

[Ian Jackson]

Marek Marczykowski-Górecki writes ("[RFC PATCH v2 16/17] libxl: access QMP socket via console for qemu-in-stubdomain"):
> Add support for talking with qemu in stubdomain via QMP connected to a
> console. Since a console doesn't have out of band connect/disconnect
> signaling, use (new) qmp_reset command at every connect, to force
> renegotiation.

I'm afraid that this is no good because it's dealing with the old
synchronous qmp code - code which Anthony (CC'd) is restructuring.

Since libxl must not trust the qmp connection to a linux stub dm, any
more than to a dom0 depriv qemu dm, I think Anthony's work is a
prerequisite for Linux stubdomain support (upstream, at least).

So this patch is currently blocked and will need to be reworked in due
course.  I'm hoping it will get a lot simpler if we can sort out
(i) async qmp along the lines of Anthony's series (ii) pv console stuff.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 07/17] libxl: add save/restore support for qemu-xen in stubdomain"):
> On Thu, Nov 01, 2018 at 05:11:21PM +0000, Ian Jackson wrote:
> > I think this hardcoded fd is troublesome.  For example, we don't have
> > anywhere to write down the list of hardcoded fds being used like this.
> > I mean, libxl and the Linux qemu stubdom wrapper script are allowed to
> > cooperate, but at least this needs a clear comment in the wrapper
> > script, and a reference here to the in-tree location of the script.
> 
> This is exactly what I'm writing about in cover letter. And indeed some
> #define would be helpful here.

Oh.  I see, the script isn't in this series.  It's in your separate
git repo.  I see now.  I think a reference to the document is more
important than a #define.

> > I'm missing the code which is transfers the data from the
> > state->saved_state to the console.  Am I just being dim ?
> 
> This is done by existing code by connecting STUBDOM_CONSOLE_RESTORE to
> that file. See libxl_dm.c:spawn_stub_launch_dm.

Ah.  Right.

TBH I feel I am not giving you a very high quality of review.  I seem
to be a bit dim today.  My apologies if this is frustrating for you.
But I didn't want to let all of this sit any longer.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> libxenvchan_server_init is asynchronous.

I went to look and I think what you mean is that it is fast.  Ie, it
does not need to wait for anything.

> libxenvchan_client_init is too,
> but it fails if called before server is ready.

That is less good.

> I have a
> wrapper[1] around libxenvchan_client_init in Qubes code which solve this
> problem with xs_watch. "libvchan: create xenstore entries in one
> transaction" patch is related to that wrapper.

Ah, yes, I saw that, I think it should go in soon if it hasn't
already.

> Maybe it should be also added to libxenvchan? Right now it only waits
> (synchronously) for server to appear (using while(...) xs_read_watch()).

That's rather poor.

> This is slightly more complex, as it also handle remote domain death
> before establishing connection as well qas save+restore local domain.
> But it can be provided as a separate function like
> libxenvchan_client_wait_for_server or such.

AIUI you would want to call such a function in libxl, because your
qemu stubdom is the server ?  In which case a synchronous call is no
good, because it would allow a rogue linux stubdom to block the entire
libxl process.

> Providing a function that could be used in libxl would be more complex,
> as it needs to integrate with libxl async API.

Oh, you're ahead of me.

> Maybe it could use
> good old trick with separate thread + pipe() for signaling readiness?

libxl has code for waiting for xs watches and domain death and so on
already.

How about this: provide a new variant of libxenvchan_client_init which
can give a return indication of the form `this server does not appear
to be set up; please watch on the following xenstore key and then call
me again when the watch fires'.  That would be simple, and not involve
further event loop entanglement, and would fit nicely into libxl.

> This way, the libxenvchan_client_wait_for_server would start separate
> thread (using own xenstore connection) and return fd that libxl can wait
> on. No need to convert libxenvchan_client_wait_for_server into callback
> hell...

That may be overkill.

> > Doesn't this potentially mean that the qmp console connection can
> > become irrecoverably desynchronised ?  I don't know how you would
> > recover from the situation where another libxl process had got halfway
> > through some qmp stuff and been terminated (for whatever reason; maybe
> > the calling toolstack crashed).
> 
> That's right, it could result in irrecoverably desynchronised
> connection. So, we need out of band reset.

Sounds complicated.  I think that is what your console state stuff is
about...

> > That's not a terrible idea but I can't see it being popular with qemu
> > upstream, so you'd end up writing a kind of bidirectional
> > qmp<->xenstore proxy.  Urgh.
> 
> Well, I do that already (for pci-ins). In bash.

hahahaha.  Well, I think vchan may be easier.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 15/17] xenconsoled: add support for non-pty output

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2015 bytes --]

On Thu, Nov 01, 2018 at 05:35:04PM +0000, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 15/17] xenconsoled: add support for non-pty output"):
> > Handle 'output' xenstore entry, as qemu does. Right now support only few
> > simple options:
> >  - "pty" (unchanged)
> >  - "file:path" (overwrite file)
> >  - "pipe:path" (read-write file/pipe)
> >  - "null"
> 
> I have always thought the qemu set of console things very awkward to
> deal with.  pipe, in particular, is very awkward to use because pipes
> have poor semantics for this.

In fact libxl usage of "pipe" isn't about pipe at all. It's about
reading from normal file. This rely on a qemu fallback "pipe" handling -
first it looks for two pipes: path.in and path.out (for in- and
out-bound data). When it doesn't find them, it fallback to just "path"
opened for read and write.
And libxl use that fallback only. With normal file (see below).

> Would it be useful if I implemented a facility for xenconsoled to make
> an AF_UNIX listening socket for each console it handles ?  People who
> wanted to talk to the console would connect() to it.

This is meant to be compatible with other console backend(s) - namely
qemu-xen and qemu-xen-traditional. If xenconsoled backend for N>1
consoles would behave differently (in a way not supported by qemu one),
each place would need additional handling for that...
There are not many places though: "file:" is used to write qemu-dm (in
stubdomain) state to a file and "pipe:" is used to read it from that
file.

Note that real pipes are not used here at all. This is just awkward
naming for "read from this file".

> Multiple connections would be supported a la screen or tmux, although
> of course for your application you'd use a lock to prevent multiple
> access.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]

On Thu, Nov 01, 2018 at 05:48:37PM +0000, Ian Jackson wrote:
> libxl has code for waiting for xs watches and domain death and so on
> already.
> 
> How about this: provide a new variant of libxenvchan_client_init which
> can give a return indication of the form `this server does not appear
> to be set up; please watch on the following xenstore key and then call
> me again when the watch fires'.  That would be simple, and not involve
> further event loop entanglement, and would fit nicely into libxl.

Doing it in this order would be susceptible to a race condition -
server appearing after libxenvchan_client_init check, but before libxl
register the watch. Also, right now libxenvchan_client_init have only
one possible error code: NULL (instead of struct libxenvchan *). Adding
more elaborate error reporting would require API change.

As the xs path is provided by libxenvchan_client_init caller anyway,
libxl can register watch before calling libxenvchan_client_init and wait
on it if libxenvchan_client_init fails. In _this particular case_ other
conditions don't need to be handled specifically, as in the worst case it
will hit startup/qmp timeout. 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1640 bytes --]

On Thu, Nov 01, 2018 at 05:31:18PM +0000, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles"):
> > Based on previous few commits, this adds basic support for multiple
> > consoles in xenconsoled. A static number of them - up to 3 (+ one
> > primary).
> 
> I'm confused.  I thought we already had support for multiple PV
> consoles.  Is the problem that the backend is in qemu rather than
> xenconsoled ?  

Yes.
One of main reasons for this whole thing is to get rid of qemu from dom0
at all. Regardless if it's handling only console, only disk or other
stuff. This is a lot of code and I don't consider asking it nicely
"please don't let rogue domain let attack any other qemu component" to
be enough. 

> I'm not sure how this patch interacts with the
> qemu-provided extra PV consoles in docs/misc/console.txt.

I try here to be compatible with qemu-provided consoles. Some limitation
may come from "xenconsoled: add support for non-pty output" patch, as it
implements only subset of qemu supported outputs.

> I'm afraid (as you probably predicted) I don't think this patch is
> suitable for upstream in its current form.  I can see that
> restructuring xenconsoled to be more dynamic is some work but I may be
> able to help there.
> 
> CCing Stefano, the author of docs/misc/console.txt, at his new
> address.
> 
> Thanks,
> Ian.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 06/17] libxl: create vkb device only for guests with graphics output

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1523 bytes --]

On Thu, 1 Nov 2018, Ian Jackson wrote:
> (CCing Stefano's new address.)

Thank you, I am still responsive at my kernel.org address too.


> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 06/17] libxl: create vkb device only for guests with graphics output"):
> > The forced vkb device is meant for better performance of qemu access
> > (at least according to ebbd2561b4cefb299f0f68a88b2788504223de18 "libxl:
> > Add a vkbd frontend/backend pair for HVM guests"), which isn't used if
> > there is no configured channel to actually access that keyboard.
> 
> I think the background here is that the "usb keyboard/mouse" referred
> to in ebbd2561 is supposedly used only for vnc ?

Yes. The idea is that if the guest doesn't use the usb keyboard/mouse,
QEMU will need far less frequent wakeups in Dom0.


> I think we still support SDL though, so maybe that needs to be checked
> too.  I did `git-grep -i sdl tools/libxl' and there's a few
> occurrences of something like this
>   tools/libxl/libxl_dm.c:        if (!sdl && !vnc)
> in particular, in one of these cases it passes -nographic to qemu.
> 
> Probably the situations where we shouldn't unconditionally provide a
> vkb are those where -nographic is passed to qemu ?

Yes, if the configured machine already is meant to be used without
emulated usb keyboard mouse, then vkb is not necessary.


> Maybe we need a helper inline function for sdl || vnc.  If you decide
> to introduce one, please do that in a patch before this one.
 

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1507 bytes --]

On Thu, 1 Nov 2018, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles"):
> > Based on previous few commits, this adds basic support for multiple
> > consoles in xenconsoled. A static number of them - up to 3 (+ one
> > primary).
> 
> I'm confused.  I thought we already had support for multiple PV
> consoles.  Is the problem that the backend is in qemu rather than
> xenconsoled ?  I'm not sure how this patch interacts with the
> qemu-provided extra PV consoles in docs/misc/console.txt.

I haven't read this patch, but yes, it is as you wrote. Multiple PV
consoles are only suppored by QEMU, with the interface described in
docs/misc/console.txt. It could be nice to be able to support them with
xenconsoled, but we need to be careful they don't conflict. There is a
way to specify the desired console backend, either QEMU or xenconsoled,
so it shouldn't be a problem to have both backends being able to do
multiple consoles. But we wouldn't want to have a different interface
from the one described in docs/misc/console.txt, otherwise we'll break
existing guests (Linux for instance.)


> I'm afraid (as you probably predicted) I don't think this patch is
> suitable for upstream in its current form.  I can see that
> restructuring xenconsoled to be more dynamic is some work but I may be
> able to help there.
> 
> CCing Stefano, the author of docs/misc/console.txt, at his new
> address.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> Doing it in this order would be susceptible to a race condition -
> server appearing after libxenvchan_client_init check, but before libxl
> register the watch.

I see the race you mean but happily xenstore already has a general
mechanism for avoiding it: after setting up a watch, it always fires
once immediately.  (Obviously we could have done the equivalent thing
open-coded in the caller of my proposed new init function variant.)

> Also, right now libxenvchan_client_init have only
> one possible error code: NULL (instead of struct libxenvchan *). Adding
> more elaborate error reporting would require API change.

I think libxenvchan_client_init sets errno.  All of the functions it
calls do so, so the errno value is passed through.  So we would just
need to reserve a specific errno value for this.

> As the xs path is provided by libxenvchan_client_init caller anyway,
> libxl can register watch before calling libxenvchan_client_init and wait
> on it

Yes.  (Sorry I didn't see that parameter yesterday.  I was really
being quite dim.)

Although because of the xswatch behaviour I describe above, libxl can
simply set up the watch unconditionally, and call
libxenvchan_client_init in the xswatch event handler function.

> if libxenvchan_client_init fails.

I'm not a fan of this.  I tend to be quite picky about error
handling.  I think we should define a specific errno value for `server
not set up'.  ENOENT is what it currently returns, so if we use that
we won't break existing clients.

As belt and braces we should probably have libxenvchan_client_init
turn any ENOENT other than from the xs_read of ring-ref into EIO with
a log error message.

I worked up a patch to do that, which I will post in a moment.  It
turned into a series...

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 0/8] libvchan: Minor improvements

[Ian Jackson]

Mostly internal tidying, but also an API promise about what ENOENT
means from libxenvchan_client_init.

Ian Jackson (8):
  tools/libvchan: Initialise xs_transaction_t to XBT_NULL, not NULL
  tools/xenstore: Document that xs_close(0) is OK.
  tools/libvchan: init_xs_srv: Simplify error handling (1)
  tools/libvchan: init_xs_srv: Simplify error handling (2)
  tools/libvchan: init_xs_srv: Turn xs retry from goto into for (;;)
  tools/libvchan: Add xentoollog to direct dependencies
  tools/libvchan: libxenvchan_*_init: Promise an errno
  tools/libvchan: libxenvchan_client_init: use ENOENT for no server

 tools/libvchan/Makefile           |  6 ++--
 tools/libvchan/init.c             | 72 ++++++++++++++++++++++-----------------
 tools/libvchan/libxenvchan.h      |  8 +++--
 tools/libvchan/xenvchan.pc.in     |  2 +-
 tools/xenstore/include/xenstore.h |  2 +-
 5 files changed, 52 insertions(+), 38 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/8] tools/libvchan: Initialise xs_transaction_t to XBT_NULL, not NULL

[Ian Jackson]

This is an integer type, not a pointer.

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

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index ba5a6eb29e..180833dc2f 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -250,7 +250,7 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 	char buf[64];
 	char ref[16];
 	char* domid_str = NULL;
-	xs_transaction_t xs_trans = NULL;
+	xs_transaction_t xs_trans = XBT_NULL;
 	xs = xs_domain_open();
 	if (!xs)
 		goto fail;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/8] tools/xenstore: Document that xs_close(0) is OK.

[Ian Jackson]

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/xenstore/include/xenstore.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstore/include/xenstore.h b/tools/xenstore/include/xenstore.h
index 064b62c455..889dc23863 100644
--- a/tools/xenstore/include/xenstore.h
+++ b/tools/xenstore/include/xenstore.h
@@ -77,7 +77,7 @@ typedef uint32_t xs_transaction_t;
 struct xs_handle *xs_open(unsigned long flags);
 
 /* Close the connection to the xs daemon. */
-void xs_close(struct xs_handle *xsh);
+void xs_close(struct xs_handle *xsh /* NULL ok */);
 
 /* Connect to the xs daemon.
  * Returns a handle or NULL.
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/8] tools/libvchan: init_xs_srv: Simplify error handling (1)

[Ian Jackson]

* Use xs_close instead of the deprecated xs_daemon_close.

* Initialise xs to NULL.    That means xs_close can now be called in
  all cases.  Move it to the fail clause.

* free(domid_str) is already safe in all cases since domid_str is
  initialised to NULL.  Move it to the fail clause.

No overall functional change: xs_close is the same as xs_daemon_close;
and it and free are now sometimes called on NULL, but those are no-ops.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libvchan/init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 180833dc2f..9c61c720d1 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -245,7 +245,7 @@ fail:
 static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base, int ring_ref)
 {
 	int ret = -1;
-	struct xs_handle *xs;
+	struct xs_handle *xs = NULL;
 	struct xs_permissions perms[2];
 	char buf[64];
 	char ref[16];
@@ -292,9 +292,9 @@ retry_transaction:
 		ret = 0;
 	}
  fail_xs_open:
-	free(domid_str);
-	xs_daemon_close(xs);
  fail:
+	free(domid_str);
+	xs_close(xs);
 	return ret;
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/8] tools/libvchan: init_xs_srv: Simplify error handling (2)

[Ian Jackson]

* Abolish fail_xs_open which is now exactly the same as fail.

* Change all gotos to refer to fail instead.

No functional change.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libvchan/init.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 9c61c720d1..f099765a38 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -256,7 +256,7 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 		goto fail;
 	domid_str = xs_read(xs, 0, "domid", NULL);
 	if (!domid_str)
-		goto fail_xs_open;
+		goto fail;
 
 	// owner domain is us
 	perms[0].id = atoi(domid_str);
@@ -269,21 +269,21 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 retry_transaction:
 	xs_trans = xs_transaction_start(xs);
 	if (!xs_trans)
-		goto fail_xs_open;
+		goto fail;
 
 	snprintf(ref, sizeof ref, "%d", ring_ref);
 	snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
 	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail_xs_open;
+		goto fail;
 	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail_xs_open;
+		goto fail;
 
 	snprintf(ref, sizeof ref, "%d", ctrl->event_port);
 	snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
 	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail_xs_open;
+		goto fail;
 	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail_xs_open;
+		goto fail;
 
 	if (!xs_transaction_end(xs, xs_trans, 0)) {
 		if (errno == EAGAIN)
@@ -291,7 +291,6 @@ retry_transaction:
 	} else {
 		ret = 0;
 	}
- fail_xs_open:
  fail:
 	free(domid_str);
 	xs_close(xs);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/8] tools/libvchan: init_xs_srv: Turn xs retry from goto into for (; ; )

[Ian Jackson]

No functional change.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libvchan/init.c | 50 ++++++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index f099765a38..d987acd338 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -266,31 +266,33 @@ static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base
 	perms[1].id = domain;
 	perms[1].perms = XS_PERM_READ;
 
-retry_transaction:
-	xs_trans = xs_transaction_start(xs);
-	if (!xs_trans)
-		goto fail;
-
-	snprintf(ref, sizeof ref, "%d", ring_ref);
-	snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
-	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail;
-	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail;
-
-	snprintf(ref, sizeof ref, "%d", ctrl->event_port);
-	snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
-	if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
-		goto fail;
-	if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
-		goto fail;
-
-	if (!xs_transaction_end(xs, xs_trans, 0)) {
-		if (errno == EAGAIN)
-			goto retry_transaction;
-	} else {
-		ret = 0;
+	for (;;) {
+		xs_trans = xs_transaction_start(xs);
+		if (!xs_trans)
+			goto fail;
+
+		snprintf(ref, sizeof ref, "%d", ring_ref);
+		snprintf(buf, sizeof buf, "%s/ring-ref", xs_base);
+		if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+			goto fail;
+		if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+			goto fail;
+
+		snprintf(ref, sizeof ref, "%d", ctrl->event_port);
+		snprintf(buf, sizeof buf, "%s/event-channel", xs_base);
+		if (!xs_write(xs, xs_trans, buf, ref, strlen(ref)))
+			goto fail;
+		if (!xs_set_permissions(xs, xs_trans, buf, perms, 2))
+			goto fail;
+
+		if (xs_transaction_end(xs, xs_trans, 0))
+			break;
+		else if (errno != EAGAIN)
+			goto fail;
+		/* EAGAIN, retry */
 	}
+	ret = 0;
+
  fail:
 	free(domid_str);
 	xs_close(xs);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/8] tools/libvchan: Add xentoollog to direct dependencies

[Ian Jackson]

We are going to add a call to xtl_log.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libvchan/Makefile       | 6 +++---
 tools/libvchan/xenvchan.pc.in | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libvchan/Makefile b/tools/libvchan/Makefile
index de9b44978f..c236a0f9e6 100644
--- a/tools/libvchan/Makefile
+++ b/tools/libvchan/Makefile
@@ -10,9 +10,9 @@ NODE_OBJS = node.o
 NODE2_OBJS = node-select.o
 
 LIBVCHAN_PIC_OBJS = $(patsubst %.o,%.opic,$(LIBVCHAN_OBJS))
-LIBVCHAN_LIBS = $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) $(LDLIBS_libxenevtchn)
-$(LIBVCHAN_OBJS) $(LIBVCHAN_PIC_OBJS): CFLAGS += $(CFLAGS_libxenstore) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
-$(NODE_OBJS) $(NODE2_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
+LIBVCHAN_LIBS = $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog)
+$(LIBVCHAN_OBJS) $(LIBVCHAN_PIC_OBJS): CFLAGS += $(CFLAGS_libxenstore) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn) $(CFLAGS_libxentoollog)
+$(NODE_OBJS) $(NODE2_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn) $(CFLAGS_libxentoollog)
 
 MAJOR = 4.12
 MINOR = 0
diff --git a/tools/libvchan/xenvchan.pc.in b/tools/libvchan/xenvchan.pc.in
index 6fd13108d2..4b055c6c8f 100644
--- a/tools/libvchan/xenvchan.pc.in
+++ b/tools/libvchan/xenvchan.pc.in
@@ -7,4 +7,4 @@ Description: The Xenvchan library for Xen hypervisor
 Version: @@version@@
 Cflags: -I${includedir} @@cflagslocal@@
 Libs: @@libsflag@@${libdir} -lxenvchan
-Requires.private: xentoollog,xenstore,xenevtchn,xengnttab
+Requires.private: xentoollog,xenstore,xenevtchn,xengnttab,xentoollog
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 7/8] tools/libvchan: libxenvchan_*_init: Promise an errno

[Ian Jackson]

Thse functiosn do in fact leave errno set.  We are going to want to
use this.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libvchan/libxenvchan.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libvchan/libxenvchan.h b/tools/libvchan/libxenvchan.h
index d6010b145d..e4ccca1ff0 100644
--- a/tools/libvchan/libxenvchan.h
+++ b/tools/libvchan/libxenvchan.h
@@ -95,7 +95,7 @@ struct libxenvchan {
  * @param xs_path Base xenstore path for storing ring/event data
  * @param send_min The minimum size (in bytes) of the send ring (left)
  * @param recv_min The minimum size (in bytes) of the receive ring (right)
- * @return The structure, or NULL in case of an error
+ * @return The structure, or NULL in case of an error (setting errno)
  */
 struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
                                             int domain, const char* xs_path,
@@ -108,7 +108,7 @@ struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
  * @param logger Logger for libxc errors
  * @param domain The peer domain to connect to
  * @param xs_path Base xenstore path for storing ring/event data
- * @return The structure, or NULL in case of an error
+ * @return The structure, or NULL in case of an error (setting errno)
  */
 struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
                                             int domain, const char* xs_path);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Ian Jackson]

* Promise that we will set errno to ENOENT if the server is not
  yet set up.
* Arrange that all ENOENT returns other than from the read of ring-ref
  are turned into EIO, logging when we do so.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libvchan/init.c        | 11 ++++++++++-
 tools/libvchan/libxenvchan.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index d987acd338..e58f6bf9ac 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -45,6 +45,7 @@
 #include <xen/sys/gntalloc.h>
 #include <xen/sys/gntdev.h>
 #include <libxenvchan.h>
+#include <xentoollog.h>
 
 #ifndef PAGE_SHIFT
 #define PAGE_SHIFT 12
@@ -419,7 +420,7 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 	snprintf(buf, sizeof buf, "%s/ring-ref", xs_path);
 	ref = xs_read(xs, 0, buf, &len);
 	if (!ref)
-		goto fail;
+		goto fail_allow_enoent;
 	ring_ref = atoi(ref);
 	free(ref);
 	if (!ring_ref)
@@ -452,7 +453,15 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 	if (xs)
 		xs_daemon_close(xs);
 	return ctrl;
+
  fail:
+	if (errno == ENOENT) {
+		xtl_log(logger, XTL_ERROR, errno, "vchan",
+			"error talking to server `%s', returning EIO",
+			xs_path);
+		errno = EIO;
+	}
+ fail_allow_enoent:
 	libxenvchan_close(ctrl);
 	ctrl = NULL;
 	goto out;
diff --git a/tools/libvchan/libxenvchan.h b/tools/libvchan/libxenvchan.h
index e4ccca1ff0..8a4ec2ce4c 100644
--- a/tools/libvchan/libxenvchan.h
+++ b/tools/libvchan/libxenvchan.h
@@ -105,6 +105,10 @@ struct libxenvchan *libxenvchan_server_init(struct xentoollog_logger *logger,
  * safely, however no locking is performed, so you must prevent multiple clients
  * from connecting to a single server.
  *
+ * Failing with ENOENT means the server has not yet called
+ * libxenvchan_server_init, You may wait for a server to appear by
+ * setting a xenstore watch on xs_path.
+ *
  * @param logger Logger for libxc errors
  * @param domain The peer domain to connect to
  * @param xs_path Base xenstore path for storing ring/event data
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles [and 1 more messages]

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles"):
> On Thu, Nov 01, 2018 at 05:31:18PM +0000, Ian Jackson wrote:
> > I'm confused.  I thought we already had support for multiple PV
> > consoles.  Is the problem that the backend is in qemu rather than
> > xenconsoled ?  
> 
> One of main reasons for this whole thing is to get rid of qemu from dom0
> at all. Regardless if it's handling only console, only disk or other
> stuff. This is a lot of code and I don't consider asking it nicely
> "please don't let rogue domain let attack any other qemu component" to
> be enough. 

That makes perfect sense.

I'm sorry my responses on this console stuff were so confused
yesterday.  I think I need to go back and read this lot again.

Stefano Stabellini writes ("Re: [RFC PATCH v2 13/17] xenconsoled: add support for up to 3 secondary consoles"):
> I haven't read this patch, but yes, it is as you wrote. Multiple PV
> consoles are only suppored by QEMU, with the interface described in
> docs/misc/console.txt. It could be nice to be able to support them with
> xenconsoled, but we need to be careful they don't conflict. There is a
> way to specify the desired console backend, either QEMU or xenconsoled,
> so it shouldn't be a problem to have both backends being able to do
> multiple consoles. But we wouldn't want to have a different interface
> from the one described in docs/misc/console.txt, otherwise we'll break
> existing guests (Linux for instance.)

Right.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/8] tools/libvchan: Initialise xs_transaction_t to XBT_NULL, not NULL

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:06PM +0000, Ian Jackson wrote:
> This is an integer type, not a pointer.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Good catch.

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/8] tools/xenstore: Document that xs_close(0) is OK.

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:07PM +0000, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/8] tools/libvchan: init_xs_srv: Simplify error handling (1)

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:08PM +0000, Ian Jackson wrote:
> * Use xs_close instead of the deprecated xs_daemon_close.
> 
> * Initialise xs to NULL.    That means xs_close can now be called in
>   all cases.  Move it to the fail clause.
> 
> * free(domid_str) is already safe in all cases since domid_str is
>   initialised to NULL.  Move it to the fail clause.
> 
> No overall functional change: xs_close is the same as xs_daemon_close;
> and it and free are now sometimes called on NULL, but those are no-ops.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/libvchan/init.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
> index 180833dc2f..9c61c720d1 100644
> --- a/tools/libvchan/init.c
> +++ b/tools/libvchan/init.c
> @@ -245,7 +245,7 @@ fail:
>  static int init_xs_srv(struct libxenvchan *ctrl, int domain, const char* xs_base, int ring_ref)
>  {
>  	int ret = -1;
> -	struct xs_handle *xs;
> +	struct xs_handle *xs = NULL;
>  	struct xs_permissions perms[2];
>  	char buf[64];
>  	char ref[16];
> @@ -292,9 +292,9 @@ retry_transaction:
>  		ret = 0;
>  	}
>   fail_xs_open:

This label can be deleted now.

> -	free(domid_str);
> -	xs_daemon_close(xs);
>   fail:
> +	free(domid_str);
> +	xs_close(xs);
>  	return ret;
>  }
>  
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/8] tools/libvchan: init_xs_srv: Simplify error handling (2)

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:09PM +0000, Ian Jackson wrote:
> * Abolish fail_xs_open which is now exactly the same as fail.
> 
> * Change all gotos to refer to fail instead.
> 
> No functional change.
> 

Oh  here it is.

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

Acked-by: Wei Liu <wei.liu2@citrix.com>

Feel free to add my ack to your previous patch.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/8] tools/libvchan: init_xs_srv: Turn xs retry from goto into for (; ; )

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:10PM +0000, Ian Jackson wrote:
> No functional change.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/8] tools/libvchan: Add xentoollog to direct dependencies

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:11PM +0000, Ian Jackson wrote:
> We are going to add a call to xtl_log.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 7/8] tools/libvchan: libxenvchan_*_init: Promise an errno

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:12PM +0000, Ian Jackson wrote:
> Thse functiosn do in fact leave errno set.  We are going to want to
> use this.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Wei Liu]

On Fri, Nov 02, 2018 at 05:01:13PM +0000, Ian Jackson wrote:
> * Promise that we will set errno to ENOENT if the server is not
>   yet set up.
> * Arrange that all ENOENT returns other than from the read of ring-ref
>   are turned into EIO, logging when we do so.

This sounds reasonable to me, but I would like to hear from Marek that
this doesn't break Qubes.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/8] tools/libvchan: init_xs_srv: Simplify error handling (2)

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 4/8] tools/libvchan: init_xs_srv: Simplify error handling (2)"):
> On Fri, Nov 02, 2018 at 05:01:09PM +0000, Ian Jackson wrote:
> > * Abolish fail_xs_open which is now exactly the same as fail.
> > * Change all gotos to refer to fail instead.
> > No functional change.
> 
> Oh  here it is.

Right :-).  Separating the patches like this made the first half a lot
clearer, I found.

> Feel free to add my ack to your previous patch.

Thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server"):
> On Fri, Nov 02, 2018 at 05:01:13PM +0000, Ian Jackson wrote:
> > * Promise that we will set errno to ENOENT if the server is not
> >   yet set up.
> > * Arrange that all ENOENT returns other than from the read of ring-ref
> >   are turned into EIO, logging when we do so.
> 
> This sounds reasonable to me, but I would like to hear from Marek that
> this doesn't break Qubes.

Right.  Marek, this series was intended to help with vchan for qmp.
But obviously I don't want to break anything else.  I don't think this
should break anything but I would appreciate a review or at least an
opinion...

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1140 bytes --]

On Tue, Nov 06, 2018 at 11:45:47AM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server"):
> > On Fri, Nov 02, 2018 at 05:01:13PM +0000, Ian Jackson wrote:
> > > * Promise that we will set errno to ENOENT if the server is not
> > >   yet set up.
> > > * Arrange that all ENOENT returns other than from the read of ring-ref
> > >   are turned into EIO, logging when we do so.
> > 
> > This sounds reasonable to me, but I would like to hear from Marek that
> > this doesn't break Qubes.
> 
> Right.  Marek, this series was intended to help with vchan for qmp.
> But obviously I don't want to break anything else.  I don't think this
> should break anything but I would appreciate a review or at least an
> opinion...

Does xentoollog require any explicit configuration? If yes, that would
break non-xen-tools users (including Qubes). Otherwise looks good.
Thanks!

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server

[Ian Jackson]

Marek Marczykowski-Górecki writes ("Re: [PATCH 8/8] tools/libvchan: libxenvchan_client_init: use ENOENT for no server"):
> On Tue, Nov 06, 2018 at 11:45:47AM +0000, Ian Jackson wrote:
> > Right.  Marek, this series was intended to help with vchan for qmp.
> > But obviously I don't want to break anything else.  I don't think this
> > should break anything but I would appreciate a review or at least an
> > opinion...
> 
> Does xentoollog require any explicit configuration? If yes, that would
> break non-xen-tools users (including Qubes). Otherwise looks good.
> Thanks!

Hrm.

libvchan already takes an xtl logger as an argument, and passes that
on to lower-layer libraries.  So it's already in there.  The only
difference is that it is used directly by libchan now.

But I tried to make an argument that the new log message could cause
no problem and I think that's actually wrong.  libvchan right now
passes that logger to xengntshr_open and xenevtchn_open, but those
both tolerate NULL (and make up their own logger which writes to
stderr).

But with my change, NULL will cause a crash if new error condition
occurs.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Anthony PERARD]

On Thu, Nov 01, 2018 at 06:32:07PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Nov 01, 2018 at 04:57:18PM +0000, Ian Jackson wrote:
> > Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > > 2. pv console
> > >   pros:
> > >    - no qemu modifications
> > >    - same read()/write() on libxl side
> > >   cons:
> > >    - no out of band reset, needs libxl handling for that (skipping
> > >      negotiation)
> > 
> > Doesn't this potentially mean that the qmp console connection can
> > become irrecoverably desynchronised ?  I don't know how you would
> > recover from the situation where another libxl process had got halfway
> > through some qmp stuff and been terminated (for whatever reason; maybe
> > the calling toolstack crashed).
> 
> That's right, it could result in irrecoverably desynchronised
> connection. So, we need out of band reset.

Actually, it looks like we can recover that situation without out of
band reset. It's even in the spec[1]:

  2.7 QGA Synchronization
  -----------------------

  When a client connects to QGA over a transport lacking proper
  connection semantics such as virtio-serial, QGA may have read partial
  input from a previous client.  The client needs to force QGA's parser
  into known-good state using the previous section's technique.
  Moreover, the client may receive output a previous client didn't read.
  To help with skipping that output, QGA provides the
  'guest-sync-delimited' command.  Refer to its documentation for
  details.

previous section:
  2.6 Forcing the JSON parser into known-good state
  -------------------------------------------------

  Incomplete or invalid input can leave the server's JSON parser in a
  state where it can't parse additional commands.  To get it back into
  known-good state, the client should provoke a lexical error.

  The cleanest way to do that is sending an ASCII control character
  other than '\t' (horizontal tab), '\r' (carriage return), or '\n' (new
  line).

  Sadly, older versions of QEMU can fail to flag this as an error.  If a
  client needs to deal with them, it should send a 0xFF byte.

[1] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/qmp-spec.txt;h=8f7da0245d51447be7df2b3d4b105bad9fbec0b3;hb=HEAD#l231

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2839 bytes --]

On Thu, Nov 15, 2018 at 05:41:44PM +0000, Anthony PERARD wrote:
> On Thu, Nov 01, 2018 at 06:32:07PM +0100, Marek Marczykowski-Górecki wrote:
> > On Thu, Nov 01, 2018 at 04:57:18PM +0000, Ian Jackson wrote:
> > > Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > > > 2. pv console
> > > >   pros:
> > > >    - no qemu modifications
> > > >    - same read()/write() on libxl side
> > > >   cons:
> > > >    - no out of band reset, needs libxl handling for that (skipping
> > > >      negotiation)
> > > 
> > > Doesn't this potentially mean that the qmp console connection can
> > > become irrecoverably desynchronised ?  I don't know how you would
> > > recover from the situation where another libxl process had got halfway
> > > through some qmp stuff and been terminated (for whatever reason; maybe
> > > the calling toolstack crashed).
> > 
> > That's right, it could result in irrecoverably desynchronised
> > connection. So, we need out of band reset.
> 
> Actually, it looks like we can recover that situation without out of
> band reset. It's even in the spec[1]:

That's interesting. And it mention serial console explicitly as the use
case for this. Does it apply to monitor socket too, or guest agent only?
I'd much prefer to use console, as the code would be much simpler (the
same handling for local and stubdomain qemu).

Also, this doesn't cover capabilities (re-)negotiation. While actual
capabilities are probably not a problem, libxl do store qemu version
from the server greeting (is it used anywhere?).

(...)

> previous section:
>   2.6 Forcing the JSON parser into known-good state
>   -------------------------------------------------
> 
>   Incomplete or invalid input can leave the server's JSON parser in a
>   state where it can't parse additional commands.  To get it back into
>   known-good state, the client should provoke a lexical error.
> 
>   The cleanest way to do that is sending an ASCII control character
>   other than '\t' (horizontal tab), '\r' (carriage return), or '\n' (new
>   line).
> 
>   Sadly, older versions of QEMU can fail to flag this as an error.  If a
>   client needs to deal with them, it should send a 0xFF byte.

That's weird as guest-sync-delimiter documentation (still?) mention
0xFF. In both directions. Does it mean the new recommendation is to use
ASCII control character in one direction, but expect 0xFF in the other?

> [1] https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/qmp-spec.txt;h=8f7da0245d51447be7df2b3d4b105bad9fbec0b3;hb=HEAD#l231

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Anthony PERARD]

On Thu, Nov 15, 2018 at 07:57:08PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Nov 15, 2018 at 05:41:44PM +0000, Anthony PERARD wrote:
> > On Thu, Nov 01, 2018 at 06:32:07PM +0100, Marek Marczykowski-Górecki wrote:
> > > On Thu, Nov 01, 2018 at 04:57:18PM +0000, Ian Jackson wrote:
> > > > Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > > > > 2. pv console
> > > > >   pros:
> > > > >    - no qemu modifications
> > > > >    - same read()/write() on libxl side
> > > > >   cons:
> > > > >    - no out of band reset, needs libxl handling for that (skipping
> > > > >      negotiation)
> > > > 
> > > > Doesn't this potentially mean that the qmp console connection can
> > > > become irrecoverably desynchronised ?  I don't know how you would
> > > > recover from the situation where another libxl process had got halfway
> > > > through some qmp stuff and been terminated (for whatever reason; maybe
> > > > the calling toolstack crashed).
> > > 
> > > That's right, it could result in irrecoverably desynchronised
> > > connection. So, we need out of band reset.
> > 
> > Actually, it looks like we can recover that situation without out of
> > band reset. It's even in the spec[1]:
> 
> That's interesting. And it mention serial console explicitly as the use
> case for this. Does it apply to monitor socket too, or guest agent only?
> I'd much prefer to use console, as the code would be much simpler (the
> same handling for local and stubdomain qemu).

The 'guest-sync-delimited' command doesn't seems to be available on the
monitor socket. I should have checked that ... but that would just mean
that libxl would need to tolerate the first read to be an incompleted
json-object. Then we can use the 'id' that every response have to figure
out if it was a reply sent to a previous libxl run. We can maybe encode
the pid into the id.

> Also, this doesn't cover capabilities (re-)negotiation. While actual
> capabilities are probably not a problem, libxl do store qemu version
> from the server greeting (is it used anywhere?).

The QEMU version is still available after the capabilities negociation,
one simply need to execute 'query-version'.

And yes, the version is used. So far there is one command that changes
with newer version of QEMU, 'xen-save-devices-state'.

> > previous section:
> >   2.6 Forcing the JSON parser into known-good state
> >   -------------------------------------------------
> > 
> >   Incomplete or invalid input can leave the server's JSON parser in a
> >   state where it can't parse additional commands.  To get it back into
> >   known-good state, the client should provoke a lexical error.
> > 
> >   The cleanest way to do that is sending an ASCII control character
> >   other than '\t' (horizontal tab), '\r' (carriage return), or '\n' (new
> >   line).
> > 
> >   Sadly, older versions of QEMU can fail to flag this as an error.  If a
> >   client needs to deal with them, it should send a 0xFF byte.
> 
> That's weird as guest-sync-delimiter documentation (still?) mention
> 0xFF. In both directions. Does it mean the new recommendation is to use
> ASCII control character in one direction, but expect 0xFF in the other?

Looks like it. But there is a different between the server and the
client, the server still parse JSON, but the client will actively look
for the delimiter once the command have been sent.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 3256 bytes --]

On Fri, Nov 16, 2018 at 10:39:07AM +0000, Anthony PERARD wrote:
> On Thu, Nov 15, 2018 at 07:57:08PM +0100, Marek Marczykowski-Górecki wrote:
> > On Thu, Nov 15, 2018 at 05:41:44PM +0000, Anthony PERARD wrote:
> > > On Thu, Nov 01, 2018 at 06:32:07PM +0100, Marek Marczykowski-Górecki wrote:
> > > > On Thu, Nov 01, 2018 at 04:57:18PM +0000, Ian Jackson wrote:
> > > > > Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain."):
> > > > > > 2. pv console
> > > > > >   pros:
> > > > > >    - no qemu modifications
> > > > > >    - same read()/write() on libxl side
> > > > > >   cons:
> > > > > >    - no out of band reset, needs libxl handling for that (skipping
> > > > > >      negotiation)
> > > > > 
> > > > > Doesn't this potentially mean that the qmp console connection can
> > > > > become irrecoverably desynchronised ?  I don't know how you would
> > > > > recover from the situation where another libxl process had got halfway
> > > > > through some qmp stuff and been terminated (for whatever reason; maybe
> > > > > the calling toolstack crashed).
> > > > 
> > > > That's right, it could result in irrecoverably desynchronised
> > > > connection. So, we need out of band reset.
> > > 
> > > Actually, it looks like we can recover that situation without out of
> > > band reset. It's even in the spec[1]:
> > 
> > That's interesting. And it mention serial console explicitly as the use
> > case for this. Does it apply to monitor socket too, or guest agent only?
> > I'd much prefer to use console, as the code would be much simpler (the
> > same handling for local and stubdomain qemu).
> 
> The 'guest-sync-delimited' command doesn't seems to be available on the
> monitor socket. I should have checked that ... but that would just mean
> that libxl would need to tolerate the first read to be an incompleted
> json-object. Then we can use the 'id' that every response have to figure
> out if it was a reply sent to a previous libxl run. We can maybe encode
> the pid into the id.

It may be tricky to figure out where is the end of such incomplete json
object... Suppose you read:

{ "x": { "y": 1 } } }

If you read this from the beginning looking for json, you'll get valid
json object unless you encounter the last "}" (which you may receive in
separate read() call, if you're unlucky). I'm afraid the logic for
skipping initial (possibly incomplete) object(s) may be quite complex.
Maybe better propose upstream to include 'guest-sync-delimited' also on
monitor socket too? In that case, the command naming will be awkward,
but still, similar command would be useful in that context.

> > Also, this doesn't cover capabilities (re-)negotiation. While actual
> > capabilities are probably not a problem, libxl do store qemu version
> > from the server greeting (is it used anywhere?).
> 
> The QEMU version is still available after the capabilities negociation,
> one simply need to execute 'query-version'.

That's good.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.

[Anthony PERARD]

On Sun, Nov 18, 2018 at 06:25:53PM +0100, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 16, 2018 at 10:39:07AM +0000, Anthony PERARD wrote:
> > The 'guest-sync-delimited' command doesn't seems to be available on the
> > monitor socket. I should have checked that ... but that would just mean
> > that libxl would need to tolerate the first read to be an incompleted
> > json-object. Then we can use the 'id' that every response have to figure
> > out if it was a reply sent to a previous libxl run. We can maybe encode
> > the pid into the id.
> 
> It may be tricky to figure out where is the end of such incomplete json
> object... Suppose you read:
> 
> { "x": { "y": 1 } } }
> 
> If you read this from the beginning looking for json, you'll get valid
> json object unless you encounter the last "}" (which you may receive in
> separate read() call, if you're unlucky). I'm afraid the logic for
> skipping initial (possibly incomplete) object(s) may be quite complex.

It's not that complex, all messages sent by QEMU are terminated by CRLF,
that part of the protocol. So I think that libxl already return an error
if it get something like: '{ "z": 2 } }\r\n', because of that extra }
that should not be there.

> Maybe better propose upstream to include 'guest-sync-delimited' also on
> monitor socket too? In that case, the command naming will be awkward,
> but still, similar command would be useful in that context.

It might be usefull to have.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/8] tools/xenstore: Document that xs_close(0) is OK.

[Wei Liu]

On Tue, Nov 06, 2018 at 09:41:07AM +0000, Wei Liu wrote:
> On Fri, Nov 02, 2018 at 05:01:07PM +0000, Ian Jackson wrote:
> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

I have applied patch 1 and 2 in this series to reduce the length of your
patch queue.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 3258 bytes --]

On Thu, Nov 01, 2018 at 05:21:39PM +0000, Ian Jackson wrote:
> Marek Marczykowski-Górecki writes ("[RFC PATCH v2 11/17] xenconsoled: add support for consoles using 'state' xenstore entry"):
> > Add support for standard xenbus initialization protocol using 'state'
> > xenstore entry. It will be necessary for secondary consoles.
> > For consoles supporting it, read 'state' entry on the frontend and
> > proceed accordingly - either init console or close it. When closing,
> > make sure all the in-transit data is flushed (both from shared ring and
> > from local buffer), if possible. This is especially important for
> > MiniOS-based qemu stubdomain, which closes console just after writing
> > device model state to it.
> > For consoles without 'state' field behavior is unchanged - on any watch
> > event try to connect console, as long as domain is alive.
> 
> I'm not opposed to the introduction of this state field.  The code
> looks plausible.
> 
> But:
> 
> Firstly, you have put the protocol description in your commit
> message (and it seems rather informal).  Can you please provide
> a comprehensive protocol specification in-tree ?  You need to patch
>    docs/misc/console.txt
> I think.
> 
> I say `comprehensive' because your text is not particularly clearly
> about who is supposed to `flush' which data exactly when.  Nor what
> `flushing' means (does it ever mean discarding?)
> 
> Secondly: what about backwards compatibility ?  I think we need to at
> least think about the possibility that there are some guest frontends
> out there which may look for a `state' node and do something
> undesirable with it.  I think this possibility is remote but it should
> be mentioned in the commit message.

Note that this commit _does not_ invent any new protocol at all. It merely
add support for the protocol that is used by additional consoles. The
backend side was implemented by qemu only before, now I add support for
it to xenconsoled.

This is about (additional) consoles living in
/local/domain/$DOMID/device/console/$DEVID, not the special-kind-of-thing
living in /local/domain/$DOMID/console. Is there any protocol specification
for it already anywhere? I can't see it xen/include/public/io/ as it's
for other device types - console.h have only struct xencons_interface
declaration without any comment. I can't find it in other places either.
If there is one, it should be added to docs/misc/console.txt and/or
xen/include/public/io/console.h. Otherwise I can add some basic spec to
docs/misc/console.txt.

> What about the possibility that one or the other end of the connection
> may be replaced by a different implementation, so that the peer
> appears to gain or lose support for `state' ?

Actually, I'm doing similar thing here ;) previously xenconsoled didn't
know anything about 'state' field and it was one of the reason it
couldn't handle multiple consoles per domain.

> I'll be able to review the code more effectively when there is a
> formal protocol spec to compare it to...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel