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

This uses the qmp functionality, and is thus only available for qemu-xen,
not qemu-traditional.

Devices must be removed by "id", an identifying string, which must be
specified when the device is created.  The caller can either pass one
in to request; if none is passed in, then libxl will choose one and pass
it back to the caller.

qemu will reject duplicate ids.  There is a small possibility that the
libxl-chosen id my collide with a previously created one, in which
case the add will fail.  It would be nice if the library could
automatically modify it until it found a unique one, but at the moment
qmp_run_command() doesn''t return which error the command failed by, so
the caller can''t tell that the command failed because of a duplicate
id.

Since it''s additional work, and it''s not clear that the
situation can
actually happen in practice, I''m considering it a "maybe do
later".

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 tools/libxl/libxl.c          |   85 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl.h          |   32 ++++++++++++++++
 tools/libxl/libxl_internal.h |    3 ++
 tools/libxl/libxl_qmp.c      |   45 ++++++++++++++++++++++
 tools/libxl/libxl_types.idl  |    8 ++++
 5 files changed, 173 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 572c2c6..34b648e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2498,6 +2498,91 @@ out:
     return AO_INPROGRESS;
 }
 
+int libxl_hvm_host_usb_add(libxl_ctx *ctx, uint32_t domid,
+                           libxl_device_host_usb *dev)
+{
+    GC_INIT(ctx);
+    int rc, dm_ver;
+
+    libxl_domain_type type = libxl__domain_type(gc, domid);
+    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    if (type != LIBXL_DOMAIN_TYPE_HVM) {
+        LOG(ERROR, "hvm-host-usb-add requires an HVM domain");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (libxl_get_stubdom_id(ctx, domid) != 0) {
+        LOG(ERROR, "hvm-host-usb-add doesn''t work for stub
domains");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    dm_ver = libxl__device_model_version_running(gc, domid);
+    if (dm_ver == -1) {
+        LOG(ERROR, "cannot determine device model version");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        rc = libxl__qmp_host_usb_add(gc, domid, dev);
+    } else {
+        LOG(ERROR, "hvm-host-usb-add not yet implemented for
qemu-traditional");
+        rc = ERROR_FAIL;
+    }
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+int libxl_hvm_host_usb_del(libxl_ctx *ctx, uint32_t domid,
+                           const char * id)
+{
+    GC_INIT(ctx);
+    int rc, dm_ver;
+
+    libxl_domain_type type = libxl__domain_type(gc, domid);
+    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+    if (type != LIBXL_DOMAIN_TYPE_HVM) {
+        LOG(ERROR, "hvm-host-usb-del requires an HVM domain");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    if (libxl_get_stubdom_id(ctx, domid) != 0) {
+        LOG(ERROR, "hvm-host-usb-del doesn''t work for stub
domains");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    dm_ver = libxl__device_model_version_running(gc, domid);
+    if (dm_ver == -1) {
+        LOG(ERROR, "cannot determine device model version");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        rc = libxl__qmp_host_usb_del(gc, domid, id);
+    } else {
+        LOG(ERROR, "hvm-host-usb-del not yet implemented for
qemu-traditional");
+        rc = ERROR_FAIL;
+    }
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+
 /* libxl__alloc_vdev only works on the local domain, that is the domain
  * where the toolstack is running */
 static char * libxl__alloc_vdev(libxl__gc *gc, void *get_vdev_user,
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 030aa86..e6ae8e5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -75,6 +75,15 @@
 #define LIBXL_HAVE_FIRMWARE_PASSTHROUGH 1
 
 /*
+ * LIBXL_HAVE_HVM_HOST_USB indicates that the structure libxl_device_host_usb
+ * exists, as well as the following two functions, relating to passing through
+ * host USB devices to HVM guests:
+ * - libxl_hvm_host_usb_add
+ * - libxl_hvm_host_usb_del.
+ */
+#define LIBXL_HAVE_HVM_HOST_USB 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
@@ -719,6 +728,29 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid,
libxl_device_disk *disk,
                        const libxl_asyncop_how *ao_how)
                        LIBXL_EXTERNAL_CALLERS_ONLY;
 
+/*
+ * HVM Host USB
+ * 
+ * Add and remove specified host USB device to an HVM guest.
+ *
+ * libxl_device_host_usb contains four potential specifiers which are
initialized
+ * to LIBXL_DEVICE_HOST_USB_ANY.  According to the qemu documentation, any
+ * parameter that is left empty will match anything.  It is recommended to
+ * at least specify hostbus.hostid, or vendorid:productid.
+ *
+ * Devices must be removed by id, a string provided when the device is added.
+ * The caller can specify the ''id'' field of
libxl_device_host_usb.  If this is
+ * left NULL, then libxl will choose one and return the value in the id field.
+ * Id''s must be unique to the device within a VM; attempting to assign
a device
+ * with an existing id will return an error.
+ */
+#define LIBXL_DEVICE_HOST_USB_ANY (-1)
+int libxl_hvm_host_usb_add(libxl_ctx *ctx, uint32_t domid,
+                           libxl_device_host_usb *dev)
+                           LIBXL_EXTERNAL_CALLERS_ONLY;
+int libxl_hvm_host_usb_del(libxl_ctx *ctx, uint32_t domid, const char * id)
+                           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_internal.h b/tools/libxl/libxl_internal.h
index 8be086d..2fbd193 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1411,6 +1411,9 @@ _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);
+_hidden int libxl__qmp_host_usb_add(libxl__gc *gc, int domid, 
+                                    libxl_device_host_usb *dev);
+_hidden int libxl__qmp_host_usb_del(libxl__gc *gc, int domid,  const char *id);
 /* 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..2e92950 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 "host-usb-%04x.%04x-%04x.%04x"
 
 typedef int (*qmp_callback_t)(libxl__qmp_handler *qmp,
                               const libxl__json_object *tree,
@@ -929,6 +930,50 @@ int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid,
     }
 }
 
+int libxl__qmp_host_usb_add(libxl__gc *gc, int domid,
+                            libxl_device_host_usb *dev)
+{
+    libxl__json_object *args = NULL;
+
+    if (dev->id == NULL )
+    {
+        dev->id = libxl__sprintf(NOGC, HOST_USB_QDEV_ID,
+                                 (uint16_t) dev->hostbus,
+                                 (uint16_t) dev->hostaddr,
+                                 (uint16_t) dev->vendorid,
+                                 (uint16_t) dev->productid);
+    }
+
+    qmp_parameters_add_string(gc, &args, "driver",
"usb-host");
+    if(dev->hostbus != LIBXL_DEVICE_HOST_USB_ANY) {
+        QMP_PARAMETERS_SPRINTF(&args, "hostbus",
"0x%x", dev->hostbus);
+    }
+    if(dev->hostaddr != LIBXL_DEVICE_HOST_USB_ANY) {
+        QMP_PARAMETERS_SPRINTF(&args, "hostaddr",
"0x%x", dev->hostaddr);
+    }
+    if(dev->vendorid != LIBXL_DEVICE_HOST_USB_ANY) {
+        QMP_PARAMETERS_SPRINTF(&args, "vendorid",
"0x%x", dev->vendorid);
+    }
+    if(dev->productid != LIBXL_DEVICE_HOST_USB_ANY) {
+        QMP_PARAMETERS_SPRINTF(&args, "productid",
"0x%x", dev->productid);
+    }
+
+    qmp_parameters_add_string(gc, &args, "id", dev->id);
+
+    return qmp_run_command(gc, domid, "device_add", args, NULL,
NULL);
+}
+
+int libxl__qmp_host_usb_del(libxl__gc *gc, int domid, 
+                            const char *id)
+{
+    libxl__json_object *args = NULL;
+
+    qmp_parameters_add_string(gc, &args, "id", id);
+
+    return qmp_run_command(gc, domid, "device_del", args, NULL,
NULL);
+}
+
+
 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 f3c212b..45ee531 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -406,6 +406,14 @@ libxl_device_vtpm = Struct("device_vtpm", [
     ("uuid",             libxl_uuid),
 ])
 
+libxl_device_host_usb = Struct("device_host_usb", [
+    ("hostbus",   integer, {''init_val'':
''LIBXL_DEVICE_HOST_USB_ANY''}),
+    ("hostaddr",  integer, {''init_val'':
''LIBXL_DEVICE_HOST_USB_ANY''}),
+    ("vendorid",  integer, {''init_val'':
''LIBXL_DEVICE_HOST_USB_ANY''}),
+    ("productid", integer, {''init_val'':
''LIBXL_DEVICE_HOST_USB_ANY''}),
+    ("id",         string),
+    ])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
-- 
1.7.9.5

Add commands to add and remove host USB to HVM guests.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 docs/man/xl.pod.1         |   45 ++++++++++
 tools/libxl/xl.h          |    2 +
 tools/libxl/xl_cmdimpl.c  |  208 +++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/xl_cmdtable.c |   10 +++
 4 files changed, 265 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index a0e298e..9c582e9 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1110,6 +1110,51 @@ List virtual network interfaces for a domain.
 
 =back
 
+=head2 HVM DEVICES
+
+=over 4
+
+=item B<hvm-host-usb-add> [I<-i devname>] I<domain-id>
I<device-specifier>
+
+Passes through the host USB device specified by I<device-specifier> to
the
+HVM domain I<domain-id>.  Device-specifier can be one of the following:
+
+=over 4
+
+=item <hostbus>.<hostaddr>
+
+=item <vendorid>:<productid>
+
+=item <hostbus>.<hostaddr>:<vendorid>:<productid>
+
+=back
+
+The best way to find out the information for the device is typically using
+lsusb.
+
+Using the I<-i> option, you can specify a I<devname> for the
+device. This is an arbitrary string that can be used by
+I<hvm-host-usb-del> to remove the device later.  If no name is
+specified, then one will be chosen automatically.  In any case the
+devname will be printed to stdout if the device is successfully added.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=item B<hvm-host-usb-del> I<domain-id> I<devname>
+
+Remove the host USB device from I<domain-id> which is specified by
I<devname>.
+This I<devname> is a string that was specified when the device was
inserted
+by B<hvm-host-usb-add>.
+
+Devices specified in the config file do not have an associated
+devname, and thus cannot be removed using this command.
+
+This command is only available for domains using qemu-xen, not
+qemu-traditional.
+
+=back
+
 =head2 VTPM DEVICES
 
 =over 4
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index b881f92..8eb368c 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -35,6 +35,8 @@ 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_hvm_host_usb_add(int argc, char **argv);
+int main_hvm_host_usb_del(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);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2d40f8f..ca2f023 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2584,6 +2584,214 @@ int main_cd_insert(int argc, char **argv)
     return 0;
 }
 
+
+
+static int parse_usb_specifier_hbhavipi(libxl_device_host_usb *dev, const char
*s)
+{
+    const char * vid, *pid, *p;
+    const char * hostbus, *hostaddr;
+
+    hostbus = s;
+    hostaddr = vid = pid = NULL;
+
+#define is_dec(_c) ((_c) >= ''0'' && (_c) <=
''9'')
+#define is_hex(_c) (is_dec(_c) || ((_c) >= ''a'' &&
(_c) <= ''f''))
+
+    /* Match [0-9]+\.[0-9]:[0-9a-f]+:[0-9a-f] */
+
+    /* First look for [0-9]+\.[0-9]:*/
+    if ( !is_dec(*hostbus) )
+        return -1;
+
+    for(p=s; *p; p++) {
+        if(*p == ''.'') {
+            if ( !hostaddr )
+                hostaddr = p+1;
+            else {
+                return -1;
+            }
+        } else if (*p == '':'') {
+            break;
+        } else if (!is_dec(*p)) {
+            return -1;
+        }
+    }
+    if ( !hostaddr || !is_dec(*hostaddr) )
+        return -1;
+    if ( *p != '':'' )
+        return -1;
+
+    p++;
+    /* Now match [0-9a-f]+:[0-9a-f] */
+    vid = p;
+    if ( !is_hex(*vid) )
+        return -1;
+
+    for(; *p; p++) {
+        if(*p == '':'') {
+            if ( !pid )
+                pid = p+1;
+            else
+                return -1;
+        } else if (!is_hex(*p)) {
+            return -1;
+        }
+    }
+    if (!pid || !is_hex(*pid))
+        return -1;
+
+    dev->hostbus  = strtoul(hostbus,  NULL, 10);
+    dev->hostaddr = strtoul(hostaddr, NULL, 10);
+    dev->vendorid  = strtoul(vid, NULL, 16);
+    dev->productid = strtoul(pid, NULL, 16);
+
+    return 0;
+}
+
+static int parse_usb_specifier_vipi(libxl_device_host_usb *dev, const char *s)
+{
+    const char * vid, *pid, *p;
+
+    vid = s;
+    pid = NULL;
+
+    /* Match [0-9a-f]+:[0-9a-f] */
+    if ( !is_hex(*vid) )
+        return -1;
+
+    for(p=s; *p; p++) {
+        if(*p == '':'') {
+            if ( !pid )
+                pid = p+1;
+            else
+                return -1;
+        } else if (!is_hex(*p)) {
+            return -1;
+        }
+    }
+    if (!pid || !is_hex(*pid))
+        return -1;
+    dev->vendorid  = strtoul(vid, NULL, 16);
+    dev->productid = strtoul(pid, NULL, 16);
+
+    return 0;
+}
+
+static int parse_usb_specifier_hbha(libxl_device_host_usb *dev, const char *s)
+{
+    const char * hostbus, *hostaddr, *p;
+
+    hostbus = s;
+    hostaddr=NULL;
+
+    /* Match [0-9]+\.[0-9] */
+    if (!is_dec(*hostbus))
+        return -1;
+
+    for(p=s; *p; p++) {
+        if(*p == ''.'') {
+            if ( !hostaddr )
+                hostaddr = p+1;
+            else {
+                return -1;
+            }
+        } else if (!is_dec(*p)) {
+            return -1;
+        }
+    }
+    if (!hostaddr || !is_dec(*hostaddr))
+        return -1;
+    dev->hostbus  = strtoul(hostbus,  NULL, 10);
+    dev->hostaddr = strtoul(hostaddr, NULL, 10);
+
+    return 0;
+}
+
+static int parse_usb_specifier(libxl_device_host_usb *dev, const char *s)
+{
+    /*
+     * Acceptable formats:
+     * - hostbus.hostaddr
+     * - vendorid:productid
+     * - hostbus.hostaddr:vendorid:productid
+     */
+    if ( !parse_usb_specifier_hbha(dev, s) )
+        return 0;
+    if ( !parse_usb_specifier_vipi(dev, s) )
+        return 0;
+    if ( !parse_usb_specifier_hbhavipi(dev, s) )
+        return 0;
+
+    return -1;
+}
+
+#undef is_dec
+#undef is_hex
+
+static int hvm_host_usb_add(uint32_t domid, const char * device, char * id)
+{
+    libxl_device_host_usb usbdev;
+    int rc;
+
+    libxl_device_host_usb_init(&usbdev);
+
+    if ( id )
+        usbdev.id = strdup(id);
+
+    if ( parse_usb_specifier(&usbdev, device) < 0 )
+        return -1;
+
+    if ( (rc = libxl_hvm_host_usb_add(ctx, domid, &usbdev)) >= 0 )
+        printf("Added device with name %s\n", usbdev.id);
+
+    libxl_device_host_usb_dispose(&usbdev);
+
+    return rc;
+}
+
+int main_hvm_host_usb_add(int argc, char **argv)
+{
+    uint32_t domid;
+    int opt = 0, rc;
+    const char *device;
+    char *id = NULL;
+
+    SWITCH_FOREACH_OPT(opt, "i:", NULL, "hvm-host-usb-add",
2) {
+    case ''i'':
+        id = optarg;
+        break;
+    }
+
+    domid = find_domain(argv[optind]);
+    device = argv[optind + 1];
+
+    rc = hvm_host_usb_add(domid, device, id);
+    if ( rc < 0 )
+        return -1;
+    else
+        return 0;
+}
+
+int main_hvm_host_usb_del(int argc, char **argv)
+{
+    uint32_t domid;
+    char * id;
+    int opt = 0, rc;
+
+    SWITCH_FOREACH_OPT(opt, "", NULL, "hvm-host-usb-del",
2) {
+        /* No options */
+    }
+
+    domid = find_domain(argv[optind]);
+    id = argv[optind + 1];
+
+    rc = libxl_hvm_host_usb_del(ctx, domid, id);
+    if ( rc < 0 )
+        return -1;
+    else
+        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 b4a87ca..0c57364 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -187,6 +187,16 @@ struct cmd_spec cmd_table[] = {
       "Eject a cdrom from a guest''s cd drive",
       "<Domain> <VirtualDevice>",
     },
+    { "hvm-host-usb-add",
+      &main_hvm_host_usb_add, 1, 1,
+      "Hot-plug a host usb device to an HVM domain.",
+      "<Domain> [-i devname]
(<hostbus>.<hostaddr>|<vendorid>:<productid>)",
+    },
+    { "hvm-host-usb-del",
+      &main_hvm_host_usb_del, 1, 1,
+      "Hot-unplug the named device from an HVM domain.",
+      "<Domain> <devname>",
+    },
     { "mem-max",
       &main_memmax, 0, 1,
       "Set the maximum amount reservation for a domain",
-- 
1.7.9.5

On 19/03/13 13:09, George Dunlap wrote:
> This uses the qmp functionality, and is thus only available for qemu-xen,
> not qemu-traditional.
> 
> Devices must be removed by "id", an identifying string, which
must be
> specified when the device is created.  The caller can either pass one
> in to request; if none is passed in, then libxl will choose one and pass
> it back to the caller.
> 
> qemu will reject duplicate ids.  There is a small possibility that the
> libxl-chosen id my collide with a previously created one, in which
> case the add will fail.  It would be nice if the library could
> automatically modify it until it found a unique one, but at the moment
> qmp_run_command() doesn''t return which error the command failed
by, so
> the caller can''t tell that the command failed because of a
duplicate
> id.
> 
> Since it''s additional work, and it''s not clear that the
situation can
> actually happen in practice, I''m considering it a "maybe do
later".
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  tools/libxl/libxl.c          |   85
++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl.h          |   32 ++++++++++++++++
>  tools/libxl/libxl_internal.h |    3 ++
>  tools/libxl/libxl_qmp.c      |   45 ++++++++++++++++++++++
>  tools/libxl/libxl_types.idl  |    8 ++++
>  5 files changed, 173 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 572c2c6..34b648e 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2498,6 +2498,91 @@ out:
>      return AO_INPROGRESS;
>  }
> 
> +int libxl_hvm_host_usb_add(libxl_ctx *ctx, uint32_t domid,
> +                           libxl_device_host_usb *dev)
Would it make sense to just call this function libxl_device_usb_add and
fail if guest type is not HVM?

If we later add usb support to PV guests, I would prefer to avoid having
another libxl_pv_host_usb_add or libxl_pvh_host_usb_add.

Also you should add a "const libxl_asyncop_how *ao_how", even if this
is
not an async op right now, we might want to make it async in the future.
> +{
> +    GC_INIT(ctx);
> +    int rc, dm_ver;
> +
> +    libxl_domain_type type = libxl__domain_type(gc, domid);
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    if (type != LIBXL_DOMAIN_TYPE_HVM) {
> +        LOG(ERROR, "hvm-host-usb-add requires an HVM domain");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    if (libxl_get_stubdom_id(ctx, domid) != 0) {
> +        LOG(ERROR, "hvm-host-usb-add doesn''t work for stub
domains");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    dm_ver = libxl__device_model_version_running(gc, domid);
> +    if (dm_ver == -1) {
> +        LOG(ERROR, "cannot determine device model version");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> +        rc = libxl__qmp_host_usb_add(gc, domid, dev);
> +    } else {
> +        LOG(ERROR, "hvm-host-usb-add not yet implemented for
qemu-traditional");
> +        rc = ERROR_FAIL;
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_hvm_host_usb_del(libxl_ctx *ctx, uint32_t domid,
> +                           const char * id)
Same here, libxl_device_usb_remove will probably be a better name to
keep in sync with the current device functions, and it''s also missing a
"const libxl_asyncop_how *ao_how".

Is it possible to pass a libxl_device_host_usb *dev instead of an id? So
that the function resembles to the other _remove/_destroy functions.

On 03/19/2013 01:14 PM, Roger Pau Monne wrote:
> On 19/03/13 13:09, George Dunlap wrote:
>> This uses the qmp functionality, and is thus only available for
qemu-xen,
>> not qemu-traditional.
>>
>> Devices must be removed by "id", an identifying string, which
must be
>> specified when the device is created.  The caller can either pass one
>> in to request; if none is passed in, then libxl will choose one and
pass
>> it back to the caller.
>>
>> qemu will reject duplicate ids.  There is a small possibility that the
>> libxl-chosen id my collide with a previously created one, in which
>> case the add will fail.  It would be nice if the library could
>> automatically modify it until it found a unique one, but at the moment
>> qmp_run_command() doesn''t return which error the command
failed by, so
>> the caller can''t tell that the command failed because of a
duplicate
>> id.
>>
>> Since it''s additional work, and it''s not clear that
the situation can
>> actually happen in practice, I''m considering it a "maybe
do later".
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>>   tools/libxl/libxl.c          |   85
++++++++++++++++++++++++++++++++++++++++++
>>   tools/libxl/libxl.h          |   32 ++++++++++++++++
>>   tools/libxl/libxl_internal.h |    3 ++
>>   tools/libxl/libxl_qmp.c      |   45 ++++++++++++++++++++++
>>   tools/libxl/libxl_types.idl  |    8 ++++
>>   5 files changed, 173 insertions(+)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 572c2c6..34b648e 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -2498,6 +2498,91 @@ out:
>>       return AO_INPROGRESS;
>>   }
>>
>> +int libxl_hvm_host_usb_add(libxl_ctx *ctx, uint32_t domid,
>> +                           libxl_device_host_usb *dev)
>
> Would it make sense to just call this function libxl_device_usb_add and
> fail if guest type is not HVM?
>
> If we later add usb support to PV guests, I would prefer to avoid having
> another libxl_pv_host_usb_add or libxl_pvh_host_usb_add.
I had thought about something like that, but the basic problem is that 
HVM guests can use PVUSB as well. I don''t think libxl is the right
place
to be deciding whether to use qemu or PVUSB if both are available.
> Also you should add a "const libxl_asyncop_how *ao_how", even if
this is
> not an async op right now, we might want to make it async in the future.
OK -- I''ll try to copy libxl_insert_cdrom(), since that seems to do the
"fake async" thing.
>
>> +{
>> +    GC_INIT(ctx);
>> +    int rc, dm_ver;
>> +
>> +    libxl_domain_type type = libxl__domain_type(gc, domid);
>> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +    if (type != LIBXL_DOMAIN_TYPE_HVM) {
>> +        LOG(ERROR, "hvm-host-usb-add requires an HVM
domain");
>> +        rc = ERROR_INVAL;
>> +        goto out;
>> +    }
>> +
>> +    if (libxl_get_stubdom_id(ctx, domid) != 0) {
>> +        LOG(ERROR, "hvm-host-usb-add doesn''t work for
stub domains");
>> +        rc = ERROR_INVAL;
>> +        goto out;
>> +    }
>> +
>> +    dm_ver = libxl__device_model_version_running(gc, domid);
>> +    if (dm_ver == -1) {
>> +        LOG(ERROR, "cannot determine device model version");
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +
>> +    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
>> +        rc = libxl__qmp_host_usb_add(gc, domid, dev);
>> +    } else {
>> +        LOG(ERROR, "hvm-host-usb-add not yet implemented for
qemu-traditional");
>> +        rc = ERROR_FAIL;
>> +    }
>> +
>> +out:
>> +    GC_FREE;
>> +    return rc;
>> +}
>> +
>> +int libxl_hvm_host_usb_del(libxl_ctx *ctx, uint32_t domid,
>> +                           const char * id)
>
> Same here, libxl_device_usb_remove will probably be a better name to
> keep in sync with the current device functions, and it''s also
missing a
> "const libxl_asyncop_how *ao_how".
>
> Is it possible to pass a libxl_device_host_usb *dev instead of an id? So
> that the function resembles to the other _remove/_destroy functions.
The only way qmp allows you to remove a device is via id.  And at the 
moment there is no way via qmp to list USB devices to find out which 
ones might have an id.

I suppose if we allow the user to pass *dev though, then usb_del could 
either use dev->id (if it exists), or re-construct the default id and 
try to remove it if not.

  -George

On 19/03/13 15:04, George Dunlap wrote:
> On 03/19/2013 01:14 PM, Roger Pau Monne wrote:
>> On 19/03/13 13:09, George Dunlap wrote:
>>> This uses the qmp functionality, and is thus only available for
qemu-xen,
>>> not qemu-traditional.
>>>
>>> Devices must be removed by "id", an identifying string,
which must be
>>> specified when the device is created.  The caller can either pass
one
>>> in to request; if none is passed in, then libxl will choose one and
pass
>>> it back to the caller.
>>>
>>> qemu will reject duplicate ids.  There is a small possibility that
the
>>> libxl-chosen id my collide with a previously created one, in which
>>> case the add will fail.  It would be nice if the library could
>>> automatically modify it until it found a unique one, but at the
moment
>>> qmp_run_command() doesn''t return which error the command
failed by, so
>>> the caller can''t tell that the command failed because of a
duplicate
>>> id.
>>>
>>> Since it''s additional work, and it''s not clear
that the situation can
>>> actually happen in practice, I''m considering it a
"maybe do later".
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> ---
>>>   tools/libxl/libxl.c          |   85
++++++++++++++++++++++++++++++++++++++++++
>>>   tools/libxl/libxl.h          |   32 ++++++++++++++++
>>>   tools/libxl/libxl_internal.h |    3 ++
>>>   tools/libxl/libxl_qmp.c      |   45 ++++++++++++++++++++++
>>>   tools/libxl/libxl_types.idl  |    8 ++++
>>>   5 files changed, 173 insertions(+)
>>>
>>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>>> index 572c2c6..34b648e 100644
>>> --- a/tools/libxl/libxl.c
>>> +++ b/tools/libxl/libxl.c
>>> @@ -2498,6 +2498,91 @@ out:
>>>       return AO_INPROGRESS;
>>>   }
>>>
>>> +int libxl_hvm_host_usb_add(libxl_ctx *ctx, uint32_t domid,
>>> +                           libxl_device_host_usb *dev)
>>
>> Would it make sense to just call this function libxl_device_usb_add and
>> fail if guest type is not HVM?
>>
>> If we later add usb support to PV guests, I would prefer to avoid
having
>> another libxl_pv_host_usb_add or libxl_pvh_host_usb_add.
> 
> I had thought about something like that, but the basic problem is that 
> HVM guests can use PVUSB as well. I don''t think libxl is the right
place
> to be deciding whether to use qemu or PVUSB if both are available.
I guess using both (PVUSB + Qemu) for HVM, and let the guest decide
won''t work, right?

Since libxl doesn''t support PVUSB right now you could always name this
libxl_device_usb_add and sort the problem out once we have PVUSB
support. IMHO I would really prefer to only have one public function to
add USB devices if possible.
> 
>> Also you should add a "const libxl_asyncop_how *ao_how", even
if this is
>> not an async op right now, we might want to make it async in the
future.
> 
> OK -- I''ll try to copy libxl_insert_cdrom(), since that seems to
do the
> "fake async" thing.
> 
>>
>>> +{
>>> +    GC_INIT(ctx);
>>> +    int rc, dm_ver;
>>> +
>>> +    libxl_domain_type type = libxl__domain_type(gc, domid);
>>> +    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
>>> +        rc = ERROR_FAIL;
>>> +        goto out;
>>> +    }
>>> +    if (type != LIBXL_DOMAIN_TYPE_HVM) {
>>> +        LOG(ERROR, "hvm-host-usb-add requires an HVM
domain");
>>> +        rc = ERROR_INVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (libxl_get_stubdom_id(ctx, domid) != 0) {
>>> +        LOG(ERROR, "hvm-host-usb-add doesn''t work
for stub domains");
>>> +        rc = ERROR_INVAL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    dm_ver = libxl__device_model_version_running(gc, domid);
>>> +    if (dm_ver == -1) {
>>> +        LOG(ERROR, "cannot determine device model
version");
>>> +        rc = ERROR_FAIL;
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
>>> +        rc = libxl__qmp_host_usb_add(gc, domid, dev);
>>> +    } else {
>>> +        LOG(ERROR, "hvm-host-usb-add not yet implemented for
qemu-traditional");
>>> +        rc = ERROR_FAIL;
>>> +    }
>>> +
>>> +out:
>>> +    GC_FREE;
>>> +    return rc;
>>> +}
>>> +
>>> +int libxl_hvm_host_usb_del(libxl_ctx *ctx, uint32_t domid,
>>> +                           const char * id)
>>
>> Same here, libxl_device_usb_remove will probably be a better name to
>> keep in sync with the current device functions, and it''s also
missing a
>> "const libxl_asyncop_how *ao_how".
>>
>> Is it possible to pass a libxl_device_host_usb *dev instead of an id?
So
>> that the function resembles to the other _remove/_destroy functions.
> 
> The only way qmp allows you to remove a device is via id.  And at the 
> moment there is no way via qmp to list USB devices to find out which 
> ones might have an id.
> 
> I suppose if we allow the user to pass *dev though, then usb_del could 
> either use dev->id (if it exists), or re-construct the default id and 
> try to remove it if not.
You could pass the same id that you are passing as an argument inside
libxl_device_usb id field.

On Tue, Mar 19, 2013 at 5:55 PM, Roger Pau Monné <roger.pau@citrix.com>
wrote:
>> I had thought about something like that, but the basic problem is that
>> HVM guests can use PVUSB as well. I don''t think libxl is the
right place
>> to be deciding whether to use qemu or PVUSB if both are available.
>
> I guess using both (PVUSB + Qemu) for HVM, and let the guest decide
> won''t work, right?
How is the guest in a position to decide?  One requires libxl to talk
to qmp, the other requires libxl to do a bunch of xenstore writes.
The second requires not only a funcitonal PVUSB front-end in the
guest, but also a functional PVUSB back-end; and apparently the PVUSB
back-end is decent in the SuSE "classic Xen" kernels, but fairly well
broken in the pvops kernels at the moment.  The guest is in no
position to know whether the PVUSB back-end is reliable or not, and
thus no position to decide for itself.
> Since libxl doesn''t support PVUSB right now you could always name
this
> libxl_device_usb_add and sort the problem out once we have PVUSB
> support.
I have to completely disagree with this.  The current xm commands
evolved this way, and the result is that the naming is incredibly
confusing: there are two sets of commands because there have to be
(see below), but it''s really difficult to tell which one goes with
which.  Given that we are definitely planning on doing qemu USB and PV
USB, we should take the opportunity to plan ahead what kind of
interface we''re shooting for.  If we have to have a different set of
interfaces for each one, they need to be clear and consistent.
> IMHO I would really prefer to only have one public function to
> add USB devices if possible.
That was my goal initially as well.  However, I just don''t think
it''s
practical.  The underlying interfaces to which the commands must map
are too different -- there are extra steps which need to be done for
PVUSB that don''t need to be done for qemu-xen and vice versa, and
functionality that is available with one but not the other.  Trying to
shove them into one interface means either 1) having some of the
advertised interface functionality not work for one or the other, or
2) making the interface support only what is supported in both.  #1 is
bad because it makes the interface much more complicated and difficult
for the programmer to use; #2 is bad because we don''t make full use of
the functionality available.

There are three sets of interfaces we need to consider:

* qemu-traditional
- Communication method: qemu-monitor
- Available commands:
 usb-add: Accepts "legacy" USB device
 usb-del: Accepts same arguments as above
 usb-list: List existing usb devices
- Unique aspects:
  Can add emulated tablets, mice, keyboard, &c, as well as host USB.
  usb-del accepts same arguments as usb-add, so no need for
''id''
  usb-list available
  Explicit wildcards needed
  Only a single command needed to add
  No way to make USB busses; topology handled automatically

* qemu-xen
 - Communication method: qmp
 - Available commands:
 device-add: Accepts "qdev"-style devices, which are more generic.
Also accepts ''id''
 device-remove: Only accepts ''id''
- Unique aspects:
  Implicit wildcards -- any factor not specified matches anything
  No way to list USB devices
  Must specify an id on creation in order to remove a device
  Only a single command needed to add
  Adding emulated tablets, mice, keyboard &c has a very different
syntax to host-usb, not easy to make the same interface work for both
  No way to make USB busses; topology handled automatically

* PVUSB
 - Communication method: xenbus
 - Available commands:
  usb-hc-create/destroy
  usb-attach/detach
  usb-assignable-devices
- Unique aspects:
  I haven''t looked at this in detail, but this is what it looks like:
  You can (must?) create different virtual busses
  You must make a USB device "assignable" before attaching it (?)
  You must specify the USB bus to attach it to
  Devices are removed by vbus.vport rather than by id, or by vendorid:productid

I''m focusing on getting qemu-xen host-usb support working at the
moment.  It''s possible that the qemu-xen commands could be mostly
mapped on to qemu-traditional commands; but there''s just too much
mismatch between PVUSB and qemu-xen to make it worthwhile, I think.

>> I suppose if we allow the user to pass *dev though, then usb_del could
>> either use dev->id (if it exists), or re-construct the default id
and
>> try to remove it if not.
>
> You could pass the same id that you are passing as an argument inside
> libxl_device_usb id field.
Passing a whole structure in when only one element is going to be read
-- requiring the caller to declare the structure, initialize it, set
the element, then tear it down afterwards -- just for a char *?  That
seems incredibly silly to me.

But having the option of just re-specifying the device the same way
you did the first time, rather than having to keep the arbitrary
string around somewhere, does make sense.

 -George

George Dunlap writes ("Re: [Xen-devel] [PATCH 1/2] libxl: Introduce
functions to add and remove host USB devices to an HVM
guest"):
> There are three sets of interfaces we need to consider:
For disks and networks we offer the same device via both interfaces,
where available.  I guess we can''t do that for USB ?

If it is actually necessary for the caller to specify, they should
specify the the protocol by which the device is exposed to the guest,
not the implementation.

Perhaps we could make the protocol an optional parameter somehow, so
that if the user just wants "make such-and-such device available",
they can ask for that and the tools will DTRT ?

Many of your comments relate to differences in the protocols between
libxl and qemu or backends.  Syntactic differences should IMO be
hidden by libxl.  There is generally no good reason for semantic
differences, I think ?

Ian.

On 20/03/13 16:51, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH 1/2] libxl: Introduce
functions to add and remove host USB devices to an HVM guest"):
>> There are three sets of interfaces we need to consider:
> For disks and networks we offer the same device via both interfaces,
> where available.  I guess we can''t do that for USB ?
I don''t think the requisite infrastructure is there yet; I suspect that
usbback needs to have the device assigned to it before it can be made 
available to usbfront, and that this would conflict with qemu having it 
passed through.  Also, there are magic ports the PV front-ends write to 
cause qemu to yank out the emulated block/nic devices before connecting 
to the back-ends -- I don''t think such things exist for USB yet.
> If it is actually necessary for the caller to specify, they should
> specify the the protocol by which the device is exposed to the guest,
> not the implementation.
>
> Perhaps we could make the protocol an optional parameter somehow, so
> that if the user just wants "make such-and-such device
available",
> they can ask for that and the tools will DTRT ?
>
> Many of your comments relate to differences in the protocols between
> libxl and qemu or backends.  Syntactic differences should IMO be
> hidden by libxl.  There is generally no good reason for semantic
> differences, I think ?
There are some semantic differences that I think are important (or could 
be important).  One big one is that PVUSB appears to require the caller 
to specify the virtual topology used, while with qemu it is not possible 
to specify the virtual topology.  This gives us a few options for a 
unified interface:

1. Don''t expose virtual topology options to the caller.  If using qemu,
just make the call; if using PVUSB, do what qemu does in automatically 
adding and removing the necessary virtual busses.

2. Expose virtual topology options to the caller.  If using qemu, then 
ignore these / return an error.

3. Add support to qemu for specifying virtual topology.

I don''t particularly like either of the first two options.  #1 removes 
the flexibility of the user specifying their own topology for PVUSB, 
even though it''s available; #2 make the interface more complicated to 
interact with for HVM.  #3 would be nice, but it''s definitely not going
to happen for 4.3.  (And honestly it''s a lot more work than
I''m probably
in for.)

PVUSB also (it seems) requires devices to be assigned to usbback before 
they can be given to guests.  So in the pv case, device_add() would have 
to do assign then attach, and device_del would have to do detach then 
de-assign.  That''s probably not so bad.

I''m guessing that PVUSB requires you to specify devices by 
hostbus.hostaddr, not by productid:vendorid.  Having a unified interface 
would probably mean only allowing any device to be specified by 
hostbus.hostaddr.  This is technically a masking of the functionality of 
qmp (namely, the ability to specify by vendorid.productid), but that''s 
probably not a terrible loss.

Another difference is that PVUSB (and qemu-traditional) remove devices 
by the guest virtual topology, while qmp must remove them using the id 
assigned when setting it up.  However, there is no way to tell when 
calling device_add what the virtual topology will end up looking like, 
and no way to query it from outside of the guest once it''s done.  So it
seems the options would be:

1. Force the "namespace" for deletion to be host-device-spec for all 
protocols.

2. Have different ways to delete depending on whether you''re PVUSB or
qmp.

3. Implement methods for qmp to discover guest virtual topology

I''m not a fan of #1; #2 again suffers from making the interface more 
complicated; and #3 is, I''m guessing, more work than can be done by
4.3.

Another aspect of all this is that it would be nice, at some point in 
the future, to expose more of the qdev interface.  qdev allows you to 
plug in virtual USB sticks, and a whole range of emulated devices.

And, having a unified interface would more or less force me to actually 
implement the PVUSB interface by 4.3, to make sure that the interface 
can be properly implemented, and that we haven''t missed anything.

  -George

George Dunlap writes ("Re: [Xen-devel] [PATCH 1/2] libxl: Introduce
functions to add and remove host USB devices to an HVM
guest"):
> There are some semantic differences that I think are important (or could 
> be important).  One big one is that PVUSB appears to require the caller 
> to specify the virtual topology used, while with qemu it is not possible 
> to specify the virtual topology.  This gives us a few options for a 
> unified interface:
The obvious answer to this is to make specifying the virtual topology
optional in the unified syntax.

(TBH I''m not sure why anyone would ever want to specify a particular
virtual topology.  I''m sure most people would prefer just to let the
tools set something up.)
> PVUSB also (it seems) requires devices to be assigned to usbback before 
> they can be given to guests.  So in the pv case, device_add() would have 
> to do assign then attach, and device_del would have to do detach then 
> de-assign.  That''s probably not so bad.
I definitely think this should happen automatically.
> I''m guessing that PVUSB requires you to specify devices by 
> hostbus.hostaddr, not by productid:vendorid.  Having a unified interface 
> would probably mean only allowing any device to be specified by 
> hostbus.hostaddr.  This is technically a masking of the functionality of 
> qmp (namely, the ability to specify by vendorid.productid), but
that''s
> probably not a terrible loss.
Well, it would involve that, or implemnting some kind of search
facility so that the host path is looked up appropriately.  Clearly a
reasonable interface would allow the user to specify the device either
way.
> Another difference is that PVUSB (and qemu-traditional) remove devices 
> by the guest virtual topology, while qmp must remove them using the id 
> assigned when setting it up.  However, there is no way to tell when 
> calling device_add what the virtual topology will end up looking like, 
> and no way to query it from outside of the guest once it''s done. 
So it
> seems the options would be:
You''ve missed

 4. Maintain in the toolstack whatever information is necessary to do
    device listing and removal.
> Another aspect of all this is that it would be nice, at some point in 
> the future, to expose more of the qdev interface.  qdev allows you to 
> plug in virtual USB sticks, and a whole range of emulated devices.
> 
> And, having a unified interface would more or less force me to actually 
> implement the PVUSB interface by 4.3, to make sure that the interface 
> can be properly implemented, and that we haven''t missed anything.
I think the right approach is to think what interface we want,
implement the subset of it we care about and have time for right now,
and declare failure to support this interface by other implementations
a bug.  I don''t think there are any aspects of a plausible and sane
interface that are in principle excluded by any of the
implementations.

Ian.

On 20.03.2013 19:26, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [PATCH 1/2] libxl: Introduce
functions to add and remove host USB devices to an HVM guest"):
>> There are some semantic differences that I think are important (or
could
>> be important).  One big one is that PVUSB appears to require the caller
>> to specify the virtual topology used, while with qemu it is not
possible
>> to specify the virtual topology.  This gives us a few options for a 
>> unified interface:
> 
> The obvious answer to this is to make specifying the virtual topology
> optional in the unified syntax.
> 
> (TBH I''m not sure why anyone would ever want to specify a
particular
> virtual topology.  I''m sure most people would prefer just to let
the
> tools set something up.)
Yes, specifying topology by hand (which basically means creating one USB 1.1
bus and one USB 2.0 bus) is only inconvenience in PVUSB. It should be done
automatically.
>> PVUSB also (it seems) requires devices to be assigned to usbback before
>> they can be given to guests.  So in the pv case, device_add() would
have
>> to do assign then attach, and device_del would have to do detach then 
>> de-assign.  That''s probably not so bad.
> 
> I definitely think this should happen automatically.
Indeed. This is one/two writes to sysfs. One possible difficulty: backend can
be in some domU instead of dom0, but then IMHO assigning device to usbback can
be left to the user.

But important thing: interface needs to allow specify (optional) backend
domain in addition to device itself (with default to dom0). Same as other
interfaces like block or network.

-- 
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab



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