[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 20/09/16 11:05, George Dunlap wrote: > On Tue, Sep 20, 2016 at 7:17 AM, Juergen Gross <jgross@xxxxxxxx> 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@xxxxxxxx> >>> >>> 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |