hw/i386/pc: Split PIIX3 southbridge from i440FX northbridge

From: Philippe Mathieu-Daudé <[email protected]>

The PIIX4 Southbridge is not used by the PC machine, but by the Malta board (MIPS). Add a new section to keep it covered.

Suggested-by Michael S. Tsirkin <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 MAINTAINERS | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

MAINTAINERS +6 -2

index fe4dc51b08..c9f625fc2e 100644
@@ -1230,7 +1230,6 @@ F: hw/pci-host/q35.c
 F: hw/pci-host/pam.c
 F: include/hw/pci-host/q35.h
 F: include/hw/pci-host/pam.h
-F: hw/isa/piix4.c
 F: hw/isa/lpc_ich9.c
 F: hw/i2c/smbus_ich9.c
 F: hw/acpi/piix4.c

@@ -1716,6 +1715,12 @@ F: hw/display/edid*
 F: include/hw/display/edid.h
 F: qemu-edid.c
 
+PIIX4 South Bridge (i82371AB)
+M: Hervé Poussineau <[email protected]>
+M: Philippe Mathieu-Daudé <[email protected]>
+S: Maintained
+F: hw/isa/piix4.c
+
 Firmware configuration (fw_cfg)
 M: Philippe Mathieu-Daudé <[email protected]>
 R: Laszlo Ersek <[email protected]>
--

2.21.0

From: Hervé Poussineau <[email protected]>

The RCR I/O port (0xcf9) is used to generate a hard reset or a soft reset.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 46 insertions(+), 3 deletions(-)

hw/isa/piix4.c +1 -0

index 890d999abf..d0b18e0586 100644
@@ -2,6 +2,7 @@
  * QEMU PIIX4 PCI Bridge Emulation
  *
  * Copyright (c) 2006 Fabrice Bellard
+ * Copyright (c) 2018 Hervé Poussineau
  *
  * Permission is hereby granted, free of charge, to any person obtaining

a copy
* of this software and associated documentation files (the "Software"),
to deal

hw/isa/piix4.c +37 -2

@@ -28,11 +29,17 @@
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
+#include "sysemu/reset.h"
+#include "sysemu/runstate.h"
 
 PCIDevice *piix4_dev;
 
 typedef struct PIIX4State {
     PCIDevice dev;
+
+    /* Reset Control Register */
+    MemoryRegion rcr_mem;
+    uint8_t rcr;
 } PIIX4State;
 
 #define TYPE_PIIX4_PCI_DEVICE "PIIX4"

@@ -87,15 +94,51 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
+static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
+                            unsigned int len)
+{
+    PIIX4State *s = opaque;
+
+    if (val & 4) {
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        return;
+    }
+
+    s->rcr = val & 2; /* keep System Reset type only */
+}
+
+static uint64_t piix4_rcr_read(void *opaque, hwaddr addr, unsigned int
 len)
+{
+    PIIX4State *s = opaque;
+
+    return s->rcr;
+}
+
+static const MemoryRegionOps piix4_rcr_ops = {
+    .read = piix4_rcr_read,
+    .write = piix4_rcr_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
-    PIIX4State *d = PIIX4_PCI_DEVICE(dev);
+    PIIX4State *s = PIIX4_PCI_DEVICE(dev);
 
-    if (!isa_bus_new(DEVICE(d), pci_address_space(dev),

hw/isa/piix4.c +3 -0

+                                        &s->rcr_mem, 1);
+
+    piix4_dev = dev;
 }

int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
--
2.21.0

Philippe Mathieu-Daudé <[email protected] <mailto:[email protected]>> 于 2019年10月18日周五 下午9:50写道：

    From: Hervé Poussineau <[email protected]
    <mailto:[email protected]>>

    The RCR I/O port (0xcf9) is used to generate a hard reset or a soft
    reset.

    Acked-by: Michael S. Tsirkin <[email protected] <mailto:[email protected]>>
    Acked-by: Paolo Bonzini <[email protected]
    <mailto:[email protected]>>
    Signed-off-by: Hervé Poussineau <[email protected]
    <mailto:[email protected]>>
    Message-Id: <[email protected]
    <mailto:[email protected]>>
    Reviewed-by: Aleksandar Markovic <[email protected]
    <mailto:[email protected]>>
    [PMD: rebased, updated includes]
    Signed-off-by: Philippe Mathieu-Daudé <[email protected]
    <mailto:[email protected]>>
    ---
      hw/isa/piix4.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
      1 file changed, 46 insertions(+), 3 deletions(-)

    diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
    index 890d999abf..d0b18e0586 100644
    --- a/hw/isa/piix4.c
    +++ b/hw/isa/piix4.c
    @@ -2,6 +2,7 @@
       * QEMU PIIX4 PCI Bridge Emulation
       *
       * Copyright (c) 2006 Fabrice Bellard
    + * Copyright (c) 2018 Hervé Poussineau
       *
       * Permission is hereby granted, free of charge, to any person
    obtaining a copy
       * of this software and associated documentation files (the
    "Software"), to deal
    @@ -28,11 +29,17 @@
      #include "hw/isa/isa.h"
      #include "hw/sysbus.h"
      #include "migration/vmstate.h"
    +#include "sysemu/reset.h"
    +#include "sysemu/runstate.h"

      PCIDevice *piix4_dev;

      typedef struct PIIX4State {
          PCIDevice dev;
    +
    +    /* Reset Control Register */
    +    MemoryRegion rcr_mem;
    +    uint8_t rcr;
      } PIIX4State;

      #define TYPE_PIIX4_PCI_DEVICE "PIIX4"
    @@ -87,15 +94,51 @@ static const VMStateDescription vmstate_piix4 = {
          }
      };

    +static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
    +                            unsigned int len)
    +{
    +    PIIX4State *s = opaque;
    +
    +    if (val & 4) {
    +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
    +        return;
    +    }
    +
    +    s->rcr = val & 2; /* keep System Reset type only */
    +}
    +
    +static uint64_t piix4_rcr_read(void *opaque, hwaddr addr, unsigned
    int len)
    +{
    +    PIIX4State *s = opaque;
    +
    +    return s->rcr;
    +}
    +
    +static const MemoryRegionOps piix4_rcr_ops = {
    +    .read = piix4_rcr_read,
    +    .write = piix4_rcr_write,
    +    .endianness = DEVICE_LITTLE_ENDIAN,
    +    .impl = {
    +        .min_access_size = 1,
    +        .max_access_size = 1,
    +    },
    +};
    +
      static void piix4_realize(PCIDevice *dev, Error **errp)
      {
    -    PIIX4State *d = PIIX4_PCI_DEVICE(dev);
    +    PIIX4State *s = PIIX4_PCI_DEVICE(dev);

    -    if (!isa_bus_new(DEVICE(d), pci_address_space(dev),
    +    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
                           pci_address_space_io(dev), errp)) {
              return;
          }
    -    piix4_dev = &d->dev;
    +
    +    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
    +                          "reset-control", 1);
    +    memory_region_add_subregion_overlap(pci_address_space_io(dev),
    0xcf9,

Can we use 'RCR_IOPORT' instead of constant value here? Also don't see this change in later patches of this seirals.

Anyway

Reviewed-by Li Qiang <[email protected] <mailto:[email protected]>>

Thanks,
Li Qiang

    +                                        &s->rcr_mem, 1);
    +
    +    piix4_dev = dev;
      }

      int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
    -- 
    2.21.0

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out. Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++-----------
 hw/mips/mips_malta.c | 32 +++++++++++++-------------------
 include/hw/i386/pc.h |  1 -
 3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +19 -2

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/irq.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
 typedef struct PIIX4State {
     PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
     /* Reset Control Register */
     MemoryRegion rcr_mem;

@@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
+static void piix4_request_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
+static void piix4_set_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)
 {

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {
+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);

hw/isa/piix4.c +4 -9

+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
     piix4_dev = dev;
 }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-
 static void piix4_class_init(ObjectClass *klass, void *data)

 {
     DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +2 -8

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
     SysBusDevice parent_obj;
 
     MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];
 } MaltaState;
 
 static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
     int64_t kernel_entry, bootloader_run_addr;
     PCIBus *pci_bus;
     ISABus *isa_bus;
-    qemu_irq *isa_irq;
     qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
     int piix4_devfn;
     I2CBus *smbus;
     DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
     /* Board ID = 0x420 (Malta Board with CoreLV) */
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
-     * on piix4, and piix4 depends on pci_bus.  To stop the cycle we have
-     * qemu_irq_proxy() adds an extra bit of indirection, allowing us
-     * to resolve the isa_irq -> i8259 dependency after i8259 is

initialized.

hw/mips/mips_malta.c +6 -5

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
     /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 

hw/mips/mips_malta.c +0 -1

-    isa_bus_irqs(isa_bus, s->i8259);
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -1

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const char
 *pci_type,
 PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */

void pc_system_flash_create(PCMachineState *pcms);
--
2.21.0

Philippe Mathieu-Daudé <[email protected] <mailto:[email protected]>> 于 2019年10月18日周五 下午9:52写道：

    From: Hervé Poussineau <[email protected]
    <mailto:[email protected]>>

    Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4
    gpio out.
    Remove i8259 instanciated in malta board, to not have it twice.

    We can also remove the now unused piix4_init() function.

    Acked-by: Michael S. Tsirkin <[email protected] <mailto:[email protected]>>
    Acked-by: Paolo Bonzini <[email protected]
    <mailto:[email protected]>>
    Signed-off-by: Hervé Poussineau <[email protected]
    <mailto:[email protected]>>
    Message-Id: <[email protected]
    <mailto:[email protected]>>
    Reviewed-by: Aleksandar Markovic <[email protected]
    <mailto:[email protected]>>
    [PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]
    Signed-off-by: Philippe Mathieu-Daudé <[email protected]
    <mailto:[email protected]>>
    ---
      hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++-----------
      hw/mips/mips_malta.c | 32 +++++++++++++-------------------
      include/hw/i386/pc.h |  1 -
      3 files changed, 45 insertions(+), 31 deletions(-)

    diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
    index d0b18e0586..9c37c85ae2 100644
    --- a/hw/isa/piix4.c
    +++ b/hw/isa/piix4.c
    @@ -24,6 +24,7 @@
       */

      #include "qemu/osdep.h"
    +#include "hw/irq.h"
      #include "hw/i386/pc.h"
      #include "hw/pci/pci.h"
      #include "hw/isa/isa.h"
    @@ -36,6 +37,8 @@ PCIDevice *piix4_dev;

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

          /* Reset Control Register */
          MemoryRegion rcr_mem;
    @@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
          }
      };

    +static void piix4_request_i8259_irq(void *opaque, int irq, int level)
    +{
    +    PIIX4State *s = opaque;
    +    qemu_set_irq(s->cpu_intr, level);
    +}
    +
    +static void piix4_set_i8259_irq(void *opaque, int irq, int level)
    +{
    +    PIIX4State *s = opaque;
    +    qemu_set_irq(s->isa[irq], level);
    +}
    +
      static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                                  unsigned int len)
      {
    @@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
      static void piix4_realize(PCIDevice *dev, Error **errp)
      {
          PIIX4State *s = PIIX4_PCI_DEVICE(dev);
    +    ISABus *isa_bus;
    +    qemu_irq *i8259_out_irq;

    -    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
    -                     pci_address_space_io(dev), errp)) {
    +    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
    +                          pci_address_space_io(dev), errp);
    +    if (!isa_bus) {
              return;
          }

    +    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
    +                            "isa", ISA_NUM_IRQS);
    +    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
    +                             "intr", 1);
    +

Does the piix4 hardware has the GPIO for interrupt? Seems not.

          memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
                                "reset-control", 1);
          memory_region_add_subregion_overlap(pci_address_space_io(dev),
    0xcf9,
                                              &s->rcr_mem, 1);

    +    /* initialize i8259 pic */
    +    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
    +    s->isa = i8259_init(isa_bus, *i8259_out_irq);

In i8259_init, we also allocate 16 input line and 1 output line. Seems it is duplicated with the GPIO allocation in previous.

Also
Maybe here can uses
i8259(isa_bus, qemu_allocate_irq(piix4_request_i8259_irq, s, 0));

    +
    +    /* initialize ISA irqs */
    +    isa_bus_irqs(isa_bus, s->isa);
    +
          piix4_dev = dev;
      }

    -int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
    -{
    -    PCIDevice *d;
    -
    -    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
    -    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
    -    return d->devfn;
    -}
    -
      static void piix4_class_init(ObjectClass *klass, void *data)
      {
          DeviceClass *dc = DEVICE_CLASS(klass);
    diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
    index 4d9c64b36a..7d25ab6c23 100644
    --- a/hw/mips/mips_malta.c
    +++ b/hw/mips/mips_malta.c
    @@ -97,7 +97,7 @@ typedef struct {
          SysBusDevice parent_obj;

          MIPSCPSState cps;
    -    qemu_irq *i8259;
    +    qemu_irq i8259[16];
      } MaltaState;

      static ISADevice *pit;
    @@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
          int64_t kernel_entry, bootloader_run_addr;
          PCIBus *pci_bus;
          ISABus *isa_bus;
    -    qemu_irq *isa_irq;
          qemu_irq cbus_irq, i8259_irq;
    +    PCIDevice *pci;
          int piix4_devfn;
          I2CBus *smbus;
          DriveInfo *dinfo;
    @@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
          /* Board ID = 0x420 (Malta Board with CoreLV) */
          stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);

    -    /*
    -     * We have a circular dependency problem: pci_bus depends on
    isa_irq,
    -     * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
    -     * on piix4, and piix4 depends on pci_bus.  To stop the cycle
    we have
    -     * qemu_irq_proxy() adds an extra bit of indirection, allowing us
    -     * to resolve the isa_irq -> i8259 dependency after i8259 is
    initialized.
    -     */
    -    isa_irq = qemu_irq_proxy(&s->i8259, 16);
    -
          /* Northbridge */
    -    pci_bus = gt64120_register(isa_irq);
    +    pci_bus = gt64120_register(s->i8259);

          /* Southbridge */
          ide_drive_get(hd, ARRAY_SIZE(hd));

    -    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
    +    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
    +                                          true, "PIIX4");
    +    dev = DEVICE(pci);
    +    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
    +    piix4_devfn = pci->devfn;

    -    /*
    -     * Interrupt controller
    -     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
    -     */
    -    s->i8259 = i8259_init(isa_bus, i8259_irq);
    +    /* 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);
    +    }

Also here s->i8259 and the piix4 isa point to the same input line. Seems duplicated.

I have come up with a more cleaner patch as following:

Though 'i8259_init' is called in the mips_malta_init. But is uses the isa bus from piix4 device. And seems it's more clean.
You can test it with more tests.

Author: Li Qiang <[email protected] <mailto:[email protected]>>
Date:   Mon Oct 21 22:41:17 2019 +0800

    piix4

hw/isa/piix4.c +2 -8

index d0b18e0586..66a041040a 100644
@@ -24,6 +24,7 @@
   */
 
  #include "qemu/osdep.h"
+#include "hw/irq.h"
  #include "hw/i386/pc.h"
  #include "hw/pci/pci.h"
  #include "hw/isa/isa.h"

@@ -46,6 +47,7 @@ typedef struct PIIX4State {
  #define PIIX4_PCI_DEVICE(obj) \
      OBJECT_CHECK(PIIX4State, (obj), TYPE_PIIX4_PCI_DEVICE)
 
+
  static void piix4_isa_reset(DeviceState *dev)
  {
      PIIX4State *d = PIIX4_PCI_DEVICE(dev);

@@ -141,14 +143,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
      piix4_dev = dev;
  }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
 
  static void piix4_class_init(ObjectClass *klass, void *data)
  {

hw/mips/mips_malta.c +4 -8

index 4d9c64b36a..420e0e9e80 100644
@@ -28,6 +28,7 @@
  #include "cpu.h"
  #include "hw/i386/pc.h"
  #include "hw/isa/superio.h"
+//#include "hw/isa/piix4.h"
  #include "hw/dma/i8257.h"
  #include "hw/char/serial.h"
  #include "net/net.h"

@@ -97,7 +98,7 @@ typedef struct {
      SysBusDevice parent_obj;
 
      MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[ISA_NUM_IRQS];
  } MaltaState;
 
  static ISADevice *pit;

@@ -1235,8 +1236,9 @@ void mips_malta_init(MachineState *machine)
      int64_t kernel_entry, bootloader_run_addr;
      PCIBus *pci_bus;
      ISABus *isa_bus;
-    qemu_irq *isa_irq;
      qemu_irq cbus_irq, i8259_irq;
+    qemu_irq *i8259;
+    PCIDevice *pci;
      int piix4_devfn;
      I2CBus *smbus;
      DriveInfo *dinfo;

@@ -1407,29 +1409,24 @@ void mips_malta_init(MachineState *machine)
      /* Board ID = 0x420 (Malta Board with CoreLV) */
      stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
-     * on piix4, and piix4 depends on pci_bus.  To stop the cycle we have
-     * qemu_irq_proxy() adds an extra bit of indirection, allowing us
-     * to resolve the isa_irq -> i8259 dependency after i8259 is 

initialized.
-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
     /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);

     /* Southbridge */
     ide_drive_get(hd, ARRAY_SIZE(hd));

-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;

-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);

+    i8259 = i8259_init(isa_bus, i8259_irq);
+    for (int i = 0; i < ISA_NUM_IRQS; i++) {
+        s->i8259[i] = i8259[i];
+    }
+    g_free(i8259);
     isa_bus_irqs(isa_bus, s->i8259); pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1); pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");

    -    isa_bus_irqs(isa_bus, s->i8259);
          pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
          pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
          smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
    diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
    index 37bfd95113..374f3e8835 100644
    --- a/include/hw/i386/pc.h
    +++ b/include/hw/i386/pc.h
    @@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const
    char *pci_type,
      PCIBus *find_i440fx(void);

      /* piix4.c */
      extern PCIDevice *piix4_dev;
    -int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);

      /* pc_sysfw.c */
      void pc_system_flash_create(PCMachineState *pcms);
    -- 
    2.21.0

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out.
Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++--------
---
 hw/mips/mips_malta.c | 32 +++++++++++++-------------------
 include/hw/i386/pc.h |  1 -
 3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +15 -0

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/irq.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
 typedef struct PIIX4State {
     PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
     /* Reset Control Register */
     MemoryRegion rcr_mem;

@@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
+static void piix4_request_i8259_irq(void *opaque, int irq, int
 level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
+static void piix4_set_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)

{

hw/isa/piix4.c +2 -2

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {

hw/isa/piix4.c +7 -9

+    /* initialize i8259 pic */
+    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s,
 1);
+    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
     piix4_dev = dev;
 }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-
 static void piix4_class_init(ObjectClass *klass, void *data)

 {
     DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +2 -5

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
     SysBusDevice parent_obj;
 
     MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];
 } MaltaState;
 
 static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
     int64_t kernel_entry, bootloader_run_addr;
     PCIBus *pci_bus;
     ISABus *isa_bus;
-    qemu_irq *isa_irq;
     qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
     int piix4_devfn;
     I2CBus *smbus;
     DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
     /* Board ID = 0x420 (Malta Board with CoreLV) */
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on
 isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA

depends
- * on piix4, and piix4 depends on pci_bus. To stop the cycle we
have
- * qemu_irq_proxy() adds an extra bit of indirection, allowing
us
- * to resolve the isa_irq -> i8259 dependency after i8259 is
initialized.

hw/mips/mips_malta.c +11 -11

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
     /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 
-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);
+    /* 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);
+    }
 
-    isa_bus_irqs(isa_bus, s->i8259);
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -1

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const
 char *pci_type,
 PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */

void pc_system_flash_create(PCMachineState *pcms);

Quoted 72 lines · Philippe Mathieu-Daudé

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out.
Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
  hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++--------
---
  hw/mips/mips_malta.c | 32 +++++++++++++-------------------
  include/hw/i386/pc.h |  1 -
  3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +15 -0

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
   */
 
  #include "qemu/osdep.h"
+#include "hw/irq.h"
  #include "hw/i386/pc.h"
  #include "hw/pci/pci.h"
  #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
  typedef struct PIIX4State {
      PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
      /* Reset Control Register */
      MemoryRegion rcr_mem;

@@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
      }
  };
 
+static void piix4_request_i8259_irq(void *opaque, int irq, int
 level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
+static void piix4_set_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+
  static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                              unsigned int len)

  {

hw/isa/piix4.c +2 -2

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
  static void piix4_realize(PCIDevice *dev, Error **errp)
  {
      PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {

hw/isa/piix4.c static const MemoryRegionOps piix4_rcr_ops = { · line 157

Quoted 10 lines

hw/isa/piix4.c +7 -0

+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
          return;
      }
 
+    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
+                            "isa", ISA_NUM_IRQS);
+    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
+                             "intr", 1);

My question is not about this patch:

The function name is "qdev_init_gpio_out_named" but support more than 1 gpio, right? in this case, the name shouldn't be something like "qdev_init_gpios_out_named"?

hw/isa/piix4.c static const MemoryRegionOps piix4_rcr_ops = { · line 164

Quoted 7 lines

hw/isa/piix4.c +1 -0

+
      memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops,
 s,
                            "reset-control", 1);
      memory_region_add_subregion_overlap(pci_address_space_io(dev),
 0xcf9,
                                          &s->rcr_mem, 1);

Why do you use the priority 1 in this case?

Quoted 31 lines

hw/isa/piix4.c +7 -9

+    /* initialize i8259 pic */
+    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s,
 1);
+    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
      piix4_dev = dev;
  }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-
  static void piix4_class_init(ObjectClass *klass, void *data)

  {
      DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +0 -0

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
      SysBusDevice parent_obj;
 

hw/mips/mips_malta.c typedef struct { · line 100

Quoted 3 lines

hw/mips/mips_malta.c +1 -1

      MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];

Quoted 71 lines

hw/mips/mips_malta.c +1 -4

  } MaltaState;
 
  static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
      int64_t kernel_entry, bootloader_run_addr;
      PCIBus *pci_bus;
      ISABus *isa_bus;
-    qemu_irq *isa_irq;
      qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
      int piix4_devfn;
      I2CBus *smbus;
      DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
      /* Board ID = 0x420 (Malta Board with CoreLV) */
      stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on
 isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA

depends
- * on piix4, and piix4 depends on pci_bus. To stop the cycle we
have
- * qemu_irq_proxy() adds an extra bit of indirection, allowing
us
- * to resolve the isa_irq -> i8259 dependency after i8259 is
initialized.

hw/mips/mips_malta.c +11 -11

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
      /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
      /* Southbridge */
      ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 
-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);
+    /* 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);
+    }
 
-    isa_bus_irqs(isa_bus, s->i8259);
      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -1

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const
 char *pci_type,
  PCIBus *find_i440fx(void);
  /* piix4.c */
  extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 

Quoted 2 lines

include/hw/i386/pc.h +0 -0

  /* pc_sysfw.c */

  void pc_system_flash_create(PCMachineState *pcms);

Quoted 91 lines · Esteban Bosse

Quoted 77 lines · Philippe Mathieu-Daudé

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out.
Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
  hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++-----
---
---
  hw/mips/mips_malta.c | 32 +++++++++++++-------------------
  include/hw/i386/pc.h |  1 -
  3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +15 -0

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
   */
 
  #include "qemu/osdep.h"
+#include "hw/irq.h"
  #include "hw/i386/pc.h"
  #include "hw/pci/pci.h"
  #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
  typedef struct PIIX4State {
      PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
      /* Reset Control Register */
      MemoryRegion rcr_mem;

@@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4
 = {
      }
  };
 
+static void piix4_request_i8259_irq(void *opaque, int irq, int
 level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
+static void piix4_set_i8259_irq(void *opaque, int irq, int
 level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+

static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t
val,
unsigned int len)
{

hw/isa/piix4.c +2 -2

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops
 = {
  static void piix4_realize(PCIDevice *dev, Error **errp)
  {
      PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {

hw/isa/piix4.c static const MemoryRegionOps piix4_rcr_ops · line 158

Quoted 10 lines

hw/isa/piix4.c +7 -0

+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
          return;
      }
 
+    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
+                            "isa", ISA_NUM_IRQS);
+    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
+                             "intr", 1);

My question is not about this patch:

Quoted 4 lines

The function name is "qdev_init_gpio_out_named" but support more than 1
gpio, right? in this case, the name shouldn't be something like "qdev_init_gpios_out_named"?

Indeed devices can have various IRQ output lines.

Note, QEMU does not intend to model full devices, but only the part required to run a guest. If a guest doesn't use some part of a device, QEMU will likely not model it.

For example, sometimes a device can have N output IRQ to signal various error conditions, which are usually used by specific firmwares in embedded devices. QEMU might not model embedded boards using this device but we can find it in a generic machine which runs a full operating system. So far these OS don't care about handling these errors, so QEMU will only model the IRQ line required to run the OS, no more. This is on purpose.

Now about the naming, I have no preference which form is better.

Quoted 43 lines

hw/isa/piix4.c static const MemoryRegionOps piix4_rcr_ops · line 167

Quoted 9 lines

hw/isa/piix4.c +1 -0

+
      memory_region_init_io(&s->rcr_mem, OBJECT(dev),
 &piix4_rcr_ops,
 s,
                            "reset-control", 1);
      memory_region_add_subregion_overlap(pci_address_space_io(de
 v),
 0xcf9,
                                          &s->rcr_mem, 1);

Why do you use the priority 1 in this case?

Quoted 33 lines

hw/isa/piix4.c +7 -5

+    /* initialize i8259 pic */
+    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq,
 s,
 1);
+    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
      piix4_dev = dev;
  }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true,

"PIIX4");

hw/isa/piix4.c +0 -4

-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-
  static void piix4_class_init(ObjectClass *klass, void *data)
  {
      DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +0 -0

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
      SysBusDevice parent_obj;
 

Quoted 3 lines

hw/mips/mips_malta.c typedef struct { · line 100

Quoted 3 lines

hw/mips/mips_malta.c +1 -1

      MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];

16 -> ISA_NUM_IRQS

Quoted 79 lines

Quoted 79 lines

hw/mips/mips_malta.c +1 -4

  } MaltaState;
 
  static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
      int64_t kernel_entry, bootloader_run_addr;
      PCIBus *pci_bus;
      ISABus *isa_bus;
-    qemu_irq *isa_irq;
      qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
      int piix4_devfn;
      I2CBus *smbus;
      DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState
 *machine)
      /* Board ID = 0x420 (Malta Board with CoreLV) */
      stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10,
 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on
 isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA

depends
- * on piix4, and piix4 depends on pci_bus. To stop the
cycle we
have
- * qemu_irq_proxy() adds an extra bit of indirection,
allowing
us
- * to resolve the isa_irq -> i8259 dependency after i8259 is
initialized.

hw/mips/mips_malta.c +2 -5

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
      /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
      /* Southbridge */
      ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10,

0),

hw/mips/mips_malta.c +4 -3

+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 
-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie

interrupt 2

hw/mips/mips_malta.c +5 -3

-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);
+    /* 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);
+    }
 
-    isa_bus_irqs(isa_bus, s->i8259);
      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-

uhci");
      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -0

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type,

const
char *pci_type,
PCIBus *find_i440fx(void);
/* piix4.c */
extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);

Quoted 2 lines

Quoted 2 lines

  /* pc_sysfw.c */
  void pc_system_flash_create(PCMachineState *pcms);

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out.
Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++--------
---
 hw/mips/mips_malta.c | 32 +++++++++++++-------------------
 include/hw/i386/pc.h |  1 -
 3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +3 -0

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/irq.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
 typedef struct PIIX4State {
     PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
     /* Reset Control Register */
     MemoryRegion rcr_mem;

hw/isa/piix4.c +7 -0

+
+static void piix4_set_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)

{

hw/isa/piix4.c +17 -11

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {
+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
         return;
     }
 
+    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
+                            "isa", ISA_NUM_IRQS);
+    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
+                             "intr", 1);
+
     memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops,
 s,
                           "reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev),
 0xcf9,
                                         &s->rcr_mem, 1);
 
+    /* initialize i8259 pic */
+    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s,
 1);
+    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
     piix4_dev = dev;
 }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-

static void piix4_class_init(ObjectClass *klass, void *data)

 {
     DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +2 -5

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
     SysBusDevice parent_obj;
 
     MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];
 } MaltaState;
 
 static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
     int64_t kernel_entry, bootloader_run_addr;
     PCIBus *pci_bus;
     ISABus *isa_bus;
-    qemu_irq *isa_irq;
     qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
     int piix4_devfn;
     I2CBus *smbus;
     DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
     /* Board ID = 0x420 (Malta Board with CoreLV) */
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on
 isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA

depends
- * on piix4, and piix4 depends on pci_bus. To stop the cycle we
have
- * qemu_irq_proxy() adds an extra bit of indirection, allowing
us
- * to resolve the isa_irq -> i8259 dependency after i8259 is
initialized.

hw/mips/mips_malta.c +11 -11

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
     /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 
-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);
+    /* 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);
+    }
 
-    isa_bus_irqs(isa_bus, s->i8259);
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -1

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const
 char *pci_type,
 PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */

void pc_system_flash_create(PCMachineState *pcms);

Quoted 47 lines · Philippe Mathieu-Daudé

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out.
Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
  hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++--------
---
  hw/mips/mips_malta.c | 32 +++++++++++++-------------------
  include/hw/i386/pc.h |  1 -
  3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +3 -0

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
   */
 
  #include "qemu/osdep.h"
+#include "hw/irq.h"
  #include "hw/i386/pc.h"
  #include "hw/pci/pci.h"
  #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
  typedef struct PIIX4State {
      PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
      /* Reset Control Register */
      MemoryRegion rcr_mem;

@@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
      }
  };
 

hw/isa/piix4.c static const VMStateDescription vmstate_piix4 = { · line 104

Quoted 6 lines

hw/isa/piix4.c +5 -0

+static void piix4_request_i8259_irq(void *opaque, int irq, int
 level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}

I would like to understand why in PIIX4State *s = opaque; its not necessary a cast or a object macro magic.
Something like:
PIIX4State *s = (PIIX4State*)opaque;
PIIX4State *s = PIIX4STATE(opaque);

Quoted 141 lines

hw/isa/piix4.c +7 -0

+
+static void piix4_set_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+
  static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                              unsigned int len)

  {

hw/isa/piix4.c +17 -11

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
  static void piix4_realize(PCIDevice *dev, Error **errp)
  {
      PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {
+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
          return;
      }
 
+    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
+                            "isa", ISA_NUM_IRQS);
+    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
+                             "intr", 1);
+
      memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops,
 s,
                            "reset-control", 1);
      memory_region_add_subregion_overlap(pci_address_space_io(dev),
 0xcf9,
                                          &s->rcr_mem, 1);
 
+    /* initialize i8259 pic */
+    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s,
 1);
+    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
      piix4_dev = dev;
  }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-

  static void piix4_class_init(ObjectClass *klass, void *data)
  {
      DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +2 -5

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
      SysBusDevice parent_obj;
 
      MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];
  } MaltaState;
 
  static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
      int64_t kernel_entry, bootloader_run_addr;
      PCIBus *pci_bus;
      ISABus *isa_bus;
-    qemu_irq *isa_irq;
      qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
      int piix4_devfn;
      I2CBus *smbus;
      DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
      /* Board ID = 0x420 (Malta Board with CoreLV) */
      stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on
 isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA

depends
- * on piix4, and piix4 depends on pci_bus. To stop the cycle we
have
- * qemu_irq_proxy() adds an extra bit of indirection, allowing
us
- * to resolve the isa_irq -> i8259 dependency after i8259 is
initialized.

hw/mips/mips_malta.c +11 -11

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
      /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
      /* Southbridge */
      ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 
-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);
+    /* 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);
+    }
 
-    isa_bus_irqs(isa_bus, s->i8259);
      pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
      pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
      smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -1

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const
 char *pci_type,
  PCIBus *find_i440fx(void);
  /* piix4.c */
  extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 

Quoted 2 lines

include/hw/i386/pc.h +0 -0

  /* pc_sysfw.c */

  void pc_system_flash_create(PCMachineState *pcms);

hw/isa/piix4.c static const VMStateDescription vmstate_piix4 = { · line 104

Quoted 6 lines · Philippe Mathieu-Daudé

+static void piix4_request_i8259_irq(void *opaque, int irq, int level)

hw/isa/piix4.c +4 -0

+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}

I would like to understand why in PIIX4State *s = opaque; its not necessary a cast or a object macro magic.
Something like:
PIIX4State *s = (PIIX4State*)opaque;
PIIX4State *s = PIIX4STATE(opaque);

Quoted 13 lines · Esteban Bosse

hw/isa/piix4.c static const VMStateDescription vmstate_piix4 = { · line 104

Quoted 6 lines · Philippe Mathieu-Daudé

+static void piix4_request_i8259_irq(void *opaque, int irq, int level)

hw/isa/piix4.c +4 -0

+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}

I would like to understand why in PIIX4State *s = opaque; its not necessary a cast or a object macro magic.
Something like:
PIIX4State *s = (PIIX4State*)opaque;
PIIX4State *s = PIIX4STATE(opaque);

The simple answer to "why don't we need a cast" is "because the type of 'opaque' is 'void *', and in C there is no need to explicitly cast a 'void *' as it will be implicitly converted to the pointer type of the destination". (This is different from C++, which does require an explicit cast for void*.)

For QOM types, QEMU conventionally uses the QOM casting macro to convert a pointer-to-instance to pointer-to-instance-of-parent-class and vice versa. In some places, like this one, what we have is just a void* representing opaque data having been passed around. You could use the QOM cast macro here, which would add a bit of extra type-safety, but the project doesn't have a strong convention here on whether to do so or not, so you'll often see the just-assignment code.

thanks
-- PMM

From: Hervé Poussineau <[email protected]>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out. Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++-----------
 hw/mips/mips_malta.c | 32 +++++++++++++-------------------
 include/hw/i386/pc.h |  1 -
 3 files changed, 45 insertions(+), 31 deletions(-)

hw/isa/piix4.c +32 -11

index d0b18e0586..9c37c85ae2 100644
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/irq.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"

@@ -36,6 +37,8 @@ PCIDevice *piix4_dev;
 
 typedef struct PIIX4State {
     PCIDevice dev;
+    qemu_irq cpu_intr;
+    qemu_irq *isa;
 
     /* Reset Control Register */
     MemoryRegion rcr_mem;

@@ -94,6 +97,18 @@ static const VMStateDescription vmstate_piix4 = {
     }
 };
 
+static void piix4_request_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->cpu_intr, level);
+}
+
+static void piix4_set_i8259_irq(void *opaque, int irq, int level)
+{
+    PIIX4State *s = opaque;
+    qemu_set_irq(s->isa[irq], level);
+}
+
 static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
                             unsigned int len)
 {

@@ -127,29 +142,35 @@ static const MemoryRegionOps piix4_rcr_ops = {
 static void piix4_realize(PCIDevice *dev, Error **errp)
 {
     PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    ISABus *isa_bus;
+    qemu_irq *i8259_out_irq;
 
-    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
-                     pci_address_space_io(dev), errp)) {
+    isa_bus = isa_bus_new(DEVICE(dev), pci_address_space(dev),
+                          pci_address_space_io(dev), errp);
+    if (!isa_bus) {
         return;
     }
 
+    qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
+                            "isa", ISA_NUM_IRQS);
+    qdev_init_gpio_out_named(DEVICE(dev), &s->cpu_intr,
+                             "intr", 1);
+
     memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
                           "reset-control", 1);
     memory_region_add_subregion_overlap(pci_address_space_io(dev), 0xcf9,
                                         &s->rcr_mem, 1);
 
+    /* initialize i8259 pic */
+    i8259_out_irq = qemu_allocate_irqs(piix4_request_i8259_irq, s, 1);
+    s->isa = i8259_init(isa_bus, *i8259_out_irq);
+
+    /* initialize ISA irqs */
+    isa_bus_irqs(isa_bus, s->isa);
+
     piix4_dev = dev;
 }
 
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, "PIIX4");
-    *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-    return d->devfn;
-}
-
 static void piix4_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);

hw/mips/mips_malta.c +2 -8

index 4d9c64b36a..7d25ab6c23 100644
@@ -97,7 +97,7 @@ typedef struct {
     SysBusDevice parent_obj;
 
     MIPSCPSState cps;
-    qemu_irq *i8259;
+    qemu_irq i8259[16];
 } MaltaState;
 
 static ISADevice *pit;

@@ -1235,8 +1235,8 @@ void mips_malta_init(MachineState *machine)
     int64_t kernel_entry, bootloader_run_addr;
     PCIBus *pci_bus;
     ISABus *isa_bus;
-    qemu_irq *isa_irq;
     qemu_irq cbus_irq, i8259_irq;
+    PCIDevice *pci;
     int piix4_devfn;
     I2CBus *smbus;
     DriveInfo *dinfo;

@@ -1407,30 +1407,24 @@ void mips_malta_init(MachineState *machine)
     /* Board ID = 0x420 (Malta Board with CoreLV) */
     stl_p(memory_region_get_ram_ptr(bios_copy) + 0x10, 0x00000420);
 
-    /*
-     * We have a circular dependency problem: pci_bus depends on isa_irq,
-     * isa_irq is provided by i8259, i8259 depends on ISA, ISA depends
-     * on piix4, and piix4 depends on pci_bus.  To stop the cycle we have
-     * qemu_irq_proxy() adds an extra bit of indirection, allowing us
-     * to resolve the isa_irq -> i8259 dependency after i8259 is

initialized.

hw/mips/mips_malta.c +11 -11

-     */
-    isa_irq = qemu_irq_proxy(&s->i8259, 16);
-
     /* Northbridge */
-    pci_bus = gt64120_register(isa_irq);
+    pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
-    piix4_devfn = piix4_init(pci_bus, &isa_bus, 80);
+    pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
+                                          true, "PIIX4");
+    dev = DEVICE(pci);
+    isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
+    piix4_devfn = pci->devfn;
 
-    /*
-     * Interrupt controller
-     * The 8259 is attached to the MIPS CPU INT0 pin, ie interrupt 2
-     */
-    s->i8259 = i8259_init(isa_bus, i8259_irq);
+    /* 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);
+    }
 
-    isa_bus_irqs(isa_bus, s->i8259);
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
     pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

include/hw/i386/pc.h +0 -1

index 37bfd95113..374f3e8835 100644
@@ -286,7 +286,6 @@ PCIBus *i440fx_init(const char *host_type, const char
 *pci_type,
 PCIBus *find_i440fx(void);
 /* piix4.c */
 extern PCIDevice *piix4_dev;
-int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
 
 /* pc_sysfw.c */

void pc_system_flash_create(PCMachineState *pcms);
--
2.21.0

On Friday, October 18, 2019, Philippe Mathieu-Daudé <[email protected] <mailto:[email protected]>> wrote:

From: Hervé Poussineau <[email protected]
<mailto:[email protected]>>

Add ISA irqs as piix4 gpio in, and CPU interrupt request as piix4 gpio out.
Remove i8259 instanciated in malta board, to not have it twice.

We can also remove the now unused piix4_init() function.

Acked-by Michael S. Tsirkin <[email protected] <mailto:[email protected]>>

Acked-by Paolo Bonzini <[email protected]

<mailto:[email protected]>>

Signed-off-by Hervé Poussineau <[email protected]

<mailto:[email protected]>>
Message-Id: <[email protected] <mailto:[email protected]>>

Reviewed-by Aleksandar Markovic <[email protected]

<mailto:[email protected]>>

[PMD: rebased, updated includes, use ISA_NUM_IRQS in for loop]

Signed-off-by Philippe Mathieu-Daudé <[email protected]

<mailto:[email protected]>>
---
 hw/isa/piix4.c       | 43 ++++++++++++++++++++++++++++++++----------- hw/mips/mips_malta.c | 32 +++++++++++++------------------- include/hw/i386/pc.h |  1 -
 3 files changed, 45 insertions(+), 31 deletions(-)

A detail: In the title:
Add a i8259  -> Add an i8259

From: Philippe Mathieu-Daudé <[email protected]>

This function isn't used anymore.

This reverts commit 22ec3283efba9ba0792790da786d6776d83f2a92.

Reviewed-by Thomas Huth <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/core/irq.c    | 14 --------------
 include/hw/irq.h |  5 -----
 2 files changed, 19 deletions(-)

hw/core/irq.c +0 -14

index 7cc0295d0e..fb3045b912 100644
@@ -120,20 +120,6 @@ qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2)
     return qemu_allocate_irq(qemu_splitirq, s, 0);
 }
 
-static void proxy_irq_handler(void *opaque, int n, int level)
-{
-    qemu_irq **target = opaque;
-
-    if (*target) {
-        qemu_set_irq((*target)[n], level);
-    }
-}
-
-qemu_irq *qemu_irq_proxy(qemu_irq **target, int n)
-{
-    return qemu_allocate_irqs(proxy_irq_handler, target, n);
-}
-
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler,
 int n)
 {

     int i;

include/hw/irq.h +0 -5

index fe527f6f51..24ba0ece11 100644
@@ -51,11 +51,6 @@ qemu_irq qemu_irq_invert(qemu_irq irq);
  */
 qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2);
 
-/* Returns a new IRQ set which connects 1:1 to another IRQ set, which
- * may be set later.
- */
-qemu_irq *qemu_irq_proxy(qemu_irq **target, int n);
-
 /* For internal use in qtest.  Similar to qemu_irq_split, but operating
    on an existing vector of qemu_irq.  */
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler,

int n);
--
2.21.0

From: Philippe Mathieu-Daudé <[email protected]>

This function isn't used anymore.

This reverts commit 22ec3283efba9ba0792790da786d6776d83f2a92.

Reviewed-by Thomas Huth <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/core/irq.c    | 14 --------------
 include/hw/irq.h |  5 -----
 2 files changed, 19 deletions(-)

hw/core/irq.c +0 -14

index 7cc0295d0e..fb3045b912 100644
@@ -120,20 +120,6 @@ qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq
 irq2)
     return qemu_allocate_irq(qemu_splitirq, s, 0);
 }
 
-static void proxy_irq_handler(void *opaque, int n, int level)
-{
-    qemu_irq **target = opaque;
-
-    if (*target) {
-        qemu_set_irq((*target)[n], level);
-    }
-}
-
-qemu_irq *qemu_irq_proxy(qemu_irq **target, int n)
-{
-    return qemu_allocate_irqs(proxy_irq_handler, target, n);
-}
-
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler
 handler, int n)

 {
     int i;

include/hw/irq.h +0 -1

index fe527f6f51..24ba0ece11 100644
@@ -51,11 +51,6 @@ qemu_irq qemu_irq_invert(qemu_irq irq);
  */
 qemu_irq qemu_irq_split(qemu_irq irq1, qemu_irq irq2);
 
-/* Returns a new IRQ set which connects 1:1 to another IRQ set,

which
- * may be set later.
- */

include/hw/irq.h +0 -2

-qemu_irq *qemu_irq_proxy(qemu_irq **target, int n);
-
 /* For internal use in qtest.  Similar to qemu_irq_split, but

operating
on an existing vector of qemu_irq. */
void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler
handler, int n);

From: Hervé Poussineau <[email protected]>

Other piix4 parts are already named piix4-ide and piix4-usb-uhci.

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 1 -
 hw/mips/mips_malta.c | 2 +-
 include/hw/isa/isa.h | 2 ++
 3 files changed, 3 insertions(+), 2 deletions(-)

hw/isa/piix4.c +0 -1

index 9c37c85ae2..ac9383a658 100644
@@ -45,7 +45,6 @@ typedef struct PIIX4State {
     uint8_t rcr;
 } PIIX4State;
 
-#define TYPE_PIIX4_PCI_DEVICE "PIIX4"
 #define PIIX4_PCI_DEVICE(obj) \
     OBJECT_CHECK(PIIX4State, (obj), TYPE_PIIX4_PCI_DEVICE)
 

hw/mips/mips_malta.c +1 -1

index 7d25ab6c23..e499b7a6bb 100644
@@ -1414,7 +1414,7 @@ void mips_malta_init(MachineState *machine)
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
     pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
-                                          true, "PIIX4");
+                                          true, TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
     isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
     piix4_devfn = pci->devfn;

include/hw/isa/isa.h +2 -1

index 018ada4f6f..79f703fd6c 100644
@@ -147,4 +147,6 @@ static inline ISABus *isa_bus_from_device(ISADevice *d)
     return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
 }
 
+#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
+
 #endif
--

2.21.0

From: Hervé Poussineau <[email protected]>

Other piix4 parts are already named piix4-ide and piix4-usb-uhci.

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 1 -
 hw/mips/mips_malta.c | 2 +-
 include/hw/isa/isa.h | 2 ++
 3 files changed, 3 insertions(+), 2 deletions(-)

hw/isa/piix4.c +0 -1

index 9c37c85ae2..ac9383a658 100644
@@ -45,7 +45,6 @@ typedef struct PIIX4State {
     uint8_t rcr;
 } PIIX4State;
 
-#define TYPE_PIIX4_PCI_DEVICE "PIIX4"
 #define PIIX4_PCI_DEVICE(obj) \
     OBJECT_CHECK(PIIX4State, (obj), TYPE_PIIX4_PCI_DEVICE)
 

hw/mips/mips_malta.c +1 -1

index 7d25ab6c23..e499b7a6bb 100644
@@ -1414,7 +1414,7 @@ void mips_malta_init(MachineState *machine)
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
     pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
-                                          true, "PIIX4");
+                                          true,
 TYPE_PIIX4_PCI_DEVICE);
     dev = DEVICE(pci);
     isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));

     piix4_devfn = pci->devfn;

include/hw/isa/isa.h +2 -0

index 018ada4f6f..79f703fd6c 100644
@@ -147,4 +147,6 @@ static inline ISABus
 *isa_bus_from_device(ISADevice *d)
     return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
 }
 
+#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
+

#endif

From: Hervé Poussineau <[email protected]>

Remove i8257 instantiated in malta board, to not have it twice.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 4 ++++
 hw/mips/mips_malta.c | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

hw/isa/piix4.c +1 -0

index ac9383a658..0b24d8323c 100644
@@ -29,6 +29,7 @@
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/dma/i8257.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"

hw/mips/mips_malta.c +0 -3

index e499b7a6bb..df247177ca 100644
@@ -28,7 +28,6 @@
 #include "cpu.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/superio.h"
-#include "hw/dma/i8257.h"
 #include "hw/char/serial.h"
 #include "net/net.h"
 #include "hw/boards.h"

@@ -1430,7 +1429,6 @@ void mips_malta_init(MachineState *machine)
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
                           isa_get_irq(NULL, 9), NULL, 0, NULL);
     pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
-    i8257_dma_init(isa_bus, 0);
     mc146818_rtc_init(isa_bus, 2000, NULL);
 
     /* generate SPD EEPROM data */
--

2.21.0

Philippe Mathieu-Daudé <[email protected] <mailto:[email protected]>> 于 2019年10月18日周五 下午9:55写道：

    From: Hervé Poussineau <[email protected]
    <mailto:[email protected]>>

    Remove i8257 instantiated in malta board, to not have it twice.

    Acked-by: Michael S. Tsirkin <[email protected] <mailto:[email protected]>>
    Acked-by: Paolo Bonzini <[email protected]
    <mailto:[email protected]>>
    Signed-off-by: Hervé Poussineau <[email protected]
    <mailto:[email protected]>>
    Message-Id: <[email protected]
    <mailto:[email protected]>>
    Reviewed-by: Aleksandar Markovic <[email protected]
    <mailto:[email protected]>>
    [PMD: rebased]
    Signed-off-by: Philippe Mathieu-Daudé <[email protected]
    <mailto:[email protected]>>
    ---
      hw/isa/piix4.c       | 4 ++++
      hw/mips/mips_malta.c | 2 --
      2 files changed, 4 insertions(+), 2 deletions(-)

    diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
    index ac9383a658..0b24d8323c 100644
    --- a/hw/isa/piix4.c
    +++ b/hw/isa/piix4.c
    @@ -29,6 +29,7 @@
      #include "hw/pci/pci.h"
      #include "hw/isa/isa.h"
      #include "hw/sysbus.h"
    +#include "hw/dma/i8257.h"
      #include "migration/vmstate.h"
      #include "sysemu/reset.h"
      #include "sysemu/runstate.h"
    @@ -167,6 +168,9 @@ static void piix4_realize(PCIDevice *dev, Error
    **errp)
          /* initialize ISA irqs */
          isa_bus_irqs(isa_bus, s->isa);

    +    /* DMA */
    +    i8257_dma_init(isa_bus, 0);
    +
          piix4_dev = dev;
      }

Could you please explain why this is better calling 'i8257_dma_init' in piix4 realize function instead of calling it in mips_malta_init.

I'm still a little of which things should be done in realize and which should be done in qom instance init function.

Thanks,
Li Qiang

    diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
    index e499b7a6bb..df247177ca 100644
    --- a/hw/mips/mips_malta.c
    +++ b/hw/mips/mips_malta.c
    @@ -28,7 +28,6 @@
      #include "cpu.h"
      #include "hw/i386/pc.h"
      #include "hw/isa/superio.h"
    -#include "hw/dma/i8257.h"
      #include "hw/char/serial.h"
      #include "net/net.h"
      #include "hw/boards.h"
    @@ -1430,7 +1429,6 @@ void mips_malta_init(MachineState *machine)
          smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,

                                isa_get_irq(NULL, 9), NULL, 0, NULL);
          pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
    -    i8257_dma_init(isa_bus, 0);
          mc146818_rtc_init(isa_bus, 2000, NULL);

          /* generate SPD EEPROM data */
    -- 
    2.21.0

From: Hervé Poussineau <[email protected]>

Remove i8257 instantiated in malta board, to not have it twice.

Acked-by Michael S. Tsirkin <[email protected]>

Acked-by Paolo Bonzini <[email protected]>

Signed-off-by Hervé Poussineau <[email protected]>

Message-Id: <[email protected]>

Reviewed-by Aleksandar Markovic <[email protected]>

[PMD: rebased]

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/isa/piix4.c       | 4 ++++
 hw/mips/mips_malta.c | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

hw/isa/piix4.c +4 -0

index ac9383a658..0b24d8323c 100644
@@ -29,6 +29,7 @@
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/dma/i8257.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"

@@ -167,6 +168,9 @@ static void piix4_realize(PCIDevice *dev, Error
 **errp)
     /* initialize ISA irqs */
     isa_bus_irqs(isa_bus, s->isa);
 
+    /* DMA */
+    i8257_dma_init(isa_bus, 0);
+
     piix4_dev = dev;
 }

hw/mips/mips_malta.c +0 -1

index e499b7a6bb..df247177ca 100644
@@ -28,7 +28,6 @@
 #include "cpu.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/superio.h"
-#include "hw/dma/i8257.h"
 #include "hw/char/serial.h"
 #include "net/net.h"
 #include "hw/boards.h"

In the next commit we'll refactor the PIIX4 code out of mips_malta_init(). As a preliminary step, add the 'ide_drives' variable and create the drive array dynamically.

Reviewed-by Aleksandar Markovic <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/mips/mips_malta.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

hw/mips/mips_malta.c +5 -3

index 528c34a1c3..774bb810f6 100644
@@ -1235,7 +1235,8 @@ void mips_malta_init(MachineState *machine)
     int piix4_devfn;
     I2CBus *smbus;
     DriveInfo *dinfo;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
+    const size_t ide_drives = MAX_IDE_BUS * MAX_IDE_DEVS;
+    DriveInfo **hd;
     int fl_idx = 0;
     int be;
 

@@ -1406,7 +1407,8 @@ void mips_malta_init(MachineState *machine)
     pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
-    ide_drive_get(hd, ARRAY_SIZE(hd));
+    hd = g_new(DriveInfo *, ide_drives);
+    ide_drive_get(hd, ide_drives);
 
     pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
                                           true, TYPE_PIIX4_PCI_DEVICE);

@@ -1421,6 +1423,7 @@ void mips_malta_init(MachineState *machine)
     }
 
     pci_piix4_ide_init(pci_bus, hd, piix4_devfn + 1);
+    g_free(hd);
     pci_create_simple(pci_bus, piix4_devfn + 2, "piix4-usb-uhci");
     smbus = piix4_pm_init(pci_bus, piix4_devfn + 3, 0x1100,
                           isa_get_irq(NULL, 9), NULL, 0, NULL);
--