From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chun Yan Liu" <cyliu@suse.com>
Subject: Re: [PATCH V4 5/7] xl: add pvusb commands
Date: Fri, 12 Jun 2015 02:03:23 -0600
Message-ID: <557B02CB02000066000D4CB2@relay2.provo.novell.com>
References: <1433906441-3280-1-git-send-email-cyliu@suse.com>
	<1433906441-3280-6-git-send-email-cyliu@suse.com>
	<557A8C57.1040108@suse.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <557A8C57.1040108@suse.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: xen-devel@lists.xen.org, Juergen Gross <JGross@suse.com>
Cc: george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, Simon Cao <caobosimon@gmail.com>, wei.liu2@citrix.com, ian.campbell@citrix.com
List-Id: xen-devel@lists.xenproject.org



>>> On 6/12/2015 at 03:37 PM, in message <557A8C57.1040108@suse.com>, Juergen Gross
<jgross@suse.com> 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@suse.com> 
> > Signed-off-by: Simon Cao <caobosimon@gmail.com> 
> > --- 
> >   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@lists.xen.org 
> http://lists.xen.org/xen-devel 
>  
>