Re: [PATCH 4/4] via-ide: Avoid using isa_get_irq()

[BALATON Zoltan]

On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/17/21 21:39, BALATON Zoltan wrote:
> > On Sun, 17 Oct 2021, Philippe Mathieu-Daudé wrote:
> > > On 10/15/21 03:06, BALATON Zoltan wrote:
> > > > Use via_isa_set_irq() which better encapsulates irq handling in the
> > > > vt82xx model and avoids using isa_get_irq() that has a comment saying
> > > > it should not be used.
> > > >
> > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > > ---
> > > >  hw/ide/via.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/ide/via.c b/hw/ide/via.c
> > > > index 94cc2142c7..252d18f4ac 100644
> > > > --- a/hw/ide/via.c
> > > > +++ b/hw/ide/via.c
> > > > @@ -29,7 +29,7 @@
> > > >  #include "migration/vmstate.h"
> > > >  #include "qemu/module.h"
> > > >  #include "sysemu/dma.h"
> > > > -
> > > > +#include "hw/isa/vt82c686.h"
> > > >  #include "hw/ide/pci.h"
> > > >  #include "trace.h"
> > > >
> > > > @@ -112,7 +112,7 @@ static void via_ide_set_irq(void *opaque, int n,
> > > > int level)
> > > >          d->config[0x70 + n * 8] &= ~0x80;
> > > >      }
> > > >
> > > > -    qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
> > > > +    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
> > >
> > > Since pci_get_function_0() is expensive, we should cache
> > > 'PCIDevice *func0' in PCIIDEState, setting the pointer in
> > > via_ide_realize(). Do you mind sending a follow-up patch?
> >
> > On the other hand, IMO PCIIDEState should be about PCI IDE stuff so to
> > keep this clean this would need subclassing it to VIAIDEState and put
> > the func0 pointer there.
>
> I expect any multi-function PCI embedding an IDE controller to route
> its IRQs via Func#0, but I'm not a PCI expert.

We don't have many in QEMU, I think only via and piix and not sure what 
piix does (currently using isa_get_irq, that's where I got this from for 
via).

> > But then we probably need to cast between
> > VIAIDE and PCIIDE and likely we're back to the same level of
> > expensiveness.
>
> realize() is called once, get_irq() multiple times.

Sure but we need to pass something to set_irq which will be then 
VIAIDEState that maybe we'll need to cast to something else but now we 
also cast it to PCI_DEVICE so maybe we could cache that too?

Regards,
BALATON Zoltan

> > The main source why of pci_get_function_0 is expensive is
> > probably the QOM cast to PCI_BUS in pci_get_bus() the rest is just
> > pointer and array index dereferences which should not be too bad. And
> > the reason why QOM casts are expensive is because we have
> > --enable-qom-debug enabled by default. I've tried to send a patch before
> > to disable this on release builds and only enable it with --enable-debug
> > or when explicitly asked but it was rejected saying that these asserts
> > are useful. Maybe we can just live with qemu_set_irq and not bother and
> > drop this series. (You can cherry pick the first patch removing code
> > duplication from via isa if you want.)
> >
> > Regards,
> > BALATON Zoltan