[05/15] memory: introduce life_ops to MemoryRegion

Commit Message

Liu Ping Fan Aug. 8, 2012, 6:25 a.m. UTC

From: Liu Ping Fan <[e-mail]>

The types of referred object by MemoryRegion are variable, ex,
another mr, DeviceState, or other struct defined by drivers.
So the refer/unrefer may be different by drivers.

Using this ops, we can mange the backend object.

Signed-off-by: Liu Ping Fan <[e-mail]>
---
 hw/ide/piix.c |    6 ++--
 hw/pckbd.c    |    6 +++-
 hw/serial.c   |    2 +-
 ioport.c      |    3 +-
 memory.c      |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 memory.h      |   16 +++++++++++++
 6 files changed, 94 insertions(+), 8 deletions(-)

Comments

Avi Kivity Aug. 8, 2012, 9:18 a.m. UTC | #1

On 08/08/2012 09:25 AM, Liu Ping Fan wrote:
> From: Liu Ping Fan <[e-mail]>
> 
> The types of referred object by MemoryRegion are variable, ex,
> another mr, DeviceState, or other struct defined by drivers.
> So the refer/unrefer may be different by drivers.
> 
> Using this ops, we can mange the backend object.
> 

Seems to be a needless abstration - we already have lifetime manangement
for objects.

I suggested previously to replace the opaque parameter with an Object,
and use Object's refcounting.  That's a lot of work, but IMO is worth it
as the opaques are dangerous to live lying around.

Patch

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index f5a74c2..bdd70b1 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -93,11 +93,11 @@  static void bmdma_setup_bar(PCIIDEState *d)
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
 
-        memory_region_init_io(&bm->extra_io, &piix_bmdma_ops, bm,
+        memory_region_init_io_ext(&bm->extra_io, &piix_bmdma_ops, NULL, bm,
                               "piix-bmdma", 4);
         memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io);
-        memory_region_init_io(&bm->addr_ioport, &bmdma_addr_ioport_ops, bm,
-                              "bmdma", 4);
+        memory_region_init_io_ext(&bm->addr_ioport, &bmdma_addr_ioport_ops,
+                              NULL, bm, "bmdma", 4);
         memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport);
     }
 }
diff --git a/hw/pckbd.c b/hw/pckbd.c
index 69857ba..de3c46d 100644
--- a/hw/pckbd.c
+++ b/hw/pckbd.c
@@ -485,10 +485,12 @@  static int i8042_initfn(ISADevice *dev)
     isa_init_irq(dev, &s->irq_kbd, 1);
     isa_init_irq(dev, &s->irq_mouse, 12);
 
-    memory_region_init_io(isa_s->io + 0, &i8042_data_ops, s, "i8042-data", 1);
+    memory_region_init_io_ext(isa_s->io + 0, &i8042_data_ops, NULL, s,
+                                "i8042-data", 1);
     isa_register_ioport(dev, isa_s->io + 0, 0x60);
 
-    memory_region_init_io(isa_s->io + 1, &i8042_cmd_ops, s, "i8042-cmd", 1);
+    memory_region_init_io_ext(isa_s->io + 1, &i8042_cmd_ops, NULL, s,
+                                "i8042-cmd", 1);
     isa_register_ioport(dev, isa_s->io + 1, 0x64);
 
     s->kbd = ps2_kbd_init(kbd_update_kbd_irq, s);
diff --git a/hw/serial.c b/hw/serial.c
index a421d1e..e992c6a 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -794,7 +794,7 @@  static int serial_isa_initfn(ISADevice *dev)
     serial_init_core(s);
     qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3);
 
-    memory_region_init_io(&s->io, &serial_io_ops, s, "serial", 8);
+    memory_region_init_io_ext(&s->io, &serial_io_ops, NULL, s, "serial", 8);
     isa_register_ioport(dev, &s->io, isa->iobase);
     return 0;
 }
diff --git a/ioport.c b/ioport.c
index 6e4ca0d..768e271 100644
--- a/ioport.c
+++ b/ioport.c
@@ -384,7 +384,8 @@  static void portio_list_add_1(PortioList *piolist,
      * Use an alias so that the callback is called with an absolute address,
      * rather than an offset relative to to start + off_low.
      */
-    memory_region_init_io(region, ops, piolist->opaque, piolist->name,
+    memory_region_init_io_ext(region, ops, NULL, piolist->opaque,
+                          piolist->name,
                           INT64_MAX);
     memory_region_init_alias(alias, piolist->name,
                              region, start + off_low, off_high - off_low);
diff --git a/memory.c b/memory.c
index 5986532..80c7529 100644
--- a/memory.c
+++ b/memory.c
@@ -19,6 +19,7 @@ 
 #include "bitops.h"
 #include "kvm.h"
 #include <assert.h>
+#include "hw/qdev.h"
 
 #define WANT_EXEC_OBSOLETE
 #include "exec-obsolete.h"
@@ -799,6 +800,7 @@  static bool memory_region_wrong_endianness(MemoryRegion *mr)
 #endif
 }
 
+static MemoryRegionLifeOps nops;
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)
@@ -809,6 +811,7 @@  void memory_region_init(MemoryRegion *mr,
     if (size == UINT64_MAX) {
         mr->size = int128_2_64();
     }
+    mr->life_ops = &nops;
     mr->addr = 0;
     mr->subpage = false;
     mr->enabled = true;
@@ -931,6 +934,66 @@  static void memory_region_dispatch_write(MemoryRegion *mr,
                               memory_region_write_accessor, mr);
 }
 
+static void mr_object_get(MemoryRegion *mr)
+{
+    object_dynamic_cast_assert(OBJECT(mr->opaque), TYPE_DEVICE);
+    object_ref(OBJECT(mr->opaque));
+}
+
+static void mr_object_put(MemoryRegion *mr)
+{
+    object_unref(OBJECT(mr->opaque));
+}
+
+static MemoryRegionLifeOps obj_ops = {
+    .get = mr_object_get,
+    .put = mr_object_put,
+};
+
+static void mr_alias_get(MemoryRegion *mr)
+{
+}
+
+static void mr_alias_put(MemoryRegion *mr)
+{
+}
+
+static MemoryRegionLifeOps alias_ops = {
+    .get = mr_alias_get,
+    .put = mr_alias_put,
+};
+
+static void mr_nop_get(MemoryRegion *mr)
+{
+}
+
+static void mr_nop_put(MemoryRegion *mr)
+{
+}
+
+static MemoryRegionLifeOps nops = {
+    .get = mr_nop_get,
+    .put = mr_nop_put,
+};
+
+void memory_region_init_io_ext(MemoryRegion *mr,
+                           const MemoryRegionOps *ops,
+                           MemoryRegionLifeOps *life_ops,
+                           void *opaque,
+                           const char *name,
+                           uint64_t size)
+{
+    memory_region_init(mr, name, size);
+    mr->ops = ops;
+    if (life_ops == NULL) {
+        mr->life_ops = &nops;
+    }
+    mr->opaque = opaque;
+    mr->terminates = true;
+    mr->destructor = memory_region_destructor_iomem;
+    mr->ram_addr = ~(ram_addr_t)0;
+}
+
 void memory_region_init_io(MemoryRegion *mr,
                            const MemoryRegionOps *ops,
                            void *opaque,
@@ -939,6 +1002,9 @@  void memory_region_init_io(MemoryRegion *mr,
 {
     memory_region_init(mr, name, size);
     mr->ops = ops;
+    if (opaque != NULL) {
+        mr->life_ops = &obj_ops;
+    }
     mr->opaque = opaque;
     mr->terminates = true;
     mr->destructor = memory_region_destructor_iomem;
@@ -975,6 +1041,7 @@  void memory_region_init_alias(MemoryRegion *mr,
                               uint64_t size)
 {
     memory_region_init(mr, name, size);
+    mr->life_ops = &alias_ops;
     mr->alias = orig;
     mr->alias_offset = offset;
 }
@@ -1027,7 +1094,7 @@  void memory_region_init_reservation(MemoryRegion *mr,
                                     const char *name,
                                     uint64_t size)
 {
-    memory_region_init_io(mr, &reservation_ops, mr, name, size);
+    memory_region_init_io_ext(mr, &reservation_ops, &nops, mr, name, size);
 }
 
 void memory_region_destroy(MemoryRegion *mr)
diff --git a/memory.h b/memory.h
index fe6aefa..8fb543b 100644
--- a/memory.h
+++ b/memory.h
@@ -28,6 +28,7 @@ 
 #include "qemu-thread.h"
 
 typedef struct MemoryRegionOps MemoryRegionOps;
+typedef struct MemoryRegionLifeOps MemoryRegionLifeOps;
 typedef struct MemoryRegion MemoryRegion;
 typedef struct MemoryRegionPortio MemoryRegionPortio;
 typedef struct MemoryRegionMmio MemoryRegionMmio;
@@ -52,6 +53,11 @@  struct MemoryRegionIORange {
     target_phys_addr_t offset;
 };
 
+struct MemoryRegionLifeOps {
+    void (*get)(MemoryRegion *mr);
+    void (*put)(MemoryRegion *mr);
+};
+
 /*
  * Memory region callbacks
  */
@@ -120,6 +126,7 @@  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
 struct MemoryRegion {
     /* All fields are private - violators will be prosecuted */
     const MemoryRegionOps *ops;
+    MemoryRegionLifeOps *life_ops;
     void *opaque;
     MemoryRegion *parent;
     Int128 size;
@@ -209,6 +216,7 @@  struct MemoryListener {
 };
 
 extern QemuMutex mem_map_lock;
+
 /**
  * memory_region_init: Initialize a memory region
  *
@@ -222,6 +230,14 @@  extern QemuMutex mem_map_lock;
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size);
+
+void memory_region_init_io_ext(MemoryRegion *mr,
+                           const MemoryRegionOps *ops,
+                           MemoryRegionLifeOps *life_ops,
+                           void *opaque,
+                           const char *name,
+                           uint64_t size);
+
 /**
  * memory_region_init_io: Initialize an I/O memory region.
  *