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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 3/4/2015 at 10:41 PM, in message <54F71992.5080306@xxxxxxxxxxxxx>, George
Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote: 
> On 03/04/2015 08:28 AM, Chun Yan Liu wrote: 
> >  
> >  
> >>>> On 3/4/2015 at 01:15 AM, in message <54F5EC4E.6020607@xxxxxxxxxxxxx>, 
> >>>> George 
> > Dunlap <george.dunlap@xxxxxxxxxxxxx> 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@xxxxxxxx>  
> >>> Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>  
> >>   
> >> 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. 

Thanks. Got it. Will update.

>  
> 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). 

Thanks for explanation. So I'll add usb controllers to domain_config, and
for that libxl_device_usbctrl should be finally able to represent pvusb 
controller
and qemu emulated controller. Currently, it's defined to represent a pvusb
controller, in future, when HVM qemu emulated USB is implemented, it could
be extended.

- Chunyan

>  
> 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 
>  
>  


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