[PATCH 0/3] malta: Move PCI interrupt handling from gt64xxx to piix4

[PATCH 0/3] malta: Move PCI interrupt handling from gt64xxx to piix4

[Bernhard Beschow]

Hi,

first-time contributor here. Inspired by an article in LWN [1] I figured I'd
get my hands dirty with QEMU development. According to the article my goal is
to eliminate some "accidental complexity".

While studying the code I noticed some (accidental?) differences between piix3
and piix4 where the PCI interrupts are handled. Moreover, I noticed presence of
global variables in piix4 which probably constitute a limitation of QOM's idea
of configuration-driven machine creation. By applying piix3 concepts, i.e.
moving the interrupt handling from gt64xxx to piix4, it's possible to both
eliminate the differences and resolve the global variables.

The patch series is structured as follows: Patch 1 eliminates the differences,
patch 3 resolves the global variables. Patch 2 is a preparation for patch 3.
Some of my further comments regarding the patches are:

Patch 1:
* pci_slot_get_pirq() looks quite malta-specific. Neither gt64xxx nor piix4
  seem to be the perfect fit. So I moved it to piix4, analogous to piix3.
* The i8259 property moved from MaltaState to PIIX4State looks quite redundant
  to the isa property. Could isa be used instead, eliminating i8259?

Patch 2:
* Besides piix4, there were four further cases where the PIC array was passed
  as the opaque parameter to the pci_map_irq_fn's. AFAICS in all other cases
  the DeviceState is passed instead. With this patch, consistency is
  esablished.
* Passing PIIX4State to piix4_set_irq() paves the way for eliminating all
  global variables left in piix4.c (see patch 3).

Comments welcome.

Cheers
Bernhard

[1] https://lwn.net/Articles/872321/

Bernhard Beschow (3):
  malta: Move PCI interrupt handling from gt64xxx to piix4
  pci: Always pass own DeviceState to pci_map_irq_fn's
  isa/piix4: Resolve global variables

 hw/isa/piix4.c                | 62 ++++++++++++++++++++++++++++++++---
 hw/mips/gt64xxx_pci.c         | 62 +++--------------------------------
 hw/mips/malta.c               |  6 +---
 hw/pci-host/sh_pci.c          |  6 ++--
 hw/pci-host/versatile.c       |  6 ++--
 hw/ppc/ppc440_pcix.c          |  6 ++--
 hw/ppc/ppc4xx_pci.c           |  6 ++--
 include/hw/mips/mips.h        |  2 +-
 include/hw/southbridge/piix.h |  2 --
 9 files changed, 77 insertions(+), 81 deletions(-)

-- 
2.34.1

[PATCH 1/3] malta: Move PCI interrupt handling from gt64xxx 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         | 58 +++++++++++++++++++++++++++++++++++++++
 hw/mips/gt64xxx_pci.c  | 62 ++++--------------------------------------
 hw/mips/malta.c        |  6 +---
 include/hw/mips/mips.h |  2 +-
 4 files changed, 65 insertions(+), 63 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0fe7b69bc4..5a86308689 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,30 @@ struct PIIX4State {
 
 OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
 
+static int pci_irq_levels[4];
+
+static void piix4_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;
+
+    /* 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_irq_levels[i];
+            }
+        }
+        qemu_set_irq(pic[pic_irq], pic_level);
+    }
+}
+
 static void piix4_isa_reset(DeviceState *dev)
 {
     PIIX4State *d = PIIX4_PCI_DEVICE(dev);
@@ -248,8 +273,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 +308,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 +323,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 c7480bd019..9e23e32eff 100644
--- a/hw/mips/gt64xxx_pci.c
+++ b/hw/mips/gt64xxx_pci.c
@@ -981,56 +981,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 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;
-
-    /* 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_irq_levels[i];
-            }
-        }
-        qemu_set_irq(pic[pic_irq], pic_level);
-    }
-}
-
-
 static void gt64120_reset(DeviceState *dev)
 {
     GT64120State *s = GT64120_PCI_HOST_BRIDGE(dev);
@@ -1207,7 +1157,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;
@@ -1218,12 +1168,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.34.1

[PATCH 2/3] pci: Always pass own DeviceState to pci_map_irq_fn's

[Bernhard Beschow]

Passing own DeviceState rather than just the IRQs allows for resolving
global variables.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c          | 6 +++---
 hw/pci-host/sh_pci.c    | 6 +++---
 hw/pci-host/versatile.c | 6 +++---
 hw/ppc/ppc440_pcix.c    | 6 +++---
 hw/ppc/ppc4xx_pci.c     | 6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 5a86308689..a31e9714cf 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -60,7 +60,7 @@ static int pci_irq_levels[4];
 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;
 
     pci_irq_levels[irq_num] = level;
 
@@ -75,7 +75,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
                 pic_level |= pci_irq_levels[i];
             }
         }
-        qemu_set_irq(pic[pic_irq], pic_level);
+        qemu_set_irq(s->i8259[pic_irq], pic_level);
     }
 }
 
@@ -323,7 +323,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);
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 4);
 
     for (int i = 0; i < ISA_NUM_IRQS; i++) {
         s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
diff --git a/hw/pci-host/sh_pci.c b/hw/pci-host/sh_pci.c
index 719d6ca2a6..ae0aa462b3 100644
--- a/hw/pci-host/sh_pci.c
+++ b/hw/pci-host/sh_pci.c
@@ -111,9 +111,9 @@ static int sh_pci_map_irq(PCIDevice *d, int irq_num)
 
 static void sh_pci_set_irq(void *opaque, int irq_num, int level)
 {
-    qemu_irq *pic = opaque;
+    SHPCIState *s = opaque;
 
-    qemu_set_irq(pic[irq_num], level);
+    qemu_set_irq(s->irq[irq_num], level);
 }
 
 static void sh_pci_device_realize(DeviceState *dev, Error **errp)
@@ -128,7 +128,7 @@ static void sh_pci_device_realize(DeviceState *dev, Error **errp)
     }
     phb->bus = pci_register_root_bus(dev, "pci",
                                      sh_pci_set_irq, sh_pci_map_irq,
-                                     s->irq,
+                                     s,
                                      get_system_memory(),
                                      get_system_io(),
                                      PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index f66384fa02..5fbcb72d7d 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -362,9 +362,9 @@ static int pci_vpb_rv_map_irq(PCIDevice *d, int irq_num)
 
 static void pci_vpb_set_irq(void *opaque, int irq_num, int level)
 {
-    qemu_irq *pic = opaque;
+    PCIVPBState *s = opaque;
 
-    qemu_set_irq(pic[irq_num], level);
+    qemu_set_irq(s->irq[irq_num], level);
 }
 
 static void pci_vpb_reset(DeviceState *d)
@@ -422,7 +422,7 @@ static void pci_vpb_realize(DeviceState *dev, Error **errp)
         mapfn = pci_vpb_map_irq;
     }
 
-    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s->irq, 4);
+    pci_bus_irqs(&s->pci_bus, pci_vpb_set_irq, mapfn, s, 4);
 
     /* Our memory regions are:
      * 0 : our control registers
diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index 788d25514a..291c1bfbe7 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -431,14 +431,14 @@ static int ppc440_pcix_map_irq(PCIDevice *pci_dev, int irq_num)
 
 static void ppc440_pcix_set_irq(void *opaque, int irq_num, int level)
 {
-    qemu_irq *pci_irq = opaque;
+    PPC440PCIXState *s = opaque;
 
     trace_ppc440_pcix_set_irq(irq_num);
     if (irq_num < 0) {
         error_report("%s: PCI irq %d", __func__, irq_num);
         return;
     }
-    qemu_set_irq(*pci_irq, level);
+    qemu_set_irq(s->irq, level);
 }
 
 static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
@@ -492,7 +492,7 @@ static void ppc440_pcix_realize(DeviceState *dev, Error **errp)
     sysbus_init_irq(sbd, &s->irq);
     memory_region_init(&s->busmem, OBJECT(dev), "pci bus memory", UINT64_MAX);
     h->bus = pci_register_root_bus(dev, NULL, ppc440_pcix_set_irq,
-                         ppc440_pcix_map_irq, &s->irq, &s->busmem,
+                         ppc440_pcix_map_irq, s, &s->busmem,
                          get_system_io(), PCI_DEVFN(0, 0), 1, TYPE_PCI_BUS);
 
     s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0), "ppc4xx-host-bridge");
diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
index 5df97e6d15..f6718746a1 100644
--- a/hw/ppc/ppc4xx_pci.c
+++ b/hw/ppc/ppc4xx_pci.c
@@ -256,11 +256,11 @@ static int ppc4xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
 
 static void ppc4xx_pci_set_irq(void *opaque, int irq_num, int level)
 {
-    qemu_irq *pci_irqs = opaque;
+    PPC4xxPCIState *s = opaque;
 
     trace_ppc4xx_pci_set_irq(irq_num);
     assert(irq_num >= 0 && irq_num < PPC4xx_PCI_NUM_DEVS);
-    qemu_set_irq(pci_irqs[irq_num], level);
+    qemu_set_irq(s->irq[irq_num], level);
 }
 
 static const VMStateDescription vmstate_pci_master_map = {
@@ -319,7 +319,7 @@ static void ppc4xx_pcihost_realize(DeviceState *dev, Error **errp)
     }
 
     b = pci_register_root_bus(dev, NULL, ppc4xx_pci_set_irq,
-                              ppc4xx_pci_map_irq, s->irq, get_system_memory(),
+                              ppc4xx_pci_map_irq, s, get_system_memory(),
                               get_system_io(), 0, ARRAY_SIZE(s->irq),
                               TYPE_PCI_BUS);
     h->bus = b;
-- 
2.34.1

[PATCH 3/3] isa/piix4: Resolve global variables

[Bernhard Beschow]

Now that piix4_set_irq's opaque parameter references own PIIX4State,
piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/piix4.c                | 22 +++++++++-------------
 include/hw/southbridge/piix.h |  2 --
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index a31e9714cf..964e09cf7f 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -39,14 +39,14 @@
 #include "sysemu/runstate.h"
 #include "qom/object.h"
 
-PCIDevice *piix4_dev;
-
 struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa;
     qemu_irq i8259[ISA_NUM_IRQS];
 
+    int pci_irq_levels[PIIX_NUM_PIRQS];
+
     RTCState rtc;
     /* Reset Control Register */
     MemoryRegion rcr_mem;
@@ -55,24 +55,22 @@ struct PIIX4State {
 
 OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
 
-static int pci_irq_levels[4];
-
 static void piix4_set_irq(void *opaque, int irq_num, int level)
 {
     int i, pic_irq, pic_level;
     PIIX4State *s = opaque;
 
-    pci_irq_levels[irq_num] = level;
+    s->pci_irq_levels[irq_num] = level;
 
     /* 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) {
+    pic_irq = s->dev.config[PIIX_PIRQCA + irq_num];
+    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++) {
-            if (pic_irq == piix4_dev->config[PIIX_PIRQCA + i]) {
-                pic_level |= pci_irq_levels[i];
+        for (i = 0; i < PIIX_NUM_PIRQS; i++) {
+            if (pic_irq == s->dev.config[PIIX_PIRQCA + i]) {
+                pic_level |= s->pci_irq_levels[i];
             }
         }
         qemu_set_irq(s->i8259[pic_irq], pic_level);
@@ -223,8 +221,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)
@@ -323,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, 4);
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
 
     for (int i = 0; i < ISA_NUM_IRQS; i++) {
         s->i8259[i] = qdev_get_gpio_in_named(dev, "isa", i);
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.34.1

Re: [PATCH 0/3] malta: Move PCI interrupt handling from gt64xxx to piix4

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 12/1/22 22:36, Bernhard Beschow wrote:
> Hi,
> 
> first-time contributor here. Inspired by an article in LWN [1] I figured I'd
> get my hands dirty with QEMU development. According to the article my goal is
> to eliminate some "accidental complexity".
> 
> While studying the code I noticed some (accidental?) differences between piix3
> and piix4 where the PCI interrupts are handled. Moreover, I noticed presence of
> global variables in piix4 which probably constitute a limitation of QOM's idea
> of configuration-driven machine creation. By applying piix3 concepts, i.e.
> moving the interrupt handling from gt64xxx to piix4, it's possible to both
> eliminate the differences and resolve the global variables.
> 
> The patch series is structured as follows: Patch 1 eliminates the differences,
> patch 3 resolves the global variables. Patch 2 is a preparation for patch 3.
> Some of my further comments regarding the patches are:
> 
> Patch 1:
> * pci_slot_get_pirq() looks quite malta-specific. Neither gt64xxx nor piix4
>    seem to be the perfect fit. So I moved it to piix4, analogous to piix3.
> * The i8259 property moved from MaltaState to PIIX4State looks quite redundant
>    to the isa property. Could isa be used instead, eliminating i8259?
> 
> Patch 2:
> * Besides piix4, there were four further cases where the PIC array was passed
>    as the opaque parameter to the pci_map_irq_fn's. AFAICS in all other cases
>    the DeviceState is passed instead. With this patch, consistency is
>    esablished.
> * Passing PIIX4State to piix4_set_irq() paves the way for eliminating all
>    global variables left in piix4.c (see patch 3).
> 
> Comments welcome.
> 
> Cheers
> Bernhard
> 
> [1] https://lwn.net/Articles/872321/
> 
> Bernhard Beschow (3):
>    malta: Move PCI interrupt handling from gt64xxx to piix4
>    pci: Always pass own DeviceState to pci_map_irq_fn's
>    isa/piix4: Resolve global variables

Did you forget to sent the patches?

Re: [PATCH 0/3] malta: Move PCI interrupt handling from gt64xxx to piix4

[Bernhard Beschow]

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

Hi Philippe,

On Thu, Jan 13, 2022 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Bernhard,
>
> On 12/1/22 22:36, Bernhard Beschow wrote:
> > Hi,
> >
> > first-time contributor here. Inspired by an article in LWN [1] I figured
> I'd
> > get my hands dirty with QEMU development. According to the article my
> goal is
> > to eliminate some "accidental complexity".
> >
> > While studying the code I noticed some (accidental?) differences between
> piix3
> > and piix4 where the PCI interrupts are handled. Moreover, I noticed
> presence of
> > global variables in piix4 which probably constitute a limitation of
> QOM's idea
> > of configuration-driven machine creation. By applying piix3 concepts,
> i.e.
> > moving the interrupt handling from gt64xxx to piix4, it's possible to
> both
> > eliminate the differences and resolve the global variables.
> >
> > The patch series is structured as follows: Patch 1 eliminates the
> differences,
> > patch 3 resolves the global variables. Patch 2 is a preparation for
> patch 3.
> > Some of my further comments regarding the patches are:
> >
> > Patch 1:
> > * pci_slot_get_pirq() looks quite malta-specific. Neither gt64xxx nor
> piix4
> >    seem to be the perfect fit. So I moved it to piix4, analogous to
> piix3.
> > * The i8259 property moved from MaltaState to PIIX4State looks quite
> redundant
> >    to the isa property. Could isa be used instead, eliminating i8259?
> >
> > Patch 2:
> > * Besides piix4, there were four further cases where the PIC array was
> passed
> >    as the opaque parameter to the pci_map_irq_fn's. AFAICS in all other
> cases
> >    the DeviceState is passed instead. With this patch, consistency is
> >    esablished.
> > * Passing PIIX4State to piix4_set_irq() paves the way for eliminating all
> >    global variables left in piix4.c (see patch 3).
> >
> > Comments welcome.
> >
> > Cheers
> > Bernhard
> >
> > [1] https://lwn.net/Articles/872321/
> >
> > Bernhard Beschow (3):
> >    malta: Move PCI interrupt handling from gt64xxx to piix4
> >    pci: Always pass own DeviceState to pci_map_irq_fn's
> >    isa/piix4: Resolve global variables
>
> Did you forget to sent the patches?
>

I can see my patches in-reply-to my cover letter here [1]. Do I miss
something?

[1]  https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg02786.html

[-- Attachment #2: Type: text/html, Size: 3166 bytes --]

Re: [PATCH 0/3] malta: Move PCI interrupt handling from gt64xxx to piix4

[Philippe Mathieu-Daudé]

On 13/1/22 12:22, Bernhard Beschow wrote:
> Hi Philippe,
> 
> On Thu, Jan 13, 2022 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org 
> <mailto:f4bug@amsat.org>> wrote:
> 
>      > Bernhard Beschow (3):
>      >    malta: Move PCI interrupt handling from gt64xxx to piix4
>      >    pci: Always pass own DeviceState to pci_map_irq_fn's
>      >    isa/piix4: Resolve global variables
> 
>     Did you forget to sent the patches?
> 
> 
> I can see my patches in-reply-to my cover letter here [1]. Do I miss 
> something?
> 
> [1] 
> https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg02786.html 
> <https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg02786.html>

I should have checked there first. I found the patches in my SPAM box,
apparently due to "SPF=SOFTFAIL" (no clue...):

Authentication-Results: mx.google.com;
        dkim=pass header.i=@gmail.com header.s=20210112 header.b="Sf/DBOt0";
        spf=softfail (google.com: domain of transitioning 
shentey@gmail.com does not designate 172.105.152.211 as permitted 
sender) smtp.mailfrom=shentey@gmail.com;
        dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com

Re: [PATCH 2/3] pci: Always pass own DeviceState to pci_map_irq_fn's

[Peter Maydell]

On Wed, 12 Jan 2022 at 21:36, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Passing own DeviceState rather than just the IRQs allows for resolving
> global variables.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

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

thanks
-- PMM

Re: [PATCH 3/3] isa/piix4: Resolve global variables

[Peter Maydell]

On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Now that piix4_set_irq's opaque parameter references own PIIX4State,
> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/isa/piix4.c                | 22 +++++++++-------------
>  include/hw/southbridge/piix.h |  2 --
>  2 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index a31e9714cf..964e09cf7f 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -39,14 +39,14 @@
>  #include "sysemu/runstate.h"
>  #include "qom/object.h"
>
> -PCIDevice *piix4_dev;
> -
>  struct PIIX4State {
>      PCIDevice dev;
>      qemu_irq cpu_intr;
>      qemu_irq *isa;
>      qemu_irq i8259[ISA_NUM_IRQS];
>
> +    int pci_irq_levels[PIIX_NUM_PIRQS];
> +

I wondered how we were migrating this state, and the answer
seems to be that we aren't (and weren't before, when it was
a global variable, so this is a pre-existing bug).

Does the malta platform support migration save/load?
We should probably add this field to the vmstate struct
(which will be a migration compatibility break, which is OK
as the malta board isn't versioned.)

-- PMM

Re: [PATCH 1/3] malta: Move PCI interrupt handling from gt64xxx to piix4

[Philippe Mathieu-Daudé via]

On 12/1/22 22:36, 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         | 58 +++++++++++++++++++++++++++++++++++++++
>   hw/mips/gt64xxx_pci.c  | 62 ++++--------------------------------------
>   hw/mips/malta.c        |  6 +---
>   include/hw/mips/mips.h |  2 +-
>   4 files changed, 65 insertions(+), 63 deletions(-)

Great!

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

Re: [PATCH 2/3] pci: Always pass own DeviceState to pci_map_irq_fn's

[Philippe Mathieu-Daudé via]

On 12/1/22 22:36, Bernhard Beschow wrote:
> Passing own DeviceState rather than just the IRQs allows for resolving
> global variables.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c          | 6 +++---
>   hw/pci-host/sh_pci.c    | 6 +++---
>   hw/pci-host/versatile.c | 6 +++---
>   hw/ppc/ppc440_pcix.c    | 6 +++---
>   hw/ppc/ppc4xx_pci.c     | 6 +++---
>   5 files changed, 15 insertions(+), 15 deletions(-)

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

Re: [PATCH 3/3] isa/piix4: Resolve global variables

[Philippe Mathieu-Daudé via]

On 12/1/22 22:36, Bernhard Beschow wrote:
> Now that piix4_set_irq's opaque parameter references own PIIX4State,
> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c                | 22 +++++++++-------------
>   include/hw/southbridge/piix.h |  2 --
>   2 files changed, 9 insertions(+), 15 deletions(-)

Finally!

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

Re: [PATCH 3/3] isa/piix4: Resolve global variables

[Philippe Mathieu-Daudé via]

On 14/1/22 14:36, Peter Maydell wrote:
> On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> Now that piix4_set_irq's opaque parameter references own PIIX4State,
>> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/isa/piix4.c                | 22 +++++++++-------------
>>   include/hw/southbridge/piix.h |  2 --
>>   2 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index a31e9714cf..964e09cf7f 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -39,14 +39,14 @@
>>   #include "sysemu/runstate.h"
>>   #include "qom/object.h"
>>
>> -PCIDevice *piix4_dev;
>> -
>>   struct PIIX4State {
>>       PCIDevice dev;
>>       qemu_irq cpu_intr;
>>       qemu_irq *isa;
>>       qemu_irq i8259[ISA_NUM_IRQS];
>>
>> +    int pci_irq_levels[PIIX_NUM_PIRQS];
>> +
> 
> I wondered how we were migrating this state, and the answer
> seems to be that we aren't (and weren't before, when it was
> a global variable, so this is a pre-existing bug).

Indeed the migrated VM starts with PCI IRQ levels zeroed.

> Does the malta platform support migration save/load?

Maybe a "best effort" support, but not versioned machines.

> We should probably add this field to the vmstate struct
> (which will be a migration compatibility break, which is OK
> as the malta board isn't versioned.)

Yeah good catch.

Bernhard, do you mind adding it?

Regards,

Phil.

Re: [PATCH 3/3] isa/piix4: Resolve global variables

[BB]

Am 30. Januar 2022 23:53:42 MEZ schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
>On 14/1/22 14:36, Peter Maydell wrote:
>> On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote:
>>>
>>> Now that piix4_set_irq's opaque parameter references own PIIX4State,
>>> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.
>>>
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   hw/isa/piix4.c                | 22 +++++++++-------------
>>>   include/hw/southbridge/piix.h |  2 --
>>>   2 files changed, 9 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>>> index a31e9714cf..964e09cf7f 100644
>>> --- a/hw/isa/piix4.c
>>> +++ b/hw/isa/piix4.c
>>> @@ -39,14 +39,14 @@
>>>   #include "sysemu/runstate.h"
>>>   #include "qom/object.h"
>>>
>>> -PCIDevice *piix4_dev;
>>> -
>>>   struct PIIX4State {
>>>       PCIDevice dev;
>>>       qemu_irq cpu_intr;
>>>       qemu_irq *isa;
>>>       qemu_irq i8259[ISA_NUM_IRQS];
>>>
>>> +    int pci_irq_levels[PIIX_NUM_PIRQS];
>>> +
>> 
>> I wondered how we were migrating this state, and the answer
>> seems to be that we aren't (and weren't before, when it was
>> a global variable, so this is a pre-existing bug).
>
>Indeed the migrated VM starts with PCI IRQ levels zeroed.
>
>> Does the malta platform support migration save/load?
>
>Maybe a "best effort" support, but not versioned machines.
>
>> We should probably add this field to the vmstate struct
>> (which will be a migration compatibility break, which is OK
>> as the malta board isn't versioned.)
>
>Yeah good catch.
>
>Bernhard, do you mind adding it?

Sure, I'll give it a try. Shall I submit a v2 of this patch series then? If so, would it be ok to change the topic of the cover letter or would that be confusing?

Last but not least: How to treat the version_id and the version parameters of the new and existing fields?

Regards,

Bernhard.

Re: [PATCH 3/3] isa/piix4: Resolve global variables

[Michael S. Tsirkin]

On Thu, Feb 10, 2022 at 12:16:34AM +0100, BB wrote:
> Am 30. Januar 2022 23:53:42 MEZ schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>:
> >On 14/1/22 14:36, Peter Maydell wrote:
> >> On Wed, 12 Jan 2022 at 22:02, Bernhard Beschow <shentey@gmail.com> wrote:
> >>>
> >>> Now that piix4_set_irq's opaque parameter references own PIIX4State,
> >>> piix4_dev becomes redundant and pci_irq_levels can be moved into PIIX4State.
> >>>
> >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >>> ---
> >>>   hw/isa/piix4.c                | 22 +++++++++-------------
> >>>   include/hw/southbridge/piix.h |  2 --
> >>>   2 files changed, 9 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> >>> index a31e9714cf..964e09cf7f 100644
> >>> --- a/hw/isa/piix4.c
> >>> +++ b/hw/isa/piix4.c
> >>> @@ -39,14 +39,14 @@
> >>>   #include "sysemu/runstate.h"
> >>>   #include "qom/object.h"
> >>>
> >>> -PCIDevice *piix4_dev;
> >>> -
> >>>   struct PIIX4State {
> >>>       PCIDevice dev;
> >>>       qemu_irq cpu_intr;
> >>>       qemu_irq *isa;
> >>>       qemu_irq i8259[ISA_NUM_IRQS];
> >>>
> >>> +    int pci_irq_levels[PIIX_NUM_PIRQS];
> >>> +
> >> 
> >> I wondered how we were migrating this state, and the answer
> >> seems to be that we aren't (and weren't before, when it was
> >> a global variable, so this is a pre-existing bug).
> >
> >Indeed the migrated VM starts with PCI IRQ levels zeroed.
> >
> >> Does the malta platform support migration save/load?
> >
> >Maybe a "best effort" support, but not versioned machines.
> >
> >> We should probably add this field to the vmstate struct
> >> (which will be a migration compatibility break, which is OK
> >> as the malta board isn't versioned.)
> >
> >Yeah good catch.
> >
> >Bernhard, do you mind adding it?
> 
> Sure, I'll give it a try. Shall I submit a v2 of this patch series then? If so, would it be ok to change the topic of the cover letter or would that be confusing?

It's ok to change the subject of the cover letter.

> Last but not least: How to treat the version_id and the version parameters of the new and existing fields?
> 
> Regards,
> 
> Bernhard.