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

[Laszlo Ersek]

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 <address@hidden>
> ---
>  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.

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

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