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

[BALATON Zoltan]

On Wed, 22 Feb 2023, BALATON Zoltan wrote:
> 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?

After thinking about that more I think this is the reason and this patch 
just uncovered a defficiency in the PIC model. I would not care much it 
this was only sound vs. USB but it's also sound vs. PCI cards e.g. network 
so until that's fixed in i8259 I can hack around that here like this:

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index b16620daf8..a6cf55a632 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -597,6 +597,7 @@ void via_isa_set_irq(PCIDevice *d, ViaISAIRQSourceBit n, 
int level)
 {
     ViaISAState *s = VIA_ISA(pci_get_function_0(d));
     uint8_t isa_irq = 0, max_irq = 15;
+    int old_level;

     if (n == VIA_IRQ_USB0 && d == PCI_DEVICE(&s->uhci[1])) {
         n++;
@@ -637,11 +638,16 @@ void via_isa_set_irq(PCIDevice *d, ViaISAIRQSourceBit n, 
int level)
         return;
     }

+    old_level = !!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 (old_level && !!s->isa_irq_state[isa_irq]) {
+        /* Only needed because i8259 model does not support level sensitive */
+        qemu_set_irq(s->isa_irqs[isa_irq], 0);
+    }
     qemu_set_irq(s->isa_irqs[isa_irq], !!s->isa_irq_state[isa_irq]);
 }

Unless somebody has a better idea I'll go with this for a v2 and let this 
be cleaned up sometimes in the future when sombody gets around to improve 
the PIC model.

Regards,
BALATON Zoltan