Re: [PATCH v7 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

On Sun, 5 Mar 2023, Bernhard Beschow wrote:
> Am 5. März 2023 14:05:49 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 42 insertions(+)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 8900d87f59..e5aa467506 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -600,6 +600,46 @@ 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);
> > +
> > +    /* IRQ 0 and 15 mean disabled, IRQ 2 is reserved */
>
> The vt82c686b datasheet mentions that IRQs 2, 8 and 13 are reserved (-> 
> guest errors) while only 0 means disabled. I think below code should 
> reflect this.

We can't because 8 and 13 are allowed for USB and ac97 but 15 means 
disabled for those. My original implementation would have allowed to 
implement that but this one from you mixes everyting with the PIRQ pins so 
we can't implement different reserved/disabled values for different IRQ 
sources so this is the best we can do. IRQ 0 is already handled as 
disabled by the code below.

Regards,
BALATON Zoltan

> > +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
> > +        if (pic_irq == 2) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
> > +        }
> > +        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);
> > @@ -620,6 +660,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)) {