[PATCH 0/5] cmd646: move device-specific BMDMA registers to separate memory region

[PATCH 0/5] cmd646: move device-specific BMDMA registers to separate memory region

[Mark Cave-Ayland]

This series follows on from a comment I made on Bernhard's PCI IDE tidy-up series [1]
that it should be possible to further consolidate the BMDMA registers into the PCI
IDE device with some minor rework to the CMD646 device.

It does this by moving the CMD646 device-specific BMDMA registers to a separate
memory region, and then aliasing the device-specific BMDMA registers from 
the existing BMDMAState memory region. With this in place it should be fairly
trivial to extend the consolidation implementation in [1].

Before "info mtree" output:
    0000000000008200-000000000000820f (prio 1, i/o): cmd646-bmdma
      0000000000008200-0000000000008203 (prio 0, i/o): cmd646-bmdma-bus
      0000000000008204-0000000000008207 (prio 0, i/o): cmd646-bmdma-ioport
      0000000000008208-000000000000820b (prio 0, i/o): cmd646-bmdma-bus
      000000000000820c-000000000000820f (prio 0, i/o): cmd646-bmdma-ioport

After "info mtree" output:
    0000000000008200-000000000000820f (prio 1, i/o): cmd646-bmdma
      0000000000008200-0000000000008203 (prio 0, i/o): cmd646-bmdma-bus
        0000000000008201-0000000000008201 (prio 0, i/o): alias cmd646-bmdma[1] @cmd646-bmdma 0000000000000001-0000000000000001
        0000000000008203-0000000000008203 (prio 0, i/o): alias cmd646-bmdma[3] @cmd646-bmdma 0000000000000003-0000000000000003
      0000000000008204-0000000000008207 (prio 0, i/o): cmd646-bmdma-ioport
      0000000000008208-000000000000820b (prio 0, i/o): cmd646-bmdma-bus
        0000000000008209-0000000000008209 (prio 0, i/o): alias cmd646-bmdma[1] @cmd646-bmdma 0000000000000001-0000000000000001
        000000000000820b-000000000000820b (prio 0, i/o): alias cmd646-bmdma[3] @cmd646-bmdma 0000000000000003-0000000000000003
      000000000000820c-000000000000820f (prio 0, i/o): cmd646-bmdma-ioport

The series was tested by confirming that breakpoints on the CMD646-specific
BMDMA registers were being hit and that my test Debian install ISO still
boots under qemu-system-sparc64.

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

[1] https://patchew.org/QEMU/20230422150728.176512-1-shentey@gmail.com/


Mark Cave-Ayland (5):
  cmd646: checkpatch fixes
  cmd646: create separate header and QOM type for CMD646_IDE
  cmd646: use TYPE_CMD646_IDE instead of hardcoded "cmd646-ide" string
  cmd646: rename cmd646_bmdma_ops to bmdma_ops
  cmd646: move device-specific BMDMA registers to separate memory region

 hw/alpha/dp264.c        |   4 +-
 hw/ide/cmd646.c         | 122 ++++++++++++++++++++++++++++++----------
 hw/sparc64/sun4u.c      |   4 +-
 include/hw/ide/cmd646.h |  42 ++++++++++++++
 4 files changed, 139 insertions(+), 33 deletions(-)
 create mode 100644 include/hw/ide/cmd646.h

-- 
2.30.2

[PATCH 1/5] cmd646: checkpatch fixes

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a68357c1c5..20f1e41d57 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -96,7 +96,7 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
         return ((uint64_t)1 << (size * 8)) - 1;
     }
 
-    switch(addr & 3) {
+    switch (addr & 3) {
     case 0:
         val = bm->cmd;
         break;
@@ -133,7 +133,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
     }
 
     trace_bmdma_write_cmd646(addr, val);
-    switch(addr & 3) {
+    switch (addr & 3) {
     case 0:
         bmdma_cmd_writeb(bm, val);
         break;
@@ -144,7 +144,8 @@ static void bmdma_write(void *opaque, hwaddr addr,
         cmd646_update_irq(pci_dev);
         break;
     case 2:
-        bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
+        bm->status = (val & 0x60) | (bm->status & 1) |
+                     (bm->status & ~val & 0x06);
         break;
     case 3:
         if (bm == &bm->pci_dev->bmdma[0]) {
@@ -167,7 +168,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
     int i;
 
     memory_region_init(&d->bmdma_bar, OBJECT(d), "cmd646-bmdma", 16);
-    for(i = 0;i < 2; i++) {
+    for (i = 0; i < 2; i++) {
         bm = &d->bmdma[i];
         memory_region_init_io(&bm->extra_io, OBJECT(d), &cmd646_bmdma_ops, bm,
                               "cmd646-bmdma-bus", 4);
@@ -255,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
 
     pci_conf[PCI_CLASS_PROG] = 0x8f;
 
-    pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
+    pci_conf[CNTRL] = CNTRL_EN_CH0; /* enable IDE0 */
     if (d->secondary) {
         /* XXX: if not enabled, really disable the seconday IDE controller */
         pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
@@ -289,7 +290,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
     /* TODO: RST# value should be 0 */
-    pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
+    pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt on pin 1 */
 
     qdev_init_gpio_in(ds, cmd646_set_irq, 2);
     for (i = 0; i < 2; i++) {
-- 
2.30.2

[PATCH 2/5] cmd646: create separate header and QOM type for CMD646_IDE

[Mark Cave-Ayland]

This will enable CMD646-specific fields to be added to CMD6464IDEState in
future.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c         |  4 +++-
 include/hw/ide/cmd646.h | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/ide/cmd646.h

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 20f1e41d57..a3e227fba7 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -33,6 +33,7 @@
 #include "sysemu/reset.h"
 
 #include "hw/ide/pci.h"
+#include "hw/ide/cmd646.h"
 #include "trace.h"
 
 /* CMD646 specific */
@@ -339,9 +340,10 @@ static void cmd646_ide_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo cmd646_ide_info = {
-    .name          = "cmd646-ide",
+    .name          = TYPE_CMD646_IDE,
     .parent        = TYPE_PCI_IDE,
     .class_init    = cmd646_ide_class_init,
+    .instance_size = sizeof(CMD646IDEState),
 };
 
 static void cmd646_ide_register_types(void)
diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
new file mode 100644
index 0000000000..4780b1132c
--- /dev/null
+++ b/include/hw/ide/cmd646.h
@@ -0,0 +1,38 @@
+/*
+ * QEMU IDE Emulation: PCI cmd646 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.
+ */
+
+#ifndef HW_IDE_CMD646_H
+#define HW_IDE_CMD646_H
+
+#define TYPE_CMD646_IDE "cmd646-ide"
+OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
+
+#include "hw/ide/pci.h"
+
+struct CMD646IDEState {
+    PCIIDEState parent_obj;
+};
+
+#endif
-- 
2.30.2

[PATCH 3/5] cmd646: use TYPE_CMD646_IDE instead of hardcoded "cmd646-ide" string

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/alpha/dp264.c   | 4 ++--
 hw/sparc64/sun4u.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 03495e1e60..f2affecad9 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -13,7 +13,7 @@
 #include "alpha_sys.h"
 #include "qemu/error-report.h"
 #include "hw/rtc/mc146818rtc.h"
-#include "hw/ide/pci.h"
+#include "hw/ide/cmd646.h"
 #include "hw/isa/superio.h"
 #include "net/net.h"
 #include "qemu/cutils.h"
@@ -132,7 +132,7 @@ static void clipper_init(MachineState *machine)
     isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
 
     /* IDE disk setup.  */
-    pci_dev = pci_create_simple(pci_bus, -1, "cmd646-ide");
+    pci_dev = pci_create_simple(pci_bus, -1, TYPE_CMD646_IDE);
     pci_ide_create_devs(pci_dev);
 
     /* Load PALcode.  Given that this is not "real" cpu palcode,
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index e2858a0331..66b55eabd1 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -50,7 +50,7 @@
 #include "hw/sparc/sparc64.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"
-#include "hw/ide/pci.h"
+#include "hw/ide/cmd646.h"
 #include "hw/loader.h"
 #include "hw/fw-path-provider.h"
 #include "elf.h"
@@ -673,7 +673,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
         qemu_macaddr_default_if_unset(&macaddr);
     }
 
-    pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
+    pci_dev = pci_new(PCI_DEVFN(3, 0), TYPE_CMD646_IDE);
     qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
     pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
     pci_ide_create_devs(pci_dev);
-- 
2.30.2

[PATCH 4/5] cmd646: rename cmd646_bmdma_ops to bmdma_ops

[Mark Cave-Ayland]

This is to allow us to use the cmd646_bmdma_ops name for the CMD646
device-specific registers in the next commit.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a3e227fba7..9103da581f 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -158,7 +158,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
     }
 }
 
-static const MemoryRegionOps cmd646_bmdma_ops = {
+static const MemoryRegionOps bmdma_ops = {
     .read = bmdma_read,
     .write = bmdma_write,
 };
@@ -171,7 +171,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
     memory_region_init(&d->bmdma_bar, OBJECT(d), "cmd646-bmdma", 16);
     for (i = 0; i < 2; i++) {
         bm = &d->bmdma[i];
-        memory_region_init_io(&bm->extra_io, OBJECT(d), &cmd646_bmdma_ops, bm,
+        memory_region_init_io(&bm->extra_io, OBJECT(d), &bmdma_ops, bm,
                               "cmd646-bmdma-bus", 4);
         memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
         memory_region_init_io(&bm->addr_ioport, OBJECT(d),
-- 
2.30.2

[PATCH 5/5] cmd646: move device-specific BMDMA registers to separate memory region

[Mark Cave-Ayland]

The aim here is to eliminate any device-specific registers from the main BMDMA
bar memory region so it can potentially be moved into the shared PCI IDE device.

For each BMDMA bus create a new cmd646-bmdma-specific memory region representing
the device-specific BMDMA registers and then map them using aliases onto the
existing BMDMAState memory region.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/cmd646.c         | 111 +++++++++++++++++++++++++++++++---------
 include/hw/ide/cmd646.h |   4 ++
 2 files changed, 90 insertions(+), 25 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 9103da581f..dd495f2e1b 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -90,7 +90,6 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
 {
     BMDMAState *bm = opaque;
-    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
     uint32_t val;
 
     if (size != 1) {
@@ -101,19 +100,9 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
     case 0:
         val = bm->cmd;
         break;
-    case 1:
-        val = pci_dev->config[MRDMODE];
-        break;
     case 2:
         val = bm->status;
         break;
-    case 3:
-        if (bm == &bm->pci_dev->bmdma[0]) {
-            val = pci_dev->config[UDIDETCR0];
-        } else {
-            val = pci_dev->config[UDIDETCR1];
-        }
-        break;
     default:
         val = 0xff;
         break;
@@ -127,7 +116,6 @@ static void bmdma_write(void *opaque, hwaddr addr,
                         uint64_t val, unsigned size)
 {
     BMDMAState *bm = opaque;
-    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
 
     if (size != 1) {
         return;
@@ -138,23 +126,10 @@ static void bmdma_write(void *opaque, hwaddr addr,
     case 0:
         bmdma_cmd_writeb(bm, val);
         break;
-    case 1:
-        pci_dev->config[MRDMODE] =
-            (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
-        cmd646_update_dma_interrupts(pci_dev);
-        cmd646_update_irq(pci_dev);
-        break;
     case 2:
         bm->status = (val & 0x60) | (bm->status & 1) |
                      (bm->status & ~val & 0x06);
         break;
-    case 3:
-        if (bm == &bm->pci_dev->bmdma[0]) {
-            pci_dev->config[UDIDETCR0] = val;
-        } else {
-            pci_dev->config[UDIDETCR1] = val;
-        }
-        break;
     }
 }
 
@@ -181,6 +156,91 @@ static void bmdma_setup_bar(PCIIDEState *d)
     }
 }
 
+static uint64_t cmd646_bmdma_read(void *opaque, hwaddr addr, unsigned size)
+{
+    BMDMAState *bm = opaque;
+    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
+    uint32_t val;
+
+    if (size != 1) {
+        return ((uint64_t)1 << (size * 8)) - 1;
+    }
+
+    switch (addr & 3) {
+    case 1:
+        val = pci_dev->config[MRDMODE];
+        break;
+    case 3:
+        if (bm == &bm->pci_dev->bmdma[0]) {
+            val = pci_dev->config[UDIDETCR0];
+        } else {
+            val = pci_dev->config[UDIDETCR1];
+        }
+        break;
+    default:
+        val = 0xff;
+        break;
+    }
+
+    trace_bmdma_read_cmd646(addr, val);
+    return val;
+}
+
+static void cmd646_bmdma_write(void *opaque, hwaddr addr, uint64_t val,
+                               unsigned size)
+{
+    BMDMAState *bm = opaque;
+    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
+
+    if (size != 1) {
+        return;
+    }
+
+    trace_bmdma_write_cmd646(addr, val);
+    switch (addr & 3) {
+    case 1:
+        pci_dev->config[MRDMODE] =
+            (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
+        cmd646_update_dma_interrupts(pci_dev);
+        cmd646_update_irq(pci_dev);
+        break;
+    case 3:
+        if (bm == &bm->pci_dev->bmdma[0]) {
+            pci_dev->config[UDIDETCR0] = val;
+        } else {
+            pci_dev->config[UDIDETCR1] = val;
+        }
+        break;
+    }
+}
+
+static const MemoryRegionOps cmd646_bmdma_ops = {
+    .read = cmd646_bmdma_read,
+    .write = cmd646_bmdma_write,
+};
+
+static void cmd646_bmdma_setup(PCIIDEState *d)
+{
+    CMD646IDEState *s = CMD646_IDE(d);
+    BMDMAState *bm;
+    int i;
+
+    /* Setup aliases for device-specific BMDMA registers */
+    for (i = 0; i < 2; i++) {
+        bm = &d->bmdma[i];
+        memory_region_init_io(&s->bmdma_mem[i], OBJECT(d), &cmd646_bmdma_ops,
+                              bm, "cmd646-bmdma", 4);
+        memory_region_init_alias(&s->bmdma_mem_alias[i][0], OBJECT(d),
+                                 "cmd646-bmdma[1]", &s->bmdma_mem[i], 0x1, 1);
+        memory_region_add_subregion(&bm->extra_io, 0x1,
+                                    &s->bmdma_mem_alias[i][0]);
+        memory_region_init_alias(&s->bmdma_mem_alias[i][1], OBJECT(d),
+                                 "cmd646-bmdma[3]", &s->bmdma_mem[i], 0x3, 1);
+        memory_region_add_subregion(&bm->extra_io, 0x3,
+                                    &s->bmdma_mem_alias[i][1]);
+    }
+}
+
 static void cmd646_update_irq(PCIDevice *pd)
 {
     int pci_level;
@@ -289,6 +349,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
 
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+    cmd646_bmdma_setup(d);
 
     /* TODO: RST# value should be 0 */
     pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt on pin 1 */
diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
index 4780b1132c..5329964533 100644
--- a/include/hw/ide/cmd646.h
+++ b/include/hw/ide/cmd646.h
@@ -29,10 +29,14 @@
 #define TYPE_CMD646_IDE "cmd646-ide"
 OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
 
+#include "exec/memory.h"
 #include "hw/ide/pci.h"
 
 struct CMD646IDEState {
     PCIIDEState parent_obj;
+
+    MemoryRegion bmdma_mem[2];
+    MemoryRegion bmdma_mem_alias[2][2];
 };
 
 #endif
-- 
2.30.2

Re: [PATCH 2/5] cmd646: create separate header and QOM type for CMD646_IDE

[Bernhard Beschow]

Am 9. Juni 2023 18:51:16 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>This will enable CMD646-specific fields to be added to CMD6464IDEState in
>future.
>
>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>---
> hw/ide/cmd646.c         |  4 +++-
> include/hw/ide/cmd646.h | 38 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 41 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/ide/cmd646.h
>
>diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>index 20f1e41d57..a3e227fba7 100644
>--- a/hw/ide/cmd646.c
>+++ b/hw/ide/cmd646.c
>@@ -33,6 +33,7 @@
> #include "sysemu/reset.h"
> 
> #include "hw/ide/pci.h"
>+#include "hw/ide/cmd646.h"
> #include "trace.h"
> 
> /* CMD646 specific */
>@@ -339,9 +340,10 @@ static void cmd646_ide_class_init(ObjectClass *klass, void *data)
> }
> 
> static const TypeInfo cmd646_ide_info = {
>-    .name          = "cmd646-ide",
>+    .name          = TYPE_CMD646_IDE,
>     .parent        = TYPE_PCI_IDE,
>     .class_init    = cmd646_ide_class_init,
>+    .instance_size = sizeof(CMD646IDEState),
> };
> 
> static void cmd646_ide_register_types(void)
>diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
>new file mode 100644
>index 0000000000..4780b1132c
>--- /dev/null
>+++ b/include/hw/ide/cmd646.h
>@@ -0,0 +1,38 @@
>+/*
>+ * QEMU IDE Emulation: PCI cmd646 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.
>+ */
>+
>+#ifndef HW_IDE_CMD646_H
>+#define HW_IDE_CMD646_H
>+
>+#define TYPE_CMD646_IDE "cmd646-ide"
>+OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
>+
>+#include "hw/ide/pci.h"
>+
>+struct CMD646IDEState {
>+    PCIIDEState parent_obj;

No public / private sections here? From the naming this attribute is often considered private and the rest public. I guess what this scheme communicates is that `parent_obj` should be accessed via `IDE_PCI()` only.

>+};
>+
>+#endif

Re: [PATCH 4/5] cmd646: rename cmd646_bmdma_ops to bmdma_ops

[Philippe Mathieu-Daudé]

On 9/6/23 20:51, Mark Cave-Ayland wrote:
> This is to allow us to use the cmd646_bmdma_ops name for the CMD646
> device-specific registers in the next commit.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/ide/cmd646.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/5] cmd646: checkpatch fixes

[Philippe Mathieu-Daudé]

On 9/6/23 20:51, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/ide/cmd646.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/5] cmd646: create separate header and QOM type for CMD646_IDE

[Mark Cave-Ayland]

On 12/06/2023 10:21, Bernhard Beschow wrote:

> Am 9. Juni 2023 18:51:16 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> This will enable CMD646-specific fields to be added to CMD6464IDEState in
>> future.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/ide/cmd646.c         |  4 +++-
>> include/hw/ide/cmd646.h | 38 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 41 insertions(+), 1 deletion(-)
>> create mode 100644 include/hw/ide/cmd646.h
>>
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 20f1e41d57..a3e227fba7 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -33,6 +33,7 @@
>> #include "sysemu/reset.h"
>>
>> #include "hw/ide/pci.h"
>> +#include "hw/ide/cmd646.h"
>> #include "trace.h"
>>
>> /* CMD646 specific */
>> @@ -339,9 +340,10 @@ static void cmd646_ide_class_init(ObjectClass *klass, void *data)
>> }
>>
>> static const TypeInfo cmd646_ide_info = {
>> -    .name          = "cmd646-ide",
>> +    .name          = TYPE_CMD646_IDE,
>>      .parent        = TYPE_PCI_IDE,
>>      .class_init    = cmd646_ide_class_init,
>> +    .instance_size = sizeof(CMD646IDEState),
>> };
>>
>> static void cmd646_ide_register_types(void)
>> diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
>> new file mode 100644
>> index 0000000000..4780b1132c
>> --- /dev/null
>> +++ b/include/hw/ide/cmd646.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * QEMU IDE Emulation: PCI cmd646 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.
>> + */
>> +
>> +#ifndef HW_IDE_CMD646_H
>> +#define HW_IDE_CMD646_H
>> +
>> +#define TYPE_CMD646_IDE "cmd646-ide"
>> +OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
>> +
>> +#include "hw/ide/pci.h"
>> +
>> +struct CMD646IDEState {
>> +    PCIIDEState parent_obj;
> 
> No public / private sections here? From the naming this attribute is often considered private and the rest public. I guess what this scheme communicates is that `parent_obj` should be accessed via `IDE_PCI()` only.

Whilst the public/private comments were included in the original QOM documentation, 
it's not something that tends to be used now. These days people are looking to use 
interfaces such as SysBus to indicated which MemoryRegions and IRQs are "public", but 
certainly the detail is still up for active discussion.

The QOM cast macros will allow the object referenced to be converted to any of the 
parent types directly, so indeed there should be no direct references to parent_obj.


ATB,

Mark.

Re: [PATCH 5/5] cmd646: move device-specific BMDMA registers to separate memory region

[Bernhard Beschow]

Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>The aim here is to eliminate any device-specific registers from the main BMDMA
>bar memory region so it can potentially be moved into the shared PCI IDE device.
>
>For each BMDMA bus create a new cmd646-bmdma-specific memory region representing
>the device-specific BMDMA registers and then map them using aliases onto the
>existing BMDMAState memory region.
>
>Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>---
> hw/ide/cmd646.c         | 111 +++++++++++++++++++++++++++++++---------
> include/hw/ide/cmd646.h |   4 ++
> 2 files changed, 90 insertions(+), 25 deletions(-)
>
>diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>index 9103da581f..dd495f2e1b 100644
>--- a/hw/ide/cmd646.c
>+++ b/hw/ide/cmd646.c
>@@ -90,7 +90,6 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                            unsigned size)
> {
>     BMDMAState *bm = opaque;
>-    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>     uint32_t val;
> 
>     if (size != 1) {
>@@ -101,19 +100,9 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>     case 0:
>         val = bm->cmd;
>         break;
>-    case 1:
>-        val = pci_dev->config[MRDMODE];
>-        break;
>     case 2:
>         val = bm->status;
>         break;
>-    case 3:
>-        if (bm == &bm->pci_dev->bmdma[0]) {
>-            val = pci_dev->config[UDIDETCR0];
>-        } else {
>-            val = pci_dev->config[UDIDETCR1];
>-        }
>-        break;
>     default:
>         val = 0xff;
>         break;
>@@ -127,7 +116,6 @@ static void bmdma_write(void *opaque, hwaddr addr,
>                         uint64_t val, unsigned size)
> {
>     BMDMAState *bm = opaque;
>-    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
> 
>     if (size != 1) {
>         return;
>@@ -138,23 +126,10 @@ static void bmdma_write(void *opaque, hwaddr addr,
>     case 0:
>         bmdma_cmd_writeb(bm, val);
>         break;
>-    case 1:
>-        pci_dev->config[MRDMODE] =
>-            (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
>-        cmd646_update_dma_interrupts(pci_dev);
>-        cmd646_update_irq(pci_dev);
>-        break;
>     case 2:
>         bm->status = (val & 0x60) | (bm->status & 1) |
>                      (bm->status & ~val & 0x06);
>         break;
>-    case 3:
>-        if (bm == &bm->pci_dev->bmdma[0]) {
>-            pci_dev->config[UDIDETCR0] = val;
>-        } else {
>-            pci_dev->config[UDIDETCR1] = val;
>-        }
>-        break;
>     }
> }
> 
>@@ -181,6 +156,91 @@ static void bmdma_setup_bar(PCIIDEState *d)
>     }
> }
> 
>+static uint64_t cmd646_bmdma_read(void *opaque, hwaddr addr, unsigned size)
>+{
>+    BMDMAState *bm = opaque;
>+    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>+    uint32_t val;
>+
>+    if (size != 1) {
>+        return ((uint64_t)1 << (size * 8)) - 1;
>+    }
>+
>+    switch (addr & 3) {
>+    case 1:
>+        val = pci_dev->config[MRDMODE];
>+        break;
>+    case 3:
>+        if (bm == &bm->pci_dev->bmdma[0]) {
>+            val = pci_dev->config[UDIDETCR0];
>+        } else {
>+            val = pci_dev->config[UDIDETCR1];
>+        }
>+        break;
>+    default:
>+        val = 0xff;
>+        break;
>+    }
>+
>+    trace_bmdma_read_cmd646(addr, val);
>+    return val;
>+}
>+
>+static void cmd646_bmdma_write(void *opaque, hwaddr addr, uint64_t val,
>+                               unsigned size)
>+{
>+    BMDMAState *bm = opaque;
>+    PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev);
>+
>+    if (size != 1) {
>+        return;
>+    }
>+
>+    trace_bmdma_write_cmd646(addr, val);
>+    switch (addr & 3) {
>+    case 1:
>+        pci_dev->config[MRDMODE] =
>+            (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30);
>+        cmd646_update_dma_interrupts(pci_dev);
>+        cmd646_update_irq(pci_dev);
>+        break;
>+    case 3:
>+        if (bm == &bm->pci_dev->bmdma[0]) {
>+            pci_dev->config[UDIDETCR0] = val;
>+        } else {
>+            pci_dev->config[UDIDETCR1] = val;
>+        }
>+        break;
>+    }
>+}
>+
>+static const MemoryRegionOps cmd646_bmdma_ops = {
>+    .read = cmd646_bmdma_read,
>+    .write = cmd646_bmdma_write,
>+};
>+
>+static void cmd646_bmdma_setup(PCIIDEState *d)
>+{
>+    CMD646IDEState *s = CMD646_IDE(d);
>+    BMDMAState *bm;
>+    int i;
>+
>+    /* Setup aliases for device-specific BMDMA registers */
>+    for (i = 0; i < 2; i++) {

I'd use `ARRAY_SIZE()` instead of the hardcoded constant here.

>+        bm = &d->bmdma[i];
>+        memory_region_init_io(&s->bmdma_mem[i], OBJECT(d), &cmd646_bmdma_ops,
>+                              bm, "cmd646-bmdma", 4);
>+        memory_region_init_alias(&s->bmdma_mem_alias[i][0], OBJECT(d),
>+                                 "cmd646-bmdma[1]", &s->bmdma_mem[i], 0x1, 1);
>+        memory_region_add_subregion(&bm->extra_io, 0x1,
>+                                    &s->bmdma_mem_alias[i][0]);
>+        memory_region_init_alias(&s->bmdma_mem_alias[i][1], OBJECT(d),
>+                                 "cmd646-bmdma[3]", &s->bmdma_mem[i], 0x3, 1);
>+        memory_region_add_subregion(&bm->extra_io, 0x3,
>+                                    &s->bmdma_mem_alias[i][1]);
>+    }
>+}
>+
> static void cmd646_update_irq(PCIDevice *pd)
> {
>     int pci_level;
>@@ -289,6 +349,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
> 
>     bmdma_setup_bar(d);
>     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>+    cmd646_bmdma_setup(d);
> 
>     /* TODO: RST# value should be 0 */
>     pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt on pin 1 */
>diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h
>index 4780b1132c..5329964533 100644
>--- a/include/hw/ide/cmd646.h
>+++ b/include/hw/ide/cmd646.h
>@@ -29,10 +29,14 @@
> #define TYPE_CMD646_IDE "cmd646-ide"
> OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE)
> 
>+#include "exec/memory.h"
> #include "hw/ide/pci.h"
> 
> struct CMD646IDEState {
>     PCIIDEState parent_obj;
>+
>+    MemoryRegion bmdma_mem[2];
>+    MemoryRegion bmdma_mem_alias[2][2];

The added complexity of a two-dimensional alias array seems like a tough call for me. I'm not totally against it but I'm reluctant.

Best regards,
Bernhard

> };
> 
> #endif

Re: [PATCH 5/5] cmd646: move device-specific BMDMA registers to separate memory region

[Philippe Mathieu-Daudé]

On 12/6/23 21:28, Bernhard Beschow wrote:
> 
> 
> Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> The aim here is to eliminate any device-specific registers from the main BMDMA
>> bar memory region so it can potentially be moved into the shared PCI IDE device.
>>
>> For each BMDMA bus create a new cmd646-bmdma-specific memory region representing
>> the device-specific BMDMA registers and then map them using aliases onto the
>> existing BMDMAState memory region.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/ide/cmd646.c         | 111 +++++++++++++++++++++++++++++++---------
>> include/hw/ide/cmd646.h |   4 ++
>> 2 files changed, 90 insertions(+), 25 deletions(-)


>> struct CMD646IDEState {
>>      PCIIDEState parent_obj;
>> +
>> +    MemoryRegion bmdma_mem[2];
>> +    MemoryRegion bmdma_mem_alias[2][2];
> 
> The added complexity of a two-dimensional alias array seems like a tough call for me. I'm not totally against it but I'm reluctant.

Alternative:

         struct {
             MemoryRegion mem;
             MemoryRegion mem_alias[2];
         } bmdma[2];

>> };
>>
>> #endif
> 

Re: [PATCH 5/5] cmd646: move device-specific BMDMA registers to separate memory region

[Bernhard Beschow]

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

On Tue, Jun 13, 2023 at 12:39 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 12/6/23 21:28, Bernhard Beschow wrote:
> >
> >
> > Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland <
> mark.cave-ayland@ilande.co.uk>:
> >> The aim here is to eliminate any device-specific registers from the
> main BMDMA
> >> bar memory region so it can potentially be moved into the shared PCI
> IDE device.
> >>
> >> For each BMDMA bus create a new cmd646-bmdma-specific memory region
> representing
> >> the device-specific BMDMA registers and then map them using aliases
> onto the
> >> existing BMDMAState memory region.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >> hw/ide/cmd646.c         | 111 +++++++++++++++++++++++++++++++---------
> >> include/hw/ide/cmd646.h |   4 ++
> >> 2 files changed, 90 insertions(+), 25 deletions(-)
>
>
> >> struct CMD646IDEState {
> >>      PCIIDEState parent_obj;
> >> +
> >> +    MemoryRegion bmdma_mem[2];
> >> +    MemoryRegion bmdma_mem_alias[2][2];
> >
> > The added complexity of a two-dimensional alias array seems like a tough
> call for me. I'm not totally against it but I'm reluctant.
>
> Alternative:
>
>          struct {
>              MemoryRegion mem;
>              MemoryRegion mem_alias[2];
>

If `mem_alias` became an anonymous struct as well we could avoid fiddling
with two indices in a matrix, lowering the complexity.

Best regards,
Bernhard

>          } bmdma[2];
>
> >> };
> >>
> >> #endif
> >
>
>

[-- Attachment #2: Type: text/html, Size: 2441 bytes --]