| Message ID | 1373637490-6262-1-git-send-email-fabio.fantoni@m2r.biz |
|---|---|
| State | New |
| Headers | show |
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
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
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))
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)) >
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)) >
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.
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))
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(-)