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

[Bernhard Beschow]

Am 22. Februar 2023 21:12:01 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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.
>
>I'm aware of that, we're using that in sam460ex which connects all PCI 
>interrupt lines to a single IRQ and Peter explored and explained it in a 
>comment there when that was discovered. First we had a patch with or-irq but 
>due to this behaviot that's not needed for PCI interrupts. But the VT8132 
>could change what ISA IRQ you route the sub functions to.

That depends on the sub function if you can do that. And if so, then it depends 
on whether the function is still in PCI mode (see below).

>It happens that on pegasos2 by default all of those are routed to IRQ9 except 
>IDE

All *PCI* interrupts are routed to IRQ9 while IDE legacy interrupts are routed 
to the compatible ISA IRQs. Note that the IDE function must only trigger the 
ISA IRQs if it is in legacy mode while it must only trigger the PCI IRQ in 
non-legacy mode. See https://www.bswd.com/pciide.pdf for more details on this 
particular topic.

>but what if a guest changes ac97 to use a different interrupt? Then it's not a 
>PCI interrupt any more so you can't use pci_set_irq in via=ac97.

How would it do that? AFAICS there is no dedicated register to configure which 
IRQ to use. This means that it can only trigger an interrupt via its PCI intx 
pin which is subject to the PCI -> ISA IRQ router.

> There are only 4 PCI INT lines but the VIA components can be routed to 13 or 
> 14 ISA IRQs.

Pure PCI components are only able to trigger one of the four PCI intx pins they 
are *hardwired* to. Each component has only one pin. Which ISA IRQ gets 
triggered through that pin can be selected from 13 or 14 ISA IRQs as you say by 
means of the three configuration registers of the PCI -> ISA IRQ router.

>How do you keep track of that with only the PCI bus interrupts?

Devices that operate in ISA mode such as the IDE function shall have their own, 
dedicated ISA IRQs assigned by the guest. Otherwise this causes a classic 
interrupt conflict, just like in the olden ISA days. If the function operates 
in PCI mode, it must not trigger the ISA IRQs, regardless of whether they are 
assigned or not.

There is also the power management function whose ACPI interrupt (SCI) can be 
routed by means of a dedicated register. Again, a guest must make sure here to 
not configure interrupt conflicts.

>I don't get your approach.

I hope that I could help you get a better understanding. The linked .pdf is 
good and comprehensive reading material.

Best regards,
Bernhard

>
>>> 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
>> 
>> So there must be an issue somewhere else...
>
>I'll have a look later but my patch attempts to handle the USB controller 
>interrupts. There may be another bug somewhere in USB emulation though, we 
>have similar problem with mac99 with older MacOS guests. Considering that USB 
>devices probably did not work at all before this patch it's at least still an 
>imptovement. :-)
>
>Regards,
>BALATON Zoltan