[Qemu-devel] [PATCH v1] dump: Set correct vaddr for ELF dump

[Qemu-devel] [PATCH v1] dump: Set correct vaddr for ELF dump

[Jon Doron]

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(-)

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)
-- 
2.19.2

Re: [Qemu-devel] [PATCH v1] dump: Set correct vaddr for ELF dump

[Marc-André Lureau]

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
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v1] dump: Set correct vaddr for ELF dump

[Laszlo Ersek]

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

Re: [Qemu-devel] [PATCH v1] dump: Set correct vaddr for ELF dump

[Jon Doron]

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

Re: [Qemu-devel] [PATCH v1] dump: Set correct vaddr for ELF dump

[Laszlo Ersek]

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