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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@eu.citrix.com>
>> CC: Ian Jackson <ian.jackson@citrix.com>
>> CC: Roger Pau Monne <roger.pau@citrix.com>
>> CC: sstanisi@cbnco.com
>> ---
>>   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