[Xen-devel] çåï Re: [PATCH V3 3/6] libxl: add pvusb API

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

>>> å 2:06 ç 2015/5/20 äïåèæ
<CAFLBxZZHFFWo6Tm7602y3+x5kX65-w4obFS1VdVb8KqnzdAmDg@xxxxxxxxxxxxxx> äïGeorge
Dunlap <George.Dunlap@xxxxxxxxxxxxx> ååï
> On Sun, Apr 19, 2015 at 4:50 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote:
> > Add pvusb APIs, including:
> >  - attach/detach (create/destroy) virtual usb controller.
> >  - attach/detach usb device
> >  - list usb controller and usb devices
> >  - some other helper functions
> >
> > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx>
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
> 
> OK, getting closer. :-)
> 
> A number of comments:

Hi, George, thanks very much!
I'm so sorry for missing the message and not reply immediately.
Before sending new version, I'm answering some of your questions here.
And there are a couple of comments, I still have some hesitation to follow.
All others, I agree and will update as you suggested.

> 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 6bc75c5..cbe3519 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -114,6 +114,12 @@
> >  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >
> >  /*
> > + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
> > + * USB devices through pvusb.
> > + */
> > +#define LIBXL_HAVE_PVUSB 1
> 
> It seems like we should document somewhere how we expect these to be
> used -- namely the connection between usbctrl and usb devices.  In
> particular, that you can either add a usbctrl, and then add a usb
> device to it specifically; or if you don't specify a usbctrl when
> calling usb_add, it will find the most reasonable one to add it to,
> even creating one for you if you didn't have one.
> 
> 
> > diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
> > new file mode 100644
> > index 0000000..4e4975a
> > --- /dev/null
> > +++ b/tools/libxl/libxl_pvusb.c
> 
> > +int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid,
> > +                              libxl_device_usbctrl *usbctrl)
> > +{
> > +    int rc = 0;
> > +
> > +    rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
> > +    if (rc < 0) goto out;
> > +
> > +    if (usbctrl->devid == -1) {
> 
> Hmm, I was doing to say that this shouldn't be a magic constant, but
> it looks like that's what all the other devices do :-/
> 
> > +static bool is_usb_in_array(libxl_device_usb *usbs, int num,
> > +                            libxl_device_usb *usb)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < num; i++) {
> > +        if (!strcmp(usbs[i].busid, usb->busid) )
> 
> Here (and elsewhere) you should probably use the COMPARE_USB() macro
> you define in this patch.
> 
> > +/* check if USB device type is assignable */
> > +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
> > +{
> > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    int classcode;
> > +    char *filename;
> > +    void *buf;
> > +
> > +    assert(usb->busid);
> > +
> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", usb->busid);
> > +    if (libxl_read_file_contents(ctx, filename, &buf, NULL) < 0)
> > +        return false;
> > +
> > +    sscanf(buf, "%x", &classcode);
> > +    if (classcode == USBHUB_CLASS_CODE)
> > +        return false;
> > +
> > +    return true;
> 
> Just checking, is it really the case that *all* USB classes are
> assignable, *except* for hubs?

Referring to xm pvusb implementation, only HUB is excluded, so I
just keep the same.

> 
> This is a minor thing, but this might be simper to do this:
> 
>  return classcode != USBHUB_CLASS_CODE;
> 
> > +/* get usb devices under certain usb controller */
> > +static int libxl__device_usb_list(libxl__gc *gc, uint32_t domid, int 
> usbctrl,
> > +                                  libxl_device_usb **usbs, int *num)
> > +{
> > +    char *be_path, *num_devs;
> > +    int n, i;
> > +
> > +    *usbs = NULL;
> > +    *num = 0;
> > +
> > +    be_path = GCSPRINTF("%s/backend/vusb/%d/%d",
> > +                        libxl__xs_get_dompath(gc, 0), domid, usbctrl);
> > +    num_devs = libxl__xs_read(gc, XBT_NULL,
> > +                              GCSPRINTF("%s/num-ports", be_path));
> > +    if (!num_devs)
> > +        return 0;
> > +
> > +    n = atoi(num_devs);
> > +    *usbs = calloc(n, sizeof(libxl_device_usb));
> > +
> > +    for (i = 0; i < n; i++) {
> > +        char *busid;
> > +        libxl_device_usb *usb = NULL;
> > +
> > +        busid = libxl__xs_read(gc, XBT_NULL,
> > +                               GCSPRINTF("%s/port/%d", be_path, i + 1));
> > +        if (busid && strcmp(busid, "")) {
> > +            usb = *usbs + *num;
> > +            usb->ctrl = usbctrl;
> > +            usb->port = i + 1;
> > +            usb->busid = strdup(busid);
> 
> This needs to populate the hostbus / hostaddr as well; busid is pretty
> useless to users / external callers.

I thought about that when implementing, but finally not added to codes 
considering:
* for all libxl pvusb internal usage, busid is enough.
* for toolstack usage, if we want to expose users useful information about 
bus:addr,
vendorID:devieID, we have libxl_device_usb_getinfo API, with this API callers 
can get
all information including hostbus, hostaddr.

If that couldn't satisfy qemu usage, I can add a translating to fill in hostbus 
and
hostaddr too.

> 
> > +/* get all usb devices of the domain */
> > +static libxl_device_usb *
> > +libxl_device_usb_list_all(libxl__gc *gc, uint32_t domid, int *num)
> > +{
> > +    char **usbctrls;
> > +    unsigned int nd, i, j;
> > +    char *be_path;
> > +    int rc;
> > +    libxl_device_usb *usbs = NULL;
> > +
> > +    *num = 0;
> > +
> > +    be_path = GCSPRINTF("/local/domain/%d/backend/vusb/%d",
> > +                        LIBXL_TOOLSTACK_DOMID, domid);
> > +    usbctrls = libxl__xs_directory(gc, XBT_NULL, be_path, &nd);
> > +
> > +    for (i = 0; i < nd; i++) {
> > +        int nc = 0;
> > +        libxl_device_usb *tmp = NULL;
> > +        rc = libxl__device_usb_list(gc, domid, atoi(usbctrls[i]), &tmp, 
> &nc);
> > +        if (!nc) continue;
> > +
> > +        usbs = realloc(usbs, sizeof(libxl_device_usb)*((*num) + nc));
> > +        for(j = 0; j < nc; j++) {
> > +            usbs[*num].ctrl = tmp[j].ctrl;
> > +            usbs[*num].port = tmp[j].port;
> > +            usbs[*num].busid = strdup(tmp[j].busid);
> 
> This needs to copy the hostaddr and busaddr as well, as these are
> primarily what an external caller will want.

Same to above.

> 
> > +/* find first unused controller:port and give that to usb device */
> > +static int
> > +libxl__device_usb_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
> > +                                      libxl_device_usb *usb)
> > +{
> > +    libxl_ctx *ctx = CTX;
> > +    libxl_device_usbctrl *usbctrls;
> > +    libxl_device_usb *usbs = NULL;
> > +    int numctrl, numusb, i, j, rc = -1;
> > +    char *be_path, *tmp;
> > +
> > +    usbctrls = libxl_device_usbctrl_list(ctx, domid, &numctrl);
> > +    if ( !numctrl)
> > +        goto out;
> > +
> > +    for (i = 0; i < numctrl; i++) {
> > +        rc = libxl__device_usb_list(gc, domid, usbctrls[i].devid,
> > +                                    &usbs, &numusb);
> > +        if (rc) continue;
> > +
> > +        if (!usbctrls[i].ports || numusb == usbctrls[i].ports)
> > +            continue;
> > +
> > +        for (j = 1; i <= numusb; j++) {
> 
> Port numbers start at 1, do they?  Interesting...

Keep same as xm implementation. I think that should be proved right.

> 
> Er, but isn't the middle thing just plain wrong?  For one, you want to
> be comparing j not i.  I can't see that i is updated inside the loop,
> so ATM this will loop forever.
> 
> For two, you want to compare to usbctrls[i].ports (the total number of
> ports), not to numusb (the number of currently assigned devices).
> 
> > +static int libxl__device_usb_setdefault(libxl__gc *gc, uint32_t domid,
> > +                                        libxl_device_usb *usb)
> > +{
> > +    char *be_path, *tmp;
> > +
> > +    if (usb->ctrl == -1) {
> 
> Oh, right -- libxl_devid's default to -1, don't they?  I'll save the
> grumbling about the lack of a #define here then. (Or rather, I'll
> grumble at the library rather than you.)
> 
> > +        int ret = libxl__device_usb_set_default_usbctrl(gc, domid, usb);
> > +        /* If no existing ctrl to host this usb device, setup a new one */
> > +        if (ret) {
> > +            libxl_device_usbctrl usbctrl;
> > +            libxl_device_usbctrl_init(&usbctrl);
> > +            libxl__device_usbctrl_add(gc, domid, &usbctrl);
> 
> I think in the previous round I asked what would happen if this
> failed, and you said you would fail later, but that you'd change it to
> check for an error here and bail out earlier.
> 
> > +/* Cann't write '.' or ':' into Xenstore as key. So, change '.' to '_',
> > + * change ':' to '-'.
> > + */
> > +static char *usb_interface_encode(char *busid)
> 
> Maybe usb_interface_xenstore_encode, to make it clear that you're just
> encoding it to store in xenstore?
> 
> > +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb)
> > +{
> > +    char **intfs = NULL;
> > +    char *path;
> > +    int num = 0, i;
> > +    int rc = 0;
> > +    char *usb_encode = NULL;
> > +
> > +    if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
> > +        return ERROR_FAIL;
> > +
> > +    usb_encode = usb_interface_encode(usb->busid);
> > +
> > +    for (i = 0; i < num; i++){
> > +        char *intf = intfs[i];
> > +        char *drvpath = NULL;
> > +
> > +        if (usb_intf_is_assigned(gc, intf) > 0) {
> > +            /* unbind interface from usbback driver */
> > +            if (unbind_usb_intf(gc, intf, NULL) < 0) {
> > +                rc = ERROR_FAIL;
> > +                goto out;
> > +            }
> > +        }
> > +
> > +        /* bind interface to its originial driver */
> > +        drvpath = libxl__xs_read(gc, XBT_NULL,
> > +                  GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",
> > +                  usb_encode, usb_interface_encode(intf)));
> > +        if (drvpath) {
> > +            if (bind_usb_intf(gc, intf, drvpath) < 0) {
> > +                LOGE(ERROR, "Couldn't bind %s to %s", intf, drvpath);
> > +                rc = ERROR_FAIL;
> > +                goto out;
> 
> I think this function probably shouldn't fail if it can't re-bind to
> the original driver.  If nothing else, this will be bad because now
> the USB device has at least one of its interfaces unbound from
> usbback, but the other ones still bound to it.  All interfaces should
> be unbound from usbback regardless.
> 
> I also think that while a warning should be logged, that the function
> as a whole should return success if it managed to unbind, even if it
> didn't manage to rebind.  (But feel free to argue otherwise.)
> 
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index 0866433..a6db614 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -533,6 +533,22 @@ libxl_device_pci = Struct("device_pci", [
> >      ("seize", bool),
> >      ])
> >
> > +libxl_device_usbctrl = Struct("device_usbctrl", [
> > +    ("devid", libxl_devid),
> > +    ("version", integer),
> > +    ("ports", integer),
> > +    ("backend_domid", libxl_domid),
> > +    ("backend_domname", string),
> > +   ])
> > +
> > +libxl_device_usb = Struct("device_usb", [
> > +    ("ctrl", libxl_devid),
> > +    ("port", integer),
> > +    ("busid", string),
> 
> So it looks like "busid" is purely an internal string that you're
> putting in the device struct for convenience, so that you don't have
> to either keep translating bus:addr into the sysfs node, or pass
> around the second string as well.  Is that right?

Yes, right. Actually in libxl pvusb, internally it uses busid, hostbus:hostaddr
is only a toolstack interface.

It is 'busid' saved in xenstore for pvusb driver's usage, and everywhere 
comparing
work (e.g. check usb device existence before removing a usb device) it uses
'busid' to compare. So having a struct element is very convenient, otherwise, 
there
are many places that need to translate from busid to/from bus:addr.

Personally I still prefer to keep 'busid', but if that's really unacceptable,
I can also update.

- Chunyan

> 
> I think libxl does something similar to this with "backend_domname"
> and "backend_domid"; but in that case I believe you *can* specify the
> backend_domid if you want to.  In this case, you're exposing a struct
> element which is clobbered immediately, at least by usb_add and
> usb_remove.
> 
> Maintainers, what do you think?
> 
> Other than that, I think this patch as-is looks in pretty good shape
> (haven't taken a close look at the rest in the series yet); the
> primary things are making the "hostbus:hostaddr" actually the primary
> interface; at a minimum filling in that information when doing
> queries.  I'd ideally getting rid of "busid" from the external
> interface altogether.
> 
>  -George
> 
> _______________________________________________
> 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