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

["Andreas Färber" <afaerber@suse.de>]

Am 18.02.2014 13:54, schrieb Markus Armbruster:
> Peter, can you merge this patch?  v2 is from December 20, and the
> changes since then have been limited to the commit message.
> 
> In case Peter doesn't want to take it directly: Andreas, would you be
> willing to take it through your tree?  It's not really QOM, but your
> tree is the closest fit I can find.

Last thing I read was some open discussions related to migration
problems, so I didn't take it yet. If those are resolved, as a qdev
patch I would take it through qom-next, if mst acks for the PC part.

Regards,
Andreas

> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> From: Alexander Graf <agraf@suse.de>
>>
>> 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.  Breaks
>> migration unless you change bus=ide.0 to bus=ide.1 on the destination.
>>
>> 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 only 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
>>
>> -> 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
>>    mips        mips            ide.0               2
>>    mips64      mips            ide.0               2
>>    mips64el    mips            ide.0               2
>>    mipsel      mips            ide.0               2
>>    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
>>
>> -> 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.
>>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Anthony Liguori <aliguori@amazon.com>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> 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
>>
>> v3 -> v4:
>>
>>   - Explain impact on migration more clearly in 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 {


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg