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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Sep 16, 2016 at 10:51:18AM +0200, Juergen Groß wrote:
> 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.

I see. I missed that dev->backend_kind is changed to dev->kind in the
diff.

> >> {
> >>     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?

Yes (and the other place as well).

Wei.

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