Re: [PATCH-for-9.0 v2 7/8] hw/arm/bcm2836: Move code after error checks

[Peter Maydell]

On Thu, 23 Nov 2023 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> First run the code that can return errors, then on success
> run what alters the instance state.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/bcm2836.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 03e6eb2fb2..e56935f3e5 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -119,13 +119,6 @@ static void bcm2836_realize(DeviceState *dev, Error 
> **errp)
>          return;
>      }
>
> -    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, bc->ctrl_base);
> -
> -    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
> -        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
> -    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
> -        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0));
> -
>      for (n = 0; n < BCM283X_NCPUS; n++) {
>          object_property_set_int(OBJECT(&s->cpu[n].core), "mp-affinity",
>                                  (bc->clusterid << 8) | n, &error_abort);
> @@ -158,6 +151,13 @@ static void bcm2836_realize(DeviceState *dev, Error 
> **errp)
>          qdev_connect_gpio_out(DEVICE(&s->cpu[n].core), GTIMER_SEC,
>                  qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq", n));
>      }
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, bc->ctrl_base);
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
> +                    qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 
> 0));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
> +                    qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 
> 0));
>  }
>

There's no particular harm in moving the code, but given the loop
we are already doing some IRQ-connection work before doing some
things which might fail.

We don't in general do a particularly consistent job with
tidying up in realize methods that fail halfway through, but
in general "connect an IRQ between two devices both of which
are owned by this container device" and "map an MR of this device
we own into a container region we also own" are not things
which affect the overall simulation, and in theory should
be cleanup-able later (maybe even automatically by refcount
if we're really lucky).

-- PMM