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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> ---
>  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é <address@hidden>

Regards,

Phil.