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 <bala...@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
