Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
>>> On 3/3/2016 at 05:27 PM, in message , George Dunlap wrote: > On Thu, Mar 3, 2016 at 2:59 AM, Chun Yan Liu wrote: > On 3/3/2016 at 02:46 AM, in message <56d7350f.7010...@citrix.com>, George > > Dunlap wrote: > >> Sorry, just looked through the rest of the series, and there's one more > >> thing. > >> > >> Neither here nor in the man page do we explain what to do if something > >> goes wrong with the detach. I think the best thing to do is probably to > >> make the logged error messages more helpful. > >> > >> What about something like this: > >> > >> * On failure to unbind: "Error removing device from guest. Try running > >> usbdev-detach again." > >> > >> * On failure to rebind: "USB device removed from guest, but couldn't > >> re-bind to domain 0. Try removing and re-inserting the USB device or > >> reloading the driver modules." > > Here, user could first try 'echo xxx > sysfs_driver_path/bind", so maybe: > > "Try binding USB device to original driver by echoing the device to > > [driver_path]/bind, or removing and re-inserting the USB device, or > > reloading the driver modules." > > Yes, I had thought about the idea of giving the user specific commands > to retry. The question is how much we can expect the user to do to > recover the state. At some point "reloading the driver module" was > seen as something reasonable for a reasonably advanced user, while > "messing around with sysfs" was seen to be something too technical. > But as you say, giving them the exact command to cut-and-paste might > be somewhat more reasonable. > > I'm still inclined to say that we should just stick with module > reloading and removing and re-inserting the device, but I wouldn't > insist on it. I see. Just use what you suggested. Will update and repost. Thanks, Chunyan > > If we do print this kind of help message, then we should make sure to > print a more complete message that includes cut-and-paste commands for > *all* remaining unbound interfaces. > > -George > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
>>> On 3/3/2016 at 05:20 PM, in message , George Dunlap wrote: > On Thu, Mar 3, 2016 at 3:11 AM, Chun Yan Liu wrote: > On 3/3/2016 at 02:32 AM, in message <56d731b1.60...@citrix.com>, George > Dunlap > > wrote: > >> On 01/03/16 08:09, Chunyan Liu wrote: > >> > +static int usbdev_rebind(libxl__gc *gc, const char *busid) > >> > +{ > >> > +char **intfs = NULL; > >> > +char *usbdev_encode = NULL; > >> > +char *path = NULL; > >> > +int i, num = 0; > >> > +int rc; > >> > + > >> > +rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); > >> > +if (rc) goto out; > >> > + > >> > +usbdev_encode = usb_interface_xenstore_encode(gc, busid); > >> > + > >> > +for (i = 0; i < num; i++) { > >> > +char *intf = intfs[i]; > >> > +char *usbintf_encode = NULL; > >> > +const char *drvpath; > >> > + > >> > +/* rebind USB interface to its originial driver */ > >> > +usbintf_encode = usb_interface_xenstore_encode(gc, intf); > >> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", > >> > + usbdev_encode, usbintf_encode); > >> > +rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); > >> > +if (rc) goto out; > >> > + > >> > +if (drvpath) { > >> > +rc = bind_usbintf(gc, intf, drvpath); > >> > +if (rc) { > >> > +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); > >> > +goto out; > >> > +} > >> > +} > >> > +} > >> > + > >> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); > >> > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); > >> > + > >> > +out: > >> > >> So it looks like if one of the re-binds fails, then it stops where it is > >> and leaves the USBBACK re-bind info in xenstore. In that case it's not > >> clear to me how that information would ever be removed. > >> > >> I think until such time as we have a command to re-attempt the re-bind, > >> if there's an error in the actual rebind, it should just break out of > >> the for loop, and remove the re-bind nodes, and document a way to let > >> the user try to clean things up. > > > > Just according to last time discussion about how to handle the rebind > > failure, seems Ian preferred to add a xl command to deal with rebind > > in future, based on that need, I think driver_path info should be kept > > in xenstore then. Without that need, I agree it's best to remove > > xenstore nodes. So, keep or remove? > > Well when we have such a command, then yes, we'll need to keep the > nodes for it to use and delete. But at the moment, we have no such > command, so these nodes will just sit around cluttering up the libxl > xenstore space, and nothing will delete them (unless a user manually > runs xenstore-rm). > > So I think for now we should delete them on failure; and at some point > later, when someone implements a recovery command, then they should > change this code to not delete the xenstore nodes (and also change the > log messages to point to that command). OK. Got it. Will update. - Chunyan > > -George > > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
On Thu, Mar 3, 2016 at 2:59 AM, Chun Yan Liu wrote: On 3/3/2016 at 02:46 AM, in message <56d7350f.7010...@citrix.com>, George > Dunlap wrote: >> Sorry, just looked through the rest of the series, and there's one more >> thing. >> >> Neither here nor in the man page do we explain what to do if something >> goes wrong with the detach. I think the best thing to do is probably to >> make the logged error messages more helpful. >> >> What about something like this: >> >> * On failure to unbind: "Error removing device from guest. Try running >> usbdev-detach again." >> >> * On failure to rebind: "USB device removed from guest, but couldn't >> re-bind to domain 0. Try removing and re-inserting the USB device or >> reloading the driver modules." > Here, user could first try 'echo xxx > sysfs_driver_path/bind", so maybe: > "Try binding USB device to original driver by echoing the device to > [driver_path]/bind, or removing and re-inserting the USB device, or > reloading the driver modules." Yes, I had thought about the idea of giving the user specific commands to retry. The question is how much we can expect the user to do to recover the state. At some point "reloading the driver module" was seen as something reasonable for a reasonably advanced user, while "messing around with sysfs" was seen to be something too technical. But as you say, giving them the exact command to cut-and-paste might be somewhat more reasonable. I'm still inclined to say that we should just stick with module reloading and removing and re-inserting the device, but I wouldn't insist on it. If we do print this kind of help message, then we should make sure to print a more complete message that includes cut-and-paste commands for *all* remaining unbound interfaces. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
On Thu, Mar 3, 2016 at 3:11 AM, Chun Yan Liu wrote: On 3/3/2016 at 02:32 AM, in message <56d731b1.60...@citrix.com>, George Dunlap > wrote: >> On 01/03/16 08:09, Chunyan Liu wrote: >> > +static int usbdev_rebind(libxl__gc *gc, const char *busid) >> > +{ >> > +char **intfs = NULL; >> > +char *usbdev_encode = NULL; >> > +char *path = NULL; >> > +int i, num = 0; >> > +int rc; >> > + >> > +rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); >> > +if (rc) goto out; >> > + >> > +usbdev_encode = usb_interface_xenstore_encode(gc, busid); >> > + >> > +for (i = 0; i < num; i++) { >> > +char *intf = intfs[i]; >> > +char *usbintf_encode = NULL; >> > +const char *drvpath; >> > + >> > +/* rebind USB interface to its originial driver */ >> > +usbintf_encode = usb_interface_xenstore_encode(gc, intf); >> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", >> > + usbdev_encode, usbintf_encode); >> > +rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); >> > +if (rc) goto out; >> > + >> > +if (drvpath) { >> > +rc = bind_usbintf(gc, intf, drvpath); >> > +if (rc) { >> > +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); >> > +goto out; >> > +} >> > +} >> > +} >> > + >> > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); >> > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); >> > + >> > +out: >> >> So it looks like if one of the re-binds fails, then it stops where it is >> and leaves the USBBACK re-bind info in xenstore. In that case it's not >> clear to me how that information would ever be removed. >> >> I think until such time as we have a command to re-attempt the re-bind, >> if there's an error in the actual rebind, it should just break out of >> the for loop, and remove the re-bind nodes, and document a way to let >> the user try to clean things up. > > Just according to last time discussion about how to handle the rebind > failure, seems Ian preferred to add a xl command to deal with rebind > in future, based on that need, I think driver_path info should be kept > in xenstore then. Without that need, I agree it's best to remove > xenstore nodes. So, keep or remove? Well when we have such a command, then yes, we'll need to keep the nodes for it to use and delete. But at the moment, we have no such command, so these nodes will just sit around cluttering up the libxl xenstore space, and nothing will delete them (unless a user manually runs xenstore-rm). So I think for now we should delete them on failure; and at some point later, when someone implements a recovery command, then they should change this code to not delete the xenstore nodes (and also change the log messages to point to that command). -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
>>> On 3/3/2016 at 02:32 AM, in message <56d731b1.60...@citrix.com>, George >>> Dunlap wrote: > On 01/03/16 08:09, Chunyan Liu 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: Simon Cao > > Signed-off-by: George Dunlap > > Signed-off-by: Chunyan Liu > > --- > > Changes: > > reorder usbdev_remove to following three steps: > > 1. Unassign all interfaces from usbback, stopping and returning an > > error as soon as one attempt fails > > 2. Remove the pvusb xenstore nodes, stopping and returning an error > > if it fails > > 3. Attempt to re-assign all interfaces to the original drivers, > > stopping and returning an error as soon as one attempt fails. > > Thanks, Chunyan! One minor comment about these changes... > > > +static int usbdev_rebind(libxl__gc *gc, const char *busid) > > +{ > > +char **intfs = NULL; > > +char *usbdev_encode = NULL; > > +char *path = NULL; > > +int i, num = 0; > > +int rc; > > + > > +rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); > > +if (rc) goto out; > > + > > +usbdev_encode = usb_interface_xenstore_encode(gc, busid); > > + > > +for (i = 0; i < num; i++) { > > +char *intf = intfs[i]; > > +char *usbintf_encode = NULL; > > +const char *drvpath; > > + > > +/* rebind USB interface to its originial driver */ > > +usbintf_encode = usb_interface_xenstore_encode(gc, intf); > > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", > > + usbdev_encode, usbintf_encode); > > +rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); > > +if (rc) goto out; > > + > > +if (drvpath) { > > +rc = bind_usbintf(gc, intf, drvpath); > > +if (rc) { > > +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); > > +goto out; > > +} > > +} > > +} > > + > > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); > > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); > > + > > +out: > > So it looks like if one of the re-binds fails, then it stops where it is > and leaves the USBBACK re-bind info in xenstore. In that case it's not > clear to me how that information would ever be removed. > > I think until such time as we have a command to re-attempt the re-bind, > if there's an error in the actual rebind, it should just break out of > the for loop, and remove the re-bind nodes, and document a way to let > the user try to clean things up. Just according to last time discussion about how to handle the rebind failure, seems Ian preferred to add a xl command to deal with rebind in future, based on that need, I think driver_path info should be kept in xenstore then. Without that need, I agree it's best to remove xenstore nodes. So, keep or remove? [Post last time Ian's idea] [start] The only wrinkle is that the obvious implementation reads the old bindings from xenstore between 1 and 2, deletes the information from xenstore in 2, and uses that information in step 3, which is cheating (and leads to the sysfs-wrangling-required state). But that is quite easy to avoid: - record the old driver bindings somewhere in xenstore (keyed by the physical host device, not the guest domain) - provide a libxl call and corresponding xl command which attempts to rebind But, FAOD, I do not want to block this patch until such a thing is implemented. I think it is sufficient to document the existence of the awkward state, and the likely workarounds. [end] > > > +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, > > +libxl_device_usbdev *usbdev) > > +{ > > +int rc; > > +char *busid; > > +libxl_device_usbctrl usbctrl; > > +libxl_usbctrlinfo usbctrlinfo; > > + > > +libxl_device_usbctrl_init(&usbctrl); > > +libxl_usbctrlinfo_init(&usbctrlinfo); > > +usbctrl.devid = usbdev->ctrl; > > + > > +rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo); > > +if (rc) goto out; > > + > > +switch (usbctrlinfo.type) { > > +case LIBXL_USBCTRL_TYPE_PV: > > +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); > > +if (!busid) { > > +rc = ERROR_FAIL; > > +goto out; > > +} > > + > > +rc = usbback_dev_unassign(gc, busid); > > +if (rc) goto out; > > + > > +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); > > +if (rc) goto out; > > + > > +rc = usbdev_rebind(gc, busid); > > +if (rc) goto out; > > I think we need a comment here saying why we're doing things in this > order. Maybe: > >
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
>>> On 3/3/2016 at 02:46 AM, in message <56d7350f.7010...@citrix.com>, George Dunlap wrote: > On 02/03/16 18:32, George Dunlap wrote: > > On 01/03/16 08:09, Chunyan Liu 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: Simon Cao > >> Signed-off-by: George Dunlap > >> Signed-off-by: Chunyan Liu > >> --- > >> Changes: > >> reorder usbdev_remove to following three steps: > >> 1. Unassign all interfaces from usbback, stopping and returning an > >> error as soon as one attempt fails > >> 2. Remove the pvusb xenstore nodes, stopping and returning an error > >> if it fails > >> 3. Attempt to re-assign all interfaces to the original drivers, > >> stopping and returning an error as soon as one attempt fails. > > > > Thanks, Chunyan! One minor comment about these changes... > > > >> +static int usbdev_rebind(libxl__gc *gc, const char *busid) > >> +{ > >> +char **intfs = NULL; > >> +char *usbdev_encode = NULL; > >> +char *path = NULL; > >> +int i, num = 0; > >> +int rc; > >> + > >> +rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); > >> +if (rc) goto out; > >> + > >> +usbdev_encode = usb_interface_xenstore_encode(gc, busid); > >> + > >> +for (i = 0; i < num; i++) { > >> +char *intf = intfs[i]; > >> +char *usbintf_encode = NULL; > >> +const char *drvpath; > >> + > >> +/* rebind USB interface to its originial driver */ > >> +usbintf_encode = usb_interface_xenstore_encode(gc, intf); > >> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", > >> + usbdev_encode, usbintf_encode); > >> +rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); > >> +if (rc) goto out; > >> + > >> +if (drvpath) { > >> +rc = bind_usbintf(gc, intf, drvpath); > >> +if (rc) { > >> +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); > >> +goto out; > >> +} > >> +} > >> +} > >> + > >> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); > >> +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); > >> + > >> +out: > > > > So it looks like if one of the re-binds fails, then it stops where it is > > and leaves the USBBACK re-bind info in xenstore. In that case it's not > > clear to me how that information would ever be removed. > > > > I think until such time as we have a command to re-attempt the re-bind, > > if there's an error in the actual rebind, it should just break out of > > the for loop, and remove the re-bind nodes, and document a way to let > > the user try to clean things up. > > > >> +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, > >> +libxl_device_usbdev *usbdev) > >> +{ > >> +int rc; > >> +char *busid; > >> +libxl_device_usbctrl usbctrl; > >> +libxl_usbctrlinfo usbctrlinfo; > >> + > >> +libxl_device_usbctrl_init(&usbctrl); > >> +libxl_usbctrlinfo_init(&usbctrlinfo); > >> +usbctrl.devid = usbdev->ctrl; > >> + > >> +rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, > >> &usbctrlinfo); > >> +if (rc) goto out; > >> + > >> +switch (usbctrlinfo.type) { > >> +case LIBXL_USBCTRL_TYPE_PV: > >> +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); > >> +if (!busid) { > >> +rc = ERROR_FAIL; > >> +goto out; > >> +} > >> + > >> +rc = usbback_dev_unassign(gc, busid); > >> +if (rc) goto out; > >> + > >> +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); > >> +if (rc) goto out; > >> + > >> +rc = usbdev_rebind(gc, busid); > >> +if (rc) goto out; > > > > I think we need a comment here saying why we're doing things in this > > order. Maybe: > > > > "Things are done in this order to balance simplicity with robustness in > > the case of failure: > > * We unbind all interfaces before rebinding any interfaces, so that we > > never get into a situation where some interfaces are assigned to usbback > > and some are assigned to the original drivers. > > * We also unbind the interfaces before removing the pvusb xenstore > > nodes, so that if the unbind fails in the middle, the device still shows > > up in xl usb-list, and the user can re-try removing it." > > Sorry, just looked through the rest of the series, and there's one more > thing. > > Neither here nor in the man page do we explain what to do if something > goes wrong with the detach. I think the best thing to do is probably to > make the logged error messages more helpful. > > What about something like this: > > * On fai
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
On 02/03/16 18:32, George Dunlap wrote: > On 01/03/16 08:09, Chunyan Liu 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: Simon Cao >> Signed-off-by: George Dunlap >> Signed-off-by: Chunyan Liu >> --- >> Changes: >> reorder usbdev_remove to following three steps: >> 1. Unassign all interfaces from usbback, stopping and returning an >> error as soon as one attempt fails >> 2. Remove the pvusb xenstore nodes, stopping and returning an error >> if it fails >> 3. Attempt to re-assign all interfaces to the original drivers, >> stopping and returning an error as soon as one attempt fails. > > Thanks, Chunyan! One minor comment about these changes... > >> +static int usbdev_rebind(libxl__gc *gc, const char *busid) >> +{ >> +char **intfs = NULL; >> +char *usbdev_encode = NULL; >> +char *path = NULL; >> +int i, num = 0; >> +int rc; >> + >> +rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); >> +if (rc) goto out; >> + >> +usbdev_encode = usb_interface_xenstore_encode(gc, busid); >> + >> +for (i = 0; i < num; i++) { >> +char *intf = intfs[i]; >> +char *usbintf_encode = NULL; >> +const char *drvpath; >> + >> +/* rebind USB interface to its originial driver */ >> +usbintf_encode = usb_interface_xenstore_encode(gc, intf); >> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", >> + usbdev_encode, usbintf_encode); >> +rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); >> +if (rc) goto out; >> + >> +if (drvpath) { >> +rc = bind_usbintf(gc, intf, drvpath); >> +if (rc) { >> +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); >> +goto out; >> +} >> +} >> +} >> + >> +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); >> +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); >> + >> +out: > > So it looks like if one of the re-binds fails, then it stops where it is > and leaves the USBBACK re-bind info in xenstore. In that case it's not > clear to me how that information would ever be removed. > > I think until such time as we have a command to re-attempt the re-bind, > if there's an error in the actual rebind, it should just break out of > the for loop, and remove the re-bind nodes, and document a way to let > the user try to clean things up. > >> +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, >> +libxl_device_usbdev *usbdev) >> +{ >> +int rc; >> +char *busid; >> +libxl_device_usbctrl usbctrl; >> +libxl_usbctrlinfo usbctrlinfo; >> + >> +libxl_device_usbctrl_init(&usbctrl); >> +libxl_usbctrlinfo_init(&usbctrlinfo); >> +usbctrl.devid = usbdev->ctrl; >> + >> +rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo); >> +if (rc) goto out; >> + >> +switch (usbctrlinfo.type) { >> +case LIBXL_USBCTRL_TYPE_PV: >> +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); >> +if (!busid) { >> +rc = ERROR_FAIL; >> +goto out; >> +} >> + >> +rc = usbback_dev_unassign(gc, busid); >> +if (rc) goto out; >> + >> +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); >> +if (rc) goto out; >> + >> +rc = usbdev_rebind(gc, busid); >> +if (rc) goto out; > > I think we need a comment here saying why we're doing things in this > order. Maybe: > > "Things are done in this order to balance simplicity with robustness in > the case of failure: > * We unbind all interfaces before rebinding any interfaces, so that we > never get into a situation where some interfaces are assigned to usbback > and some are assigned to the original drivers. > * We also unbind the interfaces before removing the pvusb xenstore > nodes, so that if the unbind fails in the middle, the device still shows > up in xl usb-list, and the user can re-try removing it." Sorry, just looked through the rest of the series, and there's one more thing. Neither here nor in the man page do we explain what to do if something goes wrong with the detach. I think the best thing to do is probably to make the logged error messages more helpful. What about something like this: * On failure to unbind: "Error removing device from guest. Try running usbdev-detach again." * On failure to rebind: "USB device removed from guest, but couldn't re-bind to domain 0. Try removing and re-inserting the USB device or reloading the driver modules." What do you think? -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
On 01/03/16 08:09, Chunyan Liu 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: Simon Cao > Signed-off-by: George Dunlap > Signed-off-by: Chunyan Liu > --- > Changes: > reorder usbdev_remove to following three steps: > 1. Unassign all interfaces from usbback, stopping and returning an > error as soon as one attempt fails > 2. Remove the pvusb xenstore nodes, stopping and returning an error > if it fails > 3. Attempt to re-assign all interfaces to the original drivers, > stopping and returning an error as soon as one attempt fails. Thanks, Chunyan! One minor comment about these changes... > +static int usbdev_rebind(libxl__gc *gc, const char *busid) > +{ > +char **intfs = NULL; > +char *usbdev_encode = NULL; > +char *path = NULL; > +int i, num = 0; > +int rc; > + > +rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num); > +if (rc) goto out; > + > +usbdev_encode = usb_interface_xenstore_encode(gc, busid); > + > +for (i = 0; i < num; i++) { > +char *intf = intfs[i]; > +char *usbintf_encode = NULL; > +const char *drvpath; > + > +/* rebind USB interface to its originial driver */ > +usbintf_encode = usb_interface_xenstore_encode(gc, intf); > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", > + usbdev_encode, usbintf_encode); > +rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); > +if (rc) goto out; > + > +if (drvpath) { > +rc = bind_usbintf(gc, intf, drvpath); > +if (rc) { > +LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath); > +goto out; > +} > +} > +} > + > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); > + > +out: So it looks like if one of the re-binds fails, then it stops where it is and leaves the USBBACK re-bind info in xenstore. In that case it's not clear to me how that information would ever be removed. I think until such time as we have a command to re-attempt the re-bind, if there's an error in the actual rebind, it should just break out of the for loop, and remove the re-bind nodes, and document a way to let the user try to clean things up. > +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid, > +libxl_device_usbdev *usbdev) > +{ > +int rc; > +char *busid; > +libxl_device_usbctrl usbctrl; > +libxl_usbctrlinfo usbctrlinfo; > + > +libxl_device_usbctrl_init(&usbctrl); > +libxl_usbctrlinfo_init(&usbctrlinfo); > +usbctrl.devid = usbdev->ctrl; > + > +rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo); > +if (rc) goto out; > + > +switch (usbctrlinfo.type) { > +case LIBXL_USBCTRL_TYPE_PV: > +busid = usbdev_busid_from_ctrlport(gc, domid, usbdev); > +if (!busid) { > +rc = ERROR_FAIL; > +goto out; > +} > + > +rc = usbback_dev_unassign(gc, busid); > +if (rc) goto out; > + > +rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev); > +if (rc) goto out; > + > +rc = usbdev_rebind(gc, busid); > +if (rc) goto out; I think we need a comment here saying why we're doing things in this order. Maybe: "Things are done in this order to balance simplicity with robustness in the case of failure: * We unbind all interfaces before rebinding any interfaces, so that we never get into a situation where some interfaces are assigned to usbback and some are assigned to the original drivers. * We also unbind the interfaces before removing the pvusb xenstore nodes, so that if the unbind fails in the middle, the device still shows up in xl usb-list, and the user can re-try removing it." Other than that, I gave this patche a moderately thorough review again today, and I think everything else looks good to me. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH V15 4/6] libxl: add pvusb API
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: Simon Cao Signed-off-by: George Dunlap Signed-off-by: Chunyan Liu --- Changes: reorder usbdev_remove to following three steps: 1. Unassign all interfaces from usbback, stopping and returning an error as soon as one attempt fails 2. Remove the pvusb xenstore nodes, stopping and returning an error if it fails 3. Attempt to re-assign all interfaces to the original drivers, stopping and returning an error as soon as one attempt fails. tools/libxl/Makefile |3 +- tools/libxl/libxl.c | 18 + tools/libxl/libxl.h | 77 ++ tools/libxl/libxl_device.c |5 +- tools/libxl/libxl_internal.h | 18 + tools/libxl/libxl_osdeps.h | 13 + tools/libxl/libxl_pvusb.c| 1596 ++ tools/libxl/libxl_types.idl | 46 + tools/libxl/libxl_types_internal.idl |1 + tools/libxl/libxl_utils.c| 18 + tools/libxl/libxl_utils.h|5 + 11 files changed, 1798 insertions(+), 2 deletions(-) create mode 100644 tools/libxl/libxl_pvusb.c diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index 789a12e..8fa7b87 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -105,7 +105,8 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \ libxl_stream_read.o libxl_stream_write.o \ libxl_save_callout.o _libxl_save_msgs_callout.o \ libxl_qmp.o libxl_event.o libxl_fork.o \ - libxl_dom_suspend.o libxl_dom_save.o $(LIBXL_OBJS-y) + libxl_dom_suspend.o libxl_dom_save.o libxl_pvusb.o \ +$(LIBXL_OBJS-y) LIBXL_OBJS += libxl_genid.o LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 2ab5ad3..1e68688 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -4102,6 +4102,8 @@ out: * libxl_device_vkb_destroy * libxl_device_vfb_remove * libxl_device_vfb_destroy + * libxl_device_usbctrl_remove + * libxl_device_usbctrl_destroy */ #define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\ int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \ @@ -4159,6 +4161,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1) DEFINE_DEVICE_REMOVE(vtpm, remove, 0) DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) +/* usbctrl */ +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0) +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1) + /* channel/console hotunplug is not implemented. There are 2 possibilities: * 1. add support for secondary consoles to xenconsoled * 2. dynamically add/remove qemu chardevs via qmp messages. */ @@ -4174,6 +4180,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) * libxl_device_disk_add * libxl_device_nic_add * libxl_device_vtpm_add + * libxl_device_usbctrl_add + * libxl_device_usbdev_add */ #define DEFINE_DEVICE_ADD(type) \ @@ -4205,6 +4213,12 @@ DEFINE_DEVICE_ADD(nic) /* vtpm */ DEFINE_DEVICE_ADD(vtpm) +/* usbctrl */ +DEFINE_DEVICE_ADD(usbctrl) + +/* usb */ +DEFINE_DEVICE_ADD(usbdev) + #undef DEFINE_DEVICE_ADD /**/ @@ -6750,6 +6764,10 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid, MERGE(pci, pcidevs, COMPARE_PCI, {}); +MERGE(usbctrl, usbctrls, COMPARE_USBCTRL, {}); + +MERGE(usbdev, usbdevs, COMPARE_USB, {}); + /* Take care of removable device. We maintain invariant in the * insert / remove operation so that: * 1. if xenstore is "empty" while JSON is not, the result diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 0859383..5cc3ce3 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -123,6 +123,12 @@ #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1 /* + * LIBXL_HAVE_PVUSB indicates functions for plugging in USB devices + * through pvusb -- both hotplug and at domain creation time.. + */ +#define LIBXL_HAVE_PVUSB 1 + +/* * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the * libxl_vendor_device field is present in the hvm sections of * libxl_domain_build_info. This field tells libxl which @@ -1536,6 +1542,77 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk, const libxl_asyncop_how *ao_how) LIBXL_EXTERNAL_CALLERS_ONLY; +/* + * USB + * + * For each device removed or added, one of these protocols is available: + * - PV (i.e., PVUSB) + * - DEVICEMODEL (i.e, qemu) + * + * PV is available for either PV or HVM domains. DEVICEMODEL is only + * available for HVM domains. The caller can