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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu <cyliu@xxxxxxxx> 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 '-')

>  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@xxxxxxxx>
> Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
>
> ---
> 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.)

> +B<ports=number> is the total ports of the usb controller.

Is there a maximum number of ports?

> +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)."

> +
> +=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."

> +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.)


> +/* 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().


> +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.

Everything else looks good, thanks.

 -George

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