From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:33218)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Ian.Campbell@citrix.com>) id 1UxbJo-0005dJ-0w
	for qemu-devel@nongnu.org; Fri, 12 Jul 2013 07:15:25 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <Ian.Campbell@citrix.com>) id 1UxbJi-00087i-Ko
	for qemu-devel@nongnu.org; Fri, 12 Jul 2013 07:15:19 -0400
Received: from smtp.citrix.com ([66.165.176.89]:27498)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <Ian.Campbell@citrix.com>) id 1UxbJi-00087T-Ay
	for qemu-devel@nongnu.org; Fri, 12 Jul 2013 07:15:14 -0400
Message-ID: <1373627711.12223.25.camel@hastur.hellion.org.uk>
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 12 Jul 2013 12:15:11 +0100
In-Reply-To: <51DFE351.4000102@eu.citrix.com>
References: <1373624555-4403-1-git-send-email-fabio.fantoni@m2r.biz>
	<51DFE351.4000102@eu.citrix.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v3] libxl: usb2 and usb3 controller support
 for upstream qemu
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: George Dunlap <george.dunlap@eu.citrix.com>
Cc: xen-devel@lists.xensource.com, Stefano.Stabellini@eu.citrix.com, wei.lui2@citrix.com, Ian.Jackson@eu.citrix.com, qemu-devel@nongnu.org, Fabio Fantoni <fabio.fantoni@m2r.biz>, pbonzini@redhat.com

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

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH v3] libxl: usb2 and usb3 controller support
 for upstream qemu
Date: Fri, 12 Jul 2013 12:15:11 +0100
Message-ID: <1373627711.12223.25.camel@hastur.hellion.org.uk>
References: <1373624555-4403-1-git-send-email-fabio.fantoni@m2r.biz>
	<51DFE351.4000102@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Return-path: <qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org>
In-Reply-To: <51DFE351.4000102@eu.citrix.com>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
To: George Dunlap <george.dunlap@eu.citrix.com>
Cc: xen-devel@lists.xensource.com, Stefano.Stabellini@eu.citrix.com, wei.lui2@citrix.com, Ian.Jackson@eu.citrix.com, qemu-devel@nongnu.org, Fabio Fantoni <fabio.fantoni@m2r.biz>, pbonzini@redhat.com
List-Id: xen-devel@lists.xenproject.org

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