qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus
@ 2014-02-03 19:36 Alexander Graf
  2014-02-04 10:33 ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Alexander Graf @ 2014-02-03 19:36 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Paolo Bonzini, Markus Armbruster, Anthony Liguori

When we have 2 separate qdev devices that both create a qbus of the
same type without specifying a bus name or device name, we end up
with two buses of the same name, such as ide.0 on the Mac machines:

  dev: macio-ide, id ""
    bus: ide.0
      type IDE
  dev: macio-ide, id ""
    bus: ide.0
      type IDE

If we now spawn a device that connects to a ide.0 the last created
bus gets the device, with the first created bus inaccessible to the
command line.

After some discussion on IRC we concluded that the best quick fix way
forward for this is to make automated bus-class type based allocation
count a global counter. That's what this patch implements. With this
we instead get

  dev: macio-ide, id ""
    bus: ide.1
      type IDE
  dev: macio-ide, id ""
    bus: ide.0
      type IDE

on the example mentioned above.

This also means that if you did -device ...,bus=ide.0 you got a device
on the first bus (the last created one) before this patch and get that
device on the second one (the first created one) now.

This is intended and makes the bus enumeration work as expected.

As per review request follows a list of otherwise affected boards and
the reasoning for the conclusion that they are ok:

   target      machine         bus id              times
   ------      -------         ------              -----

   aarch64     n800            i2c-bus.0           2
   aarch64     n810            i2c-bus.0           2
   arm         n800            i2c-bus.0           2
   arm         n810            i2c-bus.0           2

-> Devices are created explicitly on one of the two buses, using
   s->mpu->i2c[0], so no change to the guest.

   aarch64     vexpress-a15    virtio-mmio-bus.0   4
   aarch64     vexpress-a9     virtio-mmio-bus.0   4
   aarch64     virt            virtio-mmio-bus.0   32
   arm         vexpress-a15    virtio-mmio-bus.0   4
   arm         vexpress-a9     virtio-mmio-bus.0   4
   arm         virt            virtio-mmio-bus.0   32

-> Migration drivers need to access virtio-mmio-bus.4/32 rather
   than .0 on the destination from old->new. Bugfix as it allows
   coldplug for specific buses.

   aarch64     xilinx-zynq-a9  usb-bus.0           2
   arm         xilinx-zynq-a9  usb-bus.0           2
   mips64el    fulong2e        usb-bus.0           2

-> Normal USB operation not affected. Migration driver needs command
   line to use the other bus.

   i386        isapc           ide.0               2
   x86_64      isapc           ide.0               2

-> Fix part of this patch.

   mips        mips            ide.0               2
   mips64      mips            ide.0               2
   mips64el    mips            ide.0               2
   mipsel      mips            ide.0               2

-> Not affected, the bus is not stored anywhere.

   ppc         g3beige         ide.0               2
   ppc         mac99           ide.0               2
   ppc         prep            ide.0               2
   ppc64       g3beige         ide.0               2
   ppc64       mac99           ide.0               2
   ppc64       prep            ide.0               2

-> Bugfix

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - add fix for isapc which was searching for 2 buses called "ide.0"
  - explain the semantic change more in the commit message

v2 -> v3:

  - add board list to commit message
---
 hw/core/qdev.c         | 20 +++++++++++++-------
 hw/i386/pc_piix.c      |  8 +++++++-
 include/hw/qdev-core.h |  2 ++
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 82a9123..e7985fe 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -430,27 +430,33 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id)
 static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
+    BusClass *bc;
     char *buf;
-    int i,len;
+    int i, len, bus_id;
 
     bus->parent = parent;
 
     if (name) {
         bus->name = g_strdup(name);
     } else if (bus->parent && bus->parent->id) {
-        /* parent device has id -> use it for bus name */
+        /* parent device has id -> use it plus parent-bus-id for bus name */
+        bus_id = bus->parent->num_child_bus;
+
         len = strlen(bus->parent->id) + 16;
         buf = g_malloc(len);
-        snprintf(buf, len, "%s.%d", bus->parent->id, bus->parent->num_child_bus);
+        snprintf(buf, len, "%s.%d", bus->parent->id, bus_id);
         bus->name = buf;
     } else {
-        /* no id -> use lowercase bus type for bus name */
+        /* no id -> use lowercase bus type plus global bus-id for bus name */
+        bc = BUS_GET_CLASS(bus);
+        bus_id = bc->automatic_ids++;
+
         len = strlen(typename) + 16;
         buf = g_malloc(len);
-        len = snprintf(buf, len, "%s.%d", typename,
-                       bus->parent ? bus->parent->num_child_bus : 0);
-        for (i = 0; i < len; i++)
+        len = snprintf(buf, len, "%s.%d", typename, bus_id);
+        for (i = 0; i < len; i++) {
             buf[i] = qemu_tolower(buf[i]);
+        }
         bus->name = buf;
     }
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a327d71..62a9a22 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -221,10 +221,16 @@ static void pc_init1(QEMUMachineInitArgs *args,
     } else {
         for(i = 0; i < MAX_IDE_BUS; i++) {
             ISADevice *dev;
+            char busname[] = "ide.0";
             dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
                                ide_irq[i],
                                hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]);
-            idebus[i] = qdev_get_child_bus(DEVICE(dev), "ide.0");
+            /*
+             * The ide bus name is ide.0 for the first bus and ide.1 for the
+             * second one.
+             */
+            busname[4] = '0' + i;
+            idebus[i] = qdev_get_child_bus(DEVICE(dev), busname);
         }
     }
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2c4f140..a299bc6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -172,6 +172,8 @@ struct BusClass {
     void (*reset)(BusState *bus);
     /* maximum devices allowed on the bus, 0: no limit. */
     int max_dev;
+    /* number of automatically allocated bus ids (e.g. ide.0) */
+    int automatic_ids;
 };
 
 typedef struct BusChild {
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus
@ 2013-12-04 20:24 Alexander Graf
  2013-12-05  8:58 ` Paolo Bonzini
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Alexander Graf @ 2013-12-04 20:24 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Paolo Bonzini, Markus Armbruster, Anthony Liguori

When we have 2 separate qdev devices that both create a qbus of the
same type without specifying a bus name or device name, we end up
with two buses of the same name, such as ide.0 on the Mac machines:

  dev: macio-ide, id ""
    bus: ide.0
      type IDE
  dev: macio-ide, id ""
    bus: ide.0
      type IDE

If we now spawn a device that connects to a ide.0 the last created
bus gets the device, with the first created bus inaccessible to the
command line.

After some discussion on IRC we concluded that the best quick fix way
forward for this is to make automated bus-class type based allocation
count a global counter. That's what this patch implements. With this
we instead get

  dev: macio-ide, id ""
    bus: ide.1
      type IDE
  dev: macio-ide, id ""
    bus: ide.0
      type IDE

on the example mentioned above.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Anthony Liguori <aliguori@amazon.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/core/qdev.c         | 20 +++++++++++++-------
 include/hw/qdev-core.h |  2 ++
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e374a93..959130c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -409,27 +409,33 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id)
 static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
 {
     const char *typename = object_get_typename(OBJECT(bus));
+    BusClass *bc;
     char *buf;
-    int i,len;
+    int i, len, bus_id;
 
     bus->parent = parent;
 
     if (name) {
         bus->name = g_strdup(name);
     } else if (bus->parent && bus->parent->id) {
-        /* parent device has id -> use it for bus name */
+        /* parent device has id -> use it plus parent-bus-id for bus name */
+        bus_id = bus->parent->num_child_bus;
+
         len = strlen(bus->parent->id) + 16;
         buf = g_malloc(len);
-        snprintf(buf, len, "%s.%d", bus->parent->id, bus->parent->num_child_bus);
+        snprintf(buf, len, "%s.%d", bus->parent->id, bus_id);
         bus->name = buf;
     } else {
-        /* no id -> use lowercase bus type for bus name */
+        /* no id -> use lowercase bus type plus global bus-id for bus name */
+        bc = BUS_GET_CLASS(bus);
+        bus_id = bc->automatic_ids++;
+
         len = strlen(typename) + 16;
         buf = g_malloc(len);
-        len = snprintf(buf, len, "%s.%d", typename,
-                       bus->parent ? bus->parent->num_child_bus : 0);
-        for (i = 0; i < len; i++)
+        len = snprintf(buf, len, "%s.%d", typename, bus_id);
+        for (i = 0; i < len; i++) {
             buf[i] = qemu_tolower(buf[i]);
+        }
         bus->name = buf;
     }
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f2043a6..09f8527 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -161,6 +161,8 @@ struct BusClass {
     int (*reset)(BusState *bus);
     /* maximum devices allowed on the bus, 0: no limit. */
     int max_dev;
+    /* number of automatically allocated bus ids (e.g. ide.0) */
+    int automatic_ids;
 };
 
 typedef struct BusChild {
-- 
1.8.1.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-02-06 15:12 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-03 19:36 [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus Alexander Graf
2014-02-04 10:33 ` Markus Armbruster
2014-02-04 11:48   ` Paolo Bonzini
2014-02-04 12:14     ` Markus Armbruster
2014-02-05 17:25       ` Paolo Bonzini
2014-02-06 14:41         ` Markus Armbruster
2014-02-06 14:41           ` Alexander Graf
2014-02-06 15:11             ` Markus Armbruster
  -- strict thread matches above, loose matches on Subject: below --
2013-12-04 20:24 Alexander Graf
2013-12-05  8:58 ` Paolo Bonzini
2013-12-05  9:44 ` Markus Armbruster
2013-12-05 10:33   ` Paolo Bonzini
2013-12-05 11:20     ` Markus Armbruster
2013-12-05 11:38       ` Paolo Bonzini
2013-12-05 10:23 ` Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).