[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.