Fabio Fantoni
2013-Jul-12 13:58 UTC
[PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
Usage: usbversion=1|2|3 (default=2)
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.
Default is 2.
Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
---
docs/man/xl.cfg.pod.5 | 6 ++++++
tools/libxl/libxl.h | 14 ++++++++++++++
tools/libxl/libxl_create.c | 3 +++
tools/libxl/libxl_dm.c | 25 ++++++++++++++++++++++++-
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl_cmdimpl.c | 2 ++
6 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 069b73f..602d428 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1154,6 +1154,12 @@ 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.
+Default is 2.
+
=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 37e4d82..0ffe2cd 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -297,6 +297,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 usb controller is only usb1.
+ */
+#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 0c32d0b..9683740 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -229,6 +229,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
return ERROR_INVAL;
}
+ if (!b_info->u.hvm.usbversion)
+ b_info->u.hvm.usbversion = 2;
+
if (b_info->u.hvm.timer_mode == LIBXL_TIMER_MODE_DEFAULT)
b_info->u.hvm.timer_mode
LIBXL_TIMER_MODE_NO_DELAY_FOR_MISSED_TICKS;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 7e54c02..cad91ba 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -492,7 +492,30 @@ static char ** libxl__build_device_model_args_new(libxl__gc
*gc,
__func__);
return NULL;
}
- flexarray_append(dm_args, "-usb");
+
+ switch (b_info->u.hvm.usbversion) {
+ case 1:
+ flexarray_vappend(dm_args,
+ "-device", "piix3-usb-uhci,id=usb",
NULL);
+ break;
+ case 2:
+ flexarray_vappend(dm_args,
"-device","ich9-usb-ehci1,id=usb,"
+ "bus=pci.0,addr=0x1d.0x7", NULL);
+ for (i = 1; i < 4; i++)
+ flexarray_vappend(dm_args, "-device",
libxl__sprintf(gc,
+
"ich9-usb-uhci%d,masterbus=usb.0,firstport=%d,"
+ "bus=pci.0%s,addr=0x1d.%#x", i, 2*(i-1), i ==
1 ?
+ ",multifunction=on" : "", i-1),
NULL);
+ break;
+ case 3:
+ flexarray_vappend(dm_args,
+ "-device", "nec-usb-xhci,id=usb",
NULL);
+ break;
+ default:
+ LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
+ "usbversion parameter is invalid must be between 1 and
3");
+ return NULL;
+ }
if (b_info->u.hvm.usbdevice) {
flexarray_vappend(dm_args,
"-usbdevice",
b_info->u.hvm.usbdevice, NULL);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d218a2d..100f36c 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -325,6 +325,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 8a478ba..a618ede 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1495,6 +1495,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
George Dunlap
2013-Jul-12 16:13 UTC
Re: [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
On 12/07/13 14:58, Fabio Fantoni wrote:> Usage: usbversion=1|2|3 (default=2) > 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. > Default is 2. > > Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz>Thank you for your enthusiastic respin and resubmission; however, it''s generally a good idea to wait for a day or two to make sure that points are sufficiently discussed before doing so. For instance, I probably would have quibbled with Ian about the for() loop under usbversion=2; and we didn''t finish talking about the "id=usb" component. -George
Dario Faggioli
2013-Jul-12 22:12 UTC
Re: [Xen-devel] [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
On ven, 2013-07-12 at 15:58 +0200, Fabio Fantoni wrote:> Usage: usbversion=1|2|3 (default=2) > 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. > Default is 2. > > Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz> >FWIW, Acked-by: Dario Faggioli <dario.faggioli@citrix.com>> > /* > + * 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 usb controller is only usb1. > + */ > +#define LIBXL_HAVE_BUILDINFO_USBVERSION 1 > + > +/* >Yes, exactly what I (and George) meant. Thanks. :-) Dario -- <<This happens because I choose it to happen!>> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
Fabio Fantoni
2013-Jul-15 09:11 UTC
Re: [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
Il 12/07/2013 15:58, Fabio Fantoni ha scritto:> Usage: usbversion=1|2|3 (default=2) > 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. > Default is 2. > > Signed-off-by: Fabio Fantoni <fabio.fantoni@m2r.biz> > --- > docs/man/xl.cfg.pod.5 | 6 ++++++ > tools/libxl/libxl.h | 14 ++++++++++++++ > tools/libxl/libxl_create.c | 3 +++ > tools/libxl/libxl_dm.c | 25 ++++++++++++++++++++++++- > tools/libxl/libxl_types.idl | 1 + > tools/libxl/xl_cmdimpl.c | 2 ++ > 6 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > index 069b73f..602d428 100644 > --- a/docs/man/xl.cfg.pod.5 > +++ b/docs/man/xl.cfg.pod.5 > @@ -1154,6 +1154,12 @@ 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. > +Default is 2. > + > =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 37e4d82..0ffe2cd 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -297,6 +297,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 usb controller is only usb1. > + */ > +#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 0c32d0b..9683740 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -229,6 +229,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > return ERROR_INVAL; > } > > + if (!b_info->u.hvm.usbversion) > + b_info->u.hvm.usbversion = 2; > + > if (b_info->u.hvm.timer_mode == LIBXL_TIMER_MODE_DEFAULT) > b_info->u.hvm.timer_mode > LIBXL_TIMER_MODE_NO_DELAY_FOR_MISSED_TICKS; > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c > index 7e54c02..cad91ba 100644 > --- a/tools/libxl/libxl_dm.c > +++ b/tools/libxl/libxl_dm.c > @@ -492,7 +492,30 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc, > __func__); > return NULL; > } > - flexarray_append(dm_args, "-usb"); > + > + switch (b_info->u.hvm.usbversion) { > + case 1: > + flexarray_vappend(dm_args, > + "-device", "piix3-usb-uhci,id=usb", NULL); > + break; > + case 2: > + flexarray_vappend(dm_args, "-device","ich9-usb-ehci1,id=usb," > + "bus=pci.0,addr=0x1d.0x7", NULL); > + for (i = 1; i < 4; i++) > + flexarray_vappend(dm_args, "-device", libxl__sprintf(gc, > + "ich9-usb-uhci%d,masterbus=usb.0,firstport=%d," > + "bus=pci.0%s,addr=0x1d.%#x", i, 2*(i-1), i == 1 ? > + ",multifunction=on" : "", i-1), NULL); > + break; > + case 3: > + flexarray_vappend(dm_args, > + "-device", "nec-usb-xhci,id=usb", NULL); > + break; > + default: > + LIBXL__LOG(CTX, LIBXL__LOG_ERROR, > + "usbversion parameter is invalid must be between 1 and 3"); > + return NULL; > + } > if (b_info->u.hvm.usbdevice) { > flexarray_vappend(dm_args, > "-usbdevice", b_info->u.hvm.usbdevice, NULL);Reposted the tests results: Tested on linux domU (ubuntu 12.04 64 bit) with usb redirection: - with usb1 and usb2 working and no problem found. - with usb3 linux sees the usb3 controller but usbredirection not working (tested with qemu 1.3 of xen-unstable) Tested on windows 7 pro 64 bit domU with usb redirection: - with usb1 not working, windows sees the usb devices (flash mass storage) with error (unable to start device, code 10). - with usb2 working and no problem found. - with usb3 not working, windows sees the usb controller but usbredirection is not working (tested with qemu 1.3 of xen-unstable) Qemu log on usb3 test: xhci_cap_read: reg 2 unimplemented xhci: unimplemented command 52 xhci: ERDP out of bounds: 7e7d5000 xhci: ER[7] at 0 len 0 xhci: asserted controller error xhci: ERDP out of bounds: 7eace000 xhci: ER[6] at 0 len 0 xhci: asserted controller error ... xhci: slot 1 has no device xhci: error firing data transfer - For usb1 and usb3 I used the qemu parameters posted by Paolo Bonzini last week. Someone can help me to fix/improve that please? - About usb2 controller is there the option to remove one or more harcoded parameters without cause problems or is all absolutely necessary? Thanks for any reply.> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index d218a2d..100f36c 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -325,6 +325,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 8a478ba..a618ede 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1495,6 +1495,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))
Ian Campbell
2013-Sep-13 11:59 UTC
Re: [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
On Mon, 2013-07-15 at 11:11 +0200, Fabio Fantoni wrote: Stefano or Anthony -- your input as qemu-xen types would be appreciated here on this patch.> > + case 2: > > + flexarray_vappend(dm_args, "-device","ich9-usb-ehci1,id=usb," > > + "bus=pci.0,addr=0x1d.0x7", NULL); > > + for (i = 1; i < 4; i++) > > + flexarray_vappend(dm_args, "-device", libxl__sprintf(gc, > > + "ich9-usb-uhci%d,masterbus=usb.0,firstport=%d," > > + "bus=pci.0%s,addr=0x1d.%#x", i, 2*(i-1), i == 1 ? > > + ",multifunction=on" : "", i-1), NULL); > > + break;Please format this in some sort of halfway readable style. Using GCSPRINTF might help, as would putting newlines in more logical places. Also you don''t need _vappend, _append_pair would do.> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > > index d218a2d..100f36c 100644 > > --- a/tools/libxl/libxl_types.idl > > +++ b/tools/libxl/libxl_types.idl > > @@ -325,6 +325,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > > ("serial", string), > > ("boot", string), > > ("usb", libxl_defbool), > > + ("usbversion", integer),Some sort of whitepace damage?> > # 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 8a478ba..a618ede 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -1495,6 +1495,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;and again.> > switch (xlu_cfg_get_list_as_string_list(config, "usbdevice", > > &b_info->u.hvm.usbdevice_list, > > 1)) >
Fabio Fantoni
2013-Sep-13 13:42 UTC
Re: [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
Il 13/09/2013 13:59, Ian Campbell ha scritto:> On Mon, 2013-07-15 at 11:11 +0200, Fabio Fantoni wrote: > > Stefano or Anthony -- your input as qemu-xen types would be appreciated > here on this patch. > >>> + case 2: >>> + flexarray_vappend(dm_args, "-device","ich9-usb-ehci1,id=usb," >>> + "bus=pci.0,addr=0x1d.0x7", NULL); >>> + for (i = 1; i < 4; i++) >>> + flexarray_vappend(dm_args, "-device", libxl__sprintf(gc, >>> + "ich9-usb-uhci%d,masterbus=usb.0,firstport=%d," >>> + "bus=pci.0%s,addr=0x1d.%#x", i, 2*(i-1), i == 1 ? >>> + ",multifunction=on" : "", i-1), NULL); >>> + break; > Please format this in some sort of halfway readable style. > > Using GCSPRINTF might help, as would putting newlines in more logical > places. > > Also you don''t need _vappend, _append_pair would do.Thanks for reply. I''ll do them on next version if I understand right the "halfway readable style".> >>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >>> index d218a2d..100f36c 100644 >>> --- a/tools/libxl/libxl_types.idl >>> +++ b/tools/libxl/libxl_types.idl >>> @@ -325,6 +325,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ >>> ("serial", string), >>> ("boot", string), >>> ("usb", libxl_defbool), >>> + ("usbversion", integer), > Some sort of whitepace damage?On source code seem correct (usbversion with same whitespace of usb before the "(" and also aligned to view), I don''t know what cause this problem. I try to remove the patch and reapply it from git format-patch file (with patch -p1 -i ...) and also in that case the source code seem correct.> >>> # 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 8a478ba..a618ede 100644 >>> --- a/tools/libxl/xl_cmdimpl.c >>> +++ b/tools/libxl/xl_cmdimpl.c >>> @@ -1495,6 +1495,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; > and again. > >>> switch (xlu_cfg_get_list_as_string_list(config, "usbdevice", >>> &b_info->u.hvm.usbdevice_list, >>> 1)) >
Ian Campbell
2013-Sep-13 13:54 UTC
Re: [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu
On Fri, 2013-09-13 at 15:42 +0200, Fabio Fantoni wrote:> >>> + case 2: > >>> + flexarray_vappend(dm_args, "-device","ich9-usb-ehci1,id=usb," > >>> + "bus=pci.0,addr=0x1d.0x7", NULL); > >>> + for (i = 1; i < 4; i++) > >>> + flexarray_vappend(dm_args, "-device", libxl__sprintf(gc, > >>> + "ich9-usb-uhci%d,masterbus=usb.0,firstport=%d," > >>> + "bus=pci.0%s,addr=0x1d.%#x", i, 2*(i-1), i == 1 ? > >>> + ",multifunction=on" : "", i-1), NULL); > >>> + break; > > Please format this in some sort of halfway readable style. > > > > Using GCSPRINTF might help, as would putting newlines in more logical > > places. > > > > Also you don''t need _vappend, _append_pair would do. > > Thanks for reply. > I''ll do them on next version if I understand right the "halfway readable > style".I hope you can use your judgement here, rather than just wrapping at 80 chars with no regard for the readability. Ian.