[Qemu-devel] [PATCH 0/2] Remove memory_region_set_offset()

[Qemu-devel] [PATCH 0/2] Remove memory_region_set_offset()

[Avi Kivity]

memory_region_set_offset() is used in only one place, and is deprecated.
Remove the single use and the function itself.

Avi Kivity (2):
  ioport: change portio_list not to use memory_region_set_offset()
  memory: remove memory_region_set_offset()

 ioport.c |   25 +++++++++++++++++++------
 ioport.h |    1 +
 memory.c |   26 ++++++++++----------------
 memory.h |    9 ---------
 4 files changed, 30 insertions(+), 31 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH 1/2] ioport: change portio_list not to use memory_region_set_offset()

[Avi Kivity]

memory_region_set_offset() will be going away soon, so don't use it.
Use an alias instead.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 ioport.c |   25 +++++++++++++++++++------
 ioport.h |    1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/ioport.c b/ioport.c
index 36fa3a4..505b252 100644
--- a/ioport.c
+++ b/ioport.c
@@ -328,6 +328,7 @@ void portio_list_init(PortioList *piolist,
     piolist->ports = callbacks;
     piolist->nr = 0;
     piolist->regions = g_new0(MemoryRegion *, n);
+    piolist->aliases = g_new0(MemoryRegion *, n);
     piolist->address_space = NULL;
     piolist->opaque = opaque;
     piolist->name = name;
@@ -336,6 +337,7 @@ void portio_list_init(PortioList *piolist,
 void portio_list_destroy(PortioList *piolist)
 {
     g_free(piolist->regions);
+    g_free(piolist->aliases);
 }
 
 static void portio_list_add_1(PortioList *piolist,
@@ -345,7 +347,7 @@ static void portio_list_add_1(PortioList *piolist,
 {
     MemoryRegionPortio *pio;
     MemoryRegionOps *ops;
-    MemoryRegion *region;
+    MemoryRegion *region, *alias;
     unsigned i;
 
     /* Copy the sub-list and null-terminate it.  */
@@ -362,12 +364,19 @@ static void portio_list_add_1(PortioList *piolist,
     ops->old_portio = pio;
 
     region = g_new(MemoryRegion, 1);
+    alias = g_new(MemoryRegion, 1);
+    /*
+     * 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,
-                          off_high - off_low);
-    memory_region_set_offset(region, start + off_low);
+                          UINT64_MAX);
+    memory_region_init_alias(alias, piolist->name,
+                             region, start + off_low, off_high - off_low);
     memory_region_add_subregion(piolist->address_space,
-                                start + off_low, region);
+                                start + off_low, alias);
     piolist->regions[piolist->nr++] = region;
+    piolist->aliases[piolist->nr++] = alias;
 }
 
 void portio_list_add(PortioList *piolist,
@@ -409,15 +418,19 @@ void portio_list_add(PortioList *piolist,
 
 void portio_list_del(PortioList *piolist)
 {
-    MemoryRegion *mr;
+    MemoryRegion *mr, *alias;
     unsigned i;
 
     for (i = 0; i < piolist->nr; ++i) {
         mr = piolist->regions[i];
-        memory_region_del_subregion(piolist->address_space, mr);
+        alias = piolist->aliases[i];
+        memory_region_del_subregion(piolist->address_space, alias);
+        memory_region_destroy(alias);
         memory_region_destroy(mr);
         g_free((MemoryRegionOps *)mr->ops);
         g_free(mr);
+        g_free(alias);
         piolist->regions[i] = NULL;
+        piolist->aliases[i] = NULL;
     }
 }
diff --git a/ioport.h b/ioport.h
index ae3e9da..ab29c89 100644
--- a/ioport.h
+++ b/ioport.h
@@ -60,6 +60,7 @@ typedef struct PortioList {
     struct MemoryRegion *address_space;
     unsigned nr;
     struct MemoryRegion **regions;
+    struct MemoryRegion **aliases;
     void *opaque;
     const char *name;
 } PortioList;
-- 
1.7.7.1

[Qemu-devel] [PATCH 2/2] memory: remove memory_region_set_offset()

[Avi Kivity]

memory_region_set_offset() complicates the API, and has been deprecated
since its introduction.  Now that it is no longer used, remove it.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 memory.c |   26 ++++++++++----------------
 memory.h |    9 ---------
 2 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/memory.c b/memory.c
index 5ab2112..32b3901 100644
--- a/memory.c
+++ b/memory.c
@@ -399,17 +399,17 @@ static void memory_region_iorange_read(IORange *iorange,
 
         *data = ((uint64_t)1 << (width * 8)) - 1;
         if (mrp) {
-            *data = mrp->read(mr->opaque, offset + mr->offset);
+            *data = mrp->read(mr->opaque, offset);
         } else if (width == 2) {
             mrp = find_portio(mr, offset, 1, false);
             assert(mrp);
-            *data = mrp->read(mr->opaque, offset + mr->offset) |
-                    (mrp->read(mr->opaque, offset + mr->offset + 1) << 8);
+            *data = mrp->read(mr->opaque, offset) |
+                    (mrp->read(mr->opaque, offset + 1) << 8);
         }
         return;
     }
     *data = 0;
-    access_with_adjusted_size(offset + mr->offset, data, width,
+    access_with_adjusted_size(offset, data, width,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_read_accessor, mr);
@@ -426,16 +426,16 @@ static void memory_region_iorange_write(IORange *iorange,
         const MemoryRegionPortio *mrp = find_portio(mr, offset, width, true);
 
         if (mrp) {
-            mrp->write(mr->opaque, offset + mr->offset, data);
+            mrp->write(mr->opaque, offset, data);
         } else if (width == 2) {
             mrp = find_portio(mr, offset, 1, false);
             assert(mrp);
-            mrp->write(mr->opaque, offset + mr->offset, data & 0xff);
-            mrp->write(mr->opaque, offset + mr->offset + 1, data >> 8);
+            mrp->write(mr->opaque, offset, data & 0xff);
+            mrp->write(mr->opaque, offset + 1, data >> 8);
         }
         return;
     }
-    access_with_adjusted_size(offset + mr->offset, &data, width,
+    access_with_adjusted_size(offset, &data, width,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_write_accessor, mr);
@@ -861,7 +861,6 @@ void memory_region_init(MemoryRegion *mr,
         mr->size = int128_2_64();
     }
     mr->addr = 0;
-    mr->offset = 0;
     mr->subpage = false;
     mr->enabled = true;
     mr->terminates = false;
@@ -923,7 +922,7 @@ static uint64_t memory_region_dispatch_read1(MemoryRegion *mr,
     }
 
     /* FIXME: support unaligned access */
-    access_with_adjusted_size(addr + mr->offset, &data, size,
+    access_with_adjusted_size(addr, &data, size,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_read_accessor, mr);
@@ -977,7 +976,7 @@ static void memory_region_dispatch_write(MemoryRegion *mr,
     }
 
     /* FIXME: support unaligned access */
-    access_with_adjusted_size(addr + mr->offset, &data, size,
+    access_with_adjusted_size(addr, &data, size,
                               mr->ops->impl.min_access_size,
                               mr->ops->impl.max_access_size,
                               memory_region_write_accessor, mr);
@@ -1084,11 +1083,6 @@ bool memory_region_is_rom(MemoryRegion *mr)
     return mr->ram && mr->readonly;
 }
 
-void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t offset)
-{
-    mr->offset = offset;
-}
-
 void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
 {
     uint8_t mask = 1 << client;
diff --git a/memory.h b/memory.h
index 70f57fb..9fe45a4 100644
--- a/memory.h
+++ b/memory.h
@@ -115,7 +115,6 @@ struct MemoryRegion {
     MemoryRegion *parent;
     Int128 size;
     target_phys_addr_t addr;
-    target_phys_addr_t offset;
     void (*destructor)(MemoryRegion *mr);
     ram_addr_t ram_addr;
     IORange iorange;
@@ -343,14 +342,6 @@ bool memory_region_is_rom(MemoryRegion *mr);
 void *memory_region_get_ram_ptr(MemoryRegion *mr);
 
 /**
- * memory_region_set_offset: Sets an offset to be added to MemoryRegionOps
- *                           callbacks.
- *
- * This function is deprecated and should not be used in new code.
- */
-void memory_region_set_offset(MemoryRegion *mr, target_phys_addr_t offset);
-
-/**
  * memory_region_set_log: Turn dirty logging on or off for a region.
  *
  * Turns dirty logging on or off for a specified client (display, migration).
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH 0/2] Remove memory_region_set_offset()

[Andreas Färber]

Am 08.01.2012 18:57, schrieb Avi Kivity:
> memory_region_set_offset() is used in only one place, and is deprecated.
> Remove the single use and the function itself.

Does the removal of the offset mean that memory_region_find() can be
simplified to just return a MemoryRegion* now? :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/2] Remove memory_region_set_offset()

[Avi Kivity]

On 01/08/2012 08:59 PM, Andreas Färber wrote:
> Am 08.01.2012 18:57, schrieb Avi Kivity:
> > memory_region_set_offset() is used in only one place, and is deprecated.
> > Remove the single use and the function itself.
>
> Does the removal of the offset mean that memory_region_find() can be
> simplified to just return a MemoryRegion* now? :)

No.  In particular, aliases allow almost the same offsetting
capabilities (the difference is that aliases cannot offset beyond ->size
(or before 0) while set_offset() can.

Consider a vga window at 0xa0000 pointing at the framebuffer at
0xe0000000, with alias_offset 0x10000.

A memory_region_find() that finds it will return (mr,
.offset_within_region = 0x10000), .size = 0x8000, there is no way this
can be expressed by returning just a memory region (well you could
return the alias, but that complicates things for the caller).

-- 
error compiling committee.c: too many arguments to function