[PATCH v3] add support for libvirt-like channels

[PATCH v3] add support for libvirt-like channels

[David Scott]

Hi,

Ian C -- I've tried to fix up everything you've highlighted. Let me
know if there's something I missed or misunderstood!

On hotplug/unplug: this patch set does not include hotplug or unplug. I've
got an early prototype of this but it's more complicated than I'd like
because it needs to manage qemu chardevs via qmp messages.

Note that hotplug and unplug are not needed by the libvirt use-case,
so I think the best thing to do is to use (static) qemu backends for
now and then fill in the gaps in xenconsoled later and switch over.

The blurb:

Several libvirt applications (e.g. oVirt, CloudStack) make use of 'channels': 
low-bandwidth private host <-> guest communication links which resemble serial 
ports. Typical uses include: 

* initial VM configuration without using the network: read the config data 
directly from the channel on boot 

* controlling a guest agent: signal shutdown reqests, exchange clipboard data, 
trigger resolution changes 

This patch set re-uses the existing PV console protocol implemented by qemu 
to provide this service. 

If you declare a channel in your .xl file as follows: 

channel = [ "type=socket,path=/tmp/mysock,name=guest-agent" ] 

then an extra PV console will be added to your guest. This console has the 
extra key in the frontend 

name = guest-agent 

which allows udev scripts in the VM to create a named device in a well-known 
location (e.g. /dev/xen-channels/guest-agent, similar to the KVM /dev/vports). 
The qemu process in the backend domain will proxy the data to/from the named 
Unix domain socket (in this case /tmp/mysock). 

Note this mechanism is intended for low-bandwidth communication only. If an 
application requires a high-bandwith connection then it should use a direct 
vchan connection (and not proxy it via a qemu). 

Changes since v2:
* trim down the 'kinds' of channels to PTY and SOCKET -- these seem the most
  useful and we can add more later
* add a channelinfo (queryable by 'channel-list') to check the state of each
  channel, and for a kind=PTY discover the slave device path
* IDL: switched to KeyedUnion for both the channel and channelinfo since
  each 'kind' will have different parameters (e.g. only SOCKET has PATH)
* write all the backend configuration parameters to xenstore -- where we were
  using qemu -chardev some crucial information was only on the command-line.
* add LIBXL_HAVE_DEVICE_CHANNEL
* docs: replace 'should' with 'will' e.g. the backend will be connected to
  a Unix domain socket
* squash all the libxl patches together into a coherent whole
* explain that there is no registry of channel names and so people should use
  unique information to create them (e.g. include domain name and interface
  version)

Changes since v1: 
* extend xl.cfg(5) 
* add docs/misc/channel.txt 
* libxl_types.idl: omit unnecessary init_val = 0 
* xl_cmdimpl.c: fixed over-long lines 
* xl_cmdimpl.c: use xrealloc (via ARRAY_EXTEND_INIT) 
* xl_cmdimpl.c: exit() on parse failure rather than ignore configuration 
* libxl_create.c: use libxl__device_console_init instead of memset 
* libxl_create.c: use libxl__strdup(NOGC rather than raw strdup 
* libxl.c: add name=<name> to console frontend 
* libxl.c: resolve the backend_domid from backend_domname 
* libxl_dm.c: channels[i].devid rather than i 
* libxl_dm.c: fix indentation 
* libxl_dm.c: use GCSPRINTF convenience macro 

Signed-off-by: David Scott <dave.scott@citrix.com>

[PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[David Scott]

A channel is a low-bandwidth private byte stream similar to a serial
link. Typical uses of channels are

  1. to provide initial configuration information to a VM on boot
     (example use: CloudStack's cloud-early-config service)
  2. to signal/query an in-guest agent
     (example use: oVirt's guest agent)

Channels are similar to virtio-serial devices, and are intended to be
used in the implementation of libvirt <channel>s when running on Xen.

Note: if an application requires a high-bandwidth link then it should use
vchan instead.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 docs/misc/channel.txt |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 docs/misc/channel.txt

diff --git a/docs/misc/channel.txt b/docs/misc/channel.txt
new file mode 100644
index 0000000..26c2289
--- /dev/null
+++ b/docs/misc/channel.txt
@@ -0,0 +1,52 @@
+Xen PV Channels
+------------------------------------------------------------------------
+                                                             David Scott
+                                                   dave.scott@citrix.com
+
+
+A channel is a low-bandwidth private byte stream similar to a serial
+link. Typical uses of channels are
+
+  1. to provide initial configuration information to a VM on boot
+     (example use: CloudStack's cloud-early-config service)
+  2. to signal/query an in-guest agent
+     (example use: oVirt's guest agent)
+
+Channels are similar to virtio-serial devices, and are intended to be
+used in the implementation of libvirt <channel>s when running on Xen.
+
+Note: if an application requires a high-bandwidth link then it should use
+vchan instead.
+
+From the frontend's point of view, a channel is a PV console with a
+name, a where the name can be used to locate the correct device. The
+name is stored in the frontend xenstore directory:
+
+  /local/domain/$DOMID/device/console/$DEVID/name
+
+The frontend can check for this key when the console is hotplugged,
+and handle the device appropriately. For example the frontend could
+spawn a guest agent when a channel with a well-known name is created,
+and still spawn regular getty processes when a normal console is created.
+
+The backend has an associated 'kind' which describes what the backend
+should do with the data. Thee are two defined values: 'pty' and 'socket'.
+
+If 'kind=pty' then the backend will connect to a PTY like a regular
+console. The pty device will be written into the 'tty' key in the
+frontend.
+
+If 'kind=socket' then the backend will create a listening Unix domain
+socket in the path given by 'path=<path>'. Connections will be
+accepted and the data proxied in both directions.
+
+In the default implementation the backend is implemented via qemu
+in "xenpv" mode (i.e. the 'console' device in xenstore will have
+'type=ioemu'). Futhermore if 'kind=socket' then the console 'output'
+in xenstore will be set to:
+ 
+  output = chardev:libxl-channel$DEVID
+
+The qemu commandline contains one "-chardev id=libxl-channel$DEVID,..."
+option per channel.
+
-- 
1.7.10.4

[PATCH v3 2/3] libxl: add support for channels

[David Scott]

A 'channel' is a low-bandwidth private communication channel that
resembles a physical serial port.

This patch adds a list of channels to the IDL for the domain config.
Each channel has a 'kind' which describes what should happen to
the data. Currently defined kinds are:

  * PTY: the I/O surfaces as a pty in the backend domain
  * SOCKET: a listening Unix domain socket accepts a connection in
    the backend domain and proxies

Channels are implemented using PV consoles backed by qemu (not
xenconsoled).

Channels may be listed but don't currently support hotplug/unplug.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/libxl.c                  |  235 ++++++++++++++++++++++++++++++++--
 tools/libxl/libxl.h                  |   17 +++
 tools/libxl/libxl_create.c           |   38 ++++--
 tools/libxl/libxl_dm.c               |   43 ++++++-
 tools/libxl/libxl_internal.h         |   10 +-
 tools/libxl/libxl_types.idl          |   37 ++++++
 tools/libxl/libxl_types_internal.idl |    4 +
 7 files changed, 359 insertions(+), 25 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 900b8d4..8a069cd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3185,11 +3185,11 @@ const char *libxl__device_nic_devname(libxl__gc *gc,
 /******************************************************************************/
 int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                               libxl__device_console *console,
-                              libxl__domain_build_state *state)
+                              libxl__domain_build_state *state,
+                              libxl__device *device)
 {
     flexarray_t *front, *ro_front;
     flexarray_t *back;
-    libxl__device device;
     int rc;
 
     if (console->devid && state) {
@@ -3201,12 +3201,12 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     ro_front = flexarray_make(gc, 16, 1);
     back = flexarray_make(gc, 16, 1);
 
-    device.backend_devid = console->devid;
-    device.backend_domid = console->backend_domid;
-    device.backend_kind = LIBXL__DEVICE_KIND_CONSOLE;
-    device.devid = console->devid;
-    device.domid = domid;
-    device.kind = LIBXL__DEVICE_KIND_CONSOLE;
+    device->backend_devid = console->devid;
+    device->backend_domid = console->backend_domid;
+    device->backend_kind = LIBXL__DEVICE_KIND_CONSOLE;
+    device->devid = console->devid;
+    device->domid = domid;
+    device->kind = LIBXL__DEVICE_KIND_CONSOLE;
 
     flexarray_append(back, "frontend-id");
     flexarray_append(back, libxl__sprintf(gc, "%d", domid));
@@ -3219,6 +3219,19 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(back, "protocol");
     flexarray_append(back, LIBXL_XENCONSOLE_PROTOCOL);
 
+    if (console->name) {
+        flexarray_append(ro_front, "name");
+        flexarray_append(ro_front, console->name);
+    }
+    if (console->kind) {
+        flexarray_append(back, "kind");
+        flexarray_append(back, console->kind);
+    }
+    if (console->path) {
+        flexarray_append(back, "path");
+        flexarray_append(back, console->path);
+    }
+
     flexarray_append(front, "backend-id");
     flexarray_append(front, libxl__sprintf(gc, "%d", console->backend_domid));
 
@@ -3245,8 +3258,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(front, "protocol");
         flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
     }
-
-    libxl__device_generic_add(gc, XBT_NULL, &device,
+    libxl__device_generic_add(gc, XBT_NULL, device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
                               libxl__xs_kvs_of_flexarray(gc, ro_front, ro_front->count));
@@ -3257,6 +3269,209 @@ out:
 
 /******************************************************************************/
 
+int libxl__init_console_from_channel(libxl__gc *gc,
+                                     libxl__device_console *console,
+                                     int dev_num,
+                                     libxl_device_channel *channel)
+{
+    int rc;
+    libxl__device_console_init(console);
+    console->devid = dev_num;
+    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+    if (!channel->name){
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                   "channel %d has no name", channel->devid);
+        return ERROR_INVAL;
+    }
+    console->name = libxl__strdup(NOGC, channel->name);
+
+    if (channel->backend_domname) {
+        rc = libxl_domain_qualifier_to_domid(CTX, channel->backend_domname,
+                                             &channel->backend_domid);
+        if (rc < 0) return rc;
+    }
+
+    console->backend_domid = channel->backend_domid;
+
+    switch (channel->kind) {
+        case LIBXL_CHANNEL_KIND_UNKNOWN:
+            LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                       "channel %d has no kind", channel->devid);
+            return ERROR_INVAL;
+        case LIBXL_CHANNEL_KIND_PTY:
+            console->kind = libxl__strdup(NOGC, "pty");
+            console->output = libxl__sprintf(NOGC, "pty");
+            break;
+        case LIBXL_CHANNEL_KIND_SOCKET:
+            console->kind = libxl__strdup(NOGC, "socket");
+            if (!channel->u.socket.path) {
+                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                           "channel %d has no path", channel->devid);
+                return ERROR_INVAL;
+            }
+            console->path = libxl__strdup(NOGC, channel->u.socket.path);
+            console->output = libxl__sprintf(NOGC, "chardev:libxl-channel%d",
+                                             channel->devid);
+            break;
+        default:
+            /* We've forgotten to add the clause */
+            LOG(ERROR, "%s: unknown channel kind %d", __func__, channel->kind);
+            return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
+static int libxl__device_channel_from_xs_be(libxl__gc *gc,
+                                            const char *be_path,
+                                            libxl_device_channel *channel)
+{
+    const char *tmp;
+    int rc;
+
+    libxl_device_channel_init(channel);
+
+    /* READ_BACKEND is from libxl__device_nic_from_xs_be above */
+    channel->name = READ_BACKEND(NOGC, "name");
+    tmp = READ_BACKEND(gc, "kind");
+    if (!strcmp(tmp, "pty")) {
+        channel->kind = LIBXL_CHANNEL_KIND_PTY;
+    } else if (!strcmp(tmp, "socket")) {
+        channel->kind = LIBXL_CHANNEL_KIND_SOCKET;
+        channel->u.socket.path = READ_BACKEND(NOGC, "path");
+    } else {
+	rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+ out:
+    return rc;
+}
+
+static int libxl__append_channel_list_of_type(libxl__gc *gc,
+                                              uint32_t domid,
+                                              const char *type,
+                                              libxl_device_channel **channels,
+                                              int *nchannels)
+{
+    char *fe_path = NULL, *be_path = NULL;
+    char **dir = NULL;
+    unsigned int n = 0, devid = 0;
+    libxl_device_channel *next = NULL;
+    int rc = 0, i;
+
+    fe_path = GCSPRINTF("%s/device/%s",
+                        libxl__xs_get_dompath(gc, domid), type);
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
+    if (!dir || !n)
+      goto out;
+
+    for (i = 0; i < n; i++) {
+        const char *p, *name;
+        libxl_device_channel *tmp;
+        p = libxl__sprintf(gc, "%s/%s", fe_path, dir[i]);
+        name = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/name", p));
+        /* 'channels' are consoles with names, so ignore all consoles
+           without names */
+        if (!name) continue;
+        be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend", p));
+        tmp = realloc(*channels,
+                      sizeof(libxl_device_channel) * (*nchannels + devid + 1));
+        if (!tmp) {
+          rc = ERROR_NOMEM;
+          goto out;
+        }
+        *channels = tmp;
+        next = *channels + *nchannels + devid;
+        rc = libxl__device_channel_from_xs_be(gc, be_path, next);
+        if (rc) goto out;
+        next->devid = devid;
+        devid++;
+    }
+    *nchannels += devid;
+    return 0;
+
+ out:
+    return rc;
+}
+
+libxl_device_channel *libxl_device_channel_list(libxl_ctx *ctx,
+                                                uint32_t domid,
+                                                int *num)
+{
+    GC_INIT(ctx);
+    libxl_device_channel *channels = NULL;
+    int rc;
+
+    *num = 0;
+
+    rc = libxl__append_channel_list_of_type(gc, domid, "console", &channels, num);
+    if (rc) goto out_err;
+
+    GC_FREE;
+    return channels;
+
+out_err:
+    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to list channels");
+    while (*num) {
+        (*num)--;
+        libxl_device_channel_dispose(&channels[*num]);
+    }
+    free(channels);
+    return NULL;
+}
+
+int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
+                                 libxl_device_channel *channel,
+                                 libxl_channelinfo *channelinfo)
+{
+    GC_INIT(ctx);
+    char *dompath, *fe_path;
+    char *val;
+
+    dompath = libxl__xs_get_dompath(gc, domid);
+    channelinfo->devid = channel->devid;
+
+    fe_path = libxl__sprintf(gc, "%s/device/console/%d", dompath,
+                             channelinfo->devid + 1);
+    channelinfo->backend = xs_read(ctx->xsh, XBT_NULL,
+                                   libxl__sprintf(gc, "%s/backend",
+                                   fe_path), NULL);
+    if (!channelinfo->backend) {
+        GC_FREE;
+        return ERROR_FAIL;
+    }
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", fe_path));
+    channelinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", fe_path));
+    channelinfo->state = val ? strtoul(val, NULL, 10) : -1;
+    channelinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
+                                    GCSPRINTF("%s/frontend",
+                                    channelinfo->backend), NULL);
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/frontend-id",
+                         channelinfo->backend));
+    channelinfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/ring-ref", fe_path));
+    channelinfo->rref = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/port", fe_path));
+    channelinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+
+    channelinfo->kind = channel->kind;
+    switch (channel->kind) {
+         case LIBXL_CHANNEL_KIND_PTY:
+             val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/tty", fe_path));
+             channelinfo->u.pty.path = strdup(val);
+             break;
+         default:
+             break;
+    }
+    GC_FREE;
+    return 0;
+}
+
+/******************************************************************************/
+
 int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb)
 {
     int rc;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 80947c3..8d54456 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -482,6 +482,15 @@
  */
 #define LIBXL_HAVE_DEVICE_PCI_SEIZE 1
 
+/*
+ * LIBXL_HAVE_DEVICE_CHANNEL
+ *
+ * If this is defined, then the libxl_device_channel struct exists
+ * and channels can be attached to a domain both at build-time and at
+ * run-time by hotplug.
+ */
+#define LIBXL_HAVE_DEVICE_CHANNEL 1
+
 /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
  * called from within libxl itself. Callers outside libxl, who
  * do not #include libxl_internal.h, are fine. */
@@ -956,6 +965,14 @@ libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num
 int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
                               libxl_device_nic *nic, libxl_nicinfo *nicinfo);
 
+/* Virtual Channels */
+libxl_device_channel *libxl_device_channel_list(libxl_ctx *ctx,
+                                                uint32_t domid,
+                                                int *num);
+int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
+                                 libxl_device_channel *channel,
+                                 libxl_channelinfo *channelinfo);
+
 /* Virtual TPMs */
 int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
                           const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..ec5a4d6 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -355,14 +355,16 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     return 0;
 }
 
-static int init_console_info(libxl__device_console *console, int dev_num)
+static int init_console_info(libxl__gc *gc,
+                             libxl__device_console *console,
+                             int dev_num)
 {
-    memset(console, 0x00, sizeof(libxl__device_console));
+    libxl__device_console_init(console);
     console->devid = dev_num;
     console->consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
-    console->output = strdup("pty");
-    if (!console->output)
-        return ERROR_NOMEM;
+    console->output = libxl__strdup(NOGC, "pty");
+    /* console->{name,kind,path} are NULL on normal consoles.
+       Only 'channels' when mapped to consoles have a string name. */
     return 0;
 }
 
@@ -1110,17 +1112,31 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         }
     }
 
+    /* For both HVM and PV the 0th console is a regular console. We
+       map channels to IOEMU consoles starting at 1 */
+    for (i = 0; i < d_config->num_channels; i++) {
+        libxl__device_console console;
+        libxl__device device;
+        ret = libxl__init_console_from_channel(gc, &console, i + 1,
+                                               &d_config->channels[i]);
+        if ( ret )
+            goto error_out;
+        libxl__device_console_add(gc, domid, &console, NULL, &device);
+        libxl__device_console_dispose(&console);
+    }
+
     switch (d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
     {
         libxl__device_console console;
+        libxl__device device;
         libxl_device_vkb vkb;
 
-        ret = init_console_info(&console, 0);
+        ret = init_console_info(gc, &console, 0);
         if ( ret )
             goto error_out;
         console.backend_domid = state->console_domid;
-        libxl__device_console_add(gc, domid, &console, state);
+        libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
         libxl_device_vkb_init(&vkb);
@@ -1138,22 +1154,24 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     {
         int need_qemu = 0;
         libxl__device_console console;
+        libxl__device device;
 
         for (i = 0; i < d_config->num_vfbs; i++) {
             libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]);
             libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]);
         }
 
-        ret = init_console_info(&console, 0);
+        ret = init_console_info(gc, &console, 0);
         if ( ret )
             goto error_out;
 
         need_qemu = libxl__need_xenpv_qemu(gc, 1, &console,
                 d_config->num_vfbs, d_config->vfbs,
-                d_config->num_disks, &d_config->disks[0]);
+                d_config->num_disks, &d_config->disks[0],
+                d_config->num_channels, &d_config->channels[0]);
 
         console.backend_domid = state->console_domid;
-        libxl__device_console_add(gc, domid, &console, state);
+        libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
         if (need_qemu) {
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 51ab2bf..4338b08 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -394,8 +394,9 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     const libxl_sdl_info *sdl = dm_sdl(guest_config);
     const char *keymap = dm_keymap(guest_config);
     flexarray_t *dm_args;
-    int i;
+    int i, kind, devid;
     uint64_t ram_size;
+    const char *path, *chardev;
 
     dm_args = flexarray_make(gc, 16, 1);
 
@@ -412,6 +413,27 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_append(dm_args, "-mon");
     flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
 
+    for (i = 0; i < guest_config->num_channels; i++) {
+        kind = guest_config->channels[i].kind;
+        devid = guest_config->channels[i].devid;
+        switch (kind) {
+            case LIBXL_CHANNEL_KIND_PTY:
+                chardev = GCSPRINTF("pty,id=libxl-channel%d", devid);
+                break;
+            case LIBXL_CHANNEL_KIND_SOCKET:
+                path = guest_config->channels[i].u.socket.path;
+                chardev = GCSPRINTF("socket,id=libxl-channel%d,path=%s,"
+                                    "server,nowait", devid, path);
+                break;
+            default:
+                /* We've forgotten to add the clause */
+                LOG(ERROR, "%s: unknown channel kind %d", __func__, kind);
+                return NULL;
+        }
+        flexarray_append(dm_args, "-chardev");
+        flexarray_append(dm_args, (void*)chardev);
+    }
+
     /*
      * Remove default devices created by qemu. Qemu will create only devices
      * defined by xen, since the devices not defined by xen are not usable.
@@ -1067,6 +1089,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     }
 
     for (i = 0; i < num_console; i++) {
+        libxl__device device;
         console[i].devid = i;
         console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
         /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
@@ -1097,7 +1120,8 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
                 break;
         }
         ret = libxl__device_console_add(gc, dm_domid, &console[i],
-                        i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL);
+                        i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
+                        &device);
         if (ret)
             goto out;
     }
@@ -1517,7 +1541,8 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_consoles, libxl__device_console *consoles,
         int nr_vfbs, libxl_device_vfb *vfbs,
-        int nr_disks, libxl_device_disk *disks)
+        int nr_disks, libxl_device_disk *disks,
+        int nr_channels, libxl_device_channel *channels)
 {
     int i, ret = 0;
     uint32_t domid;
@@ -1557,6 +1582,18 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
         }
     }
 
+    if (nr_channels > 0) {
+        ret = libxl__get_domid(gc, &domid);
+        if (ret) goto out;
+        for (i = 0; i < nr_channels; i++) {
+            if (channels[i].kind == LIBXL_CHANNEL_KIND_SOCKET &&
+                channels[i].backend_domid == domid) {
+                ret = 1;
+                goto out;
+            }
+        }
+    }
+
 out:
     return ret;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 082749e..8053663 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1018,7 +1018,8 @@ _hidden int libxl__device_disk_dev_number(const char *virtpath,
 
 _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                                       libxl__device_console *console,
-                                      libxl__domain_build_state *state);
+                                      libxl__domain_build_state *state,
+                                      libxl__device *device);
 
 _hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
         libxl__device *device, char **bents, char **fents, char **ro_fents);
@@ -1031,6 +1032,10 @@ _hidden int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
                                     const char *state);
 _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
                             libxl_nic_type *nictype);
+_hidden int libxl__init_console_from_channel(libxl__gc *gc,
+                                             libxl__device_console *console,
+                                             int dev_num,
+                                             libxl_device_channel *channel);
 
 /*
  * For each aggregate type which can be used as an input we provide:
@@ -1450,7 +1455,8 @@ _hidden const char *libxl__domain_device_model(libxl__gc *gc,
 _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_consoles, libxl__device_console *consoles,
         int nr_vfbs, libxl_device_vfb *vfbs,
-        int nr_disks, libxl_device_disk *disks);
+        int nr_disks, libxl_device_disk *disks,
+        int nr_channels, libxl_device_channel *channels);
 
 /*
  * This function will cause the whole libxl process to hang
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 52f1aa9..025bffd 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -51,6 +51,14 @@ libxl_domain_type = Enumeration("domain_type", [
     (2, "PV"),
     ], init_val = -1)
 
+libxl_channel_kind = Enumeration("channel_kind", [
+    (0, "UNKNOWN"),
+    # Connect to a pty in the backend domain:
+    (1, "PTY"),
+    # Listen on a Unix domain socket in the backend domain:
+    (2, "SOCKET"),
+    ])
+
 libxl_device_model_version = Enumeration("device_model_version", [
     (0, "UNKNOWN"),
     (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
@@ -245,6 +253,22 @@ libxl_cpupoolinfo = Struct("cpupoolinfo", [
     ("cpumap",      libxl_bitmap)
     ], dir=DIR_OUT)
 
+libxl_channelinfo = Struct("channelinfo", [
+    ("backend", string),
+    ("backend_id", uint32),
+    ("frontend", string),
+    ("frontend_id", uint32),
+    ("devid", libxl_devid),
+    ("state", integer),
+    ("evtch", integer),
+    ("rref", integer),
+    ("u", KeyedUnion(None, libxl_channel_kind, "kind",
+           [("unknown", None),
+            ("pty", Struct(None, [("path", string),])),
+            ("socket", None),
+           ])),
+    ], dir=DIR_OUT)
+
 libxl_vminfo = Struct("vminfo", [
     ("uuid", libxl_uuid),
     ("domid", libxl_domid),
@@ -457,6 +481,18 @@ libxl_device_vtpm = Struct("device_vtpm", [
     ("uuid",             libxl_uuid),
 ])
 
+libxl_device_channel = Struct("device_channel", [
+    ("backend_domid", libxl_domid),
+    ("backend_domname", string),
+    ("devid", libxl_devid),
+    ("name", string),
+    ("u", KeyedUnion(None, libxl_channel_kind, "kind",
+           [("unknown", None),
+            ("pty", None),
+            ("socket", Struct(None, [("path", string)])),
+           ])),
+])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -467,6 +503,7 @@ libxl_domain_config = Struct("domain_config", [
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
+    ("channels", Array(libxl_device_channel, "num_channels")),
 
     ("on_poweroff", libxl_action_on_shutdown),
     ("on_reboot", libxl_action_on_shutdown),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index cb9444f..7d1e22c 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -32,6 +32,10 @@ libxl__device_console = Struct("device_console", [
     ("devid", integer),
     ("consback", libxl__console_backend),
     ("output", string),
+    # These are specific to the 'channels', built on top of consoles:
+    ("name", string),
+    ("kind", string),
+    ("path", string),
     ])
 
 libxl__device_action = Enumeration("device_action", [
-- 
1.7.10.4

[PATCH v3 3/3] xl: add support for channels

[David Scott]

This adds support for channel declarations of the form:

  channel = [ "name=...,kind=...[,path=...][,backend=...]" ]

where 'name' is a label to identify the channel to the frontend.

If 'kind = pty' then the channel is connected to a pty in the
  backend domain
If 'kind = socket' then the channel is connected to a Unix domain
  socket given by 'path = ...' in the backend domain.

This patch also adds the command:

  xl channel-list <domain>

which allows the state of channels to be queried. In particular if
'kind=pty' this will show the path of the pty slave device.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 docs/man/xl.cfg.pod.5     |   51 ++++++++++++++++++++++++
 docs/man/xl.pod.1         |   10 +++++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |   97 ++++++++++++++++++++++++++++++++++++++++++++-
 tools/libxl/xl_cmdtable.c |    5 +++
 5 files changed, 163 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a94d037..5069049 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -470,6 +470,57 @@ L<qemu(1)> manpage. The default is B<en-us>.
 
 =back
 
+=item B<channel=[ "CHANNEL_SPEC_STRING", "CHANNEL_SPEC_STRING", ...]>
+
+Specifies the virtual channels to be provided to the guest. A
+channel is a low-bandwidth, bidirectional byte stream, which resembles
+a serial link. Typical uses for channels include transmitting VM
+configuration after boot and signalling to in-guest agents. Please see
+F<docs/misc/channels.txt> for more details.
+
+Each B<CHANNEL_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
+settings, from the following list:
+
+=over 4
+
+=item C<backend=DOMAIN>
+
+Specify the backend domain name or id. This parameter is optional. If
+this parameter is omitted then the toolstack domain will be assumed.
+
+=item C<name=NAME>
+
+Specify the string name for this device. This parameter is mandatory.
+This should be a well-known name for the specific application (e.g.
+guest agent) and should be used by the frontend to connect the
+application to the right channel device. There is no formal registry
+of channel names, so application authors are encouraged to make their
+names unique by including domain name and version number in the string
+(e.g. org.mydomain.guestagent.1).
+
+=item C<kind=KIND>
+
+Specify how the backend will be implemented. This following options are
+available:
+
+=over 4
+
+=item B<kind=SOCKET>:
+
+The backend will bind a Unix domain socket (at the path given by
+B<path=PATH>), call listen and accept connections. The backend will proxy
+data between the channel and the connected socket.
+
+=item B<kind=PTY>:
+
+The backend will create a pty and proxy data between the channel and the
+master device. The command B<xl channel-list> can be used to discover the
+assigned slave device.
+
+=back
+
+=back
+
 =item B<pci=[ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ... ]>
 
 Specifies the host PCI devices to passthrough to this guest. Each B<PCI_SPEC_STRING>
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..827f5a5 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1194,6 +1194,16 @@ List virtual network interfaces for a domain.
 
 =back
 
+=head2 CHANNEL DEVICES
+
+=over 4
+
+=item B<channel-list> I<domain-id>
+
+List virtual channel interfaces for a domain.
+
+=back
+
 =head2 VTPM DEVICES
 
 =over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 10a2e66..afc5f8b 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -78,6 +78,7 @@ int main_top(int argc, char **argv);
 int main_networkattach(int argc, char **argv);
 int main_networklist(int argc, char **argv);
 int main_networkdetach(int argc, char **argv);
+int main_channellist(int argc, char **argv);
 int main_blockattach(int argc, char **argv);
 int main_blocklist(int argc, char **argv);
 int main_blockdetach(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5195914..a5697ef 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -736,7 +736,7 @@ static void parse_config_data(const char *config_source,
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
-    XLU_ConfigList *ioports, *irqs, *iomem;
+    XLU_ConfigList *channels, *ioports, *irqs, *iomem;
     int num_ioports, num_irqs, num_iomem;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
@@ -1289,6 +1289,58 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
+        d_config->num_channels = 0;
+        d_config->channels = NULL;
+        while ((buf = xlu_cfg_get_listitem (channels,
+                d_config->num_channels)) != NULL) {
+            libxl_device_channel *chn;
+            char *buf2 = strdup(buf);
+            char *p, *p2;
+            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
+                                    libxl_device_channel_init);
+
+            p = strtok(buf2, ",");
+            if (!p)
+                goto skip_channel;
+            do {
+                while (*p == ' ')
+                    p++;
+                if ((p2 = strchr(p, '=')) == NULL)
+                    break;
+                *p2 = '\0';
+                if (!strcmp(p, "backend")) {
+                    free(chn->backend_domname);
+                    chn->backend_domname = strdup(p2 + 1);
+                } else if (!strcmp(p, "name")) {
+                    free(chn->name);
+                    chn->name = strdup(p2 + 1);
+                } else if (!strcmp(p, "kind")) {
+                    if (chn->kind != LIBXL_CHANNEL_KIND_UNKNOWN) {
+                        fprintf(stderr, "a channel may have only one kind\n");
+                        exit(1);
+                    }
+                    if (!strcmp(p2 + 1, "pty")) {
+                        chn->kind = LIBXL_CHANNEL_KIND_PTY;
+                    } else if (!strcmp(p2 + 1, "socket")) {
+                        chn->kind = LIBXL_CHANNEL_KIND_SOCKET;
+                    } else {
+                        fprintf(stderr, "unknown channel kind '%s'\n", p2 + 1);
+                        exit(1);
+                    }
+                } else if (!strcmp(p, "path")) {
+                    free(chn->u.socket.path);
+                    chn->u.socket.path = strdup(p2 + 1);
+                } else {
+                    fprintf(stderr, "unknown channel parameter '%s',"
+                                    " ignoring\n", p);
+                }
+            } while ((p = strtok(NULL, ",")) != NULL);
+skip_channel:
+            free(buf2);
+        }
+    }
+
     if (!xlu_cfg_get_list (config, "vif", &nics, 0, 0)) {
         d_config->num_nics = 0;
         d_config->nics = NULL;
@@ -5921,6 +5973,49 @@ int main_networkdetach(int argc, char **argv)
     return 0;
 }
 
+int main_channellist(int argc, char **argv)
+{
+    int opt;
+    libxl_device_channel *channels;
+    libxl_channelinfo channelinfo;
+    int nb, i;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "channel-list", 1) {
+        /* No options */
+    }
+
+    /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
+    printf("%-3s %-2s %-5s %-6s %8s %-10s %-30s\n",
+           "Idx", "BE", "state", "evt-ch", "ring-ref", "kind", "");
+    for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
+        uint32_t domid = find_domain(*argv);
+        channels = libxl_device_channel_list(ctx, domid, &nb);
+        if (!channels) {
+            continue;
+        }
+        for (i = 0; i < nb; ++i) {
+            if (!libxl_device_channel_getinfo(ctx, domid, &channels[i], &channelinfo)) {
+                printf("%-3d %-2d ", channels[i].devid, channelinfo.backend_id);
+                printf("%-5d ", channelinfo.state);
+                printf("%-6d %-8d ", channelinfo.evtch, channelinfo.rref);
+                printf("%-10s ", libxl_channel_kind_to_string(channels[i].kind));
+                switch (channels[i].kind) {
+                    case LIBXL_CHANNEL_KIND_PTY:
+                        printf("%-30s ", channelinfo.u.pty.path);
+                        break;
+                    default:
+                        break;
+                }
+                printf("\n");
+                libxl_channelinfo_dispose(&channelinfo);
+            }
+            libxl_device_channel_dispose(&channels[i]);
+        }
+        free(channels);
+    }
+    return 0;
+}
+
 int main_blockattach(int argc, char **argv)
 {
     int opt;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4279b9f..e8d01ae 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -335,6 +335,11 @@ struct cmd_spec cmd_table[] = {
       "Destroy a domain's virtual network device",
       "<Domain> <DevId|mac>",
     },
+    { "channel-list",
+      &main_channellist, 0, 0,
+      "List virtual channel devices for a domain",
+      "<Domain(s)>",
+    },
     { "block-attach",
       &main_blockattach, 1, 1,
       "Create a new virtual block device",
-- 
1.7.10.4

Re: [PATCH v3 3/3] xl: add support for channels

[Wei Liu]

On Mon, Jun 23, 2014 at 09:29:01AM +0100, David Scott wrote:
[...]
> +int main_channellist(int argc, char **argv)
> +{
> +    int opt;
> +    libxl_device_channel *channels;
> +    libxl_channelinfo channelinfo;
> +    int nb, i;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "channel-list", 1) {
> +        /* No options */
> +    }
> +
> +    /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */

Minor nit. This comment is not acurate.

Wei.

> +    printf("%-3s %-2s %-5s %-6s %8s %-10s %-30s\n",
> +           "Idx", "BE", "state", "evt-ch", "ring-ref", "kind", "");

Re: [PATCH v3 3/3] xl: add support for channels

[Dave Scott]

Hi Wei,

On 23 Jun 2014, at 11:02, Wei Liu <wei.liu2@citrix.com> wrote:

> On Mon, Jun 23, 2014 at 09:29:01AM +0100, David Scott wrote:
> [...]
>> +int main_channellist(int argc, char **argv)
>> +{
>> +    int opt;
>> +    libxl_device_channel *channels;
>> +    libxl_channelinfo channelinfo;
>> +    int nb, i;
>> +
>> +    SWITCH_FOREACH_OPT(opt, "", NULL, "channel-list", 1) {
>> +        /* No options */
>> +    }
>> +
>> +    /*      Idx  BE   MAC   Hdl  Sta  evch txr/rxr  BE-path */
> 
> Minor nit. This comment is not accurate.

Oops, I think I’ve revealed where I cut ’n pasted this code from :-)

Thanks!
Dave

> 
> Wei.
> 
>> +    printf("%-3s %-2s %-5s %-6s %8s %-10s %-30s\n",
>> +           "Idx", "BE", "state", "evt-ch", "ring-ref", "kind", "");

Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[Ian Jackson]

David Scott writes ("[PATCH v3 1/3] docs: add a document describing the 'channels' mechanism"):
> A channel is a low-bandwidth private byte stream similar to a serial
> link. Typical uses of channels are
...

Thanks for this.  I have some comments on this patch (I haven't read
the others yet but will do so shortly).

A minor comment: your commit message recapitulates the contents of the
document in the patch.  If you are going to have a separate docs patch
the commit message should probably say something like "introduce the
documentation" or something.

> diff --git a/docs/misc/channel.txt b/docs/misc/channel.txt
> new file mode 100644
> index 0000000..26c2289
> --- /dev/null
> +++ b/docs/misc/channel.txt
> @@ -0,0 +1,52 @@
> +Xen PV Channels

I think much of this ought to go in context-specific documents, rather
than providing the whole stack in a single file.

> +------------------------------------------------------------------------
> +                                                             David Scott
> +                                                   dave.scott@citrix.com

I don't think leaving authorship information in the document is
sensible.  It will just inhibit people from editing it.  If someone
wants to know the author, the vcs logs are available.

> +A channel is a low-bandwidth private byte stream similar to a serial
> +link. Typical uses of channels are
> +
> +  1. to provide initial configuration information to a VM on boot
> +     (example use: CloudStack's cloud-early-config service)
> +  2. to signal/query an in-guest agent
> +     (example use: oVirt's guest agent)

This is a useful overview.

> +Channels are similar to virtio-serial devices, and are intended to be
> +used in the implementation of libvirt <channel>s when running on Xen.

It might be better to say that channels are similar to emulated serial
links.

> +From the frontend's point of view, a channel is a PV console with a
> +name, a where the name can be used to locate the correct device. The
> +name is stored in the frontend xenstore directory:
> +
> +  /local/domain/$DOMID/device/console/$DEVID/name

This information about the xenstore protocol should be in
docs/misc/xenstore-paths.markdown, not here.

> +The frontend can check for this key when the console is hotplugged,
> +and handle the device appropriately. For example the frontend could
> +spawn a guest agent when a channel with a well-known name is created,
> +and still spawn regular getty processes when a normal console is created.

You should set up a registry (in the channels.txt document, not here)
of the expected values for "name" with their intended semantics.

> +In the default implementation the backend is implemented via qemu
> +in "xenpv" mode (i.e. the 'console' device in xenstore will have
> +'type=ioemu'). Futhermore if 'kind=socket' then the console 'output'
> +in xenstore will be set to:
> + 
> +  output = chardev:libxl-channel$DEVID
> +
> +The qemu commandline contains one "-chardev id=libxl-channel$DEVID,..."
> +option per channel.

AFAICT this is an implementation detail, not part of any interface.
It should therefore be in a comment somewhere.  (Or perhaps even in a
commit message.)

Thanks,
Ian.

Re: [PATCH v3 2/3] libxl: add support for channels

[Ian Jackson]

David Scott writes ("[PATCH v3 2/3] libxl: add support for channels"):
> A 'channel' is a low-bandwidth private communication channel that
> resembles a physical serial port.
> 
> This patch adds a list of channels to the IDL for the domain config.
> Each channel has a 'kind' which describes what should happen to
> the data. Currently defined kinds are:
> 
>   * PTY: the I/O surfaces as a pty in the backend domain
>   * SOCKET: a listening Unix domain socket accepts a connection in
>     the backend domain and proxies

I don't think the term "kind" here is particularly aposite.  The
"kind" here doesn't affect the guest protocol - it just controls how
the device appears in the guest.

We currently use a variety of device-specific terms for the
information which specifies the host resources to be exposed for any
particular virtual device.  Eg, "pdev" for disks; "bridge" for vifs.

The word "backend" is attractive but is used for disks to refer to the
specific provisioning strategy (choice of implementation).

Perhaps "method" or "connection".  Consulting a thesaurus yielded
"style", "disposal", "usage", "disposition", "associate" ("assoc"?),
"termination" ("term"?).

> Channels are implemented using PV consoles backed by qemu (not
> xenconsoled).

Why should an additional channel not be handled by xenconsoled and
logged ?

> Channels may be listed but don't currently support hotplug/unplug.

It might be easier to wait with the listing until after Wei's
save/restore changes.

Thanks,
Ian.

Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[Ian Jackson]

David Scott writes ("[PATCH v3 1/3] docs: add a document describing the 'channels' mechanism"):
> A channel is a low-bandwidth private byte stream similar to a serial
> link. Typical uses of channels are
...
> +From the frontend's point of view, a channel is a PV console with a
> +name, a where the name can be used to locate the correct device. The
> +name is stored in the frontend xenstore directory:
> +
> +  /local/domain/$DOMID/device/console/$DEVID/name

I forgot to ask: what will a guest do that doesn't understand these
special-purpose "console"s ?

Ian.

Re: [PATCH v3 3/3] xl: add support for channels

[Ian Jackson]

David Scott writes ("[PATCH v3 3/3] xl: add support for channels"):
> This adds support for channel declarations of the form:
>   channel = [ "name=...,kind=...[,path=...][,backend=...]" ]

> +    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
> +        d_config->num_channels = 0;
> +        d_config->channels = NULL;
> +        while ((buf = xlu_cfg_get_listitem (channels,
> +                d_config->num_channels)) != NULL) {
> +            libxl_device_channel *chn;
> +            char *buf2 = strdup(buf);
> +            char *p, *p2;
> +            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
> +                                    libxl_device_channel_init);

I appreciate that you're just following the example of the vif
configuration here, but I think this is rather too much open-coded
string handling.

> +                if (!strcmp(p, "backend")) {
> +                    free(chn->backend_domname);
> +                    chn->backend_domname = strdup(p2 + 1);

At the very least, can we provide a macro or function or something to
do this ?

Thanks,
Ian.

Re: [PATCH v3 2/3] libxl: add support for channels

[Dave Scott]

Hi Ian,

Thanks for the review!

On 23 Jun 2014, at 15:52, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> David Scott writes ("[PATCH v3 2/3] libxl: add support for channels"):
>> A 'channel' is a low-bandwidth private communication channel that
>> resembles a physical serial port.
>> 
>> This patch adds a list of channels to the IDL for the domain config.
>> Each channel has a 'kind' which describes what should happen to
>> the data. Currently defined kinds are:
>> 
>>  * PTY: the I/O surfaces as a pty in the backend domain
>>  * SOCKET: a listening Unix domain socket accepts a connection in
>>    the backend domain and proxies
> 
> I don't think the term "kind" here is particularly aposite.  The
> "kind" here doesn't affect the guest protocol - it just controls how
> the device appears in the guest.
> 
> We currently use a variety of device-specific terms for the
> information which specifies the host resources to be exposed for any
> particular virtual device.  Eg, "pdev" for disks; "bridge" for vifs.
> 
> The word "backend" is attractive but is used for disks to refer to the
> specific provisioning strategy (choice of implementation).
> 
> Perhaps "method" or "connection".  Consulting a thesaurus yielded
> "style", "disposal", "usage", "disposition", "associate" ("assoc"?),
> "termination" ("term"?).

OK, I’ll mull over some of the options.

> 
>> Channels are implemented using PV consoles backed by qemu (not
>> xenconsoled).
> 
> Why should an additional channel not be handled by xenconsoled and
> logged ?

I believe the current situation (unchanged by these patches) is that xenconsoled is only capable of handling console 0, but that qemu is capable of handling all consoles. Console 0 has always been a bit special: pre-configured by the domain builder and ending up in a special place in xenstore. Consoles 1+ are treated more like regular devices (with state = … keys etc), and end up in /local/domain/%d/device/console/%d.

My plan was to build these ‘channel’ things on top of the existing additional console support, which happens to rely on qemu. It would be nice to teach xenconsoled about these new channels/consoles but I thought I’d try a simple implementation first. This should all be hidden behind the interface so we can change the implementation later.

> 
>> Channels may be listed but don't currently support hotplug/unplug.
> 
> It might be easier to wait with the listing until after Wei's
> save/restore changes.

Thanks for the heads-up — I’ll check out Wei’s changes.

Cheers,
Dave

Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[Dave Scott]

On 23 Jun 2014, at 15:53, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> David Scott writes ("[PATCH v3 1/3] docs: add a document describing the 'channels' mechanism"):
>> A channel is a low-bandwidth private byte stream similar to a serial
>> link. Typical uses of channels are
> ...
>> +From the frontend's point of view, a channel is a PV console with a
>> +name, a where the name can be used to locate the correct device. The
>> +name is stored in the frontend xenstore directory:
>> +
>> +  /local/domain/$DOMID/device/console/$DEVID/name
> 
> I forgot to ask: what will a guest do that doesn't understand these
> special-purpose "console"s ?

In a Linux VM they’ll just appear as /dev/hvcX devices. I assume something similar in a *BSD.

Cheers,
Dave

Re: [PATCH v3 2/3] libxl: add support for channels

[Ian Jackson]

Dave Scott writes ("Re: [PATCH v3 2/3] libxl: add support for channels"):
> On 23 Jun 2014, at 15:52, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Why should an additional channel not be handled by xenconsoled and
> > logged ?
> 
> I believe the current situation (unchanged by these patches) is that
> xenconsoled is only capable of handling console 0, [...]

Yes.  I guess what I'm saying is that this is IMO a deficiency.
Obviously there's no need for you to rectify this right now but we
should leave space to extend the API in case we decide to fix it
later.

> My plan was to build these ‘channel’ things on top of the existing
> additional console support, which happens to rely on qemu. It would
> be nice to teach xenconsoled about these new channels/consoles but I
> thought I’d try a simple implementation first. This should all be
> hidden behind the interface so we can change the implementation
> later.

Right.  One of the reasons I'm asking these questions is that because
your document described all the layers at once, I wasn't sure which
parts of the text were parts of the libxl-user-visible specification
and which were implementation details.

Thanks,
Ian.

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

Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[Ian Jackson]

Dave Scott writes ("Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism"):
> On 23 Jun 2014, at 15:53, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > I forgot to ask: what will a guest do that doesn't understand these
> > special-purpose "console"s ?
> 
> In a Linux VM they’ll just appear as /dev/hvcX devices. I assume something similar in a *BSD.

... and then what would happen with them ?  Nothing ?  If so, fine.

I'm just worried that something might automatically mess with them.
E.g. I have found a horrid thing called "modem-manager" on some
desktop Linuces which opens (all?) serial devices and tries blathering
AT commands at them...

Ian.

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

Re: [PATCH v3 2/3] libxl: add support for channels

[Dave Scott]

On 24 Jun 2014, at 11:41, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> Dave Scott writes ("Re: [PATCH v3 2/3] libxl: add support for channels"):
>> On 23 Jun 2014, at 15:52, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>>> Why should an additional channel not be handled by xenconsoled and
>>> logged ?
>> 
>> I believe the current situation (unchanged by these patches) is that
>> xenconsoled is only capable of handling console 0, [...]
> 
> Yes.  I guess what I'm saying is that this is IMO a deficiency.
> Obviously there's no need for you to rectify this right now but we
> should leave space to extend the API in case we decide to fix it
> later.

I agree — I’d like to see a more capable xenconsoled in future.

> 
>> My plan was to build these ‘channel’ things on top of the existing
>> additional console support, which happens to rely on qemu. It would
>> be nice to teach xenconsoled about these new channels/consoles but I
>> thought I’d try a simple implementation first. This should all be
>> hidden behind the interface so we can change the implementation
>> later.
> 
> Right.  One of the reasons I'm asking these questions is that because
> your document described all the layers at once, I wasn't sure which
> parts of the text were parts of the libxl-user-visible specification
> and which were implementation details.

OK, I think I’ve got some work to do on the documentation. I’ll try to take a step back clearly separate the interface-level stuff from the miscellaneous implementation-level details.

Thanks again,
Dave

Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[Dave Scott]

On 24 Jun 2014, at 11:43, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:

> Dave Scott writes ("Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism"):
>> On 23 Jun 2014, at 15:53, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>>> I forgot to ask: what will a guest do that doesn't understand these
>>> special-purpose "console"s ?
>> 
>> In a Linux VM they’ll just appear as /dev/hvcX devices. I assume something similar in a *BSD.
> 
> ... and then what would happen with them ?  Nothing ?  If so, fine.
> 
> I'm just worried that something might automatically mess with them.
> E.g. I have found a horrid thing called "modem-manager" on some
> desktop Linuces which opens (all?) serial devices and tries blathering
> AT commands at them…

The uses for these channels all involve talking to some specific software inside the guest, for example an “early configuration” service or a guest agent. It only makes sense to connect a channel to a guest if your guest has been configured to expect it. You would install your VM as normal, install your special software, shutdown, add the channels, possibly clone/publish your guest as a template and then restart.

You’re right that an unconfigured or misconfigured guest could do anything in theory. Your AT command scenario sounds quite realistic and would make a good example in the docs of why in-guest configuration is necessary.

I think I should elaborate more on the use-cases in the docs to make all this clearer.

Cheers,
Dave

Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism

[Ian Campbell]

On Tue, 2014-06-24 at 12:15 +0100, Dave Scott wrote:
> On 24 Jun 2014, at 11:43, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> 
> > Dave Scott writes ("Re: [PATCH v3 1/3] docs: add a document describing the 'channels' mechanism"):
> >> On 23 Jun 2014, at 15:53, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> >>> I forgot to ask: what will a guest do that doesn't understand these
> >>> special-purpose "console"s ?
> >> 
> >> In a Linux VM they’ll just appear as /dev/hvcX devices. I assume something similar in a *BSD.
> > 
> > ... and then what would happen with them ?  Nothing ?  If so, fine.
> > 
> > I'm just worried that something might automatically mess with them.
> > E.g. I have found a horrid thing called "modem-manager" on some
> > desktop Linuces which opens (all?) serial devices and tries blathering
> > AT commands at them…
> 
> The uses for these channels all involve talking to some specific
> software inside the guest, for example an “early configuration”
> service or a guest agent. It only makes sense to connect a channel to
> a guest if your guest has been configured to expect it. You would
> install your VM as normal, install your special software, shutdown,
> add the channels, possibly clone/publish your guest as a template and
> then restart.
> 
> You’re right that an unconfigured or misconfigured guest could do
> anything in theory. Your AT command scenario sounds quite realistic
> and would make a good example in the docs of why in-guest
> configuration is necessary.
> 
> I think I should elaborate more on the use-cases in the docs to make all this clearer.

it also implies that the backend should be tolerant of frontends talking
nonsense at them, but of course that's also true for security reasons...

Ian.


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

Re: [PATCH v3 3/3] xl: add support for channels

[Ian Campbell]

On Mon, 2014-06-23 at 15:57 +0100, Ian Jackson wrote:
> David Scott writes ("[PATCH v3 3/3] xl: add support for channels"):
> > This adds support for channel declarations of the form:
> >   channel = [ "name=...,kind=...[,path=...][,backend=...]" ]
> 
> > +    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
> > +        d_config->num_channels = 0;
> > +        d_config->channels = NULL;
> > +        while ((buf = xlu_cfg_get_listitem (channels,
> > +                d_config->num_channels)) != NULL) {
> > +            libxl_device_channel *chn;
> > +            char *buf2 = strdup(buf);
> > +            char *p, *p2;
> > +            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
> > +                                    libxl_device_channel_init);
> 
> I appreciate that you're just following the example of the vif
> configuration here, but I think this is rather too much open-coded
> string handling.

FWIW I think there are a few places in xl where a precanned parser for
strings of comma-separated key=value syntax would be quite valuable (the
existing one is very disk specific due to the quirks for that syntax).

I don't expect Dave to write one though...

> 
> > +                if (!strcmp(p, "backend")) {
> > +                    free(chn->backend_domname);
> > +                    chn->backend_domname = strdup(p2 + 1);
> 
> At the very least, can we provide a macro or function or something to
> do this ?

I think the existing replace_string already does this.

Ian

Re: [PATCH v3 3/3] xl: add support for channels

[Dave Scott]

On 2 Jul 2014, at 16:33, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Mon, 2014-06-23 at 15:57 +0100, Ian Jackson wrote:
>> David Scott writes ("[PATCH v3 3/3] xl: add support for channels"):
>>> This adds support for channel declarations of the form:
>>>  channel = [ "name=...,kind=...[,path=...][,backend=...]" ]
>> 
>>> +    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
>>> +        d_config->num_channels = 0;
>>> +        d_config->channels = NULL;
>>> +        while ((buf = xlu_cfg_get_listitem (channels,
>>> +                d_config->num_channels)) != NULL) {
>>> +            libxl_device_channel *chn;
>>> +            char *buf2 = strdup(buf);
>>> +            char *p, *p2;
>>> +            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
>>> +                                    libxl_device_channel_init);
>> 
>> I appreciate that you're just following the example of the vif
>> configuration here, but I think this is rather too much open-coded
>> string handling.
> 
> FWIW I think there are a few places in xl where a precanned parser for
> strings of comma-separated key=value syntax would be quite valuable (the
> existing one is very disk specific due to the quirks for that syntax).
> 
> I don't expect Dave to write one though…

I’ve had a go at decomposing this into ‘split_string_into_string_list’, ‘trim’ and ‘split_string_into_pair’ so at least the fiddly pointer-level stuff is in shareable functions. I’ll send around the updated patch set later today.

> 
>> 
>>> +                if (!strcmp(p, "backend")) {
>>> +                    free(chn->backend_domname);
>>> +                    chn->backend_domname = strdup(p2 + 1);
>> 
>> At the very least, can we provide a macro or function or something to
>> do this ?
> 
> I think the existing replace_string already does this.

Ah yes so it does. I’ll replace my new replace function with ‘replace_string' before I send the patches :-)

Cheers,
Dave

[PATCH v4] add support for libvirt-like channels

[David Scott]

Hi,

Sorry for the delay in resubmitting this. I think I've addressed all the
comments from the last round -- let me know if I've missed something!

Note: I've included a fix for a recently-introduced bug in the IDL generator.
This only affects me because my KeyedUnion descriminator is called
'connection' rather than 'type'.

Note: I've seen a problem with some Linux console frontends which attempt
to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
directory. [ this key is already present, it's not added by these patches ]
Since 'type' refers to the toolstack's choice of implementation service
(which is none of the guest's business) I think the read/only permissions
are right. The location of the key is not ideal (it should be in the
backend directory IMHO) but this is the case with several other keys located
in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
bug which should be fixed there. These patches work with Mirage VMs and
with Linux if I workaround the permissions by giving the guest read/write
to the 'type' key. 

The blurb:

Several libvirt applications (e.g. oVirt, CloudStack) make use of 'channels': 
low-bandwidth private host <-> guest communication links which resemble serial 
ports. Typical uses include: 

* initial VM configuration without using the network: read the config data 
directly from the channel on boot 

* controlling a guest agent: signal shutdown reqests, exchange clipboard data, 
trigger resolution changes 

This patch set re-uses the existing PV console protocol implemented by qemu 
to provide this service. 

If you declare a channel in your .xl file as follows: 

channel = [ "connection=socket,path=/tmp/mysock,name=org.mydomain.guest-agent" ] 

then an extra PV console will be added to your guest. This console has the 
extra key in the frontend 

name = org.mydomain.guest-agent 

which allows udev scripts in the VM to create a named device in a well-known 
location (e.g. /dev/xen-channels/guest-agent, similar to the KVM /dev/vports). 
The qemu process in the backend domain will proxy the data to/from the named 
Unix domain socket (in this case /tmp/mysock). 

Note this mechanism is intended for low-bandwidth communication only. If an 
application requires a high-bandwith connection then it should use a direct 
vchan connection (and not proxy it via a qemu). 

Changes since v3:
* docs: coalesce the docs patch into the libxl patch
* docs: in channels.txt give high-level usage information, pitfalls and
        channel registry
* docs: move the xenstore paths into the existing console.txt, which is
        referenced from xenstore-paths.markdown
* idl: rename 'kind' to 'connection'
* xl: add parser functions for comma-separated lists of pairs

Changes since v2:
* trim down the 'kinds' of channels to PTY and SOCKET -- these seem the most
 useful and we can add more later
* add a channelinfo (queryable by 'channel-list') to check the state of each
 channel, and for a kind=PTY discover the slave device path
* IDL: switched to KeyedUnion for both the channel and channelinfo since
 each 'kind' will have different parameters (e.g. only SOCKET has PATH)
* write all the backend configuration parameters to xenstore -- where we were
 using qemu -chardev some crucial information was only on the command-line.
* add LIBXL_HAVE_DEVICE_CHANNEL
* docs: replace 'should' with 'will' e.g. the backend will be connected to
 a Unix domain socket
* squash all the libxl patches together into a coherent whole
* explain that there is no registry of channel names and so people should use
 unique information to create them (e.g. include domain name and interface
 version)

Changes since v1: 
* extend xl.cfg(5) 
* add docs/misc/channel.txt 
* libxl_types.idl: omit unnecessary init_val = 0 
* xl_cmdimpl.c: fixed over-long lines 
* xl_cmdimpl.c: use xrealloc (via ARRAY_EXTEND_INIT) 
* xl_cmdimpl.c: exit() on parse failure rather than ignore configuration 
* libxl_create.c: use libxl__device_console_init instead of memset 
* libxl_create.c: use libxl__strdup(NOGC rather than raw strdup 
* libxl.c: add name=<name> to console frontend 
* libxl.c: resolve the backend_domid from backend_domname 
* libxl_dm.c: channels[i].devid rather than i 
* libxl_dm.c: fix indentation 
* libxl_dm.c: use GCSPRINTF convenience macro 

Signed-off-by: David Scott <dave.scott@citrix.com>

[PATCH v4 1/3] libxl IDL: the name of a KeyedUnion descriminator need not be 'type'

[David Scott]

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/libxl/gentypes.py |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
index 4b0c996..3e73821 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -440,7 +440,7 @@ def libxl_C_type_parse_json(ty, w, v, indent = "    ", parent = None, discrimina
                          (f.type.keyvar.name + "." + x.name, w)
                     s += "    if (x) {\n"
                     (nparent, fexpr) = ty.member(v, f.type.keyvar, parent is None)
-                    s += "        %s_init_type(%s, %s);\n" % (ty.typename, v, x.enumname)
+                    s += "        %s_init_%s(%s, %s);\n" % (ty.typename, f.type.keyvar.name, v, x.enumname)
                     (nparent,fexpr) = ty.member(v, f, parent is None)
                     s += libxl_C_type_parse_json(f.type, "x", fexpr, "  ", nparent, x.enumname)
                     s += "    }\n"
-- 
1.7.10.4

[PATCH v4 2/3] libxl: add support for 'channels'

[David Scott]

A 'channel':
  - is a low-bandwidth private communication channel that resembles
    a physical serial port.
  - is implemented as a PV console with a well-known string name
    which is used to hook the channel to the appropriate software
    in the guest (i.e. some kind of guest agent).
  - has a backend 'connection' which describes what should happen
    to the data.

The following 'connection' types are defined:

 * PTY: the I/O surfaces as a pty in the backend domain
 * SOCKET: a listening Unix domain socket accepts a connection in
   the backend domain and proxies

Channels may be listed but don't currently support hotplug/unplug.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 docs/misc/channel.txt                |   95 +++++++++++++
 docs/misc/console.txt                |   53 +++++---
 tools/libxl/libxl.c                  |  249 ++++++++++++++++++++++++++++++++--
 tools/libxl/libxl.h                  |   17 +++
 tools/libxl/libxl_create.c           |   38 ++++--
 tools/libxl/libxl_dm.c               |   46 ++++++-
 tools/libxl/libxl_internal.h         |   10 +-
 tools/libxl/libxl_types.idl          |   35 +++++
 tools/libxl/libxl_types_internal.idl |    4 +
 9 files changed, 505 insertions(+), 42 deletions(-)
 create mode 100644 docs/misc/channel.txt

diff --git a/docs/misc/channel.txt b/docs/misc/channel.txt
new file mode 100644
index 0000000..1b8e405
--- /dev/null
+++ b/docs/misc/channel.txt
@@ -0,0 +1,95 @@
+Xen PV Channels
+===============
+
+A channel is a low-bandwidth private byte stream similar to a serial
+link. Typical uses of channels are
+
+  1. to provide initial configuration information to a VM on boot
+     (example use: CloudStack's cloud-early-config service)
+  2. to signal/query an in-guest agent
+     (example use: oVirt's guest agent)
+
+Channels are similar to virtio-serial devices and emulated serial links.
+Channels are intended to be used in the implementation of libvirt <channel>s
+when running on Xen.
+
+Note: if an application requires a high-bandwidth link then it should use
+vchan instead.
+
+How to use channels: an example
+-------------------------------
+
+Consider a cloud deployment where VMs are cloned from pre-made templates,
+and customised on first boot by an in-guest agent which sets the IP address,
+hostname, ssh keys etc. To install the system the cloud administrator would
+first:
+
+  1. Install a guest as normal (no channel configuration necessary)
+  2. Install the in-guest agent specific to the cloud software. This will
+     prepare the guest to communicate over the channel, and also prepare
+     the guest to be cloned safely (sometimes known as "sysprepping")
+  3. Shutdown the guest
+  4. Register the guest as a template with the cloud orchestration software
+  5. Install the cloud orchestration agent in dom0
+
+At runtime, when a cloud tenant requests that a VM is created from the template,
+the sequence of events would be:
+
+  1. A VM is "cloned" from the template
+  2. A unique Unix domain socket path in dom0 is allocated
+     (e.g. /my/cloud/software/talk/to/domain/<vm uuid>)
+  3. Domain configuration is created for the VM, listing the channel
+     name expected by the in-guest agent. In xl syntax this would be:
+
+     channel = [ "connection=socket, name=org.my.cloud.software.agent.version1,
+                  path = /my/cloud/software/talk/to/domain/<vm uuid>" ]
+
+  4. The VM is started
+  5. In dom0 the cloud orchestration agent connects to the Unix domain
+     socket, writes a handshake message and waits for a reply
+  6. In the guest, a udev rule (part of the guest agent package) is activated
+     by the hotplug event, and it starts the in-guest agent.
+  7. The in-guest agent completes a handshake with the dom0 agent
+  8. The dom0 agent transmits the unique VM configuration: hostname, IP
+     address, ssh keys etc etc
+  9. The in-guest agent receives the configuration and applies it.
+
+Using channels avoids having to use a temporary disk device or network
+connection.
+
+Design recommendations and pitfalls
+-----------------------------------
+
+It's necessary to install channel-specific software (an "agent") into the guest
+before you can use a channel. By default a channel will appear as a device
+which could be mistaken for a serial port or regular console. It is known
+that some software will proactively seek out serial ports and issue AT commands
+at them; make sure such software is disabled!
+
+Since channels are identified by names, application authors must ensure their
+channel names are unique to avoid clashes. We recommend that channel names
+include parts unique to the application such as a domain names. To assist
+prevent clashes we recommend authors add their names to our global channel
+registry at the end of this document.
+
+Limitations
+-----------
+
+Hotplug and unplug of channels is not currently implemented.
+
+Channel name registry
+---------------------
+
+It is important that channel names are globally unique. To help ensure
+that no-one's name clashes with yours, please add yours to this list.
+
+Key:
+N: Name
+C: Contact
+D: Short description of use, possibly including a URL to your software
+   or API
+
+N: org.xenproject.guest.clipboard.0.1
+C: David Scott <dave.scott@citrix.com>
+D: Share clipboard data via an in-guest agent. See:
+   http://wiki.xenproject.org/wiki/Clipboard_sharing_protocol
diff --git a/docs/misc/console.txt b/docs/misc/console.txt
index 8a53a95..eb08aff 100644
--- a/docs/misc/console.txt
+++ b/docs/misc/console.txt
@@ -9,10 +9,11 @@ relevant information in xenstore under /local/domain/$DOMID/console.
 
 Now many years after the introduction of the pv console we have
 multiple pv consoles support for pv and hvm guests; multiple pv
-console backends (qemu and xenconsoled) and emulated serial cards too.
+console backends (qemu and xenconsoled, see limitations below) and
+emulated serial cards too.
 
 This document tries to describe how the whole system works and how the
-different components interact with each others.
+different components interact with each other.
 
 The first PV console path in xenstore remains:
 
@@ -23,28 +24,47 @@ live in:
 
 /local/domain/$DOMID/device/console/$DEVID.
 
-The output of a PV console, whether it should be a file, a pty, a
-socket, or something else, is specified by the toolstack in the xenstore
-node "output", under the relevant console section.
-For example:
+PV consoles have
+* (optional) string names;
+* 'connection' information describing to what they should be
+  connected; and
+* a 'type' indicating which daemon should process the data.
+
+We call a PV console with a name a "channel", in reference to the libvirt
+concept with the same name and purpose. The file "channels.txt" describes
+how to use channels and includes a registry of well-known channel names.
+
+The toolstack writes 'connection' information in the xenstore backend in
+nodes: "connection", "path" and "output". The nodes "connection" and "path"
+express the high-level intention, while the "output" node is used for
+internal signalling between the toolstack and the implementation daemon.
+
+If the toolstack wants the console to be connected to a pty, it will write
+to the backend:
+
+connection = pty
+output = pty
 
-# xenstore-read /local/domain/26/device/console/1/output
-pty
+The backend will write the pty device name to the "tty" node in the
+console frontend.
 
-The backend chosen for a particular console is specified by the
-toolstack in the "type" node on xenstore, under the relevant console
-section.
+If the toolstack wants a listening Unix domain socket to be created at path
+<path>, a connection accepted and data proxied to the console, it will write:
+
+connection = socket
+path = <path>
+output = chardev:<some internal identifier>
+
+The backend implementation daemon chosen for a particular console is specified
+by the toolstack in the "type" node in the xenstore frontend.
 For example:
 
 # xenstore-read /local/domain/26/console/1/type
-xenconsoled
+ioemu
 
 The supported values are only xenconsoled or ioemu; xenconsoled has
 several limitations: it can only be used for the first PV console and it
-can only have a pty as output.
-
-If the output is a pty, backends write the device name to the "tty" node
-in xenstore under the relevant console path.
+can only connect to a pty.
 
 Emulated serials are provided by qemu-dm only to hvm guests; the number
 of emulated serials depends on how many "-serial" command line options
@@ -54,7 +74,6 @@ xenstore in the following path:
 
 /local/domain/$DOMID/serial/$SERIAL_NUM/tty
 
-
 xenconsole is the tool to connect to a PV console or an emulated serial
 that has a pty as output. Xenconsole takes a domid as parameter plus an
 optional console type (pv for PV consoles or serial for emulated
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3526539..769e6cf 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3265,11 +3265,11 @@ const char *libxl__device_nic_devname(libxl__gc *gc,
 /******************************************************************************/
 int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                               libxl__device_console *console,
-                              libxl__domain_build_state *state)
+                              libxl__domain_build_state *state,
+                              libxl__device *device)
 {
     flexarray_t *front, *ro_front;
     flexarray_t *back;
-    libxl__device device;
     int rc;
 
     if (console->devid && state) {
@@ -3281,12 +3281,12 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     ro_front = flexarray_make(gc, 16, 1);
     back = flexarray_make(gc, 16, 1);
 
-    device.backend_devid = console->devid;
-    device.backend_domid = console->backend_domid;
-    device.backend_kind = LIBXL__DEVICE_KIND_CONSOLE;
-    device.devid = console->devid;
-    device.domid = domid;
-    device.kind = LIBXL__DEVICE_KIND_CONSOLE;
+    device->backend_devid = console->devid;
+    device->backend_domid = console->backend_domid;
+    device->backend_kind = LIBXL__DEVICE_KIND_CONSOLE;
+    device->devid = console->devid;
+    device->domid = domid;
+    device->kind = LIBXL__DEVICE_KIND_CONSOLE;
 
     flexarray_append(back, "frontend-id");
     flexarray_append(back, libxl__sprintf(gc, "%d", domid));
@@ -3299,6 +3299,19 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(back, "protocol");
     flexarray_append(back, LIBXL_XENCONSOLE_PROTOCOL);
 
+    if (console->name) {
+        flexarray_append(ro_front, "name");
+        flexarray_append(ro_front, console->name);
+    }
+    if (console->connection) {
+        flexarray_append(back, "connection");
+        flexarray_append(back, console->connection);
+    }
+    if (console->path) {
+        flexarray_append(back, "path");
+        flexarray_append(back, console->path);
+    }
+
     flexarray_append(front, "backend-id");
     flexarray_append(front, libxl__sprintf(gc, "%d", console->backend_domid));
 
@@ -3325,8 +3338,7 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(front, "protocol");
         flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
     }
-
-    libxl__device_generic_add(gc, XBT_NULL, &device,
+    libxl__device_generic_add(gc, XBT_NULL, device,
                               libxl__xs_kvs_of_flexarray(gc, back, back->count),
                               libxl__xs_kvs_of_flexarray(gc, front, front->count),
                               libxl__xs_kvs_of_flexarray(gc, ro_front, ro_front->count));
@@ -3337,6 +3349,219 @@ out:
 
 /******************************************************************************/
 
+int libxl__init_console_from_channel(libxl__gc *gc,
+                                     libxl__device_console *console,
+                                     int dev_num,
+                                     libxl_device_channel *channel)
+{
+    int rc;
+    libxl__device_console_init(console);
+    console->devid = dev_num;
+    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
+    if (!channel->name){
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                   "channel %d has no name", channel->devid);
+        return ERROR_INVAL;
+    }
+    console->name = libxl__strdup(NOGC, channel->name);
+
+    if (channel->backend_domname) {
+        rc = libxl_domain_qualifier_to_domid(CTX, channel->backend_domname,
+                                             &channel->backend_domid);
+        if (rc < 0) return rc;
+    }
+
+    console->backend_domid = channel->backend_domid;
+
+    /* The xenstore 'output' node tells the backend what to connect the console
+       to. If the channel has "connection = pty" then the "output" node will be
+       set to "pty". If the channel has "connection = socket" then the "output"
+       node will be set to "chardev:libxl-channel%d". This tells the qemu
+       backend to proxy data between the console ring and the character device
+       with id "libxl-channel%d". These character devices are currently defined
+       on the qemu command-line via "-chardev" options in libxl_dm.c */
+
+    switch (channel->connection) {
+        case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
+            LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                       "channel %d has no defined connection; "
+                       "to where should it be connected?", channel->devid);
+            return ERROR_INVAL;
+        case LIBXL_CHANNEL_CONNECTION_PTY:
+            console->connection = libxl__strdup(NOGC, "pty");
+            console->output = libxl__sprintf(NOGC, "pty");
+            break;
+        case LIBXL_CHANNEL_CONNECTION_SOCKET:
+            console->connection = libxl__strdup(NOGC, "socket");
+            if (!channel->u.socket.path) {
+                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+                           "channel %d has no path", channel->devid);
+                return ERROR_INVAL;
+            }
+            console->path = libxl__strdup(NOGC, channel->u.socket.path);
+            console->output = libxl__sprintf(NOGC, "chardev:libxl-channel%d",
+                                             channel->devid);
+            break;
+        default:
+            /* We've forgotten to add the clause */
+            LOG(ERROR, "%s: missing implementation for channel connection %d",
+                __func__, channel->connection);
+            return ERROR_INVAL;
+    }
+
+    return 0;
+}
+
+static int libxl__device_channel_from_xs_be(libxl__gc *gc,
+                                            const char *be_path,
+                                            libxl_device_channel *channel)
+{
+    const char *tmp;
+    int rc;
+
+    libxl_device_channel_init(channel);
+
+    /* READ_BACKEND is from libxl__device_nic_from_xs_be above */
+    channel->name = READ_BACKEND(NOGC, "name");
+    tmp = READ_BACKEND(gc, "connection");
+    if (!strcmp(tmp, "pty")) {
+        channel->connection = LIBXL_CHANNEL_CONNECTION_PTY;
+    } else if (!strcmp(tmp, "socket")) {
+        channel->connection = LIBXL_CHANNEL_CONNECTION_SOCKET;
+        channel->u.socket.path = READ_BACKEND(NOGC, "path");
+    } else {
+	rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+ out:
+    return rc;
+}
+
+static int libxl__append_channel_list_of_type(libxl__gc *gc,
+                                              uint32_t domid,
+                                              const char *type,
+                                              libxl_device_channel **channels,
+                                              int *nchannels)
+{
+    char *fe_path = NULL, *be_path = NULL;
+    char **dir = NULL;
+    unsigned int n = 0, devid = 0;
+    libxl_device_channel *next = NULL;
+    int rc = 0, i;
+
+    fe_path = GCSPRINTF("%s/device/%s",
+                        libxl__xs_get_dompath(gc, domid), type);
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
+    if (!dir || !n)
+      goto out;
+
+    for (i = 0; i < n; i++) {
+        const char *p, *name;
+        libxl_device_channel *tmp;
+        p = libxl__sprintf(gc, "%s/%s", fe_path, dir[i]);
+        name = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/name", p));
+        /* 'channels' are consoles with names, so ignore all consoles
+           without names */
+        if (!name) continue;
+        be_path = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend", p));
+        tmp = realloc(*channels,
+                      sizeof(libxl_device_channel) * (*nchannels + devid + 1));
+        if (!tmp) {
+          rc = ERROR_NOMEM;
+          goto out;
+        }
+        *channels = tmp;
+        next = *channels + *nchannels + devid;
+        rc = libxl__device_channel_from_xs_be(gc, be_path, next);
+        if (rc) goto out;
+        next->devid = devid;
+        devid++;
+    }
+    *nchannels += devid;
+    return 0;
+
+ out:
+    return rc;
+}
+
+libxl_device_channel *libxl_device_channel_list(libxl_ctx *ctx,
+                                                uint32_t domid,
+                                                int *num)
+{
+    GC_INIT(ctx);
+    libxl_device_channel *channels = NULL;
+    int rc;
+
+    *num = 0;
+
+    rc = libxl__append_channel_list_of_type(gc, domid, "console", &channels, num);
+    if (rc) goto out_err;
+
+    GC_FREE;
+    return channels;
+
+out_err:
+    LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to list channels");
+    while (*num) {
+        (*num)--;
+        libxl_device_channel_dispose(&channels[*num]);
+    }
+    free(channels);
+    return NULL;
+}
+
+int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
+                                 libxl_device_channel *channel,
+                                 libxl_channelinfo *channelinfo)
+{
+    GC_INIT(ctx);
+    char *dompath, *fe_path;
+    char *val;
+
+    dompath = libxl__xs_get_dompath(gc, domid);
+    channelinfo->devid = channel->devid;
+
+    fe_path = libxl__sprintf(gc, "%s/device/console/%d", dompath,
+                             channelinfo->devid + 1);
+    channelinfo->backend = xs_read(ctx->xsh, XBT_NULL,
+                                   libxl__sprintf(gc, "%s/backend",
+                                   fe_path), NULL);
+    if (!channelinfo->backend) {
+        GC_FREE;
+        return ERROR_FAIL;
+    }
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/backend-id", fe_path));
+    channelinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/state", fe_path));
+    channelinfo->state = val ? strtoul(val, NULL, 10) : -1;
+    channelinfo->frontend = xs_read(ctx->xsh, XBT_NULL,
+                                    GCSPRINTF("%s/frontend",
+                                    channelinfo->backend), NULL);
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/frontend-id",
+                         channelinfo->backend));
+    channelinfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/ring-ref", fe_path));
+    channelinfo->rref = val ? strtoul(val, NULL, 10) : -1;
+    val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/port", fe_path));
+    channelinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
+
+    channelinfo->connection = channel->connection;
+    switch (channel->connection) {
+         case LIBXL_CHANNEL_CONNECTION_PTY:
+             val = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/tty", fe_path));
+             channelinfo->u.pty.path = strdup(val);
+             break;
+         default:
+             break;
+    }
+    GC_FREE;
+    return 0;
+}
+
+/******************************************************************************/
+
 int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb)
 {
     int rc;
@@ -3601,6 +3826,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
 DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
 DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
 
+/* channel/console hotunplug is not implemented. There are 2 possibilities:
+ * 1. add support for secondary consoles to xenconsoled
+ * 2. dynamically add/remove qemu chardevs via qmp messages. */
+
 #undef DEFINE_DEVICE_REMOVE
 
 /******************************************************************************/
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5ae6532..c102227 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -544,6 +544,15 @@ typedef struct libxl__ctx libxl_ctx;
  */
 #define LIBXL_HAVE_DEVICE_PCI_SEIZE 1
 
+/*
+ * LIBXL_HAVE_DEVICE_CHANNEL
+ *
+ * If this is defined, then the libxl_device_channel struct exists
+ * and channels can be attached to a domain both at build-time and at
+ * run-time by hotplug.
+ */
+#define LIBXL_HAVE_DEVICE_CHANNEL 1
+
 /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
  * called from within libxl itself. Callers outside libxl, who
  * do not #include libxl_internal.h, are fine. */
@@ -1061,6 +1070,14 @@ libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num
 int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
                               libxl_device_nic *nic, libxl_nicinfo *nicinfo);
 
+/* Virtual Channels */
+libxl_device_channel *libxl_device_channel_list(libxl_ctx *ctx,
+                                                uint32_t domid,
+                                                int *num);
+int libxl_device_channel_getinfo(libxl_ctx *ctx, uint32_t domid,
+                                 libxl_device_channel *channel,
+                                 libxl_channelinfo *channelinfo);
+
 /* Virtual TPMs */
 int libxl_device_vtpm_add(libxl_ctx *ctx, uint32_t domid, libxl_device_vtpm *vtpm,
                           const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0686f96..bfd179c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -349,14 +349,16 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     return 0;
 }
 
-static int init_console_info(libxl__device_console *console, int dev_num)
+static int init_console_info(libxl__gc *gc,
+                             libxl__device_console *console,
+                             int dev_num)
 {
-    memset(console, 0x00, sizeof(libxl__device_console));
+    libxl__device_console_init(console);
     console->devid = dev_num;
     console->consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
-    console->output = strdup("pty");
-    if (!console->output)
-        return ERROR_NOMEM;
+    console->output = libxl__strdup(NOGC, "pty");
+    /* console->{name,connection,path} are NULL on normal consoles.
+       Only 'channels' when mapped to consoles have a string name. */
     return 0;
 }
 
@@ -1155,17 +1157,31 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         }
     }
 
+    /* For both HVM and PV the 0th console is a regular console. We
+       map channels to IOEMU consoles starting at 1 */
+    for (i = 0; i < d_config->num_channels; i++) {
+        libxl__device_console console;
+        libxl__device device;
+        ret = libxl__init_console_from_channel(gc, &console, i + 1,
+                                               &d_config->channels[i]);
+        if ( ret )
+            goto error_out;
+        libxl__device_console_add(gc, domid, &console, NULL, &device);
+        libxl__device_console_dispose(&console);
+    }
+
     switch (d_config->c_info.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
     {
         libxl__device_console console;
+        libxl__device device;
         libxl_device_vkb vkb;
 
-        ret = init_console_info(&console, 0);
+        ret = init_console_info(gc, &console, 0);
         if ( ret )
             goto error_out;
         console.backend_domid = state->console_domid;
-        libxl__device_console_add(gc, domid, &console, state);
+        libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
         libxl_device_vkb_init(&vkb);
@@ -1183,22 +1199,24 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     {
         int need_qemu = 0;
         libxl__device_console console;
+        libxl__device device;
 
         for (i = 0; i < d_config->num_vfbs; i++) {
             libxl__device_vfb_add(gc, domid, &d_config->vfbs[i]);
             libxl__device_vkb_add(gc, domid, &d_config->vkbs[i]);
         }
 
-        ret = init_console_info(&console, 0);
+        ret = init_console_info(gc, &console, 0);
         if ( ret )
             goto error_out;
 
         need_qemu = libxl__need_xenpv_qemu(gc, 1, &console,
                 d_config->num_vfbs, d_config->vfbs,
-                d_config->num_disks, &d_config->disks[0]);
+                d_config->num_disks, &d_config->disks[0],
+                d_config->num_channels, &d_config->channels[0]);
 
         console.backend_domid = state->console_domid;
-        libxl__device_console_add(gc, domid, &console, state);
+        libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
         if (need_qemu) {
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index addacdb..5fcdd04 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -394,8 +394,9 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     const libxl_sdl_info *sdl = dm_sdl(guest_config);
     const char *keymap = dm_keymap(guest_config);
     flexarray_t *dm_args;
-    int i;
+    int i, connection, devid;
     uint64_t ram_size;
+    const char *path, *chardev;
 
     dm_args = flexarray_make(gc, 16, 1);
 
@@ -412,6 +413,28 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_append(dm_args, "-mon");
     flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
 
+    for (i = 0; i < guest_config->num_channels; i++) {
+        connection = guest_config->channels[i].connection;
+        devid = guest_config->channels[i].devid;
+        switch (connection) {
+            case LIBXL_CHANNEL_CONNECTION_PTY:
+                chardev = GCSPRINTF("pty,id=libxl-channel%d", devid);
+                break;
+            case LIBXL_CHANNEL_CONNECTION_SOCKET:
+                path = guest_config->channels[i].u.socket.path;
+                chardev = GCSPRINTF("socket,id=libxl-channel%d,path=%s,"
+                                    "server,nowait", devid, path);
+                break;
+            default:
+                /* We've forgotten to add the clause */
+                LOG(ERROR, "%s: unknown channel connection %d",
+                    __func__, connection);
+                return NULL;
+        }
+        flexarray_append(dm_args, "-chardev");
+        flexarray_append(dm_args, (void*)chardev);
+    }
+
     /*
      * Remove default devices created by qemu. Qemu will create only devices
      * defined by xen, since the devices not defined by xen are not usable.
@@ -1071,6 +1094,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
     }
 
     for (i = 0; i < num_console; i++) {
+        libxl__device device;
         console[i].devid = i;
         console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
         /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
@@ -1101,7 +1125,8 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
                 break;
         }
         ret = libxl__device_console_add(gc, dm_domid, &console[i],
-                        i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL);
+                        i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
+                        &device);
         if (ret)
             goto out;
     }
@@ -1521,7 +1546,8 @@ int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_consoles, libxl__device_console *consoles,
         int nr_vfbs, libxl_device_vfb *vfbs,
-        int nr_disks, libxl_device_disk *disks)
+        int nr_disks, libxl_device_disk *disks,
+        int nr_channels, libxl_device_channel *channels)
 {
     int i, ret = 0;
     uint32_t domid;
@@ -1561,6 +1587,20 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
         }
     }
 
+    if (nr_channels > 0) {
+        ret = libxl__get_domid(gc, &domid);
+        if (ret) goto out;
+        for (i = 0; i < nr_channels; i++) {
+            if (channels[i].backend_domid == domid) {
+                /* xenconsoled is limited to the first console only.
+                   Until this restriction is removed we must use qemu for
+                   secondary consoles which includes all channels. */
+                ret = 1;
+                goto out;
+            }
+        }
+    }
+
 out:
     return ret;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index beb052e..0c4ddc7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1016,7 +1016,8 @@ _hidden int libxl__device_disk_dev_number(const char *virtpath,
 
 _hidden int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
                                       libxl__device_console *console,
-                                      libxl__domain_build_state *state);
+                                      libxl__domain_build_state *state,
+                                      libxl__device *device);
 
 _hidden int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
         libxl__device *device, char **bents, char **fents, char **ro_fents);
@@ -1029,6 +1030,10 @@ _hidden int libxl__wait_for_backend(libxl__gc *gc, const char *be_path,
                                     const char *state);
 _hidden int libxl__nic_type(libxl__gc *gc, libxl__device *dev,
                             libxl_nic_type *nictype);
+_hidden int libxl__init_console_from_channel(libxl__gc *gc,
+                                             libxl__device_console *console,
+                                             int dev_num,
+                                             libxl_device_channel *channel);
 
 /*
  * For each aggregate type which can be used as an input we provide:
@@ -1448,7 +1453,8 @@ _hidden const char *libxl__domain_device_model(libxl__gc *gc,
 _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_consoles, libxl__device_console *consoles,
         int nr_vfbs, libxl_device_vfb *vfbs,
-        int nr_disks, libxl_device_disk *disks);
+        int nr_disks, libxl_device_disk *disks,
+        int nr_channels, libxl_device_channel *channels);
 
 /*
  * This function will cause the whole libxl process to hang
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a412f9c..fcaf32f 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -66,6 +66,12 @@ libxl_domain_type = Enumeration("domain_type", [
     (2, "PV"),
     ], init_val = "LIBXL_DOMAIN_TYPE_INVALID")
 
+libxl_channel_connection = Enumeration("channel_connection", [
+    (0, "UNKNOWN"),
+    (1, "PTY"),
+    (2, "SOCKET"), # a listening Unix domain socket
+    ])
+
 libxl_device_model_version = Enumeration("device_model_version", [
     (0, "UNKNOWN"),
     (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
@@ -262,6 +268,22 @@ libxl_cpupoolinfo = Struct("cpupoolinfo", [
     ("cpumap",      libxl_bitmap)
     ], dir=DIR_OUT)
 
+libxl_channelinfo = Struct("channelinfo", [
+    ("backend", string),
+    ("backend_id", uint32),
+    ("frontend", string),
+    ("frontend_id", uint32),
+    ("devid", libxl_devid),
+    ("state", integer),
+    ("evtch", integer),
+    ("rref", integer),
+    ("u", KeyedUnion(None, libxl_channel_connection, "connection",
+           [("unknown", None),
+            ("pty", Struct(None, [("path", string),])),
+            ("socket", None),
+           ])),
+    ], dir=DIR_OUT)
+
 libxl_vminfo = Struct("vminfo", [
     ("uuid", libxl_uuid),
     ("domid", libxl_domid),
@@ -479,6 +501,18 @@ libxl_device_vtpm = Struct("device_vtpm", [
     ("uuid",             libxl_uuid),
 ])
 
+libxl_device_channel = Struct("device_channel", [
+    ("backend_domid", libxl_domid),
+    ("backend_domname", string),
+    ("devid", libxl_devid),
+    ("name", string),
+    ("u", KeyedUnion(None, libxl_channel_connection, "connection",
+           [("unknown", None),
+            ("pty", None),
+            ("socket", Struct(None, [("path", string)])),
+           ])),
+])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -489,6 +523,7 @@ libxl_domain_config = Struct("domain_config", [
     ("vfbs", Array(libxl_device_vfb, "num_vfbs")),
     ("vkbs", Array(libxl_device_vkb, "num_vkbs")),
     ("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
+    ("channels", Array(libxl_device_channel, "num_channels")),
 
     ("on_poweroff", libxl_action_on_shutdown),
     ("on_reboot", libxl_action_on_shutdown),
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 800361b..a62167c 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -34,6 +34,10 @@ libxl__device_console = Struct("device_console", [
     ("devid", integer),
     ("consback", libxl__console_backend),
     ("output", string),
+    # These are specific to the 'channels', built on top of consoles:
+    ("name", string),
+    ("connection", string),
+    ("path", string),
     ])
 
 libxl__device_action = Enumeration("device_action", [
-- 
1.7.10.4

[PATCH v4 3/3] xl: add support for 'channels'

[David Scott]

This adds support for channel declarations of the form:

 channel = [ "name=...,connection=...[,path=...][,backend=...]" ]

where 'name' is a label to identify the channel to the frontend.

If 'connection = pty' then the channel is connected to a pty in the
 backend domain
If 'connection = socket' then the channel is connected to a Unix domain
 socket given by 'path = ...' in the backend domain.

This patch also adds the command:

 xl channel-list <domain>

which allows the state of channels to be queried. In particular if
'connection=pty' this will show the path of the pty slave device.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 docs/man/xl.cfg.pod.5     |   51 +++++++++++
 docs/man/xl.pod.1         |   10 +++
 tools/libxl/xl.h          |    1 +
 tools/libxl/xl_cmdimpl.c  |  207 ++++++++++++++++++++++++++++++++++++++++++---
 tools/libxl/xl_cmdtable.c |    5 ++
 5 files changed, 264 insertions(+), 10 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index ff9ea77..6c43e7da 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -470,6 +470,57 @@ L<qemu(1)> manpage. The default is B<en-us>.
 
 =back
 
+=item B<channel=[ "CHANNEL_SPEC_STRING", "CHANNEL_SPEC_STRING", ...]>
+
+Specifies the virtual channels to be provided to the guest. A
+channel is a low-bandwidth, bidirectional byte stream, which resembles
+a serial link. Typical uses for channels include transmitting VM
+configuration after boot and signalling to in-guest agents. Please see
+F<docs/misc/channels.txt> for more details.
+
+Each B<CHANNEL_SPEC_STRING> is a comma-separated list of C<KEY=VALUE>
+settings, from the following list:
+
+=over 4
+
+=item C<backend=DOMAIN>
+
+Specify the backend domain name or id. This parameter is optional. If
+this parameter is omitted then the toolstack domain will be assumed.
+
+=item C<name=NAME>
+
+Specify the string name for this device. This parameter is mandatory.
+This should be a well-known name for the specific application (e.g.
+guest agent) and should be used by the frontend to connect the
+application to the right channel device. There is no formal registry
+of channel names, so application authors are encouraged to make their
+names unique by including domain name and version number in the string
+(e.g. org.mydomain.guestagent.1).
+
+=item C<connection=CONNECTION>
+
+Specify how the backend will be implemented. This following options are
+available:
+
+=over 4
+
+=item B<connection=SOCKET>:
+
+The backend will bind a Unix domain socket (at the path given by
+B<path=PATH>), call listen and accept connections. The backend will proxy
+data between the channel and the connected socket.
+
+=item B<connection=PTY>:
+
+The backend will create a pty and proxy data between the channel and the
+master device. The command B<xl channel-list> can be used to discover the
+assigned slave device.
+
+=back
+
+=back
+
 =item B<pci=[ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ... ]>
 
 Specifies the host PCI devices to passthrough to this guest. Each B<PCI_SPEC_STRING>
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..827f5a5 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1194,6 +1194,16 @@ List virtual network interfaces for a domain.
 
 =back
 
+=head2 CHANNEL DEVICES
+
+=over 4
+
+=item B<channel-list> I<domain-id>
+
+List virtual channel interfaces for a domain.
+
+=back
+
 =head2 VTPM DEVICES
 
 =over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 10a2e66..afc5f8b 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -78,6 +78,7 @@ int main_top(int argc, char **argv);
 int main_networkattach(int argc, char **argv);
 int main_networklist(int argc, char **argv);
 int main_networkdetach(int argc, char **argv);
+int main_channellist(int argc, char **argv);
 int main_blockattach(int argc, char **argv);
 int main_blocklist(int argc, char **argv);
 int main_blockdetach(int argc, char **argv);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 68df548..48e16c0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -298,6 +298,18 @@ static void *xrealloc(void *ptr, size_t sz) {
     return r;
 }
 
+static char *xstrdup(const char *x)
+{
+    char *r;
+    r = strdup(x);
+    if (!r) {
+        fprintf(stderr, "xl: Unable to strdup a string of length %zu.\n",
+                strlen(x));
+        exit(-ERROR_FAIL);
+    }
+    return r;
+}
+
 #define ARRAY_EXTEND_INIT(array,count,initfn)                           \
     ({                                                                  \
         typeof((count)) array_extend_old_count = (count);               \
@@ -516,7 +528,7 @@ static void split_string_into_string_list(const char *str,
 
     s = strdup(str);
     if (s == NULL) {
-        fprintf(stderr, "unable to allocate memory to parse bootloader args\n");
+        fprintf(stderr, "unable to allocate memory to split string\n");
         exit(-1);
     }
 
@@ -532,7 +544,7 @@ static void split_string_into_string_list(const char *str,
 
     sl = malloc((nr+1) * sizeof (char *));
     if (sl == NULL) {
-        fprintf(stderr, "unable to allocate memory for bootloader args\n");
+        fprintf(stderr, "unable to allocate memory to split string\n");
         exit(-1);
     }
 
@@ -549,6 +561,64 @@ static void split_string_into_string_list(const char *str,
     free(s);
 }
 
+typedef int (*char_predicate_t)(const int c);
+
+static void trim(char_predicate_t predicate, const char *input, char **output)
+{
+    char *p, *q, *tmp;
+    if (*input == '\000')
+        return;
+    /* Input has length >= 1 */
+
+    p = tmp = xstrdup(input);
+    /* Skip past the first whitespace */
+    while ((*p != '\000') && (predicate(*p)))
+        p ++;
+    q = p + strlen(p) - 1;
+    /* q points to the last non-NULL character */
+    while ((q > p) && (predicate(*q)))
+        q --;
+    /* q points to the last character we want */
+    q ++;
+    *q = '\000';
+    *output = xstrdup(p);
+    free(tmp);
+}
+
+static int split_string_into_pair(const char *str,
+                                  const char *delim,
+                                  char **a,
+                                  char **b)
+{
+    char *s, *p, *saveptr, *aa = NULL, *bb = NULL;
+    int rc = 0;
+
+    s = xstrdup(str);
+
+    p = strtok_r(s, delim, &saveptr);
+    if (p == NULL) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    aa = xstrdup(p);
+    p = strtok_r(NULL, delim, &saveptr);
+    if (p == NULL) {
+        rc = ERROR_INVAL;
+        goto out;
+    }
+    bb = xstrdup(p);
+
+    *a = aa;
+    aa = NULL;
+    *b = bb;
+    bb = NULL;
+out:
+    free(s);
+    free(aa);
+    free(bb);
+    return rc;
+}
+
 static int parse_range(const char *str, unsigned long *a, unsigned long *b)
 {
     const char *nstr;
@@ -690,6 +760,12 @@ static void parse_top_level_sdl_options(XLU_Config *config,
     xlu_cfg_replace_string (config, "xauthority", &sdl->xauthority, 0);
 }
 
+static void replace_string(char **str, const char *val)
+{
+    free(*str);
+    *str = strdup(val);
+}
+
 static void parse_config_data(const char *config_source,
                               const char *config_data,
                               int config_len,
@@ -699,7 +775,7 @@ static void parse_config_data(const char *config_source,
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
-    XLU_ConfigList *ioports, *irqs, *iomem;
+    XLU_ConfigList *channels, *ioports, *irqs, *iomem;
     int num_ioports, num_irqs, num_iomem, num_cpus;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
@@ -1229,6 +1305,79 @@ static void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list (config, "channel", &channels, 0, 0)) {
+        d_config->num_channels = 0;
+        d_config->channels = NULL;
+        while ((buf = xlu_cfg_get_listitem (channels,
+                d_config->num_channels)) != NULL) {
+            libxl_device_channel *chn;
+            libxl_string_list pairs;
+            char *path = NULL;
+            int len;
+
+            chn = ARRAY_EXTEND_INIT(d_config->channels, d_config->num_channels,
+                                   libxl_device_channel_init);
+
+            split_string_into_string_list(buf, ",", &pairs);
+            len = libxl_string_list_length(&pairs);
+            for (i = 0; i < len; i++) {
+                char *key, *key_untrimmed, *value, *value_untrimmed;
+                int rc;
+                rc = split_string_into_pair(pairs[i], "=",
+                                            &key_untrimmed,
+                                            &value_untrimmed);
+                if (rc != 0) {
+                    fprintf(stderr, "failed to parse channel configuration: %s",
+                            pairs[i]);
+                    exit(1);
+                }
+                trim(isspace, key_untrimmed, &key);
+                trim(isspace, value_untrimmed, &value);
+
+                if (!strcmp(key, "backend")) {
+                    replace_string(&chn->backend_domname, value);
+                } else if (!strcmp(key, "name")) {
+                    replace_string(&chn->name, value);
+                } else if (!strcmp(key, "path")) {
+                    replace_string(&path, value);
+                } else if (!strcmp(key, "connection")) {
+                    if (!strcmp(value, "pty")) {
+                        chn->connection = LIBXL_CHANNEL_CONNECTION_PTY;
+                    } else if (!strcmp(value, "socket")) {
+                        chn->connection = LIBXL_CHANNEL_CONNECTION_SOCKET;
+                    } else {
+                        fprintf(stderr, "unknown channel connection '%s'\n",
+                                value);
+                        exit(1);
+                    }
+                } else {
+                    fprintf(stderr, "unknown channel parameter '%s',"
+                                  " ignoring\n", key);
+                }
+                free(key);
+                free(key_untrimmed);
+                free(value);
+                free(value_untrimmed);
+            }
+            switch (chn->connection){
+            case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
+                fprintf(stderr, "channel has unknown 'connection'\n");
+                exit(1);
+            case LIBXL_CHANNEL_CONNECTION_SOCKET:
+                if (!path) {
+                    fprintf(stderr, "channel connection 'socket' requires path=..\n");
+                    exit(1);
+                }
+                chn->u.socket.path = xstrdup(path);
+                break;
+            default:
+                break;
+            }
+            libxl_string_list_dispose(&pairs);
+            free(path);
+        }
+    }
+
     if (!xlu_cfg_get_list (config, "vif", &nics, 0, 0)) {
         d_config->num_nics = 0;
         d_config->nics = NULL;
@@ -1815,13 +1964,6 @@ static int match_option_size(const char *prefix, size_t len,
 #define MATCH_OPTION(prefix, arg, oparg) \
     match_option_size((prefix "="), sizeof((prefix)), (arg), &(oparg))
 
-static void replace_string(char **str, const char *val)
-{
-    free(*str);
-    *str = strdup(val);
-}
-
-
 /* Preserve a copy of a domain under a new name. Updates *r_domid */
 static int preserve_domain(uint32_t *r_domid, libxl_event *event,
                            libxl_domain_config *d_config)
@@ -5818,6 +5960,51 @@ int main_networkdetach(int argc, char **argv)
     return 0;
 }
 
+int main_channellist(int argc, char **argv)
+{
+    int opt;
+    libxl_device_channel *channels;
+    libxl_channelinfo channelinfo;
+    int nb, i;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "channel-list", 1) {
+        /* No options */
+    }
+
+    /*      Idx BE state evt-ch ring-ref connection params*/
+    printf("%-3s %-2s %-5s %-6s %8s %-10s %-30s\n",
+           "Idx", "BE", "state", "evt-ch", "ring-ref", "connection", "");
+    for (argv += optind, argc -= optind; argc > 0; --argc, ++argv) {
+        uint32_t domid = find_domain(*argv);
+        channels = libxl_device_channel_list(ctx, domid, &nb);
+        if (!channels) {
+            continue;
+        }
+        for (i = 0; i < nb; ++i) {
+            if (!libxl_device_channel_getinfo(ctx, domid, &channels[i],
+                &channelinfo)) {
+                printf("%-3d %-2d ", channels[i].devid, channelinfo.backend_id);
+                printf("%-5d ", channelinfo.state);
+                printf("%-6d %-8d ", channelinfo.evtch, channelinfo.rref);
+                printf("%-10s ", libxl_channel_connection_to_string(
+                       channels[i].connection));
+                switch (channels[i].connection) {
+                    case LIBXL_CHANNEL_CONNECTION_PTY:
+                        printf("%-30s ", channelinfo.u.pty.path);
+                        break;
+                    default:
+                        break;
+                }
+                printf("\n");
+                libxl_channelinfo_dispose(&channelinfo);
+            }
+            libxl_device_channel_dispose(&channels[i]);
+        }
+        free(channels);
+    }
+    return 0;
+}
+
 int main_blockattach(int argc, char **argv)
 {
     int opt;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4279b9f..e8d01ae 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -335,6 +335,11 @@ struct cmd_spec cmd_table[] = {
       "Destroy a domain's virtual network device",
       "<Domain> <DevId|mac>",
     },
+    { "channel-list",
+      &main_channellist, 0, 0,
+      "List virtual channel devices for a domain",
+      "<Domain(s)>",
+    },
     { "block-attach",
       &main_blockattach, 1, 1,
       "Create a new virtual block device",
-- 
1.7.10.4

Re: [PATCH v4 1/3] libxl IDL: the name of a KeyedUnion descriminator need not be 'type'

[Ian Campbell]

On Tue, 2014-07-22 at 16:05 +0100, David Scott wrote:
> Signed-off-by: David Scott <dave.scott@citrix.com>

"discriminator"

Since there are no existing KeyedUnions who's discriminator is not
"type" there should be no change to the resulting code, but I've not
gone and confirmed that myself.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I've applied this one and fixed the typo on the way.

> ---
>  tools/libxl/gentypes.py |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 4b0c996..3e73821 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -440,7 +440,7 @@ def libxl_C_type_parse_json(ty, w, v, indent = "    ", parent = None, discrimina
>                           (f.type.keyvar.name + "." + x.name, w)
>                      s += "    if (x) {\n"
>                      (nparent, fexpr) = ty.member(v, f.type.keyvar, parent is None)
> -                    s += "        %s_init_type(%s, %s);\n" % (ty.typename, v, x.enumname)
> +                    s += "        %s_init_%s(%s, %s);\n" % (ty.typename, f.type.keyvar.name, v, x.enumname)
>                      (nparent,fexpr) = ty.member(v, f, parent is None)
>                      s += libxl_C_type_parse_json(f.type, "x", fexpr, "  ", nparent, x.enumname)
>                      s += "    }\n"

Re: [PATCH v4] add support for libvirt-like channels

[David Vrabel]

On 22/07/14 16:05, David Scott wrote:
> 
> Note: I've seen a problem with some Linux console frontends which attempt
> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
> directory. [ this key is already present, it's not added by these patches ]
> Since 'type' refers to the toolstack's choice of implementation service
> (which is none of the guest's business) I think the read/only permissions
> are right. The location of the key is not ideal (it should be in the
> backend directory IMHO) but this is the case with several other keys located
> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
> bug which should be fixed there. These patches work with Mirage VMs and
> with Linux if I workaround the permissions by giving the guest read/write
> to the 'type' key. 

Which Linux frontends?

David

Re: [PATCH v4] add support for libvirt-like channels

[Dave Scott]

On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote:

> On 22/07/14 16:05, David Scott wrote:
>> 
>> Note: I've seen a problem with some Linux console frontends which attempt
>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
>> directory. [ this key is already present, it's not added by these patches ]
>> Since 'type' refers to the toolstack's choice of implementation service
>> (which is none of the guest's business) I think the read/only permissions
>> are right. The location of the key is not ideal (it should be in the
>> backend directory IMHO) but this is the case with several other keys located
>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
>> bug which should be fixed there. These patches work with Mirage VMs and
>> with Linux if I workaround the permissions by giving the guest read/write
>> to the 'type' key. 
> 
> Which Linux front ends?

I just tested a Debian jessie with

  # uname -a
  Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux

In my xenstored access log I see:

  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/ring-ref 8
  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/port 10
  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/type ioemu

— I think the last line is suspicious.

Cheers,
Dave

Re: [PATCH v4] add support for libvirt-like channels

[David Vrabel]

On 07/08/14 14:37, Dave Scott wrote:
> 
> On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote:
> 
>> On 22/07/14 16:05, David Scott wrote:
>>>
>>> Note: I've seen a problem with some Linux console frontends which attempt
>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
>>> directory. [ this key is already present, it's not added by these patches ]
>>> Since 'type' refers to the toolstack's choice of implementation service
>>> (which is none of the guest's business) I think the read/only permissions
>>> are right. The location of the key is not ideal (it should be in the
>>> backend directory IMHO) but this is the case with several other keys located
>>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
>>> bug which should be fixed there. These patches work with Mirage VMs and
>>> with Linux if I workaround the permissions by giving the guest read/write
>>> to the 'type' key. 
>>
>> Which Linux front ends?
> 
> I just tested a Debian jessie with
> 
>   # uname -a
>   Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux
> 
> In my xenstored access log I see:
> 
>   Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/ring-ref 8
>   Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/port 10
>   Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/type ioemu
> 
> — I think the last line is suspicious.

This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen:
implement multiconsole support).

Stefano, can you advise on whether it it safe to remove the write of the
"type" key or whether we need to make it conditional on it being absent.

David

Re: [PATCH v4] add support for libvirt-like channels

[Stefano Stabellini]

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

On Thu, 7 Aug 2014, David Vrabel wrote:
> On 07/08/14 14:37, Dave Scott wrote:
> > 
> > On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote:
> > 
> >> On 22/07/14 16:05, David Scott wrote:
> >>>
> >>> Note: I've seen a problem with some Linux console frontends which attempt
> >>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
> >>> directory. [ this key is already present, it's not added by these patches ]
> >>> Since 'type' refers to the toolstack's choice of implementation service
> >>> (which is none of the guest's business) I think the read/only permissions
> >>> are right. The location of the key is not ideal (it should be in the
> >>> backend directory IMHO) but this is the case with several other keys located
> >>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
> >>> bug which should be fixed there. These patches work with Mirage VMs and
> >>> with Linux if I workaround the permissions by giving the guest read/write
> >>> to the 'type' key. 
> >>
> >> Which Linux front ends?
> > 
> > I just tested a Debian jessie with
> > 
> >   # uname -a
> >   Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux
> > 
> > In my xenstored access log I see:
> > 
> >   Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/ring-ref 8
> >   Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/port 10
> >   Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/type ioemu
> > 
> > — I think the last line is suspicious.
> 
> This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen:
> implement multiconsole support).
> 
> Stefano, can you advise on whether it it safe to remove the write of the
> "type" key or whether we need to make it conditional on it being absent.
 
I think that the original idea was that since only ioemu backends (QEMU)
are capable of handling multiple consoles and the new xenstore based
console protocol, then the "type" had to be "ioemu".

I agree that the type key has no business being in the frontend
directory.
I also agree that the frontend shouldn't be writing it, since the
backend would have already been started anyway.

I think it would be OK to:

---
xen_hvc: no reason to write the type key on xenstore

The backend type is chosen by the toolstack. Regardless the frontend
should not care.

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

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 636c9ba..b8491f5 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device *dev,
 			    evtchn);
 	if (ret)
 		goto error_xenbus;
-	ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu");
-	if (ret)
-		goto error_xenbus;
 	ret = xenbus_transaction_end(xbt, 0);
 	if (ret) {
 		if (ret == -EAGAIN)

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

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

Re: [PATCH v4] add support for libvirt-like channels

[Dave Scott]

On 7 Aug 2014, at 15:26, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com> wrote:

> On Thu, 7 Aug 2014, David Vrabel wrote:
>> On 07/08/14 14:37, Dave Scott wrote:
>>> 
>>> On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote:
>>> 
>>>> On 22/07/14 16:05, David Scott wrote:
>>>>> 
>>>>> Note: I've seen a problem with some Linux console frontends which attempt
>>>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
>>>>> directory. [ this key is already present, it's not added by these patches ]
>>>>> Since 'type' refers to the toolstack's choice of implementation service
>>>>> (which is none of the guest's business) I think the read/only permissions
>>>>> are right. The location of the key is not ideal (it should be in the
>>>>> backend directory IMHO) but this is the case with several other keys located
>>>>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
>>>>> bug which should be fixed there. These patches work with Mirage VMs and
>>>>> with Linux if I workaround the permissions by giving the guest read/write
>>>>> to the 'type' key. 
>>>> 
>>>> Which Linux front ends?
>>> 
>>> I just tested a Debian jessie with
>>> 
>>>  # uname -a
>>>  Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux
>>> 
>>> In my xenstored access log I see:
>>> 
>>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/ring-ref 8
>>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/port 10
>>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/type ioemu
>>> 
>>> — I think the last line is suspicious.
>> 
>> This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen:
>> implement multiconsole support).
>> 
>> Stefano, can you advise on whether it it safe to remove the write of the
>> "type" key or whether we need to make it conditional on it being absent.
> 
> I think that the original idea was that since only ioemu backends (QEMU)
> are capable of handling multiple consoles and the new xenstore based
> console protocol, then the "type" had to be "ioemu".
> 
> I agree that the type key has no business being in the frontend
> directory.
> I also agree that the frontend shouldn't be writing it, since the
> backend would have already been started anyway.

Would you like me to move the key to the backend directory? I could leave a writable (but unused) key in the frontend directory for backwards compatibility. Or should I leave this alone?

> 
> I think it would be OK to:
> 
> ---
> xen_hvc: no reason to write the type key on xenstore
> 
> The backend type is chosen by the toolstack. Regardless the frontend
> should not care.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 636c9ba..b8491f5 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device *dev,
> 			    evtchn);
> 	if (ret)
> 		goto error_xenbus;
> -	ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu");
> -	if (ret)
> -		goto error_xenbus;
> 	ret = xenbus_transaction_end(xbt, 0);
> 	if (ret) {
> 		if (ret == -EAGAIN)

This looks good to me :-)

Cheers,
Dave

Re: [PATCH v4] add support for libvirt-like channels

[Stefano Stabellini]

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

On Mon, 11 Aug 2014, Dave Scott wrote:
> On 7 Aug 2014, at 15:26, Stefano Stabellini <Stefano.Stabellini@eu.citrix.com> wrote:
> 
> > On Thu, 7 Aug 2014, David Vrabel wrote:
> >> On 07/08/14 14:37, Dave Scott wrote:
> >>> 
> >>> On 28 Jul 2014, at 15:22, David Vrabel <david.vrabel@citrix.com> wrote:
> >>> 
> >>>> On 22/07/14 16:05, David Scott wrote:
> >>>>> 
> >>>>> Note: I've seen a problem with some Linux console frontends which attempt
> >>>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
> >>>>> directory. [ this key is already present, it's not added by these patches ]
> >>>>> Since 'type' refers to the toolstack's choice of implementation service
> >>>>> (which is none of the guest's business) I think the read/only permissions
> >>>>> are right. The location of the key is not ideal (it should be in the
> >>>>> backend directory IMHO) but this is the case with several other keys located
> >>>>> in the frontend such as 'limit' and 'tty'. I think this is a Linux frontend
> >>>>> bug which should be fixed there. These patches work with Mirage VMs and
> >>>>> with Linux if I workaround the permissions by giving the guest read/write
> >>>>> to the 'type' key. 
> >>>> 
> >>>> Which Linux front ends?
> >>> 
> >>> I just tested a Debian jessie with
> >>> 
> >>>  # uname -a
> >>>  Linux jessie 3.13-6-686-pae #1 SMP Debian 3.13.5-1 (2014-03-04) i686 GNU/Linux
> >>> 
> >>> In my xenstored access log I see:
> >>> 
> >>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/ring-ref 8
> >>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/port 10
> >>>  Aug  7 14:28:30 localhost xenstored:  D40.6        write     device/console/1/type ioemu
> >>> 
> >>> — I think the last line is suspicious.
> >> 
> >> This was introduced by Stefano in 3.4 with 02e19f9c7cac (hvc_xen:
> >> implement multiconsole support).
> >> 
> >> Stefano, can you advise on whether it it safe to remove the write of the
> >> "type" key or whether we need to make it conditional on it being absent.
> > 
> > I think that the original idea was that since only ioemu backends (QEMU)
> > are capable of handling multiple consoles and the new xenstore based
> > console protocol, then the "type" had to be "ioemu".
> > 
> > I agree that the type key has no business being in the frontend
> > directory.
> > I also agree that the frontend shouldn't be writing it, since the
> > backend would have already been started anyway.
> 
> Would you like me to move the key to the backend directory? I could leave a writable (but unused) key in the frontend directory for backwards compatibility. Or should I leave this alone?

Like you say, we cannot get rid of key in the frontend directory without
breaking existing backends. In particular QEMU is often built out of the
Xen tree by distros: we cannot rely on it having the necessary
modifications. Duplicating the key to the backend directory, with the
intent of removing the copy in the frontend directory might be the
right thing to do. We could change all the backends to look in the
backend directory then in a couple of years finally remove the copy
in the frontend path. I am not sure if it's worth the effort though :-)


> > I think it would be OK to:
> > 
> > ---
> > xen_hvc: no reason to write the type key on xenstore
> > 
> > The backend type is chosen by the toolstack. Regardless the frontend
> > should not care.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index 636c9ba..b8491f5 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -402,9 +402,6 @@ static int xencons_connect_backend(struct xenbus_device *dev,
> > 			    evtchn);
> > 	if (ret)
> > 		goto error_xenbus;
> > -	ret = xenbus_printf(xbt, dev->nodename, "type", "ioemu");
> > -	if (ret)
> > -		goto error_xenbus;
> > 	ret = xenbus_transaction_end(xbt, 0);
> > 	if (ret) {
> > 		if (ret == -EAGAIN)
> 
> This looks good to me :-)

I'll send out the patch.

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

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

Re: [PATCH v4] add support for libvirt-like channels

[Ian Campbell]

On Mon, 2014-07-28 at 15:22 +0100, David Vrabel wrote:
> On 22/07/14 16:05, David Scott wrote:
> > 
> > Note: I've seen a problem with some Linux console frontends which attempt
> > to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
> > directory. [ this key is already present, it's not added by these patches ]
> > Since 'type' refers to the toolstack's choice of implementation service
> > (which is none of the guest's business) I think the read/only permissions
> > are right. The location of the key is not ideal (it should be in the
> > backend directory IMHO) but this is the case with several other keys located
> > in the frontend such as 'limit' and 'tty'.

I think this probably arose because the first PV console is not (or at
least historically wasn't) a normal front/back pair so I think it may
only have had a f.e. directory. It's likely that this then carried over
into the support for multiple console channels which do follow the
front/back model.

>  I think this is a Linux frontend
> > bug which should be fixed there.

Yeah, irrespective of the above it was never right for the frontend to
try and write this node AFAICT.

>  These patches work with Mirage VMs and
> > with Linux if I workaround the permissions by giving the guest read/write
> > to the 'type' key. 
> 
> Which Linux frontends?

Did this get answered/sorted (if so then I'm afraid I've lost that
subthread).

Any idea why this only happens with these patches, since as you say the
node is already there?

Does this affect the primary console device or only secondary ones? Or
perhaps only secondary ones created using this channels infrastructure? 

I'm trying to get a feel for how worried I should be about the potential
for regressing existing guests with this change.

Ian.

Re: [PATCH v4] add support for libvirt-like channels

[Dave Scott]

Hi,

On 10 Sep 2014, at 14:07, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Mon, 2014-07-28 at 15:22 +0100, David Vrabel wrote:
>> On 22/07/14 16:05, David Scott wrote:
>>> 
>>> Note: I've seen a problem with some Linux console frontends which attempt
>>> to overwrite the read-only key 'type' (= xenconsoled|ioemu) in the frontend
>>> directory. [ this key is already present, it's not added by these patches ]
>>> Since 'type' refers to the toolstack's choice of implementation service
>>> (which is none of the guest's business) I think the read/only permissions
>>> are right. The location of the key is not ideal (it should be in the
>>> backend directory IMHO) but this is the case with several other keys located
>>> in the frontend such as 'limit' and 'tty'.
> 
> I think this probably arose because the first PV console is not (or at
> least historically wasn't) a normal front/back pair so I think it may
> only have had a f.e. directory. It's likely that this then carried over
> into the support for multiple console channels which do follow the
> front/back model.

Aha, that explains a lot.

> 
>> I think this is a Linux frontend
>>> bug which should be fixed there.
> 
> Yeah, irrespective of the above it was never right for the frontend to
> try and write this node AFAICT.
> 
>> These patches work with Mirage VMs and
>>> with Linux if I workaround the permissions by giving the guest read/write
>>> to the 'type' key. 
>> 
>> Which Linux frontends?
> 
> Did this get answered/sorted (if so then I'm afraid I've lost that
> subthread).

Yes, Stefano and David removed the write in the Linux frontend:

https://lkml.org/lkml/2014/8/11/219

I’m not sure if this has been merged yet.

> Any idea why this only happens with these patches, since as you say the
> node is already there?
> 
> Does this affect the primary console device or only secondary ones? Or
> perhaps only secondary ones created using this channels infrastructure? 

It seems to only affect secondary consoles (which includes the channels).

> I'm trying to get a feel for how worried I should be about the potential
> for regressing existing guests with this change.

I believe the ‘type’ frontend key has always been read/only (at least within recent memory):

28d386fc        (Ian Jackson    2013-06-25 11:24:22 +0100       3321)    flexarray_append(ro_front, "type");
ffa165bb        (Ian Campbell   2012-03-01 12:26:14 +0000       3322)    if (console->consback == LIBXL__CONSOLE_BACKEND_XENNSOLED)
28d386fc        (Ian Jackson    2013-06-25 11:24:22 +0100       3323)        flexarray_append(ro_front, "xenconsoled");
5588033d        (Ian Campbell   2010-10-05 17:51:28 +0100       3324)    else
28d386fc        (Ian Jackson    2013-06-25 11:24:22 +0100       3325)        flexarray_append(ro_front, "ioemu”);

As a sanity check I just fired up a Fedora 20 guest with a single (primary) console. In xenstore the key is read-only as expected:

$ sudo xenstore-ls /local/domain/3/console -p
backend = "/local/domain/0/backend/console/3/0"  . . . . . .  (n0,r3)
backend-id = "0" . . . . . . . . . . . . . . . . . . . . . .  (n3,r0)
limit = "1048576"  . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
type = "xenconsoled" . . . . . . . . . . . . . . . . . . . .  (n0,r3)
output = "pty" . . . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
tty = "/dev/pts/1" . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
port = "2" . . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
ring-ref = "1044479" . . . . . . . . . . . . . . . . . . . .  (n0,r3)
vnc-listen = "0.0.0.0" . . . . . . . . . . . . . . . . . . .  (n0,r3)
vnc-port = "5900"  . . . . . . . . . . . . . . . . . . . . .  (n0,r3)

And when I attached ’screen’ to ‘/dev/pts/1’ I found a getty.

I also had a quick look at what pre-libxl xapi does — it seems not to create a ‘type’ key at all. Primary consoles seem to work ok.

I did a quick rebase and retest of these patches. I’m personally fairly confident that nothing bad will happen but I’m a little biased ;-)

Let me know if I should resend. If you want to take a look they’re hosted in this branch:

  git pull git://github.com/djs55/xen add-channels10

Cheers,
Dave

Re: [PATCH v4] add support for libvirt-like channels

[David Vrabel]

On 10/09/14 14:45, Dave Scott wrote:
> Hi,
> 
> On 10 Sep 2014, at 14:07, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
>> On Mon, 2014-07-28 at 15:22 +0100, David Vrabel wrote:
>>> On 22/07/14 16:05, David Scott wrote:
>>> Which Linux frontends?
>>
>> Did this get answered/sorted (if so then I'm afraid I've lost that
>> subthread).
> 
> Yes, Stefano and David removed the write in the Linux frontend:
> 
> https://lkml.org/lkml/2014/8/11/219
> 
> I’m not sure if this has been merged yet.

Well, Stefano posted a patch but didn't follow up on it and it has not
been picked up by the tty maintainer.

David

Re: [PATCH v4] add support for libvirt-like channels

[Ian Campbell]

On Wed, 2014-09-10 at 14:45 +0100, Dave Scott wrote:

> > I'm trying to get a feel for how worried I should be about the potential
> > for regressing existing guests with this change.
> 
> I believe the ‘type’ frontend key has always been read/only (at least within recent memory):
> 
> 28d386fc        (Ian Jackson    2013-06-25 11:24:22 +0100       3321)    flexarray_append(ro_front, "type");
> ffa165bb        (Ian Campbell   2012-03-01 12:26:14 +0000       3322)    if (console->consback == LIBXL__CONSOLE_BACKEND_XENNSOLED)
> 28d386fc        (Ian Jackson    2013-06-25 11:24:22 +0100       3323)        flexarray_append(ro_front, "xenconsoled");
> 5588033d        (Ian Campbell   2010-10-05 17:51:28 +0100       3324)    else
> 28d386fc        (Ian Jackson    2013-06-25 11:24:22 +0100       3325)        flexarray_append(ro_front, "ioemu”);

Ah, looks like it was since XSA-57, which was about unduly lose xenstore
permissions.

> As a sanity check I just fired up a Fedora 20 guest with a single (primary) console. In xenstore the key is read-only as expected:
> 
> $ sudo xenstore-ls /local/domain/3/console -p
> backend = "/local/domain/0/backend/console/3/0"  . . . . . .  (n0,r3)
> backend-id = "0" . . . . . . . . . . . . . . . . . . . . . .  (n3,r0)
> limit = "1048576"  . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> type = "xenconsoled" . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> output = "pty" . . . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> tty = "/dev/pts/1" . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> port = "2" . . . . . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> ring-ref = "1044479" . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> vnc-listen = "0.0.0.0" . . . . . . . . . . . . . . . . . . .  (n0,r3)
> vnc-port = "5900"  . . . . . . . . . . . . . . . . . . . . .  (n0,r3)
> 
> And when I attached ’screen’ to ‘/dev/pts/1’ I found a getty.
> 
> I also had a quick look at what pre-libxl xapi does — it seems not to create a ‘type’ key at all. Primary consoles seem to work ok.
> 
> I did a quick rebase and retest of these patches. I’m personally fairly confident that nothing bad will happen but I’m a little biased ;-)
> 
> Let me know if I should resend. If you want to take a look they’re hosted in this branch:
> 
>   git pull git://github.com/djs55/xen add-channels10

I think you should probably wait for IanJ's input, since he had some
thoughts last time wrt the protocol docs etc.

Ian

> 
> Cheers,
> Dave



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

Re: [PATCH v4 2/3] libxl: add support for 'channels'

[Ian Campbell]

On Tue, 2014-07-22 at 16:05 +0100, David Scott wrote:
> diff --git a/docs/misc/console.txt b/docs/misc/console.txt
> index 8a53a95..eb08aff 100644
> --- a/docs/misc/console.txt
> +++ b/docs/misc/console.txt
> @@ -9,10 +9,11 @@ relevant information in xenstore under /local/domain/$DOMID/console.
>  
>  Now many years after the introduction of the pv console we have
>  multiple pv consoles support for pv and hvm guests; multiple pv
> -console backends (qemu and xenconsoled) and emulated serial cards too.
> +console backends (qemu and xenconsoled, see limitations below) and
> +emulated serial cards too.
>  
>  This document tries to describe how the whole system works and how the
> -different components interact with each others.
> +different components interact with each other.
>  
>  The first PV console path in xenstore remains:
>  
> @@ -23,28 +24,47 @@ live in:
>  
>  /local/domain/$DOMID/device/console/$DEVID.
>  
> -The output of a PV console, whether it should be a file, a pty, a
> -socket, or something else, is specified by the toolstack in the xenstore
> -node "output", under the relevant console section.
> -For example:
> +PV consoles have
> +* (optional) string names;
> +* 'connection' information describing to what they should be
> +  connected; and
> +* a 'type' indicating which daemon should process the data.
> +
> +We call a PV console with a name a "channel", in reference to the libvirt
> +concept with the same name and purpose. The file "channels.txt" describes
> +how to use channels and includes a registry of well-known channel names.
> +
> +The toolstack writes 'connection' information in the xenstore backend in
> +nodes: "connection", "path" and "output". The nodes "connection" and "path"
> +express the high-level intention, while the "output" node is used for
> +internal signalling between the toolstack and the implementation daemon.

"connection" and "path" are both new, is that correct? Are they in any
way "channel", as opposed to serial, specific?

They correspond (roughly) to "method" and "method-specific-arguments" is
that right?

> +If the toolstack wants the console to be connected to a pty, it will write
> +to the backend:
> +
> +connection = pty
> +output = pty

Could we omit output in this case, since pty has no further parameters?

>  
> -# xenstore-read /local/domain/26/device/console/1/output
> -pty
> +The backend will write the pty device name to the "tty" node in the
> +console frontend.
>  
> -The backend chosen for a particular console is specified by the
> -toolstack in the "type" node on xenstore, under the relevant console
> -section.
> +If the toolstack wants a listening Unix domain socket to be created at path
> +<path>, a connection accepted and data proxied to the console, it will write:
> +
> +connection = socket
> +path = <path>
> +output = chardev:<some internal identifier>

Internal to whom? The toolstack? (this looks suspiciously like something
specific to the qemu backend)

I don't really understand the need for all three of these things,
especially how path and output are distinct. I have a feeling output may
be badly named and confusing me.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 3526539..769e6cf 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3299,6 +3299,19 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>      flexarray_append(back, "protocol");
>      flexarray_append(back, LIBXL_XENCONSOLE_PROTOCOL);
>  
> +    if (console->name) {

It might be worth sanity checking that a supposedly primary console
doesn't have any properties which cannot be used with it?

> +        flexarray_append(ro_front, "name");
> +        flexarray_append(ro_front, console->name);
> +    }
> +    if (console->connection) {
> +        flexarray_append(back, "connection");
> +        flexarray_append(back, console->connection);
> +    }
> +    if (console->path) {
> +        flexarray_append(back, "path");
> +        flexarray_append(back, console->path);
> +    }
> +
>      flexarray_append(front, "backend-id");
>      flexarray_append(front, libxl__sprintf(gc, "%d", console->backend_domid));
>  
> @@ -3337,6 +3349,219 @@ out:
>  
>  /******************************************************************************/
>  
> +int libxl__init_console_from_channel(libxl__gc *gc,
> +                                     libxl__device_console *console,
> +                                     int dev_num,
> +                                     libxl_device_channel *channel)

Can channel be const?

> +{
> +    int rc;
> +    libxl__device_console_init(console);
> +    console->devid = dev_num;
> +    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> +    if (!channel->name){
> +        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> +                   "channel %d has no name", channel->devid);
> +        return ERROR_INVAL;
> +    }
> +    console->name = libxl__strdup(NOGC, channel->name);
> +
> +    if (channel->backend_domname) {
> +        rc = libxl_domain_qualifier_to_domid(CTX, channel->backend_domname,
> +                                             &channel->backend_domid);
> +        if (rc < 0) return rc;
> +    }
> +
> +    console->backend_domid = channel->backend_domid;
> +
> +    /* The xenstore 'output' node tells the backend what to connect the console
> +       to. If the channel has "connection = pty" then the "output" node will be
> +       set to "pty". If the channel has "connection = socket" then the "output"
> +       node will be set to "chardev:libxl-channel%d". This tells the qemu
> +       backend to proxy data between the console ring and the character device
> +       with id "libxl-channel%d". These character devices are currently defined
> +       on the qemu command-line via "-chardev" options in libxl_dm.c */
> +
> +    switch (channel->connection) {
> +        case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
> +            LIBXL__LOG(CTX, LIBXL__LOG_ERROR,

The LOG* macro family is useful to reduce the length of these lines.

> +                       "channel %d has no defined connection; "
> +                       "to where should it be connected?", channel->devid);
> +            return ERROR_INVAL;
> +        case LIBXL_CHANNEL_CONNECTION_PTY:
> +            console->connection = libxl__strdup(NOGC, "pty");
> +            console->output = libxl__sprintf(NOGC, "pty");
> +            break;
> +        case LIBXL_CHANNEL_CONNECTION_SOCKET:
> +            console->connection = libxl__strdup(NOGC, "socket");
> +            if (!channel->u.socket.path) {
> +                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> +                           "channel %d has no path", channel->devid);
> +                return ERROR_INVAL;
> +            }
> +            console->path = libxl__strdup(NOGC, channel->u.socket.path);
> +            console->output = libxl__sprintf(NOGC, "chardev:libxl-channel%d",
> +                                             channel->devid);
> +            break;
> +        default:
> +            /* We've forgotten to add the clause */
> +            LOG(ERROR, "%s: missing implementation for channel connection %d",
> +                __func__, channel->connection);
> +            return ERROR_INVAL;

Just a plain abort() is fine here.

> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__device_channel_from_xs_be(libxl__gc *gc,
> +                                            const char *be_path,
> +                                            libxl_device_channel *channel)
> +{
> +    const char *tmp;
> +    int rc;
> +
> +    libxl_device_channel_init(channel);
> +
> +    /* READ_BACKEND is from libxl__device_nic_from_xs_be above */

Please refactor it to the top of the file or something.

> +    channel->name = READ_BACKEND(NOGC, "name");
> +    tmp = READ_BACKEND(gc, "connection");
> +    if (!strcmp(tmp, "pty")) {
> +        channel->connection = LIBXL_CHANNEL_CONNECTION_PTY;
> +    } else if (!strcmp(tmp, "socket")) {
> +        channel->connection = LIBXL_CHANNEL_CONNECTION_SOCKET;
> +        channel->u.socket.path = READ_BACKEND(NOGC, "path");
> +    } else {
> +	rc = ERROR_INVAL;

Stray tab.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index a412f9c..fcaf32f 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl

The new interface looks good to me.

Ian.

Re: [PATCH v4 2/3] libxl: add support for 'channels'

[Dave Scott]

Hi Ian,

On 10 Sep 2014, at 15:41, Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Tue, 2014-07-22 at 16:05 +0100, David Scott wrote:
>> diff --git a/docs/misc/console.txt b/docs/misc/console.txt
>> index 8a53a95..eb08aff 100644
>> --- a/docs/misc/console.txt
>> +++ b/docs/misc/console.txt
>> @@ -9,10 +9,11 @@ relevant information in xenstore under /local/domain/$DOMID/console.
>> 
>> Now many years after the introduction of the pv console we have
>> multiple pv consoles support for pv and hvm guests; multiple pv
>> -console backends (qemu and xenconsoled) and emulated serial cards too.
>> +console backends (qemu and xenconsoled, see limitations below) and
>> +emulated serial cards too.
>> 
>> This document tries to describe how the whole system works and how the
>> -different components interact with each others.
>> +different components interact with each other.
>> 
>> The first PV console path in xenstore remains:
>> 
>> @@ -23,28 +24,47 @@ live in:
>> 
>> /local/domain/$DOMID/device/console/$DEVID.
>> 
>> -The output of a PV console, whether it should be a file, a pty, a
>> -socket, or something else, is specified by the toolstack in the xenstore
>> -node "output", under the relevant console section.
>> -For example:
>> +PV consoles have
>> +* (optional) string names;
>> +* 'connection' information describing to what they should be
>> +  connected; and
>> +* a 'type' indicating which daemon should process the data.
>> +
>> +We call a PV console with a name a "channel", in reference to the libvirt
>> +concept with the same name and purpose. The file "channels.txt" describes
>> +how to use channels and includes a registry of well-known channel names.
>> +
>> +The toolstack writes 'connection' information in the xenstore backend in
>> +nodes: "connection", "path" and "output". The nodes "connection" and "path"
>> +express the high-level intention, while the "output" node is used for
>> +internal signalling between the toolstack and the implementation daemon.
> 
> "connection" and "path" are both new, is that correct? Are they in any
> way "channel", as opposed to serial, specific?
> 
> They correspond (roughly) to "method" and "method-specific-arguments" is
> that right?

“connection” and “path” are indeed new, and yeah, they do correspond roughly to “method” and “method-specific-arguments”.

The summary is:

  - “connection” and “path” are the interface: a backend running libxl/xld (or something else) could read these keys and create properly configured backends
  - “output” is a pre-existing key used by libxl to signal qemu i.e. it’s part of the secondary console implementation at the moment. If we enhanced xenconsoled to support secondary consoles then we could drop this.

Initially I thought we could rely only on the pre-existing “output” key, which is fine if “connection=output=pty” but in the case of a unix domain socket you end up with:

  connection = socket
  path = /tmp/sock
  output = chardev:libxl-1

where ‘chardev:libxl-1’ is an identifier that only appears on the qemu commandline i.e. it’s not enough by itself for a driver domain implementation to use. A driver domain implementation would read ‘connection’, ‘path’, and synthesise the ‘output’ key and qemu command-line itself.

Since the doc describes itself as a design doc, I thought I should include some of the gory detail. Perhaps I need to emphasise which bits are purely gory implementation details.

>> +If the toolstack wants the console to be connected to a pty, it will write
>> +to the backend:
>> +
>> +connection = pty
>> +output = pty
> 
> Could we omit output in this case, since pty has no further parameters?

Alas qemu needs ‘output = pty’. We could omit ‘connection = pty’ if we wanted, although I thought it would be simpler to always write the configuration data without cooking it too much.

> 
>> 
>> -# xenstore-read /local/domain/26/device/console/1/output
>> -pty
>> +The backend will write the pty device name to the "tty" node in the
>> +console frontend.
>> 
>> -The backend chosen for a particular console is specified by the
>> -toolstack in the "type" node on xenstore, under the relevant console
>> -section.
>> +If the toolstack wants a listening Unix domain socket to be created at path
>> +<path>, a connection accepted and data proxied to the console, it will write:
>> +
>> +connection = socket
>> +path = <path>
>> +output = chardev:<some internal identifier>
> 
> Internal to whom? The toolstack? (this looks suspiciously like something
> specific to the qemu backend)
> 
> I don't really understand the need for all three of these things,
> especially how path and output are distinct. I have a feeling output may
> be badly named and confusing me.

Perhaps if I labelled the ‘output’ key as a qemu implementation artefact which should be ignored we could claim there are only 2 keys :-)

If we enhanced xenconsoled to support secondary consoles then we could favour it over qemu and omit the ‘output’ key in those cases.

Thanks for the other comments — I’ll start investigating them.

Cheers,
Dave

> 
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 3526539..769e6cf 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -3299,6 +3299,19 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
>>     flexarray_append(back, "protocol");
>>     flexarray_append(back, LIBXL_XENCONSOLE_PROTOCOL);
>> 
>> +    if (console->name) {
> 
> It might be worth sanity checking that a supposedly primary console
> doesn't have any properties which cannot be used with it?
> 
>> +        flexarray_append(ro_front, "name");
>> +        flexarray_append(ro_front, console->name);
>> +    }
>> +    if (console->connection) {
>> +        flexarray_append(back, "connection");
>> +        flexarray_append(back, console->connection);
>> +    }
>> +    if (console->path) {
>> +        flexarray_append(back, "path");
>> +        flexarray_append(back, console->path);
>> +    }
>> +
>>     flexarray_append(front, "backend-id");
>>     flexarray_append(front, libxl__sprintf(gc, "%d", console->backend_domid));
>> 
>> @@ -3337,6 +3349,219 @@ out:
>> 
>> /******************************************************************************/
>> 
>> +int libxl__init_console_from_channel(libxl__gc *gc,
>> +                                     libxl__device_console *console,
>> +                                     int dev_num,
>> +                                     libxl_device_channel *channel)
> 
> Can channel be const?
> 
>> +{
>> +    int rc;
>> +    libxl__device_console_init(console);
>> +    console->devid = dev_num;
>> +    console->consback = LIBXL__CONSOLE_BACKEND_IOEMU;
>> +    if (!channel->name){
>> +        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
>> +                   "channel %d has no name", channel->devid);
>> +        return ERROR_INVAL;
>> +    }
>> +    console->name = libxl__strdup(NOGC, channel->name);
>> +
>> +    if (channel->backend_domname) {
>> +        rc = libxl_domain_qualifier_to_domid(CTX, channel->backend_domname,
>> +                                             &channel->backend_domid);
>> +        if (rc < 0) return rc;
>> +    }
>> +
>> +    console->backend_domid = channel->backend_domid;
>> +
>> +    /* The xenstore 'output' node tells the backend what to connect the console
>> +       to. If the channel has "connection = pty" then the "output" node will be
>> +       set to "pty". If the channel has "connection = socket" then the "output"
>> +       node will be set to "chardev:libxl-channel%d". This tells the qemu
>> +       backend to proxy data between the console ring and the character device
>> +       with id "libxl-channel%d". These character devices are currently defined
>> +       on the qemu command-line via "-chardev" options in libxl_dm.c */
>> +
>> +    switch (channel->connection) {
>> +        case LIBXL_CHANNEL_CONNECTION_UNKNOWN:
>> +            LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> 
> The LOG* macro family is useful to reduce the length of these lines.
> 
>> +                       "channel %d has no defined connection; "
>> +                       "to where should it be connected?", channel->devid);
>> +            return ERROR_INVAL;
>> +        case LIBXL_CHANNEL_CONNECTION_PTY:
>> +            console->connection = libxl__strdup(NOGC, "pty");
>> +            console->output = libxl__sprintf(NOGC, "pty");
>> +            break;
>> +        case LIBXL_CHANNEL_CONNECTION_SOCKET:
>> +            console->connection = libxl__strdup(NOGC, "socket");
>> +            if (!channel->u.socket.path) {
>> +                LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
>> +                           "channel %d has no path", channel->devid);
>> +                return ERROR_INVAL;
>> +            }
>> +            console->path = libxl__strdup(NOGC, channel->u.socket.path);
>> +            console->output = libxl__sprintf(NOGC, "chardev:libxl-channel%d",
>> +                                             channel->devid);
>> +            break;
>> +        default:
>> +            /* We've forgotten to add the clause */
>> +            LOG(ERROR, "%s: missing implementation for channel connection %d",
>> +                __func__, channel->connection);
>> +            return ERROR_INVAL;
> 
> Just a plain abort() is fine here.
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int libxl__device_channel_from_xs_be(libxl__gc *gc,
>> +                                            const char *be_path,
>> +                                            libxl_device_channel *channel)
>> +{
>> +    const char *tmp;
>> +    int rc;
>> +
>> +    libxl_device_channel_init(channel);
>> +
>> +    /* READ_BACKEND is from libxl__device_nic_from_xs_be above */
> 
> Please refactor it to the top of the file or something.
> 
>> +    channel->name = READ_BACKEND(NOGC, "name");
>> +    tmp = READ_BACKEND(gc, "connection");
>> +    if (!strcmp(tmp, "pty")) {
>> +        channel->connection = LIBXL_CHANNEL_CONNECTION_PTY;
>> +    } else if (!strcmp(tmp, "socket")) {
>> +        channel->connection = LIBXL_CHANNEL_CONNECTION_SOCKET;
>> +        channel->u.socket.path = READ_BACKEND(NOGC, "path");
>> +    } else {
>> +	rc = ERROR_INVAL;
> 
> Stray tab.
> 
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index a412f9c..fcaf32f 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
> 
> The new interface looks good to me.
> 
> Ian.
>