Re: [PATCH RFC V2 1/5] libxl: add pvusb definitions

[George Dunlap <george.dunlap@eu.citrix.com>]

On 03/04/2015 08:28 AM, Chun Yan Liu wrote:
> 
> 
>>>> On 3/4/2015 at 01:15 AM, in message <54F5EC4E.6020607@eu.citrix.com>, George
> Dunlap <george.dunlap@eu.citrix.com> wrote: 
>> On 01/19/2015 08:28 AM, Chunyan Liu wrote: 
>>> To attach a usb device, a virtual usb controller should be created first. 
>>> This patch defines usbctrl and usbdevice related structs. 
>>>  
>>> Signed-off-by: Chunyan Liu <cyliu@suse.com> 
>>> Signed-off-by: Simon Cao <caobosimon@gmail.com> 
>>  
>> Chunyan, thanks for picking up this work! 
>>  
>> A couple of things.  First, I think that having the IDL stuff separate 
>> from the patches where they are used actually makes it *harder* to 
>> review, because you can't easily go to the code where it's used and see 
>> what's actually happening. 
>>  
>> I think that the IDL stuff used in patch 3 should be in patch 3; and the 
>> domain creation IDL stuff should be included in patch 5. 
> 
> Tha's OK. I'll update.

Great, thanks.

>>> +libxl_device_usbctrl = Struct("device_usbctrl", [ 
>>> +    ("name", string), 
>>> +    ("type", libxl_usbctrl_type), 
>>> +    ("backend_domid", libxl_domid), 
>>> +    ("backend_domname", string), 
>>> +    ("devid", libxl_devid), 
>>> +    ("usb_version", uint8), 
>>> +    ("num_ports", uint8), 
>>> +    ]) 
>>> + 
>>> +libxl_device_usb = Struct("device_usb", [ 
>>> +    ("ctrl", integer), 
>>> +    ("port", integer), 
>>> +    ("intf", string), 
>>> +    ("u", KeyedUnion(None, libxl_usb_type, "type", 
>>> +        [("hostdev", Struct(None, [ 
>>> +            ("hostbus",   integer), 
>>> +            ("hostaddr",  integer) ])) 
>>> +        ])) 
>>> +    ]) 
>>  
>> So "intf" here is wrong.  To begin with, it's information specific to 
>> the "hostdev" type; so it would go under the "type" keyed union under 
>> "hostdev". 
>>  
>> Secondly, this requires people to figure out that their media reader has 
>> an intf of "1-2.1.1:1.0".  I don't think that's a good idea, for two 
>> reasons: one, it just seems like a really hard interface to use.  I 
>> couldn't find any straightforward tools to map USB devices onto intf; 
> 
> Right. One can only use usb-assignable-list for a fast look. That
> follows the old xend toolstack way.
> 
>> tools like "lsusb" instead give you a bus:addr combination.  Secondly, 
>> it's inconsistent with qemu -- which means we'd either have to have two 
>> different ways of specifying the device, or we'd need to translate from 
>> "intf" back into bus:addr
> 
> You are right. Using bus:addr could unify qemu and pvusb. I also thought
> about that. Only concern is it's different from old xend toolstack usage.
> If that doesn't affect, we can update to use bus:addr, no problem.

Right, I see.

I think overall that the bus:addr is a better interface; it's also
what's exposed by lsusb, qemu, and libvirt, AFAICT.  So I definitely
think that at the libxl level that's what we should be using.

We're already defining a new config file format, right?  So domain
configs that used pvusb with xend won't work with this patch series
anyway, correct?

If we're not going to make something 100% compatible, I don't see any
particular value in making it 25% compatible. :-)

>> I think the right thing to do here is to take "intf" out of this struct, 
>> and to translate "bus:addr" into intf internally. 
>>  
>> It looks like the values qemu and lsusb use can be found in "busnum" and 
>> "devnum" in the sysfs files.  You've already got code to scan those 
>> devices; you just need to add a bit of code to find which of those 
>> corresponds to a given "hostbus:hostaddr" combination. 
>>  
>>> + 
>>>  libxl_device_vtpm = Struct("device_vtpm", [ 
>>>      ("backend_domid",    libxl_domid), 
>>>      ("backend_domname",  string), 
>>> @@ -547,6 +578,7 @@ libxl_domain_config = Struct("domain_config", [ 
>>>      ("disks", Array(libxl_device_disk, "num_disks")), 
>>>      ("nics", Array(libxl_device_nic, "num_nics")), 
>>>      ("pcidevs", Array(libxl_device_pci, "num_pcidevs")), 
>>> +    ("usbs", Array(libxl_device_usb, "num_usbs")), 
>>  
>> Any reason you don't make it possible to specify usb controllers as well?
> 
> For qemu emulated HVM usb device, usb controller is created by qemu, no
> way to specify it (?) Also I wonder if specifying usb controller is necessary,
> seems it won't affect without usb controller here. Correct me if I'm wrong.
> If it's necessary, we can add it.

On the contrary, there is a way to specify it. :-)  See
"usbversion=[n]".  At the moment we specify the usb device on the qemu
command-line; but I'm pretty sure that we can use qmp to hot-plug a USB
controller just like we can use it to hot-plug a USB device.

qemu will automatically hot-plug a USB controller as necessary, similar
to your "automatically create a pvusb controller" functionality for
pvusb add.  But there may be circumstances where we want to specify a
controller (for instance, if you want to be able to control what kind of
controller it is).

My long-term vision is to have the USB stuff unified.  Instead of
passing in USB devices on the qemu command-line, as we do now, we'd plug
them in after starting qemu but before letting the VM run (similar to
the way we do things with PCI pass-through).

In any case, I agree with your design decision that you shouldn't *have*
to specify a controller.  However, I think you should be able to specify
a controller if you wish.

Adding that functionality to libxl should be pretty straightforward.
Coming up with a sensible way to specify it in the xl config file would
be a bit more work, and I would be open to the argument that it
shouldn't be a requirement for this series to go in.

 -George