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
>> <bala...@eik.bme.hu>:
>>> On Wed, 22 Feb 2023, Bernhard Beschow wrote:
>>>> On Wed, Feb 22, 2023 at 4:38 PM Bernhard Beschow <shen...@gmail.com>
>>>> wrote:
>>>>> On Tue, Feb 21, 2023 at 7:44 PM BALATON Zoltan <bala...@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
