[Qemu-devel] [PATCH 0/2] memory: cleanup users of memory_region_get_ram_addr

[Qemu-devel] [PATCH 0/2] memory: cleanup users of memory_region_get_ram_addr

[Paolo Bonzini]

The result of memory_region_get_ram_addr is often added in the
caller of qemu_get_ram_ptr and then subtracted in qemu_get_ram_ptr;
avoid the indirection.  Prompted by mst's remark on confusion
between hwaddr and ram_addr_t.

Paolo

Paolo Bonzini (2):
  memory: remove unnecessary masking of MemoryRegion ram_addr
  memory: hide ram_addr_t from qemu_get_ram_ptr users

 exec.c                       | 49 +++++++++++++++-----------------------------
 include/exec/memory.h        |  1 -
 memory.c                     |  7 +++----
 migration/savevm.c           |  4 ++--
 scripts/dump-guest-memory.py | 19 +++--------------
 translate-all.c              |  3 +--
 6 files changed, 25 insertions(+), 58 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] memory: remove unnecessary masking of MemoryRegion ram_addr

[Paolo Bonzini]

mr->ram_block->offset is already aligned to both host and target size
(see qemu_ram_alloc_internal).  Remove further masking as it is
unnecessary.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c             | 21 +++++++--------------
 memory.c           |  5 ++---
 migration/savevm.c |  4 ++--
 translate-all.c    |  3 +--
 4 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/exec.c b/exec.c
index f398d21..001b669 100644
--- a/exec.c
+++ b/exec.c
@@ -1042,8 +1042,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
 
     if (memory_region_is_ram(section->mr)) {
         /* Normal RAM.  */
-        iotlb = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
-            + xlat;
+        iotlb = memory_region_get_ram_addr(section->mr) + xlat;
         if (!section->readonly) {
             iotlb |= PHYS_SECTION_NOTDIRTY;
         } else {
@@ -3093,9 +3092,7 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr,
     } else {
         /* RAM case */
         ptr = qemu_get_ram_ptr(mr->ram_block,
-                               (memory_region_get_ram_addr(mr)
-                                & TARGET_PAGE_MASK)
-                               + addr1);
+                               memory_region_get_ram_addr(mr) + addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldl_le_p(ptr);
@@ -3189,9 +3186,7 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr,
     } else {
         /* RAM case */
         ptr = qemu_get_ram_ptr(mr->ram_block,
-                               (memory_region_get_ram_addr(mr)
-                                & TARGET_PAGE_MASK)
-                               + addr1);
+                               memory_region_get_ram_addr(mr) + addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldq_le_p(ptr);
@@ -3305,9 +3300,7 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as,
     } else {
         /* RAM case */
         ptr = qemu_get_ram_ptr(mr->ram_block,
-                               (memory_region_get_ram_addr(mr)
-                                & TARGET_PAGE_MASK)
-                               + addr1);
+                               memory_region_get_ram_addr(mr) + addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = lduw_le_p(ptr);
@@ -3389,7 +3382,7 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
 
         r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
     } else {
-        addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
+        addr1 += memory_region_get_ram_addr(mr);
         ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         stl_p(ptr, val);
 
@@ -3444,7 +3437,7 @@ static inline void address_space_stl_internal(AddressSpace *as,
         r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
     } else {
         /* RAM case */
-        addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
+        addr1 += memory_region_get_ram_addr(mr);
         ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
@@ -3554,7 +3547,7 @@ static inline void address_space_stw_internal(AddressSpace *as,
         r = memory_region_dispatch_write(mr, addr1, val, 2, attrs);
     } else {
         /* RAM case */
-        addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
+        addr1 += memory_region_get_ram_addr(mr);
         ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
diff --git a/memory.c b/memory.c
index 95f7209..49c9b14 100644
--- a/memory.c
+++ b/memory.c
@@ -1640,7 +1640,7 @@ int memory_region_get_fd(MemoryRegion *mr)
 
     assert(mr->ram_block);
 
-    return qemu_get_ram_fd(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
+    return qemu_get_ram_fd(memory_region_get_ram_addr(mr));
 }
 
 void *memory_region_get_ram_ptr(MemoryRegion *mr)
@@ -1654,8 +1654,7 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
         mr = mr->alias;
     }
     assert(mr->ram_block);
-    ptr = qemu_get_ram_ptr(mr->ram_block,
-                           memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
+    ptr = qemu_get_ram_ptr(mr->ram_block, memory_region_get_ram_addr(mr));
     rcu_read_unlock();
 
     return ptr + offset;
diff --git a/migration/savevm.c b/migration/savevm.c
index 0a33c22..cbba062 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2229,13 +2229,13 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
 {
-    qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
+    qemu_ram_set_idstr(memory_region_get_ram_addr(mr),
                        memory_region_name(mr), dev);
 }
 
 void vmstate_unregister_ram(MemoryRegion *mr, DeviceState *dev)
 {
-    qemu_ram_unset_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
+    qemu_ram_unset_idstr(memory_region_get_ram_addr(mr));
 }
 
 void vmstate_register_ram_global(MemoryRegion *mr)
diff --git a/translate-all.c b/translate-all.c
index e9f409b..6820d8e 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1550,8 +1550,7 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr)
         rcu_read_unlock();
         return;
     }
-    ram_addr = (memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK)
-        + addr;
+    ram_addr = memory_region_get_ram_addr(mr) + addr;
     tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
     rcu_read_unlock();
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
address that is relative to the MemoryRegion.  This basically means
what address_space_translate returns.

invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
not need it at all.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c                       | 40 +++++++++++++++-------------------------
 include/exec/memory.h        |  1 -
 memory.c                     |  4 ++--
 scripts/dump-guest-memory.py | 19 +++----------------
 4 files changed, 20 insertions(+), 44 deletions(-)

diff --git a/exec.c b/exec.c
index 001b669..ca9e3b6 100644
--- a/exec.c
+++ b/exec.c
@@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
 
     if (block == NULL) {
         block = qemu_get_ram_block(addr);
+        addr -= block->offset;
     }
 
     if (xen_enabled() && block->host == NULL) {
@@ -1889,7 +1890,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
 
         block->host = xen_map_cache(block->offset, block->max_length, 1);
     }
-    return ramblock_ptr(block, addr - block->offset);
+    return ramblock_ptr(block, addr);
 }
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
@@ -1901,16 +1902,15 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
                                  hwaddr *size)
 {
     RAMBlock *block = ram_block;
-    ram_addr_t offset_inside_block;
     if (*size == 0) {
         return NULL;
     }
 
     if (block == NULL) {
         block = qemu_get_ram_block(addr);
+        addr -= block->offset;
     }
-    offset_inside_block = addr - block->offset;
-    *size = MIN(*size, block->max_length - offset_inside_block);
+    *size = MIN(*size, block->max_length - addr);
 
     if (xen_enabled() && block->host == NULL) {
         /* We need to check if the requested address is in the RAM
@@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
         block->host = xen_map_cache(block->offset, block->max_length, 1);
     }
 
-    return ramblock_ptr(block, offset_inside_block);
+    return ramblock_ptr(block, addr);
 }
 
 /*
@@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
                                      hwaddr length)
 {
     uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
+    addr += memory_region_get_ram_addr(mr);
+
     /* No early return if dirty_log_mask is or becomes 0, because
      * cpu_physical_memory_set_dirty_range will still call
      * xen_modified_memory.
@@ -2616,7 +2618,6 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
                 abort();
             }
         } else {
-            addr1 += memory_region_get_ram_addr(mr);
             /* RAM case */
             ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
             memcpy(ptr, buf, l);
@@ -2709,8 +2710,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
             }
         } else {
             /* RAM case */
-            ptr = qemu_get_ram_ptr(mr->ram_block,
-                                   memory_region_get_ram_addr(mr) + addr1);
+            ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
             memcpy(buf, ptr, l);
         }
 
@@ -2793,7 +2793,6 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
               memory_region_is_romd(mr))) {
             l = memory_access_size(mr, l, addr1);
         } else {
-            addr1 += memory_region_get_ram_addr(mr);
             /* ROM/RAM case */
             ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
             switch (type) {
@@ -2953,7 +2952,6 @@ void *address_space_map(AddressSpace *as,
     hwaddr done = 0;
     hwaddr l, xlat, base;
     MemoryRegion *mr, *this_mr;
-    ram_addr_t raddr;
     void *ptr;
 
     if (len == 0) {
@@ -2962,7 +2960,7 @@ void *address_space_map(AddressSpace *as,
 
     l = len;
     rcu_read_lock();
-    mr = address_space_translate(as, addr, &xlat, &l, is_write);
+    mr = address_space_translate(as, addr, &base, &l, is_write);
 
     if (!memory_access_is_direct(mr, is_write)) {
         if (atomic_xchg(&bounce.in_use, true)) {
@@ -2987,9 +2985,6 @@ void *address_space_map(AddressSpace *as,
         return bounce.buffer;
     }
 
-    base = xlat;
-    raddr = memory_region_get_ram_addr(mr);
-
     for (;;) {
         len -= l;
         addr += l;
@@ -3007,7 +3002,7 @@ void *address_space_map(AddressSpace *as,
 
     memory_region_ref(mr);
     *plen = done;
-    ptr = qemu_ram_ptr_length(mr->ram_block, raddr + base, plen);
+    ptr = qemu_ram_ptr_length(mr->ram_block, base, plen);
     rcu_read_unlock();
 
     return ptr;
@@ -3091,8 +3086,7 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr,
 #endif
     } else {
         /* RAM case */
-        ptr = qemu_get_ram_ptr(mr->ram_block,
-                               memory_region_get_ram_addr(mr) + addr1);
+        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldl_le_p(ptr);
@@ -3185,8 +3179,7 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr,
 #endif
     } else {
         /* RAM case */
-        ptr = qemu_get_ram_ptr(mr->ram_block,
-                               memory_region_get_ram_addr(mr) + addr1);
+        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldq_le_p(ptr);
@@ -3299,8 +3292,7 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as,
 #endif
     } else {
         /* RAM case */
-        ptr = qemu_get_ram_ptr(mr->ram_block,
-                               memory_region_get_ram_addr(mr) + addr1);
+        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = lduw_le_p(ptr);
@@ -3382,13 +3374,13 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
 
         r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
     } else {
-        addr1 += memory_region_get_ram_addr(mr);
         ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         stl_p(ptr, val);
 
         dirty_log_mask = memory_region_get_dirty_log_mask(mr);
         dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
-        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
+        cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr) + addr,
+                                            4, dirty_log_mask);
         r = MEMTX_OK;
     }
     if (result) {
@@ -3437,7 +3429,6 @@ static inline void address_space_stl_internal(AddressSpace *as,
         r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
     } else {
         /* RAM case */
-        addr1 += memory_region_get_ram_addr(mr);
         ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
@@ -3547,7 +3538,6 @@ static inline void address_space_stw_internal(AddressSpace *as,
         r = memory_region_dispatch_write(mr, addr1, val, 2, attrs);
     } else {
         /* RAM case */
-        addr1 += memory_region_get_ram_addr(mr);
         ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2de7898..326c2a2 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1419,7 +1419,6 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
             l = len;
             mr = address_space_translate(as, addr, &addr1, &l, false);
             if (len == l && memory_access_is_direct(mr, false)) {
-                addr1 += memory_region_get_ram_addr(mr);
                 ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
                 memcpy(buf, ptr, len);
             } else {
diff --git a/memory.c b/memory.c
index 49c9b14..bc47d0b 100644
--- a/memory.c
+++ b/memory.c
@@ -1654,10 +1654,10 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
         mr = mr->alias;
     }
     assert(mr->ram_block);
-    ptr = qemu_get_ram_ptr(mr->ram_block, memory_region_get_ram_addr(mr));
+    ptr = qemu_get_ram_ptr(mr->ram_block, offset);
     rcu_read_unlock();
 
-    return ptr + offset;
+    return ptr;
 }
 
 ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index c0a2e99..ae21f97 100644
--- a/scripts/dump-guest-memory.py
+++ b/scripts/dump-guest-memory.py
@@ -328,23 +328,10 @@ def qlist_foreach(head, field_str):
         yield var
 
 
-def qemu_get_ram_block(ram_addr):
-    """Returns the RAMBlock struct to which the given address belongs."""
-
-    ram_blocks = gdb.parse_and_eval("ram_list.blocks")
-
-    for block in qlist_foreach(ram_blocks, "next"):
-        if (ram_addr - block["offset"]) < block["used_length"]:
-            return block
-
-    raise gdb.GdbError("Bad ram offset %x" % ram_addr)
-
-
-def qemu_get_ram_ptr(ram_addr):
+def qemu_get_ram_ptr(block, offset):
     """Returns qemu vaddr for given guest physical address."""
 
-    block = qemu_get_ram_block(ram_addr)
-    return block["host"] + (ram_addr - block["offset"])
+    return block["host"] + offset
 
 
 def memory_region_get_ram_ptr(memory_region):
@@ -352,7 +339,7 @@ def memory_region_get_ram_ptr(memory_region):
         return (memory_region_get_ram_ptr(memory_region["alias"].dereference())
                 + memory_region["alias_offset"])
 
-    return qemu_get_ram_ptr(memory_region["ram_block"]["offset"])
+    return qemu_get_ram_ptr(memory_region["ram_block"], 0)
 
 
 def get_guest_phys_blocks():
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Fam Zheng]

On Thu, 03/24 12:03, Paolo Bonzini wrote:
> Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
> address that is relative to the MemoryRegion.  This basically means
> what address_space_translate returns.
> 
> invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
> not need it at all.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                       | 40 +++++++++++++++-------------------------
>  include/exec/memory.h        |  1 -
>  memory.c                     |  4 ++--
>  scripts/dump-guest-memory.py | 19 +++----------------
>  4 files changed, 20 insertions(+), 44 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 001b669..ca9e3b6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)

Shall we rename the parameter to "offset" then?  I don't know, but that seems
easier to read for me.

>  
>      if (block == NULL) {
>          block = qemu_get_ram_block(addr);
> +        addr -= block->offset;
>      }
>  
>      if (xen_enabled() && block->host == NULL) {
> @@ -1889,7 +1890,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
>  
>          block->host = xen_map_cache(block->offset, block->max_length, 1);
>      }
> -    return ramblock_ptr(block, addr - block->offset);
> +    return ramblock_ptr(block, addr);
>  }
>  
>  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> @@ -1901,16 +1902,15 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>                                   hwaddr *size)
>  {
>      RAMBlock *block = ram_block;
> -    ram_addr_t offset_inside_block;
>      if (*size == 0) {
>          return NULL;
>      }
>  
>      if (block == NULL) {
>          block = qemu_get_ram_block(addr);
> +        addr -= block->offset;
>      }
> -    offset_inside_block = addr - block->offset;
> -    *size = MIN(*size, block->max_length - offset_inside_block);
> +    *size = MIN(*size, block->max_length - addr);
>  
>      if (xen_enabled() && block->host == NULL) {
>          /* We need to check if the requested address is in the RAM
> @@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>          block->host = xen_map_cache(block->offset, block->max_length, 1);
>      }
>  
> -    return ramblock_ptr(block, offset_inside_block);
> +    return ramblock_ptr(block, addr);
>  }
>  
>  /*
> @@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                       hwaddr length)
>  {
>      uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +    addr += memory_region_get_ram_addr(mr);
> +

If called by address_space_unmap, is this addition still correct?

void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                         int is_write, hwaddr access_len)
{
    if (buffer != bounce.buffer) {
        MemoryRegion *mr;
        ram_addr_t addr1;

        mr = qemu_ram_addr_from_host(buffer, &addr1);
        assert(mr != NULL);
        if (is_write) {
            invalidate_and_set_dirty(mr, addr1, access_len);
                                          ^
                                          `-- IIUC this is not an offset into
                                              mr, is it?

>      /* No early return if dirty_log_mask is or becomes 0, because
>       * cpu_physical_memory_set_dirty_range will still call
>       * xen_modified_memory.
> @@ -2616,7 +2618,6 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
>                  abort();
>              }
>          } else {
> -            addr1 += memory_region_get_ram_addr(mr);
>              /* RAM case */
>              ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              memcpy(ptr, buf, l);
> @@ -2709,8 +2710,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
>              }
>          } else {
>              /* RAM case */
> -            ptr = qemu_get_ram_ptr(mr->ram_block,
> -                                   memory_region_get_ram_addr(mr) + addr1);
> +            ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              memcpy(buf, ptr, l);
>          }
>  
> @@ -3382,13 +3374,13 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
>  
>          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
>      } else {
> -        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          stl_p(ptr, val);
>  
>          dirty_log_mask = memory_region_get_dirty_log_mask(mr);
>          dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> -        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
> +        cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr) + addr,

Is this line too long?

> +                                            4, dirty_log_mask);
>          r = MEMTX_OK;
>      }
>      if (result) {

Re: [Qemu-devel] [PATCH 1/2] memory: remove unnecessary masking of MemoryRegion ram_addr

[Fam Zheng]

On Thu, 03/24 12:03, Paolo Bonzini wrote:
> mr->ram_block->offset is already aligned to both host and target size
> (see qemu_ram_alloc_internal).  Remove further masking as it is
> unnecessary.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

----- Original Message -----
> From: "Fam Zheng" <famz@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, "arei gonglei" <arei.gonglei@huawei.com>, mst@redhat.com
> Sent: Friday, March 25, 2016 7:20:38 AM
> Subject: Re: [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users
> 
> On Thu, 03/24 12:03, Paolo Bonzini wrote:
> > Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
> > address that is relative to the MemoryRegion.  This basically means
> > what address_space_translate returns.
> > 
> > invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
> > not need it at all.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  exec.c                       | 40 +++++++++++++++-------------------------
> >  include/exec/memory.h        |  1 -
> >  memory.c                     |  4 ++--
> >  scripts/dump-guest-memory.py | 19 +++----------------
> >  4 files changed, 20 insertions(+), 44 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 001b669..ca9e3b6 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block,
> > ram_addr_t addr)
> 
> Shall we rename the parameter to "offset" then?  I don't know, but that seems
> easier to read for me.

Good question.  I'm not sure about that because of the block == NULL case,
where the address is absolute.

> > @@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block,
> > ram_addr_t addr,
> >          block->host = xen_map_cache(block->offset, block->max_length, 1);
> >      }
> >  
> > -    return ramblock_ptr(block, offset_inside_block);
> > +    return ramblock_ptr(block, addr);
> >  }
> >  
> >  /*
> > @@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion
> > *mr, hwaddr addr,
> >                                       hwaddr length)
> >  {
> >      uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> > +    addr += memory_region_get_ram_addr(mr);
> > +
> 
> If called by address_space_unmap, is this addition still correct?

No, thanks for the careful review!  That's another opportunity
for cleanup actually, splitting the (few) users of qemu_ram_addr_from_host
that really need a ram_addr_t and those (the majority) that need a
MemoryRegion and offset.  They can use two different functions.  I'll
defer this to 2.7 and post the patches to do so later.

> > @@ -3382,13 +3374,13 @@ void address_space_stl_notdirty(AddressSpace *as,
> > hwaddr addr, uint32_t val,
> >  
> >          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
> >      } else {
> > -        addr1 += memory_region_get_ram_addr(mr);
> >          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
> >          stl_p(ptr, val);
> >  
> >          dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> >          dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> > -        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
> > +        cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr)
> > + addr,
> 
> Is this line too long?

It's 82 characters

Paolo

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Fam Zheng]

On Fri, 03/25 07:58, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Fam Zheng" <famz@redhat.com>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>
> > Cc: qemu-devel@nongnu.org, "arei gonglei" <arei.gonglei@huawei.com>, mst@redhat.com
> > Sent: Friday, March 25, 2016 7:20:38 AM
> > Subject: Re: [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users
> > 
> > On Thu, 03/24 12:03, Paolo Bonzini wrote:
> > > Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
> > > address that is relative to the MemoryRegion.  This basically means
> > > what address_space_translate returns.
> > > 
> > > invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
> > > not need it at all.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  exec.c                       | 40 +++++++++++++++-------------------------
> > >  include/exec/memory.h        |  1 -
> > >  memory.c                     |  4 ++--
> > >  scripts/dump-guest-memory.py | 19 +++----------------
> > >  4 files changed, 20 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/exec.c b/exec.c
> > > index 001b669..ca9e3b6 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block,
> > > ram_addr_t addr)
> > 
> > Shall we rename the parameter to "offset" then?  I don't know, but that seems
> > easier to read for me.
> 
> Good question.  I'm not sure about that because of the block == NULL case,
> where the address is absolute.
> 
> > > @@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block,
> > > ram_addr_t addr,
> > >          block->host = xen_map_cache(block->offset, block->max_length, 1);
> > >      }
> > >  
> > > -    return ramblock_ptr(block, offset_inside_block);
> > > +    return ramblock_ptr(block, addr);
> > >  }
> > >  
> > >  /*
> > > @@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion
> > > *mr, hwaddr addr,
> > >                                       hwaddr length)
> > >  {
> > >      uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> > > +    addr += memory_region_get_ram_addr(mr);
> > > +
> > 
> > If called by address_space_unmap, is this addition still correct?
> 
> No, thanks for the careful review!  That's another opportunity
> for cleanup actually, splitting the (few) users of qemu_ram_addr_from_host
> that really need a ram_addr_t and those (the majority) that need a
> MemoryRegion and offset.  They can use two different functions.  I'll
> defer this to 2.7 and post the patches to do so later.

Good idea. The above "block == NULL" qemu_get_ram_ptr callers could use a
separate function, too - frankly I don't like that function interface too much.
What do you think?

Fam

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

> > > If called by address_space_unmap, is this addition still correct?
> > 
> > No, thanks for the careful review!  That's another opportunity
> > for cleanup actually, splitting the (few) users of qemu_ram_addr_from_host
> > that really need a ram_addr_t and those (the majority) that need a
> > MemoryRegion and offset.  They can use two different functions.  I'll
> > defer this to 2.7 and post the patches to do so later.
> 
> Good idea. The above "block == NULL" qemu_get_ram_ptr callers could use a
> separate function, too - frankly I don't like that function interface too
> much.
> What do you think?

I don't know, at least block == NULL has a clear meaning.  It's not entirely
satisfying, but the users are readable and the ones that pass NULL stand out.

In the case of qemu_ram_addr_from_host, on the other hand, there's a clear
opportunity to avoid bugs.

Paolo

Re: [Qemu-devel] [PATCH 1/2] memory: remove unnecessary masking of MemoryRegion ram_addr

[Michael S. Tsirkin]

On Thu, Mar 24, 2016 at 12:03:34PM +0100, Paolo Bonzini wrote:
> mr->ram_block->offset is already aligned to both host and target size
> (see qemu_ram_alloc_internal).  Remove further masking as it is
> unnecessary.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  exec.c             | 21 +++++++--------------
>  memory.c           |  5 ++---
>  migration/savevm.c |  4 ++--
>  translate-all.c    |  3 +--
>  4 files changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index f398d21..001b669 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1042,8 +1042,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>  
>      if (memory_region_is_ram(section->mr)) {
>          /* Normal RAM.  */
> -        iotlb = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
> -            + xlat;
> +        iotlb = memory_region_get_ram_addr(section->mr) + xlat;
>          if (!section->readonly) {
>              iotlb |= PHYS_SECTION_NOTDIRTY;
>          } else {
> @@ -3093,9 +3092,7 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr,
>      } else {
>          /* RAM case */
>          ptr = qemu_get_ram_ptr(mr->ram_block,
> -                               (memory_region_get_ram_addr(mr)
> -                                & TARGET_PAGE_MASK)
> -                               + addr1);
> +                               memory_region_get_ram_addr(mr) + addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = ldl_le_p(ptr);
> @@ -3189,9 +3186,7 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr,
>      } else {
>          /* RAM case */
>          ptr = qemu_get_ram_ptr(mr->ram_block,
> -                               (memory_region_get_ram_addr(mr)
> -                                & TARGET_PAGE_MASK)
> -                               + addr1);
> +                               memory_region_get_ram_addr(mr) + addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = ldq_le_p(ptr);
> @@ -3305,9 +3300,7 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as,
>      } else {
>          /* RAM case */
>          ptr = qemu_get_ram_ptr(mr->ram_block,
> -                               (memory_region_get_ram_addr(mr)
> -                                & TARGET_PAGE_MASK)
> -                               + addr1);
> +                               memory_region_get_ram_addr(mr) + addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = lduw_le_p(ptr);
> @@ -3389,7 +3382,7 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
>  
>          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
>      } else {
> -        addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> +        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          stl_p(ptr, val);
>  
> @@ -3444,7 +3437,7 @@ static inline void address_space_stl_internal(AddressSpace *as,
>          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
>      } else {
>          /* RAM case */
> -        addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> +        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> @@ -3554,7 +3547,7 @@ static inline void address_space_stw_internal(AddressSpace *as,
>          r = memory_region_dispatch_write(mr, addr1, val, 2, attrs);
>      } else {
>          /* RAM case */
> -        addr1 += memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK;
> +        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> diff --git a/memory.c b/memory.c
> index 95f7209..49c9b14 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1640,7 +1640,7 @@ int memory_region_get_fd(MemoryRegion *mr)
>  
>      assert(mr->ram_block);
>  
> -    return qemu_get_ram_fd(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
> +    return qemu_get_ram_fd(memory_region_get_ram_addr(mr));
>  }
>  
>  void *memory_region_get_ram_ptr(MemoryRegion *mr)
> @@ -1654,8 +1654,7 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>          mr = mr->alias;
>      }
>      assert(mr->ram_block);
> -    ptr = qemu_get_ram_ptr(mr->ram_block,
> -                           memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
> +    ptr = qemu_get_ram_ptr(mr->ram_block, memory_region_get_ram_addr(mr));
>      rcu_read_unlock();
>  
>      return ptr + offset;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0a33c22..cbba062 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2229,13 +2229,13 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>  
>  void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
>  {
> -    qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
> +    qemu_ram_set_idstr(memory_region_get_ram_addr(mr),
>                         memory_region_name(mr), dev);
>  }
>  
>  void vmstate_unregister_ram(MemoryRegion *mr, DeviceState *dev)
>  {
> -    qemu_ram_unset_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK);
> +    qemu_ram_unset_idstr(memory_region_get_ram_addr(mr));
>  }
>  
>  void vmstate_register_ram_global(MemoryRegion *mr)
> diff --git a/translate-all.c b/translate-all.c
> index e9f409b..6820d8e 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1550,8 +1550,7 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr)
>          rcu_read_unlock();
>          return;
>      }
> -    ram_addr = (memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK)
> -        + addr;
> +    ram_addr = memory_region_get_ram_addr(mr) + addr;
>      tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
>      rcu_read_unlock();
>  }
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Michael S. Tsirkin]

On Thu, Mar 24, 2016 at 12:03:35PM +0100, Paolo Bonzini wrote:
> Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
> address that is relative to the MemoryRegion.  This basically means
> what address_space_translate returns.
> 
> invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
> not need it at all.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


I agree but I think we need a better name for this function.
qemu_ram_offset_to_ptr?
Will also serve to make sure backporting patches across this
API change does not cause issues.


> ---
>  exec.c                       | 40 +++++++++++++++-------------------------
>  include/exec/memory.h        |  1 -
>  memory.c                     |  4 ++--
>  scripts/dump-guest-memory.py | 19 +++----------------
>  4 files changed, 20 insertions(+), 44 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 001b669..ca9e3b6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
>  
>      if (block == NULL) {
>          block = qemu_get_ram_block(addr);
> +        addr -= block->offset;
>      }
>  
>      if (xen_enabled() && block->host == NULL) {
> @@ -1889,7 +1890,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
>  
>          block->host = xen_map_cache(block->offset, block->max_length, 1);
>      }
> -    return ramblock_ptr(block, addr - block->offset);
> +    return ramblock_ptr(block, addr);
>  }
>  
>  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> @@ -1901,16 +1902,15 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>                                   hwaddr *size)
>  {
>      RAMBlock *block = ram_block;
> -    ram_addr_t offset_inside_block;
>      if (*size == 0) {
>          return NULL;
>      }
>  
>      if (block == NULL) {
>          block = qemu_get_ram_block(addr);
> +        addr -= block->offset;
>      }
> -    offset_inside_block = addr - block->offset;
> -    *size = MIN(*size, block->max_length - offset_inside_block);
> +    *size = MIN(*size, block->max_length - addr);
>  
>      if (xen_enabled() && block->host == NULL) {
>          /* We need to check if the requested address is in the RAM
> @@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>          block->host = xen_map_cache(block->offset, block->max_length, 1);
>      }
>  
> -    return ramblock_ptr(block, offset_inside_block);
> +    return ramblock_ptr(block, addr);
>  }
>  
>  /*
> @@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                       hwaddr length)
>  {
>      uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +    addr += memory_region_get_ram_addr(mr);
> +
>      /* No early return if dirty_log_mask is or becomes 0, because
>       * cpu_physical_memory_set_dirty_range will still call
>       * xen_modified_memory.
> @@ -2616,7 +2618,6 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
>                  abort();
>              }
>          } else {
> -            addr1 += memory_region_get_ram_addr(mr);
>              /* RAM case */
>              ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              memcpy(ptr, buf, l);
> @@ -2709,8 +2710,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
>              }
>          } else {
>              /* RAM case */
> -            ptr = qemu_get_ram_ptr(mr->ram_block,
> -                                   memory_region_get_ram_addr(mr) + addr1);
> +            ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              memcpy(buf, ptr, l);
>          }
>  
> @@ -2793,7 +2793,6 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
>                memory_region_is_romd(mr))) {
>              l = memory_access_size(mr, l, addr1);
>          } else {
> -            addr1 += memory_region_get_ram_addr(mr);
>              /* ROM/RAM case */
>              ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              switch (type) {
> @@ -2953,7 +2952,6 @@ void *address_space_map(AddressSpace *as,
>      hwaddr done = 0;
>      hwaddr l, xlat, base;
>      MemoryRegion *mr, *this_mr;
> -    ram_addr_t raddr;
>      void *ptr;
>  
>      if (len == 0) {
> @@ -2962,7 +2960,7 @@ void *address_space_map(AddressSpace *as,
>  
>      l = len;
>      rcu_read_lock();
> -    mr = address_space_translate(as, addr, &xlat, &l, is_write);
> +    mr = address_space_translate(as, addr, &base, &l, is_write);
>  
>      if (!memory_access_is_direct(mr, is_write)) {
>          if (atomic_xchg(&bounce.in_use, true)) {
> @@ -2987,9 +2985,6 @@ void *address_space_map(AddressSpace *as,
>          return bounce.buffer;
>      }
>  
> -    base = xlat;
> -    raddr = memory_region_get_ram_addr(mr);
> -
>      for (;;) {
>          len -= l;
>          addr += l;
> @@ -3007,7 +3002,7 @@ void *address_space_map(AddressSpace *as,
>  
>      memory_region_ref(mr);
>      *plen = done;
> -    ptr = qemu_ram_ptr_length(mr->ram_block, raddr + base, plen);
> +    ptr = qemu_ram_ptr_length(mr->ram_block, base, plen);
>      rcu_read_unlock();
>  
>      return ptr;
> @@ -3091,8 +3086,7 @@ static inline uint32_t address_space_ldl_internal(AddressSpace *as, hwaddr addr,
>  #endif
>      } else {
>          /* RAM case */
> -        ptr = qemu_get_ram_ptr(mr->ram_block,
> -                               memory_region_get_ram_addr(mr) + addr1);
> +        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = ldl_le_p(ptr);
> @@ -3185,8 +3179,7 @@ static inline uint64_t address_space_ldq_internal(AddressSpace *as, hwaddr addr,
>  #endif
>      } else {
>          /* RAM case */
> -        ptr = qemu_get_ram_ptr(mr->ram_block,
> -                               memory_region_get_ram_addr(mr) + addr1);
> +        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = ldq_le_p(ptr);
> @@ -3299,8 +3292,7 @@ static inline uint32_t address_space_lduw_internal(AddressSpace *as,
>  #endif
>      } else {
>          /* RAM case */
> -        ptr = qemu_get_ram_ptr(mr->ram_block,
> -                               memory_region_get_ram_addr(mr) + addr1);
> +        ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = lduw_le_p(ptr);
> @@ -3382,13 +3374,13 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
>  
>          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
>      } else {
> -        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          stl_p(ptr, val);
>  
>          dirty_log_mask = memory_region_get_dirty_log_mask(mr);
>          dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> -        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
> +        cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr) + addr,
> +                                            4, dirty_log_mask);
>          r = MEMTX_OK;
>      }
>      if (result) {
> @@ -3437,7 +3429,6 @@ static inline void address_space_stl_internal(AddressSpace *as,
>          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
>      } else {
>          /* RAM case */
> -        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> @@ -3547,7 +3538,6 @@ static inline void address_space_stw_internal(AddressSpace *as,
>          r = memory_region_dispatch_write(mr, addr1, val, 2, attrs);
>      } else {
>          /* RAM case */
> -        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2de7898..326c2a2 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1419,7 +1419,6 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
>              l = len;
>              mr = address_space_translate(as, addr, &addr1, &l, false);
>              if (len == l && memory_access_is_direct(mr, false)) {
> -                addr1 += memory_region_get_ram_addr(mr);
>                  ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>                  memcpy(buf, ptr, len);
>              } else {
> diff --git a/memory.c b/memory.c
> index 49c9b14..bc47d0b 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1654,10 +1654,10 @@ void *memory_region_get_ram_ptr(MemoryRegion *mr)
>          mr = mr->alias;
>      }
>      assert(mr->ram_block);
> -    ptr = qemu_get_ram_ptr(mr->ram_block, memory_region_get_ram_addr(mr));
> +    ptr = qemu_get_ram_ptr(mr->ram_block, offset);
>      rcu_read_unlock();
>  
> -    return ptr + offset;
> +    return ptr;
>  }
>  
>  ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index c0a2e99..ae21f97 100644
> --- a/scripts/dump-guest-memory.py
> +++ b/scripts/dump-guest-memory.py
> @@ -328,23 +328,10 @@ def qlist_foreach(head, field_str):
>          yield var
>  
>  
> -def qemu_get_ram_block(ram_addr):
> -    """Returns the RAMBlock struct to which the given address belongs."""
> -
> -    ram_blocks = gdb.parse_and_eval("ram_list.blocks")
> -
> -    for block in qlist_foreach(ram_blocks, "next"):
> -        if (ram_addr - block["offset"]) < block["used_length"]:
> -            return block
> -
> -    raise gdb.GdbError("Bad ram offset %x" % ram_addr)
> -
> -
> -def qemu_get_ram_ptr(ram_addr):
> +def qemu_get_ram_ptr(block, offset):
>      """Returns qemu vaddr for given guest physical address."""
>  
> -    block = qemu_get_ram_block(ram_addr)
> -    return block["host"] + (ram_addr - block["offset"])
> +    return block["host"] + offset
>  
>  
>  def memory_region_get_ram_ptr(memory_region):
> @@ -352,7 +339,7 @@ def memory_region_get_ram_ptr(memory_region):
>          return (memory_region_get_ram_ptr(memory_region["alias"].dereference())
>                  + memory_region["alias_offset"])
>  
> -    return qemu_get_ram_ptr(memory_region["ram_block"]["offset"])
> +    return qemu_get_ram_ptr(memory_region["ram_block"], 0)
>  
>  
>  def get_guest_phys_blocks():
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

On 03/04/2016 15:49, Michael S. Tsirkin wrote:
> 
> I agree but I think we need a better name for this function.
> qemu_ram_offset_to_ptr?
> Will also serve to make sure backporting patches across this
> API change does not cause issues.

Yes, this makes sense.  If it were all in 2.6, there would be no
released version with an absolute ram_addr_t argument *and* a RAMBlock*
argument, but we will do the incompatible change in 2.7 and then it
makes sense to rename it.

Paolo

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

On 03/04/2016 15:49, Michael S. Tsirkin wrote:
> I agree but I think we need a better name for this function.
> qemu_ram_offset_to_ptr?

What about qemu_map_ram_ptr?

Paolo

> Will also serve to make sure backporting patches across this
> API change does not cause issues.

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Michael S. Tsirkin]

On Mon, Apr 04, 2016 at 10:03:12AM +0200, Paolo Bonzini wrote:
> 
> 
> On 03/04/2016 15:49, Michael S. Tsirkin wrote:
> > I agree but I think we need a better name for this function.
> > qemu_ram_offset_to_ptr?
> 
> What about qemu_map_ram_ptr?
> 
> Paolo

OK but this seems to imply there's also an unmap operation?

> > Will also serve to make sure backporting patches across this
> > API change does not cause issues.

Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Paolo Bonzini]

On 04/04/2016 10:38, Michael S. Tsirkin wrote:
> > > I agree but I think we need a better name for this function.
> > > qemu_ram_offset_to_ptr?
> > 
> > What about qemu_map_ram_ptr?
> > 
> > Paolo
> 
> OK but this seems to imply there's also an unmap operation?

Actually there is one, even though it's almost always a nop---it's
xen_invalidate_map_cache_entry.

Paolo