[PATCH 0/7] hw/ide: Clean up hw/ide/qdev.c and include/hw/ide/internal.h

[PATCH 0/7] hw/ide: Clean up hw/ide/qdev.c and include/hw/ide/internal.h

[Thomas Huth]

While trying to make it possible to compile-out the CompactFlash IDE device
in downstream distributions (first patch), we noticed that there are more
things in the IDE code that could use a proper clean up:

First, hw/ide/qdev.c is quite a mix between IDE BUS specific functions
and (disk) device specific functions. Thus the second patch splits qdev.c
into two new separate files to make it more obvious which part belongs
to which kind of devices.

The remaining patches unentangle include/hw/ide/internal.h, which is meant
as a header that should only be used internally to the IDE subsystem, but
which is currently exposed to the world since include/hw/ide/pci.h includes
this header, too. Thus we move the definitions that are also required for
non-IDE code to other new header files, so we can finally change pci.h to
stop including internal.h. After these changes, internal.h is only included
by files in hw/ide/ as it should be.

Thomas Huth (7):
  hw/ide: Add the possibility to disable the CompactFlash device in the
    build
  hw/ide: Split qdev.c into ide-bus.c and ide-dev.c
  hw/ide: Move IDE device related definitions to ide-dev.h
  hw/ide: Move IDE bus related definitions to a new header ide-bus.h
  hw/ide: Move IDE DMA related definitions to a separate header
    ide-dma.h
  hw/ide: Remove the include/hw/ide.h legacy file
  hw/ide: Stop exposing internal.h to non-IDE files

 include/hw/ide.h             |   9 --
 include/hw/ide/ide-bus.h     |  41 +++++++
 include/hw/ide/ide-dev.h     | 186 +++++++++++++++++++++++++++++++
 include/hw/ide/ide-dma.h     |  30 +++++
 include/hw/ide/internal.h    | 209 +----------------------------------
 include/hw/ide/pci.h         |   3 +-
 hw/i386/pc.c                 |   2 +-
 hw/ide/cf.c                  |  58 ++++++++++
 hw/ide/cmd646.c              |   1 +
 hw/ide/ide-bus.c             | 111 +++++++++++++++++++
 hw/ide/{qdev.c => ide-dev.c} | 137 +----------------------
 hw/ide/pci.c                 |   1 +
 hw/ide/piix.c                |   1 +
 hw/ide/sii3112.c             |   1 +
 hw/ide/via.c                 |   1 +
 hw/arm/Kconfig               |   2 +
 hw/ide/Kconfig               |  32 ++++--
 hw/ide/meson.build           |   4 +-
 18 files changed, 465 insertions(+), 364 deletions(-)
 delete mode 100644 include/hw/ide.h
 create mode 100644 include/hw/ide/ide-bus.h
 create mode 100644 include/hw/ide/ide-dev.h
 create mode 100644 include/hw/ide/ide-dma.h
 create mode 100644 hw/ide/cf.c
 create mode 100644 hw/ide/ide-bus.c
 rename hw/ide/{qdev.c => ide-dev.c} (67%)

-- 
2.43.2

[PATCH 1/7] hw/ide: Add the possibility to disable the CompactFlash device in the build

[Thomas Huth]

For distros like downstream RHEL, it would be helpful to allow to disable
the CompactFlash device. For making this possible, we need a separate
Kconfig switch for this device, and the code should reside in a separate
file. Let's also introduce a new header ide-dev.h which can be used to
collect definitions related to IDE devices.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ide/ide-dev.h | 41 ++++++++++++++++++++++++++++
 hw/ide/cf.c              | 58 ++++++++++++++++++++++++++++++++++++++++
 hw/ide/qdev.c            | 51 ++---------------------------------
 hw/ide/Kconfig           |  4 +++
 hw/ide/meson.build       |  1 +
 5 files changed, 106 insertions(+), 49 deletions(-)
 create mode 100644 include/hw/ide/ide-dev.h
 create mode 100644 hw/ide/cf.c

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
new file mode 100644
index 0000000000..7e9663cda9
--- /dev/null
+++ b/include/hw/ide/ide-dev.h
@@ -0,0 +1,41 @@
+/*
+ * ide device definitions
+ *
+ * Copyright (c) 2009 Gerd Hoffmann <kraxel@redhat.com>
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef IDE_DEV_H
+#define IDE_DEV_H
+
+#include "hw/qdev-properties.h"
+#include "hw/block/block.h"
+#include "hw/ide/internal.h"
+
+typedef struct IDEDrive {
+    IDEDevice dev;
+} IDEDrive;
+
+#define DEFINE_IDE_DEV_PROPERTIES()                     \
+    DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
+    DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
+    DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+    DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
+    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
+    DEFINE_PROP_STRING("model", IDEDrive, dev.model)
+
+void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
+
+#endif
diff --git a/hw/ide/cf.c b/hw/ide/cf.c
new file mode 100644
index 0000000000..2a425cb0f2
--- /dev/null
+++ b/hw/ide/cf.c
@@ -0,0 +1,58 @@
+/*
+ * ide CompactFlash support
+ *
+ * This code is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/ide/ide-dev.h"
+#include "qapi/qapi-types-block.h"
+
+static void ide_cf_realize(IDEDevice *dev, Error **errp)
+{
+    ide_dev_initfn(dev, IDE_CFATA, errp);
+}
+
+static Property ide_cf_properties[] = {
+    DEFINE_IDE_DEV_PROPERTIES(),
+    DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
+    DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
+                IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ide_cf_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
+
+    k->realize  = ide_cf_realize;
+    dc->fw_name = "drive";
+    dc->desc    = "virtual CompactFlash card";
+    device_class_set_props(dc, ide_cf_properties);
+}
+
+static const TypeInfo ide_cf_info = {
+    .name          = "ide-cf",
+    .parent        = TYPE_IDE_DEVICE,
+    .instance_size = sizeof(IDEDrive),
+    .class_init    = ide_cf_class_init,
+};
+
+static void ide_cf_register_type(void)
+{
+    type_register_static(&ide_cf_info);
+}
+
+type_init(ide_cf_register_type)
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 1b3b4da01d..4189313d30 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -24,12 +24,9 @@
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
-#include "hw/ide/internal.h"
-#include "hw/qdev-properties.h"
-#include "hw/qdev-properties-system.h"
+#include "hw/ide/ide-dev.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
-#include "hw/block/block.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 #include "qapi/visitor.h"
@@ -158,11 +155,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit)
 
 /* --------------------------------- */
 
-typedef struct IDEDrive {
-    IDEDevice dev;
-} IDEDrive;
-
-static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
+void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
     IDEState *s = bus->ifs + dev->unit;
@@ -283,19 +276,6 @@ static void ide_cd_realize(IDEDevice *dev, Error **errp)
     ide_dev_initfn(dev, IDE_CD, errp);
 }
 
-static void ide_cf_realize(IDEDevice *dev, Error **errp)
-{
-    ide_dev_initfn(dev, IDE_CFATA, errp);
-}
-
-#define DEFINE_IDE_DEV_PROPERTIES()                     \
-    DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
-    DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
-    DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
-    DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),   \
-    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
-    DEFINE_PROP_STRING("model", IDEDrive, dev.model)
-
 static Property ide_hd_properties[] = {
     DEFINE_IDE_DEV_PROPERTIES(),
     DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
@@ -346,32 +326,6 @@ static const TypeInfo ide_cd_info = {
     .class_init    = ide_cd_class_init,
 };
 
-static Property ide_cf_properties[] = {
-    DEFINE_IDE_DEV_PROPERTIES(),
-    DEFINE_BLOCK_CHS_PROPERTIES(IDEDrive, dev.conf),
-    DEFINE_PROP_BIOS_CHS_TRANS("bios-chs-trans",
-                IDEDrive, dev.chs_trans, BIOS_ATA_TRANSLATION_AUTO),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void ide_cf_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
-
-    k->realize  = ide_cf_realize;
-    dc->fw_name = "drive";
-    dc->desc    = "virtual CompactFlash card";
-    device_class_set_props(dc, ide_cf_properties);
-}
-
-static const TypeInfo ide_cf_info = {
-    .name          = "ide-cf",
-    .parent        = TYPE_IDE_DEVICE,
-    .instance_size = sizeof(IDEDrive),
-    .class_init    = ide_cf_class_init,
-};
-
 static void ide_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
@@ -396,7 +350,6 @@ static void ide_register_types(void)
     type_register_static(&ide_bus_info);
     type_register_static(&ide_hd_info);
     type_register_static(&ide_cd_info);
-    type_register_static(&ide_cf_info);
     type_register_static(&ide_device_type_info);
 }
 
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index dd85fa3619..b93d6743d5 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -57,3 +57,7 @@ config IDE_SII3112
     bool
     select IDE_PCI
     select IDE_QDEV
+
+config IDE_CF
+    bool
+    default y if IDE_QDEV
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index e050eef942..d2e5b45c9e 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -1,6 +1,7 @@
 system_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
 system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
 system_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
+system_ss.add(when: 'CONFIG_IDE_CF', if_true: files('cf.c'))
 system_ss.add(when: 'CONFIG_IDE_CMD646', if_true: files('cmd646.c'))
 system_ss.add(when: 'CONFIG_IDE_CORE', if_true: files('core.c', 'atapi.c'))
 system_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c', 'ioport.c'))
-- 
2.43.2

[PATCH 2/7] hw/ide: Split qdev.c into ide-bus.c and ide-dev.c

[Thomas Huth]

qdev.c is a mixture between IDE bus specific functions and IDE device
functions. Let's split it up to make it more obvious which part is
related to bus handling and which part is related to device handling.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ide/ide-bus.c             | 111 +++++++++++++++++++++++++++++++++++
 hw/ide/{qdev.c => ide-dev.c} |  87 +--------------------------
 hw/arm/Kconfig               |   2 +
 hw/ide/Kconfig               |  30 ++++++----
 hw/ide/meson.build           |   3 +-
 5 files changed, 134 insertions(+), 99 deletions(-)
 create mode 100644 hw/ide/ide-bus.c
 rename hw/ide/{qdev.c => ide-dev.c} (78%)

diff --git a/hw/ide/ide-bus.c b/hw/ide/ide-bus.c
new file mode 100644
index 0000000000..57fe67b29c
--- /dev/null
+++ b/hw/ide/ide-bus.c
@@ -0,0 +1,111 @@
+/*
+ * ide bus support for qdev.
+ *
+ * Copyright (c) 2009 Gerd Hoffmann <kraxel@redhat.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/module.h"
+#include "hw/ide/internal.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/runstate.h"
+
+static char *idebus_get_fw_dev_path(DeviceState *dev);
+static void idebus_unrealize(BusState *qdev);
+
+static void ide_bus_class_init(ObjectClass *klass, void *data)
+{
+    BusClass *k = BUS_CLASS(klass);
+
+    k->get_fw_dev_path = idebus_get_fw_dev_path;
+    k->unrealize = idebus_unrealize;
+}
+
+static void idebus_unrealize(BusState *bus)
+{
+    IDEBus *ibus = IDE_BUS(bus);
+
+    if (ibus->vmstate) {
+        qemu_del_vm_change_state_handler(ibus->vmstate);
+    }
+}
+
+static const TypeInfo ide_bus_info = {
+    .name = TYPE_IDE_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(IDEBus),
+    .class_init = ide_bus_class_init,
+};
+
+void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
+                 int bus_id, int max_units)
+{
+    qbus_init(idebus, idebus_size, TYPE_IDE_BUS, dev, NULL);
+    idebus->bus_id = bus_id;
+    idebus->max_units = max_units;
+}
+
+static char *idebus_get_fw_dev_path(DeviceState *dev)
+{
+    char path[30];
+
+    snprintf(path, sizeof(path), "%s@%x", qdev_fw_name(dev),
+             ((IDEBus *)dev->parent_bus)->bus_id);
+
+    return g_strdup(path);
+}
+
+IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
+{
+    DeviceState *dev;
+
+    dev = qdev_new(drive->media_cd ? "ide-cd" : "ide-hd");
+    qdev_prop_set_uint32(dev, "unit", unit);
+    qdev_prop_set_drive_err(dev, "drive", blk_by_legacy_dinfo(drive),
+                            &error_fatal);
+    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
+    return DO_UPCAST(IDEDevice, qdev, dev);
+}
+
+int ide_get_geometry(BusState *bus, int unit,
+                     int16_t *cyls, int8_t *heads, int8_t *secs)
+{
+    IDEState *s = &DO_UPCAST(IDEBus, qbus, bus)->ifs[unit];
+
+    if (s->drive_kind != IDE_HD || !s->blk) {
+        return -1;
+    }
+
+    *cyls = s->cylinders;
+    *heads = s->heads;
+    *secs = s->sectors;
+    return 0;
+}
+
+int ide_get_bios_chs_trans(BusState *bus, int unit)
+{
+    return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
+}
+
+static void ide_bus_register_type(void)
+{
+    type_register_static(&ide_bus_info);
+}
+
+type_init(ide_bus_register_type)
diff --git a/hw/ide/qdev.c b/hw/ide/ide-dev.c
similarity index 78%
rename from hw/ide/qdev.c
rename to hw/ide/ide-dev.c
index 4189313d30..15d088fd06 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/ide-dev.c
@@ -1,5 +1,5 @@
 /*
- * ide bus support for qdev.
+ * IDE device functions
  *
  * Copyright (c) 2009 Gerd Hoffmann <kraxel@redhat.com>
  *
@@ -18,71 +18,21 @@
  */
 
 #include "qemu/osdep.h"
-#include "sysemu/dma.h"
 #include "qapi/error.h"
 #include "qapi/qapi-types-block.h"
 #include "qemu/error-report.h"
-#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "hw/ide/ide-dev.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
-#include "sysemu/runstate.h"
 #include "qapi/visitor.h"
 
-/* --------------------------------- */
-
-static char *idebus_get_fw_dev_path(DeviceState *dev);
-static void idebus_unrealize(BusState *qdev);
-
 static Property ide_props[] = {
     DEFINE_PROP_UINT32("unit", IDEDevice, unit, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void ide_bus_class_init(ObjectClass *klass, void *data)
-{
-    BusClass *k = BUS_CLASS(klass);
-
-    k->get_fw_dev_path = idebus_get_fw_dev_path;
-    k->unrealize = idebus_unrealize;
-}
-
-static void idebus_unrealize(BusState *bus)
-{
-    IDEBus *ibus = IDE_BUS(bus);
-
-    if (ibus->vmstate) {
-        qemu_del_vm_change_state_handler(ibus->vmstate);
-    }
-}
-
-static const TypeInfo ide_bus_info = {
-    .name = TYPE_IDE_BUS,
-    .parent = TYPE_BUS,
-    .instance_size = sizeof(IDEBus),
-    .class_init = ide_bus_class_init,
-};
-
-void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
-                 int bus_id, int max_units)
-{
-    qbus_init(idebus, idebus_size, TYPE_IDE_BUS, dev, NULL);
-    idebus->bus_id = bus_id;
-    idebus->max_units = max_units;
-}
-
-static char *idebus_get_fw_dev_path(DeviceState *dev)
-{
-    char path[30];
-
-    snprintf(path, sizeof(path), "%s@%x", qdev_fw_name(dev),
-             ((IDEBus*)dev->parent_bus)->bus_id);
-
-    return g_strdup(path);
-}
-
 static void ide_qdev_realize(DeviceState *qdev, Error **errp)
 {
     IDEDevice *dev = IDE_DEVICE(qdev);
@@ -121,40 +71,6 @@ static void ide_qdev_realize(DeviceState *qdev, Error **errp)
     dc->realize(dev, errp);
 }
 
-IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
-{
-    DeviceState *dev;
-
-    dev = qdev_new(drive->media_cd ? "ide-cd" : "ide-hd");
-    qdev_prop_set_uint32(dev, "unit", unit);
-    qdev_prop_set_drive_err(dev, "drive", blk_by_legacy_dinfo(drive),
-                            &error_fatal);
-    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
-    return DO_UPCAST(IDEDevice, qdev, dev);
-}
-
-int ide_get_geometry(BusState *bus, int unit,
-                     int16_t *cyls, int8_t *heads, int8_t *secs)
-{
-    IDEState *s = &DO_UPCAST(IDEBus, qbus, bus)->ifs[unit];
-
-    if (s->drive_kind != IDE_HD || !s->blk) {
-        return -1;
-    }
-
-    *cyls = s->cylinders;
-    *heads = s->heads;
-    *secs = s->sectors;
-    return 0;
-}
-
-int ide_get_bios_chs_trans(BusState *bus, int unit)
-{
-    return DO_UPCAST(IDEBus, qbus, bus)->ifs[unit].chs_trans;
-}
-
-/* --------------------------------- */
-
 void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
@@ -347,7 +263,6 @@ static const TypeInfo ide_device_type_info = {
 
 static void ide_register_types(void)
 {
-    type_register_static(&ide_bus_info);
     type_register_static(&ide_hd_info);
     type_register_static(&ide_cd_info);
     type_register_static(&ide_device_type_info);
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 29abe1da29..b372b819a4 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -275,6 +275,8 @@ config SBSA_REF
     select USB_XHCI_SYSBUS
     select WDT_SBSA
     select BOCHS_DISPLAY
+    select IDE_BUS
+    select IDE_DEV
 
 config SABRELITE
     bool
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index b93d6743d5..6dfc5a2129 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -1,51 +1,58 @@
 config IDE_CORE
     bool
 
-config IDE_QDEV
+config IDE_BUS
     bool
     select IDE_CORE
 
+config IDE_DEV
+    bool
+    depends on IDE_BUS
+
 config IDE_PCI
     bool
     depends on PCI
-    select IDE_QDEV
+    select IDE_BUS
+    select IDE_DEV
 
 config IDE_ISA
     bool
     depends on ISA_BUS
-    select IDE_QDEV
+    select IDE_BUS
+    select IDE_DEV
 
 config IDE_PIIX
     bool
     select IDE_PCI
-    select IDE_QDEV
 
 config IDE_CMD646
     bool
     select IDE_PCI
-    select IDE_QDEV
 
 config IDE_MACIO
     bool
-    select IDE_QDEV
+    select IDE_BUS
+    select IDE_DEV
 
 config IDE_MMIO
     bool
-    select IDE_QDEV
+    select IDE_BUS
+    select IDE_DEV
 
 config IDE_VIA
     bool
     select IDE_PCI
-    select IDE_QDEV
 
 config MICRODRIVE
     bool
-    select IDE_QDEV
+    select IDE_BUS
+    select IDE_DEV
     depends on PCMCIA
 
 config AHCI
     bool
-    select IDE_QDEV
+    select IDE_BUS
+    select IDE_DEV
 
 config AHCI_ICH9
     bool
@@ -56,8 +63,7 @@ config AHCI_ICH9
 config IDE_SII3112
     bool
     select IDE_PCI
-    select IDE_QDEV
 
 config IDE_CF
     bool
-    default y if IDE_QDEV
+    default y if IDE_BUS
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index d2e5b45c9e..d09705cac0 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -1,15 +1,16 @@
 system_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
 system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
 system_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
+system_ss.add(when: 'CONFIG_IDE_BUS', if_true: files('ide-bus.c'))
 system_ss.add(when: 'CONFIG_IDE_CF', if_true: files('cf.c'))
 system_ss.add(when: 'CONFIG_IDE_CMD646', if_true: files('cmd646.c'))
 system_ss.add(when: 'CONFIG_IDE_CORE', if_true: files('core.c', 'atapi.c'))
+system_ss.add(when: 'CONFIG_IDE_DEV', if_true: files('ide-dev.c'))
 system_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c', 'ioport.c'))
 system_ss.add(when: 'CONFIG_IDE_MACIO', if_true: files('macio.c'))
 system_ss.add(when: 'CONFIG_IDE_MMIO', if_true: files('mmio.c'))
 system_ss.add(when: 'CONFIG_IDE_PCI', if_true: files('pci.c'))
 system_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c'))
-system_ss.add(when: 'CONFIG_IDE_QDEV', if_true: files('qdev.c'))
 system_ss.add(when: 'CONFIG_IDE_SII3112', if_true: files('sii3112.c'))
 system_ss.add(when: 'CONFIG_IDE_VIA', if_true: files('via.c'))
 system_ss.add(when: 'CONFIG_MICRODRIVE', if_true: files('microdrive.c'))
-- 
2.43.2

[PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Thomas Huth]

Let's start to unentangle internal.h by moving public IDE device
related definitions to ide-dev.h.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ide/ide-dev.h  | 145 +++++++++++++++++++++++++++++++++++++-
 include/hw/ide/internal.h | 145 +-------------------------------------
 hw/ide/ide-dev.c          |   1 +
 3 files changed, 146 insertions(+), 145 deletions(-)

diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index 7e9663cda9..de88784a25 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -20,9 +20,152 @@
 #ifndef IDE_DEV_H
 #define IDE_DEV_H
 
+#include "sysemu/dma.h"
 #include "hw/qdev-properties.h"
 #include "hw/block/block.h"
-#include "hw/ide/internal.h"
+
+typedef struct IDEDevice IDEDevice;
+typedef struct IDEState IDEState;
+typedef struct IDEDMA IDEDMA;
+typedef struct IDEDMAOps IDEDMAOps;
+typedef struct IDEBus IDEBus;
+
+typedef void EndTransferFunc(IDEState *);
+
+#define MAX_IDE_DEVS 2
+
+#define TYPE_IDE_DEVICE "ide-device"
+OBJECT_DECLARE_TYPE(IDEDevice, IDEDeviceClass, IDE_DEVICE)
+
+typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
+
+struct unreported_events {
+    bool eject_request;
+    bool new_media;
+};
+
+enum ide_dma_cmd {
+    IDE_DMA_READ = 0,
+    IDE_DMA_WRITE,
+    IDE_DMA_TRIM,
+    IDE_DMA_ATAPI,
+    IDE_DMA__COUNT
+};
+
+/* NOTE: IDEState represents in fact one drive */
+struct IDEState {
+    IDEBus *bus;
+    uint8_t unit;
+    /* ide config */
+    IDEDriveKind drive_kind;
+    int drive_heads, drive_sectors;
+    int cylinders, heads, sectors, chs_trans;
+    int64_t nb_sectors;
+    int mult_sectors;
+    int identify_set;
+    uint8_t identify_data[512];
+    int drive_serial;
+    char drive_serial_str[21];
+    char drive_model_str[41];
+    uint64_t wwn;
+    /* ide regs */
+    uint8_t feature;
+    uint8_t error;
+    uint32_t nsector;
+    uint8_t sector;
+    uint8_t lcyl;
+    uint8_t hcyl;
+    /* other part of tf for lba48 support */
+    uint8_t hob_feature;
+    uint8_t hob_nsector;
+    uint8_t hob_sector;
+    uint8_t hob_lcyl;
+    uint8_t hob_hcyl;
+
+    uint8_t select;
+    uint8_t status;
+
+    bool io8;
+    bool reset_reverts;
+
+    /* set for lba48 access */
+    uint8_t lba48;
+    BlockBackend *blk;
+    char version[9];
+    /* ATAPI specific */
+    struct unreported_events events;
+    uint8_t sense_key;
+    uint8_t asc;
+    bool tray_open;
+    bool tray_locked;
+    uint8_t cdrom_changed;
+    int packet_transfer_size;
+    int elementary_transfer_size;
+    int32_t io_buffer_index;
+    int lba;
+    int cd_sector_size;
+    int atapi_dma; /* true if dma is requested for the packet cmd */
+    BlockAcctCookie acct;
+    BlockAIOCB *pio_aiocb;
+    QEMUIOVector qiov;
+    QLIST_HEAD(, IDEBufferedRequest) buffered_requests;
+    /* ATA DMA state */
+    uint64_t io_buffer_offset;
+    int32_t io_buffer_size;
+    QEMUSGList sg;
+    /* PIO transfer handling */
+    int req_nb_sectors; /* number of sectors per interrupt */
+    EndTransferFunc *end_transfer_func;
+    uint8_t *data_ptr;
+    uint8_t *data_end;
+    uint8_t *io_buffer;
+    /* PIO save/restore */
+    int32_t io_buffer_total_len;
+    int32_t cur_io_buffer_offset;
+    int32_t cur_io_buffer_len;
+    uint8_t end_transfer_fn_idx;
+    QEMUTimer *sector_write_timer; /* only used for win2k install hack */
+    uint32_t irq_count; /* counts IRQs when using win2k install hack */
+    /* CF-ATA extended error */
+    uint8_t ext_error;
+    /* CF-ATA metadata storage */
+    uint32_t mdata_size;
+    uint8_t *mdata_storage;
+    int media_changed;
+    enum ide_dma_cmd dma_cmd;
+    /* SMART */
+    uint8_t smart_enabled;
+    uint8_t smart_autosave;
+    int smart_errors;
+    uint8_t smart_selftest_count;
+    uint8_t *smart_selftest_data;
+    /* AHCI */
+    int ncq_queues;
+};
+
+struct IDEDeviceClass {
+    DeviceClass parent_class;
+    void (*realize)(IDEDevice *dev, Error **errp);
+};
+
+struct IDEDevice {
+    DeviceState qdev;
+    uint32_t unit;
+    BlockConf conf;
+    int chs_trans;
+    char *version;
+    char *serial;
+    char *model;
+    uint64_t wwn;
+    /*
+     * 0x0000        - rotation rate not reported
+     * 0x0001        - non-rotating medium (SSD)
+     * 0x0002-0x0400 - reserved
+     * 0x0401-0xffe  - rotations per minute
+     * 0xffff        - reserved
+     */
+    uint16_t rotation_rate;
+};
 
 typedef struct IDEDrive {
     IDEDevice dev;
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 3bdcc75597..5cc109fe82 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -8,24 +8,16 @@
  */
 
 #include "hw/ide.h"
-#include "sysemu/dma.h"
-#include "hw/block/block.h"
 #include "exec/ioport.h"
+#include "hw/ide/ide-dev.h"
 
 /* debug IDE devices */
 #define USE_DMA_CDROM
 #include "qom/object.h"
 
-typedef struct IDEDevice IDEDevice;
-typedef struct IDEState IDEState;
-typedef struct IDEDMA IDEDMA;
-typedef struct IDEDMAOps IDEDMAOps;
-
 #define TYPE_IDE_BUS "IDE"
 OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
 
-#define MAX_IDE_DEVS 2
-
 /* Device/Head ("select") Register */
 #define ATA_DEV_SELECT          0x10
 /* ATA1,3: Defined as '1'.
@@ -328,10 +320,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
 #define SMART_DISABLE         0xd9
 #define SMART_STATUS          0xda
 
-typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
-
-typedef void EndTransferFunc(IDEState *);
-
 typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
 typedef void DMAVoidFunc(const IDEDMA *);
 typedef int DMAIntFunc(const IDEDMA *, bool);
@@ -339,19 +327,6 @@ typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
 typedef void DMAu32Func(const IDEDMA *, uint32_t);
 typedef void DMAStopFunc(const IDEDMA *, bool);
 
-struct unreported_events {
-    bool eject_request;
-    bool new_media;
-};
-
-enum ide_dma_cmd {
-    IDE_DMA_READ = 0,
-    IDE_DMA_WRITE,
-    IDE_DMA_TRIM,
-    IDE_DMA_ATAPI,
-    IDE_DMA__COUNT
-};
-
 extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
 
 extern const MemoryRegionPortio ide_portio_list[];
@@ -369,97 +344,6 @@ typedef struct IDEBufferedRequest {
     bool orphaned;
 } IDEBufferedRequest;
 
-/* NOTE: IDEState represents in fact one drive */
-struct IDEState {
-    IDEBus *bus;
-    uint8_t unit;
-    /* ide config */
-    IDEDriveKind drive_kind;
-    int drive_heads, drive_sectors;
-    int cylinders, heads, sectors, chs_trans;
-    int64_t nb_sectors;
-    int mult_sectors;
-    int identify_set;
-    uint8_t identify_data[512];
-    int drive_serial;
-    char drive_serial_str[21];
-    char drive_model_str[41];
-    uint64_t wwn;
-    /* ide regs */
-    uint8_t feature;
-    uint8_t error;
-    uint32_t nsector;
-    uint8_t sector;
-    uint8_t lcyl;
-    uint8_t hcyl;
-    /* other part of tf for lba48 support */
-    uint8_t hob_feature;
-    uint8_t hob_nsector;
-    uint8_t hob_sector;
-    uint8_t hob_lcyl;
-    uint8_t hob_hcyl;
-
-    uint8_t select;
-    uint8_t status;
-
-    bool io8;
-    bool reset_reverts;
-
-    /* set for lba48 access */
-    uint8_t lba48;
-    BlockBackend *blk;
-    char version[9];
-    /* ATAPI specific */
-    struct unreported_events events;
-    uint8_t sense_key;
-    uint8_t asc;
-    bool tray_open;
-    bool tray_locked;
-    uint8_t cdrom_changed;
-    int packet_transfer_size;
-    int elementary_transfer_size;
-    int32_t io_buffer_index;
-    int lba;
-    int cd_sector_size;
-    int atapi_dma; /* true if dma is requested for the packet cmd */
-    BlockAcctCookie acct;
-    BlockAIOCB *pio_aiocb;
-    QEMUIOVector qiov;
-    QLIST_HEAD(, IDEBufferedRequest) buffered_requests;
-    /* ATA DMA state */
-    uint64_t io_buffer_offset;
-    int32_t io_buffer_size;
-    QEMUSGList sg;
-    /* PIO transfer handling */
-    int req_nb_sectors; /* number of sectors per interrupt */
-    EndTransferFunc *end_transfer_func;
-    uint8_t *data_ptr;
-    uint8_t *data_end;
-    uint8_t *io_buffer;
-    /* PIO save/restore */
-    int32_t io_buffer_total_len;
-    int32_t cur_io_buffer_offset;
-    int32_t cur_io_buffer_len;
-    uint8_t end_transfer_fn_idx;
-    QEMUTimer *sector_write_timer; /* only used for win2k install hack */
-    uint32_t irq_count; /* counts IRQs when using win2k install hack */
-    /* CF-ATA extended error */
-    uint8_t ext_error;
-    /* CF-ATA metadata storage */
-    uint32_t mdata_size;
-    uint8_t *mdata_storage;
-    int media_changed;
-    enum ide_dma_cmd dma_cmd;
-    /* SMART */
-    uint8_t smart_enabled;
-    uint8_t smart_autosave;
-    int smart_errors;
-    uint8_t smart_selftest_count;
-    uint8_t *smart_selftest_data;
-    /* AHCI */
-    int ncq_queues;
-};
-
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
     DMAVoidFunc *pio_transfer;
@@ -502,33 +386,6 @@ struct IDEBus {
     VMChangeStateEntry *vmstate;
 };
 
-#define TYPE_IDE_DEVICE "ide-device"
-OBJECT_DECLARE_TYPE(IDEDevice, IDEDeviceClass, IDE_DEVICE)
-
-struct IDEDeviceClass {
-    DeviceClass parent_class;
-    void (*realize)(IDEDevice *dev, Error **errp);
-};
-
-struct IDEDevice {
-    DeviceState qdev;
-    uint32_t unit;
-    BlockConf conf;
-    int chs_trans;
-    char *version;
-    char *serial;
-    char *model;
-    uint64_t wwn;
-    /*
-     * 0x0000        - rotation rate not reported
-     * 0x0001        - non-rotating medium (SSD)
-     * 0x0002-0x0400 - reserved
-     * 0x0401-0xffe  - rotations per minute
-     * 0xffff        - reserved
-     */
-    uint16_t rotation_rate;
-};
-
 /* These are used for the error_status field of IDEBus */
 #define IDE_RETRY_MASK 0xf8
 #define IDE_RETRY_DMA  0x08
diff --git a/hw/ide/ide-dev.c b/hw/ide/ide-dev.c
index 15d088fd06..c8e2033469 100644
--- a/hw/ide/ide-dev.c
+++ b/hw/ide/ide-dev.c
@@ -23,6 +23,7 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "hw/ide/ide-dev.h"
+#include "hw/ide/internal.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
-- 
2.43.2

[PATCH 4/7] hw/ide: Move IDE bus related definitions to a new header ide-bus.h

[Thomas Huth]

Let's consolidate the public IDE bus related functions in a separate
header.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ide/ide-bus.h  | 41 +++++++++++++++++++++++++++++++++++++++
 include/hw/ide/internal.h | 38 +-----------------------------------
 2 files changed, 42 insertions(+), 37 deletions(-)
 create mode 100644 include/hw/ide/ide-bus.h

diff --git a/include/hw/ide/ide-bus.h b/include/hw/ide/ide-bus.h
new file mode 100644
index 0000000000..e0460700ed
--- /dev/null
+++ b/include/hw/ide/ide-bus.h
@@ -0,0 +1,41 @@
+#ifndef HW_IDE_BUS_H
+#define HW_IDE_BUS_H
+
+#include "exec/ioport.h"
+#include "hw/ide/ide-dev.h"
+
+struct IDEBus {
+    BusState qbus;
+    IDEDevice *master;
+    IDEDevice *slave;
+    IDEState ifs[2];
+    QEMUBH *bh;
+
+    int bus_id;
+    int max_units;
+    IDEDMA *dma;
+    uint8_t unit;
+    uint8_t cmd;
+    qemu_irq irq; /* bus output */
+
+    int error_status;
+    uint8_t retry_unit;
+    int64_t retry_sector_num;
+    uint32_t retry_nsector;
+    PortioList portio_list;
+    PortioList portio2_list;
+    VMChangeStateEntry *vmstate;
+};
+
+#define TYPE_IDE_BUS "IDE"
+OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
+
+void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
+                  int bus_id, int max_units);
+IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
+
+int ide_get_geometry(BusState *bus, int unit,
+                     int16_t *cyls, int8_t *heads, int8_t *secs);
+int ide_get_bios_chs_trans(BusState *bus, int unit);
+
+#endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 5cc109fe82..642bd1a979 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -8,16 +8,12 @@
  */
 
 #include "hw/ide.h"
-#include "exec/ioport.h"
-#include "hw/ide/ide-dev.h"
+#include "hw/ide/ide-bus.h"
 
 /* debug IDE devices */
 #define USE_DMA_CDROM
 #include "qom/object.h"
 
-#define TYPE_IDE_BUS "IDE"
-OBJECT_DECLARE_SIMPLE_TYPE(IDEBus, IDE_BUS)
-
 /* Device/Head ("select") Register */
 #define ATA_DEV_SELECT          0x10
 /* ATA1,3: Defined as '1'.
@@ -363,29 +359,6 @@ struct IDEDMA {
     BlockAIOCB *aiocb;
 };
 
-struct IDEBus {
-    BusState qbus;
-    IDEDevice *master;
-    IDEDevice *slave;
-    IDEState ifs[2];
-    QEMUBH *bh;
-
-    int bus_id;
-    int max_units;
-    IDEDMA *dma;
-    uint8_t unit;
-    uint8_t cmd;
-    qemu_irq irq; /* bus output */
-
-    int error_status;
-    uint8_t retry_unit;
-    int64_t retry_sector_num;
-    uint32_t retry_nsector;
-    PortioList portio_list;
-    PortioList portio2_list;
-    VMChangeStateEntry *vmstate;
-};
-
 /* These are used for the error_status field of IDEBus */
 #define IDE_RETRY_MASK 0xf8
 #define IDE_RETRY_DMA  0x08
@@ -502,15 +475,6 @@ void ide_cancel_dma_sync(IDEState *s);
 void ide_atapi_cmd(IDEState *s);
 void ide_atapi_cmd_reply_end(IDEState *s);
 
-/* hw/ide/qdev.c */
-void ide_bus_init(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
-                  int bus_id, int max_units);
-IDEDevice *ide_bus_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
-
-int ide_get_geometry(BusState *bus, int unit,
-                     int16_t *cyls, int8_t *heads, int8_t *secs);
-int ide_get_bios_chs_trans(BusState *bus, int unit);
-
 int ide_handle_rw_error(IDEState *s, int error, int op);
 
 #endif /* HW_IDE_INTERNAL_H */
-- 
2.43.2

[PATCH 5/7] hw/ide: Move IDE DMA related definitions to a separate header ide-dma.h

[Thomas Huth]

These definitions are required outside of the hw/ide/ code, too,
so lets's move them from internal.h to a new header called ide-dma.h.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ide/ide-dma.h  | 30 ++++++++++++++++++++++++++++++
 include/hw/ide/internal.h | 27 +--------------------------
 2 files changed, 31 insertions(+), 26 deletions(-)
 create mode 100644 include/hw/ide/ide-dma.h

diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
new file mode 100644
index 0000000000..fb82966bdd
--- /dev/null
+++ b/include/hw/ide/ide-dma.h
@@ -0,0 +1,30 @@
+#ifndef HW_IDE_DMA_H
+#define HW_IDE_DMA_H
+
+typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
+typedef void DMAVoidFunc(const IDEDMA *);
+typedef int DMAIntFunc(const IDEDMA *, bool);
+typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
+typedef void DMAu32Func(const IDEDMA *, uint32_t);
+typedef void DMAStopFunc(const IDEDMA *, bool);
+
+struct IDEDMAOps {
+    DMAStartFunc *start_dma;
+    DMAVoidFunc *pio_transfer;
+    DMAInt32Func *prepare_buf;
+    DMAu32Func *commit_buf;
+    DMAIntFunc *rw_buf;
+    DMAVoidFunc *restart;
+    DMAVoidFunc *restart_dma;
+    DMAStopFunc *set_inactive;
+    DMAVoidFunc *cmd_done;
+    DMAVoidFunc *reset;
+};
+
+struct IDEDMA {
+    const struct IDEDMAOps *ops;
+    QEMUIOVector qiov;
+    BlockAIOCB *aiocb;
+};
+
+#endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 642bd1a979..d1d3fcd23a 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -9,6 +9,7 @@
 
 #include "hw/ide.h"
 #include "hw/ide/ide-bus.h"
+#include "hw/ide/ide-dma.h"
 
 /* debug IDE devices */
 #define USE_DMA_CDROM
@@ -316,13 +317,6 @@
 #define SMART_DISABLE         0xd9
 #define SMART_STATUS          0xda
 
-typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
-typedef void DMAVoidFunc(const IDEDMA *);
-typedef int DMAIntFunc(const IDEDMA *, bool);
-typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
-typedef void DMAu32Func(const IDEDMA *, uint32_t);
-typedef void DMAStopFunc(const IDEDMA *, bool);
-
 extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
 
 extern const MemoryRegionPortio ide_portio_list[];
@@ -340,25 +334,6 @@ typedef struct IDEBufferedRequest {
     bool orphaned;
 } IDEBufferedRequest;
 
-struct IDEDMAOps {
-    DMAStartFunc *start_dma;
-    DMAVoidFunc *pio_transfer;
-    DMAInt32Func *prepare_buf;
-    DMAu32Func *commit_buf;
-    DMAIntFunc *rw_buf;
-    DMAVoidFunc *restart;
-    DMAVoidFunc *restart_dma;
-    DMAStopFunc *set_inactive;
-    DMAVoidFunc *cmd_done;
-    DMAVoidFunc *reset;
-};
-
-struct IDEDMA {
-    const struct IDEDMAOps *ops;
-    QEMUIOVector qiov;
-    BlockAIOCB *aiocb;
-};
-
 /* These are used for the error_status field of IDEBus */
 #define IDE_RETRY_MASK 0xf8
 #define IDE_RETRY_DMA  0x08
-- 
2.43.2

[PATCH 6/7] hw/ide: Remove the include/hw/ide.h legacy file

[Thomas Huth]

There was only one prototype left in this legacy file. Move it to
ide-dev.h to finally get rid of it.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ide.h          | 9 ---------
 include/hw/ide/ide-dev.h  | 2 ++
 include/hw/ide/internal.h | 1 -
 3 files changed, 2 insertions(+), 10 deletions(-)
 delete mode 100644 include/hw/ide.h

diff --git a/include/hw/ide.h b/include/hw/ide.h
deleted file mode 100644
index db963bdb77..0000000000
--- a/include/hw/ide.h
+++ /dev/null
@@ -1,9 +0,0 @@
-#ifndef HW_IDE_H
-#define HW_IDE_H
-
-#include "exec/memory.h"
-
-/* ide/core.c */
-void ide_drive_get(DriveInfo **hd, int max_bus);
-
-#endif /* HW_IDE_H */
diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
index de88784a25..ad55997442 100644
--- a/include/hw/ide/ide-dev.h
+++ b/include/hw/ide/ide-dev.h
@@ -181,4 +181,6 @@ typedef struct IDEDrive {
 
 void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp);
 
+void ide_drive_get(DriveInfo **hd, int max_bus);
+
 #endif
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index d1d3fcd23a..0fc2013374 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -7,7 +7,6 @@
  * non-internal declarations are in hw/ide.h
  */
 
-#include "hw/ide.h"
 #include "hw/ide/ide-bus.h"
 #include "hw/ide/ide-dma.h"
 
-- 
2.43.2

[PATCH 7/7] hw/ide: Stop exposing internal.h to non-IDE files

[Thomas Huth]

include/hw/ide/internal.h is currently included by include/hw/ide/pci.h
and thus exposed to a lot of files that are not part of the IDE subsystem.
Stop including internal.h there and use the appropriate new headers
ide-bus.h and ide-dma.h instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ide/pci.h | 3 ++-
 hw/i386/pc.c         | 2 +-
 hw/ide/cmd646.c      | 1 +
 hw/ide/pci.c         | 1 +
 hw/ide/piix.c        | 1 +
 hw/ide/sii3112.c     | 1 +
 hw/ide/via.c         | 1 +
 7 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index a814a0a7c3..e1e012c387 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -1,7 +1,8 @@
 #ifndef HW_IDE_PCI_H
 #define HW_IDE_PCI_H
 
-#include "hw/ide/internal.h"
+#include "hw/ide/ide-bus.h"
+#include "hw/ide/ide-dma.h"
 #include "hw/pci/pci_device.h"
 #include "qom/object.h"
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 196827531a..22d0c29575 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -31,7 +31,7 @@
 #include "hw/i386/fw_cfg.h"
 #include "hw/i386/vmport.h"
 #include "sysemu/cpus.h"
-#include "hw/ide/internal.h"
+#include "hw/ide/ide-bus.h"
 #include "hw/timer/hpet.h"
 #include "hw/loader.h"
 #include "hw/rtc/mc146818rtc.h"
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c0bcfa4414..23d213ff01 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -33,6 +33,7 @@
 #include "sysemu/reset.h"
 
 #include "hw/ide/pci.h"
+#include "hw/ide/internal.h"
 #include "trace.h"
 
 /* CMD646 specific */
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index ca85d8474c..73efeec7f4 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -30,6 +30,7 @@
 #include "sysemu/dma.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
+#include "hw/ide/internal.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
 
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 4e5e12935f..1773a068c3 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -30,6 +30,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/pci/pci.h"
+#include "hw/ide/internal.h"
 #include "hw/ide/piix.h"
 #include "hw/ide/pci.h"
 #include "trace.h"
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 63dc4a0494..321b9e46a1 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/ide/internal.h"
 #include "hw/ide/pci.h"
 #include "qemu/module.h"
 #include "trace.h"
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 3f3c484253..cf151e70ec 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -25,6 +25,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/ide/internal.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
-- 
2.43.2

Re: [PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Philippe Mathieu-Daudé]

On 19/2/24 11:49, Thomas Huth wrote:
> Let's start to unentangle internal.h by moving public IDE device
> related definitions to ide-dev.h.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   include/hw/ide/ide-dev.h  | 145 +++++++++++++++++++++++++++++++++++++-
>   include/hw/ide/internal.h | 145 +-------------------------------------
>   hw/ide/ide-dev.c          |   1 +
>   3 files changed, 146 insertions(+), 145 deletions(-)
> 
> diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
> index 7e9663cda9..de88784a25 100644
> --- a/include/hw/ide/ide-dev.h
> +++ b/include/hw/ide/ide-dev.h
> @@ -20,9 +20,152 @@
>   #ifndef IDE_DEV_H
>   #define IDE_DEV_H
>   
> +#include "sysemu/dma.h"

Not required.

>   #include "hw/qdev-properties.h"
>   #include "hw/block/block.h"
> -#include "hw/ide/internal.h"
> +
> +typedef struct IDEDevice IDEDevice;
> +typedef struct IDEState IDEState;

> +typedef struct IDEDMA IDEDMA;
> +typedef struct IDEDMAOps IDEDMAOps;
> +typedef struct IDEBus IDEBus;

Looking at next patches, better forward-declare IDEBus and
IDEDMA in "qemu/typedefs.h".

IDEDMAOps and "sysemu/dma.h" belong to "hw/ide/ide-dma.h.

Re: [PATCH 0/7] hw/ide: Clean up hw/ide/qdev.c and include/hw/ide/internal.h

[Philippe Mathieu-Daudé]

On 19/2/24 11:49, Thomas Huth wrote:
> While trying to make it possible to compile-out the CompactFlash IDE device
> in downstream distributions (first patch), we noticed that there are more
> things in the IDE code that could use a proper clean up:
> 
> First, hw/ide/qdev.c is quite a mix between IDE BUS specific functions
> and (disk) device specific functions. Thus the second patch splits qdev.c
> into two new separate files to make it more obvious which part belongs
> to which kind of devices.
> 
> The remaining patches unentangle include/hw/ide/internal.h, which is meant
> as a header that should only be used internally to the IDE subsystem, but
> which is currently exposed to the world since include/hw/ide/pci.h includes
> this header, too. Thus we move the definitions that are also required for
> non-IDE code to other new header files, so we can finally change pci.h to
> stop including internal.h. After these changes, internal.h is only included
> by files in hw/ide/ as it should be.
> 
> Thomas Huth (7):
>    hw/ide: Add the possibility to disable the CompactFlash device in the
>      build
>    hw/ide: Split qdev.c into ide-bus.c and ide-dev.c
>    hw/ide: Move IDE device related definitions to ide-dev.h

Modulo comments in "hw/ide/ide-dev.h", series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>    hw/ide: Move IDE bus related definitions to a new header ide-bus.h
>    hw/ide: Move IDE DMA related definitions to a separate header
>      ide-dma.h
>    hw/ide: Remove the include/hw/ide.h legacy file
>    hw/ide: Stop exposing internal.h to non-IDE files

Re: [PATCH 2/7] hw/ide: Split qdev.c into ide-bus.c and ide-dev.c

[BALATON Zoltan]

On Mon, 19 Feb 2024, Thomas Huth wrote:
> qdev.c is a mixture between IDE bus specific functions and IDE device
> functions. Let's split it up to make it more obvious which part is
> related to bus handling and which part is related to device handling.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/ide/ide-bus.c             | 111 +++++++++++++++++++++++++++++++++++
> hw/ide/{qdev.c => ide-dev.c} |  87 +--------------------------
> hw/arm/Kconfig               |   2 +
> hw/ide/Kconfig               |  30 ++++++----
> hw/ide/meson.build           |   3 +-
> 5 files changed, 134 insertions(+), 99 deletions(-)
> create mode 100644 hw/ide/ide-bus.c
> rename hw/ide/{qdev.c => ide-dev.c} (78%)
[...]
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 29abe1da29..b372b819a4 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -275,6 +275,8 @@ config SBSA_REF
>     select USB_XHCI_SYSBUS
>     select WDT_SBSA
>     select BOCHS_DISPLAY
> +    select IDE_BUS
> +    select IDE_DEV
>
> config SABRELITE
>     bool
> diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
> index b93d6743d5..6dfc5a2129 100644
> --- a/hw/ide/Kconfig
> +++ b/hw/ide/Kconfig
> @@ -1,51 +1,58 @@
> config IDE_CORE
>     bool
>
> -config IDE_QDEV
> +config IDE_BUS
>     bool
>     select IDE_CORE

Maybe we can assume if something has an IDE bus it also wants to connect 
IDE devices to it so just select IDE_DEV here and not at every place 
IDE_BUS is selected? Or is there a place that only wants IDE_BUS?

Regards,
BALATON Zoltan

> +config IDE_DEV
> +    bool
> +    depends on IDE_BUS
> +
> config IDE_PCI
>     bool
>     depends on PCI
> -    select IDE_QDEV
> +    select IDE_BUS
> +    select IDE_DEV
>
> config IDE_ISA
>     bool
>     depends on ISA_BUS
> -    select IDE_QDEV
> +    select IDE_BUS
> +    select IDE_DEV
>
> config IDE_PIIX
>     bool
>     select IDE_PCI
> -    select IDE_QDEV
>
> config IDE_CMD646
>     bool
>     select IDE_PCI
> -    select IDE_QDEV
>
> config IDE_MACIO
>     bool
> -    select IDE_QDEV
> +    select IDE_BUS
> +    select IDE_DEV
>
> config IDE_MMIO
>     bool
> -    select IDE_QDEV
> +    select IDE_BUS
> +    select IDE_DEV
>
> config IDE_VIA
>     bool
>     select IDE_PCI
> -    select IDE_QDEV
>
> config MICRODRIVE
>     bool
> -    select IDE_QDEV
> +    select IDE_BUS
> +    select IDE_DEV
>     depends on PCMCIA
>
> config AHCI
>     bool
> -    select IDE_QDEV
> +    select IDE_BUS
> +    select IDE_DEV
>
> config AHCI_ICH9
>     bool
> @@ -56,8 +63,7 @@ config AHCI_ICH9
> config IDE_SII3112
>     bool
>     select IDE_PCI
> -    select IDE_QDEV
>
> config IDE_CF
>     bool
> -    default y if IDE_QDEV
> +    default y if IDE_BUS
> diff --git a/hw/ide/meson.build b/hw/ide/meson.build
> index d2e5b45c9e..d09705cac0 100644
> --- a/hw/ide/meson.build
> +++ b/hw/ide/meson.build
> @@ -1,15 +1,16 @@
> system_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
> system_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
> system_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
> +system_ss.add(when: 'CONFIG_IDE_BUS', if_true: files('ide-bus.c'))
> system_ss.add(when: 'CONFIG_IDE_CF', if_true: files('cf.c'))
> system_ss.add(when: 'CONFIG_IDE_CMD646', if_true: files('cmd646.c'))
> system_ss.add(when: 'CONFIG_IDE_CORE', if_true: files('core.c', 'atapi.c'))
> +system_ss.add(when: 'CONFIG_IDE_DEV', if_true: files('ide-dev.c'))
> system_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c', 'ioport.c'))
> system_ss.add(when: 'CONFIG_IDE_MACIO', if_true: files('macio.c'))
> system_ss.add(when: 'CONFIG_IDE_MMIO', if_true: files('mmio.c'))
> system_ss.add(when: 'CONFIG_IDE_PCI', if_true: files('pci.c'))
> system_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c'))
> -system_ss.add(when: 'CONFIG_IDE_QDEV', if_true: files('qdev.c'))
> system_ss.add(when: 'CONFIG_IDE_SII3112', if_true: files('sii3112.c'))
> system_ss.add(when: 'CONFIG_IDE_VIA', if_true: files('via.c'))
> system_ss.add(when: 'CONFIG_MICRODRIVE', if_true: files('microdrive.c'))
>

Re: [PATCH 5/7] hw/ide: Move IDE DMA related definitions to a separate header ide-dma.h

[BALATON Zoltan]

On Mon, 19 Feb 2024, Thomas Huth wrote:
> These definitions are required outside of the hw/ide/ code, too,
> so lets's move them from internal.h to a new header called ide-dma.h.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> include/hw/ide/ide-dma.h  | 30 ++++++++++++++++++++++++++++++
> include/hw/ide/internal.h | 27 +--------------------------
> 2 files changed, 31 insertions(+), 26 deletions(-)
> create mode 100644 include/hw/ide/ide-dma.h
>
> diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
> new file mode 100644
> index 0000000000..fb82966bdd
> --- /dev/null
> +++ b/include/hw/ide/ide-dma.h
> @@ -0,0 +1,30 @@
> +#ifndef HW_IDE_DMA_H
> +#define HW_IDE_DMA_H
> +
> +typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
> +typedef void DMAVoidFunc(const IDEDMA *);
> +typedef int DMAIntFunc(const IDEDMA *, bool);
> +typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
> +typedef void DMAu32Func(const IDEDMA *, uint32_t);
> +typedef void DMAStopFunc(const IDEDMA *, bool);
> +
> +struct IDEDMAOps {
> +    DMAStartFunc *start_dma;
> +    DMAVoidFunc *pio_transfer;
> +    DMAInt32Func *prepare_buf;
> +    DMAu32Func *commit_buf;
> +    DMAIntFunc *rw_buf;
> +    DMAVoidFunc *restart;
> +    DMAVoidFunc *restart_dma;
> +    DMAStopFunc *set_inactive;
> +    DMAVoidFunc *cmd_done;
> +    DMAVoidFunc *reset;
> +};
> +
> +struct IDEDMA {
> +    const struct IDEDMAOps *ops;
> +    QEMUIOVector qiov;
> +    BlockAIOCB *aiocb;

Doesn't this need to #include something to define QEMUIOVector and 
BlockAIOCB and some of the DMA and IDE types not defined above?

Regards,
BALATON Zoltan

> +};
> +
> +#endif
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 642bd1a979..d1d3fcd23a 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -9,6 +9,7 @@
>
> #include "hw/ide.h"
> #include "hw/ide/ide-bus.h"
> +#include "hw/ide/ide-dma.h"
>
> /* debug IDE devices */
> #define USE_DMA_CDROM
> @@ -316,13 +317,6 @@
> #define SMART_DISABLE         0xd9
> #define SMART_STATUS          0xda
>
> -typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
> -typedef void DMAVoidFunc(const IDEDMA *);
> -typedef int DMAIntFunc(const IDEDMA *, bool);
> -typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
> -typedef void DMAu32Func(const IDEDMA *, uint32_t);
> -typedef void DMAStopFunc(const IDEDMA *, bool);
> -
> extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
>
> extern const MemoryRegionPortio ide_portio_list[];
> @@ -340,25 +334,6 @@ typedef struct IDEBufferedRequest {
>     bool orphaned;
> } IDEBufferedRequest;
>
> -struct IDEDMAOps {
> -    DMAStartFunc *start_dma;
> -    DMAVoidFunc *pio_transfer;
> -    DMAInt32Func *prepare_buf;
> -    DMAu32Func *commit_buf;
> -    DMAIntFunc *rw_buf;
> -    DMAVoidFunc *restart;
> -    DMAVoidFunc *restart_dma;
> -    DMAStopFunc *set_inactive;
> -    DMAVoidFunc *cmd_done;
> -    DMAVoidFunc *reset;
> -};
> -
> -struct IDEDMA {
> -    const struct IDEDMAOps *ops;
> -    QEMUIOVector qiov;
> -    BlockAIOCB *aiocb;
> -};
> -
> /* These are used for the error_status field of IDEBus */
> #define IDE_RETRY_MASK 0xf8
> #define IDE_RETRY_DMA  0x08
>

Re: [PATCH 2/7] hw/ide: Split qdev.c into ide-bus.c and ide-dev.c

[Thomas Huth]

On 19/02/2024 12.45, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Thomas Huth wrote:
>> qdev.c is a mixture between IDE bus specific functions and IDE device
>> functions. Let's split it up to make it more obvious which part is
>> related to bus handling and which part is related to device handling.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/ide/ide-bus.c             | 111 +++++++++++++++++++++++++++++++++++
>> hw/ide/{qdev.c => ide-dev.c} |  87 +--------------------------
>> hw/arm/Kconfig               |   2 +
>> hw/ide/Kconfig               |  30 ++++++----
>> hw/ide/meson.build           |   3 +-
>> 5 files changed, 134 insertions(+), 99 deletions(-)
>> create mode 100644 hw/ide/ide-bus.c
>> rename hw/ide/{qdev.c => ide-dev.c} (78%)
> [...]
>> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
>> index 29abe1da29..b372b819a4 100644
>> --- a/hw/arm/Kconfig
>> +++ b/hw/arm/Kconfig
>> @@ -275,6 +275,8 @@ config SBSA_REF
>>     select USB_XHCI_SYSBUS
>>     select WDT_SBSA
>>     select BOCHS_DISPLAY
>> +    select IDE_BUS
>> +    select IDE_DEV
>>
>> config SABRELITE
>>     bool
>> diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
>> index b93d6743d5..6dfc5a2129 100644
>> --- a/hw/ide/Kconfig
>> +++ b/hw/ide/Kconfig
>> @@ -1,51 +1,58 @@
>> config IDE_CORE
>>     bool
>>
>> -config IDE_QDEV
>> +config IDE_BUS
>>     bool
>>     select IDE_CORE
> 
> Maybe we can assume if something has an IDE bus it also wants to connect IDE 
> devices to it so just select IDE_DEV here and not at every place IDE_BUS is 
> selected? Or is there a place that only wants IDE_BUS?

Currently not, but I think it's conceptually much cleaner if we are explicit 
here.

  Thomas

Re: [PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Thomas Huth]

On 19/02/2024 12.32, Philippe Mathieu-Daudé wrote:
> On 19/2/24 11:49, Thomas Huth wrote:
>> Let's start to unentangle internal.h by moving public IDE device
>> related definitions to ide-dev.h.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/hw/ide/ide-dev.h  | 145 +++++++++++++++++++++++++++++++++++++-
>>   include/hw/ide/internal.h | 145 +-------------------------------------
>>   hw/ide/ide-dev.c          |   1 +
>>   3 files changed, 146 insertions(+), 145 deletions(-)
>>
>> diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
>> index 7e9663cda9..de88784a25 100644
>> --- a/include/hw/ide/ide-dev.h
>> +++ b/include/hw/ide/ide-dev.h
>> @@ -20,9 +20,152 @@
>>   #ifndef IDE_DEV_H
>>   #define IDE_DEV_H
>> +#include "sysemu/dma.h"
> 
> Not required.

It's required for QEMUSGList that is used in struct IDEState.

>>   #include "hw/qdev-properties.h"
>>   #include "hw/block/block.h"
>> -#include "hw/ide/internal.h"
>> +
>> +typedef struct IDEDevice IDEDevice;
>> +typedef struct IDEState IDEState;
> 
>> +typedef struct IDEDMA IDEDMA;
>> +typedef struct IDEDMAOps IDEDMAOps;
>> +typedef struct IDEBus IDEBus;
> 
> Looking at next patches, better forward-declare IDEBus and
> IDEDMA in "qemu/typedefs.h".

I really dislike using qemu/typedefs.h for things that are not really part 
of the core framework, since it's a 
touch-it-once-and-everything-gets-recompiled header. So IMHO the typedefs 
here are the lesser evil.

> IDEDMAOps and "sysemu/dma.h" belong to "hw/ide/ide-dma.h.

Ok, I can move the typedef for IDEDMAOps to ide-dma.h instead.

  Thomas

Re: [PATCH 5/7] hw/ide: Move IDE DMA related definitions to a separate header ide-dma.h

[Thomas Huth]

On 19/02/2024 12.53, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Thomas Huth wrote:
>> These definitions are required outside of the hw/ide/ code, too,
>> so lets's move them from internal.h to a new header called ide-dma.h.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> include/hw/ide/ide-dma.h  | 30 ++++++++++++++++++++++++++++++
>> include/hw/ide/internal.h | 27 +--------------------------
>> 2 files changed, 31 insertions(+), 26 deletions(-)
>> create mode 100644 include/hw/ide/ide-dma.h
>>
>> diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
>> new file mode 100644
>> index 0000000000..fb82966bdd
>> --- /dev/null
>> +++ b/include/hw/ide/ide-dma.h
>> @@ -0,0 +1,30 @@
>> +#ifndef HW_IDE_DMA_H
>> +#define HW_IDE_DMA_H
>> +
>> +typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc 
>> *);
>> +typedef void DMAVoidFunc(const IDEDMA *);
>> +typedef int DMAIntFunc(const IDEDMA *, bool);
>> +typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
>> +typedef void DMAu32Func(const IDEDMA *, uint32_t);
>> +typedef void DMAStopFunc(const IDEDMA *, bool);
>> +
>> +struct IDEDMAOps {
>> +    DMAStartFunc *start_dma;
>> +    DMAVoidFunc *pio_transfer;
>> +    DMAInt32Func *prepare_buf;
>> +    DMAu32Func *commit_buf;
>> +    DMAIntFunc *rw_buf;
>> +    DMAVoidFunc *restart;
>> +    DMAVoidFunc *restart_dma;
>> +    DMAStopFunc *set_inactive;
>> +    DMAVoidFunc *cmd_done;
>> +    DMAVoidFunc *reset;
>> +};
>> +
>> +struct IDEDMA {
>> +    const struct IDEDMAOps *ops;
>> +    QEMUIOVector qiov;
>> +    BlockAIOCB *aiocb;
> 
> Doesn't this need to #include something to define QEMUIOVector and 
> BlockAIOCB and some of the DMA and IDE types not defined above?

Yes, it currently works by accident since the header is only included in 
spots where those things are already defined, but I'll better add some 
#include statements here so that this header can also be used stand-alone in 
other spots.

  Thomas

Re: [PATCH 3/7] hw/ide: Move IDE device related definitions to ide-dev.h

[Philippe Mathieu-Daudé]

On 19/2/24 20:17, Thomas Huth wrote:
> On 19/02/2024 12.32, Philippe Mathieu-Daudé wrote:
>> On 19/2/24 11:49, Thomas Huth wrote:
>>> Let's start to unentangle internal.h by moving public IDE device
>>> related definitions to ide-dev.h.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   include/hw/ide/ide-dev.h  | 145 +++++++++++++++++++++++++++++++++++++-
>>>   include/hw/ide/internal.h | 145 +-------------------------------------
>>>   hw/ide/ide-dev.c          |   1 +
>>>   3 files changed, 146 insertions(+), 145 deletions(-)
>>>
>>> diff --git a/include/hw/ide/ide-dev.h b/include/hw/ide/ide-dev.h
>>> index 7e9663cda9..de88784a25 100644
>>> --- a/include/hw/ide/ide-dev.h
>>> +++ b/include/hw/ide/ide-dev.h
>>> @@ -20,9 +20,152 @@
>>>   #ifndef IDE_DEV_H
>>>   #define IDE_DEV_H
>>> +#include "sysemu/dma.h"
>>
>> Not required.
> 
> It's required for QEMUSGList that is used in struct IDEState.

Oh, OK.

>>>   #include "hw/qdev-properties.h"
>>>   #include "hw/block/block.h"
>>> -#include "hw/ide/internal.h"
>>> +
>>> +typedef struct IDEDevice IDEDevice;
>>> +typedef struct IDEState IDEState;
>>
>>> +typedef struct IDEDMA IDEDMA;
>>> +typedef struct IDEDMAOps IDEDMAOps;
>>> +typedef struct IDEBus IDEBus;
>>
>> Looking at next patches, better forward-declare IDEBus and
>> IDEDMA in "qemu/typedefs.h".
> 
> I really dislike using qemu/typedefs.h for things that are not really 
> part of the core framework, since it's a 
> touch-it-once-and-everything-gets-recompiled header. So IMHO the 
> typedefs here are the lesser evil.

OK then.

>> IDEDMAOps and "sysemu/dma.h" belong to "hw/ide/ide-dma.h.
> 
> Ok, I can move the typedef for IDEDMAOps to ide-dma.h instead.
> 
>   Thomas
> 
>