From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chun Yan Liu" Subject: Re: [PATCH V7 3/7] libxl: add pvusb API Date: Fri, 09 Oct 2015 02:12:50 -0600 Message-ID: <561811B202000066000521C2@relay2.provo.novell.com> References: <1443147102-6471-1-git-send-email-cyliu@suse.com> <1443147102-6471-4-git-send-email-cyliu@suse.com> <560C2204.9030707@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <560C2204.9030707@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap , xen-devel@lists.xen.org Cc: Juergen Gross , wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, Jim Fehlig , Simon Cao List-Id: xen-devel@lists.xenproject.org >>> On 10/1/2015 at 01:55 AM, in message <560C2204.9030707@citrix.com>, George Dunlap wrote: > 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. Considering usb driver domain, does have problem. I agree it should compare , I'll update the MACRO. For #3, I won't use COMAPRE_USB macro then but extend codes to compare bus.addr directly. > > > +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). As Ian replied, it depends on the free time. To be strict, since we have libxl_device_usbctrl_list_free() soon later, it is not OK. But since in libxl_device_usbctrl struct, except for backend_domname, all others are int type, since backend_domname won't be called in later work, so it won't be affected after libxl_device_usbctrl_list_freei. Anyway, I'll update to follow vtpm and nic ways to copy individule elements. - Chunyan ~ > > > +/* > > + * 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 > >