| Message ID | 1531470464-21522-7-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix crashes with introspection of ARM devices | expand |
On 13/07/2018 10:27, Thomas Huth wrote: > aux_create_slave() calls qdev_init_nofail() which in turn "realizes" > the corresponding object. Thus this most not be called from an > instance_init function. Move the code to the realize function instead. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/display/xlnx_dp.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > + s->aux_bus = aux_init_bus(dev, "aux"); aux_init_bus can remain in the same place, and likewise the qdev_create that assigns to s->edid. The only thing that has to move is the qdev_init_nofail and aux_bus_map_device, like this: ----------------- 8< ------------------ From: Paolo Bonzini <pbonzinI@redhat.com> Subject: [PATCH] hw/display/xlnx_dp: Move problematic code from instance_init to realize aux_create_slave() calls qdev_init_nofail() which in turn "realizes" the corresponding object. This is unlike qdev_create(), and it is wrong because qdev_init_nofail() must not be called from an instance_init function. Move qdev_init_nofail() and the subsequent aux_map_slave into the caller's realize function. Reported-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c index 51301220e8..589ef59dfd 100644 --- a/hw/display/xlnx_dp.c +++ b/hw/display/xlnx_dp.c @@ -1234,7 +1234,7 @@ static void xlnx_dp_init(Object *obj) /* * Initialize DPCD and EDID.. */ - s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000)); + s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd")); s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc")); i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50); @@ -1248,6 +1248,9 @@ static void xlnx_dp_realize(DeviceState *dev, Error **errp) DisplaySurface *surface; struct audsettings as; + qdev_init_nofail(DEVICE(s->dpcd)); + aux_map_slave(AUX_SLAVE(s->dpcd), 0x0000); + s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s); surface = qemu_console_surface(s->console); xlnx_dpdma_set_host_data_location(s->dpdma, DP_GRAPHIC_DMA_CHANNEL, diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c index b8a8721201..2fe807d42f 100644 --- a/hw/misc/auxbus.c +++ b/hw/misc/auxbus.c @@ -74,9 +74,11 @@ AUXBus *aux_init_bus(DeviceState *parent, const char *name) return bus; } -static void aux_bus_map_device(AUXBus *bus, AUXSlave *dev, hwaddr addr) +void aux_map_slave(AUXSlave *aux_dev, hwaddr addr) { - memory_region_add_subregion(bus->aux_io, addr, dev->mmio); + DeviceState *dev = DEVICE(aux_dev); + AUXBus *bus = AUX_BUS(qdev_get_parent_bus(dev)); + memory_region_add_subregion(bus->aux_io, addr, aux_dev->mmio); } static bool aux_bus_is_bridge(AUXBus *bus, DeviceState *dev) @@ -260,15 +262,13 @@ static void aux_slave_dev_print(Monitor *mon, DeviceState *dev, int indent) memory_region_size(s->mmio)); } -DeviceState *aux_create_slave(AUXBus *bus, const char *type, uint32_t addr) +DeviceState *aux_create_slave(AUXBus *bus, const char *type) { DeviceState *dev; dev = DEVICE(object_new(type)); assert(dev); qdev_set_parent_bus(dev, &bus->qbus); - qdev_init_nofail(dev); - aux_bus_map_device(AUX_BUS(qdev_get_parent_bus(dev)), AUX_SLAVE(dev), addr); return dev; } diff --git a/include/hw/misc/auxbus.h b/include/hw/misc/auxbus.h index 68ade8a90f..c15b444748 100644 --- a/include/hw/misc/auxbus.h +++ b/include/hw/misc/auxbus.h @@ -123,6 +123,18 @@ I2CBus *aux_get_i2c_bus(AUXBus *bus); */ void aux_init_mmio(AUXSlave *aux_slave, MemoryRegion *mmio); -DeviceState *aux_create_slave(AUXBus *bus, const char *name, uint32_t addr); +/* aux_create_slave: Create a new device on an AUX bus + * + * @bus The AUX bus for the new device. + * @name The type of the device to be created. + */ +DeviceState *aux_create_slave(AUXBus *bus, const char *name); + +/* aux_map_slave: Map the mmio for an AUX slave on the bus. + * + * @dev The AUX slave. + * @addr The address for the slave's mmio. + */ +void aux_map_slave(AUXSlave *dev, hwaddr addr); #endif /* HW_MISC_AUXBUS_H */ Paolo > + /* Initialize DPCD and EDID */ > s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000)); > s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc")); > i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50); > > fifo8_create(&s->rx_fifo, 16); > fifo8_create(&s->tx_fifo, 16); > -} > - > -static void xlnx_dp_realize(DeviceState *dev, Error **errp) > -{ > - XlnxDPState *s = XLNX_DP(dev); > - DisplaySurface *surface; > - struct audsettings as; > > s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s); > surface = qemu_console_surface(s->console); >
On 13.07.2018 13:13, Paolo Bonzini wrote: > On 13/07/2018 10:27, Thomas Huth wrote: >> aux_create_slave() calls qdev_init_nofail() which in turn "realizes" >> the corresponding object. Thus this most not be called from an >> instance_init function. Move the code to the realize function instead. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/display/xlnx_dp.c | 23 +++++++++-------------- >> 1 file changed, 9 insertions(+), 14 deletions(-) > >> + s->aux_bus = aux_init_bus(dev, "aux"); > > aux_init_bus can remain in the same place, and likewise the qdev_create > that assigns to s->edid. > > The only thing that has to move is the qdev_init_nofail and > aux_bus_map_device, like this: > > ----------------- 8< ------------------ > From: Paolo Bonzini <pbonzinI@redhat.com> > Subject: [PATCH] hw/display/xlnx_dp: Move problematic code from instance_init to realize Your patch looks good at a first quick glance, but it seems not to work as expected: When I now run QEMU like this: echo "{'execute':'qmp_capabilities'}" \ "{'execute':'device-list-properties'," \ "'arguments':{'typename':'xlnx,zynqmp'}}" \ "{'execute': 'human-monitor-command', " \ "'arguments': {'command-line': 'info qtree'}}" | \ aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio then QEMU ends up in an endless loop and I've got to kill it. Thomas
On 13/07/2018 17:59, Thomas Huth wrote: > Your patch looks good at a first quick glance, but it seems not to work as expected: When I now run QEMU like this: > > echo "{'execute':'qmp_capabilities'}" \ > "{'execute':'device-list-properties'," \ > "'arguments':{'typename':'xlnx,zynqmp'}}" \ > "{'execute': 'human-monitor-command', " \ > "'arguments': {'command-line': 'info qtree'}}" | \ > aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio > > then QEMU ends up in an endless loop and I've got to kill it. There are two more bugs that my patch makes un-latent, where the objects are created but not added as children. Therefore when you call object_unparent on them, nothing happens. In particular dpcd and edid give you an infinite loop in bus_unparent, because device_unparent is not called and does not remove them from the list of devices on the bus. The following incremental changes fix everything for me. Note that aux_create_slave/qdev_create already do the unref for you. Thanks, Paolo diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c index 589ef59dfd..6439bd05ef 100644 --- a/hw/display/xlnx_dp.c +++ b/hw/display/xlnx_dp.c @@ -1235,8 +1235,11 @@ static void xlnx_dp_init(Object *obj) * Initialize DPCD and EDID.. */ s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd")); + object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd), NULL); + s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc")); i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50); + object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid), NULL); fifo8_create(&s->rx_fifo, 16); fifo8_create(&s->tx_fifo, 16); diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c index 2fe807d42f..0e56d9a8a4 100644 --- a/hw/misc/auxbus.c +++ b/hw/misc/auxbus.c @@ -32,6 +32,7 @@ #include "hw/misc/auxbus.h" #include "hw/i2c/i2c.h" #include "monitor/monitor.h" +#include "qapi/error.h" #ifndef DEBUG_AUX #define DEBUG_AUX 0 @@ -63,9 +64,14 @@ static void aux_bus_class_init(ObjectClass *klass, void *data) AUXBus *aux_init_bus(DeviceState *parent, const char *name) { AUXBus *bus; + Object *auxtoi2c; bus = AUX_BUS(qbus_create(TYPE_AUX_BUS, parent, name)); - bus->bridge = AUXTOI2C(qdev_create(BUS(bus), TYPE_AUXTOI2C)); + auxtoi2c = object_new_with_props(TYPE_AUXTOI2C, OBJECT(bus), "i2c", + &error_abort, NULL); + qdev_set_parent_bus(DEVICE(auxtoi2c), BUS(bus)); + + bus->bridge = AUXTOI2C(auxtoi2c); /* Memory related. */ bus->aux_io = g_malloc(sizeof(*bus->aux_io));
On 13.07.2018 19:13, Paolo Bonzini wrote: > On 13/07/2018 17:59, Thomas Huth wrote: >> Your patch looks good at a first quick glance, but it seems not to work as expected: When I now run QEMU like this: >> >> echo "{'execute':'qmp_capabilities'}" \ >> "{'execute':'device-list-properties'," \ >> "'arguments':{'typename':'xlnx,zynqmp'}}" \ >> "{'execute': 'human-monitor-command', " \ >> "'arguments': {'command-line': 'info qtree'}}" | \ >> aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio >> >> then QEMU ends up in an endless loop and I've got to kill it. > > There are two more bugs that my patch makes un-latent, where the > objects are created but not added as children. Therefore when > you call object_unparent on them, nothing happens. > > In particular dpcd and edid give you an infinite loop in bus_unparent, > because device_unparent is not called and does not remove them from > the list of devices on the bus. > > The following incremental changes fix everything for me. Note that > aux_create_slave/qdev_create already do the unref for you. Thanks, that fixes the problem, indeed. I'll squash this into your patch and send out a v3 series. Thomas
diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c index 5130122..c7542d8 100644 --- a/hw/display/xlnx_dp.c +++ b/hw/display/xlnx_dp.c @@ -1225,28 +1225,23 @@ static void xlnx_dp_init(Object *obj) xlnx_dp_set_dpdma, OBJ_PROP_LINK_STRONG, &error_abort); +} - /* - * Initialize AUX Bus. - */ - s->aux_bus = aux_init_bus(DEVICE(obj), "aux"); +static void xlnx_dp_realize(DeviceState *dev, Error **errp) +{ + XlnxDPState *s = XLNX_DP(dev); + DisplaySurface *surface; + struct audsettings as; - /* - * Initialize DPCD and EDID.. - */ + s->aux_bus = aux_init_bus(dev, "aux"); + + /* Initialize DPCD and EDID */ s->dpcd = DPCD(aux_create_slave(s->aux_bus, "dpcd", 0x00000)); s->edid = I2CDDC(qdev_create(BUS(aux_get_i2c_bus(s->aux_bus)), "i2c-ddc")); i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50); fifo8_create(&s->rx_fifo, 16); fifo8_create(&s->tx_fifo, 16); -} - -static void xlnx_dp_realize(DeviceState *dev, Error **errp) -{ - XlnxDPState *s = XLNX_DP(dev); - DisplaySurface *surface; - struct audsettings as; s->console = graphic_console_init(dev, 0, &xlnx_dp_gfx_ops, s); surface = qemu_console_surface(s->console);
aux_create_slave() calls qdev_init_nofail() which in turn "realizes" the corresponding object. Thus this most not be called from an instance_init function. Move the code to the realize function instead. Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/display/xlnx_dp.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)