Re: [PATCH 0/5] Pegasos2 fixes and audio output support

[BALATON Zoltan]

On Wed, 22 Feb 2023, Bernhard Beschow wrote:
> Am 22. Februar 2023 19:25:16 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Wed, 22 Feb 2023, Bernhard Beschow wrote:
> > > On Wed, Feb 22, 2023 at 4:38 PM Bernhard Beschow <shentey@gmail.com> wrote:
> > > > On Tue, Feb 21, 2023 at 7:44 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > > > > This series fixes PCI interrupts on the ppc/pegasos2 machine and adds
> > > > > partial implementation of the via-ac97 sound part enough to get audio
> > > > > output. I'd like this to be merged for QEMU 8.0.
> > > > >
> > > > > Regards,
> > > > > BALATON Zoltan
> > > > >
> > > > > BALATON Zoltan (5):
> > > > >   hw/isa/vt82c686: Implement interrupt routing in via_isa_set_irq
> > > > >   hw/isa/vt82c686: Implement PIRQ pins
> > > > >   hw/ppc/pegasos2: Fix PCI interrupt routing
> > > > >   hw/audio/ac97: Split off some definitions to a header
> > > > >   hw/audio/via-ac97: Basic implementation of audio playback
> > > > >
> > > > >  hw/audio/ac97.c            |  43 +---
> > > > >  hw/audio/ac97.h            |  65 ++++++
> > > > >  hw/audio/trace-events      |   6 +
> > > > >  hw/audio/via-ac97.c        | 436 ++++++++++++++++++++++++++++++++++++-
> > > > >  hw/ide/via.c               |   2 +-
> > > > >  hw/isa/vt82c686.c          |  61 +++++-
> > > > >  hw/pci-host/mv64361.c      |   4 -
> > > > >  hw/ppc/pegasos2.c          |  26 ++-
> > > > >  hw/usb/vt82c686-uhci-pci.c |   5 +-
> > > > >  include/hw/isa/vt82c686.h  |  39 +++-
> > > > >  10 files changed, 626 insertions(+), 61 deletions(-)
> > > > >  create mode 100644 hw/audio/ac97.h
> > > > >
> > > > > --
> > > > > 2.30.7
> > > > Wow, the MorphOS people paid attention to sound design. Thanks for
> > > > presenting it to us, Zoltan!
> > > >
> > > > I've had a closer look at your series and I think it can be simplified:
> > > > Patch 2 can be implemented quite straight-forward like I proposed in a
> > > > private mail: https://github.com/shentok/qemu/commit/via-priq-routing.
> > > > Then, in order to make patch 3 "hw/ppc/pegasos2: Fix PCI interrupt routing"
> > > > working, one can expose the PCI interrupts with a single line like you do
> > > > in patch 2. With this, patch 1 "hw/isa/vt82c686: Implement interrupt
> > > > routing in via_isa_set_irq" isn't needed any longer and can be omitted.
> > > >
> > > > In via-ac97, rather than using via_isa_set_irq(), pci_set_irq() can be
> > > > used instead. pci_set_irq() internally takes care of all the ISA interrupt
> > > > level tracking patch 1 attempted to address.
> > >
> > > Here is a proof of concept branch to demonstrate that the simplification
> > > actually works: https://github.com/shentok/qemu/commits/pegasos2 (Tested
> > > with MorphOS with and without pegasos2.rom).
> >
> > Does this only work because both the via-ac97 and the PCI interrupts are mapped 
> > to the same ISA IRQ and you've only tested sound? The guest could configure 
> > each device to use a different IRQ, also mapping them so they share one ISA 
> > interrupt. What happens if multiple devices are mapped to IRQ 9 (which is the 
> > case on pegasos2 where PCI cards, ac97 and USB all share this IRQ) and more 
> > than one such device wants to raise an interrupt at the same time? If you ack 
> > the ac97 interrupt but a PCI network card or the USB part still wants to get 
> > the CPUs attention the ISA IRQ should remain raised until all devices are 
> > serviced.
>
> pci_bus_get_irq_level(), used in via_isa_set_pci_irq(), should handle
> exactly that case very well.
>
> > I don't see a way to track the status of all devices in a single qemu_irq which 
> > can only be up or down so we need something to store the state of each source.
>
> pci_set_irq() causes pci_bus_change_irq_level() to be called.
> pci_bus_change_irq_level() tracks the sum of all irq levels of all
> devices attached to a particular pin in irq_count. Have a look at
> pci_bus_change_irq_level() and you will understand better.
>
> > My patch adds a state register to each ISA IRQ line for all possible sources 
> > which could probably be stored once but then for each change of ISA IRQ status 
> > all the mapped devices should be checked and combined so it's easier to store 
> > them for each IRQ. Does your approach still work if you play sound, and copy 
> > something from network to a USB device at the same time? (I'm not sure mine 
> > does not have remaining bugs but I don't think this can be simplified that way 
> > but if you can prove it would work I don't mind taking an alternative version 
> > but I'm not convinced yet.)
>
> Well, I can't prove that my approach works but unfortunately I can
> prove that both our approaches cause a freeze :/ Try:
> 1. Start `qemu-system-ppc -M pegasos2 -bios pegasos2.rom -rtc
> base=localtime -device ati-vga,guest_hwcursor=true,romfile="" -cdrom
> morphos-3.17.iso -device usb-mouse -device usb-kbd`
> 2. Move the mouse while sound is playing
> -> Observe the VM to freeze

Not quite sure why but it seems to happen when both the ac97 and USB raise 
the interrupt and the guest driver seems to get confused. Adding some 
debug logging:

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index b16620daf8..f840e5a8d0 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -636,12 +636,13 @@ void via_isa_set_irq(PCIDevice *d, 
ViaISAIRQSourceBit n, int level)
     if (!isa_irq) {
         return;
     }
-
+if (n > 1) fprintf(stderr, "%s: %d %d %d %x -> ", __func__, n, level, isa_irq, 
s->isa_irq_state[isa_irq]);
     if (level) {
         s->isa_irq_state[isa_irq] |= BIT(n);
     } else {
         s->isa_irq_state[isa_irq] &= ~BIT(n);
     }
+if (n > 1) fprintf(stderr, "%x\n", s->isa_irq_state[isa_irq]);
     qemu_set_irq(s->isa_irqs[isa_irq], !!s->isa_irq_state[isa_irq]);
 }

I see in the normal case when there's only one interrupt for USB only:

via_isa_set_irq: 2 1 9 0 -> 4
usb_uhci_mmio_readw addr 0x0002, ret 0x0001
usb_uhci_mmio_writew addr 0x0002, val 0x0001
via_isa_set_irq: 2 0 9 4 -> 0

For sound only:

via_ac97_sgd_fetch addr=0x43b70bc --F len=3528
via_isa_set_irq: 8 1 9 0 -> 100
usb_uhci_mmio_readw addr 0x0002, ret 0x0000
usb_uhci_mmio_readw addr 0x0002, ret 0x0000
via_ac97_sgd_read 0x0 1 -> 0xc9
via_ac97_sgd_write 0x0 1 <- 0x1
via_isa_set_irq: 8 0 9 100 -> 0
via_ac97_sgd_read 0x4 4 -> 0x439cbe8
via_ac97_sgd_fetch addr=0x43c70bc -E- len=3528
via_isa_set_irq: 8 1 9 0 -> 100
via_ac97_sgd_read 0x4 4 -> 0x439cbe0
via_ac97_sgd_read 0x4 4 -> 0x439cbe0
via_ac97_sgd_read 0x10 1 -> 0x0
usb_uhci_mmio_readw addr 0x0002, ret 0x0000
usb_uhci_mmio_readw addr 0x0002, ret 0x0000
via_ac97_sgd_read 0x0 1 -> 0xca
via_ac97_sgd_write 0x0 1 <- 0x2
via_isa_set_irq: 8 0 9 100 -> 0
via_ac97_sgd_read 0x4 4 -> 0x439cbe0

but it stops acking irqs when both are raised or it seems USB IRQ is 
raised while it's in the guest IRQ handler:

via_ac97_sgd_fetch addr=0x43c70bc -E- len=3528
via_isa_set_irq: 8 1 9 0 -> 100
usb_uhci_mmio_readw addr 0x0002, ret 0x0000
usb_uhci_mmio_readw addr 0x0002, ret 0x0000
via_isa_set_irq: 2 1 9 100 -> 104
via_ac97_sgd_read 0x0 1 -> 0xca
via_ac97_sgd_write 0x0 1 <- 0x2
via_isa_set_irq: 8 0 9 104 -> 4
via_ac97_sgd_read 0x4 4 -> 0x439cbe0
via_ac97_sgd_fetch addr=0x43b70bc --F len=3528
via_isa_set_irq: 8 1 9 4 -> 104
via_ac97_sgd_read 0x4 4 -> 0x439cbe8
via_ac97_sgd_read 0x4 4 -> 0x439cbe8
via_ac97_sgd_read 0x10 1 -> 0x0
usb_uhci_mmio_readw addr 0x0006, ret 0x06bf
usb_uhci_mmio_readw addr 0x0010, ret 0x0085
usb_uhci_mmio_writew addr 0x0010, val 0x0085
usb_uhci_mmio_readw addr 0x0012, ret 0x0085
usb_uhci_mmio_writew addr 0x0012, val 0x0085
usb_uhci_mmio_readw addr 0x0006, ret 0x06b7
usb_uhci_mmio_readw addr 0x0010, ret 0x0080
usb_uhci_mmio_writew addr 0x0010, val 0x0080
usb_uhci_mmio_readw addr 0x0012, ret 0x0080
usb_uhci_mmio_writew addr 0x0012, val 0x0080
usb_uhci_mmio_readw addr 0x0006, ret 0x0759
usb_uhci_mmio_readw addr 0x0010, ret 0x0085
usb_uhci_mmio_writew addr 0x0010, val 0x0085
usb_uhci_mmio_readw addr 0x0012, ret 0x0085
usb_uhci_mmio_writew addr 0x0012, val 0x0085
usb_uhci_mmio_readw addr 0x0006, ret 0x0752
usb_uhci_mmio_readw addr 0x0010, ret 0x0080
usb_uhci_mmio_writew addr 0x0010, val 0x0080
usb_uhci_mmio_readw addr 0x0012, ret 0x0080
usb_uhci_mmio_writew addr 0x0012, val 0x0080
via_isa_set_irq: 2 1 9 104 -> 104
usb_uhci_mmio_readw addr 0x0006, ret 0x07f1
usb_uhci_mmio_readw addr 0x0010, ret 0x0085
usb_uhci_mmio_writew addr 0x0010, val 0x0085
usb_uhci_mmio_readw addr 0x0012, ret 0x0085
usb_uhci_mmio_writew addr 0x0012, val 0x0085
usb_uhci_mmio_readw addr 0x0006, ret 0x07e9

It seems to not notice the USB interrupt any more after that although 
sound playback stops but mouse still moves but otherwise does not work. 
I'm not sure this is not a guest bug as it seems an interrupt handler 
should disable interrupts to not get interrupted. Could this be reproduced 
with Linux? I'd still go wit this patch series for 8.0 because the default 
case works and this was also tested with two PCI cards on AmigaOS4 which 
works not while it did not work at all before so this could be debugged 
and fixed later but adding this series makes the machine generally usable 
at least without USB devices. With -d unimp I also get these logs when 
booting MorphOS:

ok boot cd boot.img
ISO-9660 filesystem:  System-ID: "MORPHOS"  Volume-ID: "MorphOSBoot"
Root dir: "" flags=0x2 extent=0x20 size=0x1800
31.127| Memory used before SYS_Init: 9MB
i8259: level sensitive irq not supported
i8259: level sensitive irq not supported

Could it be the PIC emulation should be fixed for this?

Regards,
BALATON Zoltan