Patchwork [PATCHv2,6/6] dump: fix memory region handling

login
register
mail settings
Submitter Rabin Vincent
Date March 24, 2013, 5:27 p.m.
Message ID <1364146041-27041-7-git-send-email-rabin@rab.in>
Download mbox | patch
Permalink /patch/230471/
State New
Headers show

Comments

Rabin Vincent - March 24, 2013, 5:27 p.m.
RAMBlock.offset does not provide the physical address of the memory
region.  This is available in the MemoryRegion's address.  The wrong
usage leads to incorrect physical addreses in the ELF.  Fix it.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 dump.c                |   19 +++++++++++--------
 include/exec/memory.h |    7 +++++++
 memory.c              |   12 ++++++++++++
 memory_mapping.c      |    6 ++++--
 4 files changed, 34 insertions(+), 10 deletions(-)
Peter Maydell - March 24, 2013, 6:36 p.m.
On 24 March 2013 17:27, Rabin Vincent <rabin@rab.in> wrote:
>  /**
> + * memory_region_get_addr: Get the address of a memory region
> + *
> + * @mr: the memory region
> + */
> +hwaddr memory_region_get_addr(MemoryRegion *mr);

I'm afraid this doesn't make sense. A MemoryRegion by itself has
no "address" -- it could be mapped into several places in several
different address maps or none at all.

-- PMM
Rabin Vincent - March 24, 2013, 7:35 p.m.
2013/3/24 Peter Maydell <peter.maydell@linaro.org>:
> On 24 March 2013 17:27, Rabin Vincent <rabin@rab.in> wrote:
>>  /**
>> + * memory_region_get_addr: Get the address of a memory region
>> + *
>> + * @mr: the memory region
>> + */
>> +hwaddr memory_region_get_addr(MemoryRegion *mr);
>
> I'm afraid this doesn't make sense. A MemoryRegion by itself has
> no "address" -- it could be mapped into several places in several
> different address maps or none at all.

OK.  Do you mean that such a function can be used internally to the dump
code where it gets the MemoryRegion only from the RAMBlock.mr or do you
mean the dump code also shouldn't be doing it that way?

If you mean the latter, could you please suggest an alternative way to
handle this?  The problem is that the dump code assumes that
RAMBlock.offset provides the physical address, and this appears to not
be the case.  For example, with a dump generated from vexpress I get
these Program Headers in the dump:

  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  NOTE           0x0000f4 0x00000000 0x00000000 0x002a0 0x002a0     0
  LOAD           0x000394 0x00000000 0x00000000 0x8000000 0x8000000     0
  LOAD           0x8000394 0x00000000 0x08000000 0x4000000 0x4000000     0
  LOAD           0xc000394 0x00000000 0x0c000000 0x4000000 0x4000000     0
  LOAD           0x10000394 0x00000000 0x10000000 0x2000000 0x2000000     0
  LOAD           0x12000394 0x00000000 0x12000000 0x800000 0x800000     0

The physical addresses are completely wrong, and with the patch I get
the right ones:

  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  NOTE           0x0000f4 0x00000000 0x00000000 0x002a0 0x002a0     0
  LOAD           0x8000394 0x00000000 0x40000000 0x4000000 0x4000000     0
  LOAD           0xc000394 0x00000000 0x44000000 0x4000000 0x4000000     0
  LOAD           0x10000394 0x00000000 0x48000000 0x2000000 0x2000000     0
  LOAD           0x12000394 0x00000000 0x4c000000 0x800000 0x800000     0
  LOAD           0x000394 0x00000000 0x60000000 0x8000000 0x8000000     0

Thanks.
Peter Maydell - March 24, 2013, 8:18 p.m.
On 24 March 2013 19:35, Rabin Vincent <rabin@rab.in> wrote:
> 2013/3/24 Peter Maydell <peter.maydell@linaro.org>:
>> On 24 March 2013 17:27, Rabin Vincent <rabin@rab.in> wrote:
>>>  /**
>>> + * memory_region_get_addr: Get the address of a memory region
>>> + *
>>> + * @mr: the memory region
>>> + */
>>> +hwaddr memory_region_get_addr(MemoryRegion *mr);
>>
>> I'm afraid this doesn't make sense. A MemoryRegion by itself has
>> no "address" -- it could be mapped into several places in several
>> different address maps or none at all.
>
> OK.  Do you mean that such a function can be used internally to the dump
> code where it gets the MemoryRegion only from the RAMBlock.mr or do you
> mean the dump code also shouldn't be doing it that way?

I mean that the dump code is wrong if it's trying to ask "what
is the address of this MemoryRegion (or RAMBlock)" at all. That
question doesn't have a well defined single answer.

> If you mean the latter, could you please suggest an alternative way to
> handle this?  The problem is that the dump code assumes that
> RAMBlock.offset provides the physical address, and this appears to not
> be the case.

That is also wrong, both for the reason you state and also because
ramblocks can be aliased into multiple physical addresses too.
I suspect you need to change anything that's trying to iterate
through memory with "for each block in ram_list" to instead actually
iterate through the system address space and say "if this is RAM
then...". For instance with the vexpress-a9 you presumably want
a LOAD section for both physaddr 0 (the alias) and physaddr
0x60000000 (the usual address).

-- PMM

Patch

diff --git a/dump.c b/dump.c
index 4b7d76c..4b0353a 100644
--- a/dump.c
+++ b/dump.c
@@ -16,6 +16,7 @@ 
 #include "cpu.h"
 #include "exec/cpu-all.h"
 #include "exec/hwaddr.h"
+#include "exec/memory.h"
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"
 #include "sysemu/dump.h"
@@ -432,26 +433,28 @@  static hwaddr get_offset(hwaddr phys_addr,
     }
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
+        hwaddr baddr = memory_region_get_addr(block->mr);
+
         if (s->has_filter) {
-            if (block->offset >= s->begin + s->length ||
-                block->offset + block->length <= s->begin) {
+            if (baddr >= s->begin + s->length ||
+                baddr + block->length <= s->begin) {
                 /* This block is out of the range */
                 continue;
             }
 
-            if (s->begin <= block->offset) {
-                start = block->offset;
+            if (s->begin <= baddr) {
+                start = baddr;
             } else {
                 start = s->begin;
             }
 
-            size_in_block = block->length - (start - block->offset);
-            if (s->begin + s->length < block->offset + block->length) {
-                size_in_block -= block->offset + block->length -
+            size_in_block = block->length - (start - baddr);
+            if (s->begin + s->length < baddr + block->length) {
+                size_in_block -= baddr + block->length -
                                  (s->begin + s->length);
             }
         } else {
-            start = block->offset;
+            start = baddr;
             size_in_block = block->length;
         }
 
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2322732..9227190 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -665,6 +665,13 @@  void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          unsigned priority);
 
 /**
+ * memory_region_get_addr: Get the address of a memory region
+ *
+ * @mr: the memory region
+ */
+hwaddr memory_region_get_addr(MemoryRegion *mr);
+
+/**
  * memory_region_get_ram_addr: Get the ram address associated with a memory
  *                             region
  *
diff --git a/memory.c b/memory.c
index 92a2196..f90fd19 100644
--- a/memory.c
+++ b/memory.c
@@ -1427,6 +1427,18 @@  void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset)
     memory_region_transaction_commit();
 }
 
+hwaddr memory_region_get_addr(MemoryRegion *mr)
+{
+    hwaddr addr = 0;
+
+    while (mr) {
+	    addr += mr->addr;
+	    mr = mr->parent;
+    }
+
+    return addr;
+}
+
 ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
 {
     return mr->ram_addr;
diff --git a/memory_mapping.c b/memory_mapping.c
index ff45b3a..cf0751c 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -13,6 +13,7 @@ 
 
 #include "cpu.h"
 #include "exec/cpu-all.h"
+#include "exec/memory.h"
 #include "sysemu/memory_mapping.h"
 
 static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list,
@@ -201,7 +202,7 @@  int qemu_get_guest_memory_mapping(MemoryMappingList *list)
      * address.
      */
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        offset = block->offset;
+        offset = memory_region_get_addr(block->mr);
         length = block->length;
         create_new_memory_mapping(list, offset, offset, length);
     }
@@ -214,7 +215,8 @@  void qemu_get_guest_simple_memory_mapping(MemoryMappingList *list)
     RAMBlock *block;
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-        create_new_memory_mapping(list, block->offset, 0, block->length);
+        create_new_memory_mapping(list, memory_region_get_addr(block->mr),
+                                  0, block->length);
     }
 }