Re: [Qemu-devel] [PATCH v2 06/16] hw/display/xlnx_dp: Move problematic code from instance_init to realize

[Paolo Bonzini]

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 <address@hidden>
> ---
>  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 <address@hidden>
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 <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>

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