[Qemu-devel] PCI: how handle multifunction / compound devices best?

[Qemu-devel] PCI: how handle multifunction / compound devices best?

[Gerd Hoffmann]

  Hi folks,

I'm still looking for a sane way to handle multifunction pci devices,
specifically a EHCI USB controller with UHCI companion controllers.

Here comes a small (incomplete[1]) two patch series to make the issue
more clear.  The first patch adds a function which creates all devices
with the properties set as needed.  The second patch adds a '-usb2'
switch to windup this in a user-friendly way.

Adding a -usb2 switch sucks though.  I'd prefer to have some way to
create such devices via -device, but without asking the user to create
all functions individually on the command line, i.e. have something
along the lines of ...

 qemu -device ich9-usb,slot=08

... instead of ...

 qemu \
  -device ich9-usb-ehci1,addr=08.7,multifunction=on,id=ehci \
  -device ich9-usb-uhci1,addr=08.0,multifunction=on,masterbus=ehci.0,firstport=0 \
  -device ich9-usb-uhci2,addr=08.1,multifunction=on,masterbus=ehci.0,firstport=2 \
  -device ich9-usb-uhci3,addr=08.2,multifunction=on,masterbus=ehci.0,firstport=4

Suggestions?

cheers,
  Gerd

[1] full series @ http://www.kraxel.org/cgit/qemu/log/?h=usb.18

Gerd Hoffmann (2):
  usb: add ich9_ehci_with_companion_init()
  usb: windup ich9_ehci_with_companion_init via -usb2 switch.

 hw/pc_piix.c    |   14 ++++++++++----
 hw/usb-ehci.c   |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb.h        |    3 +++
 qemu-options.hx |   13 +++++++++++--
 vl.c            |    3 +++
 5 files changed, 76 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/2] usb: add ich9_ehci_with_companion_init()

[Gerd Hoffmann]

Add convinience function which creates a ehci controller with three
companion uhci controllers, with all properties are setup correctly.

The guest will see this:

[root@fedora64 ~]# lspci -s2
00:02.0 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #1 (rev 03)
00:02.1 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #2 (rev 03)
00:02.2 USB Controller: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #3 (rev 03)
00:02.7 USB Controller: Intel Corporation 82801I (ICH9 Family) USB2 EHCI Controller #1 (rev 03)

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb-ehci.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb.h      |    3 +++
 2 files changed, 52 insertions(+), 0 deletions(-)

diff --git a/hw/usb-ehci.c b/hw/usb-ehci.c
index a4758f9..d24eecb 100644
--- a/hw/usb-ehci.c
+++ b/hw/usb-ehci.c
@@ -2354,6 +2354,55 @@ static void ehci_register(void)
 }
 device_init(ehci_register);
 
+int ich9_ehci_with_companion_init(PCIBus *bus, int devfn, const char *id)
+{
+    PCIDevice *ehci = NULL, *uhci1 = NULL, *uhci2 = NULL, *uhci3 = NULL;
+    char masterbus[32];
+    int slot = PCI_SLOT(devfn);
+
+    snprintf(masterbus, sizeof(masterbus), "%s.0", id);
+
+    ehci = pci_create_multifunction(bus, PCI_DEVFN(slot, 7), true,
+                                    "ich9-usb-ehci1");
+    if (NULL == ehci) {
+        goto fail;
+    }
+    ehci->qdev.id = id;
+    qdev_init_nofail(&ehci->qdev);
+
+    uhci1 = pci_create_multifunction(bus, PCI_DEVFN(slot, 0), true,
+                                     "ich9-usb-uhci1");
+    if (NULL == uhci1) {
+        goto fail;
+    }
+    qdev_prop_set_string(&uhci1->qdev, "masterbus", masterbus);
+    qdev_prop_set_uint32(&uhci1->qdev, "firstport", 0);
+    qdev_init_nofail(&uhci1->qdev);
+
+    uhci2 = pci_create_multifunction(bus, PCI_DEVFN(slot, 1), true,
+                                     "ich9-usb-uhci2");
+    if (NULL == uhci2) {
+        goto fail;
+    }
+    qdev_prop_set_string(&uhci2->qdev, "masterbus", masterbus);
+    qdev_prop_set_uint32(&uhci2->qdev, "firstport", 2);
+    qdev_init_nofail(&uhci2->qdev);
+
+    uhci3 = pci_create_multifunction(bus, PCI_DEVFN(slot, 2), true,
+                                     "ich9-usb-uhci3");
+    if (NULL == uhci3) {
+        goto fail;
+    }
+    qdev_prop_set_string(&uhci3->qdev, "masterbus", masterbus);
+    qdev_prop_set_uint32(&uhci3->qdev, "firstport", 4);
+    qdev_init_nofail(&uhci3->qdev);
+    return 0;
+
+fail:
+    return -1;
+}
+
+
 /*
  * vim: expandtab ts=4
  */
diff --git a/hw/usb.h b/hw/usb.h
index ded2de2..2f90b3a 100644
--- a/hw/usb.h
+++ b/hw/usb.h
@@ -313,6 +313,9 @@ void usb_hid_datain_cb(USBDevice *dev, void *opaque, void (*datain)(void *));
 /* usb-bt.c */
 USBDevice *usb_bt_init(HCIInfo *hci);
 
+/* usb-ehci.c */
+int ich9_ehci_with_companion_init(PCIBus *bus, int devfn, const char *id);
+
 /* usb ports of the VM */
 
 #define VM_USB_HUB_SIZE 8
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] usb: windup ich9_ehci_with_companion_init via -usb2 switch.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pc_piix.c    |   14 ++++++++++----
 qemu-options.hx |   13 +++++++++++--
 vl.c            |    3 +++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index c5c16b4..c9aed67 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -39,6 +39,7 @@
 #include "blockdev.h"
 #include "smbus.h"
 #include "xen.h"
+#include "usb.h"
 #ifdef CONFIG_XEN
 #  include <xen/hvm/hvm_info_table.h>
 #endif
@@ -134,6 +135,15 @@ static void pc_init1(ram_addr_t ram_size,
 
     pc_register_ferr_irq(isa_get_irq(13));
 
+    if (pci_enabled && usb_enabled) {
+        if (usb_enabled == 2) {
+            int slot = PCI_SLOT(piix3_devfn) + 1;
+            ich9_ehci_with_companion_init(pci_bus, PCI_DEVFN(slot, 0), "ehci");
+        } else {
+            usb_uhci_piix3_init(pci_bus, piix3_devfn + 2);
+        }
+    }
+
     pc_vga_init(pci_enabled? pci_bus: NULL);
 
     if (xen_enabled()) {
@@ -172,10 +182,6 @@ static void pc_init1(ram_addr_t ram_size,
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
                  idebus[0], idebus[1], rtc_state);
 
-    if (pci_enabled && usb_enabled) {
-        usb_uhci_piix3_init(pci_bus, piix3_devfn + 2);
-    }
-
     if (pci_enabled && acpi_enabled) {
         i2c_bus *smbus;
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 37e54ee..c59b532 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -411,7 +411,7 @@ STEXI
 ETEXI
 
 DEF("usb", 0, QEMU_OPTION_usb,
-    "-usb            enable the USB driver (will be the default soon)\n",
+    "-usb            enable the USB 1.1 driver (will be the default soon)\n",
     QEMU_ARCH_ALL)
 STEXI
 USB options:
@@ -419,7 +419,16 @@ USB options:
 
 @item -usb
 @findex -usb
-Enable the USB driver (will be the default soon)
+Enable the USB 1.1 driver (will be the default soon)
+ETEXI
+
+DEF("usb2", 0, QEMU_OPTION_usb2,
+    "-usb2           enable the USB 2.0 driver\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -usb2
+@findex -usb2
+Enable the USB 2.0 driver
 ETEXI
 
 DEF("usbdevice", HAS_ARG, QEMU_OPTION_usbdevice,
diff --git a/vl.c b/vl.c
index 52402a2..f4f23ce 100644
--- a/vl.c
+++ b/vl.c
@@ -2698,6 +2698,9 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_usb:
                 usb_enabled = 1;
                 break;
+            case QEMU_OPTION_usb2:
+                usb_enabled = 2;
+                break;
             case QEMU_OPTION_usbdevice:
                 usb_enabled = 1;
                 add_device_config(DEV_USB, optarg);
-- 
1.7.1

Re: [Qemu-devel] PCI: how handle multifunction / compound devices best?

[Anthony Liguori]

On 07/01/2011 06:27 AM, Gerd Hoffmann wrote:
>    Hi folks,
>
> I'm still looking for a sane way to handle multifunction pci devices,
> specifically a EHCI USB controller with UHCI companion controllers.
>
> Here comes a small (incomplete[1]) two patch series to make the issue
> more clear.  The first patch adds a function which creates all devices
> with the properties set as needed.  The second patch adds a '-usb2'
> switch to windup this in a user-friendly way.
>
> Adding a -usb2 switch sucks though.  I'd prefer to have some way to
> create such devices via -device, but without asking the user to create
> all functions individually on the command line, i.e. have something
> along the lines of ...
>
>   qemu -device ich9-usb,slot=08

This is the place where are current infrastructure pretty much stinks.

The first problem is that the device factory interface should be 
involved with device addressing.  We really should have:

qemu -device ich9-usb,id=uhci0 -set i440fx.slot[8]=uhci0

The device doesn't care what it's PCI slot address is.

ich9-usb should be able to create its functions too as part of its 
initialization.  I don't see an easy answer here unless we do away with 
the current bus model.  There's no way we can do composition given the 
bus model we have.

Best we could do is syntactic sugar for what you have below.

Regards,

Anthony Liguori

>
> ... instead of ...
>
>   qemu \
>    -device ich9-usb-ehci1,addr=08.7,multifunction=on,id=ehci \
>    -device ich9-usb-uhci1,addr=08.0,multifunction=on,masterbus=ehci.0,firstport=0 \
>    -device ich9-usb-uhci2,addr=08.1,multifunction=on,masterbus=ehci.0,firstport=2 \
>    -device ich9-usb-uhci3,addr=08.2,multifunction=on,masterbus=ehci.0,firstport=4
>
> Suggestions?
>
> cheers,
>    Gerd
>
> [1] full series @ http://www.kraxel.org/cgit/qemu/log/?h=usb.18
>
> Gerd Hoffmann (2):
>    usb: add ich9_ehci_with_companion_init()
>    usb: windup ich9_ehci_with_companion_init via -usb2 switch.
>
>   hw/pc_piix.c    |   14 ++++++++++----
>   hw/usb-ehci.c   |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/usb.h        |    3 +++
>   qemu-options.hx |   13 +++++++++++--
>   vl.c            |    3 +++
>   5 files changed, 76 insertions(+), 6 deletions(-)
>
>

Re: [Qemu-devel] PCI: how handle multifunction / compound devices best?

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 07/01/2011 06:27 AM, Gerd Hoffmann wrote:
>>    Hi folks,
>>
>> I'm still looking for a sane way to handle multifunction pci devices,
>> specifically a EHCI USB controller with UHCI companion controllers.
>>
>> Here comes a small (incomplete[1]) two patch series to make the issue
>> more clear.  The first patch adds a function which creates all devices
>> with the properties set as needed.  The second patch adds a '-usb2'
>> switch to windup this in a user-friendly way.
>>
>> Adding a -usb2 switch sucks though.  I'd prefer to have some way to
>> create such devices via -device, but without asking the user to create
>> all functions individually on the command line, i.e. have something
>> along the lines of ...
>>
>>   qemu -device ich9-usb,slot=08
>
> This is the place where are current infrastructure pretty much stinks.
>
> The first problem is that the device factory interface should be
> involved with device addressing.  We really should have:
>
> qemu -device ich9-usb,id=uhci0 -set i440fx.slot[8]=uhci0
>
> The device doesn't care what it's PCI slot address is.

What exactly would such a split buy us?

-device is not a message to the device, it's a message to whatever makes
devices.  Therefore, the fact that we specify the device address with
-device doesn't mean we've artificially made the device to care for its
address.

[...]

Re: [Qemu-devel] PCI: how handle multifunction / compound devices best?

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi folks,
>
> I'm still looking for a sane way to handle multifunction pci devices,
> specifically a EHCI USB controller with UHCI companion controllers.
>
> Here comes a small (incomplete[1]) two patch series to make the issue
> more clear.  The first patch adds a function which creates all devices
> with the properties set as needed.  The second patch adds a '-usb2'
> switch to windup this in a user-friendly way.
>
> Adding a -usb2 switch sucks though.  I'd prefer to have some way to
> create such devices via -device, but without asking the user to create
> all functions individually on the command line, i.e. have something
> along the lines of ...
>
>  qemu -device ich9-usb,slot=08
>
> ... instead of ...
>
>  qemu \
>   -device ich9-usb-ehci1,addr=08.7,multifunction=on,id=ehci \
>   -device ich9-usb-uhci1,addr=08.0,multifunction=on,masterbus=ehci.0,firstport=0 \
>   -device ich9-usb-uhci2,addr=08.1,multifunction=on,masterbus=ehci.0,firstport=2 \
>   -device ich9-usb-uhci3,addr=08.2,multifunction=on,masterbus=ehci.0,firstport=4
>
> Suggestions?

This special case of composition is simple enough that some kind of
macro expansion could work.

A device defines a name, properties and a bunch of methods.

A device macro could define a name, properties (the macro parameters),
and an expansion.  Expansions could be as simple as a list of -device
where the property values can also be macro parameter names.

-device MACRO,... could then be replaced by MACRO's expansion with the
property values substituted.  Use to make -device ich9-usb,... expand
into its functions.

Just an idea; I'm not claiming this is the way to go.

Device macros destroy the 1:1 relationship between -device and device
tree nodes.  Or rather what's left of it: we already have a device that
expands into multiple devices, namely usb-storage.  But it's an ad hoc
hack, which has caused us some grief.

Re: [Qemu-devel] PCI: how handle multifunction / compound devices best?

[Gerd Hoffmann]

   Hi,

> Device macros destroy the 1:1 relationship between -device and device
> tree nodes.  Or rather what's left of it: we already have a device that
> expands into multiple devices, namely usb-storage.  But it's an ad hoc
> hack, which has caused us some grief.

Exactly thats why I don't feel like adding more ad hoc hacks ...

cheers,
   Gerd