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 <[email protected]> >>> --- >>> 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. > 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. > 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
