[PATCH 5/8] hw/ide/pci.c: Coding style update to fix checkpatch errors

[PATCH 5/8] hw/ide/pci.c: Coding style update to fix checkpatch errors

[BALATON Zoltan]

Spaces are required around a + operator and if statements should have
braces even for single line. Also make it simpler by reversing the
condition instead of breaking the loop.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4fc76c5225..e0c84392e2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -485,9 +485,9 @@ void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
     int i;
 
     for (i = 0; i < 4; i++) {
-        if (hd_table[i] == NULL)
-            continue;
-        ide_create_drive(d->bus+bus[i], unit[i], hd_table[i]);
+        if (hd_table[i]) {
+            ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
+        }
     }
 }
 
-- 
2.21.1

[PATCH 0/8] Misc hw/ide legacy clean up

[BALATON Zoltan]

These are some clean ups to remove more legacy init functions and
lessen dependence on include/hw/ide.h with some simplifications in
board code. There should be no functional change.

BALATON Zoltan (8):
  hw/ide: Get rid of piix3_init functions
  hw/ide: Get rid of piix4_init function
  hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h
  hw/ide: Move MAX_IDE_BUS define to one header
  hw/ide/pci.c: Coding style update to fix checkpatch errors
  hw/ide: Do ide_drive_get() within pci_ide_create_devs()
  hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h
  hw/ide: Remove unneeded inclusion of hw/ide.h

 hw/alpha/dp264.c              | 15 +++------------
 hw/hppa/hppa_sys.h            |  1 -
 hw/hppa/machine.c             |  3 ---
 hw/i386/pc_piix.c             | 20 +++++++++-----------
 hw/ide/ahci_internal.h        |  1 +
 hw/ide/pci.c                  | 10 ++++++----
 hw/ide/piix.c                 | 31 +------------------------------
 hw/isa/piix4.c                | 14 +++++---------
 hw/mips/mips_fulong2e.c       |  6 +-----
 hw/mips/mips_malta.c          |  6 ++----
 hw/mips/mips_r4k.c            |  4 +---
 hw/ppc/mac_newworld.c         |  2 --
 hw/ppc/mac_oldworld.c         |  2 --
 hw/ppc/prep.c                 |  3 ---
 hw/sparc64/sun4u.c            |  7 +------
 include/hw/ide.h              |  6 ------
 include/hw/ide/internal.h     |  3 +++
 include/hw/ide/pci.h          |  3 ++-
 include/hw/misc/macio/macio.h |  1 +
 include/hw/southbridge/piix.h |  3 +--
 20 files changed, 37 insertions(+), 104 deletions(-)

-- 
2.21.1

[PATCH 2/8] hw/ide: Get rid of piix4_init function

[BALATON Zoltan]

This removes pci_piix4_ide_init() function similar to clean up done to
other ide devices.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/piix.c    | 12 +-----------
 hw/isa/piix4.c   |  5 ++++-
 include/hw/ide.h |  1 -
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 8bcd6b72c2..3b2de4c312 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
     }
 }
 
-/* hd_table must contain 4 block drivers */
-/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
-PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-    PCIDevice *dev;
-
-    dev = pci_create_simple(bus, devfn, "piix4-ide");
-    pci_ide_create_devs(dev, hd_table);
-    return dev;
-}
-
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
@@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
     .class_init    = piix3_ide_class_init,
 };
 
+/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
 static void piix4_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 7edec5e149..0ab4787658 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -35,6 +35,7 @@
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
 #include "hw/ide.h"
+#include "hw/ide/pci.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
@@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
         *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
     }
 
+    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
     hd = g_new(DriveInfo *, ide_drives);
     ide_drive_get(hd, ide_drives);
-    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
+    pci_ide_create_devs(pci, hd);
     g_free(hd);
+
     pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
     if (smbus) {
         *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 883bbaeb9b..21bd8f23f1 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
-PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
 
 /* ide-mmio.c */
-- 
2.21.1

[PATCH 3/8] hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h

[BALATON Zoltan]

After previous patches we don't need hw/pci/pci.h any more in
hw/ide.h. Some files depended on implicit inclusion by this header
which are also fixed up here.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/ahci_internal.h        | 1 +
 include/hw/ide.h              | 1 -
 include/hw/ide/pci.h          | 1 +
 include/hw/misc/macio/macio.h | 1 +
 4 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 73424516da..bab0459774 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -27,6 +27,7 @@
 #include "hw/ide/ahci.h"
 #include "hw/ide/internal.h"
 #include "hw/sysbus.h"
+#include "hw/pci/pci.h"
 
 #define AHCI_MEM_BAR_SIZE         0x1000
 #define AHCI_MAX_PORTS            32
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 21bd8f23f1..d52c211f32 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -2,7 +2,6 @@
 #define HW_IDE_H
 
 #include "hw/isa/isa.h"
-#include "hw/pci/pci.h"
 #include "exec/memory.h"
 
 #define MAX_IDE_DEVS	2
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index a9f2c33e68..98ffa7dfcd 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -2,6 +2,7 @@
 #define HW_IDE_PCI_H
 
 #include "hw/ide/internal.h"
+#include "hw/pci/pci.h"
 
 #define BM_STATUS_DMAING 0x01
 #define BM_STATUS_ERROR  0x02
diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
index 070a694eb5..87335a991c 100644
--- a/include/hw/misc/macio/macio.h
+++ b/include/hw/misc/macio/macio.h
@@ -27,6 +27,7 @@
 #define MACIO_H
 
 #include "hw/char/escc.h"
+#include "hw/pci/pci.h"
 #include "hw/ide/internal.h"
 #include "hw/intc/heathrow_pic.h"
 #include "hw/misc/macio/cuda.h"
-- 
2.21.1

[PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[BALATON Zoltan]

The pci_ide_create_devs() function takes a hd_table parameter but all
callers just pass what ide_drive_get() returns so we can do it locally
simplifying callers and removing hd_table parameter.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/alpha/dp264.c              | 13 +++----------
 hw/i386/pc_piix.c             |  9 +++++----
 hw/ide/pci.c                  |  5 +++--
 hw/isa/piix4.c                | 10 ++--------
 hw/mips/mips_fulong2e.c       |  4 +---
 hw/mips/mips_malta.c          |  2 +-
 hw/sparc64/sun4u.c            |  6 +-----
 include/hw/ide/pci.h          |  2 +-
 include/hw/southbridge/piix.h |  3 +--
 9 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 0f58b1b668..e23172f177 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -15,7 +15,6 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/timer/i8254.h"
 #include "hw/isa/superio.h"
@@ -56,6 +55,7 @@ static void clipper_init(MachineState *machine)
     const char *initrd_filename = machine->initrd_filename;
     AlphaCPU *cpus[4];
     PCIBus *pci_bus;
+    PCIDevice *pci_dev;
     ISABus *isa_bus;
     qemu_irq rtc_irq;
     long size, i;
@@ -98,15 +98,8 @@ static void clipper_init(MachineState *machine)
     isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
 
     /* IDE disk setup.  */
-    {
-        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-        PCIDevice *pci_dev;
-
-        ide_drive_get(hd, ARRAY_SIZE(hd));
-
-        pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
-        pci_ide_create_devs(pci_dev, hd);
-    }
+    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
+    pci_ide_create_devs(pci_dev);
 
     /* Load PALcode.  Given that this is not "real" cpu palcode,
        but one explicitly written for the emulation, we might as
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b363a69e2e..a7a696d0c8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -84,7 +84,6 @@ static void pc_init1(MachineState *machine,
     int piix3_devfn = -1;
     qemu_irq smi_irq;
     GSIState *gsi_state;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
     MemoryRegion *ram_memory;
@@ -238,20 +237,22 @@ static void pc_init1(MachineState *machine,
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
 
-    ide_drive_get(hd, ARRAY_SIZE(hd));
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
 
         dev = pci_create_simple(pci_bus, piix3_devfn + 1,
                                 xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
-        pci_ide_create_devs(dev, hd);
+        pci_ide_create_devs(dev);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
         pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
     }
 #ifdef CONFIG_IDE_ISA
-else {
+    else {
+        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
         int i;
+
+        ide_drive_get(hd, ARRAY_SIZE(hd));
         for (i = 0; i < MAX_IDE_BUS; i++) {
             ISADevice *dev;
             char busname[] = "ide.0";
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index e0c84392e2..03d6eef592 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -476,14 +476,15 @@ const VMStateDescription vmstate_ide_pci = {
     }
 };
 
-/* hd_table must contain 4 block drivers */
-void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
+void pci_ide_create_devs(PCIDevice *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
+    DriveInfo *hd_table[MAX_IDE_BUS * MAX_IDE_DEVS];
     static const int bus[4]  = { 0, 0, 1, 1 };
     static const int unit[4] = { 0, 1, 0, 1 };
     int i;
 
+    ide_drive_get(hd_table, ARRAY_SIZE(hd_table));
     for (i = 0; i < 4; i++) {
         if (hd_table[i]) {
             ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0ab4787658..13fa1660c3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -241,11 +241,8 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
-                          I2CBus **smbus, size_t ide_buses)
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
-    size_t ide_drives = ide_buses * MAX_IDE_DEVS;
-    DriveInfo **hd;
     PCIDevice *pci;
     DeviceState *dev;
 
@@ -257,10 +254,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
     }
 
     pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
-    hd = g_new(DriveInfo *, ide_drives);
-    ide_drive_get(hd, ide_drives);
-    pci_ide_create_devs(pci, hd);
-    g_free(hd);
+    pci_ide_create_devs(pci);
 
     pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
     if (smbus) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 3690b76061..508dc57737 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -238,7 +238,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
 {
     qemu_irq *i8259;
     ISABus *isa_bus;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     PCIDevice *dev;
 
     isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
@@ -258,8 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
 
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
-    ide_drive_get(hd, ARRAY_SIZE(hd));
-    pci_ide_create_devs(dev, hd);
+    pci_ide_create_devs(dev);
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 6f51e33e7b..1da5271922 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1401,7 +1401,7 @@ void mips_malta_init(MachineState *machine)
     pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus, &isa_bus, &smbus, MAX_IDE_BUS);
+    dev = piix4_create(pci_bus, &isa_bus, &smbus);
 
     /* Interrupt controller */
     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 74acfd39b3..329e82c0f0 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -50,7 +50,6 @@
 #include "hw/sparc/sparc64.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
@@ -562,7 +561,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     PCIBus *pci_bus, *pci_busA, *pci_busB;
     PCIDevice *ebus, *pci_dev;
     SysBusDevice *s;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     DeviceState *iommu, *dev;
     FWCfgState *fw_cfg;
     NICInfo *nd;
@@ -662,12 +660,10 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
         qemu_macaddr_default_if_unset(&macaddr);
     }
 
-    ide_drive_get(hd, ARRAY_SIZE(hd));
-
     pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
     qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
     qdev_init_nofail(&pci_dev->qdev);
-    pci_ide_create_devs(pci_dev, hd);
+    pci_ide_create_devs(pci_dev);
 
     /* Map NVRAM into I/O (ebus) space */
     nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 98ffa7dfcd..dd504e5a0b 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -63,7 +63,7 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
 void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
-void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table);
+void pci_ide_create_devs(PCIDevice *dev);
 
 extern const VMStateDescription vmstate_ide_pci;
 extern const MemoryRegionOps pci_ide_cmd_le_ops;
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 152628c6d9..02bd741209 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -68,7 +68,6 @@ extern PCIDevice *piix4_dev;
 
 PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
-                          I2CBus **smbus, size_t ide_buses);
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
 
 #endif
-- 
2.21.1

[PATCH 7/8] hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h

[BALATON Zoltan]

We can move it next to the MAX_IDE_BUS define now that less files use
it.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 include/hw/ide.h          | 2 --
 include/hw/ide/internal.h | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/ide.h b/include/hw/ide.h
index d52c211f32..c5ce5da4f4 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -4,8 +4,6 @@
 #include "hw/isa/isa.h"
 #include "exec/memory.h"
 
-#define MAX_IDE_DEVS	2
-
 /* ide-isa.c */
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1a49d35959..fd3cae4acf 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -28,6 +28,7 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define IDE_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
 
 #define MAX_IDE_BUS 2
+#define MAX_IDE_DEVS 2
 
 /* Bits of HD_STATUS */
 #define ERR_STAT		0x01
-- 
2.21.1

[PATCH 1/8] hw/ide: Get rid of piix3_init functions

[BALATON Zoltan]

This removes pci_piix3_ide_init() and pci_piix3_xen_ide_init()
functions similar to clean up done to other ide devices.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/i386/pc_piix.c | 10 +++++-----
 hw/ide/pci.c      |  1 +
 hw/ide/piix.c     | 21 +--------------------
 include/hw/ide.h  |  2 --
 4 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e2d98243bc..c399398739 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -39,6 +39,7 @@
 #include "hw/usb.h"
 #include "net/net.h"
 #include "hw/ide.h"
+#include "hw/ide/pci.h"
 #include "hw/irq.h"
 #include "sysemu/kvm.h"
 #include "hw/kvm/clock.h"
@@ -242,11 +243,10 @@ static void pc_init1(MachineState *machine,
     ide_drive_get(hd, ARRAY_SIZE(hd));
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
-        if (xen_enabled()) {
-            dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1);
-        } else {
-            dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
-        }
+
+        dev = pci_create_simple(pci_bus, piix3_devfn + 1,
+                                xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
+        pci_ide_create_devs(dev, hd);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
         pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 1a6a287e76..4fc76c5225 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -476,6 +476,7 @@ const VMStateDescription vmstate_ide_pci = {
     }
 };
 
+/* hd_table must contain 4 block drivers */
 void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
 {
     PCIIDEState *d = PCI_IDE(dev);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bc575b4d70..8bcd6b72c2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -197,15 +197,6 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
     return 0;
 }
 
-PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-    PCIDevice *dev;
-
-    dev = pci_create_simple(bus, devfn, "piix3-ide-xen");
-    pci_ide_create_devs(dev, hd_table);
-    return dev;
-}
-
 static void pci_piix_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
@@ -217,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
     }
 }
 
-/* hd_table must contain 4 block drivers */
-/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
-PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
-{
-    PCIDevice *dev;
-
-    dev = pci_create_simple(bus, devfn, "piix3-ide");
-    pci_ide_create_devs(dev, hd_table);
-    return dev;
-}
-
 /* hd_table must contain 4 block drivers */
 /* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
@@ -239,6 +219,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
     return dev;
 }
 
+/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/ide.h b/include/hw/ide.h
index dea0ecf5be..883bbaeb9b 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -12,8 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
 
 /* ide-pci.c */
-PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
-PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
 
-- 
2.21.1

[PATCH 8/8] hw/ide: Remove unneeded inclusion of hw/ide.h

[BALATON Zoltan]

After previous clean ups we can drop direct inclusion of hw/ide.h from
several places.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/hppa/hppa_sys.h      | 1 -
 hw/hppa/machine.c       | 1 -
 hw/i386/pc_piix.c       | 1 -
 hw/isa/piix4.c          | 1 -
 hw/mips/mips_fulong2e.c | 1 -
 hw/ppc/mac_newworld.c   | 1 -
 hw/ppc/mac_oldworld.c   | 1 -
 hw/ppc/prep.c           | 1 -
 8 files changed, 8 deletions(-)

diff --git a/hw/hppa/hppa_sys.h b/hw/hppa/hppa_sys.h
index 4d08501464..0b18271cc9 100644
--- a/hw/hppa/hppa_sys.h
+++ b/hw/hppa/hppa_sys.h
@@ -5,7 +5,6 @@
 
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
-#include "hw/ide.h"
 #include "hw/boards.h"
 #include "hw/intc/i8259.h"
 
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 1f9a390f99..d1fbf781c5 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -13,7 +13,6 @@
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/timer/i8254.h"
 #include "hw/char/serial.h"
 #include "hw/net/lasi_82596.h"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a7a696d0c8..ac78b07451 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -38,7 +38,6 @@
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
 #include "net/net.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/irq.h"
 #include "sysemu/kvm.h"
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 13fa1660c3..136a763e3f 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -34,7 +34,6 @@
 #include "hw/dma/i8257.h"
 #include "hw/timer/i8254.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "migration/vmstate.h"
 #include "sysemu/reset.h"
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 508dc57737..ab6634d538 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -36,7 +36,6 @@
 #include "audio/audio.h"
 #include "qemu/log.h"
 #include "hw/loader.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "elf.h"
 #include "hw/isa/vt82c686.h"
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index daa1523feb..dfe5aeab0a 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -62,7 +62,6 @@
 #include "hw/char/escc.h"
 #include "hw/misc/macio/macio.h"
 #include "hw/ppc/openpic.h"
-#include "hw/ide.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 2478748c78..4c1b1f35a1 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -41,7 +41,6 @@
 #include "hw/nvram/fw_cfg.h"
 #include "hw/char/escc.h"
 #include "hw/misc/macio/macio.h"
-#include "hw/ide.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index e1b1549e58..52bded7e5e 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -37,7 +37,6 @@
 #include "hw/boards.h"
 #include "qemu/error-report.h"
 #include "qemu/log.h"
-#include "hw/ide.h"
 #include "hw/irq.h"
 #include "hw/loader.h"
 #include "hw/rtc/mc146818rtc.h"
-- 
2.21.1

[PATCH 4/8] hw/ide: Move MAX_IDE_BUS define to one header

[BALATON Zoltan]

There are several definitions of MAX_IDE_BUS in different boards (some
of them unused) with the same value. Move it to include/hw/ide/internal.h
to have it in a central place.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/alpha/dp264.c          | 2 --
 hw/hppa/machine.c         | 2 --
 hw/i386/pc_piix.c         | 2 --
 hw/mips/mips_fulong2e.c   | 1 -
 hw/mips/mips_malta.c      | 4 +---
 hw/mips/mips_r4k.c        | 4 +---
 hw/ppc/mac_newworld.c     | 1 -
 hw/ppc/mac_oldworld.c     | 1 -
 hw/ppc/prep.c             | 2 --
 hw/sparc64/sun4u.c        | 1 -
 include/hw/ide/internal.h | 2 ++
 11 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 27595767e5..0f58b1b668 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -24,8 +24,6 @@
 #include "qemu/cutils.h"
 #include "net/net.h"
 
-#define MAX_IDE_BUS 2
-
 static uint64_t cpu_alpha_superpage_to_phys(void *opaque, uint64_t addr)
 {
     if (((addr >> 41) & 3) == 2) {
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 9175f4b790..1f9a390f99 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -24,8 +24,6 @@
 #include "qemu/log.h"
 #include "net/net.h"
 
-#define MAX_IDE_BUS 2
-
 static ISABus *hppa_isa_bus(void)
 {
     ISABus *isa_bus;
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c399398739..b363a69e2e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -63,8 +63,6 @@
 #include "sysemu/numa.h"
 #include "hw/mem/nvdimm.h"
 
-#define MAX_IDE_BUS 2
-
 #ifdef CONFIG_IDE_ISA
 static const int ide_iobase[MAX_IDE_BUS] = { 0x1f0, 0x170 };
 static const int ide_iobase2[MAX_IDE_BUS] = { 0x3f6, 0x376 };
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 639ba2a091..3690b76061 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -55,7 +55,6 @@
 
 /* fulong 2e has a 512k flash: Winbond W39L040AP70Z */
 #define BIOS_SIZE               (512 * KiB)
-#define MAX_IDE_BUS             2
 
 /*
  * PMON is not part of qemu and released with BSD license, anyone
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index d380f73d7b..6f51e33e7b 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -40,7 +40,7 @@
 #include "sysemu/arch_init.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
-#include "hw/ide.h"
+#include "hw/ide/internal.h"
 #include "hw/irq.h"
 #include "hw/loader.h"
 #include "elf.h"
@@ -68,8 +68,6 @@
 
 #define FLASH_SIZE          0x400000
 
-#define MAX_IDE_BUS         2
-
 typedef struct {
     MemoryRegion iomem;
     MemoryRegion iomem_lo; /* 0 - 0x900 */
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index ad8b75e286..2e5372bda0 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -25,7 +25,7 @@
 #include "hw/block/flash.h"
 #include "qemu/log.h"
 #include "hw/mips/bios.h"
-#include "hw/ide.h"
+#include "hw/ide/internal.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "hw/rtc/mc146818rtc.h"
@@ -37,8 +37,6 @@
 #include "sysemu/runstate.h"
 #include "qemu/error-report.h"
 
-#define MAX_IDE_BUS 2
-
 static const int ide_iobase[2] = { 0x1f0, 0x170 };
 static const int ide_iobase2[2] = { 0x3f6, 0x376 };
 static const int ide_irq[2] = { 14, 15 };
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b8189bf7a4..daa1523feb 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -75,7 +75,6 @@
 #include "hw/sysbus.h"
 #include "trace.h"
 
-#define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
 #define TBFREQ (100UL * 1000UL * 1000UL)
 #define CLOCKFREQ (900UL * 1000UL * 1000UL)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 440c406eb4..2478748c78 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -51,7 +51,6 @@
 #include "kvm_ppc.h"
 #include "exec/address-spaces.h"
 
-#define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
 #define TBFREQ 16600000UL
 #define CLOCKFREQ 266000000UL
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 111cc80867..e1b1549e58 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -56,8 +56,6 @@
 /* SMP is not enabled, for now */
 #define MAX_CPUS 1
 
-#define MAX_IDE_BUS 2
-
 #define CFG_ADDR 0xf0000510
 
 #define KERNEL_LOAD_ADDR 0x01000000
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d33e84f831..74acfd39b3 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -66,7 +66,6 @@
 #define PBM_PCI_IO_BASE      (PBM_SPECIAL_BASE + 0x02000000ULL)
 #define PROM_FILENAME        "openbios-sparc64"
 #define NVRAM_SIZE           0x2000
-#define MAX_IDE_BUS          2
 #define BIOS_CFG_IOPORT      0x510
 #define FW_CFG_SPARC64_WIDTH (FW_CFG_ARCH_LOCAL + 0x00)
 #define FW_CFG_SPARC64_HEIGHT (FW_CFG_ARCH_LOCAL + 0x01)
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1bc1fc73e5..1a49d35959 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -27,6 +27,8 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define TYPE_IDE_BUS "IDE"
 #define IDE_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
 
+#define MAX_IDE_BUS 2
+
 /* Bits of HD_STATUS */
 #define ERR_STAT		0x01
 #define INDEX_STAT		0x02
-- 
2.21.1

Re: [PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[BALATON Zoltan]

On Fri, 13 Mar 2020, BALATON Zoltan wrote:
> The pci_ide_create_devs() function takes a hd_table parameter but all
> callers just pass what ide_drive_get() returns so we can do it locally
> simplifying callers and removing hd_table parameter.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/alpha/dp264.c              | 13 +++----------
> hw/i386/pc_piix.c             |  9 +++++----
> hw/ide/pci.c                  |  5 +++--
> hw/isa/piix4.c                | 10 ++--------
> hw/mips/mips_fulong2e.c       |  4 +---
> hw/mips/mips_malta.c          |  2 +-
> hw/sparc64/sun4u.c            |  6 +-----
> include/hw/ide/pci.h          |  2 +-
> include/hw/southbridge/piix.h |  3 +--
> 9 files changed, 18 insertions(+), 36 deletions(-)
>
> diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
> index 0f58b1b668..e23172f177 100644
> --- a/hw/alpha/dp264.c
> +++ b/hw/alpha/dp264.c
> @@ -15,7 +15,6 @@
> #include "qemu/error-report.h"
> #include "sysemu/sysemu.h"
> #include "hw/rtc/mc146818rtc.h"
> -#include "hw/ide.h"
> #include "hw/ide/pci.h"
> #include "hw/timer/i8254.h"
> #include "hw/isa/superio.h"
> @@ -56,6 +55,7 @@ static void clipper_init(MachineState *machine)
>     const char *initrd_filename = machine->initrd_filename;
>     AlphaCPU *cpus[4];
>     PCIBus *pci_bus;
> +    PCIDevice *pci_dev;
>     ISABus *isa_bus;
>     qemu_irq rtc_irq;
>     long size, i;
> @@ -98,15 +98,8 @@ static void clipper_init(MachineState *machine)
>     isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
>
>     /* IDE disk setup.  */
> -    {
> -        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
> -        PCIDevice *pci_dev;
> -
> -        ide_drive_get(hd, ARRAY_SIZE(hd));
> -
> -        pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
> -        pci_ide_create_devs(pci_dev, hd);
> -    }
> +    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
> +    pci_ide_create_devs(pci_dev);

Additionally, I think it may also make sense to move pci_ide_create_devs 
call into the realize methods of these IDE controllers so boards do not 
need to do it explicitely. These calls always follow the creation of the 
device immediately so could just be done internally in IDE device and 
simplify it further. I can attempt to prepare additional patches for that 
but first I'd like to hear if anyone has anything against that to avoid 
doing useless work.

Regards,
BALATON Zoltan

>     /* Load PALcode.  Given that this is not "real" cpu palcode,
>        but one explicitly written for the emulation, we might as
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b363a69e2e..a7a696d0c8 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -84,7 +84,6 @@ static void pc_init1(MachineState *machine,
>     int piix3_devfn = -1;
>     qemu_irq smi_irq;
>     GSIState *gsi_state;
> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>     BusState *idebus[MAX_IDE_BUS];
>     ISADevice *rtc_state;
>     MemoryRegion *ram_memory;
> @@ -238,20 +237,22 @@ static void pc_init1(MachineState *machine,
>
>     pc_nic_init(pcmc, isa_bus, pci_bus);
>
> -    ide_drive_get(hd, ARRAY_SIZE(hd));
>     if (pcmc->pci_enabled) {
>         PCIDevice *dev;
>
>         dev = pci_create_simple(pci_bus, piix3_devfn + 1,
>                                 xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
> -        pci_ide_create_devs(dev, hd);
> +        pci_ide_create_devs(dev);
>         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
>         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
>         pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
>     }
> #ifdef CONFIG_IDE_ISA
> -else {
> +    else {
> +        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>         int i;
> +
> +        ide_drive_get(hd, ARRAY_SIZE(hd));
>         for (i = 0; i < MAX_IDE_BUS; i++) {
>             ISADevice *dev;
>             char busname[] = "ide.0";
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index e0c84392e2..03d6eef592 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -476,14 +476,15 @@ const VMStateDescription vmstate_ide_pci = {
>     }
> };
>
> -/* hd_table must contain 4 block drivers */
> -void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
> +void pci_ide_create_devs(PCIDevice *dev)
> {
>     PCIIDEState *d = PCI_IDE(dev);
> +    DriveInfo *hd_table[MAX_IDE_BUS * MAX_IDE_DEVS];
>     static const int bus[4]  = { 0, 0, 1, 1 };
>     static const int unit[4] = { 0, 1, 0, 1 };
>     int i;
>
> +    ide_drive_get(hd_table, ARRAY_SIZE(hd_table));
>     for (i = 0; i < 4; i++) {
>         if (hd_table[i]) {
>             ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 0ab4787658..13fa1660c3 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -241,11 +241,8 @@ static void piix4_register_types(void)
>
> type_init(piix4_register_types)
>
> -DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
> -                          I2CBus **smbus, size_t ide_buses)
> +DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
> {
> -    size_t ide_drives = ide_buses * MAX_IDE_DEVS;
> -    DriveInfo **hd;
>     PCIDevice *pci;
>     DeviceState *dev;
>
> @@ -257,10 +254,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>     }
>
>     pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
> -    hd = g_new(DriveInfo *, ide_drives);
> -    ide_drive_get(hd, ide_drives);
> -    pci_ide_create_devs(pci, hd);
> -    g_free(hd);
> +    pci_ide_create_devs(pci);
>
>     pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>     if (smbus) {
> diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
> index 3690b76061..508dc57737 100644
> --- a/hw/mips/mips_fulong2e.c
> +++ b/hw/mips/mips_fulong2e.c
> @@ -238,7 +238,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
> {
>     qemu_irq *i8259;
>     ISABus *isa_bus;
> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>     PCIDevice *dev;
>
>     isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
> @@ -258,8 +257,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>
>     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
> -    ide_drive_get(hd, ARRAY_SIZE(hd));
> -    pci_ide_create_devs(dev, hd);
> +    pci_ide_create_devs(dev);
>
>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
>     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 6f51e33e7b..1da5271922 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -1401,7 +1401,7 @@ void mips_malta_init(MachineState *machine)
>     pci_bus = gt64120_register(s->i8259);
>
>     /* Southbridge */
> -    dev = piix4_create(pci_bus, &isa_bus, &smbus, MAX_IDE_BUS);
> +    dev = piix4_create(pci_bus, &isa_bus, &smbus);
>
>     /* Interrupt controller */
>     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 74acfd39b3..329e82c0f0 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -50,7 +50,6 @@
> #include "hw/sparc/sparc64.h"
> #include "hw/nvram/fw_cfg.h"
> #include "hw/sysbus.h"
> -#include "hw/ide.h"
> #include "hw/ide/pci.h"
> #include "hw/loader.h"
> #include "hw/fw-path-provider.h"
> @@ -562,7 +561,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>     PCIBus *pci_bus, *pci_busA, *pci_busB;
>     PCIDevice *ebus, *pci_dev;
>     SysBusDevice *s;
> -    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>     DeviceState *iommu, *dev;
>     FWCfgState *fw_cfg;
>     NICInfo *nd;
> @@ -662,12 +660,10 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>         qemu_macaddr_default_if_unset(&macaddr);
>     }
>
> -    ide_drive_get(hd, ARRAY_SIZE(hd));
> -
>     pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
>     qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
>     qdev_init_nofail(&pci_dev->qdev);
> -    pci_ide_create_devs(pci_dev, hd);
> +    pci_ide_create_devs(pci_dev);
>
>     /* Map NVRAM into I/O (ebus) space */
>     nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index 98ffa7dfcd..dd504e5a0b 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -63,7 +63,7 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
> void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
> extern MemoryRegionOps bmdma_addr_ioport_ops;
> -void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table);
> +void pci_ide_create_devs(PCIDevice *dev);
>
> extern const VMStateDescription vmstate_ide_pci;
> extern const MemoryRegionOps pci_ide_cmd_le_ops;
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 152628c6d9..02bd741209 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -68,7 +68,6 @@ extern PCIDevice *piix4_dev;
>
> PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
>
> -DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
> -                          I2CBus **smbus, size_t ide_buses);
> +DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
>
> #endif
>

Re: [PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[Paolo Bonzini]

On 13/03/20 23:16, BALATON Zoltan wrote:
>>
>> +    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
>> +    pci_ide_create_devs(pci_dev);
> 
> Additionally, I think it may also make sense to move pci_ide_create_devs
> call into the realize methods of these IDE controllers so boards do not
> need to do it explicitely. These calls always follow the creation of the
> device immediately so could just be done internally in IDE device and
> simplify it further. I can attempt to prepare additional patches for
> that but first I'd like to hear if anyone has anything against that to
> avoid doing useless work.

No, it's better to do it separately.  I think that otherwise you could
add another IDE controller with -device, and both controllers would try
to add the drives.

Basically, separating the call means that only automatically added
controllers obey "if=ide".

Paolo

Re: [PATCH 0/8] Misc hw/ide legacy clean up

[Mark Cave-Ayland]

On 13/03/2020 21:14, BALATON Zoltan wrote:

> These are some clean ups to remove more legacy init functions and
> lessen dependence on include/hw/ide.h with some simplifications in
> board code. There should be no functional change.
> 
> BALATON Zoltan (8):
>   hw/ide: Get rid of piix3_init functions
>   hw/ide: Get rid of piix4_init function
>   hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h
>   hw/ide: Move MAX_IDE_BUS define to one header
>   hw/ide/pci.c: Coding style update to fix checkpatch errors
>   hw/ide: Do ide_drive_get() within pci_ide_create_devs()
>   hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h
>   hw/ide: Remove unneeded inclusion of hw/ide.h
> 
>  hw/alpha/dp264.c              | 15 +++------------
>  hw/hppa/hppa_sys.h            |  1 -
>  hw/hppa/machine.c             |  3 ---
>  hw/i386/pc_piix.c             | 20 +++++++++-----------
>  hw/ide/ahci_internal.h        |  1 +
>  hw/ide/pci.c                  | 10 ++++++----
>  hw/ide/piix.c                 | 31 +------------------------------
>  hw/isa/piix4.c                | 14 +++++---------
>  hw/mips/mips_fulong2e.c       |  6 +-----
>  hw/mips/mips_malta.c          |  6 ++----
>  hw/mips/mips_r4k.c            |  4 +---
>  hw/ppc/mac_newworld.c         |  2 --
>  hw/ppc/mac_oldworld.c         |  2 --
>  hw/ppc/prep.c                 |  3 ---
>  hw/sparc64/sun4u.c            |  7 +------
>  include/hw/ide.h              |  6 ------
>  include/hw/ide/internal.h     |  3 +++
>  include/hw/ide/pci.h          |  3 ++-
>  include/hw/misc/macio/macio.h |  1 +
>  include/hw/southbridge/piix.h |  3 +--
>  20 files changed, 37 insertions(+), 104 deletions(-)

This looks like a good clean-up to me, but certainly it would be good to get a second
opinion from people more familiar with the IDE code internals.

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH 0/8] Misc hw/ide legacy clean up

[Paolo Bonzini]

On 14/03/20 12:45, Mark Cave-Ayland wrote:
> On 13/03/2020 21:14, BALATON Zoltan wrote:
> 
>> These are some clean ups to remove more legacy init functions and
>> lessen dependence on include/hw/ide.h with some simplifications in
>> board code. There should be no functional change.
>>
>> BALATON Zoltan (8):
>>   hw/ide: Get rid of piix3_init functions
>>   hw/ide: Get rid of piix4_init function
>>   hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h
>>   hw/ide: Move MAX_IDE_BUS define to one header
>>   hw/ide/pci.c: Coding style update to fix checkpatch errors
>>   hw/ide: Do ide_drive_get() within pci_ide_create_devs()
>>   hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h
>>   hw/ide: Remove unneeded inclusion of hw/ide.h
>>
>>  hw/alpha/dp264.c              | 15 +++------------
>>  hw/hppa/hppa_sys.h            |  1 -
>>  hw/hppa/machine.c             |  3 ---
>>  hw/i386/pc_piix.c             | 20 +++++++++-----------
>>  hw/ide/ahci_internal.h        |  1 +
>>  hw/ide/pci.c                  | 10 ++++++----
>>  hw/ide/piix.c                 | 31 +------------------------------
>>  hw/isa/piix4.c                | 14 +++++---------
>>  hw/mips/mips_fulong2e.c       |  6 +-----
>>  hw/mips/mips_malta.c          |  6 ++----
>>  hw/mips/mips_r4k.c            |  4 +---
>>  hw/ppc/mac_newworld.c         |  2 --
>>  hw/ppc/mac_oldworld.c         |  2 --
>>  hw/ppc/prep.c                 |  3 ---
>>  hw/sparc64/sun4u.c            |  7 +------
>>  include/hw/ide.h              |  6 ------
>>  include/hw/ide/internal.h     |  3 +++
>>  include/hw/ide/pci.h          |  3 ++-
>>  include/hw/misc/macio/macio.h |  1 +
>>  include/hw/southbridge/piix.h |  3 +--
>>  20 files changed, 37 insertions(+), 104 deletions(-)
> 
> This looks like a good clean-up to me, but certainly it would be good to get a second
> opinion from people more familiar with the IDE code internals.
> 
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Yes, it looks good to me.  Thanks!

Paolo

Re: [PATCH 1/8] hw/ide: Get rid of piix3_init functions

[Philippe Mathieu-Daudé]

On 3/13/20 10:14 PM, BALATON Zoltan wrote:
> This removes pci_piix3_ide_init() and pci_piix3_xen_ide_init()
> functions similar to clean up done to other ide devices.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/i386/pc_piix.c | 10 +++++-----
>   hw/ide/pci.c      |  1 +
>   hw/ide/piix.c     | 21 +--------------------
>   include/hw/ide.h  |  2 --
>   4 files changed, 7 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e2d98243bc..c399398739 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -39,6 +39,7 @@
>   #include "hw/usb.h"
>   #include "net/net.h"
>   #include "hw/ide.h"
> +#include "hw/ide/pci.h"
>   #include "hw/irq.h"
>   #include "sysemu/kvm.h"
>   #include "hw/kvm/clock.h"
> @@ -242,11 +243,10 @@ static void pc_init1(MachineState *machine,
>       ide_drive_get(hd, ARRAY_SIZE(hd));
>       if (pcmc->pci_enabled) {
>           PCIDevice *dev;
> -        if (xen_enabled()) {
> -            dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1);
> -        } else {
> -            dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
> -        }
> +
> +        dev = pci_create_simple(pci_bus, piix3_devfn + 1,
> +                                xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
> +        pci_ide_create_devs(dev, hd);
>           idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
>           idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
>           pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 1a6a287e76..4fc76c5225 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -476,6 +476,7 @@ const VMStateDescription vmstate_ide_pci = {
>       }
>   };
>   
> +/* hd_table must contain 4 block drivers */
>   void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index bc575b4d70..8bcd6b72c2 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -197,15 +197,6 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux)
>       return 0;
>   }
>   
> -PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> -{
> -    PCIDevice *dev;
> -
> -    dev = pci_create_simple(bus, devfn, "piix3-ide-xen");
> -    pci_ide_create_devs(dev, hd_table);
> -    return dev;
> -}
> -
>   static void pci_piix_ide_exitfn(PCIDevice *dev)
>   {
>       PCIIDEState *d = PCI_IDE(dev);
> @@ -217,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>       }
>   }
>   
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
> -PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> -{
> -    PCIDevice *dev;
> -
> -    dev = pci_create_simple(bus, devfn, "piix3-ide");
> -    pci_ide_create_devs(dev, hd_table);
> -    return dev;
> -}
> -
>   /* hd_table must contain 4 block drivers */
>   /* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>   PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> @@ -239,6 +219,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
>       return dev;
>   }
>   
> +/* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index dea0ecf5be..883bbaeb9b 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,8 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                           DriveInfo *hd0, DriveInfo *hd1);
>   
>   /* ide-pci.c */
> -PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> -PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>   PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>   
> 

Re: [PATCH 2/8] hw/ide: Get rid of piix4_init function

[Philippe Mathieu-Daudé]

On 3/13/20 10:14 PM, BALATON Zoltan wrote:
> This removes pci_piix4_ide_init() function similar to clean up done to
> other ide devices.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ide/piix.c    | 12 +-----------
>   hw/isa/piix4.c   |  5 ++++-
>   include/hw/ide.h |  1 -
>   3 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 8bcd6b72c2..3b2de4c312 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>       }
>   }
>   
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> -{
> -    PCIDevice *dev;
> -
> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
> -    pci_ide_create_devs(dev, hd_table);
> -    return dev;
> -}
> -
>   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>   {
> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>       .class_init    = piix3_ide_class_init,
>   };
>   
> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>   static void piix4_ide_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 7edec5e149..0ab4787658 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -35,6 +35,7 @@
>   #include "hw/timer/i8254.h"
>   #include "hw/rtc/mc146818rtc.h"
>   #include "hw/ide.h"
> +#include "hw/ide/pci.h"
>   #include "migration/vmstate.h"
>   #include "sysemu/reset.h"
>   #include "sysemu/runstate.h"
> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>           *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>       }
>   
> +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");

Why are you re-assigning 'pci'?

>       hd = g_new(DriveInfo *, ide_drives);
>       ide_drive_get(hd, ide_drives);
> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> +    pci_ide_create_devs(pci, hd);
>       g_free(hd);
> +
>       pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>       if (smbus) {
>           *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 883bbaeb9b..21bd8f23f1 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                           DriveInfo *hd0, DriveInfo *hd1);
>   
>   /* ide-pci.c */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>   
>   /* ide-mmio.c */
> 

Re: [PATCH 3/8] hw/ide: Remove now unneded #include "hw/pci/pci.h" from hw/ide.h

[Philippe Mathieu-Daudé]

On 3/13/20 10:14 PM, BALATON Zoltan wrote:
> After previous patches we don't need hw/pci/pci.h any more in
> hw/ide.h. Some files depended on implicit inclusion by this header
> which are also fixed up here.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/ide/ahci_internal.h        | 1 +
>   include/hw/ide.h              | 1 -
>   include/hw/ide/pci.h          | 1 +
>   include/hw/misc/macio/macio.h | 1 +
>   4 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
> index 73424516da..bab0459774 100644
> --- a/hw/ide/ahci_internal.h
> +++ b/hw/ide/ahci_internal.h
> @@ -27,6 +27,7 @@
>   #include "hw/ide/ahci.h"
>   #include "hw/ide/internal.h"
>   #include "hw/sysbus.h"
> +#include "hw/pci/pci.h"
>   
>   #define AHCI_MEM_BAR_SIZE         0x1000
>   #define AHCI_MAX_PORTS            32
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 21bd8f23f1..d52c211f32 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -2,7 +2,6 @@
>   #define HW_IDE_H
>   
>   #include "hw/isa/isa.h"
> -#include "hw/pci/pci.h"
>   #include "exec/memory.h"
>   
>   #define MAX_IDE_DEVS	2
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index a9f2c33e68..98ffa7dfcd 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -2,6 +2,7 @@
>   #define HW_IDE_PCI_H
>   
>   #include "hw/ide/internal.h"
> +#include "hw/pci/pci.h"
>   
>   #define BM_STATUS_DMAING 0x01
>   #define BM_STATUS_ERROR  0x02
> diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
> index 070a694eb5..87335a991c 100644
> --- a/include/hw/misc/macio/macio.h
> +++ b/include/hw/misc/macio/macio.h
> @@ -27,6 +27,7 @@
>   #define MACIO_H
>   
>   #include "hw/char/escc.h"
> +#include "hw/pci/pci.h"
>   #include "hw/ide/internal.h"
>   #include "hw/intc/heathrow_pic.h"
>   #include "hw/misc/macio/cuda.h"
> 

Re: [PATCH 5/8] hw/ide/pci.c: Coding style update to fix checkpatch errors

[Philippe Mathieu-Daudé]

On 3/13/20 10:14 PM, BALATON Zoltan wrote:
> Spaces are required around a + operator and if statements should have
> braces even for single line. Also make it simpler by reversing the
> condition instead of breaking the loop.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/ide/pci.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 4fc76c5225..e0c84392e2 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -485,9 +485,9 @@ void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
>       int i;
>   
>       for (i = 0; i < 4; i++) {
> -        if (hd_table[i] == NULL)
> -            continue;
> -        ide_create_drive(d->bus+bus[i], unit[i], hd_table[i]);
> +        if (hd_table[i]) {
> +            ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
> +        }
>       }
>   }
>   
> 

Re: [PATCH 2/8] hw/ide: Get rid of piix4_init function

[BALATON Zoltan]

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

On Sat, 14 Mar 2020, Philippe Mathieu-Daudé wrote:
> On 3/13/20 10:14 PM, BALATON Zoltan wrote:
>> This removes pci_piix4_ide_init() function similar to clean up done to
>> other ide devices.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ide/piix.c    | 12 +-----------
>>   hw/isa/piix4.c   |  5 ++++-
>>   include/hw/ide.h |  1 -
>>   3 files changed, 5 insertions(+), 13 deletions(-)
>> 
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 8bcd6b72c2..3b2de4c312 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>       }
>>   }
>>   -/* hd_table must contain 4 block drivers */
>> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
>> devfn)
>> -{
>> -    PCIDevice *dev;
>> -
>> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
>> -    pci_ide_create_devs(dev, hd_table);
>> -    return dev;
>> -}
>> -
>>   /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>>   static void piix3_ide_class_init(ObjectClass *klass, void *data)
>>   {
>> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>>       .class_init    = piix3_ide_class_init,
>>   };
>>   +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>>   static void piix4_ide_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 7edec5e149..0ab4787658 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -35,6 +35,7 @@
>>   #include "hw/timer/i8254.h"
>>   #include "hw/rtc/mc146818rtc.h"
>>   #include "hw/ide.h"
>> +#include "hw/ide/pci.h"
>>   #include "migration/vmstate.h"
>>   #include "sysemu/reset.h"
>>   #include "sysemu/runstate.h"
>> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus 
>> **isa_bus,
>>           *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>       }
>>   +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>
> Why are you re-assigning 'pci'?

Need a place to store it to pass to pci_ide_create_devs below and pci is 
unused at this point so it can be reused for this.  (The variable pci 
pointing to a PCIDevice was only used at the beginning of the function to 
cast to dev then it's not needed any more.) Since this is very short func 
and the reassign is right after its previous usage this should not be too 
confusing and avoids needing to define another only once used variable fot 
this. See also patch 6 (http://patchwork.ozlabs.org/patch/1254687/) that 
simplifies it further.

We could also do without this variable and write:

dev = DEVICE(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0),
              true, TYPE_PIIX4_PCI_DEVICE));

or after patch 6 even

pci_ide_create_devs(pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide"));

but I think those are less readable than reusing variable pci here.

Regards,
BALATON Zoltan

>>       hd = g_new(DriveInfo *, ide_drives);
>>       ide_drive_get(hd, ide_drives);
>> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
>> +    pci_ide_create_devs(pci, hd);
>>       g_free(hd);
>> +
>>       pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>>       if (smbus) {
>>           *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index 883bbaeb9b..21bd8f23f1 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int 
>> iobase2, int isairq,
>>                           DriveInfo *hd0, DriveInfo *hd1);
>>     /* ide-pci.c */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
>> devfn);
>>   int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>>     /* ide-mmio.c */
>> 
>
>

Re: [PATCH 2/8] hw/ide: Get rid of piix4_init function

[Michael S. Tsirkin]

On Fri, Mar 13, 2020 at 10:14:34PM +0100, BALATON Zoltan wrote:
> This removes pci_piix4_ide_init() function similar to clean up done to
> other ide devices.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/piix.c    | 12 +-----------
>  hw/isa/piix4.c   |  5 ++++-
>  include/hw/ide.h |  1 -
>  3 files changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index 8bcd6b72c2..3b2de4c312 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>      }
>  }
>  
> -/* hd_table must contain 4 block drivers */
> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> -{
> -    PCIDevice *dev;
> -
> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
> -    pci_ide_create_devs(dev, hd_table);
> -    return dev;
> -}
> -
>  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>  static void piix3_ide_class_init(ObjectClass *klass, void *data)
>  {
> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>      .class_init    = piix3_ide_class_init,
>  };
>  
> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>  static void piix4_ide_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 7edec5e149..0ab4787658 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -35,6 +35,7 @@
>  #include "hw/timer/i8254.h"
>  #include "hw/rtc/mc146818rtc.h"
>  #include "hw/ide.h"
> +#include "hw/ide/pci.h"
>  #include "migration/vmstate.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/runstate.h"
> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>          *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>      }
>  
> +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>      hd = g_new(DriveInfo *, ide_drives);
>      ide_drive_get(hd, ide_drives);
> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> +    pci_ide_create_devs(pci, hd);
>      g_free(hd);
> +

Why not move pci_create_simple down, and declare a new variable?
-    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
+    pci_dev = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
+    pci_ide_create_devs(pci_dev, hd);

makes it clearer what's going on imho.

>      pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>      if (smbus) {
>          *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
> diff --git a/include/hw/ide.h b/include/hw/ide.h
> index 883bbaeb9b..21bd8f23f1 100644
> --- a/include/hw/ide.h
> +++ b/include/hw/ide.h
> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>                          DriveInfo *hd0, DriveInfo *hd1);
>  
>  /* ide-pci.c */
> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>  int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>  
>  /* ide-mmio.c */
> -- 
> 2.21.1

Re: [PATCH 2/8] hw/ide: Get rid of piix4_init function

[BALATON Zoltan]

On Sun, 15 Mar 2020, Michael S. Tsirkin wrote:
> On Fri, Mar 13, 2020 at 10:14:34PM +0100, BALATON Zoltan wrote:
>> This removes pci_piix4_ide_init() function similar to clean up done to
>> other ide devices.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/piix.c    | 12 +-----------
>>  hw/isa/piix4.c   |  5 ++++-
>>  include/hw/ide.h |  1 -
>>  3 files changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index 8bcd6b72c2..3b2de4c312 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -208,17 +208,6 @@ static void pci_piix_ide_exitfn(PCIDevice *dev)
>>      }
>>  }
>>
>> -/* hd_table must contain 4 block drivers */
>> -/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
>> -{
>> -    PCIDevice *dev;
>> -
>> -    dev = pci_create_simple(bus, devfn, "piix4-ide");
>> -    pci_ide_create_devs(dev, hd_table);
>> -    return dev;
>> -}
>> -
>>  /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
>>  static void piix3_ide_class_init(ObjectClass *klass, void *data)
>>  {
>> @@ -247,6 +236,7 @@ static const TypeInfo piix3_ide_xen_info = {
>>      .class_init    = piix3_ide_class_init,
>>  };
>>
>> +/* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */
>>  static void piix4_ide_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>> index 7edec5e149..0ab4787658 100644
>> --- a/hw/isa/piix4.c
>> +++ b/hw/isa/piix4.c
>> @@ -35,6 +35,7 @@
>>  #include "hw/timer/i8254.h"
>>  #include "hw/rtc/mc146818rtc.h"
>>  #include "hw/ide.h"
>> +#include "hw/ide/pci.h"
>>  #include "migration/vmstate.h"
>>  #include "sysemu/reset.h"
>>  #include "sysemu/runstate.h"
>> @@ -255,10 +256,12 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
>>          *isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>      }
>>
>> +    pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
>>      hd = g_new(DriveInfo *, ide_drives);
>>      ide_drive_get(hd, ide_drives);
>> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
>> +    pci_ide_create_devs(pci, hd);
>>      g_free(hd);
>> +
>
> Why not move pci_create_simple down, and declare a new variable?
> -    pci_piix4_ide_init(pci_bus, hd, pci->devfn + 1);
> +    pci_dev = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
> +    pci_ide_create_devs(pci_dev, hd);
>
> makes it clearer what's going on imho.

Ends up there after patch 6. Do you still think a new variable would be 
needed for this after that patch? It's pretty simple and clear without all 
the hd array stuff even reusing pci variable. (Or I could rename pci to 
pci_dev but really don't think it worth having two once used variable in 
such a simple function. Normally such variables are called dev but in this 
function that name is taken for a DeviceState *variable.)

Regards,
BALATON Zoltan

>
>>      pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
>>      if (smbus) {
>>          *smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100,
>> diff --git a/include/hw/ide.h b/include/hw/ide.h
>> index 883bbaeb9b..21bd8f23f1 100644
>> --- a/include/hw/ide.h
>> +++ b/include/hw/ide.h
>> @@ -12,7 +12,6 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
>>                          DriveInfo *hd0, DriveInfo *hd1);
>>
>>  /* ide-pci.c */
>> -PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
>>  int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
>>
>>  /* ide-mmio.c */
>> --
>> 2.21.1
>
>

Re: [PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/03/20 23:16, BALATON Zoltan wrote:
>>>
>>> +    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
>>> +    pci_ide_create_devs(pci_dev);
>> 
>> Additionally, I think it may also make sense to move pci_ide_create_devs
>> call into the realize methods of these IDE controllers so boards do not
>> need to do it explicitely. These calls always follow the creation of the
>> device immediately so could just be done internally in IDE device and
>> simplify it further. I can attempt to prepare additional patches for
>> that but first I'd like to hear if anyone has anything against that to
>> avoid doing useless work.
>
> No, it's better to do it separately.  I think that otherwise you could
> add another IDE controller with -device, and both controllers would try
> to add the drives.

Correct.

Creating device frontends for -drive if=ide is the board's job.  Boards
may delegate to suitable helpers.  I'd very much prefer these helpers
not to live with device model code.  Board and device model code should
be cleanly separated to to reduce the temptation to muddle their
responsibilities.  It's separation of concerns.

I actually wish we had separate sub-trees for boards and devices instead
of keeping both in hw/.

> Basically, separating the call means that only automatically added
> controllers obey "if=ide".

Re: [PATCH 1/8] hw/ide: Get rid of piix3_init functions

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> This removes pci_piix3_ide_init() and pci_piix3_xen_ide_init()
> functions similar to clean up done to other ide devices.

Got a commit hash for "done to other ide devices"?

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Re: [PATCH 4/8] hw/ide: Move MAX_IDE_BUS define to one header

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> There are several definitions of MAX_IDE_BUS in different boards (some
> of them unused) with the same value. Move it to include/hw/ide/internal.h
> to have it in a central place.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

This one feels a bit questionable.

The number of (PATA) IDE buses provides by a host bus adapter depends on
the HBA.  It happens to be 2 for all HBAs we implement, but it could
really be anything.

Similar for SATA, where the common number is 6, but could really be
anything.  I can't see offhand whether any HBA we implement provides a
different number.

By moving MAX_IDE_BUS to include/hw/ide/internal.h, you bake the
accidental commonality into the interface to the IDE core.  I'd prefer
not to.

Re: [PATCH 0/8] Misc hw/ide legacy clean up

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> These are some clean ups to remove more legacy init functions and
> lessen dependence on include/hw/ide.h with some simplifications in
> board code. There should be no functional change.

PATCH 1 could quote precedence more clearly in the commit message, but
that's detail.

I don't like PATCH 4.

PATCH 1-3,5-8:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 4/8] hw/ide: Move MAX_IDE_BUS define to one header

[Philippe Mathieu-Daudé]

On 3/16/20 7:53 AM, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
> 
>> There are several definitions of MAX_IDE_BUS in different boards (some
>> of them unused) with the same value. Move it to include/hw/ide/internal.h
>> to have it in a central place.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> 
> This one feels a bit questionable.
> 
> The number of (PATA) IDE buses provides by a host bus adapter depends on
> the HBA.  It happens to be 2 for all HBAs we implement, but it could
> really be anything.
> 
> Similar for SATA, where the common number is 6, but could really be
> anything.  I can't see offhand whether any HBA we implement provides a
> different number.
> 
> By moving MAX_IDE_BUS to include/hw/ide/internal.h, you bake the
> accidental commonality into the interface to the IDE core.  I'd prefer
> not to.

I agree with Markus here (I kept this commit tagged because I was 
thinking the same but didn't know how to express it correctly. Thanks 
Markus!).

Re: [PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[Philippe Mathieu-Daudé]

On 3/16/20 7:23 AM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 13/03/20 23:16, BALATON Zoltan wrote:
>>>>
>>>> +    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
>>>> +    pci_ide_create_devs(pci_dev);
>>>
>>> Additionally, I think it may also make sense to move pci_ide_create_devs
>>> call into the realize methods of these IDE controllers so boards do not
>>> need to do it explicitely. These calls always follow the creation of the
>>> device immediately so could just be done internally in IDE device and
>>> simplify it further. I can attempt to prepare additional patches for
>>> that but first I'd like to hear if anyone has anything against that to
>>> avoid doing useless work.
>>
>> No, it's better to do it separately.  I think that otherwise you could
>> add another IDE controller with -device, and both controllers would try
>> to add the drives.
> 
> Correct.
> 
> Creating device frontends for -drive if=ide is the board's job.  Boards
> may delegate to suitable helpers.  I'd very much prefer these helpers
> not to live with device model code.  Board and device model code should
> be cleanly separated to to reduce the temptation to muddle their
> responsibilities.  It's separation of concerns.
> 
> I actually wish we had separate sub-trees for boards and devices instead
> of keeping both in hw/.

Never too late!

To be clear, you suggest:

- one dir with machines, boards, system-on-module
- one dir with devices, cpu, system-on-chips

Correct?

> 
>> Basically, separating the call means that only automatically added
>> controllers obey "if=ide".
> 

Re: [PATCH 1/8] hw/ide: Get rid of piix3_init functions

[BALATON Zoltan]

On Mon, 16 Mar 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> This removes pci_piix3_ide_init() and pci_piix3_xen_ide_init()
>> functions similar to clean up done to other ide devices.
>
> Got a commit hash for "done to other ide devices"?

Not yet in master, patches to CMD646 from Mark are queued in ide tree and 
I've submitted another one for via-ide as part of another series fixing 
that model which is also part of the latest version of that series revised 
by Mark. These are hopefully merged together so should appear close in log 
to be clear which these refer to.

Regards,
BALATON Zoltan

[PATCH v2] hw/ide: Move MAX_IDE_DEVS define to hw/ide/internal.h

[BALATON Zoltan]

We can move this define now that less files use it to internal.h to
further reduce dependency on hw/ide.h.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: Alternative version of patch 7 that applies without
    [PATCH 4/8] hw/ide: Move MAX_IDE_BUS define to one header

 include/hw/ide.h          | 2 --
 include/hw/ide/internal.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/ide.h b/include/hw/ide.h
index d52c211f32..c5ce5da4f4 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -4,8 +4,6 @@
 #include "hw/isa/isa.h"
 #include "exec/memory.h"
 
-#define MAX_IDE_DEVS	2
-
 /* ide-isa.c */
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 1bc1fc73e5..55da35d768 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -27,6 +27,8 @@ typedef struct IDEDMAOps IDEDMAOps;
 #define TYPE_IDE_BUS "IDE"
 #define IDE_BUS(obj) OBJECT_CHECK(IDEBus, (obj), TYPE_IDE_BUS)
 
+#define MAX_IDE_DEVS 2
+
 /* Bits of HD_STATUS */
 #define ERR_STAT		0x01
 #define INDEX_STAT		0x02
-- 
2.21.1

Re: [PATCH 0/8] Misc hw/ide legacy clean up

[BALATON Zoltan]

On Mon, 16 Mar 2020, Markus Armbruster wrote:
> BALATON Zoltan <balaton@eik.bme.hu> writes:
>> These are some clean ups to remove more legacy init functions and
>> lessen dependence on include/hw/ide.h with some simplifications in
>> board code. There should be no functional change.
>
> PATCH 1 could quote precedence more clearly in the commit message, but
> that's detail.
>
> I don't like PATCH 4.

Sent alternative v2 version of patch 7 so you can drop patch 4 if you 
like, the rest of the series should apply unchanged. Note that there might 
be some places where MAX_IDE_BUS is defined but not used and current code 
probably has assumption about this being 2 elsewhere and would break with 
any other value so other than philosophical there should be no reason to 
keep this defined everywhere.

> PATCH 1-3,5-8:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks.

Regards,
BALATON Zoltan

[PATCH v2] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[BALATON Zoltan]

The pci_ide_create_devs() function takes a hd_table parameter but all
callers just pass what ide_drive_get() returns so we can do it locally
simplifying callers and removing hd_table parameter.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: Alternative version that applies without [PATCH 4/8] hw/ide: Move
    MAX_IDE_BUS define to one header

 hw/alpha/dp264.c              | 13 +++----------
 hw/i386/pc_piix.c             |  9 +++++----
 hw/ide/pci.c                  |  4 +++-
 hw/isa/piix4.c                | 10 ++--------
 hw/mips/mips_fulong2e.c       |  4 +---
 hw/mips/mips_malta.c          |  2 +-
 hw/sparc64/sun4u.c            |  6 +-----
 include/hw/ide/pci.h          |  2 +-
 include/hw/southbridge/piix.h |  3 +--
 9 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 27595767e5..f7751b18f6 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -15,7 +15,6 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/timer/i8254.h"
 #include "hw/isa/superio.h"
@@ -58,6 +57,7 @@ static void clipper_init(MachineState *machine)
     const char *initrd_filename = machine->initrd_filename;
     AlphaCPU *cpus[4];
     PCIBus *pci_bus;
+    PCIDevice *pci_dev;
     ISABus *isa_bus;
     qemu_irq rtc_irq;
     long size, i;
@@ -100,15 +100,8 @@ static void clipper_init(MachineState *machine)
     isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
 
     /* IDE disk setup.  */
-    {
-        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
-        PCIDevice *pci_dev;
-
-        ide_drive_get(hd, ARRAY_SIZE(hd));
-
-        pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
-        pci_ide_create_devs(pci_dev, hd);
-    }
+    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
+    pci_ide_create_devs(pci_dev);
 
     /* Load PALcode.  Given that this is not "real" cpu palcode,
        but one explicitly written for the emulation, we might as
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c399398739..9216596ec6 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -86,7 +86,6 @@ static void pc_init1(MachineState *machine,
     int piix3_devfn = -1;
     qemu_irq smi_irq;
     GSIState *gsi_state;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
     MemoryRegion *ram_memory;
@@ -240,20 +239,22 @@ static void pc_init1(MachineState *machine,
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
 
-    ide_drive_get(hd, ARRAY_SIZE(hd));
     if (pcmc->pci_enabled) {
         PCIDevice *dev;
 
         dev = pci_create_simple(pci_bus, piix3_devfn + 1,
                                 xen_enabled() ? "piix3-ide-xen" : "piix3-ide");
-        pci_ide_create_devs(dev, hd);
+        pci_ide_create_devs(dev);
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
         pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
     }
 #ifdef CONFIG_IDE_ISA
-else {
+    else {
+        DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
         int i;
+
+        ide_drive_get(hd, ARRAY_SIZE(hd));
         for (i = 0; i < MAX_IDE_BUS; i++) {
             ISADevice *dev;
             char busname[] = "ide.0";
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index e0c84392e2..97347f07f1 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -477,13 +477,15 @@ const VMStateDescription vmstate_ide_pci = {
 };
 
 /* hd_table must contain 4 block drivers */
-void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
+void pci_ide_create_devs(PCIDevice *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
+    DriveInfo *hd_table[2 * MAX_IDE_DEVS];
     static const int bus[4]  = { 0, 0, 1, 1 };
     static const int unit[4] = { 0, 1, 0, 1 };
     int i;
 
+    ide_drive_get(hd_table, ARRAY_SIZE(hd_table));
     for (i = 0; i < 4; i++) {
         if (hd_table[i]) {
             ide_create_drive(d->bus + bus[i], unit[i], hd_table[i]);
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 0ab4787658..13fa1660c3 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -241,11 +241,8 @@ static void piix4_register_types(void)
 
 type_init(piix4_register_types)
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
-                          I2CBus **smbus, size_t ide_buses)
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus)
 {
-    size_t ide_drives = ide_buses * MAX_IDE_DEVS;
-    DriveInfo **hd;
     PCIDevice *pci;
     DeviceState *dev;
 
@@ -257,10 +254,7 @@ DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
     }
 
     pci = pci_create_simple(pci_bus, pci->devfn + 1, "piix4-ide");
-    hd = g_new(DriveInfo *, ide_drives);
-    ide_drive_get(hd, ide_drives);
-    pci_ide_create_devs(pci, hd);
-    g_free(hd);
+    pci_ide_create_devs(pci);
 
     pci_create_simple(pci_bus, pci->devfn + 2, "piix4-usb-uhci");
     if (smbus) {
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 639ba2a091..0f312b5a35 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -239,7 +239,6 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
 {
     qemu_irq *i8259;
     ISABus *isa_bus;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     PCIDevice *dev;
 
     isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
@@ -259,8 +258,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
 
     dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
-    ide_drive_get(hd, ARRAY_SIZE(hd));
-    pci_ide_create_devs(dev, hd);
+    pci_ide_create_devs(dev);
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index d380f73d7b..e4c4de1b4e 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1403,7 +1403,7 @@ void mips_malta_init(MachineState *machine)
     pci_bus = gt64120_register(s->i8259);
 
     /* Southbridge */
-    dev = piix4_create(pci_bus, &isa_bus, &smbus, MAX_IDE_BUS);
+    dev = piix4_create(pci_bus, &isa_bus, &smbus);
 
     /* Interrupt controller */
     qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d33e84f831..6abfcb30f8 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -50,7 +50,6 @@
 #include "hw/sparc/sparc64.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
-#include "hw/ide.h"
 #include "hw/ide/pci.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
@@ -563,7 +562,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     PCIBus *pci_bus, *pci_busA, *pci_busB;
     PCIDevice *ebus, *pci_dev;
     SysBusDevice *s;
-    DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     DeviceState *iommu, *dev;
     FWCfgState *fw_cfg;
     NICInfo *nd;
@@ -663,12 +661,10 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
         qemu_macaddr_default_if_unset(&macaddr);
     }
 
-    ide_drive_get(hd, ARRAY_SIZE(hd));
-
     pci_dev = pci_create(pci_busA, PCI_DEVFN(3, 0), "cmd646-ide");
     qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
     qdev_init_nofail(&pci_dev->qdev);
-    pci_ide_create_devs(pci_dev, hd);
+    pci_ide_create_devs(pci_dev);
 
     /* Map NVRAM into I/O (ebus) space */
     nvram = m48t59_init(NULL, 0, 0, NVRAM_SIZE, 1968, 59);
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 98ffa7dfcd..dd504e5a0b 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -63,7 +63,7 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
 void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
-void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table);
+void pci_ide_create_devs(PCIDevice *dev);
 
 extern const VMStateDescription vmstate_ide_pci;
 extern const MemoryRegionOps pci_ide_cmd_le_ops;
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index 152628c6d9..02bd741209 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -68,7 +68,6 @@ extern PCIDevice *piix4_dev;
 
 PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
 
-DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus,
-                          I2CBus **smbus, size_t ide_buses);
+DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
 
 #endif
-- 
2.21.1

Re: [PATCH 0/8] Misc hw/ide legacy clean up

[BALATON Zoltan]

On Mon, 16 Mar 2020, BALATON Zoltan wrote:
> On Mon, 16 Mar 2020, Markus Armbruster wrote:
>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>> These are some clean ups to remove more legacy init functions and
>>> lessen dependence on include/hw/ide.h with some simplifications in
>>> board code. There should be no functional change.
>> 
>> PATCH 1 could quote precedence more clearly in the commit message, but
>> that's detail.
>> 
>> I don't like PATCH 4.
>
> Sent alternative v2 version of patch 7 so you can drop patch 4 if you like,

and patch 6 v2 also sent that is affected as well if you drop patch 4.

> the rest of the series should apply unchanged. Note that there might be some 
> places where MAX_IDE_BUS is defined but not used and current code probably 
> has assumption about this being 2 elsewhere and would break with any other 
> value so other than philosophical there should be no reason to keep this 
> defined everywhere.
>
>> PATCH 1-3,5-8:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks.
>
> Regards,
> BALATON Zoltan
>

Re: [PATCH 6/8] hw/ide: Do ide_drive_get() within pci_ide_create_devs()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 3/16/20 7:23 AM, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 13/03/20 23:16, BALATON Zoltan wrote:
>>>>>
>>>>> +    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
>>>>> +    pci_ide_create_devs(pci_dev);
>>>>
>>>> Additionally, I think it may also make sense to move pci_ide_create_devs
>>>> call into the realize methods of these IDE controllers so boards do not
>>>> need to do it explicitely. These calls always follow the creation of the
>>>> device immediately so could just be done internally in IDE device and
>>>> simplify it further. I can attempt to prepare additional patches for
>>>> that but first I'd like to hear if anyone has anything against that to
>>>> avoid doing useless work.
>>>
>>> No, it's better to do it separately.  I think that otherwise you could
>>> add another IDE controller with -device, and both controllers would try
>>> to add the drives.
>>
>> Correct.
>>
>> Creating device frontends for -drive if=ide is the board's job.  Boards
>> may delegate to suitable helpers.  I'd very much prefer these helpers
>> not to live with device model code.  Board and device model code should
>> be cleanly separated to to reduce the temptation to muddle their
>> responsibilities.  It's separation of concerns.
>>
>> I actually wish we had separate sub-trees for boards and devices instead
>> of keeping both in hw/.
>
> Never too late!
>
> To be clear, you suggest:
>
> - one dir with machines, boards, system-on-module
> - one dir with devices, cpu, system-on-chips
>
> Correct?

In QOM terms:

* One sub-tree with descendants of TYPE_DEVICE
* One sub-tree with descendants of TYPE_MACHINE

Re: [PATCH 0/8] Misc hw/ide legacy clean up

[John Snow]

On 3/16/20 9:41 AM, BALATON Zoltan wrote:
> On Mon, 16 Mar 2020, BALATON Zoltan wrote:
>> On Mon, 16 Mar 2020, Markus Armbruster wrote:
>>> BALATON Zoltan <balaton@eik.bme.hu> writes:
>>>> These are some clean ups to remove more legacy init functions and
>>>> lessen dependence on include/hw/ide.h with some simplifications in
>>>> board code. There should be no functional change.
>>>
>>> PATCH 1 could quote precedence more clearly in the commit message, but
>>> that's detail.
>>>
>>> I don't like PATCH 4.
>>
>> Sent alternative v2 version of patch 7 so you can drop patch 4 if you
>> like,
> 
> and patch 6 v2 also sent that is affected as well if you drop patch 4.
> 
>> the rest of the series should apply unchanged. Note that there might
>> be some places where MAX_IDE_BUS is defined but not used and current
>> code probably has assumption about this being 2 elsewhere and would
>> break with any other value so other than philosophical there should be
>> no reason to keep this defined everywhere.
>>
>>> PATCH 1-3,5-8:
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>> Thanks.
>>
>> Regards,
>> BALATON Zoltan
>>
> 

Can you do me a favor and send a proper v2 of the whole series, with
review tags applied?

--js