[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER
From: |
Peter Maydell |
Subject: |
Re: [PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER |
Date: |
Mon, 24 Jul 2023 16:01:24 +0100 |
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
- Re: [PATCH v2 11/19] hw/timer/arm_timer: Convert ArmTimer::freq to uint32_t type, (continued)
- [PATCH v2 14/19] hw/timer/arm_timer: Pass timer output IRQ as parameter to arm_timer_new, Philippe Mathieu-Daudé, 2023/07/04
- [PATCH v2 15/19] hw/timer/arm_timer: Fix misuse of SysBus IRQ in IntegratorPIT, Philippe Mathieu-Daudé, 2023/07/04
- [PATCH v2 12/19] hw/timer/arm_timer: Use array of frequency in SP804Timer, Philippe Mathieu-Daudé, 2023/07/04
- [PATCH v2 16/19] hw/timer/arm_timer: Extract icp_pit_realize() from icp_pit_init(), Philippe Mathieu-Daudé, 2023/07/04
- [PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER, Philippe Mathieu-Daudé, 2023/07/04
- Re: [PATCH v2 17/19] hw/timer/arm_timer: QDev'ify ARM_TIMER,
Peter Maydell <=
- [PATCH v2 18/19] hw/timer/arm_timer: Map ARM_TIMER MMIO regions into IntegratorPIT, Philippe Mathieu-Daudé, 2023/07/04
- [PATCH v2 19/19] hw/timer/arm_timer: Map ARM_TIMER MMIO regions into SP804Timer, Philippe Mathieu-Daudé, 2023/07/04
- Re: [PATCH v2 00/19] hw/timer/arm_timer: QOM'ify ARM_TIMER and correct sysbus/irq in ICP_PIT, Philippe Mathieu-Daudé, 2023/07/04