Xen devel - Nov 2013 - [PATCH v7 1/2] libxl: usb2 and usb3 controller support for upstream qemu

Usage: usbversion=1|2|3 (default=0, no usb controller defined)
Specifies the type of an emulated USB bus in the guest. 1 for usb1,
2 for usb2 and 3 for usb3, it is available only with upstream qemu.
The old usb and usbdevice parameters cannot be used with this.

Changes from v6:
- now usbversion cannot be used with usb and usbdevice parameters
- now default is 0 (no usb controller defined)
Will be used only with usb redirection (from spice client) and
new usb passthrough (from dom0) with hotplug.

Changes from v5:
changed usb2 controller qemu parameters:
- removed bus
- added multifunction on all devices

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
---
 docs/man/xl.cfg.pod.5       |    7 +++++++
 tools/libxl/libxl.h         |   14 ++++++++++++++
 tools/libxl/libxl_create.c  |    9 +++++++++
 tools/libxl/libxl_dm.c      |   24 ++++++++++++++++++++++++
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |    2 ++
 6 files changed, 57 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index d2d8921..3151e80 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1189,6 +1189,13 @@ device.
 
 Enables or disables an emulated USB bus in the guest.
 
+=item B<usbversion=NUMBER>
+ 
+Specifies the type of an emulated USB bus in the guest. 1 for usb1,
+2 for usb2 and 3 for usb3, it is available only with upstream qemu.
+The old usb and usbdevice parameters cannot be used with this.
+Default is 0 (no usb controller defined).
+
 =item B<usbdevice=[ "DEVICE", "DEVICE", ...]>
 
 Adds B<DEVICE>s to the emulated USB bus. The USB bus must also be
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9379694..7f8b65a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -311,6 +311,20 @@
 #define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST 1
 
 /*
+ * LIBXL_HAVE_BUILDINFO_USBVERSION
+ *
+ * If this is defined, then the libxl_domain_build_info structure will
+ * contain hvm.usbversion, a integer type that contains a USB
+ * controller version to specify on the qemu upstream command-line.
+ *
+ * If it is set, callers may use hvm.usbversion to specify if the usb
+ * controller is usb1, usb2 or usb3.
+ *
+ * If this is not defined, the hvm.usbversion field does not exist.
+ */
+#define LIBXL_HAVE_BUILDINFO_USBVERSION 1
+
+/*
  * LIBXL_HAVE_DEVICE_BACKEND_DOMNAME
  *
  * If this is defined, libxl_device_* structures containing a backend_domid
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9d793ba..14009dc 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -280,6 +280,15 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.usb,               
false);
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,  
true);
 
+        if (b_info->u.hvm.usbversion && 
+            ( libxl_defbool_val(b_info->u.hvm.usb)
+            || b_info->u.hvm.usbdevice_list
+            || b_info->u.hvm.usbdevice) ){
+            LOG(ERROR,"usbversion cannot be enabled with usb or"
+            "usbdevice parameters.");
+            return ERROR_INVAL;
+        }
+
         if (!b_info->u.hvm.boot) {
             b_info->u.hvm.boot = strdup("cda");
             if (!b_info->u.hvm.boot) return ERROR_NOMEM;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 85a08af..1c31179 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -527,6 +527,30 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
                                       *p, NULL);
                 }
             }
+        } else if (b_info->u.hvm.usbversion) {
+            switch (b_info->u.hvm.usbversion) {
+            case 1:
+                flexarray_vappend(dm_args,
+                    "-device", "piix3-usb-uhci,id=usb",
NULL);
+                break;
+            case 2:
+                flexarray_append_pair(dm_args, "-device",
+                   
"ich9-usb-ehci1,id=usb,addr=0x1d.0x7,multifunction=on");
+                for (i = 1; i < 4; i++)
+                    flexarray_append_pair(dm_args, "-device",
+                        GCSPRINTF("ich9-usb-uhci%d,masterbus=usb.0,"
+                       
"firstport=%d,addr=0x1d.%#x,multifunction=on",
+                        i, 2*(i-1), i-1));
+                break;
+            case 3:
+                flexarray_vappend(dm_args,
+                    "-device", "nec-usb-xhci,id=usb",
NULL);
+                break;
+            default:
+                LOG(ERROR, "%s: usbversion parameter is invalid, "
+                    "must be between 1 and 3", __func__);
+                return NULL;
+            }
         }
         if (b_info->u.hvm.soundhw) {
             flexarray_vappend(dm_args, "-soundhw",
b_info->u.hvm.soundhw, NULL);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ce003c6..ee312a4 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -360,6 +360,7 @@ libxl_domain_build_info =
Struct("domain_build_info",[
                                        ("serial",           string),
                                        ("boot",             string),
                                        ("usb",             
libxl_defbool),
+                                       ("usbversion",       integer),
                                        # usbdevice:
                                        # - "tablet" for absolute
mouse,
                                        # - "mouse" for PS/2 protocol
relative mouse
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5bd9b15..f78a19a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1540,6 +1540,8 @@ skip_vfb:
         xlu_cfg_replace_string (config, "serial",
&b_info->u.hvm.serial, 0);
         xlu_cfg_replace_string (config, "boot",
&b_info->u.hvm.boot, 0);
         xlu_cfg_get_defbool(config, "usb", &b_info->u.hvm.usb,
0);
+        if (!xlu_cfg_get_long (config, "usbversion", &l, 0))
+            b_info->u.hvm.usbversion = l;
         switch (xlu_cfg_get_list_as_string_list(config, "usbdevice",
                                                
&b_info->u.hvm.usbdevice_list,
                                                 1))
-- 
1.7.9.5

Usage: spiceusbredirection=NUMBER (default=0)

Enables spice usbredirection. Creates NUMBER usbredirection channels
for redirection of up to 4 usb devices from spice client to domU''s
qemu.
It requires an usb controller and if not defined will automatically adds
an usb2 controller.

Changes from v3:
- fixed condition that enable usbversion if it isn''t defined in
presence
  of usbredirection enabled

Changes from v2:
- updated for usbversion patch v7
- now usbredirection cannot be used with usb and usbdevice parameters
- if usbversion is undefined it will creates an usb2 controller

Changes from v1:
- Now can be setted the number of redirection channels.
- Various code improvements.

Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
---
 docs/man/xl.cfg.pod.5       |    7 +++++++
 tools/libxl/libxl.h         |   11 +++++++++++
 tools/libxl/libxl_create.c  |   10 +++++++---
 tools/libxl/libxl_dm.c      |   12 ++++++++++++
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |    2 ++
 6 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 3151e80..3dedd61 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1165,6 +1165,13 @@ requires vdagent service installed on domU o.s. to work.
The default is 0.
 Enables Spice clipboard sharing (copy/paste). It requires spicevdagent
 enabled. The default is false (0).
 
+=item B<spiceusbredirection=NUMBER>
+
+Enables spice usbredirection. Creates NUMBER usbredirection channels
+for redirection of up to 4 usb devices from spice client to domU''s
qemu.
+It requires an usb controller and if not defined it will automatically adds
+an usb2 controller. The default is disabled (0).
+
 =back
 
 =head3 Miscellaneous Emulated Hardware
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7f8b65a..b72056d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -376,6 +376,17 @@
 #define LIBXL_HAVE_SPICE_VDAGENT 1
 
 /*
+ * LIBXL_HAVE_SPICE_USBREDIRECTION
+ *
+ * If defined, then the libxl_spice_info structure will contain an integer type
+ * field: usbredirection. This value defines if Spice usbredirection is enabled
+ * and with how much channels.
+ *
+ * If this is not defined, the Spice usbredirection support is ignored.
+ */
+#define LIBXL_HAVE_SPICE_USBREDIREDIRECTION 1
+
+/*
  * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS 1
  *
  * If this is defined, libxl_domain_create_restore()''s API has changed
to
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 14009dc..25dc5f6 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -280,12 +280,16 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         libxl_defbool_setdefault(&b_info->u.hvm.usb,               
false);
         libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,  
true);
 
-        if (b_info->u.hvm.usbversion && 
+        if (!b_info->u.hvm.usbversion && 
+            (b_info->u.hvm.spice.usbredirection > 0) )
+            b_info->u.hvm.usbversion = 2;
+
+        if ((b_info->u.hvm.usbversion ||
b_info->u.hvm.spice.usbredirection) &&
             ( libxl_defbool_val(b_info->u.hvm.usb)
             || b_info->u.hvm.usbdevice_list
             || b_info->u.hvm.usbdevice) ){
-            LOG(ERROR,"usbversion cannot be enabled with usb or"
-            "usbdevice parameters.");
+            LOG(ERROR,"usbversion and/or usbredirection cannot be "
+            "enabled with usb and/or usbdevice parameters.");
             return ERROR_INVAL;
         }
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 1c31179..7be0a50 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -551,6 +551,18 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
                     "must be between 1 and 3", __func__);
                 return NULL;
             }
+            if (b_info->u.hvm.spice.usbredirection >= 0 &&
+                b_info->u.hvm.spice.usbredirection < 5) {
+                for (i = 1; i <= b_info->u.hvm.spice.usbredirection; i++)
+                    flexarray_vappend(dm_args, "-chardev",
libxl__sprintf(gc,
+                        "spicevmc,name=usbredir,id=usbrc%d", i),
"-device",
+                        libxl__sprintf(gc,
"usb-redir,chardev=usbrc%d,"
+                        "id=usbrc%d", i, i), NULL);
+            } else {
+                LOG(ERROR, "%s: usbredirection parameter is invalid,
"
+                    "it must be between 1 and 4", __func__);
+                return NULL;
+            }
         }
         if (b_info->u.hvm.soundhw) {
             flexarray_vappend(dm_args, "-soundhw",
b_info->u.hvm.soundhw, NULL);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ee312a4..f919f8c 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -200,6 +200,7 @@ libxl_spice_info = Struct("spice_info", [
     ("agent_mouse", libxl_defbool),
     ("vdagent",     libxl_defbool),
     ("clipboard_sharing", libxl_defbool),
+    ("usbredirection", integer),
     ])
 
 libxl_sdl_info = Struct("sdl_info", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f78a19a..e3cc0a4 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1534,6 +1534,8 @@ skip_vfb:
                             &b_info->u.hvm.spice.vdagent, 0);
         xlu_cfg_get_defbool(config, "spice_clipboard_sharing",
                             &b_info->u.hvm.spice.clipboard_sharing, 0);
+        if (!xlu_cfg_get_long (config, "spiceusbredirection", &l,
0))
+            b_info->u.hvm.spice.usbredirection = l;
         xlu_cfg_get_defbool(config, "nographic",
&b_info->u.hvm.nographic, 0);
         xlu_cfg_get_defbool(config, "gfx_passthru", 
                             &b_info->u.hvm.gfx_passthru, 0);
-- 
1.7.9.5

Fabio Fantoni writes ("[PATCH v7 1/2] libxl: usb2 and usb3 controller
support for upstream qemu"):
> Usage: usbversion=1|2|3 (default=0, no usb controller defined)
> Specifies the type of an emulated USB bus in the guest. 1 for usb1,
> 2 for usb2 and 3 for usb3, it is available only with upstream qemu.
> The old usb and usbdevice parameters cannot be used with this.
> 
> Changes from v6:
> - now usbversion cannot be used with usb and usbdevice parameters
> - now default is 0 (no usb controller defined)
> Will be used only with usb redirection (from spice client) and
> new usb passthrough (from dom0) with hotplug.
...
> +=item B<usbversion=NUMBER>
> + 
> +Specifies the type of an emulated USB bus in the guest. 1 for usb1,
> +2 for usb2 and 3 for usb3, it is available only with upstream qemu.
> +The old usb and usbdevice parameters cannot be used with this.
> +Default is 0 (no usb controller defined).
> +
I''ve reread the thread and I don''t understand the backwards
compatibility implications here.

What is wrong with the usb and usbdevice parameters. ?

The "usb" parameter was able to turn on, and off, usb support in trad
qemu (and that was always usb1).  The previous default appears to be
0, i.e. off.  But if you said "usb=1" you would get usb1.  There
doesn''t seem to be anything in your patch that makes that work with
qemu-upstream; instead you simply invent a totally new mechanism.

Why do you describe the usbdevice parameter as "old" ?

If you are formally deprecating "usb" and/or "usbdevice" you
need to
replace them with something, and have a compatibility mechanism for
them, I think.

Perhaps I just don''t understand the whole support matrix.  But even
if we are implementing partial support right now, we ought to
(a) clearly document what is deprecated and what isn''t
(b) not deprecate anything until its replacement exists
(c) have an API and config design which can be coherently extended to
the full support matrix.

Thanks,
Ian.

On 12/03/2013 04:17 PM, Ian Jackson wrote:
> Fabio Fantoni writes ("[PATCH v7 1/2] libxl: usb2 and usb3 controller
support for upstream qemu"):
>> Usage: usbversion=1|2|3 (default=0, no usb controller defined)
>> Specifies the type of an emulated USB bus in the guest. 1 for usb1,
>> 2 for usb2 and 3 for usb3, it is available only with upstream qemu.
>> The old usb and usbdevice parameters cannot be used with this.
>>
>> Changes from v6:
>> - now usbversion cannot be used with usb and usbdevice parameters
>> - now default is 0 (no usb controller defined)
>> Will be used only with usb redirection (from spice client) and
>> new usb passthrough (from dom0) with hotplug.
> ...
>> +=item B<usbversion=NUMBER>
>> +
>> +Specifies the type of an emulated USB bus in the guest. 1 for usb1,
>> +2 for usb2 and 3 for usb3, it is available only with upstream qemu.
>> +The old usb and usbdevice parameters cannot be used with this.
>> +Default is 0 (no usb controller defined).
>> +
>
> I''ve reread the thread and I don''t understand the
backwards
> compatibility implications here.
>
> What is wrong with the usb and usbdevice parameters. ?
>
> The "usb" parameter was able to turn on, and off, usb support in
trad
> qemu (and that was always usb1).  The previous default appears to be
> 0, i.e. off.  But if you said "usb=1" you would get usb1.  There
> doesn''t seem to be anything in your patch that makes that work
with
> qemu-upstream; instead you simply invent a totally new mechanism.
>
> Why do you describe the usbdevice parameter as "old" ?
usbdevice basically accepts a string to be passed as-is to the qemu 
command-line after "-usbdevice".  This argement is now deprecated in 
qemu; although supported in a backwards-compatible fashion (and thus 
still works with qemu-upstream), it doesn''t (I infer from
Fabio''s
description) work with usb2 and usb3.  This is a qemu limitation, not a 
Xen limitation.

The "new" way of specifying devices (USB or otherwise) is qdev (the 
documentation for which is cleverly hidden in a text file the qemu 
source tree).  qdev specification is available both from the 
command-line, and over qmp.

Going forward, I think the thing we want to do is take my "usb
hotplug"
series which didn''t make it into 4.3 and have it replace the
"usbdevice"
interface.  We should make the mechanism for adding USB devices take the 
same path at boot as during hot-plug -- similar to the way PCI devices 
take the same basic paths whether specified in the config file at boot 
or done after the VM is running.

I think at the moment to get a usb2/3 device, you''d have to manually 
craft your own qdev parameter and specify it via the
"device_model_args"
config parameter.

  -George

George Dunlap writes ("Re: [PATCH v7 1/2] libxl: usb2 and usb3 controller
support for upstream qemu"):
> On 12/03/2013 04:17 PM, Ian Jackson wrote:
> > Why do you describe the usbdevice parameter as "old" ?
> 
> usbdevice basically accepts a string to be passed as-is to the qemu 
> command-line after "-usbdevice".  This argement is now deprecated
in
> qemu; although supported in a backwards-compatible fashion (and thus 
> still works with qemu-upstream), it doesn''t (I infer from
Fabio''s
> description) work with usb2 and usb3.  This is a qemu limitation, not a 
> Xen limitation.
So "usbdevice" is indeed being partially deprecated here, in some
sense, but we don''t have a replacement.

I''m not sure that describing it as "old" in the documentation
is fair,
then.  Even if it''s regarded as "old" by upstream qemu.  A
user would
reasonably ask what to use instead.

Perhaps the docs should say "unfortunately this does not work with the
usbdevice parameter" and leave the question of resolving the problem
for another day.

Ian.

Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice usbredirection support
for upstream qemu"):
> Usage: spiceusbredirection=NUMBER (default=0)
...
> +=item B<spiceusbredirection=NUMBER>
> +
> +Enables spice usbredirection. Creates NUMBER usbredirection channels
> +for redirection of up to 4 usb devices from spice client to
domU''s qemu.
> +It requires an usb controller and if not defined it will automatically
adds
> +an usb2 controller. The default is disabled (0).
I see we have:
  spice
  spiceport
  spicetls_port
  spicehost
  spicedisable_ticketing
  spicepasswd
  spiceagent_mouse
  spicevdagent
  spice_clipboard_sharing

This is a mess in terms of the spelling of the parameters.  But I
guess the new one is with the majority.

The bulk of the patch seems fine.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

This needs a freeze exception.  George ?

Ian.

On Tue, Dec 3, 2013 at 5:13 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:
> Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice usbredirection
support for upstream qemu"):
>> Usage: spiceusbredirection=NUMBER (default=0)
> ...
>> +=item B<spiceusbredirection=NUMBER>
>> +
>> +Enables spice usbredirection. Creates NUMBER usbredirection channels
>> +for redirection of up to 4 usb devices from spice client to
domU''s qemu.
>> +It requires an usb controller and if not defined it will automatically
adds
>> +an usb2 controller. The default is disabled (0).
>
> I see we have:
>   spice
>   spiceport
>   spicetls_port
>   spicehost
>   spicedisable_ticketing
>   spicepasswd
>   spiceagent_mouse
>   spicevdagent
>   spice_clipboard_sharing
>
> This is a mess in terms of the spelling of the parameters.  But I
> guess the new one is with the majority.
>
> The bulk of the patch seems fine.
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> This needs a freeze exception.  George ?
Hrm.  I definitely would have given an Ack two weeks ago when this was
first submitted.  Now it seems fairly late in the freeze.

The patches themselves are fairly simple, and have had a lot of
review.  If there is a bug, it is very unlikely to affect users who
are not directly using these features (which are off by default); for
users who are using these, the worst that could happen is that the xl
create fails, or the feature does not work.  (i.e., random guest or
host crashes cannot be a result of a bug here, but of a bug somewhere
else that this might trigger).

I guess the big risk here is the interface.  This series only
introduces two new elements: "usbversion", and
"spice.usbredirection".
 Both of those seem like things we would want to introduce, and given
that, it''s hard to imagine what other interface we would even use.
We''ve had a lot of review cycles for the interface, so I think that is
overall fairly low risk.

I''m inclined therefore to give this a release Ack.  Any other opinions?

 -George

George Dunlap writes ("Re: [Xen-devel] [PATCH v7 2/2] libxl: spice
usbredirection support for upstream qemu"):
> I guess the big risk here is the interface.  This series only
> introduces two new elements: "usbversion", and
"spice.usbredirection".
>  Both of those seem like things we would want to introduce, and given
> that, it''s hard to imagine what other interface we would even use.
> We''ve had a lot of review cycles for the interface, so I think
that is
> overall fairly low risk.
> 
> I''m inclined therefore to give this a release Ack.  Any other
opinions?
I concur with your reasoning and your conclusion.

Ian.

On Thu, 2013-12-05 at 14:22 +0000, George Dunlap wrote:
> On Tue, Dec 3, 2013 at 5:13 PM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> > Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice usbredirection
support for upstream qemu"):
> >> Usage: spiceusbredirection=NUMBER (default=0)
> > ...
> >> +=item B<spiceusbredirection=NUMBER>
> >> +
> >> +Enables spice usbredirection. Creates NUMBER usbredirection
channels
> >> +for redirection of up to 4 usb devices from spice client to
domU''s qemu.
> >> +It requires an usb controller and if not defined it will
automatically adds
> >> +an usb2 controller. The default is disabled (0).
> >
> > I see we have:
> >   spice
> >   spiceport
> >   spicetls_port
> >   spicehost
> >   spicedisable_ticketing
> >   spicepasswd
> >   spiceagent_mouse
> >   spicevdagent
> >   spice_clipboard_sharing
> >
> > This is a mess in terms of the spelling of the parameters.  But I
> > guess the new one is with the majority.
> >
> > The bulk of the patch seems fine.
> >
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >
> > This needs a freeze exception.  George ?
> 
> Hrm.  I definitely would have given an Ack two weeks ago when this was
> first submitted.  Now it seems fairly late in the freeze.
> 
> The patches themselves are fairly simple, and have had a lot of
> review.  If there is a bug, it is very unlikely to affect users who
> are not directly using these features (which are off by default); for
> users who are using these, the worst that could happen is that the xl
> create fails, or the feature does not work.  (i.e., random guest or
> host crashes cannot be a result of a bug here, but of a bug somewhere
> else that this might trigger).
> 
> I guess the big risk here is the interface.  This series only
> introduces two new elements: "usbversion", and
"spice.usbredirection".
>  Both of those seem like things we would want to introduce, and given
> that, it''s hard to imagine what other interface we would even use.
> We''ve had a lot of review cycles for the interface, so I think
that is
> overall fairly low risk.
My only question would be should usbversion be an enum rather than a
numeric version, especially since USB1 had two main controller types
(ohci and uhci). USB 2+ seems to be pretty standard with ehci and xhci.
An enum is also more self documenting. As a side effect it offers you a
parsing function so you can use descriptive text in the cfg file too.

On the other hand maybe users are going to be happier with usb1/2/3 than
with uhci/ehci/xhci (TBH I had to look up which was which...)

Has a qemu person acked the command lines used to create each of these
configurations? To my non-usb aware mind it looks odd that USB2
explicitly creates the legacy USB1 controller (AIUI this is how USB2 is
supposed to be done) while USB3 doesn''t create any legacy controllers.
I
don''t know how USB3 is suppose to work, so perhaps USB3 does differ
from
USB2 in this way, I don''t know.

Ian.
> 
> I''m inclined therefore to give this a release Ack.  Any other
opinions?
> 
>  -George

On 05/12/13 14:32, Ian Campbell wrote:
> 
> Has a qemu person acked the command lines used to create each of these
> configurations? To my non-usb aware mind it looks odd that USB2
> explicitly creates the legacy USB1 controller (AIUI this is how USB2 is
> supposed to be done) while USB3 doesn''t create any legacy
controllers. I
> don''t know how USB3 is suppose to work, so perhaps USB3 does
differ from
> USB2 in this way, I don''t know.
xhci controllers support all speeds of devices (low, full, high and
super) so the legacy controllers are not needed.

David

On Thu, 2013-12-05 at 14:40 +0000, David Vrabel wrote:
> On 05/12/13 14:32, Ian Campbell wrote:
> > 
> > Has a qemu person acked the command lines used to create each of these
> > configurations? To my non-usb aware mind it looks odd that USB2
> > explicitly creates the legacy USB1 controller (AIUI this is how USB2
is
> > supposed to be done) while USB3 doesn''t create any legacy
controllers. I
> > don''t know how USB3 is suppose to work, so perhaps USB3 does
differ from
> > USB2 in this way, I don''t know.
> 
> xhci controllers support all speeds of devices (low, full, high and
> super) so the legacy controllers are not needed.
Thanks, so that removes most of my concern but I would still prefer to
see a qemu person ack the specific command lines we use here.

Ian.

On 12/05/2013 02:32 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 14:22 +0000, George Dunlap wrote:
>> On Tue, Dec 3, 2013 at 5:13 PM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
>>> Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice
usbredirection support for upstream qemu"):
>>>> Usage: spiceusbredirection=NUMBER (default=0)
>>> ...
>>>> +=item B<spiceusbredirection=NUMBER>
>>>> +
>>>> +Enables spice usbredirection. Creates NUMBER usbredirection
channels
>>>> +for redirection of up to 4 usb devices from spice client to
domU''s qemu.
>>>> +It requires an usb controller and if not defined it will
automatically adds
>>>> +an usb2 controller. The default is disabled (0).
>>> I see we have:
>>>    spice
>>>    spiceport
>>>    spicetls_port
>>>    spicehost
>>>    spicedisable_ticketing
>>>    spicepasswd
>>>    spiceagent_mouse
>>>    spicevdagent
>>>    spice_clipboard_sharing
>>>
>>> This is a mess in terms of the spelling of the parameters.  But I
>>> guess the new one is with the majority.
>>>
>>> The bulk of the patch seems fine.
>>>
>>> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>>>
>>> This needs a freeze exception.  George ?
>> Hrm.  I definitely would have given an Ack two weeks ago when this was
>> first submitted.  Now it seems fairly late in the freeze.
>>
>> The patches themselves are fairly simple, and have had a lot of
>> review.  If there is a bug, it is very unlikely to affect users who
>> are not directly using these features (which are off by default); for
>> users who are using these, the worst that could happen is that the xl
>> create fails, or the feature does not work.  (i.e., random guest or
>> host crashes cannot be a result of a bug here, but of a bug somewhere
>> else that this might trigger).
>>
>> I guess the big risk here is the interface.  This series only
>> introduces two new elements: "usbversion", and
"spice.usbredirection".
>>   Both of those seem like things we would want to introduce, and given
>> that, it''s hard to imagine what other interface we would even
use.
>> We''ve had a lot of review cycles for the interface, so I think
that is
>> overall fairly low risk.
> My only question would be should usbversion be an enum rather than a
> numeric version, especially since USB1 had two main controller types
> (ohci and uhci). USB 2+ seems to be pretty standard with ehci and xhci.
> An enum is also more self documenting. As a side effect it offers you a
> parsing function so you can use descriptive text in the cfg file too.
>
> On the other hand maybe users are going to be happier with usb1/2/3 than
> with uhci/ehci/xhci (TBH I had to look up which was which...)
>
> Has a qemu person acked the command lines used to create each of these
> configurations? To my non-usb aware mind it looks odd that USB2
> explicitly creates the legacy USB1 controller (AIUI this is how USB2 is
> supposed to be done) while USB3 doesn''t create any legacy
controllers. I
> don''t know how USB3 is suppose to work, so perhaps USB3 does
differ from
> USB2 in this way, I don''t know.
I''m pretty sure Fabio had a number of back-and-forths on the qemu list 
before formulating these patches.  Paulo Bonzini also responded to I 
think it was Ian Jackson''s question about the stability of the 
command-line arguments, saying that the command-line arguments were a 
stable interface that would be supported in a backwards-compatible 
manner.  And Stefano also replied saying the qemu arguments look OK.  So 
those have gotten about as much attention as I think they ever would.

Re having an enum -- would it be possible in theory to switch to an enum 
at some point in the future, in a backwards-compatible manner? I.e., 
make the enum values for 1, 2, and 3 act just as numerical values 1, 2, 
and 3 do now?

Overall I''m still inclined to say that the "interface risk"
is fairly
low.  If you think differently, we can let it wait until 4.5; otherwise 
I think we should check it in.

  -George

On Thu, 2013-12-05 at 14:44 +0000, George Dunlap wrote:
> On 12/05/2013 02:32 PM, Ian Campbell wrote:
> > On Thu, 2013-12-05 at 14:22 +0000, George Dunlap wrote:
> >> On Tue, Dec 3, 2013 at 5:13 PM, Ian Jackson
<Ian.Jackson@eu.citrix.com> wrote:
> >>> Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice
usbredirection support for upstream qemu"):
> >>>> Usage: spiceusbredirection=NUMBER (default=0)
> >>> ...
> >>>> +=item B<spiceusbredirection=NUMBER>
> >>>> +
> >>>> +Enables spice usbredirection. Creates NUMBER
usbredirection channels
> >>>> +for redirection of up to 4 usb devices from spice client
to domU''s qemu.
> >>>> +It requires an usb controller and if not defined it will
automatically adds
> >>>> +an usb2 controller. The default is disabled (0).
> >>> I see we have:
> >>>    spice
> >>>    spiceport
> >>>    spicetls_port
> >>>    spicehost
> >>>    spicedisable_ticketing
> >>>    spicepasswd
> >>>    spiceagent_mouse
> >>>    spicevdagent
> >>>    spice_clipboard_sharing
> >>>
> >>> This is a mess in terms of the spelling of the parameters. 
But I
> >>> guess the new one is with the majority.
> >>>
> >>> The bulk of the patch seems fine.
> >>>
> >>> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >>>
> >>> This needs a freeze exception.  George ?
> >> Hrm.  I definitely would have given an Ack two weeks ago when this
was
> >> first submitted.  Now it seems fairly late in the freeze.
> >>
> >> The patches themselves are fairly simple, and have had a lot of
> >> review.  If there is a bug, it is very unlikely to affect users
who
> >> are not directly using these features (which are off by default);
for
> >> users who are using these, the worst that could happen is that the
xl
> >> create fails, or the feature does not work.  (i.e., random guest
or
> >> host crashes cannot be a result of a bug here, but of a bug
somewhere
> >> else that this might trigger).
> >>
> >> I guess the big risk here is the interface.  This series only
> >> introduces two new elements: "usbversion", and
"spice.usbredirection".
> >>   Both of those seem like things we would want to introduce, and
given
> >> that, it''s hard to imagine what other interface we would
even use.
> >> We''ve had a lot of review cycles for the interface, so I
think that is
> >> overall fairly low risk.
> > My only question would be should usbversion be an enum rather than a
> > numeric version, especially since USB1 had two main controller types
> > (ohci and uhci). USB 2+ seems to be pretty standard with ehci and
xhci.
> > An enum is also more self documenting. As a side effect it offers you
a
> > parsing function so you can use descriptive text in the cfg file too.
> >
> > On the other hand maybe users are going to be happier with usb1/2/3
than
> > with uhci/ehci/xhci (TBH I had to look up which was which...)
> >
> > Has a qemu person acked the command lines used to create each of these
> > configurations? To my non-usb aware mind it looks odd that USB2
> > explicitly creates the legacy USB1 controller (AIUI this is how USB2
is
> > supposed to be done) while USB3 doesn''t create any legacy
controllers. I
> > don''t know how USB3 is suppose to work, so perhaps USB3 does
differ from
> > USB2 in this way, I don''t know.
> 
> I''m pretty sure Fabio had a number of back-and-forths on the qemu
list
> before formulating these patches.  Paulo Bonzini also responded to I 
> think it was Ian Jackson''s question about the stability of the 
> command-line arguments, saying that the command-line arguments were a 
> stable interface that would be supported in a backwards-compatible 
> manner.  And Stefano also replied saying the qemu arguments look OK.  So 
> those have gotten about as much attention as I think they ever would.
Well, it would be nice then if that were made apparent in the changelog,
perhaps in the form of an acked-by or even just mentioning it in the
message, the fact that there was a lot of back and forth about it just
makes that more necessary.
> Re having an enum -- would it be possible in theory to switch to an enum 
> at some point in the future, in a backwards-compatible manner? I.e., 
> make the enum values for 1, 2, and 3 act just as numerical values 1, 2, 
> and 3 do now?
int -> enum should be possible, I think, unless there is some oddity
about C type conversions which I''m forgetting.

Of course people who using LIBXL_API_VERSION <= 4.4 would not be able to
use the enum names in their code, so we would have to be careful not to
change them (which for a regular enum which was never an int we might
think we can safely do).
> Overall I''m still inclined to say that the "interface
risk" is fairly
> low.  If you think differently, we can let it wait until 4.5; otherwise 
> I think we should check it in.
> 
>   -George
>

On Tue, 2013-12-03 at 17:13 +0000, Ian Jackson wrote:
> Fabio Fantoni writes ("[PATCH v7 2/2] libxl: spice usbredirection
support for upstream qemu"):
> > Usage: spiceusbredirection=NUMBER (default=0)
> ...
> > +=item B<spiceusbredirection=NUMBER>
> > +
> > +Enables spice usbredirection. Creates NUMBER usbredirection channels
> > +for redirection of up to 4 usb devices from spice client to
domU''s qemu.
> > +It requires an usb controller and if not defined it will
automatically adds
> > +an usb2 controller. The default is disabled (0).
> 
> I see we have:
>   spice
>   spiceport
>   spicetls_port
>   spicehost
>   spicedisable_ticketing
>   spicepasswd
>   spiceagent_mouse
>   spicevdagent
>   spice_clipboard_sharing
> 
> This is a mess in terms of the spelling of the parameters.  But I
> guess the new one is with the majority.
> 
> The bulk of the patch seems fine.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> This needs a freeze exception.  George ?
Per the ensuing discussion I''ve now applied this on top of v8-iwj of
patch #1.
> 
> Ian.