From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chun Yan Liu" <cyliu@suse.com>
Subject: Re: [PATCH V8 5/7] xl: add pvusb commands
Date: Thu, 12 Nov 2015 19:43:11 -0700
Message-ID: <5645BEBF0200006600082641@relay2.provo.novell.com>
References: <1445418510-19614-1-git-send-email-cyliu@suse.com>
	<1445418510-19614-6-git-send-email-cyliu@suse.com>
	<CAFLBxZb3LeZY1V78GdswCBB0B1_069-DbY-tuNVHnQ2h=ywT4A@mail.gmail.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: <CAFLBxZb3LeZY1V78GdswCBB0B1_069-DbY-tuNVHnQ2h=ywT4A@mail.gmail.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: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Juergen Gross <JGross@suse.com>, Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, Jim Fehlig <JFEHLIG@suse.com>, Simon Cao <caobosimon@gmail.com>
List-Id: xen-devel@lists.xenproject.org



>>> On 11/12/2015 at 07:38 PM, in message
<CAFLBxZb3LeZY1V78GdswCBB0B1_069-DbY-tuNVHnQ2h=ywT4A@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 pvusb commands: usbctrl-attach, usbctrl-detach, usb-list, 
> > usb-attach and usb-detach. 
> > 
> > To attach a usb device to guest through pvusb, one could follow 
> > following example: 
> > 
> >  #xl usbctrl-attach test_vm version=1 ports=8 
> > 
> >  #xl usb-list test_vm 
> >  will show the usb controllers and port usage under the domain. 
> > 
> >  #xl usbattach test_vm hostbus=1 hostaddr=2 
>  
> Nit: usb-attach (missing a '-') 

Oh, yes.

>  
> >  will find the first usable controller:port, and attach usb 
> >  device whose busnum is 1 and devnum is 6. 
> >  One could also specify which <controller> and which <port>. 
> > 
> >  #xl usb-detach test_vm 0 1 
> >  will detach USB device under controller 0 port 1. 
> > 
> >  #xl usbctrl-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> 
> > 
> > --- 
> > Changes: 
> >   - change usb-attach parameter from hostbus.hostaddr to 
> >     hostbus=xx hostaddr= 
> >   - since we get rid of libxl_device_usb_getinfo, so adjust usb-list 
> >     information a little bit. 
> >   - parse_usb_config and parse_usbctrl_config following parse_nic_config 
> >     way, put in this patch, and shared domcreate routine. 
> > 
> >  docs/man/xl.pod.1         |  40 ++++++++ 
> >  tools/libxl/xl.h          |   5 + 
> >  tools/libxl/xl_cmdimpl.c  | 250  
> ++++++++++++++++++++++++++++++++++++++++++++++ 
> >  tools/libxl/xl_cmdtable.c |  25 +++++ 
> >  4 files changed, 320 insertions(+) 
> > 
> > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 
> > index d0cd612..f09a872 100644 
> > --- a/docs/man/xl.pod.1 
> > +++ b/docs/man/xl.pod.1 
> > @@ -1345,6 +1345,46 @@ List pass-through pci devices for a domain. 
> > 
> >  =back 
> > 
> > +=head1 USB PASS-THROUGH 
> > + 
> > +=over 4 
> > + 
> > +=item B<usbctrl-attach> I<domain-id> I[<type=val>] [I<version=val>]  
> [I<ports=number>] 
> > + 
> > +Create a new USB controller for the specified domain. 
> > +B<type=val> is the usb controller type, currently only support 'pv'. 
>  
> "B<type=val> is the usb controller type.  Currently only 'pv' and 
> 'auto' are supported." 
>  
> Note the period rather than the comma. 
>  
> (See below on how to make this true.) 
>  
> > +B<version=val> is the usb controller version, could be 1 (USB1.1) or 2  
> (USB2.0). 
>  
> "<version=val> is the usb controller version.  Possible values include 
> 1 (USB1.1) and 2 (USB2.0)." 
>  
> (Same thing wrt the period.) 

Thanks. Will check all.

>  
> > +B<ports=number> is the total ports of the usb controller. 
>  
> Is there a maximum number of ports? 

Yes, according to Juergen, it should be no more than 31.

>  
> > +By default, it will create a USB2.0 controller with 8 ports. 
> > + 
> > +=item B<usbctrl-detach> I<domain-id> I<devid> 
> > + 
> > +Destroy a USB controller from the specified domain. 
> > +B<devid> is devid of the USB controller. 
> > + 
> > +If B<-f> is specified, B<xl> is going to forcefully remove the device even 
> > +without guest's collaboration. 
>  
> "If B<-f> is specified, B<xl> will forcefully remove removal (i.e., 
> without the guest's cooperation)." 

Thanks.

>  
> > + 
> > +=item B<usb-attach> I<domain-id> I<hostbus=busnum> I<hostaddr=devnum>  
> [I<controller=devid> [I<port=number>]] 
> > + 
> > +Hot-plug a new pass-through USB device to the specified domain. 
> > +B<bus.addr> is the busnum.devnum of the physical USB device to pass-through. 
>  
> "hostbus and hostaddr are the bus and device number of the physical 
> USB device to pass through." 

Thanks. Forgot to update here.

>  
> > +B<controller=devid> B<port=number> is the USB controller:port to hotplug the 
> > +USB device to. By default, it will find the first available  
> controller:port 
> > +and use it; if there is no controller, it will create one. 
> > + 
> > +=item B<usb-detach> I<domain-id> I<controller=devid> I<port=number> 
> > + 
> > +Hot-unplug a previously assigned USB device from a domain. 
> > +B<controller=devid> and B<port=number> is USB controller:port in guest where  
> the 
> > +USB device is attached to. 
> > + 
> > +=item B<usb-list> I<domain-id> 
> > + 
> > +List pass-through usb devices for a domain. 
> > + 
> > +=back 
> > + 
> >  =head1 TMEM 
> > 
> >  =over 4 
> > diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h 
> > index 0021112..ddd9690 100644 
> > --- a/tools/libxl/xl.h 
> > +++ b/tools/libxl/xl.h 
> > @@ -85,6 +85,11 @@ int main_blockdetach(int argc, char **argv); 
> >  int main_vtpmattach(int argc, char **argv); 
> >  int main_vtpmlist(int argc, char **argv); 
> >  int main_vtpmdetach(int argc, char **argv); 
> > +int main_usbctrl_attach(int argc, char **argv); 
> > +int main_usbctrl_detach(int argc, char **argv); 
> > +int main_usbattach(int argc, char **argv); 
> > +int main_usbdetach(int argc, char **argv); 
> > +int main_usblist(int argc, char **argv); 
> >  int main_uptime(int argc, char **argv); 
> >  int main_claims(int argc, char **argv); 
> >  int main_tmem_list(int argc, char **argv); 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c 
> > index 365798b..e6ff6f4 100644 
> > --- a/tools/libxl/xl_cmdimpl.c 
> > +++ b/tools/libxl/xl_cmdimpl.c 
> > @@ -1255,6 +1255,64 @@ static void parse_vnuma_config(const XLU_Config  
> *config, 
> >      free(vcpu_parsed); 
> >  } 
> > 
> > +/* Parses usbctrl data and adds info into usbctrl 
> > + * Returns 1 if the input token does not match one of the keys 
> > + * or parsed values are not correct. Successful parse returns 0 */ 
> > +static int parse_usbctrl_config(libxl_device_usbctrl *usbctrl, char  
> *token) 
> > +{ 
> > +    char *oparg; 
> > + 
> > +    if (MATCH_OPTION("type", token, oparg)) { 
> > +        if (!strcmp("pv", oparg)) { 
> > +            usbctrl->type = LIBXL_USBCTRL_TYPE_PV; 
> > +        } else { 
> > +            fprintf(stderr, 
> > +                    "Unsupported USB controller type '%s'\n", oparg); 
> > +            return 1; 
> > +        } 
>  
> One of the reasons for making the enum types in the IDL is that the 
> IDL then automatically generates functions for converting to/from 
> strings. 
>  
> So here I would do something like: 
>  
> if(libxl_usbctrl_type_from_string(optarg, &usbctrl->type)) { 
>  fprintf(stderr, "Invalid usbctrl type: %s\n", optarg); 
>  exit(1); 
> } 
>  
> (Just to emphasize -- you don't need to write the above function; it's 
> generated automatically.) 

Yes, I know. I'll adjust codes.

>  
>  
> > +/* Parses usb data and adds info into usb 
> > + * Returns 1 if the input token does not match one of the keys 
> > + * or parsed values are not correct. Successful parse returns 0 */ 
> > +static int parse_usb_config(libxl_device_usb *usb, char *token) 
> > +{ 
> > +    char *oparg; 
> > + 
> > +    if (MATCH_OPTION("devtype", token, oparg)) { 
> > +        if (!strcmp("hostdev", oparg)) { 
> > +            usb->devtype = LIBXL_USBDEV_TYPE_HOSTDEV; 
>  
> Same thing here, with libxl_usbdev_type_to_string(). 

Got it.

>  
>  
> > +int main_usbctrl_attach(int argc, char **argv) 
> > +{ 
> > +    uint32_t domid; 
> > +    int opt, rc = 0; 
> > +    libxl_device_usbctrl usbctrl; 
> > + 
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "usbctrl-attach", 1) { 
> > +        /* No options */ 
> > +    } 
> > + 
> > +    domid = find_domain(argv[optind++]); 
> > + 
> > +    libxl_device_usbctrl_init(&usbctrl); 
> > + 
> > +    for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) { 
> > +        if (parse_usbctrl_config(&usbctrl, *argv)) 
> > +            return 1; 
> > +    } 
> > + 
> > +    rc = libxl_device_usbctrl_add(ctx, domid, &usbctrl, 0); 
> > +    if (rc) { 
> > +        fprintf(stderr, "libxl_device_usbctrl_add failed.\n"); 
> > +        rc = 1; 
> > +    } 
> > + 
> > +    libxl_device_usbctrl_dispose(&usbctrl); 
> > +    return rc; 
> > +} 
> > + 
> > +int main_usbctrl_detach(int argc, char **argv) 
> > +{ 
> > +    uint32_t domid; 
> > +    int opt, devid, rc; 
> > +    libxl_device_usbctrl usbctrl; 
> > + 
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "usbctrl-detach", 2) { 
> > +        /* No options */ 
> > +    } 
> > + 
> > +    domid = find_domain(argv[optind]); 
> > +    devid = atoi(argv[optind+1]); 
> > + 
> > +    libxl_device_usbctrl_init(&usbctrl); 
> > +    if (libxl_devid_to_device_usbctrl(ctx, domid, devid, &usbctrl)) { 
> > +        fprintf(stderr, "Unknown device %s.\n", argv[optind+1]); 
> > +        return 1; 
> > +    } 
> > + 
> > +    rc = libxl_device_usbctrl_remove(ctx, domid, &usbctrl, 0); 
> > +    if (rc) { 
> > +        fprintf(stderr, "libxl_device_usbctrl_remove failed.\n"); 
> > +        rc = 1; 
> > +    } 
> > + 
> > +    libxl_device_usbctrl_dispose(&usbctrl); 
> > +    return rc; 
> > + 
> > +} 
> > + 
> > +int main_usbattach(int argc, char **argv) 
> > +{ 
> > +    uint32_t domid; 
> > +    int opt, rc; 
> > +    libxl_device_usb usb; 
> > + 
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) { 
> > +        /* No options */ 
> > +    } 
> > + 
> > +    libxl_device_usb_init(&usb); 
> > + 
> > +    domid = find_domain(argv[optind++]); 
> > + 
> > +    for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) { 
> > +        if (parse_usb_config(&usb, *argv)) 
> > +            return 1; 
> > +    } 
> > + 
> > +    rc = libxl_device_usb_add(ctx, domid, &usb, 0); 
> > +    if (rc) { 
> > +        fprintf(stderr, "libxl_device_usb_add failed.\n"); 
> > +        rc = 1; 
> > +    } 
> > + 
> > +    libxl_device_usb_dispose(&usb); 
> > +    return rc; 
> > +} 
> > + 
> > +int main_usbdetach(int argc, char **argv) 
> > +{ 
> > +    uint32_t domid; 
> > +    int ctrl, port; 
> > +    int opt, rc = 1; 
> > +    libxl_device_usb usb; 
> > + 
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-detach", 3) { 
> > +        /* No options */ 
> > +    } 
> > + 
> > +    domid = find_domain(argv[optind]); 
> > +    ctrl = atoi(argv[optind+1]); 
> > +    port = atoi(argv[optind+2]); 
> > + 
> > +    if (argc - optind > 3) { 
> > +        fprintf(stderr, "Invalid arguments.\n"); 
> > +        return 1; 
> > +    } 
> > + 
> > +    libxl_device_usb_init(&usb); 
> > +    if (libxl_ctrlport_to_device_usb(ctx, domid, ctrl, port, &usb)) { 
> > +        fprintf(stderr, "Unknown device at controller %d port %d.\n", 
> > +                ctrl, port); 
> > +        return 1; 
> > +    } 
> > + 
> > +    rc = libxl_device_usb_remove(ctx, domid, &usb, 0); 
> > +    if (rc) { 
> > +        fprintf(stderr, "libxl_device_usb_remove failed.\n"); 
> > +        rc = 1; 
> > +    } 
> > + 
> > +    libxl_device_usb_dispose(&usb); 
> > +    return rc; 
> > +} 
> > + 
> > +int main_usblist(int argc, char **argv) 
> > +{ 
> > +    uint32_t domid; 
> > +    libxl_device_usbctrl *usbctrls; 
> > +    libxl_usbctrlinfo usbctrlinfo; 
> > +    int numctrl, i, j, opt; 
> > + 
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-list", 1) { 
> > +        /* No options */ 
> > +    } 
> > + 
> > +    domid = find_domain(argv[optind++]); 
> > + 
> > +    if (argc > optind) { 
> > +        fprintf(stderr, "Invalid arguments.\n"); 
> > +        exit(-1); 
> > +    } 
> > + 
> > +    usbctrls = libxl_device_usbctrl_list(ctx, domid, &numctrl); 
> > +    if (!usbctrls) { 
> > +        return 0; 
> > +    } 
> > + 
> > +    for (i = 0; i < numctrl; ++i) { 
> > +        printf("%-6s %-6s %-3s %-5s %-7s %-5s %-30s\n", 
> > +                "Devid", "Type", "BE", "state", "usb-ver", "ports",  
> "BE-path"); 
> > + 
> > +        libxl_usbctrlinfo_init(&usbctrlinfo); 
> > + 
> > +        if (!libxl_device_usbctrl_getinfo(ctx, domid, 
> > +                                &usbctrls[i], &usbctrlinfo)) { 
> > +            printf("%-6d %-6s %-3d %-5d %-7d %-5d %-30s\n", 
> > +                    usbctrlinfo.devid, 
> > +                    libxl_usbctrl_type_to_string(usbctrlinfo.type), 
>  
> Oh, I see you already know about the "type_to_string()" functions. :-) 
>  
> > +                    usbctrlinfo.backend_id, usbctrlinfo.state, 
> > +                    usbctrlinfo.version, usbctrlinfo.ports, 
> > +                    usbctrlinfo.backend); 
> > + 
> > +            for (j = 1; j <= usbctrlinfo.ports; j++) { 
> > +                libxl_device_usb usb; 
> > +                libxl_usbinfo usbinfo; 
> > + 
> > +                libxl_device_usb_init(&usb); 
> > +                libxl_usbinfo_init(&usbinfo); 
> > + 
> > +                printf("  Port %d:", j); 
> > + 
> > +                if (!libxl_ctrlport_to_device_usb(ctx, domid, 
> > +                                                  usbctrlinfo.devid, j,  
> &usb)) { 
> > +                    printf(" Bus %03x Device %03x\n", 
> > +                           usbinfo.busnum, usbinfo.devnum); 
> > +                } else { 
> > +                    printf("\n"); 
> > +                } 
> > + 
> > +                libxl_usbinfo_dispose(&usbinfo); 
>  
> Er, what's going on with the usbinfo?  Did you take the usbinfo stuff 
> out of libxl_pvusb.c, but forget to take it out of libxl_types.idl and 
> here? 
>  
> I guess you really want usb.u.hostdev.* here. 

Yes, you're totally right. I'll adjust codes.

Thanks.

- Chunyan

>  
> Everything else looks good, thanks. 
>  
>  -George 
>  
>