[PATCH v8 0/6] Pegasos2 fixes and audio output support

[PATCH v8 0/6] Pegasos2 fixes and audio output support

[BALATON Zoltan]

Changes in this version:
v8:
- Last change to logging in patch 3 as asked by Bernhard
v7:
- Added a comment and log to patch 3 as asked by Bernhard
- Added missed R-b tag to patch 5
- Patch 4 in this version is

Based-on: <20230304114043.121024-1-shentey@gmail.com>

(hw/isa/vt82c686: Fix wiring of PIC -> CPU interrupt)
so whatever fix for that will be taken this should apply (revert and
corresponding patch for fixing same problem was in v6 which can still
be mixed and matched in this series if needed)

Only patches 1-3 don't yet have review, patch 1 and 2 are optional
(patch 1 adds debug property useful for testing, should be easy to
review; patch 2 fixes MorphOS on pegasos2 with PCI and USB devices)
patch 3 is needed for the rest of the series and has been beaten to
death already so it's about time to come to a conclusion with it at
last.

BALATON Zoltan (4):
  hw/display/sm501: Add debug property to control pixman usage
  hw/isa/vt82c686: Implement PCI IRQ routing
  hw/ppc/pegasos2: Fix PCI interrupt routing
  hw/audio/via-ac97: Basic implementation of audio playback

Bernhard Beschow (1):
  hw/usb/vt82c686-uhci-pci: Use PCI IRQ routing

David Woodhouse (1):
  hw/intc/i8259: Implement legacy LTIM Edge/Level Bank Select

 hw/audio/trace-events           |   6 +
 hw/audio/via-ac97.c             | 455 +++++++++++++++++++++++++++++++-
 hw/display/sm501.c              |  18 +-
 hw/intc/i8259.c                 |  10 +-
 hw/intc/i8259_common.c          |  24 +-
 hw/isa/trace-events             |   1 +
 hw/isa/vt82c686.c               |  44 ++-
 hw/pci-host/mv64361.c           |   4 -
 hw/ppc/pegasos2.c               |  26 +-
 hw/usb/vt82c686-uhci-pci.c      |  12 -
 include/hw/isa/i8259_internal.h |   1 +
 include/hw/isa/vt82c686.h       |  25 ++
 12 files changed, 592 insertions(+), 34 deletions(-)

-- 
2.30.8

[PATCH v8 1/6] 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>
Tested-by: Rene Engel <ReneEngel80@emailn.de>
---
 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

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

[BALATON Zoltan]

From: David Woodhouse <dwmw2@infradead.org>

Back in the mists of time, before EISA came along and required per-pin
level control in the ELCR register, the i8259 had a single 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 VT8231 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>
[balaton: updated commit message as asked by author]
Tested-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 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.30.8

[PATCH v8 3/6] 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>
Tested-by: Rene Engel <ReneEngel80@emailn.de>
---
 hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 8900d87f59..3383ab7e2d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
     qemu_set_irq(s->isa_irqs_in[n], 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);
+
+    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
+    if (!pic_irq) {
+        return;
+    }
+    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
+    }
+
+    /* 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);
@@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
 
+    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
+
     /* RTC */
     qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
-- 
2.30.8

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[Mark Cave-Ayland]

On 06/03/2023 12:33, BALATON Zoltan wrote:

> The real VIA south bridges implement a PCI IRQ router which is configured
> by the BIOS or the OS. In order to respect these configurations, QEMU
> needs to implement it as well. 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>
> Tested-by: Rene Engel <ReneEngel80@emailn.de>
> ---
>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 8900d87f59..3383ab7e2d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>       qemu_set_irq(s->isa_irqs_in[n], 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);
> +
> +    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
> +    if (!pic_irq) {
> +        return;
> +    }
> +    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
> +    }
> +
> +    /* 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);
> @@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>       i8254_pit_init(isa_bus, 0x40, 0, NULL);
>       i8257_dma_init(isa_bus, 0);
>   
> +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
> +
>       /* RTC */
>       qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>       if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {

Writing my previous response where I asked about the additional PCI interrupt 
connections to the MV64361, I realised that if you forget about the Northbridge 
component for a moment then things start to make a bit more sense.

At the moment we have 3 different scenarios:

    1) ISA IRQ -> 8259 for all ISA devices

    2) ISA IRQ -> 8259 *OR*
       PCI IRQ -> PCI IRQ router -> 8259 for the PCI-IDE device

    3) PCI IRQ -> PCI IRQ router -> 8259 for PCI devices

When you look at it this way it is easy to see for case 3 that the PCI IRQ routing 
mechanism is handled by *_set_irq() and *_map_irq() callbacks. And so with that in 
mind, re-reading the VIA datasheet I came up with the following PoC for discussion: 
https://github.com/mcayland/qemu/commits/via-poc.

It needs a bit of work, but at first glance it ticks all the boxes in that the PCI 
bus IRQs are all internal to the VIA southbridge (no global via_isa_set_irq() 
function and no overriding of PCI bus IRQs), there are separate legacy and PCI IRQs 
for the via-ide device, and the PCI IRQ routing bears at least a passing resemblance 
to the datasheet.


ATB,

Mark.

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
> On 06/03/2023 12:33, BALATON Zoltan wrote:
>> The real VIA south bridges implement a PCI IRQ router which is configured
>> by the BIOS or the OS. In order to respect these configurations, QEMU
>> needs to implement it as well. 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>
>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>> ---
>>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 8900d87f59..3383ab7e2d 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>>       qemu_set_irq(s->isa_irqs_in[n], 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);
>> +
>> +    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
>> +    if (!pic_irq) {
>> +        return;
>> +    }
>> +    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
>> +    }
>> +
>> +    /* 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);
>> @@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>       i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>       i8257_dma_init(isa_bus, 0);
>>   +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", 
>> PCI_NUM_PINS);
>> +
>>       /* RTC */
>>       qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>       if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>
> Writing my previous response where I asked about the additional PCI interrupt 
> connections to the MV64361, I realised that if you forget about the 
> Northbridge component for a moment then things start to make a bit more 
> sense.
>
> At the moment we have 3 different scenarios:
>
>   1) ISA IRQ -> 8259 for all ISA devices
>
>   2) ISA IRQ -> 8259 *OR*
>      PCI IRQ -> PCI IRQ router -> 8259 for the PCI-IDE device
>
>   3) PCI IRQ -> PCI IRQ router -> 8259 for PCI devices
>
> When you look at it this way it is easy to see for case 3 that the PCI IRQ 
> routing mechanism is handled by *_set_irq() and *_map_irq() callbacks. And so 
> with that in mind, re-reading the VIA datasheet I came up with the following 
> PoC for discussion: https://github.com/mcayland/qemu/commits/via-poc.

At first glance this is basically what I had in v1 of this series just 
using PCI function numbers instead of an enum to find the IRQ source.

> It needs a bit of work, but at first glance it ticks all the boxes in that 
> the PCI bus IRQs are all internal to the VIA southbridge (no global 
> via_isa_set_irq() function and no overriding of PCI bus IRQs), there are 
> separate legacy and PCI IRQs for the via-ide device, and the PCI IRQ routing 
> bears at least a passing resemblance to the datasheet.

Given that we only have a few hours left until the freeze I hope you're 
not proposing to drop this series and postpone all this to the next 
release, only that we do this clean up in the next devel cycle. You were 
away when this series were on the list for review so this is a bit late 
now for a big rewrite. (Especially that what you propose is a variant of 
the original I've submitted that I had to change due to other review 
comments.)

Since this version was tested and works please accept this for QEMU 8.0 
now then we can work out your idea in the next devel cycle but until then 
this version allows people to run AmigaOS on pegasos2 with sound which is 
the goal I want to achieve for QEMU 8.0 and does not introduce any change 
to via-ide which was good enough for the last two years so it should be 
good enough for a few more months.

Regards,
BALATON Zoltan

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[Mark Cave-Ayland]

On 07/03/2023 00:20, BALATON Zoltan wrote:

> On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
>> On 06/03/2023 12:33, BALATON Zoltan wrote:
>>> The real VIA south bridges implement a PCI IRQ router which is configured
>>> by the BIOS or the OS. In order to respect these configurations, QEMU
>>> needs to implement it as well. 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>
>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>> ---
>>>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>> index 8900d87f59..3383ab7e2d 100644
>>> --- a/hw/isa/vt82c686.c
>>> +++ b/hw/isa/vt82c686.c
>>> @@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>>>       qemu_set_irq(s->isa_irqs_in[n], 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);
>>> +
>>> +    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
>>> +    if (!pic_irq) {
>>> +        return;
>>> +    }
>>> +    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
>>> +    }
>>> +
>>> +    /* 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);
>>> @@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>>       i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>       i8257_dma_init(isa_bus, 0);
>>>   +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
>>> +
>>>       /* RTC */
>>>       qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>       if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>
>> Writing my previous response where I asked about the additional PCI interrupt 
>> connections to the MV64361, I realised that if you forget about the Northbridge 
>> component for a moment then things start to make a bit more sense.
>>
>> At the moment we have 3 different scenarios:
>>
>>   1) ISA IRQ -> 8259 for all ISA devices
>>
>>   2) ISA IRQ -> 8259 *OR*
>>      PCI IRQ -> PCI IRQ router -> 8259 for the PCI-IDE device
>>
>>   3) PCI IRQ -> PCI IRQ router -> 8259 for PCI devices
>>
>> When you look at it this way it is easy to see for case 3 that the PCI IRQ routing 
>> mechanism is handled by *_set_irq() and *_map_irq() callbacks. And so with that in 
>> mind, re-reading the VIA datasheet I came up with the following PoC for discussion: 
>> https://github.com/mcayland/qemu/commits/via-poc.
> 
> At first glance this is basically what I had in v1 of this series just using PCI 
> function numbers instead of an enum to find the IRQ source.

I can see the similarity however the difference here is that the PCI routing is done 
using the existing PCI routing functions in QEMU, rather than requiring an external 
function call specific to VIA devices.

>> It needs a bit of work, but at first glance it ticks all the boxes in that the PCI 
>> bus IRQs are all internal to the VIA southbridge (no global via_isa_set_irq() 
>> function and no overriding of PCI bus IRQs), there are separate legacy and PCI IRQs 
>> for the via-ide device, and the PCI IRQ routing bears at least a passing 
>> resemblance to the datasheet.
> 
> Given that we only have a few hours left until the freeze I hope you're not proposing 
> to drop this series and postpone all this to the next release, only that we do this 
> clean up in the next devel cycle. You were away when this series were on the list for 
> review so this is a bit late now for a big rewrite. (Especially that what you propose 
> is a variant of the original I've submitted that I had to change due to other review 
> comments.)
> 
> Since this version was tested and works please accept this for QEMU 8.0 now then we 
> can work out your idea in the next devel cycle but until then this version allows 
> people to run AmigaOS on pegasos2 with sound which is the goal I want to achieve for 
> QEMU 8.0 and does not introduce any change to via-ide which was good enough for the 
> last two years so it should be good enough for a few more months.

My understanding from the thread was that testing shows there are still hangs when 
using sound/USB/IDE simultaneously with this series, no? Or has that now been fixed?

I completely understand it can be frustrating not getting patches merged, but often 
as developers on less popular machines it can take a long time. My perspective here 
is that both you and Bernhard have out-of-tree patches for using the VIA 
southbridges, and during review Bernhard has raised legitimate review questions based 
upon his experience.

To me it makes sense to resolve these outstanding issues first to provide a solution 
that works for everyone, rather than pushing to merge a series that still has 
reliability issues and where there is lack of consensus between developers. The worst 
case scenario to me is that these patches get merged, people report that QEMU is 
unreliable for AmigaOS, and then we end up repeating this entire process yet again 
several months down the line when Bernhard submits his series for upstream.


ATB,

Mark.

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

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

On Tue, 7 Mar 2023, Mark Cave-Ayland wrote:
> On 07/03/2023 00:20, BALATON Zoltan wrote:
>> On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
>>> On 06/03/2023 12:33, BALATON Zoltan wrote:
>>>> The real VIA south bridges implement a PCI IRQ router which is configured
>>>> by the BIOS or the OS. In order to respect these configurations, QEMU
>>>> needs to implement it as well. 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>
>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>>> ---
>>>>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 42 insertions(+)
>>>> 
>>>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>>>> index 8900d87f59..3383ab7e2d 100644
>>>> --- a/hw/isa/vt82c686.c
>>>> +++ b/hw/isa/vt82c686.c
>>>> @@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>>>>       qemu_set_irq(s->isa_irqs_in[n], 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);
>>>> +
>>>> +    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
>>>> +    if (!pic_irq) {
>>>> +        return;
>>>> +    }
>>>> +    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");
>>>> +    }
>>>> +
>>>> +    /* 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);
>>>> @@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error 
>>>> **errp)
>>>>       i8254_pit_init(isa_bus, 0x40, 0, NULL);
>>>>       i8257_dma_init(isa_bus, 0);
>>>>   +    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", 
>>>> PCI_NUM_PINS);
>>>> +
>>>>       /* RTC */
>>>>       qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>>>>       if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>> 
>>> Writing my previous response where I asked about the additional PCI 
>>> interrupt connections to the MV64361, I realised that if you forget about 
>>> the Northbridge component for a moment then things start to make a bit 
>>> more sense.
>>> 
>>> At the moment we have 3 different scenarios:
>>> 
>>>   1) ISA IRQ -> 8259 for all ISA devices
>>> 
>>>   2) ISA IRQ -> 8259 *OR*
>>>      PCI IRQ -> PCI IRQ router -> 8259 for the PCI-IDE device
>>> 
>>>   3) PCI IRQ -> PCI IRQ router -> 8259 for PCI devices
>>> 
>>> When you look at it this way it is easy to see for case 3 that the PCI IRQ 
>>> routing mechanism is handled by *_set_irq() and *_map_irq() callbacks. And 
>>> so with that in mind, re-reading the VIA datasheet I came up with the 
>>> following PoC for discussion: 
>>> https://github.com/mcayland/qemu/commits/via-poc.
>> 
>> At first glance this is basically what I had in v1 of this series just 
>> using PCI function numbers instead of an enum to find the IRQ source.
>
> I can see the similarity however the difference here is that the PCI routing 
> is done using the existing PCI routing functions in QEMU, rather than 
> requiring an external function call specific to VIA devices.
>
>>> It needs a bit of work, but at first glance it ticks all the boxes in that 
>>> the PCI bus IRQs are all internal to the VIA southbridge (no global 
>>> via_isa_set_irq() function and no overriding of PCI bus IRQs), there are 
>>> separate legacy and PCI IRQs for the via-ide device, and the PCI IRQ 
>>> routing bears at least a passing resemblance to the datasheet.
>> 
>> Given that we only have a few hours left until the freeze I hope you're not 
>> proposing to drop this series and postpone all this to the next release, 
>> only that we do this clean up in the next devel cycle. You were away when 
>> this series were on the list for review so this is a bit late now for a big 
>> rewrite. (Especially that what you propose is a variant of the original 
>> I've submitted that I had to change due to other review comments.)
>> 
>> Since this version was tested and works please accept this for QEMU 8.0 now 
>> then we can work out your idea in the next devel cycle but until then this 
>> version allows people to run AmigaOS on pegasos2 with sound which is the 
>> goal I want to achieve for QEMU 8.0 and does not introduce any change to 
>> via-ide which was good enough for the last two years so it should be good 
>> enough for a few more months.
>
> My understanding from the thread was that testing shows there are still hangs 
> when using sound/USB/IDE simultaneously with this series, no? Or has that now 
> been fixed?

No. This series fiixes sound/USB/PCI interrupts which is needed to get 
AmigaOS work and be usable on pegasos2. The hang Bernhard found with 
usb-mouse was only affecting MorphOS with this series which uses level 
sensitive mode of i8259 which wasn't implemented. Patch 2 in this series 
thanks to David Woodhouse fixes that (so did my work around before that 
patch) and MorphOS on pegasos2 is not a priority as it already runs on 
mac99 so what I'd like to make work here is AmigaOS for which it's the 
only G4 CPU platform now. This is important as it's much faster than the 
PPC440 version and may be able to run with KVM eventually but to find that 
out this should get in first so people can start to test it. We can always 
improve it later including implementing a better model of IRQ routing in 
VT8231. What we have in this series now works for all guests and all 
important patches have been tested and now reviewed. So I hope Philippe 
can pick this up and then we have time for this discussion afterwards.

> I completely understand it can be frustrating not getting patches merged, but 
> often as developers on less popular machines it can take a long time. My 
> perspective here is that both you and Bernhard have out-of-tree patches for 
> using the VIA southbridges, and during review Bernhard has raised legitimate 
> review questions based upon his experience.

Those review questions have been addressed, I've accepted Bernhard's 
alternative patch even though I think it's not entirely correct and 
although the first series was already tested I've re-done that based on 
Bernhard's idea and asked Rene to test all of it again. That's when you 
came along a few days before the freeze and blocking this without even 
fully understanding what it's about. That is what's frustrating.

> To me it makes sense to resolve these outstanding issues first to provide a 
> solution that works for everyone, rather than pushing to merge a series that

There are no issues to resolvc regatding functionality. All versions of 
this series that I have submitted were tested and are working and achieve 
the goal to make it possible to run AmigaOS on pegasos2 and get sound with 
MorphOS which are not yet possible currently. Nobody showed these patches 
would break anything (which would be surprising anyway as these are only 
used by pegasos2 and fuloong2e the latter of which has never been finished 
so only still around to have a way to test these components independent of 
pegasos2). A solution for everyone would be to merge this series now so 
they can use it in QEMU 8.0 then we have time to improve it and make the 
model conteptually more correct but there are no missing functionality 
that would prevent guests from running with this series so no reason to 
keep this out now.

> still has reliability issues and where there is lack of consensus between 
> developers. The worst case scenario to me is that these patches get merged, 
> people report that QEMU is unreliable for AmigaOS, and then we end up 
> repeating this entire process yet again several months down the line when 
> Bernhard submits his series for upstream.

I don't even know what to say to that. It already took me more time 
arguing with you about it than writing the whole series. We have pegasos2 
in QEMU already which these really small patches that Bernhard now also 
agrees could be accepted for now would allow to run two more guests and 
reach usable state with them that is much better than what's possible now 
and there are several people who can't compile their QEMU from off-tree 
sources but would happily use it from their distro packages or binaries 
provided for release versions. But you just don't care about those people 
or my work and would hold this back indefinitely becuase maybe it could 
break some off-tree changes not even finished or submitted yet or maybe we 
will find a bug later. What's the freeze time for if not for finding bugs 
and fixing them. What's the development window for if not imrpving code 
already there?

Again this is now tested, reviewed and isn't known to break anything 
that's already there or even make it less clean, in fact it does make 
existing code a bit cleaner and fixes some issues so the only problem is 
that you think there must be a better way doing it or do it more fully 
than this series does it but you've failed to say that during review 
because you were away.

Philippe, Peter or any other maintainer please put an end on this 
suffering and submit a pull request with any version of this series (as 
I've said all versions I've sent are tested and working) now so we have it 
working and then we can rewrite it later however Mark wants in the future 
but let not make people who want to use it wait because of unreasonable 
concerns. Putting this off to wait until some other unfinished and 
unrelated machine is written just makes no sense.

Regards,
BALATON Zoltan

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[Alex Bennée]

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

> On Tue, 7 Mar 2023, Mark Cave-Ayland wrote:
>> On 07/03/2023 00:20, BALATON Zoltan wrote:
>>> On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
>>>> On 06/03/2023 12:33, BALATON Zoltan wrote:
<snip>
>>> Given that we only have a few hours left until the freeze I hope
>>> you're not proposing to drop this series and postpone all this to
>>> the next release,

This sort of passive aggressive framing isn't helpful or conducive to
collaboration. We should be striving to merge code based on the merits
of the patches not on how close we are to a given release.

>>> only that we do this clean up in the next devel
>>> cycle. You were away when this series were on the list for review
>>> so this is a bit late now for a big rewrite. (Especially that what
>>> you propose is a variant of the original I've submitted that I had
>>> to change due to other review comments.)
>>> Since this version was tested and works please accept this for QEMU
>>> 8.0 now then we can work out your idea in the next devel cycle but
>>> until then this version allows people to run AmigaOS on pegasos2
>>> with sound which is the goal I want to achieve for QEMU 8.0 and
>>> does not introduce any change to via-ide which was good enough for
>>> the last two years so it should be good enough for a few more
>>> months.
>>
>> My understanding from the thread was that testing shows there are
>> still hangs when using sound/USB/IDE simultaneously with this
>> series, no? Or has that now been fixed?
>
> No. This series fiixes sound/USB/PCI interrupts which is needed to get
> AmigaOS work and be usable on pegasos2. The hang Bernhard found with
> usb-mouse was only affecting MorphOS with this series which uses level
> sensitive mode of i8259 which wasn't implemented. Patch 2 in this
> series thanks to David Woodhouse fixes that (so did my work around
> before that patch) and MorphOS on pegasos2 is not a priority as it
> already runs on mac99 so what I'd like to make work here is AmigaOS
> for which it's the only G4 CPU platform now. This is important as it's
> much faster than the PPC440 version and may be able to run with KVM
> eventually but to find that out this should get in first so people can
> start to test it. We can always improve it later including
> implementing a better model of IRQ routing in VT8231. What we have in
> this series now works for all guests and all important patches have
> been tested and now reviewed. So I hope Philippe can pick this up and
> then we have time for this discussion afterwards.

We shouldn't make perfect the enemy of the good. If the changes are well
localised, reviewed and tested and importantly don't introduce
regressions then we shouldn't hold things up purely on the basis of a
not meeting a preferred style* of an individual maintainer. Obviously
the barrier for entry rises as the impact on the rest of the code base
increases. We have more than enough experience of introducing new APIs
and regretting it later to be understandably cautious in this regard.

(* as opposed to documented coding style which is a valid reason to
reject patches)

>> I completely understand it can be frustrating not getting patches
>> merged, but often as developers on less popular machines it can take
>> a long time. My perspective here is that both you and Bernhard have
>> out-of-tree patches for using the VIA southbridges, and during
>> review Bernhard has raised legitimate review questions based upon
>> his experience.
>
> Those review questions have been addressed, I've accepted Bernhard's
> alternative patch even though I think it's not entirely correct and
> although the first series was already tested I've re-done that based
> on Bernhard's idea and asked Rene to test all of it again. That's when
> you came along a few days before the freeze and blocking this without
> even fully understanding what it's about. That is what's frustrating.

While using Based-on gives enough information to reconstruct a final
tree perhaps it would be simpler to post a full series relative to
master to make for easier review and merging?

>> To me it makes sense to resolve these outstanding issues first to
>> provide a solution that works for everyone, rather than pushing to
>> merge a series that
>
> There are no issues to resolvc regatding functionality. All versions
> of this series that I have submitted were tested and are working and
> achieve the goal to make it possible to run AmigaOS on pegasos2 and
> get sound with MorphOS which are not yet possible currently. Nobody
> showed these patches would break anything (which would be surprising
> anyway as these are only used by pegasos2 and fuloong2e the latter of
> which has never been finished so only still around to have a way to
> test these components independent of pegasos2). A solution for
> everyone would be to merge this series now so they can use it in QEMU
> 8.0 then we have time to improve it and make the model conteptually
> more correct but there are no missing functionality that would prevent
> guests from running with this series so no reason to keep this out
> now.

Regressions would be a good reason to avoid premature merging.

>> still has reliability issues and where there is lack of consensus
>> between developers. The worst case scenario to me is that these
>> patches get merged, people report that QEMU is unreliable for
>> AmigaOS, and then we end up repeating this entire process yet again
>> several months down the line when Bernhard submits his series for
>> upstream.

Do we have any indication that AmigaOS (I assume as a guest) is less
reliable on this series? Is this an area where it can only be confirmed
by manual testing?

I'm not sure we can gate things on a manual test only a few people can
run. This is an argument for improving our testing coverage.

> I don't even know what to say to that. It already took me more time
> arguing with you about it than writing the whole series. We have
> pegasos2 in QEMU already which these really small patches that
> Bernhard now also agrees could be accepted for now would allow to run
> two more guests and reach usable state with them that is much better
> than what's possible now and there are several people who can't
> compile their QEMU from off-tree sources but would happily use it from
> their distro packages or binaries provided for release versions. But
> you just don't care about those people or my work and would hold this
> back indefinitely becuase maybe it could break some off-tree changes
> not even finished or submitted yet or maybe we will find a bug later.

Please don't assume peoples motivation in their feedback - its not
helpful. We should proceed with the default assumption that everyone
wants to improve the project even if opinions on how to do so differ.

> What's the freeze time for if not for finding bugs and fixing them.
> What's the development window for if not imrpving code already there?

We fix bugs that might of slipped in during development - we don't
knowingly introduce a bug with a promise to fix it during freeze.

> Again this is now tested, reviewed and isn't known to break anything
> that's already there or even make it less clean, in fact it does make
> existing code a bit cleaner and fixes some issues so the only problem
> is that you think there must be a better way doing it or do it more
> fully than this series does it but you've failed to say that during
> review because you were away.
>
> Philippe, Peter or any other maintainer please put an end on this
> suffering and submit a pull request with any version of this series
> (as I've said all versions I've sent are tested and working) now so we
> have it working and then we can rewrite it later however Mark wants in
> the future but let not make people who want to use it wait because of
> unreasonable concerns. Putting this off to wait until some other
> unfinished and unrelated machine is written just makes no sense.

I've added the PC machine maintainers to the CC because AFAICT they are
also maintainers for the systems touched here. From my point of view if
the maintainers of the affected subsystems are happy, code is reviewed
and there are no known regressions then there isn't a barrier to getting
this code merged.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

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

On Tue, 7 Mar 2023, Alex Bennée wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> On Tue, 7 Mar 2023, Mark Cave-Ayland wrote:
>>> On 07/03/2023 00:20, BALATON Zoltan wrote:
>>>> On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
>>>>> On 06/03/2023 12:33, BALATON Zoltan wrote:
> <snip>
>>>> Given that we only have a few hours left until the freeze I hope
>>>> you're not proposing to drop this series and postpone all this to
>>>> the next release,
>
> This sort of passive aggressive framing isn't helpful or conducive to
> collaboration. We should be striving to merge code based on the merits
> of the patches not on how close we are to a given release.
>
>>>> only that we do this clean up in the next devel
>>>> cycle. You were away when this series were on the list for review
>>>> so this is a bit late now for a big rewrite. (Especially that what
>>>> you propose is a variant of the original I've submitted that I had
>>>> to change due to other review comments.)
>>>> Since this version was tested and works please accept this for QEMU
>>>> 8.0 now then we can work out your idea in the next devel cycle but
>>>> until then this version allows people to run AmigaOS on pegasos2
>>>> with sound which is the goal I want to achieve for QEMU 8.0 and
>>>> does not introduce any change to via-ide which was good enough for
>>>> the last two years so it should be good enough for a few more
>>>> months.
>>>
>>> My understanding from the thread was that testing shows there are
>>> still hangs when using sound/USB/IDE simultaneously with this
>>> series, no? Or has that now been fixed?
>>
>> No. This series fiixes sound/USB/PCI interrupts which is needed to get
>> AmigaOS work and be usable on pegasos2. The hang Bernhard found with
>> usb-mouse was only affecting MorphOS with this series which uses level
>> sensitive mode of i8259 which wasn't implemented. Patch 2 in this
>> series thanks to David Woodhouse fixes that (so did my work around
>> before that patch) and MorphOS on pegasos2 is not a priority as it
>> already runs on mac99 so what I'd like to make work here is AmigaOS
>> for which it's the only G4 CPU platform now. This is important as it's
>> much faster than the PPC440 version and may be able to run with KVM
>> eventually but to find that out this should get in first so people can
>> start to test it. We can always improve it later including
>> implementing a better model of IRQ routing in VT8231. What we have in
>> this series now works for all guests and all important patches have
>> been tested and now reviewed. So I hope Philippe can pick this up and
>> then we have time for this discussion afterwards.
>
> We shouldn't make perfect the enemy of the good. If the changes are well
> localised, reviewed and tested and importantly don't introduce
> regressions then we shouldn't hold things up purely on the basis of a
> not meeting a preferred style* of an individual maintainer. Obviously
> the barrier for entry rises as the impact on the rest of the code base
> increases. We have more than enough experience of introducing new APIs
> and regretting it later to be understandably cautious in this regard.
>
> (* as opposed to documented coding style which is a valid reason to
> reject patches)
>
>>> I completely understand it can be frustrating not getting patches
>>> merged, but often as developers on less popular machines it can take
>>> a long time. My perspective here is that both you and Bernhard have
>>> out-of-tree patches for using the VIA southbridges, and during
>>> review Bernhard has raised legitimate review questions based upon
>>> his experience.
>>
>> Those review questions have been addressed, I've accepted Bernhard's
>> alternative patch even though I think it's not entirely correct and
>> although the first series was already tested I've re-done that based
>> on Bernhard's idea and asked Rene to test all of it again. That's when
>> you came along a few days before the freeze and blocking this without
>> even fully understanding what it's about. That is what's frustrating.
>
> While using Based-on gives enough information to reconstruct a final
> tree perhaps it would be simpler to post a full series relative to
> master to make for easier review and merging?

The last version v9 I've sent today is based on master without any other 
dependency but it wasn't decided until today what fix will be taken for an 
issue in master that was introduced independntly of this series a week ago 
and I had to rebase this on all propsed fixes for that change until it 
finally turned out I should not target the fix I was prevously was told to 
do.

>>> To me it makes sense to resolve these outstanding issues first to
>>> provide a solution that works for everyone, rather than pushing to
>>> merge a series that
>>
>> There are no issues to resolvc regatding functionality. All versions
>> of this series that I have submitted were tested and are working and
>> achieve the goal to make it possible to run AmigaOS on pegasos2 and
>> get sound with MorphOS which are not yet possible currently. Nobody
>> showed these patches would break anything (which would be surprising
>> anyway as these are only used by pegasos2 and fuloong2e the latter of
>> which has never been finished so only still around to have a way to
>> test these components independent of pegasos2). A solution for
>> everyone would be to merge this series now so they can use it in QEMU
>> 8.0 then we have time to improve it and make the model conteptually
>> more correct but there are no missing functionality that would prevent
>> guests from running with this series so no reason to keep this out
>> now.
>
> Regressions would be a good reason to avoid premature merging.
>
>>> still has reliability issues and where there is lack of consensus
>>> between developers. The worst case scenario to me is that these
>>> patches get merged, people report that QEMU is unreliable for
>>> AmigaOS, and then we end up repeating this entire process yet again
>>> several months down the line when Bernhard submits his series for
>>> upstream.
>
> Do we have any indication that AmigaOS (I assume as a guest) is less
> reliable on this series? Is this an area where it can only be confirmed
> by manual testing?

AmigaOS runs well on this series and can use PCI cards which it cannot on 
current master. The goal of this series is to allow that because otherwise 
it cannot use network and sound without which it's not really usable. With 
this series it finally can be used as shown on the video by Rene who 
tested it and I posted before.

> I'm not sure we can gate things on a manual test only a few people can
> run. This is an argument for improving our testing coverage.

It needs a license for AmigaOS Pegasos 2 version to test with that but 
this series also fixes PCI IRQs for Linux so the same could be tested by 
adding a network card to a Linux guest. But Mark did not run any tests 
just ctiticised based on looking at the patches so not sure it's a 
question of patch coverage in this case.

>> I don't even know what to say to that. It already took me more time
>> arguing with you about it than writing the whole series. We have
>> pegasos2 in QEMU already which these really small patches that
>> Bernhard now also agrees could be accepted for now would allow to run
>> two more guests and reach usable state with them that is much better
>> than what's possible now and there are several people who can't
>> compile their QEMU from off-tree sources but would happily use it from
>> their distro packages or binaries provided for release versions. But
>> you just don't care about those people or my work and would hold this
>> back indefinitely becuase maybe it could break some off-tree changes
>> not even finished or submitted yet or maybe we will find a bug later.
>
> Please don't assume peoples motivation in their feedback - its not
> helpful. We should proceed with the default assumption that everyone
> wants to improve the project even if opinions on how to do so differ.
>
>> What's the freeze time for if not for finding bugs and fixing them.
>> What's the development window for if not imrpving code already there?
>
> We fix bugs that might of slipped in during development - we don't
> knowingly introduce a bug with a promise to fix it during freeze.

This series does not introduce any bugs that we know about. All versions 
I've submitted work equally well without introducing regressions so all 
the different versions were only necessary to implement the same in 
different ways not to fix any regressions but to satisfy different views 
of different people. The regression we now have in master was introduced 
by a different series that wasn't sufficiently tested before merging. This 
series now also has a patch to fix that.

>> Again this is now tested, reviewed and isn't known to break anything
>> that's already there or even make it less clean, in fact it does make
>> existing code a bit cleaner and fixes some issues so the only problem
>> is that you think there must be a better way doing it or do it more
>> fully than this series does it but you've failed to say that during
>> review because you were away.
>>
>> Philippe, Peter or any other maintainer please put an end on this
>> suffering and submit a pull request with any version of this series
>> (as I've said all versions I've sent are tested and working) now so we
>> have it working and then we can rewrite it later however Mark wants in
>> the future but let not make people who want to use it wait because of
>> unreasonable concerns. Putting this off to wait until some other
>> unfinished and unrelated machine is written just makes no sense.
>
> I've added the PC machine maintainers to the CC because AFAICT they are
> also maintainers for the systems touched here. From my point of view if
> the maintainers of the affected subsystems are happy, code is reviewed
> and there are no known regressions then there isn't a barrier to getting
> this code merged.

The only patch that touches pc machine is the i8259 LTIM patch from David 
which is only needed for MorphOS as nothing else uses that mode of the 
i8259 PIC so if you can't decide on that one patch then just drop it and 
merge the rest of the series which is enough for AmigaOS to work. That 
patch being a bug fix could also wait a bit more, no reason to delay the 
whole series because of this patch.


I've said it before but here it is again to make it clearer:

v9-0001-hw-display-sm501-Add-debug-property-to-control-pi.patch
v9-0002-hw-intc-i8259-Implement-legacy-LTIM-Edge-Level-Ba.patch

The above two are optional but would be nice to have. Patch 1 adds a debug 
aid for testing sm501 emulation, patch 2 is only needed for MorphOS on 
pegasos2 which is less important than getting AmigaOS to work.

v9-0003-Revert-hw-isa-vt82c686-Remove-intermediate-IRQ-fo.patch

This fixes up Philippe's series which caused me to need to rebase the 
series several times.

v9-0004-hw-isa-vt82c686-Implement-PCI-IRQ-routing.patch
v9-0005-hw-ppc-pegasos2-Fix-PCI-interrupt-routing.patch
v9-0006-hw-usb-vt82c686-uhci-pci-Use-PCI-IRQ-routing.patch

These three are needed to fix PCI interrupts on pegasos2 and is the 
minimum we need for AmigaOS (with some fix for current breakage like the 
revert above).

v9-0007-hw-audio-via-ac97-Basic-implementation-of-audio-p.patch

This one implements audio output for pegasos2 but AmigaOS can use a sound 
card instead with the above PCI IRQ patches so this could be optional but 
since it's reviewed and tested no reason to not merge it but it depends on 
the other patches before.

Are there any more questions or concerns that I shuold answer about these?

Regards,
BALATON Zoltan

Re: [PATCH v8 3/6] hw/isa/vt82c686: Implement PCI IRQ routing

[Bernhard Beschow]

Am 6. März 2023 12:33:31 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>
>Tested-by: Rene Engel <ReneEngel80@emailn.de>
>---
> hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
>diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>index 8900d87f59..3383ab7e2d 100644
>--- a/hw/isa/vt82c686.c
>+++ b/hw/isa/vt82c686.c
>@@ -600,6 +600,46 @@ void via_isa_set_irq(PCIDevice *d, int n, int level)
>     qemu_set_irq(s->isa_irqs_in[n], 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);
>+
>+    /* IRQ 0: disabled, IRQ 2,8,13: reserved */
>+    if (!pic_irq) {
>+        return;
>+    }
>+    if (unlikely(pic_irq == 2 || pic_irq == 8 || pic_irq == 13)) {
>+        qemu_log_mask(LOG_GUEST_ERROR, "Invalid ISA IRQ routing");

I wonder whether we should return here or not. But well, that's probably why it's reserved. Let's see how it goes and revist in case we learn more...

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>+    }
>+
>+    /* 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);
>@@ -620,6 +660,8 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     i8254_pit_init(isa_bus, 0x40, 0, NULL);
>     i8257_dma_init(isa_bus, 0);
> 
>+    qdev_init_gpio_in_named(dev, via_isa_set_pci_irq, "pirq", PCI_NUM_PINS);
>+
>     /* RTC */
>     qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
>     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {

[PATCH v8 4/6] 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>
Tested-by: Rene Engel <ReneEngel80@emailn.de>
---
 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 b0ada9c963..ded5dc2dc9 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,7 +167,11 @@ 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_new_multifunction(PCI_DEVFN(12, 0), true,
@@ -164,6 +179,9 @@ static void pegasos2_init(MachineState *machine)
     qdev_connect_gpio_out(DEVICE(via), 0,
                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
     pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
+    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"),
@@ -269,6 +287,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 v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[Mark Cave-Ayland]

On 06/03/2023 12:33, 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>
> Tested-by: Rene Engel <ReneEngel80@emailn.de>
> ---
>   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 b0ada9c963..ded5dc2dc9 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);
> +}
> +

Can you explain a bit more about how the PCI interrupt lines are connected to both 
the MV64361 and VT8231? The reason for asking is that I see a similar pattern in the 
bonito board, but there I can't see how those lines would be used because they can 
also raise a CPU interrupt, yet it is a different one compared to the 8259.

Given that we know from Bernhard's tests that the fuloong2e board works with 
pci_bus_irqs() included in via_isa_realize() which overwrites the bonito equivalent, 
I'm wondering if the mv_pirq array is actually needed at all and whether it may just 
be a debugging aid? Certainly it makes things simpler to just route everything to the 
VIA device.

>   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,7 +167,11 @@ 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);

This doesn't make sense to me either, since the PCI bus IRQs should be owned by the 
device that contains the PCI bus and not the board.

>       /* VIA VT8231 South Bridge (multifunction PCI device) */
>       via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), true,
> @@ -164,6 +179,9 @@ static void pegasos2_init(MachineState *machine)
>       qdev_connect_gpio_out(DEVICE(via), 0,
>                             qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>       pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
> +    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"),
> @@ -269,6 +287,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);

This should be a separate commit because it's not part of the PCI interrupt routing 
as per my comment at https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg00193.html.


ATB,

Mark.

Re: [PATCH v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[BALATON Zoltan]

On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
> On 06/03/2023 12:33, 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>
>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>> ---
>>   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 b0ada9c963..ded5dc2dc9 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);
>> +}
>> +
>
> Can you explain a bit more about how the PCI interrupt lines are connected to 
> both the MV64361 and VT8231? The reason for asking is that I see a similar

I think I already did that in previous replies but I try again. In 
pagasosII schematics at https://www.powerdeveloper.org/platforms/pegasos/schematics

Sheet 2 Marvell System Controller shows PINTA-D are connected to MV64361 
MPP12-15 pins that are also called GPP in the docs (I think these are 
referred to as either Multi Purpose or General Purpose Pins).

Sheet 18 VIA VT8231 Southbridge shows INTR_A-D connected to VT8231 PINTA-D 
pins, which are connected to INTA-D on sheet 13 PCI Top Level. Sheet 1 Top 
Level Pegasos shows that Sheet 13 INTA-D are connected to Sheet 2 PINTA-D.

> pattern in the bonito board, but there I can't see how those lines would be 
> used because they can also raise a CPU interrupt, yet it is a different one 
> compared to the 8259.

Both the MV64361 and VT8231 have interrupt mask registers which allow the 
guest to choose where it wants an interrupt from. I guess different guests 
could use one or the other, unlikely they would use both but they could 
even do that if they wanted. I guess in bonito it's also maskable so the 
guest could set up irqs by programing the apprpriate north and south 
bridge registers. A guest which has ISA drivers (probably most) may likely 
want to route them to ISA IRQs in the VIA chip. Maybe this strange 
behaviour has to do with Windows compatibility and standards back then 
which never really took off like CHRP for PPC.

> Given that we know from Bernhard's tests that the fuloong2e board works with 
> pci_bus_irqs() included in via_isa_realize() which overwrites the bonito 
> equivalent, I'm wondering if the mv_pirq array is actually needed at all and 
> whether it may just be a debugging aid? Certainly it makes things simpler to 
> just route everything to the VIA device.

In any case calling pci_bus_irqs in the VIA device would be wrong and 
connections should be made here by the board as that correctly models the 
hardware separating components and allows different boards to use the chip 
model in the future. I remember that I've implemenred connections to 
MV64361 for something but I don't remember which guest so as this already 
works and models what the schematics say I see no reason not to have it 
or remove it.

Regards,
BALATON Zoltan

>>   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,7 +167,11 @@ 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);
>
> This doesn't make sense to me either, since the PCI bus IRQs should be owned 
> by the device that contains the PCI bus and not the board.
>
>>       /* VIA VT8231 South Bridge (multifunction PCI device) */
>>       via = OBJECT(pci_new_multifunction(PCI_DEVFN(12, 0), true,
>> @@ -164,6 +179,9 @@ static void pegasos2_init(MachineState *machine)
>>       qdev_connect_gpio_out(DEVICE(via), 0,
>>                             qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>>       pci_realize_and_unref(PCI_DEVICE(via), pci_bus, &error_fatal);
>> +    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"),
>> @@ -269,6 +287,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);
>
> This should be a separate commit because it's not part of the PCI interrupt 
> routing as per my comment at 
> https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg00193.html.
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[Mark Cave-Ayland]

On 06/03/2023 22:00, BALATON Zoltan wrote:

> On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
>> On 06/03/2023 12:33, 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>
>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>> ---
>>>   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 b0ada9c963..ded5dc2dc9 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);
>>> +}
>>> +
>>
>> Can you explain a bit more about how the PCI interrupt lines are connected to both 
>> the MV64361 and VT8231? The reason for asking is that I see a similar
> 
> I think I already did that in previous replies but I try again. In pagasosII 
> schematics at https://www.powerdeveloper.org/platforms/pegasos/schematics
> 
> Sheet 2 Marvell System Controller shows PINTA-D are connected to MV64361 MPP12-15 
> pins that are also called GPP in the docs (I think these are referred to as either 
> Multi Purpose or General Purpose Pins).
> 
> Sheet 18 VIA VT8231 Southbridge shows INTR_A-D connected to VT8231 PINTA-D pins, 
> which are connected to INTA-D on sheet 13 PCI Top Level. Sheet 1 Top Level Pegasos 
> shows that Sheet 13 INTA-D are connected to Sheet 2 PINTA-D.
> 
>> pattern in the bonito board, but there I can't see how those lines would be used 
>> because they can also raise a CPU interrupt, yet it is a different one compared to 
>> the 8259.
> 
> Both the MV64361 and VT8231 have interrupt mask registers which allow the guest to 
> choose where it wants an interrupt from. I guess different guests could use one or 
> the other, unlikely they would use both but they could even do that if they wanted. I 
> guess in bonito it's also maskable so the guest could set up irqs by programing the 
> apprpriate north and south bridge registers. A guest which has ISA drivers (probably 
> most) may likely want to route them to ISA IRQs in the VIA chip. Maybe this strange 
> behaviour has to do with Windows compatibility and standards back then which never 
> really took off like CHRP for PP
>
>> Given that we know from Bernhard's tests that the fuloong2e board works with 
>> pci_bus_irqs() included in via_isa_realize() which overwrites the bonito 
>> equivalent, I'm wondering if the mv_pirq array is actually needed at all and 
>> whether it may just be a debugging aid? Certainly it makes things simpler to just 
>> route everything to the VIA device.
> 
> In any case calling pci_bus_irqs in the VIA device would be wrong and connections 

Why? In general the PCI bus should be managed completely internally and not exposed 
to the board. This should certainly be the case for the VIA chipsets which can be 
used across multiple QEMU machines.

> should be made here by the board as that correctly models the hardware separating 
> components and allows different boards to use the chip model in the future. I 
> remember that I've implemenred connections to MV64361 for something but I don't 
> remember which guest so as this already works and models what the schematics say I 
> see no reason not to have it or remove it.

That doesn't really help much I'm afraid - are the extra pirq lines required or not? 
If so, which guest OS are they being used for? This information should be included in 
the commit message and a code comment, as if you don't document things like this you 
risk someone attempting a tidy-up refactoring which will break your guest OS.


ATB,

Mark.

Re: [PATCH v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[BALATON Zoltan]

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

On Tue, 7 Mar 2023, Mark Cave-Ayland wrote:
> On 06/03/2023 22:00, BALATON Zoltan wrote:
>> On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
>>> On 06/03/2023 12:33, 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>
>>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>>> ---
>>>>   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 b0ada9c963..ded5dc2dc9 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);
>>>> +}
>>>> +
>>> 
>>> Can you explain a bit more about how the PCI interrupt lines are connected 
>>> to both the MV64361 and VT8231? The reason for asking is that I see a 
>>> similar
>> 
>> I think I already did that in previous replies but I try again. In 
>> pagasosII schematics at 
>> https://www.powerdeveloper.org/platforms/pegasos/schematics
>> 
>> Sheet 2 Marvell System Controller shows PINTA-D are connected to MV64361 
>> MPP12-15 pins that are also called GPP in the docs (I think these are 
>> referred to as either Multi Purpose or General Purpose Pins).
>> 
>> Sheet 18 VIA VT8231 Southbridge shows INTR_A-D connected to VT8231 PINTA-D 
>> pins, which are connected to INTA-D on sheet 13 PCI Top Level. Sheet 1 Top 
>> Level Pegasos shows that Sheet 13 INTA-D are connected to Sheet 2 PINTA-D.
>> 
>>> pattern in the bonito board, but there I can't see how those lines would 
>>> be used because they can also raise a CPU interrupt, yet it is a different 
>>> one compared to the 8259.
>> 
>> Both the MV64361 and VT8231 have interrupt mask registers which allow the 
>> guest to choose where it wants an interrupt from. I guess different guests 
>> could use one or the other, unlikely they would use both but they could 
>> even do that if they wanted. I guess in bonito it's also maskable so the 
>> guest could set up irqs by programing the apprpriate north and south bridge 
>> registers. A guest which has ISA drivers (probably most) may likely want to 
>> route them to ISA IRQs in the VIA chip. Maybe this strange behaviour has to 
>> do with Windows compatibility and standards back then which never really 
>> took off like CHRP for PP
>> 
>>> Given that we know from Bernhard's tests that the fuloong2e board works 
>>> with pci_bus_irqs() included in via_isa_realize() which overwrites the 
>>> bonito equivalent, I'm wondering if the mv_pirq array is actually needed 
>>> at all and whether it may just be a debugging aid? Certainly it makes 
>>> things simpler to just route everything to the VIA device.
>> 
>> In any case calling pci_bus_irqs in the VIA device would be wrong and 
>> connections 
>
> Why? In general the PCI bus should be managed completely internally and not 
> exposed to the board. This should certainly be the case for the VIA chipsets 
> which can be used across multiple QEMU machines.

The PCI bus belongs to the Mv64361 so can't be managed by VT8231 
internally which is a south bridge and a PCI device. The datasheet clearly 
says it has pins for PCI interrupts that it can route to ISA interrupts 
which is what I've modelled. Other times you want me to model every 
unnecessary detail of a chip with QDev but in this case you'd be OK to 
break things by replacing IRQs of a bus this device is connected to and 
not even owning? I can't follow what you think.

>> should be made here by the board as that correctly models the hardware 
>> separating components and allows different boards to use the chip model in 
>> the future. I remember that I've implemenred connections to MV64361 for 
>> something but I don't remember which guest so as this already works and 
>> models what the schematics say I see no reason not to have it or remove it.
>
> That doesn't really help much I'm afraid - are the extra pirq lines required 
> or not? If so, which guest OS are they being used for? This information 
> should be included in the commit message and a code comment, as if you don't 
> document things like this you risk someone attempting a tidy-up refactoring 
> which will break your guest OS.

My understanding of the schematics is what I've written and it matches the 
code now so I'm happy with that. I'll make sure that no refactoring will 
break guests I care about and removing this could break something while 
leaving doesn't.

Regards,
BALATON Zoltan

Re: [PATCH v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[BALATON Zoltan]

On Mon, 6 Mar 2023, Mark Cave-Ayland wrote:
> On 06/03/2023 12:33, 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>
>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>> ---
>>   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 b0ada9c963..ded5dc2dc9 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);
>> +}
>> +
>
> Can you explain a bit more about how the PCI interrupt lines are connected to 
> both the MV64361 and VT8231? The reason for asking is that I see a similar 
> pattern in the bonito board, but there I can't see how those lines would be 
> used because they can also raise a CPU interrupt, yet it is a different one 
> compared to the 8259.
>
> Given that we know from Bernhard's tests that the fuloong2e board works with 
> pci_bus_irqs() included in via_isa_realize() which overwrites the bonito 
> equivalent, I'm wondering if the mv_pirq array is actually needed at all and

Also I'd be cautious of tests on fuloong2e unless it was done with binary 
known to work on real machine as we have found before that those binaries 
for the real machine expect IDE to use IRQ 14/15 like pegasos2 guests but 
the e.g. the default config in Linux would also work with native IRQs as 
documented in the datasheet but that's apparently not how it really works 
on real hardware (proven by binaries written for and tested on real 
hardware did not work with the model which followed the datasheet) so we 
had to change that later to match hardware. You may remember this, it was 
found around the time when we tested via-ide with different guests and 
some worked some didn't with the native mode IRQs. This suggests that 
datasheets and tests with code not verified to work on real hardware is 
unreliable (so are PCI standards that the VIA chip apparently does not 
follow). So I'd only trust guests that were running on the real machine.

Regards,
BALATON Zoltan

Re: [PATCH v8 4/6] hw/ppc/pegasos2: Fix PCI interrupt routing

[Mark Cave-Ayland]

On 06/03/2023 22:14, BALATON Zoltan wrote:

>> Can you explain a bit more about how the PCI interrupt lines are connected to both 
>> the MV64361 and VT8231? The reason for asking is that I see a similar pattern in 
>> the bonito board, but there I can't see how those lines would be used because they 
>> can also raise a CPU interrupt, yet it is a different one compared to the 8259.
>>
>> Given that we know from Bernhard's tests that the fuloong2e board works with 
>> pci_bus_irqs() included in via_isa_realize() which overwrites the bonito 
>> equivalent, I'm wondering if the mv_pirq array is actually needed at all and
> 
> Also I'd be cautious of tests on fuloong2e unless it was done with binary known to 
> work on real machine as we have found before that those binaries for the real machine 
> expect IDE to use IRQ 14/15 like pegasos2 guests but the e.g. the default config in 
> Linux would also work with native IRQs as documented in the datasheet but that's 
> apparently not how it really works on real hardware (proven by binaries written for 
> and tested on real hardware did not work with the model which followed the datasheet) 
> so we had to change that later to match hardware. You may remember this, it was found 
> around the time when we tested via-ide with different guests and some worked some 
> didn't with the native mode IRQs. This suggests that datasheets and tests with code 
> not verified to work on real hardware is unreliable (so are PCI standards that the 
> VIA chip apparently does not follow). So I'd only trust guests that were running on 
> the real machine.

This may have been true when these discussions occurred a couple of years ago, but 
things are different now. I'm now confident that there is no such thing as a "half 
native" mode, and in fact the confusing behaviour being exhibited was because the 
code was trying to work around the fact that the VIA device was missing PCI IRQ routing.

Can you provide details of the kernel, initrd and rootfs that you use to test the 
fuloong2e board?


ATB,

Mark.

[PATCH v8 5/6] 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>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Tested-by: Rene Engel <ReneEngel80@emailn.de>
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 v8 6/6] 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>
Reviewed-by: Volker Rümelin <vr_qemu@t-online.de>
Tested-by: Rene Engel <ReneEngel80@emailn.de>
---
 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 3383ab7e2d..1572966f1a 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -554,7 +554,7 @@ struct ViaISAState {
     PCIIDEState ide;
     UHCIState uhci[2];
     ViaPMState pm;
-    PCIDevice ac97;
+    ViaAC97State ac97;
     PCIDevice mc97;
 };
 
diff --git a/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