Re: [Qemu-devel] [PATCH v4] libxl: usb2 and usb3 controller support for upstream qemu

[Fabio Fantoni <fabio.fantoni@m2r.biz>]

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))