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

Thomas Huth posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1522869306-17292-1-git-send-email-thuth@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test s390x passed
There is a newer version of this series
hw/arm/integratorcp.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH for-2.12] hw/arm/integratorcp: Don't do things that could be fatal in the instance_init
Posted by Thomas Huth 5 years, 11 months ago
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.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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..9e2df34 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 = 0x01000048;
     /* ??? What should the high bits of this value be?  */
@@ -276,20 +275,28 @@ static void integratorcm_init(Object *obj)
     s->cm_init = 0x00000112;
     s->cm_refcnt_offset = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 24,
                                    1000);
-    memory_region_init_ram(&s->flash, obj, "integrator.flash", 0x100000,
-                           &error_fatal);
 
-    memory_region_init_io(&s->iomem, obj, &integratorcm_ops, s,
-                          "integratorcm", 0x00800000);
-    sysbus_init_mmio(dev, &s->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(&s->flash, (Object *)d, "integrator.flash", 0x100000,
+                           &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    memory_region_init_io(&s->iomem, (Object *)d, &integratorcm_ops, s,
+                          "integratorcm", 0x00800000);
+    sysbus_init_mmio(dev, &s->iomem);
+
+    integratorcm_do_remap(s);
 
     if (s->memsz >= 256) {
         integrator_spd[31] = 64;
-- 
1.8.3.1


Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12] hw/arm/integratorcp: Don't do things that could be fatal in the instance_init
Posted by Philippe Mathieu-Daudé 5 years, 11 months ago
Hi Thomas,

On 04/04/2018 04:15 PM, 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  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..9e2df34 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 = 0x01000048;
>      /* ??? What should the high bits of this value be?  */
> @@ -276,20 +275,28 @@ static void integratorcm_init(Object *obj)
>      s->cm_init = 0x00000112;
>      s->cm_refcnt_offset = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), 24,
>                                     1000);
> -    memory_region_init_ram(&s->flash, obj, "integrator.flash", 0x100000,
> -                           &error_fatal);
>  
> -    memory_region_init_io(&s->iomem, obj, &integratorcm_ops, s,
> -                          "integratorcm", 0x00800000);
> -    sysbus_init_mmio(dev, &s->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(&s->flash, (Object *)d, "integrator.flash", 0x100000,

To keep consistency:

    memory_region_init_ram(&s->flash, OBJECT(d), "integrator.flash", ...

> +                           &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    memory_region_init_io(&s->iomem, (Object *)d, &integratorcm_ops, s,

    memory_region_init_io(&s->iomem, OBJECT(d), &integratorcm_ops, s,

> +                          "integratorcm", 0x00800000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +
> +    integratorcm_do_remap(s);
>  
>      if (s->memsz >= 256) {
>          integrator_spd[31] = 64;
> 

With the macro instead of the cast:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Regards,

Phil.