|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 3/4] libxl: add HVM usb passthrough support
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@xxxxxxxx>
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |