|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V3 3/6] libxl: add pvusb API
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:
> 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?
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.
> +/* 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.
> +/* 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...
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?
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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |