[PATCH v2 0/4] libxl: add HVM USB passthrough capability

[PATCH v2 0/4] libxl: add HVM USB passthrough capability

[Juergen Gross]

Add the capability to pass USB devices to HVM domains by using the
emulation of USB controllers of qemu.

The user interface via xl is the same as for pvusb passthrough, only
the type of the usbctrl is different: instead of "qusb" (qemu-based
pvusb backend) or "vusb" (kernel-based pvusb backend) the type
"devicemodel" is used.

Especially the communication with qemu via qmp commands is based on
the patches of George Dunlap sent in 2014:

https://lists.xen.org/archives/html/xen-devel/2014-06/msg00085.html

Changes in V2:
- patches 1-3 removed as already pushed
- split out (new) patch 1 from patch 3 (was 5) as requested by Wei Liu
- addressed code style issues in patch 3 (was 5) as requested by Wei Liu
- added some assert()s n patch 3 (was 5) as requested by Wei Liu

Juergen Gross (4):
  libxl: add function to remove usb controller xenstore entries
  libxl: add basic support for devices without backend
  libxl: add HVM usb passthrough support
  docs: add HVM USB passthrough documentation

 docs/man/xl.cfg.pod.5.in             |  12 +-
 tools/libxl/libxl_device.c           |  62 +++--
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/libxl_usb.c              | 435 +++++++++++++++++++++++++++--------
 tools/libxl/libxl_xshelp.c           |   6 +-
 tools/libxl/xl_cmdimpl.c             |   4 +-
 6 files changed, 400 insertions(+), 120 deletions(-)

-- 
2.6.6


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

[PATCH v2 1/4] libxl: add function to remove usb controller xenstore entries

[Juergen Gross]

In case of failure when trying to add a new USB controller to a domain
libxl might leak xenstore entries. Add a function to remove them and
call this function in case of failure.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
This patch might be a backport candidate to 4.7 (will have to modify
tools/libxl/libxl_pvusb.c instead).
---
 tools/libxl/libxl_usb.c | 58 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 6b30e0f..2493464 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -194,6 +194,47 @@ out:
     return rc;
 }
 
+static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
+{
+    const char *be_path;
+    int r;
+
+    r = libxl__xs_read_checked(gc, XBT_NULL,
+                               GCSPRINTF("%s/backend", libxl_path),
+                               &be_path);
+    if (r || !be_path) return NULL;
+
+    return be_path;
+}
+
+static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
+                                               libxl_device_usbctrl *usbctrl)
+{
+    const char *libxl_path, *be_path;
+    xs_transaction_t t = XBT_NULL;
+    int rc;
+
+    libxl_path = GCSPRINTF("%s/device/vusb/%d",
+                           libxl__xs_libxl_path(gc, domid), usbctrl->devid);
+    be_path = vusb_be_from_xs_libxl(gc, libxl_path);
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+
+        libxl__xs_path_cleanup(gc, t, be_path);
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    return;
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+}
+
 static char *pvusb_get_device_type(libxl_usbctrl_type type)
 {
     switch (type) {
@@ -250,13 +291,15 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
 
     GCNEW(device);
     rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
-    if (rc) goto out;
+    if (rc) goto outrm;
 
     aodev->dev = device;
     aodev->action = LIBXL__DEVICE_ACTION_ADD;
     libxl__wait_device_connection(egc, aodev);
     return;
 
+outrm:
+    libxl__device_usbctrl_del_xenstore(gc, domid, usbctrl);
 out:
     aodev->rc = rc;
     aodev->callback(egc, aodev);
@@ -340,19 +383,6 @@ out:
     return;
 }
 
-static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
-{
-    const char *be_path;
-    int r;
-
-    r = libxl__xs_read_checked(gc, XBT_NULL,
-                               GCSPRINTF("%s/backend", libxl_path),
-                               &be_path);
-    if (r || !be_path) return NULL;
-
-    return be_path;
-}
-
 libxl_device_usbctrl *
 libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
 {
-- 
2.6.6


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

[PATCH v2 2/4] libxl: add basic support for devices without backend

[Juergen Gross]

With the planned support of HVM USB passthrough via the USB emulation
capabilities of qemu libxl has to support guest devices which have no
back- and frontend. Information about those devices will live in the
libxl part of Xenstore only.

Add some basic support to libxl to be able to cope with this scenario.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_device.c           | 59 ++++++++++++++++++++++++------------
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_xshelp.c           |  6 +++-
 3 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index dbf157d..f53f772 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -114,15 +114,21 @@ int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
         libxl__device *device, char **bents, char **fents, char **ro_fents)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    char *frontend_path, *backend_path, *libxl_path;
+    char *frontend_path = NULL, *backend_path = NULL, *libxl_path;
     struct xs_permissions frontend_perms[2];
     struct xs_permissions ro_frontend_perms[2];
     struct xs_permissions backend_perms[2];
     int create_transaction = t == XBT_NULL;
+    int libxl_only = device->backend_kind == LIBXL__DEVICE_KIND_NONE;
     int rc;
 
-    frontend_path = libxl__device_frontend_path(gc, device);
-    backend_path = libxl__device_backend_path(gc, device);
+    if (libxl_only) {
+        /* bents should be set as this is used to setup libxl_path content. */
+        assert(!fents && !ro_fents);
+    } else {
+        frontend_path = libxl__device_frontend_path(gc, device);
+        backend_path = libxl__device_backend_path(gc, device);
+    }
     libxl_path = libxl__device_libxl_path(gc, device);
 
     frontend_perms[0].id = device->domid;
@@ -144,13 +150,15 @@ retry_transaction:
     rc = libxl__xs_rm_checked(gc, t, libxl_path);
     if (rc) goto out;
 
-    rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
-                                 frontend_path);
-    if (rc) goto out;
+    if (!libxl_only) {
+        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
+                                     frontend_path);
+        if (rc) goto out;
 
-    rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
-                                 backend_path);
-    if (rc) goto out;
+        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
+                                     backend_path);
+        if (rc) goto out;
+    }
 
     /* xxx much of this function lacks error checks! */
 
@@ -179,12 +187,15 @@ retry_transaction:
     }
 
     if (bents) {
-        xs_rm(ctx->xsh, t, backend_path);
-        xs_mkdir(ctx->xsh, t, backend_path);
-        xs_set_permissions(ctx->xsh, t, backend_path, backend_perms, ARRAY_SIZE(backend_perms));
-        xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
-                 frontend_path, strlen(frontend_path));
-        libxl__xs_writev(gc, t, backend_path, bents);
+        if (!libxl_only) {
+            xs_rm(ctx->xsh, t, backend_path);
+            xs_mkdir(ctx->xsh, t, backend_path);
+            xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
+                               ARRAY_SIZE(backend_perms));
+            xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
+                     frontend_path, strlen(frontend_path));
+            libxl__xs_writev(gc, t, backend_path, bents);
+        }
 
         /*
          * We make a copy of everything for the backend in the libxl
@@ -194,6 +205,9 @@ retry_transaction:
          * instead.  But there are still places in libxl that try to
          * reconstruct a config from xenstore.
          *
+         * For devices without PV backend (e.g. USB devices emulated via qemu)
+         * only the libxl path is written.
+         *
          * This duplication will typically produces duplicate keys
          * which will go out of date, but that's OK because nothing
          * reads those.  For example, there is usually
@@ -679,14 +693,20 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 {
-    const char *be_path = libxl__device_backend_path(gc, dev);
-    const char *fe_path = libxl__device_frontend_path(gc, dev);
+    const char *be_path = NULL;
+    const char *fe_path = NULL;
     const char *libxl_path = libxl__device_libxl_path(gc, dev);
     const char *tapdisk_path = GCSPRINTF("%s/%s", be_path, "tapdisk-params");
     const char *tapdisk_params;
     xs_transaction_t t = 0;
     int rc;
     uint32_t domid;
+    int libxl_only = dev->backend_kind == LIBXL__DEVICE_KIND_NONE;
+
+    if (!libxl_only) {
+        be_path = libxl__device_backend_path(gc, dev);
+        fe_path = libxl__device_frontend_path(gc, dev);
+    }
 
     rc = libxl__get_domid(gc, &domid);
     if (rc) goto out;
@@ -704,10 +724,11 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
              * The toolstack domain is in charge of removing the
              * frontend and libxl paths.
              */
-            libxl__xs_path_cleanup(gc, t, fe_path);
+            if (!libxl_only)
+                libxl__xs_path_cleanup(gc, t, fe_path);
             libxl__xs_path_cleanup(gc, t, libxl_path);
         }
-        if (dev->backend_domid == domid) {
+        if (dev->backend_domid == domid && !libxl_only) {
             /*
              * The driver domain is in charge of removing what it can
              * from the backend path.
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 177f9b7..82e5c07 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -14,6 +14,7 @@ libxl__qmp_message_type = Enumeration("qmp_message_type", [
     ])
 
 libxl__device_kind = Enumeration("device_kind", [
+    (0, "NONE"),
     (1, "VIF"),
     (2, "VBD"),
     (3, "QDISK"),
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 4982b52..b3bac6d 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -20,8 +20,12 @@
 char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array)
 {
     char **kvs;
-    int i, length = array->count;
+    int i, length;
 
+    if (!array)
+        return NULL;
+
+    length = array->count;
     if (!length)
         return NULL;
 
-- 
2.6.6


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

[PATCH v2 3/4] libxl: add HVM usb passthrough support

[Juergen Gross]

Add HVM usb passthrough support to libxl by using qemu's capability
to emulate standard USB controllers.

A USB controller is added via qmp command to the emulated hardware
when a usbctrl device of type DEVICEMODEL is requested. Depending on
the requested speed the appropriate hardware type is selected. A host
USB device can then be added to the emulated USB controller via qmp
command.

Removing of the devices is done via qmp commands, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2: code style issues (Wei Liu)
    adding some assert()s (Wei Liu)
    split out libxl__device_usbctrl_del_xenstore() (Wei Liu)
---
 tools/libxl/libxl_device.c |   3 +-
 tools/libxl/libxl_usb.c    | 397 +++++++++++++++++++++++++++++++++++----------
 tools/libxl/xl_cmdimpl.c   |   4 +-
 3 files changed, 311 insertions(+), 93 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index f53f772..2acfb48 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -808,8 +808,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
                 aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
                 aodev->dev = dev;
                 aodev->force = drs->force;
-                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
-                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
+                if (dev->kind == LIBXL__DEVICE_KIND_VUSB)
                     libxl__initiate_device_usbctrl_remove(egc, aodev);
                 else
                     libxl__initiate_device_generic_remove(egc, aodev);
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 2493464..09bfa24 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -17,6 +17,7 @@
 
 #include "libxl_internal.h"
 #include <inttypes.h>
+#include <xen/io/usbif.h>
 
 #define USBBACK_INFO_PATH "/libxl/usbback"
 
@@ -43,12 +44,6 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
     int rc;
     libxl_domain_type domtype = libxl__domain_type(gc, domid);
 
-    if (!usbctrl->version)
-        usbctrl->version = 2;
-
-    if (!usbctrl->ports)
-        usbctrl->ports = 8;
-
     if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
         if (domtype == LIBXL_DOMAIN_TYPE_PV) {
             rc = usbback_is_loaded(gc);
@@ -62,6 +57,71 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
         }
     }
 
+    switch (usbctrl->type) {
+    case LIBXL_USBCTRL_TYPE_PV:
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        if (!usbctrl->version)
+            usbctrl->version = 2;
+        if (usbctrl->version < 1 || usbctrl->version > 2) {
+            LOG(ERROR,
+                "USB version for paravirtualized devices must be 1 or 2");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        if (!usbctrl->ports)
+            usbctrl->ports = 8;
+        if (usbctrl->ports < 1 || usbctrl->ports > USBIF_MAX_PORTNR) {
+            LOG(ERROR, "Number of ports for USB controller is limited to %u",
+                USBIF_MAX_PORTNR);
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        break;
+    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        if (!usbctrl->version)
+            usbctrl->version = 2;
+        switch (usbctrl->version) {
+        case 1:
+            /* uhci controller in qemu has fixed number of ports. */
+            if (usbctrl->ports && usbctrl->ports != 2) {
+                LOG(ERROR,
+                    "Number of ports for USB controller of version 1 is always 2");
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            usbctrl->ports = 2;
+            break;
+        case 2:
+            /* ehci controller in qemu has fixed number of ports. */
+            if (usbctrl->ports && usbctrl->ports != 6) {
+                LOG(ERROR,
+                    "Number of ports for USB controller of version 2 is always 6");
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            usbctrl->ports = 6;
+            break;
+        case 3:
+            if (!usbctrl->ports)
+                usbctrl->ports = 8;
+            /* xhci controller in qemu supports up to 15 ports. */
+            if (usbctrl->ports > 15) {
+                LOG(ERROR,
+                    "Number of ports for USB controller of version 3 is limited to 15");
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            break;
+        default:
+            LOG(ERROR, "Illegal USB version");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        break;
+    default:
+        break;
+    }
+
     rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
                               &usbctrl->backend_domid);
 
@@ -75,9 +135,20 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
 {
     device->backend_devid   = usbctrl->devid;
     device->backend_domid   = usbctrl->backend_domid;
-    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
-                              ? LIBXL__DEVICE_KIND_VUSB
-                              : LIBXL__DEVICE_KIND_QUSB;
+    switch (usbctrl->type) {
+    case LIBXL_USBCTRL_TYPE_PV:
+        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
+        break;
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
+        break;
+    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
+        break;
+    default:
+        assert(0); /* can't really happen. */
+        break;
+    }
     device->devid           = usbctrl->devid;
     device->domid           = domid;
     device->kind            = LIBXL__DEVICE_KIND_VUSB;
@@ -85,6 +156,35 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+static const char *vusb_be_from_xs_libxl_type(libxl__gc *gc,
+                                              const char *libxl_path,
+                                              libxl_usbctrl_type type)
+{
+    const char *be_path = NULL, *tmp;
+    int r;
+
+    if (type == LIBXL_USBCTRL_TYPE_AUTO) {
+        r = libxl__xs_read_checked(gc, XBT_NULL,
+                                   GCSPRINTF("%s/type", libxl_path), &tmp);
+        if (r || libxl_usbctrl_type_from_string(tmp, &type))
+            goto out;
+    }
+
+    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+        be_path = libxl_path;
+        goto out;
+    }
+
+    r = libxl__xs_read_checked(gc, XBT_NULL,
+                               GCSPRINTF("%s/backend", libxl_path),
+                               &be_path);
+    if (r)
+        be_path = NULL;
+
+out:
+    return be_path;
+}
+
 /* Add usbctrl information to xenstore.
  *
  * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
@@ -96,7 +196,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
                                               bool update_json)
 {
     libxl__device *device;
-    flexarray_t *front;
+    flexarray_t *front = NULL;
     flexarray_t *back;
     xs_transaction_t t = XBT_NULL;
     int i, rc;
@@ -112,13 +212,21 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
     rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
     if (rc) goto out;
 
-    front = flexarray_make(gc, 4, 1);
     back = flexarray_make(gc, 12, 1);
 
-    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
-    flexarray_append_pair(back, "online", "1");
-    flexarray_append_pair(back, "state",
-                          GCSPRINTF("%d", XenbusStateInitialising));
+    if (device->backend_kind != LIBXL__DEVICE_KIND_NONE) {
+        front = flexarray_make(gc, 4, 1);
+
+        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
+        flexarray_append_pair(back, "online", "1");
+        flexarray_append_pair(back, "state",
+                              GCSPRINTF("%d", XenbusStateInitialising));
+        flexarray_append_pair(front, "backend-id",
+                              GCSPRINTF("%d", usbctrl->backend_domid));
+        flexarray_append_pair(front, "state",
+                              GCSPRINTF("%d", XenbusStateInitialising));
+    }
+
     flexarray_append_pair(back, "type",
                           (char *)libxl_usbctrl_type_to_string(usbctrl->type));
     flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", usbctrl->version));
@@ -127,11 +235,6 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
     for (i = 0; i < usbctrl->ports; i++)
         flexarray_append_pair(back, GCSPRINTF("port/%d", i + 1), "");
 
-    flexarray_append_pair(front, "backend-id",
-                          GCSPRINTF("%d", usbctrl->backend_domid));
-    flexarray_append_pair(front, "state",
-                          GCSPRINTF("%d", XenbusStateInitialising));
-
     if (update_json) {
         lock = libxl__lock_domain_userdata(gc, domid);
         if (!lock) {
@@ -196,15 +299,7 @@ out:
 
 static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
 {
-    const char *be_path;
-    int r;
-
-    r = libxl__xs_read_checked(gc, XBT_NULL,
-                               GCSPRINTF("%s/backend", libxl_path),
-                               &be_path);
-    if (r || !be_path) return NULL;
-
-    return be_path;
+    return vusb_be_from_xs_libxl_type(gc, libxl_path, LIBXL_USBCTRL_TYPE_AUTO);
 }
 
 static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
@@ -216,7 +311,7 @@ static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
 
     libxl_path = GCSPRINTF("%s/device/vusb/%d",
                            libxl__xs_libxl_path(gc, domid), usbctrl->devid);
-    be_path = vusb_be_from_xs_libxl(gc, libxl_path);
+    be_path = vusb_be_from_xs_libxl_type(gc, libxl_path, usbctrl->type);
 
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
@@ -247,6 +342,93 @@ static char *pvusb_get_device_type(libxl_usbctrl_type type)
     }
 }
 
+/* Send qmp commands to create a usb controller in qemu.
+ *
+ * Depending on the speed (usbctrl->version) we create:
+ * - piix3-usb-uhci (version=1), always 2 ports
+ * - usb-ehci       (version=2), always 6 ports
+ * - nec-usb-xhci   (version=3), up to 15 ports
+ */
+static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, uint32_t domid,
+                                         libxl_device_usbctrl *usbctrl)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 8, 1);
+
+    switch (usbctrl->version) {
+    case 1:
+        flexarray_append_pair(qmp_args, "driver", "piix3-usb-uhci");
+        break;
+    case 2:
+        flexarray_append_pair(qmp_args, "driver", "usb-ehci");
+        break;
+    case 3:
+        flexarray_append_pair(qmp_args, "driver", "nec-usb-xhci");
+        flexarray_append_pair(qmp_args, "p2", GCSPRINTF("%d", usbctrl->ports));
+        flexarray_append_pair(qmp_args, "p3", GCSPRINTF("%d", usbctrl->ports));
+        break;
+    default:
+        assert(0); /* Should not be possible. */
+        break;
+    }
+
+    flexarray_append_pair(qmp_args, "id",
+                          GCSPRINTF("xenusb-%d", usbctrl->devid));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_add", qmp_args);
+}
+
+/* Send qmp commands to delete a usb controller in qemu.  */
+static int libxl__device_usbctrl_del_hvm(libxl__gc *gc, uint32_t domid,
+                                         int devid)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 2, 1);
+    flexarray_append_pair(qmp_args, "id", GCSPRINTF("xenusb-%d", devid));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_del", qmp_args);
+}
+
+/* Send qmp commands to create a usb device in qemu. */
+static int libxl__device_usbdev_add_hvm(libxl__gc *gc, uint32_t domid,
+                                        libxl_device_usbdev *usbdev)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 12, 1);
+    flexarray_append_pair(qmp_args, "id",
+                          GCSPRINTF("xenusb-%d-%d",
+                                    usbdev->u.hostdev.hostbus,
+                                    usbdev->u.hostdev.hostaddr));
+    flexarray_append_pair(qmp_args, "driver", "usb-host");
+    flexarray_append_pair(qmp_args, "bus",
+                          GCSPRINTF("xenusb-%d.0", usbdev->ctrl));
+    flexarray_append_pair(qmp_args, "port", GCSPRINTF("%d", usbdev->port));
+    flexarray_append_pair(qmp_args, "hostbus",
+                          GCSPRINTF("%d", usbdev->u.hostdev.hostbus));
+    flexarray_append_pair(qmp_args, "hostaddr",
+                          GCSPRINTF("%d", usbdev->u.hostdev.hostaddr));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_add", qmp_args);
+}
+
+/* Send qmp commands to delete a usb device in qemu. */
+static int libxl__device_usbdev_del_hvm(libxl__gc *gc, uint32_t domid,
+                                        libxl_device_usbdev *usbdev)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 2, 1);
+    flexarray_append_pair(qmp_args, "id",
+                          GCSPRINTF("xenusb-%d-%d",
+                                    usbdev->u.hostdev.hostbus,
+                                    usbdev->u.hostdev.hostaddr));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_del", qmp_args);
+}
+
 /* AO operation to add a usb controller.
  *
  * Generally, it does:
@@ -278,13 +460,6 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
         }
     }
 
-    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
-        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
-        LOG(ERROR, "Unsupported USB controller type");
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
     rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
                                             aodev->update_json);
     if (rc) goto out;
@@ -293,6 +468,12 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
     rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
     if (rc) goto outrm;
 
+    if (device->backend_kind == LIBXL__DEVICE_KIND_NONE) {
+        rc = libxl__device_usbctrl_add_hvm(gc, domid, usbctrl);
+        if (rc) goto outrm;
+        goto out;
+    }
+
     aodev->dev = device;
     aodev->action = LIBXL__DEVICE_ACTION_ADD;
     libxl__wait_device_connection(egc, aodev);
@@ -347,13 +528,6 @@ void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
     rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
     if (rc) goto out;
 
-    if (usbctrlinfo.type != LIBXL_USBCTRL_TYPE_PV &&
-        usbctrlinfo.type != LIBXL_USBCTRL_TYPE_QUSB) {
-        LOG(ERROR, "Unsupported USB controller type");
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
     /* Remove usb devices first */
     rc = libxl__device_usbdev_list_for_usbctrl(gc, domid, usbctrl_devid,
                                                &usbdevs, &num_usbdev);
@@ -368,6 +542,13 @@ void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
         }
     }
 
+    if (usbctrlinfo.type == LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+        rc = libxl__device_usbctrl_del_hvm(gc, domid, usbctrl_devid);
+        if (!rc)
+            libxl__device_usbctrl_del_xenstore(gc, domid, &usbctrl);
+        goto out;
+    }
+
     libxl_device_usbctrl_dispose(&usbctrl);
     libxl_usbctrlinfo_dispose(&usbctrlinfo);
 
@@ -428,15 +609,20 @@ libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
     })
 
             libxl_path = GCSPRINTF("%s/%s", libxl_vusbs_path, *entry);
-            be_path = READ_SUBPATH(libxl_path, "backend");
-            if (!be_path) goto out;
-            ret = libxl__backendpath_parse_domid(gc, be_path,
-                                                &usbctrl->backend_domid);
+            libxl_usbctrl_type_from_string(READ_SUBPATH(libxl_path, "type"),
+                                           &usbctrl->type);
+            if (usbctrl->type == LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+                be_path = libxl_path;
+                ret = libxl__get_domid(gc, &usbctrl->backend_domid);
+            } else {
+                be_path = READ_SUBPATH(libxl_path, "backend");
+                if (!be_path) goto out;
+                ret = libxl__backendpath_parse_domid(gc, be_path,
+                                                     &usbctrl->backend_domid);
+            }
             if (ret) goto out;
             usbctrl->version = READ_SUBPATH_INT(be_path, "usb-ver");
             usbctrl->ports = READ_SUBPATH_INT(be_path, "num-ports");
-            libxl_usbctrl_type_from_string(READ_SUBPATH(libxl_path, "type"),
-                                           &usbctrl->type);
 
 #undef READ_SUBPATH
 #undef READ_SUBPATH_INT
@@ -481,24 +667,33 @@ int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
         tmp ? atoi(tmp) : -1;                                           \
     })
 
-    dompath = libxl__xs_get_dompath(gc, domid);
-    fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, usbctrl->devid);
     libxl_dom_path = libxl__xs_libxl_path(gc, domid);
     libxl_path = GCSPRINTF("%s/device/vusb/%d", libxl_dom_path, usbctrl->devid);
-    be_path = READ_SUBPATH(libxl_path, "backend");
-    usbctrlinfo->backend = libxl__strdup(NOGC, be_path);
-    rc = libxl__backendpath_parse_domid(gc, be_path, &usbctrl->backend_domid);
-    if (rc) goto out;
-    usbctrlinfo->state = READ_SUBPATH_INT(fe_path, "state");
-    usbctrlinfo->evtch = READ_SUBPATH_INT(fe_path, "event-channel");
-    usbctrlinfo->ref_urb = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
-    usbctrlinfo->ref_conn = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
-    usbctrlinfo->frontend = libxl__strdup(NOGC, fe_path);
-    usbctrlinfo->frontend_id = domid;
-    usbctrlinfo->ports = READ_SUBPATH_INT(be_path, "num-ports");
-    usbctrlinfo->version = READ_SUBPATH_INT(be_path, "usb-ver");;
-    tmp = READ_SUBPATH(libxl_path, "type");
-    libxl_usbctrl_type_from_string(tmp, &usbctrlinfo->type);
+    libxl_usbctrl_type_from_string(READ_SUBPATH(libxl_path, "type"),
+                                   &usbctrlinfo->type);
+
+    if (usbctrlinfo->type != LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+        dompath = libxl__xs_get_dompath(gc, domid);
+        fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, usbctrl->devid);
+        be_path = READ_SUBPATH(libxl_path, "backend");
+        usbctrlinfo->backend = libxl__strdup(NOGC, be_path);
+        rc = libxl__backendpath_parse_domid(gc, be_path,
+                                            &usbctrl->backend_domid);
+        if (rc) goto out;
+        usbctrlinfo->state = READ_SUBPATH_INT(fe_path, "state");
+        usbctrlinfo->evtch = READ_SUBPATH_INT(fe_path, "event-channel");
+        usbctrlinfo->ref_urb = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
+        usbctrlinfo->ref_conn = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
+        usbctrlinfo->frontend = libxl__strdup(NOGC, fe_path);
+        usbctrlinfo->frontend_id = domid;
+        usbctrlinfo->ports = READ_SUBPATH_INT(be_path, "num-ports");
+        usbctrlinfo->version = READ_SUBPATH_INT(be_path, "usb-ver");
+    } else {
+        usbctrlinfo->ports = READ_SUBPATH_INT(libxl_path, "num-ports");
+        usbctrlinfo->version = READ_SUBPATH_INT(libxl_path, "usb-ver");
+        rc = libxl__get_domid(gc, &usbctrl->backend_domid);
+        if (rc) goto out;
+    }
 
 #undef READ_SUBPATH
 #undef READ_SUBPATH_INT
@@ -790,6 +985,21 @@ libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t domid, int *num)
     return usbdevs;
 }
 
+static char *pvusb_get_port_path(libxl__gc *gc, uint32_t domid,
+                                 libxl_usbctrl_type type, int ctrl, int port)
+{
+    char *path;
+
+    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
+        path = GCSPRINTF("%s/device/vusb", libxl__xs_libxl_path(gc, domid));
+    else
+        path = GCSPRINTF("%s/backend/%s/%d",
+                         libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+                         pvusb_get_device_type(type), domid);
+
+    return GCSPRINTF("%s/%d/port/%d", path, ctrl, port);
+}
+
 /* find first unused controller:port and give that to usb device */
 static int
 libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
@@ -809,10 +1019,8 @@ libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
         for (j = 0; j < usbctrls[i].ports; j++) {
             const char *path, *tmp;
 
-            path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                             libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                             pvusb_get_device_type(usbctrls[i].type),
-                             domid, usbctrls[i].devid, j + 1);
+            path = pvusb_get_port_path(gc, domid, usbctrls[i].type,
+                                       usbctrls[i].devid, j + 1);
             rc = libxl__xs_read_checked(gc, XBT_NULL, path, &tmp);
             if (rc) goto out;
 
@@ -881,13 +1089,6 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
                 }
             }
 
-            if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
-                usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
-                LOG(ERROR, "Unsupported USB controller type");
-                rc = ERROR_FAIL;
-                goto out;
-            }
-
             rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
                                                     update_json);
             if (rc) goto out;
@@ -1013,10 +1214,8 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
             if (rc) goto out;
         }
 
-        be_path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                            libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                            pvusb_get_device_type(type),
-                            domid, usbdev->ctrl, usbdev->port);
+        be_path = pvusb_get_port_path(gc, domid, type, usbdev->ctrl,
+                                      usbdev->port);
 
         LOG(DEBUG, "Adding usb device %s to xenstore: controller %d, port %d",
             busid, usbdev->ctrl, usbdev->port);
@@ -1044,10 +1243,7 @@ static int libxl__device_usbdev_remove_xenstore(libxl__gc *gc, uint32_t domid,
 {
     char *be_path;
 
-    be_path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                        libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                        pvusb_get_device_type(type),
-                        domid, usbdev->ctrl, usbdev->port);
+    be_path = pvusb_get_port_path(gc, domid, type, usbdev->ctrl, usbdev->port);
 
     LOG(DEBUG, "Removing usb device from xenstore: controller %d, port %d",
         usbdev->ctrl, usbdev->port);
@@ -1060,10 +1256,8 @@ static char *usbdev_busid_from_ctrlport(libxl__gc *gc, uint32_t domid,
                                         libxl_usbctrl_type type)
 {
     return libxl__xs_read(gc, XBT_NULL,
-                          GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                              libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                              pvusb_get_device_type(type),
-                              domid, usbdev->ctrl, usbdev->port));
+                          pvusb_get_port_path(gc, domid, type, usbdev->ctrl,
+                                              usbdev->port));
 }
 
 /* get original driver path of usb interface, stored in @drvpath */
@@ -1427,6 +1621,18 @@ static int do_usbdev_add(libxl__gc *gc, uint32_t domid,
 
         break;
     case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        rc = libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
+                                               LIBXL_USBCTRL_TYPE_DEVICEMODEL,
+                                               update_json);
+        if (rc) goto out;
+
+        rc = libxl__device_usbdev_add_hvm(gc, domid, usbdev);
+        if (rc) {
+            libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                             LIBXL_USBCTRL_TYPE_DEVICEMODEL);
+            goto out;
+        }
+        break;
     default:
         LOG(ERROR, "Unsupported usb controller type");
         rc = ERROR_FAIL;
@@ -1594,6 +1800,19 @@ static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
 
         break;
     case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                              LIBXL_USBCTRL_TYPE_DEVICEMODEL);
+        if (rc) goto out;
+
+        rc = libxl__device_usbdev_del_hvm(gc, domid, usbdev);
+        if (rc) {
+            libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
+                                              LIBXL_USBCTRL_TYPE_DEVICEMODEL,
+                                              false);
+            goto out;
+        }
+
+        break;
     default:
         LOG(ERROR, "Unsupported usb controller type");
         rc = ERROR_FAIL;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 62237d0..cb43c00 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3728,14 +3728,14 @@ int main_usblist(int argc, char **argv)
     }
 
     for (i = 0; i < numctrl; ++i) {
-        printf("%-6s %-6s %-3s %-5s %-7s %-5s\n",
+        printf("%-6s %-12s %-3s %-5s %-7s %-5s\n",
                 "Devid", "Type", "BE", "state", "usb-ver", "ports");
 
         libxl_usbctrlinfo_init(&usbctrlinfo);
 
         if (!libxl_device_usbctrl_getinfo(ctx, domid,
                                 &usbctrls[i], &usbctrlinfo)) {
-            printf("%-6d %-6s %-3d %-5d %-7d %-5d\n",
+            printf("%-6d %-12s %-3d %-5d %-7d %-5d\n",
                     usbctrlinfo.devid,
                     libxl_usbctrl_type_to_string(usbctrlinfo.type),
                     usbctrlinfo.backend_id, usbctrlinfo.state,
-- 
2.6.6


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

[PATCH v2 4/4] docs: add HVM USB passthrough documentation

[Juergen Gross]

Update the man page regarding passthrough of USB devices to HVM
domains via qemu USB emulation.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 docs/man/xl.cfg.pod.5.in | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 77a1be3..076b2a6 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -745,19 +745,25 @@ Specifies the usb controller type.
 
 "qusb" specifies a qemu base backend for pvusb.
 
+"devicemodel" specifies a USB controller emulated by qemu. It will show up as
+a PCI-device in the guest.
+
 "auto" (the default) determines whether a kernel based backend is installed.
 If this is the case, "pv" is selected, "qusb" will be selected if no kernel
-backend is currently available.
+backend is currently available. For HVM domains "devicemodel" is being selected.
 
 =item B<version=VERSION>
 
 Specifies the usb controller version.  Possible values include
-1 (USB1.1) and 2 (USB2.0). Default is 2 (USB2.0).
+1 (USB1.1), 2 (USB2.0) and 3 (USB3.0). Default is 2 (USB2.0). 3 (USB3.0) is
+available for the type "devicemodel" only.
 
 =item B<ports=PORTS>
 
 Specifies the total ports of the usb controller. The maximum
-number is 31. Default is 8.
+number is 31. Default is 8. With the type "devicemodel" the number of ports
+is more limited: a USB1.1 controller always has 2 ports, a USB2.0 controller
+always has 6 ports and a USB3.0 controller can have up to 15 ports.
 
 USB controller ids start from 0.  In line with the USB spec, however,
 ports on a controller start from 1.
-- 
2.6.6


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

Re: [PATCH v2 1/4] libxl: add function to remove usb controller xenstore entries

[Wei Liu]

On Mon, Sep 19, 2016 at 02:40:07PM +0200, Juergen Gross wrote:
> In case of failure when trying to add a new USB controller to a domain
> libxl might leak xenstore entries. Add a function to remove them and
> call this function in case of failure.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

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

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

Re: [PATCH v2 3/4] libxl: add HVM usb passthrough support

[Wei Liu]

On Mon, Sep 19, 2016 at 02:40:09PM +0200, Juergen Gross wrote:
> Add HVM usb passthrough support to libxl by using qemu's capability
> to emulate standard USB controllers.
> 
> A USB controller is added via qmp command to the emulated hardware
> when a usbctrl device of type DEVICEMODEL is requested. Depending on
> the requested speed the appropriate hardware type is selected. A host
> USB device can then be added to the emulated USB controller via qmp
> command.
> 
> Removing of the devices is done via qmp commands, too.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

AIUI the design has been hashed out in the past two years. The approach
taken looks sensible to me.

Code-wise it also looks good:

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

I will give Ian and George a chance to look at this series, in case I
misunderstand something about the design.

Wei.

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

Re: [PATCH v2 3/4] libxl: add HVM usb passthrough support

[George Dunlap]

On 19/09/16 13:40, Juergen Gross wrote:
> Add HVM usb passthrough support to libxl by using qemu's capability
> to emulate standard USB controllers.
> 
> A USB controller is added via qmp command to the emulated hardware
> when a usbctrl device of type DEVICEMODEL is requested. Depending on
> the requested speed the appropriate hardware type is selected. A host
> USB device can then be added to the emulated USB controller via qmp
> command.
> 
> Removing of the devices is done via qmp commands, too.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Thanks Juergen!  That was a pretty great turn-around time. :-)

Overall everything looks reasonable, with just a few nits...

> ---
> V2: code style issues (Wei Liu)
>     adding some assert()s (Wei Liu)
>     split out libxl__device_usbctrl_del_xenstore() (Wei Liu)
> ---
>  tools/libxl/libxl_device.c |   3 +-
>  tools/libxl/libxl_usb.c    | 397 +++++++++++++++++++++++++++++++++++----------
>  tools/libxl/xl_cmdimpl.c   |   4 +-
>  3 files changed, 311 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index f53f772..2acfb48 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -808,8 +808,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
>                  aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
>                  aodev->dev = dev;
>                  aodev->force = drs->force;
> -                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
> -                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
> +                if (dev->kind == LIBXL__DEVICE_KIND_VUSB)
>                      libxl__initiate_device_usbctrl_remove(egc, aodev);
>                  else
>                      libxl__initiate_device_generic_remove(egc, aodev);
> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
> index 2493464..09bfa24 100644
> --- a/tools/libxl/libxl_usb.c
> +++ b/tools/libxl/libxl_usb.c
> @@ -17,6 +17,7 @@
>  
>  #include "libxl_internal.h"
>  #include <inttypes.h>
> +#include <xen/io/usbif.h>
>  
>  #define USBBACK_INFO_PATH "/libxl/usbback"
>  
> @@ -43,12 +44,6 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>      int rc;
>      libxl_domain_type domtype = libxl__domain_type(gc, domid);
>  
> -    if (!usbctrl->version)
> -        usbctrl->version = 2;
> -
> -    if (!usbctrl->ports)
> -        usbctrl->ports = 8;
> -
>      if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
>          if (domtype == LIBXL_DOMAIN_TYPE_PV) {
>              rc = usbback_is_loaded(gc);
> @@ -62,6 +57,71 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>          }
>      }
>  
> +    switch (usbctrl->type) {
> +    case LIBXL_USBCTRL_TYPE_PV:
> +    case LIBXL_USBCTRL_TYPE_QUSB:
> +        if (!usbctrl->version)
> +            usbctrl->version = 2;
> +        if (usbctrl->version < 1 || usbctrl->version > 2) {
> +            LOG(ERROR,
> +                "USB version for paravirtualized devices must be 1 or 2");
> +            rc = ERROR_INVAL;
> +            goto out;
> +        }
> +        if (!usbctrl->ports)
> +            usbctrl->ports = 8;
> +        if (usbctrl->ports < 1 || usbctrl->ports > USBIF_MAX_PORTNR) {
> +            LOG(ERROR, "Number of ports for USB controller is limited to %u",
> +                USBIF_MAX_PORTNR);
> +            rc = ERROR_INVAL;
> +            goto out;
> +        }
> +        break;
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        if (!usbctrl->version)
> +            usbctrl->version = 2;
> +        switch (usbctrl->version) {
> +        case 1:
> +            /* uhci controller in qemu has fixed number of ports. */
> +            if (usbctrl->ports && usbctrl->ports != 2) {
> +                LOG(ERROR,
> +                    "Number of ports for USB controller of version 1 is always 2");
> +                rc = ERROR_INVAL;
> +                goto out;
> +            }
> +            usbctrl->ports = 2;
> +            break;
> +        case 2:
> +            /* ehci controller in qemu has fixed number of ports. */
> +            if (usbctrl->ports && usbctrl->ports != 6) {
> +                LOG(ERROR,
> +                    "Number of ports for USB controller of version 2 is always 6");
> +                rc = ERROR_INVAL;
> +                goto out;
> +            }
> +            usbctrl->ports = 6;
> +            break;
> +        case 3:
> +            if (!usbctrl->ports)
> +                usbctrl->ports = 8;
> +            /* xhci controller in qemu supports up to 15 ports. */
> +            if (usbctrl->ports > 15) {
> +                LOG(ERROR,
> +                    "Number of ports for USB controller of version 3 is limited to 15");
> +                rc = ERROR_INVAL;
> +                goto out;
> +            }
> +            break;
> +        default:
> +            LOG(ERROR, "Illegal USB version");
> +            rc = ERROR_INVAL;
> +            goto out;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +
>      rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
>                                &usbctrl->backend_domid);
>  
> @@ -75,9 +135,20 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>  {
>      device->backend_devid   = usbctrl->devid;
>      device->backend_domid   = usbctrl->backend_domid;
> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
> -                              ? LIBXL__DEVICE_KIND_VUSB
> -                              : LIBXL__DEVICE_KIND_QUSB;
> +    switch (usbctrl->type) {
> +    case LIBXL_USBCTRL_TYPE_PV:
> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
> +        break;
> +    case LIBXL_USBCTRL_TYPE_QUSB:
> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
> +        break;
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
> +        break;
> +    default:
> +        assert(0); /* can't really happen. */
> +        break;
> +    }
>      device->devid           = usbctrl->devid;
>      device->domid           = domid;
>      device->kind            = LIBXL__DEVICE_KIND_VUSB;
> @@ -85,6 +156,35 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>      return 0;
>  }
>  
> +static const char *vusb_be_from_xs_libxl_type(libxl__gc *gc,
> +                                              const char *libxl_path,
> +                                              libxl_usbctrl_type type)
> +{
> +    const char *be_path = NULL, *tmp;
> +    int r;
> +
> +    if (type == LIBXL_USBCTRL_TYPE_AUTO) {
> +        r = libxl__xs_read_checked(gc, XBT_NULL,
> +                                   GCSPRINTF("%s/type", libxl_path), &tmp);
> +        if (r || libxl_usbctrl_type_from_string(tmp, &type))
> +            goto out;
> +    }
> +
> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
> +        be_path = libxl_path;
> +        goto out;
> +    }
> +
> +    r = libxl__xs_read_checked(gc, XBT_NULL,
> +                               GCSPRINTF("%s/backend", libxl_path),
> +                               &be_path);
> +    if (r)
> +        be_path = NULL;
> +
> +out:
> +    return be_path;
> +}
> +
>  /* Add usbctrl information to xenstore.
>   *
>   * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
> @@ -96,7 +196,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>                                                bool update_json)
>  {
>      libxl__device *device;
> -    flexarray_t *front;
> +    flexarray_t *front = NULL;
>      flexarray_t *back;
>      xs_transaction_t t = XBT_NULL;
>      int i, rc;
> @@ -112,13 +212,21 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>      if (rc) goto out;
>  
> -    front = flexarray_make(gc, 4, 1);
>      back = flexarray_make(gc, 12, 1);
>  
> -    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> -    flexarray_append_pair(back, "online", "1");
> -    flexarray_append_pair(back, "state",
> -                          GCSPRINTF("%d", XenbusStateInitialising));
> +    if (device->backend_kind != LIBXL__DEVICE_KIND_NONE) {
> +        front = flexarray_make(gc, 4, 1);
> +
> +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> +        flexarray_append_pair(back, "online", "1");
> +        flexarray_append_pair(back, "state",
> +                              GCSPRINTF("%d", XenbusStateInitialising));
> +        flexarray_append_pair(front, "backend-id",
> +                              GCSPRINTF("%d", usbctrl->backend_domid));
> +        flexarray_append_pair(front, "state",
> +                              GCSPRINTF("%d", XenbusStateInitialising));
> +    }
> +
>      flexarray_append_pair(back, "type",
>                            (char *)libxl_usbctrl_type_to_string(usbctrl->type));
>      flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", usbctrl->version));
> @@ -127,11 +235,6 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>      for (i = 0; i < usbctrl->ports; i++)
>          flexarray_append_pair(back, GCSPRINTF("port/%d", i + 1), "");
>  
> -    flexarray_append_pair(front, "backend-id",
> -                          GCSPRINTF("%d", usbctrl->backend_domid));
> -    flexarray_append_pair(front, "state",
> -                          GCSPRINTF("%d", XenbusStateInitialising));
> -
>      if (update_json) {
>          lock = libxl__lock_domain_userdata(gc, domid);
>          if (!lock) {
> @@ -196,15 +299,7 @@ out:
>  
>  static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
>  {
> -    const char *be_path;
> -    int r;
> -
> -    r = libxl__xs_read_checked(gc, XBT_NULL,
> -                               GCSPRINTF("%s/backend", libxl_path),
> -                               &be_path);
> -    if (r || !be_path) return NULL;
> -
> -    return be_path;
> +    return vusb_be_from_xs_libxl_type(gc, libxl_path, LIBXL_USBCTRL_TYPE_AUTO);
>  }
>  
>  static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
> @@ -216,7 +311,7 @@ static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
>  
>      libxl_path = GCSPRINTF("%s/device/vusb/%d",
>                             libxl__xs_libxl_path(gc, domid), usbctrl->devid);
> -    be_path = vusb_be_from_xs_libxl(gc, libxl_path);
> +    be_path = vusb_be_from_xs_libxl_type(gc, libxl_path, usbctrl->type);
>  
>      for (;;) {
>          rc = libxl__xs_transaction_start(gc, &t);
> @@ -247,6 +342,93 @@ static char *pvusb_get_device_type(libxl_usbctrl_type type)
>      }
>  }
>  
> +/* Send qmp commands to create a usb controller in qemu.
> + *
> + * Depending on the speed (usbctrl->version) we create:
> + * - piix3-usb-uhci (version=1), always 2 ports
> + * - usb-ehci       (version=2), always 6 ports
> + * - nec-usb-xhci   (version=3), up to 15 ports
> + */
> +static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, uint32_t domid,
> +                                         libxl_device_usbctrl *usbctrl)
> +{
> +    flexarray_t *qmp_args;
> +
> +    qmp_args = flexarray_make(gc, 8, 1);
> +
> +    switch (usbctrl->version) {
> +    case 1:
> +        flexarray_append_pair(qmp_args, "driver", "piix3-usb-uhci");
> +        break;
> +    case 2:
> +        flexarray_append_pair(qmp_args, "driver", "usb-ehci");
> +        break;
> +    case 3:
> +        flexarray_append_pair(qmp_args, "driver", "nec-usb-xhci");
> +        flexarray_append_pair(qmp_args, "p2", GCSPRINTF("%d", usbctrl->ports));
> +        flexarray_append_pair(qmp_args, "p3", GCSPRINTF("%d", usbctrl->ports));
> +        break;
> +    default:
> +        assert(0); /* Should not be possible. */
> +        break;
> +    }
> +
> +    flexarray_append_pair(qmp_args, "id",
> +                          GCSPRINTF("xenusb-%d", usbctrl->devid));
> +
> +    return libxl__qmp_run_command_flexarray(gc, domid, "device_add", qmp_args);
> +}
> +
> +/* Send qmp commands to delete a usb controller in qemu.  */
> +static int libxl__device_usbctrl_del_hvm(libxl__gc *gc, uint32_t domid,
> +                                         int devid)
> +{
> +    flexarray_t *qmp_args;
> +
> +    qmp_args = flexarray_make(gc, 2, 1);
> +    flexarray_append_pair(qmp_args, "id", GCSPRINTF("xenusb-%d", devid));
> +
> +    return libxl__qmp_run_command_flexarray(gc, domid, "device_del", qmp_args);
> +}
> +
> +/* Send qmp commands to create a usb device in qemu. */
> +static int libxl__device_usbdev_add_hvm(libxl__gc *gc, uint32_t domid,
> +                                        libxl_device_usbdev *usbdev)
> +{
> +    flexarray_t *qmp_args;
> +
> +    qmp_args = flexarray_make(gc, 12, 1);
> +    flexarray_append_pair(qmp_args, "id",
> +                          GCSPRINTF("xenusb-%d-%d",
> +                                    usbdev->u.hostdev.hostbus,
> +                                    usbdev->u.hostdev.hostaddr));
> +    flexarray_append_pair(qmp_args, "driver", "usb-host");
> +    flexarray_append_pair(qmp_args, "bus",
> +                          GCSPRINTF("xenusb-%d.0", usbdev->ctrl));
> +    flexarray_append_pair(qmp_args, "port", GCSPRINTF("%d", usbdev->port));
> +    flexarray_append_pair(qmp_args, "hostbus",
> +                          GCSPRINTF("%d", usbdev->u.hostdev.hostbus));
> +    flexarray_append_pair(qmp_args, "hostaddr",
> +                          GCSPRINTF("%d", usbdev->u.hostdev.hostaddr));
> +
> +    return libxl__qmp_run_command_flexarray(gc, domid, "device_add", qmp_args);
> +}
> +
> +/* Send qmp commands to delete a usb device in qemu. */
> +static int libxl__device_usbdev_del_hvm(libxl__gc *gc, uint32_t domid,
> +                                        libxl_device_usbdev *usbdev)
> +{
> +    flexarray_t *qmp_args;
> +
> +    qmp_args = flexarray_make(gc, 2, 1);
> +    flexarray_append_pair(qmp_args, "id",
> +                          GCSPRINTF("xenusb-%d-%d",
> +                                    usbdev->u.hostdev.hostbus,
> +                                    usbdev->u.hostdev.hostaddr));
> +
> +    return libxl__qmp_run_command_flexarray(gc, domid, "device_del", qmp_args);
> +}
> +
>  /* AO operation to add a usb controller.
>   *
>   * Generally, it does:
> @@ -278,13 +460,6 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>          }
>      }
>  
> -    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
> -        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
> -        LOG(ERROR, "Unsupported USB controller type");
> -        rc = ERROR_FAIL;
> -        goto out;
> -    }
> -
>      rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
>                                              aodev->update_json);
>      if (rc) goto out;
> @@ -293,6 +468,12 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>      if (rc) goto outrm;
>  
> +    if (device->backend_kind == LIBXL__DEVICE_KIND_NONE) {
> +        rc = libxl__device_usbctrl_add_hvm(gc, domid, usbctrl);
> +        if (rc) goto outrm;
> +        goto out;
> +    }

This is sort of minor, but this seems like the wrong conditional.  Is
there a reason you did't check for usbctrl->type == HVM here (as I think
you did on the usbdev_add path)?

> +static char *pvusb_get_port_path(libxl__gc *gc, uint32_t domid,
> +                                 libxl_usbctrl_type type, int ctrl, int port)
> +{
> +    char *path;
> +
> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
> +        path = GCSPRINTF("%s/device/vusb", libxl__xs_libxl_path(gc, domid));
> +    else
> +        path = GCSPRINTF("%s/backend/%s/%d",
> +                         libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
> +                         pvusb_get_device_type(type), domid);
> +
> +    return GCSPRINTF("%s/%d/port/%d", path, ctrl, port);
> +}

Should this function be named "vusb_get..." rather than "pvusb_get...""?
 After all, it returns the port even for non-PV usb ports.

I think that's all I had on this at a quick pass-through.

The one other thing to bring up is how this new emulated usb interface
in the config file (usbctrl and usbdev) will interact with the old
emulated usb interface (usb, usbversion, and usbdevice).  If we enable a
reasonable set of emulated-only devices (the tablet in particular), I
think we should deprecate the old interfaces (though continue supporting
them for backwards compatibility, of course).  Thoughts?

 -George




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

Re: [PATCH v2 3/4] libxl: add HVM usb passthrough support

[Juergen Gross]

On 19/09/16 17:31, George Dunlap wrote:
> On 19/09/16 13:40, Juergen Gross wrote:
>> Add HVM usb passthrough support to libxl by using qemu's capability
>> to emulate standard USB controllers.
>>
>> A USB controller is added via qmp command to the emulated hardware
>> when a usbctrl device of type DEVICEMODEL is requested. Depending on
>> the requested speed the appropriate hardware type is selected. A host
>> USB device can then be added to the emulated USB controller via qmp
>> command.
>>
>> Removing of the devices is done via qmp commands, too.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Thanks Juergen!  That was a pretty great turn-around time. :-)
> 
> Overall everything looks reasonable, with just a few nits...
> 
>> @@ -278,13 +460,6 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>>          }
>>      }
>>  
>> -    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
>> -        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
>> -        LOG(ERROR, "Unsupported USB controller type");
>> -        rc = ERROR_FAIL;
>> -        goto out;
>> -    }
>> -
>>      rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
>>                                              aodev->update_json);
>>      if (rc) goto out;
>> @@ -293,6 +468,12 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>>      if (rc) goto outrm;
>>  
>> +    if (device->backend_kind == LIBXL__DEVICE_KIND_NONE) {
>> +        rc = libxl__device_usbctrl_add_hvm(gc, domid, usbctrl);
>> +        if (rc) goto outrm;
>> +        goto out;
>> +    }
> 
> This is sort of minor, but this seems like the wrong conditional.  Is
> there a reason you did't check for usbctrl->type == HVM here (as I think
> you did on the usbdev_add path)?

No, I don't think so. I don't want to wait for the backend to
connect in case it isn't even there, so the conditional seems to
be just the right one.

In the usbdev_add path I have to take different measures for each
of the possible three controller types (visb, qusb or devicemodel),
so doing a switch over usbctrl->type seems to be the natural choice.

> 
>> +static char *pvusb_get_port_path(libxl__gc *gc, uint32_t domid,
>> +                                 libxl_usbctrl_type type, int ctrl, int port)
>> +{
>> +    char *path;
>> +
>> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
>> +        path = GCSPRINTF("%s/device/vusb", libxl__xs_libxl_path(gc, domid));
>> +    else
>> +        path = GCSPRINTF("%s/backend/%s/%d",
>> +                         libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
>> +                         pvusb_get_device_type(type), domid);
>> +
>> +    return GCSPRINTF("%s/%d/port/%d", path, ctrl, port);
>> +}
> 
> Should this function be named "vusb_get..." rather than "pvusb_get...""?
>  After all, it returns the port even for non-PV usb ports.

Yes, you are right.

> I think that's all I had on this at a quick pass-through.
> 
> The one other thing to bring up is how this new emulated usb interface
> in the config file (usbctrl and usbdev) will interact with the old
> emulated usb interface (usb, usbversion, and usbdevice).  If we enable a
> reasonable set of emulated-only devices (the tablet in particular), I
> think we should deprecate the old interfaces (though continue supporting
> them for backwards compatibility, of course).  Thoughts?

The old interface is available for domain config only, right?

I think we should try to convert this stuff to the new interface in
order to be able to use it for a running domain, too. But I think this
will be a 4.9 topic.


Juergen

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

Re: [PATCH v2 3/4] libxl: add HVM usb passthrough support

[George Dunlap]

On Tue, Sep 20, 2016 at 7:17 AM, Juergen Gross <jgross@suse.com> wrote:
> On 19/09/16 17:31, George Dunlap wrote:
>> On 19/09/16 13:40, Juergen Gross wrote:
>>> Add HVM usb passthrough support to libxl by using qemu's capability
>>> to emulate standard USB controllers.
>>>
>>> A USB controller is added via qmp command to the emulated hardware
>>> when a usbctrl device of type DEVICEMODEL is requested. Depending on
>>> the requested speed the appropriate hardware type is selected. A host
>>> USB device can then be added to the emulated USB controller via qmp
>>> command.
>>>
>>> Removing of the devices is done via qmp commands, too.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> Thanks Juergen!  That was a pretty great turn-around time. :-)
>>
>> Overall everything looks reasonable, with just a few nits...
>>
>>> @@ -278,13 +460,6 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>>>          }
>>>      }
>>>
>>> -    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
>>> -        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
>>> -        LOG(ERROR, "Unsupported USB controller type");
>>> -        rc = ERROR_FAIL;
>>> -        goto out;
>>> -    }
>>> -
>>>      rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
>>>                                              aodev->update_json);
>>>      if (rc) goto out;
>>> @@ -293,6 +468,12 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>>>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>>>      if (rc) goto outrm;
>>>
>>> +    if (device->backend_kind == LIBXL__DEVICE_KIND_NONE) {
>>> +        rc = libxl__device_usbctrl_add_hvm(gc, domid, usbctrl);
>>> +        if (rc) goto outrm;
>>> +        goto out;
>>> +    }
>>
>> This is sort of minor, but this seems like the wrong conditional.  Is
>> there a reason you did't check for usbctrl->type == HVM here (as I think
>> you did on the usbdev_add path)?
>
> No, I don't think so. I don't want to wait for the backend to
> connect in case it isn't even there, so the conditional seems to
> be just the right one.

Right, so there are two things this path is doing:
1. Not waiting for the backend because there is no backend
2. Sending qmp commands to add the usb controller because this is a
devicemodel-based controller

As it happens at the moment, "No backend" <=> "DM-based controller".
The conditional you've chosen assumes that "No backend" => "DM-based
controller".  It seems to me that a more natural (and less likely to
become untrue) assumption is "DM-based controller" => "No backend".

Alternately, if we really wanted to be strict, we could have two
conditionals -- one for usbctrl_add_hvm(), and one for skipping
waiting for the backend. :-)

Anyway, I continue think checking for DM is a bit more natural here,
but it's fairly minor, so I won't press the point anymore.

>> The one other thing to bring up is how this new emulated usb interface
>> in the config file (usbctrl and usbdev) will interact with the old
>> emulated usb interface (usb, usbversion, and usbdevice).  If we enable a
>> reasonable set of emulated-only devices (the tablet in particular), I
>> think we should deprecate the old interfaces (though continue supporting
>> them for backwards compatibility, of course).  Thoughts?
>
> The old interface is available for domain config only, right?

Yes, that's right.

> I think we should try to convert this stuff to the new interface in
> order to be able to use it for a running domain, too. But I think this
> will be a 4.9 topic.

The issue with that is that the old interface passes strings straight
through to qemu; so if the user knows the magic runes to get an
arbitrary device, they can get it.  In the new interface, we
explicitly encode specific devices.  Since we don't plan (I don't
think) on duplicating all of qemu's funcitonality, implementing the
old interface with the new one would mean losing functionality for
some people.  Deprecating the old interface allows existing configs to
work while pointing people to the newer, more consistent (and more
functional) interface.

But in any case, we can't deprecate the old interface until we at
least have a USB tablet in the new interface; so this is certainly a
4.9 topic.  I just wanted to raise it before I forgot about it.

 -George

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

Re: [PATCH v2 3/4] libxl: add HVM usb passthrough support

[Juergen Gross]

On 20/09/16 11:05, George Dunlap wrote:
> On Tue, Sep 20, 2016 at 7:17 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 19/09/16 17:31, George Dunlap wrote:
>>> On 19/09/16 13:40, Juergen Gross wrote:
>>>> Add HVM usb passthrough support to libxl by using qemu's capability
>>>> to emulate standard USB controllers.
>>>>
>>>> A USB controller is added via qmp command to the emulated hardware
>>>> when a usbctrl device of type DEVICEMODEL is requested. Depending on
>>>> the requested speed the appropriate hardware type is selected. A host
>>>> USB device can then be added to the emulated USB controller via qmp
>>>> command.
>>>>
>>>> Removing of the devices is done via qmp commands, too.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Thanks Juergen!  That was a pretty great turn-around time. :-)
>>>
>>> Overall everything looks reasonable, with just a few nits...
>>>
>>>> @@ -278,13 +460,6 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>>>>          }
>>>>      }
>>>>
>>>> -    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
>>>> -        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
>>>> -        LOG(ERROR, "Unsupported USB controller type");
>>>> -        rc = ERROR_FAIL;
>>>> -        goto out;
>>>> -    }
>>>> -
>>>>      rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
>>>>                                              aodev->update_json);
>>>>      if (rc) goto out;
>>>> @@ -293,6 +468,12 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
>>>>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>>>>      if (rc) goto outrm;
>>>>
>>>> +    if (device->backend_kind == LIBXL__DEVICE_KIND_NONE) {
>>>> +        rc = libxl__device_usbctrl_add_hvm(gc, domid, usbctrl);
>>>> +        if (rc) goto outrm;
>>>> +        goto out;
>>>> +    }
>>>
>>> This is sort of minor, but this seems like the wrong conditional.  Is
>>> there a reason you did't check for usbctrl->type == HVM here (as I think
>>> you did on the usbdev_add path)?
>>
>> No, I don't think so. I don't want to wait for the backend to
>> connect in case it isn't even there, so the conditional seems to
>> be just the right one.
> 
> Right, so there are two things this path is doing:
> 1. Not waiting for the backend because there is no backend
> 2. Sending qmp commands to add the usb controller because this is a
> devicemodel-based controller
> 
> As it happens at the moment, "No backend" <=> "DM-based controller".
> The conditional you've chosen assumes that "No backend" => "DM-based
> controller".  It seems to me that a more natural (and less likely to
> become untrue) assumption is "DM-based controller" => "No backend".
> 
> Alternately, if we really wanted to be strict, we could have two
> conditionals -- one for usbctrl_add_hvm(), and one for skipping
> waiting for the backend. :-)
> 
> Anyway, I continue think checking for DM is a bit more natural here,
> but it's fairly minor, so I won't press the point anymore.
> 
>>> The one other thing to bring up is how this new emulated usb interface
>>> in the config file (usbctrl and usbdev) will interact with the old
>>> emulated usb interface (usb, usbversion, and usbdevice).  If we enable a
>>> reasonable set of emulated-only devices (the tablet in particular), I
>>> think we should deprecate the old interfaces (though continue supporting
>>> them for backwards compatibility, of course).  Thoughts?
>>
>> The old interface is available for domain config only, right?
> 
> Yes, that's right.
> 
>> I think we should try to convert this stuff to the new interface in
>> order to be able to use it for a running domain, too. But I think this
>> will be a 4.9 topic.
> 
> The issue with that is that the old interface passes strings straight
> through to qemu; so if the user knows the magic runes to get an
> arbitrary device, they can get it.  In the new interface, we
> explicitly encode specific devices.  Since we don't plan (I don't
> think) on duplicating all of qemu's funcitonality, implementing the
> old interface with the new one would mean losing functionality for
> some people.  Deprecating the old interface allows existing configs to
> work while pointing people to the newer, more consistent (and more
> functional) interface.
> 
> But in any case, we can't deprecate the old interface until we at
> least have a USB tablet in the new interface; so this is certainly a
> 4.9 topic.  I just wanted to raise it before I forgot about it.

I agree. I'll see what can be done in 4.9.


Juergen


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

Re: [PATCH v2 4/4] docs: add HVM USB passthrough documentation

[Ian Jackson]

Juergen Gross writes ("[PATCH v2 4/4] docs: add HVM USB passthrough documentation"):
> Update the man page regarding passthrough of USB devices to HVM
> domains via qemu USB emulation.

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

By which I also mean that I approve of the API.  Thanks!


However,

> +"devicemodel" specifies a USB controller emulated by qemu. It will show up as
> +a PCI-device in the guest.
> +
>  "auto" (the default) determines whether a kernel based backend is installed.
>  If this is the case, "pv" is selected, "qusb" will be selected if no kernel
> -backend is currently available.
> +backend is currently available. For HVM domains "devicemodel" is being selected.

This should read:
   For HVM domains "devicemodel" will be selected.

But this can be fixed up later; I'm happy to submit such a patch.

Also you might want to consider semantic newlines for your future
patches to the markdown.
  http://rhodesmill.org/brandon/2012/one-sentence-per-line/



Regards,
Ian.

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

Re: [PATCH v2 4/4] docs: add HVM USB passthrough documentation

[Juergen Gross]

On 20/09/16 12:25, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH v2 4/4] docs: add HVM USB passthrough documentation"):
>> Update the man page regarding passthrough of USB devices to HVM
>> domains via qemu USB emulation.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> By which I also mean that I approve of the API.  Thanks!
> 
> 
> However,
> 
>> +"devicemodel" specifies a USB controller emulated by qemu. It will show up as
>> +a PCI-device in the guest.
>> +
>>  "auto" (the default) determines whether a kernel based backend is installed.
>>  If this is the case, "pv" is selected, "qusb" will be selected if no kernel
>> -backend is currently available.
>> +backend is currently available. For HVM domains "devicemodel" is being selected.
> 
> This should read:
>    For HVM domains "devicemodel" will be selected.

Okay, will change.

> But this can be fixed up later; I'm happy to submit such a patch.

No need for that. George wanted a rename of a function in Patch 3, so
I'm going to send V3 anyway.

> Also you might want to consider semantic newlines for your future
> patches to the markdown.
>   http://rhodesmill.org/brandon/2012/one-sentence-per-line/

Thanks for the hint. I'll try to remember it. :-)


Juergen


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

Re: [PATCH v2 4/4] docs: add HVM USB passthrough documentation

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH v2 4/4] docs: add HVM USB passthrough documentation"):
> On 20/09/16 12:25, Ian Jackson wrote:
> > This should read:
> >    For HVM domains "devicemodel" will be selected.
> 
> Okay, will change.

Great, thanks.

> > Also you might want to consider semantic newlines for your future
> > patches to the markdown.
> >   http://rhodesmill.org/brandon/2012/one-sentence-per-line/
> 
> Thanks for the hint. I'll try to remember it. :-)

I only encountered that fairly recently but it's already improving my
life...

Regards,
Ian.

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