[PATCH 0/6] libxl: add HVM USB passthrough capability

[PATCH 0/6] libxl: add HVM USB passthrough capability

[Juergen Gross]

Add the capability to pass USB devices to HVM domains by using the
emulation of USB controllers of qemu.

The user interface via xl is the same as for pvusb passthrough, only
the type of the usbctrl is different: instead of "qusb" (qemu-based
pvusb backend) or "vusb" (kernel-based pvusb backend) the type
"devicemodel" is used.

Especially the communication with qemu via qmp commands is based on
the patches of George Dunlap sent in 2014:

https://lists.xen.org/archives/html/xen-devel/2014-06/msg00085.html

Juergen Gross (6):
  libxl: rename libcl_pvusb.c to libxl_usb.c
  libxl: add libxl__qmp_run_command_flexarray() function
  libxl: dont pass array size to libxl__xs_kvs_of_flexarray()
  libxl: add basic support for devices without backend
  libxl: add HVM usb passthrough support
  docs: add HVM USB passthrough documentation

 docs/man/xl.cfg.pod.5.in                   |  12 +-
 tools/libxl/Makefile                       |   2 +-
 tools/libxl/libxl.c                        |  22 +-
 tools/libxl/libxl_device.c                 |  62 +++--
 tools/libxl/libxl_internal.h               |   5 +-
 tools/libxl/libxl_nic.c                    |   6 +-
 tools/libxl/libxl_pci.c                    |   7 +-
 tools/libxl/libxl_qmp.c                    |  16 ++
 tools/libxl/libxl_types_internal.idl       |   1 +
 tools/libxl/{libxl_pvusb.c => libxl_usb.c} | 423 ++++++++++++++++++++++-------
 tools/libxl/libxl_vtpm.c                   |   6 +-
 tools/libxl/libxl_xshelp.c                 |   8 +-
 tools/libxl/xl_cmdimpl.c                   |   4 +-
 13 files changed, 429 insertions(+), 145 deletions(-)
 rename tools/libxl/{libxl_pvusb.c => libxl_usb.c} (80%)

-- 
2.6.6


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

[PATCH 1/6] libxl: rename libcl_pvusb.c to libxl_usb.c

[Juergen Gross]

Rename libcl_pvusb.c to libxl_usb.c in order to reflect future support
of USB passthrough via qemu emulated USB controllers.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/Makefile                       | 2 +-
 tools/libxl/{libxl_pvusb.c => libxl_usb.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename tools/libxl/{libxl_pvusb.c => libxl_usb.c} (100%)

diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 6994c58..f32169a 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -108,7 +108,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
 			libxl_stream_read.o libxl_stream_write.o \
 			libxl_save_callout.o _libxl_save_msgs_callout.o \
 			libxl_qmp.o libxl_event.o libxl_fork.o \
-			libxl_dom_suspend.o libxl_dom_save.o libxl_pvusb.o \
+			libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \
 			libxl_vtpm.o libxl_nic.o \
                         $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_usb.c
similarity index 100%
rename from tools/libxl/libxl_pvusb.c
rename to tools/libxl/libxl_usb.c
-- 
2.6.6


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

Re: [PATCH 1/6] libxl: rename libcl_pvusb.c to libxl_usb.c

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:21AM +0200, Juergen Gross wrote:
> Rename libcl_pvusb.c to libxl_usb.c in order to reflect future support
> of USB passthrough via qemu emulated USB controllers.
> 

Typo "libcl" in both subject line and commit message.

> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/Makefile                       | 2 +-
>  tools/libxl/{libxl_pvusb.c => libxl_usb.c} | 0
>  2 files changed, 1 insertion(+), 1 deletion(-)
>  rename tools/libxl/{libxl_pvusb.c => libxl_usb.c} (100%)
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 6994c58..f32169a 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -108,7 +108,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
>  			libxl_stream_read.o libxl_stream_write.o \
>  			libxl_save_callout.o _libxl_save_msgs_callout.o \
>  			libxl_qmp.o libxl_event.o libxl_fork.o \
> -			libxl_dom_suspend.o libxl_dom_save.o libxl_pvusb.o \
> +			libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \
>  			libxl_vtpm.o libxl_nic.o \
>                          $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_usb.c
> similarity index 100%
> rename from tools/libxl/libxl_pvusb.c
> rename to tools/libxl/libxl_usb.c
> -- 
> 2.6.6
> 

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

[PATCH 2/6] libxl: add libxl__qmp_run_command_flexarray() function

[Juergen Gross]

Add a function libxl__qmp_run_command_flexarray() to run a qmp command
with an array of arguments. The arguments are name-value pairs stored
in a flexarray.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_qmp.c      | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3f29aa6..ecbfdad 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1753,6 +1753,9 @@ typedef struct libxl__qmp_handler libxl__qmp_handler;
  */
 _hidden libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc,
                                                   uint32_t domid);
+_hidden int libxl__qmp_run_command_flexarray(libxl__gc *gc, int domid,
+                                             const char *cmd,
+                                             flexarray_t *array);
 /* ask to QEMU the serial port information and store it in xenstore. */
 _hidden int libxl__qmp_query_serial(libxl__qmp_handler *qmp);
 _hidden int libxl__qmp_pci_add(libxl__gc *gc, int d, libxl_device_pci *pcidev);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 0d8d5f4..f8addf9 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -827,6 +827,22 @@ static int qmp_run_command(libxl__gc *gc, int domid,
     return rc;
 }
 
+int libxl__qmp_run_command_flexarray(libxl__gc *gc, int domid,
+                                     const char *cmd, flexarray_t *array)
+{
+    libxl__json_object *args = NULL;
+    int i;
+    void *name, *value;
+
+    for (i = 0; i < array->count; i += 2) {
+        flexarray_get(array, i, &name);
+        flexarray_get(array, i + 1, &value);
+        qmp_parameters_add_string(gc, &args, (char *)name, (char *)value);
+    }
+
+    return qmp_run_command(gc, domid, cmd, args, NULL, NULL);
+}
+
 int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
 {
     libxl__qmp_handler *qmp = NULL;
-- 
2.6.6


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

Re: [PATCH 2/6] libxl: add libxl__qmp_run_command_flexarray() function

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:22AM +0200, Juergen Gross wrote:
> Add a function libxl__qmp_run_command_flexarray() to run a qmp command
> with an array of arguments. The arguments are name-value pairs stored
> in a flexarray.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxl/libxl_internal.h |  3 +++
>  tools/libxl/libxl_qmp.c      | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 3f29aa6..ecbfdad 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -1753,6 +1753,9 @@ typedef struct libxl__qmp_handler libxl__qmp_handler;
>   */
>  _hidden libxl__qmp_handler *libxl__qmp_initialize(libxl__gc *gc,
>                                                    uint32_t domid);
> +_hidden int libxl__qmp_run_command_flexarray(libxl__gc *gc, int domid,
> +                                             const char *cmd,
> +                                             flexarray_t *array);
>  /* ask to QEMU the serial port information and store it in xenstore. */
>  _hidden int libxl__qmp_query_serial(libxl__qmp_handler *qmp);
>  _hidden int libxl__qmp_pci_add(libxl__gc *gc, int d, libxl_device_pci *pcidev);
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 0d8d5f4..f8addf9 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -827,6 +827,22 @@ static int qmp_run_command(libxl__gc *gc, int domid,
>      return rc;
>  }
>  
> +int libxl__qmp_run_command_flexarray(libxl__gc *gc, int domid,
> +                                     const char *cmd, flexarray_t *array)
> +{
> +    libxl__json_object *args = NULL;
> +    int i;
> +    void *name, *value;
> +
> +    for (i = 0; i < array->count; i += 2) {
> +        flexarray_get(array, i, &name);
> +        flexarray_get(array, i + 1, &value);
> +        qmp_parameters_add_string(gc, &args, (char *)name, (char *)value);
> +    }
> +
> +    return qmp_run_command(gc, domid, cmd, args, NULL, NULL);
> +}
> +
>  int libxl__qmp_pci_add(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
>  {
>      libxl__qmp_handler *qmp = NULL;
> -- 
> 2.6.6
> 

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

[PATCH 3/6] libxl: dont pass array size to libxl__xs_kvs_of_flexarray()

[Juergen Gross]

Instead of passing the array size as an argument when calling
libxl__xs_kvs_of_flexarray() let the function get the size from the
array instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl.c          | 22 +++++++++++-----------
 tools/libxl/libxl_internal.h |  2 +-
 tools/libxl/libxl_nic.c      |  6 ++----
 tools/libxl/libxl_pci.c      |  7 +++----
 tools/libxl/libxl_usb.c      |  6 +++---
 tools/libxl/libxl_vtpm.c     |  6 ++----
 tools/libxl/libxl_xshelp.c   |  4 ++--
 7 files changed, 24 insertions(+), 29 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 577ed35..b7b8b08 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2315,8 +2315,8 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         }
 
         libxl__device_generic_add(gc, t, device,
-                                  libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                                  libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                                  libxl__xs_kvs_of_flexarray(gc, back),
+                                  libxl__xs_kvs_of_flexarray(gc, front),
                                   NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
@@ -2735,7 +2735,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
             goto out;
         }
 
-        char **kvs = libxl__xs_kvs_of_flexarray(gc, empty, empty->count);
+        char **kvs = libxl__xs_kvs_of_flexarray(gc, empty);
 
         rc = libxl__xs_writev(gc, t, be_path, kvs);
         if (rc) goto out;
@@ -2777,7 +2777,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk *disk,
         rc = libxl__set_domain_configuration(gc, domid, &d_config);
         if (rc) goto out;
 
-        char **kvs = libxl__xs_kvs_of_flexarray(gc, insert, insert->count);
+        char **kvs = libxl__xs_kvs_of_flexarray(gc, insert);
 
         rc = libxl__xs_writev(gc, t, be_path, kvs);
         if (rc) goto out;
@@ -3147,9 +3147,9 @@ int libxl__device_console_add(libxl__gc *gc, uint32_t domid,
         flexarray_append(front, LIBXL_XENCONSOLE_PROTOCOL);
     }
     libxl__device_generic_add(gc, XBT_NULL, device,
-                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                              libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                              libxl__xs_kvs_of_flexarray(gc, ro_front, ro_front->count));
+                              libxl__xs_kvs_of_flexarray(gc, back),
+                              libxl__xs_kvs_of_flexarray(gc, front),
+                              libxl__xs_kvs_of_flexarray(gc, ro_front));
     rc = 0;
 out:
     return rc;
@@ -3476,8 +3476,8 @@ int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(front, GCSPRINTF("%d", XenbusStateInitialising));
 
     libxl__device_generic_add(gc, XBT_NULL, &device,
-                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                              libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                              libxl__xs_kvs_of_flexarray(gc, back),
+                              libxl__xs_kvs_of_flexarray(gc, front),
                               NULL);
     rc = 0;
 out:
@@ -3589,8 +3589,8 @@ int libxl__device_vfb_add(libxl__gc *gc, uint32_t domid, libxl_device_vfb *vfb)
     flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
 
     libxl__device_generic_add(gc, XBT_NULL, &device,
-                              libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                              libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                              libxl__xs_kvs_of_flexarray(gc, back),
+                              libxl__xs_kvs_of_flexarray(gc, front),
                               NULL);
     rc = 0;
 out:
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ecbfdad..ec4fc23 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -676,7 +676,7 @@ _hidden int libxl__remove_directory(libxl__gc *gc, const char *path);
 _hidden int libxl__remove_file_or_directory(libxl__gc *gc, const char *path);
 
 
-_hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length);
+_hidden char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array);
 
 /* treats kvs as pairs of keys and values and writes each to dir. */
 _hidden int libxl__xs_writev(libxl__gc *gc, xs_transaction_t t,
diff --git a/tools/libxl/libxl_nic.c b/tools/libxl/libxl_nic.c
index d1caa90..220a028 100644
--- a/tools/libxl/libxl_nic.c
+++ b/tools/libxl/libxl_nic.c
@@ -266,10 +266,8 @@ static void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
         }
 
         libxl__device_generic_add(gc, t, device,
-                                  libxl__xs_kvs_of_flexarray(gc, back,
-                                                             back->count),
-                                  libxl__xs_kvs_of_flexarray(gc, front,
-                                                             front->count),
+                                  libxl__xs_kvs_of_flexarray(gc, back),
+                                  libxl__xs_kvs_of_flexarray(gc, front),
                                   NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 22398a4..6f8f49c 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -104,8 +104,8 @@ int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
     flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
 
     return libxl__device_generic_add(gc, XBT_NULL, &device,
-                                     libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                                     libxl__xs_kvs_of_flexarray(gc, front, front->count),
+                                     libxl__xs_kvs_of_flexarray(gc, back),
+                                     libxl__xs_kvs_of_flexarray(gc, front),
                                      NULL);
 }
 
@@ -172,8 +172,7 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
         rc = libxl__set_domain_configuration(gc, domid, &d_config);
         if (rc) goto out;
 
-        libxl__xs_writev(gc, t, be_path,
-                         libxl__xs_kvs_of_flexarray(gc, back, back->count));
+        libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 75f7b8b..6b30e0f 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -177,9 +177,9 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
         }
 
         libxl__device_generic_add(gc, t, device,
-                          libxl__xs_kvs_of_flexarray(gc, back, back->count),
-                          libxl__xs_kvs_of_flexarray(gc, front, front->count),
-                          NULL);
+                                  libxl__xs_kvs_of_flexarray(gc, back),
+                                  libxl__xs_kvs_of_flexarray(gc, front),
+                                  NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
diff --git a/tools/libxl/libxl_vtpm.c b/tools/libxl/libxl_vtpm.c
index 29a0817..8588569 100644
--- a/tools/libxl/libxl_vtpm.c
+++ b/tools/libxl/libxl_vtpm.c
@@ -141,10 +141,8 @@ static void libxl__device_vtpm_add(libxl__egc *egc, uint32_t domid,
         }
 
         libxl__device_generic_add(gc, t, device,
-                                  libxl__xs_kvs_of_flexarray(gc, back,
-                                                             back->count),
-                                  libxl__xs_kvs_of_flexarray(gc, front,
-                                                             front->count),
+                                  libxl__xs_kvs_of_flexarray(gc, back),
+                                  libxl__xs_kvs_of_flexarray(gc, front),
                                   NULL);
 
         rc = libxl__xs_transaction_commit(gc, &t);
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index e1412a6..4982b52 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -17,10 +17,10 @@
 
 #include "libxl_internal.h"
 
-char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array, int length)
+char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array)
 {
     char **kvs;
-    int i;
+    int i, length = array->count;
 
     if (!length)
         return NULL;
-- 
2.6.6


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

Re: [PATCH 3/6] libxl: dont pass array size to libxl__xs_kvs_of_flexarray()

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:23AM +0200, Juergen Gross wrote:
> Instead of passing the array size as an argument when calling
> libxl__xs_kvs_of_flexarray() let the function get the size from the
> array instead.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 3/6] libxl: dont pass array size to libxl__xs_kvs_of_flexarray()

[Wei Liu]

On Fri, Sep 09, 2016 at 11:00:23AM +0100, Wei Liu wrote:
> On Thu, Sep 08, 2016 at 09:20:23AM +0200, Juergen Gross wrote:
> > Instead of passing the array size as an argument when calling
> > libxl__xs_kvs_of_flexarray() let the function get the size from the
> > array instead.
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Patch 1-3 pushed.

Wei.

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

[PATCH 4/6] libxl: add basic support for devices without backend

[Juergen Gross]

With the planned support of HVM USB passthrough via the USB emulation
capabilities of qemu libxl has to support guest devices which have no
back- and frontend. Information about those devices will live in the
libxl part of Xenstore only.

Add some basic support to libxl to be able to cope with this scenario.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_device.c           | 59 ++++++++++++++++++++++++------------
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_xshelp.c           |  6 +++-
 3 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 9c77b62..5211f20 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -114,15 +114,21 @@ int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
         libxl__device *device, char **bents, char **fents, char **ro_fents)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    char *frontend_path, *backend_path, *libxl_path;
+    char *frontend_path = NULL, *backend_path = NULL, *libxl_path;
     struct xs_permissions frontend_perms[2];
     struct xs_permissions ro_frontend_perms[2];
     struct xs_permissions backend_perms[2];
     int create_transaction = t == XBT_NULL;
+    int libxl_only = device->backend_kind == LIBXL__DEVICE_KIND_NONE;
     int rc;
 
-    frontend_path = libxl__device_frontend_path(gc, device);
-    backend_path = libxl__device_backend_path(gc, device);
+    if (libxl_only) {
+        /* bents should be set as this is used to setup libxl_path content. */
+        assert(!fents && !ro_fents);
+    } else {
+        frontend_path = libxl__device_frontend_path(gc, device);
+        backend_path = libxl__device_backend_path(gc, device);
+    }
     libxl_path = libxl__device_libxl_path(gc, device);
 
     frontend_perms[0].id = device->domid;
@@ -144,13 +150,15 @@ retry_transaction:
     rc = libxl__xs_rm_checked(gc, t, libxl_path);
     if (rc) goto out;
 
-    rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
-                                 frontend_path);
-    if (rc) goto out;
+    if (!libxl_only) {
+        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
+                                     frontend_path);
+        if (rc) goto out;
 
-    rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
-                                 backend_path);
-    if (rc) goto out;
+        rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
+                                     backend_path);
+        if (rc) goto out;
+    }
 
     /* xxx much of this function lacks error checks! */
 
@@ -179,12 +187,15 @@ retry_transaction:
     }
 
     if (bents) {
-        xs_rm(ctx->xsh, t, backend_path);
-        xs_mkdir(ctx->xsh, t, backend_path);
-        xs_set_permissions(ctx->xsh, t, backend_path, backend_perms, ARRAY_SIZE(backend_perms));
-        xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
-                 frontend_path, strlen(frontend_path));
-        libxl__xs_writev(gc, t, backend_path, bents);
+        if (!libxl_only) {
+            xs_rm(ctx->xsh, t, backend_path);
+            xs_mkdir(ctx->xsh, t, backend_path);
+            xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
+                               ARRAY_SIZE(backend_perms));
+            xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
+                     frontend_path, strlen(frontend_path));
+            libxl__xs_writev(gc, t, backend_path, bents);
+        }
 
         /*
          * We make a copy of everything for the backend in the libxl
@@ -194,6 +205,9 @@ retry_transaction:
          * instead.  But there are still places in libxl that try to
          * reconstruct a config from xenstore.
          *
+         * For devices without backend (e.g. USB devices emulated via qemu)
+         * only the libxl path is written.
+         *
          * This duplication will typically produces duplicate keys
          * which will go out of date, but that's OK because nothing
          * reads those.  For example, there is usually
@@ -662,12 +676,18 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 {
-    const char *be_path = libxl__device_backend_path(gc, dev);
-    const char *fe_path = libxl__device_frontend_path(gc, dev);
+    const char *be_path = NULL;
+    const char *fe_path = NULL;
     const char *libxl_path = libxl__device_libxl_path(gc, dev);
     xs_transaction_t t = 0;
     int rc;
     uint32_t domid;
+    int libxl_only = dev->backend_kind == LIBXL__DEVICE_KIND_NONE;
+
+    if (!libxl_only) {
+        be_path = libxl__device_backend_path(gc, dev);
+        fe_path = libxl__device_frontend_path(gc, dev);
+    }
 
     rc = libxl__get_domid(gc, &domid);
     if (rc) goto out;
@@ -681,10 +701,11 @@ int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
              * The toolstack domain is in charge of removing the
              * frontend and libxl paths.
              */
-            libxl__xs_path_cleanup(gc, t, fe_path);
+            if (!libxl_only)
+                libxl__xs_path_cleanup(gc, t, fe_path);
             libxl__xs_path_cleanup(gc, t, libxl_path);
         }
-        if (dev->backend_domid == domid) {
+        if (dev->backend_domid == domid && !libxl_only) {
             /*
              * The driver domain is in charge of removing what it can
              * from the backend path.
diff --git a/tools/libxl/libxl_types_internal.idl b/tools/libxl/libxl_types_internal.idl
index 177f9b7..82e5c07 100644
--- a/tools/libxl/libxl_types_internal.idl
+++ b/tools/libxl/libxl_types_internal.idl
@@ -14,6 +14,7 @@ libxl__qmp_message_type = Enumeration("qmp_message_type", [
     ])
 
 libxl__device_kind = Enumeration("device_kind", [
+    (0, "NONE"),
     (1, "VIF"),
     (2, "VBD"),
     (3, "QDISK"),
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 4982b52..b3bac6d 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -20,8 +20,12 @@
 char **libxl__xs_kvs_of_flexarray(libxl__gc *gc, flexarray_t *array)
 {
     char **kvs;
-    int i, length = array->count;
+    int i, length;
 
+    if (!array)
+        return NULL;
+
+    length = array->count;
     if (!length)
         return NULL;
 
-- 
2.6.6


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

Re: [PATCH 4/6] libxl: add basic support for devices without backend

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:24AM +0200, Juergen Gross wrote:
> With the planned support of HVM USB passthrough via the USB emulation
> capabilities of qemu libxl has to support guest devices which have no
> back- and frontend. Information about those devices will live in the
> libxl part of Xenstore only.
> 
> Add some basic support to libxl to be able to cope with this scenario.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Code-wise, this patch looks good:

Acked-by: Wei Liu <wei.liu2@citrix.com>


>          /*
>           * We make a copy of everything for the backend in the libxl
> @@ -194,6 +205,9 @@ retry_transaction:
>           * instead.  But there are still places in libxl that try to
>           * reconstruct a config from xenstore.
>           *
> +         * For devices without backend (e.g. USB devices emulated via qemu)
> +         * only the libxl path is written.
> +         *

Without PV backend.

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

[PATCH 5/6] libxl: add HVM usb passthrough support

[Juergen Gross]

Add HVM usb passthrough support to libxl by using qemu's capability
to emulate standard USB controllers.

A USB controller is added via qmp command to the emulated hardware
when a usbctrl device of type DEVICEMODEL is requested. Depending on
the requested speed the appropriate hardware type is selected. A host
USB device can then be added to the emulated USB controller via qmp
command.

Removing of the devices is done via qmp commands, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_device.c |   3 +-
 tools/libxl/libxl_usb.c    | 417 +++++++++++++++++++++++++++++++++++----------
 tools/libxl/xl_cmdimpl.c   |   4 +-
 3 files changed, 331 insertions(+), 93 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 5211f20..c6f15db 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -782,8 +782,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
                 aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
                 aodev->dev = dev;
                 aodev->force = drs->force;
-                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
-                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
+                if (dev->kind == LIBXL__DEVICE_KIND_VUSB)
                     libxl__initiate_device_usbctrl_remove(egc, aodev);
                 else
                     libxl__initiate_device_generic_remove(egc, aodev);
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 6b30e0f..40c5a84 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -17,6 +17,7 @@
 
 #include "libxl_internal.h"
 #include <inttypes.h>
+#include <xen/io/usbif.h>
 
 #define USBBACK_INFO_PATH "/libxl/usbback"
 
@@ -43,12 +44,6 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
     int rc;
     libxl_domain_type domtype = libxl__domain_type(gc, domid);
 
-    if (!usbctrl->version)
-        usbctrl->version = 2;
-
-    if (!usbctrl->ports)
-        usbctrl->ports = 8;
-
     if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
         if (domtype == LIBXL_DOMAIN_TYPE_PV) {
             rc = usbback_is_loaded(gc);
@@ -62,6 +57,67 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
         }
     }
 
+    switch (usbctrl->type) {
+    case LIBXL_USBCTRL_TYPE_PV:
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        if (!usbctrl->version)
+            usbctrl->version = 2;
+        if (usbctrl->version < 1 || usbctrl->version > 2) {
+            LOG(ERROR, "USB version for paravirtualized devices must be 1 or 2");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        if (!usbctrl->ports)
+            usbctrl->ports = 8;
+        if (usbctrl->ports < 1 || usbctrl->ports > USBIF_MAX_PORTNR) {
+            LOG(ERROR, "Number of ports for USB controller is limited to %u",
+                USBIF_MAX_PORTNR);
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        break;
+    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        if (!usbctrl->version)
+            usbctrl->version = 2;
+        switch (usbctrl->version) {
+        case 1:
+            /* uhci controller in qemu has fixed number of ports. */
+            if (usbctrl->ports && usbctrl->ports != 2) {
+                LOG(ERROR, "Number of ports for USB controller of version 1 is always 2");
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            usbctrl->ports = 2;
+            break;
+        case 2:
+            /* ehci controller in qemu has fixed number of ports. */
+            if (usbctrl->ports && usbctrl->ports != 6) {
+                LOG(ERROR, "Number of ports for USB controller of version 2 is always 6");
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            usbctrl->ports = 6;
+            break;
+        case 3:
+            if (!usbctrl->ports)
+                usbctrl->ports = 8;
+            /* xhci controller in qemu supports up to 15 ports. */
+            if (usbctrl->ports > 15) {
+                LOG(ERROR, "Number of ports for USB controller of version 3 is limited to 15");
+                rc = ERROR_INVAL;
+                goto out;
+            }
+            break;
+        default:
+            LOG(ERROR, "Illegal USB version");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+        break;
+    default:
+        break;
+    }
+
     rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
                               &usbctrl->backend_domid);
 
@@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
 {
     device->backend_devid   = usbctrl->devid;
     device->backend_domid   = usbctrl->backend_domid;
-    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
-                              ? LIBXL__DEVICE_KIND_VUSB
-                              : LIBXL__DEVICE_KIND_QUSB;
+    switch (usbctrl->type) {
+    case LIBXL_USBCTRL_TYPE_PV:
+        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
+        break;
+    case LIBXL_USBCTRL_TYPE_QUSB:
+        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
+        break;
+    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
+        break;
+    default:
+        break;
+    }
     device->devid           = usbctrl->devid;
     device->domid           = domid;
     device->kind            = LIBXL__DEVICE_KIND_VUSB;
@@ -85,6 +151,31 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+static const char *vusb_be_from_xs_libxl_type(libxl__gc *gc,
+                                              const char *libxl_path,
+                                              libxl_usbctrl_type type)
+{
+    const char *be_path, *tmp;
+    int r;
+
+    if (type == LIBXL_USBCTRL_TYPE_AUTO) {
+        r = libxl__xs_read_checked(gc, XBT_NULL,
+                                   GCSPRINTF("%s/type", libxl_path), &tmp);
+        if (r || libxl_usbctrl_type_from_string(tmp, &type))
+            return NULL;
+    }
+
+    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
+        return libxl_path;
+
+    r = libxl__xs_read_checked(gc, XBT_NULL,
+                               GCSPRINTF("%s/backend", libxl_path),
+                               &be_path);
+    if (r || !be_path) return NULL;
+
+    return be_path;
+}
+
 /* Add usbctrl information to xenstore.
  *
  * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
@@ -96,7 +187,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
                                               bool update_json)
 {
     libxl__device *device;
-    flexarray_t *front;
+    flexarray_t *front = NULL;
     flexarray_t *back;
     xs_transaction_t t = XBT_NULL;
     int i, rc;
@@ -112,13 +203,21 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
     rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
     if (rc) goto out;
 
-    front = flexarray_make(gc, 4, 1);
     back = flexarray_make(gc, 12, 1);
 
-    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
-    flexarray_append_pair(back, "online", "1");
-    flexarray_append_pair(back, "state",
-                          GCSPRINTF("%d", XenbusStateInitialising));
+    if (device->backend_kind != LIBXL__DEVICE_KIND_NONE) {
+        front = flexarray_make(gc, 4, 1);
+
+        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
+        flexarray_append_pair(back, "online", "1");
+        flexarray_append_pair(back, "state",
+                              GCSPRINTF("%d", XenbusStateInitialising));
+        flexarray_append_pair(front, "backend-id",
+                              GCSPRINTF("%d", usbctrl->backend_domid));
+        flexarray_append_pair(front, "state",
+                              GCSPRINTF("%d", XenbusStateInitialising));
+    }
+
     flexarray_append_pair(back, "type",
                           (char *)libxl_usbctrl_type_to_string(usbctrl->type));
     flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", usbctrl->version));
@@ -127,11 +226,6 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
     for (i = 0; i < usbctrl->ports; i++)
         flexarray_append_pair(back, GCSPRINTF("port/%d", i + 1), "");
 
-    flexarray_append_pair(front, "backend-id",
-                          GCSPRINTF("%d", usbctrl->backend_domid));
-    flexarray_append_pair(front, "state",
-                          GCSPRINTF("%d", XenbusStateInitialising));
-
     if (update_json) {
         lock = libxl__lock_domain_userdata(gc, domid);
         if (!lock) {
@@ -194,6 +288,34 @@ out:
     return rc;
 }
 
+static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
+                                               libxl_device_usbctrl *usbctrl)
+{
+    const char *libxl_path, *be_path;
+    xs_transaction_t t = XBT_NULL;
+    int rc;
+
+    libxl_path = GCSPRINTF("%s/device/vusb/%d",
+                           libxl__xs_libxl_path(gc, domid), usbctrl->devid);
+    be_path = vusb_be_from_xs_libxl_type(gc, libxl_path, usbctrl->type);
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+
+        libxl__xs_path_cleanup(gc, t, be_path);
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    return;
+
+out:
+    libxl__xs_transaction_abort(gc, &t);
+}
+
 static char *pvusb_get_device_type(libxl_usbctrl_type type)
 {
     switch (type) {
@@ -206,6 +328,92 @@ static char *pvusb_get_device_type(libxl_usbctrl_type type)
     }
 }
 
+/* Send qmp commands to create a usb controller in qemu.
+ *
+ * Depending on the speed (usbctrl->version) we create:
+ * - piix3-usb-uhci (version=1), always 2 ports
+ * - usb-ehci       (version=2), always 6 ports
+ * - nec-usb-xhci   (version=3), up to 15 ports
+ */
+static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, uint32_t domid,
+                                         libxl_device_usbctrl *usbctrl)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 8, 1);
+
+    switch (usbctrl->version) {
+    case 1:
+        flexarray_append_pair(qmp_args, "driver", "piix3-usb-uhci");
+        break;
+    case 2:
+        flexarray_append_pair(qmp_args, "driver", "usb-ehci");
+        break;
+    case 3:
+        flexarray_append_pair(qmp_args, "driver", "nec-usb-xhci");
+        flexarray_append_pair(qmp_args, "p2", GCSPRINTF("%d", usbctrl->ports));
+        flexarray_append_pair(qmp_args, "p3", GCSPRINTF("%d", usbctrl->ports));
+        break;
+    default:
+        break;
+    }
+
+    flexarray_append_pair(qmp_args, "id",
+                          GCSPRINTF("xenusb-%d", usbctrl->devid));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_add", qmp_args);
+}
+
+/* Send qmp commands to delete a usb controller in qemu.  */
+static int libxl__device_usbctrl_del_hvm(libxl__gc *gc, uint32_t domid,
+                                         int devid)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 2, 1);
+    flexarray_append_pair(qmp_args, "id", GCSPRINTF("xenusb-%d", devid));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_del", qmp_args);
+}
+
+/* Send qmp commands to create a usb device in qemu. */
+static int libxl__device_usbdev_add_hvm(libxl__gc *gc, uint32_t domid,
+                                        libxl_device_usbdev *usbdev)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 12, 1);
+    flexarray_append_pair(qmp_args, "id",
+                          GCSPRINTF("xenusb-%d-%d",
+                                    usbdev->u.hostdev.hostbus,
+                                    usbdev->u.hostdev.hostaddr));
+    flexarray_append_pair(qmp_args, "driver", "usb-host");
+    flexarray_append_pair(qmp_args, "bus",
+                          GCSPRINTF("xenusb-%d.0", usbdev->ctrl));
+    flexarray_append_pair(qmp_args, "port", GCSPRINTF("%d", usbdev->port));
+    flexarray_append_pair(qmp_args, "hostbus",
+                          GCSPRINTF("%d", usbdev->u.hostdev.hostbus));
+    flexarray_append_pair(qmp_args, "hostaddr",
+                          GCSPRINTF("%d", usbdev->u.hostdev.hostaddr));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_add", qmp_args);
+}
+
+/* Send qmp commands to delete a usb device in qemu. */
+static int libxl__device_usbdev_del_hvm(libxl__gc *gc, uint32_t domid,
+                                        libxl_device_usbdev *usbdev)
+{
+    flexarray_t *qmp_args;
+
+    qmp_args = flexarray_make(gc, 2, 1);
+    flexarray_append_pair(qmp_args, "id",
+                          GCSPRINTF("xenusb-%d-%d",
+                                    usbdev->u.hostdev.hostbus,
+                                    usbdev->u.hostdev.hostaddr));
+
+    return libxl__qmp_run_command_flexarray(gc, domid, "device_del", qmp_args);
+}
+
 /* AO operation to add a usb controller.
  *
  * Generally, it does:
@@ -237,26 +445,27 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
         }
     }
 
-    if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
-        usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
-        LOG(ERROR, "Unsupported USB controller type");
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
     rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
                                             aodev->update_json);
     if (rc) goto out;
 
     GCNEW(device);
     rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
-    if (rc) goto out;
+    if (rc) goto outrm;
+
+    if (device->backend_kind == LIBXL__DEVICE_KIND_NONE) {
+        rc = libxl__device_usbctrl_add_hvm(gc, domid, usbctrl);
+        if (rc) goto outrm;
+        goto out;
+    }
 
     aodev->dev = device;
     aodev->action = LIBXL__DEVICE_ACTION_ADD;
     libxl__wait_device_connection(egc, aodev);
     return;
 
+outrm:
+    libxl__device_usbctrl_del_xenstore(gc, domid, usbctrl);
 out:
     aodev->rc = rc;
     aodev->callback(egc, aodev);
@@ -304,13 +513,6 @@ void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
     rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
     if (rc) goto out;
 
-    if (usbctrlinfo.type != LIBXL_USBCTRL_TYPE_PV &&
-        usbctrlinfo.type != LIBXL_USBCTRL_TYPE_QUSB) {
-        LOG(ERROR, "Unsupported USB controller type");
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
     /* Remove usb devices first */
     rc = libxl__device_usbdev_list_for_usbctrl(gc, domid, usbctrl_devid,
                                                &usbdevs, &num_usbdev);
@@ -325,6 +527,13 @@ void libxl__initiate_device_usbctrl_remove(libxl__egc *egc,
         }
     }
 
+    if (usbctrlinfo.type == LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+        rc = libxl__device_usbctrl_del_hvm(gc, domid, usbctrl_devid);
+        if (!rc)
+            libxl__device_usbctrl_del_xenstore(gc, domid, &usbctrl);
+        goto out;
+    }
+
     libxl_device_usbctrl_dispose(&usbctrl);
     libxl_usbctrlinfo_dispose(&usbctrlinfo);
 
@@ -342,15 +551,7 @@ out:
 
 static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
 {
-    const char *be_path;
-    int r;
-
-    r = libxl__xs_read_checked(gc, XBT_NULL,
-                               GCSPRINTF("%s/backend", libxl_path),
-                               &be_path);
-    if (r || !be_path) return NULL;
-
-    return be_path;
+    return vusb_be_from_xs_libxl_type(gc, libxl_path, LIBXL_USBCTRL_TYPE_AUTO);
 }
 
 libxl_device_usbctrl *
@@ -398,15 +599,20 @@ libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
     })
 
             libxl_path = GCSPRINTF("%s/%s", libxl_vusbs_path, *entry);
-            be_path = READ_SUBPATH(libxl_path, "backend");
-            if (!be_path) goto out;
-            ret = libxl__backendpath_parse_domid(gc, be_path,
-                                                &usbctrl->backend_domid);
+            libxl_usbctrl_type_from_string(READ_SUBPATH(libxl_path, "type"),
+                                           &usbctrl->type);
+            if (usbctrl->type == LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+                be_path = libxl_path;
+                ret = libxl__get_domid(gc, &usbctrl->backend_domid);
+            } else {
+                be_path = READ_SUBPATH(libxl_path, "backend");
+                if (!be_path) goto out;
+                ret = libxl__backendpath_parse_domid(gc, be_path,
+                                                     &usbctrl->backend_domid);
+            }
             if (ret) goto out;
             usbctrl->version = READ_SUBPATH_INT(be_path, "usb-ver");
             usbctrl->ports = READ_SUBPATH_INT(be_path, "num-ports");
-            libxl_usbctrl_type_from_string(READ_SUBPATH(libxl_path, "type"),
-                                           &usbctrl->type);
 
 #undef READ_SUBPATH
 #undef READ_SUBPATH_INT
@@ -451,24 +657,33 @@ int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
         tmp ? atoi(tmp) : -1;                                           \
     })
 
-    dompath = libxl__xs_get_dompath(gc, domid);
-    fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, usbctrl->devid);
     libxl_dom_path = libxl__xs_libxl_path(gc, domid);
     libxl_path = GCSPRINTF("%s/device/vusb/%d", libxl_dom_path, usbctrl->devid);
-    be_path = READ_SUBPATH(libxl_path, "backend");
-    usbctrlinfo->backend = libxl__strdup(NOGC, be_path);
-    rc = libxl__backendpath_parse_domid(gc, be_path, &usbctrl->backend_domid);
-    if (rc) goto out;
-    usbctrlinfo->state = READ_SUBPATH_INT(fe_path, "state");
-    usbctrlinfo->evtch = READ_SUBPATH_INT(fe_path, "event-channel");
-    usbctrlinfo->ref_urb = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
-    usbctrlinfo->ref_conn = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
-    usbctrlinfo->frontend = libxl__strdup(NOGC, fe_path);
-    usbctrlinfo->frontend_id = domid;
-    usbctrlinfo->ports = READ_SUBPATH_INT(be_path, "num-ports");
-    usbctrlinfo->version = READ_SUBPATH_INT(be_path, "usb-ver");;
-    tmp = READ_SUBPATH(libxl_path, "type");
-    libxl_usbctrl_type_from_string(tmp, &usbctrlinfo->type);
+    libxl_usbctrl_type_from_string(READ_SUBPATH(libxl_path, "type"),
+                                   &usbctrlinfo->type);
+
+    if (usbctrlinfo->type != LIBXL_USBCTRL_TYPE_DEVICEMODEL) {
+        dompath = libxl__xs_get_dompath(gc, domid);
+        fe_path = GCSPRINTF("%s/device/vusb/%d", dompath, usbctrl->devid);
+        be_path = READ_SUBPATH(libxl_path, "backend");
+        usbctrlinfo->backend = libxl__strdup(NOGC, be_path);
+        rc = libxl__backendpath_parse_domid(gc, be_path,
+                                            &usbctrl->backend_domid);
+        if (rc) goto out;
+        usbctrlinfo->state = READ_SUBPATH_INT(fe_path, "state");
+        usbctrlinfo->evtch = READ_SUBPATH_INT(fe_path, "event-channel");
+        usbctrlinfo->ref_urb = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
+        usbctrlinfo->ref_conn = READ_SUBPATH_INT(fe_path, "urb-ring-ref");
+        usbctrlinfo->frontend = libxl__strdup(NOGC, fe_path);
+        usbctrlinfo->frontend_id = domid;
+        usbctrlinfo->ports = READ_SUBPATH_INT(be_path, "num-ports");
+        usbctrlinfo->version = READ_SUBPATH_INT(be_path, "usb-ver");
+    } else {
+        usbctrlinfo->ports = READ_SUBPATH_INT(libxl_path, "num-ports");
+        usbctrlinfo->version = READ_SUBPATH_INT(libxl_path, "usb-ver");
+        rc = libxl__get_domid(gc, &usbctrl->backend_domid);
+        if (rc) goto out;
+    }
 
 #undef READ_SUBPATH
 #undef READ_SUBPATH_INT
@@ -760,6 +975,21 @@ libxl_device_usbdev_list(libxl_ctx *ctx, uint32_t domid, int *num)
     return usbdevs;
 }
 
+static char *pvusb_get_port_path(libxl__gc *gc, uint32_t domid,
+                                 libxl_usbctrl_type type, int ctrl, int port)
+{
+    char *path;
+
+    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
+        path = GCSPRINTF("%s/device/vusb", libxl__xs_libxl_path(gc, domid));
+    else
+        path = GCSPRINTF("%s/backend/%s/%d",
+                         libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+                         pvusb_get_device_type(type), domid);
+
+    return GCSPRINTF("%s/%d/port/%d", path, ctrl, port);
+}
+
 /* find first unused controller:port and give that to usb device */
 static int
 libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
@@ -779,10 +1009,8 @@ libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
         for (j = 0; j < usbctrls[i].ports; j++) {
             const char *path, *tmp;
 
-            path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                             libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                             pvusb_get_device_type(usbctrls[i].type),
-                             domid, usbctrls[i].devid, j + 1);
+            path = pvusb_get_port_path(gc, domid, usbctrls[i].type,
+                                       usbctrls[i].devid, j + 1);
             rc = libxl__xs_read_checked(gc, XBT_NULL, path, &tmp);
             if (rc) goto out;
 
@@ -851,13 +1079,6 @@ static int libxl__device_usbdev_setdefault(libxl__gc *gc,
                 }
             }
 
-            if (usbctrl->type != LIBXL_USBCTRL_TYPE_PV &&
-                usbctrl->type != LIBXL_USBCTRL_TYPE_QUSB) {
-                LOG(ERROR, "Unsupported USB controller type");
-                rc = ERROR_FAIL;
-                goto out;
-            }
-
             rc = libxl__device_usbctrl_add_xenstore(gc, domid, usbctrl,
                                                     update_json);
             if (rc) goto out;
@@ -983,10 +1204,8 @@ static int libxl__device_usbdev_add_xenstore(libxl__gc *gc, uint32_t domid,
             if (rc) goto out;
         }
 
-        be_path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                            libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                            pvusb_get_device_type(type),
-                            domid, usbdev->ctrl, usbdev->port);
+        be_path = pvusb_get_port_path(gc, domid, type, usbdev->ctrl,
+                                      usbdev->port);
 
         LOG(DEBUG, "Adding usb device %s to xenstore: controller %d, port %d",
             busid, usbdev->ctrl, usbdev->port);
@@ -1014,10 +1233,7 @@ static int libxl__device_usbdev_remove_xenstore(libxl__gc *gc, uint32_t domid,
 {
     char *be_path;
 
-    be_path = GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                        libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                        pvusb_get_device_type(type),
-                        domid, usbdev->ctrl, usbdev->port);
+    be_path = pvusb_get_port_path(gc, domid, type, usbdev->ctrl, usbdev->port);
 
     LOG(DEBUG, "Removing usb device from xenstore: controller %d, port %d",
         usbdev->ctrl, usbdev->port);
@@ -1030,10 +1246,8 @@ static char *usbdev_busid_from_ctrlport(libxl__gc *gc, uint32_t domid,
                                         libxl_usbctrl_type type)
 {
     return libxl__xs_read(gc, XBT_NULL,
-                          GCSPRINTF("%s/backend/%s/%d/%d/port/%d",
-                              libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
-                              pvusb_get_device_type(type),
-                              domid, usbdev->ctrl, usbdev->port));
+                          pvusb_get_port_path(gc, domid, type, usbdev->ctrl,
+                                              usbdev->port));
 }
 
 /* get original driver path of usb interface, stored in @drvpath */
@@ -1397,6 +1611,18 @@ static int do_usbdev_add(libxl__gc *gc, uint32_t domid,
 
         break;
     case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        rc = libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
+                                               LIBXL_USBCTRL_TYPE_DEVICEMODEL,
+                                               update_json);
+        if (rc) goto out;
+
+        rc = libxl__device_usbdev_add_hvm(gc, domid, usbdev);
+        if (rc) {
+            libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                             LIBXL_USBCTRL_TYPE_DEVICEMODEL);
+            goto out;
+        }
+        break;
     default:
         LOG(ERROR, "Unsupported usb controller type");
         rc = ERROR_FAIL;
@@ -1564,6 +1790,19 @@ static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
 
         break;
     case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev,
+                                              LIBXL_USBCTRL_TYPE_DEVICEMODEL);
+        if (rc) goto out;
+
+        rc = libxl__device_usbdev_del_hvm(gc, domid, usbdev);
+        if (rc) {
+            libxl__device_usbdev_add_xenstore(gc, domid, usbdev,
+                                              LIBXL_USBCTRL_TYPE_DEVICEMODEL,
+                                              false);
+            goto out;
+        }
+
+        break;
     default:
         LOG(ERROR, "Unsupported usb controller type");
         rc = ERROR_FAIL;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 62237d0..cb43c00 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3728,14 +3728,14 @@ int main_usblist(int argc, char **argv)
     }
 
     for (i = 0; i < numctrl; ++i) {
-        printf("%-6s %-6s %-3s %-5s %-7s %-5s\n",
+        printf("%-6s %-12s %-3s %-5s %-7s %-5s\n",
                 "Devid", "Type", "BE", "state", "usb-ver", "ports");
 
         libxl_usbctrlinfo_init(&usbctrlinfo);
 
         if (!libxl_device_usbctrl_getinfo(ctx, domid,
                                 &usbctrls[i], &usbctrlinfo)) {
-            printf("%-6d %-6s %-3d %-5d %-7d %-5d\n",
+            printf("%-6d %-12s %-3d %-5d %-7d %-5d\n",
                     usbctrlinfo.devid,
                     libxl_usbctrl_type_to_string(usbctrlinfo.type),
                     usbctrlinfo.backend_id, usbctrlinfo.state,
-- 
2.6.6


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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
> Add HVM usb passthrough support to libxl by using qemu's capability
> to emulate standard USB controllers.
> 
> A USB controller is added via qmp command to the emulated hardware
> when a usbctrl device of type DEVICEMODEL is requested. Depending on
> the requested speed the appropriate hardware type is selected. A host
> USB device can then be added to the emulated USB controller via qmp
> command.
> 
> Removing of the devices is done via qmp commands, too.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
[...]
>  
> @@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>  {
>      device->backend_devid   = usbctrl->devid;
>      device->backend_domid   = usbctrl->backend_domid;
> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
> -                              ? LIBXL__DEVICE_KIND_VUSB
> -                              : LIBXL__DEVICE_KIND_QUSB;
> +    switch (usbctrl->type) {
> +    case LIBXL_USBCTRL_TYPE_PV:
> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
> +        break;
> +    case LIBXL_USBCTRL_TYPE_QUSB:
> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
> +        break;
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;

I'm not quite sure if I follow this. Shouldn't we need a new kind of
backend for this?

Wei.

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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Juergen Gross]

On 09/09/16 12:00, Wei Liu wrote:
> On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
>> Add HVM usb passthrough support to libxl by using qemu's capability
>> to emulate standard USB controllers.
>>
>> A USB controller is added via qmp command to the emulated hardware
>> when a usbctrl device of type DEVICEMODEL is requested. Depending on
>> the requested speed the appropriate hardware type is selected. A host
>> USB device can then be added to the emulated USB controller via qmp
>> command.
>>
>> Removing of the devices is done via qmp commands, too.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> [...]
>>  
>> @@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>>  {
>>      device->backend_devid   = usbctrl->devid;
>>      device->backend_domid   = usbctrl->backend_domid;
>> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
>> -                              ? LIBXL__DEVICE_KIND_VUSB
>> -                              : LIBXL__DEVICE_KIND_QUSB;
>> +    switch (usbctrl->type) {
>> +    case LIBXL_USBCTRL_TYPE_PV:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_QUSB:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
> 
> I'm not quite sure if I follow this. Shouldn't we need a new kind of
> backend for this?

Depends on what do you call a backend.

I'd say a backend is something which corresponds to a frontend in the
guest. As long as you don't want to call a normal hardware driver a
"frontend" we don't need a backend.

At least the "backend" (qemu, requiring no update for this) won't use
Xenstore for detecting addition/removal of devices: all is done via
explicit qmp commands issued from libxl to qemu.


Juergen

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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Wei Liu]

On Fri, Sep 09, 2016 at 12:13:46PM +0200, Juergen Gross wrote:
> On 09/09/16 12:00, Wei Liu wrote:
> > On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
> >> Add HVM usb passthrough support to libxl by using qemu's capability
> >> to emulate standard USB controllers.
> >>
> >> A USB controller is added via qmp command to the emulated hardware
> >> when a usbctrl device of type DEVICEMODEL is requested. Depending on
> >> the requested speed the appropriate hardware type is selected. A host
> >> USB device can then be added to the emulated USB controller via qmp
> >> command.
> >>
> >> Removing of the devices is done via qmp commands, too.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > [...]
> >>  
> >> @@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> >>  {
> >>      device->backend_devid   = usbctrl->devid;
> >>      device->backend_domid   = usbctrl->backend_domid;
> >> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
> >> -                              ? LIBXL__DEVICE_KIND_VUSB
> >> -                              : LIBXL__DEVICE_KIND_QUSB;
> >> +    switch (usbctrl->type) {
> >> +    case LIBXL_USBCTRL_TYPE_PV:
> >> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
> >> +        break;
> >> +    case LIBXL_USBCTRL_TYPE_QUSB:
> >> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
> >> +        break;
> >> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> >> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
> > 
> > I'm not quite sure if I follow this. Shouldn't we need a new kind of
> > backend for this?
> 
> Depends on what do you call a backend.
> 
> I'd say a backend is something which corresponds to a frontend in the
> guest. As long as you don't want to call a normal hardware driver a
> "frontend" we don't need a backend.
> 
> At least the "backend" (qemu, requiring no update for this) won't use
> Xenstore for detecting addition/removal of devices: all is done via
> explicit qmp commands issued from libxl to qemu.
> 

After having a closer look, this is internal detail of libxl so I don't
think I care that much.

I will review this patch in detail later.

Wei.

> 
> Juergen

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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[George Dunlap]

On 09/09/16 11:00, Wei Liu wrote:
> On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
>> Add HVM usb passthrough support to libxl by using qemu's capability
>> to emulate standard USB controllers.
>>
>> A USB controller is added via qmp command to the emulated hardware
>> when a usbctrl device of type DEVICEMODEL is requested. Depending on
>> the requested speed the appropriate hardware type is selected. A host
>> USB device can then be added to the emulated USB controller via qmp
>> command.
>>
>> Removing of the devices is done via qmp commands, too.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> [...]
>>  
>> @@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>>  {
>>      device->backend_devid   = usbctrl->devid;
>>      device->backend_domid   = usbctrl->backend_domid;
>> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
>> -                              ? LIBXL__DEVICE_KIND_VUSB
>> -                              : LIBXL__DEVICE_KIND_QUSB;
>> +    switch (usbctrl->type) {
>> +    case LIBXL_USBCTRL_TYPE_PV:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_QUSB:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
> 
> I'm not quite sure if I follow this. Shouldn't we need a new kind of
> backend for this?

If you look at the previous patch, Jueren used "backend_kind == NONE"
specifically to mean libxl shouldn't do any messing around with setting
up or tearing down backends.  If we don't use "NONE", then we have to
special-case all the controller types that don't need backends.
Juergen's way seems reasonable (having only taken a quick look at it).

 -George


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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
> Add HVM usb passthrough support to libxl by using qemu's capability
> to emulate standard USB controllers.
> 
> A USB controller is added via qmp command to the emulated hardware
> when a usbctrl device of type DEVICEMODEL is requested. Depending on
> the requested speed the appropriate hardware type is selected. A host
> USB device can then be added to the emulated USB controller via qmp
> command.
> 
> Removing of the devices is done via qmp commands, too.

Overall the code looks plausible. But the code seems to do more than
what is stated in commit message. Some comments below.

> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  tools/libxl/libxl_device.c |   3 +-
>  tools/libxl/libxl_usb.c    | 417 +++++++++++++++++++++++++++++++++++----------
>  tools/libxl/xl_cmdimpl.c   |   4 +-
>  3 files changed, 331 insertions(+), 93 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 5211f20..c6f15db 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -782,8 +782,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
>                  aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
>                  aodev->dev = dev;
>                  aodev->force = drs->force;
> -                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
> -                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
> +                if (dev->kind == LIBXL__DEVICE_KIND_VUSB)

This looks a bit suspicious to me. This could be rather obvious but I'm
not sure because I didn't follow closely on the overall design of PV /
HVM USB passthrough.

AIUI:

VUSB -> PV USB with in-kernel backend
QUSB -> PV USB with QEMU backend

Why is QUSB deleted here?

If there is refactoring involved, can that be separated out?

>                      libxl__initiate_device_usbctrl_remove(egc, aodev);
>                  else
>                      libxl__initiate_device_generic_remove(egc, aodev);
> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
> index 6b30e0f..40c5a84 100644
> --- a/tools/libxl/libxl_usb.c
> +++ b/tools/libxl/libxl_usb.c
> @@ -17,6 +17,7 @@
>  
>  #include "libxl_internal.h"
>  #include <inttypes.h>
> +#include <xen/io/usbif.h>
>  
>  #define USBBACK_INFO_PATH "/libxl/usbback"
>  
> @@ -43,12 +44,6 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>      int rc;
>      libxl_domain_type domtype = libxl__domain_type(gc, domid);
>  
> -    if (!usbctrl->version)
> -        usbctrl->version = 2;
> -
> -    if (!usbctrl->ports)
> -        usbctrl->ports = 8;
> -
>      if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
>          if (domtype == LIBXL_DOMAIN_TYPE_PV) {
>              rc = usbback_is_loaded(gc);
> @@ -62,6 +57,67 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>          }
>      }
>  
> +    switch (usbctrl->type) {
> +    case LIBXL_USBCTRL_TYPE_PV:
> +    case LIBXL_USBCTRL_TYPE_QUSB:
> +        if (!usbctrl->version)
> +            usbctrl->version = 2;
> +        if (usbctrl->version < 1 || usbctrl->version > 2) {
> +            LOG(ERROR, "USB version for paravirtualized devices must be 1 or 2");
> +            rc = ERROR_INVAL;
> +            goto out;
> +        }
> +        if (!usbctrl->ports)
> +            usbctrl->ports = 8;
> +        if (usbctrl->ports < 1 || usbctrl->ports > USBIF_MAX_PORTNR) {
> +            LOG(ERROR, "Number of ports for USB controller is limited to %u",
> +                USBIF_MAX_PORTNR);
> +            rc = ERROR_INVAL;
> +            goto out;
> +        }
> +        break;
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        if (!usbctrl->version)
> +            usbctrl->version = 2;
> +        switch (usbctrl->version) {
> +        case 1:
> +            /* uhci controller in qemu has fixed number of ports. */
> +            if (usbctrl->ports && usbctrl->ports != 2) {
> +                LOG(ERROR, "Number of ports for USB controller of version 1 is always 2");

Please wrap long line if possible.

> +                rc = ERROR_INVAL;
> +                goto out;
> +            }
> +            usbctrl->ports = 2;
> +            break;
> +        case 2:
> +            /* ehci controller in qemu has fixed number of ports. */
> +            if (usbctrl->ports && usbctrl->ports != 6) {
> +                LOG(ERROR, "Number of ports for USB controller of version 2 is always 6");
> +                rc = ERROR_INVAL;
> +                goto out;
> +            }
> +            usbctrl->ports = 6;
> +            break;
> +        case 3:
> +            if (!usbctrl->ports)
> +                usbctrl->ports = 8;
> +            /* xhci controller in qemu supports up to 15 ports. */
> +            if (usbctrl->ports > 15) {
> +                LOG(ERROR, "Number of ports for USB controller of version 3 is limited to 15");
> +                rc = ERROR_INVAL;
> +                goto out;
> +            }
> +            break;
> +        default:
> +            LOG(ERROR, "Illegal USB version");
> +            rc = ERROR_INVAL;
> +            goto out;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +
>      rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
>                                &usbctrl->backend_domid);
>  
> @@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>  {
>      device->backend_devid   = usbctrl->devid;
>      device->backend_domid   = usbctrl->backend_domid;
> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
> -                              ? LIBXL__DEVICE_KIND_VUSB
> -                              : LIBXL__DEVICE_KIND_QUSB;
> +    switch (usbctrl->type) {
> +    case LIBXL_USBCTRL_TYPE_PV:
> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
> +        break;
> +    case LIBXL_USBCTRL_TYPE_QUSB:
> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
> +        break;
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
> +        break;
> +    default:
> +        break;

Shouldn't we return some sort of error here?

> +    }
>      device->devid           = usbctrl->devid;
>      device->domid           = domid;
>      device->kind            = LIBXL__DEVICE_KIND_VUSB;
> @@ -85,6 +151,31 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>      return 0;
>  }
>  
> +static const char *vusb_be_from_xs_libxl_type(libxl__gc *gc,
> +                                              const char *libxl_path,
> +                                              libxl_usbctrl_type type)
> +{
> +    const char *be_path, *tmp;
> +    int r;
> +
> +    if (type == LIBXL_USBCTRL_TYPE_AUTO) {
> +        r = libxl__xs_read_checked(gc, XBT_NULL,
> +                                   GCSPRINTF("%s/type", libxl_path), &tmp);
> +        if (r || libxl_usbctrl_type_from_string(tmp, &type))
> +            return NULL;
> +    }
> +
> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
> +        return libxl_path;
> +
> +    r = libxl__xs_read_checked(gc, XBT_NULL,
> +                               GCSPRINTF("%s/backend", libxl_path),
> +                               &be_path);
> +    if (r || !be_path) return NULL;
> +
> +    return be_path;

Please use goto style error handling.

> +}
> +
>  /* Add usbctrl information to xenstore.
>   *
>   * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
> @@ -96,7 +187,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>                                                bool update_json)
>  {
>      libxl__device *device;
> -    flexarray_t *front;
> +    flexarray_t *front = NULL;
>      flexarray_t *back;
>      xs_transaction_t t = XBT_NULL;
>      int i, rc;
> @@ -112,13 +203,21 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>      if (rc) goto out;
>  
> -    front = flexarray_make(gc, 4, 1);
>      back = flexarray_make(gc, 12, 1);
>  
> -    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> -    flexarray_append_pair(back, "online", "1");
> -    flexarray_append_pair(back, "state",
> -                          GCSPRINTF("%d", XenbusStateInitialising));
> +    if (device->backend_kind != LIBXL__DEVICE_KIND_NONE) {
> +        front = flexarray_make(gc, 4, 1);
> +
> +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> +        flexarray_append_pair(back, "online", "1");
> +        flexarray_append_pair(back, "state",
> +                              GCSPRINTF("%d", XenbusStateInitialising));
> +        flexarray_append_pair(front, "backend-id",
> +                              GCSPRINTF("%d", usbctrl->backend_domid));
> +        flexarray_append_pair(front, "state",
> +                              GCSPRINTF("%d", XenbusStateInitialising));
> +    }
> +
>      flexarray_append_pair(back, "type",
>                            (char *)libxl_usbctrl_type_to_string(usbctrl->type));
>      flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", usbctrl->version));
> @@ -127,11 +226,6 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>      for (i = 0; i < usbctrl->ports; i++)
>          flexarray_append_pair(back, GCSPRINTF("port/%d", i + 1), "");
>  
> -    flexarray_append_pair(front, "backend-id",
> -                          GCSPRINTF("%d", usbctrl->backend_domid));
> -    flexarray_append_pair(front, "state",
> -                          GCSPRINTF("%d", XenbusStateInitialising));
> -
>      if (update_json) {
>          lock = libxl__lock_domain_userdata(gc, domid);
>          if (!lock) {
> @@ -194,6 +288,34 @@ out:
>      return rc;
>  }
>  
> +static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
> +                                               libxl_device_usbctrl *usbctrl)
> +{
> +    const char *libxl_path, *be_path;
> +    xs_transaction_t t = XBT_NULL;
> +    int rc;
> +
> +    libxl_path = GCSPRINTF("%s/device/vusb/%d",
> +                           libxl__xs_libxl_path(gc, domid), usbctrl->devid);
> +    be_path = vusb_be_from_xs_libxl_type(gc, libxl_path, usbctrl->type);
> +
> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        libxl__xs_path_cleanup(gc, t, be_path);
> +
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
> +
> +    return;
> +
> +out:
> +    libxl__xs_transaction_abort(gc, &t);
> +}
> +

It seems that the need for this function is because we didn't add one in
previous version. If that's the case, is it possible to have a separate
patch for it so that we can backport it?

>  static char *pvusb_get_device_type(libxl_usbctrl_type type)
>  {
>      switch (type) {
> @@ -206,6 +328,92 @@ static char *pvusb_get_device_type(libxl_usbctrl_type type)
>      }
>  }
>  
> +/* Send qmp commands to create a usb controller in qemu.
> + *
> + * Depending on the speed (usbctrl->version) we create:
> + * - piix3-usb-uhci (version=1), always 2 ports
> + * - usb-ehci       (version=2), always 6 ports
> + * - nec-usb-xhci   (version=3), up to 15 ports
> + */
> +static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, uint32_t domid,
> +                                         libxl_device_usbctrl *usbctrl)
> +{
> +    flexarray_t *qmp_args;
> +
> +    qmp_args = flexarray_make(gc, 8, 1);
> +
> +    switch (usbctrl->version) {
> +    case 1:
> +        flexarray_append_pair(qmp_args, "driver", "piix3-usb-uhci");
> +        break;
> +    case 2:
> +        flexarray_append_pair(qmp_args, "driver", "usb-ehci");
> +        break;
> +    case 3:
> +        flexarray_append_pair(qmp_args, "driver", "nec-usb-xhci");
> +        flexarray_append_pair(qmp_args, "p2", GCSPRINTF("%d", usbctrl->ports));
> +        flexarray_append_pair(qmp_args, "p3", GCSPRINTF("%d", usbctrl->ports));
> +        break;
> +    default:
> +        break;

Return ERROR_INVAL?

Wei.

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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Juergen Groß]

On 09/15/2016 05:38 PM, Wei Liu wrote:
> On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
>> Add HVM usb passthrough support to libxl by using qemu's capability
>> to emulate standard USB controllers.
>>
>> A USB controller is added via qmp command to the emulated hardware
>> when a usbctrl device of type DEVICEMODEL is requested. Depending on
>> the requested speed the appropriate hardware type is selected. A host
>> USB device can then be added to the emulated USB controller via qmp
>> command.
>>
>> Removing of the devices is done via qmp commands, too.
>
> Overall the code looks plausible. But the code seems to do more than
> what is stated in commit message. Some comments below.

Thanks. I'm just travelling, so my answers are based on my memory what I
did in the patch. I'll double check before sending out V2.

>
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  tools/libxl/libxl_device.c |   3 +-
>>  tools/libxl/libxl_usb.c    | 417 +++++++++++++++++++++++++++++++++++----------
>>  tools/libxl/xl_cmdimpl.c   |   4 +-
>>  3 files changed, 331 insertions(+), 93 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 5211f20..c6f15db 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -782,8 +782,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
>>                  aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
>>                  aodev->dev = dev;
>>                  aodev->force = drs->force;
>> -                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
>> -                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
>> +                if (dev->kind == LIBXL__DEVICE_KIND_VUSB)
>
> This looks a bit suspicious to me. This could be rather obvious but I'm
> not sure because I didn't follow closely on the overall design of PV /
> HVM USB passthrough.
>
> AIUI:
>
> VUSB -> PV USB with in-kernel backend
> QUSB -> PV USB with QEMU backend
>
> Why is QUSB deleted here?

It isn't. :-)

A USB passthrough device will always be of kind == VUSB. The backend
kind may differ, though. This is to ensure a proper device numbering
regardless of the backend type used.

> If there is refactoring involved, can that be separated out?
>
>>                      libxl__initiate_device_usbctrl_remove(egc, aodev);
>>                  else
>>                      libxl__initiate_device_generic_remove(egc, aodev);
>> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
>> index 6b30e0f..40c5a84 100644
>> --- a/tools/libxl/libxl_usb.c
>> +++ b/tools/libxl/libxl_usb.c
>> @@ -17,6 +17,7 @@
>>
>>  #include "libxl_internal.h"
>>  #include <inttypes.h>
>> +#include <xen/io/usbif.h>
>>
>>  #define USBBACK_INFO_PATH "/libxl/usbback"
>>
>> @@ -43,12 +44,6 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>>      int rc;
>>      libxl_domain_type domtype = libxl__domain_type(gc, domid);
>>
>> -    if (!usbctrl->version)
>> -        usbctrl->version = 2;
>> -
>> -    if (!usbctrl->ports)
>> -        usbctrl->ports = 8;
>> -
>>      if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
>>          if (domtype == LIBXL_DOMAIN_TYPE_PV) {
>>              rc = usbback_is_loaded(gc);
>> @@ -62,6 +57,67 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
>>          }
>>      }
>>
>> +    switch (usbctrl->type) {
>> +    case LIBXL_USBCTRL_TYPE_PV:
>> +    case LIBXL_USBCTRL_TYPE_QUSB:
>> +        if (!usbctrl->version)
>> +            usbctrl->version = 2;
>> +        if (usbctrl->version < 1 || usbctrl->version > 2) {
>> +            LOG(ERROR, "USB version for paravirtualized devices must be 1 or 2");
>> +            rc = ERROR_INVAL;
>> +            goto out;
>> +        }
>> +        if (!usbctrl->ports)
>> +            usbctrl->ports = 8;
>> +        if (usbctrl->ports < 1 || usbctrl->ports > USBIF_MAX_PORTNR) {
>> +            LOG(ERROR, "Number of ports for USB controller is limited to %u",
>> +                USBIF_MAX_PORTNR);
>> +            rc = ERROR_INVAL;
>> +            goto out;
>> +        }
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
>> +        if (!usbctrl->version)
>> +            usbctrl->version = 2;
>> +        switch (usbctrl->version) {
>> +        case 1:
>> +            /* uhci controller in qemu has fixed number of ports. */
>> +            if (usbctrl->ports && usbctrl->ports != 2) {
>> +                LOG(ERROR, "Number of ports for USB controller of version 1 is always 2");
>
> Please wrap long line if possible.

Okay.

>
>> +                rc = ERROR_INVAL;
>> +                goto out;
>> +            }
>> +            usbctrl->ports = 2;
>> +            break;
>> +        case 2:
>> +            /* ehci controller in qemu has fixed number of ports. */
>> +            if (usbctrl->ports && usbctrl->ports != 6) {
>> +                LOG(ERROR, "Number of ports for USB controller of version 2 is always 6");
>> +                rc = ERROR_INVAL;
>> +                goto out;
>> +            }
>> +            usbctrl->ports = 6;
>> +            break;
>> +        case 3:
>> +            if (!usbctrl->ports)
>> +                usbctrl->ports = 8;
>> +            /* xhci controller in qemu supports up to 15 ports. */
>> +            if (usbctrl->ports > 15) {
>> +                LOG(ERROR, "Number of ports for USB controller of version 3 is limited to 15");
>> +                rc = ERROR_INVAL;
>> +                goto out;
>> +            }
>> +            break;
>> +        default:
>> +            LOG(ERROR, "Illegal USB version");
>> +            rc = ERROR_INVAL;
>> +            goto out;
>> +        }
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>>      rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
>>                                &usbctrl->backend_domid);
>>
>> @@ -75,9 +131,19 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>>  {
>>      device->backend_devid   = usbctrl->devid;
>>      device->backend_domid   = usbctrl->backend_domid;
>> -    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
>> -                              ? LIBXL__DEVICE_KIND_VUSB
>> -                              : LIBXL__DEVICE_KIND_QUSB;
>> +    switch (usbctrl->type) {
>> +    case LIBXL_USBCTRL_TYPE_PV:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_QUSB:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
>> +        break;
>> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
>> +        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
>> +        break;
>> +    default:
>> +        break;
>
> Shouldn't we return some sort of error here?

This case should not be possible.
Are you okay with me adding an assert() here?

>> +    }
>>      device->devid           = usbctrl->devid;
>>      device->domid           = domid;
>>      device->kind            = LIBXL__DEVICE_KIND_VUSB;
>> @@ -85,6 +151,31 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
>>      return 0;
>>  }
>>
>> +static const char *vusb_be_from_xs_libxl_type(libxl__gc *gc,
>> +                                              const char *libxl_path,
>> +                                              libxl_usbctrl_type type)
>> +{
>> +    const char *be_path, *tmp;
>> +    int r;
>> +
>> +    if (type == LIBXL_USBCTRL_TYPE_AUTO) {
>> +        r = libxl__xs_read_checked(gc, XBT_NULL,
>> +                                   GCSPRINTF("%s/type", libxl_path), &tmp);
>> +        if (r || libxl_usbctrl_type_from_string(tmp, &type))
>> +            return NULL;
>> +    }
>> +
>> +    if (type == LIBXL_USBCTRL_TYPE_DEVICEMODEL)
>> +        return libxl_path;
>> +
>> +    r = libxl__xs_read_checked(gc, XBT_NULL,
>> +                               GCSPRINTF("%s/backend", libxl_path),
>> +                               &be_path);
>> +    if (r || !be_path) return NULL;
>> +
>> +    return be_path;
>
> Please use goto style error handling.

Okay.

>
>> +}
>> +
>>  /* Add usbctrl information to xenstore.
>>   *
>>   * Adding a usb controller will add a new 'qusb' or 'vusb' device in xenstore,
>> @@ -96,7 +187,7 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>>                                                bool update_json)
>>  {
>>      libxl__device *device;
>> -    flexarray_t *front;
>> +    flexarray_t *front = NULL;
>>      flexarray_t *back;
>>      xs_transaction_t t = XBT_NULL;
>>      int i, rc;
>> @@ -112,13 +203,21 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>>      rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
>>      if (rc) goto out;
>>
>> -    front = flexarray_make(gc, 4, 1);
>>      back = flexarray_make(gc, 12, 1);
>>
>> -    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
>> -    flexarray_append_pair(back, "online", "1");
>> -    flexarray_append_pair(back, "state",
>> -                          GCSPRINTF("%d", XenbusStateInitialising));
>> +    if (device->backend_kind != LIBXL__DEVICE_KIND_NONE) {
>> +        front = flexarray_make(gc, 4, 1);
>> +
>> +        flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
>> +        flexarray_append_pair(back, "online", "1");
>> +        flexarray_append_pair(back, "state",
>> +                              GCSPRINTF("%d", XenbusStateInitialising));
>> +        flexarray_append_pair(front, "backend-id",
>> +                              GCSPRINTF("%d", usbctrl->backend_domid));
>> +        flexarray_append_pair(front, "state",
>> +                              GCSPRINTF("%d", XenbusStateInitialising));
>> +    }
>> +
>>      flexarray_append_pair(back, "type",
>>                            (char *)libxl_usbctrl_type_to_string(usbctrl->type));
>>      flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", usbctrl->version));
>> @@ -127,11 +226,6 @@ static int libxl__device_usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
>>      for (i = 0; i < usbctrl->ports; i++)
>>          flexarray_append_pair(back, GCSPRINTF("port/%d", i + 1), "");
>>
>> -    flexarray_append_pair(front, "backend-id",
>> -                          GCSPRINTF("%d", usbctrl->backend_domid));
>> -    flexarray_append_pair(front, "state",
>> -                          GCSPRINTF("%d", XenbusStateInitialising));
>> -
>>      if (update_json) {
>>          lock = libxl__lock_domain_userdata(gc, domid);
>>          if (!lock) {
>> @@ -194,6 +288,34 @@ out:
>>      return rc;
>>  }
>>
>> +static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
>> +                                               libxl_device_usbctrl *usbctrl)
>> +{
>> +    const char *libxl_path, *be_path;
>> +    xs_transaction_t t = XBT_NULL;
>> +    int rc;
>> +
>> +    libxl_path = GCSPRINTF("%s/device/vusb/%d",
>> +                           libxl__xs_libxl_path(gc, domid), usbctrl->devid);
>> +    be_path = vusb_be_from_xs_libxl_type(gc, libxl_path, usbctrl->type);
>> +
>> +    for (;;) {
>> +        rc = libxl__xs_transaction_start(gc, &t);
>> +        if (rc) goto out;
>> +
>> +        libxl__xs_path_cleanup(gc, t, be_path);
>> +
>> +        rc = libxl__xs_transaction_commit(gc, &t);
>> +        if (!rc) break;
>> +        if (rc < 0) goto out;
>> +    }
>> +
>> +    return;
>> +
>> +out:
>> +    libxl__xs_transaction_abort(gc, &t);
>> +}
>> +
>
> It seems that the need for this function is because we didn't add one in
> previous version. If that's the case, is it possible to have a separate
> patch for it so that we can backport it?

Okay.

>
>>  static char *pvusb_get_device_type(libxl_usbctrl_type type)
>>  {
>>      switch (type) {
>> @@ -206,6 +328,92 @@ static char *pvusb_get_device_type(libxl_usbctrl_type type)
>>      }
>>  }
>>
>> +/* Send qmp commands to create a usb controller in qemu.
>> + *
>> + * Depending on the speed (usbctrl->version) we create:
>> + * - piix3-usb-uhci (version=1), always 2 ports
>> + * - usb-ehci       (version=2), always 6 ports
>> + * - nec-usb-xhci   (version=3), up to 15 ports
>> + */
>> +static int libxl__device_usbctrl_add_hvm(libxl__gc *gc, uint32_t domid,
>> +                                         libxl_device_usbctrl *usbctrl)
>> +{
>> +    flexarray_t *qmp_args;
>> +
>> +    qmp_args = flexarray_make(gc, 8, 1);
>> +
>> +    switch (usbctrl->version) {
>> +    case 1:
>> +        flexarray_append_pair(qmp_args, "driver", "piix3-usb-uhci");
>> +        break;
>> +    case 2:
>> +        flexarray_append_pair(qmp_args, "driver", "usb-ehci");
>> +        break;
>> +    case 3:
>> +        flexarray_append_pair(qmp_args, "driver", "nec-usb-xhci");
>> +        flexarray_append_pair(qmp_args, "p2", GCSPRINTF("%d", usbctrl->ports));
>> +        flexarray_append_pair(qmp_args, "p3", GCSPRINTF("%d", usbctrl->ports));
>> +        break;
>> +    default:
>> +        break;
>
> Return ERROR_INVAL?

Should not be possible. I think I'll go with assert() again.


Juergen

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

Re: [PATCH 5/6] libxl: add HVM usb passthrough support

[Wei Liu]

On Fri, Sep 16, 2016 at 10:51:18AM +0200, Juergen Groß wrote:
> On 09/15/2016 05:38 PM, Wei Liu wrote:
> >On Thu, Sep 08, 2016 at 09:20:25AM +0200, Juergen Gross wrote:
> >>Add HVM usb passthrough support to libxl by using qemu's capability
> >>to emulate standard USB controllers.
> >>
> >>A USB controller is added via qmp command to the emulated hardware
> >>when a usbctrl device of type DEVICEMODEL is requested. Depending on
> >>the requested speed the appropriate hardware type is selected. A host
> >>USB device can then be added to the emulated USB controller via qmp
> >>command.
> >>
> >>Removing of the devices is done via qmp commands, too.
> >
> >Overall the code looks plausible. But the code seems to do more than
> >what is stated in commit message. Some comments below.
> 
> Thanks. I'm just travelling, so my answers are based on my memory what I
> did in the patch. I'll double check before sending out V2.
> 
> >
> >>
> >>Signed-off-by: Juergen Gross <jgross@suse.com>
> >>---
> >> tools/libxl/libxl_device.c |   3 +-
> >> tools/libxl/libxl_usb.c    | 417 +++++++++++++++++++++++++++++++++++----------
> >> tools/libxl/xl_cmdimpl.c   |   4 +-
> >> 3 files changed, 331 insertions(+), 93 deletions(-)
> >>
> >>diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> >>index 5211f20..c6f15db 100644
> >>--- a/tools/libxl/libxl_device.c
> >>+++ b/tools/libxl/libxl_device.c
> >>@@ -782,8 +782,7 @@ void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state *drs)
> >>                 aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
> >>                 aodev->dev = dev;
> >>                 aodev->force = drs->force;
> >>-                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
> >>-                    dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
> >>+                if (dev->kind == LIBXL__DEVICE_KIND_VUSB)
> >
> >This looks a bit suspicious to me. This could be rather obvious but I'm
> >not sure because I didn't follow closely on the overall design of PV /
> >HVM USB passthrough.
> >
> >AIUI:
> >
> >VUSB -> PV USB with in-kernel backend
> >QUSB -> PV USB with QEMU backend
> >
> >Why is QUSB deleted here?
> 
> It isn't. :-)
> 
> A USB passthrough device will always be of kind == VUSB. The backend
> kind may differ, though. This is to ensure a proper device numbering
> regardless of the backend type used.

I see. I missed that dev->backend_kind is changed to dev->kind in the
diff.

> >> {
> >>     device->backend_devid   = usbctrl->devid;
> >>     device->backend_domid   = usbctrl->backend_domid;
> >>-    device->backend_kind    = (usbctrl->type == LIBXL_USBCTRL_TYPE_PV)
> >>-                              ? LIBXL__DEVICE_KIND_VUSB
> >>-                              : LIBXL__DEVICE_KIND_QUSB;
> >>+    switch (usbctrl->type) {
> >>+    case LIBXL_USBCTRL_TYPE_PV:
> >>+        device->backend_kind = LIBXL__DEVICE_KIND_VUSB;
> >>+        break;
> >>+    case LIBXL_USBCTRL_TYPE_QUSB:
> >>+        device->backend_kind = LIBXL__DEVICE_KIND_QUSB;
> >>+        break;
> >>+    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> >>+        device->backend_kind = LIBXL__DEVICE_KIND_NONE;
> >>+        break;
> >>+    default:
> >>+        break;
> >
> >Shouldn't we return some sort of error here?
> 
> This case should not be possible.
> Are you okay with me adding an assert() here?

Yes (and the other place as well).

Wei.

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

[PATCH 6/6] docs: add HVM USB passthrough documentation

[Juergen Gross]

Update the man page regarding passthrough of USB devices to HVM
domains via qemu USB emulation.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/man/xl.cfg.pod.5.in | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 77a1be3..076b2a6 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -745,19 +745,25 @@ Specifies the usb controller type.
 
 "qusb" specifies a qemu base backend for pvusb.
 
+"devicemodel" specifies a USB controller emulated by qemu. It will show up as
+a PCI-device in the guest.
+
 "auto" (the default) determines whether a kernel based backend is installed.
 If this is the case, "pv" is selected, "qusb" will be selected if no kernel
-backend is currently available.
+backend is currently available. For HVM domains "devicemodel" is being selected.
 
 =item B<version=VERSION>
 
 Specifies the usb controller version.  Possible values include
-1 (USB1.1) and 2 (USB2.0). Default is 2 (USB2.0).
+1 (USB1.1), 2 (USB2.0) and 3 (USB3.0). Default is 2 (USB2.0). 3 (USB3.0) is
+available for the type "devicemodel" only.
 
 =item B<ports=PORTS>
 
 Specifies the total ports of the usb controller. The maximum
-number is 31. Default is 8.
+number is 31. Default is 8. With the type "devicemodel" the number of ports
+is more limited: a USB1.1 controller always has 2 ports, a USB2.0 controller
+always has 6 ports and a USB3.0 controller can have up to 15 ports.
 
 USB controller ids start from 0.  In line with the USB spec, however,
 ports on a controller start from 1.
-- 
2.6.6


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

Re: [PATCH 6/6] docs: add HVM USB passthrough documentation

[Wei Liu]

On Thu, Sep 08, 2016 at 09:20:26AM +0200, Juergen Gross wrote:
> Update the man page regarding passthrough of USB devices to HVM
> domains via qemu USB emulation.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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