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 <th...@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é <f4...@amsat.org>
Regards,
Phil.
