[PATCH v2 0/2] hw/ide: implement ich6 ide controller support

[PATCH v2 0/2] hw/ide: implement ich6 ide controller support

[Liav Albani]

This is version 2 of this patch, this time a patch series, after following
the suggestions from BALATON Zoltan. I implemented this device because I have an
old machine from 2009 which has the ICH7 south bridge in it, so when I tried to
run Linux on it, it booted just fine (as you might expect), but when I tried to
boot with the SerenityOS kernel, it struggled to initialize the IDE controller.
Therefore, upstreaming these changes might be beneficial to other OS developers
and hobbyists out there, and I will use this to fix the issues within the
SerenityOS kernel, without the need of preparing a bare metal setup each time I
need to test the code of the kernel.

Please keep in mind that while this is usable right now with the Q35 chipset,
when trying to boot with an i440FX machine, SeaBIOS doesn't handle this device
very well, so it tries no matter what type of IDE controller it sees to assign
the IO ports to legacy values. I have a patch I wrote locally which I gladly
will send to SeaBIOS, that fixes this problem by ensuring that when attaching a
storage device to this controller, SeaBIOS will relocate the IO ports to other
values so there's no collision with the default PIIX3/4 IDE controller. Even if
SeaBIOS didn't configure this device correctly, Linux will relocate the IO ports
and the user can still use the attached storage devices, given that the user
managed to boot from a storage device that is not attached to the ICH6 IDE
controller but to other storage controller in the system.

Liav Albani (2):
  hw/ide: split bmdma read and write functions from piix.c
  hw/ide: add ich6 ide controller device emulation

 hw/i386/Kconfig          |   2 +
 hw/ide/Kconfig           |   5 +
 hw/ide/bmdma.c           |  84 ++++++++++++++++
 hw/ide/ich6.c            | 204 +++++++++++++++++++++++++++++++++++++++
 hw/ide/meson.build       |   3 +-
 hw/ide/piix.c            |  51 +---------
 include/hw/ide/bmdma.h   |  34 +++++++
 include/hw/ide/pci.h     |   1 +
 include/hw/pci/pci_ids.h |   1 +
 9 files changed, 336 insertions(+), 49 deletions(-)
 create mode 100644 hw/ide/bmdma.c
 create mode 100644 hw/ide/ich6.c
 create mode 100644 include/hw/ide/bmdma.h

-- 
2.35.1

[PATCH v2 1/2] hw/ide: split bmdma read and write functions from piix.c

[Liav Albani]

This is a preparation before implementing another PCI IDE controller
that relies on these functions, so these can be shared between both
implementations.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 hw/ide/bmdma.c         | 84 ++++++++++++++++++++++++++++++++++++++++++
 hw/ide/meson.build     |  2 +-
 hw/ide/piix.c          | 51 ++-----------------------
 include/hw/ide/bmdma.h | 34 +++++++++++++++++
 4 files changed, 122 insertions(+), 49 deletions(-)
 create mode 100644 hw/ide/bmdma.c
 create mode 100644 include/hw/ide/bmdma.h

diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
new file mode 100644
index 0000000000..d12ed730dd
--- /dev/null
+++ b/hw/ide/bmdma.c
@@ -0,0 +1,84 @@
+/*
+ * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 and ICH6/7.
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * Copyright (c) 2006 Openedhand Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+
+#include "hw/ide/bmdma.h"
+#include "hw/ide/pci.h"
+#include "trace.h"
+
+uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size)
+{
+    BMDMAState *bm = opaque;
+    uint32_t val;
+
+    if (size != 1) {
+        return ((uint64_t)1 << (size * 8)) - 1;
+    }
+
+    switch (addr & 3) {
+    case 0:
+        val = bm->cmd;
+        break;
+    case 2:
+        val = bm->status;
+        break;
+    default:
+        val = 0xff;
+        break;
+    }
+
+    trace_bmdma_read(addr, val);
+    return val;
+}
+
+void intel_ide_bmdma_write(void *opaque, hwaddr addr,
+                        uint64_t val, unsigned size)
+{
+    BMDMAState *bm = opaque;
+
+    if (size != 1) {
+        return;
+    }
+
+    trace_bmdma_write(addr, val);
+
+    switch (addr & 3) {
+    case 0:
+        bmdma_cmd_writeb(bm, val);
+        break;
+    case 2:
+        uint8_t cur_val = bm->status;
+        bm->status = (val & 0x60) | (cur_val & 1) | (cur_val & ~val & 0x06);
+        break;
+    }
+}
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index ddcb3b28d2..38f9ae7178 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -7,7 +7,7 @@ softmmu_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c', 'ioport.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_MACIO', if_true: files('macio.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_MMIO', if_true: files('mmio.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_PCI', if_true: files('pci.c'))
-softmmu_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c'))
+softmmu_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c', 'bmdma.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_QDEV', if_true: files('qdev.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_SII3112', if_true: files('sii3112.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_VIA', if_true: files('via.c'))
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..8f94809eee 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -33,57 +33,12 @@
 #include "sysemu/dma.h"
 
 #include "hw/ide/pci.h"
+#include "hw/ide/bmdma.h"
 #include "trace.h"
 
-static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
-{
-    BMDMAState *bm = opaque;
-    uint32_t val;
-
-    if (size != 1) {
-        return ((uint64_t)1 << (size * 8)) - 1;
-    }
-
-    switch(addr & 3) {
-    case 0:
-        val = bm->cmd;
-        break;
-    case 2:
-        val = bm->status;
-        break;
-    default:
-        val = 0xff;
-        break;
-    }
-
-    trace_bmdma_read(addr, val);
-    return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
-                        uint64_t val, unsigned size)
-{
-    BMDMAState *bm = opaque;
-
-    if (size != 1) {
-        return;
-    }
-
-    trace_bmdma_write(addr, val);
-
-    switch(addr & 3) {
-    case 0:
-        bmdma_cmd_writeb(bm, val);
-        break;
-    case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
-        break;
-    }
-}
-
 static const MemoryRegionOps piix_bmdma_ops = {
-    .read = bmdma_read,
-    .write = bmdma_write,
+    .read = intel_ide_bmdma_read,
+    .write = intel_ide_bmdma_write,
 };
 
 static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/include/hw/ide/bmdma.h b/include/hw/ide/bmdma.h
new file mode 100644
index 0000000000..ce76d395f5
--- /dev/null
+++ b/include/hw/ide/bmdma.h
@@ -0,0 +1,34 @@
+/*
+ * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 and ICH6/7.
+ *
+ * Copyright (c) 2022 Liav Albani
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_IDE_BMDMA_H
+#define HW_IDE_BMDMA_H
+
+#include "hw/ide/internal.h"
+
+uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size);
+void intel_ide_bmdma_write(void *opaque, hwaddr addr, uint64_t val,
+                           unsigned size);
+
+#endif
-- 
2.35.1

[PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[Liav Albani]

This type of IDE controller has support for relocating the IO ports and
doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the controller.

There's no x86 chipset in QEMU that will try to attach this device by
default. It is considered a legacy-free device in the aspect of PCI bus
resource management as the guest OS can relocate the IO ports as it sees
fit to its needs. However, this is still a legacy device that belongs to
chipsets from late 2000s.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 hw/i386/Kconfig          |   2 +
 hw/ide/Kconfig           |   5 +
 hw/ide/ich6.c            | 204 +++++++++++++++++++++++++++++++++++++++
 hw/ide/meson.build       |   1 +
 include/hw/ide/pci.h     |   1 +
 include/hw/pci/pci_ids.h |   1 +
 6 files changed, 214 insertions(+)
 create mode 100644 hw/ide/ich6.c

diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index d22ac4a4b9..a18de2d962 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -75,6 +75,7 @@ config I440FX
     select PCI_I440FX
     select PIIX3
     select IDE_PIIX
+    select IDE_ICH6
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
@@ -101,6 +102,7 @@ config Q35
     select PCI_EXPRESS_Q35
     select LPC_ICH9
     select AHCI_ICH9
+    select IDE_ICH6
     select DIMM
     select SMBIOS
     select FW_CFG_DMA
diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
index dd85fa3619..63304325a5 100644
--- a/hw/ide/Kconfig
+++ b/hw/ide/Kconfig
@@ -38,6 +38,11 @@ config IDE_VIA
     select IDE_PCI
     select IDE_QDEV
 
+config IDE_ICH6
+    bool
+    select IDE_PCI
+    select IDE_QDEV
+
 config MICRODRIVE
     bool
     select IDE_QDEV
diff --git a/hw/ide/ich6.c b/hw/ide/ich6.c
new file mode 100644
index 0000000000..8f46d3fce2
--- /dev/null
+++ b/hw/ide/ich6.c
@@ -0,0 +1,204 @@
+/*
+ * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
+ *
+ * Copyright (c) 2022 Liav Albani
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "sysemu/block-backend.h"
+#include "sysemu/blockdev.h"
+#include "sysemu/dma.h"
+
+#include "hw/ide/pci.h"
+#include "hw/ide/bmdma.h"
+#include "trace.h"
+
+static const MemoryRegionOps ich6_bmdma_ops = {
+    .read = intel_ide_bmdma_read,
+    .write = intel_ide_bmdma_write,
+};
+
+static void bmdma_setup_bar(PCIIDEState *d)
+{
+    int i;
+
+    memory_region_init(&d->bmdma_bar, OBJECT(d), "ich6-bmdma-container", 16);
+    for (i = 0; i < 2; i++) {
+        BMDMAState *bm = &d->bmdma[i];
+
+        memory_region_init_io(&bm->extra_io, OBJECT(d), &ich6_bmdma_ops, bm,
+                              "ich6-bmdma", 4);
+        memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
+        memory_region_init_io(&bm->addr_ioport, OBJECT(d),
+                              &bmdma_addr_ioport_ops, bm, "bmdma", 4);
+        memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
+    }
+}
+
+static void ich6_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t val,
+                                    int l)
+{
+    uint32_t i;
+
+    pci_default_write_config(d, addr, val, l);
+
+    for (i = addr; i < addr + l; i++) {
+        switch (i) {
+        case 0x40:
+            pci_default_write_config(d, i, 0x8000, 2);
+            continue;
+        case 0x42:
+            pci_default_write_config(d, i, 0x8000, 2);
+            continue;
+        }
+    }
+}
+
+static void ich6_ide_reset(DeviceState *dev)
+{
+    PCIIDEState *d = PCI_IDE(dev);
+    PCIDevice *pd = PCI_DEVICE(d);
+    uint8_t *pci_conf = pd->config;
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        ide_bus_reset(&d->bus[i]);
+    }
+
+    /* TODO: this is the default. do not override. */
+    pci_conf[PCI_COMMAND] = 0x00;
+    /* TODO: this is the default. do not override. */
+    pci_conf[PCI_COMMAND + 1] = 0x00;
+    /* TODO: use pci_set_word */
+    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
+    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
+    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
+}
+
+static int pci_ich6_init_ports(PCIIDEState *d)
+{
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
+        ide_init2(&d->bus[i], d->native_irq);
+
+        bmdma_init(&d->bus[i], &d->bmdma[i], d);
+        d->bmdma[i].bus = &d->bus[i];
+        ide_register_restart_cb(&d->bus[i]);
+    }
+
+    return 0;
+}
+
+static void pci_ich6_ide_realize(PCIDevice *dev, Error **errp)
+{
+    PCIIDEState *d = PCI_IDE(dev);
+    uint8_t *pci_conf = dev->config;
+    int rc;
+
+    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin A */
+
+    /* PCI native mode-only controller, supports bus mastering */
+    pci_conf[PCI_CLASS_PROG] = 0x85;
+
+    bmdma_setup_bar(d);
+    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+
+    d->native_irq = pci_allocate_irq(&d->parent_obj);
+    /* Address Map Register - Non Combined Mode, MAP.USCC = 0 */
+    pci_conf[0x90]   = 0;
+
+    /* IDE Decode enabled by default */
+    pci_set_long(pci_conf + 0x40, 0x80008000);
+
+    /* IDE Timing control - Disable UDMA controls */
+    pci_set_long(pci_conf + 0x48, 0x00000000);
+
+    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
+
+    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
+                          &d->bus[0], "ich6-ide0-data", 8);
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
+
+    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
+                          &d->bus[0], "ich6-ide0-cmd", 4);
+    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
+
+    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
+                          &d->bus[1], "ich6-ide1-data", 8);
+    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
+
+    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
+                          &d->bus[1], "ich6-ide1-cmd", 4);
+    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
+
+    rc = pci_ich6_init_ports(d);
+    if (rc) {
+        error_setg_errno(errp, -rc, "Failed to realize %s",
+                         object_get_typename(OBJECT(dev)));
+    }
+}
+
+static void pci_ich6_ide_exitfn(PCIDevice *dev)
+{
+    PCIIDEState *d = PCI_IDE(dev);
+    unsigned i;
+
+    for (i = 0; i < 2; ++i) {
+        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
+        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
+    }
+}
+
+static void ich6_ide_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    dc->reset = ich6_ide_reset;
+    k->realize = pci_ich6_ide_realize;
+    k->exit = pci_ich6_ide_exitfn;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82801GB;
+    k->class_id = PCI_CLASS_STORAGE_IDE;
+    k->config_read = pci_default_read_config;
+    k->config_write = ich6_pci_config_write;
+    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo ich6_ide_info = {
+    .name          = "ich6-ide",
+    .parent        = TYPE_PCI_IDE,
+    .class_init    = ich6_ide_class_init,
+};
+
+static void ich6_ide_register_types(void)
+{
+    type_register_static(&ich6_ide_info);
+}
+
+type_init(ich6_ide_register_types)
diff --git a/hw/ide/meson.build b/hw/ide/meson.build
index 38f9ae7178..6899e082db 100644
--- a/hw/ide/meson.build
+++ b/hw/ide/meson.build
@@ -1,5 +1,6 @@
 softmmu_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
 softmmu_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
+softmmu_ss.add(when: 'CONFIG_IDE_ICH6', if_true: files('ich6.c', 'bmdma.c'))
 softmmu_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_CMD646', if_true: files('cmd646.c'))
 softmmu_ss.add(when: 'CONFIG_IDE_CORE', if_true: files('core.c', 'atapi.c'))
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..d8bf08e728 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -53,6 +53,7 @@ struct PCIIDEState {
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];
+    qemu_irq native_irq; /* used only for ich6-ide */
 };
 
 static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index 11abe22d46..cf8767977c 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -244,6 +244,7 @@
 #define PCI_DEVICE_ID_INTEL_82371AB      0x7111
 #define PCI_DEVICE_ID_INTEL_82371AB_2    0x7112
 #define PCI_DEVICE_ID_INTEL_82371AB_3    0x7113
+#define PCI_DEVICE_ID_INTEL_82801GB      0x27c0
 
 #define PCI_DEVICE_ID_INTEL_ICH9_0       0x2910
 #define PCI_DEVICE_ID_INTEL_ICH9_1       0x2917
-- 
2.35.1

Re: [PATCH v2 1/2] hw/ide: split bmdma read and write functions from piix.c

[BALATON Zoltan]

On Fri, 18 Feb 2022, Liav Albani wrote:
> This is a preparation before implementing another PCI IDE controller
> that relies on these functions, so these can be shared between both
> implementations.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
> hw/ide/bmdma.c         | 84 ++++++++++++++++++++++++++++++++++++++++++
> hw/ide/meson.build     |  2 +-
> hw/ide/piix.c          | 51 ++-----------------------
> include/hw/ide/bmdma.h | 34 +++++++++++++++++
> 4 files changed, 122 insertions(+), 49 deletions(-)
> create mode 100644 hw/ide/bmdma.c
> create mode 100644 include/hw/ide/bmdma.h
>
> diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
> new file mode 100644
> index 0000000000..d12ed730dd
> --- /dev/null
> +++ b/hw/ide/bmdma.c
> @@ -0,0 +1,84 @@
> +/*
> + * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 and ICH6/7.
> + *
> + * Copyright (c) 2003 Fabrice Bellard
> + * Copyright (c) 2006 Openedhand Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/dma.h"
> +
> +#include "hw/ide/bmdma.h"
> +#include "hw/ide/pci.h"
> +#include "trace.h"

Are you sure you need all these includes? I haven't checked but I think 
there may be some unneeded ones here. On the other hand I'm not sure 
putting these in a new file is worth it. There are already some bmdma_* 
functions in hw/ide/pci.c which are declared in include/hw/ide/pci.h so 
you could just move these there too then no new file, new header nor 
changes to meson.build is needed in this patch..

> +
> +uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size)

As I said before these aren't intel specific as the same functions also 
appear in other ide controllers such as via.c so maybe a better name would 
be bmdma_default_read and bmdma_default_write or somehting similar so 
these can be also reused by other non-intel devices too.

Regards,
BALATON Zoltan

> +{
> +    BMDMAState *bm = opaque;
> +    uint32_t val;
> +
> +    if (size != 1) {
> +        return ((uint64_t)1 << (size * 8)) - 1;
> +    }
> +
> +    switch (addr & 3) {
> +    case 0:
> +        val = bm->cmd;
> +        break;
> +    case 2:
> +        val = bm->status;
> +        break;
> +    default:
> +        val = 0xff;
> +        break;
> +    }
> +
> +    trace_bmdma_read(addr, val);
> +    return val;
> +}
> +
> +void intel_ide_bmdma_write(void *opaque, hwaddr addr,
> +                        uint64_t val, unsigned size)
> +{
> +    BMDMAState *bm = opaque;
> +
> +    if (size != 1) {
> +        return;
> +    }
> +
> +    trace_bmdma_write(addr, val);
> +
> +    switch (addr & 3) {
> +    case 0:
> +        bmdma_cmd_writeb(bm, val);
> +        break;
> +    case 2:
> +        uint8_t cur_val = bm->status;
> +        bm->status = (val & 0x60) | (cur_val & 1) | (cur_val & ~val & 0x06);
> +        break;
> +    }
> +}
> diff --git a/hw/ide/meson.build b/hw/ide/meson.build
> index ddcb3b28d2..38f9ae7178 100644
> --- a/hw/ide/meson.build
> +++ b/hw/ide/meson.build
> @@ -7,7 +7,7 @@ softmmu_ss.add(when: 'CONFIG_IDE_ISA', if_true: files('isa.c', 'ioport.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_MACIO', if_true: files('macio.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_MMIO', if_true: files('mmio.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_PCI', if_true: files('pci.c'))
> -softmmu_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c'))
> +softmmu_ss.add(when: 'CONFIG_IDE_PIIX', if_true: files('piix.c', 'ioport.c', 'bmdma.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_QDEV', if_true: files('qdev.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_SII3112', if_true: files('sii3112.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_VIA', if_true: files('via.c'))
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index ce89fd0aa3..8f94809eee 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -33,57 +33,12 @@
> #include "sysemu/dma.h"
>
> #include "hw/ide/pci.h"
> +#include "hw/ide/bmdma.h"
> #include "trace.h"
>
> -static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
> -{
> -    BMDMAState *bm = opaque;
> -    uint32_t val;
> -
> -    if (size != 1) {
> -        return ((uint64_t)1 << (size * 8)) - 1;
> -    }
> -
> -    switch(addr & 3) {
> -    case 0:
> -        val = bm->cmd;
> -        break;
> -    case 2:
> -        val = bm->status;
> -        break;
> -    default:
> -        val = 0xff;
> -        break;
> -    }
> -
> -    trace_bmdma_read(addr, val);
> -    return val;
> -}
> -
> -static void bmdma_write(void *opaque, hwaddr addr,
> -                        uint64_t val, unsigned size)
> -{
> -    BMDMAState *bm = opaque;
> -
> -    if (size != 1) {
> -        return;
> -    }
> -
> -    trace_bmdma_write(addr, val);
> -
> -    switch(addr & 3) {
> -    case 0:
> -        bmdma_cmd_writeb(bm, val);
> -        break;
> -    case 2:
> -        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
> -        break;
> -    }
> -}
> -
> static const MemoryRegionOps piix_bmdma_ops = {
> -    .read = bmdma_read,
> -    .write = bmdma_write,
> +    .read = intel_ide_bmdma_read,
> +    .write = intel_ide_bmdma_write,
> };
>
> static void bmdma_setup_bar(PCIIDEState *d)
> diff --git a/include/hw/ide/bmdma.h b/include/hw/ide/bmdma.h
> new file mode 100644
> index 0000000000..ce76d395f5
> --- /dev/null
> +++ b/include/hw/ide/bmdma.h
> @@ -0,0 +1,34 @@
> +/*
> + * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 and ICH6/7.
> + *
> + * Copyright (c) 2022 Liav Albani
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_IDE_BMDMA_H
> +#define HW_IDE_BMDMA_H
> +
> +#include "hw/ide/internal.h"
> +
> +uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size);
> +void intel_ide_bmdma_write(void *opaque, hwaddr addr, uint64_t val,
> +                           unsigned size);
> +
> +#endif
>

Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[BALATON Zoltan]

On Fri, 18 Feb 2022, Liav Albani wrote:
> This type of IDE controller has support for relocating the IO ports and
> doesn't use IRQ 14 and 15 but one allocated PCI IRQ for the controller.
>
> There's no x86 chipset in QEMU that will try to attach this device by
> default. It is considered a legacy-free device in the aspect of PCI bus
> resource management as the guest OS can relocate the IO ports as it sees
> fit to its needs. However, this is still a legacy device that belongs to
> chipsets from late 2000s.
>
> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
> hw/i386/Kconfig          |   2 +
> hw/ide/Kconfig           |   5 +
> hw/ide/ich6.c            | 204 +++++++++++++++++++++++++++++++++++++++
> hw/ide/meson.build       |   1 +
> include/hw/ide/pci.h     |   1 +
> include/hw/pci/pci_ids.h |   1 +
> 6 files changed, 214 insertions(+)
> create mode 100644 hw/ide/ich6.c
>
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index d22ac4a4b9..a18de2d962 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -75,6 +75,7 @@ config I440FX
>     select PCI_I440FX
>     select PIIX3
>     select IDE_PIIX
> +    select IDE_ICH6
>     select DIMM
>     select SMBIOS
>     select FW_CFG_DMA
> @@ -101,6 +102,7 @@ config Q35
>     select PCI_EXPRESS_Q35
>     select LPC_ICH9
>     select AHCI_ICH9
> +    select IDE_ICH6
>     select DIMM
>     select SMBIOS
>     select FW_CFG_DMA
> diff --git a/hw/ide/Kconfig b/hw/ide/Kconfig
> index dd85fa3619..63304325a5 100644
> --- a/hw/ide/Kconfig
> +++ b/hw/ide/Kconfig
> @@ -38,6 +38,11 @@ config IDE_VIA
>     select IDE_PCI
>     select IDE_QDEV
>
> +config IDE_ICH6
> +    bool
> +    select IDE_PCI
> +    select IDE_QDEV
> +
> config MICRODRIVE
>     bool
>     select IDE_QDEV
> diff --git a/hw/ide/ich6.c b/hw/ide/ich6.c
> new file mode 100644
> index 0000000000..8f46d3fce2
> --- /dev/null
> +++ b/hw/ide/ich6.c
> @@ -0,0 +1,204 @@
> +/*
> + * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.

This is a small thing, but if these two are the same maybe keeping this 
comment but using the ich7 name everywhere else would make it less likely 
to get it confused with ich9. I mean ich6 and ich9 is easily confused, 
while ich7 is clearly distinct. But maybe it's just me, calling it ich6 is 
also correct and can be preferred by someone else.

> + *
> + * Copyright (c) 2022 Liav Albani
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "sysemu/block-backend.h"
> +#include "sysemu/blockdev.h"
> +#include "sysemu/dma.h"
> +
> +#include "hw/ide/pci.h"
> +#include "hw/ide/bmdma.h"
> +#include "trace.h"
> +
> +static const MemoryRegionOps ich6_bmdma_ops = {
> +    .read = intel_ide_bmdma_read,
> +    .write = intel_ide_bmdma_write,
> +};
> +
> +static void bmdma_setup_bar(PCIIDEState *d)
> +{
> +    int i;
> +
> +    memory_region_init(&d->bmdma_bar, OBJECT(d), "ich6-bmdma-container", 16);
> +    for (i = 0; i < 2; i++) {
> +        BMDMAState *bm = &d->bmdma[i];
> +
> +        memory_region_init_io(&bm->extra_io, OBJECT(d), &ich6_bmdma_ops, bm,
> +                              "ich6-bmdma", 4);
> +        memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
> +        memory_region_init_io(&bm->addr_ioport, OBJECT(d),
> +                              &bmdma_addr_ioport_ops, bm, "bmdma", 4);
> +        memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
> +    }
> +}
> +
> +static void ich6_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t val,
> +                                    int l)
> +{
> +    uint32_t i;
> +
> +    pci_default_write_config(d, addr, val, l);
> +
> +    for (i = addr; i < addr + l; i++) {
> +        switch (i) {
> +        case 0x40:
> +            pci_default_write_config(d, i, 0x8000, 2);
> +            continue;
> +        case 0x42:
> +            pci_default_write_config(d, i, 0x8000, 2);
> +            continue;
> +        }
> +    }

I'm not sure what this tries to do but this for cycle looks suspicious 
here. It's also only calls pci_default_write_config() so why didn't the 
default worked and why was this override needed?

> +}
> +
> +static void ich6_ide_reset(DeviceState *dev)
> +{
> +    PCIIDEState *d = PCI_IDE(dev);
> +    PCIDevice *pd = PCI_DEVICE(d);
> +    uint8_t *pci_conf = pd->config;
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        ide_bus_reset(&d->bus[i]);
> +    }
> +
> +    /* TODO: this is the default. do not override. */
> +    pci_conf[PCI_COMMAND] = 0x00;
> +    /* TODO: this is the default. do not override. */
> +    pci_conf[PCI_COMMAND + 1] = 0x00;
> +    /* TODO: use pci_set_word */
> +    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
> +    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
> +    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
> +}
> +
> +static int pci_ich6_init_ports(PCIIDEState *d)
> +{
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> +        ide_init2(&d->bus[i], d->native_irq);
> +
> +        bmdma_init(&d->bus[i], &d->bmdma[i], d);
> +        d->bmdma[i].bus = &d->bus[i];
> +        ide_register_restart_cb(&d->bus[i]);
> +    }
> +
> +    return 0;
> +}
> +
> +static void pci_ich6_ide_realize(PCIDevice *dev, Error **errp)
> +{
> +    PCIIDEState *d = PCI_IDE(dev);
> +    uint8_t *pci_conf = dev->config;
> +    int rc;

All in all this device looks very similar to piix ide devices with only 
some differentces so I wonder if ir could be implemented as another device 
in piix.c. We already have 3 variants there: piix3-ide, piix3-ide-xen, and 
piix4-ide so maybe putting this device in piix.c could avoid some code 
duplication. In that case moving out bmdma_{read,write} were not needed 
either although maybe that's a good idea anyway to share it with other 
devices.

> +
> +    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin A */
> +
> +    /* PCI native mode-only controller, supports bus mastering */
> +    pci_conf[PCI_CLASS_PROG] = 0x85;
> +
> +    bmdma_setup_bar(d);
> +    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
> +
> +    d->native_irq = pci_allocate_irq(&d->parent_obj);

Is this irq and the new field in PCIIDEState really needed? If this device 
is using PCI interrupts could it do the same as CMD646 ide does instead?

That's all for now, I haven't checked the docs of these ide controllers so 
I'm not sure if these have switchable native and legacy modes like via has 
and we again getting the problem that we can't model that easily or these 
are really different with one having only legacy and the ich6/7 only 
native modes.

Regards.
BALATON Zoltan

> +    /* Address Map Register - Non Combined Mode, MAP.USCC = 0 */
> +    pci_conf[0x90]   = 0;
> +
> +    /* IDE Decode enabled by default */
> +    pci_set_long(pci_conf + 0x40, 0x80008000);
> +
> +    /* IDE Timing control - Disable UDMA controls */
> +    pci_set_long(pci_conf + 0x48, 0x00000000);
> +
> +    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
> +
> +    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
> +                          &d->bus[0], "ich6-ide0-data", 8);
> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
> +
> +    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[0], "ich6-ide0-cmd", 4);
> +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
> +
> +    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
> +                          &d->bus[1], "ich6-ide1-data", 8);
> +    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
> +
> +    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[1], "ich6-ide1-cmd", 4);
> +    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
> +
> +    rc = pci_ich6_init_ports(d);
> +    if (rc) {
> +        error_setg_errno(errp, -rc, "Failed to realize %s",
> +                         object_get_typename(OBJECT(dev)));
> +    }
> +}
> +
> +static void pci_ich6_ide_exitfn(PCIDevice *dev)
> +{
> +    PCIIDEState *d = PCI_IDE(dev);
> +    unsigned i;
> +
> +    for (i = 0; i < 2; ++i) {
> +        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
> +        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
> +    }
> +}
> +
> +static void ich6_ide_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    dc->reset = ich6_ide_reset;
> +    k->realize = pci_ich6_ide_realize;
> +    k->exit = pci_ich6_ide_exitfn;
> +    k->vendor_id = PCI_VENDOR_ID_INTEL;
> +    k->device_id = PCI_DEVICE_ID_INTEL_82801GB;
> +    k->class_id = PCI_CLASS_STORAGE_IDE;
> +    k->config_read = pci_default_read_config;
> +    k->config_write = ich6_pci_config_write;
> +    set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> +    dc->hotpluggable = false;
> +}
> +
> +static const TypeInfo ich6_ide_info = {
> +    .name          = "ich6-ide",
> +    .parent        = TYPE_PCI_IDE,
> +    .class_init    = ich6_ide_class_init,
> +};
> +
> +static void ich6_ide_register_types(void)
> +{
> +    type_register_static(&ich6_ide_info);
> +}
> +
> +type_init(ich6_ide_register_types)
> diff --git a/hw/ide/meson.build b/hw/ide/meson.build
> index 38f9ae7178..6899e082db 100644
> --- a/hw/ide/meson.build
> +++ b/hw/ide/meson.build
> @@ -1,5 +1,6 @@
> softmmu_ss.add(when: 'CONFIG_AHCI', if_true: files('ahci.c'))
> softmmu_ss.add(when: 'CONFIG_AHCI_ICH9', if_true: files('ich.c'))
> +softmmu_ss.add(when: 'CONFIG_IDE_ICH6', if_true: files('ich6.c', 'bmdma.c'))
> softmmu_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('ahci-allwinner.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_CMD646', if_true: files('cmd646.c'))
> softmmu_ss.add(when: 'CONFIG_IDE_CORE', if_true: files('core.c', 'atapi.c'))
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..d8bf08e728 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -53,6 +53,7 @@ struct PCIIDEState {
>     MemoryRegion bmdma_bar;
>     MemoryRegion cmd_bar[2];
>     MemoryRegion data_bar[2];
> +    qemu_irq native_irq; /* used only for ich6-ide */
> };
>
> static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index 11abe22d46..cf8767977c 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -244,6 +244,7 @@
> #define PCI_DEVICE_ID_INTEL_82371AB      0x7111
> #define PCI_DEVICE_ID_INTEL_82371AB_2    0x7112
> #define PCI_DEVICE_ID_INTEL_82371AB_3    0x7113
> +#define PCI_DEVICE_ID_INTEL_82801GB      0x27c0
>
> #define PCI_DEVICE_ID_INTEL_ICH9_0       0x2910
> #define PCI_DEVICE_ID_INTEL_ICH9_1       0x2917
>

Re: [PATCH v2 1/2] hw/ide: split bmdma read and write functions from piix.c

[Liav Albani]

On 2/19/22 02:12, BALATON Zoltan wrote:
> On Fri, 18 Feb 2022, Liav Albani wrote:
>> This is a preparation before implementing another PCI IDE controller
>> that relies on these functions, so these can be shared between both
>> implementations.
>>
>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>> ---
>> hw/ide/bmdma.c         | 84 ++++++++++++++++++++++++++++++++++++++++++
>> hw/ide/meson.build     |  2 +-
>> hw/ide/piix.c          | 51 ++-----------------------
>> include/hw/ide/bmdma.h | 34 +++++++++++++++++
>> 4 files changed, 122 insertions(+), 49 deletions(-)
>> create mode 100644 hw/ide/bmdma.c
>> create mode 100644 include/hw/ide/bmdma.h
>>
>> diff --git a/hw/ide/bmdma.c b/hw/ide/bmdma.c
>> new file mode 100644
>> index 0000000000..d12ed730dd
>> --- /dev/null
>> +++ b/hw/ide/bmdma.c
>> @@ -0,0 +1,84 @@
>> +/*
>> + * QEMU IDE Emulation: Intel PCI Bus master IDE support for PIIX3/4 
>> and ICH6/7.
>> + *
>> + * Copyright (c) 2003 Fabrice Bellard
>> + * Copyright (c) 2006 Openedhand Ltd.
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a copy
>> + * of this software and associated documentation files (the 
>> "Software"), to deal
>> + * in the Software without restriction, including without limitation 
>> the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or sell
>> + * copies of the Software, and to permit persons to whom the 
>> Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be 
>> included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT 
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
>> DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/pci/pci.h"
>> +#include "migration/vmstate.h"
>> +#include "qapi/error.h"
>> +#include "qemu/module.h"
>> +#include "sysemu/block-backend.h"
>> +#include "sysemu/blockdev.h"
>> +#include "sysemu/dma.h"
>> +
>> +#include "hw/ide/bmdma.h"
>> +#include "hw/ide/pci.h"
>> +#include "trace.h"
>
> Are you sure you need all these includes? I haven't checked but I 
> think there may be some unneeded ones here. On the other hand I'm not 
> sure putting these in a new file is worth it. There are already some 
> bmdma_* functions in hw/ide/pci.c which are declared in 
> include/hw/ide/pci.h so you could just move these there too then no 
> new file, new header nor changes to meson.build is needed in this patch..
>
Good question, probably not. I'll try to build without them, adding only 
what seems necessary to complete the build. Also, I think adding those 
functions to hw/ide/pci.c is a very good idea as it will make the patch 
smaller and it also makes total sense to me - Bus Master capabilities 
only appear on PCI IDE controllers and not on the ISA IDE controller 
(AFAIK).

It will happen in v3 :)

>> +
>> +uint64_t intel_ide_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>
> As I said before these aren't intel specific as the same functions 
> also appear in other ide controllers such as via.c so maybe a better 
> name would be bmdma_default_read and bmdma_default_write or somehting 
> similar so these can be also reused by other non-intel devices too.
>
Right, I see now that via.c uses the exact same functions... I'll change 
it in v3. The names bmdma_default_read and bmdma_default_write seem like 
a good choice to me.
> Regards,
> BALATON Zoltan
Thanks for the review!

Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[Liav Albani]

On 2/19/22 02:50, BALATON Zoltan wrote:
>> +/*
>> + * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
>
> This is a small thing, but if these two are the same maybe keeping 
> this comment but using the ich7 name everywhere else would make it 
> less likely to get it confused with ich9. I mean ich6 and ich9 is 
> easily confused, while ich7 is clearly distinct. But maybe it's just 
> me, calling it ich6 is also correct and can be preferred by someone else.
ICH6 and ICH7 IDE controllers are quite the same as far as I know. I 
could change it, but then one could argue that the name ich6-ide seems 
like "ich9-ide", so not sure if we can really go on this path.
>
>> +static void ich6_pci_config_write(PCIDevice *d, uint32_t addr, 
>> uint32_t val,
>> +                                    int l)
>> +{
>> +    uint32_t i;
>> +
>> +    pci_default_write_config(d, addr, val, l);
>> +
>> +    for (i = addr; i < addr + l; i++) {
>> +        switch (i) {
>> +        case 0x40:
>> +            pci_default_write_config(d, i, 0x8000, 2);
>> +            continue;
>> +        case 0x42:
>> +            pci_default_write_config(d, i, 0x8000, 2);
>> +            continue;
>> +        }
>> +    }
>
> I'm not sure what this tries to do but this for cycle looks suspicious 
> here. It's also only calls pci_default_write_config() so why didn't 
> the default worked and why was this override needed?
>
It's just a loop to ensure that if the guest OS tries to disable an IDE 
channel from the PCI config space, it seems "stuck" on enabled. I should 
probably put a comment on this to explain this, but I definitely don't 
want the guest OS to be able to disable any IDE channel for now (on real 
hardware, it does that, but I think we can either skip this feature or 
implement in a future patch, as Linux deals with the controller just fine).
>> +}
>> +
>> +static void ich6_ide_reset(DeviceState *dev)
>> +{
>> +    PCIIDEState *d = PCI_IDE(dev);
>> +    PCIDevice *pd = PCI_DEVICE(d);
>> +    uint8_t *pci_conf = pd->config;
>> +    int i;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        ide_bus_reset(&d->bus[i]);
>> +    }
>> +
>> +    /* TODO: this is the default. do not override. */
>> +    pci_conf[PCI_COMMAND] = 0x00;
>> +    /* TODO: this is the default. do not override. */
>> +    pci_conf[PCI_COMMAND + 1] = 0x00;
>> +    /* TODO: use pci_set_word */
>> +    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
>> +    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
>> +    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
>> +}
>> +
>> +static int pci_ich6_init_ports(PCIIDEState *d)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> +        ide_init2(&d->bus[i], d->native_irq);
>> +
>> +        bmdma_init(&d->bus[i], &d->bmdma[i], d);
>> +        d->bmdma[i].bus = &d->bus[i];
>> +        ide_register_restart_cb(&d->bus[i]);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void pci_ich6_ide_realize(PCIDevice *dev, Error **errp)
>> +{
>> +    PCIIDEState *d = PCI_IDE(dev);
>> +    uint8_t *pci_conf = dev->config;
>> +    int rc;
>
> All in all this device looks very similar to piix ide devices with 
> only some differentces so I wonder if ir could be implemented as 
> another device in piix.c. We already have 3 variants there: piix3-ide, 
> piix3-ide-xen, and piix4-ide so maybe putting this device in piix.c 
> could avoid some code duplication. In that case moving out 
> bmdma_{read,write} were not needed either although maybe that's a good 
> idea anyway to share it with other devices.
>
As for putting the ich6-ide code in piix.c  - I'm not against it. 
Although, in the long run, if I send more patches to QEMU, I rather 
split the files a bit more in the /hw/ide directory as there's a lot of 
code duplication to solve.
So, what we could do instead, is to share more code between the devices 
and still keep them in separate files.

As for moving out the bmdma_{write,read} functions - I'm still in the 
previous mind that we need to move them out as via.c shares the same 
code but currently has code duplications for it, and that I want to 
solve as part of making the IDE code more clean and to add more features.

However, if it seems necessary to do this cleanup before implementing 
this ich6-ide controller, I'm more than happy to wait on this, send a 
patch to solve and clean up some issues in the IDE code, and then 
re-send this as v3.
I might even consider doing that now if nobody rejects this idea :)

>> +
>> +    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin A */
>> +
>> +    /* PCI native mode-only controller, supports bus mastering */
>> +    pci_conf[PCI_CLASS_PROG] = 0x85;
>> +
>> +    bmdma_setup_bar(d);
>> +    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>> +
>> +    d->native_irq = pci_allocate_irq(&d->parent_obj);
>
> Is this irq and the new field in PCIIDEState really needed? If this 
> device is using PCI interrupts could it do the same as CMD646 ide does 
> instead?
>
I looked into how cmd646.c does that, but it creates that with the 
qdev_init_gpio_in function. The AHCI controller seems to use 
pci_allocate_irq function (in ich.c), as well as other hardware devices 
in QEMU, but the rtl8139 device doesn't have such field and still works, 
so not sure what to do and how to make it to work in the best, simple 
way here.
> That's all for now, I haven't checked the docs of these ide 
> controllers so I'm not sure if these have switchable native and legacy 
> modes like via has and we again getting the problem that we can't 
> model that easily or these are really different with one having only 
> legacy and the ich6/7 only native modes.

If I recall correctly, on my ICH7 test machine you can switch between 
native and legacy mode, but it doesn't mean that there couldn't be a 
device on some motherboard in 2009 that doesn't allow you to switch 
between legacy and native mode. However, I rather not put the option 
because I want software to deal with a PCI device that is free from 
legacy IO ports completely. Also, I am pretty sure that if I change the 
settings in the BIOS menu then it could be stuck to native mode (if for 
example, I enable both the PATA connector and the 4 SATA ports), but 
still not sure about this. This is a feature I'll look into it in the 
future, as I definitely want to improve on this and maybe allow the user 
to configure the ich6-ide device to have a different MAP register to 
simulate 4 SATA ports and 2 PATA connections.

I'll wait a few more days to let other people send comments on this 
before sending v3. Thanks again for the valuable comments :)

Best regards,
Liav

>
> Regards.
> BALATON Zoltan

Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[BALATON Zoltan]

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

On Sat, 19 Feb 2022, Liav Albani wrote:
> On 2/19/22 02:50, BALATON Zoltan wrote:
>>> +/*
>>> + * QEMU IDE Emulation: PCI ICH6/ICH7 IDE support.
>> 
>> This is a small thing, but if these two are the same maybe keeping this 
>> comment but using the ich7 name everywhere else would make it less likely 
>> to get it confused with ich9. I mean ich6 and ich9 is easily confused, 
>> while ich7 is clearly distinct. But maybe it's just me, calling it ich6 is 
>> also correct and can be preferred by someone else.
> ICH6 and ICH7 IDE controllers are quite the same as far as I know. I could 
> change it, but then one could argue that the name ich6-ide seems like 
> "ich9-ide", so not sure if we can really go on this path.

I think we don't actually have ich9-ide, we only have piix3, piix4 and 
ahci, the latter is used by ich9. I just said that calling this new device 
ich7-ide instead of ich6-ide would make it more clear it has nothing to do 
with ich9. It's already clear with calling it ich6 unless somebody 
misreads that which is easier with 6 than 7. Also according to 
https://en.wikipedia.org/wiki/I/O_Controller_Hub the native mode PATA 
controller first appeared in ICH3 but that says limited so maybe it was 
changed later. I don't know anthing about these so can't tell which are 
similar. Also if your reference system you're emulating is actually an 
ICH7 then calling it that would make sense to me.

>>> +static void ich6_pci_config_write(PCIDevice *d, uint32_t addr, uint32_t 
>>> val,
>>> +                                    int l)
>>> +{
>>> +    uint32_t i;
>>> +
>>> +    pci_default_write_config(d, addr, val, l);
>>> +
>>> +    for (i = addr; i < addr + l; i++) {
>>> +        switch (i) {
>>> +        case 0x40:
>>> +            pci_default_write_config(d, i, 0x8000, 2);
>>> +            continue;
>>> +        case 0x42:
>>> +            pci_default_write_config(d, i, 0x8000, 2);
>>> +            continue;
>>> +        }
>>> +    }
>> 
>> I'm not sure what this tries to do but this for cycle looks suspicious 
>> here. It's also only calls pci_default_write_config() so why didn't the 
>> default worked and why was this override needed?
>> 
> It's just a loop to ensure that if the guest OS tries to disable an IDE 
> channel from the PCI config space, it seems "stuck" on enabled. I should 
> probably put a comment on this to explain this, but I definitely don't want 
> the guest OS to be able to disable any IDE channel for now (on real hardware, 
> it does that, but I think we can either skip this feature or implement in a 
> future patch, as Linux deals with the controller just fine).

I still don't get what this tries to achieve but I think it's very likely 
wrong so if it's not needed it's better to just drop it for now. It can 
also be addressed in a follow up patch later.

>>> +}
>>> +
>>> +static void ich6_ide_reset(DeviceState *dev)
>>> +{
>>> +    PCIIDEState *d = PCI_IDE(dev);
>>> +    PCIDevice *pd = PCI_DEVICE(d);
>>> +    uint8_t *pci_conf = pd->config;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < 2; i++) {
>>> +        ide_bus_reset(&d->bus[i]);
>>> +    }
>>> +
>>> +    /* TODO: this is the default. do not override. */
>>> +    pci_conf[PCI_COMMAND] = 0x00;
>>> +    /* TODO: this is the default. do not override. */
>>> +    pci_conf[PCI_COMMAND + 1] = 0x00;
>>> +    /* TODO: use pci_set_word */
>>> +    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
>>> +    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
>>> +    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
>>> +}
>>> +
>>> +static int pci_ich6_init_ports(PCIIDEState *d)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < 2; i++) {
>>> +        ide_bus_init(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>>> +        ide_init2(&d->bus[i], d->native_irq);
>>> +
>>> +        bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>> +        d->bmdma[i].bus = &d->bus[i];
>>> +        ide_register_restart_cb(&d->bus[i]);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void pci_ich6_ide_realize(PCIDevice *dev, Error **errp)
>>> +{
>>> +    PCIIDEState *d = PCI_IDE(dev);
>>> +    uint8_t *pci_conf = dev->config;
>>> +    int rc;
>> 
>> All in all this device looks very similar to piix ide devices with only 
>> some differentces so I wonder if ir could be implemented as another device 
>> in piix.c. We already have 3 variants there: piix3-ide, piix3-ide-xen, and 
>> piix4-ide so maybe putting this device in piix.c could avoid some code 
>> duplication. In that case moving out bmdma_{read,write} were not needed 
>> either although maybe that's a good idea anyway to share it with other 
>> devices.
>> 
> As for putting the ich6-ide code in piix.c  - I'm not against it. Although,

The real question if this controller is similiar enough to piix or it's 
different just for this initial version it's reusing most code but this 
will change in the future. If the latter then putting it in a separate 
file is better, if it will stay like it then putting it in piix to share 
code already there is better.

> in the long run, if I send more patches to QEMU, I rather split the files a 
> bit more in the /hw/ide directory as there's a lot of code duplication to 
> solve.
> So, what we could do instead, is to share more code between the devices and 
> still keep them in separate files.

I just wanted to avoid more code duplication by adding new files that will 
look like the devices that's already there. But if this is just a first 
step and you'll send more patches to implement more features that will 
make it more different, then having it in a separate file is better. But I 
could only judge from the patch so far which is almost identical to piix, 
only differentce is irq and port handling so this could be a variant of 
piix then as well if that's all you want for this device.

> As for moving out the bmdma_{write,read} functions - I'm still in the 
> previous mind that we need to move them out as via.c shares the same code but 
> currently has code duplications for it, and that I want to solve as part of 
> making the IDE code more clean and to add more features.
>
> However, if it seems necessary to do this cleanup before implementing this 
> ich6-ide controller, I'm more than happy to wait on this, send a patch to 
> solve and clean up some issues in the IDE code, and then re-send this as v3.
> I might even consider doing that now if nobody rejects this idea :)

It can be in the same series but still good to have as separate patch so 
it can be reviewed independently and maybe also merged independently. The 
maintainer can always squash patches together later but separating them is 
usually only the original contributor can easily and best do.

>>> +
>>> +    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin A */
>>> +
>>> +    /* PCI native mode-only controller, supports bus mastering */
>>> +    pci_conf[PCI_CLASS_PROG] = 0x85;
>>> +
>>> +    bmdma_setup_bar(d);
>>> +    pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>>> +
>>> +    d->native_irq = pci_allocate_irq(&d->parent_obj);
>> 
>> Is this irq and the new field in PCIIDEState really needed? If this device 
>> is using PCI interrupts could it do the same as CMD646 ide does instead?
>> 
> I looked into how cmd646.c does that, but it creates that with the 
> qdev_init_gpio_in function. The AHCI controller seems to use pci_allocate_irq 
> function (in ich.c), as well as other hardware devices in QEMU, but the 
> rtl8139 device doesn't have such field and still works, so not sure what to 
> do and how to make it to work in the best, simple way here.

The cmd646 is similar as it's a PCI device and shares the same PCIIDEState 
so I think doing the same is best for cnosistency and to avoid needing to 
change the shared PCIIDEState that should be a generic PCI IDE controller 
model but still has one cmd646 specific field. Using qdev_init_gpio_in 
attaches the irq to the device so it will be kept track of without needing 
to store it anywhere so I think it's not a bad way to do it for this case. 
AHCI has its own device state and stores the irq there, I don't know what 
rtl8139 does. There may be multiple correct ways to do it but following 
similar examples is probably a good idea.

>> That's all for now, I haven't checked the docs of these ide controllers so 
>> I'm not sure if these have switchable native and legacy modes like via has 
>> and we again getting the problem that we can't model that easily or these 
>> are really different with one having only legacy and the ich6/7 only native 
>> modes.
>
> If I recall correctly, on my ICH7 test machine you can switch between native 
> and legacy mode, but it doesn't mean that there couldn't be a device on some 
> motherboard in 2009 that doesn't allow you to switch between legacy and

OK so we probably again have a case we had with via-ide which also has 
legacy mode where it behaves like an ISA controller using legacy ports and 
irq 14-15 or a native mode where ports can be set by PCI BARs and irqs may 
or may not use PCI interrupt. Due to how ISA code is organised, which is 
an old part of QEMU we can't easily implement this (the main problem is 
once ISA ports are created we can't delete them so we're stuck in legacy 
mode) so what we have is only emulating one mode for these controller 
models. I think the cmd646 is completely PCI device (not sure it had a 
legacy mode), the via-ide is native mode but with ISA interrupts and piix 
is probably only legacy mode but not sure if it had a native mode. Maybe 
this ich-ide is the native mode of piix? In that case could it be modelled 
that way adding an option to piix so it can be set up in native mode or 
legacy mode? We would still not be able to switch between them from the 
guest as on real hardware but machines or users could decide when creating 
the device what mode they want. Or is the ich ide completely different 
from piix?

> native mode. However, I rather not put the option because I want software to 
> deal with a PCI device that is free from legacy IO ports completely. Also, I 
> am pretty sure that if I change the settings in the BIOS menu then it could 
> be stuck to native mode (if for example, I enable both the PATA connector and 
> the 4 SATA ports), but still not sure about this. This is a feature I'll look 
> into it in the future, as I definitely want to improve on this and maybe 
> allow the user to configure the ich6-ide device to have a different MAP 
> register to simulate 4 SATA ports and 2 PATA connections.

OK then it will probably be different from piix so maybe this justifies it 
putting it in a different file and code that is similar now will become 
more different in the future.

> I'll wait a few more days to let other people send comments on this before 
> sending v3. Thanks again for the valuable comments :)

That's probably a good idea, I hope others will have some time to comment 
as well. I'm trying to help but I can only give ideas, not definitive 
answers for your questions.

Regards.
BALATON Zoltan

Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[Gerd Hoffmann]

  Hi,

> > ICH6 and ICH7 IDE controllers are quite the same as far as I know. I
> > could change it, but then one could argue that the name ich6-ide seems
> > like "ich9-ide", so not sure if we can really go on this path.
> 
> I think we don't actually have ich9-ide, we only have piix3, piix4 and ahci,
> the latter is used by ich9. I just said that calling this new device
> ich7-ide instead of ich6-ide would make it more clear it has nothing to do
> with ich9.

Well, there actually is ich9-ide in physical hardware.  And it's quite
simliar for all ich6 -> ich9 (and possibly more) physical hardware.

The hardware implements both ide and sata.  Typically the bios setup
offers to pick ide or sata mode for the storage controller, and on boot
the chipset is configured accordingly by the firmware.

qemu never bothered to implement ide mode for q35/ich9.  When a guest OS
is so old that it doesn't come with a sata driver there is the option to
just use the 'pc' machine type.  And usually that's the better choice
anyway because these old guests tend to have problems with other q35
components too.

So I'm wondering why you implement ich{6,7,9}-ide in the first place?
What is the use case / benefit?

take care,
  Gerd

Re: [PATCH v2 2/2] hw/ide: add ich6 ide controller device emulation

[Liav Albani]

On 2/21/22 13:33, Gerd Hoffmann wrote:
>    Hi,
>
>>> ICH6 and ICH7 IDE controllers are quite the same as far as I know. I
>>> could change it, but then one could argue that the name ich6-ide seems
>>> like "ich9-ide", so not sure if we can really go on this path.
>> I think we don't actually have ich9-ide, we only have piix3, piix4 and ahci,
>> the latter is used by ich9. I just said that calling this new device
>> ich7-ide instead of ich6-ide would make it more clear it has nothing to do
>> with ich9.
> Well, there actually is ich9-ide in physical hardware.  And it's quite
> simliar for all ich6 -> ich9 (and possibly more) physical hardware.
I know, but I based it on Intel documentation and the ICH7 machine I 
have from 2009. Also, according to libata in Linux, the enum of 
piix_controller_ids include ich5, ich6 and ich8 (and much more, these 
are more relevant in the list for this reason), and by looking into what 
ich6_sata is being used for, I can see it matches the IDE controller PCI 
ID on the ICH7 machine I use, so that's another reason to choose this name.
> The hardware implements both ide and sata.  Typically the bios setup
> offers to pick ide or sata mode for the storage controller, and on boot
> the chipset is configured accordingly by the firmware.
Yes, and then the BIOS configures the MAP register to indicate the setup 
that it was decided by the user in the BIOS configuration.
However, setting the MAP register to zero is a valid value - it 
indicates you have only SATA connectors in use (at least that's what 
Linux thinks), according to the Intel ICH5 Serial ATA Controller RPM.
> qemu never bothered to implement ide mode for q35/ich9.  When a guest OS
> is so old that it doesn't come with a sata driver there is the option to
> just use the 'pc' machine type.  And usually that's the better choice
> anyway because these old guests tend to have problems with other q35
> components too.
That's true if you care about giving emulation only for the benefits of 
the guest (so you only care about supporting what the guest OS can 
expect from standard IDE controller, not edge cases), but my approach is 
looking at a very different goal.
> So I'm wondering why you implement ich{6,7,9}-ide in the first place?
> What is the use case / benefit?

I talked about it in the last patch about this topic I've sent (v1 to be 
precise), but let me describe it again :)
I'm a SerenityOS developer, as you might remember or not, I've talked to 
you (Gerd) in the past about SeaBIOS topics related to the OS off-list.
As I said before in this mail, I tend to test the SerenityOS kernel on 
the ICH7 machine I have from 2009. That machine has 4 SATA ports and you 
can connect 2 PATA devices (as one parallel cable can be used to connect 
two devices at once, to one connector on the mainboard).

I've seen that the kernel struggled to use the IDE controller - the main 
problem we have is long timeouts because of some problematic pattern in 
our code. However, on regular QEMU PC and Q35 machines everything boots 
fine. When I wrote this emulation component, I saw the same problem I 
had on the bare metal machine, so it is a convenient feature for me to 
debug this problem without having to use the bare metal machine - it 
helps saving lots of time for me by avoiding the need to compile a 
kernel, put it on the SATA harddrive and try to boot it in the rapid 
compile-boot-test cycle I have here.

I thought it might be beneficial for other OS developers and hobbyists 
like me to have such component. For now, it's an IDE 
ICH5/6/7/9-compatible controller, supporting only PCI IDE native mode - 
which means you can relocate the resources to anything you want on the 
IO space, so it's a legacy-free device in the sense of PCI bus resource 
management, but still a legacy device that to use it on bare metal you 
need a machine from late 2000s.

Also, I do see a point in expanding this controller with more features. 
For example, some ICH6 IDE controllers had AHCI mode within them, so you 
could actually enable the AHCI mode and disable IDE mode if you know 
what you're doing - you will probably need to assign the IDE PCI BARs 
correctly first if you want IDE mode in such controller, or ignore it 
and go with AHCI mode instead. Also, this emulation component is only 
about PCI IDE native mode currently, but we can easily put it that you 
can switch channels between compatibility mode and native mode if wanted 
to. My ICH7 test machine has such controller - it allows you to switch 
between the two modes, so the OS can decide what to do with the IDE 
controller according to its needs.

> take care,
>    Gerd
>
Best regards,
Liav