[PATCH v2 0/4] Fix IRQ routing in via south bridge

[PATCH v2 0/4] Fix IRQ routing in via south bridge

[BALATON Zoltan]

Changes in v2:
- Avoid a potential problem noticed by Volker before with not clearing
interrupts when mapping is changed (may not happen in practice but we
can handle it here) also add some comments explaining how it works.
- Update some commit messages

This is going back to my otiginal proposal in
https://patchew.org/QEMU/cover.1677004414.git.balaton@eik.bme.hu/
implementing routing of interrupts from device functions and PCI
devices to ISA interrupts. On pegasos2 the firmware sets evertyhing to
share IRQ 9 so the current simpified version worked for taht but with
the amigaone machine its firmware makes use of this feature and
assigns different interrupts to functions and PCI devices so we need
to properly impelent this. Since any ISA interrupt can be controlled
by any interrupt source (different functions of the multifunction
device plus the 4 input pins from PCI devices) there are more than 4
possible sources so this can't be handled by just the 4 PCI interrupt
lines. We need to keep track of the state of each interrupt source to
be able to determine the level of the ISA interrupt and avoid one
device clearing it while other still has an interrupt.

This fixes USB on amigaone and maybe other bugs not discovered yet.

Regards,
BALATON Zoltan

BALATON Zoltan (4):
  hw/isa/vt82c686: Bring back via_isa_set_irq()
  hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
  hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq()
  hw/audio/via-ac97: Route interrupts using via_isa_set_irq()

 hw/audio/via-ac97.c        |  8 ++--
 hw/isa/vt82c686.c          | 75 +++++++++++++++++++++++++-------------
 hw/usb/vt82c686-uhci-pci.c |  9 +++++
 include/hw/isa/vt82c686.h  |  2 +
 4 files changed, 64 insertions(+), 30 deletions(-)

-- 
2.30.9

[PATCH v2 1/4] hw/isa/vt82c686: Bring back via_isa_set_irq()

[BALATON Zoltan]

The VIA intergrated south bridge chips combine several functions and
allow routing their interrupts to any of the ISA IRQs (also allowing
multiple components to share the same ISA IRQ, e.g. pegasos2 firmware
configures USB, sound and PCI to all use IRQ 9). Bring back
via_isa_set_irq() and change it to take the PCIDevice that wants to
change an IRQ and keep track of the interrupt status of each source
separately and do the mapping to ISA IRQ within the ISA bridge to
allow different sources to control the same ISA IRQ lines.

This may not handle cases when the ISA IRQ is also controlled by
devices directly, not going through via_isa_set_irq() such as serial,
parallel or keyboard but these IRQs being conventionally fixed are not
likely for guests to change or share with other devices so hopefully
this does not cause a problem in practice.

This reverts commit 4e5a20b6da9b1f6d2e9621ed7eb8b239560104ae.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/isa/vt82c686.h |  2 ++
 2 files changed, 41 insertions(+)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 57bdfb4e78..c1826c77eb 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -549,6 +549,7 @@ struct ViaISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs_in;
+    uint16_t isa_irq_state[ISA_NUM_IRQS];
     ViaSuperIOState via_sio;
     MC146818RtcState rtc;
     PCIIDEState ide;
@@ -592,6 +593,44 @@ static const TypeInfo via_isa_info = {
     },
 };
 
+void via_isa_set_irq(PCIDevice *d, int pin, int level)
+{
+    ViaISAState *s = VIA_ISA(pci_get_function_0(d));
+    int n = PCI_FUNC(d->devfn);
+    uint8_t isa_irq = d->config[PCI_INTERRUPT_LINE], max_irq = 15;
+
+    switch (n) {
+    case 2: /* USB ports 0-1 */
+    case 3: /* USB ports 2-3 */
+        max_irq = 14;
+        break;
+    }
+
+    if (unlikely(isa_irq > max_irq || isa_irq == 2)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing %d for %d",
+                      isa_irq, n);
+        return;
+    }
+    if (isa_irq == 0) {
+        return; /* disabled */
+    }
+
+    /*
+     * In addition to recording status of sources mapped to each isa_irq we
+     * keep track of all sources in IRQ 0 and use that as a mask to avoid stuck
+     * interrupts in case mapping of a source is changed while IRQ is raised.
+     */
+    if (level) {
+        s->isa_irq_state[isa_irq] |= BIT(n);
+        s->isa_irq_state[0] |= BIT(n);
+    } else {
+        s->isa_irq_state[isa_irq] &= ~BIT(n);
+        s->isa_irq_state[0] &= ~BIT(n);
+    }
+    s->isa_irq_state[isa_irq] &= s->isa_irq_state[0];
+    qemu_set_irq(s->isa_irqs_in[isa_irq], !!s->isa_irq_state[isa_irq]);
+}
+
 static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
 {
     ViaISAState *s = opaque;
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index b6e95b2851..da1722daf2 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -34,4 +34,6 @@ struct ViaAC97State {
     uint32_t ac97_cmd;
 };
 
+void via_isa_set_irq(PCIDevice *d, int n, int level);
+
 #endif
-- 
2.30.9

Re: [PATCH v2 1/4] hw/isa/vt82c686: Bring back via_isa_set_irq()

[BALATON Zoltan]

On Sun, 29 Oct 2023, BALATON Zoltan wrote:
> The VIA intergrated south bridge chips combine several functions and
> allow routing their interrupts to any of the ISA IRQs (also allowing
> multiple components to share the same ISA IRQ, e.g. pegasos2 firmware
> configures USB, sound and PCI to all use IRQ 9). Bring back
> via_isa_set_irq() and change it to take the PCIDevice that wants to
> change an IRQ and keep track of the interrupt status of each source
> separately and do the mapping to ISA IRQ within the ISA bridge to
> allow different sources to control the same ISA IRQ lines.
>
> This may not handle cases when the ISA IRQ is also controlled by
> devices directly, not going through via_isa_set_irq() such as serial,
> parallel or keyboard but these IRQs being conventionally fixed are not
> likely for guests to change or share with other devices so hopefully
> this does not cause a problem in practice.
>
> This reverts commit 4e5a20b6da9b1f6d2e9621ed7eb8b239560104ae.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/isa/vt82c686.c         | 39 +++++++++++++++++++++++++++++++++++++++
> include/hw/isa/vt82c686.h |  2 ++
> 2 files changed, 41 insertions(+)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 57bdfb4e78..c1826c77eb 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -549,6 +549,7 @@ struct ViaISAState {
>     PCIDevice dev;
>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs_in;
> +    uint16_t isa_irq_state[ISA_NUM_IRQS];
>     ViaSuperIOState via_sio;
>     MC146818RtcState rtc;
>     PCIIDEState ide;
> @@ -592,6 +593,44 @@ static const TypeInfo via_isa_info = {
>     },
> };
>
> +void via_isa_set_irq(PCIDevice *d, int pin, int level)
> +{
> +    ViaISAState *s = VIA_ISA(pci_get_function_0(d));
> +    int n = PCI_FUNC(d->devfn);
> +    uint8_t isa_irq = d->config[PCI_INTERRUPT_LINE], max_irq = 15;
> +
> +    switch (n) {
> +    case 2: /* USB ports 0-1 */
> +    case 3: /* USB ports 2-3 */
> +        max_irq = 14;
> +        break;
> +    }
> +
> +    if (unlikely(isa_irq > max_irq || isa_irq == 2)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing %d for %d",
> +                      isa_irq, n);
> +        return;
> +    }
> +    if (isa_irq == 0) {
> +        return; /* disabled */
> +    }
> +
> +    /*
> +     * In addition to recording status of sources mapped to each isa_irq we
> +     * keep track of all sources in IRQ 0 and use that as a mask to avoid stuck
> +     * interrupts in case mapping of a source is changed while IRQ is raised.
> +     */
> +    if (level) {
> +        s->isa_irq_state[isa_irq] |= BIT(n);
> +        s->isa_irq_state[0] |= BIT(n);
> +    } else {
> +        s->isa_irq_state[isa_irq] &= ~BIT(n);
> +        s->isa_irq_state[0] &= ~BIT(n);
> +    }
> +    s->isa_irq_state[isa_irq] &= s->isa_irq_state[0];

Thinking about it some more it still does not catch the case when IRQ is 
still raised so this could temporarly raise the old IRQ as well so maybe 
we can just drop this and go with the v1 of the series. That works well 
enough becuase in practice guests don't change these often, only at boot 
so probably there are no pending interrupts yet so this may not be an 
issue. If we find it is then the proper fix should be adding a PCI config 
write func to every source and clear the old interrupt with 
via_isa_set_irq before changing the PCI_INTERRUPT_LINE value but I don't 
think it's worth implementing that now until we find something that 
it would fix. So ignore this v2 and take the v1 instead.

Regards,
BALATON Zoltan

> +    qemu_set_irq(s->isa_irqs_in[isa_irq], !!s->isa_irq_state[isa_irq]);
> +}
> +
> static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> {
>     ViaISAState *s = opaque;
> diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
> index b6e95b2851..da1722daf2 100644
> --- a/include/hw/isa/vt82c686.h
> +++ b/include/hw/isa/vt82c686.h
> @@ -34,4 +34,6 @@ struct ViaAC97State {
>     uint32_t ac97_cmd;
> };
>
> +void via_isa_set_irq(PCIDevice *d, int n, int level);
> +
> #endif
>

[PATCH v2 2/4] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts

[BALATON Zoltan]

This device is part of a superio/ISA bridge chip and IRQs from it are
routed to an ISA interrupt. Use via_isa_set_irq() function to implement
this in a vt82c686-uhci-pci specific irq handler.

This reverts commit 422a6e8075752bc5342afd3eace23a4990dd7d98.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/vt82c686-uhci-pci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index b4884c9011..6162806172 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -1,7 +1,14 @@
 #include "qemu/osdep.h"
+#include "hw/irq.h"
 #include "hw/isa/vt82c686.h"
 #include "hcd-uhci.h"
 
+static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
+{
+    UHCIState *s = opaque;
+    via_isa_set_irq(&s->dev, 0, level);
+}
+
 static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
 {
     UHCIState *s = UHCI(dev);
@@ -15,6 +22,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + 0xc0, 0x00002000);
 
     usb_uhci_common_realize(dev, errp);
+    object_unref(s->irq);
+    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
 }
 
 static UHCIInfo uhci_info[] = {
-- 
2.30.9

[PATCH v2 3/4] hw/isa/vt82c686: Route PIRQ inputs using via_isa_set_irq()

[BALATON Zoltan]

The chip has 4 pins (called PIRQA-D in VT82C686B and PINTA-D in
VT8231) that are meant to be connected to PCI IRQ lines and allow
routing PCI interrupts to the ISA PIC. Route these in
via_isa_set_irq() to make it possible to share them with internal
functions that can also be routed to the same ISA IRQs.

Fixes: 2fdadd02e675caca4aba4ae26317701fe2c4c901
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 65 +++++++++++++++++------------------------------
 1 file changed, 24 insertions(+), 41 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index c1826c77eb..c44bfeeab0 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -593,6 +593,21 @@ static const TypeInfo via_isa_info = {
     },
 };
 
+static int via_isa_get_pci_irq(const ViaISAState *s, int pin)
+{
+    switch (pin) {
+    case 0:
+        return s->dev.config[0x55] >> 4;
+    case 1:
+        return s->dev.config[0x56] & 0xf;
+    case 2:
+        return s->dev.config[0x56] >> 4;
+    case 3:
+        return s->dev.config[0x57] >> 4;
+    }
+    return 0;
+}
+
 void via_isa_set_irq(PCIDevice *d, int pin, int level)
 {
     ViaISAState *s = VIA_ISA(pci_get_function_0(d));
@@ -600,6 +615,10 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
     uint8_t isa_irq = d->config[PCI_INTERRUPT_LINE], max_irq = 15;
 
     switch (n) {
+    case 0: /* PIRQ/PINT inputs */
+        isa_irq = via_isa_get_pci_irq(s, pin);
+        n = 12 + pin;
+        break;
     case 2: /* USB ports 0-1 */
     case 3: /* USB ports 2-3 */
         max_irq = 14;
@@ -631,50 +650,15 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
     qemu_set_irq(s->isa_irqs_in[isa_irq], !!s->isa_irq_state[isa_irq]);
 }
 
-static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
-{
-    ViaISAState *s = opaque;
-    qemu_set_irq(s->cpu_intr, level);
-}
-
-static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num)
+static void via_isa_pirq(void *opaque, int pin, int level)
 {
-    switch (irq_num) {
-    case 0:
-        return s->dev.config[0x55] >> 4;
-    case 1:
-        return s->dev.config[0x56] & 0xf;
-    case 2:
-        return s->dev.config[0x56] >> 4;
-    case 3:
-        return s->dev.config[0x57] >> 4;
-    }
-    return 0;
+    via_isa_set_irq(opaque, pin, level);
 }
 
-static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
+static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
 {
     ViaISAState *s = opaque;
-    PCIBus *bus = pci_get_bus(&s->dev);
-    int i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
-
-    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
-    if (!pic_irq) {
-        return;
-    }
-    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
-        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
-    }
-
-    /* The pic level is the logical OR of all the PCI irqs mapped to it. */
-    pic_level = 0;
-    for (i = 0; i < PCI_NUM_PINS; i++) {
-        if (pic_irq == via_isa_get_pci_irq(s, i)) {
-            pic_level |= pci_bus_get_irq_level(bus, i);
-        }
-    }
-    /* Now we change the pic irq level according to the via irq mappings. */
-    qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
+    qemu_set_irq(s->cpu_intr, level);
 }
 
 static void via_isa_realize(PCIDevice *d, Error **errp)
@@ -687,6 +671,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     int i;
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    qdev_init_gpio_in_named(dev, via_isa_pirq, "pirq", PCI_NUM_PINS);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
@@ -700,8 +685,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
 
-    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
-
     /* RTC */
     qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
-- 
2.30.9

[PATCH v2 4/4] hw/audio/via-ac97: Route interrupts using via_isa_set_irq()

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/audio/via-ac97.c | 8 ++++----
 hw/isa/vt82c686.c   | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
index 30095a4c7a..4c127a1def 100644
--- a/hw/audio/via-ac97.c
+++ b/hw/audio/via-ac97.c
@@ -211,14 +211,14 @@ static void out_cb(void *opaque, int avail)
                     AUD_set_active_out(s->vo, 0);
                 }
                 if (c->type & STAT_EOL) {
-                    pci_set_irq(&s->dev, 1);
+                    via_isa_set_irq(&s->dev, 0, 1);
                 }
             }
             if (CLEN_IS_FLAG(c)) {
                 c->stat |= STAT_FLAG;
                 c->stat |= STAT_PAUSED;
                 if (c->type & STAT_FLAG) {
-                    pci_set_irq(&s->dev, 1);
+                    via_isa_set_irq(&s->dev, 0, 1);
                 }
             }
             if (CLEN_IS_STOP(c)) {
@@ -305,13 +305,13 @@ static void sgd_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
         if (val & STAT_EOL) {
             s->aur.stat &= ~(STAT_EOL | STAT_PAUSED);
             if (s->aur.type & STAT_EOL) {
-                pci_set_irq(&s->dev, 0);
+                via_isa_set_irq(&s->dev, 0, 0);
             }
         }
         if (val & STAT_FLAG) {
             s->aur.stat &= ~(STAT_FLAG | STAT_PAUSED);
             if (s->aur.type & STAT_FLAG) {
-                pci_set_irq(&s->dev, 0);
+                via_isa_set_irq(&s->dev, 0, 0);
             }
         }
         break;
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index c44bfeeab0..bf148f4c1b 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -621,6 +621,7 @@ void via_isa_set_irq(PCIDevice *d, int pin, int level)
         break;
     case 2: /* USB ports 0-1 */
     case 3: /* USB ports 2-3 */
+    case 5: /* AC97 audio */
         max_irq = 14;
         break;
     }
-- 
2.30.9