Re: [PATCH v2 10/15] hw/southbridge/ich9: Add the DMI-to-PCI bridge

[Bernhard Beschow]

Am 26. Februar 2024 11:14:09 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>Instantiate TYPE_ICH_DMI_PCI_BRIDGE in TYPE_ICH9_SOUTHBRIDGE.
>
>Since the Q35 machine doesn't use it, add the 'd2p-enabled'
>property to disable it.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> include/hw/southbridge/ich9.h |  9 ---------
> hw/i386/pc_q35.c              |  1 +
> hw/southbridge/ich9.c         | 27 +++++++++++++++++++++++++++
> hw/southbridge/Kconfig        |  1 +
> 4 files changed, 29 insertions(+), 9 deletions(-)
>
>diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
>index 162ae3baa1..b9122d299d 100644
>--- a/include/hw/southbridge/ich9.h
>+++ b/include/hw/southbridge/ich9.h
>@@ -108,15 +108,6 @@ struct ICH9LPCState {
> #define ICH9_USB_UHCI1_DEV                      29
> #define ICH9_USB_UHCI1_FUNC                     0
> 
>-/* D30:F0 DMI-to-PCI bridge */
>-#define ICH9_D2P_BRIDGE                         "ICH9 D2P BRIDGE"
>-#define ICH9_D2P_BRIDGE_SAVEVM_VERSION          0
>-
>-#define ICH9_D2P_BRIDGE_DEV                     30
>-#define ICH9_D2P_BRIDGE_FUNC                    0
>-
>-#define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
>-
> /* D31:F0 LPC Processor Interface */
> #define ICH9_RST_CNT_IOPORT                     0xCF9
> 
>diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>index 8c8a2f65b8..f951cf1e3a 100644
>--- a/hw/i386/pc_q35.c
>+++ b/hw/i386/pc_q35.c
>@@ -226,6 +226,7 @@ static void pc_q35_init(MachineState *machine)
>     object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
>     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
>                              OBJECT(pcms->pcibus), &error_abort);
>+    qdev_prop_set_bit(ich9, "d2p-enabled", false);
>     qdev_realize_and_unref(ich9, NULL, &error_fatal);
> 
>     /* create ISA bus */
>diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
>index f3a9b932ab..8c4356ff74 100644
>--- a/hw/southbridge/ich9.c
>+++ b/hw/southbridge/ich9.c
>@@ -12,16 +12,23 @@
> #include "hw/qdev-properties.h"
> #include "hw/southbridge/ich9.h"
> #include "hw/pci/pci.h"
>+#include "hw/pci-bridge/ich9_dmi.h"
>+
>+#define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)

Something along the lines of ICH9_DMI_PCI_DEVFN seems more clear to me.

> 
> struct ICH9State {
>     DeviceState parent_obj;
> 
>+    I82801b11Bridge d2p;

Same here and essentially all identifiers and properties with "d2p" in their 
name.

Best regards,
Bernhard

>+
>     PCIBus *pci_bus;
>+    bool d2p_enabled;
> };
> 
> static Property ich9_props[] = {
>     DEFINE_PROP_LINK("mch-pcie-bus", ICH9State, pci_bus,
>                      TYPE_PCIE_BUS, PCIBus *),
>+    DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
>@@ -29,6 +36,22 @@ static void ich9_init(Object *obj)
> {
> }
> 
>+static bool ich9_realize_d2p(ICH9State *s, Error **errp)
>+{
>+    if (!module_object_class_by_name(TYPE_ICH_DMI_PCI_BRIDGE)) {
>+        error_setg(errp, "DMI-to-PCI function not available in this build");
>+        return false;
>+    }
>+    object_initialize_child(OBJECT(s), "d2p", &s->d2p, 
>TYPE_ICH_DMI_PCI_BRIDGE);
>+    qdev_prop_set_int32(DEVICE(&s->d2p), "addr", ICH9_D2P_DEVFN);
>+    if (!qdev_realize(DEVICE(&s->d2p), BUS(s->pci_bus), errp)) {
>+        return false;
>+    }
>+    object_property_add_alias(OBJECT(s), "pci.0", OBJECT(&s->d2p), "pci.0");
>+
>+    return true;
>+}
>+
> static void ich9_realize(DeviceState *dev, Error **errp)
> {
>     ICH9State *s = ICH9_SOUTHBRIDGE(dev);
>@@ -37,6 +60,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
>         error_setg(errp, "'pcie-bus' property must be set");
>         return;
>     }
>+
>+    if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
>+        return;
>+    }
> }
> 
> static void ich9_class_init(ObjectClass *klass, void *data)
>diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
>index 852b7f346f..db7259bf6f 100644
>--- a/hw/southbridge/Kconfig
>+++ b/hw/southbridge/Kconfig
>@@ -3,3 +3,4 @@
> config ICH9
>     bool
>     depends on PCI_EXPRESS
>+    imply I82801B11