Re: [Xen-devel] [PATCH V4 5/7] xl: add pvusb commands

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

>>> On 6/12/2015 at 03:37 PM, in message <557A8C57.1040108@xxxxxxxx>, Juergen 
>>> Gross
<jgross@xxxxxxxx> wrote: 
> On 06/10/2015 05:20 AM, Chunyan Liu wrote: 
> > Add pvusb commands: usb-ctrl-attach, usb-ctrl-detach, usb-list, 
> > usb-attach and usb-detach. 
> > 
> > To attach a usb device to guest through pvusb, one could follow 
> > following example: 
> > 
> >   #xl usb-ctrl-attach test_vm version=1 num_ports=8 
> > 
> >   #xl usb-list test_vm 
> >   will show the usb controllers and port usage under the domain. 
> > 
> >   #xl usb-assignable-list 
> >   will list assignable USB devices 
>  
> xl usb-assignable-list is not part of this patch. Either merge this 
> patch and the following one, or describe the command in the next patch. 

Oh, yes, I forget to split.

>  
> > 
> >   #xl usb-attach test_vm 1.6 
> >   will find the first usable controller:port, and attach usb 
> >   device whose bus address is 1.6 (busnum is 1, devnum is 6) 
> >   to it. One could also specify which <controller> and which <port>. 
> > 
> >   #xl usb-detach test_vm 1.6 
> > 
> >   #xl usb-ctrl-detach test_vm dev_id 
> >   will destroy the controller with specified dev_id. Dev_id 
> >   can be traced in usb-list info. 
> > 
> > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx> 
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> 
> > --- 
> >   docs/man/xl.pod.1         |  38 +++++++ 
> >   tools/libxl/xl.h          |   5 + 
> >   tools/libxl/xl_cmdimpl.c  | 251  
> ++++++++++++++++++++++++++++++++++++++++++++++ 
> >   tools/libxl/xl_cmdtable.c |  25 +++++ 
> >   4 files changed, 319 insertions(+) 
> > 
>  
> ... 
>  
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c 
> > index c858068..b29d0fc 100644 
> > --- a/tools/libxl/xl_cmdimpl.c 
> > +++ b/tools/libxl/xl_cmdimpl.c 
> > @@ -3219,6 +3219,257 @@ int main_cd_insert(int argc, char **argv) 
> >       return 0; 
> >   } 
> > 
> > +static void usbinfo_print(libxl_device_usb *usbs, int num) { 
> > +    int i; 
>  
> Blank line missing. 
>  
> > +    if (usbs == NULL) 
> > +         return; 
> > +    for (i = 0; i < num; i++) { 
> > +        libxl_usbinfo usbinfo; 
>  
> Blank line missing. 
>  
> > +        libxl_usbinfo_init(&usbinfo); 
> > + 
> > +        if (usbs[i].port) 
> > +            printf("  Port %d:", usbs[i].port); 
> > +        if (!libxl_device_usb_getinfo(ctx, &usbs[i], &usbinfo)) { 
> > +            printf(" Bus %03x Device %03x: ID %04x:%04x %s %s\n", 
> > +                    usbinfo.busnum, usbinfo.devnum, 
> > +                    usbinfo.idVendor, usbinfo.idProduct, 
> > +                    usbinfo.manuf ?: "", usbinfo.prod ?: ""); 
>  
> Is it really possible for a device to be assigned but without a port 
> number? 

For assigned usb device, it's not possible. But this function will be called
in two places, one is to list assigned usb devices in 'xl usb-list'; another
is to list assignable usb devices in 'xl usb-assignable-list'. If
'usb-assignable-list' is not taken, this could be improved.

>  
> I'd rather combine the two if's and printf statements. This would avoid 
> the case where "  Port 1: Port 2: ..." is printed due to a failing 
> libxl_device_usb_getinfo() for port 1. 
>  
> > +        } 
> > +        libxl_usbinfo_dispose(&usbinfo); 
> > +    } 
> > +} 
> > + 
> > +int main_usbctrl_attach(int argc, char **argv) 
> > +{ 
> > +    uint32_t domid; 
> > +    int opt; 
> > +    char *oparg; 
> > +    libxl_device_usbctrl usbctrl; 
> > + 
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-ctrl-attach", 1) { 
> > +        /* No options */ 
> > +    } 
> > + 
> > +    domid = find_domain(argv[optind++]); 
> > + 
> > +    libxl_device_usbctrl_init(&usbctrl); 
> > + 
> > +    while (argc > optind) { 
> > +        if (MATCH_OPTION("type", argv[optind], oparg)) { 
> > +            if (!strcmp(oparg, "pv")) { 
> > +               usbctrl.protocol = LIBXL_USB_PROTOCOL_PV; 
> > +            } else { 
> > +               fprintf(stderr, "unsupported type `%s'\n", oparg); 
> > +               exit(-1); 
> > +            } 
> > +        } else if (MATCH_OPTION("version", argv[optind], oparg)) { 
> > +            usbctrl.version = atoi(oparg); 
>  
> Shouldn't you check for valid versions? 

OK. Will check.

>  
> > +        } else if (MATCH_OPTION("ports", argv[optind], oparg)) { 
> > +            usbctrl.ports = atoi(oparg); 
>  
> Same here for number of ports. Otherwise you could blow up xenstore by 
> e.g. specifying 2 billion ports here. 

Will add check. What's the supported max ports? 

>  
> > +        } else { 
> > +            fprintf(stderr, "unrecognized argument `%s'\n", argv[optind]); 
> > +            exit(-1); 
> > +        } 
> > +        optind++; 
> > +    } 
> > + 
> > +    if (usbctrl.protocol == LIBXL_USB_PROTOCOL_AUTO) 
> > +        usbctrl.protocol = LIBXL_USB_PROTOCOL_PV; 
>  
> Is this really necessary? You do it in libxl, too. 
Yes, seems not necessary. Anyway (from config file or hotplug), it will
call libxl_device_usbctrl_setdefault and do that.

Thanks,
Chunyan
>  
>  
> Juergen 
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 
>  
>  


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