Re: [PATCH v6] libxl: usb2 and usb3 controller support for upstream qemu

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Sep 23, 2013 at 04:32:02PM +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.
> 
> 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       |    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 769767b..f768784 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1168,6 +1168,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 4cab294..e27308e 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -305,6 +305,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 7567238..a6bfb0a 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 43c3bec..4ee28b3 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -511,7 +511,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_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:
> +                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);

There is one issue here, when using -usbdevice x, QEMU will always add
an usb v1 controller (almost equivalent to the "case 1" us usbversion).
So, this usbversion=x does not seems to belong to the if(usb ||
usbdevice).

On it's on, this patch does not add anything to QEMU, the machine will
just have more usb controller when someone will add an usbdevice
(tablet for example).

usbversion seems only usefull when used with usbredirection, so it's
probably worth adding the controller only when usbversion or
usbredirection is in the vm config file.

In other words, I think this will be better:
- leave intacte the if(usb || usbdevice) block.
- add if (usbversion) {
    add_controller_vX
  }
- and in the second patch that add usbredirection, change
  if(usbversion) to if(usbversion||spice.usbredirectino)

What do you think?

(and now, I'm going to try the second patch with spice usbredirection)

-- 
Anthony PERARD