Re: [PATCH V7 5/7] xl: add pvusb commands

[George Dunlap <george.dunlap@citrix.com>]

On 25/09/15 03:11, 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

So all the way back in v2 of this series, I suggested making the
arguments for usb-ctrl-attach and usb-attach mirror the format that is
found in the config file[1], at which point you replied "That could be,
I can update".  But you didn't change the interface in v3, so I
suggested it again[2], and there was no argument or discussion about it.

(There was a long back-and-forth with Juergen at that point about
usb-assignable-list, so [2] may have gotten lost in the noise.)

I still think that's the best interface to use.  Do you have reasons to
favor the interface you propose here?

 -George

[1]
marc.info/?i=<CAFLBxZb1N3_9PVvg-yC8dyVaiySZVRA3H2e8496vHNEfvrm6Zg@mail.gmail.com>

[2]
marc.info/?i=<CAFLBxZbJcdFi2cXOvQRncbdV0HBL73d8sjXKry+VaRjvkQtRwA@mail.gmail.com>

> 
>  #xl usb-list test_vm
>  will show the usb controllers and port usage under the domain.
> 
>  #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 0 1
>  will detach USB device under controller 0 port 1.
> 
>  #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         |  40 ++++++++
>  tools/libxl/xl.h          |   5 +
>  tools/libxl/xl_cmdimpl.c  | 232 ++++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/xl_cmdtable.c |  25 +++++
>  4 files changed, 302 insertions(+)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index f22c3f3..4c92c78 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<usb-ctrl-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<version=val> is the usb controller version, could be 1 (USB1.1) or 2 (USB2.0).
> +B<ports=number> is the total ports of the usb controller.
> +By default, it will create a USB2.0 controller with 8 ports.
> +
> +=item B<usb-ctrl-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.
> +
> +=item B<usb-attach> I<domain-id> I<bus.addr> [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.
> +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 6c19c0d..26f6c1e 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 2706759..6ae9479 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3367,6 +3367,238 @@ int main_cd_insert(int argc, char **argv)
>      return 0;
>  }
>  
> +int main_usbctrl_attach(int argc, char **argv)
> +{
> +    uint32_t domid;
> +    int opt, rc = 1;
> +    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.type = LIBXL_USBCTRL_TYPE_PV;
> +            } else {
> +                fprintf(stderr, "unsupported type `%s'\n", oparg);
> +                goto out;
> +            }
> +        } else if (MATCH_OPTION("version", argv[optind], oparg)) {
> +            usbctrl.version = atoi(oparg);
> +            if (usbctrl.version != 1 && usbctrl.version != 2) {
> +                fprintf(stderr, "unsupported version `%s'\n", oparg);
> +                goto out;
> +            }
> +        } else if (MATCH_OPTION("ports", argv[optind], oparg)) {
> +            usbctrl.ports = atoi(oparg);
> +            if (usbctrl.ports < 1 || usbctrl.ports > 31) {
> +                fprintf(stderr, "unsupported ports `%s'\n", oparg);
> +                goto out;
> +            }
> +        } else {
> +            fprintf(stderr, "unrecognized argument `%s'\n", argv[optind]);
> +            goto out;
> +        }
> +        optind++;
> +    }
> +
> +    rc = libxl_device_usbctrl_add(ctx, domid, &usbctrl, 0);
> +    if (rc)
> +        fprintf(stderr, "libxl_device_usbctrl_add failed.\n");
> +
> +out:
> +    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, "usb-ctrl-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");
> +
> +    libxl_device_usbctrl_dispose(&usbctrl);
> +    return rc;
> +
> +}
> +
> +int main_usbattach(int argc, char **argv)
> +{
> +    uint32_t domid;
> +    char *devname, *p;
> +    int opt, rc = 1;
> +    char *oparg;
> +    libxl_device_usb usb;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) {
> +        /* No options */
> +    }
> +
> +    libxl_device_usb_init(&usb);
> +
> +    domid = find_domain(argv[optind++]);
> +    devname = argv[optind++];
> +    p = strchr(devname, '.');
> +    if (p) {
> +        usb.u.hostdev.hostbus = strtoul(devname, NULL, 0);
> +        usb.u.hostdev.hostaddr = strtoul(p + 1, NULL, 0);
> +    }
> +
> +    if (usb.u.hostdev.hostbus < 1 || usb.u.hostdev.hostaddr < 1) {
> +        fprintf(stderr, "Invalid usb device.\n");
> +        goto out;
> +    }
> +
> +    while (argc > optind) {
> +        if (MATCH_OPTION("controller", argv[optind], oparg)) {
> +            usb.ctrl = atoi(oparg);
> +        } else if (MATCH_OPTION("port", argv[optind], oparg)) {
> +            usb.port = atoi(oparg);
> +        } else {
> +            fprintf(stderr, "unrecognized argument `%s'\n", argv[optind]);
> +            goto out;
> +        }
> +        optind++;
> +    }
> +
> +    rc = libxl_device_usb_add(ctx, domid, &usb, 0);
> +    if (rc)
> +        fprintf(stderr, "libxl_device_usb_add failed.\n");
> +
> +out:
> +    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");
> +        goto out;
> +    }
> +
> +    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);
> +        goto out;
> +    }
> +
> +    rc = libxl_device_usb_remove(ctx, domid, &usb, 0);
> +    if (rc)
> +        fprintf(stderr, "libxl_device_usb_remove failed.\n");
> +
> +out:
> +    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),
> +                    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);
> +
> +                usb.ctrl = usbctrlinfo.devid;
> +                usb.port = j;
> +                if (!libxl_device_usb_getinfo(ctx, domid, &usb, &usbinfo)) {
> +                    printf(" Bus %03x Device %03x: ID %04x:%04x %s %s\n",
> +                            usbinfo.busnum, usbinfo.devnum,
> +                            usbinfo.idVendor, usbinfo.idProduct,
> +                            usbinfo.manuf ?: "", usbinfo.prod ?: "");
> +                } else {
> +                    printf("\n");
> +                }
> +                libxl_usbinfo_dispose(&usbinfo);
> +                libxl_device_usb_dispose(&usb);
> +            }
> +        }
> +
> +        libxl_usbctrlinfo_dispose(&usbctrlinfo);
> +    }
> +
> +    libxl_device_usbctrl_list_free(usbctrls, numctrl);
> +    return 0;
> +}
> +
>  int main_console(int argc, char **argv)
>  {
>      uint32_t domid;
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 0071f12..46f276e 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -551,6 +551,31 @@ struct cmd_spec cmd_table[] = {
>      },
>  
>  #endif
> +    { "usb-ctrl-attach",
> +      &main_usbctrl_attach, 0, 1,
> +      "Create a virtual USB controller for a domain",
> +      "<Domain> [type=pv] [version=<version>] [ports=<number>]",
> +    },
> +    { "usb-ctrl-detach",
> +      &main_usbctrl_detach, 0, 1,
> +      "Remove the virtual USB controller specified by <DevId> for a domain",
> +      "<Domain> <DevId>",
> +    },
> +    { "usb-attach",
> +      &main_usbattach, 0, 1,
> +      "Attach a USB device to a domain",
> +      "<Domain> <bus.addr> [controller=<DevId> [port=<port>]]",
> +    },
> +    { "usb-detach",
> +      &main_usbdetach, 0, 1,
> +      "Detach a USB device from a domain",
> +      "<Domain> <controller> <port>",
> +    },
> +    { "usb-list",
> +      &main_usblist, 0, 0,
> +      "List information about USB devices for a domain",
> +      "<Domain>",
> +    },
>  };
>  
>  int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);
>