Re: [PATCH v6 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow <shentey@gmail.com>]

Am 4. März 2023 14:48:20 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>The real VIA south bridges implement a PCI IRQ router which is configured
>by the BIOS or the OS. In order to respect these configurations, QEMU
>needs to implement it as well. The real chip may allow routing IRQs from
>internal functions independently of PCI interrupts but since guests
>usually configute it to a single shared interrupt we don't model that
>here for simplicity.
>
>Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>
>Suggested-by: Bernhard Beschow <shentey@gmail.com>
>Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>Tested-by: Rene Engel <ReneEngel80@emailn.de>
>---
> hw/isa/vt82c686.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
>diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>index f4c40965cd..51c0dd4c41 100644
>--- a/hw/isa/vt82c686.c
>+++ b/hw/isa/vt82c686.c
>@@ -598,6 +598,42 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>     qemu_set_irq(s->isa_irqs_in[n], level);
> }
> 
>+static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num)
>+{
>+    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;
>+}
>+
>+static void via_isa_set_pci_irq(void *opaque, int irq_num, 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);
>+
>+    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {

In the previous iteration I already mentioned this: Why "pic_irq > 14"? Please either remove or put a comment in the code since the datasheet allows it. Otherwise this leads to hard to comprehend and therefore hard to maintain code.

Moreover, "pic_irq == 2" is reserved which we should log a guest error for. Otherwise, misbehaving guests will go unnoticed, which is exactly what guest errors should prevent. Also, logging a guest error here makes the code more self documenting.

Note that excess logging can always be filtered out using grep.

Best regards,
Bernhard

>+        return;
>+    }
>+
>+    /* 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);
>+}
>+
> static void via_isa_realize(PCIDevice *d, Error **errp)
> {
>     ViaISAState *s = VIA_ISA(d);
>@@ -619,6 +655,8 @@ 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)) {