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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

 -George

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