Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-05-03 Thread Mark Cave-Ayland 

On 27/04/2023 13:55, BALATON Zoltan wrote:


On Thu, 27 Apr 2023, Mark Cave-Ayland wrote:

On 27/04/2023 00:24, BALATON Zoltan wrote:

On Wed, 26 Apr 2023, Bernhard Beschow wrote:
Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  -extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  -const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  -const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-    case 0x80 ... 0x87:
-    val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-    break;
-    case 0x8a:
-    val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-    break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-    case 0xc0 ... 0xc7:
-    val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-    break;
-    case 0xca:
-    val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-    break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-    case 0x80 ... 0x87:
-    pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-    break;
-    case 0x8a:
-    pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-    break;
-    case 0xc0 ... 0xc7:
-    pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-    break;
-    case 0xca:
-    pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-    break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)

    /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+    memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+    memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-   

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-27 Thread BALATON Zoltan 

On Thu, 27 Apr 2023, Mark Cave-Ayland wrote:

On 27/04/2023 00:24, BALATON Zoltan wrote:

On Wed, 26 Apr 2023, Bernhard Beschow wrote:
Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:

Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI 
as a

standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  -extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr 
addr,

  ide_ctrl_write(bus, addr + 2, data);
  }
  -const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr 
addr,

  }
  }
  -const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr 
addr,

  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-    case 0x80 ... 0x87:
-    val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, 
size);

-    break;
-    case 0x8a:
-    val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-    break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-    case 0xc0 ... 0xc7:
-    val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, 
size);

-    break;
-    case 0xca:
-    val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-    break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr 
addr,

  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, 
size);

  break;
-    case 0x80 ... 0x87:
-    pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, 
size);

-    break;
-    case 0x8a:
-    pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-    break;
-    case 0xc0 ... 0xc7:
-    pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, 
size);

-    break;
-    case 0xca:
-    pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-    break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, 
Error **errp)

  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, 
>data_ops[0]);
+    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, 
>cmd_ops[0]);
+    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, 
>data_ops[1]);
+    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, 
>cmd_ops[1]);

+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, 
Error **errp)

    /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 
0x80, 8);

-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", 
>data_ops[0], 0,

+ memory_region_size(>data_ops[0]));
+    memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 
0x88, 4);

-    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", 
>cmd_ops[0], 0,

+ memory_region_size(>cmd_ops[0]));
+    memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr,

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-27 Thread Mark Cave-Ayland 

On 27/04/2023 00:24, BALATON Zoltan wrote:


On Wed, 26 Apr 2023, Bernhard Beschow wrote:
Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  -extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  -const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  -const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-    case 0x80 ... 0x87:
-    val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-    break;
-    case 0x8a:
-    val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-    break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-    case 0xc0 ... 0xc7:
-    val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-    break;
-    case 0xca:
-    val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-    break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-    case 0x80 ... 0x87:
-    pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-    break;
-    case 0x8a:
-    pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-    break;
-    case 0xc0 ... 0xc7:
-    pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-    break;
-    case 0xca:
-    pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-    break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
    /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+    memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+    memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >mmio, 0xc0, 8);
-    pci_register_bar(dev, 2,

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-26 Thread BALATON Zoltan 

On Wed, 26 Apr 2023, Bernhard Beschow wrote:

Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  -extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  -const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  -const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
/* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >mmio, 0xc0, 8);
-pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
+

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-26 Thread Bernhard Beschow 
Am 26. April 2023 11:41:54 UTC schrieb Mark Cave-Ayland 
:
>On 22/04/2023 16:07, Bernhard Beschow wrote:
>
>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
>> standard-compliant PCI IDE device.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>>   include/hw/ide/pci.h |  2 --
>>   hw/ide/pci.c |  4 ++--
>>   hw/ide/sii3112.c | 50 
>>   3 files changed, 20 insertions(+), 36 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 5025df5b82..dbb4b13161 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>>   extern MemoryRegionOps bmdma_addr_ioport_ops;
>>   void pci_ide_create_devs(PCIDevice *dev);
>>   -extern const MemoryRegionOps pci_ide_cmd_le_ops;
>> -extern const MemoryRegionOps pci_ide_data_le_ops;
>>   #endif
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index b2fcc00a64..97ccc75aa6 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
>>   ide_ctrl_write(bus, addr + 2, data);
>>   }
>>   -const MemoryRegionOps pci_ide_cmd_le_ops = {
>> +static const MemoryRegionOps pci_ide_cmd_le_ops = {
>>   .read = pci_ide_status_read,
>>   .write = pci_ide_ctrl_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
>>   }
>>   }
>>   -const MemoryRegionOps pci_ide_data_le_ops = {
>> +static const MemoryRegionOps pci_ide_data_le_ops = {
>>   .read = pci_ide_data_read,
>>   .write = pci_ide_data_write,
>>   .endianness = DEVICE_LITTLE_ENDIAN,
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 0af897a9ef..9cf920369f 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr 
>> addr,
>>   val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
>>   val |= (uint32_t)d->i.bmdma[1].status << 16;
>>   break;
>> -case 0x80 ... 0x87:
>> -val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
>> -break;
>> -case 0x8a:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
>> -break;
>>   case 0xa0:
>>   val = d->regs[0].confstat;
>>   break;
>> -case 0xc0 ... 0xc7:
>> -val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
>> -break;
>> -case 0xca:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
>> -break;
>>   case 0xe0:
>>   val = d->regs[1].confstat;
>>   break;
>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>>   case 0x0c ... 0x0f:
>>   bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
>>   break;
>> -case 0x80 ... 0x87:
>> -pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
>> -break;
>> -case 0x8a:
>> -pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
>> -break;
>> -case 0xc0 ... 0xc7:
>> -pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
>> -break;
>> -case 0xca:
>> -pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
>> -break;
>>   case 0x100:
>>   d->regs[0].scontrol = val & 0xfff;
>>   if (val & 1) {
>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>   pci_config_set_interrupt_pin(dev->config, 1);
>>   pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
>>   +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
>> +pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
>> +pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
>> +pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
>> +
>>   /* BAR5 is in PCI memory space */
>>   memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
>>"sii3112.bar5", 0x200);
>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
>>   mr = g_new(MemoryRegion, 1);
>> -memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 
>> 8);
>> -pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
>> +memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", 
>> >data_ops[0], 0,
>> + memory_region_size(>data_ops[0]));
>> +memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
>>   mr = g_new(MemoryRegion, 1);
>> -memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 
>> 4);
>> -pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
>> +memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 
>> 0,
>> +

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-26 Thread Mark Cave-Ayland 

On 22/04/2023 16:07, Bernhard Beschow wrote:


Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
  include/hw/ide/pci.h |  2 --
  hw/ide/pci.c |  4 ++--
  hw/ide/sii3112.c | 50 
  3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
  extern MemoryRegionOps bmdma_addr_ioport_ops;
  void pci_ide_create_devs(PCIDevice *dev);
  
-extern const MemoryRegionOps pci_ide_cmd_le_ops;

-extern const MemoryRegionOps pci_ide_data_le_ops;
  #endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
  ide_ctrl_write(bus, addr + 2, data);
  }
  
-const MemoryRegionOps pci_ide_cmd_le_ops = {

+static const MemoryRegionOps pci_ide_cmd_le_ops = {
  .read = pci_ide_status_read,
  .write = pci_ide_ctrl_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
  }
  }
  
-const MemoryRegionOps pci_ide_data_le_ops = {

+static const MemoryRegionOps pci_ide_data_le_ops = {
  .read = pci_ide_data_read,
  .write = pci_ide_data_write,
  .endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
  val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
  val |= (uint32_t)d->i.bmdma[1].status << 16;
  break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
  case 0xa0:
  val = d->regs[0].confstat;
  break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
  case 0xe0:
  val = d->regs[1].confstat;
  break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
  case 0x0c ... 0x0f:
  bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
  break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
  case 0x100:
  d->regs[0].scontrol = val & 0xfff;
  if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  pci_config_set_interrupt_pin(dev->config, 1);
  pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
  
+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);

+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
  /* BAR5 is in PCI memory space */
  memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
   "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
  
  /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */

  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >cmd_ops[0], 0,
+ memory_region_size(>cmd_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x88, mr, 1);
  mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >mmio, 0xc0, 8);
-pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", >data_ops[1], 0,
+

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-23 Thread BALATON Zoltan 

On Sun, 23 Apr 2023, Bernhard Beschow wrote:

Am 22. April 2023 21:10:14 UTC schrieb BALATON Zoltan :

On Sat, 22 Apr 2023, Bernhard Beschow wrote:

Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
include/hw/ide/pci.h |  2 --
hw/ide/pci.c |  4 ++--
hw/ide/sii3112.c | 50 
3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
extern MemoryRegionOps bmdma_addr_ioport_ops;
void pci_ide_create_devs(PCIDevice *dev);

-extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
#endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
ide_ctrl_write(bus, addr + 2, data);
}

-const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
.read = pci_ide_status_read,
.write = pci_ide_ctrl_write,
.endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
}
}

-const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
.read = pci_ide_data_read,
.write = pci_ide_data_write,
.endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
val |= (uint32_t)d->i.bmdma[1].status << 16;
break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
case 0xa0:
val = d->regs[0].confstat;
break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
case 0xe0:
val = d->regs[1].confstat;
break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
case 0x0c ... 0x0f:
bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
case 0x100:
d->regs[0].scontrol = val & 0xfff;
if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
pci_config_set_interrupt_pin(dev->config, 1);
pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);

+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
/* BAR5 is in PCI memory space */
memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
 "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)

/* BAR0-BAR4 are PCI I/O space aliases into BAR5 */


This patch breaks the above comment


Indeed. It's now the other way around.


OK, then adjust comments as well, also the other one about BAR5 at the top 
which may not be true any more if you remove stuff from BAR5 and alias the 
other BARs instead. The idea here was to follow the data sheet which 
documents the memory space BAR5 and other io BARs are just io space 
aliases of parts of the memory mapped registers. Those are to support 
easily porting older drivers but other drivers may only map BAR5 where all 
the regs are available.


but I think you should not mess with BAR0-4 at all and leave to to 
aliased into BAR5. These have the same registers mirrored and some 
guests access them via the memory mapped BAR5 while others prefer the 
io mapped BAR0-4 so removing these from BAR5 would break some guests.


BARs 0-3 are the PCI-native BARs and BAR4 is the BMDMA BAR which are 
mapped by via and cmd646 already since they support these modes. SIL3112 
supports these modes as well but had custom implementations so far while

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-23 Thread Bernhard Beschow 



Am 22. April 2023 21:10:14 UTC schrieb BALATON Zoltan :
>On Sat, 22 Apr 2023, Bernhard Beschow wrote:
>> Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
>> standard-compliant PCI IDE device.
>> 
>> Signed-off-by: Bernhard Beschow 
>> ---
>> include/hw/ide/pci.h |  2 --
>> hw/ide/pci.c |  4 ++--
>> hw/ide/sii3112.c | 50 
>> 3 files changed, 20 insertions(+), 36 deletions(-)
>> 
>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index 5025df5b82..dbb4b13161 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
>> extern MemoryRegionOps bmdma_addr_ioport_ops;
>> void pci_ide_create_devs(PCIDevice *dev);
>> 
>> -extern const MemoryRegionOps pci_ide_cmd_le_ops;
>> -extern const MemoryRegionOps pci_ide_data_le_ops;
>> #endif
>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
>> index b2fcc00a64..97ccc75aa6 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
>> ide_ctrl_write(bus, addr + 2, data);
>> }
>> 
>> -const MemoryRegionOps pci_ide_cmd_le_ops = {
>> +static const MemoryRegionOps pci_ide_cmd_le_ops = {
>> .read = pci_ide_status_read,
>> .write = pci_ide_ctrl_write,
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> @@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
>> }
>> }
>> 
>> -const MemoryRegionOps pci_ide_data_le_ops = {
>> +static const MemoryRegionOps pci_ide_data_le_ops = {
>> .read = pci_ide_data_read,
>> .write = pci_ide_data_write,
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 0af897a9ef..9cf920369f 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr 
>> addr,
>> val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
>> val |= (uint32_t)d->i.bmdma[1].status << 16;
>> break;
>> -case 0x80 ... 0x87:
>> -val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
>> -break;
>> -case 0x8a:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
>> -break;
>> case 0xa0:
>> val = d->regs[0].confstat;
>> break;
>> -case 0xc0 ... 0xc7:
>> -val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
>> -break;
>> -case 0xca:
>> -val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
>> -break;
>> case 0xe0:
>> val = d->regs[1].confstat;
>> break;
>> @@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
>> case 0x0c ... 0x0f:
>> bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
>> break;
>> -case 0x80 ... 0x87:
>> -pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
>> -break;
>> -case 0x8a:
>> -pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
>> -break;
>> -case 0xc0 ... 0xc7:
>> -pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
>> -break;
>> -case 0xca:
>> -pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
>> -break;
>> case 0x100:
>> d->regs[0].scontrol = val & 0xfff;
>> if (val & 1) {
>> @@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>> pci_config_set_interrupt_pin(dev->config, 1);
>> pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
>> 
>> +pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
>> +pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
>> +pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
>> +pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
>> +
>> /* BAR5 is in PCI memory space */
>> memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
>>  "sii3112.bar5", 0x200);
>> @@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>> 
>> /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
>
>This patch breaks the above comment

Indeed. It's now the other way around.

>but I think you should not mess with BAR0-4 at all and leave to to aliased 
>into BAR5. These have the same registers mirrored and some guests access them 
>via the memory mapped BAR5 while others prefer the io mapped BAR0-4 so 
>removing these from BAR5 would break some guests.

BARs 0-3 are the PCI-native BARs and BAR4 is the BMDMA BAR which are mapped by 
via and cmd646 already since they support these modes. SIL3112 supports these 
modes as well but had custom implementations so far while ignoring the 
attributes of the parent class. Now that the parent class already initializes 
these attributes we can just reuse them here which in addition makes it very 
obvious that

Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops

2023-04-22 Thread BALATON Zoltan 

On Sat, 22 Apr 2023, Bernhard Beschow wrote:

Allows to unexport pci_ide_{cmd,data}_le_ops and models TYPE_SII3112_PCI as a
standard-compliant PCI IDE device.

Signed-off-by: Bernhard Beschow 
---
include/hw/ide/pci.h |  2 --
hw/ide/pci.c |  4 ++--
hw/ide/sii3112.c | 50 
3 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 5025df5b82..dbb4b13161 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,4 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
extern MemoryRegionOps bmdma_addr_ioport_ops;
void pci_ide_create_devs(PCIDevice *dev);

-extern const MemoryRegionOps pci_ide_cmd_le_ops;
-extern const MemoryRegionOps pci_ide_data_le_ops;
#endif
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b2fcc00a64..97ccc75aa6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -60,7 +60,7 @@ static void pci_ide_ctrl_write(void *opaque, hwaddr addr,
ide_ctrl_write(bus, addr + 2, data);
}

-const MemoryRegionOps pci_ide_cmd_le_ops = {
+static const MemoryRegionOps pci_ide_cmd_le_ops = {
.read = pci_ide_status_read,
.write = pci_ide_ctrl_write,
.endianness = DEVICE_LITTLE_ENDIAN,
@@ -98,7 +98,7 @@ static void pci_ide_data_write(void *opaque, hwaddr addr,
}
}

-const MemoryRegionOps pci_ide_data_le_ops = {
+static const MemoryRegionOps pci_ide_data_le_ops = {
.read = pci_ide_data_read,
.write = pci_ide_data_write,
.endianness = DEVICE_LITTLE_ENDIAN,
diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 0af897a9ef..9cf920369f 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,21 +88,9 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
val |= (d->regs[1].confstat & (1UL << 11) ? (1 << 4) : 0);
val |= (uint32_t)d->i.bmdma[1].status << 16;
break;
-case 0x80 ... 0x87:
-val = pci_ide_data_le_ops.read(>i.bus[0], addr - 0x80, size);
-break;
-case 0x8a:
-val = pci_ide_cmd_le_ops.read(>i.bus[0], 2, size);
-break;
case 0xa0:
val = d->regs[0].confstat;
break;
-case 0xc0 ... 0xc7:
-val = pci_ide_data_le_ops.read(>i.bus[1], addr - 0xc0, size);
-break;
-case 0xca:
-val = pci_ide_cmd_le_ops.read(>i.bus[1], 2, size);
-break;
case 0xe0:
val = d->regs[1].confstat;
break;
@@ -171,18 +159,6 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
case 0x0c ... 0x0f:
bmdma_addr_ioport_ops.write(>i.bmdma[1], addr - 12, val, size);
break;
-case 0x80 ... 0x87:
-pci_ide_data_le_ops.write(>i.bus[0], addr - 0x80, val, size);
-break;
-case 0x8a:
-pci_ide_cmd_le_ops.write(>i.bus[0], 2, val, size);
-break;
-case 0xc0 ... 0xc7:
-pci_ide_data_le_ops.write(>i.bus[1], addr - 0xc0, val, size);
-break;
-case 0xca:
-pci_ide_cmd_le_ops.write(>i.bus[1], 2, val, size);
-break;
case 0x100:
d->regs[0].scontrol = val & 0xfff;
if (val & 1) {
@@ -259,6 +235,11 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
pci_config_set_interrupt_pin(dev->config, 1);
pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);

+pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[0]);
+pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[0]);
+pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, >data_ops[1]);
+pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, >cmd_ops[1]);
+
/* BAR5 is in PCI memory space */
memory_region_init_io(>mmio, OBJECT(d), _reg_ops, d,
 "sii3112.bar5", 0x200);
@@ -266,17 +247,22 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)

/* BAR0-BAR4 are PCI I/O space aliases into BAR5 */


This patch breaks the above comment but I think you should not mess with 
BAR0-4 at all and leave to to aliased into BAR5. These have the same 
registers mirrored and some guests access them via the memory mapped BAR5 
while others prefer the io mapped BAR0-4 so removing these from BAR5 would 
break some guests. If you want to remove something from BAR5 and map 
subregions implementing those instead then I think only BAR5 needs to be 
chnaged or I'm not getting what is happening here so a more detailed 
commit message would be needed.


Was this tested? A minimal test might be booting AROS and MorphOS on 
sam460ex.


Regards,
BALATON Zoltan


mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >mmio, 0x80, 8);
-pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", >data_ops[0], 0,
+ memory_region_size(>data_ops[0]));
+memory_region_add_subregion_overlap(>mmio, 0x80, mr, 1);
mr = g_new(MemoryRegion, 1);
-memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", >mmio, 0x88, 4);
-