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

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

[Philippe Mathieu-Daudé]

Since v1 [1]:
- Rebased on top of Bernhard patches
- Rename files with 'ich9_' prefix (Bernhard)

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 [2]. Once the LPC/ISA part is
done, it might be easier to fix it.

[1] https://lore.kernel.org/qemu-devel/20240219163855.87326-1-philmd@linaro.org/
[2] https://lore.kernel.org/qemu-devel/cd0e13c6-c03d-411f-83a5-1d4d28ea4345@linaro.org/

Philippe Mathieu-Daudé (15):
  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/ide: Rename ich.c -> ich9_ahci.c
  hw/i2c/smbus: Extract QOM ICH9 definitions to 'ich9_smbus.h'
  hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.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/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/ich9_smbus.h               |  25 +++
 include/hw/isa/ich9_lpc.h                 | 166 +++++++++++++++
 include/hw/pci-bridge/ich9_dmi.h          |  20 ++
 include/hw/southbridge/ich9.h             | 235 +---------------------
 hw/acpi/ich9.c                            |   9 +-
 hw/acpi/ich9_tco.c                        |   5 +-
 hw/i2c/{smbus_ich9.c => ich9_smbus.c}     |  36 +++-
 hw/i386/acpi-build.c                      |   1 +
 hw/i386/pc_q35.c                          | 126 +++---------
 hw/ide/{ich.c => ich9_ahci.c}             |   0
 hw/isa/{lpc_ich9.c => ich9_lpc.c}         |  37 +++-
 hw/pci-bridge/{i82801b11.c => ich9_dmi.c} |  11 +-
 hw/southbridge/ich9.c                     | 213 ++++++++++++++++++++
 tests/qtest/tco-test.c                    |   2 +-
 hw/Kconfig                                |   1 +
 hw/i2c/meson.build                        |   2 +-
 hw/i386/Kconfig                           |   3 +-
 hw/ide/meson.build                        |   2 +-
 hw/isa/meson.build                        |   2 +-
 hw/meson.build                            |   1 +
 hw/pci-bridge/meson.build                 |   2 +-
 hw/southbridge/Kconfig                    |  11 +
 hw/southbridge/meson.build                |   3 +
 26 files changed, 587 insertions(+), 368 deletions(-)
 create mode 100644 include/hw/i2c/ich9_smbus.h
 create mode 100644 include/hw/isa/ich9_lpc.h
 create mode 100644 include/hw/pci-bridge/ich9_dmi.h
 rename hw/i2c/{smbus_ich9.c => ich9_smbus.c} (77%)
 rename hw/ide/{ich.c => ich9_ahci.c} (100%)
 rename hw/isa/{lpc_ich9.c => ich9_lpc.c} (95%)
 rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)
 create mode 100644 hw/southbridge/ich9.c
 create mode 100644 hw/southbridge/Kconfig
 create mode 100644 hw/southbridge/meson.build

-- 
2.41.0

[PATCH v2 01/15] 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 992799171f..e3f14c28a8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1816,12 +1816,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
@@ -2614,6 +2609,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 v2 02/15] 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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 45a4102e75..dcad6000d9 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -124,6 +124,7 @@ static void pc_q35_init(MachineState *machine)
     X86MachineState *x86ms = X86_MACHINE(machine);
     Object *phb;
     PCIDevice *lpc;
+    Object *lpc_obj;
     DeviceState *lpc_dev;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
@@ -223,6 +224,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));
@@ -231,7 +233,7 @@ static void pc_q35_init(MachineState *machine)
     }
     pci_realize_and_unref(lpc, pcms->pcibus, &error_fatal);
 
-    x86ms->rtc = ISA_DEVICE(object_resolve_path_component(OBJECT(lpc), "rtc"));
+    x86ms->rtc = ISA_DEVICE(object_resolve_path_component(lpc_obj, "rtc"));
 
     object_property_add_link(OBJECT(machine), PC_MACHINE_ACPI_DEVICE_PROP,
                              TYPE_HOTPLUG_HANDLER,
@@ -239,13 +241,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);
 
@@ -255,7 +257,7 @@ static void pc_q35_init(MachineState *machine)
                                    "true", true);
     }
 
-    isa_bus = ISA_BUS(qdev_get_child_bus(lpc_dev, "isa.0"));
+    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(lpc_obj), "isa.0"));
 
     if (x86ms->pic == ON_OFF_AUTO_ON || x86ms->pic == ON_OFF_AUTO_AUTO) {
         pc_i8259_create(isa_bus, gsi_state->i8259_irq);
-- 
2.41.0

[PATCH v2 03/15] 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 6205de6046..4c75d1b56b 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 v2 04/15] 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 4c75d1b56b..daf93361eb 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 v2 05/15] 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

[PATCH v2 06/15] hw/ide: Rename ich.c -> ich9_ahci.c

[Philippe Mathieu-Daudé]

Most ICH9-related files use the 'ich9_' prefix.
Mention 'AHCI' in the file name.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/{ich.c => ich9_ahci.c} | 0
 hw/ide/meson.build            | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename hw/ide/{ich.c => ich9_ahci.c} (100%)

diff --git a/hw/ide/ich.c b/hw/ide/ich9_ahci.c
similarity index 100%
rename from hw/ide/ich.c
rename to hw/ide/ich9_ahci.c
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index d09705cac0..2a7cd8405d 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -1,5 +1,5 @@
 system_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
-system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
+system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich9_ahci.c'))
 system_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
 system_ss.add(when: 'CONFIG_IDE_BUS', if_true: files('ide-bus.c'))
 system_ss.add(when: 'CONFIG_IDE_CF', if_true: files('cf.c'))
-- 
2.41.0

Re: [PATCH v2 06/15] hw/ide: Rename ich.c -> ich9_ahci.c

[BALATON Zoltan]

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

On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> Most ICH9-related files use the 'ich9_' prefix.
> Mention 'AHCI' in the file name.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/ide/{ich.c => ich9_ahci.c} | 0

That would rather be ahci-ich9.h then analogous to ahci-allwinnet.c 
maybe?

Regsards,
BALATON Zoltan

> hw/ide/meson.build            | 2 +-
> 2 files changed, 1 insertion(+), 1 deletion(-)
> rename hw/ide/{ich.c => ich9_ahci.c} (100%)
>
> diff --git a/hw/ide/ich.c b/hw/ide/ich9_ahci.c
> similarity index 100%
> rename from hw/ide/ich.c
> rename to hw/ide/ich9_ahci.c
> diff --git a/hw/ide/meson.build b/hw/ide/meson.build
> index d09705cac0..2a7cd8405d 100644
> --- a/hw/ide/meson.build
> +++ b/hw/ide/meson.build
> @@ -1,5 +1,5 @@
> system_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
> -system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
> +system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich9_ahci.c'))
> system_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
> system_ss.add(when: 'CONFIG_IDE_BUS', if_true: files('ide-bus.c'))
> system_ss.add(when: 'CONFIG_IDE_CF', if_true: files('cf.c'))
>

Re: [PATCH v2 06/15] hw/ide: Rename ich.c -> ich9_ahci.c

[Philippe Mathieu-Daudé]

On 26/2/24 13:52, BALATON Zoltan wrote:
> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Most ICH9-related files use the 'ich9_' prefix.
>> Mention 'AHCI' in the file name.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/ide/{ich.c => ich9_ahci.c} | 0
> 
> That would rather be ahci-ich9.h then analogous to ahci-allwinnet.c maybe?

We can have all ICH9 functions named ich9_foo.c, or all AHCI
implementations named bar_ahci.c. But currently there is no
particular style enforced and we have a mix. I don't mind much.

> Regsards,
> BALATON Zoltan
> 
>> hw/ide/meson.build            | 2 +-
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>> rename hw/ide/{ich.c => ich9_ahci.c} (100%)

Re: [PATCH v2 06/15] hw/ide: Rename ich.c -> ich9_ahci.c

[BALATON Zoltan]

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

On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 26/2/24 13:52, BALATON Zoltan wrote:
>> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> Most ICH9-related files use the 'ich9_' prefix.
>>> Mention 'AHCI' in the file name.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/ide/{ich.c => ich9_ahci.c} | 0
>> 
>> That would rather be ahci-ich9.h then analogous to ahci-allwinnet.c maybe?
>
> We can have all ICH9 functions named ich9_foo.c, or all AHCI
> implementations named bar_ahci.c. But currently there is no
> particular style enforced and we have a mix. I don't mind much.

I think if it's in hw/ide then it should follow conventions used by that 
part. (Which did not follow any convention befure but Thomas adding a 
bunch of headers using - made it have a convention now.) If it's a variant 
of AHCI like the allwinner one then it should be groupped with that and 
named accordingly. If it would be together with other ich9 parts then it 
could be grouped there or if there wasn't already an example for ahci 
variant, but in this case only ahci-ich9 seems consistent within IDE now.

Regsards,
BALATON Zoltan

[PATCH v2 07/15] hw/i2c/smbus: Extract QOM ICH9 definitions to 'ich9_smbus.h'

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                           |  1 +
 include/hw/i2c/ich9_smbus.h           | 25 +++++++++++++++++++++++++
 hw/i2c/{smbus_ich9.c => ich9_smbus.c} | 15 ++-------------
 hw/i2c/meson.build                    |  2 +-
 4 files changed, 29 insertions(+), 14 deletions(-)
 create mode 100644 include/hw/i2c/ich9_smbus.h
 rename hw/i2c/{smbus_ich9.c => ich9_smbus.c} (95%)

diff --git a/MAINTAINERS b/MAINTAINERS
index e3f14c28a8..0849283287 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2617,6 +2617,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/i2c/ich9_smbus.h
 F: include/hw/southbridge/ich9.h
 
 PIIX4 South Bridge (i82371AB)
diff --git a/include/hw/i2c/ich9_smbus.h b/include/hw/i2c/ich9_smbus.h
new file mode 100644
index 0000000000..d6f46ba89c
--- /dev/null
+++ b/include/hw/i2c/ich9_smbus.h
@@ -0,0 +1,25 @@
+/*
+ * QEMU ICH9 SMBus emulation
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_I2C_ICH9_SMBUS_H
+#define HW_I2C_ICH9_SMBUS_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/ich9_smbus.c
similarity index 95%
rename from hw/i2c/smbus_ich9.c
rename to hw/i2c/ich9_smbus.c
index 208f263ac5..35f526d71c 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/ich9_smbus.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/ich9_smbus.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();
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index b58bc167db..f1a122c5ec 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -2,7 +2,7 @@ i2c_ss = ss.source_set()
 i2c_ss.add(when: 'CONFIG_I2C', if_true: files('core.c'))
 i2c_ss.add(when: 'CONFIG_SMBUS', if_true: files('smbus_slave.c', 'smbus_master.c'))
 i2c_ss.add(when: 'CONFIG_ACPI_SMBUS', if_true: files('pm_smbus.c'))
-i2c_ss.add(when: 'CONFIG_ACPI_ICH9', if_true: files('smbus_ich9.c'))
+i2c_ss.add(when: 'CONFIG_ACPI_ICH9', if_true: files('ich9_smbus.c'))
 i2c_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_i2c.c'))
 i2c_ss.add(when: 'CONFIG_BITBANG_I2C', if_true: files('bitbang_i2c.c'))
 i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c'))
-- 
2.41.0

[PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS                               |  1 +
 include/hw/pci-bridge/ich9_dmi.h          | 20 ++++++++++++++++++++
 include/hw/southbridge/ich9.h             |  2 --
 hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++-------
 hw/pci-bridge/meson.build                 |  2 +-
 5 files changed, 26 insertions(+), 10 deletions(-)
 create mode 100644 include/hw/pci-bridge/ich9_dmi.h
 rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0849283287..52282c680e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c
 F: hw/isa/lpc_ich9.c
 F: include/hw/acpi/ich9*.h
 F: include/hw/i2c/ich9_smbus.h
+F: include/hw/pci-bridge/ich9_dmi.h
 F: include/hw/southbridge/ich9.h
 
 PIIX4 South Bridge (i82371AB)
diff --git a/include/hw/pci-bridge/ich9_dmi.h b/include/hw/pci-bridge/ich9_dmi.h
new file mode 100644
index 0000000000..7cf5d9d9b2
--- /dev/null
+++ b/include/hw/pci-bridge/ich9_dmi.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_ICH9_DMI_H
+#define HW_PCI_BRIDGE_ICH9_DMI_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/ich9_dmi.c
similarity index 95%
rename from hw/pci-bridge/i82801b11.c
rename to hw/pci-bridge/ich9_dmi.c
index c140919cbc..927e48bf2e 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/ich9_dmi.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/ich9_dmi.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,
diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build
index f2a60434dd..d746487193 100644
--- a/hw/pci-bridge/meson.build
+++ b/hw/pci-bridge/meson.build
@@ -1,6 +1,6 @@
 pci_ss = ss.source_set()
 pci_ss.add(files('pci_bridge_dev.c'))
-pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c'))
+pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c'))
 pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c'))
 pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: files('pcie_root_port.c', 'gen_pcie_root_port.c'))
 pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: files('pcie_pci_bridge.c'))
-- 
2.41.0

Re: [PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

[BALATON Zoltan]

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

On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
> "hw/pci-bridge/ich9_dmi.h" header.

Since this is effectively an empty object (that's not even instantiated by 
default) I still think that instead of adding even more files for it all 
this could just be moved to hw/isa/lpc_ich9.c and define there as an 
internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, 
ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's 
instance size. That just adds the realize function and a type definition 
and gets rid of boilerplate scattered around the source tree which just 
adds complexity for no reason. But I don't care too much about it, just 
wanted to say again that if something can be kept simple I'd prefer that 
over making it more complex and for this device it looks already too 
complex for what it does or used for.

Regards,
BALATON Zoltan

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS                               |  1 +
> include/hw/pci-bridge/ich9_dmi.h          | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h             |  2 --
> hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++-------
> hw/pci-bridge/meson.build                 |  2 +-
> 5 files changed, 26 insertions(+), 10 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich9_dmi.h
> rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0849283287..52282c680e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
> F: include/hw/i2c/ich9_smbus.h
> +F: include/hw/pci-bridge/ich9_dmi.h
> F: include/hw/southbridge/ich9.h
>
> PIIX4 South Bridge (i82371AB)
> diff --git a/include/hw/pci-bridge/ich9_dmi.h b/include/hw/pci-bridge/ich9_dmi.h
> new file mode 100644
> index 0000000000..7cf5d9d9b2
> --- /dev/null
> +++ b/include/hw/pci-bridge/ich9_dmi.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_ICH9_DMI_H
> +#define HW_PCI_BRIDGE_ICH9_DMI_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/ich9_dmi.c
> similarity index 95%
> rename from hw/pci-bridge/i82801b11.c
> rename to hw/pci-bridge/ich9_dmi.c
> index c140919cbc..927e48bf2e 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/ich9_dmi.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/ich9_dmi.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,
> diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build
> index f2a60434dd..d746487193 100644
> --- a/hw/pci-bridge/meson.build
> +++ b/hw/pci-bridge/meson.build
> @@ -1,6 +1,6 @@
> pci_ss = ss.source_set()
> pci_ss.add(files('pci_bridge_dev.c'))
> -pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c'))
> +pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c'))
> pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c'))
> pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: files('pcie_root_port.c', 'gen_pcie_root_port.c'))
> pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: files('pcie_pci_bridge.c'))
>

Re: [PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

[Philippe Mathieu-Daudé]

On 26/2/24 14:01, BALATON Zoltan wrote:
> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>> "hw/pci-bridge/ich9_dmi.h" header.
> 
> Since this is effectively an empty object (that's not even instantiated 
> by default) I still think that instead of adding even more files for it 
> all this could just be moved to hw/isa/lpc_ich9.c and define there as an 
> internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, 
> ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's 
> instance size. That just adds the realize function and a type definition 
> and gets rid of boilerplate scattered around the source tree which just 
> adds complexity for no reason. But I don't care too much about it, just 
> wanted to say again that if something can be kept simple I'd prefer that 
> over making it more complex and for this device it looks already too 
> complex for what it does or used for.

My understanding was project coherency and style is preferred over
simplifications / optimizations, so we prefer explicit TYPE_FOO
and corresponding OBJECT_DECLARE_XXX() in a public include/hw/foo
header -- because it will end up copy/pasted --, but I might be
wrong.

> Regards,
> BALATON Zoltan
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> MAINTAINERS                               |  1 +
>> include/hw/pci-bridge/ich9_dmi.h          | 20 ++++++++++++++++++++
>> include/hw/southbridge/ich9.h             |  2 --
>> hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++-------
>> hw/pci-bridge/meson.build                 |  2 +-
>> 5 files changed, 26 insertions(+), 10 deletions(-)
>> create mode 100644 include/hw/pci-bridge/ich9_dmi.h
>> rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0849283287..52282c680e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c
>> F: hw/isa/lpc_ich9.c
>> F: include/hw/acpi/ich9*.h
>> F: include/hw/i2c/ich9_smbus.h
>> +F: include/hw/pci-bridge/ich9_dmi.h
>> F: include/hw/southbridge/ich9.h
>>
>> PIIX4 South Bridge (i82371AB)
>> diff --git a/include/hw/pci-bridge/ich9_dmi.h 
>> b/include/hw/pci-bridge/ich9_dmi.h
>> new file mode 100644
>> index 0000000000..7cf5d9d9b2
>> --- /dev/null
>> +++ b/include/hw/pci-bridge/ich9_dmi.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_ICH9_DMI_H
>> +#define HW_PCI_BRIDGE_ICH9_DMI_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/ich9_dmi.c
>> similarity index 95%
>> rename from hw/pci-bridge/i82801b11.c
>> rename to hw/pci-bridge/ich9_dmi.c
>> index c140919cbc..927e48bf2e 100644
>> --- a/hw/pci-bridge/i82801b11.c
>> +++ b/hw/pci-bridge/ich9_dmi.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/ich9_dmi.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,
>> diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build
>> index f2a60434dd..d746487193 100644
>> --- a/hw/pci-bridge/meson.build
>> +++ b/hw/pci-bridge/meson.build
>> @@ -1,6 +1,6 @@
>> pci_ss = ss.source_set()
>> pci_ss.add(files('pci_bridge_dev.c'))
>> -pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c'))
>> +pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c'))
>> pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c'))
>> pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: 
>> files('pcie_root_port.c', 'gen_pcie_root_port.c'))
>> pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: 
>> files('pcie_pci_bridge.c'))
>>

Re: [PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

[BALATON Zoltan]

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

On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> On 26/2/24 14:01, BALATON Zoltan wrote:
>> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>> "hw/pci-bridge/ich9_dmi.h" header.
>> 
>> Since this is effectively an empty object (that's not even instantiated by 
>> default) I still think that instead of adding even more files for it all 
>> this could just be moved to hw/isa/lpc_ich9.c and define there as an 
>> internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, 
>> ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's 
>> instance size. That just adds the realize function and a type definition 
>> and gets rid of boilerplate scattered around the source tree which just 
>> adds complexity for no reason. But I don't care too much about it, just 
>> wanted to say again that if something can be kept simple I'd prefer that 
>> over making it more complex and for this device it looks already too 
>> complex for what it does or used for.
>
> My understanding was project coherency and style is preferred over
> simplifications / optimizations, so we prefer explicit TYPE_FOO
> and corresponding OBJECT_DECLARE_XXX() in a public include/hw/foo
> header -- because it will end up copy/pasted --, but I might be
> wrong.

For classes that are actually used that may be true but this class isn't 
used anywhere just defined for the user to be instantiated from command 
line or config. So you won't even need a type name in code currently. It's 
also an internal part of ICH southbridge so no need to be exposed outside 
of that as it's only useful within that chip. Finally it has no 
functionality, just an empty boilerplate so that the device appears in 
lspci output but it does nothing. Therefore, I think it's simpler to just 
move all of it within the same file where the southbridge is implemented 
and define as empty object there like the via-mc97 device in 
hw/audio/via-ac97.c which serves the same purpose to show the part to the 
guest but its empty device without function. That would save half of all 
this boilerplate that just adds complexity now. If sometimes in the future 
this device gains some functionality then it can be split off and add all 
the defines and stuff around it but that's not needed yet so why not keep 
it simple?

Regards,
BALATON Zoltan

Re: [PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

[Philippe Mathieu-Daudé]

On 26/2/24 14:23, BALATON Zoltan wrote:
> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
>> On 26/2/24 14:01, BALATON Zoltan wrote:
>>> On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
>>>> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
>>>> "hw/pci-bridge/ich9_dmi.h" header.
>>>
>>> Since this is effectively an empty object (that's not even 
>>> instantiated by default) I still think that instead of adding even 
>>> more files for it all this could just be moved to hw/isa/lpc_ich9.c 
>>> and define there as an internal object and drop the 
>>> OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE) and 
>>> just use the size of the superclass as it's instance size. That just 
>>> adds the realize function and a type definition and gets rid of 
>>> boilerplate scattered around the source tree which just adds 
>>> complexity for no reason. But I don't care too much about it, just 
>>> wanted to say again that if something can be kept simple I'd prefer 
>>> that over making it more complex and for this device it looks already 
>>> too complex for what it does or used for.
>>
>> My understanding was project coherency and style is preferred over
>> simplifications / optimizations, so we prefer explicit TYPE_FOO
>> and corresponding OBJECT_DECLARE_XXX() in a public include/hw/foo
>> header -- because it will end up copy/pasted --, but I might be
>> wrong.
> 
> For classes that are actually used that may be true but this class isn't 
> used anywhere just defined for the user to be instantiated from command 
> line or config. So you won't even need a type name in code currently. 
> It's also an internal part of ICH southbridge so no need to be exposed 
> outside of that as it's only useful within that chip. Finally it has no 
> functionality, just an empty boilerplate so that the device appears in 
> lspci output but it does nothing. Therefore, I think it's simpler to 
> just move all of it within the same file where the southbridge is 
> implemented and define as empty object there like the via-mc97 device in 
> hw/audio/via-ac97.c which serves the same purpose to show the part to 
> the guest but its empty device without function. That would save half of 
> all this boilerplate that just adds complexity now. If sometimes in the 
> future this device gains some functionality then it can be split off and 
> add all the defines and stuff around it but that's not needed yet so why 
> not keep it simple?

Like you, I don't care much about the ICH9 device. However it seemed
a complex-enough chipset worth trying to model it right with QDev so
we could then split the code in QOM Properties / QDev API / real
implementation to prototype a DSL, generate the QDev boiler plate and
keep the implementation part. Again, price worth to spend a bit in
order to get something simpler later.

[PATCH v2 09/15] 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 52282c680e..4576339053 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2616,6 +2616,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/i2c/ich9_smbus.h
 F: include/hw/pci-bridge/ich9_dmi.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 dcad6000d9..8c8a2f65b8 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -123,6 +123,7 @@ static void pc_q35_init(MachineState *machine)
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     X86MachineState *x86ms = X86_MACHINE(machine);
     Object *phb;
+    DeviceState *ich9;
     PCIDevice *lpc;
     Object *lpc_obj;
     DeviceState *lpc_dev;
@@ -221,6 +222,12 @@ static void pc_q35_init(MachineState *machine)
     /* irq lines */
     gsi_state = pc_gsi_create(&x86ms->gsi, true);
 
+    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(pcms->pcibus), &error_abort);
+    qdev_realize_and_unref(ich9, NULL, &error_fatal);
+
     /* create ISA bus */
     lpc = pci_new_multifunction(PCI_DEVFN(ICH9_LPC_DEV, ICH9_LPC_FUNC),
                                 TYPE_ICH9_LPC_DEVICE);
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 v2 10/15] 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 8c8a2f65b8..f951cf1e3a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -226,6 +226,7 @@ static void pc_q35_init(MachineState *machine)
     object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
                              OBJECT(pcms->pcibus), &error_abort);
+    qdev_prop_set_bit(ich9, "d2p-enabled", false);
     qdev_realize_and_unref(ich9, NULL, &error_fatal);
 
     /* create ISA bus */
diff --git a/hw/southbridge/ich9.c b/hw/southbridge/ich9.c
index f3a9b932ab..8c4356ff74 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -12,16 +12,23 @@
 #include "hw/qdev-properties.h"
 #include "hw/southbridge/ich9.h"
 #include "hw/pci/pci.h"
+#include "hw/pci-bridge/ich9_dmi.h"
+
+#define ICH9_D2P_DEVFN          PCI_DEVFN(30, 0)
 
 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(),
 };
 
@@ -29,6 +36,22 @@ static void ich9_init(Object *obj)
 {
 }
 
+static bool ich9_realize_d2p(ICH9State *s, Error **errp)
+{
+    if (!module_object_class_by_name(TYPE_ICH_DMI_PCI_BRIDGE)) {
+        error_setg(errp, "DMI-to-PCI function not available in this build");
+        return false;
+    }
+    object_initialize_child(OBJECT(s), "d2p", &s->d2p, TYPE_ICH_DMI_PCI_BRIDGE);
+    qdev_prop_set_int32(DEVICE(&s->d2p), "addr", ICH9_D2P_DEVFN);
+    if (!qdev_realize(DEVICE(&s->d2p), BUS(s->pci_bus), errp)) {
+        return false;
+    }
+    object_property_add_alias(OBJECT(s), "pci.0", OBJECT(&s->d2p), "pci.0");
+
+    return true;
+}
+
 static void ich9_realize(DeviceState *dev, Error **errp)
 {
     ICH9State *s = ICH9_SOUTHBRIDGE(dev);
@@ -37,6 +60,10 @@ static void ich9_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "'pcie-bus' property must be set");
         return;
     }
+
+    if (s->d2p_enabled && !ich9_realize_d2p(s, errp)) {
+        return;
+    }
 }
 
 static void ich9_class_init(ObjectClass *klass, void *data)
diff --git a/hw/southbridge/Kconfig b/hw/southbridge/Kconfig
index 852b7f346f..db7259bf6f 100644
--- a/hw/southbridge/Kconfig
+++ b/hw/southbridge/Kconfig
@@ -3,3 +3,4 @@
 config ICH9
     bool
     depends on PCI_EXPRESS
+    imply I82801B11
-- 
2.41.0

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

[Bernhard Beschow]

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

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

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

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

Best regards,
Bernhard

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

[PATCH v2 11/15] 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              | 21 +++------------------
 hw/southbridge/ich9.c         | 35 +++++++++++++++++++++++++++++++++++
 hw/i386/Kconfig               |  1 -
 hw/southbridge/Kconfig        |  1 +
 6 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4576339053..7d1b3e0d99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2615,10 +2615,12 @@ M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
 S: Supported
 F: hw/acpi/ich9*.c
 F: hw/i2c/smbus_ich9.c
+F: hw/ide/ich9_ahci.c
 F: hw/isa/lpc_ich9.c
 F: hw/southbridge/ich9.c
 F: include/hw/acpi/ich9*.h
 F: include/hw/i2c/ich9_smbus.h
+F: include/hw/ide/ahci-pci.h
 F: include/hw/pci-bridge/ich9_dmi.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 f951cf1e3a..6903719b97 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -60,9 +60,6 @@
 #include "hw/i386/acpi-build.h"
 #include "target/i386/cpu.h"
 
-/* ICH9 AHCI has 6 ports */
-#define MAX_SATA_PORTS     6
-
 struct ehci_companions {
     const char *name;
     int func;
@@ -134,7 +131,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;
@@ -227,6 +223,7 @@ static void pc_q35_init(MachineState *machine)
     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
                              OBJECT(pcms->pcibus), &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);
 
     /* create ISA bus */
@@ -287,20 +284,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(pcms->pcibus,
-                                               PCI_DEVFN(ICH9_SATA1_DEV,
-                                                         ICH9_SATA1_FUNC),
-                                               "ich9-ahci");
-        ich9 = ICH9_AHCI(pdev);
-        pcms->idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
-        pcms->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);
+        pcms->idebus[0] = qdev_get_child_bus(ich9, "ide.0");
+        pcms->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 8c4356ff74..37255bb941 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/ich9_dmi.h"
+#include "hw/ide/ahci-pci.h"
+#include "hw/ide/ide-dev.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(),
 };
 
@@ -52,6 +60,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_realize(DeviceState *dev, Error **errp)
 {
     ICH9State *s = ICH9_SOUTHBRIDGE(dev);
@@ -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 v2 11/15] hw/southbridge/ich9: Add a AHCI function

[Bernhard Beschow]

Am 26. Februar 2024 11:14:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>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              | 21 +++------------------
> hw/southbridge/ich9.c         | 35 +++++++++++++++++++++++++++++++++++
> hw/i386/Kconfig               |  1 -
> hw/southbridge/Kconfig        |  1 +
> 6 files changed, 41 insertions(+), 23 deletions(-)
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 4576339053..7d1b3e0d99 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -2615,10 +2615,12 @@ M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> S: Supported
> F: hw/acpi/ich9*.c
> F: hw/i2c/smbus_ich9.c
>+F: hw/ide/ich9_ahci.c
> F: hw/isa/lpc_ich9.c
> F: hw/southbridge/ich9.c
> F: include/hw/acpi/ich9*.h
> F: include/hw/i2c/ich9_smbus.h
>+F: include/hw/ide/ahci-pci.h
> F: include/hw/pci-bridge/ich9_dmi.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 f951cf1e3a..6903719b97 100644
>--- a/hw/i386/pc_q35.c
>+++ b/hw/i386/pc_q35.c
>@@ -60,9 +60,6 @@
> #include "hw/i386/acpi-build.h"
> #include "target/i386/cpu.h"
> 
>-/* ICH9 AHCI has 6 ports */
>-#define MAX_SATA_PORTS     6
>-
> struct ehci_companions {
>     const char *name;
>     int func;
>@@ -134,7 +131,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;
>@@ -227,6 +223,7 @@ static void pc_q35_init(MachineState *machine)
>     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
>                              OBJECT(pcms->pcibus), &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);
> 
>     /* create ISA bus */
>@@ -287,20 +284,8 @@ static void pc_q35_init(MachineState *machine)
>                          0xff0104);
> 
>     if (pcms->sata_enabled) {
>-        PCIDevice *pdev;
>-        AHCIPCIState *ich9;

The ahci include and perhaps all ide includes can be removed here.

Any plans for a v3?

Best regards,
Bernhard

>-
>-        /* ahci and SATA device, for q35 1 ahci controller is built-in */
>-        pdev = pci_create_simple_multifunction(pcms->pcibus,
>-                                               PCI_DEVFN(ICH9_SATA1_DEV,
>-                                                         ICH9_SATA1_FUNC),
>-                                               "ich9-ahci");
>-        ich9 = ICH9_AHCI(pdev);
>-        pcms->idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
>-        pcms->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);
>+        pcms->idebus[0] = qdev_get_child_bus(ich9, "ide.0");
>+        pcms->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 8c4356ff74..37255bb941 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/ich9_dmi.h"
>+#include "hw/ide/ahci-pci.h"
>+#include "hw/ide/ide-dev.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(),
> };
> 
>@@ -52,6 +60,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_realize(DeviceState *dev, Error **errp)
> {
>     ICH9State *s = ICH9_SOUTHBRIDGE(dev);
>@@ -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

[PATCH v2 12/15] 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/ich9_smbus.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/ich9_smbus.c b/hw/i2c/ich9_smbus.c
index 35f526d71c..1c3b9964de 100644
--- a/hw/i2c/ich9_smbus.c
+++ b/hw/i2c/ich9_smbus.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 6903719b97..0ff94b7afd 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -224,6 +224,7 @@ static void pc_q35_init(MachineState *machine)
                              OBJECT(pcms->pcibus), &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);
 
     /* create ISA bus */
@@ -294,15 +295,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(pcms->pcibus,
-                                              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 37255bb941..413e9187e4 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -15,9 +15,11 @@
 #include "hw/pci-bridge/ich9_dmi.h"
 #include "hw/ide/ahci-pci.h"
 #include "hw/ide/ide-dev.h"
+#include "hw/i2c/ich9_smbus.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(),
 };
 
@@ -83,6 +88,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_realize(DeviceState *dev, Error **errp)
 {
     ICH9State *s = ICH9_SOUTHBRIDGE(dev);
@@ -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 v2 13/15] 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 0ff94b7afd..63fec8b439 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"
@@ -60,59 +58,6 @@
 #include "hw/i386/acpi-build.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)
 {
@@ -225,6 +170,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);
 
     /* create ISA bus */
@@ -289,11 +236,6 @@ static void pc_q35_init(MachineState *machine)
         pcms->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(pcms->pcibus, 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 413e9187e4..f05012959d 100644
--- a/hw/southbridge/ich9.c
+++ b/hw/southbridge/ich9.c
@@ -16,12 +16,18 @@
 #include "hw/ide/ahci-pci.h"
 #include "hw/ide/ide-dev.h"
 #include "hw/i2c/ich9_smbus.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(),
 };
 
@@ -100,6 +110,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(1))
+        || !module_object_class_by_name("ich9-usb-ehci1")) {
+        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_realize(DeviceState *dev, Error **errp)
 {
     ICH9State *s = ICH9_SOUTHBRIDGE(dev);
@@ -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 v2 14/15] hw/southbridge/ich9: Extract LPC definitions to 'hw/isa/ich9_lpc.h'

[Philippe Mathieu-Daudé]

Most ICH9-related files use the 'ich9_' prefix,
rename lpc_ich9.c as ich9_lpc.c.

Restrict 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 => ich9_lpc.c} |  34 +++++-
 tests/qtest/tco-test.c            |   2 +-
 hw/isa/meson.build                |   2 +-
 10 files changed, 209 insertions(+), 158 deletions(-)
 create mode 100644 include/hw/isa/ich9_lpc.h
 rename hw/isa/{lpc_ich9.c => ich9_lpc.c} (95%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d1b3e0d99..6b783e3360 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2621,6 +2621,7 @@ F: hw/southbridge/ich9.c
 F: include/hw/acpi/ich9*.h
 F: include/hw/i2c/ich9_smbus.h
 F: include/hw/ide/ahci-pci.h
+F: include/hw/isa/ich9_lpc.h
 F: include/hw/pci-bridge/ich9_dmi.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 daf93361eb..1f41ab49c4 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -37,6 +37,7 @@
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/acpi/ich9_tco.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 e724494c7b..67a141efc4 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 63fec8b439..14df9e910b 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/ich9_lpc.c
similarity index 95%
rename from hw/isa/lpc_ich9.c
rename to hw/isa/ich9_lpc.c
index bd727b2320..17d4a95bd2 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/ich9_lpc.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
@@ -41,6 +41,7 @@
 #include "hw/isa/apm.h"
 #include "hw/pci/pci.h"
 #include "hw/southbridge/ich9.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/ich9.h"
 #include "hw/pci/pci_bus.h"
@@ -53,10 +54,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"
 
diff --git a/hw/isa/meson.build b/hw/isa/meson.build
index 3219282217..3b5504f78d 100644
--- a/hw/isa/meson.build
+++ b/hw/isa/meson.build
@@ -8,4 +8,4 @@ system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
 system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
 system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
 
-specific_ss.add(when: 'CONFIG_LPC_ICH9', if_true: files('lpc_ich9.c'))
+specific_ss.add(when: 'CONFIG_LPC_ICH9', if_true: files('ich9_lpc.c'))
-- 
2.41.0

[PATCH v2 15/15] 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              | 20 ++++++--------------
 hw/isa/ich9_lpc.c             |  3 +++
 hw/southbridge/ich9.c         | 15 +++++++++++++++
 hw/i386/Kconfig               |  1 -
 hw/southbridge/Kconfig        |  1 +
 6 files changed, 25 insertions(+), 19 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 14df9e910b..31ab0ae77b 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"
@@ -67,9 +66,7 @@ static void pc_q35_init(MachineState *machine)
     X86MachineState *x86ms = X86_MACHINE(machine);
     Object *phb;
     DeviceState *ich9;
-    PCIDevice *lpc;
     Object *lpc_obj;
-    DeviceState *lpc_dev;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     MemoryRegion *pci_memory = g_new(MemoryRegion, 1);
@@ -168,24 +165,19 @@ static void pc_q35_init(MachineState *machine)
     object_property_add_child(OBJECT(machine), "ich9", OBJECT(ich9));
     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
                              OBJECT(pcms->pcibus), &error_abort);
+    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+        qdev_connect_gpio_out_named(ich9, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
+    }
     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? */
     qdev_prop_set_uint8(ich9, "ehci-count", machine_usb(machine) ? 1 : 0);
     qdev_realize_and_unref(ich9, NULL, &error_fatal);
 
-    /* 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));
-    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-        qdev_connect_gpio_out_named(lpc_dev, ICH9_GPIO_GSI, i, x86ms->gsi[i]);
-    }
-    pci_realize_and_unref(lpc, pcms->pcibus, &error_fatal);
+    /* ISA bus */
+    lpc_obj = object_resolve_path_component(OBJECT(ich9), "lpc");
 
     x86ms->rtc = ISA_DEVICE(object_resolve_path_component(lpc_obj, "rtc"));
 
diff --git a/hw/isa/ich9_lpc.c b/hw/isa/ich9_lpc.c
index 17d4a95bd2..2339f66e0f 100644
--- a/hw/isa/ich9_lpc.c
+++ b/hw/isa/ich9_lpc.c
@@ -54,6 +54,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 f05012959d..521925b462 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/ich9_dmi.h"
+#include "hw/isa/ich9_lpc.h"
 #include "hw/ide/ahci-pci.h"
 #include "hw/ide/ide-dev.h"
 #include "hw/i2c/ich9_smbus.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];
@@ -57,6 +60,14 @@ static Property ich9_props[] = {
 
 static void ich9_init(Object *obj)
 {
+    ICH9State *s = ICH9_SOUTHBRIDGE(obj);
+
+    object_initialize_child(obj, "lpc", &s->lpc, TYPE_ICH9_LPC_DEVICE);
+    qdev_pass_gpios(DEVICE(&s->lpc), DEVICE(s), ICH9_GPIO_GSI);
+    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 bool ich9_realize_d2p(ICH9State *s, Error **errp)
@@ -163,6 +174,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

Re: [PATCH v2 00/15] hw/southbridge: Extract ICH9 QOM container model

[Philippe Mathieu-Daudé]

On 26/2/24 12:13, Philippe Mathieu-Daudé wrote:

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

> Philippe Mathieu-Daudé (15):
>    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/ide: Rename ich.c -> ich9_ahci.c
>    hw/i2c/smbus: Extract QOM ICH9 definitions to 'ich9_smbus.h'
>    hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.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/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

Branch available here:
https://gitlab.com/philmd/qemu/-/commits/ich9_qom-v2/

Re: [PATCH v2 00/15] hw/southbridge: Extract ICH9 QOM container model

[Bernhard Beschow]

Am 26. Februar 2024 11:13:59 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Since v1 [1]:
>- Rebased on top of Bernhard patches
>- Rename files with 'ich9_' prefix (Bernhard)
>
>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.

I really like the simplicity of the machine code and that the ICH9 southbridge becomes a proper device rather than being scattered around in machine code. I've made some reviews in form of a branch: https://github.com/shentok/qemu/commits/philmd/ich9_qom-v2/

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

Would be nice if the pattern could then also be applied to the VIA southbridges, otherwise this could break my via-apollo-pro-133t branch: https://github.com/shentok/qemu/tree/via-apollo-pro-133t

Best regards,
Bernhard

>
>Also we'd need to decouple the cpu_interrupt() calls between hw/
>and target/.
>
>Note that GSI is currently broken [2]. Once the LPC/ISA part is
>done, it might be easier to fix it.
>
>[1] https://lore.kernel.org/qemu-devel/20240219163855.87326-1-philmd@linaro.org/
>[2] https://lore.kernel.org/qemu-devel/cd0e13c6-c03d-411f-83a5-1d4d28ea4345@linaro.org/
>
>Philippe Mathieu-Daudé (15):
>  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/ide: Rename ich.c -> ich9_ahci.c
>  hw/i2c/smbus: Extract QOM ICH9 definitions to 'ich9_smbus.h'
>  hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.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/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/ich9_smbus.h               |  25 +++
> include/hw/isa/ich9_lpc.h                 | 166 +++++++++++++++
> include/hw/pci-bridge/ich9_dmi.h          |  20 ++
> include/hw/southbridge/ich9.h             | 235 +---------------------
> hw/acpi/ich9.c                            |   9 +-
> hw/acpi/ich9_tco.c                        |   5 +-
> hw/i2c/{smbus_ich9.c => ich9_smbus.c}     |  36 +++-
> hw/i386/acpi-build.c                      |   1 +
> hw/i386/pc_q35.c                          | 126 +++---------
> hw/ide/{ich.c => ich9_ahci.c}             |   0
> hw/isa/{lpc_ich9.c => ich9_lpc.c}         |  37 +++-
> hw/pci-bridge/{i82801b11.c => ich9_dmi.c} |  11 +-
> hw/southbridge/ich9.c                     | 213 ++++++++++++++++++++
> tests/qtest/tco-test.c                    |   2 +-
> hw/Kconfig                                |   1 +
> hw/i2c/meson.build                        |   2 +-
> hw/i386/Kconfig                           |   3 +-
> hw/ide/meson.build                        |   2 +-
> hw/isa/meson.build                        |   2 +-
> hw/meson.build                            |   1 +
> hw/pci-bridge/meson.build                 |   2 +-
> hw/southbridge/Kconfig                    |  11 +
> hw/southbridge/meson.build                |   3 +
> 26 files changed, 587 insertions(+), 368 deletions(-)
> create mode 100644 include/hw/i2c/ich9_smbus.h
> create mode 100644 include/hw/isa/ich9_lpc.h
> create mode 100644 include/hw/pci-bridge/ich9_dmi.h
> rename hw/i2c/{smbus_ich9.c => ich9_smbus.c} (77%)
> rename hw/ide/{ich.c => ich9_ahci.c} (100%)
> rename hw/isa/{lpc_ich9.c => ich9_lpc.c} (95%)
> rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)
> create mode 100644 hw/southbridge/ich9.c
> create mode 100644 hw/southbridge/Kconfig
> create mode 100644 hw/southbridge/meson.build
>

Re: [PATCH v2 00/15] hw/southbridge: Extract ICH9 QOM container model

[Michael S. Tsirkin]

On Mon, Feb 26, 2024 at 12:13:59PM +0100, Philippe Mathieu-Daudé wrote:
> Since v1 [1]:
> - Rebased on top of Bernhard patches
> - Rename files with 'ich9_' prefix (Bernhard)
> 
> 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.

Bernhard had some comments so I hope you'll address them so
I can merge - after the release presumably.


> Also we'd need to decouple the cpu_interrupt() calls between hw/
> and target/.
> 
> Note that GSI is currently broken [2]. Once the LPC/ISA part is
> done, it might be easier to fix it.
> 
> [1] https://lore.kernel.org/qemu-devel/20240219163855.87326-1-philmd@linaro.org/
> [2] https://lore.kernel.org/qemu-devel/cd0e13c6-c03d-411f-83a5-1d4d28ea4345@linaro.org/
> 
> Philippe Mathieu-Daudé (15):
>   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/ide: Rename ich.c -> ich9_ahci.c
>   hw/i2c/smbus: Extract QOM ICH9 definitions to 'ich9_smbus.h'
>   hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.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/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/ich9_smbus.h               |  25 +++
>  include/hw/isa/ich9_lpc.h                 | 166 +++++++++++++++
>  include/hw/pci-bridge/ich9_dmi.h          |  20 ++
>  include/hw/southbridge/ich9.h             | 235 +---------------------
>  hw/acpi/ich9.c                            |   9 +-
>  hw/acpi/ich9_tco.c                        |   5 +-
>  hw/i2c/{smbus_ich9.c => ich9_smbus.c}     |  36 +++-
>  hw/i386/acpi-build.c                      |   1 +
>  hw/i386/pc_q35.c                          | 126 +++---------
>  hw/ide/{ich.c => ich9_ahci.c}             |   0
>  hw/isa/{lpc_ich9.c => ich9_lpc.c}         |  37 +++-
>  hw/pci-bridge/{i82801b11.c => ich9_dmi.c} |  11 +-
>  hw/southbridge/ich9.c                     | 213 ++++++++++++++++++++
>  tests/qtest/tco-test.c                    |   2 +-
>  hw/Kconfig                                |   1 +
>  hw/i2c/meson.build                        |   2 +-
>  hw/i386/Kconfig                           |   3 +-
>  hw/ide/meson.build                        |   2 +-
>  hw/isa/meson.build                        |   2 +-
>  hw/meson.build                            |   1 +
>  hw/pci-bridge/meson.build                 |   2 +-
>  hw/southbridge/Kconfig                    |  11 +
>  hw/southbridge/meson.build                |   3 +
>  26 files changed, 587 insertions(+), 368 deletions(-)
>  create mode 100644 include/hw/i2c/ich9_smbus.h
>  create mode 100644 include/hw/isa/ich9_lpc.h
>  create mode 100644 include/hw/pci-bridge/ich9_dmi.h
>  rename hw/i2c/{smbus_ich9.c => ich9_smbus.c} (77%)
>  rename hw/ide/{ich.c => ich9_ahci.c} (100%)
>  rename hw/isa/{lpc_ich9.c => ich9_lpc.c} (95%)
>  rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)
>  create mode 100644 hw/southbridge/ich9.c
>  create mode 100644 hw/southbridge/Kconfig
>  create mode 100644 hw/southbridge/meson.build
> 
> -- 
> 2.41.0