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 <address@hidden> wrote:
>>
>> On 01/07/19 13:14, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Dec 25, 2018 at 5:52 PM Jon Doron <address@hidden> 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 <address@hidden>
>>>
>>> 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