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

["Chun Yan Liu" <cyliu@suse.com>]

>>> On 3/3/2015 at 07:10 PM, in message <1425381019.24959.87.camel@citrix.com>, Ian
Campbell <ian.campbell@citrix.com> wrote: 
> On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote: 
>  
> Sorry for the long delay in replying. 
>  
> > To attach a usb device, a virtual usb controller should be created first. 
> > This patch defines usbctrl and usbdevice related structs. 
>  
> Per <54CA17DF0200006600095E3D@relay2.provo.novell.com> please could you 
> mention here that the HVM guest related parts (i.e. 
> LIBXL_USBCTRL_TYPE_DEVICEMODEL) and libxl_usb_type are placeholders for 
> emulated HVM support. 

Yes, I agree it's better placed in libxl_usb_type rather than ctrl_type.

>  
> In fact I wonder if it should just be omitted, we will need a LIBXL_HAVE 
> for HVM USB support anyway once it is implemented so we can add the enum 
> then. 

It won't harm to omit it for current pvusb work. Acceptable to me to
add enum later when adding HVM qemu emulated usb device implementation.

>  
> Or will we -- do we think returning an error for such an HVM guest with 
> USB devices configured now is acceptable and for it to silently start 
> working at some point in the future is OK? 
>  
> >  
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > Signed-off-by: Simon Cao <caobosimon@gmail.com> 
> > --- 
> >  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 
> > @@ -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), 
>  
> I think int would be fine for both of these last two (and is a bit 
> kinder to language bindings). 

OK. Will update.

>  
> > +    ]) 
> > + 
> > +libxl_device_usb = Struct("device_usb", [ 
> > +    ("ctrl", integer), 
>  
> Is this an index into something? If so what? 

To usb controller index.
A usb device should be connected to a usb port of a usb controller.
e.g.: there is 2 usb controllers in system, each with 8 ports, then:
1st usb controller index will be 0, port will be 1~8.
2nd usb controller index will be 1, port will be 1~8.
To attach a usb device through pvusb way, it should be pointed to
connect to which controller and which port.

>  
> There seems to be no usbctrl array added to the domain_config struct, so 
> I'm unsure how this is used. 
>  
> > +    ("port", integer), 
>  
> Port on the hub? 
> > +    ("intf", string), 
>  
> What is this one? (This may just be my lack of usb knowledge)

It means sysfs interface for the usb device under /sys/bus/usb/devices/,
like: 2-1.6.

>  
> > +    ("u", KeyedUnion(None, libxl_usb_type, "type", 
> > +        [("hostdev", Struct(None, [ 
> > +            ("hostbus",   integer), 
> > +            ("hostaddr",  integer) ])) 
> > +        ])) 
> > +    ]) 

This part is for HVM qemu emulated usb device too. Currently not used.

> > + 
> > @@ -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")), 
>  
> So, I'm unsure how this interacts with the controllers, which it doesn't 
> seem to be possible to specify at domain build time.

In domain config, user only needs to specify usb=['2-1.6'], by default, it will
create a default usb contoller, and probe the 1st available controller:port for
the usb device to attach. So, it can work to specify usbs here only.

Reason didn't include controller in libxl_domain_config: for HVM qemu emulated
usb device, all work is done in qemu (create usb controller and attach usb device),
no controller exists in libxl in that case.
 
>  
> You pointed me to 
> http://www.redhat.com/archives/libvir-list/2014-June/msg00038.html but 
> having had a look through I can't see it. 
>  
> For v3 please could you give an overview/summary of ow it fits together 
> in the 0/N patch. 

Sure. Thanks!

-Chunyan

>  
>  
>