Re: [PATCH v2 3/4] libxl: add HVM usb passthrough support

[Juergen Gross <jgross@suse.com>]

On 20/09/16 11:05, George Dunlap wrote:
> On Tue, Sep 20, 2016 at 7:17 AM, Juergen Gross <jgross@suse.com> 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@suse.com>
>>>
>>> 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@lists.xen.org
https://lists.xen.org/xen-devel