[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 5/6] libxl: add HVM usb passthrough support
On 09/15/2016 05:38 PM, Wei Liu wrote:
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.
Thanks. I'm just travelling, so my answers are based on my memory what I
did in the patch. I'll double check before sending out V2.
Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
---
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?
It isn't. :-)
A USB passthrough device will always be of kind == VUSB. The backend
kind may differ, though. This is to ensure a proper device numbering
regardless of the backend type used.
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.
Okay.
+ 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?
This case should not be possible.
Are you okay with me adding an assert() 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.
Okay.
+}
+
/* 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?
Okay.
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?
Should not be possible. I think I'll go with assert() again.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
