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