Re: [Xen-devel] [PATCH RFC] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("[Xen-devel] [PATCH RFC] libxl: Introduce functions to 
add and remove USB devices to an HVM guest"):
> I'm mailing this intermediate form of v3 to get feedback before I go
> all the way through the process of implementing the whole thing and
> ironing out the bugs.

Thanks.  The basic approach seems plausible.

> For each device removed or added, one of three protocols is available:
> * PVUSB
> * qemu (DEVICEMODEL)
> * stubdom (i.e., stubdom -> pvusb, domain -> stubdom qemu)

You seem to be making this specifiable separately for each device,
which doesn't seem like it could be right.

I think it would be best to make the protocol (as specified in the
libxl api, xl config files etc.), simply specify what the guest sees:
PV or HVM.  In the case of HVM, that's the stub-dm if the guest has
one or qemu in dom0 if not.


> +#define LIBXL_DEVICE_HOST_USB_ANY (-1)
> +#define LIBXL_DEVICE_USB_BACKEND_DEFAULT (-1)
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usb *dev,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;

What are the semantics if multiple host devices match *dev ?
How is the id returned ?

I think dev should probably be
  const libxl_device_usb *dev

> +    libxl__json_object *args = NULL;
> +    char *id;
> +
> +    id = libxl__sprintf(NOGC, HOST_USB_QDEV_ID,
> +                        (uint16_t) dev->u.hostdev.hostbus,
> +                        (uint16_t) dev->u.hostdev.hostaddr,
> +                        (uint16_t) dev->u.hostdev.vendorid,
> +                        (uint16_t) dev->u.hostdev.productid);

If hostbus and hostaddr aren't specified and we pass through multiple
devices with the same vendor and product id, this won't yield unique
ids.

> +struct enum_str {
> +    int min,max;
> +    const char * str[];
> +};
> +
> +static char * __enum_to_str(libxl__gc *gc, struct enum_str *e, 
> libxl_usb_protocol t)

(Trailing whitespace, and needs wrapping.)

This is a lot of enum-handling machinery.  Don't we have something
similar already which could be pressed into service ?  If not then
this should be in a separate file.

> +struct enum_str enum_protocol = {
> +    .min = 1,
> +    .max = LIBXL_USB_PROTOCOL_STUBDOM,
> +    .str = {
> +        [LIBXL_USB_PROTOCOL_PV] = "pv",
> +        [LIBXL_USB_PROTOCOL_DEVICEMODEL] = "dm",
> +        [LIBXL_USB_PROTOCOL_STUBDOM] = "stubdom",

Perhaps this should be generated by the idl compiler ?


> +static char * create_hostdev_xenstore_entry(libxl__gc *gc, uint32_t domid, l
ibxl_device_usb *usbdev, flexarray_t *a) 
> +{
> +    char * path;
> +
> +    path = libxl__sprintf(gc, "/libxl/usb/%d/%s",
> +                          domid,
> +                          libxl__sprintf(gc, USB_HOSTDEV_ID,
> +                                        (uint16_t)usbdev->u.hostdev.hostbus,

> +                                        (uint16_t)usbdev->u.hostdev.hostaddr
,
> +                                        (uint16_t)usbdev->u.hostdev.vendorid
,
> +                                        (uint16_t)usbdev->u.hostdev.producti
d));

I stopped reading here because of the wrap damage I'm afraid.
(Reproduced it by adding hard CRs where my MUA wraps it, so you can
see what it looks like.)


Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel