Re: [Qemu-devel] [PATCH 6/7] scripts/dump-guest-memory.py: add vmcoreinfo

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Wed, Jul 5, 2017 at 2:22 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 06/29/17 15:23, Marc-André Lureau wrote:
>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>>
>> To help the python script, add a little global vmcoreinfo_gdb
>> structure, that is populated with vmcoreinfo_gdb_update().
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  scripts/dump-guest-memory.py | 32 ++++++++++++++++++++++++++++++++
>>  include/hw/acpi/vmcoreinfo.h |  1 +
>>  hw/acpi/vmcoreinfo.c         | 16 ++++++++++++++++
>>  3 files changed, 49 insertions(+)
>>
>> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
>> index f7c6635f15..16c3d7cb10 100644
>> --- a/scripts/dump-guest-memory.py
>> +++ b/scripts/dump-guest-memory.py
>> @@ -120,6 +120,20 @@ class ELF(object):
>>          self.segments[0].p_filesz += ctypes.sizeof(note)
>>          self.segments[0].p_memsz += ctypes.sizeof(note)
>>
>> +
>> +    def add_vmcoreinfo_note(self, vmcoreinfo):
>> +        """Adds a vmcoreinfo note to the ELF dump."""
>> +        chead = type(get_arch_note(self.endianness, 0, 0))
>> +        header = chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
>> +        note = get_arch_note(self.endianness,
>> +                             header.n_namesz - 1, header.n_descsz)
>> +        ctypes.memmove(ctypes.pointer(note), vmcoreinfo, ctypes.sizeof(note))
>> +        header_size = ctypes.sizeof(note) - header.n_descsz
>> +
>> +        self.notes.append(note)
>> +        self.segments[0].p_filesz += ctypes.sizeof(note)
>> +        self.segments[0].p_memsz += ctypes.sizeof(note)
>> +
>>      def add_segment(self, p_type, p_paddr, p_size):
>>          """Adds a segment to the elf."""
>>
>> @@ -505,6 +519,23 @@ shape and this command should mostly work."""
>>                  cur += chunk_size
>>                  left -= chunk_size
>>
>> +    def add_vmcoreinfo(self):
>> +        qemu_core = gdb.inferiors()[0]
>> +
>> +        gdb.execute("call vmcoreinfo_gdb_update()")
>
> I think it's a bad idea to call a function from a process that's just
> crashed.

Yeah, if qemu crashed you can't use that script. But we are talking
about dump of guest kernel, so qemu didn't crash :)

>
> If this feature is so important, maybe we can simply set a global
> pointer variable at the end of vmcoreinfo_realize(); something like:
>
> static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
> {
>     static VmcoreinfoState * volatile vmcoreinfo_gdb_helper;
>     [...]
>     vmcoreinfo_gdb_helper = VMCOREINFO(dev);
> }
>
> - vmcoreinfo_gdb_helper has function scope, so no other code can abuse
>   it
> - it has static storage duration so gdb can access it at any time
> - the pointer (not the pointed-to object) is qualified volatile, so gcc
>   cannot optimize out the pointer assignment (which it might be tempted
>   to do otherwise, due to the pointer never being read within QEMU)
>
> Then you can use "vmcoreinfo_gdb_helper->vmcoreinfo_addr_le" to
> implement all the logic in "dump-guest-memory.py".

If necessary, I can try that.

>
> Just my two cents, of course.
>
> Thanks
> Laszlo
>
>> +        avail = gdb.parse_and_eval("vmcoreinfo_gdb.available")
>> +        if not avail:
>> +            return;
>> +
>> +        addr = gdb.parse_and_eval("vmcoreinfo_gdb.paddr")
>> +        size = gdb.parse_and_eval("vmcoreinfo_gdb.size")
>> +        for block in self.guest_phys_blocks:
>> +            if block["target_start"] <= addr < block["target_end"]:
>> +                haddr = block["host_addr"] + (addr - block["target_start"])
>> +                vmcoreinfo = qemu_core.read_memory(haddr, size)
>> +                self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
>> +                return
>> +
>>      def invoke(self, args, from_tty):
>>          """Handles command invocation from gdb."""
>>
>> @@ -518,6 +549,7 @@ shape and this command should mostly work."""
>>
>>          self.elf = ELF(argv[1])
>>          self.guest_phys_blocks = get_guest_phys_blocks()
>> +        self.add_vmcoreinfo()
>>
>>          with open(argv[0], "wb") as vmcore:
>>              self.dump_init(vmcore)
>> diff --git a/include/hw/acpi/vmcoreinfo.h b/include/hw/acpi/vmcoreinfo.h
>> index 40fe99c3ed..4efa678237 100644
>> --- a/include/hw/acpi/vmcoreinfo.h
>> +++ b/include/hw/acpi/vmcoreinfo.h
>> @@ -32,5 +32,6 @@ void vmcoreinfo_build_acpi(VmcoreinfoState *vis, GArray *table_data,
>>  void vmcoreinfo_add_fw_cfg(VmcoreinfoState *vis, FWCfgState *s, GArray *vmci);
>>  bool vmcoreinfo_get(VmcoreinfoState *vis, uint64_t *paddr, uint64_t *size,
>>                      Error **errp);
>> +void vmcoreinfo_gdb_update(void);
>>
>>  #endif
>> diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
>> index 216e0bb83a..75e3330813 100644
>> --- a/hw/acpi/vmcoreinfo.c
>> +++ b/hw/acpi/vmcoreinfo.c
>> @@ -145,6 +145,22 @@ bool vmcoreinfo_get(VmcoreinfoState *vis,
>>      return true;
>>  }
>>
>> +struct vmcoreinfo_gdb {
>> +    bool available;
>> +    uint64_t paddr;
>> +    uint64_t size;
>> +} vmcoreinfo_gdb;
>> +
>> +void vmcoreinfo_gdb_update(void)
>> +{
>> +    Object *vmci = find_vmcoreinfo_dev();
>> +
>> +    vmcoreinfo_gdb.available = vmci ?
>> +        vmcoreinfo_get(VMCOREINFO(vmci),
>> +                       &vmcoreinfo_gdb.paddr, &vmcoreinfo_gdb.size, NULL)
>> +        : false;
>> +}
>> +
>>  static const VMStateDescription vmstate_vmcoreinfo = {
>>      .name = "vmcoreinfo",
>>      .version_id = 1,
>>
>
>



-- 
Marc-André Lureau