From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V7 3/7] libxl: add pvusb API Date: Wed, 30 Sep 2015 18:55:16 +0100 Message-ID: <560C2204.9030707@citrix.com> References: <1443147102-6471-1-git-send-email-cyliu@suse.com> <1443147102-6471-4-git-send-email-cyliu@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1443147102-6471-4-git-send-email-cyliu@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chunyan Liu , xen-devel@lists.xen.org Cc: jgross@suse.com, wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, jfehlig@suse.com, Simon Cao List-Id: xen-devel@lists.xenproject.org On 25/09/15 03:11, Chunyan Liu wrote: > Add pvusb APIs, including: > - attach/detach (create/destroy) virtual usb controller. > - attach/detach usb device > - list usb controllers and usb devices > - get information of usb controller and usb device > - some other helper functions > > Signed-off-by: Chunyan Liu > Signed-off-by: Simon Cao Hey Chunyan! This looks pretty close to being ready to check in to me. There are four basic comments I have. I'm keen to get this series in so that we can start doing more collaborative improvement; so I'll give my comments, and then talk about timing and a plan to get this in as quikcly as possible at the bottom. > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index aea4887..1e2c63e 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -4192,11 +4192,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1) > > /******************************************************************************/ > > +/* Macro for defining device remove/destroy functions for usbctrl */ > +/* Following functions are defined: > + * libxl_device_usbctrl_remove > + * libxl_device_usbctrl_destroy > + */ > + > +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f) \ > + int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \ > + uint32_t domid, libxl_device_##type *type, \ > + const libxl_asyncop_how *ao_how) \ > + { \ > + AO_CREATE(ctx, domid, ao_how); \ > + libxl__device *device; \ > + libxl__ao_device *aodev; \ > + int rc; \ > + \ > + GCNEW(device); \ > + rc = libxl__device_from_##type(gc, domid, type, device); \ > + if (rc != 0) goto out; \ > + \ > + GCNEW(aodev); \ > + libxl__prepare_ao_device(ao, aodev); \ > + aodev->action = LIBXL__DEVICE_ACTION_REMOVE; \ > + aodev->dev = device; \ > + aodev->callback = device_addrm_aocomplete; \ > + aodev->force = f; \ > + libxl__initiate_device_##type##_remove(egc, aodev); \ So this seems to be exactly the same as DEFINE_DEVICE_REMOVE(), except that you call libxl__initiate_device_usbctrl_remove() here rather than libxl__initiate_device_remove(). It seems like it might be better to have a separate patch renaming libxl__initiate_device_remove to libxl__initiate_device_generic_remove (or something like that), and then add a parameter to the definition above making it so that the definitions above pass in "generic". > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > index bee5ed5..935f25b 100644 > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs) > aodev->action = LIBXL__DEVICE_ACTION_REMOVE; > aodev->dev = dev; > aodev->force = drs->force; > + if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) { > + libxl__initiate_device_usbctrl_remove(egc, aodev); > + continue; > + } > libxl__initiate_device_remove(egc, aodev); I think this would probably be more clear if you did: if(dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) libxl__initiate_device_usbctl_remove() else libxl__initiate_device_remove() > @@ -3951,7 +3966,10 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc, > #define COMPARE_PCI(a, b) ((a)->func == (b)->func && \ > (a)->bus == (b)->bus && \ > (a)->dev == (b)->dev) > - > +#define COMPARE_USB(a, b) ((a)->u.hostdev.hostbus == (b)->u.hostdev.hostbus && \ > + (a)->u.hostdev.hostaddr == (b)->u.hostdev.hostaddr) Hmm... COMPARE_USB is used in three places: 1. Used in libxl.c:libxl_retrieve_domain_configuration() to de-duplicate JSON and xenstore configuration 2. Used in libxl_pvusb.c:libxl__device_usb_add_xentore() to avoid adding the same device twice 3. Used in libxl_pvusb.c:is_usbdev_in_array() to avoid assigning the same host device For #3, we pretty clearly want to be comparing hostbus/hostaddr. (In fact, you didn't use the COMPARE_USB macro until I suggested it in v3.) But for #1 and #2, is that what we want? Should we be comparing instead? In fact, after thinking about pvusb backends more, I think I want to assert that we do want to compare instead: is guaranteed to be unique for a domain, but it's entirely possible to have two different devices, from two different backend domains, with the same bus.addr. > +int libxl_devid_to_device_usbctrl(libxl_ctx *ctx, > + uint32_t domid, > + int devid, > + libxl_device_usbctrl *usbctrl) > +{ > + GC_INIT(ctx); > + libxl_device_usbctrl *usbctrls; > + int nb = 0; > + int i, rc = -1; > + > + usbctrls = libxl_device_usbctrl_list(ctx, domid, &nb); > + if (!nb) goto out; > + > + for (i = 0; i < nb; i++) { > + if (devid == usbctrls[i].devid) { > + *usbctrl = usbctrls[i]; libxl maintainers: Is this kind of copying OK? The analog functions for vtpm and nic both take very different approaches; both call libxl_device_[type]_init() and then fill in individual elements (albeit in different ways). > +/* > + * USB add > + */ > +static int do_usb_add(libxl__gc *gc, uint32_t domid, > + libxl_device_usb *usbdev, > + libxl__ao_device *aodev) > +{ > + libxl_ctx *ctx = CTX; > + libxl_usbctrlinfo usbctrlinfo; > + libxl_device_usbctrl usbctrl; > + int rc; > + > + 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_DEVICEMODEL: > + LOG(ERROR, "Not supported"); > + break; > + case LIBXL_USBCTRL_TYPE_PV: > + rc = libxl__device_usb_add_xenstore(gc, domid, usbdev, > + aodev->update_json); > + if (rc) goto out; > + > + rc = usbback_dev_assign(gc, usbdev); This assumes that the usb controller is dom0; but the interface explicitly allows pvusb driver domains. I think it would be enough here to do this check if the usbctrl device is dom0. We should then assume that a pvusb driver domain will be configured with all usb devices assigned to usbback already. I assume that there's a feedback mechanisms for backends for situations where the requested device can't be made, right? For example, if you have a network driver domain and request a non-existent bridge? If so, I think we can let the same mechanism worth for pvusb backends to say "I don't have that device available". > + if (rc) { > + libxl__device_usb_remove_xenstore(gc, domid, usbdev); > + goto out; > + } > + break; > + default: > + rc = ERROR_FAIL; > + goto out; > + } > + > +out: > + libxl_usbctrlinfo_dispose(&usbctrlinfo); > + return rc; > +} > + > +/* AO operation to add a usb device. > + * > + * Generally, it does: > + * 1) check if the usb device type is assignable > + * 2) check if the usb device is already assigned to a domain > + * 3) add 'busid' of the usb device to xenstore contoller/port/. > + * (PVUSB driver watches the xenstore changes and will detect that.) > + * 4) unbind usb device from original driver and bind to usbback. > + * If usb device has many interfaces, then: > + * - unbind each interface from its original driver and bind to usbback. > + * - store the original driver to xenstore for later rebinding when > + * detaching the device. > + */ > +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid, > + libxl_device_usb *usb, > + libxl__ao_device *aodev) > +{ > + STATE_AO_GC(aodev->ao); > + int rc = ERROR_FAIL; > + char *busid = NULL; > + libxl_device_usb *assigned; > + int num_assigned; > + > + assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0); > + > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, > + usb->u.hostdev.hostaddr); > + if (!busid) { > + LOG(ERROR, "USB device doesn't exist in sysfs"); > + goto out; > + } > + > + if (!is_usb_assignable(gc, usb)) { > + LOG(ERROR, "USB device is not assignable."); > + goto out; > + } Similar issue with pvusb driver domains: we can't do this check for driver domains. I wonder if we should do this check instead down on the usbback_assign_path(). If we assume that for pvusb driver domains: 1. All assignable devices will be assigned to usbback 2. No controllers will be assigned (since they can't be) 3. pvusb can say "no such device" for unassigned devices then we can safely ignore the check for pvusb driver domains > + > + /* check usb device is already assigned */ > + rc = get_assigned_devices(gc, &assigned, &num_assigned); > + if (rc) { > + LOG(ERROR, "cannot determine if device is assigned," > + " refusing to continue"); > + goto out; > + } > + > + if (is_usbdev_in_array(assigned, num_assigned, usb)) { > + LOG(ERROR, "USB device already attached to a domain"); > + rc = ERROR_FAIL; > + goto out; > + } WRT driver domains: Since we're looking inside xenstore instead of sysfs, we *can* actually do these checks. However, the "bus.addr" space is defined by the driver domain kernel, and is not unique across all driver domains: If we have one usb controller assigned to domain 0, and another assigned to a driver domain, two distinct devices are likely to end up with the same bus.addr. So to be compatible with driver domains, we'd need to have get_assigned_devices() only get devices with the same backend_domid as the controller we're checking. > +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid, > + libxl_device_usb *usb, > + libxl_usbinfo *usbinfo) > +{ > + GC_INIT(ctx); > + char *filename; > + char *busid; > + void *buf = NULL; > + int buflen, rc; > + > + usbinfo->ctrl = usb->ctrl; > + usbinfo->port = usb->port; > + > + if (libxl_ctrlport_to_device_usb(ctx, domid, > + usb->ctrl, usb->port, usb) < 0) { > + rc = ERROR_FAIL; > + goto out; > + } I suppose technically since usb isn't declared const that we haven't promised to modify it; but none of the other device_*_getinfo() functions modify the device struct; I think it would be better to follow suit and use a local variable to get the information from. However... > + > + usbinfo->devnum = usb->u.hostdev.hostaddr; > + usbinfo->busnum = usb->u.hostdev.hostbus; > + > + busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, > + usb->u.hostdev.hostaddr); > + if (!busid) { > + rc = ERROR_FAIL; > + goto out; > + } > + > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid); > + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL)) > + sscanf(buf, "%" SCNx16, &usbinfo->idVendor); > + > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid); > + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL)) > + sscanf(buf, "%" SCNx16, &usbinfo->idProduct); > + > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid); > + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) && > + buflen > 0) { > + /* replace \n to \0 */ > + if (((char *)buf)[buflen - 1] == '\n') > + ((char *)buf)[buflen - 1] = '\0'; > + usbinfo->manuf = libxl__strdup(NOGC, buf); > + } > + > + filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid); > + if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) && > + buflen > 0) { > + /* replace \n to \0 */ > + if (((char *)buf)[buflen - 1] == '\n') > + ((char *)buf)[buflen - 1] = '\0'; > + usbinfo->prod = libxl__strdup(NOGC, buf); > + } Basically, starting here, we have information which won't be available if we're using a pvusb driver domain. This information is nice-to-have, but I don't think this information is directly relevant to libxl or xl; the funcitonality to get this information is available from other libraries like libusb. I'm inclined to say that if we want to have pvusb driver domains (and I think we do), we should just get rid of this information. > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 9f6ec00..4844f18 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -595,6 +595,35 @@ libxl_device_rdm = Struct("device_rdm", [ > ("policy", libxl_rdm_reserve_policy), > ]) > > +libxl_usbctrl_type = Enumeration("usbctrl_type", [ > + (0, "AUTO"), > + (1, "PV"), > + (2, "DEVICEMODEL"), > + ]) > + > +libxl_usbdev_type = Enumeration("usbdev_type", [ > + (1, "hostdev"), > + ]) > + > +libxl_device_usbctrl = Struct("device_usbctrl", [ > + ("type", libxl_usbctrl_type), > + ("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), > + ("u", KeyedUnion(None, libxl_usbdev_type, "devtype", > + [("hostdev", Struct(None, [ > + ("hostbus", uint8), > + ("hostaddr", uint8)])), > + ])), > + ]) > + > libxl_device_dtdev = Struct("device_dtdev", [ > ("path", string), > ]) > @@ -627,6 +656,8 @@ libxl_domain_config = Struct("domain_config", [ > ("pcidevs", Array(libxl_device_pci, "num_pcidevs")), > ("rdms", Array(libxl_device_rdm, "num_rdms")), > ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")), > + ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")), > + ("usbs", Array(libxl_device_usb, "num_usbs")), > ("vfbs", Array(libxl_device_vfb, "num_vfbs")), > ("vkbs", Array(libxl_device_vkb, "num_vkbs")), > ("vtpms", Array(libxl_device_vtpm, "num_vtpms")), > @@ -676,6 +707,32 @@ libxl_vtpminfo = Struct("vtpminfo", [ > ("uuid", libxl_uuid), > ], dir=DIR_OUT) > > +libxl_usbctrlinfo = Struct("usbctrlinfo", [ > + ("type", libxl_usbctrl_type), > + ("devid", libxl_devid), > + ("version", integer), > + ("ports", integer), > + ("backend", string), > + ("backend_id", uint32), > + ("frontend", string), > + ("frontend_id", uint32), > + ("state", integer), > + ("evtch", integer), > + ("ref_urb", integer), > + ("ref_conn", integer), > + ], dir=DIR_OUT) > + > +libxl_usbinfo = Struct("usbinfo", [ > + ("ctrl", libxl_devid), > + ("port", integer), > + ("busnum", uint8), > + ("devnum", uint8), > + ("idVendor", uint16), > + ("idProduct", uint16), > + ("prod", string), > + ("manuf", string), > + ], dir=DIR_OUT) With the exception of the stuff in libxl_usbinfo we can't get if the backend isn't dom0, the interface looks good to me now. OK, so I've covered four things: 1. The DEFINE_DEVICE_REMOVE_EXT macro. I think I'd prefer checking in things are as it is, and then have a follow-up patch to refactor everything into a single DEFINE_DEVICE_REMOVE macro as described above. 2. COMPARE_USB. I think this needs to be fixed; but this should be a fairly simple change. 3. The conditional in libxl__device_destroy, the struct copy in libxl_devid_to_device_usbctrl, and the modification of the device in libxl_device_usb_getinfo are minor adjustments that can be discussed and fixed. 4. The pvusb driver domain != dom0 comments. This is somewhat secondary to your primary goal; but at the moment the code will certainly not work. We have a couple of options: a: Wait until we've fixed the driver domain functionality b: Check the code in right now, with the driver domain fuctionality explicitly disabled; that is, it will return an error if backend_domain!=0. Get a promise to implement the driver domain functionality properly for 4.7 (on the threat of removing the feature). c: Take driver domain support out of the interface for now; send follow-up patches to implement it properly. d. Take driver domain support out of the interface and leave it that way, for simplicity. I'd probably go with b or c. Thoughts? -George