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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 09/04/13 15:14, Ian Jackson wrote:
> 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.
The dichotomy isn't PV / HVM; it's PVUSB vs qemu (or "devicemodel").  
Remember that HVM guests can have PVUSB front-ends, and thus can use the 
PVUSB protocol.
The reason for specifying STUBDOM I guess is that STUBDOM is really a 
special case, where it's really both a PVUSB  and a DEVICEMODEL; so in 
theory you could specify a backend_domid.
The question, I guess, is whether we should assume that the caller can 
be trusted to know whether the VM in question is using a stub domain or 
not.  It's certainly possible to make the interface only specify PVUSB 
or DEVICEMODEL, and to make it (someday) so that calling DEVICEMODEL for 
a stubdom will set up PVUSB for the stubdom, then tell the device model 
to make the device available via emulation all automatically.
But since we're not implementing stubdom at the moment anyway, we can 
probably just take out STUBDOM entirely and discuss whether to add a new 
protocol when we actually decide to implement it.
> > +#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 in general if it matches multiple devices then it should fail.  
That's what qemu will do, so at the moment that is implemented by 
default for us.
> I think dev should probably be
>    const libxl_device_usb *dev
Sure.

> > +    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.
If multiple devices match, then qemu will fail to add the device.

We could do a lot of this checking in the library itself rather than 
qemu. (In fact for PVUSB I think we're going to have to.)  But again, 
there's no way I'm going to get that coded up by Friday; getting that 
done would mean waiting until 4.4.
> > +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 ?\
I'm not going to be able to make that happen by the feature freeze this 
Friday.
Perhaps just writing the raw numeric values for now?

> > +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.)
Well the main purpose of this mail was to see if this could be 
implemented by Friday, or if it would need to wait until 4.4.  We've 
uncovered a couple of things which if required would mean waiting -- 
what do you think?  I'm fine with waiting -- that's why I sent the 
e-mail, so I could stop early if there was little chance of success. :-)
 -George

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