George Dunlap
2013-Apr-19 15:59 UTC
[PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
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 +
8 files changed, 682 insertions(+), 2 deletions(-)
create mode 100644 tools/libxl/libxl_usb.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 2984051..866960a 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -74,7 +74,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o
libxl_pci.o \
libxl_internal.o libxl_utils.o libxl_uuid.o \
libxl_json.o libxl_aoutils.o libxl_numa.o \
libxl_save_callout.o _libxl_save_msgs_callout.o \
- libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
+ libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.o $(LIBXL_OBJS-y)
LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
$(LIBXL_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h
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 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.
+ *
+ * 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;
+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_create.c b/tools/libxl/libxl_create.c
index ae72f21..c9d06e8 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -393,7 +393,7 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
libxl_ctx *ctx = libxl__gc_owner(gc);
int flags, ret, rc, nb_vm;
char *uuid_string;
- char *dom_path, *vm_path, *libxl_path;
+ char *dom_path, *vm_path, *libxl_path, *libxl_usb_path;
struct xs_permissions roperm[2];
struct xs_permissions rwperm[1];
struct xs_permissions noperm[1];
@@ -454,6 +454,8 @@ int libxl__domain_make(libxl__gc *gc,
libxl_domain_create_info *info,
goto out;
}
+ libxl_usb_path = GCSPRINTF("%s/usb", libxl_path);
+
noperm[0].id = 0;
noperm[0].perms = XS_PERM_NONE;
@@ -477,6 +479,15 @@ retry_transaction:
xs_rm(ctx->xsh, t, libxl_path);
libxl__xs_mkdir(gc, t, libxl_path, noperm, ARRAY_SIZE(noperm));
+ /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */
+ rc = libxl__xs_rm_checked(gc, t, libxl_usb_path);
+ if (rc) goto out;
+ /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */
+ if (!libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm))) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path),
vm_path, strlen(vm_path));
rc = libxl__domain_rename(gc, *domid, 0, info->name, t);
if (rc)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3ba3a21..d2e00fa 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid,
const char *filename);
/* Set dirty bitmap logging status */
_hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool
enable);
_hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const
libxl_device_disk *disk);
+/* Same as normal, but "translated" */
+typedef struct libxl__device_usb {
+ libxl_usb_protocol protocol;
+ libxl_domid target_domid;
+ libxl_domid backend_domid;
+ libxl_domid dm_domid;
+ libxl_device_usb_type type;
+ union {
+ struct {
+ int hostbus;
+ int hostaddr;
+ } hostdev;
+ } u;
+} libxl__device_usb;
+_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid,
+ libxl__device_usb *dev);
+_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid,
+ libxl__device_usb *dev);
/* close and free the QMP handler */
_hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
/* remove the socket file, if the file has already been removed,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 644d2c0..9ad3e59 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -42,6 +42,7 @@
#define QMP_RECEIVE_BUFFER_SIZE 4096
#define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x"
+#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x"
typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
const libxl__json_object *tree,
@@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
}
}
+static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid,
+ libxl__device_usb *dev)
+{
+ libxl__json_object *args = NULL;
+ char *id;
+
+ id = GCSPRINTF(HOST_USB_QDEV_ID,
+ (uint16_t)dev->u.hostdev.hostbus,
+ (uint16_t)dev->u.hostdev.hostaddr);
+
+ qmp_parameters_add_string(gc, &args, "driver",
"usb-host");
+ QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x",
dev->u.hostdev.hostbus);
+ QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x",
dev->u.hostdev.hostaddr);
+
+ qmp_parameters_add_string(gc, &args, "id", id);
+
+ return qmp_run_command(gc, domid, "device_add", args, NULL,
NULL);
+}
+
+int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb *usbdev)
+{
+ int rc;
+ switch (usbdev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ rc = libxl__qmp_usb_hostdev_add(gc, domid, usbdev);
+ break;
+ default:
+ return ERROR_INVAL;
+ }
+ return rc;
+}
+
+
+static int libxl__qmp_usb_hostdev_remove(libxl__gc *gc, int domid,
+ libxl__device_usb *dev)
+{
+ libxl__json_object *args = NULL;
+ char *id;
+
+ id = GCSPRINTF(HOST_USB_QDEV_ID,
+ (uint16_t)dev->u.hostdev.hostbus,
+ (uint16_t)dev->u.hostdev.hostaddr);
+
+ qmp_parameters_add_string(gc, &args, "id", id);
+
+ return qmp_run_command(gc, domid, "device_del", args, NULL,
NULL);
+}
+
+int libxl__qmp_usb_remove(libxl__gc *gc, int domid,
+ libxl__device_usb *usbdev)
+{
+ int rc;
+ switch (usbdev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ rc = libxl__qmp_usb_hostdev_remove(gc, domid, usbdev);
+ break;
+ default:
+ return ERROR_INVAL;
+ }
+ return rc;
+}
+
+
+
int libxl__qmp_initializations(libxl__gc *gc, uint32_t domid,
const libxl_domain_config *guest_config)
{
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''}),
+ ("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),
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
new file mode 100644
index 0000000..c320487
--- /dev/null
+++ b/tools/libxl/libxl_usb.c
@@ -0,0 +1,526 @@
+/*
+ * Copyright (C) 2009 Citrix Ltd.
+ * Author Vincent Hanquez <vincent.hanquez@eu.citrix.com>
+ * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+
+/*
+ * /libxl/<domid>/usb/<devid>/{type, protocol, backend, [typeinfo]}
+ */
+#define USB_INFO_PATH "%s/usb"
+#define USB_HOSTDEV_ID "hostdev-%04x-%04x"
+
+/*
+ * Just use plain numbers for now. Replace these with strings at some point.
+ */
+static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ return GCSPRINTF(USB_INFO_PATH "/%s",
+ libxl__xs_libxl_path(gc, domid),
+ GCSPRINTF(USB_HOSTDEV_ID,
+ (uint16_t)usbdev->u.hostdev.hostbus,
+ (uint16_t)usbdev->u.hostdev.hostaddr));
+}
+
+static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev,
+ flexarray_t *a)
+{
+ flexarray_append_pair(a, "hostbus",
+ GCSPRINTF("%d",
+ usbdev->u.hostdev.hostbus));
+ flexarray_append_pair(a, "hostaddr",
+ GCSPRINTF("%d",
+ usbdev->u.hostdev.hostaddr));
+}
+
+static int read_hostdev_xenstore_entry(libxl__gc *gc, const char * path,
+ libxl__device_usb *usbdev)
+{
+ int rc;
+ const char * val;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/hostbus", path),
&val);
+ if (rc) goto out;
+ usbdev->u.hostdev.hostbus = atoi(val);
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/hostaddr", path),
&val);
+ if (rc) goto out;
+ usbdev->u.hostdev.hostaddr = atoi(val);
+
+ rc = 0;
+out:
+ return rc;
+}
+
+static int usb_read_xenstore(libxl__gc *gc, const char * path,
+ libxl__device_usb *usbdev)
+{
+ const char *val;
+ int rc;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/protocol", path),
&val);
+ if (rc) goto out;
+ rc = libxl_usb_protocol_from_string(val, &usbdev->protocol);
+ if (rc) goto out;
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/backend_domid", path),
&val);
+ if (rc) goto out;
+ usbdev->backend_domid = atoi(val);
+
+ rc = libxl__xs_read_checked(gc, XBT_NULL,
+ GCSPRINTF("%s/type", path),
&val);
+ if (rc) goto out;
+ rc = libxl_device_usb_type_from_string(val, &usbdev->type);
+ if (rc) goto out;
+
+ switch (usbdev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ rc = read_hostdev_xenstore_entry(gc, path, usbdev);
+ if (rc) goto out;
+ break;
+ default:
+ LOG(ERROR, "Internal error (unimplemented type)");
+ goto out;
+ }
+
+ rc = 0;
+out:
+ return rc;
+}
+
+static int usb_add_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ flexarray_t *dev;
+ char *dev_path;
+ xs_transaction_t t = 0;
+ struct xs_permissions noperm[1];
+ int rc = 0;
+
+ noperm[0].id = 0;
+ noperm[0].perms = XS_PERM_NONE;
+
+ dev = flexarray_make(gc, 16, 1);
+
+ flexarray_append_pair(dev, "protocol",
+ GCSPRINTF("%s",
+
libxl_usb_protocol_to_string(usbdev->protocol)));
+
+ flexarray_append_pair(dev, "backend_domid",
+ GCSPRINTF("%d", usbdev->backend_domid));
+
+ flexarray_append_pair(dev, "type",
+ GCSPRINTF("%s",
+
libxl_device_usb_type_to_string(usbdev->type)));
+
+ switch (usbdev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ dev_path = hostdev_xs_path(gc, domid, usbdev);
+ hostdev_xs_append_params(gc, usbdev, dev);
+ break;
+ default:
+ LOG(ERROR, "Invalid device type: %d", usbdev->type);
+ return ERROR_FAIL;
+ }
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to
xenstore");
+
+ for(;;) {
+ rc = libxl__xs_transaction_start(gc, &t);
+ if (rc) goto out;
+
+ /* Helpfully, mkdir returns 0 on failure, 1 on success */
+ if (!libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm))) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* And libxl__xs_writev *always* returns 0 no matter what */
+ libxl__xs_writev(gc, t, dev_path,
+ libxl__xs_kvs_of_flexarray(gc, dev, dev->count));
+
+ rc = libxl__xs_transaction_commit(gc, &t);
+ if (!rc) break;
+ if (rc <0) goto out;
+ }
+
+out:
+ return rc;
+}
+
+static int usb_remove_xenstore(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *dev_path;
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Removing device from
xenstore");
+
+ switch (usbdev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ dev_path = hostdev_xs_path(gc, domid, usbdev);
+ break;
+ default:
+ LOG(ERROR, "Invalid device type: %d", usbdev->type);
+ return ERROR_FAIL;
+ }
+
+ xs_rm(ctx->xsh, XBT_NULL, dev_path);
+
+ return 0;
+}
+
+static int get_assigned_devices(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb **list, int *num)
+{
+ char **devlist, *dompath;
+ unsigned int nd = 0;
+ int i, rc = 0;
+
+ *list = NULL;
+ *num = 0;
+
+ dompath = GCSPRINTF(USB_INFO_PATH,
+ libxl__xs_libxl_path(gc, domid));
+
+ devlist = libxl__xs_directory(gc, XBT_NULL, dompath, &nd);
+ if (!devlist)
+ goto out;
+
+ *list = libxl__calloc(gc, nd, sizeof(libxl__device_usb));
+
+ for(i = 0; i < nd; i++) {
+ char *path;
+
+ path = GCSPRINTF("%s/%s", dompath, devlist[i]);
+
+ rc = usb_read_xenstore(gc, path, (*list) + i);
+ if (rc) {
+ *list = NULL;
+ *num = 0;
+ goto out;
+ }
+ }
+
+ *num = nd;
+
+out:
+ return rc;
+}
+
+static int is_usbdev_type_hostdev_equal(libxl__device_usb *a,
+ libxl__device_usb *b)
+{
+ if (!memcmp(&a->u.hostdev, &b->u.hostdev,
sizeof(a->u.hostdev)))
+ return 1;
+ else
+ return 0;
+}
+
+static int is_usbdev_in_array(libxl__device_usb *assigned, int num_assigned,
+ libxl__device_usb *dev)
+{
+ int i;
+
+ /* Devices are the same if:
+ * - They have the same backend_domid
+ * - They are of the same type
+ * - Their types match
+ * In theory, someone could try to pass the same device through
+ * via both PVUSB and qemu; not comparing protocol will prevent that.
+ */
+ for(i = 0; i < num_assigned; i++) {
+ if (assigned[i].type != dev->type
+ || assigned[i].backend_domid != dev->backend_domid)
+ continue;
+
+ switch (dev->type) {
+ case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
+ if (!is_usbdev_type_hostdev_equal(dev, assigned+i))
+ continue;
+ }
+
+ return 1;
+ }
+
+ return 0;
+}
+
+static void usbdev_ext_to_int(libxl__device_usb *dev_int,
+ libxl_device_usb *dev_ext)
+{
+ dev_int->protocol = dev_ext->protocol;
+
+ if (dev_ext->backend_domid == LIBXL_DEVICE_USB_BACKEND_DEFAULT)
+ dev_int->backend_domid = 0;
+ else
+ dev_int->backend_domid = dev_ext->backend_domid;
+
+ dev_int->type = dev_ext->type;
+ memcpy(&dev_int->u, &dev_ext->u, sizeof(dev_ext->u));
+}
+
+static void usbdev_int_to_ext(libxl_device_usb *dev_ext,
+ libxl__device_usb *dev_int)
+{
+ dev_ext->protocol = dev_int->protocol;
+
+ dev_ext->backend_domid = dev_int->backend_domid;
+
+ dev_ext->type = dev_int->type;
+ memcpy(&dev_ext->u, &dev_int->u, sizeof(dev_ext->u));
+}
+
+/*
+ * USB add
+ */
+static int do_usb_add(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ int rc;
+
+ switch (usbdev->protocol) {
+ case LIBXL_USB_PROTOCOL_DEVICEMODEL:
+ switch (libxl__device_model_version_running(gc, domid)) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ LOG(ERROR, "usb-add not yet implemented for
qemu-traditional");
+ return ERROR_INVAL;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ rc = libxl__qmp_usb_add(gc, domid, usbdev);
+ if (rc)
+ goto out;
+ break;
+ default:
+ return ERROR_INVAL;
+ }
+ break;
+ default:
+ return ERROR_FAIL;
+ }
+
+ rc = usb_add_xenstore(gc, domid, usbdev);
+out:
+ return rc;
+}
+
+
+
+static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *dev_ext)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl__device_usb *assigned, _usbdev, *usbdev;
+ int rc = ERROR_FAIL, num_assigned;
+ libxl_domain_type domtype = libxl__domain_type(gc, domid);
+
+ /* Interpret incoming */
+ usbdev = &_usbdev;
+
+ usbdev->target_domid = domid;
+ usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+ usbdev_ext_to_int(usbdev, dev_ext);
+
+ if (usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO) {
+ if (domtype == LIBXL_DOMAIN_TYPE_PV) {
+ usbdev->protocol = LIBXL_USB_PROTOCOL_PV;
+ } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) {
+ /* FIXME: See if we can detect PV frontend */
+ usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL;
+ }
+ }
+
+ /* Check to make sure we''re doing something that''s
impemented */
+ if (usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) {
+ rc = ERROR_FAIL;
+ LOG(ERROR, "Protocol not implemented");
+ goto out;
+ }
+
+ if (usbdev->dm_domid != 0) {
+ rc = ERROR_FAIL;
+ LOG(ERROR, "Stubdoms not yet supported");
+ goto out;
+ }
+
+ /* Double-check to make sure it''s not already assigned */
+ rc = get_assigned_devices(gc, domid, &assigned, &num_assigned);
+ if (rc) {
+ LOG(ERROR, "cannot determine if device is assigned,"
+ " refusing to continue");
+ goto out;
+ }
+ if (is_usbdev_in_array(assigned, num_assigned, usbdev)) {
+ LOG(ERROR, "USB device already attached to a domain");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Do the add */
+ if (do_usb_add(gc, domid, usbdev))
+ rc = ERROR_FAIL;
+
+out:
+ return rc;
+}
+
+int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usb *usbdev,
+ const libxl_asyncop_how *ao_how)
+{
+ AO_CREATE(ctx, domid, ao_how);
+ int rc;
+ rc = libxl__device_usb_add(gc, domid, usbdev);
+ libxl__ao_complete(egc, ao, rc);
+ return AO_INPROGRESS;
+}
+
+/*
+ * USB remove
+ */
+static int do_usb_remove(libxl__gc *gc, uint32_t domid,
+ libxl__device_usb *usbdev)
+{
+ int rc;
+
+ switch (usbdev->protocol) {
+ case LIBXL_USB_PROTOCOL_DEVICEMODEL:
+ switch (libxl__device_model_version_running(gc, domid)) {
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+ LOG(ERROR, "usb-remove not yet implemented for
qemu-traditional");
+ return ERROR_INVAL;
+ case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+ rc = libxl__qmp_usb_remove(gc, domid, usbdev);
+ if (rc)
+ goto out;
+ break;
+ default:
+ return ERROR_INVAL;
+ }
+ break;
+ default:
+ return ERROR_FAIL;
+ }
+
+ rc = usb_remove_xenstore(gc, domid, usbdev);
+out:
+ return rc;
+}
+static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *dev_ext)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl__device_usb *assigned, _usbdev, *usbdev;
+ int rc = ERROR_FAIL, num_assigned;
+ libxl_domain_type domtype = libxl__domain_type(gc, domid);
+
+ /* Interpret incoming */
+ usbdev = &_usbdev;
+
+ usbdev->target_domid = domid;
+ usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid);
+
+ usbdev_ext_to_int(usbdev, dev_ext);
+
+ if (usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO) {
+ if (domtype == LIBXL_DOMAIN_TYPE_PV) {
+ usbdev->protocol = LIBXL_USB_PROTOCOL_PV;
+ } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) {
+ /* FIXME: See if we can detect PV frontend */
+ usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL;
+ }
+ }
+
+ /* Check to make sure we''re doing something that''s
impemented */
+ if (usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) {
+ rc = ERROR_FAIL;
+ LOG(ERROR, "Protocol not implemented");
+ goto out;
+ }
+
+ if (usbdev->dm_domid != 0) {
+ rc = ERROR_FAIL;
+ LOG(ERROR, "Stubdoms not yet supported");
+ goto out;
+ }
+
+ /* Double-check to make sure it''s not already assigned */
+ rc = get_assigned_devices(gc, domid, &assigned, &num_assigned);
+ if (rc) {
+ LOG(ERROR, "cannot determine if device is assigned,"
+ " refusing to continue");
+ goto out;
+ }
+ if (!is_usbdev_in_array(assigned, num_assigned, usbdev)) {
+ LOG(ERROR, "USB device doesn''t seem to be attached to the
domain");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ /* Do the remove */
+ if (do_usb_remove(gc, domid, usbdev))
+ rc = ERROR_FAIL;
+
+out:
+ return rc;
+}
+
+int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
+ libxl_device_usb *usbdev,
+ const libxl_asyncop_how *ao_how)
+{
+ AO_CREATE(ctx, domid, ao_how);
+ int rc;
+ rc = libxl__device_usb_remove(gc, domid, usbdev);
+ libxl__ao_complete(egc, ao, rc);
+ return AO_INPROGRESS;
+}
+
+
+libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx,
+ uint32_t domid, int *num)
+{
+ GC_INIT(ctx);
+ libxl__device_usb *devint;
+ libxl_device_usb *devext = NULL;
+ int n = 0, i, rc;
+
+ rc = get_assigned_devices(gc, domid, &devint, &n);
+ if (rc) {
+ LOG(ERROR, "Could not get assigned devices");
+ goto out;
+ }
+
+ if (n == 0)
+ goto out;
+
+ devext = libxl__calloc(NOGC, n, sizeof(libxl_device_usb));
+
+ for (i = 0; i < n; i++)
+ usbdev_int_to_ext(devext + i, devint + i);
+
+ *num = n;
+out:
+ GC_FREE;
+ return devext;
+}
diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
index ea978bf..aab65f3 100644
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -286,6 +286,7 @@ if __name__ == ''__main__'':
"domain_config",
"vcpuinfo",
"event",
+ "device_usb"
]
for t in blacklist:
--
1.7.9.5
v6:
- Rename to usb-attach and usb-detach to fit better w/ other device
add/remove commands
- Use positional arguments, to fit better w/ other device add/remove
commands
- Change specifier to "hostdev:xx.yy", looking forward to a day when
we may be able to accept something like "tablet"
- Style fix-ups
v5:
- Remove extraneous is_hex
- Replace is_dec with ctype.h isdigit()
- Replace domid -1 with INVALID_DOMID
- Fix up a couple of mistaken function names in strings
- Copy CTYPE macro from libxl_internal.h
- Fix to usb-list manpage (use -d domid)
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
---
docs/man/xl.pod.1 | 30 +++++++
tools/libxl/xl.h | 21 +++++
tools/libxl/xl_cmdimpl.c | 199 +++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 15 ++++
4 files changed, 265 insertions(+)
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 57c6a79..185fd3f 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1147,6 +1147,36 @@ List virtual network interfaces for a domain.
=back
+=head2 USB DEVICES
+
+=over 4
+
+=item B<usb-attach> I<domain-id> I<hostdev:hostbus.hostaddr>
+
+Passes through the host USB device specified by I<hostbus.hostaddr>. At
+the moment this will only work for HVM domains via qemu.
+
+The best way to find out the information for the device is typically using
+lsusb.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<usb-detach> I<domain-id> I<hostdev:hosbus.hostaddr>
+
+Remove the host USB device from I<domain-id> which is specified
+by <hostbus.hostaddr>. This command only works for devices added
+with usb-add; not for those specified in the config file.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<usb-list> I<domain-id>
+
+Show host USB devices assigned to the guest.
+
+=back
+
=head2 VTPM DEVICES
=over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 771b4af..99ad74c 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -35,6 +35,9 @@ int main_info(int argc, char **argv);
int main_sharing(int argc, char **argv);
int main_cd_eject(int argc, char **argv);
int main_cd_insert(int argc, char **argv);
+int main_usb_attach(int argc, char **argv);
+int main_usb_detach(int argc, char **argv);
+int main_usb_list(int argc, char **argv);
int main_console(int argc, char **argv);
int main_vncviewer(int argc, char **argv);
int main_pcilist(int argc, char **argv);
@@ -164,6 +167,24 @@ extern void printf_info_sexp(int domid, libxl_domain_config
*d_config);
#define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf"
#define XL_LOCK_FILE XEN_LOCK_DIR "/xl"
+/*
+ * int CTYPE(ISFOO, char c);
+ * int CTYPE(toupper, char c);
+ * int CTYPE(tolower, char c);
+ *
+ * This is necessary because passing a simple char to a ctype.h
+ * is forbidden. ctype.h macros take ints derived from _unsigned_ chars.
+ *
+ * If you have a char which might be EOF then you should already have
+ * it in an int representing an unsigned char, and you can use the
+ * <ctype.h> macros directly. This generally happens only with values
+ * from fgetc et al.
+ *
+ * For any value known to be a character (eg, anything that came from
+ * a char[]), use CTYPE.
+ */
+#define CTYPE(isfoo,c) (isfoo((unsigned char)(c)))
+
#endif /* XL_H */
/*
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 98ecf67..014b25a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2602,6 +2602,205 @@ int main_cd_insert(int argc, char **argv)
return 0;
}
+static int parse_usb_specifier(libxl_device_usb *dev, char *s)
+{
+ char *devtype, * hostbus, *hostaddr, *p;
+
+ /*
+ * General format:
+ * <type>[:<type-specific options>]
+ */
+ p = s;
+ devtype = strsep(&p, ":");
+
+ if (strcmp(devtype, "hostdev")) {
+ fprintf(stderr, "Unknown device type: %s\n", devtype);
+ return -1;
+ }
+
+ hostbus = strsep(&p, ".");
+
+ hostaddr = p;
+
+ if (!hostbus) {
+ fprintf(stderr, "Device type %s requires hostaddr\n",
+ devtype);
+ return -1;
+ }
+
+ if (!hostaddr) {
+ fprintf(stderr, "Device type %s requires hostaddr\n",
+ devtype);
+ return -1;
+ }
+
+ /* Make sure they look right */
+ for (p=hostbus; *p; p++) {
+ if (!CTYPE(isdigit,*p)) {
+ fprintf(stderr, "Bad hostbus: %s\n", hostbus);
+ return -1;
+ }
+ }
+
+ for (p=hostaddr; *p; p++) {
+ if (!CTYPE(isdigit,*p)) {
+ fprintf(stderr, "Bad hostaddr: %s\n", hostaddr);
+ return -1;
+ }
+ }
+
+ dev->type = LIBXL_DEVICE_USB_TYPE_HOSTDEV;
+
+ dev->u.hostdev.hostbus = atoi(hostbus);
+ dev->u.hostdev.hostaddr = atoi(hostaddr);
+
+ return 0;
+}
+
+static int usb_attach(uint32_t domid, char * device)
+{
+ libxl_device_usb usbdev;
+ int rc;
+
+ libxl_device_usb_init(&usbdev);
+
+ if (parse_usb_specifier(&usbdev, device)<0) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL);
+ if (rc<0)
+ fprintf(stderr, "libxl_device_usb_add failed.\n");
+
+ libxl_device_usb_dispose(&usbdev);
+
+out:
+ return rc;
+}
+
+int main_usb_attach(int argc, char **argv)
+{
+ uint32_t domid = INVALID_DOMID;
+ int opt = 0, rc;
+ char *device = NULL;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind]);
+ device = argv[optind + 1];
+
+ if (domid == INVALID_DOMID) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-attach");
+ return 2;
+ }
+
+ rc = usb_attach(domid, device);
+ if (rc<0)
+ return 1;
+ else
+ return 0;
+}
+
+static int usb_detach(uint32_t domid, libxl_device_usb_type type,
+ char * device)
+{
+ libxl_device_usb usbdev;
+ int rc;
+
+ libxl_device_usb_init(&usbdev);
+
+ if (parse_usb_specifier(&usbdev, device)<0) {
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ rc = libxl_device_usb_remove(ctx, domid, &usbdev, NULL);
+ if (rc<0)
+ fprintf(stderr, "libxl_device_usb_remove failed.\n");
+
+ libxl_device_usb_dispose(&usbdev);
+
+out:
+ return rc;
+}
+
+int main_usb_detach(int argc, char **argv)
+{
+ uint32_t domid = INVALID_DOMID;
+ int opt = 0, rc;
+ char *device = NULL;
+ int type = 0;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-detach", 2) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind]);
+ device = argv[optind + 1];
+
+ if (domid == INVALID_DOMID) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-detach");
+ return 2;
+ }
+
+ rc = usb_detach(domid, type, device);
+ if (rc < 0)
+ return 1;
+ else
+ return 0;
+}
+
+static void usb_list(uint32_t domid)
+{
+ libxl_device_usb *dev;
+ int num, i;
+
+ dev = libxl_device_usb_list(ctx, domid, &num);
+ if (dev == NULL)
+ return;
+ printf("protocol backend type device\n");
+ for (i = 0; i < num; i++) {
+ printf("%8s ",
(dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm");
+ printf("%7d ", dev[i].backend_domid);
+ printf("%7s ",
(dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown");
+ if (dev[i].type == LIBXL_DEVICE_USB_TYPE_HOSTDEV)
+ printf("%03d.%03d",
+ dev[i].u.hostdev.hostbus,
+ dev[i].u.hostdev.hostaddr);
+ printf("\n");
+ }
+ free(dev);
+}
+
+
+int main_usb_list(int argc, char **argv)
+{
+ uint32_t domid = INVALID_DOMID;
+ int opt;
+
+ SWITCH_FOREACH_OPT(opt, "", NULL, "usb-list", 1) {
+ /* No options */
+ }
+
+ domid = find_domain(argv[optind]);
+
+ if (domid == INVALID_DOMID) {
+ fprintf(stderr, "Must specify domid\n\n");
+ help("usb-list");
+ return 2;
+ }
+
+ usb_list(domid);
+ return 0;
+}
+
+
+
int main_console(int argc, char **argv)
{
uint32_t domid;
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 00899f5..d1d7c13 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -187,6 +187,21 @@ struct cmd_spec cmd_table[] = {
"Eject a cdrom from a guest''s cd drive",
"<Domain> <VirtualDevice>",
},
+ { "usb-attach",
+ &main_usb_attach, 1, 1,
+ "Hot-plug a usb device to a domain.",
+ "<Domain> <hostdev:<hostbus.hostaddr>>",
+ },
+ { "usb-detach",
+ &main_usb_detach, 1, 1,
+ "Hot-unplug a usb device from a domain.",
+ "<Domain> <hostdev:hostbus.hostaddr>",
+ },
+ { "usb-list",
+ &main_usb_list, 0, 0,
+ "List usb devices for a domain",
+ "<Domain>",
+ },
{ "mem-max",
&main_memmax, 0, 1,
"Set the maximum amount reservation for a domain",
--
1.7.9.5
Ian Campbell
2013-Apr-24 11:56 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
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 1LIBXL_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),
Ian Campbell
2013-Apr-24 12:38 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
I didn''t see an implementation of libxl__device_usb_setdefault? On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote:> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, strlen(vm_path)); > rc = libxl__domain_rename(gc, *domid, 0, info->name, t); > if (rc) > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 3ba3a21..d2e00fa 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename); > /* Set dirty bitmap logging status */ > _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable); > _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk); > +/* Same as normal, but "translated" */You can use the IDL to do internal stuff too -- see tools/libxl/libxl_types_internal.idl What does "translated" mean? Which fields and how? Might it be better to include a pointer to the original libxl_device_usb instead of duplicating some/all of these fields?> +typedef struct libxl__device_usb { > + libxl_usb_protocol protocol; > + libxl_domid target_domid; > + libxl_domid backend_domid; > + libxl_domid dm_domid;The only use of this I see is: + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); ... + if (usbdev->dm_domid != 0) { twice, both times all in the same function so you could just use a local variable. If you remove this and pass target_domid as a parameter to various functions (which is the convention in libxl) then the need for this weird internal/external representation goes away.> + libxl_device_usb_type type; > + union { > + struct { > + int hostbus; > + int hostaddr; > + } hostdev; > + } u; > +} libxl__device_usb; > +_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid, > + libxl__device_usb *dev); > +_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid, > + libxl__device_usb *dev); > /* close and free the QMP handler */ > _hidden void libxl__qmp_close(libxl__qmp_handler *qmp); > /* remove the socket file, if the file has already been removed, > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c > index 644d2c0..9ad3e59 100644 > --- a/tools/libxl/libxl_qmp.c > +++ b/tools/libxl/libxl_qmp.cI''d like Anthony''s feedback on the QMP bits (CCd)> @@ -42,6 +42,7 @@ > > #define QMP_RECEIVE_BUFFER_SIZE 4096 > #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" > +#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x" > > typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, > const libxl__json_object *tree, > @@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, > } > } > > +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, > + libxl__device_usb *dev) > +{ > + libxl__json_object *args = NULL; > + char *id; > + > + id = GCSPRINTF(HOST_USB_QDEV_ID, > + (uint16_t)dev->u.hostdev.hostbus, > + (uint16_t)dev->u.hostdev.hostaddr);You could make these uint16 in the IDL if you wanted to restrict it like that. Even without that is the cast really necessary though, given the %x format string? If you do use uint16_t then you probably want to use PRIx16 too.> + > + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); > + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", dev->u.hostdev.hostbus); > + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", dev->u.hostdev.hostaddr); > + > + qmp_parameters_add_string(gc, &args, "id", id); > + > + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); > +} > + > +int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb *usbdev) > +{ > + int rc; > + switch (usbdev->type) { > + case LIBXL_DEVICE_USB_TYPE_HOSTDEV:Will this function ever handle anything other than HOSTDEV? AIUI the only other potential type is PV which won''t come anywhere near here? The switch only really makes sense if there are going to be several valid options, if its HOSTDEV or nothing then an if (type != HOSTDEV) return ERROR_INVAL would be preferable IMO, and I would do that in the hostdev_add function and get rid of this wrapper (same comments on the remove bits)> 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''}), > + ("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),Do you not want to add a list of USB devices to the domain_build_info?> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c > new file mode 100644 > index 0000000..c320487 > --- /dev/null > +++ b/tools/libxl/libxl_usb.c > @@ -0,0 +1,526 @@ > +/* > + * Copyright (C) 2009 Citrix Ltd. > + * Author Vincent Hanquez <vincent.hanquez@eu.citrix.com> > + * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>Probably not true?> +/* > + * /libxl/<domid>/usb/<devid>/{type, protocol, backend, [typeinfo]} > + */ > +#define USB_INFO_PATH "%s/usb"This duplicates something you added in libxl_create.c I think. I nearly suggested moving that code into this file as a libxl__domain_usb_init or something like that.> +#define USB_HOSTDEV_ID "hostdev-%04x-%04x" > + > +/* > + * Just use plain numbers for now. Replace these with strings at some point. > + */ > +static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid, > + libxl__device_usb *usbdev) > +{ > + return GCSPRINTF(USB_INFO_PATH "/%s", > + libxl__xs_libxl_path(gc, domid), > + GCSPRINTF(USB_HOSTDEV_ID, > + (uint16_t)usbdev->u.hostdev.hostbus, > + (uint16_t)usbdev->u.hostdev.hostaddr)); > +} > + > +static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev, > + flexarray_t *a) > +{ > + flexarray_append_pair(a, "hostbus", > + GCSPRINTF("%d", > + usbdev->u.hostdev.hostbus)); > + flexarray_append_pair(a, "hostaddr", > + GCSPRINTF("%d", > + usbdev->u.hostdev.hostaddr));Are the second and third lines > 80 chars in total? [...]> +static int usb_add_xenstore(libxl__gc *gc, uint32_t domid, > + libxl__device_usb *usbdev) > +{[...]> + default: > + LOG(ERROR, "Invalid device type: %d", usbdev->type); > + return ERROR_FAIL; > + } > + > + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to xenstore");You use a mixture of this and the LOG(DEBUG,...) form in this patch, we tend to prefer the latter for new code.> + > + for(;;) { > + rc = libxl__xs_transaction_start(gc, &t); > + if (rc) goto out; > + > + /* Helpfully, mkdir returns 0 on failure, 1 on success */ > + if (!libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm))) { > + rc = ERROR_FAIL; > + goto out; > + } > + > + /* And libxl__xs_writev *always* returns 0 no matter what */Strictly speaking the *current* implementation always returns 0, but if someone changes that your code will break. But none of the other callers check it either, so meh ;-)> + libxl__xs_writev(gc, t, dev_path, > + libxl__xs_kvs_of_flexarray(gc, dev, dev->count)); > + > + rc = libxl__xs_transaction_commit(gc, &t); > + if (!rc) break; > + if (rc <0) goto out; > + } > + > +out:According to libxl_internal.h you need to call libxl__xs_transaction_abort on the error path. [...]> + > +static int is_usbdev_type_hostdev_equal(libxl__device_usb *a, > + libxl__device_usb *b) > +{ > + if (!memcmp(&a->u.hostdev, &b->u.hostdev, sizeof(a->u.hostdev)))I''m not 100% sure it is permissible to compare structs for equality in this way. What if the compiler decides to add some padding which doesn''t get initialised? I''m afraid this probably means you have to compare field by field.> + return 1; > + else > + return 0; > +} > +static void usbdev_ext_to_int(libxl__device_usb *dev_int, > + libxl_device_usb *dev_ext) > +{ > + dev_int->protocol = dev_ext->protocol; > + > + if (dev_ext->backend_domid == LIBXL_DEVICE_USB_BACKEND_DEFAULT) > + dev_int->backend_domid = 0; > + else > + dev_int->backend_domid = dev_ext->backend_domid; > + > + dev_int->type = dev_ext->type; > + memcpy(&dev_int->u, &dev_ext->u, sizeof(dev_ext->u));I don''t think you can be sure that these two things have been laid out the same by the compiler. If you make dev_ext->backend_domid default to 0 then I''m not entirely sure what the point of this internal/external distinction is.> +static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid, > + libxl_device_usb *dev_ext) > +{ > + libxl_ctx *ctx = libxl__gc_owner(gc); > + libxl__device_usb *assigned, _usbdev, *usbdev; > + int rc = ERROR_FAIL, num_assigned; > + libxl_domain_type domtype = libxl__domain_type(gc, domid); > + > + /* Interpret incoming */ > + usbdev = &_usbdev; > + > + usbdev->target_domid = domid; > + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); > + > + usbdev_ext_to_int(usbdev, dev_ext); > + > + if (usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO) { > + if (domtype == LIBXL_DOMAIN_TYPE_PV) { > + usbdev->protocol = LIBXL_USB_PROTOCOL_PV; > + } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) { > + /* FIXME: See if we can detect PV frontend */ > + usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL; > + } > + } > + > + /* Check to make sure we''re doing something that''s impemented */"implemented"> + if (usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) { > + rc = ERROR_FAIL; > + LOG(ERROR, "Protocol not implemented"); > + goto out; > + } > + > + if (usbdev->dm_domid != 0) { > + rc = ERROR_FAIL; > + LOG(ERROR, "Stubdoms not yet supported"); > + goto out; > + } > + > + /* Double-check to make sure it''s not already assigned */ > + rc = get_assigned_devices(gc, domid, &assigned, &num_assigned); > + if (rc) { > + LOG(ERROR, "cannot determine if device is assigned," > + " refusing to continue"); > + goto out; > + } > + if (is_usbdev_in_array(assigned, num_assigned, usbdev)) { > + LOG(ERROR, "USB device already attached to a domain"); > + rc = ERROR_FAIL; > + goto out; > + } > + > + /* Do the add */ > + if (do_usb_add(gc, domid, usbdev)) > + rc = ERROR_FAIL; > + > +out: > + return rc; > +} > + > +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, > + libxl_device_usb *usbdev, > + const libxl_asyncop_how *ao_how) > +{ > + AO_CREATE(ctx, domid, ao_how); > + int rc; > + rc = libxl__device_usb_add(gc, domid, usbdev); > + libxl__ao_complete(egc, ao, rc); > + return AO_INPROGRESS; > +}Please define this (and the remove) using the DEFINE_DEVICE_ADD/REMOVE macros in libxl.c. Not least because I''m not convinced your handling of the AO stuff is correct (or indeed present). This will probably require the libxl__device_usb_add/remove to change to do the AIO stuff too -- i.e. at the end: aodev->rc = rc; if (rc) aodev->callback(egc, aodev); return; Ian.
On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote:> + > +int main_usb_attach(int argc, char **argv) > +{ > + uint32_t domid = INVALID_DOMID; > + int opt = 0, rc; > + char *device = NULL; > + > + SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) { > + /* No options */ > + } > + > + domid = find_domain(argv[optind]); > + device = argv[optind + 1]; > + > + if (domid == INVALID_DOMID) { > + fprintf(stderr, "Must specify domid\n\n"); > + help("usb-attach"); > + return 2; > + }find_domain won''t return in this case, so no need to worry about it yourself.> +int main_usb_detach(int argc, char **argv) > +{ > + uint32_t domid = INVALID_DOMID; > + int opt = 0, rc; > + char *device = NULL; > + int type = 0; > + > + SWITCH_FOREACH_OPT(opt, "", NULL, "usb-detach", 2) { > + /* No options */ > + } > + > + domid = find_domain(argv[optind]); > + device = argv[optind + 1]; > + > + if (domid == INVALID_DOMID) { > + fprintf(stderr, "Must specify domid\n\n"); > + help("usb-detach"); > + return 2; > + }Same again.> + > + rc = usb_detach(domid, type, device); > + if (rc < 0) > + return 1; > + else > + return 0; > +} > + > +static void usb_list(uint32_t domid) > +{ > + libxl_device_usb *dev; > + int num, i; > + > + dev = libxl_device_usb_list(ctx, domid, &num); > + if (dev == NULL) > + return; > + printf("protocol backend type device\n"); > + for (i = 0; i < num; i++) { > + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm");You can use libxl_usb_protocol_to_string here.> + printf("%7d ", dev[i].backend_domid); > + printf("%7s ", (dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown");libxl_device_usb_type_to_string.> + if (dev[i].type == LIBXL_DEVICE_USB_TYPE_HOSTDEV) > + printf("%03d.%03d", > + dev[i].u.hostdev.hostbus, > + dev[i].u.hostdev.hostaddr); > + printf("\n"); > + } > + free(dev);You leak the content of the devices, you need to call libxl_device_usb_dispose on each, or better define libxl_device_usb_list_free (this is inconsistently provided by other devices, but may as well get it right for new code). Ian.
George Dunlap
2013-Apr-24 12:48 UTC
Re: [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@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
Ian Campbell
2013-Apr-24 12:53 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, 2013-04-24 at 13:48 +0100, George Dunlap wrote:> > > >> + > >> +/* > >> * 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?How would the application know whether it should expose this functionality to its users or not? It''d be pretty lame for them (either the app or the end user) to have to try it and see if it worked.> > >> + * > >> + * 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.From libxl.h: * libxl_device_<type>_remove(ctx, domid, device): * * Removes the given device from the specified domain by performing * an orderly unplug with guest co-operation. This requires that the * guest is running. * * This method is currently synchronous and therefore can block * while interacting with the guest. * * libxl_device_<type>_destroy(ctx, domid, device): * * Removes the given device from the specified domain without guest * co-operation. It is guest specific what affect this will have on * a running guest. * * This function does not interact with the guest and therefore * cannot block on the guest.> > 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".We''ll need to fix all the devices when this happens, so I think it is better for USB to be consistent with them for now.> 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).Until you support a driver domain for these devices the caller (xl) should just leave this field alone as initilised by libxl_device_usb_init. i.e. you should only set it if the user supplied a backenddomid= option in the cfg (which you don''t implement, so you should never set it). Ian.
George Dunlap
2013-Apr-24 13:32 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 24/04/13 13:38, Ian Campbell wrote:> I didn''t see an implementation of libxl__device_usb_setdefault?You mean, for the internal structure?> > On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote: >> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, strlen(vm_path)); >> rc = libxl__domain_rename(gc, *domid, 0, info->name, t); >> if (rc) >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index 3ba3a21..d2e00fa 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename); >> /* Set dirty bitmap logging status */ >> _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable); >> _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk); >> +/* Same as normal, but "translated" */ > You can use the IDL to do internal stuff too -- see > tools/libxl/libxl_types_internal.idl > > What does "translated" mean? Which fields and how? Might it be better to > include a pointer to the original libxl_device_usb instead of > duplicating some/all of these fields?I think one of the biggest thing was avoiding having to look up the stubdomain in a bunch of different functions -- that''s the dm_domid. There''s also the translation of "AUTO" protocol into PV or HVM, and (originally) the translation of BACKEND_DEFAULT into the actual backend domain. If we were willing to have the library change the protocol in the structure passed to it, then a pointer might work as well.> >> +typedef struct libxl__device_usb { >> + libxl_usb_protocol protocol; >> + libxl_domid target_domid; >> + libxl_domid backend_domid; >> + libxl_domid dm_domid; > The only use of this I see is: > + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); > ... > + if (usbdev->dm_domid != 0) { > > twice, both times all in the same function so you could just use a local > variable.A big part of that is because we don''t support stubdomains yet. When we do, there will be more complicated cases to consider.> If you remove this and pass target_domid as a parameter to various > functions (which is the convention in libxl) then the need for this > weird internal/external representation goes away.There''s also the resolution of protocol from ANY into PV or HVM; but if we''re willing to change the structure passed in by the caller, then yes, we can get rid of this struct for now and consider re-introducing it if/when we actually need it.> >> + libxl_device_usb_type type; >> + union { >> + struct { >> + int hostbus; >> + int hostaddr; >> + } hostdev; >> + } u; >> +} libxl__device_usb; >> +_hidden int libxl__qmp_usb_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev); >> +_hidden int libxl__qmp_usb_remove(libxl__gc *gc, int domid, >> + libxl__device_usb *dev); >> /* close and free the QMP handler */ >> _hidden void libxl__qmp_close(libxl__qmp_handler *qmp); >> /* remove the socket file, if the file has already been removed, >> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c >> index 644d2c0..9ad3e59 100644 >> --- a/tools/libxl/libxl_qmp.c >> +++ b/tools/libxl/libxl_qmp.c > I''d like Anthony''s feedback on the QMP bits (CCd) > >> @@ -42,6 +42,7 @@ >> >> #define QMP_RECEIVE_BUFFER_SIZE 4096 >> #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" >> +#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x" >> >> typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, >> const libxl__json_object *tree, >> @@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, >> } >> } >> >> +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev) >> +{ >> + libxl__json_object *args = NULL; >> + char *id; >> + >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t)dev->u.hostdev.hostbus, >> + (uint16_t)dev->u.hostdev.hostaddr); > You could make these uint16 in the IDL if you wanted to restrict it like > that. Even without that is the cast really necessary though, given the > %x format string? > > If you do use uint16_t then you probably want to use PRIx16 too.This actually a vestigial artifact from the fact that you could originally specify ANY in the field, which was encoded as -1. I''ll just take out now.>> + >> + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); >> + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", dev->u.hostdev.hostbus); >> + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", dev->u.hostdev.hostaddr); >> + >> + qmp_parameters_add_string(gc, &args, "id", id); >> + >> + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); >> +} >> + >> +int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb *usbdev) >> +{ >> + int rc; >> + switch (usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: > Will this function ever handle anything other than HOSTDEV? AIUI the > only other potential type is PV which won''t come anywhere near here?You''re mixing up protocol and device type. PV will only ever handle HOSTDEV types, but qmp allows a wide range of virtual devices: tablets, mice, keyboards, thumb drives, &c. I''d like at some point for all USB devices specified in the config file to be able to be listed and hot-plugged / hot-unplugged.> > (same comments on the remove bits) > >> 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''}), >> + ("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), > Do you not want to add a list of USB devices to the domain_build_info?I don''t think I can get that functionality working for 4.3. It''s definitely on my plans for the future, though.> >> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c >> new file mode 100644 >> index 0000000..c320487 >> --- /dev/null >> +++ b/tools/libxl/libxl_usb.c >> @@ -0,0 +1,526 @@ >> +/* >> + * Copyright (C) 2009 Citrix Ltd. >> + * Author Vincent Hanquez <vincent.hanquez@eu.citrix.com> >> + * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Probably not true?Haha -- oops...> >> +/* >> + * /libxl/<domid>/usb/<devid>/{type, protocol, backend, [typeinfo]} >> + */ >> +#define USB_INFO_PATH "%s/usb" > This duplicates something you added in libxl_create.c I think. I nearly > suggested moving that code into this file as a libxl__domain_usb_init or > something like that.The other option would be to make a function like libxl__xs_libxl_path(), but that seemed a bit heavy-handed to me. I guess libxl__domain_usb_init sounds good.> >> +#define USB_HOSTDEV_ID "hostdev-%04x-%04x" >> + >> +/* >> + * Just use plain numbers for now. Replace these with strings at some point. >> + */ >> +static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ >> + return GCSPRINTF(USB_INFO_PATH "/%s", >> + libxl__xs_libxl_path(gc, domid), >> + GCSPRINTF(USB_HOSTDEV_ID, >> + (uint16_t)usbdev->u.hostdev.hostbus, >> + (uint16_t)usbdev->u.hostdev.hostaddr)); >> +} >> + >> +static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev, >> + flexarray_t *a) >> +{ >> + flexarray_append_pair(a, "hostbus", >> + GCSPRINTF("%d", >> + usbdev->u.hostdev.hostbus)); >> + flexarray_append_pair(a, "hostaddr", >> + GCSPRINTF("%d", >> + usbdev->u.hostdev.hostaddr)); > Are the second and third lines > 80 chars in total?You mean, "Why did you break the GSPRINTF into two lines?" Yes, because on one line it''s > 80 chars.> > [...] >> +static int usb_add_xenstore(libxl__gc *gc, uint32_t domid, >> + libxl__device_usb *usbdev) >> +{ > [...] >> + default: >> + LOG(ERROR, "Invalid device type: %d", usbdev->type); >> + return ERROR_FAIL; >> + } >> + >> + LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to xenstore"); > You use a mixture of this and the LOG(DEBUG,...) form in this patch, we > tend to prefer the latter for new code.Ack.> >> + >> + for(;;) { >> + rc = libxl__xs_transaction_start(gc, &t); >> + if (rc) goto out; >> + >> + /* Helpfully, mkdir returns 0 on failure, 1 on success */ >> + if (!libxl__xs_mkdir(gc, t, dev_path, noperm, ARRAY_SIZE(noperm))) { >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + /* And libxl__xs_writev *always* returns 0 no matter what */ > Strictly speaking the *current* implementation always returns 0, but if > someone changes that your code will break. But none of the other callers > check it either, so meh ;-)In fact, I already have tried changing it, and the result is that domain creation fails -- so whoever does change it will have a lot of cleaning up to do anyway. :-) (This is on my to-do list for the 4.4 dev cycle.)>> + libxl__xs_writev(gc, t, dev_path, >> + libxl__xs_kvs_of_flexarray(gc, dev, dev->count)); >> + >> + rc = libxl__xs_transaction_commit(gc, &t); >> + if (!rc) break; >> + if (rc <0) goto out; >> + } >> + >> +out: > According to libxl_internal.h you need to call > libxl__xs_transaction_abort on the error path.Good catch -- sorry I missed that.> > [...] > >> + >> +static int is_usbdev_type_hostdev_equal(libxl__device_usb *a, >> + libxl__device_usb *b) >> +{ >> + if (!memcmp(&a->u.hostdev, &b->u.hostdev, sizeof(a->u.hostdev))) > I''m not 100% sure it is permissible to compare structs for equality in > this way. What if the compiler decides to add some padding which doesn''t > get initialised? > > I''m afraid this probably means you have to compare field by field.Ack.> >> + return 1; >> + else >> + return 0; >> +} >> +static void usbdev_ext_to_int(libxl__device_usb *dev_int, >> + libxl_device_usb *dev_ext) >> +{ >> + dev_int->protocol = dev_ext->protocol; >> + >> + if (dev_ext->backend_domid == LIBXL_DEVICE_USB_BACKEND_DEFAULT) >> + dev_int->backend_domid = 0; >> + else >> + dev_int->backend_domid = dev_ext->backend_domid; >> + >> + dev_int->type = dev_ext->type; >> + memcpy(&dev_int->u, &dev_ext->u, sizeof(dev_ext->u)); > I don''t think you can be sure that these two things have been laid out > the same by the compiler.Ack.> > If you make dev_ext->backend_domid default to 0 then I''m not entirely > sure what the point of this internal/external distinction is.> >> +static int libxl__device_usb_add(libxl__gc *gc, uint32_t domid, >> + libxl_device_usb *dev_ext) >> +{ >> + libxl_ctx *ctx = libxl__gc_owner(gc); >> + libxl__device_usb *assigned, _usbdev, *usbdev; >> + int rc = ERROR_FAIL, num_assigned; >> + libxl_domain_type domtype = libxl__domain_type(gc, domid); >> + >> + /* Interpret incoming */ >> + usbdev = &_usbdev; >> + >> + usbdev->target_domid = domid; >> + usbdev->dm_domid = libxl_get_stubdom_id(ctx, domid); >> + >> + usbdev_ext_to_int(usbdev, dev_ext); >> + >> + if (usbdev->protocol == LIBXL_USB_PROTOCOL_AUTO) { >> + if (domtype == LIBXL_DOMAIN_TYPE_PV) { >> + usbdev->protocol = LIBXL_USB_PROTOCOL_PV; >> + } else if (domtype == LIBXL_DOMAIN_TYPE_HVM) { >> + /* FIXME: See if we can detect PV frontend */ >> + usbdev->protocol = LIBXL_USB_PROTOCOL_DEVICEMODEL; >> + } >> + } >> + >> + /* Check to make sure we''re doing something that''s impemented */ > "implemented"> >> + if (usbdev->protocol != LIBXL_USB_PROTOCOL_DEVICEMODEL) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Protocol not implemented"); >> + goto out; >> + } >> + >> + if (usbdev->dm_domid != 0) { >> + rc = ERROR_FAIL; >> + LOG(ERROR, "Stubdoms not yet supported"); >> + goto out; >> + } >> + >> + /* Double-check to make sure it''s not already assigned */ >> + rc = get_assigned_devices(gc, domid, &assigned, &num_assigned); >> + if (rc) { >> + LOG(ERROR, "cannot determine if device is assigned," >> + " refusing to continue"); >> + goto out; >> + } >> + if (is_usbdev_in_array(assigned, num_assigned, usbdev)) { >> + LOG(ERROR, "USB device already attached to a domain"); >> + rc = ERROR_FAIL; >> + goto out; >> + } >> + >> + /* Do the add */ >> + if (do_usb_add(gc, domid, usbdev)) >> + rc = ERROR_FAIL; >> + >> +out: >> + return rc; >> +} >> + >> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, >> + libxl_device_usb *usbdev, >> + const libxl_asyncop_how *ao_how) >> +{ >> + AO_CREATE(ctx, domid, ao_how); >> + int rc; >> + rc = libxl__device_usb_add(gc, domid, usbdev); >> + libxl__ao_complete(egc, ao, rc); >> + return AO_INPROGRESS; >> +} > Please define this (and the remove) using the DEFINE_DEVICE_ADD/REMOVE > macros in libxl.c. Not least because I''m not convinced your handling of > the AO stuff is correct (or indeed present). > > This will probably require the libxl__device_usb_add/remove to change to > do the AIO stuff too -- i.e. at the end: > aodev->rc = rc; > if (rc) aodev->callback(egc, aodev); > return;These follow the template set by libxl_pci.c:libxl_device_pci_{add,remove,destroy}(), which were chosen specifically because they essentially do a null ASYNC, but interface-wise allow for real async to be added in the future. -George
George Dunlap
2013-Apr-24 13:37 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 24/04/13 13:53, Ian Campbell wrote:> On Wed, 2013-04-24 at 13:48 +0100, George Dunlap wrote: > >>>> + >>>> +/* >>>> * 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? > How would the application know whether it should expose this > functionality to its users or not? > > It''d be pretty lame for them (either the app or the end user) to have to > try it and see if it worked.On the whole, I''m beginning to think that we should just punt the whole USB thing to 4.4 and do it properly the first time, instead of doing it piecemeal.>>>> + * >>>> + * 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. > From libxl.h: > > * libxl_device_<type>_remove(ctx, domid, device): > * > * Removes the given device from the specified domain by performing > * an orderly unplug with guest co-operation. This requires that the > * guest is running. > * > * This method is currently synchronous and therefore can block > * while interacting with the guest. > * > * libxl_device_<type>_destroy(ctx, domid, device): > * > * Removes the given device from the specified domain without guest > * co-operation. It is guest specific what affect this will have on > * a running guest. > * > * This function does not interact with the guest and therefore > * cannot block on the guest.I saw that, but AFAICT there is not and never will be a distinguishable difference. I see that the same is true for pci devices, as there are two functions which call the same function internally. I''ll do the same thing. :-)> > >>> 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". > We''ll need to fix all the devices when this happens, so I think it is > better for USB to be consistent with them for now. > >> 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). > Until you support a driver domain for these devices the caller (xl) > should just leave this field alone as initilised by > libxl_device_usb_init. i.e. you should only set it if the user supplied > a backenddomid= option in the cfg (which you don''t implement, so you > should never set it).Ack. -George
Ian Campbell
2013-Apr-24 13:47 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote:> On 24/04/13 13:38, Ian Campbell wrote: > > I didn''t see an implementation of libxl__device_usb_setdefault? > > You mean, for the internal structure?No, for the external structure, but called only internally in libxl. From libxl_internal.h: /* * For each aggregate type which can be used as an input we provide: * * int libxl__<type>_setdefault(gc, <type> *p): * * Idempotently sets any members of "p" which is currently set to * a special value indicating that the defaults should be used * (per libxl_<type>_init) to a specific value. * * All libxl API functions are expected to have arranged for this * to be called before using any values within these structures. */> > > > > On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote: > >> xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/vm", dom_path), vm_path, strlen(vm_path)); > >> rc = libxl__domain_rename(gc, *domid, 0, info->name, t); > >> if (rc) > >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > >> index 3ba3a21..d2e00fa 100644 > >> --- a/tools/libxl/libxl_internal.h > >> +++ b/tools/libxl/libxl_internal.h > >> @@ -1412,6 +1412,24 @@ _hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename); > >> /* Set dirty bitmap logging status */ > >> _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enable); > >> _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk); > >> +/* Same as normal, but "translated" */ > > You can use the IDL to do internal stuff too -- see > > tools/libxl/libxl_types_internal.idl > > > > What does "translated" mean? Which fields and how? Might it be better to > > include a pointer to the original libxl_device_usb instead of > > duplicating some/all of these fields? > > I think one of the biggest thing was avoiding having to look up the > stubdomain in a bunch of different functions -- that''s the dm_domid.Yet you look it up in the two exact places that you use it...> There''s also the translation of "AUTO" protocol into PV or HVM, and > (originally) the translation of BACKEND_DEFAULT into the actual backend > domain. > > If we were willing to have the library change the protocol in the > structure passed to it, then a pointer might work as well.This is normal libxl behaviour, implemented via the setdefault function.> > If you remove this and pass target_domid as a parameter to various > > functions (which is the convention in libxl) then the need for this > > weird internal/external representation goes away. > > There''s also the resolution of protocol from ANY into PV or HVM; but if > we''re willing to change the structure passed in by the caller, then yes, > we can get rid of this struct for now and consider re-introducing it > if/when we actually need it.Yes, please just nuke it.> >> +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, > >> + libxl__device_usb *dev) > >> +{ > >> + libxl__json_object *args = NULL; > >> + char *id; > >> + > >> + id = GCSPRINTF(HOST_USB_QDEV_ID, > >> + (uint16_t)dev->u.hostdev.hostbus, > >> + (uint16_t)dev->u.hostdev.hostaddr); > > You could make these uint16 in the IDL if you wanted to restrict it like > > that. Even without that is the cast really necessary though, given the > > %x format string? > > > > If you do use uint16_t then you probably want to use PRIx16 too. > > This actually a vestigial artifact from the fact that you could > originally specify ANY in the field, which was encoded as -1. I''ll just > take out now.OK. BTW I saw some others that I didn''t comment on, so you may want to grep around a bit.> > >> + > >> + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); > >> + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", dev->u.hostdev.hostbus); > >> + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", dev->u.hostdev.hostaddr); > >> + > >> + qmp_parameters_add_string(gc, &args, "id", id); > >> + > >> + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); > >> +} > >> + > >> +int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb *usbdev) > >> +{ > >> + int rc; > >> + switch (usbdev->type) { > >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: > > Will this function ever handle anything other than HOSTDEV? AIUI the > > only other potential type is PV which won''t come anywhere near here? > > You''re mixing up protocol and device type. PV will only ever handle > HOSTDEV types, but qmp allows a wide range of virtual devices: tablets, > mice, keyboards, thumb drives, &c. I''d like at some point for all USB > devices specified in the config file to be able to be listed and > hot-plugged / hot-unplugged.Ah, right.> > (same comments on the remove bits) > > > >> 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''}), > >> + ("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), > > Do you not want to add a list of USB devices to the domain_build_info? > > I don''t think I can get that functionality working for 4.3. It''s > definitely on my plans for the future, though.It should just be a case of ("usbdevs", Array(libxl_device_usb, "num_usbdevs")), in the idl and for (i = 0; i < d_config->num_usbdevs; i++) libxl__device_usb_add(gc, domid, &d_config->usbdevs[i], 1); in e.g. domcreate_attach_pci (which would then be better named domcreate_attach_sync_devices or something).> > > >> +#define USB_HOSTDEV_ID "hostdev-%04x-%04x" > >> + > >> +/* > >> + * Just use plain numbers for now. Replace these with strings at some point. > >> + */ > >> +static char * hostdev_xs_path(libxl__gc *gc, uint32_t domid, > >> + libxl__device_usb *usbdev) > >> +{ > >> + return GCSPRINTF(USB_INFO_PATH "/%s", > >> + libxl__xs_libxl_path(gc, domid), > >> + GCSPRINTF(USB_HOSTDEV_ID, > >> + (uint16_t)usbdev->u.hostdev.hostbus, > >> + (uint16_t)usbdev->u.hostdev.hostaddr)); > >> +} > >> + > >> +static void hostdev_xs_append_params(libxl__gc *gc, libxl__device_usb *usbdev, > >> + flexarray_t *a) > >> +{ > >> + flexarray_append_pair(a, "hostbus", > >> + GCSPRINTF("%d", > >> + usbdev->u.hostdev.hostbus)); > >> + flexarray_append_pair(a, "hostaddr", > >> + GCSPRINTF("%d", > >> + usbdev->u.hostdev.hostaddr)); > > Are the second and third lines > 80 chars in total? > > You mean, "Why did you break the GSPRINTF into two lines?" Yes, because > on one line it''s > 80 chars.OK.> >> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, > >> + libxl_device_usb *usbdev, > >> + const libxl_asyncop_how *ao_how) > >> +{ > >> + AO_CREATE(ctx, domid, ao_how); > >> + int rc; > >> + rc = libxl__device_usb_add(gc, domid, usbdev); > >> + libxl__ao_complete(egc, ao, rc); > >> + return AO_INPROGRESS; > >> +} > > Please define this (and the remove) using the DEFINE_DEVICE_ADD/REMOVE > > macros in libxl.c. Not least because I''m not convinced your handling of > > the AO stuff is correct (or indeed present). > > > > This will probably require the libxl__device_usb_add/remove to change to > > do the AIO stuff too -- i.e. at the end: > > aodev->rc = rc; > > if (rc) aodev->callback(egc, aodev); > > return; > > These follow the template set by > libxl_pci.c:libxl_device_pci_{add,remove,destroy}(), which were chosen > specifically because they essentially do a null ASYNC, but > interface-wise allow for real async to be added in the future.Ah, OK then. Ian.
Ian Campbell
2013-Apr-24 13:51 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote:> There''s also the translation of "AUTO" protocol into PV or HVM, andThis made me wonder, how is libxl_device_usb_protocol different from the type of the domain? Can you (or is the intention) use PV with an HVM domain? I suppose DEVICEMODEL is HVM only? Is "protocol" really the right word for this? I''d half expect it to mean USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW
Ian Campbell
2013-Apr-24 13:53 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, 2013-04-24 at 14:37 +0100, George Dunlap wrote:> On 24/04/13 13:53, Ian Campbell wrote: > > On Wed, 2013-04-24 at 13:48 +0100, George Dunlap wrote: > > > >>>> + > >>>> +/* > >>>> * 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? > > How would the application know whether it should expose this > > functionality to its users or not? > > > > It''d be pretty lame for them (either the app or the end user) to have to > > try it and see if it worked. > > On the whole, I''m beginning to think that we should just punt the whole > USB thing to 4.4 and do it properly the first time, instead of doing it > piecemeal.I''m also starting to wonder about this amount of code so far into the freeze.> > >>>> + * > >>>> + * 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. > > From libxl.h: > > > > * libxl_device_<type>_remove(ctx, domid, device): > > * > > * Removes the given device from the specified domain by performing > > * an orderly unplug with guest co-operation. This requires that the > > * guest is running. > > * > > * This method is currently synchronous and therefore can block > > * while interacting with the guest. > > * > > * libxl_device_<type>_destroy(ctx, domid, device): > > * > > * Removes the given device from the specified domain without guest > > * co-operation. It is guest specific what affect this will have on > > * a running guest. > > * > > * This function does not interact with the guest and therefore > > * cannot block on the guest. > > I saw that, but AFAICT there is not and never will be a distinguishable > difference.I expect there will be when the PV USB front and backends are involved.> I see that the same is true for pci devices, as there are two functions > which call the same function internally. I''ll do the same thing. :-)For now that''ll do I think.
George Dunlap
2013-Apr-24 15:45 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 24/04/13 14:51, Ian Campbell wrote:> On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: > >> There''s also the translation of "AUTO" protocol into PV or HVM, and > This made me wonder, how is libxl_device_usb_protocol different from the > type of the domain? Can you (or is the intention) use PV with an HVM > domain? I suppose DEVICEMODEL is HVM only?The intention is to allow HVM guests to use either DEVICEMODEL or PV as protocols.> Is "protocol" really the right word for this? I''d half expect it to mean > USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIWBut we''re already using ''type'' for the device type. :-) I think for nics it makes sense to call it a ''type'', as for NICs we refer to the *emulated device* as a NIC, or the PV device as a NIC, which is then (virtually) plugged into a bridge somewhere. But in this case I don''t think it makes sense, as it''s the actual host device you care about, and the way the guest talks to it is either via PV or qemu. "Protocol" may not be the very best option, but at least it gives you the idea of a conduit. -George
Ian Campbell
2013-Apr-24 15:51 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, 2013-04-24 at 16:45 +0100, George Dunlap wrote:> On 24/04/13 14:51, Ian Campbell wrote: > > On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: > > > >> There''s also the translation of "AUTO" protocol into PV or HVM, and > > This made me wonder, how is libxl_device_usb_protocol different from the > > type of the domain? Can you (or is the intention) use PV with an HVM > > domain? I suppose DEVICEMODEL is HVM only? > > The intention is to allow HVM guests to use either DEVICEMODEL or PV as > protocols. > > > Is "protocol" really the right word for this? I''d half expect it to mean > > USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW > > But we''re already using ''type'' for the device type. :-) > > I think for nics it makes sense to call it a ''type'', as for NICs we > refer to the *emulated device* as a NIC, or the PV device as a NIC, > which is then (virtually) plugged into a bridge somewhere. But in this > case I don''t think it makes sense, as it''s the actual host device you > care about, and the way the guest talks to it is either via PV or qemu. > "Protocol" may not be the very best option, but at least it gives you > the idea of a conduit.It''s more like a "method" then? Ian.
Pasi Kärkkäinen
2013-Apr-24 17:49 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, Apr 24, 2013 at 04:51:46PM +0100, Ian Campbell wrote:> On Wed, 2013-04-24 at 16:45 +0100, George Dunlap wrote: > > On 24/04/13 14:51, Ian Campbell wrote: > > > On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: > > > > > >> There''s also the translation of "AUTO" protocol into PV or HVM, and > > > This made me wonder, how is libxl_device_usb_protocol different from the > > > type of the domain? Can you (or is the intention) use PV with an HVM > > > domain? I suppose DEVICEMODEL is HVM only? > > > > The intention is to allow HVM guests to use either DEVICEMODEL or PV as > > protocols. > > > > > Is "protocol" really the right word for this? I''d half expect it to mean > > > USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW > > > > But we''re already using ''type'' for the device type. :-) > > > > I think for nics it makes sense to call it a ''type'', as for NICs we > > refer to the *emulated device* as a NIC, or the PV device as a NIC, > > which is then (virtually) plugged into a bridge somewhere. But in this > > case I don''t think it makes sense, as it''s the actual host device you > > care about, and the way the guest talks to it is either via PV or qemu. > > "Protocol" may not be the very best option, but at least it gives you > > the idea of a conduit. > > It''s more like a "method" then? >"protocol" reminded me about something.. when using devicemodel/qemu method, do we currently allow specifying if the usb device should be connected to usb2 (ehci) or usb3 (xhci) virtual controller? -- Pasi
Ian Campbell
2013-Apr-25 07:44 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Wed, 2013-04-24 at 18:49 +0100, Pasi Kärkkäinen wrote:> On Wed, Apr 24, 2013 at 04:51:46PM +0100, Ian Campbell wrote: > > On Wed, 2013-04-24 at 16:45 +0100, George Dunlap wrote: > > > On 24/04/13 14:51, Ian Campbell wrote: > > > > On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: > > > > > > > >> There's also the translation of "AUTO" protocol into PV or HVM, and > > > > This made me wonder, how is libxl_device_usb_protocol different from the > > > > type of the domain? Can you (or is the intention) use PV with an HVM > > > > domain? I suppose DEVICEMODEL is HVM only? > > > > > > The intention is to allow HVM guests to use either DEVICEMODEL or PV as > > > protocols. > > > > > > > Is "protocol" really the right word for this? I'd half expect it to mean > > > > USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW > > > > > > But we're already using 'type' for the device type. :-) > > > > > > I think for nics it makes sense to call it a 'type', as for NICs we > > > refer to the *emulated device* as a NIC, or the PV device as a NIC, > > > which is then (virtually) plugged into a bridge somewhere. But in this > > > case I don't think it makes sense, as it's the actual host device you > > > care about, and the way the guest talks to it is either via PV or qemu. > > > "Protocol" may not be the very best option, but at least it gives you > > > the idea of a conduit. > > > > It's more like a "method" then? > > > > "protocol" reminded me about something.. when using devicemodel/qemu method, > do we currently allow specifying if the usb device should be connected to > usb2 (ehci) or usb3 (xhci) virtual controller?Perhaps EMU_USB2 and EMU_USB3 would be preferable names to DEVICEMODEL? Not sure which the current devicemodel mode uses, the code doesn't appear to ask qemu for one or the other explicitly, I'd imagine the default would be USB2? The qmeu manpage only mentions UHCI. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Pasi Kärkkäinen
2013-Apr-25 07:54 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Thu, Apr 25, 2013 at 08:44:58AM +0100, Ian Campbell wrote:> On Wed, 2013-04-24 at 18:49 +0100, Pasi Kärkkäinen wrote: > > On Wed, Apr 24, 2013 at 04:51:46PM +0100, Ian Campbell wrote: > > > On Wed, 2013-04-24 at 16:45 +0100, George Dunlap wrote: > > > > On 24/04/13 14:51, Ian Campbell wrote: > > > > > On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: > > > > > > > > > >> There''s also the translation of "AUTO" protocol into PV or HVM, and > > > > > This made me wonder, how is libxl_device_usb_protocol different from the > > > > > type of the domain? Can you (or is the intention) use PV with an HVM > > > > > domain? I suppose DEVICEMODEL is HVM only? > > > > > > > > The intention is to allow HVM guests to use either DEVICEMODEL or PV as > > > > protocols. > > > > > > > > > Is "protocol" really the right word for this? I''d half expect it to mean > > > > > USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW > > > > > > > > But we''re already using ''type'' for the device type. :-) > > > > > > > > I think for nics it makes sense to call it a ''type'', as for NICs we > > > > refer to the *emulated device* as a NIC, or the PV device as a NIC, > > > > which is then (virtually) plugged into a bridge somewhere. But in this > > > > case I don''t think it makes sense, as it''s the actual host device you > > > > care about, and the way the guest talks to it is either via PV or qemu. > > > > "Protocol" may not be the very best option, but at least it gives you > > > > the idea of a conduit. > > > > > > It''s more like a "method" then? > > > > > > > "protocol" reminded me about something.. when using devicemodel/qemu method, > > do we currently allow specifying if the usb device should be connected to > > usb2 (ehci) or usb3 (xhci) virtual controller? > > Perhaps EMU_USB2 and EMU_USB3 would be preferable names to DEVICEMODEL? > > Not sure which the current devicemodel mode uses, the code doesn''t > appear to ask qemu for one or the other explicitly, I''d imagine the > default would be USB2? The qmeu manpage only mentions UHCI. >I think you''re able to set the (virtual) usb controller for each device on the qemu cmdline, but dunno if qemu has any built-in logic to decide which virtual usb controller to attach to if none is specified.. like automatically attach usb3 device from the host to virtual usb3/xhci controller? -- Pasi
George Dunlap
2013-Apr-25 09:56 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 04/25/2013 08:44 AM, Ian Campbell wrote:> On Wed, 2013-04-24 at 18:49 +0100, Pasi Kärkkäinen wrote: >> On Wed, Apr 24, 2013 at 04:51:46PM +0100, Ian Campbell wrote: >>> On Wed, 2013-04-24 at 16:45 +0100, George Dunlap wrote: >>>> On 24/04/13 14:51, Ian Campbell wrote: >>>>> On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: >>>>> >>>>>> There's also the translation of "AUTO" protocol into PV or HVM, and >>>>> This made me wonder, how is libxl_device_usb_protocol different from the >>>>> type of the domain? Can you (or is the intention) use PV with an HVM >>>>> domain? I suppose DEVICEMODEL is HVM only? >>>> >>>> The intention is to allow HVM guests to use either DEVICEMODEL or PV as >>>> protocols. >>>> >>>>> Is "protocol" really the right word for this? I'd half expect it to mean >>>>> USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW >>>> >>>> But we're already using 'type' for the device type. :-) >>>> >>>> I think for nics it makes sense to call it a 'type', as for NICs we >>>> refer to the *emulated device* as a NIC, or the PV device as a NIC, >>>> which is then (virtually) plugged into a bridge somewhere. But in this >>>> case I don't think it makes sense, as it's the actual host device you >>>> care about, and the way the guest talks to it is either via PV or qemu. >>>> "Protocol" may not be the very best option, but at least it gives you >>>> the idea of a conduit. >>> >>> It's more like a "method" then? >>> >> >> "protocol" reminded me about something.. when using devicemodel/qemu method, >> do we currently allow specifying if the usb device should be connected to >> usb2 (ehci) or usb3 (xhci) virtual controller? > > Perhaps EMU_USB2 and EMU_USB3 would be preferable names to DEVICEMODEL? > > Not sure which the current devicemodel mode uses, the code doesn't > appear to ask qemu for one or the other explicitly, I'd imagine the > default would be USB2? The qmeu manpage only mentions UHCI.The purpose qemu's manpage / online docs appear mainly to be to distract and mislead unbelievers, making sure that only the truly worthy learn its secrets. A more useful document is hidden inside qemu's source tree: docs/qdev-device-use.txt. It sort of hints at the idea of being able to attach a USB device to a specific bus, but doesn't go into detail about how it might actually be done. But in any case, we now seem to be getting back into "exposing too much of qemu's internals to the user". Suppose you switch from having only a USB2 controller to a USB3 controller -- do you really want to have the user or the toolstack keep track of which is which? If you just tell qemu to plug stuff in, it will manage the topology automatically (apparently including things like adding a USB hub on-the-fly). That is I think the only functionality the vast majority of users will want to have. If we ever get to the point of needing to specify that kind of topology, I think we should just add new flags / features at that time. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On 04/24/2013 01:45 PM, Ian Campbell wrote:> On Fri, 2013-04-19 at 16:59 +0100, George Dunlap wrote: >> + >> +int main_usb_attach(int argc, char **argv) >> +{ >> + uint32_t domid = INVALID_DOMID; >> + int opt = 0, rc; >> + char *device = NULL; >> + >> + SWITCH_FOREACH_OPT(opt, "", NULL, "usb-attach", 2) { >> + /* No options */ >> + } >> + >> + domid = find_domain(argv[optind]); >> + device = argv[optind + 1]; >> + >> + if (domid == INVALID_DOMID) { >> + fprintf(stderr, "Must specify domid\n\n"); >> + help("usb-attach"); >> + return 2; >> + } > > find_domain won''t return in this case, so no need to worry about it > yourself.I find that kind of scary, actually...> >> +int main_usb_detach(int argc, char **argv) >> +{ >> + uint32_t domid = INVALID_DOMID; >> + int opt = 0, rc; >> + char *device = NULL; >> + int type = 0; >> + >> + SWITCH_FOREACH_OPT(opt, "", NULL, "usb-detach", 2) { >> + /* No options */ >> + } >> + >> + domid = find_domain(argv[optind]); >> + device = argv[optind + 1]; >> + >> + if (domid == INVALID_DOMID) { >> + fprintf(stderr, "Must specify domid\n\n"); >> + help("usb-detach"); >> + return 2; >> + } > > > Same again. > >> + >> + rc = usb_detach(domid, type, device); >> + if (rc < 0) >> + return 1; >> + else >> + return 0; >> +} >> + >> +static void usb_list(uint32_t domid) >> +{ >> + libxl_device_usb *dev; >> + int num, i; >> + >> + dev = libxl_device_usb_list(ctx, domid, &num); >> + if (dev == NULL) >> + return; >> + printf("protocol backend type device\n"); >> + for (i = 0; i < num; i++) { >> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); > > You can use libxl_usb_protocol_to_string here.Could do, but I didn''t necessarily want the long version ("devicemodel").> >> + printf("%7d ", dev[i].backend_domid); >> + printf("%7s ", (dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown"); > > libxl_device_usb_type_to_string.Will that print "unknown" in the case of unknown device types?> >> + if (dev[i].type == LIBXL_DEVICE_USB_TYPE_HOSTDEV) >> + printf("%03d.%03d", >> + dev[i].u.hostdev.hostbus, >> + dev[i].u.hostdev.hostaddr); >> + printf("\n"); >> + } >> + free(dev); > > You leak the content of the devices, you need to call > libxl_device_usb_dispose on each, or better define > libxl_device_usb_list_free (this is inconsistently provided by other > devices, but may as well get it right for new code).Ack. -George
Pasi Kärkkäinen
2013-Apr-25 10:17 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On Thu, Apr 25, 2013 at 10:56:14AM +0100, George Dunlap wrote:> On 04/25/2013 08:44 AM, Ian Campbell wrote: > >On Wed, 2013-04-24 at 18:49 +0100, Pasi Kärkkäinen wrote: > >>On Wed, Apr 24, 2013 at 04:51:46PM +0100, Ian Campbell wrote: > >>>On Wed, 2013-04-24 at 16:45 +0100, George Dunlap wrote: > >>>>On 24/04/13 14:51, Ian Campbell wrote: > >>>>>On Wed, 2013-04-24 at 14:32 +0100, George Dunlap wrote: > >>>>> > >>>>>>There''s also the translation of "AUTO" protocol into PV or HVM, and > >>>>>This made me wonder, how is libxl_device_usb_protocol different from the > >>>>>type of the domain? Can you (or is the intention) use PV with an HVM > >>>>>domain? I suppose DEVICEMODEL is HVM only? > >>>> > >>>>The intention is to allow HVM guests to use either DEVICEMODEL or PV as > >>>>protocols. > >>>> > >>>>>Is "protocol" really the right word for this? I''d half expect it to mean > >>>>>USB 1.0 vs 2.0 vs 3.0. For NICS we call this Enum libxl_nic_type. FWIW > >>>> > >>>>But we''re already using ''type'' for the device type. :-) > >>>> > >>>>I think for nics it makes sense to call it a ''type'', as for NICs we > >>>>refer to the *emulated device* as a NIC, or the PV device as a NIC, > >>>>which is then (virtually) plugged into a bridge somewhere. But in this > >>>>case I don''t think it makes sense, as it''s the actual host device you > >>>>care about, and the way the guest talks to it is either via PV or qemu. > >>>>"Protocol" may not be the very best option, but at least it gives you > >>>>the idea of a conduit. > >>> > >>>It''s more like a "method" then? > >>> > >> > >>"protocol" reminded me about something.. when using devicemodel/qemu method, > >>do we currently allow specifying if the usb device should be connected to > >>usb2 (ehci) or usb3 (xhci) virtual controller? > > > >Perhaps EMU_USB2 and EMU_USB3 would be preferable names to DEVICEMODEL? > > > >Not sure which the current devicemodel mode uses, the code doesn''t > >appear to ask qemu for one or the other explicitly, I''d imagine the > >default would be USB2? The qmeu manpage only mentions UHCI. > > The purpose qemu''s manpage / online docs appear mainly to be to > distract and mislead unbelievers, making sure that only the truly > worthy learn its secrets. A more useful document is hidden inside > qemu''s source tree: docs/qdev-device-use.txt. It sort of hints at > the idea of being able to attach a USB device to a specific bus, but > doesn''t go into detail about how it might actually be done. > > But in any case, we now seem to be getting back into "exposing too > much of qemu''s internals to the user". Suppose you switch from > having only a USB2 controller to a USB3 controller -- do you really > want to have the user or the toolstack keep track of which is which? > > If you just tell qemu to plug stuff in, it will manage the topology > automatically (apparently including things like adding a USB hub > on-the-fly). That is I think the only functionality the vast > majority of users will want to have. If we ever get to the point of > needing to specify that kind of topology, I think we should just add > new flags / features at that time. >Yep, if qemu can manage all that on its own, then it''s good! -- Pasi
On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote:> >> + for (i = 0; i < num; i++) { > >> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); > > > > You can use libxl_usb_protocol_to_string here. > > Could do, but I didn''t necessarily want the long version ("devicemodel").TBH the more I think about it the more I think DM/DEVICEMODEL in this interface is leaking an implementation detail, after all the user doesn''t really care who/what is emulating a USB controller. Protocol = {PV,OHCI,XHCI}?> > > >> + printf("%7d ", dev[i].backend_domid); > >> + printf("%7s ", (dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown"); > > > > libxl_device_usb_type_to_string. > > Will that print "unknown" in the case of unknown device types?It''ll return NULL for unknowns (except for any defined unknowns, so I suppose it returns NULL for unknown unknowns, cf [0]), so: libxl_device_usb_type_to_string(type) ? : "<unknown>" Ian. [0] http://en.wikipedia.org/wiki/There_are_known_knowns
On 04/25/2013 12:38 PM, Ian Campbell wrote:> On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: >>>> + for (i = 0; i < num; i++) { >>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); >>> >>> You can use libxl_usb_protocol_to_string here. >> >> Could do, but I didn''t necessarily want the long version ("devicemodel"). > > TBH the more I think about it the more I think DM/DEVICEMODEL in this > interface is leaking an implementation detail, after all the user > doesn''t really care who/what is emulating a USB controller. > > Protocol = {PV,OHCI,XHCI}?As we covered before: 1. I have no way of selecting OHCI vs XHCI at this point 2. Even if I did, why should the caller have to keep track of what kind of USB hardware is exposed to the guest? They should be able to just say "Add" and have stuff sorted out. In addition: 3. The point of saying DEVICEMODEL is that it''s not PV. An HVM guest may be able to do either, and should be allowed to choose. 4. In any case, PROTOCOL_AUTO should be the expected default thing which should be called 99% of the time, unless there''s a reason to choose otherwise. 5. #4 doesn''t undermine #2 as an argument, because the caller should be able to specify "use qemu" without having to remember which hardware is currently exposed to the guest.> >>> >>>> + printf("%7d ", dev[i].backend_domid); >>>> + printf("%7s ", (dev[i].type==LIBXL_DEVICE_USB_TYPE_HOSTDEV)?"hostdev":"unknown"); >>> >>> libxl_device_usb_type_to_string. >> >> Will that print "unknown" in the case of unknown device types? > > It''ll return NULL for unknowns (except for any defined unknowns, so I > suppose it returns NULL for unknown unknowns, cf [0]), so: > libxl_device_usb_type_to_string(type) ? : "<unknown>"Ack. -George
On 04/25/2013 12:57 PM, George Dunlap wrote:> On 04/25/2013 12:38 PM, Ian Campbell wrote: >> On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: >>>>> + for (i = 0; i < num; i++) { >>>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); >>>> >>>> You can use libxl_usb_protocol_to_string here. >>> >>> Could do, but I didn''t necessarily want the long version ("devicemodel"). >> >> TBH the more I think about it the more I think DM/DEVICEMODEL in this >> interface is leaking an implementation detail, after all the user >> doesn''t really care who/what is emulating a USB controller. >> >> Protocol = {PV,OHCI,XHCI}? > > As we covered before: > > 1. I have no way of selecting OHCI vs XHCI at this point > > 2. Even if I did, why should the caller have to keep track of what kind > of USB hardware is exposed to the guest? They should be able to just > say "Add" and have stuff sorted out. > > In addition: > > 3. The point of saying DEVICEMODEL is that it''s not PV. An HVM guest > may be able to do either, and should be allowed to choose.If your problem is with the name, we could call it EMULATED instead. -George
On Thu, 2013-04-25 at 12:57 +0100, George Dunlap wrote:> On 04/25/2013 12:38 PM, Ian Campbell wrote: > > On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: > >>>> + for (i = 0; i < num; i++) { > >>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); > >>> > >>> You can use libxl_usb_protocol_to_string here. > >> > >> Could do, but I didn''t necessarily want the long version ("devicemodel"). > > > > TBH the more I think about it the more I think DM/DEVICEMODEL in this > > interface is leaking an implementation detail, after all the user > > doesn''t really care who/what is emulating a USB controller. > > > > Protocol = {PV,OHCI,XHCI}? > > As we covered before:> 1. I have no way of selecting OHCI vs XHCI at this pointOK, so maybe HCI (as the generic term for a USB host controller) or EMU or something is more appropriate. I''m not too fussed about the distinction between {O,U,X}HCI right now.> 2. Even if I did, why should the caller have to keep track of what kind > of USB hardware is exposed to the guest?Perhaps because they have to make sure they have the appropriate drivers in the guest? In particular if qemu decides to use xhci will that work seemlessly with Windows XP? I think XHCI is supposed to be backwards compatible with OHCI but I''m not really sure how that works in practice.> They should be able to just say "Add" and have stuff sorted out.Sure.> 3. The point of saying DEVICEMODEL is that it''s not PV. An HVM guest > may be able to do either, and should be allowed to choose.But not PV does not imply device model, except by inspecting/understanding implementation details. What you are really asking for is an emulated USB host controller and you don''t care where it comes from.> 4. In any case, PROTOCOL_AUTO should be the expected default thing which > should be called 99% of the time, unless there''s a reason to choose > otherwise. > > 5. #4 doesn''t undermine #2 as an argument, because the caller should be > able to specify "use qemu" without having to remember which hardware is > currently exposed to the guest.My point is that the user doesn''t want to specify "use qemu" they want to specify "use an emulated USB host controller". How does spice USB redirection fit into this scheme? Remember that SPICE is also, as an implementation detail, implemented by QEMU. Ian.
On 04/25/2013 01:08 PM, Ian Campbell wrote:> On Thu, 2013-04-25 at 12:57 +0100, George Dunlap wrote: >> On 04/25/2013 12:38 PM, Ian Campbell wrote: >>> On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: >>>>>> + for (i = 0; i < num; i++) { >>>>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); >>>>> >>>>> You can use libxl_usb_protocol_to_string here. >>>> >>>> Could do, but I didn''t necessarily want the long version ("devicemodel"). >>> >>> TBH the more I think about it the more I think DM/DEVICEMODEL in this >>> interface is leaking an implementation detail, after all the user >>> doesn''t really care who/what is emulating a USB controller. >>> >>> Protocol = {PV,OHCI,XHCI}? >> >> As we covered before: > >> 1. I have no way of selecting OHCI vs XHCI at this point > > OK, so maybe HCI (as the generic term for a USB host controller) or EMU > or something is more appropriate. I''m not too fussed about the > distinction between {O,U,X}HCI right now. > >> 2. Even if I did, why should the caller have to keep track of what kind >> of USB hardware is exposed to the guest? > > Perhaps because they have to make sure they have the appropriate drivers > in the guest? > > In particular if qemu decides to use xhci will that work seemlessly with > Windows XP? I think XHCI is supposed to be backwards compatible with > OHCI but I''m not really sure how that works in practice. > >> They should be able to just say "Add" and have stuff sorted out. > > Sure. > >> 3. The point of saying DEVICEMODEL is that it''s not PV. An HVM guest >> may be able to do either, and should be allowed to choose. > > But not PV does not imply device model, except by > inspecting/understanding implementation details. What you are really > asking for is an emulated USB host controller and you don''t care where > it comes from. > >> 4. In any case, PROTOCOL_AUTO should be the expected default thing which >> should be called 99% of the time, unless there''s a reason to choose >> otherwise. >> >> 5. #4 doesn''t undermine #2 as an argument, because the caller should be >> able to specify "use qemu" without having to remember which hardware is >> currently exposed to the guest. > > My point is that the user doesn''t want to specify "use qemu" they want > to specify "use an emulated USB host controller".Right -- so your only problem is that you don''t want users to have to know "emulated" <=> "device model". That''s fine.> > How does spice USB redirection fit into this scheme? Remember that SPICE > is also, as an implementation detail, implemented by QEMU.I don''t know much about spice, but wouldn''t that be the job of the spice client to handle that? It would be kind of scary if the admin of a machine could tell a remote spice client to expose arbitrary devices on that host to the vm. :-) -George
On 04/25/2013 01:14 PM, George Dunlap wrote:> On 04/25/2013 01:08 PM, Ian Campbell wrote: >> On Thu, 2013-04-25 at 12:57 +0100, George Dunlap wrote: >>> On 04/25/2013 12:38 PM, Ian Campbell wrote: >>>> On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: >>>>>>> + for (i = 0; i < num; i++) { >>>>>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); >>>>>> >>>>>> You can use libxl_usb_protocol_to_string here. >>>>> >>>>> Could do, but I didn''t necessarily want the long version ("devicemodel"). >>>> >>>> TBH the more I think about it the more I think DM/DEVICEMODEL in this >>>> interface is leaking an implementation detail, after all the user >>>> doesn''t really care who/what is emulating a USB controller. >>>> >>>> Protocol = {PV,OHCI,XHCI}? >>> >>> As we covered before: >> >>> 1. I have no way of selecting OHCI vs XHCI at this point >> >> OK, so maybe HCI (as the generic term for a USB host controller) or EMU >> or something is more appropriate. I''m not too fussed about the >> distinction between {O,U,X}HCI right now. >> >>> 2. Even if I did, why should the caller have to keep track of what kind >>> of USB hardware is exposed to the guest? >> >> Perhaps because they have to make sure they have the appropriate drivers >> in the guest? >> >> In particular if qemu decides to use xhci will that work seemlessly with >> Windows XP? I think XHCI is supposed to be backwards compatible with >> OHCI but I''m not really sure how that works in practice. >> >>> They should be able to just say "Add" and have stuff sorted out. >> >> Sure. >> >>> 3. The point of saying DEVICEMODEL is that it''s not PV. An HVM guest >>> may be able to do either, and should be allowed to choose. >> >> But not PV does not imply device model, except by >> inspecting/understanding implementation details. What you are really >> asking for is an emulated USB host controller and you don''t care where >> it comes from. >> >>> 4. In any case, PROTOCOL_AUTO should be the expected default thing which >>> should be called 99% of the time, unless there''s a reason to choose >>> otherwise. >>> >>> 5. #4 doesn''t undermine #2 as an argument, because the caller should be >>> able to specify "use qemu" without having to remember which hardware is >>> currently exposed to the guest. >> >> My point is that the user doesn''t want to specify "use qemu" they want >> to specify "use an emulated USB host controller". > > Right -- so your only problem is that you don''t want users to have to > know "emulated" <=> "device model". That''s fine.Hmm... So, I think I''m remembering why I chose "device model" in the first place. The problem is that the *device itself* is not emulated -- it''s a real device which is exposed via the device model. We''re emulating the instructions and the usb hardware used to access the device, but not the device itself. The device model is a conduit, which is why "protocol" seemed like the closest fit in terms of a word. -George
On Thu, 2013-04-25 at 13:21 +0100, George Dunlap wrote:> On 04/25/2013 01:14 PM, George Dunlap wrote: > > On 04/25/2013 01:08 PM, Ian Campbell wrote: > >> On Thu, 2013-04-25 at 12:57 +0100, George Dunlap wrote: > >>> On 04/25/2013 12:38 PM, Ian Campbell wrote: > >>>> On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: > >>>>>>> + for (i = 0; i < num; i++) { > >>>>>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); > >>>>>> > >>>>>> You can use libxl_usb_protocol_to_string here. > >>>>> > >>>>> Could do, but I didn''t necessarily want the long version ("devicemodel"). > >>>> > >>>> TBH the more I think about it the more I think DM/DEVICEMODEL in this > >>>> interface is leaking an implementation detail, after all the user > >>>> doesn''t really care who/what is emulating a USB controller. > >>>> > >>>> Protocol = {PV,OHCI,XHCI}? > >>> > >>> As we covered before: > >> > >>> 1. I have no way of selecting OHCI vs XHCI at this point > >> > >> OK, so maybe HCI (as the generic term for a USB host controller) or EMU > >> or something is more appropriate. I''m not too fussed about the > >> distinction between {O,U,X}HCI right now. > >> > >>> 2. Even if I did, why should the caller have to keep track of what kind > >>> of USB hardware is exposed to the guest? > >> > >> Perhaps because they have to make sure they have the appropriate drivers > >> in the guest? > >> > >> In particular if qemu decides to use xhci will that work seemlessly with > >> Windows XP? I think XHCI is supposed to be backwards compatible with > >> OHCI but I''m not really sure how that works in practice. > >> > >>> They should be able to just say "Add" and have stuff sorted out. > >> > >> Sure. > >> > >>> 3. The point of saying DEVICEMODEL is that it''s not PV. An HVM guest > >>> may be able to do either, and should be allowed to choose. > >> > >> But not PV does not imply device model, except by > >> inspecting/understanding implementation details. What you are really > >> asking for is an emulated USB host controller and you don''t care where > >> it comes from. > >> > >>> 4. In any case, PROTOCOL_AUTO should be the expected default thing which > >>> should be called 99% of the time, unless there''s a reason to choose > >>> otherwise. > >>> > >>> 5. #4 doesn''t undermine #2 as an argument, because the caller should be > >>> able to specify "use qemu" without having to remember which hardware is > >>> currently exposed to the guest. > >> > >> My point is that the user doesn''t want to specify "use qemu" they want > >> to specify "use an emulated USB host controller". > > > > Right -- so your only problem is that you don''t want users to have to > > know "emulated" <=> "device model". That''s fine. > > Hmm... > > So, I think I''m remembering why I chose "device model" in the first > place. The problem is that the *device itself* is not emulated -- it''s > a real device which is exposed via the device model. We''re emulating > the instructions and the usb hardware used to access the device, but not > the device itself. The device model is a conduit, which is why > "protocol" seemed like the closest fit in terms of a word.I was thinking of this in terms of the bus/"protocol" being emulated, but not in terms of how the actual device was provided. The provision of the type of device is the "kind" field I think? Ian.
On Thu, 2013-04-25 at 13:14 +0100, George Dunlap wrote:> > How does spice USB redirection fit into this scheme? Remember that SPICE > > is also, as an implementation detail, implemented by QEMU. > > I don''t know much about spice, but wouldn''t that be the job of the spice > client to handle that? > > It would be kind of scary if the admin of a machine could tell a remote > spice client to expose arbitrary devices on that host to the vm. :-)Oh right yes. I had thought they did host<->guest USB redirection too.
Pasi Kärkkäinen
2013-Apr-25 13:42 UTC
Re: [PATCH v6 2/2] xl: Add commands for usb hot-plug
On Thu, Apr 25, 2013 at 12:57:50PM +0100, George Dunlap wrote:> On 04/25/2013 12:38 PM, Ian Campbell wrote: > >On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: > >>>>+ for (i = 0; i < num; i++) { > >>>>+ printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); > >>> > >>>You can use libxl_usb_protocol_to_string here. > >> > >>Could do, but I didn''t necessarily want the long version ("devicemodel"). > > > >TBH the more I think about it the more I think DM/DEVICEMODEL in this > >interface is leaking an implementation detail, after all the user > >doesn''t really care who/what is emulating a USB controller. > > > >Protocol = {PV,OHCI,XHCI}? > > As we covered before: > > 1. I have no way of selecting OHCI vs XHCI at this point >https://github.com/qemu/qemu/blob/master/docs/usb2.txt qemu -device usb-ehci,id=ehci \ -device usb-tablet,bus=usb-bus.0 \ -device usb-storage,bus=ehci.0,drive=usbstick "This attaches a usb tablet to the UHCI adapter and a usb mass storage device to the EHCI adapter." http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04013.html qemu $args -device nec-usb-xhci,id=xhci -device usb-tablet,bus=xhci.0 http://www.linux-kvm.com/content/qemu-kvm-11-adds-experimental-support-usb-30 qemu-kvm -device nec-usb-xhci,id=xhci -device usb-storage,bus=xhci.0,drive=usbstick So it seems it''s possible to create ehci/usb2.0 and xhci/usb3.0 controllers as needed, and attach devices to both of them by specifying the virtual bus..> 2. Even if I did, why should the caller have to keep track of what > kind of USB hardware is exposed to the guest? They should be able > to just say "Add" and have stuff sorted out. >Compatibility might be one reason why users want to specify specific virtual controller.. ie. some usb 2.0 devices don''t work properly in usb 3.0 controllers, unfortunately.. -- Pasi
On 04/25/2013 02:42 PM, Pasi Kärkkäinen wrote:> On Thu, Apr 25, 2013 at 12:57:50PM +0100, George Dunlap wrote: >> On 04/25/2013 12:38 PM, Ian Campbell wrote: >>> On Thu, 2013-04-25 at 11:16 +0100, George Dunlap wrote: >>>>>> + for (i = 0; i < num; i++) { >>>>>> + printf("%8s ", (dev[i].protocol==LIBXL_USB_PROTOCOL_PV)?"pv":"dm"); >>>>> >>>>> You can use libxl_usb_protocol_to_string here. >>>> >>>> Could do, but I didn''t necessarily want the long version ("devicemodel"). >>> >>> TBH the more I think about it the more I think DM/DEVICEMODEL in this >>> interface is leaking an implementation detail, after all the user >>> doesn''t really care who/what is emulating a USB controller. >>> >>> Protocol = {PV,OHCI,XHCI}? >> >> As we covered before: >> >> 1. I have no way of selecting OHCI vs XHCI at this point >> > > https://github.com/qemu/qemu/blob/master/docs/usb2.txt > > qemu -device usb-ehci,id=ehci \ > -device usb-tablet,bus=usb-bus.0 \ > -device usb-storage,bus=ehci.0,drive=usbstick > > "This attaches a usb tablet to the UHCI adapter and a usb mass storage > device to the EHCI adapter." > > > http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04013.html > qemu $args -device nec-usb-xhci,id=xhci -device usb-tablet,bus=xhci.0 > > http://www.linux-kvm.com/content/qemu-kvm-11-adds-experimental-support-usb-30 > qemu-kvm -device nec-usb-xhci,id=xhci -device usb-storage,bus=xhci.0,drive=usbstick > > > So it seems it''s possible to create ehci/usb2.0 and xhci/usb3.0 controllers as needed, > and attach devices to both of them by specifying the virtual bus.. > > > >> 2. Even if I did, why should the caller have to keep track of what >> kind of USB hardware is exposed to the guest? They should be able >> to just say "Add" and have stuff sorted out. >> > > Compatibility might be one reason why users want to specify specific virtual controller.. > ie. some usb 2.0 devices don''t work properly in usb 3.0 controllers, unfortunately..I''ll take a look at implementing some of this when I come back to this series after the 4.3 release. But it seems like having a "controller type" option in the device field might be useful. We should also look and see what libvirt exposes, at least for KVM, to get an idea what kinds of things might be useful to implement. -George
Anthony PERARD
2013-Apr-25 14:19 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 19/04/13 16:59, George Dunlap wrote:> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c > index 644d2c0..9ad3e59 100644 > --- a/tools/libxl/libxl_qmp.c > +++ b/tools/libxl/libxl_qmp.c > @@ -42,6 +42,7 @@ > > #define QMP_RECEIVE_BUFFER_SIZE 4096 > #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" > +#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x" > > typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, > const libxl__json_object *tree, > @@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, > } > } > > +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, > + libxl__device_usb *dev) > +{ > + libxl__json_object *args = NULL; > + char *id; > + > + id = GCSPRINTF(HOST_USB_QDEV_ID, > + (uint16_t)dev->u.hostdev.hostbus, > + (uint16_t)dev->u.hostdev.hostaddr); > + > + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); > + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x",dev->u.hostdev.hostbus);> + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x",dev->u.hostdev.hostaddr);> + > + qmp_parameters_add_string(gc, &args, "id", id);You can use QMP_PARAMETERS_SPRINTF here instead of having a separate GCSPRINTF call. The former do the same call to sprintf as the later.> + > + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); > +} > + > +int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb*usbdev)> +{ > + int rc; > + switch (usbdev->type) { > + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: > + rc = libxl__qmp_usb_hostdev_add(gc, domid, usbdev); > + break; > + default: > + return ERROR_INVAL; > + } > + return rc; > +} > + > + > +static int libxl__qmp_usb_hostdev_remove(libxl__gc *gc, int domid, > + libxl__device_usb *dev) > +{ > + libxl__json_object *args = NULL; > + char *id; > + > + id = GCSPRINTF(HOST_USB_QDEV_ID, > + (uint16_t)dev->u.hostdev.hostbus, > + (uint16_t)dev->u.hostdev.hostaddr); > + > + qmp_parameters_add_string(gc, &args, "id", id); > + > + return qmp_run_command(gc, domid, "device_del", args, NULL, NULL);There is already a "device_del" which can be called: static int qmp_device_del(libxl__gc *gc, int domid, char *id); -- Anthony PERARD
George Dunlap
2013-Apr-25 14:31 UTC
Re: [PATCH v6 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest
On 04/25/2013 03:19 PM, Anthony PERARD wrote:> On 19/04/13 16:59, George Dunlap wrote: >> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c >> index 644d2c0..9ad3e59 100644 >> --- a/tools/libxl/libxl_qmp.c >> +++ b/tools/libxl/libxl_qmp.c >> @@ -42,6 +42,7 @@ >> >> #define QMP_RECEIVE_BUFFER_SIZE 4096 >> #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" >> +#define HOST_USB_QDEV_ID "usb-hostdev-%04x.%04x" >> >> typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp, >> const libxl__json_object *tree, >> @@ -929,6 +930,70 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, >> } >> } >> >> +static int libxl__qmp_usb_hostdev_add(libxl__gc *gc, int domid, >> + libxl__device_usb *dev) >> +{ >> + libxl__json_object *args = NULL; >> + char *id; >> + >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t)dev->u.hostdev.hostbus, >> + (uint16_t)dev->u.hostdev.hostaddr); >> + >> + qmp_parameters_add_string(gc, &args, "driver", "usb-host"); >> + QMP_PARAMETERS_SPRINTF(&args, "hostbus", "0x%x", > dev->u.hostdev.hostbus); >> + QMP_PARAMETERS_SPRINTF(&args, "hostaddr", "0x%x", > dev->u.hostdev.hostaddr); >> + >> + qmp_parameters_add_string(gc, &args, "id", id); > > You can use QMP_PARAMETERS_SPRINTF here instead of having a separate > GCSPRINTF call. The former do the same call to sprintf as the later.Ah, right -- another vestige of when I was passing the ''id'' string back inside the device.> >> + >> + return qmp_run_command(gc, domid, "device_add", args, NULL, NULL); >> +} >> + >> +int libxl__qmp_usb_add(libxl__gc *gc, int domid, libxl__device_usb > *usbdev) >> +{ >> + int rc; >> + switch (usbdev->type) { >> + case LIBXL_DEVICE_USB_TYPE_HOSTDEV: >> + rc = libxl__qmp_usb_hostdev_add(gc, domid, usbdev); >> + break; >> + default: >> + return ERROR_INVAL; >> + } >> + return rc; >> +} >> + >> + >> +static int libxl__qmp_usb_hostdev_remove(libxl__gc *gc, int domid, >> + libxl__device_usb *dev) >> +{ >> + libxl__json_object *args = NULL; >> + char *id; >> + >> + id = GCSPRINTF(HOST_USB_QDEV_ID, >> + (uint16_t)dev->u.hostdev.hostbus, >> + (uint16_t)dev->u.hostdev.hostaddr); >> + >> + qmp_parameters_add_string(gc, &args, "id", id); >> + >> + return qmp_run_command(gc, domid, "device_del", args, NULL, NULL); > > There is already a "device_del" which can be called: > static int qmp_device_del(libxl__gc *gc, int domid, char *id); >Ack. -George