Patchwork [v3] libxl: usb2 and usb3 controller support for upstream qemu

login
register
mail settings
Submitter Fabio Fantoni
Date July 12, 2013, 10:22 a.m.
Message ID <1373624555-4403-1-git-send-email-fabio.fantoni@m2r.biz>
Download mbox | patch
Permalink /patch/258734/
State New
Headers show

Comments

Fabio Fantoni - July 12, 2013, 10:22 a.m.
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_create.c  |    3 +++
 tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
 tools/libxl/libxl_types.idl |    1 +
 tools/libxl/xl_cmdimpl.c    |    2 ++
 5 files changed, 36 insertions(+), 1 deletion(-)
George Dunlap - July 12, 2013, 11:06 a.m.
On 12/07/13 11:22, 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>
> ---
>   docs/man/xl.cfg.pod.5       |    6 ++++++
>   tools/libxl/libxl_create.c  |    3 +++
>   tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
>   tools/libxl/libxl_types.idl |    1 +
>   tools/libxl/xl_cmdimpl.c    |    2 ++
>   5 files changed, 36 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_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..aa8e131 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", "-device","ich9-usb-uhci1,"
> +                    "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
> +                    "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
> +                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
> +                    "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
> +                    "addr=0x1d.0x2", NULL);

I'm just curious, why is this so complicated?  Is this likely to be 
fragile and break in the future?

Also, it's worth thinking a bit about the id -- maybe something slightly 
more descriptive, like "usb-hub-root" or something?

And as Dario said, you still need to add LIBXL_HAVE_USBVERSION to 
libxl.h.  The purpose of that is so that someone wants to write software 
that can compile against different versions of the library (say, a GUI 
VM manager or something), using features if they're present and not 
using them if they aren't.

> +                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))
Ian Campbell - July 12, 2013, 11:15 a.m.
On Fri, 2013-07-12 at 12:06 +0100, George Dunlap wrote:
> On 12/07/13 11:22, 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>
> > ---
> >   docs/man/xl.cfg.pod.5       |    6 ++++++
> >   tools/libxl/libxl_create.c  |    3 +++
> >   tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
> >   tools/libxl/libxl_types.idl |    1 +
> >   tools/libxl/xl_cmdimpl.c    |    2 ++
> >   5 files changed, 36 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_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..aa8e131 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", "-device","ich9-usb-uhci1,"
> > +                    "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
> > +                    "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
> > +                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
> > +                    "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
> > +                    "addr=0x1d.0x2", NULL);
> 
> I'm just curious, why is this so complicated?  Is this likely to be 
> fragile and break in the future?
> 
> Also, it's worth thinking a bit about the id -- maybe something slightly 
> more descriptive, like "usb-hub-root" or something?

Also please split into an append per -device option.

"-device","ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x1d.0x7",
"-device","ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x1d.0x0"
"-device","ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x1d.0x1"
"-device","ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x1d.0x2"

The last three could almost be a for loop:
    for (i=1; i<4; i++)
            "ich9-usb-uhci%d,masterbus=usb.o,firstport=%d,bus=pci.0%
s,addr=0x1d.%#x", i, i == 1 ? ",multifunction=on" : "", 2*(i-1), i-1;

Or something like that...

> 
> And as Dario said, you still need to add LIBXL_HAVE_USBVERSION to 
> libxl.h.  The purpose of that is so that someone wants to write software 
> that can compile against different versions of the library (say, a GUI 
> VM manager or something), using features if they're present and not 
> using them if they aren't.
> 
> > +                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))
>
Fabio Fantoni - July 12, 2013, 12:36 p.m.
Il 12/07/2013 13:06, George Dunlap ha scritto:
> On 12/07/13 11:22, 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>
>> ---
>>   docs/man/xl.cfg.pod.5       |    6 ++++++
>>   tools/libxl/libxl_create.c  |    3 +++
>>   tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
>>   tools/libxl/libxl_types.idl |    1 +
>>   tools/libxl/xl_cmdimpl.c    |    2 ++
>>   5 files changed, 36 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_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..aa8e131 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", 
>> "-device","ich9-usb-uhci1,"
>> + "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
>> + "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
>> +                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
>> + "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
>> +                    "addr=0x1d.0x2", NULL);
>
> I'm just curious, why is this so complicated?  Is this likely to be 
> fragile and break in the future?
>
> Also, it's worth thinking a bit about the id -- maybe something 
> slightly more descriptive, like "usb-hub-root" or something?

I tried already but there are problems with retrocompatibility:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg00491.html
I was also asking if it is possible to remove some hardcoded options 
without breaking something but I had no reply.

>
> And as Dario said, you still need to add LIBXL_HAVE_USBVERSION to 
> libxl.h.  The purpose of that is so that someone wants to write 
> software that can compile against different versions of the library 
> (say, a GUI VM manager or something), using features if they're 
> present and not using them if they aren't.

Now I understand, I'll do it in next patch version.

>
>> +                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))
>
George Dunlap - July 12, 2013, 3:33 p.m.
On 12/07/13 13:36, Fabio Fantoni wrote:
> Il 12/07/2013 13:06, George Dunlap ha scritto:
>> On 12/07/13 11:22, 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>
>>> ---
>>>   docs/man/xl.cfg.pod.5       |    6 ++++++
>>>   tools/libxl/libxl_create.c  |    3 +++
>>>   tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
>>>   tools/libxl/libxl_types.idl |    1 +
>>>   tools/libxl/xl_cmdimpl.c    |    2 ++
>>>   5 files changed, 36 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_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..aa8e131 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", 
>>> "-device","ich9-usb-uhci1,"
>>> + "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
>>> + "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
>>> +                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
>>> + "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
>>> +                    "addr=0x1d.0x2", NULL);
>>
>> I'm just curious, why is this so complicated?  Is this likely to be 
>> fragile and break in the future?
>>
>> Also, it's worth thinking a bit about the id -- maybe something 
>> slightly more descriptive, like "usb-hub-root" or something?
>
> I tried already but there are problems with retrocompatibility:
> http://lists.xen.org/archives/html/xen-devel/2013-07/msg00491.html
> I was also asking if it is possible to remove some hardcoded options 
> without breaking something but I had no reply.

So this seems to be a response to the first paragraph ("why is this so 
complicated, is it fragile").  What about the second question -- 
"id=usb" instead of "id=usb-hub-root"?  Or was there something in the 
referenced thread I'm not understanding?

  -George
Fabio Fantoni - July 15, 2013, 9:10 a.m.
Il 12/07/2013 17:33, George Dunlap ha scritto:
> On 12/07/13 13:36, Fabio Fantoni wrote:
>> Il 12/07/2013 13:06, George Dunlap ha scritto:
>>> On 12/07/13 11:22, 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>
>>>> ---
>>>>   docs/man/xl.cfg.pod.5       |    6 ++++++
>>>>   tools/libxl/libxl_create.c  |    3 +++
>>>>   tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
>>>>   tools/libxl/libxl_types.idl |    1 +
>>>>   tools/libxl/xl_cmdimpl.c    |    2 ++
>>>>   5 files changed, 36 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_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..aa8e131 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", 
>>>> "-device","ich9-usb-uhci1,"
>>>> + "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
>>>> + "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
>>>> +                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
>>>> + "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
>>>> +                    "addr=0x1d.0x2", NULL);
>>>
>>> I'm just curious, why is this so complicated?  Is this likely to be 
>>> fragile and break in the future?
>>>
>>> Also, it's worth thinking a bit about the id -- maybe something 
>>> slightly more descriptive, like "usb-hub-root" or something?
>>
>> I tried already but there are problems with retrocompatibility:
>> http://lists.xen.org/archives/html/xen-devel/2013-07/msg00491.html
>> I was also asking if it is possible to remove some hardcoded options 
>> without breaking something but I had no reply.
>
> So this seems to be a response to the first paragraph ("why is this so 
> complicated, is it fragile").  What about the second question -- 
> "id=usb" instead of "id=usb-hub-root"?  Or was there something in the 
> referenced thread I'm not understanding?
>
>  -George
>
Sorry for the forgetfulness.
id=usb is short and I see it also in kvm with libvirt where on usb 
redirection channels are working even if the usb bus is not specified.
Same thing tested and working on xen, probably take usb bus id as default.
May I have to investigate further about it?
Ian Jackson - July 18, 2013, 11:09 a.m.
Fabio Fantoni writes ("Re: [PATCH v3] libxl: usb2 and usb3 controller support for upstream qemu"):
> Il 12/07/2013 17:33, George Dunlap ha scritto:
> > On 12/07/13 13:36, Fabio Fantoni wrote:
[someone wrote:]
> >>> I'm just curious, why is this so complicated?  Is this likely to be 
> >>> fragile and break in the future?
...
> >> I tried already but there are problems with retrocompatibility:
> >> http://lists.xen.org/archives/html/xen-devel/2013-07/msg00491.html
> >> I was also asking if it is possible to remove some hardcoded options 
> >> without breaking something but I had no reply.
> >
> > So this seems to be a response to the first paragraph ("why is this so 
> > complicated, is it fragile").

I'm afraid that I don't think it's really a sufficient response to
"why is this so complicated, is it fragile?".  "I don't know" is not
very convincing :-).

My worry would be that these options would change their meaning in the
future, or indeed that the whole edifice which requires callers to
specify things at this excruciating level of detail might (sensibly!)
be abolished.

Ian.
George Dunlap - July 18, 2013, 11:13 a.m.
On 18/07/13 12:09, Ian Jackson wrote:
> Fabio Fantoni writes ("Re: [PATCH v3] libxl: usb2 and usb3 controller support for upstream qemu"):
>> Il 12/07/2013 17:33, George Dunlap ha scritto:
>>> On 12/07/13 13:36, Fabio Fantoni wrote:
> [someone wrote:]
>>>>> I'm just curious, why is this so complicated?  Is this likely to be
>>>>> fragile and break in the future?
> ...
>>>> I tried already but there are problems with retrocompatibility:
>>>> http://lists.xen.org/archives/html/xen-devel/2013-07/msg00491.html
>>>> I was also asking if it is possible to remove some hardcoded options
>>>> without breaking something but I had no reply.
>>> So this seems to be a response to the first paragraph ("why is this so
>>> complicated, is it fragile").
> I'm afraid that I don't think it's really a sufficient response to
> "why is this so complicated, is it fragile?".  "I don't know" is not
> very convincing :-).
>
> My worry would be that these options would change their meaning in the
> future, or indeed that the whole edifice which requires callers to
> specify things at this excruciating level of detail might (sensibly!)
> be abolished.

So far qemu has been pretty good about supporting deprecated 
command-line arguments; e.g., libxl still passes outdated usb parameters 
to qemu-xen.

  -George
Anthony Liguori - July 18, 2013, 11:31 a.m.
On Thu, Jul 18, 2013 at 6:09 AM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Fabio Fantoni writes ("Re: [PATCH v3] libxl: usb2 and usb3 controller support for upstream qemu"):
>> Il 12/07/2013 17:33, George Dunlap ha scritto:
>> > On 12/07/13 13:36, Fabio Fantoni wrote:
> [someone wrote:]
>> >>> I'm just curious, why is this so complicated?  Is this likely to be
>> >>> fragile and break in the future?
> ...
>> >> I tried already but there are problems with retrocompatibility:
>> >> http://lists.xen.org/archives/html/xen-devel/2013-07/msg00491.html
>> >> I was also asking if it is possible to remove some hardcoded options
>> >> without breaking something but I had no reply.
>> >
>> > So this seems to be a response to the first paragraph ("why is this so
>> > complicated, is it fragile").
>
> I'm afraid that I don't think it's really a sufficient response to
> "why is this so complicated, is it fragile?".  "I don't know" is not
> very convincing :-).

It's a multi-function PCI device that provides separate functions for
each legacy interface.  There are three distinct UHCI interfaces
presumably to enable supporting more ports.

In an ideal world, we would support this by having a single -device
ich-usb that did the right thing but we don't really have a way to
model multifunction devices as a single entity today.  Hence, the
complex command line.

The command line interface is a stable interface in QEMU.  This
interface will not change over time.

> My worry would be that these options would change their meaning in the
> future, or indeed that the whole edifice which requires callers to
> specify things at this excruciating level of detail might (sensibly!)
> be abolished.

Patches are welcome :-)  Seriously, I would love for someone to do
proper multi-function support such that we could support hotplugging a
multi-function device.

The current way of creating each function will always be supported though.

Regards,

Anthony Liguori

> Ian.
>
Ian Jackson - July 18, 2013, 12:15 p.m.
Anthony Liguori writes ("Re: [Qemu-devel] [PATCH v3] libxl: usb2 and usb3 controller support for upstream qemu"):
> The current way of creating each function will always be supported though.

Thanks for that reply.  I'm reassured.

Ian.
Andreas Färber - July 18, 2013, 12:31 p.m.
Am 12.07.2013 13:06, schrieb George Dunlap:
> On 12/07/13 11:22, 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>
>> ---
>>   docs/man/xl.cfg.pod.5       |    6 ++++++
>>   tools/libxl/libxl_create.c  |    3 +++
>>   tools/libxl/libxl_dm.c      |   25 ++++++++++++++++++++++++-
>>   tools/libxl/libxl_types.idl |    1 +
>>   tools/libxl/xl_cmdimpl.c    |    2 ++
>>   5 files changed, 36 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_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..aa8e131 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",
>> "-device","ich9-usb-uhci1,"
>> +                   
>> "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
>> +                   
>> "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
>> +                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
>> +                   
>> "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
>> +                    "addr=0x1d.0x2", NULL);
> 
> I'm just curious, why is this so complicated?  Is this likely to be
> fragile and break in the future?

As pointed out previously, the bus=pci.0 bit will break with different
PCI host bridges, such as the q35 machine existing today (-M q35 uses
pcie.0 instead and it has been discussed to make q35 the default at some
point). I had thus suggested to use a variable for the bus name to
abstract it. For -M pc-i440fx-1.5 etc. pci.0 should continue to work.

Regards,
Andreas
Paolo Bonzini - July 18, 2013, 12:35 p.m.
Il 18/07/2013 14:31, Andreas Färber ha scritto:
>> > I'm just curious, why is this so complicated?  Is this likely to be
>> > fragile and break in the future?
> As pointed out previously, the bus=pci.0 bit will break with different
> PCI host bridges, such as the q35 machine existing today (-M q35 uses
> pcie.0 instead and it has been discussed to make q35 the default at some
> point). I had thus suggested to use a variable for the bus name to
> abstract it. For -M pc-i440fx-1.5 etc. pci.0 should continue to work.

I think if we ever made a PCIe machine the default, it would be
different from what today's q35.  In particular it probably should
include a default DMI-to-PCI bridge, so as to make the command line
compatible with i440FX-based machines.

Paolo
Fabio Fantoni - Aug. 14, 2013, 10:30 a.m.
Il 18/07/2013 14:35, Paolo Bonzini ha scritto:
> Il 18/07/2013 14:31, Andreas Färber ha scritto:
>>>> I'm just curious, why is this so complicated?  Is this likely to be
>>>> fragile and break in the future?
>> As pointed out previously, the bus=pci.0 bit will break with different
>> PCI host bridges, such as the q35 machine existing today (-M q35 uses
>> pcie.0 instead and it has been discussed to make q35 the default at some
>> point). I had thus suggested to use a variable for the bus name to
>> abstract it. For -M pc-i440fx-1.5 etc. pci.0 should continue to work.
> I think if we ever made a PCIe machine the default, it would be
> different from what today's q35.  In particular it probably should
> include a default DMI-to-PCI bridge, so as to make the command line
> compatible with i440FX-based machines.
>
> Paolo

FWIK q35 is not supported now on xen.
Anyone on this?

Based on Anthony Liguori reply the usb2 hardcoded parameter should be 
stable and will be supported in future:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg01692.html

There is also reply of Ian Jackson:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg01702.html

I had also posted v4 of patch:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg01101.html


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); seems
windows bugs.

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

About usb3 seems that qemu does not support some functionalities for now.

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



There wasn't any reply about this for one month. usb1 controller is 
already supported by xen but it has some problems with latest windows. 
usb2 works with both linux and windows but it needs this patch or a 
similar one to add support on xen. usb3 can be removed for now if 
someone think unimplemented functions are a problem.
Thanks for any reply.
Fabio Fantoni - Sept. 11, 2013, 10:14 a.m.
Il 14/08/2013 12:30, Fabio Fantoni ha scritto:
> Il 18/07/2013 14:35, Paolo Bonzini ha scritto:
>> Il 18/07/2013 14:31, Andreas Färber ha scritto:
>>>>> I'm just curious, why is this so complicated?  Is this likely to be
>>>>> fragile and break in the future?
>>> As pointed out previously, the bus=pci.0 bit will break with different
>>> PCI host bridges, such as the q35 machine existing today (-M q35 uses
>>> pcie.0 instead and it has been discussed to make q35 the default at 
>>> some
>>> point). I had thus suggested to use a variable for the bus name to
>>> abstract it. For -M pc-i440fx-1.5 etc. pci.0 should continue to work.
>> I think if we ever made a PCIe machine the default, it would be
>> different from what today's q35.  In particular it probably should
>> include a default DMI-to-PCI bridge, so as to make the command line
>> compatible with i440FX-based machines.
>>
>> Paolo
>
> FWIK q35 is not supported now on xen.
> Anyone on this?
>
> Based on Anthony Liguori reply the usb2 hardcoded parameter should be 
> stable and will be supported in future:
> http://lists.xen.org/archives/html/xen-devel/2013-07/msg01692.html
>
> There is also reply of Ian Jackson:
> http://lists.xen.org/archives/html/xen-devel/2013-07/msg01702.html
>
> I had also posted v4 of patch:
> http://lists.xen.org/archives/html/xen-devel/2013-07/msg01101.html
>
>
> 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); seems
> windows bugs.
>
> - 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)
>
> About usb3 seems that qemu does not support some functionalities for now.
>
> 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
>
>
>
> There wasn't any reply about this for one month. usb1 controller is 
> already supported by xen but it has some problems with latest windows. 
> usb2 works with both linux and windows but it needs this patch or a 
> similar one to add support on xen. usb3 can be removed for now if 
> someone think unimplemented functions are a problem.
> Thanks for any reply.

Ping
Ian Campbell - Sept. 11, 2013, 10:19 a.m.
On Wed, 2013-09-11 at 12:14 +0200, Fabio Fantoni wrote:
> Ping

Is this directed and Xen or qmeu folks?

What are the outstanding questions which need to be answered?

It doesn't look to me like Ian's concerns from the (great?) grandparent
have been addressed? On the other hand I appear to be missing some of
the mails in this thread.

Ian.
Fabio Fantoni - Sept. 11, 2013, 11:38 a.m.
Il 11/09/2013 12:19, Ian Campbell ha scritto:
> On Wed, 2013-09-11 at 12:14 +0200, Fabio Fantoni wrote:
>> Ping
> Is this directed and Xen or qmeu folks?
>
> What are the outstanding questions which need to be answered?
>
> It doesn't look to me like Ian's concerns from the (great?) grandparent
> have been addressed? On the other hand I appear to be missing some of
> the mails in this thread.
>
> Ian.
>
>

I quoted the complete mail:
http://lists.xen.org/archives/html/xen-devel/2013-08/msg01425.html

The ping was directed to xen-devel, I added qemu-devel to get some 
definite answers about qemu hardcoded parameters on which you have doubts.
There were answers from qemu developersand after also positive reply 
from Ian Jackson.
I did a v4 of this patch with the missed advices but no more replies 
after qemu parameters question that seems solved on this old thread.

All details are on mail linked above.

Thanks for any reply and sorry for my bad english.
Fabio Fantoni - Sept. 11, 2013, 1:19 p.m.
Il 11/09/2013 13:38, Fabio Fantoni ha scritto:
> Il 11/09/2013 12:19, Ian Campbell ha scritto:
>> On Wed, 2013-09-11 at 12:14 +0200, Fabio Fantoni wrote:
>>> Ping
>> Is this directed and Xen or qmeu folks?
>>
>> What are the outstanding questions which need to be answered?
>>
>> It doesn't look to me like Ian's concerns from the (great?) grandparent
>> have been addressed? On the other hand I appear to be missing some of
>> the mails in this thread.
>>
>> Ian.
>>
>>
>
> I quoted the complete mail:
> http://lists.xen.org/archives/html/xen-devel/2013-08/msg01425.html
>
> The ping was directed to xen-devel, I added qemu-devel to get some 
> definite answers about qemu hardcoded parameters on which you have 
> doubts.
> There were answers from qemu developersand after also positive reply 
> from Ian Jackson.
> I did a v4 of this patch with the missed advices but no more replies 
> after qemu parameters question that seems solved on this old thread.
>
> All details are on mail linked above.
>
> Thanks for any reply and sorry for my bad english.

To clarify my previous mail the mainly question is:
Is the patch v4 ok or does it need other improvement?
The only doubt I have about is on usb3 controller support. I'm in doubt 
if to add it or not because some parts of it were included only in the 
latest versions of qemu, and from my recents test maybe stillincomplete 
somewhere.

Thanks for any reply.

Patch

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_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..aa8e131 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", "-device","ich9-usb-uhci1,"
+                    "masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,"
+                    "addr=0x1d.0x0","-device","ich9-usb-uhci2,masterbus=usb.0,"
+                    "firstport=2,bus=pci.0,addr=0x1d.0x1", "-device",
+                    "ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,"
+                    "addr=0x1d.0x2", 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))