| Message ID | 20181225125344.4482-1-arilou@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] dump: Set correct vaddr for ELF dump | expand |
Hi On Tue, Dec 25, 2018 at 5:52 PM Jon Doron <arilou@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 <arilou@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 > >
On 01/07/19 13:14, Marc-André Lureau wrote: > Hi > > On Tue, Dec 25, 2018 at 5:52 PM Jon Doron <arilou@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 <arilou@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
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 Thanks, -- Jon. On Mon, Jan 7, 2019 at 8:04 PM Laszlo Ersek <lersek@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 <arilou@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 <arilou@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
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 <lersek@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 <arilou@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 <arilou@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
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; 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 phdr.p_filesz = p_size phdr.p_memsz = p_size self.segments.append(phdr)
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 <arilou@gmail.com> --- dump.c | 4 ++-- scripts/dump-guest-memory.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)