|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V7 3/7] libxl: add pvusb API
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.
> +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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |