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

[BALATON Zoltan]

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. But then we probably need to cast between VIAIDE and 
PCIIDE and likely we're back to the same level of expensiveness. 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