Re: [PATCH v3 0/3] Introduce a new Write Protected pin inverted property

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

On Wed, 27 Nov 2024 at 11:26, Cédric Le Goater <clg@kaod.org> wrote:
>
>
> > Having to modify sdhci.c internals is dubious, since inversion
> > occurs out of this block. If this is the soc/board layer, isn't
> > better to model at this level? Smth like:
> >
> > -- >8 --
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index be3eb70cdd7..aad9be66b75 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> >       }
> >       aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
> >                       sc->memmap[ASPEED_DEV_SDHCI]);
> > +    irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
> >       sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
> > -                       aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
> > +                       sc->sdhci_wp_inverted ? qemu_irq_invert(irq) : irq);
> >
> >       /* eMMC */
> >       if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
> > ---
>
> Nice ! I didn't know about qemu_irq_invert().

I am not a fan of qemu_irq_invert(). It's one of those ancient
pre-QOM APIs that we ideally would get rid of at some point.
Two problems with it:

(1) It allocates and returns a qemu_irq directly,
so in the patch above you're effectively leaking that
allocation. (Not a big deal since the SoC object is going to
be around for the life of the QEMU process, but probably
the clang leak-sanitizer will complain.)

(2) It calls qemu_irq_raise() directly, immediately. This is
kind of bogus in a realize function, where you're not supposed
to be raising IRQ lines yet; it also doesn't do anything about
reset, so if the device on the other end *did* care about seeing
that 0->1 transition then it will be broken on system-reset
because the transition won't happen. (Handling "device is supposed
to have an asserted-as-1 line coming out of reset" is not
something that we do very well in QEMU generally. In theory
3-phase reset is supposed to help with this by letting you do
the assert-the-line in the reset-exit phase, but in practice we
typically just don't model the line-assertion at all and
trust that the reset state of the device on the far end is
what it ought to be anyway...)

I would not recommend using qemu_irq_invert() in new code.

I guess in an ideal world we'd implement a QOM object
that encapsulated the the "not gate" logic, similar to
TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
so it doesn't get reset properly on system reset and so
the "what happens to the output on reset" is still not
really correct.)

thanks
-- PMM