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

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

[BALATON Zoltan]

v9:
- OK here it is again with a revert. *sigh*

BALATON Zoltan (5):
  hw/display/sm501: Add debug property to control pixman usage
  Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"
  hw/isa/vt82c686: Implement PCI IRQ routing
  hw/ppc/pegasos2: Fix PCI interrupt routing
  hw/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               |  54 +++-
 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, 601 insertions(+), 35 deletions(-)

-- 
2.30.8

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

[BALATON Zoltan]

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

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
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 v9 2/7] 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 v9 3/7] Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"

[BALATON Zoltan]

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

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

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

[PATCH v9 4/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

The real VIA south bridges implement a PCI IRQ router which is configured
by the BIOS or the OS. In order to respect these configurations, QEMU
needs to implement it as well. The real chip may allow routing IRQs from
internal functions independently of PCI interrupts but since guests
usually configute it to a single shared interrupt we don't model that
here for simplicity.

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

Suggested-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>
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 01e0148967..71da316052 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -604,6 +604,46 @@ static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
     qemu_set_irq(s->cpu_intr, level);
 }
 
+static int via_isa_get_pci_irq(const ViaISAState *s, int irq_num)
+{
+    switch (irq_num) {
+    case 0:
+        return s->dev.config[0x55] >> 4;
+    case 1:
+        return s->dev.config[0x56] & 0xf;
+    case 2:
+        return s->dev.config[0x56] >> 4;
+    case 3:
+        return s->dev.config[0x57] >> 4;
+    }
+    return 0;
+}
+
+static void via_isa_set_pci_irq(void *opaque, int irq_num, int level)
+{
+    ViaISAState *s = opaque;
+    PCIBus *bus = pci_get_bus(&s->dev);
+    int i, pic_level, pic_irq = via_isa_get_pci_irq(s, irq_num);
+
+    /* 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);
@@ -627,6 +667,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

[PATCH v9 5/7] hw/ppc/pegasos2: Fix PCI interrupt routing

[BALATON Zoltan]

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

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

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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 7cc375df05..f1650be5ee 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -73,6 +73,8 @@ struct Pegasos2MachineState {
     MachineState parent_obj;
     PowerPCCPU *cpu;
     DeviceState *mv;
+    qemu_irq mv_pirq[PCI_NUM_PINS];
+    qemu_irq via_pirq[PCI_NUM_PINS];
     Vof *vof;
     void *fdt_blob;
     uint64_t kernel_addr;
@@ -95,6 +97,15 @@ static void pegasos2_cpu_reset(void *opaque)
     }
 }
 
+static void pegasos2_pci_irq(void *opaque, int n, int level)
+{
+    Pegasos2MachineState *pm = opaque;
+
+    /* PCI interrupt lines are connected to both MV64361 and VT8231 */
+    qemu_set_irq(pm->mv_pirq[n], level);
+    qemu_set_irq(pm->via_pirq[n], level);
+}
+
 static void pegasos2_init(MachineState *machine)
 {
     Pegasos2MachineState *pm = PEGASOS2_MACHINE(machine);
@@ -106,7 +117,7 @@ static void pegasos2_init(MachineState *machine)
     I2CBus *i2c_bus;
     const char *fwname = machine->firmware ?: PROM_FILENAME;
     char *filename;
-    int sz;
+    int i, sz;
     uint8_t *spd_data;
 
     /* init CPU */
@@ -156,11 +167,18 @@ static void pegasos2_init(MachineState *machine)
     /* Marvell Discovery II system controller */
     pm->mv = DEVICE(sysbus_create_simple(TYPE_MV64361, -1,
                           qdev_get_gpio_in(DEVICE(pm->cpu), PPC6xx_INPUT_INT)));
+    for (i = 0; i < PCI_NUM_PINS; i++) {
+        pm->mv_pirq[i] = qdev_get_gpio_in_named(pm->mv, "gpp", 12 + i);
+    }
     pci_bus = mv64361_get_pci_bus(pm->mv, 1);
+    pci_bus_irqs(pci_bus, pegasos2_pci_irq, pm, PCI_NUM_PINS);
 
     /* VIA VT8231 South Bridge (multifunction PCI device) */
     via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0),
                                                  true, TYPE_VT8231_ISA));
+    for (i = 0; i < PCI_NUM_PINS; i++) {
+        pm->via_pirq[i] = qdev_get_gpio_in_named(DEVICE(via), "pirq", i);
+    }
     object_property_add_alias(OBJECT(machine), "rtc-time",
                               object_resolve_path_component(via, "rtc"),
                               "date");
@@ -267,6 +285,12 @@ static void pegasos2_machine_reset(MachineState *machine, ShutdownCause reason)
                               PCI_INTERRUPT_LINE, 2, 0x9);
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
                               0x50, 1, 0x2);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x55, 1, 0x90);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x56, 1, 0x99);
+    pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 0) << 8) |
+                              0x57, 1, 0x90);
 
     pegasos2_pci_config_write(pm, 1, (PCI_DEVFN(12, 1) << 8) |
                               PCI_INTERRUPT_LINE, 2, 0x109);
-- 
2.30.8

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

[BALATON Zoltan]

From: Bernhard Beschow <shentey@gmail.com>

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

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
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 v9 7/7] hw/audio/via-ac97: Basic implementation of audio playback

[BALATON Zoltan]

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

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

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

[Daniel Henrique Barboza]

On 3/7/23 08:42, BALATON Zoltan wrote:
> 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*[]) {

Checkpatch will nag about it claiming that we need spaces between '*'. The maintainer
can fix it in-tree though.


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

> +        &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;

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

[Daniel Henrique Barboza]

On 3/7/23 08:42, BALATON Zoltan wrote:
> This partially reverts commit bb98e0f59cde846666d9fddc60ae74ef7ddfca17
> keeping the rename of a state field but reverting other cahanges which
> break interrupts on pegasos2.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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

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

[Peter Maydell]

On Tue, 7 Mar 2023 at 13:36, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
> On 3/7/23 08:42, BALATON Zoltan wrote:
> > @@ -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*[]) {
>
> Checkpatch will nag about it claiming that we need spaces between '*'. The maintainer
> can fix it in-tree though.

checkpatch nags because it fails to correctly parse this piece
of C syntax and thinks the '*' is a multiplication operator;
we should just ignore it in this instance.

thanks
-- PMM

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

[David Woodhouse]

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

On Tue, 2023-03-07 at 10:36 -0300, Daniel Henrique Barboza wrote:
> 
> Checkpatch will nag about it claiming that we need spaces between
> '*'. The maintainer can fix it in-tree though.

I saw that and explicitly didn't care. 3/5 of the existing examples
within the tree look like that one — which is how the docs tell us to
do it — so I figured checkpatch could just be sad.

> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Thanks.

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

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

[BALATON Zoltan]

On Tue, 7 Mar 2023, Daniel Henrique Barboza wrote:
> On 3/7/23 08:42, BALATON Zoltan wrote:
>> 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*[]) {
>
> Checkpatch will nag about it claiming that we need spaces between '*'. The 
> maintainer
> can fix it in-tree though.

I had that before in another patch but was told this is a checkpatch false 
positive I can disregard.

Regards,
BALATON Zoltan

>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
>> +        &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;
>
>

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

[Daniel Henrique Barboza]

On 3/7/23 10:39, David Woodhouse wrote:
> On Tue, 2023-03-07 at 10:36 -0300, Daniel Henrique Barboza wrote:
>>
>> Checkpatch will nag about it claiming that we need spaces between
>> '*'. The maintainer can fix it in-tree though.
> 
> I saw that and explicitly didn't care. 3/5 of the existing examples
> within the tree look like that one — which is how the docs tell us to
> do it — so I figured checkpatch could just be sad.

Regardless of how much we care about checkpatch happiness my r-b stands.

Peter said we can ignore it in this case, so yeah, checkpatch will probably
take a L.


Thanks,


Daniel

> 
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> Thanks.
	

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

[Philippe Mathieu-Daudé]

On 7/3/23 12:42, BALATON Zoltan wrote:
> Add a property to allow disabling pixman and always use the fallbacks
> for different operations which is useful for testing different drawing
> methods or debugging pixman related issues.
> 
> 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(-)


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

FYI, alternatively:

DEFINE_PROP_BIT("x-pixman-fill", SM501PCIState, state.use_pixman, 0, 1),
DEFINE_PROP_BIT("x-pixman-blit", SM501PCIState, state.use_pixman, 1, 1),
DEFINE_PROP_BIT("x-pixman-overlap-blit", SM501PCIState,
                 state.use_pixman, 2, 1),

>       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");
> +}

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

[BALATON Zoltan]

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

On Tue, 7 Mar 2023, Philippe Mathieu-Daudé wrote:
> On 7/3/23 12:42, BALATON Zoltan wrote:
>> Add a property to allow disabling pixman and always use the fallbacks
>> for different operations which is useful for testing different drawing
>> methods or debugging pixman related issues.
>> 
>> 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(-)
>
>
>>   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),
>
> FYI, alternatively:
>
> DEFINE_PROP_BIT("x-pixman-fill", SM501PCIState, state.use_pixman, 0, 1),
> DEFINE_PROP_BIT("x-pixman-blit", SM501PCIState, state.use_pixman, 1, 1),
> DEFINE_PROP_BIT("x-pixman-overlap-blit", SM501PCIState,
>                state.use_pixman, 2, 1),

Hmm, interesting but this would result in harder to use longer command 
lines. In this case when for a debug property expected to be used by 
experienced users a single value is easier to use.

Regards,
BALATON Zoltan

>>       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");
>> +}
>
>
>

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

[Philippe Mathieu-Daudé]

On 7/3/23 12:42, BALATON Zoltan wrote:
> v9:
> - OK here it is again with a revert. *sigh*
> 
> BALATON Zoltan (5):
>    hw/display/sm501: Add debug property to control pixman usage
>    Revert "hw/isa/vt82c686: Remove intermediate IRQ forwarder"
>    hw/isa/vt82c686: Implement PCI IRQ routing
>    hw/ppc/pegasos2: Fix PCI interrupt routing
>    hw/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

Since Daniel reviewed, I'm queuing this via mips-next.

Re: [PATCH v9 4/7] hw/isa/vt82c686: Implement PCI IRQ routing

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 7/3/23 12:42, 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>
> Reviewed-by: Bernhard Beschow <shentey@gmail.com>
> Tested-by: Rene Engel <ReneEngel80@emailn.de>
> ---
>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)


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

Shouldn't this be & 0xf?

(This is why I prefer extract8() over manual bits extraction)

> +    }
> +    return 0;
> +}

Re: [PATCH v9 4/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

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

On Mon, 30 Oct 2023, Philippe Mathieu-Daudé wrote:
> On 7/3/23 12:42, 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>
>> Reviewed-by: Bernhard Beschow <shentey@gmail.com>
>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>> ---
>>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 42 insertions(+)
>
>
>> +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;
>
> Shouldn't this be & 0xf?

No, the INTD value is actually in the high byte of reg 0x57. See e.g. page 
73 in the VT8231 doc Revision 2.32.

> (This is why I prefer extract8() over manual bits extraction)

(I have two problems with deposit/extract. Have to remember which operand 
is what so it's less obvious to me and they have an assert which is not a 
good idea in an interrupt handlet that could be called millinos of times.)

Regards,
BALATON Zoltan

Re: [PATCH v9 4/7] hw/isa/vt82c686: Implement PCI IRQ routing

[BALATON Zoltan]

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

On Mon, 30 Oct 2023, BALATON Zoltan wrote:
> On Mon, 30 Oct 2023, Philippe Mathieu-Daudé wrote:
>> On 7/3/23 12:42, 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>
>>> Reviewed-by: Bernhard Beschow <shentey@gmail.com>
>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>> ---
>>>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>> 
>> 
>>> +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;
>> 
>> Shouldn't this be & 0xf?
>
> No, the INTD value is actually in the high byte of reg 0x57. See e.g. page 73

I mean high bits or nibble but you get what I mean.

> in the VT8231 doc Revision 2.32.
>
>> (This is why I prefer extract8() over manual bits extraction)
>
> (I have two problems with deposit/extract. Have to remember which operand is 
> what so it's less obvious to me and they have an assert which is not a good 
> idea in an interrupt handlet that could be called millinos of times.)

Also "handler" above...

This is a very old message from when it was added, the current series just 
moves this unchanged.

Regards,
BALATON Zoltan

Re: [PATCH v9 4/7] hw/isa/vt82c686: Implement PCI IRQ routing

[Philippe Mathieu-Daudé]

On 30/10/23 10:02, BALATON Zoltan wrote:
> On Mon, 30 Oct 2023, Philippe Mathieu-Daudé wrote:
>> On 7/3/23 12:42, 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>
>>> Reviewed-by: Bernhard Beschow <shentey@gmail.com>
>>> Tested-by: Rene Engel <ReneEngel80@emailn.de>
>>> ---
>>>   hw/isa/vt82c686.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>
>>
>>> +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;
>>
>> Shouldn't this be & 0xf?
> 
> No, the INTD value is actually in the high byte of reg 0x57. See e.g. 
> page 73 in the VT8231 doc Revision 2.32.

Correct (I was looking at rev 0.8 which is incomplete there).

Thanks,

Phil.