Re: [PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER

[Peter Maydell]

On Tue, 4 Jul 2023 at 15:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Introduce the ARM_TIMER sysbus device, exposing one output IRQ
> and a single MMIO region.
>
> arm_timer_new() is converted as QOM instance init()/finalize()
> handlers. Note in arm_timer_finalize() we release a ptimer handle
> which was previously leaked.
>
> ArmTimer is directly embedded into SP804Timer/IntegratorPIT,
> and is initialized as a QOM child.
>
> Since the timer frequency belongs to ARM_TIMER, have it hold the
> QOM property. SP804Timer/IntegratorPIT directly access it.
>
> For IntegratorPIT, each ARM_TIMER sysbus output IRQ is wired as
> input IRQ.
>
> For the SP804Timer, we add a TYPE_OR_IRQ to OR the ARM_TIMER sysbus
> output IRQs together, exposing a single output IRQ.
> The Kconfig entry have to select the OR_IRQ dependency.

How much did you test the migration compat handling in this patch?
TYPE_OR_IRQ has its own migration state, and I forget how
it works when you migrate from a machine without a device
to one that has it. Does it just work and the extra device
ends up with the same state it has at reset?

Anyway, we should list in the commit message all the
affected boards (whether that's "migration break" or
"migration compat, forwards only").

> +static int sp804_post_load(void *opaque, int version_id)
> +{
> +    SP804Timer *s = opaque;
> +
> +    if (version_id < 2) {
> +        for (unsigned i = 0; i < ARRAY_SIZE(s->timer); i++) {
> +            qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->irq_orgate), i),
> +                         s->mig_v1_level[i]);
> +        }
> +    }
> +    return 0;
> +}

Is it really OK to call qemu_set_irq() in a post_load
handler? This is going to end up causing the OR gate
to call qemu_set_irq() on whatever its output is connected
to, and there's no guarantee about migration order between
us and whatever that other device is...

thanks
-- PMM