Re: [PATCH v7 RFC 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-06-18 at 14:22 +0100, George Dunlap wrote:
> On 06/18/2014 02:08 PM, Ian Campbell wrote:
> > On Mon, 2014-06-02 at 14:44 +0100, George Dunlap wrote:
> >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> >> index c7aa817..963e650 100644
> >> --- a/tools/libxl/libxl.h
> >> +++ b/tools/libxl/libxl.h
> >> @@ -82,6 +82,12 @@
> >>   #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
> >>   
> >>   /*
> >> + * LIBXL_HAVE_DEVICE_USB indicates the functions for doing hot-plug of
> >> + * USB devices.
> >> + */
> >> +#define LIBXL_HAVE_DEVICE_USB 1
> >> +
> >> +/*
> >>    * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
> >>    * libxl_vendor_device field is present in the hvm sections of
> >>    * libxl_domain_build_info. This field tells libxl which
> >> @@ -924,6 +930,40 @@ 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 implemented is HOSTDEV.
> >> + *
> >> + * This uses the qmp functionality, and is thus only available for
> >> + * qemu-xen, not qemu-traditional.
> >> + */
> >> +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;
> >> +int libxl_device_usb_destroy(libxl_ctx *ctx, uint32_t domid,
> >> +                             libxl_device_usb *dev,
> >> +                             const libxl_asyncop_how *ao_how)
> >> +                             LIBXL_EXTERNAL_CALLERS_ONLY;
> >> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
> >> +                                        int *num)
> >> +                          LIBXL_EXTERNAL_CALLERS_ONLY;
> > No _getinfo? (Might only make sense with the PV stuff I guess)
> 
> IIRC the pattern I saw for other devices was:
> * libxl_device_$FOO struct is used to add, remove, destroy
> * libl_device_$FOO_list returns an array of libxl_device_$FOO
> * libl_device_$FOO_getinfo is only used when there's information you 
> need which is not in libxl_device_$FOO struct (and hence isn't returned 
> by _list).

Right, getinfo is runtime stuff, like the evtchn and shared ring (or
stats, I guess). Hence probably only for PV unless there is something
like that for the emulated stuff.


> 
> 
> >> +        ("backend_domid",    libxl_domid),
> >> +        ("backend_domname",  string),
> >> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
> >> +                         [("hostdev", Struct(None, [
> >> +                                ("hostbus",   integer),
> >> +                                ("hostaddr",  integer) ]))
> > No need to express the host topology I think (because you can build that
> > from the bus,addr tuples)?
> 
> I don't really follow.  You mean, we can drop 'host' from the last two 
> elements, and just call them "bus" and "addr"?

Gah, I started writing one thing and then reunderstood usb and wrote
half another.

What I was trying to say is that you don't need hostaddr to describe the
full USB topology path to the device because the (bus,addr) tuple you've
given already does so (because each hub effectively creates a new bus
number, so they all look like toplevel buses in this representation).

But you could drop the host prefix, yes.

Ian.