[PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[George Dunlap]

This patch exposes a generic interface which can be expanded in the
future to implement USB for PVUSB, qemu, and stubdoms.  It can also be
extended to include other types of USB other than host USB (for example,
tablets, mice, or keyboards).

For each device removed or added, one of two protocols is available:
* PVUSB
* qemu (DEVICEMODEL)

The caller can additionally specify "AUTO", in which case the library will
try to determine the best protocol automatically.

At the moment, the only protocol implemented is DEVICEMODEL, and the only
device type impelmented is HOSTDEV.

This uses the qmp functionality, and is thus only available for
qemu-xen, not qemu-traditional.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
History:
v7:
 - Correct spelling mistake
 - Add libxl_device_usb_destroy
 - Add backend_domname to device_struct
 - Don't have a default backend
 - Implement libxl__device_usb_setdefault
 - Get rid of "internal" usbdev struct
 - Fix author of libxl_usb.c
 - Use LOG() consistently
 - Don't unnecessarily cast hostbus/hostaddr to uint16 in the USB xenstore path
 - Create libxl__domain_usb_init function to put all usb-related xenstore stuff
   in one file
 - Call libxl__xs_transaction_abort on the failure path of usb_add_xenstore
 - Do element-by-element compare rather than memcmp
 - Use QMP_PARAMETERS_SPRINTF for id in libxl__qmp_usb_hostdev_{add,remove}
   rather than a separate GSPRINTF
 - Fix memory leak in failure path of get_assigned_devices()
 - Handle non-NULL empty list returned by libxl__xs_directory in get_assigned_devices
 - Don't garbage-collect the list when called from libxl_device_usb_list()
v6:
 - Return a proper error code if libxl__xs_mkdir fails
 - Make casts cuddly
 - Add a space after switch statements
v5:
 - fix erroneous use of NOGC in qmp functions
 - Don't check return of libxl__sprintf in libxl_create.c
 - Check return values of new xs actions in libxl_create.c
 - Use updated template for xenstore transactions, do checks
 - use xs_read_checked
 - Fixed if (x) spacing to match coding style
 - use GCSFPRINTF
 - use libxl__calloc
 - Fix comment for LIBXL_HAVE_USB
 - use idl-generated *_from_string() and *_to_string() functions

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: sstanisi@cbnco.com
CC: Fabio Fantoni <fabio.fantoni@m2r.biz>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
CC: Simon (Bo) Cao <caobosimon@gmail.com>
---
 tools/libxl/Makefile           |    2 +-
 tools/libxl/libxl.c            |    3 +-
 tools/libxl/libxl.h            |   40 +++
 tools/libxl/libxl_create.c     |    3 +
 tools/libxl/libxl_internal.h   |   11 +
 tools/libxl/libxl_qmp.c        |   56 +++++
 tools/libxl/libxl_types.idl    |   22 ++
 tools/libxl/libxl_usb.c        |  521 ++++++++++++++++++++++++++++++++++++++++
 tools/ocaml/libs/xl/genwrap.py |    1 +
 9 files changed, 656 insertions(+), 3 deletions(-)
 create mode 100644 tools/libxl/libxl_usb.c

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 4cfa275..a50e6cf 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -76,7 +76,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
 			libxl_internal.o libxl_utils.o libxl_uuid.o \
 			libxl_json.o libxl_aoutils.o libxl_numa.o \
 			libxl_save_callout.o _libxl_save_msgs_callout.o \
-			libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
+			libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.o $(LIBXL_OBJS-y)
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
 
 LIBXL_TESTS += timedereg
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4ea7abb..7ef5e25 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1767,8 +1767,7 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
     return nextid;
 }
 
-static int libxl__resolve_domid(libxl__gc *gc, const char *name,
-                                uint32_t *domid)
+int libxl__resolve_domid(libxl__gc *gc, const char *name, uint32_t *domid)
 {
     if (!name)
         return 0;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index c7aa817..963e650 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -82,6 +82,12 @@
 #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
 
 /*
+ * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
+ * USB devices.
+ */
+#define LIBXL_HAVE_DEVICE_USB 1
+
+/*
  * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
  * libxl_vendor_device field is present in the hvm sections of
  * libxl_domain_build_info. This field tells libxl which
@@ -924,6 +930,40 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
                        const libxl_asyncop_how *ao_how)
                        LIBXL_EXTERNAL_CALLERS_ONLY;
 
+/*
+ * USB
+ * 
+ * For each device removed or added, one of these protocols is available:
+ * - PV (i.e., PVUSB)
+ * - DEVICEMODEL (i.e, qemu)
+ *
+ * PV is available for either PV or HVM domains.  DEVICEMODEL is only
+ * available for HVM domains.  The caller can additionally specify
+ * "AUTO", in which case the library will try to determine the best
+ * protocol automatically.
+ *
+ * At the moment, the only protocol implemented is DEVICEMODEL, and the only
+ * device type implemented is HOSTDEV.
+ *
+ * This uses the qmp functionality, and is thus only available for
+ * qemu-xen, not qemu-traditional.
+ */
+int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
+                         libxl_device_usb *dev,
+                         const libxl_asyncop_how *ao_how)
+                         LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, 
+                            libxl_device_usb *dev,
+                            const libxl_asyncop_how *ao_how)
+                            LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_device_usb_destroy(libxl_ctx *ctx, uint32_t domid, 
+                             libxl_device_usb *dev,
+                             const libxl_asyncop_how *ao_how)
+                             LIBXL_EXTERNAL_CALLERS_ONLY;
+libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, 
+                                        int *num)
+                          LIBXL_EXTERNAL_CALLERS_ONLY;
+
 /* Network Interfaces */
 int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic,
                          const libxl_asyncop_how *ao_how)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d015cf4..80a75a4 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -537,6 +537,9 @@ retry_transaction:
     xs_rm(ctx->xsh, t, libxl_path);
     libxl__xs_mkdir(gc, t, libxl_path, noperm, ARRAY_SIZE(noperm));
 
+    rc = libxl__domain_usb_init(gc, t, libxl_path, noperm, ARRAY_SIZE(noperm));
+    if (rc) goto out;
+
     xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, strlen(vm_path));
     rc = libxl__domain_rename(gc, *domid, 0, info->name, t);
     if (rc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c2b73c4..bf394e4 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1031,6 +1031,11 @@ _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__resolve_domid(libxl__gc *gc, const char *name, 
+                                 uint32_t *domid);
+_hidden int libxl__domain_usb_init(libxl__gc *gc, xs_transaction_t t,
+                                   char *libxl_path, struct xs_permissions *perm,
+                                   int perm_size);
 
 /*
  * For each aggregate type which can be used as an input we provide:
@@ -1056,6 +1061,8 @@ _hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm
 _hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb);
 _hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
 _hidden int libxl__device_pci_setdefault(libxl__gc *gc, libxl_device_pci *pci);
+_hidden int libxl__device_usb_setdefault(libxl__gc *gc, libxl_device_usb *usb,
+                                         uint32_t domid);
 
 _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
                                               uint32_t domid,
@@ -1585,6 +1592,10 @@ _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enabl
 _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk);
 /* Add a virtual CPU */
 _hidden int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int index);
+_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid,
+                               libxl_device_usb *dev);
+_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid,
+                                  libxl_device_usb *dev);
 /* close and free the QMP handler */
 _hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
 /* remove the socket file, if the file has already been removed,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 8433e42..56ae368 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -42,6 +42,7 @@
 
 #define QMP_RECEIVE_BUFFER_SIZE 4096
 #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
+#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x"
 
 typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
                               const libxl__json_object *tree,
@@ -939,6 +940,61 @@ int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int idx)
     return qmp_run_command(gc, domid, "cpu-add", args, NULL, NULL);
 }
 
+static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid,
+                                      libxl_device_usb *usb)
+{
+    libxl__json_object *args = NULL;
+
+    qmp_parameters_add_string(gc, &args, "driver", "usb-host");
+    QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", usb->u.hostdev.hostbus);
+    QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", usb->u.hostdev.hostaddr);
+    QMP_PARAMETERS_SPRINTF(&args, "id", HOST_USB_QDEV_ID,
+                           usb->u.hostdev.hostbus, usb->u.hostdev.hostaddr);
+
+    return qmp_run_command(gc, domid, "device_add", args, NULL, NULL);
+}
+
+int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl_device_usb *usb)
+{
+    int rc;
+    switch (usb->type) {
+    case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+        rc = libxl__qmp_usb_hostdev_add(gc, domid, usb);
+        break;
+    default:
+        return ERROR_INVAL;
+    }
+    return rc;
+}
+
+
+static int libxl__qmp_usb_hostdev_remove(libxl__gc *gc, int domid,
+                                         libxl_device_usb *usb)
+{
+    libxl__json_object *args = NULL;
+
+    QMP_PARAMETERS_SPRINTF(&args, "id", HOST_USB_QDEV_ID,
+                           usb->u.hostdev.hostbus, usb->u.hostdev.hostaddr);
+
+    return qmp_run_command(gc, domid, "device_del", args, NULL, NULL);
+}
+
+int libxl__qmp_usb_remove(libxl__gc *gc, int domid,
+                          libxl_device_usb *usb)
+{
+    int rc;
+    switch (usb->type) {
+    case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+        rc = libxl__qmp_usb_hostdev_remove(gc, domid, usb);
+        break;
+    default:
+        return ERROR_INVAL;
+    }
+    return rc;
+}
+
+
+
 int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
                                const libxl_domain_config *guest_config)
 {
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 52f1aa9..ae94ad6 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -457,6 +457,28 @@ libxl_device_vtpm = Struct("device_vtpm", [
     ("uuid",             libxl_uuid),
 ])
 
+libxl_device_usb_protocol = Enumeration("usb_protocol", [
+    (0, "AUTO"),
+    (1, "PV"),
+    (2, "DEVICEMODEL"),
+    ])
+
+libxl_device_usb_type = Enumeration("device_usb_type", [
+    (1, "HOSTDEV"),
+    ])
+
+libxl_device_usb = Struct("device_usb", [
+        ("protocol", libxl_device_usb_protocol,
+         {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}),
+        ("backend_domid",    libxl_domid),
+        ("backend_domname",  string),
+        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
+                         [("hostdev", Struct(None, [
+                                ("hostbus",   integer),
+                                ("hostaddr",  integer) ]))
+                          ]))
+    ])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
new file mode 100644
index 0000000..03b6ea1
--- /dev/null
+++ b/tools/libxl/libxl_usb.c
@@ -0,0 +1,521 @@
+/*
+ * Copyright (C) 2009      Citrix Ltd.
+ * Author George Dunlap <george.dunlap@eu.citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+/*
+ * /libxl/<domid>/usb/<devid>/{type, protocol, backend, [typeinfo]}
+ */
+#define USB_INFO_PATH "%s/usb"
+#define USB_HOSTDEV_ID "hostdev-%04x-%04x"
+
+/*
+ * Just use plain numbers for now.  Replace these with strings at some point.
+ */
+static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid,
+                                    libxl_device_usb *usb)
+{
+    /* FIXME: Assert usb->type here? */
+    return GCSPRINTF(USB_INFO_PATH "/%s",
+                     libxl__xs_libxl_path(gc, domid),
+                     GCSPRINTF(USB_HOSTDEV_ID,
+                               usb->u.hostdev.hostbus,
+                               usb->u.hostdev.hostaddr));
+}
+
+static void hostdev_xs_append_params(libxl__gc *gc, libxl_device_usb *usb,
+                                  flexarray_t *a)
+{
+    /* FIXME: Assert usb->type here? */
+    flexarray_append_pair(a, "hostbus",
+                          GCSPRINTF("%d",
+                                    usb->u.hostdev.hostbus));
+    flexarray_append_pair(a, "hostaddr",
+                          GCSPRINTF("%d",
+                                    usb->u.hostdev.hostaddr));
+}
+
+static int read_hostdev_xenstore_entry(libxl__gc *gc, const char * path,
+                                       libxl_device_usb *usb)
+{
+    int rc;
+    const char * val;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                GCSPRINTF("%s/hostbus", path), &val);
+    if (rc) goto out;
+    usb->u.hostdev.hostbus = atoi(val);
+    
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                 GCSPRINTF("%s/hostaddr", path), &val);
+    if (rc) goto out;
+    usb->u.hostdev.hostaddr = atoi(val);
+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int usb_read_xenstore(libxl__gc *gc, const char * path,
+                                           libxl_device_usb *usb)
+{
+    const char *val;
+    int rc;
+
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                GCSPRINTF("%s/protocol", path), &val);
+    if (rc) goto out;
+    rc = libxl_usb_protocol_from_string(val, &usb->protocol);
+    if (rc) goto out;
+        
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                GCSPRINTF("%s/backend_domid", path), &val);
+    if (rc) goto out;
+    usb->backend_domid = atoi(val);
+    
+    rc = libxl__xs_read_checked(gc, XBT_NULL,
+                                GCSPRINTF("%s/type", path), &val);
+    if (rc) goto out;
+    rc = libxl_device_usb_type_from_string(val, &usb->type);
+    if (rc) goto out;
+
+    switch (usb->type) {
+    case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+        rc = read_hostdev_xenstore_entry(gc, path, usb);
+        if (rc) goto out;
+        break;
+    default:
+        LOG(ERROR, "Internal error (unimplemented type)");
+        goto out;
+    }
+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int usb_add_xenstore(libxl__gc *gc, uint32_t domid,
+                            libxl_device_usb *usb)
+{
+    flexarray_t *dev;
+    char *dev_path;
+    xs_transaction_t t = 0;
+    struct xs_permissions noperm[1];
+    int rc = 0;
+
+    noperm[0].id = 0;
+    noperm[0].perms = XS_PERM_NONE;
+
+    dev = flexarray_make(gc, 16, 1);
+
+    flexarray_append_pair(dev, "protocol",
+                          GCSPRINTF("%s",
+                              libxl_usb_protocol_to_string(usb->protocol)));
+
+    flexarray_append_pair(dev, "backend_domid",
+                          GCSPRINTF("%d", usb->backend_domid));
+
+    flexarray_append_pair(dev, "type",
+                          GCSPRINTF("%s",
+                               libxl_device_usb_type_to_string(usb->type)));
+
+    switch (usb->type) {
+    case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+        dev_path = hostdev_xs_path(gc, domid, usb);
+        hostdev_xs_append_params(gc, usb, dev);
+        break;
+    default:
+        LOG(ERROR, "Invalid device type: %d", usb->type);
+        return ERROR_FAIL;
+    }
+
+    LOG(DEBUG, "Adding new usb device to xenstore");
+
+    for(;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+
+        /* Helpfully, mkdir returns 0 on failure, 1 on success */
+        if (!libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm))) {
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* And libxl__xs_writev *always* returns 0 no matter what */
+        libxl__xs_writev(gc, t, dev_path,
+                         libxl__xs_kvs_of_flexarray(gc, dev, dev->count));
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc <0) goto out;
+    }
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+    return rc;
+}
+
+static int usb_remove_xenstore(libxl__gc *gc, uint32_t domid,
+                               libxl_device_usb *usb)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    char *dev_path;
+
+    LOG(DEBUG, "Removing device from xenstore");
+
+    switch (usb->type) {
+    case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+        dev_path = hostdev_xs_path(gc, domid, usb);
+        break;
+    default:
+        LOG(ERROR, "Invalid device type: %d", usb->type);
+        return ERROR_FAIL;
+    }
+
+    xs_rm(ctx->xsh, XBT_NULL, dev_path);
+
+    return 0;
+}
+
+int libxl__domain_usb_init(libxl__gc *gc, xs_transaction_t t, char *libxl_path,
+                           struct xs_permissions *perm, int perm_size) {
+    int rc;
+    char * libxl_usb_path = GCSPRINTF(USB_INFO_PATH, libxl_path);
+
+    /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */
+    rc = libxl__xs_rm_checked(gc, t, libxl_usb_path);
+    if (rc) goto out;
+
+    /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */
+    if (!libxl__xs_mkdir(gc, t, libxl_usb_path, perm, perm_size)) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+out:
+    return rc;
+}
+
+static int _get_assigned_devices(libxl__gc *gc, libxl__gc *list_gc, uint32_t domid,
+                                libxl_device_usb **list, int *num)
+{
+    char **devlist, *dompath;
+    unsigned int nd = 0;
+    int i, rc = 0;
+
+    *list = NULL;
+    *num = 0;
+
+    dompath = GCSPRINTF(USB_INFO_PATH,
+                        libxl__xs_libxl_path(gc, domid));
+
+    devlist = libxl__xs_directory(gc, XBT_NULL, dompath, &nd);
+    if (!devlist || !nd)
+        goto out;
+
+    *list = libxl__calloc(list_gc, nd, sizeof(libxl_device_usb));
+
+    for(i = 0; i < nd; i++) {
+        char *path;
+
+        path = GCSPRINTF("%s/%s", dompath, devlist[i]);
+        
+        rc = usb_read_xenstore(gc, path, (*list) + i);
+        if (rc) {
+            free(*list);
+            *list = NULL;
+            *num = 0;
+            goto out;
+        }
+    }
+
+    *num = nd;
+
+out:
+    return rc;
+}
+
+/* GC or NOGC indicates whether the list of devices is garbage collected. */
+#define GET_ASSIGNED_DEVICES_GC(gc, domid, list, num) \
+            _get_assigned_devices(gc, gc, domid, list, num)
+#define GET_ASSIGNED_DEVICES_NOGC(gc, domid, list, num)\
+             _get_assigned_devices(gc, NOGC, domid, list, num)
+
+static int is_usbdev_type_hostdev_equal(libxl_device_usb *a,
+                                        libxl_device_usb *b)
+{
+    return a->u.hostdev.hostbus == b->u.hostdev.hostbus
+           && a->u.hostdev.hostaddr == b->u.hostdev.hostaddr;
+}
+
+static int is_usbdev_in_array(libxl_device_usb *assigned, int num_assigned,
+                              libxl_device_usb *dev)
+{
+    int i;
+
+    /* Devices are the same if:
+     * - They have the same backend_domid
+     * - They are of the same type
+     * - Their types match
+     * In theory, someone could try to pass the same device through
+     * via both PVUSB and qemu; not comparing protocol will prevent that.
+     */
+    for(i = 0; i < num_assigned; i++) {
+        if (assigned[i].type != dev->type
+            || assigned[i].backend_domid != dev->backend_domid)
+            continue;
+
+        switch (dev->type) {
+        case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+            if (!is_usbdev_type_hostdev_equal(dev, assigned+i))
+                continue;
+        }
+
+        return 1;
+    }
+
+    return 0;
+}
+
+/*
+ * USB add
+ */
+static int do_usb_add(libxl__gc *gc, uint32_t domid,
+                      libxl_device_usb *usb)
+{
+    int rc;
+
+    switch (usb->protocol) {
+    case LIBXL_USB_PROTOCOL_DEVICEMODEL:
+        switch (libxl__device_model_version_running(gc, domid)) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            LOG(ERROR, "usb-add not yet implemented for qemu-traditional");
+            return ERROR_INVAL;
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            rc = libxl__qmp_usb_add(gc, domid, usb);
+            if (rc)
+                goto out;
+            break;
+        default:
+            return ERROR_INVAL;
+        }
+        break;
+    default:
+        return ERROR_FAIL;
+    }
+
+    rc = usb_add_xenstore(gc, domid, usb);
+out:
+    return rc;
+}
+
+
+int libxl__device_usb_setdefault(libxl__gc *gc, libxl_device_usb *usb, uint32_t domid)
+{
+    int rc;
+    libxl_domain_type domtype = libxl__domain_type(gc, domid);
+
+    rc = libxl__resolve_domid(gc, usb->backend_domname, &usb->backend_domid);
+    if (rc < 0) return rc;
+
+    if (usb->protocol == LIBXL_USB_PROTOCOL_AUTO) {
+        if (domtype == LIBXL_DOMAIN_TYPE_PV) {
+            usb->protocol = LIBXL_USB_PROTOCOL_PV;
+        } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) {
+            /* FIXME: See if we can detect PV frontend */
+            usb->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL;
+        }
+    }
+
+    return rc;
+}
+
+static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid,
+                                 libxl_device_usb *usb)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    libxl_device_usb *assigned;
+    int rc = ERROR_FAIL, num_assigned;
+    libxl_domid dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+    rc = libxl__device_usb_setdefault(gc, usb, domid);
+    if (rc < 0) goto out;
+
+    /* Check to make sure we're doing something that's implemented */
+    if (usb->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) {
+        rc = ERROR_FAIL;
+        LOG(ERROR, "Protocol not implemented");
+        goto out;
+    }
+
+    if (dm_domid != 0) {
+        rc = ERROR_FAIL;
+        LOG(ERROR, "Stubdoms not yet supported");
+        goto out;
+    }
+
+    /* Double-check to make sure it's not already assigned */
+    rc = GET_ASSIGNED_DEVICES_GC(gc, domid, &assigned, &num_assigned);
+    if (rc) {
+        LOG(ERROR, "cannot determine if device is assigned,"
+            " refusing to continue");
+        goto out;
+    }
+    if (is_usbdev_in_array(assigned, num_assigned, usb)) {
+        LOG(ERROR, "USB device already attached to a domain");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Do the add */
+    if (do_usb_add(gc, domid, usb))
+        rc = ERROR_FAIL;
+
+out:
+    return rc;
+}
+
+int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
+                         libxl_device_usb *usb,
+                         const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, domid, ao_how);
+    int rc;
+    rc = libxl__device_usb_add(gc, domid, usb);
+    libxl__ao_complete(egc, ao, rc);
+    return AO_INPROGRESS;
+}
+
+/*
+ * USB remove
+ */
+static int do_usb_remove(libxl__gc *gc, uint32_t domid,
+                         libxl_device_usb *usb)
+{
+    int rc;
+
+    switch (usb->protocol) {
+    case LIBXL_USB_PROTOCOL_DEVICEMODEL:
+        switch (libxl__device_model_version_running(gc, domid)) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            LOG(ERROR, "usb-remove not yet implemented for qemu-traditional");
+            return ERROR_INVAL;
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            rc = libxl__qmp_usb_remove(gc, domid, usb);
+            if (rc)
+                goto out;
+            break;
+        default:
+            return ERROR_INVAL;
+        }
+        break;
+    default:
+        return ERROR_FAIL;
+    }
+
+    rc = usb_remove_xenstore(gc, domid, usb);
+out:
+    return rc;
+}
+static int libxl__device_usb_remove_common(libxl__gc *gc, uint32_t domid,
+                                           libxl_device_usb *usb)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    libxl_device_usb *assigned;
+    int rc = ERROR_FAIL, num_assigned;
+    libxl_domid dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+    rc = libxl__device_usb_setdefault(gc, usb, domid);
+    if (rc < 0) goto out;
+
+    /* Check to make sure we're doing something that's impemented */
+    if (usb->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) {
+        rc = ERROR_FAIL;
+        LOG(ERROR, "Protocol not implemented");
+        goto out;
+    }
+
+    if (dm_domid != 0) {
+        rc = ERROR_FAIL;
+        LOG(ERROR, "Stubdoms not yet supported");
+        goto out;
+    }
+
+    /* Double-check to make sure it's not already assigned */
+    rc = GET_ASSIGNED_DEVICES_GC(gc, domid, &assigned, &num_assigned);
+    if (rc) {
+        LOG(ERROR, "cannot determine if device is assigned,"
+            " refusing to continue");
+        goto out;
+    }
+    if (!is_usbdev_in_array(assigned, num_assigned, usb)) {
+        LOG(ERROR, "USB device doesn't seem to be attached to the domain");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /* Do the remove */
+    if (do_usb_remove(gc, domid, usb))
+        rc = ERROR_FAIL;
+
+out:
+    return rc;
+}
+
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
+                            libxl_device_usb *usb,
+                            const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, domid, ao_how);
+    int rc;
+
+    rc = libxl__device_usb_remove_common(gc, domid, usb);
+
+    libxl__ao_complete(egc, ao, rc);
+    return AO_INPROGRESS;
+}
+
+int libxl_device_usb_destroy(libxl_ctx *ctx, uint32_t domid,
+                             libxl_device_usb *usb,
+                             const libxl_asyncop_how *ao_how)
+{
+    AO_CREATE(ctx, domid, ao_how);
+    int rc;
+
+    rc = libxl__device_usb_remove_common(gc, domid, usb);
+
+    libxl__ao_complete(egc, ao, rc);
+    return AO_INPROGRESS;
+}
+
+libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx,
+                                        uint32_t domid, int *num)
+{
+    GC_INIT(ctx);
+    libxl_device_usb *usb = NULL;
+    int rc;
+
+    rc = GET_ASSIGNED_DEVICES_NOGC(gc, domid, &usb, num);
+    if (rc) {
+        LOG(ERROR, "Could not get assigned devices");
+        goto out;
+    }
+
+out:
+    GC_FREE;
+    return usb;
+}
diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
index 5e43831..09f7370 100644
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -501,6 +501,7 @@ if __name__ == '__main__':
     blacklist = [
         "cpupoolinfo",
         "vcpuinfo",
+        "device_usb"
         ]
 
     for t in blacklist:
-- 
1.7.9.5

Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Ian Campbell]

On Mon, 2014-06-02 at 14:44 +0100, George Dunlap wrote:
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index c7aa817..963e650 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -82,6 +82,12 @@
>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>  
>  /*
> + * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
> + * USB devices.
> + */
> +#define LIBXL_HAVE_DEVICE_USB 1
> +
> +/*
>   * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
>   * libxl_vendor_device field is present in the hvm sections of
>   * libxl_domain_build_info. This field tells libxl which
> @@ -924,6 +930,40 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
>                         const libxl_asyncop_how *ao_how)
>                         LIBXL_EXTERNAL_CALLERS_ONLY;
>  
> +/*
> + * USB
> + * 
> + * For each device removed or added, one of these protocols is available:
> + * - PV (i.e., PVUSB)
> + * - DEVICEMODEL (i.e, qemu)
> + *
> + * PV is available for either PV or HVM domains.  DEVICEMODEL is only
> + * available for HVM domains.  The caller can additionally specify
> + * "AUTO", in which case the library will try to determine the best
> + * protocol automatically.
> + *
> + * At the moment, the only protocol implemented is DEVICEMODEL, and the only
> + * device type implemented is HOSTDEV.
> + *
> + * This uses the qmp functionality, and is thus only available for
> + * qemu-xen, not qemu-traditional.
> + */
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usb *dev,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, 
> +                            libxl_device_usb *dev,
> +                            const libxl_asyncop_how *ao_how)
> +                            LIBXL_EXTERNAL_CALLERS_ONLY;
> +int libxl_device_usb_destroy(libxl_ctx *ctx, uint32_t domid, 
> +                             libxl_device_usb *dev,
> +                             const libxl_asyncop_how *ao_how)
> +                             LIBXL_EXTERNAL_CALLERS_ONLY;
> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid, 
> +                                        int *num)
> +                          LIBXL_EXTERNAL_CALLERS_ONLY;

No _getinfo? (Might only make sense with the PV stuff I guess)

> +

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 52f1aa9..ae94ad6 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -457,6 +457,28 @@ libxl_device_vtpm = Struct("device_vtpm", [
>      ("uuid",             libxl_uuid),
>  ])
>  
> +libxl_device_usb_protocol = Enumeration("usb_protocol", [
> +    (0, "AUTO"),
> +    (1, "PV"),
> +    (2, "DEVICEMODEL"),
> +    ])
> +
> +libxl_device_usb_type = Enumeration("device_usb_type", [
> +    (1, "HOSTDEV"),
> +    ])
> +
> +libxl_device_usb = Struct("device_usb", [
> +        ("protocol", libxl_device_usb_protocol,
> +         {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}),

FYI you can omit the init_val here since it is inherent in the
libxl_device_usb_protocol as you've defined it.

(if the default were non-zero you'd want to specify it, in general we
don't)

> +        ("backend_domid",    libxl_domid),
> +        ("backend_domname",  string),
> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
> +                         [("hostdev", Struct(None, [
> +                                ("hostbus",   integer),
> +                                ("hostaddr",  integer) ]))

No need to express the host topology I think (because you can build that
from the bus,addr tuples)?

> +                          ]))
> +    ])
> +
>  libxl_domain_config = Struct("domain_config", [
>      ("c_info", libxl_domain_create_info),
>      ("b_info", libxl_domain_build_info),

Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[George Dunlap]

On 06/18/2014 02:08 PM, Ian Campbell wrote:
> On Mon, 2014-06-02 at 14:44 +0100, George Dunlap wrote:
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index c7aa817..963e650 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -82,6 +82,12 @@
>>   #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>>   
>>   /*
>> + * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
>> + * USB devices.
>> + */
>> +#define LIBXL_HAVE_DEVICE_USB 1
>> +
>> +/*
>>    * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
>>    * libxl_vendor_device field is present in the hvm sections of
>>    * libxl_domain_build_info. This field tells libxl which
>> @@ -924,6 +930,40 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
>>                          const libxl_asyncop_how *ao_how)
>>                          LIBXL_EXTERNAL_CALLERS_ONLY;
>>   
>> +/*
>> + * USB
>> + *
>> + * For each device removed or added, one of these protocols is available:
>> + * - PV (i.e., PVUSB)
>> + * - DEVICEMODEL (i.e, qemu)
>> + *
>> + * PV is available for either PV or HVM domains.  DEVICEMODEL is only
>> + * available for HVM domains.  The caller can additionally specify
>> + * "AUTO", in which case the library will try to determine the best
>> + * protocol automatically.
>> + *
>> + * At the moment, the only protocol implemented is DEVICEMODEL, and the only
>> + * device type implemented is HOSTDEV.
>> + *
>> + * This uses the qmp functionality, and is thus only available for
>> + * qemu-xen, not qemu-traditional.
>> + */
>> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
>> +                         libxl_device_usb *dev,
>> +                         const libxl_asyncop_how *ao_how)
>> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
>> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
>> +                            libxl_device_usb *dev,
>> +                            const libxl_asyncop_how *ao_how)
>> +                            LIBXL_EXTERNAL_CALLERS_ONLY;
>> +int libxl_device_usb_destroy(libxl_ctx *ctx, uint32_t domid,
>> +                             libxl_device_usb *dev,
>> +                             const libxl_asyncop_how *ao_how)
>> +                             LIBXL_EXTERNAL_CALLERS_ONLY;
>> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
>> +                                        int *num)
>> +                          LIBXL_EXTERNAL_CALLERS_ONLY;
> No _getinfo? (Might only make sense with the PV stuff I guess)

IIRC the pattern I saw for other devices was:
* libxl_device_$FOO struct is used to add, remove, destroy
* libl_device_$FOO_list returns an array of libxl_device_$FOO
* libl_device_$FOO_getinfo is only used when there's information you 
need which is not in libxl_device_$FOO struct (and hence isn't returned 
by _list).


>> +        ("backend_domid",    libxl_domid),
>> +        ("backend_domname",  string),
>> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
>> +                         [("hostdev", Struct(None, [
>> +                                ("hostbus",   integer),
>> +                                ("hostaddr",  integer) ]))
> No need to express the host topology I think (because you can build that
> from the bus,addr tuples)?

I don't really follow.  You mean, we can drop 'host' from the last two 
elements, and just call them "bus" and "addr"?

  -George

Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Ian Campbell]

On Wed, 2014-06-18 at 14:22 +0100, George Dunlap wrote:
> On 06/18/2014 02:08 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 14:44 +0100, George Dunlap wrote:
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> >> index c7aa817..963e650 100644
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -82,6 +82,12 @@
> >>   #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >>   
> >>   /*
> >> + * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
> >> + * USB devices.
> >> + */
> >> +#define LIBXL_HAVE_DEVICE_USB 1
> >> +
> >> +/*
> >>    * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
> >>    * libxl_vendor_device field is present in the hvm sections of
> >>    * libxl_domain_build_info. This field tells libxl which
> >> @@ -924,6 +930,40 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
> >>                          const libxl_asyncop_how *ao_how)
> >>                          LIBXL_EXTERNAL_CALLERS_ONLY;
> >>   
> >> +/*
> >> + * USB
> >> + *
> >> + * For each device removed or added, one of these protocols is available:
> >> + * - PV (i.e., PVUSB)
> >> + * - DEVICEMODEL (i.e, qemu)
> >> + *
> >> + * PV is available for either PV or HVM domains.  DEVICEMODEL is only
> >> + * available for HVM domains.  The caller can additionally specify
> >> + * "AUTO", in which case the library will try to determine the best
> >> + * protocol automatically.
> >> + *
> >> + * At the moment, the only protocol implemented is DEVICEMODEL, and the only
> >> + * device type implemented is HOSTDEV.
> >> + *
> >> + * This uses the qmp functionality, and is thus only available for
> >> + * qemu-xen, not qemu-traditional.
> >> + */
> >> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
> >> +                         libxl_device_usb *dev,
> >> +                         const libxl_asyncop_how *ao_how)
> >> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> >> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
> >> +                            libxl_device_usb *dev,
> >> +                            const libxl_asyncop_how *ao_how)
> >> +                            LIBXL_EXTERNAL_CALLERS_ONLY;
> >> +int libxl_device_usb_destroy(libxl_ctx *ctx, uint32_t domid,
> >> +                             libxl_device_usb *dev,
> >> +                             const libxl_asyncop_how *ao_how)
> >> +                             LIBXL_EXTERNAL_CALLERS_ONLY;
> >> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
> >> +                                        int *num)
> >> +                          LIBXL_EXTERNAL_CALLERS_ONLY;
> > No _getinfo? (Might only make sense with the PV stuff I guess)
> 
> IIRC the pattern I saw for other devices was:
> * libxl_device_$FOO struct is used to add, remove, destroy
> * libl_device_$FOO_list returns an array of libxl_device_$FOO
> * libl_device_$FOO_getinfo is only used when there's information you 
> need which is not in libxl_device_$FOO struct (and hence isn't returned 
> by _list).

Right, getinfo is runtime stuff, like the evtchn and shared ring (or
stats, I guess). Hence probably only for PV unless there is something
like that for the emulated stuff.


> 
> 
> >> +        ("backend_domid",    libxl_domid),
> >> +        ("backend_domname",  string),
> >> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
> >> +                         [("hostdev", Struct(None, [
> >> +                                ("hostbus",   integer),
> >> +                                ("hostaddr",  integer) ]))
> > No need to express the host topology I think (because you can build that
> > from the bus,addr tuples)?
> 
> I don't really follow.  You mean, we can drop 'host' from the last two 
> elements, and just call them "bus" and "addr"?

Gah, I started writing one thing and then reunderstood usb and wrote
half another.

What I was trying to say is that you don't need hostaddr to describe the
full USB topology path to the device because the (bus,addr) tuple you've
given already does so (because each hub effectively creates a new bus
number, so they all look like toplevel buses in this representation).

But you could drop the host prefix, yes.

Ian.

Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[George Dunlap]

On 06/18/2014 02:49 PM, Ian Campbell wrote:
>>>> +        ("backend_domid",    libxl_domid),
>>>> +        ("backend_domname",  string),
>>>> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
>>>> +                         [("hostdev", Struct(None, [
>>>> +                                ("hostbus",   integer),
>>>> +                                ("hostaddr",  integer) ]))
>>> No need to express the host topology I think (because you can build that
>>> from the bus,addr tuples)?
>> I don't really follow.  You mean, we can drop 'host' from the last two
>> elements, and just call them "bus" and "addr"?
> Gah, I started writing one thing and then reunderstood usb and wrote
> half another.
>
> What I was trying to say is that you don't need hostaddr to describe the
> full USB topology path to the device because the (bus,addr) tuple you've
> given already does so (because each hub effectively creates a new bus
> number, so they all look like toplevel buses in this representation).

You seem to be saying that something is redundant, or that there's extra 
information somewhere; but as there are only two bits of data (bus and 
addr), and agree that I need both, I'm having a hard time telling what 
you think is redundant / could be removed..

"hostdev" is an element of the union; so the structure should unpack 
like this:
struct {
   libxl_domid backend_domid;
   char * backend_domname;
   libxl_usb_device_type type;
   union u {
    struct {
     int hostbus, hostaddr;
    } hostdev;
   };
};

At the moment, "type" can only be "hostdev"; but I'm envisioning in the 
future that "type" might be "tablet", "mouse", "keyboard", maybe 
"mass-storage", and that the union would have more elements.

Does that clear things up?  Or am I totally confused? :-)

  -George

Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Ian Campbell]

On Wed, 2014-06-18 at 14:58 +0100, George Dunlap wrote:
> On 06/18/2014 02:49 PM, Ian Campbell wrote:
> >>>> +        ("backend_domid",    libxl_domid),
> >>>> +        ("backend_domname",  string),
> >>>> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
> >>>> +                         [("hostdev", Struct(None, [
> >>>> +                                ("hostbus",   integer),
> >>>> +                                ("hostaddr",  integer) ]))
> >>> No need to express the host topology I think (because you can build that
> >>> from the bus,addr tuples)?
> >> I don't really follow.  You mean, we can drop 'host' from the last two
> >> elements, and just call them "bus" and "addr"?
> > Gah, I started writing one thing and then reunderstood usb and wrote
> > half another.
> >
> > What I was trying to say is that you don't need hostaddr to describe the
> > full USB topology path to the device because the (bus,addr) tuple you've
> > given already does so (because each hub effectively creates a new bus
> > number, so they all look like toplevel buses in this representation).
> 
> You seem to be saying that something is redundant, or that there's extra 
> information somewhere; but as there are only two bits of data (bus and 
> addr), and agree that I need both, I'm having a hard time telling what 
> you think is redundant / could be removed..

I'm actually saying the opposite (or rather asking for confirmation that
this is the case). i.e. one might expect that *more* information would
be needed here (i.e. the full bus topology/path to a device via multiple
hubs etc) but in fact this is not needed because the topology is not
visible in this numbering scheme (each potential fork in the path
results in a whole new bus number).

Sorry, I could have been clearer about this.

> "hostdev" is an element of the union; so the structure should unpack 
> like this:
> struct {
>    libxl_domid backend_domid;
>    char * backend_domname;
>    libxl_usb_device_type type;
>    union u {
>     struct {
>      int hostbus, hostaddr;
>     } hostdev;
>    };
> };
> 
> At the moment, "type" can only be "hostdev"; but I'm envisioning in the 
> future that "type" might be "tablet", "mouse", "keyboard", maybe 
> "mass-storage", and that the union would have more elements.

This seems sensible. You brought up dropping the hostdev prefix from the
bus and addr (because you thought I was ;-)), which seemed sensible when
you think of the code which accesses these fields i.e.
dev->u.hostdev.hostbus vs dev->u.hostdev.bus. Up to you whether you want
to repeat "host" in that.

> Does that clear things up?  Or am I totally confused? :-)

I think you are remarkably unconfused given how confusing I'm being...

Ian.

Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[George Dunlap]

On 06/18/2014 03:30 PM, Ian Campbell wrote:
> On Wed, 2014-06-18 at 14:58 +0100, George Dunlap wrote:
>> On 06/18/2014 02:49 PM, Ian Campbell wrote:
>>>>>> +        ("backend_domid",    libxl_domid),
>>>>>> +        ("backend_domname",  string),
>>>>>> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
>>>>>> +                         [("hostdev", Struct(None, [
>>>>>> +                                ("hostbus",   integer),
>>>>>> +                                ("hostaddr",  integer) ]))
>>>>> No need to express the host topology I think (because you can build that
>>>>> from the bus,addr tuples)?
>>>> I don't really follow.  You mean, we can drop 'host' from the last two
>>>> elements, and just call them "bus" and "addr"?
>>> Gah, I started writing one thing and then reunderstood usb and wrote
>>> half another.
>>>
>>> What I was trying to say is that you don't need hostaddr to describe the
>>> full USB topology path to the device because the (bus,addr) tuple you've
>>> given already does so (because each hub effectively creates a new bus
>>> number, so they all look like toplevel buses in this representation).
>> You seem to be saying that something is redundant, or that there's extra
>> information somewhere; but as there are only two bits of data (bus and
>> addr), and agree that I need both, I'm having a hard time telling what
>> you think is redundant / could be removed..
> I'm actually saying the opposite (or rather asking for confirmation that
> this is the case). i.e. one might expect that *more* information would
> be needed here (i.e. the full bus topology/path to a device via multiple
> hubs etc) but in fact this is not needed because the topology is not
> visible in this numbering scheme (each potential fork in the path
> results in a whole new bus number).
>
> Sorry, I could have been clearer about this.

Ah, I see.  Yes, that's right: <bus,addr> is sufficient to specify a 
single device.

To qemu you can either specify <bus,addr> or <vendorid:productid>.  
Unfortunately both have their advantages and disavantages.  <bus,addr> 
will specify a unique device, but exactly which device that is may 
change across reboots, so you have to check each time that you're still 
passing in your USB stick rather than your webcam.

On the other hang, <vid,pid> will allow you to pass in a specific kind 
of device no matter where it's plugged in... as long as there's only one 
of them.  If you have two webcams / usb devices of the same kind, you're 
out of luck, and you have to fall back to <bus,addr> again.

I considered allowing the user to specify either one, just as you can 
with qemu. (That might mean having libxl do the <vid,pid> -> <bus,addr> 
resolution for PVUSB.)  I'm open to opinions on that one.

  -George

[PATCH v7 RFC 2/2] xl: Add commands for usb hot-plug

[George Dunlap]

Add xl commands for USB hot-plug.  Domain creation and config file
support not yet added.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
v7:
 - Don't check for invalid return values for find_domain()
 - Use {foo}_to_string in usb_list()
 - call dispose on list elements to avoid leaks
v6:
 - Rename to usb-attach and usb-detach to fit better w/ other device
   add/remove commands
 - Use positional arguments, to fit better w/ other device add/remove
   commands
 - Change specifier to "hostdev:xx.yy", looking forward to a day when
   we may be able to accept something like "tablet"
 - Style fix-ups
v5:
 - Remove extraneous is_hex
 - Replace is_dec with ctype.h isdigit()
 - Replace domid -1 with INVALID_DOMID
 - Fix up a couple of mistaken function names in strings
 - Copy CTYPE macro from libxl_internal.h
 - Fix to usb-list manpage (use -d domid)

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Roger Pau Monne <roger.pau@citrix.com>
CC: sstanisi@cbnco.com
CC: Fabio Fantoni <fabio.fantoni@m2r.biz>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
CC: Simon (Bo) Cao <caobosimon@gmail.com>
---
 docs/man/xl.pod.1         |   30 ++++++++
 tools/libxl/xl.h          |   21 ++++++
 tools/libxl/xl_cmdimpl.c  |  182 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/xl_cmdtable.c |   15 ++++
 4 files changed, 248 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 30bd4bf..e04a4a8 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1194,6 +1194,36 @@ List virtual network interfaces for a domain.
 
 =back
 
+=head2 USB DEVICES
+
+=over 4
+
+=item B<usb-attach> I<domain-id> I<hostdev:hostbus.hostaddr>
+
+Passes through the host USB device specified by I<hostbus.hostaddr>.  At
+the moment this will only work for HVM domains via qemu.
+
+The best way to find out the information for the device is typically using
+lsusb.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<usb-detach> I<domain-id> I<hostdev:hosbus.hostaddr>
+
+Remove the host USB device from I<domain-id> which is specified
+by <hostbus.hostaddr>.  This command only works for devices added
+with usb-add; not for those specified in the config file.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<usb-list> I<domain-id>
+
+Show host USB devices assigned to the guest.
+
+=back
+
 =head2 VTPM DEVICES
 
 =over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 10a2e66..2f1f612 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -35,6 +35,9 @@ int main_info(int argc, char **argv);
 int main_sharing(int argc, char **argv);
 int main_cd_eject(int argc, char **argv);
 int main_cd_insert(int argc, char **argv);
+int main_usb_attach(int argc, char **argv);
+int main_usb_detach(int argc, char **argv);
+int main_usb_list(int argc, char **argv);
 int main_console(int argc, char **argv);
 int main_vncviewer(int argc, char **argv);
 int main_pcilist(int argc, char **argv);
@@ -183,6 +186,24 @@ extern void printf_info_sexp(int domid, libxl_domain_config *d_config);
 #define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf"
 #define XL_LOCK_FILE XEN_LOCK_DIR "/xl"
 
+/*
+ * int CTYPE(ISFOO, char c);
+ * int CTYPE(toupper, char c);
+ * int CTYPE(tolower, char c);
+ *
+ * This is necessary because passing a simple char to a ctype.h
+ * is forbidden.  ctype.h macros take ints derived from _unsigned_ chars.
+ *
+ * If you have a char which might be EOF then you should already have
+ * it in an int representing an unsigned char, and you can use the
+ * <ctype.h> macros directly.  This generally happens only with values
+ * from fgetc et al.
+ *
+ * For any value known to be a character (eg, anything that came from
+ * a char[]), use CTYPE.
+ */
+#define CTYPE(isfoo,c) (isfoo((unsigned char)(c)))
+
 #endif /* XL_H */
 
 /*
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5195914..7973b4c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2771,6 +2771,188 @@ int main_cd_insert(int argc, char **argv)
     return 0;
 }
 
+static int parse_usb_specifier(libxl_device_usb *dev, char *s)
+{
+    char *devtype, * hostbus, *hostaddr, *p;
+
+    /*
+     * General format:
+     * <type>[:<type-specific options>]
+     */
+    p = s;
+    devtype = strsep(&p, ":");
+
+    if (strcmp(devtype, "hostdev")) {
+        fprintf(stderr, "Unknown device type: %s\n", devtype);
+        return -1;
+    }
+    
+    hostbus = strsep(&p, ".");
+
+    hostaddr = p;
+
+    if (!hostbus) {
+        fprintf(stderr, "Device type %s requires hostaddr\n",
+                devtype);
+        return -1;
+    }
+
+    if (!hostaddr) {
+        fprintf(stderr, "Device type %s requires hostaddr\n",
+                devtype);
+        return -1;
+    }
+
+    /* Make sure they look right */
+    for (p=hostbus; *p; p++) {
+        if (!CTYPE(isdigit,*p)) {
+            fprintf(stderr, "Bad hostbus: %s\n", hostbus);
+            return -1;
+        }
+    }
+    
+    for (p=hostaddr; *p; p++) {
+        if (!CTYPE(isdigit,*p)) {
+            fprintf(stderr, "Bad hostaddr: %s\n", hostaddr);
+            return -1;
+        }
+    }
+
+    dev->type = LIBXL_DEVICE_USB_TYPE_HOSTDEV;
+
+    dev->u.hostdev.hostbus  = atoi(hostbus);
+    dev->u.hostdev.hostaddr = atoi(hostaddr);
+
+    return 0;
+}
+
+static int usb_attach(uint32_t domid, char * device)
+{
+    libxl_device_usb usbdev;
+    int rc;
+
+    libxl_device_usb_init(&usbdev);
+
+    if (parse_usb_specifier(&usbdev, device)<0) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL);
+    if (rc<0)
+        fprintf(stderr, "libxl_device_usb_add failed.\n");
+
+    libxl_device_usb_dispose(&usbdev);
+
+out:
+    return rc;
+}
+
+int main_usb_attach(int argc, char **argv)
+{
+    uint32_t domid = INVALID_DOMID;
+    int opt = 0, rc;
+    char *device = NULL;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) {
+        /* No options */
+    }
+
+    domid = find_domain(argv[optind]);
+    device = argv[optind + 1];
+
+    rc = usb_attach(domid, device);
+    if (rc<0)
+        return 1;
+    else
+        return 0;
+}
+
+static int usb_detach(uint32_t domid, libxl_device_usb_type type,
+                      char * device)
+{
+    libxl_device_usb usbdev;
+    int rc;
+
+    libxl_device_usb_init(&usbdev);
+
+    if (parse_usb_specifier(&usbdev, device)<0) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    rc = libxl_device_usb_remove(ctx, domid, &usbdev, NULL);
+    if (rc<0)
+        fprintf(stderr, "libxl_device_usb_remove failed.\n");
+
+    libxl_device_usb_dispose(&usbdev);
+
+out:
+    return rc;
+}
+
+int main_usb_detach(int argc, char **argv)
+{
+    uint32_t domid = INVALID_DOMID;
+    int opt = 0, rc;
+    char *device = NULL;
+    int type = 0;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-detach", 2) {
+        /* No options */
+    }
+
+    domid = find_domain(argv[optind]);
+    device = argv[optind + 1];
+
+    rc = usb_detach(domid, type, device);
+    if (rc < 0)
+        return 1;
+    else
+        return 0;
+}
+
+static void usb_list(uint32_t domid)
+{
+    libxl_device_usb *dev;
+    int num, i;
+
+    dev = libxl_device_usb_list(ctx, domid, &num);
+    if (dev == NULL)
+        return;
+    printf("protocol    backend  type     device\n");
+    for (i = 0; i < num; i++) {
+        printf("%10s  ", 
+               libxl_usb_protocol_to_string(dev[i].protocol));
+        printf("%7d  ", dev[i].backend_domid);
+        printf("%7s  ", libxl_device_usb_type_to_string(dev[i].type));
+        if (dev[i].type == LIBXL_DEVICE_USB_TYPE_HOSTDEV)
+            printf("%03d.%03d",
+                   dev[i].u.hostdev.hostbus,
+                   dev[i].u.hostdev.hostaddr);
+        printf("\n");
+        libxl_device_usb_dispose(&dev[i]);
+    }
+    free(dev);
+}
+
+
+int main_usb_list(int argc, char **argv)
+{
+    uint32_t domid = INVALID_DOMID;
+    int opt;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-list", 1) {
+        /* No options */
+    }
+
+    domid = find_domain(argv[optind]);
+    usb_list(domid);
+    return 0;
+}
+
+
+
 int main_console(int argc, char **argv)
 {
     uint32_t domid;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4279b9f..c97796b 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -194,6 +194,21 @@ struct cmd_spec cmd_table[] = {
       "Eject a cdrom from a guest's cd drive",
       "<Domain> <VirtualDevice>",
     },
+    { "usb-attach",
+      &main_usb_attach, 1, 1,
+      "Hot-plug a usb device to a domain.",
+      "<Domain> <hostdev:<hostbus.hostaddr>>",
+    },
+    { "usb-detach",
+      &main_usb_detach, 1, 1,
+      "Hot-unplug a usb device from a domain.",
+      "<Domain> <hostdev:hostbus.hostaddr>",
+    },
+    { "usb-list",
+      &main_usb_list, 0, 0,
+      "List usb devices for a domain",
+      "<Domain>",
+    },
     { "mem-max",
       &main_memmax, 0, 1,
       "Set the maximum amount reservation for a domain",
-- 
1.7.9.5

Re: [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 02:44:16PM +0100, George Dunlap wrote:
> == Libvirt's interface ==
> 
> I've had a brief look at libvirt's USB interface, and learned a bit
> about libvirt's general approach to things at the Xen Hackathon last
> week.  One of the goals of libvirt is to be able to specify the
> virtual hardware in enough detail to keep it from changing when you
> upgrade the hypervisor, so that certain proprietary operating systems
> which are sensitive to this kind of thing continue to work.
> 
> Instead of specifying a controller name, you specify a controller
> index (which is different than qemu).  But instead of specifying a USB
> version number, you specify a model for the USB controller (which
> happens to be the exact name that qemu uses): "piix3-uhci",
> "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", "ich9-uhci2",
> "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci".
> 
> When specifying a USB device, libvirt has the concept of an "address"
> where you will plug it into.  Here is what the page says about it:  
>
> "USB addresses have the following additional attributes: bus (a hex
> value between 0 and 0xfff, inclusive), and port (a dotted notation of
> up to four octets, such as 1.2 or 2.1.3.1)."
> 
> It's not exactly clear to me what those numbers mean.  But after
> chatting with Daniel Berrange at the Hackathon, it looks like the
> "bus" in the address corresponds to the "index" in the controller
> specification.  It also appears that this "index" is internal to
> libvirt and is not exposed to the guest: so it should in theory be
> possible to have index 0 be a Xen PVUSB controller, 1 be an emulated
> qemu controller, 3 be an emulated PVUSB controller, and so on.


This structure/concept is something that libvirt uses more generally
than just USB. Any kind of device which has further devices attached
as a child is represented as a "controller" in libvirt. eg PCI bus,
PCI bridge, USB controller, USB hub, IDE controller, SCSI controller
etc, etc.  Any device then has an <address/> element whose 'type'
attribute specifies what type of controller it connects to, and which
maps to the specific <controller> element via its index number. 
The validate attributes for the <address> vary depending on the address
type / controller type.

You are correct that the index is completely internal to libvirt,
not exposed to QEMU or the guest. So in USB case the 'bus' attribute
on <address type='usb'/> maps to the <controller> index for the USB
controller.

WRT to choosing the type of controller, libvirt always aims to use
the name of a real hardware model if there is one, since this is
something that can be standardized across hypervisors if they're
emulating the same underlying hardware. The specific model name
we pick is first-come-first-served. So if choose 'piix3-uhci' 
based on QEMU name and later we add VMWare support and it has
called it 'piix3-usb', we'd still use 'piix3-uhci' in libvirt
XML and re-write to the VMWare specific name internally. Or the
opposite if we standardized on the VMWare name first and later
wnated to add QEMU support.

In the case of totally invented paravirt devices, we'll come up
with a name that seems most appropriate for the hypervisor which
invented the device type.

> == Draft design proposal ==
> 
> I've given it some thought, and based on the below is a suggested
> design for people to have a go at.
> 
> Basic idea: Specify named controllers.  It's controllers that are PV
> or emulated, and may have a backend domain.  When adding devices,
> specify which controller to attach it to.  Allow most things to have
> intelligent defaults.
> 
> * libxl functions
> 
> usb_controller_add
> usb_controller_remove/destroy
> usb_controller_list
> device struct:
>   name # If empty, default to hciN/pvN, where N starts at 0
>   type = {PV, EMULATED, AUTO}
>   backend_{domname,domid} # PV only
>   usbversion = {1, 2, 3}
>   numports # default 16

So with this kind of syntax, libvirt is going to have to try to
map a model name 'piix3-uhci' to a pair of type,usbversion
eg type=emulated,usbversion=1. We'd likely give the paravirt
controller model names of  xenusb1, xenusb2, xenusb3  for USB-1
USB-2 and USB-3 respectively, and again map these to type=pv,usbversion=N.



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[George Dunlap]

On Thu, Jun 5, 2014 at 11:14 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Mon, Jun 02, 2014 at 02:44:16PM +0100, George Dunlap wrote:
>> == Libvirt's interface ==
>>
>> I've had a brief look at libvirt's USB interface, and learned a bit
>> about libvirt's general approach to things at the Xen Hackathon last
>> week.  One of the goals of libvirt is to be able to specify the
>> virtual hardware in enough detail to keep it from changing when you
>> upgrade the hypervisor, so that certain proprietary operating systems
>> which are sensitive to this kind of thing continue to work.
>>
>> Instead of specifying a controller name, you specify a controller
>> index (which is different than qemu).  But instead of specifying a USB
>> version number, you specify a model for the USB controller (which
>> happens to be the exact name that qemu uses): "piix3-uhci",
>> "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", "ich9-uhci2",
>> "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci".
>>
>> When specifying a USB device, libvirt has the concept of an "address"
>> where you will plug it into.  Here is what the page says about it:
>>
>> "USB addresses have the following additional attributes: bus (a hex
>> value between 0 and 0xfff, inclusive), and port (a dotted notation of
>> up to four octets, such as 1.2 or 2.1.3.1)."
>>
>> It's not exactly clear to me what those numbers mean.  But after
>> chatting with Daniel Berrange at the Hackathon, it looks like the
>> "bus" in the address corresponds to the "index" in the controller
>> specification.  It also appears that this "index" is internal to
>> libvirt and is not exposed to the guest: so it should in theory be
>> possible to have index 0 be a Xen PVUSB controller, 1 be an emulated
>> qemu controller, 3 be an emulated PVUSB controller, and so on.
>
>
> This structure/concept is something that libvirt uses more generally
> than just USB. Any kind of device which has further devices attached
> as a child is represented as a "controller" in libvirt. eg PCI bus,
> PCI bridge, USB controller, USB hub, IDE controller, SCSI controller
> etc, etc.  Any device then has an <address/> element whose 'type'
> attribute specifies what type of controller it connects to, and which
> maps to the specific <controller> element via its index number.
> The validate attributes for the <address> vary depending on the address
> type / controller type.
>
> You are correct that the index is completely internal to libvirt,
> not exposed to QEMU or the guest. So in USB case the 'bus' attribute
> on <address type='usb'/> maps to the <controller> index for the USB
> controller.
>
> WRT to choosing the type of controller, libvirt always aims to use
> the name of a real hardware model if there is one, since this is
> something that can be standardized across hypervisors if they're
> emulating the same underlying hardware. The specific model name
> we pick is first-come-first-served. So if choose 'piix3-uhci'
> based on QEMU name and later we add VMWare support and it has
> called it 'piix3-usb', we'd still use 'piix3-uhci' in libvirt
> XML and re-write to the VMWare specific name internally. Or the
> opposite if we standardized on the VMWare name first and later
> wnated to add QEMU support.

That makes sense.

>
> In the case of totally invented paravirt devices, we'll come up
> with a name that seems most appropriate for the hypervisor which
> invented the device type.
>
>> == Draft design proposal ==
>>
>> I've given it some thought, and based on the below is a suggested
>> design for people to have a go at.
>>
>> Basic idea: Specify named controllers.  It's controllers that are PV
>> or emulated, and may have a backend domain.  When adding devices,
>> specify which controller to attach it to.  Allow most things to have
>> intelligent defaults.
>>
>> * libxl functions
>>
>> usb_controller_add
>> usb_controller_remove/destroy
>> usb_controller_list
>> device struct:
>>   name # If empty, default to hciN/pvN, where N starts at 0
>>   type = {PV, EMULATED, AUTO}
>>   backend_{domname,domid} # PV only
>>   usbversion = {1, 2, 3}
>>   numports # default 16
>
> So with this kind of syntax, libvirt is going to have to try to
> map a model name 'piix3-uhci' to a pair of type,usbversion
> eg type=emulated,usbversion=1.

Right -- if we went with an interface where you cannot specify via
libxl exactly which controller qemu uses, then we would try to make
sure that repeated invocations of the same parameters got the same
controller, so that things don't change under the guest OS's feet.  So
it should be OK for libvirt to magically know that "{ .type=EMULATED,
.usbversion=1}" would map to 'piix3-uhci', and use that to implement
libvirt's 'piix3-uhci' model.  Anything not exposed by libxl would
have to just be unimplemented until the interface was extended to
allow them.

At the moment the mapping exposed by the "usbversion" seems to be: {1,
2, 3} -> {piix3-uhci, ich9-usb-ehci1, nec-usb-xhci}.  Does that seem
like a reasonable choice?

Personally I wouldn't mind being able to either pass in "usbversion",
or a string requesting a specific controller name -- but I think IanJ
would prefer to keep those details secret.

> We'd likely give the paravirt
> controller model names of  xenusb1, xenusb2, xenusb3  for USB-1
> USB-2 and USB-3 respectively, and again map these to type=pv,usbversion=N.

That sounds reasonable.

 -George

Re: [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[Ian Campbell]

On Mon, 2014-06-02 at 14:44 +0100, George Dunlap wrote:
> == Open questions ==
> 
> Those are things I think I know; there are a couple of pertinent
> factual questions which I'm not sure of:
> 
> * Is it possible to specify PVUSB controllers and attach USB devices
> to them before the guest is up and running (i.e., before the frontend
> is connected)?  It looks like xend had a syntax for specifying virtual
> controllers and attaching virtual devices, so it seems like it should
> be possible.

Unless the PVUSB drivers are radically different to other PV devices
this ought to be possible and should just work. (Essentially you just
preload the backend with all the settings and htey get handled when the
f.e. turns up)

> * Is it possible to connect a USB 1.1 device to a PVUSB controller
> which has been specified 2.0, or would there have to be a separate
> virtual controller for each?

I'm a bit surprised that PVUSB exposes the concept of a version to the
FE at all. I suppose there is some USBish reason why the f.e. would
care.

But I don't know the answer to your question.

> * Is it possible for the toolstack to tell if dom0 (or whatever the
> specified backend domain) has PVUSB support before starting the guest?

After creating the nodes with state == XenbusStateInitialising (1) the
toolstack waits for the backend to move to XenbusStateInitWait (2)
before continuing, with a timeout. So you will detect this in a
controlled way. You can't tell before try the setup though since the
driver might be autoloaded.

(Assuming pvusbback is the same as everything else)

> * Is it possible for the toolstack to tell if domU has a working and
> connected PVUSB front-end?

It can observe the state variable being 4 I suppose. Why do you need to
know?

> * Do we want to be able to create virtual hubs for qemu-backed
> controllers at some point in the future?  Is there any groundwork we
> want to lay for that?

qemu-backed emulated or PV controllers?

I don't think emulated would make sense for a PV guest and if qemu
wanted to be a PV backend it would have to implement the usual xenstore
watches etc.

I suppose a backend type a la libxl_device_disk's = phy|tap|qdisk might
be needed for this, but I think you can probably add that in a
compatible way in the future if necessary.

> == Design questions ==
> 
> Then based on that, we have several design questions.
> 
> * How much of the "controller" thing should be exposed via libxl?  Via xl?
> 
> * This series only allows you to specify the "protocol", either PV or
> DEVICE_MODEL.  Would it be better, for instance, to get rid of that,
> and instead allow the user to specify the creation of USB controllers,
> allow them to have a type of "HCI (or emulated)" or "PV", and allow
> the user to specify which controller to attach a specific device to?
> 
> * How much "smarts" should we have in the libxl / xl about creating
> emulated/virtual controllers and of what kinds?
> 
> * How much / what kind of "smarts" should be in libxl / xl about
> choosing a controller to plug a device into?

Dunno * 4. Your proposed design looked ok to me.

> * What about config file syntax?  Should we try to reuse / extend the
> current config syntax, or create a new one?  Should the new one allow
> the specification of controllers?  Should it perhaps *require* the
> specification of controllers?

We should at least strive for any existing xm config files which use USB
to continue working, but that needn't imply that the preferred xl syntax
looks that way.

Of course if the xm syntax is horribly terribly broken then we might
make a concious choice not to carry it forward, but the default should
be compatibility.


> For reference, here are some example config snippets from the current
> xl / xm config files:
> 
> -- snip --
> # HVM USB
> usb=1
> usbdevice=['tablet','host:4.3']
> 
> # HVM USB, not compatible with the above
> usbversion=3
> 
> # xend's PVUSB config file; this creates one virtual controller, then
> # plugs hostdev 1.8 into port 1
> vusb=['usbver=2, numports=4, port_1=1-8']

Oh my. That last one is quite exciting.

> -- snip --
> 
> Given that, here is a potential config file format:
> 
> -- snip --
> # Create two controllers, one pv, one emulated
> usbcontroller=['type=pv,name=pv0,usbversion=2,numports=4',
>                'type=emul,name=hci0,usbversion=2']
> 
> # Create a controller with the defaults; PV for PV guests, emul for HVM guests
> usbcontroller=[''] 
> 
> # Same as above, but defaulting to version 2
> usbversion=2 
> 
> # I think we should be able to automatically detect which format to
> # use; so I think we should re-use usbdevice.  I could be convinced otherwise.
> usbdevice=['type=tablet','type=hostdev,hostaddr=4.3,bus=pv0']

Does this require that the usbcontroller have been specified?

I think it would be good if xl would by default create some number of
appropriate controllers without my having to specify them explicitly,
iow just saying usbdevice=[...] should be enough.

I'm not saying that you shouldn't support more specific syntax for
people who want more control, just that it shouldn't be required to do
so.

(I'm just talking xl here, at the libxl layer I think it would be fine
to require them to be explicit).


> * Rather than having to specify a controller, automatically hot-plug
> controllers as-needed.

At the xl level I think this would be good.

Ian.

Re: [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[George Dunlap]

On 06/18/2014 01:57 PM, Ian Campbell wrote:
>> * Is it possible for the toolstack to tell if domU has a working and
>> connected PVUSB front-end?
> It can observe the state variable being 4 I suppose. Why do you need to
> know?

It might be nice to be able to create both a pv and an emulated 
controller in the config file, and then have "xl usb-attach [foo]" to 
automatically plug it into the PV controller if the PV frontend seems to 
be up, and into the emulated controller if it doesn't seem to be up.

But that doesn't change the elements of the interface, just what the 
default would be if the controller field is empty.

>> * Do we want to be able to create virtual hubs for qemu-backed
>> controllers at some point in the future?  Is there any groundwork we
>> want to lay for that?
> qemu-backed emulated or PV controllers?
>
> I don't think emulated would make sense for a PV guest and if qemu
> wanted to be a PV backend it would have to implement the usual xenstore
> watches etc.

I mean, emulate an actual USB hub -- you know, it plugs into your USB 
controller and you can plug other USB devices into it.

I'm inclined not to bother with it at this point.

>> -- snip --
>>
>> Given that, here is a potential config file format:
>>
>> -- snip --
>> # Create two controllers, one pv, one emulated
>> usbcontroller=['type=pv,name=pv0,usbversion=2,numports=4',
>>                 'type=emul,name=hci0,usbversion=2']
>>
>> # Create a controller with the defaults; PV for PV guests, emul for HVM guests
>> usbcontroller=['']
>>
>> # Same as above, but defaulting to version 2
>> usbversion=2
>>
>> # I think we should be able to automatically detect which format to
>> # use; so I think we should re-use usbdevice.  I could be convinced otherwise.
>> usbdevice=['type=tablet','type=hostdev,hostaddr=4.3,bus=pv0']
> Does this require that the usbcontroller have been specified?
>
> I think it would be good if xl would by default create some number of
> appropriate controllers without my having to specify them explicitly,
> iow just saying usbdevice=[...] should be enough.
>
> I'm not saying that you shouldn't support more specific syntax for
> people who want more control, just that it shouldn't be required to do
> so.
>
> (I'm just talking xl here, at the libxl layer I think it would be fine
> to require them to be explicit).

That makes sense.

>> * Rather than having to specify a controller, automatically hot-plug
>> controllers as-needed.
> At the xl level I think this would be good.

OK, sounds good.  Thanks for the input.

  -George

Re: [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[George Dunlap]

On 06/18/2014 01:57 PM, Ian Campbell wrote:
>> * Is it possible to connect a USB 1.1 device to a PVUSB controller
>> which has been specified 2.0, or would there have to be a separate
>> virtual controller for each?
> I'm a bit surprised that PVUSB exposes the concept of a version to the
> FE at all. I suppose there is some USBish reason why the f.e. would
> care.
>
> But I don't know the answer to your question.

Simon,

Can you put "Test different USB version devices with different PVUSB bus 
version numbers" on your to-do list?

(i.e., find USB 1.1, 2.0, and 3.0 devices, and try each of them with 
usbversion set to 1, 2, and 3).

Thanks!

  -George

Re: [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[Simon Cao]

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

okay, I will do my best to test all the three devices.

Bo.


2014-06-18 22:04 GMT+08:00 George Dunlap <george.dunlap@eu.citrix.com>:

> On 06/18/2014 01:57 PM, Ian Campbell wrote:
>
>> * Is it possible to connect a USB 1.1 device to a PVUSB controller
>>> which has been specified 2.0, or would there have to be a separate
>>> virtual controller for each?
>>>
>> I'm a bit surprised that PVUSB exposes the concept of a version to the
>> FE at all. I suppose there is some USBish reason why the f.e. would
>> care.
>>
>> But I don't know the answer to your question.
>>
>
> Simon,
>
> Can you put "Test different USB version devices with different PVUSB bus
> version numbers" on your to-do list?
>
> (i.e., find USB 1.1, 2.0, and 3.0 devices, and try each of them with
> usbversion set to 1, 2, and 3).
>
> Thanks!
>
>  -George
>
>

[-- Attachment #1.2: Type: text/html, Size: 1564 bytes --]

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

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