On 01/08/19 07:31, Jon Doron wrote: > Thank you for looking into this, perhaps I could change the patch (at > least in the C part not the python script) to something like: > - phdr.p_vaddr = cpu_to_dumpXX(s, memory_mapping->virt_addr); > + phdr.p_vaddr = cpu_to_dumpXX(s, memory_mapping->virt_addr) ? > cpu_to_dumpXX(s, memory_mapping->virt_addr) : phdr.p_paddr; > > So in the case of paging where virt_addr is available we will use it
I guess that would be an improvement, yes. Regarding style: although I'm personally opposed to most GNUisms, QEMU liberally uses, for example, the GNU extension: https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html And, the "?:" operator would apply here. Thanks, Laszlo > On Mon, Jan 7, 2019 at 8:04 PM Laszlo Ersek <ler...@redhat.com> wrote: >> >> On 01/07/19 13:14, Marc-André Lureau wrote: >>> Hi >>> >>> On Tue, Dec 25, 2018 at 5:52 PM Jon Doron <ari...@gmail.com> wrote: >>>> >>>> vaddr needs to be equal to the paddr since the dump file represents the >>>> physical memory image. >>>> >>>> Without setting vaddr correctly, GDB would load all the different memory >>>> regions on top of each other to vaddr 0, thus making GDB showing the wrong >>>> memory data for a given address. >>>> >>>> Signed-off-by: Jon Doron <ari...@gmail.com> >>> >>> This is a non-trivial patch! (qemu-trivial, please ignore). >>> >>>> --- >>>> dump.c | 4 ++-- >>>> scripts/dump-guest-memory.py | 1 + >>>> 2 files changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/dump.c b/dump.c >>>> index 4ec94c5e25..bf77a119ea 100644 >>>> --- a/dump.c >>>> +++ b/dump.c >>>> @@ -192,7 +192,7 @@ static void write_elf64_load(DumpState *s, >>>> MemoryMapping *memory_mapping, >>>> phdr.p_paddr = cpu_to_dump64(s, memory_mapping->phys_addr); >>>> phdr.p_filesz = cpu_to_dump64(s, filesz); >>>> phdr.p_memsz = cpu_to_dump64(s, memory_mapping->length); >>>> - phdr.p_vaddr = cpu_to_dump64(s, memory_mapping->virt_addr); >>>> + phdr.p_vaddr = phdr.p_paddr; >>> >>> This is likely breaking paging=true somehow, which sets >>> memory_mapping->virt_addr to non-0. >>> >>> According to doc "If you want to use gdb to process the core, please >>> set @paging to true." >>> >>> Although I am not able to (gdb) x/10bx 0xa0000 for example on a core >>> produced with paging. Not sure why, anybody could help? >>> >>>> assert(memory_mapping->length >= filesz); >>>> >>>> @@ -216,7 +216,7 @@ static void write_elf32_load(DumpState *s, >>>> MemoryMapping *memory_mapping, >>>> phdr.p_paddr = cpu_to_dump32(s, memory_mapping->phys_addr); >>>> phdr.p_filesz = cpu_to_dump32(s, filesz); >>>> phdr.p_memsz = cpu_to_dump32(s, memory_mapping->length); >>>> - phdr.p_vaddr = cpu_to_dump32(s, memory_mapping->virt_addr); >>>> + phdr.p_vaddr = phdr.p_paddr; >>>> >>>> assert(memory_mapping->length >= filesz); >>>> >>>> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py >>>> index 198cd0fe40..2c587cbefc 100644 >>>> --- a/scripts/dump-guest-memory.py >>>> +++ b/scripts/dump-guest-memory.py >>>> @@ -163,6 +163,7 @@ class ELF(object): >>>> phdr = get_arch_phdr(self.endianness, self.elfclass) >>>> phdr.p_type = p_type >>>> phdr.p_paddr = p_paddr >>>> + phdr.p_vaddr = p_paddr >>> >>> With your proposed change though, I can dump memory with gdb... >>> >>>> phdr.p_filesz = p_size >>>> phdr.p_memsz = p_size >>>> self.segments.append(phdr) >>>> -- >>>> 2.19.2 >>>> >>>> >>> >>> >> >> I've never used paging-enabled dumps. First, because doing so requires >> QEMU to trust guest memory contents (see original commit 783e9b4826b9; >> or more recently/generally, the @dump-guest-memory docs in >> "qapi/misc.json"). Second, because whenever I had to deal with guest >> memory dumps, I always used "crash" (which needs no paging), and the >> subject guests were all Linux. >> >> I can't comment on paging-enabled patches for dump, except that they >> shouldn't regress the paging-disabled functionality. :) If the patches >> satisfy that, I'm fine. >> >> (I *am* surprised that GDB insists on p_vaddr equaling p_paddr; after >> all, in the guest, the virtual address is "memory_mapping->virt_addr". >> But, I would never claim to understand most of the ELF intricacies, >> and/or what GDB requires on top of those.) >> >> Thanks >> Laszlo
