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

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

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

>  
> > --- 
> >  tools/libxl/libxl_types.idl          | 58  
> +++++++++++++++++++++++++++++++++++- 
> >  tools/libxl/libxl_types_internal.idl |  1 + 
> >  2 files changed, 58 insertions(+), 1 deletion(-) 
> >  
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl 
> > index 1214d2e..0639434 100644 
> > --- a/tools/libxl/libxl_types.idl 
> > +++ b/tools/libxl/libxl_types.idl 
> > @@ -111,6 +111,16 @@ libxl_nic_type = Enumeration("nic_type", [ 
> >      (2, "VIF"), 
> >      ]) 
> >   
> > +libxl_usbctrl_type = Enumeration("usbctrl_type",[ 
> > +    (0, "AUTO"), 
> > +    (1, "PV"), 
> > +    (2, "DEVICEMODEL"), 
> > +    ]) 
> > + 
> > +libxl_usb_type = Enumeration("device_usb_type", [ 
> > +    (1, "HOSTDEV"), 
> > +    ]) 
> > + 
> >  libxl_action_on_shutdown = Enumeration("action_on_shutdown", [ 
> >      (1, "DESTROY"), 
> >   
> > @@ -394,7 +404,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ 
> >      ("ioports",          Array(libxl_ioport_range, "num_ioports")), 
> >      ("irqs",             Array(uint32, "num_irqs")), 
> >      ("iomem",            Array(libxl_iomem_range, "num_iomem")), 
> > -    ("claim_mode",      libxl_defbool), 
> > +    ("claim_mode",       libxl_defbool), 
>  
> Spurious whitespace change -- please kill this. 
>  
> >      ("event_channels",   uint32), 
> >      ("kernel",           string), 
> >      ("cmdline",          string), 
> > @@ -521,6 +531,27 @@ libxl_device_pci = Struct("device_pci", [ 
> >      ("seize", bool), 
> >      ]) 
> >   
> > +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.
 
>  
> 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.

Thanks,
Chunyan
 
>  
> Thanks again for picking this up.  I'll give feedback on the other 
> patches tomorrow. 
>  
>  -George 
>  
>  
>  


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