Re: [PATCH 13/48] hw/arm/xlnx-versal: VersalMap: add support for OR'ed IRQs

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 18 Aug 2025 at 14:30, Luc Michel <luc.michel@amd.com> wrote:
>
> Hi Peter,
>
> On 13:24 Mon 04 Aug     , Peter Maydell wrote:
> > On Wed, 16 Jul 2025 at 10:55, Luc Michel <luc.michel@amd.com> wrote:
> > >
> > > +static qemu_irq versal_get_irq_or_gate_in(Versal *s, int irq_idx,
> > > +                                          qemu_irq target_irq)
> > > +{
> > > +    Object *container = versal_get_child(s, "irq-or-gates");
> > > +    DeviceState *dev;
> > > +    g_autofree char *name;
> > > +    int idx, or_idx;
> > > +
> > > +    idx = FIELD_EX32(irq_idx, VERSAL_IRQ, IRQ);
> > > +    or_idx = FIELD_EX32(irq_idx, VERSAL_IRQ, OR_IDX);
> > > +
> > > +    name = g_strdup_printf("irq[%d]", idx);
> > > +    dev = DEVICE(object_resolve_path_at(container, name));
> > > +
> > > +    if (dev == NULL) {
> > > +        dev = qdev_new(TYPE_OR_IRQ);
> >
> > Here we create a device...
> >
> > > +        object_property_add_child(container, name, OBJECT(dev));
> > > +        qdev_prop_set_uint16(dev, "num-lines", 1 << R_VERSAL_IRQ_OR_IDX_LENGTH);
> > > +        qdev_realize_and_unref(dev, NULL, &error_abort);
> > > +        qdev_connect_gpio_out(dev, 0, target_irq);
> > > +    }
> > > +
> > > +    return qdev_get_gpio_in(dev, or_idx);
> >
> > ...but then we don't save the pointer to it, so the only
> > thing still hanging onto it is the QOM tree.
> >
> > If you want to change "embedded device struct" into
> > "allocate memory to create devices" that's fine, but the
> > SoC should still keep track of everything it's creating,
> > so that (at least in theory) it could clean it up on
> > unrealize.
>
> I'm not sure I fully understand your point:
>
>    - The OR gate device is parented to the "irq-or-gates" container,
>      which is itself parented to the SoC. So finalizing (freeing) the
>      SoC would trigger a cascade of free calls to the children,
>      including those objects right?

Ah, I hadn't noticed that we add the object as a QOM child
of the SoC here.

It does mean you can't ever get back to the OR gate object
except by walking the QOM tree, but I suppose that's OK.

It would be helpful if you could run "make check" under
the clang leak sanitizer with your patches added, to see
if it complains about anything. (Unfortunately it will
definitely complain about at least some pre-existing
leaks, I suspect.)

thanks
-- PMM