Re: [Xen-devel] [PATCH V4 3/7] libxl: add pvusb API

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Jun 15, 2015 at 7:26 PM, Juergen Gross <jgross@xxxxxxxx> wrote:
> On 06/15/2015 04:34 PM, George Dunlap wrote:
>>
>> On Mon, Jun 15, 2015 at 3:25 PM, JÃrgen Groà <jgross@xxxxxxxx> wrote:
>>>
>>> On 06/15/2015 04:17 PM, George Dunlap wrote:
>>>>
>>>>
>>>> On Wed, Jun 10, 2015 at 4:20 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote:
>>>>>
>>>>>
>>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>>>> index 23f27d4..4561e1b 100644
>>>>> --- a/tools/libxl/libxl_types.idl
>>>>> +++ b/tools/libxl/libxl_types.idl
>>>>> @@ -541,6 +541,29 @@ libxl_device_pci = Struct("device_pci", [
>>>>>        ("seize", bool),
>>>>>        ])
>>>>>
>>>>> +libxl_usb_protocol = Enumeration("usb_protocol", [
>>>>> +    (0, "AUTO"),
>>>>> +    (1, "PV"),
>>>>> +    (2, "QEMU"),
>>>>> +    ])
>>>>> +
>>>>> +libxl_device_usbctrl = Struct("device_usbctrl", [
>>>>> +    ("protocol", libxl_usb_protocol),
>>>>> +    ("devid", libxl_devid),
>>>>> +    ("version", integer),
>>>>> +    ("ports", integer),
>>>>> +    ("backend_domid", libxl_domid),
>>>>> +    ("backend_domname", string),
>>>>> +   ])
>>>>> +
>>>>> +libxl_device_usb = Struct("device_usb", [
>>>>> +    ("ctrl", libxl_devid),
>>>>> +    ("port", integer),
>>>>> +    ("busid", string),
>>>>> +    ("hostbus",   integer),
>>>>> +    ("hostaddr",  integer),
>>>>> +    ])
>>>>
>>>>
>>>>
>>>> Ian / Ian / Wei / Jim:
>>>>
>>>> Question about the design of the interface here.
>>>>
>>>> The way that most systems in Linux specify particular USB devices is
>>>> with the bus:addr format.  It's the output you get when you run tools
>>>> like lsusb, for example, and it's the interface that qemu (and thus
>>>> KVM) uses when talking about host devices to assign.  bus:addr might
>>>> look like "002:006".
>>>>
>>>> But the bus:addr is a "public api" for Linux; internally, it has a
>>>> more structured format, which contains more about the USB topology.
>>>> Chunyan is calling this "busid".  An example is something like this:
>>>> "2-3.1.1:1.0".
>>>>
>>>> Internally, pvusb needs "busid" in order to find the right sysfs
>>>> files.  qemu, on the other hand, does not take busid; so the
>>>> devicemodel / HVM implementation of USB would need bus:addr
>>>> internally.
>>>
>>>
>>>
>>> A patch for qemu to support the busid is trivial, as the structures
>>> already contain the necessary elements:
>>>
>>> --- a/hw/usb/host-legacy.c
>>> +++ b/hw/usb/host-legacy.c
>>> @@ -53,11 +53,6 @@ static int parse_filter(const char *spec, struct
>>> USBAutoFilter *f)
>>>       const char *p = spec;
>>>       int i;
>>>
>>> -    f->bus_num    = 0;
>>> -    f->addr       = 0;
>>> -    f->vendor_id  = 0;
>>> -    f->product_id = 0;
>>> -
>>>       for (i = BUS; i < DONE; i++) {
>>>           p = strpbrk(p, ":.");
>>>           if (!p) {
>>> @@ -100,32 +95,47 @@ USBDevice *usb_host_device_open(USBBus *bus, const
>>> char
>>> *devname)
>>>
>>>       dev = usb_create(bus, "usb-host");
>>>
>>> +    memset(&filter, 0, sizeof(filter));
>>> +
>>>       if (strstr(devname, "auto:")) {
>>>           if (parse_filter(devname, &filter) < 0) {
>>>               goto fail;
>>>           }
>>> -    } else {
>>> -        p = strchr(devname, '.');
>>> -        if (p) {
>>> -            filter.bus_num    = strtoul(devname, NULL, 0);
>>> -            filter.addr       = strtoul(p + 1, NULL, 0);
>>> -            filter.vendor_id  = 0;
>>> -            filter.product_id = 0;
>>> -        } else {
>>> -            p = strchr(devname, ':');
>>> -            if (p) {
>>> -                filter.bus_num    = 0;
>>> -                filter.addr       = 0;
>>> -                filter.vendor_id  = strtoul(devname, NULL, 16);
>>> -                filter.product_id = strtoul(p + 1, NULL, 16);
>>> -            } else {
>>> -                goto fail;
>>> -            }
>>> -        }
>>> +        goto out;
>>>       }
>>>
>>> +    /* Check for <bus>-<port> specification. */
>>> +    p = strchr(devname, '-');
>>> +    if (p && p != devname) {
>>> +        filter.bus_num    = strtoul(devname, NULL, 0);
>>> +        filter.port       = p + 1;
>>> +        goto out;
>>> +    }
>>
>>
>>
>> On my system bus:addr for my mouse is 002:005, while the "busid" (the
>> corresponding directory in sysfs) is 2-3.3.
>>
>> This code doesn't appear to me to parse the above properly; or did I
>> miss something?
>
>
> Filling filter.bus_num and filter.port is enough. This was the only part
> missing in qemu, finding the device via libusb using bus_num and port is
> already existing. At least it is working for me. :-)

Well, one of us is completely wrong.

Let's follow the example above:

$ ls /sys/bus/usb/devices/2-3*
/sys/bus/usb/devices/2-3@
/sys/bus/usb/devices/2-3.1@
/sys/bus/usb/devices/2-3:1.0@
/sys/bus/usb/devices/2-3.1.1@
/sys/bus/usb/devices/2-3.1:1.0@
/sys/bus/usb/devices/2-3.1.1:1.0@
/sys/bus/usb/devices/2-3.1.2@
/sys/bus/usb/devices/2-3.1.2:1.0@
/sys/bus/usb/devices/2-3.1.2:1.1@
/sys/bus/usb/devices/2-3.2@
/sys/bus/usb/devices/2-3.2:1.0@
/sys/bus/usb/devices/2-3.3@
/sys/bus/usb/devices/2-3.3:1.0@

$ for i in /sys/bus/usb/devices/2-3*/; do grep . $i/{bus,dev}num ; done
/sys/bus/usb/devices/2-3//busnum:2
/sys/bus/usb/devices/2-3//devnum:2
/sys/bus/usb/devices/2-3.1//busnum:2
/sys/bus/usb/devices/2-3.1//devnum:3
/sys/bus/usb/devices/2-3.1.1//busnum:2
/sys/bus/usb/devices/2-3.1.1//devnum:6
/sys/bus/usb/devices/2-3.1.2//busnum:2
/sys/bus/usb/devices/2-3.1.2//devnum:10
/sys/bus/usb/devices/2-3.2//busnum:2
/sys/bus/usb/devices/2-3.2//devnum:4
/sys/bus/usb/devices/2-3.3//busnum:2
/sys/bus/usb/devices/2-3.3//devnum:5

$ lsusb | grep "Bus 002"
Bus 002 Device 005: ID 046d:c016 Logitech, Inc. Optical Wheel Mouse
Bus 002 Device 004: ID f617:0905
Bus 002 Device 010: ID 1050:0111 Yubico.com
Bus 002 Device 006: ID 0424:4060 Standard Microsystems Corp. Ultra
Fast Media Reader
Bus 002 Device 003: ID 0424:2640 Standard Microsystems Corp. USB 2.0 Hub
Bus 002 Device 002: ID 0424:2514 Standard Microsystems Corp. USB 2.0 Hub
Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

In other words, there are 6 distinct devices that correspond to "bus 2
port 3".  I don't know what it was you were passing through, but
giving qemu (or libusb) only "2-3" is absolutely not enough for it to
distinquish between my mouse (002:005, at 2-3.3) and my yubikey
(002:010 at 2-3.1.2).  That's why the bus:addr convention was invented
in the first place, I presume -- to abstract away the topology of the
USB hubs for the user.

The "busid" interface that Chunyan is describing requires the caller
to find out that long name -- 2-3.1.2 -- rather than the traditional
short name (002:010).  Just accepting "2-3" is not sufficient.

 -George

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