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

[Juergen Gross <jgross@xxxxxxxx>]

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.

In the usbdev_add path I have to take different measures for each
of the possible three controller types (visb, qusb or devicemodel),
so doing a switch over usbctrl->type seems to be the natural choice.

> 
>> +static char *pvusb_get_port_path(libxl__gc *gc, uint32_t domid,
>> +                                 libxl_usbctrl_type type, int ctrl, int 
>> port)
>> +{
>> +    char *path;
>> +
>> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
>> +        path = GCSPRINTF("%s/device/vusb", libxl__xs_libxl_path(gc, domid));
>> +    else
>> +        path = GCSPRINTF("%s/backend/%s/%d",
>> +                         libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
>> +                         pvusb_get_device_type(type), domid);
>> +
>> +    return GCSPRINTF("%s/%d/port/%d", path, ctrl, port);
>> +}
> 
> Should this function be named "vusb_get..." rather than "pvusb_get...""?
>  After all, it returns the port even for non-PV usb ports.

Yes, you are right.

> I think that's all I had on this at a quick pass-through.
> 
> 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?

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.


Juergen

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