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

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

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