qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: BALATON Zoltan
Subject: Re: [PATCH 11/13] hw/ide/sii3112: Reuse PCIIDEState::{cmd,data}_ops
Date: Thu, 27 Apr 2023 14:55:40 +0200 (CEST)

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 <mark.cave-ayland@ilande.co.uk>:
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 <shentey@gmail.com>
---
  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(&d->i.bus[0], addr - 0x80, size);
-        break;
-    case 0x8a:
-        val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size);
-        break;
      case 0xa0:
          val = d->regs[0].confstat;
          break;
-    case 0xc0 ... 0xc7:
-        val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size);
-        break;
-    case 0xca:
-        val = pci_ide_cmd_le_ops.read(&d->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(&d->i.bmdma[1], addr - 12, val, size);
          break;
-    case 0x80 ... 0x87:
-        pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size);
-        break;
-    case 0x8a:
-        pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size);
-        break;
-    case 0xc0 ... 0xc7:
-        pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size);
-        break;
-    case 0xca:
-        pci_ide_cmd_le_ops.write(&d->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, &s->data_ops[0]); +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[0]); +    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->data_ops[1]); +    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &s->cmd_ops[1]);
+
      /* BAR5 is in PCI memory space */
      memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_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", &d->mmio, 0x80, 8);
-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &s->data_ops[0], 0,
+                             memory_region_size(&s->data_ops[0]));
+    memory_region_add_subregion_overlap(&d->mmio, 0x80, mr, 1);
      mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4);
-    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &s->cmd_ops[0], 0,
+                             memory_region_size(&s->cmd_ops[0]));
+    memory_region_add_subregion_overlap(&d->mmio, 0x88, mr, 1);
      mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8);
-    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &s->data_ops[1], 0,
+                             memory_region_size(&s->data_ops[1]));
+    memory_region_add_subregion_overlap(&d->mmio, 0xc0, mr, 1);
      mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4);
-    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr);
+    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &s->cmd_ops[1], 0,
+                             memory_region_size(&s->cmd_ops[1]));
+    memory_region_add_subregion_overlap(&d->mmio, 0xc8, mr, 1);
+
      mr = g_new(MemoryRegion, 1);
      memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
      pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
So if I read this right, this is now switching the aliases over on BAR5 
to allow re-use of the common IDE/BMDMA BARs in PCIIDEState? If that's 
correct then I think the commit message needs a bit more detail, 
otherwise:
That's correct. Besides improving the commit message I'll additonally 
split this patch into two to show what's going on.
Furthermore, I'd init the memory regions in sii3112's init method rather 
than in realize(). This will be more consistent with the other PCI IDE 
device models and with the other memory regions.
Why is an init method meeded? If it's not needed why add it? Just keep it 
simple. My view on these methods is that usually only a realize method is 
needed which is the normal constructor of the object, but sometimes we need 
some properties or something else to be available that can configure the 
object before realising which is when an init method is needed. Sometimes 
QEMU creates an object then destroys it without realising (I think e.g. 
when using help to query device properties) so then memory regions would be 
created and destroyed pointlessly. So unless there's a good reason to have 
an init method I'd leave this device without one to keep it simple. That 
other devices have an init method does not explain why this one should have 
one. Maybe those devices need it for some properties or they are just old 
code and not properly QOM'ified.
From memory one of the QOM developer guides recommends that all 
initialisation should be placed in .instance_init, except for objects that 
depend on properties which are set after .instance_init but before 
.realize().
Any pointer to that doc? I've tried to find it but I couldn't. The 
docs/devel/qom.rst does not say anything about this, The older collection 
of docs here: https://habkost.net/posts/2016/11/incomplete-list-of-qemu-apis.html
also does not have anything useful either, those mostly talk about 
properties instead. The header include/hw/qdev-core.h has some info but it 
only says that "Trivial field initializations should go into 
#TypeInfo.instance_init. Operations depending on @props static properties 
should go into @realize." So I don't see a clear guide on what should go 
where unless something is needed before the device is realized.
Certainly there is some flexibility around this for legacy code, however quite a few reviewers have picked up on previous series I have posted when I have forgotten to move something from .realize() to .instance_init() when allowed after refactoring.
I don't mind if things are done in init or realize as long as we have only 
one of these unless we really need both. Having object init split into two 
functions for no good reason just makes it mode confusing so I'd like to 
keep it at a single place for simlicity. Between init and realize the 
latter seems to be more appropriate for creating the things that are only 
needed when the object is really created and init is for those that may be 
needed when the object is not fully up yet, e.g. for introspection or 
working with connected objects. But for simple devices like sii3112 having 
more than a realize method probably does not make much sense. If you have 
examples of those reviews you refer to maybe that explains this more. I 
just want to avoid unneded complexity.
Regards,
BALATON Zoltan

reply via email to

[Prev in Thread] Current Thread [Next in Thread]