| Message ID | 1391456171-52565-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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?
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
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.
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 {
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(-)