Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Sep 08, 2016 at 09:20:25AM +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.

Overall the code looks plausible. But the code seems to do more than
what is stated in commit message. Some comments below.

> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libxl/libxl_device.c |   3 +-
>  tools/libxl/libxl_usb.c    | 417 +++++++++++++++++++++++++++++++++++----------
>  tools/libxl/xl_cmdimpl.c   |   4 +-
>  3 files changed, 331 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 5211f20..c6f15db 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -782,8 +782,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)

This looks a bit suspicious to me. This could be rather obvious but I'm
not sure because I didn't follow closely on the overall design of PV /
HVM USB passthrough.

AIUI:

VUSB -> PV USB with in-kernel backend
QUSB -> PV USB with QEMU backend

Why is QUSB deleted here?

If there is refactoring involved, can that be separated out?

>                      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 6b30e0f..40c5a84 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,67 @@ 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");

Please wrap long line if possible.

> +                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 +131,19 @@ 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:
> +        break;

Shouldn't we return some sort of error here?

> +    }
>      device->devid           = usbctrl->devid;
>      device->domid           = domid;
>      device->kind            = LIBXL__DEVICE_KIND_VUSB;
> @@ -85,6 +151,31 @@ 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, *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))
> +            return NULL;
> +    }
> +
> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
> +        return libxl_path;
> +
> +    r = libxl__xs_read_checked(gc, XBT_NULL,
> +                               GCSPRINTF("%s/backend", libxl_path),
> +                               &be_path);
> +    if (r || !be_path) return NULL;
> +
> +    return be_path;

Please use goto style error handling.

> +}
> +
>  /* Add usbctrl information to xenstore.
>   *
>   * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
> @@ -96,7 +187,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 +203,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 +226,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) {
> @@ -194,6 +288,34 @@ out:
>      return rc;
>  }
>  
> +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_type(gc, libxl_path, usbctrl->type);
> +
> +    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);
> +}
> +

It seems that the need for this function is because we didn't add one in
previous version. If that's the case, is it possible to have a separate
patch for it so that we can backport it?

>  static char *pvusb_get_device_type(libxl_usbctrl_type type)
>  {
>      switch (type) {
> @@ -206,6 +328,92 @@ 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:
> +        break;

Return ERROR_INVAL?

Wei.

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