[PATCH 0/5] VT82xx PCI fixes and audio output support

[PATCH 0/5] VT82xx PCI fixes and audio output support

[Bernhard Beschow]

This series is v2 of [1] ("[PATCH 0/5] Pegasos2 fixes and audio output
support"). It makes PCI interrupt routing on the VIA south bridges more
compliant to the PCI specification and adds partial implementation of the
via-ac97 sound part enough to get audio output working on the ppc/pegasos2
machine.

All credits for implementing AC'97 support go to Zoltan who would like this
series to be merged for QEMU 8.0.

v2:
* Rework PCI IRQ routing in the VIA south bridges
* Use pci_set_irq() rather than via_isa_set_irq() in via-ac97

Testing done:
* `make check`
* `qemu-system-ppc -M pegasos2 -rtc base=localtime -device
  ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso -kernel
  morphos-3.17/boot.img`
  -> There is a nice sound when the Desktop becomes visible.
* `qemu-system-ppc -M pegasos2 -bios pegasos2.rom -rtc base=localtime -device
  ati-vga,guest_hwcursor=true,romfile="" -cdrom morphos-3.17.iso`
  -> There is a nice sound when the Desktop becomes visible.
* `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`
  -> The machine hangs when audio is supposed to play while the mouse is moved.
     This behavior can also be reproduced in v1.

[1] https://lore.kernel.org/qemu-devel/cover.1677004414.git.balaton@eik.bme.hu/

BALATON Zoltan (2):
  hw/audio/ac97: Split off some definitions to a header
  hw/audio/via-ac97: Basic implementation of audio playback

Bernhard Beschow (3):
  hw/ppc/pegasos2: Initialize VT8231 PCI IRQ router
  hw/isa/vt82c686: Implement PCI IRQ routing
  hw/usb/vt82c686-uhci-pci: Use PCI IRQ routing

 hw/audio/ac97.h            |  65 ++++++
 include/hw/isa/vt82c686.h  |  25 +++
 hw/audio/ac97.c            |  43 +---
 hw/audio/via-ac97.c        | 436 ++++++++++++++++++++++++++++++++++++-
 hw/isa/vt82c686.c          |  46 +++-
 hw/ppc/pegasos2.c          |   6 +
 hw/usb/vt82c686-uhci-pci.c |  12 -
 hw/audio/trace-events      |   6 +
 8 files changed, 580 insertions(+), 59 deletions(-)
 create mode 100644 hw/audio/ac97.h

-- 
2.39.2

[PATCH 1/5] hw/ppc/pegasos2: Initialize VT8231 PCI IRQ router

[Bernhard Beschow]

The firmware of the real PegasosII board routes all PIRQx to IRQ9, so do
the same in QEMU. The PCI_INTERRUPT_LINE registers of the respective
internal PCI functions are already initialized with IRQ9 which are
currently used for routing.

Note that the PCI interrupt router isn't implemented yet in the VIA
south bridges. This change has therefore no effect until this happens.

Inspired-by:
<c046d77c20875c8cd8bfdc79b4619a98ffd0bf33.1677004415.git.balaton@eik.bme.hu>
("hw/ppc/pegasos2: Fix PCI interrupt routing")

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/pegasos2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index a9563f4fb2..41688699eb 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -268,6 +268,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
                               PCI_INTERRUPT_LINE, 2, 0x9);
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
                               0x50, 1, 0x2);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x55, 1, 0x90);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x56, 1, 0x99);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x57, 1, 0x90);
 
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 1) << 8) |
                               PCI_INTERRUPT_LINE, 2, 0x109);
-- 
2.39.2

Re: [PATCH 1/5] hw/ppc/pegasos2: Initialize VT8231 PCI IRQ router

[Mark Cave-Ayland]

On 23/02/2023 20:20, Bernhard Beschow wrote:

> The firmware of the real PegasosII board routes all PIRQx to IRQ9, so do
> the same in QEMU. The PCI_INTERRUPT_LINE registers of the respective
> internal PCI functions are already initialized with IRQ9 which are
> currently used for routing.
> 
> Note that the PCI interrupt router isn't implemented yet in the VIA
> south bridges. This change has therefore no effect until this happens.
> 
> Inspired-by:
> <c046d77c20875c8cd8bfdc79b4619a98ffd0bf33.1677004415.git.balaton@eik.bme.hu>
> ("hw/ppc/pegasos2: Fix PCI interrupt routing")
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/pegasos2.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index a9563f4fb2..41688699eb 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -268,6 +268,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
>                                 PCI_INTERRUPT_LINE, 2, 0x9);
>       pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>                                 0x50, 1, 0x2);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x55, 1, 0x90);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x56, 1, 0x99);
> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
> +                              0x57, 1, 0x90);
>   
>       pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 1) << 8) |
>                                 PCI_INTERRUPT_LINE, 2, 0x109);

The patch looks okay, but I think the commit message doesn't quite represent why it 
is required. I presume this configures the PCI IRQ router in the same way as the 
firmware so that it is possible to launch Linux directly with -kernel?


ATB,

Mark.

Re: [PATCH 1/5] hw/ppc/pegasos2: Initialize VT8231 PCI IRQ router

[BALATON Zoltan]

On Wed, 1 Mar 2023, Mark Cave-Ayland wrote:
> On 23/02/2023 20:20, Bernhard Beschow wrote:
>
>> The firmware of the real PegasosII board routes all PIRQx to IRQ9, so do
>> the same in QEMU. The PCI_INTERRUPT_LINE registers of the respective
>> internal PCI functions are already initialized with IRQ9 which are
>> currently used for routing.
>> 
>> Note that the PCI interrupt router isn't implemented yet in the VIA
>> south bridges. This change has therefore no effect until this happens.
>> 
>> Inspired-by:
>> <c046d77c20875c8cd8bfdc79b4619a98ffd0bf33.1677004415.git.balaton@eik.bme.hu>
>> ("hw/ppc/pegasos2: Fix PCI interrupt routing")
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/pegasos2.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>> index a9563f4fb2..41688699eb 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -268,6 +268,12 @@ static void pegasos2_machine_reset(MachineState 
>> *machine, ShutdownCause reason)
>>                                 PCI_INTERRUPT_LINE, 2, 0x9);
>>       pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>>                                 0x50, 1, 0x2);
>> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>> +                              0x55, 1, 0x90);
>> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>> +                              0x56, 1, 0x99);
>> +    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
>> +                              0x57, 1, 0x90);
>>         pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 1) << 8) |
>>                                 PCI_INTERRUPT_LINE, 2, 0x109);
>
> The patch looks okay, but I think the commit message doesn't quite represent 
> why it is required. I presume this configures the PCI IRQ router in the same 
> way as the firmware so that it is possible to launch Linux directly with 
> -kernel?

You're commenting on old versions. Please only review v5 now which is the 
last version we're about to commit for 8.0. You can read through older 
discussion to see where we are but probably not much use to comment on 
those now.

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

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);
 }
 
 /* TYPE_VT82C686B_ISA */
-- 
2.39.2

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

[BALATON Zoltan]

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. I've implemented that for something but can't remember 
now which guest exactly, 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? I'll try this one in 
the meantime but I'm quite sure this is wrong as it is. You can drop the 
via-ac97 patches from this series, I can submit them separately rebased on 
the final IRQ series we come up with.

Regards,
BALATON Zoltan

> }
>
> /* TYPE_VT82C686B_ISA */
>

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

[Bernhard Beschow]

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'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.

> 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'll try this one in the meantime

Sounds good to me -- that's what I wanted to achieve ;) Thanks!

Best regards,
Bernhard

> but I'm quite sure this is wrong as it is. You can drop the via-ac97 patches from this series, I can submit them separately rebased on the final IRQ series we come up with.
>
>Regards,
>BALATON Zoltan
>
>> }
>> 
>> /* TYPE_VT82C686B_ISA */
>> 

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

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

[BALATON Zoltan]

On Fri, 24 Feb 2023, BALATON Zoltan wrote:
> 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);
> +    }

Actually that should be:

+    old_state = 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_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);
+    }
+    qemu_set_irq(s->isa_irqs[isa_irq], !!s->isa_irq_state[isa_irq]);

with the old_state variable definition at the top of the func of course.

> 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
>
>

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

[Bernhard Beschow]

Am 23. Februar 2023 23:47:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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.

I can only take the three VT82xx patches as I proposed since I don't know the Pegasos2 board as well as you do and I don't want to iterate on any review comments for the other patches. I'll send my series soonish.

Best regards,
Bernhard

>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

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

[BALATON Zoltan]

On Fri, 24 Feb 2023, Bernhard Beschow wrote:
> I can only take the three VT82xx patches as I proposed since I don't 
> know the Pegasos2 board as well as you do and I don't want to iterate on 
> any review comments for the other patches. I'll send my series soonish.

Does soonish means still today? Sorry for being impatient but I'd like to 
finalise this by Monday so to be able to rebase my changes and make it 
avaialbe for testing as soon as possible, still in the weekend as people 
won't have time on weekdays, then we should need the final version of your 
alternative patches about now.

Regards,
BALATON Zoltan

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

[Bernhard Beschow]

Am 25. Februar 2023 13:12:05 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Fri, 24 Feb 2023, Bernhard Beschow wrote:
>> I can only take the three VT82xx patches as I proposed since I don't know the Pegasos2 board as well as you do and I don't want to iterate on any review comments for the other patches. I'll send my series soonish.
>
>Does soonish means still today? Sorry for being impatient but I'd like to finalise this by Monday so to be able to rebase my changes and make it avaialbe for testing as soon as possible, still in the weekend as people won't have time on weekdays, then we should need the final version of your alternative patches about now.

Sure, respin is out. It's so fresh I can't even give you a link yet ;)

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan

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

[BALATON Zoltan]

On Thu, 23 Feb 2023, BALATON Zoltan wrote:
> 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. I've implemented that for something but can't remember now which 
> guest exactly, 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? I'll try this one in the meantime but I'm quite 
> sure this is wrong as it is. You can drop the via-ac97 patches from this 
> series, I can submit them separately rebased on the final IRQ series we come 
> up with.

We'd also need the workaround I've posted yesterday for missing level 
sensitive interrupts in the PIC model here somewhere which in my series 
fixed the hang with sound and USB but it's still a problem with this 
series. After brief testing my guests still boot with this, I could not 
find the guest that needed the MV64361 gpp interrupts for PCI but let's 
not break that if it's already implemented. Can you do another version 
with these changes (restore PIRQ and pegasos2 patches and add workaround 
for level sensitive mode) so I can ask others to do some more tests with 
this over the weekend?

Regards,
BALATON Zoltan

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

[Mark Cave-Ayland]

On 23/02/2023 20:20, 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);
>   }
>   
>   /* TYPE_VT82C686B_ISA */

This looks right, however generally a PCI device shouldn't really be setting PCI bus 
IRQs: this is normally done by the PCI host bridge. Is it just the case that the x86 
world is different here for legacy reasons?


ATB,

Mark.

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

[Bernhard Beschow]

Am 1. März 2023 14:20:54 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 23/02/2023 20:20, 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);
>>   }
>>     /* TYPE_VT82C686B_ISA */
>
>This looks right, however generally a PCI device shouldn't really be setting PCI bus IRQs: this is normally done by the PCI host bridge. Is it just the case that the x86 world is different here for legacy reasons?

Well, looking at the pegasos2 schematics it seems to me that at least the intA + intB lines are connected to both chips (and that they are even shared between the AGP slot and the PCI bus). On the Marvell north bridge they seem to be connected to GPIO pins and on the VIA chip to the PCI IRQ router.

Note that GPIO pins can usually be deactivated (tristated) and that the four VIA PCI IRQs can be deactivated by assigning "interrupt 0". Such a design would allow the lines to be hardwired to both "interrupt controllers", allowing system software to use either controller, or possibly even mixed (e.g. intA to be treated by the north bridge and intB by VIA).

Such designs are currently not easily implementable in QEMU since only one IRQ handler can be assigned to a PCI bus. As a workaround, one could assign a custom IRQ handler which implements special handling.

Getting back to your question, I think you are right that assigning the IRQ handler in the VIA model may break e.g. the Fuloong2e machine where the IRQ handler is set in the north bridge. Since the VIA chip is instantiated later it now effectively replaces the handler.

It would be really neat if QEMU allowed for assigning two or more IRQ handlers to a PCI bus...

Do you think that two interrupt controllers connected to IRQ lines like that sounds reasonable?

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.

[PATCH 3/5] hw/usb/vt82c686-uhci-pci: Use PCI IRQ routing

[Bernhard Beschow]

According to the PCI specification, PCI_INTERRUPT_LINE shall have no
effect on hardware operations. Now that the VIA south bridges implement
the internal PCI interrupt router let's be more conformant to the PCI
specification.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/usb/vt82c686-uhci-pci.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index 46a901f56f..b4884c9011 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -1,17 +1,7 @@
 #include "qemu/osdep.h"
-#include "hw/irq.h"
 #include "hw/isa/vt82c686.h"
 #include "hcd-uhci.h"
 
-static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
-{
-    UHCIState *s = opaque;
-    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
-    if (irq > 0 && irq < 15) {
-        via_isa_set_irq(pci_get_function_0(&s->dev), irq, level);
-    }
-}
-
 static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
 {
     UHCIState *s = UHCI(dev);
@@ -25,8 +15,6 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + 0xc0, 0x00002000);
 
     usb_uhci_common_realize(dev, errp);
-    object_unref(s->irq);
-    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
 }
 
 static UHCIInfo uhci_info[] = {
-- 
2.39.2

Re: [PATCH 3/5] hw/usb/vt82c686-uhci-pci: Use PCI IRQ routing

[Mark Cave-Ayland]

On 23/02/2023 20:20, Bernhard Beschow wrote:

> According to the PCI specification, PCI_INTERRUPT_LINE shall have no
> effect on hardware operations. Now that the VIA south bridges implement
> the internal PCI interrupt router let's be more conformant to the PCI
> specification.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/usb/vt82c686-uhci-pci.c | 12 ------------
>   1 file changed, 12 deletions(-)
> 
> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
> index 46a901f56f..b4884c9011 100644
> --- a/hw/usb/vt82c686-uhci-pci.c
> +++ b/hw/usb/vt82c686-uhci-pci.c
> @@ -1,17 +1,7 @@
>   #include "qemu/osdep.h"
> -#include "hw/irq.h"
>   #include "hw/isa/vt82c686.h"
>   #include "hcd-uhci.h"
>   
> -static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
> -{
> -    UHCIState *s = opaque;
> -    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
> -    if (irq > 0 && irq < 15) {
> -        via_isa_set_irq(pci_get_function_0(&s->dev), irq, level);
> -    }
> -}
> -
>   static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>   {
>       UHCIState *s = UHCI(dev);
> @@ -25,8 +15,6 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
>       pci_set_long(pci_conf + 0xc0, 0x00002000);
>   
>       usb_uhci_common_realize(dev, errp);
> -    object_unref(s->irq);
> -    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
>   }
>   
>   static UHCIInfo uhci_info[] = {

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH 4/5] hw/audio/ac97: Split off some definitions to a header

[Bernhard Beschow]

From: BALATON Zoltan <balaton@eik.bme.hu>

These can be shared with other AC97 implementations.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/audio/ac97.h | 65 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/audio/ac97.c | 43 +-------------------------------
 2 files changed, 66 insertions(+), 42 deletions(-)
 create mode 100644 hw/audio/ac97.h

diff --git a/hw/audio/ac97.h b/hw/audio/ac97.h
new file mode 100644
index 0000000000..0358b56ff4
--- /dev/null
+++ b/hw/audio/ac97.h
@@ -0,0 +1,65 @@
+/*
+ * Copyright (C) 2006 InnoTek Systemberatung GmbH
+ *
+ * This file is part of VirtualBox Open Source Edition (OSE), as
+ * available from http://www.virtualbox.org. This file is free software;
+ * you can redistribute it and/or modify it under the terms of the GNU
+ * General Public License as published by the Free Software Foundation,
+ * in version 2 as it comes in the "COPYING" file of the VirtualBox OSE
+ * distribution. VirtualBox OSE is distributed in the hope that it will
+ * be useful, but WITHOUT ANY WARRANTY of any kind.
+ *
+ * If you received this file as part of a commercial VirtualBox
+ * distribution, then only the terms of your commercial VirtualBox
+ * license agreement apply instead of the previous paragraph.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#ifndef AC97_H
+#define AC97_H
+
+enum {
+    AC97_Reset                     = 0x00,
+    AC97_Master_Volume_Mute        = 0x02,
+    AC97_Headphone_Volume_Mute     = 0x04,
+    AC97_Master_Volume_Mono_Mute   = 0x06,
+    AC97_Master_Tone_RL            = 0x08,
+    AC97_PC_BEEP_Volume_Mute       = 0x0A,
+    AC97_Phone_Volume_Mute         = 0x0C,
+    AC97_Mic_Volume_Mute           = 0x0E,
+    AC97_Line_In_Volume_Mute       = 0x10,
+    AC97_CD_Volume_Mute            = 0x12,
+    AC97_Video_Volume_Mute         = 0x14,
+    AC97_Aux_Volume_Mute           = 0x16,
+    AC97_PCM_Out_Volume_Mute       = 0x18,
+    AC97_Record_Select             = 0x1A,
+    AC97_Record_Gain_Mute          = 0x1C,
+    AC97_Record_Gain_Mic_Mute      = 0x1E,
+    AC97_General_Purpose           = 0x20,
+    AC97_3D_Control                = 0x22,
+    AC97_AC_97_RESERVED            = 0x24,
+    AC97_Powerdown_Ctrl_Stat       = 0x26,
+    AC97_Extended_Audio_ID         = 0x28,
+    AC97_Extended_Audio_Ctrl_Stat  = 0x2A,
+    AC97_PCM_Front_DAC_Rate        = 0x2C,
+    AC97_PCM_Surround_DAC_Rate     = 0x2E,
+    AC97_PCM_LFE_DAC_Rate          = 0x30,
+    AC97_PCM_LR_ADC_Rate           = 0x32,
+    AC97_MIC_ADC_Rate              = 0x34,
+    AC97_6Ch_Vol_C_LFE_Mute        = 0x36,
+    AC97_6Ch_Vol_L_R_Surround_Mute = 0x38,
+    AC97_Vendor_Reserved           = 0x58,
+    AC97_Sigmatel_Analog           = 0x6c, /* We emulate a Sigmatel codec */
+    AC97_Sigmatel_Dac2Invert       = 0x6e, /* We emulate a Sigmatel codec */
+    AC97_Vendor_ID1                = 0x7c,
+    AC97_Vendor_ID2                = 0x7e
+};
+
+#define EACS_VRA 1
+#define EACS_VRM 8
+
+#define MUTE_SHIFT 15
+
+#endif /* AC97_H */
diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
index 364cdfa733..b3fb10284c 100644
--- a/hw/audio/ac97.c
+++ b/hw/audio/ac97.c
@@ -26,43 +26,7 @@
 #include "qemu/module.h"
 #include "sysemu/dma.h"
 #include "qom/object.h"
-
-enum {
-    AC97_Reset                     = 0x00,
-    AC97_Master_Volume_Mute        = 0x02,
-    AC97_Headphone_Volume_Mute     = 0x04,
-    AC97_Master_Volume_Mono_Mute   = 0x06,
-    AC97_Master_Tone_RL            = 0x08,
-    AC97_PC_BEEP_Volume_Mute       = 0x0A,
-    AC97_Phone_Volume_Mute         = 0x0C,
-    AC97_Mic_Volume_Mute           = 0x0E,
-    AC97_Line_In_Volume_Mute       = 0x10,
-    AC97_CD_Volume_Mute            = 0x12,
-    AC97_Video_Volume_Mute         = 0x14,
-    AC97_Aux_Volume_Mute           = 0x16,
-    AC97_PCM_Out_Volume_Mute       = 0x18,
-    AC97_Record_Select             = 0x1A,
-    AC97_Record_Gain_Mute          = 0x1C,
-    AC97_Record_Gain_Mic_Mute      = 0x1E,
-    AC97_General_Purpose           = 0x20,
-    AC97_3D_Control                = 0x22,
-    AC97_AC_97_RESERVED            = 0x24,
-    AC97_Powerdown_Ctrl_Stat       = 0x26,
-    AC97_Extended_Audio_ID         = 0x28,
-    AC97_Extended_Audio_Ctrl_Stat  = 0x2A,
-    AC97_PCM_Front_DAC_Rate        = 0x2C,
-    AC97_PCM_Surround_DAC_Rate     = 0x2E,
-    AC97_PCM_LFE_DAC_Rate          = 0x30,
-    AC97_PCM_LR_ADC_Rate           = 0x32,
-    AC97_MIC_ADC_Rate              = 0x34,
-    AC97_6Ch_Vol_C_LFE_Mute        = 0x36,
-    AC97_6Ch_Vol_L_R_Surround_Mute = 0x38,
-    AC97_Vendor_Reserved           = 0x58,
-    AC97_Sigmatel_Analog           = 0x6c, /* We emulate a Sigmatel codec */
-    AC97_Sigmatel_Dac2Invert       = 0x6e, /* We emulate a Sigmatel codec */
-    AC97_Vendor_ID1                = 0x7c,
-    AC97_Vendor_ID2                = 0x7e
-};
+#include "ac97.h"
 
 #define SOFT_VOLUME
 #define SR_FIFOE 16             /* rwc */
@@ -121,11 +85,6 @@ enum {
 #define BD_IOC (1 << 31)
 #define BD_BUP (1 << 30)
 
-#define EACS_VRA 1
-#define EACS_VRM 8
-
-#define MUTE_SHIFT 15
-
 #define TYPE_AC97 "AC97"
 OBJECT_DECLARE_SIMPLE_TYPE(AC97LinkState, AC97)
 
-- 
2.39.2

[PATCH 5/5] hw/audio/via-ac97: Basic implementation of audio playback

[Bernhard Beschow]

From: BALATON Zoltan <balaton@eik.bme.hu>

This adds basic implementation of the AC'97 sound part used in VIA
south bridge chips. Not all features of the device is emulated, only
one playback channel is supported but this is enough to get sound
output from some guests running on machines using this device such as
pegasos2.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 include/hw/isa/vt82c686.h |  25 +++
 hw/audio/via-ac97.c       | 436 +++++++++++++++++++++++++++++++++++++-
 hw/isa/vt82c686.c         |   2 +-
 hw/audio/trace-events     |   6 +
 4 files changed, 464 insertions(+), 5 deletions(-)

diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index e273cd38dc..da1722daf2 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -1,6 +1,8 @@
 #ifndef HW_VT82C686_H
 #define HW_VT82C686_H
 
+#include "hw/pci/pci_device.h"
+#include "audio/audio.h"
 
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
@@ -9,6 +11,29 @@
 #define TYPE_VIA_IDE "via-ide"
 #define TYPE_VIA_MC97 "via-mc97"
 
+typedef struct {
+    uint8_t stat;
+    uint8_t type;
+    uint32_t base;
+    uint32_t curr;
+    uint32_t addr;
+    uint32_t clen;
+} ViaAC97SGDChannel;
+
+OBJECT_DECLARE_SIMPLE_TYPE(ViaAC97State, VIA_AC97);
+
+struct ViaAC97State {
+    PCIDevice dev;
+    QEMUSoundCard card;
+    MemoryRegion sgd;
+    MemoryRegion fm;
+    MemoryRegion midi;
+    SWVoiceOut *vo;
+    ViaAC97SGDChannel aur;
+    uint16_t codec_regs[128];
+    uint32_t ac97_cmd;
+};
+
 void via_isa_set_irq(PCIDevice *d, int n, int level);
 
 #endif
diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
index d1a856f63d..0b1eeb4272 100644
--- a/hw/audio/via-ac97.c
+++ b/hw/audio/via-ac97.c
@@ -1,39 +1,467 @@
 /*
  * VIA south bridges sound support
  *
+ * Copyright (c) 2022-2023 BALATON Zoltan
+ *
  * This work is licensed under the GNU GPL license version 2 or later.
  */
 
 /*
- * TODO: This is entirely boiler plate just registering empty PCI devices
- * with the right ID guests expect, functionality should be added here.
+ * TODO: This is only a basic implementation of one audio playback channel
+ *       more functionality should be added here.
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/isa/vt82c686.h"
-#include "hw/pci/pci_device.h"
+#include "ac97.h"
+#include "trace.h"
+
+#define CLEN_IS_EOL(x)  ((x)->clen & BIT(31))
+#define CLEN_IS_FLAG(x) ((x)->clen & BIT(30))
+#define CLEN_IS_STOP(x) ((x)->clen & BIT(29))
+#define CLEN_LEN(x)     ((x)->clen & 0xfff)
+
+#define STAT_ACTIVE BIT(7)
+#define STAT_PAUSED BIT(6)
+#define STAT_TRIG   BIT(3)
+#define STAT_STOP   BIT(2)
+#define STAT_EOL    BIT(1)
+#define STAT_FLAG   BIT(0)
+
+#define CNTL_START  BIT(7)
+#define CNTL_TERM   BIT(6)
+#define CNTL_PAUSE  BIT(3)
+
+static void open_voice_out(ViaAC97State *s);
+
+static uint16_t codec_rates[] = { 8000, 11025, 16000, 22050, 32000, 44100,
+                                  48000 };
+
+#define CODEC_REG(s, o)  ((s)->codec_regs[(o) / 2])
+#define CODEC_VOL(vol, mask)  ((255 * ((vol) & mask)) / mask)
+
+static void codec_volume_set_out(ViaAC97State *s)
+{
+    int lvol, rvol, mute;
+
+    lvol = 255 - CODEC_VOL(CODEC_REG(s, AC97_Master_Volume_Mute) >> 8, 0x1f);
+    lvol *= 255 - CODEC_VOL(CODEC_REG(s, AC97_PCM_Out_Volume_Mute) >> 8, 0x1f);
+    lvol /= 255;
+    rvol = 255 - CODEC_VOL(CODEC_REG(s, AC97_Master_Volume_Mute), 0x1f);
+    rvol *= 255 - CODEC_VOL(CODEC_REG(s, AC97_PCM_Out_Volume_Mute), 0x1f);
+    rvol /= 255;
+    mute = CODEC_REG(s, AC97_Master_Volume_Mute) >> MUTE_SHIFT;
+    mute |= CODEC_REG(s, AC97_PCM_Out_Volume_Mute) >> MUTE_SHIFT;
+    AUD_set_volume_out(s->vo, mute, lvol, rvol);
+}
+
+static void codec_reset(ViaAC97State *s)
+{
+    memset(s->codec_regs, 0, sizeof(s->codec_regs));
+    CODEC_REG(s, AC97_Reset) = 0x6a90;
+    CODEC_REG(s, AC97_Master_Volume_Mute) = 0x8000;
+    CODEC_REG(s, AC97_Headphone_Volume_Mute) = 0x8000;
+    CODEC_REG(s, AC97_Master_Volume_Mono_Mute) = 0x8000;
+    CODEC_REG(s, AC97_Phone_Volume_Mute) = 0x8008;
+    CODEC_REG(s, AC97_Mic_Volume_Mute) = 0x8008;
+    CODEC_REG(s, AC97_Line_In_Volume_Mute) = 0x8808;
+    CODEC_REG(s, AC97_CD_Volume_Mute) = 0x8808;
+    CODEC_REG(s, AC97_Video_Volume_Mute) = 0x8808;
+    CODEC_REG(s, AC97_Aux_Volume_Mute) = 0x8808;
+    CODEC_REG(s, AC97_PCM_Out_Volume_Mute) = 0x8808;
+    CODEC_REG(s, AC97_Record_Gain_Mute) = 0x8000;
+    CODEC_REG(s, AC97_Powerdown_Ctrl_Stat) = 0x000f;
+    CODEC_REG(s, AC97_Extended_Audio_ID) = 0x0a05;
+    CODEC_REG(s, AC97_Extended_Audio_Ctrl_Stat) = 0x0400;
+    CODEC_REG(s, AC97_PCM_Front_DAC_Rate) = 48000;
+    CODEC_REG(s, AC97_PCM_LR_ADC_Rate) = 48000;
+    /* Sigmatel 9766 (STAC9766) */
+    CODEC_REG(s, AC97_Vendor_ID1) = 0x8384;
+    CODEC_REG(s, AC97_Vendor_ID2) = 0x7666;
+}
+
+static uint16_t codec_read(ViaAC97State *s, uint8_t addr)
+{
+    return CODEC_REG(s, addr);
+}
+
+static void codec_write(ViaAC97State *s, uint8_t addr, uint16_t val)
+{
+    trace_via_ac97_codec_write(addr, val);
+    switch (addr) {
+    case AC97_Reset:
+        codec_reset(s);
+        return;
+    case AC97_Master_Volume_Mute:
+    case AC97_PCM_Out_Volume_Mute:
+        if (addr == AC97_Master_Volume_Mute) {
+            if (val & BIT(13)) {
+                val |= 0x1f00;
+            }
+            if (val & BIT(5)) {
+                val |= 0x1f;
+            }
+        }
+        CODEC_REG(s, addr) = val & 0x9f1f;
+        codec_volume_set_out(s);
+        return;
+    case AC97_Extended_Audio_Ctrl_Stat:
+        CODEC_REG(s, addr) &= ~EACS_VRA;
+        CODEC_REG(s, addr) |= val & EACS_VRA;
+        if (!(val & EACS_VRA)) {
+            CODEC_REG(s, AC97_PCM_Front_DAC_Rate) = 48000;
+            CODEC_REG(s, AC97_PCM_LR_ADC_Rate) = 48000;
+            open_voice_out(s);
+        }
+        return;
+    case AC97_PCM_Front_DAC_Rate:
+    case AC97_PCM_LR_ADC_Rate:
+        if (CODEC_REG(s, AC97_Extended_Audio_Ctrl_Stat) & EACS_VRA) {
+            int i;
+            uint16_t rate = val;
+
+            for (i = 0; i < ARRAY_SIZE(codec_rates) - 1; i++) {
+                if (rate < codec_rates[i] +
+                    (codec_rates[i + 1] - codec_rates[i]) / 2) {
+                    rate = codec_rates[i];
+                    break;
+                }
+            }
+            if (rate > 48000) {
+                rate = 48000;
+            }
+            CODEC_REG(s, addr) = rate;
+            open_voice_out(s);
+        }
+        return;
+    case AC97_Powerdown_Ctrl_Stat:
+        CODEC_REG(s, addr) = (val & 0xff00) | (CODEC_REG(s, addr) & 0xff);
+        return;
+    case AC97_Extended_Audio_ID:
+    case AC97_Vendor_ID1:
+    case AC97_Vendor_ID2:
+        /* Read only registers */
+        return;
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "via-ac97: Unimplemented codec register 0x%x\n", addr);
+        CODEC_REG(s, addr) = val;
+    }
+}
+
+static void fetch_sgd(ViaAC97SGDChannel *c, PCIDevice *d)
+{
+    uint32_t b[2];
+
+    if (c->curr < c->base) {
+        c->curr = c->base;
+    }
+    pci_dma_read(d, c->curr, b, sizeof(b));
+    c->addr = le32_to_cpu(b[0]);
+    c->clen = le32_to_cpu(b[1]);
+    trace_via_ac97_sgd_fetch(c->addr, CLEN_IS_STOP(c) ? 'S' : '-',
+                             CLEN_IS_EOL(c) ? 'E' : '-',
+                             CLEN_IS_FLAG(c) ? 'F' : '-', CLEN_LEN(c));
+}
+
+static void out_cb(void *opaque, int avail)
+{
+    ViaAC97State *s = opaque;
+    ViaAC97SGDChannel *c = &s->aur;
+    int temp, to_copy, copied;
+    bool stop = false;
+    uint8_t tmpbuf[4096];
+
+    if (c->stat & STAT_PAUSED) {
+        return;
+    }
+    c->stat |= STAT_ACTIVE;
+    while (avail && !stop) {
+        if (!c->clen) {
+            fetch_sgd(c, &s->dev);
+        }
+        temp = MIN(CLEN_LEN(c), avail);
+        while (temp) {
+            to_copy = MIN(temp, sizeof(tmpbuf));
+            pci_dma_read(&s->dev, c->addr, tmpbuf, to_copy);
+            copied = AUD_write(s->vo, tmpbuf, to_copy);
+            if (!copied) {
+                stop = true;
+                break;
+            }
+            temp -= copied;
+            avail -= copied;
+            c->addr += copied;
+            c->clen -= copied;
+        }
+        if (CLEN_LEN(c) == 0) {
+            c->curr += 8;
+            if (CLEN_IS_EOL(c)) {
+                c->stat |= STAT_EOL;
+                if (c->type & CNTL_START) {
+                    c->curr = c->base;
+                    c->stat |= STAT_PAUSED;
+                } else {
+                    c->stat &= ~STAT_ACTIVE;
+                    AUD_set_active_out(s->vo, 0);
+                }
+                if (c->type & STAT_EOL) {
+                    pci_set_irq(&s->dev, 1);
+                }
+            }
+            if (CLEN_IS_FLAG(c)) {
+                c->stat |= STAT_FLAG;
+                c->stat |= STAT_PAUSED;
+                if (c->type & STAT_FLAG) {
+                    pci_set_irq(&s->dev, 1);
+                }
+            }
+            if (CLEN_IS_STOP(c)) {
+                c->stat |= STAT_STOP;
+                c->stat |= STAT_PAUSED;
+            }
+            c->clen = 0;
+            stop = true;
+        }
+    }
+}
+
+static void open_voice_out(ViaAC97State *s)
+{
+    struct audsettings as = {
+        .freq = CODEC_REG(s, AC97_PCM_Front_DAC_Rate),
+        .nchannels = s->aur.type & BIT(4) ? 2 : 1,
+        .fmt = s->aur.type & BIT(5) ? AUDIO_FORMAT_S16 : AUDIO_FORMAT_S8,
+        .endianness = 0,
+    };
+    s->vo = AUD_open_out(&s->card, s->vo, "via-ac97.out", s, out_cb, &as);
+}
+
+static uint64_t sgd_read(void *opaque, hwaddr addr, unsigned size)
+{
+    ViaAC97State *s = opaque;
+    uint64_t val = 0;
+
+    switch (addr) {
+    case 0:
+        val = s->aur.stat;
+        if (s->aur.type & CNTL_START) {
+            val |= STAT_TRIG;
+        }
+        break;
+    case 1:
+        val = s->aur.stat & STAT_PAUSED ? BIT(3) : 0;
+        break;
+    case 2:
+        val = s->aur.type;
+        break;
+    case 4:
+        val = s->aur.curr;
+        break;
+    case 0xc:
+        val = CLEN_LEN(&s->aur);
+        break;
+    case 0x80:
+        val = s->ac97_cmd;
+        break;
+    case 0x84:
+        val = s->aur.stat & STAT_FLAG;
+        if (s->aur.stat & STAT_EOL) {
+            val |= BIT(4);
+        }
+        if (s->aur.stat & STAT_STOP) {
+            val |= BIT(8);
+        }
+        if (s->aur.stat & STAT_ACTIVE) {
+            val |= BIT(12);
+        }
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "via-ac97: Unimplemented register read %"
+                      HWADDR_PRIx"\n", addr);
+    }
+    trace_via_ac97_sgd_read(addr, size, val);
+    return val;
+}
+
+static void sgd_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    ViaAC97State *s = opaque;
+
+    trace_via_ac97_sgd_write(addr, size, val);
+    switch (addr) {
+    case 0:
+        if (val & STAT_STOP) {
+            s->aur.stat &= ~STAT_PAUSED;
+        }
+        if (val & STAT_EOL) {
+            s->aur.stat &= ~(STAT_EOL | STAT_PAUSED);
+            if (s->aur.type & STAT_EOL) {
+                pci_set_irq(&s->dev, 0);
+            }
+        }
+        if (val & STAT_FLAG) {
+            s->aur.stat &= ~(STAT_FLAG | STAT_PAUSED);
+            if (s->aur.type & STAT_FLAG) {
+                pci_set_irq(&s->dev, 0);
+            }
+        }
+        break;
+    case 1:
+        if (val & CNTL_START) {
+            AUD_set_active_out(s->vo, 1);
+            s->aur.stat = STAT_ACTIVE;
+        }
+        if (val & CNTL_TERM) {
+            AUD_set_active_out(s->vo, 0);
+            s->aur.stat &= ~(STAT_ACTIVE | STAT_PAUSED);
+            s->aur.clen = 0;
+        }
+        if (val & CNTL_PAUSE) {
+            AUD_set_active_out(s->vo, 0);
+            s->aur.stat &= ~STAT_ACTIVE;
+            s->aur.stat |= STAT_PAUSED;
+        } else if (!(val & CNTL_PAUSE) && (s->aur.stat & STAT_PAUSED)) {
+            AUD_set_active_out(s->vo, 1);
+            s->aur.stat |= STAT_ACTIVE;
+            s->aur.stat &= ~STAT_PAUSED;
+        }
+        break;
+    case 2:
+    {
+        uint32_t oldval = s->aur.type;
+        s->aur.type = val;
+        if ((oldval & 0x30) != (val & 0x30)) {
+            open_voice_out(s);
+        }
+        break;
+    }
+    case 4:
+        s->aur.base = val & ~1ULL;
+        break;
+    case 0x80:
+        if (val >> 30) {
+            /* we only have primary codec */
+            break;
+        }
+        if (val & BIT(23)) { /* read reg */
+            s->ac97_cmd = val & 0xc0ff0000ULL;
+            s->ac97_cmd |= codec_read(s, (val >> 16) & 0x7f);
+            s->ac97_cmd |= BIT(25); /* data valid */
+        } else {
+            s->ac97_cmd = val & 0xc0ffffffULL;
+            codec_write(s, (val >> 16) & 0x7f, val);
+        }
+        break;
+    case 0xc:
+    case 0x84:
+        /* Read only */
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "via-ac97: Unimplemented register write %"
+                      HWADDR_PRIx"\n", addr);
+    }
+}
+
+static const MemoryRegionOps sgd_ops = {
+    .read = sgd_read,
+    .write = sgd_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t fm_read(void *opaque, hwaddr addr, unsigned size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d\n", __func__, addr, size);
+    return 0;
+}
+
+static void fm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d <= 0x%"PRIX64"\n",
+                  __func__, addr, size, val);
+}
+
+static const MemoryRegionOps fm_ops = {
+    .read = fm_read,
+    .write = fm_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t midi_read(void *opaque, hwaddr addr, unsigned size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d\n", __func__, addr, size);
+    return 0;
+}
+
+static void midi_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d <= 0x%"PRIX64"\n",
+                  __func__, addr, size, val);
+}
+
+static const MemoryRegionOps midi_ops = {
+    .read = midi_read,
+    .write = midi_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void via_ac97_reset(DeviceState *dev)
+{
+    ViaAC97State *s = VIA_AC97(dev);
+
+    codec_reset(s);
+}
 
 static void via_ac97_realize(PCIDevice *pci_dev, Error **errp)
 {
+    ViaAC97State *s = VIA_AC97(pci_dev);
+    Object *o = OBJECT(s);
+
     pci_set_word(pci_dev->config + PCI_COMMAND,
                  PCI_COMMAND_INVALIDATE | PCI_COMMAND_PARITY);
     pci_set_word(pci_dev->config + PCI_STATUS,
                  PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_MEDIUM);
     pci_set_long(pci_dev->config + PCI_INTERRUPT_PIN, 0x03);
+
+    memory_region_init_io(&s->sgd, o, &sgd_ops, s, "via-ac97.sgd", 256);
+    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->sgd);
+    memory_region_init_io(&s->fm, o, &fm_ops, s, "via-ac97.fm", 4);
+    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->fm);
+    memory_region_init_io(&s->midi, o, &midi_ops, s, "via-ac97.midi", 4);
+    pci_register_bar(pci_dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->midi);
+
+    AUD_register_card ("via-ac97", &s->card);
 }
 
+static void via_ac97_exit(PCIDevice *dev)
+{
+    ViaAC97State *s = VIA_AC97(dev);
+
+    AUD_close_out(&s->card, s->vo);
+    AUD_remove_card(&s->card);
+}
+
+static Property via_ac97_properties[] = {
+    DEFINE_AUDIO_PROPERTIES(ViaAC97State, card),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void via_ac97_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->realize = via_ac97_realize;
+    k->exit = via_ac97_exit;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_AC97;
     k->revision = 0x50;
     k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
+    device_class_set_props(dc, via_ac97_properties);
     set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
     dc->desc = "VIA AC97";
+    dc->reset = via_ac97_reset;
     /* Reason: Part of a south bridge chip */
     dc->user_creatable = false;
 }
@@ -41,7 +469,7 @@ static void via_ac97_class_init(ObjectClass *klass, void *data)
 static const TypeInfo via_ac97_info = {
     .name          = TYPE_VIA_AC97,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PCIDevice),
+    .instance_size = sizeof(ViaAC97State),
     .class_init    = via_ac97_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f24e387d63..91780345d4 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -554,7 +554,7 @@ struct ViaISAState {
     PCIIDEState ide;
     UHCIState uhci[2];
     ViaPMState pm;
-    PCIDevice ac97;
+    ViaAC97State ac97;
     PCIDevice mc97;
 };
 
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index e0e71cd9b1..6eccdaa4b5 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -11,3 +11,9 @@ hda_audio_running(const char *stream, int nr, bool running) "st %s, nr %d, run %
 hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s, %d x %s @ %d Hz"
 hda_audio_adjust(const char *stream, int pos) "st %s, pos %d"
 hda_audio_overrun(const char *stream) "st %s"
+
+#via-ac97.c
+via_ac97_codec_write(uint8_t addr, uint16_t val) "0x%x <- 0x%x"
+via_ac97_sgd_fetch(uint32_t addr, char stop, char eol, char flag, uint32_t len) "addr=0x%x %c%c%c len=%d"
+via_ac97_sgd_read(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" %d -> 0x%"PRIx64
+via_ac97_sgd_write(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" %d <- 0x%"PRIx64
-- 
2.39.2

Re: [PATCH 5/5] hw/audio/via-ac97: Basic implementation of audio playback

[Mark Cave-Ayland]

On 23/02/2023 20:20, Bernhard Beschow wrote:

> From: BALATON Zoltan <balaton@eik.bme.hu>
> 
> This adds basic implementation of the AC'97 sound part used in VIA
> south bridge chips. Not all features of the device is emulated, only
> one playback channel is supported but this is enough to get sound
> output from some guests running on machines using this device such as
> pegasos2.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   include/hw/isa/vt82c686.h |  25 +++
>   hw/audio/via-ac97.c       | 436 +++++++++++++++++++++++++++++++++++++-
>   hw/isa/vt82c686.c         |   2 +-
>   hw/audio/trace-events     |   6 +
>   4 files changed, 464 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
> index e273cd38dc..da1722daf2 100644
> --- a/include/hw/isa/vt82c686.h
> +++ b/include/hw/isa/vt82c686.h
> @@ -1,6 +1,8 @@
>   #ifndef HW_VT82C686_H
>   #define HW_VT82C686_H
>   
> +#include "hw/pci/pci_device.h"
> +#include "audio/audio.h"
>   
>   #define TYPE_VT82C686B_ISA "vt82c686b-isa"
>   #define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
> @@ -9,6 +11,29 @@
>   #define TYPE_VIA_IDE "via-ide"
>   #define TYPE_VIA_MC97 "via-mc97"
>   
> +typedef struct {
> +    uint8_t stat;
> +    uint8_t type;
> +    uint32_t base;
> +    uint32_t curr;
> +    uint32_t addr;
> +    uint32_t clen;
> +} ViaAC97SGDChannel;
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ViaAC97State, VIA_AC97);
> +
> +struct ViaAC97State {
> +    PCIDevice dev;
> +    QEMUSoundCard card;
> +    MemoryRegion sgd;
> +    MemoryRegion fm;
> +    MemoryRegion midi;
> +    SWVoiceOut *vo;
> +    ViaAC97SGDChannel aur;
> +    uint16_t codec_regs[128];
> +    uint32_t ac97_cmd;
> +};
> +
>   void via_isa_set_irq(PCIDevice *d, int n, int level);
>   
>   #endif
> diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
> index d1a856f63d..0b1eeb4272 100644
> --- a/hw/audio/via-ac97.c
> +++ b/hw/audio/via-ac97.c
> @@ -1,39 +1,467 @@
>   /*
>    * VIA south bridges sound support
>    *
> + * Copyright (c) 2022-2023 BALATON Zoltan
> + *
>    * This work is licensed under the GNU GPL license version 2 or later.
>    */
>   
>   /*
> - * TODO: This is entirely boiler plate just registering empty PCI devices
> - * with the right ID guests expect, functionality should be added here.
> + * TODO: This is only a basic implementation of one audio playback channel
> + *       more functionality should be added here.
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/log.h"
>   #include "hw/isa/vt82c686.h"
> -#include "hw/pci/pci_device.h"
> +#include "ac97.h"
> +#include "trace.h"
> +
> +#define CLEN_IS_EOL(x)  ((x)->clen & BIT(31))
> +#define CLEN_IS_FLAG(x) ((x)->clen & BIT(30))
> +#define CLEN_IS_STOP(x) ((x)->clen & BIT(29))
> +#define CLEN_LEN(x)     ((x)->clen & 0xfff)
> +
> +#define STAT_ACTIVE BIT(7)
> +#define STAT_PAUSED BIT(6)
> +#define STAT_TRIG   BIT(3)
> +#define STAT_STOP   BIT(2)
> +#define STAT_EOL    BIT(1)
> +#define STAT_FLAG   BIT(0)
> +
> +#define CNTL_START  BIT(7)
> +#define CNTL_TERM   BIT(6)
> +#define CNTL_PAUSE  BIT(3)
> +
> +static void open_voice_out(ViaAC97State *s);
> +
> +static uint16_t codec_rates[] = { 8000, 11025, 16000, 22050, 32000, 44100,
> +                                  48000 };
> +
> +#define CODEC_REG(s, o)  ((s)->codec_regs[(o) / 2])
> +#define CODEC_VOL(vol, mask)  ((255 * ((vol) & mask)) / mask)
> +
> +static void codec_volume_set_out(ViaAC97State *s)
> +{
> +    int lvol, rvol, mute;
> +
> +    lvol = 255 - CODEC_VOL(CODEC_REG(s, AC97_Master_Volume_Mute) >> 8, 0x1f);
> +    lvol *= 255 - CODEC_VOL(CODEC_REG(s, AC97_PCM_Out_Volume_Mute) >> 8, 0x1f);
> +    lvol /= 255;
> +    rvol = 255 - CODEC_VOL(CODEC_REG(s, AC97_Master_Volume_Mute), 0x1f);
> +    rvol *= 255 - CODEC_VOL(CODEC_REG(s, AC97_PCM_Out_Volume_Mute), 0x1f);
> +    rvol /= 255;
> +    mute = CODEC_REG(s, AC97_Master_Volume_Mute) >> MUTE_SHIFT;
> +    mute |= CODEC_REG(s, AC97_PCM_Out_Volume_Mute) >> MUTE_SHIFT;
> +    AUD_set_volume_out(s->vo, mute, lvol, rvol);
> +}
> +
> +static void codec_reset(ViaAC97State *s)
> +{
> +    memset(s->codec_regs, 0, sizeof(s->codec_regs));
> +    CODEC_REG(s, AC97_Reset) = 0x6a90;
> +    CODEC_REG(s, AC97_Master_Volume_Mute) = 0x8000;
> +    CODEC_REG(s, AC97_Headphone_Volume_Mute) = 0x8000;
> +    CODEC_REG(s, AC97_Master_Volume_Mono_Mute) = 0x8000;
> +    CODEC_REG(s, AC97_Phone_Volume_Mute) = 0x8008;
> +    CODEC_REG(s, AC97_Mic_Volume_Mute) = 0x8008;
> +    CODEC_REG(s, AC97_Line_In_Volume_Mute) = 0x8808;
> +    CODEC_REG(s, AC97_CD_Volume_Mute) = 0x8808;
> +    CODEC_REG(s, AC97_Video_Volume_Mute) = 0x8808;
> +    CODEC_REG(s, AC97_Aux_Volume_Mute) = 0x8808;
> +    CODEC_REG(s, AC97_PCM_Out_Volume_Mute) = 0x8808;
> +    CODEC_REG(s, AC97_Record_Gain_Mute) = 0x8000;
> +    CODEC_REG(s, AC97_Powerdown_Ctrl_Stat) = 0x000f;
> +    CODEC_REG(s, AC97_Extended_Audio_ID) = 0x0a05;
> +    CODEC_REG(s, AC97_Extended_Audio_Ctrl_Stat) = 0x0400;
> +    CODEC_REG(s, AC97_PCM_Front_DAC_Rate) = 48000;
> +    CODEC_REG(s, AC97_PCM_LR_ADC_Rate) = 48000;
> +    /* Sigmatel 9766 (STAC9766) */
> +    CODEC_REG(s, AC97_Vendor_ID1) = 0x8384;
> +    CODEC_REG(s, AC97_Vendor_ID2) = 0x7666;
> +}
> +
> +static uint16_t codec_read(ViaAC97State *s, uint8_t addr)
> +{
> +    return CODEC_REG(s, addr);
> +}
> +
> +static void codec_write(ViaAC97State *s, uint8_t addr, uint16_t val)
> +{
> +    trace_via_ac97_codec_write(addr, val);
> +    switch (addr) {
> +    case AC97_Reset:
> +        codec_reset(s);
> +        return;
> +    case AC97_Master_Volume_Mute:
> +    case AC97_PCM_Out_Volume_Mute:
> +        if (addr == AC97_Master_Volume_Mute) {
> +            if (val & BIT(13)) {
> +                val |= 0x1f00;
> +            }
> +            if (val & BIT(5)) {
> +                val |= 0x1f;
> +            }
> +        }
> +        CODEC_REG(s, addr) = val & 0x9f1f;
> +        codec_volume_set_out(s);
> +        return;
> +    case AC97_Extended_Audio_Ctrl_Stat:
> +        CODEC_REG(s, addr) &= ~EACS_VRA;
> +        CODEC_REG(s, addr) |= val & EACS_VRA;
> +        if (!(val & EACS_VRA)) {
> +            CODEC_REG(s, AC97_PCM_Front_DAC_Rate) = 48000;
> +            CODEC_REG(s, AC97_PCM_LR_ADC_Rate) = 48000;
> +            open_voice_out(s);
> +        }
> +        return;
> +    case AC97_PCM_Front_DAC_Rate:
> +    case AC97_PCM_LR_ADC_Rate:
> +        if (CODEC_REG(s, AC97_Extended_Audio_Ctrl_Stat) & EACS_VRA) {
> +            int i;
> +            uint16_t rate = val;
> +
> +            for (i = 0; i < ARRAY_SIZE(codec_rates) - 1; i++) {
> +                if (rate < codec_rates[i] +
> +                    (codec_rates[i + 1] - codec_rates[i]) / 2) {
> +                    rate = codec_rates[i];
> +                    break;
> +                }
> +            }
> +            if (rate > 48000) {
> +                rate = 48000;
> +            }
> +            CODEC_REG(s, addr) = rate;
> +            open_voice_out(s);
> +        }
> +        return;
> +    case AC97_Powerdown_Ctrl_Stat:
> +        CODEC_REG(s, addr) = (val & 0xff00) | (CODEC_REG(s, addr) & 0xff);
> +        return;
> +    case AC97_Extended_Audio_ID:
> +    case AC97_Vendor_ID1:
> +    case AC97_Vendor_ID2:
> +        /* Read only registers */
> +        return;
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "via-ac97: Unimplemented codec register 0x%x\n", addr);
> +        CODEC_REG(s, addr) = val;
> +    }
> +}
> +
> +static void fetch_sgd(ViaAC97SGDChannel *c, PCIDevice *d)
> +{
> +    uint32_t b[2];
> +
> +    if (c->curr < c->base) {
> +        c->curr = c->base;
> +    }
> +    pci_dma_read(d, c->curr, b, sizeof(b));
> +    c->addr = le32_to_cpu(b[0]);
> +    c->clen = le32_to_cpu(b[1]);
> +    trace_via_ac97_sgd_fetch(c->addr, CLEN_IS_STOP(c) ? 'S' : '-',
> +                             CLEN_IS_EOL(c) ? 'E' : '-',
> +                             CLEN_IS_FLAG(c) ? 'F' : '-', CLEN_LEN(c));
> +}
> +
> +static void out_cb(void *opaque, int avail)
> +{
> +    ViaAC97State *s = opaque;
> +    ViaAC97SGDChannel *c = &s->aur;
> +    int temp, to_copy, copied;
> +    bool stop = false;
> +    uint8_t tmpbuf[4096];
> +
> +    if (c->stat & STAT_PAUSED) {
> +        return;
> +    }
> +    c->stat |= STAT_ACTIVE;
> +    while (avail && !stop) {
> +        if (!c->clen) {
> +            fetch_sgd(c, &s->dev);
> +        }
> +        temp = MIN(CLEN_LEN(c), avail);
> +        while (temp) {
> +            to_copy = MIN(temp, sizeof(tmpbuf));
> +            pci_dma_read(&s->dev, c->addr, tmpbuf, to_copy);
> +            copied = AUD_write(s->vo, tmpbuf, to_copy);
> +            if (!copied) {
> +                stop = true;
> +                break;
> +            }
> +            temp -= copied;
> +            avail -= copied;
> +            c->addr += copied;
> +            c->clen -= copied;
> +        }

It's worth nothing that requiring a loop around AUD_write() and tracking of the avail 
buffer size will no longer be required once Volker's latest audio series is merged 
(see https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg07333.html for more 
information).

> +        if (CLEN_LEN(c) == 0) {
> +            c->curr += 8;
> +            if (CLEN_IS_EOL(c)) {
> +                c->stat |= STAT_EOL;
> +                if (c->type & CNTL_START) {
> +                    c->curr = c->base;
> +                    c->stat |= STAT_PAUSED;
> +                } else {
> +                    c->stat &= ~STAT_ACTIVE;
> +                    AUD_set_active_out(s->vo, 0);
> +                }
> +                if (c->type & STAT_EOL) {
> +                    pci_set_irq(&s->dev, 1);
> +                }
> +            }
> +            if (CLEN_IS_FLAG(c)) {
> +                c->stat |= STAT_FLAG;
> +                c->stat |= STAT_PAUSED;
> +                if (c->type & STAT_FLAG) {
> +                    pci_set_irq(&s->dev, 1);
> +                }
> +            }
> +            if (CLEN_IS_STOP(c)) {
> +                c->stat |= STAT_STOP;
> +                c->stat |= STAT_PAUSED;
> +            }
> +            c->clen = 0;
> +            stop = true;
> +        }
> +    }
> +}
> +
> +static void open_voice_out(ViaAC97State *s)
> +{
> +    struct audsettings as = {
> +        .freq = CODEC_REG(s, AC97_PCM_Front_DAC_Rate),
> +        .nchannels = s->aur.type & BIT(4) ? 2 : 1,
> +        .fmt = s->aur.type & BIT(5) ? AUDIO_FORMAT_S16 : AUDIO_FORMAT_S8,
> +        .endianness = 0,
> +    };
> +    s->vo = AUD_open_out(&s->card, s->vo, "via-ac97.out", s, out_cb, &as);
> +}
> +
> +static uint64_t sgd_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    ViaAC97State *s = opaque;
> +    uint64_t val = 0;
> +
> +    switch (addr) {
> +    case 0:
> +        val = s->aur.stat;
> +        if (s->aur.type & CNTL_START) {
> +            val |= STAT_TRIG;
> +        }
> +        break;
> +    case 1:
> +        val = s->aur.stat & STAT_PAUSED ? BIT(3) : 0;
> +        break;
> +    case 2:
> +        val = s->aur.type;
> +        break;
> +    case 4:
> +        val = s->aur.curr;
> +        break;
> +    case 0xc:
> +        val = CLEN_LEN(&s->aur);
> +        break;
> +    case 0x80:
> +        val = s->ac97_cmd;
> +        break;
> +    case 0x84:
> +        val = s->aur.stat & STAT_FLAG;
> +        if (s->aur.stat & STAT_EOL) {
> +            val |= BIT(4);
> +        }
> +        if (s->aur.stat & STAT_STOP) {
> +            val |= BIT(8);
> +        }
> +        if (s->aur.stat & STAT_ACTIVE) {
> +            val |= BIT(12);
> +        }
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "via-ac97: Unimplemented register read %"
> +                      HWADDR_PRIx"\n", addr);
> +    }
> +    trace_via_ac97_sgd_read(addr, size, val);
> +    return val;
> +}
> +
> +static void sgd_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    ViaAC97State *s = opaque;
> +
> +    trace_via_ac97_sgd_write(addr, size, val);
> +    switch (addr) {
> +    case 0:
> +        if (val & STAT_STOP) {
> +            s->aur.stat &= ~STAT_PAUSED;
> +        }
> +        if (val & STAT_EOL) {
> +            s->aur.stat &= ~(STAT_EOL | STAT_PAUSED);
> +            if (s->aur.type & STAT_EOL) {
> +                pci_set_irq(&s->dev, 0);
> +            }
> +        }
> +        if (val & STAT_FLAG) {
> +            s->aur.stat &= ~(STAT_FLAG | STAT_PAUSED);
> +            if (s->aur.type & STAT_FLAG) {
> +                pci_set_irq(&s->dev, 0);
> +            }
> +        }
> +        break;
> +    case 1:
> +        if (val & CNTL_START) {
> +            AUD_set_active_out(s->vo, 1);
> +            s->aur.stat = STAT_ACTIVE;
> +        }
> +        if (val & CNTL_TERM) {
> +            AUD_set_active_out(s->vo, 0);
> +            s->aur.stat &= ~(STAT_ACTIVE | STAT_PAUSED);
> +            s->aur.clen = 0;
> +        }
> +        if (val & CNTL_PAUSE) {
> +            AUD_set_active_out(s->vo, 0);
> +            s->aur.stat &= ~STAT_ACTIVE;
> +            s->aur.stat |= STAT_PAUSED;
> +        } else if (!(val & CNTL_PAUSE) && (s->aur.stat & STAT_PAUSED)) {
> +            AUD_set_active_out(s->vo, 1);
> +            s->aur.stat |= STAT_ACTIVE;
> +            s->aur.stat &= ~STAT_PAUSED;
> +        }
> +        break;
> +    case 2:
> +    {
> +        uint32_t oldval = s->aur.type;
> +        s->aur.type = val;
> +        if ((oldval & 0x30) != (val & 0x30)) {
> +            open_voice_out(s);
> +        }
> +        break;
> +    }
> +    case 4:
> +        s->aur.base = val & ~1ULL;
> +        break;
> +    case 0x80:
> +        if (val >> 30) {
> +            /* we only have primary codec */
> +            break;
> +        }
> +        if (val & BIT(23)) { /* read reg */
> +            s->ac97_cmd = val & 0xc0ff0000ULL;
> +            s->ac97_cmd |= codec_read(s, (val >> 16) & 0x7f);
> +            s->ac97_cmd |= BIT(25); /* data valid */
> +        } else {
> +            s->ac97_cmd = val & 0xc0ffffffULL;
> +            codec_write(s, (val >> 16) & 0x7f, val);
> +        }
> +        break;
> +    case 0xc:
> +    case 0x84:
> +        /* Read only */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "via-ac97: Unimplemented register write %"
> +                      HWADDR_PRIx"\n", addr);
> +    }
> +}
> +
> +static const MemoryRegionOps sgd_ops = {
> +    .read = sgd_read,
> +    .write = sgd_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t fm_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d\n", __func__, addr, size);
> +    return 0;
> +}
> +
> +static void fm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d <= 0x%"PRIX64"\n",
> +                  __func__, addr, size, val);
> +}
> +
> +static const MemoryRegionOps fm_ops = {
> +    .read = fm_read,
> +    .write = fm_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t midi_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d\n", __func__, addr, size);
> +    return 0;
> +}
> +
> +static void midi_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d <= 0x%"PRIX64"\n",
> +                  __func__, addr, size, val);
> +}
> +
> +static const MemoryRegionOps midi_ops = {
> +    .read = midi_read,
> +    .write = midi_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void via_ac97_reset(DeviceState *dev)
> +{
> +    ViaAC97State *s = VIA_AC97(dev);
> +
> +    codec_reset(s);
> +}
>   
>   static void via_ac97_realize(PCIDevice *pci_dev, Error **errp)
>   {
> +    ViaAC97State *s = VIA_AC97(pci_dev);
> +    Object *o = OBJECT(s);
> +
>       pci_set_word(pci_dev->config + PCI_COMMAND,
>                    PCI_COMMAND_INVALIDATE | PCI_COMMAND_PARITY);
>       pci_set_word(pci_dev->config + PCI_STATUS,
>                    PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_MEDIUM);
>       pci_set_long(pci_dev->config + PCI_INTERRUPT_PIN, 0x03);
> +
> +    memory_region_init_io(&s->sgd, o, &sgd_ops, s, "via-ac97.sgd", 256);
> +    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->sgd);
> +    memory_region_init_io(&s->fm, o, &fm_ops, s, "via-ac97.fm", 4);
> +    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->fm);
> +    memory_region_init_io(&s->midi, o, &midi_ops, s, "via-ac97.midi", 4);
> +    pci_register_bar(pci_dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->midi);
> +
> +    AUD_register_card ("via-ac97", &s->card);
>   }
>   
> +static void via_ac97_exit(PCIDevice *dev)
> +{
> +    ViaAC97State *s = VIA_AC97(dev);
> +
> +    AUD_close_out(&s->card, s->vo);
> +    AUD_remove_card(&s->card);
> +}
> +
> +static Property via_ac97_properties[] = {
> +    DEFINE_AUDIO_PROPERTIES(ViaAC97State, card),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void via_ac97_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>   
>       k->realize = via_ac97_realize;
> +    k->exit = via_ac97_exit;
>       k->vendor_id = PCI_VENDOR_ID_VIA;
>       k->device_id = PCI_DEVICE_ID_VIA_AC97;
>       k->revision = 0x50;
>       k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
> +    device_class_set_props(dc, via_ac97_properties);
>       set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
>       dc->desc = "VIA AC97";
> +    dc->reset = via_ac97_reset;
>       /* Reason: Part of a south bridge chip */
>       dc->user_creatable = false;
>   }
> @@ -41,7 +469,7 @@ static void via_ac97_class_init(ObjectClass *klass, void *data)
>   static const TypeInfo via_ac97_info = {
>       .name          = TYPE_VIA_AC97,
>       .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PCIDevice),
> +    .instance_size = sizeof(ViaAC97State),
>       .class_init    = via_ac97_class_init,
>       .interfaces = (InterfaceInfo[]) {
>           { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index f24e387d63..91780345d4 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -554,7 +554,7 @@ struct ViaISAState {
>       PCIIDEState ide;
>       UHCIState uhci[2];
>       ViaPMState pm;
> -    PCIDevice ac97;
> +    ViaAC97State ac97;
>       PCIDevice mc97;
>   };
>   
> diff --git a/hw/audio/trace-events b/hw/audio/trace-events
> index e0e71cd9b1..6eccdaa4b5 100644
> --- a/hw/audio/trace-events
> +++ b/hw/audio/trace-events
> @@ -11,3 +11,9 @@ hda_audio_running(const char *stream, int nr, bool running) "st %s, nr %d, run %
>   hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s, %d x %s @ %d Hz"
>   hda_audio_adjust(const char *stream, int pos) "st %s, pos %d"
>   hda_audio_overrun(const char *stream) "st %s"
> +
> +#via-ac97.c
> +via_ac97_codec_write(uint8_t addr, uint16_t val) "0x%x <- 0x%x"
> +via_ac97_sgd_fetch(uint32_t addr, char stop, char eol, char flag, uint32_t len) "addr=0x%x %c%c%c len=%d"
> +via_ac97_sgd_read(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" %d -> 0x%"PRIx64
> +via_ac97_sgd_write(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" %d <- 0x%"PRIx64


ATB,

Mark.

Re: [PATCH 5/5] hw/audio/via-ac97: Basic implementation of audio playback

[BALATON Zoltan]

On Wed, 1 Mar 2023, Mark Cave-Ayland wrote:
> On 23/02/2023 20:20, Bernhard Beschow wrote:
>> From: BALATON Zoltan <balaton@eik.bme.hu>
>> 
>> This adds basic implementation of the AC'97 sound part used in VIA
>> south bridge chips. Not all features of the device is emulated, only
>> one playback channel is supported but this is enough to get sound
>> output from some guests running on machines using this device such as
>> pegasos2.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   include/hw/isa/vt82c686.h |  25 +++
>>   hw/audio/via-ac97.c       | 436 +++++++++++++++++++++++++++++++++++++-
>>   hw/isa/vt82c686.c         |   2 +-
>>   hw/audio/trace-events     |   6 +
>>   4 files changed, 464 insertions(+), 5 deletions(-)
>> 
>> diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
>> index e273cd38dc..da1722daf2 100644
>> --- a/include/hw/isa/vt82c686.h
>> +++ b/include/hw/isa/vt82c686.h
>> @@ -1,6 +1,8 @@
>>   #ifndef HW_VT82C686_H
>>   #define HW_VT82C686_H
>>   +#include "hw/pci/pci_device.h"
>> +#include "audio/audio.h"
>>     #define TYPE_VT82C686B_ISA "vt82c686b-isa"
>>   #define TYPE_VT82C686B_USB_UHCI "vt82c686b-usb-uhci"
>> @@ -9,6 +11,29 @@
>>   #define TYPE_VIA_IDE "via-ide"
>>   #define TYPE_VIA_MC97 "via-mc97"
>>   +typedef struct {
>> +    uint8_t stat;
>> +    uint8_t type;
>> +    uint32_t base;
>> +    uint32_t curr;
>> +    uint32_t addr;
>> +    uint32_t clen;
>> +} ViaAC97SGDChannel;
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ViaAC97State, VIA_AC97);
>> +
>> +struct ViaAC97State {
>> +    PCIDevice dev;
>> +    QEMUSoundCard card;
>> +    MemoryRegion sgd;
>> +    MemoryRegion fm;
>> +    MemoryRegion midi;
>> +    SWVoiceOut *vo;
>> +    ViaAC97SGDChannel aur;
>> +    uint16_t codec_regs[128];
>> +    uint32_t ac97_cmd;
>> +};
>> +
>>   void via_isa_set_irq(PCIDevice *d, int n, int level);
>>     #endif
>> diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
>> index d1a856f63d..0b1eeb4272 100644
>> --- a/hw/audio/via-ac97.c
>> +++ b/hw/audio/via-ac97.c
>> @@ -1,39 +1,467 @@
>>   /*
>>    * VIA south bridges sound support
>>    *
>> + * Copyright (c) 2022-2023 BALATON Zoltan
>> + *
>>    * This work is licensed under the GNU GPL license version 2 or later.
>>    */
>>     /*
>> - * TODO: This is entirely boiler plate just registering empty PCI devices
>> - * with the right ID guests expect, functionality should be added here.
>> + * TODO: This is only a basic implementation of one audio playback channel
>> + *       more functionality should be added here.
>>    */
>>     #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>   #include "hw/isa/vt82c686.h"
>> -#include "hw/pci/pci_device.h"
>> +#include "ac97.h"
>> +#include "trace.h"
>> +
>> +#define CLEN_IS_EOL(x)  ((x)->clen & BIT(31))
>> +#define CLEN_IS_FLAG(x) ((x)->clen & BIT(30))
>> +#define CLEN_IS_STOP(x) ((x)->clen & BIT(29))
>> +#define CLEN_LEN(x)     ((x)->clen & 0xfff)
>> +
>> +#define STAT_ACTIVE BIT(7)
>> +#define STAT_PAUSED BIT(6)
>> +#define STAT_TRIG   BIT(3)
>> +#define STAT_STOP   BIT(2)
>> +#define STAT_EOL    BIT(1)
>> +#define STAT_FLAG   BIT(0)
>> +
>> +#define CNTL_START  BIT(7)
>> +#define CNTL_TERM   BIT(6)
>> +#define CNTL_PAUSE  BIT(3)
>> +
>> +static void open_voice_out(ViaAC97State *s);
>> +
>> +static uint16_t codec_rates[] = { 8000, 11025, 16000, 22050, 32000, 44100,
>> +                                  48000 };
>> +
>> +#define CODEC_REG(s, o)  ((s)->codec_regs[(o) / 2])
>> +#define CODEC_VOL(vol, mask)  ((255 * ((vol) & mask)) / mask)
>> +
>> +static void codec_volume_set_out(ViaAC97State *s)
>> +{
>> +    int lvol, rvol, mute;
>> +
>> +    lvol = 255 - CODEC_VOL(CODEC_REG(s, AC97_Master_Volume_Mute) >> 8, 
>> 0x1f);
>> +    lvol *= 255 - CODEC_VOL(CODEC_REG(s, AC97_PCM_Out_Volume_Mute) >> 8, 
>> 0x1f);
>> +    lvol /= 255;
>> +    rvol = 255 - CODEC_VOL(CODEC_REG(s, AC97_Master_Volume_Mute), 0x1f);
>> +    rvol *= 255 - CODEC_VOL(CODEC_REG(s, AC97_PCM_Out_Volume_Mute), 0x1f);
>> +    rvol /= 255;
>> +    mute = CODEC_REG(s, AC97_Master_Volume_Mute) >> MUTE_SHIFT;
>> +    mute |= CODEC_REG(s, AC97_PCM_Out_Volume_Mute) >> MUTE_SHIFT;
>> +    AUD_set_volume_out(s->vo, mute, lvol, rvol);
>> +}
>> +
>> +static void codec_reset(ViaAC97State *s)
>> +{
>> +    memset(s->codec_regs, 0, sizeof(s->codec_regs));
>> +    CODEC_REG(s, AC97_Reset) = 0x6a90;
>> +    CODEC_REG(s, AC97_Master_Volume_Mute) = 0x8000;
>> +    CODEC_REG(s, AC97_Headphone_Volume_Mute) = 0x8000;
>> +    CODEC_REG(s, AC97_Master_Volume_Mono_Mute) = 0x8000;
>> +    CODEC_REG(s, AC97_Phone_Volume_Mute) = 0x8008;
>> +    CODEC_REG(s, AC97_Mic_Volume_Mute) = 0x8008;
>> +    CODEC_REG(s, AC97_Line_In_Volume_Mute) = 0x8808;
>> +    CODEC_REG(s, AC97_CD_Volume_Mute) = 0x8808;
>> +    CODEC_REG(s, AC97_Video_Volume_Mute) = 0x8808;
>> +    CODEC_REG(s, AC97_Aux_Volume_Mute) = 0x8808;
>> +    CODEC_REG(s, AC97_PCM_Out_Volume_Mute) = 0x8808;
>> +    CODEC_REG(s, AC97_Record_Gain_Mute) = 0x8000;
>> +    CODEC_REG(s, AC97_Powerdown_Ctrl_Stat) = 0x000f;
>> +    CODEC_REG(s, AC97_Extended_Audio_ID) = 0x0a05;
>> +    CODEC_REG(s, AC97_Extended_Audio_Ctrl_Stat) = 0x0400;
>> +    CODEC_REG(s, AC97_PCM_Front_DAC_Rate) = 48000;
>> +    CODEC_REG(s, AC97_PCM_LR_ADC_Rate) = 48000;
>> +    /* Sigmatel 9766 (STAC9766) */
>> +    CODEC_REG(s, AC97_Vendor_ID1) = 0x8384;
>> +    CODEC_REG(s, AC97_Vendor_ID2) = 0x7666;
>> +}
>> +
>> +static uint16_t codec_read(ViaAC97State *s, uint8_t addr)
>> +{
>> +    return CODEC_REG(s, addr);
>> +}
>> +
>> +static void codec_write(ViaAC97State *s, uint8_t addr, uint16_t val)
>> +{
>> +    trace_via_ac97_codec_write(addr, val);
>> +    switch (addr) {
>> +    case AC97_Reset:
>> +        codec_reset(s);
>> +        return;
>> +    case AC97_Master_Volume_Mute:
>> +    case AC97_PCM_Out_Volume_Mute:
>> +        if (addr == AC97_Master_Volume_Mute) {
>> +            if (val & BIT(13)) {
>> +                val |= 0x1f00;
>> +            }
>> +            if (val & BIT(5)) {
>> +                val |= 0x1f;
>> +            }
>> +        }
>> +        CODEC_REG(s, addr) = val & 0x9f1f;
>> +        codec_volume_set_out(s);
>> +        return;
>> +    case AC97_Extended_Audio_Ctrl_Stat:
>> +        CODEC_REG(s, addr) &= ~EACS_VRA;
>> +        CODEC_REG(s, addr) |= val & EACS_VRA;
>> +        if (!(val & EACS_VRA)) {
>> +            CODEC_REG(s, AC97_PCM_Front_DAC_Rate) = 48000;
>> +            CODEC_REG(s, AC97_PCM_LR_ADC_Rate) = 48000;
>> +            open_voice_out(s);
>> +        }
>> +        return;
>> +    case AC97_PCM_Front_DAC_Rate:
>> +    case AC97_PCM_LR_ADC_Rate:
>> +        if (CODEC_REG(s, AC97_Extended_Audio_Ctrl_Stat) & EACS_VRA) {
>> +            int i;
>> +            uint16_t rate = val;
>> +
>> +            for (i = 0; i < ARRAY_SIZE(codec_rates) - 1; i++) {
>> +                if (rate < codec_rates[i] +
>> +                    (codec_rates[i + 1] - codec_rates[i]) / 2) {
>> +                    rate = codec_rates[i];
>> +                    break;
>> +                }
>> +            }
>> +            if (rate > 48000) {
>> +                rate = 48000;
>> +            }
>> +            CODEC_REG(s, addr) = rate;
>> +            open_voice_out(s);
>> +        }
>> +        return;
>> +    case AC97_Powerdown_Ctrl_Stat:
>> +        CODEC_REG(s, addr) = (val & 0xff00) | (CODEC_REG(s, addr) & 0xff);
>> +        return;
>> +    case AC97_Extended_Audio_ID:
>> +    case AC97_Vendor_ID1:
>> +    case AC97_Vendor_ID2:
>> +        /* Read only registers */
>> +        return;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      "via-ac97: Unimplemented codec register 0x%x\n", 
>> addr);
>> +        CODEC_REG(s, addr) = val;
>> +    }
>> +}
>> +
>> +static void fetch_sgd(ViaAC97SGDChannel *c, PCIDevice *d)
>> +{
>> +    uint32_t b[2];
>> +
>> +    if (c->curr < c->base) {
>> +        c->curr = c->base;
>> +    }
>> +    pci_dma_read(d, c->curr, b, sizeof(b));
>> +    c->addr = le32_to_cpu(b[0]);
>> +    c->clen = le32_to_cpu(b[1]);
>> +    trace_via_ac97_sgd_fetch(c->addr, CLEN_IS_STOP(c) ? 'S' : '-',
>> +                             CLEN_IS_EOL(c) ? 'E' : '-',
>> +                             CLEN_IS_FLAG(c) ? 'F' : '-', CLEN_LEN(c));
>> +}
>> +
>> +static void out_cb(void *opaque, int avail)
>> +{
>> +    ViaAC97State *s = opaque;
>> +    ViaAC97SGDChannel *c = &s->aur;
>> +    int temp, to_copy, copied;
>> +    bool stop = false;
>> +    uint8_t tmpbuf[4096];
>> +
>> +    if (c->stat & STAT_PAUSED) {
>> +        return;
>> +    }
>> +    c->stat |= STAT_ACTIVE;
>> +    while (avail && !stop) {
>> +        if (!c->clen) {
>> +            fetch_sgd(c, &s->dev);
>> +        }
>> +        temp = MIN(CLEN_LEN(c), avail);
>> +        while (temp) {
>> +            to_copy = MIN(temp, sizeof(tmpbuf));
>> +            pci_dma_read(&s->dev, c->addr, tmpbuf, to_copy);
>> +            copied = AUD_write(s->vo, tmpbuf, to_copy);
>> +            if (!copied) {
>> +                stop = true;
>> +                break;
>> +            }
>> +            temp -= copied;
>> +            avail -= copied;
>> +            c->addr += copied;
>> +            c->clen -= copied;
>> +        }
>
> It's worth nothing that requiring a loop around AUD_write() and tracking of 
> the avail buffer size will no longer be required once Volker's latest audio 
> series is merged (see 
> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg07333.html for more 
> information).

Volker has previously looked at it and said this will be OK after his 
series so I think this loop might still be needed and it he said it may 
miss some samples at the end now (which is likely silence anyway and we 
could not hear any difference so we tested with this version now). But I'd 
let Volker or somebody who knows more about audio part comment on it but 
now this seems to work with all clients we've tried.

Regards,
BALATON Zoltan

>> +        if (CLEN_LEN(c) == 0) {
>> +            c->curr += 8;
>> +            if (CLEN_IS_EOL(c)) {
>> +                c->stat |= STAT_EOL;
>> +                if (c->type & CNTL_START) {
>> +                    c->curr = c->base;
>> +                    c->stat |= STAT_PAUSED;
>> +                } else {
>> +                    c->stat &= ~STAT_ACTIVE;
>> +                    AUD_set_active_out(s->vo, 0);
>> +                }
>> +                if (c->type & STAT_EOL) {
>> +                    pci_set_irq(&s->dev, 1);
>> +                }
>> +            }
>> +            if (CLEN_IS_FLAG(c)) {
>> +                c->stat |= STAT_FLAG;
>> +                c->stat |= STAT_PAUSED;
>> +                if (c->type & STAT_FLAG) {
>> +                    pci_set_irq(&s->dev, 1);
>> +                }
>> +            }
>> +            if (CLEN_IS_STOP(c)) {
>> +                c->stat |= STAT_STOP;
>> +                c->stat |= STAT_PAUSED;
>> +            }
>> +            c->clen = 0;
>> +            stop = true;
>> +        }
>> +    }
>> +}
>> +
>> +static void open_voice_out(ViaAC97State *s)
>> +{
>> +    struct audsettings as = {
>> +        .freq = CODEC_REG(s, AC97_PCM_Front_DAC_Rate),
>> +        .nchannels = s->aur.type & BIT(4) ? 2 : 1,
>> +        .fmt = s->aur.type & BIT(5) ? AUDIO_FORMAT_S16 : AUDIO_FORMAT_S8,
>> +        .endianness = 0,
>> +    };
>> +    s->vo = AUD_open_out(&s->card, s->vo, "via-ac97.out", s, out_cb, &as);
>> +}
>> +
>> +static uint64_t sgd_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    ViaAC97State *s = opaque;
>> +    uint64_t val = 0;
>> +
>> +    switch (addr) {
>> +    case 0:
>> +        val = s->aur.stat;
>> +        if (s->aur.type & CNTL_START) {
>> +            val |= STAT_TRIG;
>> +        }
>> +        break;
>> +    case 1:
>> +        val = s->aur.stat & STAT_PAUSED ? BIT(3) : 0;
>> +        break;
>> +    case 2:
>> +        val = s->aur.type;
>> +        break;
>> +    case 4:
>> +        val = s->aur.curr;
>> +        break;
>> +    case 0xc:
>> +        val = CLEN_LEN(&s->aur);
>> +        break;
>> +    case 0x80:
>> +        val = s->ac97_cmd;
>> +        break;
>> +    case 0x84:
>> +        val = s->aur.stat & STAT_FLAG;
>> +        if (s->aur.stat & STAT_EOL) {
>> +            val |= BIT(4);
>> +        }
>> +        if (s->aur.stat & STAT_STOP) {
>> +            val |= BIT(8);
>> +        }
>> +        if (s->aur.stat & STAT_ACTIVE) {
>> +            val |= BIT(12);
>> +        }
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "via-ac97: Unimplemented register read %"
>> +                      HWADDR_PRIx"\n", addr);
>> +    }
>> +    trace_via_ac97_sgd_read(addr, size, val);
>> +    return val;
>> +}
>> +
>> +static void sgd_write(void *opaque, hwaddr addr, uint64_t val, unsigned 
>> size)
>> +{
>> +    ViaAC97State *s = opaque;
>> +
>> +    trace_via_ac97_sgd_write(addr, size, val);
>> +    switch (addr) {
>> +    case 0:
>> +        if (val & STAT_STOP) {
>> +            s->aur.stat &= ~STAT_PAUSED;
>> +        }
>> +        if (val & STAT_EOL) {
>> +            s->aur.stat &= ~(STAT_EOL | STAT_PAUSED);
>> +            if (s->aur.type & STAT_EOL) {
>> +                pci_set_irq(&s->dev, 0);
>> +            }
>> +        }
>> +        if (val & STAT_FLAG) {
>> +            s->aur.stat &= ~(STAT_FLAG | STAT_PAUSED);
>> +            if (s->aur.type & STAT_FLAG) {
>> +                pci_set_irq(&s->dev, 0);
>> +            }
>> +        }
>> +        break;
>> +    case 1:
>> +        if (val & CNTL_START) {
>> +            AUD_set_active_out(s->vo, 1);
>> +            s->aur.stat = STAT_ACTIVE;
>> +        }
>> +        if (val & CNTL_TERM) {
>> +            AUD_set_active_out(s->vo, 0);
>> +            s->aur.stat &= ~(STAT_ACTIVE | STAT_PAUSED);
>> +            s->aur.clen = 0;
>> +        }
>> +        if (val & CNTL_PAUSE) {
>> +            AUD_set_active_out(s->vo, 0);
>> +            s->aur.stat &= ~STAT_ACTIVE;
>> +            s->aur.stat |= STAT_PAUSED;
>> +        } else if (!(val & CNTL_PAUSE) && (s->aur.stat & STAT_PAUSED)) {
>> +            AUD_set_active_out(s->vo, 1);
>> +            s->aur.stat |= STAT_ACTIVE;
>> +            s->aur.stat &= ~STAT_PAUSED;
>> +        }
>> +        break;
>> +    case 2:
>> +    {
>> +        uint32_t oldval = s->aur.type;
>> +        s->aur.type = val;
>> +        if ((oldval & 0x30) != (val & 0x30)) {
>> +            open_voice_out(s);
>> +        }
>> +        break;
>> +    }
>> +    case 4:
>> +        s->aur.base = val & ~1ULL;
>> +        break;
>> +    case 0x80:
>> +        if (val >> 30) {
>> +            /* we only have primary codec */
>> +            break;
>> +        }
>> +        if (val & BIT(23)) { /* read reg */
>> +            s->ac97_cmd = val & 0xc0ff0000ULL;
>> +            s->ac97_cmd |= codec_read(s, (val >> 16) & 0x7f);
>> +            s->ac97_cmd |= BIT(25); /* data valid */
>> +        } else {
>> +            s->ac97_cmd = val & 0xc0ffffffULL;
>> +            codec_write(s, (val >> 16) & 0x7f, val);
>> +        }
>> +        break;
>> +    case 0xc:
>> +    case 0x84:
>> +        /* Read only */
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "via-ac97: Unimplemented register write 
>> %"
>> +                      HWADDR_PRIx"\n", addr);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps sgd_ops = {
>> +    .read = sgd_read,
>> +    .write = sgd_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static uint64_t fm_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d\n", __func__, addr, 
>> size);
>> +    return 0;
>> +}
>> +
>> +static void fm_write(void *opaque, hwaddr addr, uint64_t val, unsigned 
>> size)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d <= 0x%"PRIX64"\n",
>> +                  __func__, addr, size, val);
>> +}
>> +
>> +static const MemoryRegionOps fm_ops = {
>> +    .read = fm_read,
>> +    .write = fm_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static uint64_t midi_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d\n", __func__, addr, 
>> size);
>> +    return 0;
>> +}
>> +
>> +static void midi_write(void *opaque, hwaddr addr, uint64_t val, unsigned 
>> size)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%"HWADDR_PRIx" %d <= 0x%"PRIX64"\n",
>> +                  __func__, addr, size, val);
>> +}
>> +
>> +static const MemoryRegionOps midi_ops = {
>> +    .read = midi_read,
>> +    .write = midi_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static void via_ac97_reset(DeviceState *dev)
>> +{
>> +    ViaAC97State *s = VIA_AC97(dev);
>> +
>> +    codec_reset(s);
>> +}
>>     static void via_ac97_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>> +    ViaAC97State *s = VIA_AC97(pci_dev);
>> +    Object *o = OBJECT(s);
>> +
>>       pci_set_word(pci_dev->config + PCI_COMMAND,
>>                    PCI_COMMAND_INVALIDATE | PCI_COMMAND_PARITY);
>>       pci_set_word(pci_dev->config + PCI_STATUS,
>>                    PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_MEDIUM);
>>       pci_set_long(pci_dev->config + PCI_INTERRUPT_PIN, 0x03);
>> +
>> +    memory_region_init_io(&s->sgd, o, &sgd_ops, s, "via-ac97.sgd", 256);
>> +    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->sgd);
>> +    memory_region_init_io(&s->fm, o, &fm_ops, s, "via-ac97.fm", 4);
>> +    pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->fm);
>> +    memory_region_init_io(&s->midi, o, &midi_ops, s, "via-ac97.midi", 4);
>> +    pci_register_bar(pci_dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->midi);
>> +
>> +    AUD_register_card ("via-ac97", &s->card);
>>   }
>>   +static void via_ac97_exit(PCIDevice *dev)
>> +{
>> +    ViaAC97State *s = VIA_AC97(dev);
>> +
>> +    AUD_close_out(&s->card, s->vo);
>> +    AUD_remove_card(&s->card);
>> +}
>> +
>> +static Property via_ac97_properties[] = {
>> +    DEFINE_AUDIO_PROPERTIES(ViaAC97State, card),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>   static void via_ac97_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>         k->realize = via_ac97_realize;
>> +    k->exit = via_ac97_exit;
>>       k->vendor_id = PCI_VENDOR_ID_VIA;
>>       k->device_id = PCI_DEVICE_ID_VIA_AC97;
>>       k->revision = 0x50;
>>       k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
>> +    device_class_set_props(dc, via_ac97_properties);
>>       set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
>>       dc->desc = "VIA AC97";
>> +    dc->reset = via_ac97_reset;
>>       /* Reason: Part of a south bridge chip */
>>       dc->user_creatable = false;
>>   }
>> @@ -41,7 +469,7 @@ static void via_ac97_class_init(ObjectClass *klass, void 
>> *data)
>>   static const TypeInfo via_ac97_info = {
>>       .name          = TYPE_VIA_AC97,
>>       .parent        = TYPE_PCI_DEVICE,
>> -    .instance_size = sizeof(PCIDevice),
>> +    .instance_size = sizeof(ViaAC97State),
>>       .class_init    = via_ac97_class_init,
>>       .interfaces = (InterfaceInfo[]) {
>>           { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index f24e387d63..91780345d4 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -554,7 +554,7 @@ struct ViaISAState {
>>       PCIIDEState ide;
>>       UHCIState uhci[2];
>>       ViaPMState pm;
>> -    PCIDevice ac97;
>> +    ViaAC97State ac97;
>>       PCIDevice mc97;
>>   };
>>   diff --git a/hw/audio/trace-events b/hw/audio/trace-events
>> index e0e71cd9b1..6eccdaa4b5 100644
>> --- a/hw/audio/trace-events
>> +++ b/hw/audio/trace-events
>> @@ -11,3 +11,9 @@ hda_audio_running(const char *stream, int nr, bool 
>> running) "st %s, nr %d, run %
>>   hda_audio_format(const char *stream, int chan, const char *fmt, int freq) 
>> "st %s, %d x %s @ %d Hz"
>>   hda_audio_adjust(const char *stream, int pos) "st %s, pos %d"
>>   hda_audio_overrun(const char *stream) "st %s"
>> +
>> +#via-ac97.c
>> +via_ac97_codec_write(uint8_t addr, uint16_t val) "0x%x <- 0x%x"
>> +via_ac97_sgd_fetch(uint32_t addr, char stop, char eol, char flag, uint32_t 
>> len) "addr=0x%x %c%c%c len=%d"
>> +via_ac97_sgd_read(uint64_t addr, unsigned size, uint64_t val) "0x%"PRIx64" 
>> %d -> 0x%"PRIx64
>> +via_ac97_sgd_write(uint64_t addr, unsigned size, uint64_t val) 
>> "0x%"PRIx64" %d <- 0x%"PRIx64
>
>
> ATB,
>
> Mark.
>
>