[Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Paolo Bonzini]

Il 04/12/2013 21:24, Alexander Graf ha scritto:
> 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 {
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

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

isapc has the same issue: two onboard isa-ide devices, each providing a
bus, both buses named ide.0.

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

Commit message should explain more clearly how and when this affects bus
names.

Patch breaks isapc:

    $ qemu -nodefaults -S -display none -monitor stdio -M isapc -drive if=none,id=drive0 -device ide-cd,drive=drive0
    (qemu) Segmentation fault (core dumped)

Debugging a bit:

    (gdb) bt
    #0  0x000055555572e745 in ide_get_geometry (bus=0x0, unit=0, cyls=
        0x7fffffffdb8a, heads=0x7fffffffdb88 "\210\271qU", secs=
        0x7fffffffdb89 "\271qU") at /home/armbru/work/qemu/hw/ide/qdev.c:129
    #1  0x00005555558f1fed in pc_cmos_init_late (opaque=0x55555628b420 <arg.29452>)
        at /home/armbru/work/qemu/hw/i386/pc.c:336
    #2  0x0000555555898abc in qemu_devices_reset ()
        at /home/armbru/work/qemu/vl.c:1836
    #3  0x0000555555898b28 in qemu_system_reset (report=false)
        at /home/armbru/work/qemu/vl.c:1845
    #4  0x00005555558a0640 in main (argc=13, argv=0x7fffffffe048, envp=
        0x7fffffffe0b8) at /home/armbru/work/qemu/vl.c:4344
    (gdb) p arg->idebus
    $1 = {0x555556322e10, 0x0}
    (gdb) p i
    $2 = 2

Looks like your patch kills the second isa-ide somehow.

Your commit message doesn't state your command line, so I had to figure
out a PPC example myself:

    $ qemu-system-ppc -M mac99 -nodefaults -S -display none -monitor stdio -drive if=none,id=drive0 -device ide-cd,drive=drive0,bus=ide.0

"info qtree" before your patch:

      dev: macio-ide, id ""
        irq 2
        mmio ffffffffffffffff/0000000000001000
        bus: ide.0
          type IDE
          dev: ide-cd, id ""
            drive = drive0
            logical_block_size = 512
            physical_block_size = 512
            min_io_size = 0
            opt_io_size = 0
            bootindex = -1
            discard_granularity = 512
            ver = "1.7.50"
            wwn = 0x0
            serial = "QM00003"
            model = <null>
            unit = 0
      dev: macio-ide, id ""
        irq 2
        mmio ffffffffffffffff/0000000000001000
        bus: ide.0
          type IDE

After:

      dev: macio-ide, id ""
        irq 2
        mmio ffffffffffffffff/0000000000001000
        bus: ide.1
          type IDE
      dev: macio-ide, id ""
        irq 2
        mmio ffffffffffffffff/0000000000001000
        bus: ide.0
          type IDE
          dev: ide-cd, id ""
            drive = drive0
            logical_block_size = 512
            physical_block_size = 512
            min_io_size = 0
            opt_io_size = 0
            bootindex = -1
            discard_granularity = 512
            ver = "1.7.50"
            wwn = 0x0
            serial = "QM00001"
            model = <null>
            unit = 0

Incompatible change: device ide-cd moved to a different controller.
Great fun when you try to live migrate across your patch.

I'd expect isapc to have the same issue once its crash bug is fixed.

First law of QEMU hacking: if your patch looks simple, it's probably
wrong ;)

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

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

What I don't like about the global counter: we define the board's ABI
implicitly by device initialization order.  Bad taste and fragile, but
we do this elsewhere, too, e.g. pci_create_simple(bugs, -1, ...).

Wanted: tests to catch accidental ABI changes, covering at least the
parts we define implicitly.

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Paolo Bonzini]

Il 05/12/2013 10:44, Markus Armbruster ha scritto:
> Incompatible change: device ide-cd moved to a different controller.

Yes, it should be stated in the commit message but it's expected as
discussed yesterday on IRC.  The solution is not to use "-device" (which
was broken) if you care about backwards compatibility; use "-drive if=ide".

> Great fun when you try to live migrate across your patch.
> 
> I'd expect isapc to have the same issue once its crash bug is fixed.
> 
> First law of QEMU hacking: if your patch looks simple, it's probably
> wrong ;)

Yes, the question is how wrong and how the wrong balances the right.

Paolo

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/12/2013 10:44, Markus Armbruster ha scritto:
>> Incompatible change: device ide-cd moved to a different controller.
>
> Yes, it should be stated in the commit message but it's expected as
> discussed yesterday on IRC.  The solution is not to use "-device" (which
> was broken) if you care about backwards compatibility; use "-drive if=ide".

-device is broken for the *other* controller.  It works just fine for
this one.

>> Great fun when you try to live migrate across your patch.
>> 
>> I'd expect isapc to have the same issue once its crash bug is fixed.
>> 
>> First law of QEMU hacking: if your patch looks simple, it's probably
>> wrong ;)
>
> Yes, the question is how wrong and how the wrong balances the right.

Is it really too much bother to change the ide.0 name for the
controllers that bus=ide.0 doesn't use, and keep it for the one it does
use?

If yes, the incompatible change needs to be documented much more clearly
in the commit message.

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Paolo Bonzini]

Il 05/12/2013 12:20, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 05/12/2013 10:44, Markus Armbruster ha scritto:
>>> Incompatible change: device ide-cd moved to a different controller.
>>
>> Yes, it should be stated in the commit message but it's expected as
>> discussed yesterday on IRC.  The solution is not to use "-device" (which
>> was broken) if you care about backwards compatibility; use "-drive if=ide".
> 
> -device is broken for the *other* controller.  It works just fine for
> this one.

It is broken in that "-device bus=ide.0" would access the second
controller, the one corresponding to "-drive if=ide,bus=1", or -hdc/-hdd.

>>> Great fun when you try to live migrate across your patch.
>>>
>>> I'd expect isapc to have the same issue once its crash bug is fixed.
>>>
>>> First law of QEMU hacking: if your patch looks simple, it's probably
>>> wrong ;)
>>
>> Yes, the question is how wrong and how the wrong balances the right.
> 
> Is it really too much bother to change the ide.0 name for the
> controllers that bus=ide.0 doesn't use, and keep it for the one it does
> use?

Yes, for two reasons:

(1) practical reason, the one mentioned above: it would mean that
"-device bus=ide.0" corresponds to "-drive if=ide,bus=1" and similarly
for ide.1/bus=0.  So we would make the cure worse than the disease, in
my opinion.  This IMO is a pretty strong sign that the
backwards-compatibility problem doesn't exist and no one ever used
"-device" for built-in devices on anything other than pc IDE and pseries
SCSI.

(2) technical reason: the two are inverted because bus names currently
have a "last wins" policy.  The policy is implemented by using
QTAILQ_INSERT_HEAD in bus_add_child.  So it is not possible to know the
correct bus names unless you know how many buses you will have (e.g. for
3 buses you'd start giving numbers from ide.2 and go down from there).
And implementing this would probably be really really ugly.

> If yes, the incompatible change needs to be documented much more clearly
> in the commit message.

And in the release notes.

Paolo

[Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Actually [PATCH v3].

Alexander Graf <agraf@suse.de> writes:

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

Please add:

    This breaks migration unless you change bus=ide.0 to bus=ide.1 on
    the destination.

Should be mentioned in release notes.  Do we have a place where we
collect release notes as we go?

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

Yes.  Bus ID "i2c-bus.0" isn't exposed to the user or the guest, so
changing its interpretation is harmless.

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

"from old->new"?  Do you mean "for old->new"?

Suggest

    -> Makes -device bus= work for all virtio-mmio buses.  Breaks
       migration.  Workaround for migration from old to new: specify
       virtio-mmio-bus.4 or .32 respectively rather than .0 on the
       destination.

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

Err, what's part of this patch is adapting pc_init1() for the changed
bus name, so it doesn't break.  No need to mention what you're not
breaking.  But do mention what you're breaking: migration with
bus=ide.0, exactly like in your Mac example.

Please replace by something like this:

    -> Makes -device bus= work for all IDE buses.  Breaks migration.
       Workaround for migration from old to new: specify ide.1 rather
       than ide.0 on the destination.

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

Are you sure?  I believe these are affected exactly like isapc.

>    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

Again, just like isapc.

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

Patch looks good.

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Paolo Bonzini]

Il 04/02/2014 11:33, Markus Armbruster ha scritto:
>
>     This breaks migration unless you change bus=ide.0 to bus=ide.1 on
>     the destination.
>
> Should be mentioned in release notes.  Do we have a place where we
> collect release notes as we go?

Yes, http://wiki.qemu.org/ChangeLog/Next

Paolo

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/02/2014 11:33, Markus Armbruster ha scritto:
>>
>>     This breaks migration unless you change bus=ide.0 to bus=ide.1 on
>>     the destination.
>>
>> Should be mentioned in release notes.  Do we have a place where we
>> collect release notes as we go?
>
> Yes, http://wiki.qemu.org/ChangeLog/Next

I can record this change there, but I don't seem to have an account.

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Paolo Bonzini]

Il 04/02/2014 13:14, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 04/02/2014 11:33, Markus Armbruster ha scritto:
>>>
>>>     This breaks migration unless you change bus=ide.0 to bus=ide.1 on
>>>     the destination.
>>>
>>> Should be mentioned in release notes.  Do we have a place where we
>>> collect release notes as we go?
>>
>> Yes, http://wiki.qemu.org/ChangeLog/Next
>
> I can record this change there, but I don't seem to have an account.

Created one, will send account details offlist.

Paolo

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/02/2014 13:14, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 04/02/2014 11:33, Markus Armbruster ha scritto:
>>>>
>>>>     This breaks migration unless you change bus=ide.0 to bus=ide.1 on
>>>>     the destination.
>>>>
>>>> Should be mentioned in release notes.  Do we have a place where we
>>>> collect release notes as we go?
>>>
>>> Yes, http://wiki.qemu.org/ChangeLog/Next
>>
>> I can record this change there, but I don't seem to have an account.
>
> Created one, will send account details offlist.

I'll update the page when the patch gets committed.

Alex, want me to repost it with the commit message amended as outlined
in my review?

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Alexander Graf]

On 06.02.2014, at 15:41, Markus Armbruster <armbru@redhat.com> wrote:

> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 04/02/2014 13:14, Markus Armbruster ha scritto:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> 
>>>> Il 04/02/2014 11:33, Markus Armbruster ha scritto:
>>>>> 
>>>>>    This breaks migration unless you change bus=ide.0 to bus=ide.1 on
>>>>>    the destination.
>>>>> 
>>>>> Should be mentioned in release notes.  Do we have a place where we
>>>>> collect release notes as we go?
>>>> 
>>>> Yes, http://wiki.qemu.org/ChangeLog/Next
>>> 
>>> I can record this change there, but I don't seem to have an account.
>> 
>> Created one, will send account details offlist.
> 
> I'll update the page when the patch gets committed.
> 
> Alex, want me to repost it with the commit message amended as outlined
> in my review?

Yes, please. I'm sure if I change it you'll find some nits again.


Alex

Re: [Qemu-devel] [PATCH] qdev: Keep global allocation counter per bus

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 06.02.2014, at 15:41, Markus Armbruster <armbru@redhat.com> wrote:
>
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 04/02/2014 13:14, Markus Armbruster ha scritto:
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>> 
>>>>> Il 04/02/2014 11:33, Markus Armbruster ha scritto:
>>>>>> 
>>>>>>    This breaks migration unless you change bus=ide.0 to bus=ide.1 on
>>>>>>    the destination.
>>>>>> 
>>>>>> Should be mentioned in release notes.  Do we have a place where we
>>>>>> collect release notes as we go?
>>>>> 
>>>>> Yes, http://wiki.qemu.org/ChangeLog/Next
>>>> 
>>>> I can record this change there, but I don't seem to have an account.
>>> 
>>> Created one, will send account details offlist.
>> 
>> I'll update the page when the patch gets committed.
>> 
>> Alex, want me to repost it with the commit message amended as outlined
>> in my review?
>
> Yes, please. I'm sure if I change it you'll find some nits again.

Believe me, I felt bad about nit-picking your commit message *again*.  I
would've refrained if it wasn't about breaking backwards compatibility.
I'm not overly adverse to that, but I am a stickler for describing
exactly what gets broken.  Hope I didn't try your patience too much...

Anyway, v4 just sent.