qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/6] hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring


From: Mark Cave-Ayland
Subject: Re: [PATCH 3/6] hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring
Date: Sat, 21 May 2022 09:27:18 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0

On 13/05/2022 18:54, Bernhard Beschow wrote:

PCI interrupt wiring and device creation (piix4 only) were performed
in create() functions which are obsolete. Move these tasks into QOM
functions to modernize the code.

In order to avoid duplicate checking for xen_enabled() the piix3 realize
methods are now split.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
  hw/isa/piix3.c | 67 +++++++++++++++++++++++++++++++++-----------------
  hw/isa/piix4.c | 20 +++++++++------
  2 files changed, 57 insertions(+), 30 deletions(-)

diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 7d69420967..d15117a7c7 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -24,6 +24,7 @@
#include "qemu/osdep.h"
  #include "qemu/range.h"
+#include "qapi/error.h"
  #include "hw/southbridge/piix.h"
  #include "hw/irq.h"
  #include "hw/isa/isa.h"
@@ -280,7 +281,7 @@ static const MemoryRegionOps rcr_ops = {
      .endianness = DEVICE_LITTLE_ENDIAN
  };
-static void piix3_realize(PCIDevice *dev, Error **errp)
+static void pci_piix3_realize(PCIDevice *dev, Error **errp)
  {
      PIIX3State *d = PIIX3_PCI_DEVICE(dev);
@@ -305,7 +306,6 @@ static void pci_piix3_class_init(ObjectClass *klass, void *data)
      dc->desc        = "ISA bridge";
      dc->vmsd        = &vmstate_piix3;
      dc->hotpluggable   = false;
-    k->realize      = piix3_realize;
      k->vendor_id    = PCI_VENDOR_ID_INTEL;
      /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
      k->device_id    = PCI_DEVICE_ID_INTEL_82371SB_0;
@@ -329,11 +329,28 @@ static const TypeInfo piix3_pci_type_info = {
      },
  };
+static void piix3_realize(PCIDevice *dev, Error **errp)
+{
+    ERRP_GUARD();
+    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
+
+    pci_piix3_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
+
+    pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
+                 piix3, PIIX_NUM_PIRQS);
+    pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
+};
+

Oooof. So the reason this looks a bit odd is because we don't have an equivalent of device_class_set_parent_realize() for PCIDevice realize(). Having had a look in pci.c this looks like a similar approach for handling errors, execpt that here errp is accessed directly.

I think this is probably the best we can do for now though. Have you tried forcing pci_piix3_realize() to throw an error during testing to make sure this works as expected?

  static void piix3_class_init(ObjectClass *klass, void *data)
  {
      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
k->config_write = piix3_write_config;
+    k->realize = piix3_realize;
  }
static const TypeInfo piix3_info = {
@@ -342,11 +359,33 @@ static const TypeInfo piix3_info = {
      .class_init    = piix3_class_init,
  };
+static void piix3_xen_realize(PCIDevice *dev, Error **errp)
+{
+    ERRP_GUARD();
+    PIIX3State *piix3 = PIIX3_PCI_DEVICE(dev);
+    PCIBus *pci_bus = pci_get_bus(dev);
+
+    pci_piix3_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
+
+    /*
+     * Xen supports additional interrupt routes from the PCI devices to
+     * the IOAPIC: the four pins of each PCI device on the bus are also
+     * connected to the IOAPIC directly.
+     * These additional routes can be discovered through ACPI.
+     */
+    pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
+                 piix3, XEN_PIIX_NUM_PIRQS);
+};
+
  static void piix3_xen_class_init(ObjectClass *klass, void *data)
  {
      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
k->config_write = piix3_write_config_xen;
+    k->realize = piix3_xen_realize;
  };
static const TypeInfo piix3_xen_info = {
@@ -368,27 +407,11 @@ PIIX3State *piix3_create(PCIBus *pci_bus, ISABus 
**isa_bus)
  {
      PIIX3State *piix3;
      PCIDevice *pci_dev;
+    const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
+                                     : TYPE_PIIX3_DEVICE;
- /*
-     * Xen supports additional interrupt routes from the PCI devices to
-     * the IOAPIC: the four pins of each PCI device on the bus are also
-     * connected to the IOAPIC directly.
-     * These additional routes can be discovered through ACPI.
-     */
-    if (xen_enabled()) {
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
-                                                  TYPE_PIIX3_XEN_DEVICE);
-        piix3 = PIIX3_PCI_DEVICE(pci_dev);
-        pci_bus_irqs(pci_bus, xen_piix3_set_irq, xen_pci_slot_get_pirq,
-                     piix3, XEN_PIIX_NUM_PIRQS);
-    } else {
-        pci_dev = pci_create_simple_multifunction(pci_bus, -1, true,
-                                                  TYPE_PIIX3_DEVICE);
-        piix3 = PIIX3_PCI_DEVICE(pci_dev);
-        pci_bus_irqs(pci_bus, piix3_set_irq, pci_slot_get_pirq,
-                     piix3, PIIX_NUM_PIRQS);
-        pci_bus_set_route_irq_fn(pci_bus, piix3_route_intx_pin_to_irq);
-    }
+    pci_dev = pci_create_simple_multifunction(pci_bus, -1, true, type);
+    piix3 = PIIX3_PCI_DEVICE(pci_dev);
      *isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix3), "isa.0"));
return piix3;
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index a223b69e24..134d23aea7 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -204,6 +204,8 @@ static const MemoryRegionOps piix4_rcr_ops = {
  static void piix4_realize(PCIDevice *dev, Error **errp)
  {
      PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+    PCIDevice *pci;
+    PCIBus *pci_bus = pci_get_bus(dev);
      ISABus *isa_bus;
      qemu_irq *i8259_out_irq;
@@ -242,6 +244,15 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
          return;
      }
      s->rtc.irq = isa_get_irq(ISA_DEVICE(&s->rtc), s->rtc.isairq);
+
+    /* IDE */
+    pci = pci_create_simple(pci_bus, dev->devfn + 1, "piix4-ide");
+    pci_ide_create_devs(pci);
+
+    /* USB */
+    pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
+
+    pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
  }
static void piix4_init(Object *obj)
@@ -292,7 +303,6 @@ type_init(piix4_register_types)
DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
  {
-    PIIX4State *s;
      PCIDevice *pci;
      DeviceState *dev;
      int devfn = PCI_DEVFN(10, 0);
@@ -300,22 +310,16 @@ 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"));
      }
- pci = pci_create_simple(pci_bus, devfn + 1, "piix4-ide");
-    pci_ide_create_devs(pci);
-
-    pci_create_simple(pci_bus, devfn + 2, "piix4-usb-uhci");
      if (smbus) {
          *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
                                 qdev_get_gpio_in_named(dev, "isa", 9),
                                 NULL, 0, NULL);
      }
- pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, PIIX_NUM_PIRQS);
-
      return dev;
  }

As long as the error handling works as required:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]