[PATCH v5 0/7] Pegasos2 fixes and audio output support

[PATCH v5 0/7] Pegasos2 fixes and audio output support

[BALATON Zoltan]

Hello,

This is marked v5 to avoid confusion with previously posted
alternative versions. This series is now based on master and contains
all patches needed to get AmigaOS and MorphOS work on pegasos2 with
sound and I'd like this to be merged for 8.0.

Patch 1 is independent of the rest so could be merged separately but
further patches are needed to fix interrupts which is needed for the
last patc implementing the via-ac97 sound part of the south bridge
chip used on pegasos2 so those patches depend on each other.

Please review and somebody take care of merging this for 8.0 please. I
try to address review comments but it's likely too late to restart
from scratch so keep it reasonable, it could always be improved later
or fixed during the freeze if some issues are found.

Regards,
BALATON Zoltan

BALATON Zoltan (7):
  hw/display/sm501: Add debug property to control pixman usage
  Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"
  hw/isa/vt82c686: Implement PCI IRQ routing
  hw/ppc/pegasos2: Fix PCI interrupt routing
  hw/isa/vt82c686: Work around missing level sensitive irq in i8259
    model
  hw/usb/vt82c686-uhci-pci: Use PCI IRQ routing
  hw/audio/via-ac97: Basic implementation of audio playback

 hw/audio/trace-events      |   6 +
 hw/audio/via-ac97.c        | 455 ++++++++++++++++++++++++++++++++++++-
 hw/display/sm501.c         |  18 +-
 hw/isa/trace-events        |   1 +
 hw/isa/vt82c686.c          |  59 ++++-
 hw/pci-host/mv64361.c      |   4 -
 hw/ppc/pegasos2.c          |  26 ++-
 hw/usb/vt82c686-uhci-pci.c |  12 -
 include/hw/isa/vt82c686.h  |  25 ++
 9 files changed, 577 insertions(+), 29 deletions(-)

-- 
2.30.8

[PATCH v5 1/7] hw/display/sm501: Add debug property to control pixman usage

[BALATON Zoltan]

Add a property to allow disabling pixman and always use the fallbacks
for different operations which is useful for testing different drawing
methods or debugging pixman related issues.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 17835159fc..dbabbc4339 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -465,6 +465,7 @@ typedef struct SM501State {
     uint32_t last_width;
     uint32_t last_height;
     bool do_full_update; /* perform a full update next time */
+    uint8_t use_pixman;
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -827,7 +828,7 @@ static void sm501_2d_operation(SM501State *s)
                 de = db + (width + (height - 1) * dst_pitch) * bypp;
                 overlap = (db < se && sb < de);
             }
-            if (overlap) {
+            if (overlap && (s->use_pixman & BIT(2))) {
                 /* pixman can't do reverse blit: copy via temporary */
                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
                 uint32_t *tmp = tmp_buf;
@@ -852,13 +853,15 @@ static void sm501_2d_operation(SM501State *s)
                 if (tmp != tmp_buf) {
                     g_free(tmp);
                 }
-            } else {
+            } else if (!overlap && (s->use_pixman & BIT(1))) {
                 fallback = !pixman_blt((uint32_t *)&s->local_mem[src_base],
                                        (uint32_t *)&s->local_mem[dst_base],
                                        src_pitch * bypp / sizeof(uint32_t),
                                        dst_pitch * bypp / sizeof(uint32_t),
                                        8 * bypp, 8 * bypp, src_x, src_y,
                                        dst_x, dst_y, width, height);
+            } else {
+                fallback = true;
             }
             if (fallback) {
                 uint8_t *sp = s->local_mem + src_base;
@@ -891,7 +894,7 @@ static void sm501_2d_operation(SM501State *s)
             color = cpu_to_le16(color);
         }
 
-        if ((width == 1 && height == 1) ||
+        if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
                          dst_x, dst_y, width, height, color)) {
@@ -2035,6 +2038,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
 
 static Property sm501_sysbus_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
+    DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2122,6 +2126,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
 
 static Property sm501_pci_properties[] = {
     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
+    DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2162,11 +2167,18 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_sm501_pci;
 }
 
+static void sm501_pci_init(Object *o)
+{
+    object_property_set_description(o, "x-pixman", "Use pixman for: "
+                                    "1: fill, 2: blit, 4: overlap blit");
+}
+
 static const TypeInfo sm501_pci_info = {
     .name          = TYPE_PCI_SM501,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(SM501PCIState),
     .class_init    = sm501_pci_class_init,
+    .instance_init = sm501_pci_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
         { },
-- 
2.30.8

Re: [PATCH v5 1/7] hw/display/sm501: Add debug property to control pixman usage

[BALATON Zoltan]

On Wed, 1 Mar 2023, BALATON Zoltan wrote:
> Add a property to allow disabling pixman and always use the fallbacks
> for different operations which is useful for testing different drawing
> methods or debugging pixman related issues.

Peter,

It was verified that the already merged patches fixed the problems with 
pixman on macOS by adding the fallback so that does not need this property 
but it is still useful for debugging or testing. Are you OK with adding it 
as a debug property as described in this version or still have a concern 
about this?

Regards,
BALATON Zoltan

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 17835159fc..dbabbc4339 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -465,6 +465,7 @@ typedef struct SM501State {
>     uint32_t last_width;
>     uint32_t last_height;
>     bool do_full_update; /* perform a full update next time */
> +    uint8_t use_pixman;
>     I2CBus *i2c_bus;
>
>     /* mmio registers */
> @@ -827,7 +828,7 @@ static void sm501_2d_operation(SM501State *s)
>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>                 overlap = (db < se && sb < de);
>             }
> -            if (overlap) {
> +            if (overlap && (s->use_pixman & BIT(2))) {
>                 /* pixman can't do reverse blit: copy via temporary */
>                 int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t));
>                 uint32_t *tmp = tmp_buf;
> @@ -852,13 +853,15 @@ static void sm501_2d_operation(SM501State *s)
>                 if (tmp != tmp_buf) {
>                     g_free(tmp);
>                 }
> -            } else {
> +            } else if (!overlap && (s->use_pixman & BIT(1))) {
>                 fallback = !pixman_blt((uint32_t *)&s->local_mem[src_base],
>                                        (uint32_t *)&s->local_mem[dst_base],
>                                        src_pitch * bypp / sizeof(uint32_t),
>                                        dst_pitch * bypp / sizeof(uint32_t),
>                                        8 * bypp, 8 * bypp, src_x, src_y,
>                                        dst_x, dst_y, width, height);
> +            } else {
> +                fallback = true;
>             }
>             if (fallback) {
>                 uint8_t *sp = s->local_mem + src_base;
> @@ -891,7 +894,7 @@ static void sm501_2d_operation(SM501State *s)
>             color = cpu_to_le16(color);
>         }
>
> -        if ((width == 1 && height == 1) ||
> +        if (!(s->use_pixman & BIT(0)) || (width == 1 && height == 1) ||
>             !pixman_fill((uint32_t *)&s->local_mem[dst_base],
>                          dst_pitch * bypp / sizeof(uint32_t), 8 * bypp,
>                          dst_x, dst_y, width, height, color)) {
> @@ -2035,6 +2038,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
>
> static Property sm501_sysbus_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> +    DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2122,6 +2126,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp)
>
> static Property sm501_pci_properties[] = {
>     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> +    DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2162,11 +2167,18 @@ static void sm501_pci_class_init(ObjectClass *klass, void *data)
>     dc->vmsd = &vmstate_sm501_pci;
> }
>
> +static void sm501_pci_init(Object *o)
> +{
> +    object_property_set_description(o, "x-pixman", "Use pixman for: "
> +                                    "1: fill, 2: blit, 4: overlap blit");
> +}
> +
> static const TypeInfo sm501_pci_info = {
>     .name          = TYPE_PCI_SM501,
>     .parent        = TYPE_PCI_DEVICE,
>     .instance_size = sizeof(SM501PCIState),
>     .class_init    = sm501_pci_class_init,
> +    .instance_init = sm501_pci_init,
>     .interfaces = (InterfaceInfo[]) {
>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>         { },
>

[PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[BALATON Zoltan]

This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
keeping the rename of a state field but reverting other cahanges which
break interrupts on pegasos2.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f4c40965cd..01e0148967 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -598,15 +598,23 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
     qemu_set_irq(s->isa_irqs_in[n], level);
 }
 
+static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
+{
+    ViaISAState *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
 static void via_isa_realize(PCIDevice *d, Error **errp)
 {
     ViaISAState *s = VIA_ISA(d);
     DeviceState *dev = DEVICE(d);
     PCIBus *pci_bus = pci_get_bus(d);
+    qemu_irq *isa_irq;
     ISABus *isa_bus;
     int i;
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
+    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
 
@@ -614,7 +622,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
         return;
     }
 
-    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
+    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
-- 
2.30.8

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[BALATON Zoltan]

On Wed, 1 Mar 2023, BALATON Zoltan wrote:
> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
> keeping the rename of a state field but reverting other cahanges which
> break interrupts on pegasos2.

I've found this with just booting the MorphOS iso which now hangs without 
this revert when trying to read from the ide device. I think I've 
mentioned that I've also tried this way first but then ended up adding 
this because it was needed in a review of the patch earlier but I can't 
find that message now. For now it seems the easiest is to revert this and 
think about it later.

Regards,
BALATON Zoltan

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/isa/vt82c686.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index f4c40965cd..01e0148967 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -598,15 +598,23 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>     qemu_set_irq(s->isa_irqs_in[n], level);
> }
>
> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> +{
> +    ViaISAState *s = opaque;
> +    qemu_set_irq(s->cpu_intr, level);
> +}
> +
> static void via_isa_realize(PCIDevice *d, Error **errp)
> {
>     ViaISAState *s = VIA_ISA(d);
>     DeviceState *dev = DEVICE(d);
>     PCIBus *pci_bus = pci_get_bus(d);
> +    qemu_irq *isa_irq;
>     ISABus *isa_bus;
>     int i;
>
>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                           errp);
>
> @@ -614,7 +622,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>         return;
>     }
>
> -    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
> +    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
>     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     i8257_dma_init(isa_bus, 0);
>

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[Bernhard Beschow]

Am 1. März 2023 00:33:28 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 1 Mar 2023, BALATON Zoltan wrote:
>> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
>> keeping the rename of a state field but reverting other cahanges which
>> break interrupts on pegasos2.
>
>I've found this with just booting the MorphOS iso which now hangs without this revert when trying to read from the ide device. I think I've mentioned that I've also tried this way first but then ended up adding this because it was needed in a review of the patch earlier but I can't find that message now. For now it seems the easiest is to revert this and think about it later.

It looks like Philippe's patch should work, at least in theory. Why does the indirection work while it doesn't without?

>
>Regards,
>BALATON Zoltan
>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/isa/vt82c686.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index f4c40965cd..01e0148967 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -598,15 +598,23 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>>     qemu_set_irq(s->isa_irqs_in[n], level);
>> }
>> 
>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>> +{
>> +    ViaISAState *s = opaque;
>> +    qemu_set_irq(s->cpu_intr, level);
>> +}
>> +
>> static void via_isa_realize(PCIDevice *d, Error **errp)
>> {
>>     ViaISAState *s = VIA_ISA(d);
>>     DeviceState *dev = DEVICE(d);
>>     PCIBus *pci_bus = pci_get_bus(d);
>> +    qemu_irq *isa_irq;
>>     ISABus *isa_bus;
>>     int i;
>> 
>>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>                           errp);
>> 
>> @@ -614,7 +622,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>         return;
>>     }
>> 
>> -    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
>> +    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
>>     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>     i8257_dma_init(isa_bus, 0);
>> 

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]

On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> Am 1. März 2023 00:33:28 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Wed, 1 Mar 2023, BALATON Zoltan wrote:
>>> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
>>> keeping the rename of a state field but reverting other cahanges which
>>> break interrupts on pegasos2.
>>
>> I've found this with just booting the MorphOS iso which now hangs without this revert when trying to read from the ide device. I think I've mentioned that I've also tried this way first but then ended up adding this because it was needed in a review of the patch earlier but I can't find that message now. For now it seems the easiest is to revert this and think about it later.
>
> It looks like Philippe's patch should work, at least in theory. Why does the indirection work while it doesn't without?

I have eno idea. I've found this before because I've also thought it 
should work in the simpler way but found it doesn't, that's why I've added 
the indirection which I saw was the way other similar devices also did it. 
We're now too close to the freeze to have another try so just chose to 
revert it for now then we can find out later what's going on.

Regards,
BALATON Zoltan
>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/isa/vt82c686.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index f4c40965cd..01e0148967 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -598,15 +598,23 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>>>     qemu_set_irq(s->isa_irqs_in[n], level);
>>> }
>>>
>>> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
>>> +{
>>> +    ViaISAState *s = opaque;
>>> +    qemu_set_irq(s->cpu_intr, level);
>>> +}
>>> +
>>> static void via_isa_realize(PCIDevice *d, Error **errp)
>>> {
>>>     ViaISAState *s = VIA_ISA(d);
>>>     DeviceState *dev = DEVICE(d);
>>>     PCIBus *pci_bus = pci_get_bus(d);
>>> +    qemu_irq *isa_irq;
>>>     ISABus *isa_bus;
>>>     int i;
>>>
>>>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>                           errp);
>>>
>>> @@ -614,7 +622,7 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>>         return;
>>>     }
>>>
>>> -    s->isa_irqs_in = i8259_init(isa_bus, s->cpu_intr);
>>> +    s->isa_irqs_in = i8259_init(isa_bus, *isa_irq);
>>>     isa_bus_register_input_irqs(isa_bus, s->isa_irqs_in);
>>>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>     i8257_dma_init(isa_bus, 0);
>>>
>
>

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[Philippe Mathieu-Daudé]

On 1/3/23 01:33, BALATON Zoltan wrote:
> On Wed, 1 Mar 2023, BALATON Zoltan wrote:
>> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
>> keeping the rename of a state field but reverting other cahanges which
>> break interrupts on pegasos2.
> 
> I've found this with just booting the MorphOS iso which now hangs 
> without this revert when trying to read from the ide device.

Can you add an Avocado test booting the MorphOS iso?

> I think 
> I've mentioned that I've also tried this way first but then ended up 
> adding this because it was needed in a review of the patch earlier but I 
> can't find that message now. For now it seems the easiest is to revert 
> this and think about it later.
> 
> Regards,
> BALATON Zoltan
> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/isa/vt82c686.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[Philippe Mathieu-Daudé]

On 2/3/23 11:41, Philippe Mathieu-Daudé wrote:
> On 1/3/23 01:33, BALATON Zoltan wrote:
>> On Wed, 1 Mar 2023, BALATON Zoltan wrote:
>>> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
>>> keeping the rename of a state field but reverting other cahanges which
>>> break interrupts on pegasos2.
>>
>> I've found this with just booting the MorphOS iso which now hangs 
>> without this revert when trying to read from the ide device.
> 
> Can you add an Avocado test booting the MorphOS iso?

Arf I search on the list and someone already did that ;)
https://lore.kernel.org/qemu-devel/20210515134555.307404-3-f4bug@amsat.org/

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

On Thu, 2 Mar 2023, Philippe Mathieu-Daudé wrote:
> On 1/3/23 01:33, BALATON Zoltan wrote:
>> On Wed, 1 Mar 2023, BALATON Zoltan wrote:
>>> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
>>> keeping the rename of a state field but reverting other cahanges which
>>> break interrupts on pegasos2.
>> 
>> I've found this with just booting the MorphOS iso which now hangs without 
>> this revert when trying to read from the ide device.
>
> Can you add an Avocado test booting the MorphOS iso?

I think you had a patch for that before, so you remember where to find it? 
I can't test that though as I don't have avocado and it does not come with 
my distro. So maybe if you can revive your patch and test it that might 
work better.

Regards,
BALATON Zoltan

>> I think I've mentioned that I've also tried this way first but then ended 
>> up adding this because it was needed in a review of the patch earlier but I 
>> can't find that message now. For now it seems the easiest is to revert this 
>> and think about it later.
>> 
>> Regards,
>> BALATON Zoltan
>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/isa/vt82c686.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>
>

Re: [PATCH v5 2/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[Philippe Mathieu-Daudé]

On 2/3/23 13:37, BALATON Zoltan wrote:
> On Thu, 2 Mar 2023, Philippe Mathieu-Daudé wrote:
>> On 1/3/23 01:33, BALATON Zoltan wrote:
>>> On Wed, 1 Mar 2023, BALATON Zoltan wrote:
>>>> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
>>>> keeping the rename of a state field but reverting other cahanges which
>>>> break interrupts on pegasos2.
>>>
>>> I've found this with just booting the MorphOS iso which now hangs 
>>> without this revert when trying to read from the ide device.
>>
>> Can you add an Avocado test booting the MorphOS iso?
> 
> I think you had a patch for that before, so you remember where to find 
> it? I can't test that though as I don't have avocado and it does not 
> come with my distro. So maybe if you can revive your patch and test it 
> that might work better.

It doesn't work on Darwin/macOS neither sigh :( A few users also
reported / complained about this there.

Waiting for this since 15months or more now...
https://github.com/avocado-framework/avocado/issues/5138
https://github.com/avocado-framework/avocado/issues/4888

Personally I ended running it on a remote Linux, this is unfortunate
but ¯\_(ツ)_/¯

[PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

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. The real chip may allow routing IRQs from
internal functions independently of PCI interrupts but since guests
usually configute it to a single shared interrupt we don't model that
here for simplicity.

Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.

Suggested-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 01e0148967..018a119964 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
+
+    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
+        return;
+    }
+
+    /* 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);
+        }
+    }
+    /* Now we change the pic irq level according to the via irq mappings. */
+    qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
+}
+
 static void via_isa_realize(PCIDevice *d, Error **errp)
 {
     ViaISAState *s = VIA_ISA(d);
@@ -615,9 +651,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
 
     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
+    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
                           errp);
-
     if (!isa_bus) {
         return;
     }
-- 
2.30.8

Re: [PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow]

Am 1. März 2023 00:17:09 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>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. The real chip may allow routing IRQs from
>internal functions independently of PCI interrupts but since guests
>usually configute it to a single shared interrupt we don't model that
>here for simplicity.
>
>Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>
>Suggested-by: Bernhard Beschow <shentey@gmail.com>
>Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>---
> hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
>diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>index 01e0148967..018a119964 100644
>--- a/hw/isa/vt82c686.c
>+++ b/hw/isa/vt82c686.c
>@@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
>+
>+    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {

Where does the "pic_irq > 14" come from? It's not mentioned in the datasheet.

>+        return;
>+    }
>+
>+    /* 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);
>+        }
>+    }
>+    /* Now we change the pic irq level according to the via irq mappings. */
>+    qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
>+}
>+
> static void via_isa_realize(PCIDevice *d, Error **errp)
> {
>     ViaISAState *s = VIA_ISA(d);
>@@ -615,9 +651,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
> 
>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>+    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>                           errp);
>-
>     if (!isa_bus) {
>         return;
>     }

Re: [PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> Am 1. März 2023 00:17:09 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> 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. The real chip may allow routing IRQs from
>> internal functions independently of PCI interrupts but since guests
>> usually configute it to a single shared interrupt we don't model that
>> here for simplicity.
>>
>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>>
>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 01e0148967..018a119964 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
>> +
>> +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
>
> Where does the "pic_irq > 14" come from? It's not mentioned in the datasheet.

Check at 0x3c register of USB and AC97 functions. For the others it may be 
valid but unlikely to be used hence we just disallow it. (In my version 
which also mapped IDE here I've checkrf for each source but there's no way 
to do that in this version.)

Regards,
BALATON Zoltan

>> +        return;
>> +    }
>> +
>> +    /* 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);
>> +        }
>> +    }
>> +    /* Now we change the pic irq level according to the via irq mappings. */
>> +    qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
>> +}
>> +
>> static void via_isa_realize(PCIDevice *d, Error **errp)
>> {
>>     ViaISAState *s = VIA_ISA(d);
>> @@ -615,9 +651,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>
>>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>> +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
>>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>                           errp);
>> -
>>     if (!isa_bus) {
>>         return;
>>     }
>
>

Re: [PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow]

Am 1. März 2023 11:15:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 1 Mar 2023, Bernhard Beschow wrote:
>> Am 1. März 2023 00:17:09 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> 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. The real chip may allow routing IRQs from
>>> internal functions independently of PCI interrupts but since guests
>>> usually configute it to a single shared interrupt we don't model that
>>> here for simplicity.
>>> 
>>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>>> 
>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 01e0148967..018a119964 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
>>> +
>>> +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
>> 
>> Where does the "pic_irq > 14" come from? It's not mentioned in the datasheet.
>
>Check at 0x3c register of USB and AC97 functions. For the others it may be valid but unlikely to be used hence we just disallow it. (In my version which also mapped IDE here I've checkrf for each source but there's no way to do that in this version.)

I'm not sure what you mean. The 0x3c regs aren't related to the PCI IRQ routing regs.

Moreover, as I wrote in my other mail, I wonder what the datasheet tries to tell us here at all. The information there partly contradicts itself.

Can you please clarify?

Thanks,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>>> +        return;
>>> +    }
>>> +
>>> +    /* 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);
>>> +        }
>>> +    }
>>> +    /* Now we change the pic irq level according to the via irq mappings. */
>>> +    qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
>>> +}
>>> +
>>> static void via_isa_realize(PCIDevice *d, Error **errp)
>>> {
>>>     ViaISAState *s = VIA_ISA(d);
>>> @@ -615,9 +651,9 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>> 
>>>     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
>>>     isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>>> +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
>>>     isa_bus = isa_bus_new(dev, pci_address_space(d), pci_address_space_io(d),
>>>                           errp);
>>> -
>>>     if (!isa_bus) {
>>>         return;
>>>     }
>> 
>>

Re: [PATCH v5 3/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4311 bytes --]

On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> Am 1. März 2023 11:15:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Wed, 1 Mar 2023, Bernhard Beschow wrote:
>>> Am 1. März 2023 00:17:09 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> 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. The real chip may allow routing IRQs from
>>>> internal functions independently of PCI interrupts but since guests
>>>> usually configute it to a single shared interrupt we don't model that
>>>> here for simplicity.
>>>>
>>>> Note: The implementation was taken from piix4_set_irq() in hw/isa/piix4.
>>>>
>>>> Suggested-by: Bernhard Beschow <shentey@gmail.com>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/isa/vt82c686.c | 38 +++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 37 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 01e0148967..018a119964 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -604,6 +604,42 @@ 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 i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
>>>> +
>>>> +    if (unlikely(pic_irq == 0 || pic_irq == 2 || pic_irq > 14)) {
>>>
>>> Where does the "pic_irq > 14" come from? It's not mentioned in the datasheet.
>>
>> Check at 0x3c register of USB and AC97 functions. For the others it may be valid but unlikely to be used hence we just disallow it. (In my version which also mapped IDE here I've checkrf for each source but there's no way to do that in this version.)
>
> I'm not sure what you mean. The 0x3c regs aren't related to the PCI IRQ routing regs.
>
> Moreover, as I wrote in my other mail, I wonder what the datasheet tries to tell us here at all. The information there partly contradicts itself.
>
> Can you please clarify?

Here is the entire register desription that you've partly quoted before:

Offset 3C - Interrupt Line (00h)...................................... RW
7-4 Reserved ........................................always reads 0
3-0 USB Interrupt Routing ........................ default = 16h
0000 Disabled................................................. default
0001 IRQ1
0010 Reserved
0011 IRQ3
0100 IRQ4
0101 IRQ5
0110 IRQ6
0111 IRQ7
1000 IRQ8
1001 IRQ9
1010 IRQ10
1011 IRQ11
1100 IRQ12
1101 IRQ13
1110 IRQ14
1111 Disabled

Apart from the obvious typo stating default 16h the list below clearly 
says that the default is really 0 and 0 and 15 means Disabled (so if this 
is a copy paste error and the default should be 15 that would still mean 
it's disabled by default) and could be routed to any other ISA IRQ but 
you really should not route it to 2 as that would mess up the cascade IRQ. 
That's how I read that.

And yes I was trying to tell you rhat this is not related to the PCI IRQ 
routing regs which only set the IRQ for the PIRQ pins ahd this one sets 
the IRQ for the function it belongs to (USB, AC97, etc.) independently of 
that. Your patch which is now in the series does not implement this but 
uses pci interrupts instead and still works because guests don't seem to 
actually route IRQs to different interrupts just put everything on IRQ9 so 
your patch still works. As this makes QEMU model simpler we can do that 
and later if we ever need to model this for a guest that actually wants to 
use this feature of the chip you'll have my v1 series in the list archives 
where I've tried to implement the above. For me we can end it here.

Regards,
BALATON Zoltan

[PATCH v5 4/7] hw/ppc/pegasos2: Fix PCI interrupt routing

[BALATON Zoltan]

According to the PegasosII schematics the PCI interrupt lines are
connected to both the gpp pins of the Mv64361 north bridge and the
PINT pins of the VT8231 south bridge so guests can get interrupts from
either of these. So far we only had the MV64361 connections which
worked for on board devices but for additional PCI devices (such as
network or sound card added with -device) guest OSes expect interrupt
from the ISA IRQ 9 where the firmware routes these PCI interrupts in
VT8231 ISA bridge. After the previous patches we can now model this
and also remove the board specific connection from mv64361. Also
configure routing of these lines when using Virtual Open Firmware to
match board firmware for guests that expect this.

This fixes PCI interrupts on pegasos2 under Linux, MorphOS and AmigaOS.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/pci-host/mv64361.c |  4 ----
 hw/ppc/pegasos2.c     | 26 +++++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c
index 298564f1f5..19e8031a3f 100644
--- a/hw/pci-host/mv64361.c
+++ b/hw/pci-host/mv64361.c
@@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error **errp)
     }
     sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq);
     qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32);
-    /* FIXME: PCI IRQ connections may be board specific */
-    for (i = 0; i < PCI_NUM_PINS; i++) {
-        s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i);
-    }
 }
 
 static void mv64361_reset(DeviceState *dev)
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 7cc375df05..f1650be5ee 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -73,6 +73,8 @@ struct Pegasos2MachineState {
     MachineState parent_obj;
     PowerPCCPU *cpu;
     DeviceState *mv;
+    qemu_irq mv_pirq[PCI_NUM_PINS];
+    qemu_irq via_pirq[PCI_NUM_PINS];
     Vof *vof;
     void *fdt_blob;
     uint64_t kernel_addr;
@@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque)
     }
 }
 
+static void pegasos2_pci_irq(void *opaque, int n, int level)
+{
+    Pegasos2MachineState *pm = opaque;
+
+    /* PCI interrupt lines are connected to both MV64361 and VT8231 */
+    qemu_set_irq(pm->mv_pirq[n], level);
+    qemu_set_irq(pm->via_pirq[n], level);
+}
+
 static void pegasos2_init(MachineState *machine)
 {
     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
@@ -106,7 +117,7 @@ static void pegasos2_init(MachineState *machine)
     I2CBus *i2c_bus;
     const char *fwname = machine->firmware ?: PROM_FILENAME;
     char *filename;
-    int sz;
+    int i, sz;
     uint8_t *spd_data;
 
     /* init CPU */
@@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine)
     /* Marvell Discovery II system controller */
     pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1,
                           qdev_get_gpio_in(DEVICE(pm->cpu), PPC6xx_INPUT_INT)));
+    for (i = 0; i < PCI_NUM_PINS; i++) {
+        pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i);
+    }
     pci_bus = mv64361_get_pci_bus(pm->mv, 1);
+    pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS);
 
     /* VIA VT8231 South Bridge (multifunction PCI device) */
     via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
                                                  true, TYPE_VT8231_ISA));
+    for (i = 0; i < PCI_NUM_PINS; i++) {
+        pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i);
+    }
     object_property_add_alias(OBJECT(machine), "rtc-time",
                               object_resolve_path_component(via, "rtc"),
                               "date");
@@ -267,6 +285,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.30.8

Re: [PATCH v5 4/7] hw/ppc/pegasos2: Fix PCI interrupt routing

[Daniel Henrique Barboza]

Hi,

It seems that I can't pick this patch standalone. It will break pegasos2 boot
without patches 2 and 3 that comes beforehand.

Phil, this patch already has my ack, so feel free to slide it together with
patches 2 and 3 in your PR.


Thanks,

Daniel

On 2/28/23 21:17, BALATON Zoltan wrote:
> According to the PegasosII schematics the PCI interrupt lines are
> connected to both the gpp pins of the Mv64361 north bridge and the
> PINT pins of the VT8231 south bridge so guests can get interrupts from
> either of these. So far we only had the MV64361 connections which
> worked for on board devices but for additional PCI devices (such as
> network or sound card added with -device) guest OSes expect interrupt
> from the ISA IRQ 9 where the firmware routes these PCI interrupts in
> VT8231 ISA bridge. After the previous patches we can now model this
> and also remove the board specific connection from mv64361. Also
> configure routing of these lines when using Virtual Open Firmware to
> match board firmware for guests that expect this.
> 
> This fixes PCI interrupts on pegasos2 under Linux, MorphOS and AmigaOS.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   hw/pci-host/mv64361.c |  4 ----
>   hw/ppc/pegasos2.c     | 26 +++++++++++++++++++++++++-
>   2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-host/mv64361.c b/hw/pci-host/mv64361.c
> index 298564f1f5..19e8031a3f 100644
> --- a/hw/pci-host/mv64361.c
> +++ b/hw/pci-host/mv64361.c
> @@ -873,10 +873,6 @@ static void mv64361_realize(DeviceState *dev, Error **errp)
>       }
>       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq);
>       qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32);
> -    /* FIXME: PCI IRQ connections may be board specific */
> -    for (i = 0; i < PCI_NUM_PINS; i++) {
> -        s->pci[1].irq[i] = qdev_get_gpio_in_named(dev, "gpp", 12 + i);
> -    }
>   }
>   
>   static void mv64361_reset(DeviceState *dev)
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 7cc375df05..f1650be5ee 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -73,6 +73,8 @@ struct Pegasos2MachineState {
>       MachineState parent_obj;
>       PowerPCCPU *cpu;
>       DeviceState *mv;
> +    qemu_irq mv_pirq[PCI_NUM_PINS];
> +    qemu_irq via_pirq[PCI_NUM_PINS];
>       Vof *vof;
>       void *fdt_blob;
>       uint64_t kernel_addr;
> @@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque)
>       }
>   }
>   
> +static void pegasos2_pci_irq(void *opaque, int n, int level)
> +{
> +    Pegasos2MachineState *pm = opaque;
> +
> +    /* PCI interrupt lines are connected to both MV64361 and VT8231 */
> +    qemu_set_irq(pm->mv_pirq[n], level);
> +    qemu_set_irq(pm->via_pirq[n], level);
> +}
> +
>   static void pegasos2_init(MachineState *machine)
>   {
>       Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
> @@ -106,7 +117,7 @@ static void pegasos2_init(MachineState *machine)
>       I2CBus *i2c_bus;
>       const char *fwname = machine->firmware ?: PROM_FILENAME;
>       char *filename;
> -    int sz;
> +    int i, sz;
>       uint8_t *spd_data;
>   
>       /* init CPU */
> @@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine)
>       /* Marvell Discovery II system controller */
>       pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1,
>                             qdev_get_gpio_in(DEVICE(pm->cpu), PPC6xx_INPUT_INT)));
> +    for (i = 0; i < PCI_NUM_PINS; i++) {
> +        pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i);
> +    }
>       pci_bus = mv64361_get_pci_bus(pm->mv, 1);
> +    pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS);
>   
>       /* VIA VT8231 South Bridge (multifunction PCI device) */
>       via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
>                                                    true, TYPE_VT8231_ISA));
> +    for (i = 0; i < PCI_NUM_PINS; i++) {
> +        pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i);
> +    }
>       object_property_add_alias(OBJECT(machine), "rtc-time",
>                                 object_resolve_path_component(via, "rtc"),
>                                 "date");
> @@ -267,6 +285,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);

[PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

MorphOS sets the ISA PIC to level sensitive mode but QEMU does not
support that so this causes a freeze if multiple devices try to raise
a shared interrupt. Work around it by lowering the interrupt before
raising it again if it is already raised. This could be reverted when
the i8259 model is fixed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 018a119964..3e44a51f92 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -549,6 +549,7 @@ struct ViaISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs_in;
+    uint16_t isa_irqs_state;
     ViaSuperIOState via_sio;
     MC146818RtcState rtc;
     PCIIDEState ide;
@@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
             pic_level |= pci_bus_get_irq_level(bus, i);
         }
     }
+    /* FIXME: workaround for i8259: level sensitive irq not supported */
+    if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) {
+        qemu_irq_lower(s->isa_irqs_in[pic_irq]);
+    } else if (pic_level) {
+        s->isa_irqs_state |= BIT(pic_irq);
+    } else {
+        s->isa_irqs_state &= ~BIT(pic_irq);
+    }
     /* Now we change the pic irq level according to the via irq mappings. */
     qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
 }
-- 
2.30.8

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[Bernhard Beschow]

Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>MorphOS sets the ISA PIC to level sensitive mode but QEMU does not
>support that so this causes a freeze if multiple devices try to raise
>a shared interrupt. Work around it by lowering the interrupt before
>raising it again if it is already raised. This could be reverted when
>the i8259 model is fixed.
>
>Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>---
> hw/isa/vt82c686.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>index 018a119964..3e44a51f92 100644
>--- a/hw/isa/vt82c686.c
>+++ b/hw/isa/vt82c686.c
>@@ -549,6 +549,7 @@ struct ViaISAState {
>     PCIDevice dev;
>     qemu_irq cpu_intr;
>     qemu_irq *isa_irqs_in;
>+    uint16_t isa_irqs_state;
>     ViaSuperIOState via_sio;
>     MC146818RtcState rtc;
>     PCIIDEState ide;
>@@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
>             pic_level |= pci_bus_get_irq_level(bus, i);
>         }
>     }
>+    /* FIXME: workaround for i8259: level sensitive irq not supported */
>+    if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) {
>+        qemu_irq_lower(s->isa_irqs_in[pic_irq]);
>+    } else if (pic_level) {
>+        s->isa_irqs_state |= BIT(pic_irq);
>+    } else {
>+        s->isa_irqs_state &= ~BIT(pic_irq);
>+    }

Let's not clutter the device model with workarounds which quickly snowball into unmaintainable code. Please fix the i8259 instead.

>     /* Now we change the pic irq level according to the via irq mappings. */
>     qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
> }

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2736 bytes --]

On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> MorphOS sets the ISA PIC to level sensitive mode but QEMU does not
>> support that so this causes a freeze if multiple devices try to raise
>> a shared interrupt. Work around it by lowering the interrupt before
>> raising it again if it is already raised. This could be reverted when
>> the i8259 model is fixed.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> hw/isa/vt82c686.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 018a119964..3e44a51f92 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -549,6 +549,7 @@ struct ViaISAState {
>>     PCIDevice dev;
>>     qemu_irq cpu_intr;
>>     qemu_irq *isa_irqs_in;
>> +    uint16_t isa_irqs_state;
>>     ViaSuperIOState via_sio;
>>     MC146818RtcState rtc;
>>     PCIIDEState ide;
>> @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
>>             pic_level |= pci_bus_get_irq_level(bus, i);
>>         }
>>     }
>> +    /* FIXME: workaround for i8259: level sensitive irq not supported */
>> +    if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) {
>> +        qemu_irq_lower(s->isa_irqs_in[pic_irq]);
>> +    } else if (pic_level) {
>> +        s->isa_irqs_state |= BIT(pic_irq);
>> +    } else {
>> +        s->isa_irqs_state &= ~BIT(pic_irq);
>> +    }
>
> Let's not clutter the device model with workarounds which quickly snowball into unmaintainable code. Please fix the i8259 instead.

Do you have an idea how? I don't know PC hardware well so it's not likely 
I can do that in one day and breaking PIC model would affect a lot of 
machines among them some production critical ones. Adding this workaround 
here only affects pegasos2 and the only partially modeled likely not used 
fuloong2e board (which is mostly there because it was there before and 
good to keep it to be able to test this device model with another machine) 
and I can test these two but not all the others using i8259. So I think 
for this release this is the best we can do and if somebody more 
knowledgeable about PC hardware fixes the i8259 PIC model later this can 
be easily reverted. It's not a big clutter so unless you can show this 
breaks something I'd rather have it to keep MorphOS usable on pegasos2 
with sound and network or USB. If you can prove this breaks something we 
could drop it but not based on opinion or feelengs only some evidence.

Regards,
BALATON Zoltan

>>     /* Now we change the pic irq level according to the via irq mappings. */
>>     qemu_set_irq(s->isa_irqs_in[pic_irq], pic_level);
>> }
>
>

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3191 bytes --]

On Wed, 2023-03-01 at 12:27 +0100, BALATON Zoltan wrote:
> On Wed, 1 Mar 2023, Bernhard Beschow wrote:
> > Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > > MorphOS sets the ISA PIC to level sensitive mode but QEMU does
> > > not
> > > support that so this causes a freeze if multiple devices try to
> > > raise
> > > a shared interrupt. Work around it by lowering the interrupt
> > > before
> > > raising it again if it is already raised. This could be reverted
> > > when
> > > the i8259 model is fixed.
> > > 
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > > hw/isa/vt82c686.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > index 018a119964..3e44a51f92 100644
> > > --- a/hw/isa/vt82c686.c
> > > +++ b/hw/isa/vt82c686.c
> > > @@ -549,6 +549,7 @@ struct ViaISAState {
> > >      PCIDevice dev;
> > >      qemu_irq cpu_intr;
> > >      qemu_irq *isa_irqs_in;
> > > +    uint16_t isa_irqs_state;
> > >      ViaSuperIOState via_sio;
> > >      MC146818RtcState rtc;
> > >      PCIIDEState ide;
> > > @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void
> > > *opaque, int irq_num, int level)
> > >              pic_level |= pci_bus_get_irq_level(bus, i);
> > >          }
> > >      }
> > > +    /* FIXME: workaround for i8259: level sensitive irq not
> > > supported */
> > > +    if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) {
> > > +        qemu_irq_lower(s->isa_irqs_in[pic_irq]);
> > > +    } else if (pic_level) {
> > > +        s->isa_irqs_state |= BIT(pic_irq);
> > > +    } else {
> > > +        s->isa_irqs_state &= ~BIT(pic_irq);
> > > +    }
> > 
> > Let's not clutter the device model with workarounds which quickly
> > snowball into unmaintainable code. Please fix the i8259 instead.
> 
> Do you have an idea how? 

Let's start by understanding the problem completely. The i8259 *does*
support level-triggered interrupts. Look at the checks for s->elcr in
hw/intc/i8259.c, in pic_set_irq()...

    if (s->elcr & mask) {
        /* level triggered */
        if (level) {
            s->irr |= mask;
            s->last_irr |= mask;
        } else {
            s->irr &= ~mask;
            s->last_irr &= ~mask;
        }
    } else {
        /* edge triggered */


 ... and in pic_intack() ...


    /* We don't clear a level sensitive interrupt here */
    if (!(s->elcr & (1 << irq))) {
        s->irr &= ~(1 << irq);
    }


What qemu typically has an issue with is *shared* level-triggered
interrupts. But that would cause a level to be "forgotten" if another
input asserts and then deasserts the IRQ while one input thinks it's
holding it asserted. And I don't see how your workaround above would
have helped in that situation.

Are you sure the PIC ELCR is actually set for the lines you're having
trouble with? Is that something the Pegasos SmartFirmware would have
done, and MorphOS is expecting to inherit but isn't actually setting up
for itself?



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3855 bytes --]

On Wed, 1 Mar 2023, David Woodhouse wrote:
> On Wed, 2023-03-01 at 12:27 +0100, BALATON Zoltan wrote:
>> On Wed, 1 Mar 2023, Bernhard Beschow wrote:
>>> Am 1. März 2023 00:17:11 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> MorphOS sets the ISA PIC to level sensitive mode but QEMU does
>>>> not
>>>> support that so this causes a freeze if multiple devices try to
>>>> raise
>>>> a shared interrupt. Work around it by lowering the interrupt
>>>> before
>>>> raising it again if it is already raised. This could be reverted
>>>> when
>>>> the i8259 model is fixed.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> hw/isa/vt82c686.c | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 018a119964..3e44a51f92 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -549,6 +549,7 @@ struct ViaISAState {
>>>>      PCIDevice dev;
>>>>      qemu_irq cpu_intr;
>>>>      qemu_irq *isa_irqs_in;
>>>> +    uint16_t isa_irqs_state;
>>>>      ViaSuperIOState via_sio;
>>>>      MC146818RtcState rtc;
>>>>      PCIIDEState ide;
>>>> @@ -636,6 +637,14 @@ static void via_isa_set_pci_irq(void
>>>> *opaque, int irq_num, int level)
>>>>              pic_level |= pci_bus_get_irq_level(bus, i);
>>>>          }
>>>>      }
>>>> +    /* FIXME: workaround for i8259: level sensitive irq not
>>>> supported */
>>>> +    if ((s->isa_irqs_state & BIT(pic_irq)) && pic_level) {
>>>> +        qemu_irq_lower(s->isa_irqs_in[pic_irq]);
>>>> +    } else if (pic_level) {
>>>> +        s->isa_irqs_state |= BIT(pic_irq);
>>>> +    } else {
>>>> +        s->isa_irqs_state &= ~BIT(pic_irq);
>>>> +    }
>>>
>>> Let's not clutter the device model with workarounds which quickly
>>> snowball into unmaintainable code. Please fix the i8259 instead.
>>
>> Do you have an idea how?
>
> Let's start by understanding the problem completely. The i8259 *does*
> support level-triggered interrupts. Look at the checks for s->elcr in
> hw/intc/i8259.c, in pic_set_irq()...
>
>    if (s->elcr & mask) {
>        /* level triggered */
>        if (level) {
>            s->irr |= mask;
>            s->last_irr |= mask;
>        } else {
>            s->irr &= ~mask;
>            s->last_irr &= ~mask;
>        }
>    } else {
>        /* edge triggered */
>
>
> ... and in pic_intack() ...
>
>
>    /* We don't clear a level sensitive interrupt here */
>    if (!(s->elcr & (1 << irq))) {
>        s->irr &= ~(1 << irq);
>    }
>
>
> What qemu typically has an issue with is *shared* level-triggered
> interrupts. But that would cause a level to be "forgotten" if another
> input asserts and then deasserts the IRQ while one input thinks it's
> holding it asserted. And I don't see how your workaround above would
> have helped in that situation.
>
> Are you sure the PIC ELCR is actually set for the lines you're having
> trouble with? Is that something the Pegasos SmartFirmware would have
> done, and MorphOS is expecting to inherit but isn't actually setting up
> for itself?

No, it works with other guests like Linux and AmigaOS that use PIC as set 
up by the firmware but MorphOS tries to use it in level sensitive mode and 
likely has an IRQ handler which expects this to work. This is where I've 
debugged it and came to this workaround:

https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html

When booting MorphOS with -d unimp I see these logs:

i8259: level sensitive irq not supported
i8259: level sensitive irq not supported

which is I guess when it tries to set it for both PICs. (If you want to 
try this MorphOS iso is downloadable and instructions how to boot it is 
here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos

Regards,
BALATON Zoltan

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

On Wed, 2023-03-01 at 14:18 +0100, BALATON Zoltan wrote:
> > Are you sure the PIC ELCR is actually set for the lines you're having
> > trouble with? Is that something the Pegasos SmartFirmware would have
> > done, and MorphOS is expecting to inherit but isn't actually setting up
> > for itself?
> 
> No, it works with other guests like Linux and AmigaOS that use PIC as set 
> up by the firmware but MorphOS tries to use it in level sensitive mode and 
> likely has an IRQ handler which expects this to work. This is where I've 
> debugged it and came to this workaround:
> 
> https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html
> 
> When booting MorphOS with -d unimp I see these logs:
> 
> i8259: level sensitive irq not supported
> i8259: level sensitive irq not supported
> 
> which is I guess when it tries to set it for both PICs. (If you want to 
> try this MorphOS iso is downloadable and instructions how to boot it is 
> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos


Wow. Even looking at the PIIX3 datasheet from 1996, That 'Edge/Level
Bank Select (LTIM)' bit was documented as 'This bit is disabled. Its
function is replaced by the Edge/Level Triggerede Control (ELCR)
Registers.

We've been able to set the edge/level on a per-pin basis ever since the
ELCR was introduced with the IBM PS/2, I think.

It isn't a *correct* fix without a little bit more typing, but does
this make it work?

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 17910f3bcb..36ebcff025 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
             if (val & 0x08) {
                 qemu_log_mask(LOG_UNIMP,
                               "i8259: level sensitive irq not supported\n");
+                s->elcr = 0xff;
             }
         } else if (val & 0x08) {
             if (val & 0x04) {




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

On Wed, 1 Mar 2023, David Woodhouse wrote:
> On Wed, 2023-03-01 at 14:18 +0100, BALATON Zoltan wrote:
>>> Are you sure the PIC ELCR is actually set for the lines you're having
>>> trouble with? Is that something the Pegasos SmartFirmware would have
>>> done, and MorphOS is expecting to inherit but isn't actually setting up
>>> for itself?
>>
>> No, it works with other guests like Linux and AmigaOS that use PIC as set
>> up by the firmware but MorphOS tries to use it in level sensitive mode and
>> likely has an IRQ handler which expects this to work. This is where I've
>> debugged it and came to this workaround:
>>
>> https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html
>>
>> When booting MorphOS with -d unimp I see these logs:
>>
>> i8259: level sensitive irq not supported
>> i8259: level sensitive irq not supported
>>
>> which is I guess when it tries to set it for both PICs. (If you want to
>> try this MorphOS iso is downloadable and instructions how to boot it is
>> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos
>
>
> Wow. Even looking at the PIIX3 datasheet from 1996, That 'Edge/Level
> Bank Select (LTIM)' bit was documented as 'This bit is disabled. Its
> function is replaced by the Edge/Level Triggerede Control (ELCR)
> Registers.
>
> We've been able to set the edge/level on a per-pin basis ever since the
> ELCR was introduced with the IBM PS/2, I think.
>
> It isn't a *correct* fix without a little bit more typing, but does
> this make it work?
>
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 17910f3bcb..36ebcff025 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>             if (val & 0x08) {
>                 qemu_log_mask(LOG_UNIMP,
>                               "i8259: level sensitive irq not supported\n");
> +                s->elcr = 0xff;

This works too. I guess the log can be then removed too. Could you submit 
a proper patch or you want me to do that so we can drop the workaround for 
it? Thanks for looking into it.

Regards,
BALATON Zoltan

>             }
>         } else if (val & 0x08) {
>             if (val & 0x04) {
>
>
>
>

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

On Wed, 2023-03-01 at 19:01 +0100, BALATON Zoltan wrote:
> 
> > It isn't a *correct* fix without a little bit more typing, but does
> > this make it work?
> > 
> > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> > index 17910f3bcb..36ebcff025 100644
> > --- a/hw/intc/i8259.c
> > +++ b/hw/intc/i8259.c
> > @@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
> >              if (val & 0x08) {
> >                  qemu_log_mask(LOG_UNIMP,
> >                                "i8259: level sensitive irq not supported\n");
> > +                s->elcr = 0xff;
> 
> This works too. I guess the log can be then removed too. Could you submit 
> a proper patch or you want me to do that so we can drop the workaround for 
> it? Thanks for looking into it.


Happy for you to do the rest of the typing ... :)

So, *ideally* I think you need to introduce a new field in the
PICCommonState which records the status of the LTIM bit. And fix up the
vmstate_pic_common in hw/intc/i8259_common.c to save and restore that
(with versioning for upgrade/downgrade).

Then you find those places which currently check the bit for the
specific pin in s->elcr, and make them something like:

--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    if (s->elcr & mask) {
+    if (s->ltim || (s->elcr & mask)) {
         /* level triggered */
         if (level) {
             s->irr |= mask;

It *might* be that you should make the LTIM behaviour optional, so that
only certain incarnations of the i8259 actually get it at all and it
*wouldn't* take effect if a guest tried to set it, which is what the
PIIX3 datasheet implies. But I suspect we can get away without that.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

On Wed, 1 Mar 2023, David Woodhouse wrote:
> On Wed, 2023-03-01 at 19:01 +0100, BALATON Zoltan wrote:
>>
>>> It isn't a *correct* fix without a little bit more typing, but does
>>> this make it work?
>>>
>>> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
>>> index 17910f3bcb..36ebcff025 100644
>>> --- a/hw/intc/i8259.c
>>> +++ b/hw/intc/i8259.c
>>> @@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>>>              if (val & 0x08) {
>>>                  qemu_log_mask(LOG_UNIMP,
>>>                                "i8259: level sensitive irq not supported\n");
>>> +                s->elcr = 0xff;
>>
>> This works too. I guess the log can be then removed too. Could you submit
>> a proper patch or you want me to do that so we can drop the workaround for
>> it? Thanks for looking into it.
>
>
> Happy for you to do the rest of the typing ... :)

I don't mind the typing but this is quite a bit more involved than I 
expected. You've lost me at the vmstate stuff which I don't quite know how 
to change or test. If these were stored as bits in an ISW1 register as 
described by the docs I've looked at now then no change in migration would 
be needed but this isn't how it seems to be in QEMU so I give up on that 
in this case. Could you please do the typing then?

Thank you,
BALATON Zoltan

> So, *ideally* I think you need to introduce a new field in the
> PICCommonState which records the status of the LTIM bit. And fix up the
> vmstate_pic_common in hw/intc/i8259_common.c to save and restore that
> (with versioning for upgrade/downgrade).
>
> Then you find those places which currently check the bit for the
> specific pin in s->elcr, and make them something like:
>
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>     }
> #endif
>
> -    if (s->elcr & mask) {
> +    if (s->ltim || (s->elcr & mask)) {
>         /* level triggered */
>         if (level) {
>             s->irr |= mask;
>
> It *might* be that you should make the LTIM behaviour optional, so that
> only certain incarnations of the i8259 actually get it at all and it
> *wouldn't* take effect if a guest tried to set it, which is what the
> PIIX3 datasheet implies. But I suspect we can get away without that.
>
>

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On Wed, 2023-03-01 at 23:47 +0100, BALATON Zoltan wrote:
> On Wed, 1 Mar 2023, David Woodhouse wrote:
> > On Wed, 2023-03-01 at 19:01 +0100, BALATON Zoltan wrote:
> > > 
> > > > It isn't a *correct* fix without a little bit more typing, but does
> > > > this make it work?
> > > > 
> > > > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> > > > index 17910f3bcb..36ebcff025 100644
> > > > --- a/hw/intc/i8259.c
> > > > +++ b/hw/intc/i8259.c
> > > > @@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
> > > >              if (val & 0x08) {
> > > >                  qemu_log_mask(LOG_UNIMP,
> > > >                                "i8259: level sensitive irq not supported\n");
> > > > +                s->elcr = 0xff;
> > > 
> > > This works too. I guess the log can be then removed too. Could you submit
> > > a proper patch or you want me to do that so we can drop the workaround for
> > > it? Thanks for looking into it.
> > 
> > 
> > Happy for you to do the rest of the typing ... :)
> 
> I don't mind the typing but this is quite a bit more involved than I 
> expected. You've lost me at the vmstate stuff which I don't quite know how 
> to change or test. If these were stored as bits in an ISW1 register as 
> described by the docs I've looked at now then no change in migration would 
> be needed but this isn't how it seems to be in QEMU so I give up on that 
> in this case. Could you please do the typing then?

Yeah, it is a bit weird that we store the ICW1 byte in *separate*
elements of persistent state, each of *them* a uint8_t which holds only
a single bit of ICW1:

            s->init4 = val & 1;
            s->single_mode = val & 2;
            s->ltim = val & 8;

Still, it's a pattern that's easy enough to follow.

As for the rest of the typing, I'd basically done the bulk of it
already when showing how to adjust the existing (s->elcr&mask) checks;
there was only one more of those to type.

And then the vmstate part is basically just a cut and paste of the bit
in docs/devel/migration.rst which tells you exactly how to do it.

Patch follows. It builds, but I'll let you do the actual testing,
including migration to/from the new version, checking with
scripts/analyze-migration.py that the ltim is there when it should be,
and isn't when it shouldn't, and any other review feedback.







[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select

[David Woodhouse]

Back in the mists of time, before IBM PS/2 came along with MCA and added
per-pin level control in the ELCR register, the i8259 had a chip-wide
level-mode control in bit 3 of ICW1.

Even in the PIIX3 datasheet from 1996 this is documented as 'This bit is
disabled', but apparently MorphOS is using it in the version of the
i8259 which is in the Pegasos2 board as part of the vt82c686 chipset.

It's easy enough to implement, and I think it's harmless enough to do so
unconditionally.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 hw/intc/i8259.c                 | 10 ++++------
 hw/intc/i8259_common.c          | 24 +++++++++++++++++++++++-
 include/hw/isa/i8259_internal.h |  1 +
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 17910f3bcb..bbae2d87f4 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
     }
 #endif
 
-    if (s->elcr & mask) {
+    if (s->ltim || (s->elcr & mask)) {
         /* level triggered */
         if (level) {
             s->irr |= mask;
@@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq)
         s->isr |= (1 << irq);
     }
     /* We don't clear a level sensitive interrupt here */
-    if (!(s->elcr & (1 << irq))) {
+    if (!s->ltim && !(s->elcr & (1 << irq))) {
         s->irr &= ~(1 << irq);
     }
     pic_update_irq(s);
@@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev)
     PICCommonState *s = PIC_COMMON(dev);
 
     s->elcr = 0;
+    s->ltim = 0;
     pic_init_reset(s);
 }
 
@@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
             s->init_state = 1;
             s->init4 = val & 1;
             s->single_mode = val & 2;
-            if (val & 0x08) {
-                qemu_log_mask(LOG_UNIMP,
-                              "i8259: level sensitive irq not supported\n");
-            }
+            s->ltim = val & 8;
         } else if (val & 0x08) {
             if (val & 0x04) {
                 s->poll = 1;
diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
index af2e4a2241..c931dc2d07 100644
--- a/hw/intc/i8259_common.c
+++ b/hw/intc/i8259_common.c
@@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s)
     s->special_fully_nested_mode = 0;
     s->init4 = 0;
     s->single_mode = 0;
-    /* Note: ELCR is not reset */
+    /* Note: ELCR and LTIM are not reset */
 }
 
 static int pic_dispatch_pre_save(void *opaque)
@@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
                    s->special_fully_nested_mode);
 }
 
+static bool ltim_state_needed(void *opaque)
+{
+    PICCommonState *s = PIC_COMMON(opaque);
+
+    return !!s->ltim;
+}
+
+static const VMStateDescription vmstate_pic_ltim = {
+    .name = "i8259/ltim",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ltim_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(ltim, PICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_pic_common = {
     .name = "i8259",
     .version_id = 1,
@@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = {
         VMSTATE_UINT8(single_mode, PICCommonState),
         VMSTATE_UINT8(elcr, PICCommonState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_pic_ltim,
+        NULL
     }
 };
 
diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
index 155b098452..f9dcc4163e 100644
--- a/include/hw/isa/i8259_internal.h
+++ b/include/hw/isa/i8259_internal.h
@@ -61,6 +61,7 @@ struct PICCommonState {
     uint8_t single_mode; /* true if slave pic is not initialized */
     uint8_t elcr; /* PIIX edge/trigger selection*/
     uint8_t elcr_mask;
+    uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */
     qemu_irq int_out[1];
     uint32_t master; /* reflects /SP input pin */
     uint32_t iobase;
-- 
2.39.0

Re: [PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Thu, 2023-03-02 at 09:06 +0000, David Woodhouse wrote:
> Back in the mists of time, before IBM PS/2 came along with MCA and added
> per-pin level control in the ELCR register, the i8259 had a chip-wide
> level-mode control in bit 3 of ICW1.

Actually... I think MCA might have been level triggered system-wide,
and still used this LTIM bit. Per-pin control via ELCR probably came in
with EISA?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select

[BALATON Zoltan]

On Thu, 2 Mar 2023, David Woodhouse wrote:
> Back in the mists of time, before IBM PS/2 came along with MCA and added
> per-pin level control in the ELCR register, the i8259 had a chip-wide
> level-mode control in bit 3 of ICW1.

Thanks a lot for doing this, it's easy if you already understand the PIC 
and know where to look for the rest but takes more time if I have to learn 
all that first. I can only comment on comments, the rest looks plausible 
but I lack the knowledge to review that. I'll test this later today wirh 
MorphOS and if no problems found I can make chnages to commit message as 
you've asked and add it to the series to replace my workaround.

I'm not sure how to test migration, I've never used that with QEMU so if 
somebody can do that with PC machine that would help. The pegasos2 does 
not yet support migration because it did not work before and we are still 
implementing parts of VT8231 so it was decided to only fix vmstate saving 
after that's settled so we don't have to worry about that now.

> Even in the PIIX3 datasheet from 1996 this is documented as 'This bit is
> disabled', but apparently MorphOS is using it in the version of the
> i8259 which is in the Pegasos2 board as part of the vt82c686 chipset.

The pegasos2 actually has VT8231 which is closely related to 686B but a 
bit later member of that family.

> It's easy enough to implement, and I think it's harmless enough to do so
> unconditionally.
>
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
> hw/intc/i8259.c                 | 10 ++++------
> hw/intc/i8259_common.c          | 24 +++++++++++++++++++++++-
> include/hw/isa/i8259_internal.h |  1 +
> 3 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 17910f3bcb..bbae2d87f4 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -133,7 +133,7 @@ static void pic_set_irq(void *opaque, int irq, int level)
>     }
> #endif
>
> -    if (s->elcr & mask) {
> +    if (s->ltim || (s->elcr & mask)) {
>         /* level triggered */
>         if (level) {
>             s->irr |= mask;
> @@ -167,7 +167,7 @@ static void pic_intack(PICCommonState *s, int irq)
>         s->isr |= (1 << irq);
>     }
>     /* We don't clear a level sensitive interrupt here */
> -    if (!(s->elcr & (1 << irq))) {
> +    if (!s->ltim && !(s->elcr & (1 << irq))) {
>         s->irr &= ~(1 << irq);
>     }
>     pic_update_irq(s);
> @@ -224,6 +224,7 @@ static void pic_reset(DeviceState *dev)
>     PICCommonState *s = PIC_COMMON(dev);
>
>     s->elcr = 0;
> +    s->ltim = 0;
>     pic_init_reset(s);
> }
>
> @@ -243,10 +244,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>             s->init_state = 1;
>             s->init4 = val & 1;
>             s->single_mode = val & 2;
> -            if (val & 0x08) {
> -                qemu_log_mask(LOG_UNIMP,
> -                              "i8259: level sensitive irq not supported\n");
> -            }
> +            s->ltim = val & 8;
>         } else if (val & 0x08) {
>             if (val & 0x04) {
>                 s->poll = 1;
> diff --git a/hw/intc/i8259_common.c b/hw/intc/i8259_common.c
> index af2e4a2241..c931dc2d07 100644
> --- a/hw/intc/i8259_common.c
> +++ b/hw/intc/i8259_common.c
> @@ -51,7 +51,7 @@ void pic_reset_common(PICCommonState *s)
>     s->special_fully_nested_mode = 0;
>     s->init4 = 0;
>     s->single_mode = 0;
> -    /* Note: ELCR is not reset */
> +    /* Note: ELCR and LTIM are not reset */
> }
>
> static int pic_dispatch_pre_save(void *opaque)
> @@ -144,6 +144,24 @@ static void pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
>                    s->special_fully_nested_mode);
> }
>
> +static bool ltim_state_needed(void *opaque)
> +{
> +    PICCommonState *s = PIC_COMMON(opaque);
> +
> +    return !!s->ltim;
> +}
> +
> +static const VMStateDescription vmstate_pic_ltim = {
> +    .name = "i8259/ltim",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = ltim_state_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(ltim, PICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> static const VMStateDescription vmstate_pic_common = {
>     .name = "i8259",
>     .version_id = 1,
> @@ -168,6 +186,10 @@ static const VMStateDescription vmstate_pic_common = {
>         VMSTATE_UINT8(single_mode, PICCommonState),
>         VMSTATE_UINT8(elcr, PICCommonState),
>         VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription*[]) {
> +        &vmstate_pic_ltim,
> +        NULL
>     }
> };
>
> diff --git a/include/hw/isa/i8259_internal.h b/include/hw/isa/i8259_internal.h
> index 155b098452..f9dcc4163e 100644
> --- a/include/hw/isa/i8259_internal.h
> +++ b/include/hw/isa/i8259_internal.h
> @@ -61,6 +61,7 @@ struct PICCommonState {
>     uint8_t single_mode; /* true if slave pic is not initialized */
>     uint8_t elcr; /* PIIX edge/trigger selection*/
>     uint8_t elcr_mask;
> +    uint8_t ltim; /* Edge/Level Bank Select (pre-PIIX, chip-wide) */

Is this rather called Edge/Level Interrupt Mode or something like that?

Regards,
BALATON Zoltan

>     qemu_irq int_out[1];
>     uint32_t master; /* reflects /SP input pin */
>     uint32_t iobase;
>

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4037 bytes --]

On Thu, 2 Mar 2023, David Woodhouse wrote:
> On Wed, 2023-03-01 at 23:47 +0100, BALATON Zoltan wrote:
>> On Wed, 1 Mar 2023, David Woodhouse wrote:
>>> On Wed, 2023-03-01 at 19:01 +0100, BALATON Zoltan wrote:
>>>>
>>>>> It isn't a *correct* fix without a little bit more typing, but does
>>>>> this make it work?
>>>>>
>>>>> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
>>>>> index 17910f3bcb..36ebcff025 100644
>>>>> --- a/hw/intc/i8259.c
>>>>> +++ b/hw/intc/i8259.c
>>>>> @@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>>>>>              if (val & 0x08) {
>>>>>                  qemu_log_mask(LOG_UNIMP,
>>>>>                                "i8259: level sensitive irq not supported\n");
>>>>> +                s->elcr = 0xff;
>>>>
>>>> This works too. I guess the log can be then removed too. Could you submit
>>>> a proper patch or you want me to do that so we can drop the workaround for
>>>> it? Thanks for looking into it.
>>>
>>>
>>> Happy for you to do the rest of the typing ... :)
>>
>> I don't mind the typing but this is quite a bit more involved than I
>> expected. You've lost me at the vmstate stuff which I don't quite know how
>> to change or test. If these were stored as bits in an ISW1 register as
>> described by the docs I've looked at now then no change in migration would
>> be needed but this isn't how it seems to be in QEMU so I give up on that
>> in this case. Could you please do the typing then?
>
> Yeah, it is a bit weird that we store the ICW1 byte in *separate*
> elements of persistent state, each of *them* a uint8_t which holds only
> a single bit of ICW1:
>
>            s->init4 = val & 1;
>            s->single_mode = val & 2;
>            s->ltim = val & 8;
>
> Still, it's a pattern that's easy enough to follow.
>
> As for the rest of the typing, I'd basically done the bulk of it
> already when showing how to adjust the existing (s->elcr&mask) checks;
> there was only one more of those to type.
>
> And then the vmstate part is basically just a cut and paste of the bit
> in docs/devel/migration.rst which tells you exactly how to do it.
>
> Patch follows. It builds, but I'll let you do the actual testing,
> including migration to/from the new version, checking with
> scripts/analyze-migration.py that the ltim is there when it should be,
> and isn't when it shouldn't, and any other review feedback.

I've tested that it fixes the problem with MorphOS on pegasos2 and checked 
that a migration file created while at the firmware, before MorphOS sets 
ltim does not have the subsection while migrate after MorphOS boot has it:

                 {
                     "name": "single_mode",
                     "type": "uint8",
                     "size": 1
                 },
                 {
                     "name": "elcr",
                     "type": "uint8",
                     "size": 1
                 }
             ],
             "subsections": [
                 {
                     "vmsd_name": "i8259/ltim",
                     "version": 1,
                     "fields": [
                         {
                             "name": "ltim",
                             "type": "uint8",
                             "size": 1
                         }
                     ]
                 }
             ]
         },
         {
             "name": "i8259",
             "instance_id": 1,
             "vmsd_name": "i8259",
             "version": 1,
             "fields": [
                 {
                     "name": "last_irr",
                     "type": "uint8",
                     "size": 1
                 },

A migration file saved from the qemu-system-x86_64 default pc machine also 
lacks the subsection so it should not affect migration there unless 
something sets the bit. Is this enough testing or should I try something 
else? I've updated the commit message but did not change the comment.

Regards,
BALATON Zoltan

Re: [PATCH v5 5/7] hw/isa/vt82c686: Work around missing level sensitive irq in i8259 model

[Bernhard Beschow]

Am 1. März 2023 14:05:31 UTC schrieb David Woodhouse <dwmw2@infradead.org>:
>On Wed, 2023-03-01 at 14:18 +0100, BALATON Zoltan wrote:
>> > Are you sure the PIC ELCR is actually set for the lines you're having
>> > trouble with? Is that something the Pegasos SmartFirmware would have
>> > done, and MorphOS is expecting to inherit but isn't actually setting up
>> > for itself?
>> 
>> No, it works with other guests like Linux and AmigaOS that use PIC as set 
>> up by the firmware but MorphOS tries to use it in level sensitive mode and 
>> likely has an IRQ handler which expects this to work. This is where I've 
>> debugged it and came to this workaround:
>> 
>> https://lists.nongnu.org/archive/html/qemu-ppc/2023-02/msg00403.html
>> 
>> When booting MorphOS with -d unimp I see these logs:
>> 
>> i8259: level sensitive irq not supported
>> i8259: level sensitive irq not supported
>> 
>> which is I guess when it tries to set it for both PICs. (If you want to 
>> try this MorphOS iso is downloadable and instructions how to boot it is 
>> here: http://zero.eik.bme.hu/~balaton/qemu/amiga/#morphos
>
>
>Wow. Even looking at the PIIX3 datasheet from 1996, That 'Edge/Level
>Bank Select (LTIM)' bit was documented as 'This bit is disabled. Its
>function is replaced by the Edge/Level Triggerede Control (ELCR)
>Registers.
>
>We've been able to set the edge/level on a per-pin basis ever since the
>ELCR was introduced with the IBM PS/2, I think.
>
>It isn't a *correct* fix without a little bit more typing, but does
>this make it work?
>
>diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
>index 17910f3bcb..36ebcff025 100644
>--- a/hw/intc/i8259.c
>+++ b/hw/intc/i8259.c
>@@ -246,6 +246,7 @@ static void pic_ioport_write(void *opaque, hwaddr addr64,
>             if (val & 0x08) {
>                 qemu_log_mask(LOG_UNIMP,
>                               "i8259: level sensitive irq not supported\n");
>+                s->elcr = 0xff;

Thanks so much, David! You're a genious...

Best regards,

Bernhard

>             }
>         } else if (val & 0x08) {
>             if (val & 0x04) {
>
>
>

[PATCH v5 6/7] hw/usb/vt82c686-uhci-pci: Use PCI IRQ routing

[BALATON Zoltan]

From: Bernhard Beschow <shentey@gmail.com>

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>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 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.30.8

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

[BALATON Zoltan]

Add 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 for now but this is enough to get sound
output from some guests using this device on pegasos2.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v5: rebased on master
v3: Fixed CLEN_LEN mask, add check to avoid runaway DMA and some
tweaks to PCI config regs which now make it work with AmigaOS too.
This is probably as good as it gets for QEMU 8.0

 hw/audio/trace-events     |   6 +
 hw/audio/via-ac97.c       | 455 +++++++++++++++++++++++++++++++++++++-
 hw/isa/trace-events       |   1 +
 hw/isa/vt82c686.c         |   2 +-
 include/hw/isa/vt82c686.h |  25 +++
 5 files changed, 482 insertions(+), 7 deletions(-)

diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index e0e71cd9b1..4dec48a4fd 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 curr, uint32_t addr, char stop, char eol, char flag, uint32_t len) "curr=0x%x 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
diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
index d1a856f63d..676254b7a4 100644
--- a/hw/audio/via-ac97.c
+++ b/hw/audio/via-ac97.c
@@ -1,39 +1,482 @@
 /*
  * 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 & 0xffffff)
+
+#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;
+    }
+    if (unlikely(pci_dma_read(d, c->curr, b, sizeof(b)) != MEMTX_OK)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "via-ac97: DMA error reading SGD table\n");
+        return;
+    }
+    c->addr = le32_to_cpu(b[0]);
+    c->clen = le32_to_cpu(b[1]);
+    trace_via_ac97_sgd_fetch(c->curr, 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 0x10:
+        /* silence unimplemented log message that happens at every IRQ */
+        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 0x%"
+                      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;
+        s->aur.curr = s->aur.base;
+        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 0x%"
+                      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)
 {
-    pci_set_word(pci_dev->config + PCI_COMMAND,
-                 PCI_COMMAND_INVALIDATE | PCI_COMMAND_PARITY);
+    ViaAC97State *s = VIA_AC97(pci_dev);
+    Object *o = OBJECT(s);
+
+    /*
+     * Command register Bus Master bit is documented to be fixed at 0 but it's
+     * needed for PCI DMA to work in QEMU. The pegasos2 firmware writes 0 here
+     * and the AmigaOS driver writes 1 only enabling IO bit which works on
+     * real hardware. So set it here and fix it to 1 to allow DMA.
+     */
+    pci_set_word(pci_dev->config + PCI_COMMAND, PCI_COMMAND_MASTER);
+    pci_set_word(pci_dev->wmask + PCI_COMMAND, PCI_COMMAND_IO);
     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);
+    pci_set_byte(pci_dev->config + 0x40, 1); /* codec ready */
+
+    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 +484,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/trace-events b/hw/isa/trace-events
index c4567a9b47..1816e8307a 100644
--- a/hw/isa/trace-events
+++ b/hw/isa/trace-events
@@ -16,6 +16,7 @@ apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
 
 # vt82c686.c
 via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
+via_pm_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
 via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
 via_pm_io_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
 via_pm_io_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 3e44a51f92..138bebcf5e 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -555,7 +555,7 @@ struct ViaISAState {
     PCIIDEState ide;
     UHCIState uhci[2];
     ViaPMState pm;
-    PCIDevice ac97;
+    ViaAC97State ac97;
     PCIDevice mc97;
 };
 
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
-- 
2.30.8

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

[BALATON Zoltan]

On Wed, 1 Mar 2023, BALATON Zoltan wrote:
> Add 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 for now but this is enough to get sound
> output from some guests using this device on pegasos2.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Gerd, Volker,

Sorry for pushing this but I wanted to make sure you won't miss it. This 
patch needs a review or ack from you to be able to merge it with the other 
patches that fixing interrupt handling on pegasos2 that this patch depends 
on. I hope Philippe can take care of the other remaining patches in time 
for freeze but maybe this one needs your attention. If you already planned 
to do it then sorry again for being impatient.

Regards,
BALATON Zoltan

> ---
> v5: rebased on master
> v3: Fixed CLEN_LEN mask, add check to avoid runaway DMA and some
> tweaks to PCI config regs which now make it work with AmigaOS too.
> This is probably as good as it gets for QEMU 8.0
>
> hw/audio/trace-events     |   6 +
> hw/audio/via-ac97.c       | 455 +++++++++++++++++++++++++++++++++++++-
> hw/isa/trace-events       |   1 +
> hw/isa/vt82c686.c         |   2 +-
> include/hw/isa/vt82c686.h |  25 +++
> 5 files changed, 482 insertions(+), 7 deletions(-)
>
> diff --git a/hw/audio/trace-events b/hw/audio/trace-events
> index e0e71cd9b1..4dec48a4fd 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 curr, uint32_t addr, char stop, char eol, char flag, uint32_t len) "curr=0x%x 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
> diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
> index d1a856f63d..676254b7a4 100644
> --- a/hw/audio/via-ac97.c
> +++ b/hw/audio/via-ac97.c
> @@ -1,39 +1,482 @@
> /*
>  * 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 & 0xffffff)
> +
> +#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;
> +    }
> +    if (unlikely(pci_dma_read(d, c->curr, b, sizeof(b)) != MEMTX_OK)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "via-ac97: DMA error reading SGD table\n");
> +        return;
> +    }
> +    c->addr = le32_to_cpu(b[0]);
> +    c->clen = le32_to_cpu(b[1]);
> +    trace_via_ac97_sgd_fetch(c->curr, 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 0x10:
> +        /* silence unimplemented log message that happens at every IRQ */
> +        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 0x%"
> +                      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;
> +        s->aur.curr = s->aur.base;
> +        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 0x%"
> +                      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)
> {
> -    pci_set_word(pci_dev->config + PCI_COMMAND,
> -                 PCI_COMMAND_INVALIDATE | PCI_COMMAND_PARITY);
> +    ViaAC97State *s = VIA_AC97(pci_dev);
> +    Object *o = OBJECT(s);
> +
> +    /*
> +     * Command register Bus Master bit is documented to be fixed at 0 but it's
> +     * needed for PCI DMA to work in QEMU. The pegasos2 firmware writes 0 here
> +     * and the AmigaOS driver writes 1 only enabling IO bit which works on
> +     * real hardware. So set it here and fix it to 1 to allow DMA.
> +     */
> +    pci_set_word(pci_dev->config + PCI_COMMAND, PCI_COMMAND_MASTER);
> +    pci_set_word(pci_dev->wmask + PCI_COMMAND, PCI_COMMAND_IO);
>     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);
> +    pci_set_byte(pci_dev->config + 0x40, 1); /* codec ready */
> +
> +    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 +484,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/trace-events b/hw/isa/trace-events
> index c4567a9b47..1816e8307a 100644
> --- a/hw/isa/trace-events
> +++ b/hw/isa/trace-events
> @@ -16,6 +16,7 @@ apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
>
> # vt82c686.c
> via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> +via_pm_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> via_pm_io_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> via_pm_io_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 3e44a51f92..138bebcf5e 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -555,7 +555,7 @@ struct ViaISAState {
>     PCIIDEState ide;
>     UHCIState uhci[2];
>     ViaPMState pm;
> -    PCIDevice ac97;
> +    ViaAC97State ac97;
>     PCIDevice mc97;
> };
>
> 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
>

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

[Volker Rümelin]

> Add 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 for now but this is enough to get sound
> output from some guests using this device on pegasos2.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v5: rebased on master
> v3: Fixed CLEN_LEN mask, add check to avoid runaway DMA and some
> tweaks to PCI config regs which now make it work with AmigaOS too.
> This is probably as good as it gets for QEMU 8.0
>
>   hw/audio/trace-events     |   6 +
>   hw/audio/via-ac97.c       | 455 +++++++++++++++++++++++++++++++++++++-
>   hw/isa/trace-events       |   1 +
>   hw/isa/vt82c686.c         |   2 +-
>   include/hw/isa/vt82c686.h |  25 +++
>   5 files changed, 482 insertions(+), 7 deletions(-)
>

Reviewed-by: Volker Rümelin <vr_qemu@t-online.de>