| Message ID | 1364146041-27041-7-git-send-email-rabin@rab.in |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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); } }
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(-)