[PATCH v2 0/5] malta: Fix PCI IRQ levels to be preserved during migration

[PATCH v2 0/5] malta: Fix PCI IRQ levels to be preserved during migration

[Bernhard Beschow]

The intention behind v1 [1] was just to remove some global variables from
gt64xxx and piix4. During review it was noticed that the Malta board misses to
preserve the PCI IRQ levels during migration. Since the patch series offered an
easy fix v2 was born.

Furthermore, i8259[] was moved to PIIX4State in patch 1. This attribute seems
quite redundant to *isa to me. I therefore attempt to resolve it.

Tested with [2]:

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

It was possible to log in as root and `poweroff` the machine.
  
[1] https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg02786.html
[2] https://people.debian.org/~aurel32/qemu/mips/
  
v2:
  isa/piix4: Fix PCI IRQ levels to be preserved in VMState
  isa/piix4: Resolve redundant i8259[] attribute

Bernhard Beschow (5):
  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
  isa/piix4: Fix PCI IRQ levels to be preserved in VMState
  isa/piix4: Resolve redundant i8259[] attribute

 hw/isa/piix4.c                | 61 +++++++++++++++++++++++++++++++---
 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, 75 insertions(+), 82 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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.35.1

[PATCH v2 2/5] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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.35.1

[PATCH v2 3/5] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 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.35.1

[PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[Bernhard Beschow]

Now that pci_irq_levels[] is part of PIIX4State, the PCI IRQ levels can
be saved and restored via the VMState mechanism. This fixes migrated VMs
to start with PCI IRQ levels zeroed, which is a bug.

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

diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 964e09cf7f..a9322851db 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -45,7 +45,7 @@ struct PIIX4State {
     qemu_irq *isa;
     qemu_irq i8259[ISA_NUM_IRQS];
 
-    int pci_irq_levels[PIIX_NUM_PIRQS];
+    int32_t pci_irq_levels[PIIX_NUM_PIRQS];
 
     RTCState rtc;
     /* Reset Control Register */
@@ -128,12 +128,14 @@ static int piix4_ide_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_piix4 = {
     .name = "PIIX4",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 2,
     .post_load = piix4_ide_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(dev, PIIX4State),
         VMSTATE_UINT8_V(rcr, PIIX4State, 3),
+        VMSTATE_INT32_ARRAY_V(pci_irq_levels, PIIX4State,
+                              PIIX_NUM_PIRQS, 4),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.35.1

[PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

[Bernhard Beschow]

This is a follow-up on patch "malta: Move PCI interrupt handling from
gt64xxx 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 a9322851db..692fa8d1f9 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -43,7 +43,6 @@ struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa;
-    qemu_irq i8259[ISA_NUM_IRQS];
 
     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
 
@@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
                 pic_level |= s->pci_irq_levels[i];
             }
         }
-        qemu_set_irq(s->i8259[pic_irq], pic_level);
+        qemu_set_irq(s->isa[pic_irq], pic_level);
     }
 }
 
@@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 
     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);
-    }
-
     return dev;
 }
-- 
2.35.1

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

[BALATON Zoltan]

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

On Sat, 12 Feb 2022, 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>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Sorry for being late in commenting, I've missed the first round. Apologies 
if this causes a delay or another version.

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

Now that you don't need to pass anything to it, do you still need this 
function? Maybe what it does now could be done in the gt64120 device's 
realize functions (there seems to be at least two: gt64120_realize and 
gt64120_pci_realize but haven't checked which is more appropriate to put 
this init in) or in an init function then you can just create the gt64120 
device in malta.c with qdev_new as is more usual to do in other boards. 
This register function looks like the legacy init functions we're trying 
to get rid of so this seems to be an opportunity to clean this up. This 
could be done in a separate follow up though so may not need to be part of 
this series but may be nice to have.

Regards,.
BALATON Zoltan

>
> /* bonito.c */
> PCIBus *bonito_init(qemu_irq *pic);
>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

[BALATON Zoltan]

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> This is a follow-up on patch "malta: Move PCI interrupt handling from
> gt64xxx 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.

Should this be squashed in patch 1 or at least come after it? Or are there 
some other changes needed before this patch?

Regards,
BALATON Zoltan

> 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 a9322851db..692fa8d1f9 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -43,7 +43,6 @@ struct PIIX4State {
>     PCIDevice dev;
>     qemu_irq cpu_intr;
>     qemu_irq *isa;
> -    qemu_irq i8259[ISA_NUM_IRQS];
>
>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>
> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>                 pic_level |= s->pci_irq_levels[i];
>             }
>         }
> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>     }
> }
>
> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>
>     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);
> -    }
> -
>     return dev;
> }
>

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

[BALATON Zoltan]

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

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Passing own DeviceState rather than just the IRQs allows for resolving
> global variables.

Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?

> 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 +++---
> 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(-)

You don't seem to change any global function definition that reqires this 
change in all these devices so why can't these decide on their own what 
their preferred opaque data is for their set irq function and only change 
piix4? Am I missing something? I'm not opposed to this change but it seems 
to be unnecessary to touch other device implementations for this and may 
just make them more complex for no good reason.

Regards,
BALATON Zoltan

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

Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[BALATON Zoltan]

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

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Now that pci_irq_levels[] is part of PIIX4State, the PCI IRQ levels can
> be saved and restored via the VMState mechanism. This fixes migrated VMs
> to start with PCI IRQ levels zeroed, which is a bug.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/piix4.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 964e09cf7f..a9322851db 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -45,7 +45,7 @@ struct PIIX4State {
>     qemu_irq *isa;
>     qemu_irq i8259[ISA_NUM_IRQS];
>
> -    int pci_irq_levels[PIIX_NUM_PIRQS];
> +    int32_t pci_irq_levels[PIIX_NUM_PIRQS];

Do you really need 32 bits to store a value of 0 or 1? I saw piix3 has 
that too but do we need to do the same here? Could this be just an uint8_t 
array? That's still an overkill to store 4 bits of information but less so 
than with int32_t.

By the way the corresponding member in struct PIIXState in 
include/hw/southbridge/piix.h has a comment saying:

     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];

and only seems to be filled in piix3_pre_save() but never used. So what's 
the point of this then? Maybe piix3 also uses a bitmap to store these 
levels instead? There's a uint64_t pic_levels member above the unused 
array but I haven't checked the implementation.

Regards,
BALATON Zoltan

>
>     RTCState rtc;
>     /* Reset Control Register */
> @@ -128,12 +128,14 @@ static int piix4_ide_post_load(void *opaque, int version_id)
>
> static const VMStateDescription vmstate_piix4 = {
>     .name = "PIIX4",
> -    .version_id = 3,
> +    .version_id = 4,
>     .minimum_version_id = 2,
>     .post_load = piix4_ide_post_load,
>     .fields = (VMStateField[]) {
>         VMSTATE_PCI_DEVICE(dev, PIIX4State),
>         VMSTATE_UINT8_V(rcr, PIIX4State, 3),
> +        VMSTATE_INT32_ARRAY_V(pci_irq_levels, PIIX4State,
> +                              PIIX_NUM_PIRQS, 4),
>         VMSTATE_END_OF_LIST()
>     }
> };
>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

[BB]

Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>> gt64xxx 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.
>
>Should this be squashed in patch 1 or at least come after it? Or are there 
>some other changes needed before this patch?

I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3.

Regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 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 a9322851db..692fa8d1f9 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>     PCIDevice dev;
>>     qemu_irq cpu_intr;
>>     qemu_irq *isa;
>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>
>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>
>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>>                 pic_level |= s->pci_irq_levels[i];
>>             }
>>         }
>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>     }
>> }
>>
>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>
>>     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);
>> -    }
>> -
>>     return dev;
>> }
>>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

[Bernhard Beschow]

Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>> gt64xxx 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.
>
>Should this be squashed in patch 1 or at least come after it? Or are there 
>some other changes needed before this patch?

I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3.

Regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 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 a9322851db..692fa8d1f9 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>     PCIDevice dev;
>>     qemu_irq cpu_intr;
>>     qemu_irq *isa;
>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>
>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>
>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>>                 pic_level |= s->pci_irq_levels[i];
>>             }
>>         }
>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>     }
>> }
>>
>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>
>>     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);
>> -    }
>> -
>>     return dev;
>> }
>>

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

[Bernhard Beschow]

Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> Passing own DeviceState rather than just the IRQs allows for resolving
>> global variables.
>
>Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?

I'm referring to the typedef in pci.h because the patch establishes
consistency across the board.

>> 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 +++---
>> 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(-)
>
>You don't seem to change any global function definition that reqires this
>change in all these devices so why can't these decide on their own what
>their preferred opaque data is for their set irq function and only change
>piix4? Am I missing something? I'm not opposed to this change but it seems
>to be unnecessary to touch other device implementations for this and may
>just make them more complex for no good reason.

This patch is a segway into a direction where the type of the opaque parameter
of the pci_map_irq_fn typedef could be changed from void* to DeviceState* in
order to facilitate the more typesafe QOM casting. I see, though, why this is
confusing in the context of this patch series. I don't have strong feelings for
converting the other devices or not. So I can only change piix4 if desired.

Regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> 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;
>>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

[BALATON Zoltan]

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>>> gt64xxx 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.
>>
>> Should this be squashed in patch 1 or at least come after it? Or are there
>> some other changes needed before this patch?
>
> I didn't want to change the patch order since they've been reviewed already. But yeah, you're right: It makes sense to get this out of the way early in the patch series. I will do a v3.

I had another comment in the bottom of this message, not sure you've 
found that too, I forgot to put a warning that there are more comments 
below here. Changing the patch order or patches according to review is OK 
and adding a new patch between already reviewed ones is OK too then you 
can keep Reviewed-by on patches that did not change.

Regards,
BALATON Zoltan

> Regards,
> Bernhard
>
>>
>> Regards,
>> BALATON Zoltan
>>
>>> 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 a9322851db..692fa8d1f9 100644
>>> --- a/hw/isa/piix4.c
>>> +++ b/hw/isa/piix4.c
>>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>>     PCIDevice dev;
>>>     qemu_irq cpu_intr;
>>>     qemu_irq *isa;
>>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>>
>>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>>
>>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int level)
>>>                 pic_level |= s->pci_irq_levels[i];
>>>             }
>>>         }
>>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>>     }
>>> }
>>>
>>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
>>>
>>>     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);
>>> -    }
>>> -
>>>     return dev;
>>> }
>>>
>
>

Re: [PATCH v2 5/5] isa/piix4: Resolve redundant i8259[] attribute

[BALATON Zoltan]

On Sat, 12 Feb 2022, BALATON Zoltan wrote:
> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> Am 12. Februar 2022 14:19:51 MEZ schrieb BALATON Zoltan 
>> <balaton@eik.bme.hu>:
>>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>>> This is a follow-up on patch "malta: Move PCI interrupt handling from
>>>> gt64xxx 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.
>>> 
>>> Should this be squashed in patch 1 or at least come after it? Or are there
>>> some other changes needed before this patch?
>> 
>> I didn't want to change the patch order since they've been reviewed 
>> already. But yeah, you're right: It makes sense to get this out of the way 
>> early in the patch series. I will do a v3.
>
> I had another comment in the bottom of this message, not sure you've found 
> that too, I forgot to put a warning that there are more comments below here.

I mean at the end of patch 1 not this one, sorry was too quick to send it.

> Changing the patch order or patches according to review is OK and adding a 
> new patch between already reviewed ones is OK too then you can keep 
> Reviewed-by on patches that did not change.
>
> Regards,
> BALATON Zoltan
>
>> Regards,
>> Bernhard
>> 
>>> 
>>> Regards,
>>> BALATON Zoltan
>>> 
>>>> 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 a9322851db..692fa8d1f9 100644
>>>> --- a/hw/isa/piix4.c
>>>> +++ b/hw/isa/piix4.c
>>>> @@ -43,7 +43,6 @@ struct PIIX4State {
>>>>     PCIDevice dev;
>>>>     qemu_irq cpu_intr;
>>>>     qemu_irq *isa;
>>>> -    qemu_irq i8259[ISA_NUM_IRQS];
>>>>
>>>>     int32_t pci_irq_levels[PIIX_NUM_PIRQS];
>>>> 
>>>> @@ -73,7 +72,7 @@ static void piix4_set_irq(void *opaque, int irq_num, 
>>>> int level)
>>>>                 pic_level |= s->pci_irq_levels[i];
>>>>             }
>>>>         }
>>>> -        qemu_set_irq(s->i8259[pic_irq], pic_level);
>>>> +        qemu_set_irq(s->isa[pic_irq], pic_level);
>>>>     }
>>>> }
>>>> 
>>>> @@ -323,9 +322,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
>>>> **isa_bus, I2CBus **smbus)
>>>>
>>>>     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);
>>>> -    }
>>>> -
>>>>     return dev;
>>>> }
>>>> 
>> 
>> 
>

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

[BALATON Zoltan]

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

On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>> Passing own DeviceState rather than just the IRQs allows for resolving
>>> global variables.
>>
>> Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?
>
> I'm referring to the typedef in pci.h because the patch establishes
> consistency across the board.
>
>>> 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 +++---
>>> 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(-)
>>
>> You don't seem to change any global function definition that reqires this
>> change in all these devices so why can't these decide on their own what
>> their preferred opaque data is for their set irq function and only change
>> piix4? Am I missing something? I'm not opposed to this change but it seems
>> to be unnecessary to touch other device implementations for this and may
>> just make them more complex for no good reason.
>
> This patch is a segway into a direction where the type of the opaque parameter
> of the pci_map_irq_fn typedef could be changed from void* to DeviceState* in

I'm still not quite sure what you mean considering these typedefs in 
include/hw/pci/pci.h:

typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);

pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void 
*opaque and is what this patch appears to be changing. Am I looking at the 
wrong typedefs?

> order to facilitate the more typesafe QOM casting. I see, though, why this is
> confusing in the context of this patch series. I don't have strong feelings for
> converting the other devices or not. So I can only change piix4 if desired.

Yes that seems to be an independent cahange so maybe it's better to just 
change piix4 in this series and then have a separate patch for changing 
the void *opaque to DeviceState * independent of this series. But I'm not 
sure that's necessarily a good idea. It may give some more checks but for 
callbacks used internally by device implementations I think it can be 
expected that code that registers the callback also knows what its opaque 
data should be so it does not have to be checked additionally, especially 
in code that may be called frequently. Although in a similar via-ide 
callback I could not measure a big penalty the last time I tried but I 
suspect there still mey be some overhead involving QOM casts (maybe there 
are just bigger bottle necks in ide emulation so it was masked) so I'm not 
sure it's worth the added complexity but if others prefer that I'm not 
that much opposed to it but it's clearer as a separate change anyway.

Regards,
BALATON Zoltan

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

[Peter Maydell]

On Sat, 12 Feb 2022 at 13:27, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
> > Passing own DeviceState rather than just the IRQs allows for resolving
> > global variables.
>
> Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?
>
> > 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 +++---
> > 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(-)
>
> You don't seem to change any global function definition that reqires this
> change in all these devices so why can't these decide on their own what
> their preferred opaque data is for their set irq function and only change
> piix4? Am I missing something?

Yeah, we don't necessarily need to change all these devices,
but I think this is an area where over time we're shifting from
an older school of API design that was "we have some basically
standalone functions that take an arbitrary opaque pointer"
towards a more object-oriented design, where the opaque pointer
should probably be the pointer-to-this-object unless there's a good
reason why it needs to be something else[*], because having a function
that's part of a device being passed something other than the
device-instance pointer is a bit unexpected.

In some cases there is a good reason for opaque pointers for
function callbacks to be something else, because passing in the
device pointer would make the code noticeably more complicated.
But in the specific cases here, the only change is that the
callback functions use "s->somearray[i]" instead of
"an_argument[i]", which doesn't seem to me like a significant
complexity change.

thanks
-- PMM

Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[Peter Maydell]

On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> By the way the corresponding member in struct PIIXState in
> include/hw/southbridge/piix.h has a comment saying:
>
>      /* This member isn't used. Just for save/load compatibility */
>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>
> and only seems to be filled in piix3_pre_save() but never used. So what's
> the point of this then? Maybe piix3 also uses a bitmap to store these
> levels instead? There's a uint64_t pic_levels member above the unused
> array but I haven't checked the implementation.

I think what has happened here is that originally piix3 used
the same implementation piix4 currently does -- where it stores
locally the value of the (incoming) PCI IRQ levels, and then when it wants
to know the value of the (outgoing) PIC IRQ levels it loops round
to effectively OR together all the PCI IRQ levels for those PCI
IRQs which are configured to that particular PIC interrupt.

Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
looking at its local cache of that information in the pci_irq_levels[]
array. This is the source of the "save/load compatibility" thing --
before doing a vmsave piix3_pre_save() fills in that field so that
it appears in the outbound data stream and thus a migration from
a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
was important at the time, but could probably be cleaned up now.)

The next commit after that one is ab431c283e7055bcd, which
is an optimization that fixes the equivalent of the "XXX: optimize"
marker in the gt64120_pci_set_irq()/piix4 code -- this does
something slightly more complicated involving the pic_levels
field, in order to avoid having to do the "loop over all the
PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.

We could pick up one or both (or none!) of these two changes
for the piix4 code. (If we're breaking migration compat anyway
we wouldn't need to include the save-load compat part of
the first change.)

thanks
-- PMM

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

[BALATON Zoltan]

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

On Sat, 12 Feb 2022, BALATON Zoltan wrote:
> On Sat, 12 Feb 2022, 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>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Sorry for being late in commenting, I've missed the first round. Apologies if 
> this causes a delay or another version.
>
>> ---
>> 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++) {

Sorry one more nit. This is code movement but are we OK with declaring 
local variable within for? I thinks coding style advises against this, not 
sure if checkpatch accepts it or not. This could be cleaned up the next 
patch together with removing the i8259 field which are simple enough to do 
in one patch then this one stays as code movement and changes in next 
patch.

>> +        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
[...]
>> 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);
>
> Now that you don't need to pass anything to it, do you still need this 
> function? Maybe what it does now could be done in the gt64120 device's 
> realize functions (there seems to be at least two: gt64120_realize and 
> gt64120_pci_realize but haven't checked which is more appropriate to put this 
> init in) or in an init function then you can just create the gt64120 device 
> in malta.c with qdev_new as is more usual to do in other boards. This 
> register function looks like the legacy init functions we're trying to get 
> rid of so this seems to be an opportunity to clean this up. This could be 
> done in a separate follow up though so may not need to be part of this series 
> but may be nice to have.

I meant this comment at the end of patch 1. But maybe this is too much to 
do in this series as it needs more understanding of the gt64120 
implementation so unless you already understand it enough to clean this up 
easily now and it would be too much effort for you, then this is just for 
the record for possible future clean up. The series is good enough without 
putting in more stuff but if there's a way to go further then that would 
be nice.

Regards,.
BALATON Zoltan

>> 
>> /* bonito.c */
>> PCIBus *bonito_init(qemu_irq *pic);
>

Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[BALATON Zoltan]

On Sat, 12 Feb 2022, Peter Maydell wrote:
> On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> By the way the corresponding member in struct PIIXState in
>> include/hw/southbridge/piix.h has a comment saying:
>>
>>      /* This member isn't used. Just for save/load compatibility */
>>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>>
>> and only seems to be filled in piix3_pre_save() but never used. So what's
>> the point of this then? Maybe piix3 also uses a bitmap to store these
>> levels instead? There's a uint64_t pic_levels member above the unused
>> array but I haven't checked the implementation.
>
> I think what has happened here is that originally piix3 used
> the same implementation piix4 currently does -- where it stores
> locally the value of the (incoming) PCI IRQ levels, and then when it wants
> to know the value of the (outgoing) PIC IRQ levels it loops round
> to effectively OR together all the PCI IRQ levels for those PCI
> IRQs which are configured to that particular PIC interrupt.
>
> Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
> pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
> looking at its local cache of that information in the pci_irq_levels[]
> array. This is the source of the "save/load compatibility" thing --
> before doing a vmsave piix3_pre_save() fills in that field so that
> it appears in the outbound data stream and thus a migration from
> a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
> was important at the time, but could probably be cleaned up now.)
>
> The next commit after that one is ab431c283e7055bcd, which
> is an optimization that fixes the equivalent of the "XXX: optimize"
> marker in the gt64120_pci_set_irq()/piix4 code -- this does
> something slightly more complicated involving the pic_levels
> field, in order to avoid having to do the "loop over all the
> PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.
>
> We could pick up one or both (or none!) of these two changes
> for the piix4 code. (If we're breaking migration compat anyway
> we wouldn't need to include the save-load compat part of
> the first change.)

I'm not sure I fully get this. Currently (before this series) PIIX4State 
does not seem to have any fields to store irq state (in hw/isa/piix4.c):

struct PIIX4State {
     PCIDevice dev;
     qemu_irq cpu_intr;
     qemu_irq *isa;

     RTCState rtc;
     /* Reset Control Register */
     MemoryRegion rcr_mem;
     uint8_t rcr;
};

Patch 1 in this series introduces that by moving it from MaltaState. Then 
we could have a patch 2 to clean it up and change to the way piix3 does it 
and skip introducing the saving of this array into the migration state. It 
may still break migration but not sure if MaltaState was saved already so 
this may be already missing from migration anyway and if we do the same as 
piix3 then maybe we don't need to change the piix4 state either (as this 
is saved as part of the PCIHost state?) but I don't know much about this 
so maybe I'm wrong.

In any case PIIX3 and PIIX4 state seem to be different so there's no 
reason to worry aobut compatibility between them. It's just confusing that 
there's a common piix.h which defines a PIIXState that looks like it could 
be common but maybe it's not used by PIIX4 but only by PIIX3 or I've 
missed something as I've only looked at this briefly.

Regards,
BALATON Zoltan

Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[Peter Maydell]

On Sat, 12 Feb 2022 at 17:02, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Sat, 12 Feb 2022, Peter Maydell wrote:
> > On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >> By the way the corresponding member in struct PIIXState in
> >> include/hw/southbridge/piix.h has a comment saying:
> >>
> >>      /* This member isn't used. Just for save/load compatibility */
> >>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
> >>
> >> and only seems to be filled in piix3_pre_save() but never used. So what's
> >> the point of this then? Maybe piix3 also uses a bitmap to store these
> >> levels instead? There's a uint64_t pic_levels member above the unused
> >> array but I haven't checked the implementation.
> >
> > I think what has happened here is that originally piix3 used
> > the same implementation piix4 currently does -- where it stores
> > locally the value of the (incoming) PCI IRQ levels, and then when it wants
> > to know the value of the (outgoing) PIC IRQ levels it loops round
> > to effectively OR together all the PCI IRQ levels for those PCI
> > IRQs which are configured to that particular PIC interrupt.
> >
> > Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
> > pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
> > looking at its local cache of that information in the pci_irq_levels[]
> > array. This is the source of the "save/load compatibility" thing --
> > before doing a vmsave piix3_pre_save() fills in that field so that
> > it appears in the outbound data stream and thus a migration from
> > a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
> > was important at the time, but could probably be cleaned up now.)
> >
> > The next commit after that one is ab431c283e7055bcd, which
> > is an optimization that fixes the equivalent of the "XXX: optimize"
> > marker in the gt64120_pci_set_irq()/piix4 code -- this does
> > something slightly more complicated involving the pic_levels
> > field, in order to avoid having to do the "loop over all the
> > PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.
> >
> > We could pick up one or both (or none!) of these two changes
> > for the piix4 code. (If we're breaking migration compat anyway
> > we wouldn't need to include the save-load compat part of
> > the first change.)
>
> I'm not sure I fully get this. Currently (before this series) PIIX4State
> does not seem to have any fields to store irq state (in hw/isa/piix4.c):
>
> struct PIIX4State {
>      PCIDevice dev;
>      qemu_irq cpu_intr;
>      qemu_irq *isa;
>
>      RTCState rtc;
>      /* Reset Control Register */
>      MemoryRegion rcr_mem;
>      uint8_t rcr;
> };
>
> Patch 1 in this series introduces that by moving it from MaltaState. Then
> we could have a patch 2 to clean it up and change to the way piix3 does it
> and skip introducing the saving of this array into the migration state. It
> may still break migration but not sure if MaltaState was saved already so
> this may be already missing from migration anyway and if we do the same as
> piix3 then maybe we don't need to change the piix4 state either (as this
> is saved as part of the PCIHost state?) but I don't know much about this
> so maybe I'm wrong.

Yeah, that would work -- we weren't saving the old gt64xxx_pci.c
pci_irq_levels[] global, so we don't break anything that wasn't
already broken by not putting the newly-introduced PIIX4State
array into migration state. Then when we do the equivalent of
e735b55a8c11 the array can just be deleted again. (We can't
conveniently switch to using pci_bus_get_irq_level() before doing
patch 1 of this series, because we need the pointer to the
piix4 device object and gt64120_pci_set_irq() is only passed
a pointer directly to a qemu_irq array.)

> In any case PIIX3 and PIIX4 state seem to be different so there's no
> reason to worry aobut compatibility between them.

Yep, that's right. The only reasons to copy changes from piix3
are (a) because they're worth having in themselves and (b)
because having the two devices be the same is maybe less
confusing. (b)'s a pretty weak reason, and (a) depends on
the individual change. In this case I think making the equivalent
of e735b55a8c11 is worthwhile because it saves us having an
extra array field and migrating it, and the change is pretty
small. For ab431c283e7055bcd you could argue either way -- it's
clearly a better way to structure the irq handling, but it's only
an optimisation, not a bug fix.

-- PMM

Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[BB]

Am 12. Februar 2022 19:30:43 MEZ schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Sat, 12 Feb 2022 at 17:02, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> On Sat, 12 Feb 2022, Peter Maydell wrote:
>> > On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> >> By the way the corresponding member in struct PIIXState in
>> >> include/hw/southbridge/piix.h has a comment saying:
>> >>
>> >>      /* This member isn't used. Just for save/load compatibility */
>> >>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>> >>
>> >> and only seems to be filled in piix3_pre_save() but never used. So what's
>> >> the point of this then? Maybe piix3 also uses a bitmap to store these
>> >> levels instead? There's a uint64_t pic_levels member above the unused
>> >> array but I haven't checked the implementation.
>> >
>> > I think what has happened here is that originally piix3 used
>> > the same implementation piix4 currently does -- where it stores
>> > locally the value of the (incoming) PCI IRQ levels, and then when it wants
>> > to know the value of the (outgoing) PIC IRQ levels it loops round
>> > to effectively OR together all the PCI IRQ levels for those PCI
>> > IRQs which are configured to that particular PIC interrupt.
>> >
>> > Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
>> > pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
>> > looking at its local cache of that information in the pci_irq_levels[]
>> > array. This is the source of the "save/load compatibility" thing --
>> > before doing a vmsave piix3_pre_save() fills in that field so that
>> > it appears in the outbound data stream and thus a migration from
>> > a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
>> > was important at the time, but could probably be cleaned up now.)
>> >
>> > The next commit after that one is ab431c283e7055bcd, which
>> > is an optimization that fixes the equivalent of the "XXX: optimize"
>> > marker in the gt64120_pci_set_irq()/piix4 code -- this does
>> > something slightly more complicated involving the pic_levels
>> > field, in order to avoid having to do the "loop over all the
>> > PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.
>> >
>> > We could pick up one or both (or none!) of these two changes
>> > for the piix4 code. (If we're breaking migration compat anyway
>> > we wouldn't need to include the save-load compat part of
>> > the first change.)
>>
>> I'm not sure I fully get this. Currently (before this series) PIIX4State
>> does not seem to have any fields to store irq state (in hw/isa/piix4.c):
>>
>> struct PIIX4State {
>>      PCIDevice dev;
>>      qemu_irq cpu_intr;
>>      qemu_irq *isa;
>>
>>      RTCState rtc;
>>      /* Reset Control Register */
>>      MemoryRegion rcr_mem;
>>      uint8_t rcr;
>> };
>>
>> Patch 1 in this series introduces that by moving it from MaltaState. Then
>> we could have a patch 2 to clean it up and change to the way piix3 does it
>> and skip introducing the saving of this array into the migration state. It
>> may still break migration but not sure if MaltaState was saved already so
>> this may be already missing from migration anyway and if we do the same as
>> piix3 then maybe we don't need to change the piix4 state either (as this
>> is saved as part of the PCIHost state?) but I don't know much about this
>> so maybe I'm wrong.
>
>Yeah, that would work -- we weren't saving the old gt64xxx_pci.c
>pci_irq_levels[] global, so we don't break anything that wasn't
>already broken by not putting the newly-introduced PIIX4State
>array into migration state. Then when we do the equivalent of
>e735b55a8c11 the array can just be deleted again. (We can't
>conveniently switch to using pci_bus_get_irq_level() before doing
>patch 1 of this series, because we need the pointer to the
>piix4 device object and gt64120_pci_set_irq() is only passed
>a pointer directly to a qemu_irq array.)
>
>> In any case PIIX3 and PIIX4 state seem to be different so there's no
>> reason to worry aobut compatibility between them.
>
>Yep, that's right. The only reasons to copy changes from piix3
>are (a) because they're worth having in themselves and (b)
>because having the two devices be the same is maybe less
>confusing. (b)'s a pretty weak reason, and (a) depends on
>the individual change. In this case I think making the equivalent
>of e735b55a8c11 is worthwhile because it saves us having an
>extra array field and migrating it, and the change is pretty
>small. For ab431c283e7055bcd you could argue either way -- it's
>clearly a better way to structure the irq handling, but it's only
>an optimisation, not a bug fix.

e735b55a8c11 seems like a very elegant way for fixing migration of the IRQ levels. I'll have a look.

Regards,
Bernhard
>
>-- PMM

Re: [PATCH v2 4/5] isa/piix4: Fix PCI IRQ levels to be preserved in VMState

[Bernhard Beschow]

Am 12. Februar 2022 19:30:43 MEZ schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Sat, 12 Feb 2022 at 17:02, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> On Sat, 12 Feb 2022, Peter Maydell wrote:
>> > On Sat, 12 Feb 2022 at 13:42, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> >> By the way the corresponding member in struct PIIXState in
>> >> include/hw/southbridge/piix.h has a comment saying:
>> >>
>> >>      /* This member isn't used. Just for save/load compatibility */
>> >>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>> >>
>> >> and only seems to be filled in piix3_pre_save() but never used. So what's
>> >> the point of this then? Maybe piix3 also uses a bitmap to store these
>> >> levels instead? There's a uint64_t pic_levels member above the unused
>> >> array but I haven't checked the implementation.
>> >
>> > I think what has happened here is that originally piix3 used
>> > the same implementation piix4 currently does -- where it stores
>> > locally the value of the (incoming) PCI IRQ levels, and then when it wants
>> > to know the value of the (outgoing) PIC IRQ levels it loops round
>> > to effectively OR together all the PCI IRQ levels for those PCI
>> > IRQs which are configured to that particular PIC interrupt.
>> >
>> > Then in commit e735b55a8c11 (in 2011) piix3 was changed to call
>> > pci_bus_get_irq_level() to get the value of a PCI IRQ rather than
>> > looking at its local cache of that information in the pci_irq_levels[]
>> > array. This is the source of the "save/load compatibility" thing --
>> > before doing a vmsave piix3_pre_save() fills in that field so that
>> > it appears in the outbound data stream and thus a migration from
>> > a new QEMU to an old pre-e735b55a8c11 QEMU will still work. (This
>> > was important at the time, but could probably be cleaned up now.)
>> >
>> > The next commit after that one is ab431c283e7055bcd, which
>> > is an optimization that fixes the equivalent of the "XXX: optimize"
>> > marker in the gt64120_pci_set_irq()/piix4 code -- this does
>> > something slightly more complicated involving the pic_levels
>> > field, in order to avoid having to do the "loop over all the
>> > PCI IRQ levels" whenever it needs to set/reset a PIC interrupt.
>> >
>> > We could pick up one or both (or none!) of these two changes
>> > for the piix4 code. (If we're breaking migration compat anyway
>> > we wouldn't need to include the save-load compat part of
>> > the first change.)
>>
>> I'm not sure I fully get this. Currently (before this series) PIIX4State
>> does not seem to have any fields to store irq state (in hw/isa/piix4.c):
>>
>> struct PIIX4State {
>>      PCIDevice dev;
>>      qemu_irq cpu_intr;
>>      qemu_irq *isa;
>>
>>      RTCState rtc;
>>      /* Reset Control Register */
>>      MemoryRegion rcr_mem;
>>      uint8_t rcr;
>> };
>>
>> Patch 1 in this series introduces that by moving it from MaltaState. Then
>> we could have a patch 2 to clean it up and change to the way piix3 does it
>> and skip introducing the saving of this array into the migration state. It
>> may still break migration but not sure if MaltaState was saved already so
>> this may be already missing from migration anyway and if we do the same as
>> piix3 then maybe we don't need to change the piix4 state either (as this
>> is saved as part of the PCIHost state?) but I don't know much about this
>> so maybe I'm wrong.
>
>Yeah, that would work -- we weren't saving the old gt64xxx_pci.c
>pci_irq_levels[] global, so we don't break anything that wasn't
>already broken by not putting the newly-introduced PIIX4State
>array into migration state. Then when we do the equivalent of
>e735b55a8c11 the array can just be deleted again. (We can't
>conveniently switch to using pci_bus_get_irq_level() before doing
>patch 1 of this series, because we need the pointer to the
>piix4 device object and gt64120_pci_set_irq() is only passed
>a pointer directly to a qemu_irq array.)
>
>> In any case PIIX3 and PIIX4 state seem to be different so there's no
>> reason to worry aobut compatibility between them.
>
>Yep, that's right. The only reasons to copy changes from piix3
>are (a) because they're worth having in themselves and (b)
>because having the two devices be the same is maybe less
>confusing. (b)'s a pretty weak reason, and (a) depends on
>the individual change. In this case I think making the equivalent
>of e735b55a8c11 is worthwhile because it saves us having an
>extra array field and migrating it, and the change is pretty
>small. For ab431c283e7055bcd you could argue either way -- it's
>clearly a better way to structure the irq handling, but it's only
>an optimisation, not a bug fix.

e735b55a8c11 seems like a very elegant way for fixing migration of the IRQ levels. I'll have a look.

Regards,
Bernhard
>
>-- PMM

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

[Bernhard Beschow]

Am 12. Februar 2022 17:44:30 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, BALATON Zoltan wrote:
>> On Sat, 12 Feb 2022, 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>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Sorry for being late in commenting, I've missed the first round. Apologies if 
>> this causes a delay or another version.
>>
>>> ---
>>> 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++) {
>
>Sorry one more nit. This is code movement but are we OK with declaring 
>local variable within for? I thinks coding style advises against this, not 
>sure if checkpatch accepts it or not. This could be cleaned up the next 
>patch together with removing the i8259 field which are simple enough to do 
>in one patch then this one stays as code movement and changes in next 
>patch.

I'll move the i8259-removing patch right after the code movement patch where this loop is removed entirely.

>>> +        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
>[...]
>>> 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);
>>
>> Now that you don't need to pass anything to it, do you still need this 
>> function? Maybe what it does now could be done in the gt64120 device's 
>> realize functions (there seems to be at least two: gt64120_realize and 
>> gt64120_pci_realize but haven't checked which is more appropriate to put this 
>> init in) or in an init function then you can just create the gt64120 device 
>> in malta.c with qdev_new as is more usual to do in other boards. This 
>> register function looks like the legacy init functions we're trying to get 
>> rid of so this seems to be an opportunity to clean this up. This could be 
>> done in a separate follow up though so may not need to be part of this series 
>> but may be nice to have.
>
>I meant this comment at the end of patch 1. But maybe this is too much to 
>do in this series as it needs more understanding of the gt64120 
>implementation so unless you already understand it enough to clean this up 
>easily now and it would be too much effort for you, then this is just for 
>the record for possible future clean up. The series is good enough without 
>putting in more stuff but if there's a way to go further then that would 
>be nice.

I'll give it a shot. Merging gt64120_register() into gt64120_realize() seems to preserve relevant control flow.

Regards,
Bernhard

>Regards,.
>BALATON Zoltan
>
>>> 
>>> /* bonito.c */
>>> PCIBus *bonito_init(qemu_irq *pic);
>>

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

[Bernhard Beschow]

Am 12. Februar 2022 17:13:19 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>> Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>>> Passing own DeviceState rather than just the IRQs allows for resolving
>>>> global variables.
>>>
>>> Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?
>>
>> I'm referring to the typedef in pci.h because the patch establishes
>> consistency across the board.
>>
>>>> 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 +++---
>>>> 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(-)
>>>
>>> You don't seem to change any global function definition that reqires this
>>> change in all these devices so why can't these decide on their own what
>>> their preferred opaque data is for their set irq function and only change
>>> piix4? Am I missing something? I'm not opposed to this change but it seems
>>> to be unnecessary to touch other device implementations for this and may
>>> just make them more complex for no good reason.
>>
>> This patch is a segway into a direction where the type of the opaque parameter
>> of the pci_map_irq_fn typedef could be changed from void* to DeviceState* in
>
>I'm still not quite sure what you mean considering these typedefs in 
>include/hw/pci/pci.h:
>
>typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
>typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
>
>pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void 
>*opaque and is what this patch appears to be changing. Am I looking at the 
>wrong typedefs?

Oh sorry, I mixed things up. You're correct: I meant pci_set_irq_fn.

>> order to facilitate the more typesafe QOM casting. I see, though, why this is
>> confusing in the context of this patch series. I don't have strong feelings for
>> converting the other devices or not. So I can only change piix4 if desired.
>
>Yes that seems to be an independent cahange so maybe it's better to just 
>change piix4 in this series and then have a separate patch for changing 
>the void *opaque to DeviceState * independent of this series. But I'm not 
>sure that's necessarily a good idea. It may give some more checks but for 
>callbacks used internally by device implementations I think it can be 
>expected that code that registers the callback also knows what its opaque 
>data should be so it does not have to be checked additionally, especially 
>in code that may be called frequently. Although in a similar via-ide 
>callback I could not measure a big penalty the last time I tried but I 
>suspect there still mey be some overhead involving QOM casts (maybe there 
>are just bigger bottle necks in ide emulation so it was masked) so I'm not 
>sure it's worth the added complexity but if others prefer that I'm not 
>that much opposed to it but it's clearer as a separate change anyway.

I'll just change piix4, leaving the other devices as is. This also allows for merging this patch with the next.

Regards,
Bernhard

>Regards,
>BALATON Zoltan

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

[Bernhard Beschow]

Am 12. Februar 2022 14:18:54 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sat, 12 Feb 2022, 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>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>Sorry for being late in commenting, I've missed the first round. Apologies 
>if this causes a delay or another version.

Don't worry. Your comments are appreciated!

>> ---
>> 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);
>
>Now that you don't need to pass anything to it, do you still need this 
>function? Maybe what it does now could be done in the gt64120 device's 
>realize functions (there seems to be at least two: gt64120_realize and 
>gt64120_pci_realize but haven't checked which is more appropriate to put 
>this init in) or in an init function then you can just create the gt64120 
>device in malta.c with qdev_new as is more usual to do in other boards. 
>This register function looks like the legacy init functions we're trying 
>to get rid of so this seems to be an opportunity to clean this up. This 
>could be done in a separate follow up though so may not need to be part of 
>this series but may be nice to have.

I'll give it a shot (see my other mail).

Regards,
Bernhard

>Regards,.
>BALATON Zoltan
>
>>
>> /* bonito.c */
>> PCIBus *bonito_init(qemu_irq *pic);
>>

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

[BALATON Zoltan]

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

On Sun, 13 Feb 2022, Bernhard Beschow wrote:
> Am 12. Februar 2022 17:13:19 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>> Am 12. Februar 2022 14:27:32 MEZ schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>>> On Sat, 12 Feb 2022, Bernhard Beschow wrote:
>>>>> Passing own DeviceState rather than just the IRQs allows for resolving
>>>>> global variables.
>>>>
>>>> Do you mean pci_set_irq_fn instead of pci_map_irq_fn in the patch title?
>>>
>>> I'm referring to the typedef in pci.h because the patch establishes
>>> consistency across the board.
>>>
>>>>> 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 +++---
>>>>> 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(-)
>>>>
>>>> You don't seem to change any global function definition that reqires this
>>>> change in all these devices so why can't these decide on their own what
>>>> their preferred opaque data is for their set irq function and only change
>>>> piix4? Am I missing something? I'm not opposed to this change but it seems
>>>> to be unnecessary to touch other device implementations for this and may
>>>> just make them more complex for no good reason.
>>>
>>> This patch is a segway into a direction where the type of the opaque parameter
>>> of the pci_map_irq_fn typedef could be changed from void* to DeviceState* in
>>
>> I'm still not quite sure what you mean considering these typedefs in
>> include/hw/pci/pci.h:
>>
>> typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
>> typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
>> typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
>>
>> pci_map_irq_fn already has PCIDevice *, it's pci_set_irq_fn that has void
>> *opaque and is what this patch appears to be changing. Am I looking at the
>> wrong typedefs?
>
> Oh sorry, I mixed things up. You're correct: I meant pci_set_irq_fn.
>
>>> order to facilitate the more typesafe QOM casting. I see, though, why this is
>>> confusing in the context of this patch series. I don't have strong feelings for
>>> converting the other devices or not. So I can only change piix4 if desired.
>>
>> Yes that seems to be an independent cahange so maybe it's better to just
>> change piix4 in this series and then have a separate patch for changing
>> the void *opaque to DeviceState * independent of this series. But I'm not
>> sure that's necessarily a good idea. It may give some more checks but for
>> callbacks used internally by device implementations I think it can be
>> expected that code that registers the callback also knows what its opaque
>> data should be so it does not have to be checked additionally, especially
>> in code that may be called frequently. Although in a similar via-ide
>> callback I could not measure a big penalty the last time I tried but I
>> suspect there still mey be some overhead involving QOM casts (maybe there
>> are just bigger bottle necks in ide emulation so it was masked) so I'm not
>> sure it's worth the added complexity but if others prefer that I'm not
>> that much opposed to it but it's clearer as a separate change anyway.
>
> I'll just change piix4, leaving the other devices as is. This also allows for merging this patch with the next.

I'm not sure what will be the next patch after all the changes we were 
talking about but don't put too many changes in one patch. It may worth 
keeping this as a separate change even if it's simple and only affecting 
piix4 now just to make it clear and simpler to review and maybe bisect 
later. Generally one change per patch is preferred even if it results in a 
lot of small patches (unless maybe combining a small style fix or very 
simple one line change with another simple patch that may not worth a 
separate patch). This may be over that limit so better to keep as separate 
patch but again not sure what patch you meant to combine it with but the 
patch submission guidelines say separate changes should be separate 
patches.

Regards,
BALATON Zoltan