[PATCH v3 0/7] malta: Fix PCI IRQ levels to be preserved during migration, cleanup

[PATCH v3 0/7] malta: Fix PCI IRQ levels to be preserved during migration, cleanup

[Bernhard Beschow]

Tested with [1]:

  qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda \
  debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"

It was possible to log in as root and `poweroff` the machine.

Moreover, I ran:

  :$ make check
  Ok:                 569
  Expected Fail:      0
  Fail:               0
  Unexpected Pass:    0
  Skipped:            178
  Timeout:            0

[1] https://people.debian.org/~aurel32/qemu/mips/


v3:
  The migration bug now gets fixed in gt64xxx_pci before any cleanup. As
    suggested by PMM the patch is based on commit e735b55a8c11.
  The code movement patch now moves the already fixed code. I might be a bit
    too conservative here by removing Philippe's Reviewed-By tag.
  As suggested by BALATON Zoltan, the redundant i8259[] attribute is now
    resolved immediately after the code movement. As a side effect, it also
    removes moved code which doesn't adhere to the coding style (local loop
    variable).
  To address BALATON Zoltan's comment and to reduce the number of required
    Reviewed-By's, only piix4_set_irq() is modified to expect own DeviceState
    paremeter. Up to v2, all remaining set_irq() functions were changed this
    way.
  The patch resolving piix4's singleton variable got split into two patches:
    One which resolves the singleton variable and one which replaces magic
    constants. The split patches should be more comprehensible.
  Suggested by BALATON Zoltan, I took a chance to resolve gt64120_register(),
    a method akin to the legacy init functions we're trying to get rid of.

v2:
  isa/piix4: Fix PCI IRQ levels to be preserved in VMState
  isa/piix4: Resolve redundant i8259[] attribute

Bernhard Beschow (7):
  hw/mips/gt64xxx_pci: Fix PCI IRQ levels to be preserved during
    migration
  malta: Move PCI interrupt handling from gt64xxx_pci to piix4
  hw/isa/piix4: Resolve redundant i8259[] attribute
  hw/isa/piix4: Pass PIIX4State as opaque parameter for piix4_set_irq()
  hw/isa/piix4: Resolve global instance variable
  hw/isa/piix4: Replace some magic IRQ constants
  hw/mips/gt64xxx_pci: Resolve gt64120_register()

 hw/isa/piix4.c                | 54 +++++++++++++++++++++--
 hw/mips/gt64xxx_pci.c         | 80 +++--------------------------------
 hw/mips/malta.c               | 17 ++++----
 include/hw/mips/mips.h        |  3 --
 include/hw/southbridge/piix.h |  2 -
 5 files changed, 65 insertions(+), 91 deletions(-)

-- 
2.35.1

[PATCH v3 1/7] hw/mips/gt64xxx_pci: Fix PCI IRQ levels to be preserved during migration

[Bernhard Beschow]

Based on commit e735b55a8c11dd455e31ccd4420e6c9485191d0c:

  piix_pci: eliminate PIIX3State::pci_irq_levels

  PIIX3State::pci_irq_levels are redundant which is already tracked by
  PCIBus layer. So eliminate them.

The IRQ levels in the PCIBus layer are already preserved during
migration. By reusing them and rather than having a redundant implementation
the bug is avoided in the first place.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/mips/gt64xxx_pci.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index c7480bd019..4cbd0911f5 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -1006,14 +1006,11 @@ static int gt64120_pci_map_irq(PCIDevice *pci_dev, int irq_num)
     }
 }
 
-static int pci_irq_levels[4];
-
 static void gt64120_pci_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
     qemu_irq *pic = opaque;
-
-    pci_irq_levels[irq_num] = level;
+    PCIBus *bus = pci_get_bus(piix4_dev);
 
     /* now we change the pic irq level according to the piix irq mappings */
     /* XXX: optimize */
@@ -1023,7 +1020,7 @@ static void gt64120_pci_set_irq(void *opaque, int irq_num, int level)
         pic_level = 0;
         for (i = 0; i < 4; i++) {
             if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
-                pic_level |= pci_irq_levels[i];
+                pic_level |= pci_bus_get_irq_level(bus, i);
             }
         }
         qemu_set_irq(pic[pic_irq], pic_level);
-- 
2.35.1

[PATCH v3 2/7] malta: Move PCI interrupt handling from gt64xxx_pci to piix4

[Bernhard Beschow]

Handling PCI interrupts in piix4 increases cohesion and reduces differences
between piix4 and piix3.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c         | 55 ++++++++++++++++++++++++++++++++++++++
 hw/mips/gt64xxx_pci.c  | 60 ++++--------------------------------------
 hw/mips/malta.c        |  6 +----
 include/hw/mips/mips.h |  2 +-
 4 files changed, 62 insertions(+), 61 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0fe7b69bc4..196b56e69c 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -45,6 +45,7 @@ struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa;
+    qemu_irq i8259[ISA_NUM_IRQS];
 
     RTCState rtc;
     /* Reset Control Register */
@@ -54,6 +55,27 @@ struct PIIX4State {
 
 OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
 
+static void piix4_set_irq(void *opaque, int irq_num, int level)
+{
+    int i, pic_irq, pic_level;
+    qemu_irq *pic = opaque;
+    PCIBus *bus = pci_get_bus(piix4_dev);
+
+    /* now we change the pic irq level according to the piix irq mappings */
+    /* XXX: optimize */
+    pic_irq = piix4_dev->config[PIIX_PIRQCA + irq_num];
+    if (pic_irq < 16) {
+        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
+        pic_level = 0;
+        for (i = 0; i < 4; i++) {
+            if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
+                pic_level |= pci_bus_get_irq_level(bus, i);
+            }
+        }
+        qemu_set_irq(pic[pic_irq], pic_level);
+    }
+}
+
 static void piix4_isa_reset(DeviceState *dev)
 {
     PIIX4State *d = PIIX4_PCI_DEVICE(dev);
@@ -248,8 +270,34 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
+static int pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
+{
+    int slot;
+
+    slot = PCI_SLOT(pci_dev->devfn);
+
+    switch (slot) {
+    /* PIIX4 USB */
+    case 10:
+        return 3;
+    /* AMD 79C973 Ethernet */
+    case 11:
+        return 1;
+    /* Crystal 4281 Sound */
+    case 12:
+        return 2;
+    /* PCI slot 1 to 4 */
+    case 18 ... 21:
+        return ((slot - 18) + irq_num) & 0x03;
+    /* Unknown device, don't do any translation */
+    default:
+        return irq_num;
+    }
+}
+
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
+    PIIX4State *s;
     PCIDevice *pci;
     DeviceState *dev;
     int devfn = PCI_DEVFN(10, 0);
@@ -257,6 +305,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
     pci = pci_create_simple_multifunction(pci_bus, devfn,  true,
                                           TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
+    s = PIIX4_PCI_DEVICE(pci);
     if (isa_bus) {
         *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
     }
@@ -271,5 +320,11 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
                                NULL, 0, NULL);
     }
 
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->i8259, 4);
+
+    for (int i = 0; i < ISA_NUM_IRQS; i++) {
+        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
+    }
+
     return dev;
 }
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index 4cbd0911f5..eb205d6d70 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -29,7 +29,6 @@
 #include "hw/mips/mips.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
-#include "hw/southbridge/piix.h"
 #include "migration/vmstate.h"
 #include "hw/intc/i8259.h"
 #include "hw/irq.h"
@@ -981,53 +980,6 @@ static const MemoryRegionOps isd_mem_ops = {
     },
 };
 
-static int gt64120_pci_map_irq(PCIDevice *pci_dev, int irq_num)
-{
-    int slot;
-
-    slot = PCI_SLOT(pci_dev->devfn);
-
-    switch (slot) {
-    /* PIIX4 USB */
-    case 10:
-        return 3;
-    /* AMD 79C973 Ethernet */
-    case 11:
-        return 1;
-    /* Crystal 4281 Sound */
-    case 12:
-        return 2;
-    /* PCI slot 1 to 4 */
-    case 18 ... 21:
-        return ((slot - 18) + irq_num) & 0x03;
-    /* Unknown device, don't do any translation */
-    default:
-        return irq_num;
-    }
-}
-
-static void gt64120_pci_set_irq(void *opaque, int irq_num, int level)
-{
-    int i, pic_irq, pic_level;
-    qemu_irq *pic = opaque;
-    PCIBus *bus = pci_get_bus(piix4_dev);
-
-    /* now we change the pic irq level according to the piix irq mappings */
-    /* XXX: optimize */
-    pic_irq = piix4_dev->config[PIIX_PIRQCA + irq_num];
-    if (pic_irq < 16) {
-        /* The pic level is the logical OR of all the PCI irqs mapped to it. */
-        pic_level = 0;
-        for (i = 0; i < 4; i++) {
-            if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
-                pic_level |= pci_bus_get_irq_level(bus, i);
-            }
-        }
-        qemu_set_irq(pic[pic_irq], pic_level);
-    }
-}
-
-
 static void gt64120_reset(DeviceState *dev)
 {
     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
@@ -1204,7 +1156,7 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
                           "gt64120-isd", 0x1000);
 }
 
-PCIBus *gt64120_register(qemu_irq *pic)
+PCIBus *gt64120_register(void)
 {
     GT64120State *d;
     PCIHostState *phb;
@@ -1215,12 +1167,10 @@ PCIBus *gt64120_register(qemu_irq *pic)
     phb = PCI_HOST_BRIDGE(dev);
     memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
     address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");
-    phb->bus = pci_register_root_bus(dev, "pci",
-                                     gt64120_pci_set_irq, gt64120_pci_map_irq,
-                                     pic,
-                                     &d->pci0_mem,
-                                     get_system_io(),
-                                     PCI_DEVFN(18, 0), 4, TYPE_PCI_BUS);
+    phb->bus = pci_root_bus_new(dev, "pci",
+                                &d->pci0_mem,
+                                get_system_io(),
+                                PCI_DEVFN(18, 0), TYPE_PCI_BUS);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index b770b8d367..13254dbc89 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -97,7 +97,6 @@ struct MaltaState {
 
     Clock *cpuclk;
     MIPSCPSState cps;
-    qemu_irq i8259[ISA_NUM_IRQS];
 };
 
 static struct _loaderparams {
@@ -1391,7 +1390,7 @@ void mips_malta_init(MachineState *machine)
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
     /* Northbridge */
-    pci_bus = gt64120_register(s->i8259);
+    pci_bus = gt64120_register();
     /*
      * The whole address space decoded by the GT-64120A doesn't generate
      * exception when accessing invalid memory. Create an empty slot to
@@ -1404,9 +1403,6 @@ void mips_malta_init(MachineState *machine)
 
     /* Interrupt controller */
     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
-    for (int i = 0; i < ISA_NUM_IRQS; i++) {
-        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
-    }
 
     /* generate SPD EEPROM data */
     generate_eeprom_spd(&smbus_eeprom_buf[0 * 256], ram_size);
diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
index 6c9c8805f3..ff88942e63 100644
--- a/include/hw/mips/mips.h
+++ b/include/hw/mips/mips.h
@@ -10,7 +10,7 @@
 #include "exec/memory.h"
 
 /* gt64xxx.c */
-PCIBus *gt64120_register(qemu_irq *pic);
+PCIBus *gt64120_register(void);
 
 /* bonito.c */
 PCIBus *bonito_init(qemu_irq *pic);
-- 
2.35.1

[PATCH v3 3/7] hw/isa/piix4: Resolve redundant i8259[] attribute

[Bernhard Beschow]

This is a follow-up on patch "malta: Move PCI interrupt handling from
gt64xxx_pci to piix4" where i8259[] was moved from MaltaState to
PIIX4State to make the code movement more obvious. However, i8259[]
seems redundant to *isa, so remove it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 196b56e69c..179968b18e 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -45,7 +45,6 @@ struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa;
-    qemu_irq i8259[ISA_NUM_IRQS];
 
     RTCState rtc;
     /* Reset Control Register */
@@ -320,11 +319,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
                                NULL, 0, NULL);
     }
 
-    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->i8259, 4);
-
-    for (int i = 0; i < ISA_NUM_IRQS; i++) {
-        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
-    }
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->isa, 4);
 
     return dev;
 }
-- 
2.35.1

[PATCH v3 4/7] hw/isa/piix4: Pass PIIX4State as opaque parameter for piix4_set_irq()

[Bernhard Beschow]

Passing PIIX4State rather than just the qemu_irq allows for resolving
the global piix4_dev variable.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/piix4.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 179968b18e..caa2002e2c 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -57,7 +57,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
 static void piix4_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
-    qemu_irq *pic = opaque;
+    PIIX4State *s = opaque;
     PCIBus *bus = pci_get_bus(piix4_dev);
 
     /* now we change the pic irq level according to the piix irq mappings */
@@ -71,7 +71,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
                 pic_level |= pci_bus_get_irq_level(bus, i);
             }
         }
-        qemu_set_irq(pic[pic_irq], pic_level);
+        qemu_set_irq(s->isa[pic_irq], pic_level);
     }
 }
 
@@ -319,7 +319,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
                                NULL, 0, NULL);
     }
 
-    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->isa, 4);
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4);
 
     return dev;
 }
-- 
2.35.1

[PATCH v3 5/7] hw/isa/piix4: Resolve global instance variable

[Bernhard Beschow]

Now that piix4_set_irq's opaque parameter references own PIIX4State,
piix4_dev becomes redundant.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/piix4.c                | 10 +++-------
 include/hw/southbridge/piix.h |  2 --
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index caa2002e2c..2e9b5ccada 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -39,8 +39,6 @@
 #include "sysemu/runstate.h"
 #include "qom/object.h"
 
-PCIDevice *piix4_dev;
-
 struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
@@ -58,16 +56,16 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
     PIIX4State *s = opaque;
-    PCIBus *bus = pci_get_bus(piix4_dev);
+    PCIBus *bus = pci_get_bus(&s->dev);
 
     /* now we change the pic irq level according to the piix irq mappings */
     /* XXX: optimize */
-    pic_irq = piix4_dev->config[PIIX_PIRQCA + irq_num];
+    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
     if (pic_irq < 16) {
         /* The pic level is the logical OR of all the PCI irqs mapped to it. */
         pic_level = 0;
         for (i = 0; i < 4; i++) {
-            if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
+            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
                 pic_level |= pci_bus_get_irq_level(bus, i);
             }
         }
@@ -219,8 +217,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
         return;
     }
     isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ);
-
-    piix4_dev = dev;
 }
 
 static void piix4_init(Object *obj)
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 6387f2b612..f63f83e5c6 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -70,8 +70,6 @@ typedef struct PIIXState PIIX3State;
 DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
                          TYPE_PIIX3_PCI_DEVICE)
 
-extern PCIDevice *piix4_dev;
-
 PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
 
 DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
-- 
2.35.1

[PATCH v3 6/7] hw/isa/piix4: Replace some magic IRQ constants

[Bernhard Beschow]

This is a follow-up on patch "malta: Move PCI interrupt handling from
gt64xxx_pci to piix4". gt64xxx_pci used magic constants, and probably
didn't want to use piix4-specific constants. Now that the interrupt
handing resides in piix4, its constants can be used.

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

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 2e9b5ccada..f876c71750 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -61,10 +61,10 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
     /* now we change the pic irq level according to the piix irq mappings */
     /* XXX: optimize */
     pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
-    if (pic_irq < 16) {
+    if (pic_irq < ISA_NUM_IRQS) {
         /* The pic level is the logical OR of all the PCI irqs mapped to it. */
         pic_level = 0;
-        for (i = 0; i < 4; i++) {
+        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
             if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
                 pic_level |= pci_bus_get_irq_level(bus, i);
             }
@@ -315,7 +315,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
                                NULL, 0, NULL);
     }
 
-    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4);
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 
     return dev;
 }
-- 
2.35.1

[PATCH v3 7/7] hw/mips/gt64xxx_pci: Resolve gt64120_register()

[Bernhard Beschow]

Now that gt64120_register() lost its pic parameter, there is an
opportunity to remove it. gt64120_register() is old style by wrapping
qdev API, and the new style is to use qdev directly. So take the
opportunity and modernize the code.

Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/mips/gt64xxx_pci.c  | 21 ++++-----------------
 hw/mips/malta.c        | 13 ++++++++-----
 include/hw/mips/mips.h |  3 ---
 3 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
index eb205d6d70..e0ff1b5566 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -26,7 +26,6 @@
 #include "qapi/error.h"
 #include "qemu/units.h"
 #include "qemu/log.h"
-#include "hw/mips/mips.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
 #include "migration/vmstate.h"
@@ -1151,30 +1150,18 @@ static void gt64120_reset(DeviceState *dev)
 static void gt64120_realize(DeviceState *dev, Error **errp)
 {
     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
+    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
 
     memory_region_init_io(&s->ISD_mem, OBJECT(dev), &isd_mem_ops, s,
                           "gt64120-isd", 0x1000);
-}
-
-PCIBus *gt64120_register(void)
-{
-    GT64120State *d;
-    PCIHostState *phb;
-    DeviceState *dev;
-
-    dev = qdev_new(TYPE_GT64120_PCI_HOST_BRIDGE);
-    d = GT64120_PCI_HOST_BRIDGE(dev);
-    phb = PCI_HOST_BRIDGE(dev);
-    memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
-    address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");
+    memory_region_init(&s->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
+    address_space_init(&s->pci0_mem_as, &s->pci0_mem, "pci0-mem");
     phb->bus = pci_root_bus_new(dev, "pci",
-                                &d->pci0_mem,
+                                &s->pci0_mem,
                                 get_system_io(),
                                 PCI_DEVFN(18, 0), TYPE_PCI_BUS);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
-    return phb->bus;
 }
 
 static void gt64120_pci_realize(PCIDevice *d, Error **errp)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 13254dbc89..16fdaed3db 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -38,6 +38,7 @@
 #include "hw/mips/mips.h"
 #include "hw/mips/cpudevs.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_host.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
 #include "hw/ide.h"
@@ -1230,7 +1231,7 @@ void mips_malta_init(MachineState *machine)
     const size_t smbus_eeprom_size = 8 * 256;
     uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
     uint64_t kernel_entry, bootloader_run_addr;
-    PCIBus *pci_bus;
+    PCIHostState *phb;
     ISABus *isa_bus;
     qemu_irq cbus_irq, i8259_irq;
     I2CBus *smbus;
@@ -1390,7 +1391,9 @@ void mips_malta_init(MachineState *machine)
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
     /* Northbridge */
-    pci_bus = gt64120_register();
+    dev = qdev_new("gt64120");
+    phb = PCI_HOST_BRIDGE(dev);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     /*
      * The whole address space decoded by the GT-64120A doesn't generate
      * exception when accessing invalid memory. Create an empty slot to
@@ -1399,7 +1402,7 @@ void mips_malta_init(MachineState *machine)
     empty_slot_init("GT64120", 0, 0x20000000);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus, &isa_bus, &smbus);
+    dev = piix4_create(phb->bus, &isa_bus, &smbus);
 
     /* Interrupt controller */
     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
@@ -1414,10 +1417,10 @@ void mips_malta_init(MachineState *machine)
     isa_create_simple(isa_bus, TYPE_FDC37M81X_SUPERIO);
 
     /* Network card */
-    network_init(pci_bus);
+    network_init(phb->bus);
 
     /* Optional PCI video card */
-    pci_vga_init(pci_bus);
+    pci_vga_init(phb->bus);
 }
 
 static void mips_malta_instance_init(Object *obj)
diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
index ff88942e63..101799f7d3 100644
--- a/include/hw/mips/mips.h
+++ b/include/hw/mips/mips.h
@@ -9,9 +9,6 @@
 
 #include "exec/memory.h"
 
-/* gt64xxx.c */
-PCIBus *gt64120_register(void);
-
 /* bonito.c */
 PCIBus *bonito_init(qemu_irq *pic);
 
-- 
2.35.1

Re: [PATCH v3 6/7] hw/isa/piix4: Replace some magic IRQ constants

[Philippe Mathieu-Daudé via]

On 16/2/22 23:45, Bernhard Beschow wrote:
> This is a follow-up on patch "malta: Move PCI interrupt handling from
> gt64xxx_pci to piix4". gt64xxx_pci used magic constants, and probably
> didn't want to use piix4-specific constants. Now that the interrupt
> handing resides in piix4, its constants can be used.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 7/7] hw/mips/gt64xxx_pci: Resolve gt64120_register()

[Philippe Mathieu-Daudé via]

On 16/2/22 23:45, Bernhard Beschow wrote:
> Now that gt64120_register() lost its pic parameter, there is an
> opportunity to remove it. gt64120_register() is old style by wrapping
> qdev API, and the new style is to use qdev directly. So take the
> opportunity and modernize the code.
> 
> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/mips/gt64xxx_pci.c  | 21 ++++-----------------
>   hw/mips/malta.c        | 13 ++++++++-----
>   include/hw/mips/mips.h |  3 ---
>   3 files changed, 12 insertions(+), 25 deletions(-)

>   static void gt64120_pci_realize(PCIDevice *d, Error **errp)
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index 13254dbc89..16fdaed3db 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -38,6 +38,7 @@
>   #include "hw/mips/mips.h"
>   #include "hw/mips/cpudevs.h"
>   #include "hw/pci/pci.h"
> +#include "hw/pci/pci_host.h"
>   #include "qemu/log.h"
>   #include "hw/mips/bios.h"
>   #include "hw/ide.h"
> @@ -1230,7 +1231,7 @@ void mips_malta_init(MachineState *machine)
>       const size_t smbus_eeprom_size = 8 * 256;
>       uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
>       uint64_t kernel_entry, bootloader_run_addr;
> -    PCIBus *pci_bus;
> +    PCIHostState *phb;
>       ISABus *isa_bus;
>       qemu_irq cbus_irq, i8259_irq;
>       I2CBus *smbus;
> @@ -1390,7 +1391,9 @@ void mips_malta_init(MachineState *machine)
>       stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
>   
>       /* Northbridge */
> -    pci_bus = gt64120_register();
> +    dev = qdev_new("gt64120");
> +    phb = PCI_HOST_BRIDGE(dev);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

Nice cleanup!

I might move the phb assignation afther the realize() for clarity
(usually we only set qdev properties between qdev_new and the
realize).

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 3/7] hw/isa/piix4: Resolve redundant i8259[] attribute

[Philippe Mathieu-Daudé via]

On 16/2/22 23:45, Bernhard Beschow wrote:
> This is a follow-up on patch "malta: Move PCI interrupt handling from
> gt64xxx_pci to piix4" where i8259[] was moved from MaltaState to
> PIIX4State to make the code movement more obvious. However, i8259[]
> seems redundant to *isa, so remove it.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 2/7] malta: Move PCI interrupt handling from gt64xxx_pci to piix4

[Philippe Mathieu-Daudé via]

On 16/2/22 23:45, Bernhard Beschow wrote:
> Handling PCI interrupts in piix4 increases cohesion and reduces differences
> between piix4 and piix3.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c         | 55 ++++++++++++++++++++++++++++++++++++++
>   hw/mips/gt64xxx_pci.c  | 60 ++++--------------------------------------
>   hw/mips/malta.c        |  6 +----
>   include/hw/mips/mips.h |  2 +-
>   4 files changed, 62 insertions(+), 61 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v3 7/7] hw/mips/gt64xxx_pci: Resolve gt64120_register()

[BALATON Zoltan]

On Wed, 16 Feb 2022, Bernhard Beschow wrote:
> Now that gt64120_register() lost its pic parameter, there is an
> opportunity to remove it. gt64120_register() is old style by wrapping
> qdev API, and the new style is to use qdev directly. So take the
> opportunity and modernize the code.
>
> Suggested-by: BALATON Zoltan <balaton@eik.bme.hu>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/mips/gt64xxx_pci.c  | 21 ++++-----------------
> hw/mips/malta.c        | 13 ++++++++-----
> include/hw/mips/mips.h |  3 ---
> 3 files changed, 12 insertions(+), 25 deletions(-)

Very good but maybe it could be simplified even further, see below.

> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> index eb205d6d70..e0ff1b5566 100644
> --- a/hw/mips/gt64xxx_pci.c
> +++ b/hw/mips/gt64xxx_pci.c
> @@ -26,7 +26,6 @@
> #include "qapi/error.h"
> #include "qemu/units.h"
> #include "qemu/log.h"
> -#include "hw/mips/mips.h"
> #include "hw/pci/pci.h"
> #include "hw/pci/pci_host.h"
> #include "migration/vmstate.h"
> @@ -1151,30 +1150,18 @@ static void gt64120_reset(DeviceState *dev)
> static void gt64120_realize(DeviceState *dev, Error **errp)
> {
>     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
> +    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>
>     memory_region_init_io(&s->ISD_mem, OBJECT(dev), &isd_mem_ops, s,
>                           "gt64120-isd", 0x1000);
> -}
> -
> -PCIBus *gt64120_register(void)
> -{
> -    GT64120State *d;
> -    PCIHostState *phb;
> -    DeviceState *dev;
> -
> -    dev = qdev_new(TYPE_GT64120_PCI_HOST_BRIDGE);
> -    d = GT64120_PCI_HOST_BRIDGE(dev);
> -    phb = PCI_HOST_BRIDGE(dev);
> -    memory_region_init(&d->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
> -    address_space_init(&d->pci0_mem_as, &d->pci0_mem, "pci0-mem");
> +    memory_region_init(&s->pci0_mem, OBJECT(dev), "pci0-mem", 4 * GiB);
> +    address_space_init(&s->pci0_mem_as, &s->pci0_mem, "pci0-mem");
>     phb->bus = pci_root_bus_new(dev, "pci",
> -                                &d->pci0_mem,
> +                                &s->pci0_mem,
>                                 get_system_io(),
>                                 PCI_DEVFN(18, 0), TYPE_PCI_BUS);
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>
>     pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
> -    return phb->bus;
> }
>
> static void gt64120_pci_realize(PCIDevice *d, Error **errp)
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index 13254dbc89..16fdaed3db 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -38,6 +38,7 @@
> #include "hw/mips/mips.h"
> #include "hw/mips/cpudevs.h"
> #include "hw/pci/pci.h"
> +#include "hw/pci/pci_host.h"
> #include "qemu/log.h"
> #include "hw/mips/bios.h"
> #include "hw/ide.h"
> @@ -1230,7 +1231,7 @@ void mips_malta_init(MachineState *machine)
>     const size_t smbus_eeprom_size = 8 * 256;
>     uint8_t *smbus_eeprom_buf = g_malloc0(smbus_eeprom_size);
>     uint64_t kernel_entry, bootloader_run_addr;
> -    PCIBus *pci_bus;
> +    PCIHostState *phb;
>     ISABus *isa_bus;
>     qemu_irq cbus_irq, i8259_irq;
>     I2CBus *smbus;
> @@ -1390,7 +1391,9 @@ void mips_malta_init(MachineState *machine)
>     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
>
>     /* Northbridge */
> -    pci_bus = gt64120_register();
> +    dev = qdev_new("gt64120");
> +    phb = PCI_HOST_BRIDGE(dev);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

Since no need for setting properties you could do qdev_new and realize in 
one step with sysbus_create_simple and then get the bus from it so no need 
for going through phb either. Something like:

dev = sysbus_create_simple("gt64120", -1. 0);
pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci"));

should work with less changes.

Regards,
BALATON Zoltan

>     /*
>      * The whole address space decoded by the GT-64120A doesn't generate
>      * exception when accessing invalid memory. Create an empty slot to
> @@ -1399,7 +1402,7 @@ void mips_malta_init(MachineState *machine)
>     empty_slot_init("GT64120", 0, 0x20000000);
>
>     /* Southbridge */
> -    dev = piix4_create(pci_bus, &isa_bus, &smbus);
> +    dev = piix4_create(phb->bus, &isa_bus, &smbus);
>
>     /* Interrupt controller */
>     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> @@ -1414,10 +1417,10 @@ void mips_malta_init(MachineState *machine)
>     isa_create_simple(isa_bus, TYPE_FDC37M81X_SUPERIO);
>
>     /* Network card */
> -    network_init(pci_bus);
> +    network_init(phb->bus);
>
>     /* Optional PCI video card */
> -    pci_vga_init(pci_bus);
> +    pci_vga_init(phb->bus);
> }
>
> static void mips_malta_instance_init(Object *obj)
> diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
> index ff88942e63..101799f7d3 100644
> --- a/include/hw/mips/mips.h
> +++ b/include/hw/mips/mips.h
> @@ -9,9 +9,6 @@
>
> #include "exec/memory.h"
>
> -/* gt64xxx.c */
> -PCIBus *gt64120_register(void);
> -
> /* bonito.c */
> PCIBus *bonito_init(qemu_irq *pic);
>
>

Re: [PATCH v3 5/7] hw/isa/piix4: Resolve global instance variable

[Michael S. Tsirkin]

On Wed, Feb 16, 2022 at 11:45:17PM +0100, Bernhard Beschow wrote:
> Now that piix4_set_irq's opaque parameter references own PIIX4State,
> piix4_dev becomes redundant.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/isa/piix4.c                | 10 +++-------
>  include/hw/southbridge/piix.h |  2 --
>  2 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index caa2002e2c..2e9b5ccada 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -39,8 +39,6 @@
>  #include "sysemu/runstate.h"
>  #include "qom/object.h"
>  
> -PCIDevice *piix4_dev;
> -
>  struct PIIX4State {
>      PCIDevice dev;
>      qemu_irq cpu_intr;
> @@ -58,16 +56,16 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>  {
>      int i, pic_irq, pic_level;
>      PIIX4State *s = opaque;
> -    PCIBus *bus = pci_get_bus(piix4_dev);
> +    PCIBus *bus = pci_get_bus(&s->dev);
>  
>      /* now we change the pic irq level according to the piix irq mappings */
>      /* XXX: optimize */
> -    pic_irq = piix4_dev->config[PIIX_PIRQCA + irq_num];
> +    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
>      if (pic_irq < 16) {
>          /* The pic level is the logical OR of all the PCI irqs mapped to it. */
>          pic_level = 0;
>          for (i = 0; i < 4; i++) {
> -            if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
> +            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
>                  pic_level |= pci_bus_get_irq_level(bus, i);
>              }
>          }
> @@ -219,8 +217,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>          return;
>      }
>      isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ);
> -
> -    piix4_dev = dev;
>  }
>  
>  static void piix4_init(Object *obj)
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 6387f2b612..f63f83e5c6 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -70,8 +70,6 @@ typedef struct PIIXState PIIX3State;
>  DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>                           TYPE_PIIX3_PCI_DEVICE)
>  
> -extern PCIDevice *piix4_dev;
> -
>  PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
>  
>  DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
> -- 
> 2.35.1

Re: [PATCH v3 4/7] hw/isa/piix4: Pass PIIX4State as opaque parameter for piix4_set_irq()

[Michael S. Tsirkin]

On Wed, Feb 16, 2022 at 11:45:16PM +0100, Bernhard Beschow wrote:
> Passing PIIX4State rather than just the qemu_irq allows for resolving
> the global piix4_dev variable.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/isa/piix4.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 179968b18e..caa2002e2c 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -57,7 +57,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
>  static void piix4_set_irq(void *opaque, int irq_num, int level)
>  {
>      int i, pic_irq, pic_level;
> -    qemu_irq *pic = opaque;
> +    PIIX4State *s = opaque;
>      PCIBus *bus = pci_get_bus(piix4_dev);
>  
>      /* now we change the pic irq level according to the piix irq mappings */
> @@ -71,7 +71,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>                  pic_level |= pci_bus_get_irq_level(bus, i);
>              }
>          }
> -        qemu_set_irq(pic[pic_irq], pic_level);
> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>      }
>  }
>  
> @@ -319,7 +319,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>                                 NULL, 0, NULL);
>      }
>  
> -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->isa, 4);
> +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4);
>  
>      return dev;
>  }
> -- 
> 2.35.1
> 
> 
> 

Re: [PATCH v3 3/7] hw/isa/piix4: Resolve redundant i8259[] attribute

[Michael S. Tsirkin]

On Wed, Feb 16, 2022 at 11:45:15PM +0100, Bernhard Beschow wrote:
> This is a follow-up on patch "malta: Move PCI interrupt handling from
> gt64xxx_pci to piix4" where i8259[] was moved from MaltaState to
> PIIX4State to make the code movement more obvious. However, i8259[]
> seems redundant to *isa, so remove it.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/isa/piix4.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 196b56e69c..179968b18e 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -45,7 +45,6 @@ struct PIIX4State {
>      PCIDevice dev;
>      qemu_irq cpu_intr;
>      qemu_irq *isa;
> -    qemu_irq i8259[ISA_NUM_IRQS];
>  
>      RTCState rtc;
>      /* Reset Control Register */
> @@ -320,11 +319,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>                                 NULL, 0, NULL);
>      }
>  
> -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->i8259, 4);
> -
> -    for (int i = 0; i < ISA_NUM_IRQS; i++) {
> -        s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
> -    }
> +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s->isa, 4);
>  
>      return dev;
>  }
> -- 
> 2.35.1
> 
> 
> 

Re: [PATCH v3 6/7] hw/isa/piix4: Replace some magic IRQ constants

[Michael S. Tsirkin]

On Wed, Feb 16, 2022 at 11:45:18PM +0100, Bernhard Beschow wrote:
> This is a follow-up on patch "malta: Move PCI interrupt handling from
> gt64xxx_pci to piix4". gt64xxx_pci used magic constants, and probably
> didn't want to use piix4-specific constants. Now that the interrupt
> handing resides in piix4, its constants can be used.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/isa/piix4.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 2e9b5ccada..f876c71750 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -61,10 +61,10 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>      /* now we change the pic irq level according to the piix irq mappings */
>      /* XXX: optimize */
>      pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
> -    if (pic_irq < 16) {
> +    if (pic_irq < ISA_NUM_IRQS) {
>          /* The pic level is the logical OR of all the PCI irqs mapped to it. */
>          pic_level = 0;
> -        for (i = 0; i < 4; i++) {
> +        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
>              if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
>                  pic_level |= pci_bus_get_irq_level(bus, i);
>              }
> @@ -315,7 +315,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>                                 NULL, 0, NULL);
>      }
>  
> -    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4);
> +    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
>  
>      return dev;
>  }
> -- 
> 2.35.1
> 
> 
> 

Re: [PATCH v3 0/7] malta: Fix PCI IRQ levels to be preserved during migration, cleanup

[Michael S. Tsirkin]

On Wed, Feb 16, 2022 at 11:45:12PM +0100, Bernhard Beschow wrote:
> Tested with [1]:
> 
>   qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda \
>   debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"
> 
> It was possible to log in as root and `poweroff` the machine.
> 
> Moreover, I ran:
> 
>   :$ make check
>   Ok:                 569
>   Expected Fail:      0
>   Fail:               0
>   Unexpected Pass:    0
>   Skipped:            178
>   Timeout:            0
> 
> [1] https://people.debian.org/~aurel32/qemu/mips/
> 

Who's merging this? I assume mips guys?

> v3:
>   The migration bug now gets fixed in gt64xxx_pci before any cleanup. As
>     suggested by PMM the patch is based on commit e735b55a8c11.
>   The code movement patch now moves the already fixed code. I might be a bit
>     too conservative here by removing Philippe's Reviewed-By tag.
>   As suggested by BALATON Zoltan, the redundant i8259[] attribute is now
>     resolved immediately after the code movement. As a side effect, it also
>     removes moved code which doesn't adhere to the coding style (local loop
>     variable).
>   To address BALATON Zoltan's comment and to reduce the number of required
>     Reviewed-By's, only piix4_set_irq() is modified to expect own DeviceState
>     paremeter. Up to v2, all remaining set_irq() functions were changed this
>     way.
>   The patch resolving piix4's singleton variable got split into two patches:
>     One which resolves the singleton variable and one which replaces magic
>     constants. The split patches should be more comprehensible.
>   Suggested by BALATON Zoltan, I took a chance to resolve gt64120_register(),
>     a method akin to the legacy init functions we're trying to get rid of.
> 
> v2:
>   isa/piix4: Fix PCI IRQ levels to be preserved in VMState
>   isa/piix4: Resolve redundant i8259[] attribute
> 
> Bernhard Beschow (7):
>   hw/mips/gt64xxx_pci: Fix PCI IRQ levels to be preserved during
>     migration
>   malta: Move PCI interrupt handling from gt64xxx_pci to piix4
>   hw/isa/piix4: Resolve redundant i8259[] attribute
>   hw/isa/piix4: Pass PIIX4State as opaque parameter for piix4_set_irq()
>   hw/isa/piix4: Resolve global instance variable
>   hw/isa/piix4: Replace some magic IRQ constants
>   hw/mips/gt64xxx_pci: Resolve gt64120_register()
> 
>  hw/isa/piix4.c                | 54 +++++++++++++++++++++--
>  hw/mips/gt64xxx_pci.c         | 80 +++--------------------------------
>  hw/mips/malta.c               | 17 ++++----
>  include/hw/mips/mips.h        |  3 --
>  include/hw/southbridge/piix.h |  2 -
>  5 files changed, 65 insertions(+), 91 deletions(-)
> 
> -- 
> 2.35.1
> 
> 
> 

Re: [PATCH v3 1/7] hw/mips/gt64xxx_pci: Fix PCI IRQ levels to be preserved during migration

[Peter Maydell]

On Wed, 16 Feb 2022 at 22:45, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Based on commit e735b55a8c11dd455e31ccd4420e6c9485191d0c:
>
>   piix_pci: eliminate PIIX3State::pci_irq_levels
>
>   PIIX3State::pci_irq_levels are redundant which is already tracked by
>   PCIBus layer. So eliminate them.
>
> The IRQ levels in the PCIBus layer are already preserved during
> migration. By reusing them and rather than having a redundant implementation
> the bug is avoided in the first place.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/mips/gt64xxx_pci.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Ah, this is definitely the best place in the series to make this
fix. I didn't realize we had access to the device pointer here, I
was just looking at the fact that the opaque was the irq array
rather than the device pointer and didn't realize there was a
global floating around with it.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM