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

[George Dunlap <george.dunlap@eu.citrix.com>]

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