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

[BALATON Zoltan]

On Sun, 26 Feb 2023, Bernhard Beschow wrote:
> Am 26. Februar 2023 22:27:50 UTC schrieb "Philippe Mathieu-Daudé" 
> <philmd@linaro.org>:
> > On 25/2/23 19:11, BALATON Zoltan wrote:
> > > 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(+)
> >
> > > +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) {
> >
> > the ISA IRQ is stored in 4-bit so will always be in range.
>
> Indeed. I'd turn this into an assert to keep this assum visible. I'll do 
> another iteration of the PCI IRQ router series.

I don't like a useless assert in an irq handler that's potentially called 
a lot. If you want to keep that add a comment instead.

Also I can't use Based-on because having all patches in a single series 
helps maintainers to follow what belongs to here so this should be one 
series. You don't have to follow your one any more as it was fully 
incorporated here so this is the only version you'd have to watch.

Regards,
BALATON Zoltan

> Best regards,
> Bernhard
> > > +        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);
> > > +    }
> > > +}