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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 24/04/13 12:56, Ian Campbell wrote:
> On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote:
> > This patch exposes a generic interface which can be expanded in the
> > future to implement USB for PVUSB, qemu, and stubdoms.  It can also be
> > extended to include other types of USB other than host USB (for example,
> > tablets, mice, or keyboards).
> >
> > For each device removed or added, one of two protocols is available:
> > * PVUSB
> > * qemu (DEVICEMODEL)
> >
> > The caller can additionally specify "AUTO", in which case the library will
> > try to determine the best protocol automatically.
> >
> > At the moment, the only protocol implemented is DEVICEMODEL, and the only
> > device type impelmented is HOSTDEV.
> >
> > This uses the qmp functionality, and is thus only available for
> > qemu-xen, not qemu-traditional.
> >
> > History:
> > v6:
> >   - Return a proper error code if libxl__xs_mkdir fails
> >   - Make casts cuddly
> >   - Add a space after switch statements
> > v5:
> >   - fix erroneous use of NOGC in qmp functions
> >   - Don't check return of libxl__sprintf in libxl_create.c
> >   - Check return values of new xs actions in libxl_create.c
> >   - Use updated template for xenstore transactions, do checks
> >   - use xs_read_checked
> >   - Fixed if (x) spacing to match coding style
> >   - use GCSFPRINTF
> >   - use libxl__calloc
> >   - Fix comment for LIBXL_HAVE_USB
> >   - use idl-generated *_from_string() and *_to_string() functions
> >
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> > CC: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > CC: sstanisi@xxxxxxxxx
> > ---
> >   tools/libxl/Makefile           |    2 +-
> >   tools/libxl/libxl.h            |   37 +++
> >   tools/libxl/libxl_create.c     |   13 +-
> >   tools/libxl/libxl_internal.h   |   18 ++
> >   tools/libxl/libxl_qmp.c        |   65 +++++
> >   tools/libxl/libxl_types.idl    |   22 ++
> >   tools/libxl/libxl_usb.c        |  526 ++++++++++++++++++++++++++++++++++++++++
> >   tools/ocaml/libs/xl/genwrap.py |    1 +
> I've only commented on the interface bits (libxl.h, libxl_types.idl)
> here. I'll go over the implementation next.
>
> >   8 files changed, 682 insertions(+), 2 deletions(-)
> >   create mode 100644 tools/libxl/libxl_usb.c
> >
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 4922313..b6edd15 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -75,6 +75,12 @@
> >   #define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
> >
> >   /*
> > + * LIBXL_HAVE_USB indicates the functions for doing hot-plug of
> > + * USB devices.
> > + */
> > +#define LIBXL_HAVE_USB 1
> LIBXL_HAVE_DEVICE_USB would be consistent with the struct/interface
> naming. Not that I expect this to be ambiguous I guess.
Might as well make it consistent, as it's pretty easy. :-)

> > +
> > +/*
> >    * libxl ABI compatibility
> >    *
> >    * The only guarantee which libxl makes regarding ABI compatibility
> > @@ -735,6 +741,37 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, 
> > libxl_device_disk *disk,
> >                          const libxl_asyncop_how *ao_how)
> >                          LIBXL_EXTERNAL_CALLERS_ONLY;
> >
> > +/*
> > + * USB
> > + *
> > + * For each device removed or added, one of these protocols is available:
> > + * - PV (i.e., PVUSB)
> > + * - DEVICEMODEL (i.e, qemu)
> > + *
> > + * PV is available for either PV or HVM domains.  DEVICEMODEL is only
> > + * available for HVM domains.  The caller can additionally specify
> > + * "AUTO", in which case the library will try to determine the best
> > + * protocol automatically.
> > + *
> > + * At the moment, the only protocol implemented is DEVICEMODEL, and the only
> > + * device type impelmented is HOSTDEV.
>                   implemented
>
> If PV isn't implemented I think we should leave it out of the API for
> now, when it is implemented it will need a LIBXL_HAVE_DEVICE_USB_TYPE_PV
> or whatever anyway.
If we return -ENOTIMP or -ENOSYS from usb_add, would that be sufficient?

> > + *
> > + * This uses the qmp functionality, and is thus only available for
> > + * qemu-xen, not qemu-traditional.
> > + */
> > +#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;
> > +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
> > +                            libxl_device_usb *dev,
> > +                            const libxl_asyncop_how *ao_how)
> > +                            LIBXL_EXTERNAL_CALLERS_ONLY;
> No _destroy or _getinfo?
>
> _getinfo might be optional if there isn't any interesting info, but
> _destroy is a must IMHO.
I'm not exactly sure what the difference at this point would be between 
remove and destroy.
> > +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
> > +                                        int *num)
> > +                          LIBXL_EXTERNAL_CALLERS_ONLY;
> > +
> >   /* Network Interfaces */
> >   int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic 
> > *nic,
> >                            const libxl_asyncop_how *ao_how)
> [...]
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index fcb1ecd..3c6a709 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -408,6 +408,28 @@ libxl_device_vtpm = Struct("device_vtpm", [
> >       ("uuid",             libxl_uuid),
> >   ])
> >
> > +libxl_device_usb_protocol = Enumeration("usb_protocol", [
> > +    (0, "AUTO"),
> > +    (1, "PV"),
> > +    (2, "DEVICEMODEL"),
> > +    ])
> > +
> > +libxl_device_usb_type = Enumeration("device_usb_type", [
> > +    (1, "HOSTDEV"),
> > +    ])
> > +
> > +libxl_device_usb = Struct("device_usb", [
> > +        ("protocol", libxl_device_usb_protocol,
> > +         {'init_val': 'LIBXL_USB_PROTOCOL_AUTO'}),
> > +        ("backend_domid", libxl_domid,
> > +         {'init_val': 'LIBXL_DEVICE_USB_BACKEND_DEFAULT'}),
> I think now that ef496b81f0336f09968a318e7f81151dd4f5a0cc has gone in we
> should have a backend_domname (and appropriate handling) for new
> devices.
Ack.

> I don't think you need to set a default for a libxl_domid, the implicit
> default is 0 and if we wanted to be explicit we should do it on the
> libxl_domid type so it is consistent for all devices. Likewise I don't
> think you need LIBXL_DEVICE_USB_BACKEND_DEFAULT == -1 and the handling
> to make that 0 internally -- just make the default 0 and let the user
> change if they like (this is how the other devices work).
I think my idea was that someday you may want to say, "Have backends 
default to driver domain [foo]."  In which case, you'd want to be able 
to specify the difference between "connect to the default" and "connect 
to domain 0".
But maybe the whole "default" thing is too high-level for libxl, and I 
should just make the caller set the actual domain (and deal with 
defaults itself).
 -George

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