[PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model

[PATCH 00/14] hw/southbridge: Extract ICH9 QOM container model

[Philippe Mathieu-Daudé]

Hi,

I have a long standing southbridge QOM rework branches. Since
Bernhard is actively working on the PIIX, I'll try to refresh
and post. This is also motivated by the Dynamic Machine work
where we are trying to figure the ideal DSL for QEMU, so having
complex models well designed help.

Here we introduce the ICH9 'southbridge' as a QOM container.
Since the chipset comes as a whole, we shouldn't instantiate
its components separately. However in order to maintain old
code we expose some properties to configure the container and
not introduce any change for the Q35 machine. There is no
migration change, only QOM objects moved around.

More work remain in the LPC function (more code to remove from
Q35). Maybe worth doing in parallel with the PIIX to clean both
PC machines.

Also we'd need to decouple the cpu_interrupt() calls between hw/
and target/.

Note that GSI is currently broken [1]. Once the LPC/ISA part is
done, it might be easier to fix it.

Based-on: <20240213043859.61019-1-philmd@linaro.org> [2]
Based-on: <20240219141412.71418-1-philmd@linaro.org> [3]

[1] https://lore.kernel.org/qemu-devel/cd0e13c6-c03d-411f-83a5-1d4d28ea4345@linaro.org/
[2] https://lore.kernel.org/qemu-devel/20240213043859.61019-1-philmd@linaro.org/ (USB)
[3] https://lore.kernel.org/qemu-devel/20240219141412.71418-1-philmd@linaro.org/ (AHCI)

Philippe Mathieu-Daudé (14):
  MAINTAINERS: Add 'ICH9 South Bridge' section
  hw/i386/q35: Add local 'lpc_obj' variable
  hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h'
  hw/acpi/ich9_tco: Include 'ich9' in names
  hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration
  hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'
  hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub
  hw/southbridge/ich9: Add the DMI-to-PCI bridge
  hw/southbridge/ich9: Add a AHCI function
  hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h'
  hw/southbridge/ich9: Add the SMBus function
  hw/southbridge/ich9: Add the USB EHCI/UHCI functions
  hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h'
  hw/southbridge/ich9: Add the LPC / ISA bridge function

 MAINTAINERS                         |  21 ++-
 include/hw/acpi/ich9.h              |  15 ++
 include/hw/acpi/ich9_tco.h          |   6 +-
 include/hw/i2c/smbus_ich9.h         |  25 +++
 include/hw/isa/ich9_lpc.h           | 166 ++++++++++++++++++++
 include/hw/pci-bridge/ich_dmi_pci.h |  20 +++
 include/hw/southbridge/ich9.h       | 235 +---------------------------
 hw/acpi/ich9.c                      |   9 +-
 hw/acpi/ich9_tco.c                  |   5 +-
 hw/i2c/smbus_ich9.c                 |  36 +++--
 hw/i386/acpi-build.c                |   1 +
 hw/i386/pc_q35.c                    | 124 +++------------
 hw/isa/lpc_ich9.c                   |  37 ++++-
 hw/pci-bridge/i82801b11.c           |  11 +-
 hw/southbridge/ich9.c               | 212 +++++++++++++++++++++++++
 tests/qtest/tco-test.c              |   2 +-
 hw/Kconfig                          |   1 +
 hw/i386/Kconfig                     |   3 +-
 hw/meson.build                      |   1 +
 hw/southbridge/Kconfig              |  11 ++
 hw/southbridge/meson.build          |   3 +
 21 files changed, 581 insertions(+), 363 deletions(-)
 create mode 100644 include/hw/i2c/smbus_ich9.h
 create mode 100644 include/hw/isa/ich9_lpc.h
 create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
 create mode 100644 hw/southbridge/ich9.c
 create mode 100644 hw/southbridge/Kconfig
 create mode 100644 hw/southbridge/meson.build

-- 
2.41.0

[PATCH 01/14] MAINTAINERS: Add 'ICH9 South Bridge' section

[Philippe Mathieu-Daudé]

Extract 'ICH9 South Bridge' from the 'PC' section.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d61fb9319..1b210c5cc1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1808,12 +1808,7 @@ F: include/hw/pci-host/i440fx.h
 F: include/hw/pci-host/q35.h
 F: include/hw/pci-host/pam.h
 F: hw/isa/piix.c
-F: hw/isa/lpc_ich9.c
-F: hw/i2c/smbus_ich9.c
 F: hw/acpi/piix4.c
-F: hw/acpi/ich9*.c
-F: include/hw/acpi/ich9*.h
-F: include/hw/southbridge/ich9.h
 F: include/hw/southbridge/piix.h
 F: hw/isa/apm.c
 F: include/hw/isa/apm.h
@@ -2606,6 +2601,16 @@ F: hw/display/edid*
 F: include/hw/display/edid.h
 F: qemu-edid.c
 
+ICH9 South Bridge (82801i)
+M: Michael S. Tsirkin <mst@redhat.com>
+M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
+S: Supported
+F: hw/acpi/ich9*.c
+F: hw/i2c/smbus_ich9.c
+F: hw/isa/lpc_ich9.c
+F: include/hw/acpi/ich9*.h
+F: include/hw/southbridge/ich9.h
+
 PIIX4 South Bridge (i82371AB)
 M: Hervé Poussineau <hpoussin@reactos.org>
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
-- 
2.41.0

[PATCH 02/14] hw/i386/q35: Add local 'lpc_obj' variable

[Philippe Mathieu-Daudé]

Instead of casting OBJECT(lpc) multiple times,
do it once in the new 'lpc_obj' variable.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_q35.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a91f414922..621661a738 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -126,6 +126,7 @@ static void pc_q35_init(MachineState *machine)
     Object *phb;
     PCIBus *host_bus;
     PCIDevice *lpc;
+    Object *lpc_obj;
     DeviceState *lpc_dev;
     BusState *idebus[MAX_SATA_PORTS];
     ISADevice *rtc_state;
@@ -238,6 +239,7 @@ static void pc_q35_init(MachineState *machine)
     /* create ISA bus */
     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
                                 TYPE_ICH9_LPC_DEVICE);
+    lpc_obj = OBJECT(lpc);
     lpc_dev = DEVICE(lpc);
     qdev_prop_set_bit(lpc_dev, "smm-enabled",
                       x86_machine_is_smm_enabled(x86ms));
@@ -246,7 +248,7 @@ static void pc_q35_init(MachineState *machine)
         qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
     }
 
-    rtc_state = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
+    rtc_state = ISA_DEVICE(object_resolve_path_component(lpc_obj, "rtc"));
 
     object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                              TYPE_HOTPLUG_HANDLER,
@@ -254,13 +256,13 @@ static void pc_q35_init(MachineState *machine)
                              object_property_allow_set_link,
                              OBJ_PROP_LINK_STRONG);
     object_property_set_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
-                             OBJECT(lpc), &error_abort);
+                             lpc_obj, &error_abort);
 
-    acpi_pcihp = object_property_get_bool(OBJECT(lpc),
+    acpi_pcihp = object_property_get_bool(lpc_obj,
                                           ACPI_PM_PROP_ACPI_PCIHP_BRIDGE,
                                           NULL);
 
-    keep_pci_slot_hpc = object_property_get_bool(OBJECT(lpc),
+    keep_pci_slot_hpc = object_property_get_bool(lpc_obj,
                                                  "x-keep-pci-slot-hpc",
                                                  NULL);
 
-- 
2.41.0

[PATCH 03/14] hw/acpi/ich9: Restrict definitions from 'hw/southbridge/ich9.h'

[Philippe Mathieu-Daudé]

Restrict ACPI definitions from "hw/southbridge/ich9.h"
to the ACPI files where they are used.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/ich9.h        | 15 +++++++++++++++
 include/hw/southbridge/ich9.h | 18 ------------------
 hw/acpi/ich9.c                |  4 ++++
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 215de3c91f..3587a35c9f 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -91,4 +91,19 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 bool ich9_pm_is_hotpluggable_bus(HotplugHandler *hotplug_dev, BusState *bus);
 
 void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
+
+#define ICH9_PMIO_PM1_STS                       0x00
+#define ICH9_PMIO_PM1_EN                        0x02
+#define ICH9_PMIO_PM1_CNT                       0x04
+#define ICH9_PMIO_PM1_TMR                       0x08
+#define ICH9_PMIO_GPE0_STS                      0x20
+#define ICH9_PMIO_GPE0_EN                       0x28
+#define ICH9_PMIO_GPE0_LEN                      16
+#define ICH9_PMIO_SMI_EN                        0x30
+#define ICH9_PMIO_SMI_EN_APMC_EN                (1 << 5)
+#define ICH9_PMIO_SMI_EN_TCO_EN                 (1 << 13)
+#define ICH9_PMIO_SMI_STS                       0x34
+#define ICH9_PMIO_TCO_RLD                       0x60
+#define ICH9_PMIO_TCO_LEN                       32
+
 #endif /* HW_ACPI_ICH9_H */
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index fd01649d04..1ac4238f7e 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -183,24 +183,6 @@ struct ICH9LPCState {
 /* D31:F0 power management I/O registers
    offset from the address ICH9_LPC_PMBASE */
 
-/* ICH9 LPC PM I/O registers are 128 ports and 128-aligned */
-#define ICH9_PMIO_SIZE                          128
-#define ICH9_PMIO_MASK                          (ICH9_PMIO_SIZE - 1)
-
-#define ICH9_PMIO_PM1_STS                       0x00
-#define ICH9_PMIO_PM1_EN                        0x02
-#define ICH9_PMIO_PM1_CNT                       0x04
-#define ICH9_PMIO_PM1_TMR                       0x08
-#define ICH9_PMIO_GPE0_STS                      0x20
-#define ICH9_PMIO_GPE0_EN                       0x28
-#define ICH9_PMIO_GPE0_LEN                      16
-#define ICH9_PMIO_SMI_EN                        0x30
-#define ICH9_PMIO_SMI_EN_APMC_EN                (1 << 5)
-#define ICH9_PMIO_SMI_EN_TCO_EN                 (1 << 13)
-#define ICH9_PMIO_SMI_STS                       0x34
-#define ICH9_PMIO_TCO_RLD                       0x60
-#define ICH9_PMIO_TCO_LEN                       32
-
 /* FADT ACPI_ENABLE/ACPI_DISABLE */
 #define ICH9_APM_ACPI_ENABLE                    0x2
 #define ICH9_APM_ACPI_DISABLE                   0x3
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index be375a8b9d..228ebc9a1e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -49,6 +49,10 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
 #define ICH9_DEBUG(fmt, ...)    do { } while (0)
 #endif
 
+/* ICH9 LPC PM I/O registers are 128 ports and 128-aligned */
+#define ICH9_PMIO_SIZE                          128
+#define ICH9_PMIO_MASK                          (ICH9_PMIO_SIZE - 1)
+
 static void ich9_pm_update_sci_fn(ACPIREGS *regs)
 {
     ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
-- 
2.41.0

[PATCH 04/14] hw/acpi/ich9_tco: Include 'ich9' in names

[Philippe Mathieu-Daudé]

Make it explicit the following are ICH9 specific:
  acpi_pm_tco_init()  -> ich9_acpi_pm_tco_init()
  vmstate_tco_io_sts  -> vmstate_ich9_sm_tco.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/ich9_tco.h | 5 ++---
 hw/acpi/ich9.c             | 4 ++--
 hw/acpi/ich9_tco.c         | 4 ++--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
index 2562a7cf39..1c99781a79 100644
--- a/include/hw/acpi/ich9_tco.h
+++ b/include/hw/acpi/ich9_tco.h
@@ -75,9 +75,8 @@ typedef struct TCOIORegs {
     MemoryRegion io;
 } TCOIORegs;
 
-/* tco.c */
-void acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
+void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
 
-extern const VMStateDescription vmstate_tco_io_sts;
+extern const VMStateDescription vmstate_ich9_sm_tco;
 
 #endif /* HW_ACPI_TCO_H */
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 228ebc9a1e..660fa6a082 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -186,7 +186,7 @@ static const VMStateDescription vmstate_tco_io_state = {
     .minimum_version_id = 1,
     .needed = vmstate_test_use_tco,
     .fields = (const VMStateField[]) {
-        VMSTATE_STRUCT(tco_regs, ICH9LPCPMRegs, 1, vmstate_tco_io_sts,
+        VMSTATE_STRUCT(tco_regs, ICH9LPCPMRegs, 1, vmstate_ich9_sm_tco,
                        TCOIORegs),
         VMSTATE_END_OF_LIST()
     }
@@ -317,7 +317,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, qemu_irq sci_irq)
     memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN, &pm->io_smi);
 
     if (pm->enable_tco) {
-        acpi_pm_tco_init(&pm->tco_regs, &pm->io);
+        ich9_acpi_pm_tco_init(&pm->tco_regs, &pm->io);
     }
 
     if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge) {
diff --git a/hw/acpi/ich9_tco.c b/hw/acpi/ich9_tco.c
index 81606219f7..dd4aff82e0 100644
--- a/hw/acpi/ich9_tco.c
+++ b/hw/acpi/ich9_tco.c
@@ -224,7 +224,7 @@ static const MemoryRegionOps tco_io_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-void acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
+void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
 {
     *tr = (TCOIORegs) {
         .tco = {
@@ -250,7 +250,7 @@ void acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent)
     memory_region_add_subregion(parent, ICH9_PMIO_TCO_RLD, &tr->io);
 }
 
-const VMStateDescription vmstate_tco_io_sts = {
+const VMStateDescription vmstate_ich9_sm_tco = {
     .name = "tco io device status",
     .version_id = 1,
     .minimum_version_id = 1,
-- 
2.41.0

[PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration

[Philippe Mathieu-Daudé]

Only files including "hw/acpi/ich9_tco.h" require
the ich9_generate_smi() declaration.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/acpi/ich9_tco.h    | 1 +
 include/hw/southbridge/ich9.h | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
index 1c99781a79..68ee64942f 100644
--- a/include/hw/acpi/ich9_tco.h
+++ b/include/hw/acpi/ich9_tco.h
@@ -76,6 +76,7 @@ typedef struct TCOIORegs {
 } TCOIORegs;
 
 void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
+void ich9_generate_smi(void);
 
 extern const VMStateDescription vmstate_ich9_sm_tco;
 
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index 1ac4238f7e..bee522a4cf 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -11,8 +11,6 @@
 #include "qemu/notify.h"
 #include "qom/object.h"
 
-void ich9_generate_smi(void);
-
 #define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
 
 #define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
-- 
2.41.0

Re: [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration

[Philippe Mathieu-Daudé]

On 19/2/24 17:38, Philippe Mathieu-Daudé wrote:
> Only files including "hw/acpi/ich9_tco.h" require
> the ich9_generate_smi() declaration.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/acpi/ich9_tco.h    | 1 +
>   include/hw/southbridge/ich9.h | 2 --
>   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
> index 1c99781a79..68ee64942f 100644
> --- a/include/hw/acpi/ich9_tco.h
> +++ b/include/hw/acpi/ich9_tco.h
> @@ -76,6 +76,7 @@ typedef struct TCOIORegs {
>   } TCOIORegs;
>   
>   void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
> +void ich9_generate_smi(void);

Bah it is only used in hw/acpi/ich9_tco.c, I'll declare it
statically there.

Re: [PATCH 05/14] hw/acpi/ich9_tco: Restrict ich9_generate_smi() declaration

[Philippe Mathieu-Daudé]

On 20/2/24 07:32, Philippe Mathieu-Daudé wrote:
> On 19/2/24 17:38, Philippe Mathieu-Daudé wrote:
>> Only files including "hw/acpi/ich9_tco.h" require
>> the ich9_generate_smi() declaration.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/acpi/ich9_tco.h    | 1 +
>>   include/hw/southbridge/ich9.h | 2 --
>>   2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/include/hw/acpi/ich9_tco.h b/include/hw/acpi/ich9_tco.h
>> index 1c99781a79..68ee64942f 100644
>> --- a/include/hw/acpi/ich9_tco.h
>> +++ b/include/hw/acpi/ich9_tco.h
>> @@ -76,6 +76,7 @@ typedef struct TCOIORegs {
>>   } TCOIORegs;
>>   void ich9_acpi_pm_tco_init(TCOIORegs *tr, MemoryRegion *parent);
>> +void ich9_generate_smi(void);
> 
> Bah it is only used in hw/acpi/ich9_tco.c, I'll declare it
> statically there.

Unfortunately can't do that now because I really don't want
to add a x86 specific dependency here:

../../hw/acpi/ich9_tco.c:35:30: error: use of undeclared identifier 
'CPU_INTERRUPT_SMI'
     cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI);
                              ^

[PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[Philippe Mathieu-Daudé]

Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
"hw/pci-bridge/ich_dmi_pci.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                         |  1 +
 include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
 include/hw/southbridge/ich9.h       |  2 --
 hw/pci-bridge/i82801b11.c           | 11 ++++-------
 4 files changed, 25 insertions(+), 9 deletions(-)
 create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1b210c5cc1..50507c3dd6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
 F: hw/i2c/smbus_ich9.c
 F: hw/isa/lpc_ich9.c
 F: include/hw/acpi/ich9*.h
+F: include/hw/pci-bridge/ich_dmi_pci.h
 F: include/hw/southbridge/ich9.h
 
 PIIX4 South Bridge (i82371AB)
diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
new file mode 100644
index 0000000000..7623b32b8e
--- /dev/null
+++ b/include/hw/pci-bridge/ich_dmi_pci.h
@@ -0,0 +1,20 @@
+/*
+ * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_PCI_BRIDGE_ICH_D2P_H
+#define HW_PCI_BRIDGE_ICH_D2P_H
+
+#include "qom/object.h"
+#include "hw/pci/pci_bridge.h"
+
+#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
+OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
+
+struct I82801b11Bridge {
+    PCIBridge parent_obj;
+};
+
+#endif
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index bee522a4cf..b2abf483e0 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -114,8 +114,6 @@ struct ICH9LPCState {
 
 #define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
 
-#define ICH9_D2P_A2_REVISION                    0x92
-
 /* D31:F0 LPC Processor Interface */
 #define ICH9_RST_CNT_IOPORT                     0xCF9
 
diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index c140919cbc..dd17e35b0a 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -45,7 +45,7 @@
 #include "hw/pci/pci_bridge.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
-#include "hw/southbridge/ich9.h"
+#include "hw/pci-bridge/ich_dmi_pci.h"
 
 /*****************************************************************************/
 /* ICH9 DMI-to-PCI bridge */
@@ -53,11 +53,8 @@
 #define I82801ba_SSVID_SVID     0
 #define I82801ba_SSVID_SSID     0
 
-typedef struct I82801b11Bridge {
-    /*< private >*/
-    PCIBridge parent_obj;
-    /*< public >*/
-} I82801b11Bridge;
+
+#define ICH9_D2P_A2_REVISION                    0x92
 
 static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
 {
@@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo i82801b11_bridge_info = {
-    .name          = "i82801b11-bridge",
+    .name          = TYPE_ICH_DMI_PCI_BRIDGE,
     .parent        = TYPE_PCI_BRIDGE,
     .instance_size = sizeof(I82801b11Bridge),
     .class_init    = i82801b11_bridge_class_init,
-- 
2.41.0

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[BALATON Zoltan]

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

On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
> "hw/pci-bridge/ich_dmi_pci.h" header.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS                         |  1 +
> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h       |  2 --
> hw/pci-bridge/i82801b11.c           | 11 ++++-------
> 4 files changed, 25 insertions(+), 9 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b210c5cc1..50507c3dd6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
> +F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
>
> PIIX4 South Bridge (i82371AB)
> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
> new file mode 100644
> index 0000000000..7623b32b8e
> --- /dev/null
> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
> +#define HW_PCI_BRIDGE_ICH_D2P_H
> +
> +#include "qom/object.h"
> +#include "hw/pci/pci_bridge.h"
> +
> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
> +
> +struct I82801b11Bridge {
> +    PCIBridge parent_obj;
> +};

If this class has no fields of its own why does it need its own state 
struct defined? You could just set .instance_size = sizeof(PCIBridge) in 
the TypeInfo i82801b11_bridge_info below and delete this struct completely 
as it's not even used anywhere. One less needless QOM complication :-) For 
an example see the empty via-mc97 device in hw/audio/via-ac97.c.

Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
hw/pci-bridge/i82801b11.c where this object is defined and the #define 
TYPE_ICH_DMI_PCI_BRIDGE in hw/southbridge/ich9.h and then you don't need 
this header at all so you don't end up with:

4 files changed, 25 insertions(+), 9 deletions(-)

but really simplifying it.

Regards,
BALATON Zoltan

> +
> +#endif
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index bee522a4cf..b2abf483e0 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>
> #define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
>
> -#define ICH9_D2P_A2_REVISION                    0x92
> -
> /* D31:F0 LPC Processor Interface */
> #define ICH9_RST_CNT_IOPORT                     0xCF9
>
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index c140919cbc..dd17e35b0a 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -45,7 +45,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> -#include "hw/southbridge/ich9.h"
> +#include "hw/pci-bridge/ich_dmi_pci.h"
>
> /*****************************************************************************/
> /* ICH9 DMI-to-PCI bridge */
> @@ -53,11 +53,8 @@
> #define I82801ba_SSVID_SVID     0
> #define I82801ba_SSVID_SSID     0
>
> -typedef struct I82801b11Bridge {
> -    /*< private >*/
> -    PCIBridge parent_obj;
> -    /*< public >*/
> -} I82801b11Bridge;
> +
> +#define ICH9_D2P_A2_REVISION                    0x92
>
> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
> {
> @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
> }
>
> static const TypeInfo i82801b11_bridge_info = {
> -    .name          = "i82801b11-bridge",
> +    .name          = TYPE_ICH_DMI_PCI_BRIDGE,
>     .parent        = TYPE_PCI_BRIDGE,
>     .instance_size = sizeof(I82801b11Bridge),
>     .class_init    = i82801b11_bridge_class_init,
>

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[BALATON Zoltan]

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

On Mon, 19 Feb 2024, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich_dmi_pci.h" header.
>> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS                         |  1 +
>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h       |  2 --
>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1b210c5cc1..50507c3dd6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>> F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>> F: include/hw/southbridge/ich9.h
>> 
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>> b/include/hw/pci-bridge/ich_dmi_pci.h
>> new file mode 100644
>> index 0000000000..7623b32b8e
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>> +
>> +#include "qom/object.h"
>> +#include "hw/pci/pci_bridge.h"
>> +
>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>> +
>> +struct I82801b11Bridge {
>> +    PCIBridge parent_obj;
>> +};
>
> If this class has no fields of its own why does it need its own state struct 
> defined? You could just set .instance_size = sizeof(PCIBridge) in the 
> TypeInfo i82801b11_bridge_info below and delete this struct completely as 
> it's not even used anywhere. One less needless QOM complication :-) For an 
> example see the empty via-mc97 device in hw/audio/via-ac97.c.
>
> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in hw/pci-bridge/i82801b11.c 
> where this object is defined and the #define TYPE_ICH_DMI_PCI_BRIDGE in

You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state struct. 
But on second look what is this object at all? It's never instantiated 
anywhere. Is it used somewhere?

Regards,
BALATON Zoltan

> hw/southbridge/ich9.h and then you don't need this header at all so you don't 
> end up with:
>
> 4 files changed, 25 insertions(+), 9 deletions(-)
>
> but really simplifying it.
>
> Regards,
> BALATON Zoltan
>
>> +
>> +#endif
>> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
>> index bee522a4cf..b2abf483e0 100644
>> --- a/include/hw/southbridge/ich9.h
>> +++ b/include/hw/southbridge/ich9.h
>> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>> 
>> #define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
>> 
>> -#define ICH9_D2P_A2_REVISION                    0x92
>> -
>> /* D31:F0 LPC Processor Interface */
>> #define ICH9_RST_CNT_IOPORT                     0xCF9
>> 
>> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>> index c140919cbc..dd17e35b0a 100644
>> --- a/hw/pci-bridge/i82801b11.c
>> +++ b/hw/pci-bridge/i82801b11.c
>> @@ -45,7 +45,7 @@
>> #include "hw/pci/pci_bridge.h"
>> #include "migration/vmstate.h"
>> #include "qemu/module.h"
>> -#include "hw/southbridge/ich9.h"
>> +#include "hw/pci-bridge/ich_dmi_pci.h"
>> 
>> /*****************************************************************************/
>> /* ICH9 DMI-to-PCI bridge */
>> @@ -53,11 +53,8 @@
>> #define I82801ba_SSVID_SVID     0
>> #define I82801ba_SSVID_SSID     0
>> 
>> -typedef struct I82801b11Bridge {
>> -    /*< private >*/
>> -    PCIBridge parent_obj;
>> -    /*< public >*/
>> -} I82801b11Bridge;
>> +
>> +#define ICH9_D2P_A2_REVISION                    0x92
>> 
>> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
>> {
>> @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass 
>> *klass, void *data)
>> }
>> 
>> static const TypeInfo i82801b11_bridge_info = {
>> -    .name          = "i82801b11-bridge",
>> +    .name          = TYPE_ICH_DMI_PCI_BRIDGE,
>>     .parent        = TYPE_PCI_BRIDGE,
>>     .instance_size = sizeof(I82801b11Bridge),
>>     .class_init    = i82801b11_bridge_class_init,
>

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[Philippe Mathieu-Daudé]

On 19/2/24 19:24, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> MAINTAINERS                         |  1 +
>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>> include/hw/southbridge/ich9.h       |  2 --
>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 1b210c5cc1..50507c3dd6 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>> F: hw/i2c/smbus_ich9.c
>>> F: hw/isa/lpc_ich9.c
>>> F: include/hw/acpi/ich9*.h
>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>> F: include/hw/southbridge/ich9.h
>>>
>>> PIIX4 South Bridge (i82371AB)
>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>> new file mode 100644
>>> index 0000000000..7623b32b8e
>>> --- /dev/null
>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>> @@ -0,0 +1,20 @@
>>> +/*
>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>> +
>>> +#include "qom/object.h"
>>> +#include "hw/pci/pci_bridge.h"
>>> +
>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>> +
>>> +struct I82801b11Bridge {
>>> +    PCIBridge parent_obj;
>>> +};
>>
>> If this class has no fields of its own why does it need its own state 
>> struct defined? You could just set .instance_size = sizeof(PCIBridge) 
>> in the TypeInfo i82801b11_bridge_info below and delete this struct 
>> completely as it's not even used anywhere. One less needless QOM 
>> complication :-) For an example see the empty via-mc97 device in 
>> hw/audio/via-ac97.c.
>>
>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>> TYPE_ICH_DMI_PCI_BRIDGE in
> 
> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
> struct. But on second look what is this object at all? It's never 
> instantiated anywhere. Is it used somewhere?

Here my view is we should always define QOM type names in headers
and use them, in particular in the TypeInfo registration. To unify
style and copy/pasting, better use the QOM DECLARE_TYPE macros.
I envision that might help moving toward DSL and have HW modelling
checks done externally, before starting QEMU. But then this is my
view and I dunno about when we'll get that DSL in so I'm OK to
revisit this patch.

> Regards,
> BALATON Zoltan
> 
>> hw/southbridge/ich9.h and then you don't need this header at all so 
>> you don't end up with:
>>
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>
>> but really simplifying it.
>>
>> Regards,
>> BALATON Zoltan
>>
>>> +
>>> +#endif
>>> diff --git a/include/hw/southbridge/ich9.h 
>>> b/include/hw/southbridge/ich9.h
>>> index bee522a4cf..b2abf483e0 100644
>>> --- a/include/hw/southbridge/ich9.h
>>> +++ b/include/hw/southbridge/ich9.h
>>> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>>>
>>> #define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
>>>
>>> -#define ICH9_D2P_A2_REVISION                    0x92
>>> -
>>> /* D31:F0 LPC Processor Interface */
>>> #define ICH9_RST_CNT_IOPORT                     0xCF9
>>>
>>> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>>> index c140919cbc..dd17e35b0a 100644
>>> --- a/hw/pci-bridge/i82801b11.c
>>> +++ b/hw/pci-bridge/i82801b11.c
>>> @@ -45,7 +45,7 @@
>>> #include "hw/pci/pci_bridge.h"
>>> #include "migration/vmstate.h"
>>> #include "qemu/module.h"
>>> -#include "hw/southbridge/ich9.h"
>>> +#include "hw/pci-bridge/ich_dmi_pci.h"
>>>
>>> /*****************************************************************************/
>>> /* ICH9 DMI-to-PCI bridge */
>>> @@ -53,11 +53,8 @@
>>> #define I82801ba_SSVID_SVID     0
>>> #define I82801ba_SSVID_SSID     0
>>>
>>> -typedef struct I82801b11Bridge {
>>> -    /*< private >*/
>>> -    PCIBridge parent_obj;
>>> -    /*< public >*/
>>> -} I82801b11Bridge;
>>> +
>>> +#define ICH9_D2P_A2_REVISION                    0x92
>>>
>>> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
>>> {
>>> @@ -103,7 +100,7 @@ static void 
>>> i82801b11_bridge_class_init(ObjectClass *klass, void *data)
>>> }
>>>
>>> static const TypeInfo i82801b11_bridge_info = {
>>> -    .name          = "i82801b11-bridge",
>>> +    .name          = TYPE_ICH_DMI_PCI_BRIDGE,
>>>     .parent        = TYPE_PCI_BRIDGE,
>>>     .instance_size = sizeof(I82801b11Bridge),
>>>     .class_init    = i82801b11_bridge_class_init,
>>

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[BALATON Zoltan]

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

On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 19/2/24 19:24, BALATON Zoltan wrote:
>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>> 
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> MAINTAINERS                         |  1 +
>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>> include/hw/southbridge/ich9.h       |  2 --
>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>> 
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 1b210c5cc1..50507c3dd6 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>> F: hw/i2c/smbus_ich9.c
>>>> F: hw/isa/lpc_ich9.c
>>>> F: include/hw/acpi/ich9*.h
>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>> F: include/hw/southbridge/ich9.h
>>>> 
>>>> PIIX4 South Bridge (i82371AB)
>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>> new file mode 100644
>>>> index 0000000000..7623b32b8e
>>>> --- /dev/null
>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>> @@ -0,0 +1,20 @@
>>>> +/*
>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + */
>>>> +
>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>> +
>>>> +#include "qom/object.h"
>>>> +#include "hw/pci/pci_bridge.h"
>>>> +
>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>> +
>>>> +struct I82801b11Bridge {
>>>> +    PCIBridge parent_obj;
>>>> +};
>>> 
>>> If this class has no fields of its own why does it need its own state 
>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) in 
>>> the TypeInfo i82801b11_bridge_info below and delete this struct completely 
>>> as it's not even used anywhere. One less needless QOM complication :-) For 
>>> an example see the empty via-mc97 device in hw/audio/via-ac97.c.
>>> 
>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>>> TYPE_ICH_DMI_PCI_BRIDGE in
>> 
>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state struct. 
>> But on second look what is this object at all? It's never instantiated 
>> anywhere. Is it used somewhere?
>
> Here my view is we should always define QOM type names in headers
> and use them, in particular in the TypeInfo registration. To unify
> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
> I envision that might help moving toward DSL and have HW modelling
> checks done externally, before starting QEMU. But then this is my
> view and I dunno about when we'll get that DSL in so I'm OK to
> revisit this patch.

The question here is more if we need this object at all because it wasn't 
enstantiated before, and after your series it could be instantiated by a 
property that's never set. So unless I misunderstood somthing this whole 
thing could just be removed as dead code and let it be re-added later when 
it's actually implemented following whatever conventions we'll have then. 
No need to keep around empty placeholders that aren't used. Or does it 
serve any purpose?

Regards,
BALATON Zoltan

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[Thomas Huth]

On 20/02/2024 13.20, BALATON Zoltan wrote:
> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>> MAINTAINERS                         |  1 +
>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>> include/hw/southbridge/ich9.h       |  2 --
>>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>
>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>> F: hw/i2c/smbus_ich9.c
>>>>> F: hw/isa/lpc_ich9.c
>>>>> F: include/hw/acpi/ich9*.h
>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>> F: include/hw/southbridge/ich9.h
>>>>>
>>>>> PIIX4 South Bridge (i82371AB)
>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>> new file mode 100644
>>>>> index 0000000000..7623b32b8e
>>>>> --- /dev/null
>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>> @@ -0,0 +1,20 @@
>>>>> +/*
>>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>>> + *
>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>> + */
>>>>> +
>>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>>> +
>>>>> +#include "qom/object.h"
>>>>> +#include "hw/pci/pci_bridge.h"
>>>>> +
>>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>>> +
>>>>> +struct I82801b11Bridge {
>>>>> +    PCIBridge parent_obj;
>>>>> +};
>>>>
>>>> If this class has no fields of its own why does it need its own state 
>>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) in 
>>>> the TypeInfo i82801b11_bridge_info below and delete this struct 
>>>> completely as it's not even used anywhere. One less needless QOM 
>>>> complication :-) For an example see the empty via-mc97 device in 
>>>> hw/audio/via-ac97.c.
>>>>
>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>>
>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
>>> struct. But on second look what is this object at all? It's never 
>>> instantiated anywhere. Is it used somewhere?
>>
>> Here my view is we should always define QOM type names in headers
>> and use them, in particular in the TypeInfo registration. To unify
>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>> I envision that might help moving toward DSL and have HW modelling
>> checks done externally, before starting QEMU. But then this is my
>> view and I dunno about when we'll get that DSL in so I'm OK to
>> revisit this patch.
> 
> The question here is more if we need this object at all because it wasn't 
> enstantiated before, and after your series it could be instantiated by a 
> property that's never set. So unless I misunderstood somthing this whole 
> thing could just be removed as dead code and let it be re-added later when 
> it's actually implemented following whatever conventions we'll have then. No 
> need to keep around empty placeholders that aren't used. Or does it serve 
> any purpose?

It's apparently used by some q35 configs:

$ grep -r i82801b11 docs/
docs/config/q35-emulated.cfg:  driver = "i82801b11-bridge"

  Thomas

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[Philippe Mathieu-Daudé]

On 20/2/24 13:55, Thomas Huth wrote:
> On 20/02/2024 13.20, BALATON Zoltan wrote:
>> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>> MAINTAINERS                         |  1 +
>>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>>> include/hw/southbridge/ich9.h       |  2 --
>>>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>>> F: hw/i2c/smbus_ich9.c
>>>>>> F: hw/isa/lpc_ich9.c
>>>>>> F: include/hw/acpi/ich9*.h
>>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> F: include/hw/southbridge/ich9.h
>>>>>>
>>>>>> PIIX4 South Bridge (i82371AB)
>>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..7623b32b8e
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>> @@ -0,0 +1,20 @@
>>>>>> +/*
>>>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>>>> + *
>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>>>> +
>>>>>> +#include "qom/object.h"
>>>>>> +#include "hw/pci/pci_bridge.h"
>>>>>> +
>>>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>>>> +
>>>>>> +struct I82801b11Bridge {
>>>>>> +    PCIBridge parent_obj;
>>>>>> +};
>>>>>
>>>>> If this class has no fields of its own why does it need its own 
>>>>> state struct defined? You could just set .instance_size = 
>>>>> sizeof(PCIBridge) in the TypeInfo i82801b11_bridge_info below and 
>>>>> delete this struct completely as it's not even used anywhere. One 
>>>>> less needless QOM complication :-) For an example see the empty 
>>>>> via-mc97 device in hw/audio/via-ac97.c.
>>>>>
>>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>>>> hw/pci-bridge/i82801b11.c where this object is defined and the 
>>>>> #define TYPE_ICH_DMI_PCI_BRIDGE in
>>>>
>>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
>>>> struct. But on second look what is this object at all? It's never 
>>>> instantiated anywhere. Is it used somewhere?
>>>
>>> Here my view is we should always define QOM type names in headers
>>> and use them, in particular in the TypeInfo registration. To unify
>>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>>> I envision that might help moving toward DSL and have HW modelling
>>> checks done externally, before starting QEMU. But then this is my
>>> view and I dunno about when we'll get that DSL in so I'm OK to
>>> revisit this patch.
>>
>> The question here is more if we need this object at all because it 
>> wasn't enstantiated before, and after your series it could be 
>> instantiated by a property that's never set. So unless I misunderstood 
>> somthing this whole thing could just be removed as dead code and let 
>> it be re-added later when it's actually implemented following whatever 
>> conventions we'll have then. No need to keep around empty placeholders 
>> that aren't used. Or does it serve any purpose?

This isn't a virtual hardware, and is well specified, I'm trying to
plug all the parts we have so the full chipset can be used to create
a dynamic machine.

> It's apparently used by some q35 configs:
> 
> $ grep -r i82801b11 docs/
> docs/config/q35-emulated.cfg:  driver = "i82801b11-bridge"
> 
>   Thomas
> 
> 

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[BALATON Zoltan]

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

On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 20/2/24 13:55, Thomas Huth wrote:
>> On 20/02/2024 13.20, BALATON Zoltan wrote:
>>> On Tue, 20 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> On 19/2/24 19:24, BALATON Zoltan wrote:
>>>>> On Mon, 19 Feb 2024, BALATON Zoltan wrote:
>>>>>> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>>>>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>>>>>> 
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>> MAINTAINERS                         |  1 +
>>>>>>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>>>>>>> include/hw/southbridge/ich9.h       |  2 --
>>>>>>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>>>>>>> 4 files changed, 25 insertions(+), 9 deletions(-)
>>>>>>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> 
>>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>>> index 1b210c5cc1..50507c3dd6 100644
>>>>>>> --- a/MAINTAINERS
>>>>>>> +++ b/MAINTAINERS
>>>>>>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>>>>>>> F: hw/i2c/smbus_ich9.c
>>>>>>> F: hw/isa/lpc_ich9.c
>>>>>>> F: include/hw/acpi/ich9*.h
>>>>>>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> F: include/hw/southbridge/ich9.h
>>>>>>> 
>>>>>>> PIIX4 South Bridge (i82371AB)
>>>>>>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h 
>>>>>>> b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..7623b32b8e
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
>>>>>>> @@ -0,0 +1,20 @@
>>>>>>> +/*
>>>>>>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>>>>>>> + *
>>>>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>>>>>>> +#define HW_PCI_BRIDGE_ICH_D2P_H
>>>>>>> +
>>>>>>> +#include "qom/object.h"
>>>>>>> +#include "hw/pci/pci_bridge.h"
>>>>>>> +
>>>>>>> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>>>>>>> +
>>>>>>> +struct I82801b11Bridge {
>>>>>>> +    PCIBridge parent_obj;
>>>>>>> +};
>>>>>> 
>>>>>> If this class has no fields of its own why does it need its own state 
>>>>>> struct defined? You could just set .instance_size = sizeof(PCIBridge) 
>>>>>> in the TypeInfo i82801b11_bridge_info below and delete this struct 
>>>>>> completely as it's not even used anywhere. One less needless QOM 
>>>>>> complication :-) For an example see the empty via-mc97 device in 
>>>>>> hw/audio/via-ac97.c.
>>>>>> 
>>>>>> Then you can put the OBJECT_DECLARE_SIMPLE_TYPE in 
>>>>>> hw/pci-bridge/i82801b11.c where this object is defined and the #define 
>>>>>> TYPE_ICH_DMI_PCI_BRIDGE in
>>>>> 
>>>>> You don't even need OBJECT_DECLARE_SIMPLE_TYPE if there's no state 
>>>>> struct. But on second look what is this object at all? It's never 
>>>>> instantiated anywhere. Is it used somewhere?
>>>> 
>>>> Here my view is we should always define QOM type names in headers
>>>> and use them, in particular in the TypeInfo registration. To unify
>>>> style and copy/pasting, better use the QOM DECLARE_TYPE macros.
>>>> I envision that might help moving toward DSL and have HW modelling
>>>> checks done externally, before starting QEMU. But then this is my
>>>> view and I dunno about when we'll get that DSL in so I'm OK to
>>>> revisit this patch.
>>> 
>>> The question here is more if we need this object at all because it wasn't 
>>> enstantiated before, and after your series it could be instantiated by a 
>>> property that's never set. So unless I misunderstood somthing this whole 
>>> thing could just be removed as dead code and let it be re-added later when 
>>> it's actually implemented following whatever conventions we'll have then. 
>>> No need to keep around empty placeholders that aren't used. Or does it 
>>> serve any purpose?
>
> This isn't a virtual hardware, and is well specified, I'm trying to
> plug all the parts we have so the full chipset can be used to create
> a dynamic machine.

This isn't prevented by moving this object implementation from its 
separate file to the file where the southbridge that's the sole user of 
this device it implemented. The rationale is that there's no need to make 
it a full object with all the headers and defines when it's not used 
anywhere else than that southbridge and its "implementation" consists of 
only the TypeInfo and a realize method. These could just be defined in the 
southbridge as a local object and do away with all the other boilerplate 
which would remove some unneeded cruft. This won't affect your modelling 
of the southbridge as you can still instantiate it there but don't need to 
define this empty bridge device as external object when it's not needed 
outside of the south bridge. Just like the via-mc97 device. I did not put 
that in a separate file and add a separate header for it because it just 
seems to be overkill for an empty device that does nothing.

Regards,
BALATON Zoltan

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[Bernhard Beschow]

Am 19. Februar 2024 16:38:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>"hw/pci-bridge/ich_dmi_pci.h" header.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> MAINTAINERS                         |  1 +
> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h       |  2 --
> hw/pci-bridge/i82801b11.c           | 11 ++++-------
> 4 files changed, 25 insertions(+), 9 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 1b210c5cc1..50507c3dd6 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
>+F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
> 
> PIIX4 South Bridge (i82371AB)
>diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
>new file mode 100644
>index 0000000000..7623b32b8e
>--- /dev/null
>+++ b/include/hw/pci-bridge/ich_dmi_pci.h

Shouldn't we name the new header like its source file, i.e. i82801b11.h?

>@@ -0,0 +1,20 @@
>+/*
>+ * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>+ *
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>+#define HW_PCI_BRIDGE_ICH_D2P_H
>+
>+#include "qom/object.h"
>+#include "hw/pci/pci_bridge.h"
>+
>+#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
>+OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
>+
>+struct I82801b11Bridge {
>+    PCIBridge parent_obj;
>+};
>+
>+#endif
>diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
>index bee522a4cf..b2abf483e0 100644
>--- a/include/hw/southbridge/ich9.h
>+++ b/include/hw/southbridge/ich9.h
>@@ -114,8 +114,6 @@ struct ICH9LPCState {
> 
> #define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
> 
>-#define ICH9_D2P_A2_REVISION                    0x92
>-
> /* D31:F0 LPC Processor Interface */
> #define ICH9_RST_CNT_IOPORT                     0xCF9
> 
>diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
>index c140919cbc..dd17e35b0a 100644
>--- a/hw/pci-bridge/i82801b11.c
>+++ b/hw/pci-bridge/i82801b11.c
>@@ -45,7 +45,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
>-#include "hw/southbridge/ich9.h"
>+#include "hw/pci-bridge/ich_dmi_pci.h"
> 
> /*****************************************************************************/
> /* ICH9 DMI-to-PCI bridge */
>@@ -53,11 +53,8 @@
> #define I82801ba_SSVID_SVID     0
> #define I82801ba_SSVID_SSID     0
> 
>-typedef struct I82801b11Bridge {
>-    /*< private >*/
>-    PCIBridge parent_obj;
>-    /*< public >*/
>-} I82801b11Bridge;
>+
>+#define ICH9_D2P_A2_REVISION                    0x92
> 
> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
> {
>@@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data)
> }
> 
> static const TypeInfo i82801b11_bridge_info = {
>-    .name          = "i82801b11-bridge",
>+    .name          = TYPE_ICH_DMI_PCI_BRIDGE,
>     .parent        = TYPE_PCI_BRIDGE,
>     .instance_size = sizeof(I82801b11Bridge),
>     .class_init    = i82801b11_bridge_class_init,

Re: [PATCH 06/14] hw/pci-bridge: Extract QOM ICH definitions to 'ich_dmi_pci.h'

[Philippe Mathieu-Daudé]

On 20/2/24 20:25, Bernhard Beschow wrote:
> 
> 
> Am 19. Februar 2024 16:38:46 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich_dmi_pci.h" header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS                         |  1 +
>> include/hw/pci-bridge/ich_dmi_pci.h | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h       |  2 --
>> hw/pci-bridge/i82801b11.c           | 11 ++++-------
>> 4 files changed, 25 insertions(+), 9 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich_dmi_pci.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 1b210c5cc1..50507c3dd6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2609,6 +2609,7 @@ F: hw/acpi/ich9*.c
>> F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> +F: include/hw/pci-bridge/ich_dmi_pci.h
>> F: include/hw/southbridge/ich9.h
>>
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich_dmi_pci.h b/include/hw/pci-bridge/ich_dmi_pci.h
>> new file mode 100644
>> index 0000000000..7623b32b8e
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich_dmi_pci.h
> 
> Shouldn't we name the new header like its source file, i.e. i82801b11.h?

I'll rename sources to use the "ichX_FOO.c" pattern.

>> @@ -0,0 +1,20 @@
>> +/*
>> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef HW_PCI_BRIDGE_ICH_D2P_H
>> +#define HW_PCI_BRIDGE_ICH_D2P_H
...

[PATCH 07/14] hw/southbridge/ich9: Introduce TYPE_ICH9_SOUTHBRIDGE stub

[Philippe Mathieu-Daudé]

Start the TYPE_ICH9_SOUTHBRIDGE stub, a kind of QOM
container which will contain all the ICH9 parts.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                   |  1 +
 include/hw/southbridge/ich9.h |  3 ++
 hw/i386/pc_q35.c              |  7 ++++
 hw/southbridge/ich9.c         | 61 +++++++++++++++++++++++++++++++++++
 hw/Kconfig                    |  1 +
 hw/i386/Kconfig               |  1 +
 hw/meson.build                |  1 +
 hw/southbridge/Kconfig        |  5 +++
 hw/southbridge/meson.build    |  3 ++
 9 files changed, 83 insertions(+)
 create mode 100644 hw/southbridge/ich9.c
 create mode 100644 hw/southbridge/Kconfig
 create mode 100644 hw/southbridge/meson.build

diff --git a/MAINTAINERS b/MAINTAINERS
index 50507c3dd6..d1a2eddd4c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2608,6 +2608,7 @@ S: Supported
 F: hw/acpi/ich9*.c
 F: hw/i2c/smbus_ich9.c
 F: hw/isa/lpc_ich9.c
+F: hw/southbridge/ich9.c
 F: include/hw/acpi/ich9*.h
 F: include/hw/pci-bridge/ich_dmi_pci.h
 F: include/hw/southbridge/ich9.h
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index b2abf483e0..162ae3baa1 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -11,6 +11,9 @@
 #include "qemu/notify.h"
 #include "qom/object.h"
 
+#define TYPE_ICH9_SOUTHBRIDGE "ICH9-southbridge"
+OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
+
 #define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
 
 #define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 621661a738..311ac2be6f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -125,6 +125,7 @@ static void pc_q35_init(MachineState *machine)
     X86MachineState *x86ms = X86_MACHINE(machine);
     Object *phb;
     PCIBus *host_bus;
+    DeviceState *ich9;
     PCIDevice *lpc;
     Object *lpc_obj;
     DeviceState *lpc_dev;
@@ -233,6 +234,12 @@ static void pc_q35_init(MachineState *machine)
     host_bus = PCI_BUS(qdev_get_child_bus(DEVICE(phb), "pcie.0"));
     pcms->bus = host_bus;
 
+    ich9 = qdev_new(TYPE_ICH9_SOUTHBRIDGE);
+    object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
+    object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
+                             OBJECT(host_bus), &error_abort);
+    qdev_realize_and_unref(ich9, NULL, &error_fatal);
+
     /* irq lines */
     gsi_state = pc_gsi_create(&x86ms->gsi, true);
 
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
new file mode 100644
index 0000000000..f3a9b932ab
--- /dev/null
+++ b/hw/southbridge/ich9.c
@@ -0,0 +1,61 @@
+/*
+ * QEMU Intel ICH9 south bridge emulation
+ *
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd
+ * SPDX-FileContributor: Philippe Mathieu-Daudé
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/southbridge/ich9.h"
+#include "hw/pci/pci.h"
+
+struct ICH9State {
+    DeviceState parent_obj;
+
+    PCIBus *pci_bus;
+};
+
+static Property ich9_props[] = {
+    DEFINE_PROP_LINK("mch-pcie-bus", ICH9State, pci_bus,
+                     TYPE_PCIE_BUS, PCIBus *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ich9_init(Object *obj)
+{
+}
+
+static void ich9_realize(DeviceState *dev, Error **errp)
+{
+    ICH9State *s = ICH9_SOUTHBRIDGE(dev);
+
+    if (!s->pci_bus) {
+        error_setg(errp, "'pcie-bus' property must be set");
+        return;
+    }
+}
+
+static void ich9_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = ich9_realize;
+    device_class_set_props(dc, ich9_props);
+    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+}
+
+static const TypeInfo ich9_types[] = {
+    {
+        .name           = TYPE_ICH9_SOUTHBRIDGE,
+        .parent         = TYPE_DEVICE,
+        .instance_size  = sizeof(ICH9State),
+        .instance_init  = ich9_init,
+        .class_init     = ich9_class_init,
+    }
+};
+
+DEFINE_TYPES(ich9_types)
diff --git a/hw/Kconfig b/hw/Kconfig
index 2c00936c28..6584f2f72a 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -36,6 +36,7 @@ source scsi/Kconfig
 source sd/Kconfig
 source sensor/Kconfig
 source smbios/Kconfig
+source southbridge/Kconfig
 source ssi/Kconfig
 source timer/Kconfig
 source tpm/Kconfig
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index a1846be6f7..d21638f4f9 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -99,6 +99,7 @@ config Q35
     select PC_PCI
     select PC_ACPI
     select PCI_EXPRESS_Q35
+    select ICH9
     select LPC_ICH9
     select AHCI_ICH9
     select DIMM
diff --git a/hw/meson.build b/hw/meson.build
index 463d702683..7f9ae8659a 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -33,6 +33,7 @@ subdir('rtc')
 subdir('scsi')
 subdir('sd')
 subdir('sensor')
+subdir('southbridge')
 subdir('smbios')
 subdir('ssi')
 subdir('timer')
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
new file mode 100644
index 0000000000..852b7f346f
--- /dev/null
+++ b/hw/southbridge/Kconfig
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+config ICH9
+    bool
+    depends on PCI_EXPRESS
diff --git a/hw/southbridge/meson.build b/hw/southbridge/meson.build
new file mode 100644
index 0000000000..70c1fa3cb2
--- /dev/null
+++ b/hw/southbridge/meson.build
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+system_ss.add(when: 'CONFIG_ICH9', if_true: files('ich9.c'))
-- 
2.41.0

[PATCH 08/14] hw/southbridge/ich9: Add the DMI-to-PCI bridge

[Philippe Mathieu-Daudé]

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 311ac2be6f..2f15af540f 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -238,6 +238,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(host_bus), &error_abort);
+    qdev_prop_set_bit(ich9, "d2p-enabled", false);
     qdev_realize_and_unref(ich9, NULL, &error_fatal);
 
     /* irq lines */
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index f3a9b932ab..6df47e81fb 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -12,19 +12,42 @@
 #include "hw/qdev-properties.h"
 #include "hw/southbridge/ich9.h"
 #include "hw/pci/pci.h"
+#include "hw/pci-bridge/ich_dmi_pci.h"
+
+#define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
 
 struct ICH9State {
     DeviceState parent_obj;
 
+    I82801b11Bridge d2p;
+
     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(),
 };
 
+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_init(Object *obj)
 {
 }
@@ -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
-- 
2.41.0

[PATCH 09/14] hw/southbridge/ich9: Add a AHCI function

[Philippe Mathieu-Daudé]

Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.

Since the PC machines can disable SATA (see the
PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
property to disable it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                   |  2 ++
 include/hw/southbridge/ich9.h |  4 ----
 hw/i386/pc_q35.c              | 25 ++++---------------------
 hw/southbridge/ich9.c         | 35 +++++++++++++++++++++++++++++++++++
 hw/i386/Kconfig               |  1 -
 hw/southbridge/Kconfig        |  1 +
 6 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index d1a2eddd4c..937ebb5c96 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2607,9 +2607,11 @@ M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
 S: Supported
 F: hw/acpi/ich9*.c
 F: hw/i2c/smbus_ich9.c
+F: hw/ide/ich.c
 F: hw/isa/lpc_ich9.c
 F: hw/southbridge/ich9.c
 F: include/hw/acpi/ich9*.h
+F: include/hw/ide/ahci-pci.h
 F: include/hw/pci-bridge/ich_dmi_pci.h
 F: include/hw/southbridge/ich9.h
 
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index b9122d299d..ac7f9f4ff5 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -166,10 +166,6 @@ struct ICH9LPCState {
 
 #define ICH9_GPIO_GSI "gsi"
 
-/* D31:F2 SATA Controller #1 */
-#define ICH9_SATA1_DEV                          31
-#define ICH9_SATA1_FUNC                         2
-
 /* D31:F0 power management I/O registers
    offset from the address ICH9_LPC_PMBASE */
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 2f15af540f..060358d449 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -61,9 +61,6 @@
 #include "hw/acpi/acpi.h"
 #include "target/i386/cpu.h"
 
-/* ICH9 AHCI has 6 ports */
-#define MAX_SATA_PORTS     6
-
 struct ehci_companions {
     const char *name;
     int func;
@@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
     PCIDevice *lpc;
     Object *lpc_obj;
     DeviceState *lpc_dev;
-    BusState *idebus[MAX_SATA_PORTS];
+    BusState *idebus[2] = { };
     ISADevice *rtc_state;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
@@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
     ISABus *isa_bus;
     int i;
     ram_addr_t lowmem;
-    DriveInfo *hd[MAX_SATA_PORTS];
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     bool acpi_pcihp;
     bool keep_pci_slot_hpc;
@@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
                              OBJECT(host_bus), &error_abort);
     qdev_prop_set_bit(ich9, "d2p-enabled", false);
+    qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
     qdev_realize_and_unref(ich9, NULL, &error_fatal);
 
     /* irq lines */
@@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
                          0xff0104);
 
     if (pcms->sata_enabled) {
-        PCIDevice *pdev;
-        AHCIPCIState *ich9;
-
-        /* ahci and SATA device, for q35 1 ahci controller is built-in */
-        pdev = pci_create_simple_multifunction(host_bus,
-                                               PCI_DEVFN(ICH9_SATA1_DEV,
-                                                         ICH9_SATA1_FUNC),
-                                               "ich9-ahci");
-        ich9 = ICH9_AHCI(pdev);
-        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
-        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
-        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
-        ide_drive_get(hd, ich9->ahci.ports);
-        ahci_ide_create_devs(&ich9->ahci, hd);
-    } else {
-        idebus[0] = idebus[1] = NULL;
+        idebus[0] = qdev_get_child_bus(ich9, "ide.0");
+        idebus[1] = qdev_get_child_bus(ich9, "ide.1");
     }
 
     if (machine_usb(machine)) {
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 6df47e81fb..233dc1c5d7 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -13,22 +13,30 @@
 #include "hw/southbridge/ich9.h"
 #include "hw/pci/pci.h"
 #include "hw/pci-bridge/ich_dmi_pci.h"
+#include "hw/ide/ahci-pci.h"
+#include "hw/ide.h"
 
 #define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
+#define ICH9_SATA1_DEVFN        PCI_DEVFN(31, 2)
+
+#define SATA_PORTS              6
 
 struct ICH9State {
     DeviceState parent_obj;
 
     I82801b11Bridge d2p;
+    AHCIPCIState sata0;
 
     PCIBus *pci_bus;
     bool d2p_enabled;
+    bool sata_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_BOOL("sata-enabled", ICH9State, sata_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -48,6 +56,29 @@ static bool ich9_realize_d2p(ICH9State *s, Error **errp)
     return true;
 }
 
+static bool ich9_realize_sata(ICH9State *s, Error **errp)
+{
+    DriveInfo *hd[SATA_PORTS];
+
+    object_initialize_child(OBJECT(s), "sata[0]", &s->sata0, TYPE_ICH9_AHCI);
+    qdev_prop_set_int32(DEVICE(&s->sata0), "addr", ICH9_SATA1_DEVFN);
+    if (!qdev_realize(DEVICE(&s->sata0), BUS(s->pci_bus), errp)) {
+        return false;
+    }
+    for (unsigned i = 0; i < SATA_PORTS; i++) {
+        g_autofree char *portname = g_strdup_printf("ide.%u", i);
+
+        object_property_add_alias(OBJECT(s), portname,
+                                  OBJECT(&s->sata0), portname);
+    }
+
+    g_assert(SATA_PORTS == s->sata0.ahci.ports);
+    ide_drive_get(hd, s->sata0.ahci.ports);
+    ahci_ide_create_devs(&s->sata0.ahci, hd);
+
+    return true;
+}
+
 static void ich9_init(Object *obj)
 {
 }
@@ -64,6 +95,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
     if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
         return;
     }
+
+    if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
+        return;
+    }
 }
 
 static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d21638f4f9..226d7f6916 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -101,7 +101,6 @@ config Q35
     select PCI_EXPRESS_Q35
     select ICH9
     select LPC_ICH9
-    select AHCI_ICH9
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index db7259bf6f..f806033d48 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -4,3 +4,4 @@ config ICH9
     bool
     depends on PCI_EXPRESS
     imply I82801B11
+    select AHCI_ICH9
-- 
2.41.0

Re: [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function

[BALATON Zoltan]

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

On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
> Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.
>
> Since the PC machines can disable SATA (see the
> PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
> property to disable it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS                   |  2 ++
> include/hw/southbridge/ich9.h |  4 ----
> hw/i386/pc_q35.c              | 25 ++++---------------------
> hw/southbridge/ich9.c         | 35 +++++++++++++++++++++++++++++++++++
> hw/i386/Kconfig               |  1 -
> hw/southbridge/Kconfig        |  1 +
> 6 files changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d1a2eddd4c..937ebb5c96 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2607,9 +2607,11 @@ M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> S: Supported
> F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
> +F: hw/ide/ich.c
> F: hw/isa/lpc_ich9.c
> F: hw/southbridge/ich9.c
> F: include/hw/acpi/ich9*.h
> +F: include/hw/ide/ahci-pci.h
> F: include/hw/pci-bridge/ich_dmi_pci.h
> F: include/hw/southbridge/ich9.h
>
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index b9122d299d..ac7f9f4ff5 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -166,10 +166,6 @@ struct ICH9LPCState {
>
> #define ICH9_GPIO_GSI "gsi"
>
> -/* D31:F2 SATA Controller #1 */
> -#define ICH9_SATA1_DEV                          31
> -#define ICH9_SATA1_FUNC                         2
> -
> /* D31:F0 power management I/O registers
>    offset from the address ICH9_LPC_PMBASE */
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 2f15af540f..060358d449 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -61,9 +61,6 @@
> #include "hw/acpi/acpi.h"
> #include "target/i386/cpu.h"
>
> -/* ICH9 AHCI has 6 ports */
> -#define MAX_SATA_PORTS     6
> -
> struct ehci_companions {
>     const char *name;
>     int func;
> @@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
>     PCIDevice *lpc;
>     Object *lpc_obj;
>     DeviceState *lpc_dev;
> -    BusState *idebus[MAX_SATA_PORTS];
> +    BusState *idebus[2] = { };
>     ISADevice *rtc_state;
>     MemoryRegion *system_memory = get_system_memory();
>     MemoryRegion *system_io = get_system_io();
> @@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
>     ISABus *isa_bus;
>     int i;
>     ram_addr_t lowmem;
> -    DriveInfo *hd[MAX_SATA_PORTS];
>     MachineClass *mc = MACHINE_GET_CLASS(machine);
>     bool acpi_pcihp;
>     bool keep_pci_slot_hpc;
> @@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
>     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
>                              OBJECT(host_bus), &error_abort);
>     qdev_prop_set_bit(ich9, "d2p-enabled", false);
> +    qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
>     qdev_realize_and_unref(ich9, NULL, &error_fatal);
>
>     /* irq lines */
> @@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
>                          0xff0104);
>
>     if (pcms->sata_enabled) {

Shouldn't this condition be inverted if you only leave the else leg?

Regards,.
BALATON Zoltan

> -        PCIDevice *pdev;
> -        AHCIPCIState *ich9;
> -
> -        /* ahci and SATA device, for q35 1 ahci controller is built-in */
> -        pdev = pci_create_simple_multifunction(host_bus,
> -                                               PCI_DEVFN(ICH9_SATA1_DEV,
> -                                                         ICH9_SATA1_FUNC),
> -                                               "ich9-ahci");
> -        ich9 = ICH9_AHCI(pdev);
> -        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> -        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
> -        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
> -        ide_drive_get(hd, ich9->ahci.ports);
> -        ahci_ide_create_devs(&ich9->ahci, hd);
> -    } else {
> -        idebus[0] = idebus[1] = NULL;
> +        idebus[0] = qdev_get_child_bus(ich9, "ide.0");
> +        idebus[1] = qdev_get_child_bus(ich9, "ide.1");
>     }
>
>     if (machine_usb(machine)) {
> diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
> index 6df47e81fb..233dc1c5d7 100644
> --- a/hw/southbridge/ich9.c
> +++ b/hw/southbridge/ich9.c
> @@ -13,22 +13,30 @@
> #include "hw/southbridge/ich9.h"
> #include "hw/pci/pci.h"
> #include "hw/pci-bridge/ich_dmi_pci.h"
> +#include "hw/ide/ahci-pci.h"
> +#include "hw/ide.h"
>
> #define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
> +#define ICH9_SATA1_DEVFN        PCI_DEVFN(31, 2)
> +
> +#define SATA_PORTS              6
>
> struct ICH9State {
>     DeviceState parent_obj;
>
>     I82801b11Bridge d2p;
> +    AHCIPCIState sata0;
>
>     PCIBus *pci_bus;
>     bool d2p_enabled;
> +    bool sata_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_BOOL("sata-enabled", ICH9State, sata_enabled, true),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -48,6 +56,29 @@ static bool ich9_realize_d2p(ICH9State *s, Error **errp)
>     return true;
> }
>
> +static bool ich9_realize_sata(ICH9State *s, Error **errp)
> +{
> +    DriveInfo *hd[SATA_PORTS];
> +
> +    object_initialize_child(OBJECT(s), "sata[0]", &s->sata0, TYPE_ICH9_AHCI);
> +    qdev_prop_set_int32(DEVICE(&s->sata0), "addr", ICH9_SATA1_DEVFN);
> +    if (!qdev_realize(DEVICE(&s->sata0), BUS(s->pci_bus), errp)) {
> +        return false;
> +    }
> +    for (unsigned i = 0; i < SATA_PORTS; i++) {
> +        g_autofree char *portname = g_strdup_printf("ide.%u", i);
> +
> +        object_property_add_alias(OBJECT(s), portname,
> +                                  OBJECT(&s->sata0), portname);
> +    }
> +
> +    g_assert(SATA_PORTS == s->sata0.ahci.ports);
> +    ide_drive_get(hd, s->sata0.ahci.ports);
> +    ahci_ide_create_devs(&s->sata0.ahci, hd);
> +
> +    return true;
> +}
> +
> static void ich9_init(Object *obj)
> {
> }
> @@ -64,6 +95,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
>     if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
>         return;
>     }
> +
> +    if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
> +        return;
> +    }
> }
>
> static void ich9_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index d21638f4f9..226d7f6916 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -101,7 +101,6 @@ config Q35
>     select PCI_EXPRESS_Q35
>     select ICH9
>     select LPC_ICH9
> -    select AHCI_ICH9
>     select DIMM
>     select SMBIOS
>     select FW_CFG_DMA
> diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
> index db7259bf6f..f806033d48 100644
> --- a/hw/southbridge/Kconfig
> +++ b/hw/southbridge/Kconfig
> @@ -4,3 +4,4 @@ config ICH9
>     bool
>     depends on PCI_EXPRESS
>     imply I82801B11
> +    select AHCI_ICH9
>

Re: [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function

[Philippe Mathieu-Daudé]

On 19/2/24 19:31, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.
>>
>> Since the PC machines can disable SATA (see the
>> PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
>> property to disable it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS                   |  2 ++
>> include/hw/southbridge/ich9.h |  4 ----
>> hw/i386/pc_q35.c              | 25 ++++---------------------
>> hw/southbridge/ich9.c         | 35 +++++++++++++++++++++++++++++++++++
>> hw/i386/Kconfig               |  1 -
>> hw/southbridge/Kconfig        |  1 +
>> 6 files changed, 42 insertions(+), 26 deletions(-)


>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 2f15af540f..060358d449 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -61,9 +61,6 @@
>> #include "hw/acpi/acpi.h"
>> #include "target/i386/cpu.h"
>>
>> -/* ICH9 AHCI has 6 ports */
>> -#define MAX_SATA_PORTS     6
>> -
>> struct ehci_companions {
>>     const char *name;
>>     int func;
>> @@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
>>     PCIDevice *lpc;
>>     Object *lpc_obj;
>>     DeviceState *lpc_dev;
>> -    BusState *idebus[MAX_SATA_PORTS];
>> +    BusState *idebus[2] = { };

[*]

>>     ISADevice *rtc_state;
>>     MemoryRegion *system_memory = get_system_memory();
>>     MemoryRegion *system_io = get_system_io();
>> @@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
>>     ISABus *isa_bus;
>>     int i;
>>     ram_addr_t lowmem;
>> -    DriveInfo *hd[MAX_SATA_PORTS];
>>     MachineClass *mc = MACHINE_GET_CLASS(machine);
>>     bool acpi_pcihp;
>>     bool keep_pci_slot_hpc;
>> @@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
>>     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
>>                              OBJECT(host_bus), &error_abort);
>>     qdev_prop_set_bit(ich9, "d2p-enabled", false);
>> +    qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
>>     qdev_realize_and_unref(ich9, NULL, &error_fatal);
>>
>>     /* irq lines */
>> @@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
>>                          0xff0104);
>>
>>     if (pcms->sata_enabled) {
> 
> Shouldn't this condition be inverted if you only leave the else leg?

idebus[] is NULL-initialized in [*] so we can remove the else
ladder.

> 
> Regards,.
> BALATON Zoltan
> 
>> -        PCIDevice *pdev;
>> -        AHCIPCIState *ich9;
>> -
>> -        /* ahci and SATA device, for q35 1 ahci controller is 
>> built-in */
>> -        pdev = pci_create_simple_multifunction(host_bus,
>> -                                               PCI_DEVFN(ICH9_SATA1_DEV,
>> -                                                         
>> ICH9_SATA1_FUNC),
>> -                                               "ich9-ahci");
>> -        ich9 = ICH9_AHCI(pdev);
>> -        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
>> -        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
>> -        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
>> -        ide_drive_get(hd, ich9->ahci.ports);
>> -        ahci_ide_create_devs(&ich9->ahci, hd);
>> -    } else {
>> -        idebus[0] = idebus[1] = NULL;
>> +        idebus[0] = qdev_get_child_bus(ich9, "ide.0");
>> +        idebus[1] = qdev_get_child_bus(ich9, "ide.1");
>>     }

[PATCH 10/14] hw/i2c/smbus: Extract QOM ICH9 definitions to 'smbus_ich9.h'

[Philippe Mathieu-Daudé]

Expose TYPE_ICH9_SMB_DEVICE to the new "hw/i2c/smbus_ich9.h"
header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                 |  1 +
 include/hw/i2c/smbus_ich9.h | 25 +++++++++++++++++++++++++
 hw/i2c/smbus_ich9.c         | 15 ++-------------
 3 files changed, 28 insertions(+), 13 deletions(-)
 create mode 100644 include/hw/i2c/smbus_ich9.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 937ebb5c96..b896d953af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2611,6 +2611,7 @@ F: hw/ide/ich.c
 F: hw/isa/lpc_ich9.c
 F: hw/southbridge/ich9.c
 F: include/hw/acpi/ich9*.h
+F: include/hw/i2c/smbus_ich9.h
 F: include/hw/ide/ahci-pci.h
 F: include/hw/pci-bridge/ich_dmi_pci.h
 F: include/hw/southbridge/ich9.h
diff --git a/include/hw/i2c/smbus_ich9.h b/include/hw/i2c/smbus_ich9.h
new file mode 100644
index 0000000000..d716cbca33
--- /dev/null
+++ b/include/hw/i2c/smbus_ich9.h
@@ -0,0 +1,25 @@
+/*
+ * QEMU ICH9 SMBus emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_I2C_SMBUS_ICH9_H
+#define HW_I2C_SMBUS_ICH9_H
+
+#include "qom/object.h"
+#include "hw/pci/pci_device.h"
+#include "hw/i2c/pm_smbus.h"
+
+#define TYPE_ICH9_SMB_DEVICE "ICH9-SMB"
+
+OBJECT_DECLARE_SIMPLE_TYPE(ICH9SMBState, ICH9_SMB_DEVICE)
+
+struct ICH9SMBState {
+    PCIDevice dev;
+
+    bool irq_enabled;
+
+    PMSMBus smb;
+};
+
+#endif
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 208f263ac5..3980bca4c5 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -1,5 +1,5 @@
 /*
- * ACPI implementation
+ * QEMU ICH9 SMBus emulation
  *
  * Copyright (c) 2006 Fabrice Bellard
  * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
@@ -22,8 +22,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/range.h"
-#include "hw/i2c/pm_smbus.h"
-#include "hw/pci/pci.h"
+#include "hw/i2c/smbus_ich9.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
 
@@ -31,16 +30,6 @@
 #include "qom/object.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
-OBJECT_DECLARE_SIMPLE_TYPE(ICH9SMBState, ICH9_SMB_DEVICE)
-
-struct ICH9SMBState {
-    PCIDevice dev;
-
-    bool irq_enabled;
-
-    PMSMBus smb;
-};
-
 static bool ich9_vmstate_need_smbus(void *opaque, int version_id)
 {
     return pm_smbus_vmstate_needed();
-- 
2.41.0

[PATCH 11/14] hw/southbridge/ich9: Add the SMBus function

[Philippe Mathieu-Daudé]

Instantiate TYPE_ICH9_SMB_DEVICE in TYPE_ICH9_SOUTHBRIDGE.

Since the PC machines can disable SMBus (see the
PC_MACHINE_SMBUS dynamic property), add the 'smbus-enabled'
property to disable it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/southbridge/ich9.h | 32 --------------------------------
 hw/i2c/smbus_ich9.c           | 29 ++++++++++++++++++++++++++++-
 hw/i386/pc_q35.c              | 10 ++--------
 hw/southbridge/ich9.c         | 21 +++++++++++++++++++++
 hw/southbridge/Kconfig        |  1 +
 5 files changed, 52 insertions(+), 41 deletions(-)

diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index ac7f9f4ff5..d4b299bf3c 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -173,38 +173,6 @@ struct ICH9LPCState {
 #define ICH9_APM_ACPI_ENABLE                    0x2
 #define ICH9_APM_ACPI_DISABLE                   0x3
 
-
-/* D31:F3 SMBus controller */
-#define TYPE_ICH9_SMB_DEVICE "ICH9-SMB"
-
-#define ICH9_A2_SMB_REVISION                    0x02
-#define ICH9_SMB_PI                             0x00
-
-#define ICH9_SMB_SMBMBAR0                       0x10
-#define ICH9_SMB_SMBMBAR1                       0x14
-#define ICH9_SMB_SMBM_BAR                       0
-#define ICH9_SMB_SMBM_SIZE                      (1 << 8)
-#define ICH9_SMB_SMB_BASE                       0x20
-#define ICH9_SMB_SMB_BASE_BAR                   4
-#define ICH9_SMB_SMB_BASE_SIZE                  (1 << 5)
-#define ICH9_SMB_HOSTC                          0x40
-#define ICH9_SMB_HOSTC_SSRESET                  ((uint8_t)(1 << 3))
-#define ICH9_SMB_HOSTC_I2C_EN                   ((uint8_t)(1 << 2))
-#define ICH9_SMB_HOSTC_SMB_SMI_EN               ((uint8_t)(1 << 1))
-#define ICH9_SMB_HOSTC_HST_EN                   ((uint8_t)(1 << 0))
-
-/* D31:F3 SMBus I/O and memory mapped I/O registers */
-#define ICH9_SMB_DEV                            31
-#define ICH9_SMB_FUNC                           3
-
-#define ICH9_SMB_HST_STS                        0x00
-#define ICH9_SMB_HST_CNT                        0x02
-#define ICH9_SMB_HST_CMD                        0x03
-#define ICH9_SMB_XMIT_SLVA                      0x04
-#define ICH9_SMB_HST_D0                         0x05
-#define ICH9_SMB_HST_D1                         0x06
-#define ICH9_SMB_HOST_BLOCK_DB                  0x07
-
 #define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
 
 /* bit positions used in fw_cfg SMI feature negotiation */
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 3980bca4c5..2c18278090 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -26,10 +26,37 @@
 #include "migration/vmstate.h"
 #include "qemu/module.h"
 
-#include "hw/southbridge/ich9.h"
 #include "qom/object.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
+/* D31:F3 SMBus controller */
+
+#define ICH9_A2_SMB_REVISION                    0x02
+#define ICH9_SMB_PI                             0x00
+
+#define ICH9_SMB_SMBMBAR0                       0x10
+#define ICH9_SMB_SMBMBAR1                       0x14
+#define ICH9_SMB_SMBM_BAR                       0
+#define ICH9_SMB_SMBM_SIZE                      (1 << 8)
+#define ICH9_SMB_SMB_BASE                       0x20
+#define ICH9_SMB_SMB_BASE_BAR                   4
+#define ICH9_SMB_SMB_BASE_SIZE                  (1 << 5)
+#define ICH9_SMB_HOSTC                          0x40
+#define ICH9_SMB_HOSTC_SSRESET                  ((uint8_t)(1 << 3))
+#define ICH9_SMB_HOSTC_I2C_EN                   ((uint8_t)(1 << 2))
+#define ICH9_SMB_HOSTC_SMB_SMI_EN               ((uint8_t)(1 << 1))
+#define ICH9_SMB_HOSTC_HST_EN                   ((uint8_t)(1 << 0))
+
+/* D31:F3 SMBus I/O and memory mapped I/O registers */
+
+#define ICH9_SMB_HST_STS                        0x00
+#define ICH9_SMB_HST_CNT                        0x02
+#define ICH9_SMB_HST_CMD                        0x03
+#define ICH9_SMB_XMIT_SLVA                      0x04
+#define ICH9_SMB_HST_D0                         0x05
+#define ICH9_SMB_HST_D1                         0x06
+#define ICH9_SMB_HOST_BLOCK_DB                  0x07
+
 static bool ich9_vmstate_need_smbus(void *opaque, int version_id)
 {
     return pm_smbus_vmstate_needed();
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 060358d449..7f6ced8a6e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -236,6 +236,7 @@ static void pc_q35_init(MachineState *machine)
                              OBJECT(host_bus), &error_abort);
     qdev_prop_set_bit(ich9, "d2p-enabled", false);
     qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
+    qdev_prop_set_bit(ich9, "smbus-enabled", pcms->smbus_enabled);
     qdev_realize_and_unref(ich9, NULL, &error_fatal);
 
     /* irq lines */
@@ -309,15 +310,8 @@ static void pc_q35_init(MachineState *machine)
     }
 
     if (pcms->smbus_enabled) {
-        PCIDevice *smb;
-
+        pcms->smbus = I2C_BUS(qdev_get_child_bus(ich9, "i2c"));
         /* TODO: Populate SPD eeprom data.  */
-        smb = pci_create_simple_multifunction(host_bus,
-                                              PCI_DEVFN(ICH9_SMB_DEV,
-                                                        ICH9_SMB_FUNC),
-                                              TYPE_ICH9_SMB_DEVICE);
-        pcms->smbus = I2C_BUS(qdev_get_child_bus(DEVICE(smb), "i2c"));
-
         smbus_eeprom_init(pcms->smbus, 8, NULL, 0);
     }
 
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 233dc1c5d7..4d2c298666 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -15,9 +15,11 @@
 #include "hw/pci-bridge/ich_dmi_pci.h"
 #include "hw/ide/ahci-pci.h"
 #include "hw/ide.h"
+#include "hw/i2c/smbus_ich9.h"
 
 #define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
 #define ICH9_SATA1_DEVFN        PCI_DEVFN(31, 2)
+#define ICH9_SMB_DEVFN          PCI_DEVFN(31, 3)
 
 #define SATA_PORTS              6
 
@@ -26,10 +28,12 @@ struct ICH9State {
 
     I82801b11Bridge d2p;
     AHCIPCIState sata0;
+    ICH9SMBState smb;
 
     PCIBus *pci_bus;
     bool d2p_enabled;
     bool sata_enabled;
+    bool smbus_enabled;
 };
 
 static Property ich9_props[] = {
@@ -37,6 +41,7 @@ static Property ich9_props[] = {
                      TYPE_PCIE_BUS, PCIBus *),
     DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
     DEFINE_PROP_BOOL("sata-enabled", ICH9State, sata_enabled, true),
+    DEFINE_PROP_BOOL("smbus-enabled", ICH9State, smbus_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -79,6 +84,18 @@ static bool ich9_realize_sata(ICH9State *s, Error **errp)
     return true;
 }
 
+static bool ich9_realize_smbus(ICH9State *s, Error **errp)
+{
+    object_initialize_child(OBJECT(s), "smb", &s->smb, TYPE_ICH9_SMB_DEVICE);
+    qdev_prop_set_int32(DEVICE(&s->smb), "addr", ICH9_SMB_DEVFN);
+    if (!qdev_realize(DEVICE(&s->smb), BUS(s->pci_bus), errp)) {
+        return false;
+    }
+    object_property_add_alias(OBJECT(s), "i2c", OBJECT(&s->smb), "i2c");
+
+    return true;
+}
+
 static void ich9_init(Object *obj)
 {
 }
@@ -99,6 +116,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
     if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
         return;
     }
+
+    if (s->smbus_enabled && !ich9_realize_smbus(s, errp)) {
+        return;
+    }
 }
 
 static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index f806033d48..03e89a55d1 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -5,3 +5,4 @@ config ICH9
     depends on PCI_EXPRESS
     imply I82801B11
     select AHCI_ICH9
+    select ACPI_ICH9
-- 
2.41.0

[PATCH 12/14] hw/southbridge/ich9: Add the USB EHCI/UHCI functions

[Philippe Mathieu-Daudé]

Instantiate EHCI and UHCI in TYPE_ICH9_SOUTHBRIDGE.

Since machines can disable USB, add the 'ehci-count'
property. Machine can disable USB functions by setting
ehci-count=0.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/southbridge/ich9.h |  5 ---
 hw/i386/pc_q35.c              | 62 ++---------------------------------
 hw/southbridge/ich9.c         | 54 ++++++++++++++++++++++++++++++
 hw/southbridge/Kconfig        |  2 ++
 4 files changed, 58 insertions(+), 65 deletions(-)

diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index d4b299bf3c..7e75496b0b 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -103,11 +103,6 @@ struct ICH9LPCState {
 #define ICH9_PCIE_DEV                           28
 #define ICH9_PCIE_FUNC_MAX                      6
 
-
-/* D29:F0 USB UHCI Controller #1 */
-#define ICH9_USB_UHCI1_DEV                      29
-#define ICH9_USB_UHCI1_FUNC                     0
-
 /* 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 7f6ced8a6e..e5f5bb0db1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,8 +50,6 @@
 #include "hw/ide/ahci-pci.h"
 #include "hw/intc/ioapic.h"
 #include "hw/southbridge/ich9.h"
-#include "hw/usb.h"
-#include "hw/usb/hcd-uhci.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
@@ -61,59 +59,6 @@
 #include "hw/acpi/acpi.h"
 #include "target/i386/cpu.h"
 
-struct ehci_companions {
-    const char *name;
-    int func;
-    int port;
-};
-
-static const struct ehci_companions ich9_1d[] = {
-    { .name = TYPE_ICH9_USB_UHCI(1), .func = 0, .port = 0 },
-    { .name = TYPE_ICH9_USB_UHCI(2), .func = 1, .port = 2 },
-    { .name = TYPE_ICH9_USB_UHCI(3), .func = 2, .port = 4 },
-};
-
-static const struct ehci_companions ich9_1a[] = {
-    { .name = TYPE_ICH9_USB_UHCI(4), .func = 0, .port = 0 },
-    { .name = TYPE_ICH9_USB_UHCI(5), .func = 1, .port = 2 },
-    { .name = TYPE_ICH9_USB_UHCI(6), .func = 2, .port = 4 },
-};
-
-static int ehci_create_ich9_with_companions(PCIBus *bus, int slot)
-{
-    const struct ehci_companions *comp;
-    PCIDevice *ehci, *uhci;
-    BusState *usbbus;
-    const char *name;
-    int i;
-
-    switch (slot) {
-    case 0x1d:
-        name = "ich9-usb-ehci1";
-        comp = ich9_1d;
-        break;
-    case 0x1a:
-        name = "ich9-usb-ehci2";
-        comp = ich9_1a;
-        break;
-    default:
-        return -1;
-    }
-
-    ehci = pci_new_multifunction(PCI_DEVFN(slot, 7), name);
-    pci_realize_and_unref(ehci, bus, &error_fatal);
-    usbbus = QLIST_FIRST(&ehci->qdev.child_bus);
-
-    for (i = 0; i < 3; i++) {
-        uhci = pci_new_multifunction(PCI_DEVFN(slot, comp[i].func),
-                                     comp[i].name);
-        qdev_prop_set_string(&uhci->qdev, "masterbus", usbbus->name);
-        qdev_prop_set_uint32(&uhci->qdev, "firstport", comp[i].port);
-        pci_realize_and_unref(uhci, bus, &error_fatal);
-    }
-    return 0;
-}
-
 /* PC hardware initialisation */
 static void pc_q35_init(MachineState *machine)
 {
@@ -237,6 +182,8 @@ static void pc_q35_init(MachineState *machine)
     qdev_prop_set_bit(ich9, "d2p-enabled", false);
     qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
     qdev_prop_set_bit(ich9, "smbus-enabled", pcms->smbus_enabled);
+    /* Should we create 6 UHCI according to ich9 spec? */
+    qdev_prop_set_uint8(ich9, "ehci-count", machine_usb(machine) ? 1 : 0);
     qdev_realize_and_unref(ich9, NULL, &error_fatal);
 
     /* irq lines */
@@ -304,11 +251,6 @@ static void pc_q35_init(MachineState *machine)
         idebus[1] = qdev_get_child_bus(ich9, "ide.1");
     }
 
-    if (machine_usb(machine)) {
-        /* Should we create 6 UHCI according to ich9 spec? */
-        ehci_create_ich9_with_companions(host_bus, 0x1d);
-    }
-
     if (pcms->smbus_enabled) {
         pcms->smbus = I2C_BUS(qdev_get_child_bus(ich9, "i2c"));
         /* TODO: Populate SPD eeprom data.  */
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 4d2c298666..085d75e569 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -16,12 +16,18 @@
 #include "hw/ide/ahci-pci.h"
 #include "hw/ide.h"
 #include "hw/i2c/smbus_ich9.h"
+#include "hw/usb.h"
+#include "hw/usb/hcd-ehci.h"
+#include "hw/usb/hcd-uhci.h"
 
 #define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
 #define ICH9_SATA1_DEVFN        PCI_DEVFN(31, 2)
 #define ICH9_SMB_DEVFN          PCI_DEVFN(31, 3)
+#define ICH9_EHCI_FUNC          7
 
 #define SATA_PORTS              6
+#define EHCI_PER_FN             2
+#define UHCI_PER_FN             3
 
 struct ICH9State {
     DeviceState parent_obj;
@@ -29,11 +35,14 @@ struct ICH9State {
     I82801b11Bridge d2p;
     AHCIPCIState sata0;
     ICH9SMBState smb;
+    EHCIPCIState ehci[EHCI_PER_FN];
+    UHCIState uhci[EHCI_PER_FN * UHCI_PER_FN];
 
     PCIBus *pci_bus;
     bool d2p_enabled;
     bool sata_enabled;
     bool smbus_enabled;
+    uint8_t ehci_count;
 };
 
 static Property ich9_props[] = {
@@ -42,6 +51,7 @@ static Property ich9_props[] = {
     DEFINE_PROP_BOOL("d2p-enabled", ICH9State, d2p_enabled, true),
     DEFINE_PROP_BOOL("sata-enabled", ICH9State, sata_enabled, true),
     DEFINE_PROP_BOOL("smbus-enabled", ICH9State, smbus_enabled, true),
+    DEFINE_PROP_UINT8("ehci-count", ICH9State, ehci_count, 2),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -96,6 +106,46 @@ static bool ich9_realize_smbus(ICH9State *s, Error **errp)
     return true;
 }
 
+static bool ich9_realize_usb(ICH9State *s, Error **errp)
+{
+    if (!module_object_class_by_name(TYPE_ICH9_USB_UHCI(0))
+        || !module_object_class_by_name("ich9-usb-ehci0")) {
+        error_setg(errp, "USB functions not available in this build");
+        return false;
+    }
+    for (unsigned e = 0; e < s->ehci_count; e++) {
+        g_autofree gchar *ename = g_strdup_printf("ich9-usb-ehci%u", e + 1);
+        EHCIPCIState *ehci = &s->ehci[e];
+        const unsigned devid = e ? 0x1a : 0x1d;
+        BusState *masterbus;
+
+        object_initialize_child(OBJECT(s), "ehci[*]", ehci, ename);
+        qdev_prop_set_int32(DEVICE(ehci), "addr", PCI_DEVFN(devid,
+                                                            ICH9_EHCI_FUNC));
+        if (!qdev_realize(DEVICE(ehci), BUS(s->pci_bus), errp)) {
+            return false;
+        }
+        masterbus = QLIST_FIRST(&DEVICE(ehci)->child_bus);
+
+        for (unsigned u = 0; u < UHCI_PER_FN; u++) {
+            unsigned c = UHCI_PER_FN * e + u;
+            UHCIState *uhci = &s->uhci[c];
+            g_autofree gchar *cname = g_strdup_printf("ich9-usb-uhci%u", c + 1);
+
+            object_initialize_child(OBJECT(s), "uhci[*]", uhci, cname);
+            qdev_prop_set_bit(DEVICE(uhci), "multifunction", true);
+            qdev_prop_set_int32(DEVICE(uhci), "addr", PCI_DEVFN(devid, u));
+            qdev_prop_set_string(DEVICE(uhci), "masterbus", masterbus->name);
+            qdev_prop_set_uint32(DEVICE(uhci), "firstport", 2 * u);
+            if (!qdev_realize(DEVICE(uhci), BUS(s->pci_bus), errp)) {
+                return false;
+            }
+        }
+    }
+
+    return true;
+}
+
 static void ich9_init(Object *obj)
 {
 }
@@ -120,6 +170,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
     if (s->smbus_enabled && !ich9_realize_smbus(s, errp)) {
         return;
     }
+
+    if (!ich9_realize_usb(s, errp)) {
+        return;
+    }
 }
 
 static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index 03e89a55d1..31eb125bf7 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -6,3 +6,5 @@ config ICH9
     imply I82801B11
     select AHCI_ICH9
     select ACPI_ICH9
+    imply USB_EHCI_PCI
+    imply USB_UHCI
-- 
2.41.0

[PATCH 13/14] hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h'

[Philippe Mathieu-Daudé]

Restrict ICH9 LPC/ISA definitions by moving them
to the "hw/isa/ich9_lpc.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                   |   1 +
 include/hw/isa/ich9_lpc.h     | 166 ++++++++++++++++++++++++++++++++++
 include/hw/southbridge/ich9.h | 158 ++------------------------------
 hw/acpi/ich9.c                |   1 +
 hw/acpi/ich9_tco.c            |   1 +
 hw/i386/acpi-build.c          |   1 +
 hw/i386/pc_q35.c              |   1 +
 hw/isa/lpc_ich9.c             |  34 ++++++-
 tests/qtest/tco-test.c        |   2 +-
 9 files changed, 208 insertions(+), 157 deletions(-)
 create mode 100644 include/hw/isa/ich9_lpc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b896d953af..31ddbc565b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2613,6 +2613,7 @@ F: hw/southbridge/ich9.c
 F: include/hw/acpi/ich9*.h
 F: include/hw/i2c/smbus_ich9.h
 F: include/hw/ide/ahci-pci.h
+F: include/hw/isa/ich9_lpc.h
 F: include/hw/pci-bridge/ich_dmi_pci.h
 F: include/hw/southbridge/ich9.h
 
diff --git a/include/hw/isa/ich9_lpc.h b/include/hw/isa/ich9_lpc.h
new file mode 100644
index 0000000000..b64d88b395
--- /dev/null
+++ b/include/hw/isa/ich9_lpc.h
@@ -0,0 +1,166 @@
+/*
+ * QEMU ICH9 PCI-to-LPC/ISA bridge emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_ISA_ICH9_LPC_H
+#define HW_ISA_ICH9_LPC_H
+
+#include "exec/memory.h"
+#include "hw/isa/apm.h"
+#include "hw/acpi/ich9.h"
+#include "hw/intc/ioapic.h"
+#include "hw/pci/pci_device.h"
+#include "hw/rtc/mc146818rtc.h"
+#include "qemu/notify.h"
+#include "qom/object.h"
+
+#define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
+OBJECT_DECLARE_SIMPLE_TYPE(ICH9LPCState, ICH9_LPC_DEVICE)
+
+#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
+
+struct ICH9LPCState {
+    /* ICH9 LPC PCI to ISA bridge */
+    PCIDevice d;
+
+    /* (pci device, intx) -> pirq
+     * In real chipset case, the unused slots are never used
+     * as ICH9 supports only D25-D31 irq routing.
+     * On the other hand in qemu case, any slot/function can be populated
+     * via command line option.
+     * So fallback interrupt routing for any devices in any slots is necessary.
+    */
+    uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
+
+    MC146818RtcState rtc;
+    APMState apm;
+    ICH9LPCPMRegs pm;
+    uint32_t sci_level; /* track sci level */
+    uint8_t sci_gsi;
+
+    /* 2.24 Pin Straps */
+    struct {
+        bool spkr_hi;
+    } pin_strap;
+
+    /* 10.1 Chipset Configuration registers(Memory Space)
+     which is pointed by RCBA */
+    uint8_t chip_config[ICH9_CC_SIZE];
+
+    /*
+     * 13.7.5 RST_CNT---Reset Control Register (LPC I/F---D31:F0)
+     *
+     * register contents and IO memory region
+     */
+    uint8_t rst_cnt;
+    MemoryRegion rst_cnt_mem;
+
+    /* SMI feature negotiation via fw_cfg */
+    uint64_t smi_host_features;       /* guest-invisible, host endian */
+    uint8_t smi_host_features_le[8];  /* guest-visible, read-only, little
+                                       * endian uint64_t */
+    uint8_t smi_guest_features_le[8]; /* guest-visible, read-write, little
+                                       * endian uint64_t */
+    uint8_t smi_features_ok;          /* guest-visible, read-only; selecting it
+                                       * triggers feature lockdown */
+    uint64_t smi_negotiated_features; /* guest-invisible, host endian */
+
+    MemoryRegion rcrb_mem; /* root complex register block */
+    Notifier machine_ready;
+
+    qemu_irq gsi[IOAPIC_NUM_PINS];
+};
+
+#define ICH9_MASK(bit, ms_bit, ls_bit) \
+((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL << ls_bit) - 1)))
+
+#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
+
+/* ICH9: Chipset Configuration Registers */
+#define ICH9_CC_ADDR_MASK                       (ICH9_CC_SIZE - 1)
+
+#define ICH9_CC
+#define ICH9_CC_D28IP                           0x310C
+#define ICH9_CC_D28IP_SHIFT                     4
+#define ICH9_CC_D28IP_MASK                      0xf
+#define ICH9_CC_D28IP_DEFAULT                   0x00214321
+#define ICH9_CC_D31IR                           0x3140
+#define ICH9_CC_D30IR                           0x3142
+#define ICH9_CC_D29IR                           0x3144
+#define ICH9_CC_D28IR                           0x3146
+#define ICH9_CC_D27IR                           0x3148
+#define ICH9_CC_D26IR                           0x314C
+#define ICH9_CC_D25IR                           0x3150
+#define ICH9_CC_DIR_DEFAULT                     0x3210
+#define ICH9_CC_D30IR_DEFAULT                   0x0
+#define ICH9_CC_DIR_SHIFT                       4
+#define ICH9_CC_DIR_MASK                        0x7
+#define ICH9_CC_OIC                             0x31FF
+#define ICH9_CC_OIC_AEN                         0x1
+#define ICH9_CC_GCS                             0x3410
+#define ICH9_CC_GCS_DEFAULT                     0x00000020
+#define ICH9_CC_GCS_NO_REBOOT                   (1 << 5)
+
+/* D31:F0 LPC Processor Interface */
+#define ICH9_RST_CNT_IOPORT                     0xCF9
+
+/* D31:F1 LPC controller */
+#define ICH9_A2_LPC                             "ICH9 A2 LPC"
+#define ICH9_A2_LPC_SAVEVM_VERSION              0
+
+#define ICH9_A2_LPC_REVISION                    0x2
+#define ICH9_LPC_NB_PIRQS                       8       /* PCI A-H */
+
+#define ICH9_LPC_PMBASE                         0x40
+#define ICH9_LPC_PMBASE_BASE_ADDRESS_MASK       ICH9_MASK(32, 15, 7)
+#define ICH9_LPC_PMBASE_RTE                     0x1
+#define ICH9_LPC_PMBASE_DEFAULT                 0x1
+
+#define ICH9_LPC_ACPI_CTRL                      0x44
+#define ICH9_LPC_ACPI_CTRL_ACPI_EN              0x80
+#define ICH9_LPC_ACPI_CTRL_SCI_IRQ_SEL_MASK     ICH9_MASK(8, 2, 0)
+#define ICH9_LPC_ACPI_CTRL_9                    0x0
+#define ICH9_LPC_ACPI_CTRL_10                   0x1
+#define ICH9_LPC_ACPI_CTRL_11                   0x2
+#define ICH9_LPC_ACPI_CTRL_20                   0x4
+#define ICH9_LPC_ACPI_CTRL_21                   0x5
+#define ICH9_LPC_ACPI_CTRL_DEFAULT              0x0
+
+#define ICH9_LPC_PIRQA_ROUT                     0x60
+#define ICH9_LPC_PIRQB_ROUT                     0x61
+#define ICH9_LPC_PIRQC_ROUT                     0x62
+#define ICH9_LPC_PIRQD_ROUT                     0x63
+
+#define ICH9_LPC_PIRQE_ROUT                     0x68
+#define ICH9_LPC_PIRQF_ROUT                     0x69
+#define ICH9_LPC_PIRQG_ROUT                     0x6a
+#define ICH9_LPC_PIRQH_ROUT                     0x6b
+
+#define ICH9_LPC_PIRQ_ROUT_IRQEN                0x80
+#define ICH9_LPC_PIRQ_ROUT_MASK                 ICH9_MASK(8, 3, 0)
+#define ICH9_LPC_PIRQ_ROUT_DEFAULT              0x80
+
+#define ICH9_LPC_GEN_PMCON_1                    0xa0
+#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK           (1 << 4)
+#define ICH9_LPC_GEN_PMCON_2                    0xa2
+#define ICH9_LPC_GEN_PMCON_3                    0xa4
+#define ICH9_LPC_GEN_PMCON_LOCK                 0xa6
+
+#define ICH9_LPC_RCBA                           0xf0
+#define ICH9_LPC_RCBA_BA_MASK                   ICH9_MASK(32, 31, 14)
+#define ICH9_LPC_RCBA_EN                        0x1
+#define ICH9_LPC_RCBA_DEFAULT                   0x0
+
+#define ICH9_LPC_PIC_NUM_PINS                   16
+#define ICH9_LPC_IOAPIC_NUM_PINS                24
+
+/* D31:F0 power management I/O registers
+   offset from the address ICH9_LPC_PMBASE */
+
+/* FADT ACPI_ENABLE/ACPI_DISABLE */
+#define ICH9_APM_ACPI_ENABLE                    0x2
+#define ICH9_APM_ACPI_DISABLE                   0x3
+
+#endif
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index 7e75496b0b..d6c3b5ece3 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -1,173 +1,27 @@
+/*
+ * QEMU Intel ICH9 south bridge emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
 #ifndef HW_SOUTHBRIDGE_ICH9_H
 #define HW_SOUTHBRIDGE_ICH9_H
 
-#include "hw/isa/apm.h"
-#include "hw/acpi/ich9.h"
-#include "hw/intc/ioapic.h"
-#include "hw/pci/pci.h"
-#include "hw/pci/pci_device.h"
-#include "hw/rtc/mc146818rtc.h"
-#include "exec/memory.h"
-#include "qemu/notify.h"
 #include "qom/object.h"
 
 #define TYPE_ICH9_SOUTHBRIDGE "ICH9-southbridge"
 OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
 
-#define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
-
-#define TYPE_ICH9_LPC_DEVICE "ICH9-LPC"
-OBJECT_DECLARE_SIMPLE_TYPE(ICH9LPCState, ICH9_LPC_DEVICE)
-
-struct ICH9LPCState {
-    /* ICH9 LPC PCI to ISA bridge */
-    PCIDevice d;
-
-    /* (pci device, intx) -> pirq
-     * In real chipset case, the unused slots are never used
-     * as ICH9 supports only D25-D31 irq routing.
-     * On the other hand in qemu case, any slot/function can be populated
-     * via command line option.
-     * So fallback interrupt routing for any devices in any slots is necessary.
-    */
-    uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];
-
-    MC146818RtcState rtc;
-    APMState apm;
-    ICH9LPCPMRegs pm;
-    uint32_t sci_level; /* track sci level */
-    uint8_t sci_gsi;
-
-    /* 2.24 Pin Straps */
-    struct {
-        bool spkr_hi;
-    } pin_strap;
-
-    /* 10.1 Chipset Configuration registers(Memory Space)
-     which is pointed by RCBA */
-    uint8_t chip_config[ICH9_CC_SIZE];
-
-    /*
-     * 13.7.5 RST_CNT---Reset Control Register (LPC I/F---D31:F0)
-     *
-     * register contents and IO memory region
-     */
-    uint8_t rst_cnt;
-    MemoryRegion rst_cnt_mem;
-
-    /* SMI feature negotiation via fw_cfg */
-    uint64_t smi_host_features;       /* guest-invisible, host endian */
-    uint8_t smi_host_features_le[8];  /* guest-visible, read-only, little
-                                       * endian uint64_t */
-    uint8_t smi_guest_features_le[8]; /* guest-visible, read-write, little
-                                       * endian uint64_t */
-    uint8_t smi_features_ok;          /* guest-visible, read-only; selecting it
-                                       * triggers feature lockdown */
-    uint64_t smi_negotiated_features; /* guest-invisible, host endian */
-
-    MemoryRegion rcrb_mem; /* root complex register block */
-    Notifier machine_ready;
-
-    qemu_irq gsi[IOAPIC_NUM_PINS];
-};
-
-#define ICH9_MASK(bit, ms_bit, ls_bit) \
-((uint##bit##_t)(((1ULL << ((ms_bit) + 1)) - 1) & ~((1ULL << ls_bit) - 1)))
-
-/* ICH9: Chipset Configuration Registers */
-#define ICH9_CC_ADDR_MASK                       (ICH9_CC_SIZE - 1)
-
-#define ICH9_CC
-#define ICH9_CC_D28IP                           0x310C
-#define ICH9_CC_D28IP_SHIFT                     4
-#define ICH9_CC_D28IP_MASK                      0xf
-#define ICH9_CC_D28IP_DEFAULT                   0x00214321
-#define ICH9_CC_D31IR                           0x3140
-#define ICH9_CC_D30IR                           0x3142
-#define ICH9_CC_D29IR                           0x3144
-#define ICH9_CC_D28IR                           0x3146
-#define ICH9_CC_D27IR                           0x3148
-#define ICH9_CC_D26IR                           0x314C
-#define ICH9_CC_D25IR                           0x3150
-#define ICH9_CC_DIR_DEFAULT                     0x3210
-#define ICH9_CC_D30IR_DEFAULT                   0x0
-#define ICH9_CC_DIR_SHIFT                       4
-#define ICH9_CC_DIR_MASK                        0x7
-#define ICH9_CC_OIC                             0x31FF
-#define ICH9_CC_OIC_AEN                         0x1
-#define ICH9_CC_GCS                             0x3410
-#define ICH9_CC_GCS_DEFAULT                     0x00000020
-#define ICH9_CC_GCS_NO_REBOOT                   (1 << 5)
-
 /* D28:F[0-5] */
 #define ICH9_PCIE_DEV                           28
 #define ICH9_PCIE_FUNC_MAX                      6
 
-/* D31:F0 LPC Processor Interface */
-#define ICH9_RST_CNT_IOPORT                     0xCF9
-
 /* D31:F1 LPC controller */
-#define ICH9_A2_LPC                             "ICH9 A2 LPC"
-#define ICH9_A2_LPC_SAVEVM_VERSION              0
-
 #define ICH9_LPC_DEV                            31
 #define ICH9_LPC_FUNC                           0
 
-#define ICH9_A2_LPC_REVISION                    0x2
-#define ICH9_LPC_NB_PIRQS                       8       /* PCI A-H */
-
-#define ICH9_LPC_PMBASE                         0x40
-#define ICH9_LPC_PMBASE_BASE_ADDRESS_MASK       ICH9_MASK(32, 15, 7)
-#define ICH9_LPC_PMBASE_RTE                     0x1
-#define ICH9_LPC_PMBASE_DEFAULT                 0x1
-
-#define ICH9_LPC_ACPI_CTRL                      0x44
-#define ICH9_LPC_ACPI_CTRL_ACPI_EN              0x80
-#define ICH9_LPC_ACPI_CTRL_SCI_IRQ_SEL_MASK     ICH9_MASK(8, 2, 0)
-#define ICH9_LPC_ACPI_CTRL_9                    0x0
-#define ICH9_LPC_ACPI_CTRL_10                   0x1
-#define ICH9_LPC_ACPI_CTRL_11                   0x2
-#define ICH9_LPC_ACPI_CTRL_20                   0x4
-#define ICH9_LPC_ACPI_CTRL_21                   0x5
-#define ICH9_LPC_ACPI_CTRL_DEFAULT              0x0
-
-#define ICH9_LPC_PIRQA_ROUT                     0x60
-#define ICH9_LPC_PIRQB_ROUT                     0x61
-#define ICH9_LPC_PIRQC_ROUT                     0x62
-#define ICH9_LPC_PIRQD_ROUT                     0x63
-
-#define ICH9_LPC_PIRQE_ROUT                     0x68
-#define ICH9_LPC_PIRQF_ROUT                     0x69
-#define ICH9_LPC_PIRQG_ROUT                     0x6a
-#define ICH9_LPC_PIRQH_ROUT                     0x6b
-
-#define ICH9_LPC_PIRQ_ROUT_IRQEN                0x80
-#define ICH9_LPC_PIRQ_ROUT_MASK                 ICH9_MASK(8, 3, 0)
-#define ICH9_LPC_PIRQ_ROUT_DEFAULT              0x80
-
-#define ICH9_LPC_GEN_PMCON_1                    0xa0
-#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK           (1 << 4)
-#define ICH9_LPC_GEN_PMCON_2                    0xa2
-#define ICH9_LPC_GEN_PMCON_3                    0xa4
-#define ICH9_LPC_GEN_PMCON_LOCK                 0xa6
-
-#define ICH9_LPC_RCBA                           0xf0
-#define ICH9_LPC_RCBA_BA_MASK                   ICH9_MASK(32, 31, 14)
-#define ICH9_LPC_RCBA_EN                        0x1
-#define ICH9_LPC_RCBA_DEFAULT                   0x0
-
-#define ICH9_LPC_PIC_NUM_PINS                   16
-#define ICH9_LPC_IOAPIC_NUM_PINS                24
-
 #define ICH9_GPIO_GSI "gsi"
 
-/* D31:F0 power management I/O registers
-   offset from the address ICH9_LPC_PMBASE */
-
-/* FADT ACPI_ENABLE/ACPI_DISABLE */
-#define ICH9_APM_ACPI_ENABLE                    0x2
-#define ICH9_APM_ACPI_DISABLE                   0x3
-
 #define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
 
 /* bit positions used in fw_cfg SMI feature negotiation */
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 660fa6a082..5e293ffb2a 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -37,6 +37,7 @@
 #include "hw/acpi/ich9_tco.h"
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
 
diff --git a/hw/acpi/ich9_tco.c b/hw/acpi/ich9_tco.c
index dd4aff82e0..7499ec17db 100644
--- a/hw/acpi/ich9_tco.c
+++ b/hw/acpi/ich9_tco.c
@@ -13,6 +13,7 @@
 #include "migration/vmstate.h"
 
 #include "hw/acpi/ich9_tco.h"
+#include "hw/isa/ich9_lpc.h"
 #include "trace.h"
 
 enum {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d3ce96dd9f..34f3f03949 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -56,6 +56,7 @@
 
 /* Supported chipsets: */
 #include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/acpi/pcihp.h"
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/pc.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e5f5bb0db1..573a5a0bc0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,6 +50,7 @@
 #include "hw/ide/ahci-pci.h"
 #include "hw/intc/ioapic.h"
 #include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 70c6e8a093..685ac38c72 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -1,5 +1,5 @@
 /*
- * QEMU ICH9 Emulation
+ * QEMU ICH9 LPC PCI-to-ISA bridge Emulation
  *
  * Copyright (c) 2006 Fabrice Bellard
  * Copyright (c) 2009, 2010, 2011
@@ -42,6 +42,7 @@
 #include "hw/pci/pci.h"
 #include "hw/southbridge/ich9.h"
 #include "hw/i386/pc.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9.h"
 #include "hw/pci/pci_bus.h"
@@ -54,10 +55,35 @@
 #include "hw/acpi/acpi_aml_interface.h"
 #include "trace.h"
 
-/*****************************************************************************/
-/* ICH9 LPC PCI to ISA bridge */
+#define ICH9_A2_LPC_REVISION                    0x2
+#define ICH9_LPC_NB_PIRQS                       8       /* PCI A-H */
 
-/* chipset configuration register
+/* ICH9: Chipset Configuration Registers */
+#define ICH9_CC_ADDR_MASK                       (ICH9_CC_SIZE - 1)
+
+#define ICH9_CC
+#define ICH9_CC_D28IP                           0x310C
+#define ICH9_CC_D28IP_SHIFT                     4
+#define ICH9_CC_D28IP_MASK                      0xf
+#define ICH9_CC_D28IP_DEFAULT                   0x00214321
+#define ICH9_CC_D31IR                           0x3140
+#define ICH9_CC_D30IR                           0x3142
+#define ICH9_CC_D29IR                           0x3144
+#define ICH9_CC_D28IR                           0x3146
+#define ICH9_CC_D27IR                           0x3148
+#define ICH9_CC_D26IR                           0x314C
+#define ICH9_CC_D25IR                           0x3150
+#define ICH9_CC_DIR_DEFAULT                     0x3210
+#define ICH9_CC_D30IR_DEFAULT                   0x0
+#define ICH9_CC_DIR_SHIFT                       4
+#define ICH9_CC_DIR_MASK                        0x7
+#define ICH9_CC_OIC                             0x31FF
+#define ICH9_CC_OIC_AEN                         0x1
+#define ICH9_CC_GCS                             0x3410
+#define ICH9_CC_GCS_DEFAULT                     0x00000020
+#define ICH9_CC_GCS_NO_REBOOT                   (1 << 5)
+
+/*
  * to access chipset configuration registers, pci_[sg]et_{byte, word, long}
  * are used.
  * Although it's not pci configuration space, it's little endian as Intel.
diff --git a/tests/qtest/tco-test.c b/tests/qtest/tco-test.c
index 0547d41173..c5974e72bd 100644
--- a/tests/qtest/tco-test.c
+++ b/tests/qtest/tco-test.c
@@ -14,7 +14,7 @@
 #include "libqos/pci-pc.h"
 #include "qapi/qmp/qdict.h"
 #include "hw/pci/pci_regs.h"
-#include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/acpi/ich9.h"
 #include "hw/acpi/ich9_tco.h"
 
-- 
2.41.0

[PATCH 14/14] hw/southbridge/ich9: Add the LPC / ISA bridge function

[Philippe Mathieu-Daudé]

Instantiate TYPE_ICH9_LPC_DEVICE in TYPE_ICH9_SOUTHBRIDGE.

Expose the SMM property so the Q35 machine can disable it
(depending on the accelerator used).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/southbridge/ich9.h |  4 ----
 hw/i386/pc_q35.c              | 12 +++---------
 hw/isa/lpc_ich9.c             |  3 +++
 hw/southbridge/ich9.c         | 14 ++++++++++++++
 hw/i386/Kconfig               |  1 -
 hw/southbridge/Kconfig        |  1 +
 6 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index d6c3b5ece3..a8da4a8665 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -16,10 +16,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(ICH9State, ICH9_SOUTHBRIDGE)
 #define ICH9_PCIE_DEV                           28
 #define ICH9_PCIE_FUNC_MAX                      6
 
-/* D31:F1 LPC controller */
-#define ICH9_LPC_DEV                            31
-#define ICH9_LPC_FUNC                           0
-
 #define ICH9_GPIO_GSI "gsi"
 
 #define ICH9_LPC_SMI_NEGOTIATED_FEAT_PROP "x-smi-negotiated-features"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 573a5a0bc0..0fe43e8e2d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,7 +50,6 @@
 #include "hw/ide/ahci-pci.h"
 #include "hw/intc/ioapic.h"
 #include "hw/southbridge/ich9.h"
-#include "hw/isa/ich9_lpc.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
@@ -69,7 +68,6 @@ static void pc_q35_init(MachineState *machine)
     Object *phb;
     PCIBus *host_bus;
     DeviceState *ich9;
-    PCIDevice *lpc;
     Object *lpc_obj;
     DeviceState *lpc_dev;
     BusState *idebus[2] = { };
@@ -181,6 +179,7 @@ static void pc_q35_init(MachineState *machine)
     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
                              OBJECT(host_bus), &error_abort);
     qdev_prop_set_bit(ich9, "d2p-enabled", false);
+    qdev_prop_set_bit(ich9, "smm-enabled", x86_machine_is_smm_enabled(x86ms));
     qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
     qdev_prop_set_bit(ich9, "smbus-enabled", pcms->smbus_enabled);
     /* Should we create 6 UHCI according to ich9 spec? */
@@ -191,13 +190,8 @@ static void pc_q35_init(MachineState *machine)
     gsi_state = pc_gsi_create(&x86ms->gsi, true);
 
     /* create ISA bus */
-    lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
-                                TYPE_ICH9_LPC_DEVICE);
-    lpc_obj = OBJECT(lpc);
-    lpc_dev = DEVICE(lpc);
-    qdev_prop_set_bit(lpc_dev, "smm-enabled",
-                      x86_machine_is_smm_enabled(x86ms));
-    pci_realize_and_unref(lpc, host_bus, &error_fatal);
+    lpc_obj = object_resolve_path_component(OBJECT(ich9), "lpc");
+    lpc_dev = DEVICE(lpc_obj);
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
     }
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 685ac38c72..4d7b5c0c64 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -55,6 +55,9 @@
 #include "hw/acpi/acpi_aml_interface.h"
 #include "trace.h"
 
+#define ICH9_LPC_DEV                            31
+#define ICH9_LPC_FUNC                           0
+
 #define ICH9_A2_LPC_REVISION                    0x2
 #define ICH9_LPC_NB_PIRQS                       8       /* PCI A-H */
 
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index 085d75e569..57a05b35e1 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -13,6 +13,7 @@
 #include "hw/southbridge/ich9.h"
 #include "hw/pci/pci.h"
 #include "hw/pci-bridge/ich_dmi_pci.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/ide/ahci-pci.h"
 #include "hw/ide.h"
 #include "hw/i2c/smbus_ich9.h"
@@ -21,6 +22,7 @@
 #include "hw/usb/hcd-uhci.h"
 
 #define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
+#define ICH9_LPC_DEVFN          PCI_DEVFN(31, 0)
 #define ICH9_SATA1_DEVFN        PCI_DEVFN(31, 2)
 #define ICH9_SMB_DEVFN          PCI_DEVFN(31, 3)
 #define ICH9_EHCI_FUNC          7
@@ -34,6 +36,7 @@ struct ICH9State {
 
     I82801b11Bridge d2p;
     AHCIPCIState sata0;
+    ICH9LPCState lpc;
     ICH9SMBState smb;
     EHCIPCIState ehci[EHCI_PER_FN];
     UHCIState uhci[EHCI_PER_FN * UHCI_PER_FN];
@@ -148,6 +151,13 @@ static bool ich9_realize_usb(ICH9State *s, Error **errp)
 
 static void ich9_init(Object *obj)
 {
+    ICH9State *s = ICH9_SOUTHBRIDGE(obj);
+
+    object_initialize_child(obj, "lpc", &s->lpc, TYPE_ICH9_LPC_DEVICE);
+    qdev_prop_set_int32(DEVICE(&s->lpc), "addr", ICH9_LPC_DEVFN);
+    qdev_prop_set_bit(DEVICE(&s->lpc), "multifunction", true);
+    object_property_add_alias(obj, "smm-enabled",
+                              OBJECT(&s->lpc), "smm-enabled");
 }
 
 static void ich9_realize(DeviceState *dev, Error **errp)
@@ -163,6 +173,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!qdev_realize(DEVICE(&s->lpc), BUS(s->pci_bus), errp)) {
+        return;
+    }
+
     if (s->sata_enabled && !ich9_realize_sata(s, errp)) {
         return;
     }
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index 226d7f6916..eccc834e49 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -100,7 +100,6 @@ config Q35
     select PC_ACPI
     select PCI_EXPRESS_Q35
     select ICH9
-    select LPC_ICH9
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index 31eb125bf7..8ce62b703c 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -8,3 +8,4 @@ config ICH9
     select ACPI_ICH9
     imply USB_EHCI_PCI
     imply USB_UHCI
+    select LPC_ICH9
-- 
2.41.0