Re: [PATCH v3 4/8] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

On Sun, 26 Feb 2023, Bernhard Beschow wrote:
> Am 25. Februar 2023 18:11:49 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > From: Bernhard Beschow <shentey@gmail.com>
> >
> > 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.
> >
> > Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
> >
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > [balaton: declare gpio inputs instead of changing pci bus irqs so it can
> > be connected in board code; remove some empty lines]
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > Tested-by: Rene Engel <ReneEngel80@emailn.de>
> > ---
> > hw/isa/vt82c686.c | 39 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > index 3f9bd0c04d..4025f9bcdc 100644
> > --- a/hw/isa/vt82c686.c
> > +++ b/hw/isa/vt82c686.c
> > @@ -604,6 +604,44 @@ static void via_isa_request_i8259_irq(void *opaque, int 
> > irq, int level)
> >     qemu_set_irq(s->cpu_intr, 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 pic_irq;
> > +
> > +    /* now we change the pic irq level according to the via irq mappings */
> > +    /* XXX: optimize */
> > +    pic_irq = via_isa_get_pci_irq(s, irq_num);
> > +    if (pic_irq < ISA_NUM_IRQS) {
> > +        int i, pic_level;
> > +
> > +        /* 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);
> > +            }
> > +        }
> > +        qemu_set_irq(s->isa_irqs[pic_irq], pic_level);
> > +    }
> > +}
> > +
> > static void via_isa_realize(PCIDevice *d, Error **errp)
> > {
> >     ViaISAState *s = VIA_ISA(d);
> > @@ -614,6 +652,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_set_pci_irq, "pirq", PCI_NUM_PINS);
>
> This line is a Pegasos2 specific addition for fixing its IRQ handling. Since 
> this code must also work with the Fuloong2e board we should aim for a minimal 
> changeset here which renders this line out of scope.
>
> Let's keep the two series separate since now I need to watch two series for 
> comments. Please use Based-on: tag next time instead.

Well, it's not. It's part of the QDev model for VT8231 that allows it to 
be connected by boards so I think this belongs here otherwise this won't 
even compile because the function you've added would be unused and bail on 
-Werror. Let's not make this more difficult than it is. I'm OK with 
reasonable changes but what's your goal now? You can't get rid of this 
line as it's how QDev can model it. Either I have to call into this model 
or have to export these pins as gpios.

Regards,
BALATON Zoltan

> Thanks,
> Bernhard
>
> >     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);