Re: [PATCH 2/5] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

On Thu, 23 Feb 2023, Bernhard Beschow wrote:
> Am 23. Februar 2023 21:11:23 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Thu, 23 Feb 2023, Bernhard Beschow wrote:
> > > 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>
> > > ---
> > > hw/isa/vt82c686.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 44 insertions(+)
> > >
> > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > index 3f9bd0c04d..f24e387d63 100644
> > > --- a/hw/isa/vt82c686.c
> > > +++ b/hw/isa/vt82c686.c
> > > @@ -604,6 +604,48 @@ 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);
> > > @@ -676,6 +718,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
> > >     if (!qdev_realize(DEVICE(&s->mc97), BUS(pci_bus), errp)) {
> > >         return;
> > >     }
> > > +
> > > +    pci_bus_irqs(pci_bus, via_isa_set_pci_irq, s, PCI_NUM_PINS);
> >
> > Please no oversimplification. This replaces the connections to mv64361 gpp pins 
> > made in mv64361_realize() and breaks the interrupts that can be enabled in 
> > mv64361.
>
> Let's split our work as follows: You'll do the audio and pegasos2 
> changes including exporting the pirq properties, I'll do the first three 
> routing patches of this series as the base.

I'm OK with doing audio as said and already did the PIRQ and pegaosos2 
changes (patch 2 and 3 in my series), just take those without deleting 
half of them. So drop the last two via-ac97 patches and do the IRQ fixes 
in your way but keep working what now works.

> > I've implemented that for something but can't remember now which guest exactly,
>
> Could you please put this information into the commit message or into 
> the code? That would ease maintainance a lot.

I did, see patch 3 in my series.

> > but this would be needed so please restore my pegasos2 patch and move 
> > this there connecting both mv64361 and via-isa to PCI interrupts as 
> > shown in the schematics. That means you also need the PIRQ pins here. 
> > Can you do a new version with that?
>
> As proposed above I'd fold the first three patches into a separate 
> series which you can build upon. I have no way to test the pegasos2 IRQ 
> changes since the test cases I'm aware of either work or we agreed that 
> they can be fixed later (-> USB).

I did fix the USB just haven't sent a v2 yet due to this thread but it's 
just the change I've sent yesterday, just add this before qemu_set_irq at 
the end of via_isa_set_irq() in my series. This is what I have now:

+    uint16_t old_state;
+    if (old_state && s->isa_irq_state[isa_irq]) {
+        /* FIXME: i8259 model does not support level sensitive mode */
+        qemu_set_irq(s->isa_irqs[isa_irq], 0);
+    }

How to do that with your version I have no idea but this fixed the problem 
with my series. I can send a v2 of this patch with this change if it's not 
clear from this and the other message what I did.

> > I'll try this one in the meantime
>
> Sounds good to me -- that's what I wanted to achieve ;) Thanks!

I've answered about that in the other message, I've tried with AmigaOS, 
Debian Linux 8.11.0 netboot iso and MorphOS and they still boot but 
couldn't test them much yet. MorphOS works on my series with sound and USB 
and does not hang with the above workaround but found now it still hangs 
if I send something to it over serial (e.g. press space or enter where 
you've typed boot cd boot.img after it starts playing sound). This happens 
on both of our series but only with the via-ac97 patch so probably related 
to that. This could easily be a guest bug too so I don't care that much, 
the pegasos2 changes are more interesting to get AmigaOS run well so 
that's my main focus now, MorphOS already runs on other QEMU machines 
well. I'll still try to find this out but AmigaOS can use other sound card 
so as long as the IRQ problems are fixed it would work but we need more 
than one PCI cards working as we'd need sound card and network card for it 
to be usable. This was tested to work with my series, if you give 
alternative series I can ask to have those tested too.

Regards,
BALATON Zoltan