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