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