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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

> +
> +/*
>   * 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.

> + *
> + * 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.

> +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.

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).

> +        ("u", KeyedUnion(None, libxl_device_usb_type, "type",
> +                         [("hostdev", Struct(None, [
> +                                ("hostbus",   integer),
> +                                ("hostaddr",  integer) ]))
> +                          ]))
> +    ])
> +
>  libxl_domain_config = Struct("domain_config", [
>      ("c_info", libxl_domain_create_info),
>      ("b_info", libxl_domain_build_info),



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