|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V7 3/7] libxl: add pvusb API
>>> On 10/1/2015 at 01:55 AM, in message <560C2204.9030707@xxxxxxxxxx>, George
Dunlap <george.dunlap@xxxxxxxxxx> 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 <cyliu@xxxxxxxx>
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
>
> 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
> <ctrl,port> instead?
>
> In fact, after thinking about pvusb backends more, I think I want to
> assert that we do want to compare <ctrl,port> instead: <crtl,port> 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, <bus.addr> does have problem. I agree
it should compare <ctrl, port>, 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
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |