[PATCH 0/9] hw/ide/ahci: Housekeeping

[PATCH 0/9] hw/ide/ahci: Housekeeping

[Philippe Mathieu-Daudé]

- Split 'ahci.h' as sysbus / pci
- Inline ahci_get_num_ports()
- Directly use AHCIState::ports instead of SysbusAHCIState::num_ports

Philippe Mathieu-Daudé (9):
  hw/i386/q35: Use DEVICE() cast macro with PCIDevice object
  hw/ide/ahci: Expose AHCIPCIState structure
  hw/ide/ahci: Rename AHCI PCI function as 'pdev'
  hw/ide/ahci: Inline ahci_get_num_ports()
  hw/ide/ahci: Pass AHCI context to ahci_ide_create_devs()
  hw/ide/ahci: Convert AHCIState::ports to unsigned
  hw/ide/ahci: Do not pass 'ports' argument to ahci_realize()
  hw/ide/ahci: Remove SysbusAHCIState::num_ports field
  hw/ide/ahci: Move SysBus definitions to 'ahci-sysbus.h'

 hw/ide/ahci_internal.h         | 10 +--------
 include/hw/arm/allwinner-a10.h |  2 +-
 include/hw/arm/allwinner-r40.h |  2 +-
 include/hw/arm/xlnx-zynqmp.h   |  2 +-
 include/hw/ide/ahci-pci.h      | 22 ++++++++++++++++++++
 include/hw/ide/ahci-sysbus.h   | 35 +++++++++++++++++++++++++++++++
 include/hw/ide/ahci.h          | 38 +++-------------------------------
 hw/arm/highbank.c              |  2 +-
 hw/arm/sbsa-ref.c              |  1 +
 hw/i386/pc_q35.c               | 19 ++++++++++-------
 hw/ide/ahci-allwinner.c        |  3 +--
 hw/ide/ahci.c                  | 29 +++++++++-----------------
 hw/ide/ich.c                   |  4 +++-
 hw/mips/boston.c               | 14 +++++++------
 14 files changed, 99 insertions(+), 84 deletions(-)
 create mode 100644 include/hw/ide/ahci-pci.h
 create mode 100644 include/hw/ide/ahci-sysbus.h

-- 
2.41.0

[PATCH 1/9] hw/i386/q35: Use DEVICE() cast macro with PCIDevice object

[Philippe Mathieu-Daudé]

QDev API provides the DEVICE() macro to access the
'qdev' parent field of the PCIDevice structure.

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

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ca3f465e0..33a4413708 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -309,8 +309,8 @@ static void pc_q35_init(MachineState *machine)
                                                PCI_DEVFN(ICH9_SATA1_DEV,
                                                          ICH9_SATA1_FUNC),
                                                "ich9-ahci");
-        idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
-        idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
+        idebus[0] = qdev_get_child_bus(DEVICE(ahci), "ide.0");
+        idebus[1] = qdev_get_child_bus(DEVICE(ahci), "ide.1");
         g_assert(MAX_SATA_PORTS == ahci_get_num_ports(ahci));
         ide_drive_get(hd, ahci_get_num_ports(ahci));
         ahci_ide_create_devs(ahci, hd);
-- 
2.41.0

[PATCH 2/9] hw/ide/ahci: Expose AHCIPCIState structure

[Philippe Mathieu-Daudé]

In order to be able to QOM-embed a structure, we need
its full definition. Move it from "ahci_internal.h"
to the new "hw/ide/ahci-pci.h" header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ahci_internal.h    |  8 --------
 include/hw/ide/ahci-pci.h | 22 ++++++++++++++++++++++
 include/hw/ide/ahci.h     |  3 ---
 hw/i386/pc_q35.c          |  2 +-
 hw/ide/ahci.c             |  1 +
 hw/ide/ich.c              |  1 +
 hw/mips/boston.c          |  2 +-
 7 files changed, 26 insertions(+), 13 deletions(-)
 create mode 100644 include/hw/ide/ahci-pci.h

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index c244bbd8be..4dc2805d21 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -324,14 +324,6 @@ struct AHCIDevice {
     MemReentrancyGuard mem_reentrancy_guard;
 };
 
-struct AHCIPCIState {
-    /*< private >*/
-    PCIDevice parent_obj;
-    /*< public >*/
-
-    AHCIState ahci;
-};
-
 extern const VMStateDescription vmstate_ahci;
 
 #define VMSTATE_AHCI(_field, _state) {                               \
diff --git a/include/hw/ide/ahci-pci.h b/include/hw/ide/ahci-pci.h
new file mode 100644
index 0000000000..c2ee616962
--- /dev/null
+++ b/include/hw/ide/ahci-pci.h
@@ -0,0 +1,22 @@
+/*
+ * QEMU AHCI Emulation (PCI devices)
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_IDE_AHCI_PCI_H
+#define HW_IDE_AHCI_PCI_H
+
+#include "qom/object.h"
+#include "hw/ide/ahci.h"
+#include "hw/pci/pci_device.h"
+
+#define TYPE_ICH9_AHCI "ich9-ahci"
+OBJECT_DECLARE_SIMPLE_TYPE(AHCIPCIState, ICH9_AHCI)
+
+struct AHCIPCIState {
+    PCIDevice parent_obj;
+
+    AHCIState ahci;
+};
+
+#endif
diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 210e5e734c..6818d02063 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -52,9 +52,6 @@ typedef struct AHCIState {
 } AHCIState;
 
 
-#define TYPE_ICH9_AHCI "ich9-ahci"
-OBJECT_DECLARE_SIMPLE_TYPE(AHCIPCIState, ICH9_AHCI)
-
 int32_t ahci_get_num_ports(PCIDevice *dev);
 void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 33a4413708..ace8d3839a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -47,7 +47,7 @@
 #include "hw/display/ramfb.h"
 #include "hw/firmware/smbios.h"
 #include "hw/ide/pci.h"
-#include "hw/ide/ahci.h"
+#include "hw/ide/ahci-pci.h"
 #include "hw/intc/ioapic.h"
 #include "hw/southbridge/ich9.h"
 #include "hw/usb.h"
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0eb83a6d46..aa9381a7b2 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -36,6 +36,7 @@
 #include "sysemu/dma.h"
 #include "hw/ide/internal.h"
 #include "hw/ide/pci.h"
+#include "hw/ide/ahci-pci.h"
 #include "ahci_internal.h"
 
 #include "trace.h"
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 49f8eb8a7d..d190012a95 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -69,6 +69,7 @@
 #include "hw/isa/isa.h"
 #include "sysemu/dma.h"
 #include "hw/ide/pci.h"
+#include "hw/ide/ahci-pci.h"
 #include "ahci_internal.h"
 
 #define ICH9_MSI_CAP_OFFSET     0x80
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 4e11ff6cd6..cbcefdd693 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -24,7 +24,7 @@
 #include "hw/boards.h"
 #include "hw/char/serial.h"
 #include "hw/ide/pci.h"
-#include "hw/ide/ahci.h"
+#include "hw/ide/ahci-pci.h"
 #include "hw/loader.h"
 #include "hw/loader-fit.h"
 #include "hw/mips/bootloader.h"
-- 
2.41.0

[PATCH 3/9] hw/ide/ahci: Rename AHCI PCI function as 'pdev'

[Philippe Mathieu-Daudé]

We want to access AHCIPCIState::ahci field. In order to keep
the code simple (avoiding &ahci->ahci), rename the current
'ahci' variable as 'pdev'

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_q35.c | 15 ++++++++-------
 hw/mips/boston.c | 10 +++++-----
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ace8d3839a..e298f4ff32 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -135,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
     GSIState *gsi_state;
     ISABus *isa_bus;
     int i;
-    PCIDevice *ahci;
     ram_addr_t lowmem;
     DriveInfo *hd[MAX_SATA_PORTS];
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -304,16 +303,18 @@ static void pc_q35_init(MachineState *machine)
                          0xff0104);
 
     if (pcms->sata_enabled) {
+        PCIDevice *pdev;
+
         /* ahci and SATA device, for q35 1 ahci controller is built-in */
-        ahci = pci_create_simple_multifunction(host_bus,
+        pdev = pci_create_simple_multifunction(host_bus,
                                                PCI_DEVFN(ICH9_SATA1_DEV,
                                                          ICH9_SATA1_FUNC),
                                                "ich9-ahci");
-        idebus[0] = qdev_get_child_bus(DEVICE(ahci), "ide.0");
-        idebus[1] = qdev_get_child_bus(DEVICE(ahci), "ide.1");
-        g_assert(MAX_SATA_PORTS == ahci_get_num_ports(ahci));
-        ide_drive_get(hd, ahci_get_num_ports(ahci));
-        ahci_ide_create_devs(ahci, hd);
+        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
+        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
+        g_assert(MAX_SATA_PORTS == ahci_get_num_ports(pdev));
+        ide_drive_get(hd, ahci_get_num_ports(pdev));
+        ahci_ide_create_devs(pdev, hd);
     } else {
         idebus[0] = idebus[1] = NULL;
     }
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index cbcefdd693..0ec0b98066 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -677,7 +677,7 @@ static void boston_mach_init(MachineState *machine)
     MemoryRegion *flash, *ddr_low_alias, *lcd, *platreg;
     MemoryRegion *sys_mem = get_system_memory();
     XilinxPCIEHost *pcie2;
-    PCIDevice *ahci;
+    PCIDevice *pdev;
     DriveInfo *hd[6];
     Chardev *chr;
     int fw_size, fit_err;
@@ -769,11 +769,11 @@ static void boston_mach_init(MachineState *machine)
     qemu_chr_fe_set_handlers(&s->lcd_display, NULL, NULL,
                              boston_lcd_event, NULL, s, NULL, true);
 
-    ahci = pci_create_simple_multifunction(&PCI_BRIDGE(&pcie2->root)->sec_bus,
+    pdev = pci_create_simple_multifunction(&PCI_BRIDGE(&pcie2->root)->sec_bus,
                                            PCI_DEVFN(0, 0), TYPE_ICH9_AHCI);
-    g_assert(ARRAY_SIZE(hd) == ahci_get_num_ports(ahci));
-    ide_drive_get(hd, ahci_get_num_ports(ahci));
-    ahci_ide_create_devs(ahci, hd);
+    g_assert(ARRAY_SIZE(hd) == ahci_get_num_ports(pdev));
+    ide_drive_get(hd, ahci_get_num_ports(pdev));
+    ahci_ide_create_devs(pdev, hd);
 
     if (machine->firmware) {
         fw_size = load_image_targphys(machine->firmware,
-- 
2.41.0

[PATCH 4/9] hw/ide/ahci: Inline ahci_get_num_ports()

[Philippe Mathieu-Daudé]

Introduce the 'ich9' variable and inline ahci_get_num_ports().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ide/ahci.h | 1 -
 hw/i386/pc_q35.c      | 6 ++++--
 hw/ide/ahci.c         | 8 --------
 hw/mips/boston.c      | 6 ++++--
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 6818d02063..dbef377f3d 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -52,7 +52,6 @@ typedef struct AHCIState {
 } AHCIState;
 
 
-int32_t ahci_get_num_ports(PCIDevice *dev);
 void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd);
 
 #define TYPE_SYSBUS_AHCI "sysbus-ahci"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e298f4ff32..c50e3bfc42 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -304,16 +304,18 @@ static void pc_q35_init(MachineState *machine)
 
     if (pcms->sata_enabled) {
         PCIDevice *pdev;
+        AHCIPCIState *ich9;
 
         /* ahci and SATA device, for q35 1 ahci controller is built-in */
         pdev = pci_create_simple_multifunction(host_bus,
                                                PCI_DEVFN(ICH9_SATA1_DEV,
                                                          ICH9_SATA1_FUNC),
                                                "ich9-ahci");
+        ich9 = ICH9_AHCI(pdev);
         idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
         idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
-        g_assert(MAX_SATA_PORTS == ahci_get_num_ports(pdev));
-        ide_drive_get(hd, ahci_get_num_ports(pdev));
+        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
+        ide_drive_get(hd, ich9->ahci.ports);
         ahci_ide_create_devs(pdev, hd);
     } else {
         idebus[0] = idebus[1] = NULL;
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index aa9381a7b2..8b97c6b0e7 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1896,14 +1896,6 @@ static void sysbus_ahci_register_types(void)
 
 type_init(sysbus_ahci_register_types)
 
-int32_t ahci_get_num_ports(PCIDevice *dev)
-{
-    AHCIPCIState *d = ICH9_AHCI(dev);
-    AHCIState *ahci = &d->ahci;
-
-    return ahci->ports;
-}
-
 void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd)
 {
     AHCIPCIState *d = ICH9_AHCI(dev);
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 0ec0b98066..a6c7bc18ff 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -678,6 +678,7 @@ static void boston_mach_init(MachineState *machine)
     MemoryRegion *sys_mem = get_system_memory();
     XilinxPCIEHost *pcie2;
     PCIDevice *pdev;
+    AHCIPCIState *ich9;
     DriveInfo *hd[6];
     Chardev *chr;
     int fw_size, fit_err;
@@ -771,8 +772,9 @@ static void boston_mach_init(MachineState *machine)
 
     pdev = pci_create_simple_multifunction(&PCI_BRIDGE(&pcie2->root)->sec_bus,
                                            PCI_DEVFN(0, 0), TYPE_ICH9_AHCI);
-    g_assert(ARRAY_SIZE(hd) == ahci_get_num_ports(pdev));
-    ide_drive_get(hd, ahci_get_num_ports(pdev));
+    ich9 = ICH9_AHCI(pdev);
+    g_assert(ARRAY_SIZE(hd) == ich9->ahci.ports);
+    ide_drive_get(hd, ich9->ahci.ports);
     ahci_ide_create_devs(pdev, hd);
 
     if (machine->firmware) {
-- 
2.41.0

[PATCH 5/9] hw/ide/ahci: Pass AHCI context to ahci_ide_create_devs()

[Philippe Mathieu-Daudé]

Since ahci_ide_create_devs() is not PCI specific, pass
it an AHCIState argument instead of PCIDevice.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ide/ahci.h | 2 +-
 hw/i386/pc_q35.c      | 2 +-
 hw/ide/ahci.c         | 5 +----
 hw/mips/boston.c      | 2 +-
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index dbef377f3d..8cd55b1333 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -52,7 +52,7 @@ typedef struct AHCIState {
 } AHCIState;
 
 
-void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd);
+void ahci_ide_create_devs(AHCIState *ahci, DriveInfo **hd);
 
 #define TYPE_SYSBUS_AHCI "sysbus-ahci"
 OBJECT_DECLARE_SIMPLE_TYPE(SysbusAHCIState, SYSBUS_AHCI)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c50e3bfc42..7f4f51fcdf 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -316,7 +316,7 @@ static void pc_q35_init(MachineState *machine)
         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(pdev, hd);
+        ahci_ide_create_devs(&ich9->ahci, hd);
     } else {
         idebus[0] = idebus[1] = NULL;
     }
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8b97c6b0e7..bac1871a31 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1896,10 +1896,8 @@ static void sysbus_ahci_register_types(void)
 
 type_init(sysbus_ahci_register_types)
 
-void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd)
+void ahci_ide_create_devs(AHCIState *ahci, DriveInfo **hd)
 {
-    AHCIPCIState *d = ICH9_AHCI(dev);
-    AHCIState *ahci = &d->ahci;
     int i;
 
     for (i = 0; i < ahci->ports; i++) {
@@ -1908,5 +1906,4 @@ void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd)
         }
         ide_bus_create_drive(&ahci->dev[i].port, 0, hd[i]);
     }
-
 }
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index a6c7bc18ff..1b44fb354c 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -775,7 +775,7 @@ static void boston_mach_init(MachineState *machine)
     ich9 = ICH9_AHCI(pdev);
     g_assert(ARRAY_SIZE(hd) == ich9->ahci.ports);
     ide_drive_get(hd, ich9->ahci.ports);
-    ahci_ide_create_devs(pdev, hd);
+    ahci_ide_create_devs(&ich9->ahci, hd);
 
     if (machine->firmware) {
         fw_size = load_image_targphys(machine->firmware,
-- 
2.41.0

[PATCH 6/9] hw/ide/ahci: Convert AHCIState::ports to unsigned

[Philippe Mathieu-Daudé]

AHCIState::ports should be unsigned. Besides, we never
check it for negative value. It is unlikely it was ever
used with more than INT32_MAX ports, so it is safe to
convert it to unsigned.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ide/ahci.h | 2 +-
 hw/ide/ahci.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 8cd55b1333..604d3a0994 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -46,7 +46,7 @@ typedef struct AHCIState {
     MemoryRegion idp;       /* Index-Data Pair I/O port space */
     unsigned idp_offset;    /* Offset of index in I/O port space */
     uint32_t idp_index;     /* Current IDP index */
-    int32_t ports;
+    uint32_t ports;
     qemu_irq irq;
     AddressSpace *as;
 } AHCIState;
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bac1871a31..2c3306dae4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1819,7 +1819,7 @@ const VMStateDescription vmstate_ahci = {
     .version_id = 1,
     .post_load = ahci_state_post_load,
     .fields = (const VMStateField[]) {
-        VMSTATE_STRUCT_VARRAY_POINTER_INT32(dev, AHCIState, ports,
+        VMSTATE_STRUCT_VARRAY_POINTER_UINT32(dev, AHCIState, ports,
                                      vmstate_ahci_device, AHCIDevice),
         VMSTATE_UINT32(control_regs.cap, AHCIState),
         VMSTATE_UINT32(control_regs.ghc, AHCIState),
@@ -1827,7 +1827,7 @@ const VMStateDescription vmstate_ahci = {
         VMSTATE_UINT32(control_regs.impl, AHCIState),
         VMSTATE_UINT32(control_regs.version, AHCIState),
         VMSTATE_UINT32(idp_index, AHCIState),
-        VMSTATE_INT32_EQUAL(ports, AHCIState, NULL),
+        VMSTATE_UINT32_EQUAL(ports, AHCIState, NULL),
         VMSTATE_END_OF_LIST()
     },
 };
-- 
2.41.0

[PATCH 7/9] hw/ide/ahci: Do not pass 'ports' argument to ahci_realize()

[Philippe Mathieu-Daudé]

Explicitly set AHCIState::ports before calling ahci_realize().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ahci_internal.h | 2 +-
 hw/ide/ahci.c          | 9 +++++----
 hw/ide/ich.c           | 3 ++-
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 4dc2805d21..4e13329bb2 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -377,7 +377,7 @@ typedef struct SDBFIS {
     uint32_t payload;
 } QEMU_PACKED SDBFIS;
 
-void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports);
+void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as);
 void ahci_init(AHCIState *s, DeviceState *qdev);
 void ahci_uninit(AHCIState *s);
 
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 2c3306dae4..33f7e83687 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1614,14 +1614,14 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
                           "ahci-idp", 32);
 }
 
-void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
+void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as)
 {
     qemu_irq *irqs;
     int i;
 
     s->as = as;
-    s->ports = ports;
-    s->dev = g_new0(AHCIDevice, ports);
+    assert(s->ports > 0);
+    s->dev = g_new0(AHCIDevice, s->ports);
     ahci_reg_init(s);
     irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
     for (i = 0; i < s->ports; i++) {
@@ -1862,7 +1862,8 @@ static void sysbus_ahci_realize(DeviceState *dev, Error **errp)
 {
     SysbusAHCIState *s = SYSBUS_AHCI(dev);
 
-    ahci_realize(&s->ahci, dev, &address_space_memory, s->num_ports);
+    s->ahci.ports = s->num_ports;
+    ahci_realize(&s->ahci, dev, &address_space_memory);
 }
 
 static Property sysbus_ahci_properties[] = {
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index d190012a95..122fc7e0ab 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -113,7 +113,8 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp)
     d = ICH9_AHCI(dev);
     int ret;
 
-    ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev), 6);
+    d->ahci.ports = 6;
+    ahci_realize(&d->ahci, DEVICE(dev), pci_get_address_space(dev));
 
     pci_config_set_prog_interface(dev->config, AHCI_PROGMODE_MAJOR_REV_1);
 
-- 
2.41.0

[PATCH 8/9] hw/ide/ahci: Remove SysbusAHCIState::num_ports field

[Philippe Mathieu-Daudé]

No need to duplicate AHCIState::ports, directly access it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ide/ahci.h | 1 -
 hw/ide/ahci.c         | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 604d3a0994..c0b10c2bb4 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -63,7 +63,6 @@ struct SysbusAHCIState {
     /*< public >*/
 
     AHCIState ahci;
-    uint32_t num_ports;
 };
 
 #define TYPE_ALLWINNER_AHCI "allwinner-ahci"
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 33f7e83687..041cc87c11 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1862,12 +1862,11 @@ static void sysbus_ahci_realize(DeviceState *dev, Error **errp)
 {
     SysbusAHCIState *s = SYSBUS_AHCI(dev);
 
-    s->ahci.ports = s->num_ports;
     ahci_realize(&s->ahci, dev, &address_space_memory);
 }
 
 static Property sysbus_ahci_properties[] = {
-    DEFINE_PROP_UINT32("num-ports", SysbusAHCIState, num_ports, 1),
+    DEFINE_PROP_UINT32("num-ports", SysbusAHCIState, ahci.ports, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.41.0

[PATCH 9/9] hw/ide/ahci: Move SysBus definitions to 'ahci-sysbus.h'

[Philippe Mathieu-Daudé]

Keep "hw/ide/ahci.h" AHCI-generic.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/arm/allwinner-a10.h |  2 +-
 include/hw/arm/allwinner-r40.h |  2 +-
 include/hw/arm/xlnx-zynqmp.h   |  2 +-
 include/hw/ide/ahci-sysbus.h   | 35 ++++++++++++++++++++++++++++++++++
 include/hw/ide/ahci.h          | 29 +---------------------------
 hw/arm/highbank.c              |  2 +-
 hw/arm/sbsa-ref.c              |  1 +
 hw/ide/ahci-allwinner.c        |  3 +--
 hw/ide/ahci.c                  |  1 +
 9 files changed, 43 insertions(+), 34 deletions(-)
 create mode 100644 include/hw/ide/ahci-sysbus.h

diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
index 2eb83a17ea..67a9a17b86 100644
--- a/include/hw/arm/allwinner-a10.h
+++ b/include/hw/arm/allwinner-a10.h
@@ -5,7 +5,7 @@
 #include "hw/intc/allwinner-a10-pic.h"
 #include "hw/net/allwinner_emac.h"
 #include "hw/sd/allwinner-sdhost.h"
-#include "hw/ide/ahci.h"
+#include "hw/ide/ahci-sysbus.h"
 #include "hw/usb/hcd-ohci.h"
 #include "hw/usb/hcd-ehci.h"
 #include "hw/rtc/allwinner-rtc.h"
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index 66c38e7d90..614e74b7ed 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -22,7 +22,7 @@
 
 #include "qom/object.h"
 #include "hw/timer/allwinner-a10-pit.h"
-#include "hw/ide/ahci.h"
+#include "hw/ide/ahci-sysbus.h"
 #include "hw/intc/arm_gic.h"
 #include "hw/sd/allwinner-sdhost.h"
 #include "hw/misc/allwinner-r40-ccu.h"
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index 96358d51eb..48f7948092 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -22,7 +22,7 @@
 #include "hw/net/cadence_gem.h"
 #include "hw/char/cadence_uart.h"
 #include "hw/net/xlnx-zynqmp-can.h"
-#include "hw/ide/ahci.h"
+#include "hw/ide/ahci-sysbus.h"
 #include "hw/sd/sdhci.h"
 #include "hw/ssi/xilinx_spips.h"
 #include "hw/dma/xlnx_dpdma.h"
diff --git a/include/hw/ide/ahci-sysbus.h b/include/hw/ide/ahci-sysbus.h
new file mode 100644
index 0000000000..7ed6cad496
--- /dev/null
+++ b/include/hw/ide/ahci-sysbus.h
@@ -0,0 +1,35 @@
+/*
+ * QEMU AHCI Emulation (MMIO-mapped devices)
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_IDE_AHCI_SYSBUS_H
+#define HW_IDE_AHCI_SYSBUS_H
+
+#include "qom/object.h"
+#include "hw/sysbus.h"
+#include "hw/ide/ahci.h"
+
+#define TYPE_SYSBUS_AHCI "sysbus-ahci"
+OBJECT_DECLARE_SIMPLE_TYPE(SysbusAHCIState, SYSBUS_AHCI)
+
+struct SysbusAHCIState {
+    SysBusDevice parent_obj;
+
+    AHCIState ahci;
+};
+
+#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
+OBJECT_DECLARE_SIMPLE_TYPE(AllwinnerAHCIState, ALLWINNER_AHCI)
+
+#define ALLWINNER_AHCI_MMIO_OFF  0x80
+#define ALLWINNER_AHCI_MMIO_SIZE 0x80
+
+struct AllwinnerAHCIState {
+    SysbusAHCIState parent_obj;
+
+    MemoryRegion mmio;
+    uint32_t regs[ALLWINNER_AHCI_MMIO_SIZE/4];
+};
+
+#endif
diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index c0b10c2bb4..ba31e75ff9 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -24,8 +24,7 @@
 #ifndef HW_IDE_AHCI_H
 #define HW_IDE_AHCI_H
 
-#include "hw/sysbus.h"
-#include "qom/object.h"
+#include "exec/memory.h"
 
 typedef struct AHCIDevice AHCIDevice;
 
@@ -54,30 +53,4 @@ typedef struct AHCIState {
 
 void ahci_ide_create_devs(AHCIState *ahci, DriveInfo **hd);
 
-#define TYPE_SYSBUS_AHCI "sysbus-ahci"
-OBJECT_DECLARE_SIMPLE_TYPE(SysbusAHCIState, SYSBUS_AHCI)
-
-struct SysbusAHCIState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    AHCIState ahci;
-};
-
-#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
-OBJECT_DECLARE_SIMPLE_TYPE(AllwinnerAHCIState, ALLWINNER_AHCI)
-
-#define ALLWINNER_AHCI_MMIO_OFF  0x80
-#define ALLWINNER_AHCI_MMIO_SIZE 0x80
-
-struct AllwinnerAHCIState {
-    /*< private >*/
-    SysbusAHCIState parent_obj;
-    /*< public >*/
-
-    MemoryRegion mmio;
-    uint32_t regs[ALLWINNER_AHCI_MMIO_SIZE/4];
-};
-
 #endif /* HW_IDE_AHCI_H */
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 9fdac1cc81..c71b1a8db3 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -30,7 +30,7 @@
 #include "hw/boards.h"
 #include "qemu/error-report.h"
 #include "hw/char/pl011.h"
-#include "hw/ide/ahci.h"
+#include "hw/ide/ahci-sysbus.h"
 #include "hw/cpu/a9mpcore.h"
 #include "hw/cpu/a15mpcore.h"
 #include "qemu/log.h"
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f2adf30337..5d3a574664 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -38,6 +38,7 @@
 #include "hw/boards.h"
 #include "hw/ide/internal.h"
 #include "hw/ide/ahci_internal.h"
+#include "hw/ide/ahci-sysbus.h"
 #include "hw/intc/arm_gicv3_common.h"
 #include "hw/intc/arm_gicv3_its_common.h"
 #include "hw/loader.h"
diff --git a/hw/ide/ahci-allwinner.c b/hw/ide/ahci-allwinner.c
index b173121006..9620de8ce8 100644
--- a/hw/ide/ahci-allwinner.c
+++ b/hw/ide/ahci-allwinner.c
@@ -19,9 +19,8 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "sysemu/dma.h"
-#include "hw/ide/internal.h"
 #include "migration/vmstate.h"
-#include "ahci_internal.h"
+#include "hw/ide/ahci-sysbus.h"
 
 #include "trace.h"
 
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 041cc87c11..54c9685495 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -37,6 +37,7 @@
 #include "hw/ide/internal.h"
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci-pci.h"
+#include "hw/ide/ahci-sysbus.h"
 #include "ahci_internal.h"
 
 #include "trace.h"
-- 
2.41.0

Re: [PATCH 9/9] hw/ide/ahci: Move SysBus definitions to 'ahci-sysbus.h'

[Leif Lindholm]

On 2024-02-13 08:12, Philippe Mathieu-Daudé wrote:
> Keep "hw/ide/ahci.h" AHCI-generic.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/arm/allwinner-a10.h |  2 +-
>   include/hw/arm/allwinner-r40.h |  2 +-
>   include/hw/arm/xlnx-zynqmp.h   |  2 +-
>   include/hw/ide/ahci-sysbus.h   | 35 ++++++++++++++++++++++++++++++++++
>   include/hw/ide/ahci.h          | 29 +---------------------------
>   hw/arm/highbank.c              |  2 +-
>   hw/arm/sbsa-ref.c              |  1 +

Obviously, if the actual functional change happens, I'd be happy for 
sbsa-ref to keep building, so for that aspect:
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

>   hw/ide/ahci-allwinner.c        |  3 +--
>   hw/ide/ahci.c                  |  1 +
>   9 files changed, 43 insertions(+), 34 deletions(-)
>   create mode 100644 include/hw/ide/ahci-sysbus.h
> 
> diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
> index 2eb83a17ea..67a9a17b86 100644
> --- a/include/hw/arm/allwinner-a10.h
> +++ b/include/hw/arm/allwinner-a10.h
> @@ -5,7 +5,7 @@
>   #include "hw/intc/allwinner-a10-pic.h"
>   #include "hw/net/allwinner_emac.h"
>   #include "hw/sd/allwinner-sdhost.h"
> -#include "hw/ide/ahci.h"
> +#include "hw/ide/ahci-sysbus.h"
>   #include "hw/usb/hcd-ohci.h"
>   #include "hw/usb/hcd-ehci.h"
>   #include "hw/rtc/allwinner-rtc.h"
> diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
> index 66c38e7d90..614e74b7ed 100644
> --- a/include/hw/arm/allwinner-r40.h
> +++ b/include/hw/arm/allwinner-r40.h
> @@ -22,7 +22,7 @@
>   
>   #include "qom/object.h"
>   #include "hw/timer/allwinner-a10-pit.h"
> -#include "hw/ide/ahci.h"
> +#include "hw/ide/ahci-sysbus.h"
>   #include "hw/intc/arm_gic.h"
>   #include "hw/sd/allwinner-sdhost.h"
>   #include "hw/misc/allwinner-r40-ccu.h"
> diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
> index 96358d51eb..48f7948092 100644
> --- a/include/hw/arm/xlnx-zynqmp.h
> +++ b/include/hw/arm/xlnx-zynqmp.h
> @@ -22,7 +22,7 @@
>   #include "hw/net/cadence_gem.h"
>   #include "hw/char/cadence_uart.h"
>   #include "hw/net/xlnx-zynqmp-can.h"
> -#include "hw/ide/ahci.h"
> +#include "hw/ide/ahci-sysbus.h"
>   #include "hw/sd/sdhci.h"
>   #include "hw/ssi/xilinx_spips.h"
>   #include "hw/dma/xlnx_dpdma.h"
> diff --git a/include/hw/ide/ahci-sysbus.h b/include/hw/ide/ahci-sysbus.h
> new file mode 100644
> index 0000000000..7ed6cad496
> --- /dev/null
> +++ b/include/hw/ide/ahci-sysbus.h
> @@ -0,0 +1,35 @@
> +/*
> + * QEMU AHCI Emulation (MMIO-mapped devices)
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef HW_IDE_AHCI_SYSBUS_H
> +#define HW_IDE_AHCI_SYSBUS_H
> +
> +#include "qom/object.h"
> +#include "hw/sysbus.h"
> +#include "hw/ide/ahci.h"
> +
> +#define TYPE_SYSBUS_AHCI "sysbus-ahci"
> +OBJECT_DECLARE_SIMPLE_TYPE(SysbusAHCIState, SYSBUS_AHCI)
> +
> +struct SysbusAHCIState {
> +    SysBusDevice parent_obj;
> +
> +    AHCIState ahci;
> +};
> +
> +#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
> +OBJECT_DECLARE_SIMPLE_TYPE(AllwinnerAHCIState, ALLWINNER_AHCI)
> +
> +#define ALLWINNER_AHCI_MMIO_OFF  0x80
> +#define ALLWINNER_AHCI_MMIO_SIZE 0x80
> +
> +struct AllwinnerAHCIState {
> +    SysbusAHCIState parent_obj;
> +
> +    MemoryRegion mmio;
> +    uint32_t regs[ALLWINNER_AHCI_MMIO_SIZE/4];
> +};
> +
> +#endif
> diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
> index c0b10c2bb4..ba31e75ff9 100644
> --- a/include/hw/ide/ahci.h
> +++ b/include/hw/ide/ahci.h
> @@ -24,8 +24,7 @@
>   #ifndef HW_IDE_AHCI_H
>   #define HW_IDE_AHCI_H
>   
> -#include "hw/sysbus.h"
> -#include "qom/object.h"
> +#include "exec/memory.h"
>   
>   typedef struct AHCIDevice AHCIDevice;
>   
> @@ -54,30 +53,4 @@ typedef struct AHCIState {
>   
>   void ahci_ide_create_devs(AHCIState *ahci, DriveInfo **hd);
>   
> -#define TYPE_SYSBUS_AHCI "sysbus-ahci"
> -OBJECT_DECLARE_SIMPLE_TYPE(SysbusAHCIState, SYSBUS_AHCI)
> -
> -struct SysbusAHCIState {
> -    /*< private >*/
> -    SysBusDevice parent_obj;
> -    /*< public >*/
> -
> -    AHCIState ahci;
> -};
> -
> -#define TYPE_ALLWINNER_AHCI "allwinner-ahci"
> -OBJECT_DECLARE_SIMPLE_TYPE(AllwinnerAHCIState, ALLWINNER_AHCI)
> -
> -#define ALLWINNER_AHCI_MMIO_OFF  0x80
> -#define ALLWINNER_AHCI_MMIO_SIZE 0x80
> -
> -struct AllwinnerAHCIState {
> -    /*< private >*/
> -    SysbusAHCIState parent_obj;
> -    /*< public >*/
> -
> -    MemoryRegion mmio;
> -    uint32_t regs[ALLWINNER_AHCI_MMIO_SIZE/4];
> -};
> -
>   #endif /* HW_IDE_AHCI_H */
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 9fdac1cc81..c71b1a8db3 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -30,7 +30,7 @@
>   #include "hw/boards.h"
>   #include "qemu/error-report.h"
>   #include "hw/char/pl011.h"
> -#include "hw/ide/ahci.h"
> +#include "hw/ide/ahci-sysbus.h"
>   #include "hw/cpu/a9mpcore.h"
>   #include "hw/cpu/a15mpcore.h"
>   #include "qemu/log.h"
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index f2adf30337..5d3a574664 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -38,6 +38,7 @@
>   #include "hw/boards.h"
>   #include "hw/ide/internal.h"
>   #include "hw/ide/ahci_internal.h"
> +#include "hw/ide/ahci-sysbus.h"
>   #include "hw/intc/arm_gicv3_common.h"
>   #include "hw/intc/arm_gicv3_its_common.h"
>   #include "hw/loader.h"
> diff --git a/hw/ide/ahci-allwinner.c b/hw/ide/ahci-allwinner.c
> index b173121006..9620de8ce8 100644
> --- a/hw/ide/ahci-allwinner.c
> +++ b/hw/ide/ahci-allwinner.c
> @@ -19,9 +19,8 @@
>   #include "qemu/error-report.h"
>   #include "qemu/module.h"
>   #include "sysemu/dma.h"
> -#include "hw/ide/internal.h"
>   #include "migration/vmstate.h"
> -#include "ahci_internal.h"
> +#include "hw/ide/ahci-sysbus.h"
>   
>   #include "trace.h"
>   
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 041cc87c11..54c9685495 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -37,6 +37,7 @@
>   #include "hw/ide/internal.h"
>   #include "hw/ide/pci.h"
>   #include "hw/ide/ahci-pci.h"
> +#include "hw/ide/ahci-sysbus.h"
>   #include "ahci_internal.h"
>   
>   #include "trace.h"

Re: [PATCH 0/9] hw/ide/ahci: Housekeeping

[Michael S. Tsirkin]

On Tue, Feb 13, 2024 at 09:11:51AM +0100, Philippe Mathieu-Daudé wrote:
> - Split 'ahci.h' as sysbus / pci
> - Inline ahci_get_num_ports()
> - Directly use AHCIState::ports instead of SysbusAHCIState::num_ports


PC part:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge with rest of patches.


> Philippe Mathieu-Daudé (9):
>   hw/i386/q35: Use DEVICE() cast macro with PCIDevice object
>   hw/ide/ahci: Expose AHCIPCIState structure
>   hw/ide/ahci: Rename AHCI PCI function as 'pdev'
>   hw/ide/ahci: Inline ahci_get_num_ports()
>   hw/ide/ahci: Pass AHCI context to ahci_ide_create_devs()
>   hw/ide/ahci: Convert AHCIState::ports to unsigned
>   hw/ide/ahci: Do not pass 'ports' argument to ahci_realize()
>   hw/ide/ahci: Remove SysbusAHCIState::num_ports field
>   hw/ide/ahci: Move SysBus definitions to 'ahci-sysbus.h'
> 
>  hw/ide/ahci_internal.h         | 10 +--------
>  include/hw/arm/allwinner-a10.h |  2 +-
>  include/hw/arm/allwinner-r40.h |  2 +-
>  include/hw/arm/xlnx-zynqmp.h   |  2 +-
>  include/hw/ide/ahci-pci.h      | 22 ++++++++++++++++++++
>  include/hw/ide/ahci-sysbus.h   | 35 +++++++++++++++++++++++++++++++
>  include/hw/ide/ahci.h          | 38 +++-------------------------------
>  hw/arm/highbank.c              |  2 +-
>  hw/arm/sbsa-ref.c              |  1 +
>  hw/i386/pc_q35.c               | 19 ++++++++++-------
>  hw/ide/ahci-allwinner.c        |  3 +--
>  hw/ide/ahci.c                  | 29 +++++++++-----------------
>  hw/ide/ich.c                   |  4 +++-
>  hw/mips/boston.c               | 14 +++++++------
>  14 files changed, 99 insertions(+), 84 deletions(-)
>  create mode 100644 include/hw/ide/ahci-pci.h
>  create mode 100644 include/hw/ide/ahci-sysbus.h
> 
> -- 
> 2.41.0

Re: [PATCH 1/9] hw/i386/q35: Use DEVICE() cast macro with PCIDevice object

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> QDev API provides the DEVICE() macro to access the
> 'qdev' parent field of the PCIDevice structure.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/i386/pc_q35.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/9] hw/ide/ahci: Expose AHCIPCIState structure

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> In order to be able to QOM-embed a structure, we need
> its full definition. Move it from "ahci_internal.h"
> to the new "hw/ide/ahci-pci.h" header.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/ide/ahci_internal.h    |  8 --------
>   include/hw/ide/ahci-pci.h | 22 ++++++++++++++++++++++
>   include/hw/ide/ahci.h     |  3 ---
>   hw/i386/pc_q35.c          |  2 +-
>   hw/ide/ahci.c             |  1 +
>   hw/ide/ich.c              |  1 +
>   hw/mips/boston.c          |  2 +-
>   7 files changed, 26 insertions(+), 13 deletions(-)
>   create mode 100644 include/hw/ide/ahci-pci.h

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/9] hw/ide/ahci: Rename AHCI PCI function as 'pdev'

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> We want to access AHCIPCIState::ahci field. In order to keep
> the code simple (avoiding &ahci->ahci), rename the current
> 'ahci' variable as 'pdev'
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/i386/pc_q35.c | 15 ++++++++-------
>   hw/mips/boston.c | 10 +++++-----
>   2 files changed, 13 insertions(+), 12 deletions(-)

Re: [PATCH 4/9] hw/ide/ahci: Inline ahci_get_num_ports()

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> Introduce the 'ich9' variable and inline ahci_get_num_ports().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/ide/ahci.h | 1 -
>   hw/i386/pc_q35.c      | 6 ++++--
>   hw/ide/ahci.c         | 8 --------
>   hw/mips/boston.c      | 6 ++++--
>   4 files changed, 8 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

As far as it goes.  But it certainly highlights that

> +        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
> +        ide_drive_get(hd, ich9->ahci.ports);
....
> +    g_assert(ARRAY_SIZE(hd) == ich9->ahci.ports);
> +    ide_drive_get(hd, ich9->ahci.ports);

ports is always a constant.  Or perhaps that's only from this PCI usage?


r~

Re: [PATCH 5/9] hw/ide/ahci: Pass AHCI context to ahci_ide_create_devs()

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> Since ahci_ide_create_devs() is not PCI specific, pass
> it an AHCIState argument instead of PCIDevice.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/ide/ahci.h | 2 +-
>   hw/i386/pc_q35.c      | 2 +-
>   hw/ide/ahci.c         | 5 +----
>   hw/mips/boston.c      | 2 +-
>   4 files changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 6/9] hw/ide/ahci: Convert AHCIState::ports to unsigned

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> AHCIState::ports should be unsigned. Besides, we never
> check it for negative value. It is unlikely it was ever
> used with more than INT32_MAX ports, so it is safe to
> convert it to unsigned.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/ide/ahci.h | 2 +-
>   hw/ide/ahci.c         | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 7/9] hw/ide/ahci: Do not pass 'ports' argument to ahci_realize()

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> Explicitly set AHCIState::ports before calling ahci_realize().
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/ide/ahci_internal.h | 2 +-
>   hw/ide/ahci.c          | 9 +++++----
>   hw/ide/ich.c           | 3 ++-
>   3 files changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 8/9] hw/ide/ahci: Remove SysbusAHCIState::num_ports field

[Richard Henderson]

On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
> No need to duplicate AHCIState::ports, directly access it.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/ide/ahci.h | 1 -
>   hw/ide/ahci.c         | 3 +--
>   2 files changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/9] hw/ide/ahci: Rename AHCI PCI function as 'pdev'

[Philippe Mathieu-Daudé]

On 13/2/24 17:32, Richard Henderson wrote:
> On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
>> We want to access AHCIPCIState::ahci field. In order to keep
>> the code simple (avoiding &ahci->ahci), rename the current
>> 'ahci' variable as 'pdev'
>>
>> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
>> ---
>>   hw/i386/pc_q35.c | 15 ++++++++-------
>>   hw/mips/boston.c | 10 +++++-----
>>   2 files changed, 13 insertions(+), 12 deletions(-)
> 

I'll handle this empty reply as a R-b tag :) Thanks!

Re: [PATCH 4/9] hw/ide/ahci: Inline ahci_get_num_ports()

[Philippe Mathieu-Daudé]

On 13/2/24 17:43, Richard Henderson wrote:
> On 2/12/24 22:11, Philippe Mathieu-Daudé wrote:
>> Introduce the 'ich9' variable and inline ahci_get_num_ports().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/ide/ahci.h | 1 -
>>   hw/i386/pc_q35.c      | 6 ++++--
>>   hw/ide/ahci.c         | 8 --------
>>   hw/mips/boston.c      | 6 ++++--
>>   4 files changed, 8 insertions(+), 13 deletions(-)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> As far as it goes.  But it certainly highlights that
> 
>> +        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
>> +        ide_drive_get(hd, ich9->ahci.ports);
> ....
>> +    g_assert(ARRAY_SIZE(hd) == ich9->ahci.ports);
>> +    ide_drive_get(hd, ich9->ahci.ports);
> 
> ports is always a constant.  Or perhaps that's only from this PCI usage?

I'm just moving code around :)

I'm not sure about this assert, but TBH I consider ide_drive_get()
as legacy API (pre -blockdev, only 7 uses in the tree, half from old
boards).

I'll let that constant cleanup for later.

Regards,

Phil.

Re: [PATCH 0/9] hw/ide/ahci: Housekeeping

[Philippe Mathieu-Daudé]

On 13/2/24 09:11, Philippe Mathieu-Daudé wrote:
> - Split 'ahci.h' as sysbus / pci
> - Inline ahci_get_num_ports()
> - Directly use AHCIState::ports instead of SysbusAHCIState::num_ports
> 
> Philippe Mathieu-Daudé (9):
>    hw/i386/q35: Use DEVICE() cast macro with PCIDevice object
>    hw/ide/ahci: Expose AHCIPCIState structure
>    hw/ide/ahci: Rename AHCI PCI function as 'pdev'
>    hw/ide/ahci: Inline ahci_get_num_ports()
>    hw/ide/ahci: Pass AHCI context to ahci_ide_create_devs()
>    hw/ide/ahci: Convert AHCIState::ports to unsigned
>    hw/ide/ahci: Do not pass 'ports' argument to ahci_realize()
>    hw/ide/ahci: Remove SysbusAHCIState::num_ports field
>    hw/ide/ahci: Move SysBus definitions to 'ahci-sysbus.h'

Series queued.