Re: [Qemu-devel] [PATCH v2] hw/arm/integratorcp: Don't do things that could be fatal in the instance_init

[Peter Maydell]

On 5 April 2018 at 06:34, Thomas Huth  wrote:
> An instance_init function must not fail - and might be called multiple times,
> e.g. during device introspection with the 'device-list-properties' QMP
> command. Since the integratorcm device ignores this rule, QEMU currently
> aborts in this case (though it really should not):
>
> echo "{'execute':'qmp_capabilities'}"\
>  "{'execute':'device-list-properties',"\
>  "'arguments':{'typename':'integrator_core'}}" \
>  | arm-softmmu/qemu-system-arm -M integratorcp,accel=qtest -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 91, "minor": 11, "major": 2},
>  "package": "build-all"}, "capabilities": []}}
> {"return": {}}
> RAMBlock "integrator.flash" already registered, abort!
> Aborted (core dumped)
>
> Move the problematic code to the realize() function instead to fix this
> problem.
>
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Thomas Huth 



Applied to target-arm.next, thanks.

-- PMM

[Qemu-devel] [PATCH v2] hw/arm/integratorcp: Don't do things that could be fatal in the instance_init

[Thomas Huth]

An instance_init function must not fail - and might be called multiple times,
e.g. during device introspection with the 'device-list-properties' QMP
command. Since the integratorcm device ignores this rule, QEMU currently
aborts in this case (though it really should not):

echo "{'execute':'qmp_capabilities'}"\
 "{'execute':'device-list-properties',"\
 "'arguments':{'typename':'integrator_core'}}" \
 | arm-softmmu/qemu-system-arm -M integratorcp,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 91, "minor": 11, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{"return": {}}
RAMBlock "integrator.flash" already registered, abort!
Aborted (core dumped)

Move the problematic code to the realize() function instead to fix this
problem.

Reviewed-by: Philippe Mathieu-Daud?? 
Signed-off-by: Thomas Huth 
---
 v2: Use "OBJECT(d)" instead of "(Object *)d"

 hw/arm/integratorcp.c | 23 +++
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index e8303b8..58b40ef 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -266,7 +266,6 @@ static const MemoryRegionOps integratorcm_ops = {
 static void integratorcm_init(Object *obj)
 {
 IntegratorCMState *s = INTEGRATOR_CM(obj);
-SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
 s->cm_osc = 0x0148;
 /* ??? What should the high bits of this value be?  */
@@ -276,20 +275,28 @@ static void integratorcm_init(Object *obj)
 s->cm_init = 0x0112;
 s->cm_refcnt_offset = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 24,
1000);
-memory_region_init_ram(>flash, obj, "integrator.flash", 0x10,
-   _fatal);
 
-memory_region_init_io(>iomem, obj, _ops, s,
-  "integratorcm", 0x0080);
-sysbus_init_mmio(dev, >iomem);
-
-integratorcm_do_remap(s);
 /* ??? Save/restore.  */
 }
 
 static void integratorcm_realize(DeviceState *d, Error **errp)
 {
 IntegratorCMState *s = INTEGRATOR_CM(d);
+SysBusDevice *dev = SYS_BUS_DEVICE(d);
+Error *local_err = NULL;
+
+memory_region_init_ram(>flash, OBJECT(d), "integrator.flash", 0x10,
+   _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+
+memory_region_init_io(>iomem, OBJECT(d), _ops, s,
+  "integratorcm", 0x0080);
+sysbus_init_mmio(dev, >iomem);
+
+integratorcm_do_remap(s);
 
 if (s->memsz >= 256) {
 integrator_spd[31] = 64;
-- 
1.8.3.1