Re: [PATCH V8 7/7] domcreate: support pvusb in configuration file

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

>>> On 11/13/2015 at 12:10 AM, in message
<CAFLBxZZ_KN0vMr77+dfRTgGEWW3LSbv8m__AWfvEeCH1WBRrYw@mail.gmail.com>, George
Dunlap <George.Dunlap@eu.citrix.com> wrote: 
> On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu <cyliu@suse.com> wrote: 
> > Add code to support pvusb in domain config file. One could specify 
> > usbctrl and usb in domain's configuration file and create domain, 
> > then usb controllers will be created and usb device would be attached 
> > to guest automatically. 
> > 
> > One could specify usb controllers and usb devices in config file 
> > like this: 
> > usbctrl=['version=2,ports=4', 'version=1, ports=4', ] 
> > usbdev=['hostbus=2, hostaddr=1, controller=0,port=1', ] 
> > 
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > Signed-off-by: Simon Cao <caobosimon@gmail.com> 
> > 
> > --- 
> > changes: 
> >   - change parse_usb_config and parse_usbctrl_config, following 
> >     parse_nic_config way, and move to previous patch 
> >   - update user interface to specify hostbus, hostaddr instead of 
> >     hostbus.hostaddr for future extension 
> > 
> >  docs/man/xl.cfg.pod.5        | 81  
> ++++++++++++++++++++++++++++++++++++++++++++ 
> >  tools/libxl/libxl_create.c   | 73 +++++++++++++++++++++++++++++++++++++-- 
> >  tools/libxl/libxl_device.c   |  4 +++ 
> >  tools/libxl/libxl_internal.h |  8 +++++ 
> >  tools/libxl/xl_cmdimpl.c     | 54 ++++++++++++++++++++++++++++- 
> >  5 files changed, 216 insertions(+), 4 deletions(-) 
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 
> > index b63846a..f24c008 100644 
> > --- a/docs/man/xl.cfg.pod.5 
> > +++ b/docs/man/xl.cfg.pod.5 
> > @@ -722,6 +722,87 @@ Note this may be overridden by rdm_policy option in PCI  
> device configuration. 
> > 
> >  =back 
> > 
> > +=item B<usbctrl=[ "USBCTRL_SPEC_STRING", "USBCTRL_SPEC_STRING", ... ]> 
> > + 
> > +Specifies the USB controllers created for this guest. Each 
> > +B<USB_SPEC_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where: 
> > + 
> > +=over 4 
> > + 
> > +=item B<KEY=VALUE> 
> > + 
> > +Possible B<KEY>s are: 
> > + 
> > +=over 4 
> > + 
> > +=item B<type=TYPE> 
> > + 
> > +Specifies the protocol to implement USB controller, could be "pv"  
> (indicates 
> > +PVUSB) or "qemu" (indicates QEMU emulated). Currently only "pv" is  
> supported. 
>  
> I think most of these should basically be copied from the descriptions 
> of the arguments in patch 5. 

I'll check.

>  
> > +=item B<version=VERSION> 
> > + 
> > +Specifies version of the USB controller, could be 1 (USB1.1) or 2  
> (USB2.0). 
> > +Default is 2 (USB2.0). 
> > + 
> > +=item B<ports=PORTS> 
> > + 
> > +Specifies port number of the USB controller. Default is 8. 
> > + 
> > +Each USB controller will have an index starting from 0. On the same 
> > +controller, each port will have an index starting from 1. 
>  
> I'd say: 
>  
> "USB controler ids start from 0.  In line with the USB spec, however, 
> ports on a controller start from 1." 

Much better. Thanks!

>  
> > + 
> > +E.g. 
> > +usbctrl=["version=1,ports=4", "version=2,ports=8",] 
> > +The first controller has: 
> > +controller index = 0, and port 1,2,3,4. 
> > +The second controller has: 
> > +controller index = 1, and port 1,2,3,4,5,6,7,8. 
>  
> The example is good, but I'd use "controller id" rather than "controller  
> index".

Take it.
 
>  
> > +=back 
> > + 
> > +=back 
> > + 
> > +=item B<usbdev=[ "USB_SPEC_STRING", "USB_SPEC_STRING", ... ]> 
> > + 
> > +Specifies the host USB devices to passthrough to this guest. Each 
> > +B<USB_SPEC_STRING> has the form C<KEY=VALUE,KEY=VALUE,...> where: 
>  
> "Specifiec the USB devices to be attached to the guest at boot." 
> (i.e., don't assume all devices are hostdev.) 

Thanks, I'll update.

>  
> > +=over 4 
> > + 
> > +=item B<KEY=VALUE> 
> > + 
> > +Possible B<KEY>s are: 
> > + 
> > +=over 4 
> > + 
> > +=item B<devtype=hostdev> 
> > + 
> > +Specifies USB device type. Currently only support 'hostdev'. 
> > + 
> > +=item B<hostbus=busnum> 
> > + 
> > +Specifies busnum of the USB device from the host perspective. 
> > + 
> > +=item B<hostaddr=devnum> 
> > + 
> > +Specifies devnum of the USB device from the host perspective. 
> > + 
> > +=item B<controller=CONTROLLER> 
> > + 
> > +Specifies USB controller index, to which controller the USB device is  
> attached. 
> > + 
> > +=item B<port=PORT> 
> > + 
> > +Specifies USB port index, to which port the USB device is attached.  
> B<port=PORT> 
> > +is valid only when B<controller=CONTROLLER> is specified. Without 
> > +B<controller=CONTROLLER>, it will find the first available USB  
> controller:port 
> > +and use it. If there is no controller at all, it will create one. 
>  
> I think the last sentence should be a separate paragraph, probably 
> after the "=back".  And a more idiomatic way to write it might be: 
>  
> "If no controller is specified, an available controller:port 
> combination will be used.  If there are no available controller:port 
> options, a new controller will be created." 

Thanks very much! Much better.

>  
> Only one other minor comment from me: 
>  
> > +    if (!xlu_cfg_get_list(config, "usbctrl", &usbctrls, 0, 0)) { 
> > +        d_config->num_usbctrls = 0; 
> > +        d_config->usbctrls = NULL; 
> > +        while ((buf = xlu_cfg_get_listitem(usbctrls,  
> d_config->num_usbctrls)) 
> > +               != NULL) { 
>  
> I don't know what the other maintainers think, but particularly given 
> that this is spanning a line, I would personally take out the 
> comparison, and just make it 
>  
>  while ((buf = ...)) { 
>  ... 
> } 
>  
> Other than that, this one looks good to me. 
>  
>  -George 
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>