[PATCH] hw/ide: implement ich6 ide controller support

[PATCH] hw/ide: implement ich6 ide controller support

[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.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 hw/i386/Kconfig          |   2 +
 hw/ide/Kconfig           |   5 +
 hw/ide/bmdma.c           |  83 +++++++++++++++
 hw/ide/ich6.c            | 211 +++++++++++++++++++++++++++++++++++++++
 hw/ide/meson.build       |   3 +-
 hw/ide/piix.c            |  50 +---------
 include/hw/ide/pci.h     |   5 +
 include/hw/pci/pci_ids.h |   1 +
 8 files changed, 311 insertions(+), 49 deletions(-)
 create mode 100644 hw/ide/bmdma.c
 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/bmdma.c b/hw/ide/bmdma.c
new file mode 100644
index 0000000000..979f5974fd
--- /dev/null
+++ b/hw/ide/bmdma.c
@@ -0,0 +1,83 @@
+/*
+ * QEMU IDE Emulation: PCI PIIX3/4 support.
+ *
+ * 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/pci.h"
+#include "trace.h"
+
+uint64_t piix_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 piix_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/ich6.c b/hw/ide/ich6.c
new file mode 100644
index 0000000000..21dbc4906b
--- /dev/null
+++ b/hw/ide/ich6.c
@@ -0,0 +1,211 @@
+/*
+ * 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 "trace.h"
+
+
+
+static const MemoryRegionOps ich6_bmdma_ops = {
+    .read = piix_bmdma_read,
+    .write = piix_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 uint32_t ich6_pci_config_read(PCIDevice *d,
+                                       uint32_t address, int len)
+{
+    return pci_default_read_config(d, address, len);
+}
+
+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 = ich6_pci_config_read;
+    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 ddcb3b28d2..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'))
@@ -7,7 +8,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..5a747e5845 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -35,55 +35,9 @@
 #include "hw/ide/pci.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 = piix_bmdma_read,
+    .write = piix_bmdma_write,
 };
 
 static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..00c86d9ef5 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)
@@ -62,6 +63,10 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 }
 
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
+
+uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size);
+void piix_bmdma_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
+
 void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
 void pci_ide_create_devs(PCIDevice *dev);
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] hw/ide: implement ich6 ide controller support

[BALATON Zoltan]

On Sat, 5 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.

I haven't looked at in detail so only a few comments I've got while 
reading it. What machine needs this? In QEMU I think we only have piix and 
ich9 emulated for pc and q35 machines but maybe ich6 is also used by some 
machine I don't know about. Otherwise it looks odd to have ide part of 
ich6 but not the other parts of this chip.

> Signed-off-by: Liav Albani <liavalb@gmail.com>
> ---
> hw/i386/Kconfig          |   2 +
> hw/ide/Kconfig           |   5 +
> hw/ide/bmdma.c           |  83 +++++++++++++++
> hw/ide/ich6.c            | 211 +++++++++++++++++++++++++++++++++++++++
> hw/ide/meson.build       |   3 +-
> hw/ide/piix.c            |  50 +---------
> include/hw/ide/pci.h     |   5 +
> include/hw/pci/pci_ids.h |   1 +
> 8 files changed, 311 insertions(+), 49 deletions(-)
> create mode 100644 hw/ide/bmdma.c
> 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/bmdma.c b/hw/ide/bmdma.c
> new file mode 100644
> index 0000000000..979f5974fd
> --- /dev/null
> +++ b/hw/ide/bmdma.c
> @@ -0,0 +1,83 @@
> +/*
> + * QEMU IDE Emulation: PCI PIIX3/4 support.
> + *
> + * 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/pci.h"
> +#include "trace.h"
> +
> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)

Moving these functions to avoid duplication is a good idea but a couple of 
points:

- Maybe this should be a separate patch just for moving these out then 
another patch adding ich6 for easier review of separate changes.

- There are already several bmdma_* functions in pci.c and you add these 
to pci.h so maybe these should be moved near there in pci.c instead. (Or 
more of those bmdma_* functions moved in this new file and add its own 
header?)

- This is not piix specific so the name probably should not say that. 
Maybe somthing along the lines of pci_default_read_config so 
bmdma_default_read or similar. In fact it also appears in via.c and a more 
complex version in cmd646.c which could still reuse functions like these 
like we do with pci config write. Converting those other two to use the 
newly split off functions instead of duplicating it could be done in a 
follow up patch, if you don't want to do that I may look at via-ide but a 
patch is welcome if you have time for that, unless others think otherwise 
and we'll take a different route.

> +{
> +    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 piix_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/ich6.c b/hw/ide/ich6.c
> new file mode 100644
> index 0000000000..21dbc4906b
> --- /dev/null
> +++ b/hw/ide/ich6.c
> @@ -0,0 +1,211 @@
> +/*
> + * 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 "trace.h"
> +
> +
> +
> +static const MemoryRegionOps ich6_bmdma_ops = {
> +    .read = piix_bmdma_read,
> +    .write = piix_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 uint32_t ich6_pci_config_read(PCIDevice *d,
> +                                       uint32_t address, int len)
> +{
> +    return pci_default_read_config(d, address, len);
> +}

Why do you override this if you have nothing to do in it? Just use 
pci_default_read_config and only override ich6_pci_config_write where you 
actually has something to add to the default.

Maybe also wait for a few days for other's comments (especially the 
maintainer's opinion on this) before sending a v2 so you get all comments 
and see what to do.

Regards,
BALATON Zoltan

> +
> +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 = ich6_pci_config_read;
> +    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 ddcb3b28d2..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'))
> @@ -7,7 +8,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..5a747e5845 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -35,55 +35,9 @@
> #include "hw/ide/pci.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 = piix_bmdma_read,
> +    .write = piix_bmdma_write,
> };
>
> static void bmdma_setup_bar(PCIIDEState *d)
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..00c86d9ef5 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)
> @@ -62,6 +63,10 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
> }
>
> void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
> +
> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size);
> +void piix_bmdma_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
> +
> void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
> extern MemoryRegionOps bmdma_addr_ioport_ops;
> void pci_ide_create_devs(PCIDevice *dev);
> 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] hw/ide: implement ich6 ide controller support

[Liav Albani]

On 2/5/22 17:48, BALATON Zoltan wrote:
> On Sat, 5 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.
>
> I haven't looked at in detail so only a few comments I've got while 
> reading it. What machine needs this? In QEMU I think we only have piix 
> and ich9 emulated for pc and q35 machines but maybe ich6 is also used 
> by some machine I don't know about. Otherwise it looks odd to have ide 
> part of ich6 but not the other parts of this chip.
>
Hi BALATON,

This is my first patch to QEMU and the first time I send patches over 
the mail. I sent my github tree to John Snow (the maintainer of the IDE 
code in QEMU) for advice if I should send them here and I was encouraged 
to do that.
For the next time patch I'll put a note on writing a descriptive cover 
letter as it could have put more valuable details on why I sent this patch.

There's no such machine type emulating the ICH6 chipset in QEMU. 
However, I wrote this emulation component as a test for the SerenityOS 
kernel because I have a machine from 2009 which has
an ICH7 southbridge, so, I wanted to emulate such device with QEMU to 
ease development on it.

I found out that Linux with libata was using the controller without any 
noticeable problems, but the SerenityOS kernel struggled to use this 
device, so I decided that
I should send this patch to get it merged and then I can use it locally 
and maybe other people will benefit from it.

In regard to other components of the ICH6 chipset - I don't think it's 
worth anybody's time to actually implement them as the ICH9 chipset is 
quite close to what the ICH6 chipset offers as far as I can tell.
The idea of implementing ich6-ide controller was to enable the option of 
people like me and other OS developers to ensure their kernels operate 
correctly on such type of device,
which is legacy-free device in the aspect of PCI bus resource management 
but still is a legacy device which belongs to chipsets of late 2000s.

>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>> ---
>> hw/i386/Kconfig          |   2 +
>> hw/ide/Kconfig           |   5 +
>> hw/ide/bmdma.c           |  83 +++++++++++++++
>> hw/ide/ich6.c            | 211 +++++++++++++++++++++++++++++++++++++++
>> hw/ide/meson.build       |   3 +-
>> hw/ide/piix.c            |  50 +---------
>> include/hw/ide/pci.h     |   5 +
>> include/hw/pci/pci_ids.h |   1 +
>> 8 files changed, 311 insertions(+), 49 deletions(-)
>> create mode 100644 hw/ide/bmdma.c
>> 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/bmdma.c b/hw/ide/bmdma.c
>> new file mode 100644
>> index 0000000000..979f5974fd
>> --- /dev/null
>> +++ b/hw/ide/bmdma.c
>> @@ -0,0 +1,83 @@
>> +/*
>> + * QEMU IDE Emulation: PCI PIIX3/4 support.
>> + *
>> + * 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/pci.h"
>> +#include "trace.h"
>> +
>> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>
> Moving these functions to avoid duplication is a good idea but a 
> couple of points:
>
> - Maybe this should be a separate patch just for moving these out then 
> another patch adding ich6 for easier review of separate changes.
>
I don't mind splitting this patch into a couple of "commits" (or series 
of diffs? I don't understand the terminology yet) so one "commit" will 
be a preparation - extract the
functions to a separate file, and the next diff can implement the 
ich6-ide controller.
> - There are already several bmdma_* functions in pci.c and you add 
> these to pci.h so maybe these should be moved near there in pci.c 
> instead. (Or more of those bmdma_* functions moved in this new file 
> and add its own header?)
>
I'm not sure what is the best approach but maybe the latter suggestion 
(new file and its own header) seems quite good to me.
> - This is not piix specific so the name probably should not say that. 
> Maybe somthing along the lines of pci_default_read_config so 
> bmdma_default_read or similar. In fact it also appears in via.c and a 
> more complex version in cmd646.c which could still reuse functions 
> like these like we do with pci config write. Converting those other 
> two to use the newly split off functions instead of duplicating it 
> could be done in a follow up patch, if you don't want to do that I may 
> look at via-ide but a patch is welcome if you have time for that, 
> unless others think otherwise and we'll take a different route.
>
I want to create the best possible patch so if it's desirable to rename 
it to something more generic, that's OK for me.
I'd like to address some of the issues you mentioned (such as avoiding 
duplicates in how we use bus master DMA in the ide code) in future 
patches though, so if possible, let's keep it simple now :)

>>
>> +static uint32_t ich6_pci_config_read(PCIDevice *d,
>> +                                       uint32_t address, int len)
>> +{
>> +    return pci_default_read_config(d, address, len);
>> +}
>
> Why do you override this if you have nothing to do in it? Just use 
> pci_default_read_config and only override ich6_pci_config_write where 
> you actually has something to add to the default.
>
You're right, I should not override this function. It was probably me 
trying (I uploaded this patch to GitHub about 2 weeks ago so I don't 
remember precisely) to override it for the sake of returning a value of 
0x8000 when the guest OS tries to figure out if the IDE channel is 
disabled or not in the 0x40 and 0x42 registers in the PCI config space 
of the device.
> Maybe also wait for a few days for other's comments (especially the 
> maintainer's opinion on this) before sending a v2 so you get all 
> comments and see what to do.
>
Thank you very much for putting time into reviewing this! :)

Best regards,
Liav

Re: [PATCH] hw/ide: implement ich6 ide controller support

[BALATON Zoltan]

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

Hello,

On Sat, 5 Feb 2022, Liav Albani wrote:
> On 2/5/22 17:48, BALATON Zoltan wrote:
>> On Sat, 5 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.
>> 
>> I haven't looked at in detail so only a few comments I've got while reading 
>> it. What machine needs this? In QEMU I think we only have piix and ich9 
>> emulated for pc and q35 machines but maybe ich6 is also used by some 
>> machine I don't know about. Otherwise it looks odd to have ide part of ich6 
>> but not the other parts of this chip.
>> 
> Hi BALATON,
>
> This is my first patch to QEMU and the first time I send patches over the 
> mail. I sent my github tree to John Snow (the maintainer of the IDE code in 
> QEMU) for advice if I should send them here and I was encouraged to do that.

Welcome and thanks a lot for taking time to contribute and share your 
results. In case you're not yet aware, these docs should explain how 
patches are handled on the list:

https://www.qemu.org/docs/master/devel/submitting-a-patch.html

> For the next time patch I'll put a note on writing a descriptive cover letter 
> as it could have put more valuable details on why I sent this patch.
>
> There's no such machine type emulating the ICH6 chipset in QEMU. However, I 
> wrote this emulation component as a test for the SerenityOS kernel because I 
> have a machine from 2009 which has
> an ICH7 southbridge, so, I wanted to emulate such device with QEMU to ease 
> development on it.
>
> I found out that Linux with libata was using the controller without any 
> noticeable problems, but the SerenityOS kernel struggled to use this device, 
> so I decided that
> I should send this patch to get it merged and then I can use it locally and 
> maybe other people will benefit from it.
>
> In regard to other components of the ICH6 chipset - I don't think it's worth 
> anybody's time to actually implement them as the ICH9 chipset is quite close 
> to what the ICH6 chipset offers as far as I can tell.
> The idea of implementing ich6-ide controller was to enable the option of 
> people like me and other OS developers to ensure their kernels operate 
> correctly on such type of device,
> which is legacy-free device in the aspect of PCI bus resource management but 
> still is a legacy device which belongs to chipsets of late 2000s.

That's OK, maybe a short mention (just one sentence) in the commit message 
explaining this would help to understand why this device model was added.

>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>> ---
>>> hw/i386/Kconfig          |   2 +
>>> hw/ide/Kconfig           |   5 +
>>> hw/ide/bmdma.c           |  83 +++++++++++++++
>>> hw/ide/ich6.c            | 211 +++++++++++++++++++++++++++++++++++++++
>>> hw/ide/meson.build       |   3 +-
>>> hw/ide/piix.c            |  50 +---------
>>> include/hw/ide/pci.h     |   5 +
>>> include/hw/pci/pci_ids.h |   1 +
>>> 8 files changed, 311 insertions(+), 49 deletions(-)
>>> create mode 100644 hw/ide/bmdma.c
>>> 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/bmdma.c b/hw/ide/bmdma.c
>>> new file mode 100644
>>> index 0000000000..979f5974fd
>>> --- /dev/null
>>> +++ b/hw/ide/bmdma.c
>>> @@ -0,0 +1,83 @@
>>> +/*
>>> + * QEMU IDE Emulation: PCI PIIX3/4 support.
>>> + *
>>> + * 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/pci.h"
>>> +#include "trace.h"
>>> +
>>> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>> 
>> Moving these functions to avoid duplication is a good idea but a couple of 
>> points:
>> 
>> - Maybe this should be a separate patch just for moving these out then 
>> another patch adding ich6 for easier review of separate changes.
>> 
> I don't mind splitting this patch into a couple of "commits" (or series of 
> diffs? I don't understand the terminology yet) so one "commit" will be a 
> preparation - extract the
> functions to a separate file, and the next diff can implement the ich6-ide 
> controller.

See the docs, which should explain how to split up patches and what should 
be in one patch. This would be a patch series then. As to where to move 
these functions or how to proceed with it wait for John Snow's reply as 
he's the maintainer of this part so what he prefers is the way to go. What 
I wrote is my opinion but others may have different views so give them a 
chance to reply too then once we see what to do you can send a v2 with the 
changes that seems to be the consensus on how to best proceed.

>> - There are already several bmdma_* functions in pci.c and you add these to 
>> pci.h so maybe these should be moved near there in pci.c instead. (Or more 
>> of those bmdma_* functions moved in this new file and add its own header?)
>> 
> I'm not sure what is the best approach but maybe the latter suggestion (new 
> file and its own header) seems quite good to me.
>> - This is not piix specific so the name probably should not say that. Maybe 
>> somthing along the lines of pci_default_read_config so bmdma_default_read 
>> or similar. In fact it also appears in via.c and a more complex version in 
>> cmd646.c which could still reuse functions like these like we do with pci 
>> config write. Converting those other two to use the newly split off 
>> functions instead of duplicating it could be done in a follow up patch, if 
>> you don't want to do that I may look at via-ide but a patch is welcome if 
>> you have time for that, unless others think otherwise and we'll take a 
>> different route.
>> 
> I want to create the best possible patch so if it's desirable to rename it to 
> something more generic, that's OK for me.
> I'd like to address some of the issues you mentioned (such as avoiding 
> duplicates in how we use bus master DMA in the ide code) in future patches 
> though, so if possible, let's keep it simple now :)

Sure, this could be done afterwards too just noted it when noticed there 
are other places that use similar functions so this is a possible clean 
up that should be easy to do either as an additional patch in this series 
or a separate one later.

Regards,
BALATON Zoltan

>>> 
>>> +static uint32_t ich6_pci_config_read(PCIDevice *d,
>>> +                                       uint32_t address, int len)
>>> +{
>>> +    return pci_default_read_config(d, address, len);
>>> +}
>> 
>> Why do you override this if you have nothing to do in it? Just use 
>> pci_default_read_config and only override ich6_pci_config_write where you 
>> actually has something to add to the default.
>> 
> You're right, I should not override this function. It was probably me trying 
> (I uploaded this patch to GitHub about 2 weeks ago so I don't remember 
> precisely) to override it for the sake of returning a value of 0x8000 when 
> the guest OS tries to figure out if the IDE channel is disabled or not in the 
> 0x40 and 0x42 registers in the PCI config space of the device.
>> Maybe also wait for a few days for other's comments (especially the 
>> maintainer's opinion on this) before sending a v2 so you get all comments 
>> and see what to do.
>> 
> Thank you very much for putting time into reviewing this! :)
>
> Best regards,
> Liav
>
>

Re: [PATCH] hw/ide: implement ich6 ide controller support

[BALATON Zoltan]

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

On Sat, 5 Feb 2022, BALATON Zoltan wrote:
> Hello,

Ping? John, do you agree with my comments? Should Liav proceed to send a 
v2?

Thanks,
BALATON Zoltan

> On Sat, 5 Feb 2022, Liav Albani wrote:
>> On 2/5/22 17:48, BALATON Zoltan wrote:
>>> On Sat, 5 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.
>>> 
>>> I haven't looked at in detail so only a few comments I've got while 
>>> reading it. What machine needs this? In QEMU I think we only have piix and 
>>> ich9 emulated for pc and q35 machines but maybe ich6 is also used by some 
>>> machine I don't know about. Otherwise it looks odd to have ide part of 
>>> ich6 but not the other parts of this chip.
>>> 
>> Hi BALATON,
>> 
>> This is my first patch to QEMU and the first time I send patches over the 
>> mail. I sent my github tree to John Snow (the maintainer of the IDE code in 
>> QEMU) for advice if I should send them here and I was encouraged to do 
>> that.
>
> Welcome and thanks a lot for taking time to contribute and share your 
> results. In case you're not yet aware, these docs should explain how patches 
> are handled on the list:
>
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html
>
>> For the next time patch I'll put a note on writing a descriptive cover 
>> letter as it could have put more valuable details on why I sent this patch.
>> 
>> There's no such machine type emulating the ICH6 chipset in QEMU. However, I 
>> wrote this emulation component as a test for the SerenityOS kernel because 
>> I have a machine from 2009 which has
>> an ICH7 southbridge, so, I wanted to emulate such device with QEMU to ease 
>> development on it.
>> 
>> I found out that Linux with libata was using the controller without any 
>> noticeable problems, but the SerenityOS kernel struggled to use this 
>> device, so I decided that
>> I should send this patch to get it merged and then I can use it locally and 
>> maybe other people will benefit from it.
>> 
>> In regard to other components of the ICH6 chipset - I don't think it's 
>> worth anybody's time to actually implement them as the ICH9 chipset is 
>> quite close to what the ICH6 chipset offers as far as I can tell.
>> The idea of implementing ich6-ide controller was to enable the option of 
>> people like me and other OS developers to ensure their kernels operate 
>> correctly on such type of device,
>> which is legacy-free device in the aspect of PCI bus resource management 
>> but still is a legacy device which belongs to chipsets of late 2000s.
>
> That's OK, maybe a short mention (just one sentence) in the commit message 
> explaining this would help to understand why this device model was added.
>
>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>> ---
>>>> hw/i386/Kconfig          |   2 +
>>>> hw/ide/Kconfig           |   5 +
>>>> hw/ide/bmdma.c           |  83 +++++++++++++++
>>>> hw/ide/ich6.c            | 211 +++++++++++++++++++++++++++++++++++++++
>>>> hw/ide/meson.build       |   3 +-
>>>> hw/ide/piix.c            |  50 +---------
>>>> include/hw/ide/pci.h     |   5 +
>>>> include/hw/pci/pci_ids.h |   1 +
>>>> 8 files changed, 311 insertions(+), 49 deletions(-)
>>>> create mode 100644 hw/ide/bmdma.c
>>>> 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/bmdma.c b/hw/ide/bmdma.c
>>>> new file mode 100644
>>>> index 0000000000..979f5974fd
>>>> --- /dev/null
>>>> +++ b/hw/ide/bmdma.c
>>>> @@ -0,0 +1,83 @@
>>>> +/*
>>>> + * QEMU IDE Emulation: PCI PIIX3/4 support.
>>>> + *
>>>> + * 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/pci.h"
>>>> +#include "trace.h"
>>>> +
>>>> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>>> 
>>> Moving these functions to avoid duplication is a good idea but a couple of 
>>> points:
>>> 
>>> - Maybe this should be a separate patch just for moving these out then 
>>> another patch adding ich6 for easier review of separate changes.
>>> 
>> I don't mind splitting this patch into a couple of "commits" (or series of 
>> diffs? I don't understand the terminology yet) so one "commit" will be a 
>> preparation - extract the
>> functions to a separate file, and the next diff can implement the ich6-ide 
>> controller.
>
> See the docs, which should explain how to split up patches and what should be 
> in one patch. This would be a patch series then. As to where to move these 
> functions or how to proceed with it wait for John Snow's reply as he's the 
> maintainer of this part so what he prefers is the way to go. What I wrote is 
> my opinion but others may have different views so give them a chance to reply 
> too then once we see what to do you can send a v2 with the changes that seems 
> to be the consensus on how to best proceed.
>
>>> - There are already several bmdma_* functions in pci.c and you add these 
>>> to pci.h so maybe these should be moved near there in pci.c instead. (Or 
>>> more of those bmdma_* functions moved in this new file and add its own 
>>> header?)
>>> 
>> I'm not sure what is the best approach but maybe the latter suggestion (new 
>> file and its own header) seems quite good to me.
>>> - This is not piix specific so the name probably should not say that. 
>>> Maybe somthing along the lines of pci_default_read_config so 
>>> bmdma_default_read or similar. In fact it also appears in via.c and a more 
>>> complex version in cmd646.c which could still reuse functions like these 
>>> like we do with pci config write. Converting those other two to use the 
>>> newly split off functions instead of duplicating it could be done in a 
>>> follow up patch, if you don't want to do that I may look at via-ide but a 
>>> patch is welcome if you have time for that, unless others think otherwise 
>>> and we'll take a different route.
>>> 
>> I want to create the best possible patch so if it's desirable to rename it 
>> to something more generic, that's OK for me.
>> I'd like to address some of the issues you mentioned (such as avoiding 
>> duplicates in how we use bus master DMA in the ide code) in future patches 
>> though, so if possible, let's keep it simple now :)
>
> Sure, this could be done afterwards too just noted it when noticed there are 
> other places that use similar functions so this is a possible clean up that 
> should be easy to do either as an additional patch in this series or a 
> separate one later.
>
> Regards,
> BALATON Zoltan
>
>>>> 
>>>> +static uint32_t ich6_pci_config_read(PCIDevice *d,
>>>> +                                       uint32_t address, int len)
>>>> +{
>>>> +    return pci_default_read_config(d, address, len);
>>>> +}
>>> 
>>> Why do you override this if you have nothing to do in it? Just use 
>>> pci_default_read_config and only override ich6_pci_config_write where you 
>>> actually has something to add to the default.
>>> 
>> You're right, I should not override this function. It was probably me 
>> trying (I uploaded this patch to GitHub about 2 weeks ago so I don't 
>> remember precisely) to override it for the sake of returning a value of 
>> 0x8000 when the guest OS tries to figure out if the IDE channel is disabled 
>> or not in the 0x40 and 0x42 registers in the PCI config space of the 
>> device.
>>> Maybe also wait for a few days for other's comments (especially the 
>>> maintainer's opinion on this) before sending a v2 so you get all comments 
>>> and see what to do.
>>> 
>> Thank you very much for putting time into reviewing this! :)
>> 
>> Best regards,
>> Liav
>> 
>

Re: [PATCH] hw/ide: implement ich6 ide controller support

[Liav Albani]

Hello BALATON,

Thank you for helping keeping this patch noticeable to everyone :)

I tried to reach out to John via a private email last Saturday (two days 
ago) so I don't "spam" the mailing list for no good reason.
It might be that I should actually refrain from doing so and talk to the 
maintainer directly on the mailing list once the patch
has been submitted to the mailing list.
I've not yet seen any response from John so I assume it's a matter of 
days before he can take care of this.

Best regards,
Liav

On 2/14/22 14:26, BALATON Zoltan wrote:
> On Sat, 5 Feb 2022, BALATON Zoltan wrote:
>> Hello,
>
> Ping? John, do you agree with my comments? Should Liav proceed to send 
> a v2?
>
> Thanks,
> BALATON Zoltan
>
>> On Sat, 5 Feb 2022, Liav Albani wrote:
>>> On 2/5/22 17:48, BALATON Zoltan wrote:
>>>> On Sat, 5 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.
>>>>
>>>> I haven't looked at in detail so only a few comments I've got while 
>>>> reading it. What machine needs this? In QEMU I think we only have 
>>>> piix and ich9 emulated for pc and q35 machines but maybe ich6 is 
>>>> also used by some machine I don't know about. Otherwise it looks 
>>>> odd to have ide part of ich6 but not the other parts of this chip.
>>>>
>>> Hi BALATON,
>>>
>>> This is my first patch to QEMU and the first time I send patches 
>>> over the mail. I sent my github tree to John Snow (the maintainer of 
>>> the IDE code in QEMU) for advice if I should send them here and I 
>>> was encouraged to do that.
>>
>> Welcome and thanks a lot for taking time to contribute and share your 
>> results. In case you're not yet aware, these docs should explain how 
>> patches are handled on the list:
>>
>> https://www.qemu.org/docs/master/devel/submitting-a-patch.html
>>
>>> For the next time patch I'll put a note on writing a descriptive 
>>> cover letter as it could have put more valuable details on why I 
>>> sent this patch.
>>>
>>> There's no such machine type emulating the ICH6 chipset in QEMU. 
>>> However, I wrote this emulation component as a test for the 
>>> SerenityOS kernel because I have a machine from 2009 which has
>>> an ICH7 southbridge, so, I wanted to emulate such device with QEMU 
>>> to ease development on it.
>>>
>>> I found out that Linux with libata was using the controller without 
>>> any noticeable problems, but the SerenityOS kernel struggled to use 
>>> this device, so I decided that
>>> I should send this patch to get it merged and then I can use it 
>>> locally and maybe other people will benefit from it.
>>>
>>> In regard to other components of the ICH6 chipset - I don't think 
>>> it's worth anybody's time to actually implement them as the ICH9 
>>> chipset is quite close to what the ICH6 chipset offers as far as I 
>>> can tell.
>>> The idea of implementing ich6-ide controller was to enable the 
>>> option of people like me and other OS developers to ensure their 
>>> kernels operate correctly on such type of device,
>>> which is legacy-free device in the aspect of PCI bus resource 
>>> management but still is a legacy device which belongs to chipsets of 
>>> late 2000s.
>>
>> That's OK, maybe a short mention (just one sentence) in the commit 
>> message explaining this would help to understand why this device 
>> model was added.
>>
>>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
>>>>> ---
>>>>> hw/i386/Kconfig          |   2 +
>>>>> hw/ide/Kconfig           |   5 +
>>>>> hw/ide/bmdma.c           |  83 +++++++++++++++
>>>>> hw/ide/ich6.c            | 211 
>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>> hw/ide/meson.build       |   3 +-
>>>>> hw/ide/piix.c            |  50 +---------
>>>>> include/hw/ide/pci.h     |   5 +
>>>>> include/hw/pci/pci_ids.h |   1 +
>>>>> 8 files changed, 311 insertions(+), 49 deletions(-)
>>>>> create mode 100644 hw/ide/bmdma.c
>>>>> 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/bmdma.c b/hw/ide/bmdma.c
>>>>> new file mode 100644
>>>>> index 0000000000..979f5974fd
>>>>> --- /dev/null
>>>>> +++ b/hw/ide/bmdma.c
>>>>> @@ -0,0 +1,83 @@
>>>>> +/*
>>>>> + * QEMU IDE Emulation: PCI PIIX3/4 support.
>>>>> + *
>>>>> + * 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/pci.h"
>>>>> +#include "trace.h"
>>>>> +
>>>>> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>>>>
>>>> Moving these functions to avoid duplication is a good idea but a 
>>>> couple of points:
>>>>
>>>> - Maybe this should be a separate patch just for moving these out 
>>>> then another patch adding ich6 for easier review of separate changes.
>>>>
>>> I don't mind splitting this patch into a couple of "commits" (or 
>>> series of diffs? I don't understand the terminology yet) so one 
>>> "commit" will be a preparation - extract the
>>> functions to a separate file, and the next diff can implement the 
>>> ich6-ide controller.
>>
>> See the docs, which should explain how to split up patches and what 
>> should be in one patch. This would be a patch series then. As to 
>> where to move these functions or how to proceed with it wait for John 
>> Snow's reply as he's the maintainer of this part so what he prefers 
>> is the way to go. What I wrote is my opinion but others may have 
>> different views so give them a chance to reply too then once we see 
>> what to do you can send a v2 with the changes that seems to be the 
>> consensus on how to best proceed.
>>
>>>> - There are already several bmdma_* functions in pci.c and you add 
>>>> these to pci.h so maybe these should be moved near there in pci.c 
>>>> instead. (Or more of those bmdma_* functions moved in this new file 
>>>> and add its own header?)
>>>>
>>> I'm not sure what is the best approach but maybe the latter 
>>> suggestion (new file and its own header) seems quite good to me.
>>>> - This is not piix specific so the name probably should not say 
>>>> that. Maybe somthing along the lines of pci_default_read_config so 
>>>> bmdma_default_read or similar. In fact it also appears in via.c and 
>>>> a more complex version in cmd646.c which could still reuse 
>>>> functions like these like we do with pci config write. Converting 
>>>> those other two to use the newly split off functions instead of 
>>>> duplicating it could be done in a follow up patch, if you don't 
>>>> want to do that I may look at via-ide but a patch is welcome if you 
>>>> have time for that, unless others think otherwise and we'll take a 
>>>> different route.
>>>>
>>> I want to create the best possible patch so if it's desirable to 
>>> rename it to something more generic, that's OK for me.
>>> I'd like to address some of the issues you mentioned (such as 
>>> avoiding duplicates in how we use bus master DMA in the ide code) in 
>>> future patches though, so if possible, let's keep it simple now :)
>>
>> Sure, this could be done afterwards too just noted it when noticed 
>> there are other places that use similar functions so this is a 
>> possible clean up that should be easy to do either as an additional 
>> patch in this series or a separate one later.
>>
>> Regards,
>> BALATON Zoltan
>>
>>>>>
>>>>> +static uint32_t ich6_pci_config_read(PCIDevice *d,
>>>>> +                                       uint32_t address, int len)
>>>>> +{
>>>>> +    return pci_default_read_config(d, address, len);
>>>>> +}
>>>>
>>>> Why do you override this if you have nothing to do in it? Just use 
>>>> pci_default_read_config and only override ich6_pci_config_write 
>>>> where you actually has something to add to the default.
>>>>
>>> You're right, I should not override this function. It was probably 
>>> me trying (I uploaded this patch to GitHub about 2 weeks ago so I 
>>> don't remember precisely) to override it for the sake of returning a 
>>> value of 0x8000 when the guest OS tries to figure out if the IDE 
>>> channel is disabled or not in the 0x40 and 0x42 registers in the PCI 
>>> config space of the device.
>>>> Maybe also wait for a few days for other's comments (especially the 
>>>> maintainer's opinion on this) before sending a v2 so you get all 
>>>> comments and see what to do.
>>>>
>>> Thank you very much for putting time into reviewing this! :)
>>>
>>> Best regards,
>>> Liav
>>>
>>

Re: [PATCH] hw/ide: implement ich6 ide controller support

[John Snow]

On Mon, Feb 14, 2022 at 1:48 PM Liav Albani <liavalb@gmail.com> wrote:
>
> Hello BALATON,
>
> Thank you for helping keeping this patch noticeable to everyone :)
>
> I tried to reach out to John via a private email last Saturday (two days
> ago) so I don't "spam" the mailing list for no good reason.
> It might be that I should actually refrain from doing so and talk to the
> maintainer directly on the mailing list once the patch
> has been submitted to the mailing list.
> I've not yet seen any response from John so I assume it's a matter of
> days before he can take care of this.
>
> Best regards,
> Liav
>
> On 2/14/22 14:26, BALATON Zoltan wrote:
> > On Sat, 5 Feb 2022, BALATON Zoltan wrote:
> >> Hello,
> >
> > Ping? John, do you agree with my comments? Should Liav proceed to send
> > a v2?
> >
> > Thanks,
> > BALATON Zoltan
> >
> >> On Sat, 5 Feb 2022, Liav Albani wrote:
> >>> On 2/5/22 17:48, BALATON Zoltan wrote:
> >>>> On Sat, 5 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.
> >>>>
> >>>> I haven't looked at in detail so only a few comments I've got while
> >>>> reading it. What machine needs this? In QEMU I think we only have
> >>>> piix and ich9 emulated for pc and q35 machines but maybe ich6 is
> >>>> also used by some machine I don't know about. Otherwise it looks
> >>>> odd to have ide part of ich6 but not the other parts of this chip.
> >>>>
> >>> Hi BALATON,
> >>>
> >>> This is my first patch to QEMU and the first time I send patches
> >>> over the mail. I sent my github tree to John Snow (the maintainer of
> >>> the IDE code in QEMU) for advice if I should send them here and I
> >>> was encouraged to do that.
> >>
> >> Welcome and thanks a lot for taking time to contribute and share your
> >> results. In case you're not yet aware, these docs should explain how
> >> patches are handled on the list:
> >>
> >> https://www.qemu.org/docs/master/devel/submitting-a-patch.html
> >>
> >>> For the next time patch I'll put a note on writing a descriptive
> >>> cover letter as it could have put more valuable details on why I
> >>> sent this patch.
> >>>
> >>> There's no such machine type emulating the ICH6 chipset in QEMU.
> >>> However, I wrote this emulation component as a test for the
> >>> SerenityOS kernel because I have a machine from 2009 which has
> >>> an ICH7 southbridge, so, I wanted to emulate such device with QEMU
> >>> to ease development on it.
> >>>
> >>> I found out that Linux with libata was using the controller without
> >>> any noticeable problems, but the SerenityOS kernel struggled to use
> >>> this device, so I decided that
> >>> I should send this patch to get it merged and then I can use it
> >>> locally and maybe other people will benefit from it.
> >>>
> >>> In regard to other components of the ICH6 chipset - I don't think
> >>> it's worth anybody's time to actually implement them as the ICH9
> >>> chipset is quite close to what the ICH6 chipset offers as far as I
> >>> can tell.
> >>> The idea of implementing ich6-ide controller was to enable the
> >>> option of people like me and other OS developers to ensure their
> >>> kernels operate correctly on such type of device,
> >>> which is legacy-free device in the aspect of PCI bus resource
> >>> management but still is a legacy device which belongs to chipsets of
> >>> late 2000s.
> >>
> >> That's OK, maybe a short mention (just one sentence) in the commit
> >> message explaining this would help to understand why this device
> >> model was added.
> >>
> >>>>> Signed-off-by: Liav Albani <liavalb@gmail.com>
> >>>>> ---
> >>>>> hw/i386/Kconfig          |   2 +
> >>>>> hw/ide/Kconfig           |   5 +
> >>>>> hw/ide/bmdma.c           |  83 +++++++++++++++
> >>>>> hw/ide/ich6.c            | 211
> >>>>> +++++++++++++++++++++++++++++++++++++++
> >>>>> hw/ide/meson.build       |   3 +-
> >>>>> hw/ide/piix.c            |  50 +---------
> >>>>> include/hw/ide/pci.h     |   5 +
> >>>>> include/hw/pci/pci_ids.h |   1 +
> >>>>> 8 files changed, 311 insertions(+), 49 deletions(-)
> >>>>> create mode 100644 hw/ide/bmdma.c
> >>>>> 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/bmdma.c b/hw/ide/bmdma.c
> >>>>> new file mode 100644
> >>>>> index 0000000000..979f5974fd
> >>>>> --- /dev/null
> >>>>> +++ b/hw/ide/bmdma.c
> >>>>> @@ -0,0 +1,83 @@
> >>>>> +/*
> >>>>> + * QEMU IDE Emulation: PCI PIIX3/4 support.
> >>>>> + *
> >>>>> + * 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/pci.h"
> >>>>> +#include "trace.h"
> >>>>> +
> >>>>> +uint64_t piix_bmdma_read(void *opaque, hwaddr addr, unsigned size)
> >>>>
> >>>> Moving these functions to avoid duplication is a good idea but a
> >>>> couple of points:
> >>>>
> >>>> - Maybe this should be a separate patch just for moving these out
> >>>> then another patch adding ich6 for easier review of separate changes.
> >>>>
> >>> I don't mind splitting this patch into a couple of "commits" (or
> >>> series of diffs? I don't understand the terminology yet) so one
> >>> "commit" will be a preparation - extract the
> >>> functions to a separate file, and the next diff can implement the
> >>> ich6-ide controller.
> >>
> >> See the docs, which should explain how to split up patches and what
> >> should be in one patch. This would be a patch series then. As to
> >> where to move these functions or how to proceed with it wait for John
> >> Snow's reply as he's the maintainer of this part so what he prefers
> >> is the way to go. What I wrote is my opinion but others may have
> >> different views so give them a chance to reply too then once we see
> >> what to do you can send a v2 with the changes that seems to be the
> >> consensus on how to best proceed.
> >>
> >>>> - There are already several bmdma_* functions in pci.c and you add
> >>>> these to pci.h so maybe these should be moved near there in pci.c
> >>>> instead. (Or more of those bmdma_* functions moved in this new file
> >>>> and add its own header?)
> >>>>
> >>> I'm not sure what is the best approach but maybe the latter
> >>> suggestion (new file and its own header) seems quite good to me.
> >>>> - This is not piix specific so the name probably should not say
> >>>> that. Maybe somthing along the lines of pci_default_read_config so
> >>>> bmdma_default_read or similar. In fact it also appears in via.c and
> >>>> a more complex version in cmd646.c which could still reuse
> >>>> functions like these like we do with pci config write. Converting
> >>>> those other two to use the newly split off functions instead of
> >>>> duplicating it could be done in a follow up patch, if you don't
> >>>> want to do that I may look at via-ide but a patch is welcome if you
> >>>> have time for that, unless others think otherwise and we'll take a
> >>>> different route.
> >>>>
> >>> I want to create the best possible patch so if it's desirable to
> >>> rename it to something more generic, that's OK for me.
> >>> I'd like to address some of the issues you mentioned (such as
> >>> avoiding duplicates in how we use bus master DMA in the ide code) in
> >>> future patches though, so if possible, let's keep it simple now :)
> >>
> >> Sure, this could be done afterwards too just noted it when noticed
> >> there are other places that use similar functions so this is a
> >> possible clean up that should be easy to do either as an additional
> >> patch in this series or a separate one later.
> >>
> >> Regards,
> >> BALATON Zoltan
> >>
> >>>>>
> >>>>> +static uint32_t ich6_pci_config_read(PCIDevice *d,
> >>>>> +                                       uint32_t address, int len)
> >>>>> +{
> >>>>> +    return pci_default_read_config(d, address, len);
> >>>>> +}
> >>>>
> >>>> Why do you override this if you have nothing to do in it? Just use
> >>>> pci_default_read_config and only override ich6_pci_config_write
> >>>> where you actually has something to add to the default.
> >>>>
> >>> You're right, I should not override this function. It was probably
> >>> me trying (I uploaded this patch to GitHub about 2 weeks ago so I
> >>> don't remember precisely) to override it for the sake of returning a
> >>> value of 0x8000 when the guest OS tries to figure out if the IDE
> >>> channel is disabled or not in the 0x40 and 0x42 registers in the PCI
> >>> config space of the device.
> >>>> Maybe also wait for a few days for other's comments (especially the
> >>>> maintainer's opinion on this) before sending a v2 so you get all
> >>>> comments and see what to do.
> >>>>
> >>> Thank you very much for putting time into reviewing this! :)
> >>>
> >>> Best regards,
> >>> Liav
> >>>
> >>
>

Hi! Sorry, I am very behind on IDE work and I haven't had the time to
review this in depth. When Liav reached out to ask if the patches
should be sent to the list, I said YES because I think all patches
should be shared with the list, whether or not they make it in or not.

At the moment, I'm going to trust BALATON's analysis until I am able
to give this a much more thorough look, but that might be a little
while yet. I'm happy to take just about anything that doesn't break
existing cases, even if it's fairly experimental support for platforms
we don't normally care about.

Sorry for the non-answer, but I'll try to loop back around soon.

--js