Re: [Qemu-devel] [PATCH v2 2/4] dump: add vmcoreinfo ELF note

[Laszlo Ersek]

On 06/01/17 15:03, Marc-André Lureau wrote:
> Read vmcoreinfo note from guest memory when dump_info provides the
> address, and write it as an ELF note in the dump.
> 
> NUMBER(phys_base) in vmcoreinfo has only been recently introduced in
> Linux 4.10 ("kexec: export the value of phys_base instead of symbol
> address"). To accomadate for older kernels, modify the vmcoreinfo to add
> the new fields and help newer crash that will use it.

I think here you mean

  modify the DumpState structure

rather than

  modify the vmcoreinfo

> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/sysemu/dump.h |   2 +
>  dump.c                | 133 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 135 insertions(+)
> 
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 2672a15f8b..b8a7a1e41d 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -192,6 +192,8 @@ typedef struct DumpState {
>                                    * this could be used to calculate
>                                    * how much work we have
>                                    * finished. */
> +    uint8_t *vmcoreinfo;

Can you document that this is an ELF note?

> +    size_t vmcoreinfo_size;
>  } DumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> diff --git a/dump.c b/dump.c
> index bdf3270f02..6911ffad8b 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -27,6 +27,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qmp-commands.h"
>  #include "qapi-event.h"
> +#include "qemu/error-report.h"
>  
>  #include <zlib.h>
>  #ifdef CONFIG_LZO
> @@ -88,6 +89,8 @@ static int dump_cleanup(DumpState *s)
>              qemu_mutex_unlock_iothread();
>          }
>      }
> +    g_free(s->vmcoreinfo);
> +    s->vmcoreinfo = NULL;
>  
>      return 0;
>  }

I vaguely feel that this should be moved in front of resuming VM
execution. I don't have a strong reason, just consistency with the rest
of the cleanup.

> @@ -238,6 +241,19 @@ static inline int cpu_index(CPUState *cpu)
>      return cpu->cpu_index + 1;
>  }
>  
> +static void write_vmcoreinfo_note(WriteCoreDumpFunction f, DumpState *s,
> +                                  Error **errp)
> +{
> +    int ret;
> +
> +    if (s->vmcoreinfo) {
> +        ret = f(s->vmcoreinfo, s->vmcoreinfo_size, s);
> +        if (ret < 0) {
> +            error_setg(errp, "dump: failed to write vmcoreinfo");
> +        }
> +    }
> +}
> +
>  static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
>                                Error **errp)
>  {
> @@ -261,6 +277,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
> DumpState *s,
>              return;
>          }
>      }
> +
> +    write_vmcoreinfo_note(f, s, errp);
>  }
>  
>  static void write_elf32_note(DumpState *s, Error **errp)
> @@ -306,6 +324,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
> DumpState *s,
>              return;
>          }
>      }
> +
> +    write_vmcoreinfo_note(f, s, errp);
>  }
>  
>  static void write_elf_section(DumpState *s, int type, Error **errp)
> @@ -717,6 +737,50 @@ static int buf_write_note(const void *buf, size_t size, 
> void *opaque)
>      return 0;
>  }
>  
> +static void get_note_sizes(DumpState *s, const void *note,
> +                           uint64_t *note_head_size,
> +                           uint64_t *name_size,
> +                           uint64_t *desc_size)
> +{

I'm not happy that I have to reverse engineer what this function does.
Please document it in the commit message and/or in a function-level
comment, especially regarding the actual permitted types of *note.

Very similar functionality exists in "target/i386/arch_dump.c" already.
Is there a (remote) possibility to extract / refactor / share code?

> +    uint64_t note_head_sz;
> +    uint64_t name_sz;
> +    uint64_t desc_sz;
> +
> +    if (s->dump_info.d_class == ELFCLASS64) {

Ugh, this is extremely confusing. This refers to DumpState.dump_info,
which has type ArchDumpInfo. But in the previous patch we also introduce
a global "dump_info" variable, of type DumpInfo.

Worse, ArchDumpInfo already has a field called "phys_base" (comment:
"The target's physmem base"), and it's even filled in in
"target/arm/arch_dump.c", function cpu_get_dump_info():

/* Take a best guess at the phys_base. If we get it wrong then crash
 * will need '--machdep phys_offset=<phys-offset>' added to its command
 * line, which isn't any worse than assuming we can use zero, but being
 * wrong. This is the same algorithm the crash utility uses when
 * attempting to guess as it loads non-dumpfile formatted files.
 */

Looks like we already have some overlapping code / functionality for
this, for the ARM target?

Sorry, I'm totally lost. It must have been years since I last looked at
this code. I guess my comments might not make much sense, even.

Please post a version 3, with as detailed as possible commit messages,
explaining your entire thought process, the data flow, how this feature
fits into the old code, and all the modifications. Personally at least,
I need a complete re-introduction to this, to make heads or tails of
your changes.

I mean I certainly don't *insist* on reviewing this code, it's just that
if you'd like me to comment on it, you'll have to document all the
investigative work you've done before / while writing it.

Thanks
Laszlo



> +        const Elf64_Nhdr *hdr = note;
> +        note_head_sz = sizeof(Elf64_Nhdr);
> +        name_sz = hdr->n_namesz;
> +        desc_sz = hdr->n_descsz;
> +    } else {
> +        const Elf32_Nhdr *hdr = note;
> +        note_head_sz = sizeof(Elf32_Nhdr);
> +        name_sz = hdr->n_namesz;
> +        desc_sz = hdr->n_descsz;
> +    }
> +
> +    if (note_head_size) {
> +        *note_head_size = note_head_sz;
> +    }
> +    if (name_size) {
> +        *name_size = name_sz;
> +    }
> +    if (desc_size) {
> +        *desc_size = desc_sz;
> +    }
> +}
> +
> +static void set_note_desc_size(DumpState *s, void *note,
> +                               uint64_t desc_size)
> +{
> +    if (s->dump_info.d_class == ELFCLASS64) {
> +        Elf64_Nhdr *hdr = note;
> +        hdr->n_descsz = desc_size;
> +    } else {
> +        Elf32_Nhdr *hdr = note;
> +        hdr->n_descsz = desc_size;
> +    }
> +}
> +
>  /* write common header, sub header and elf note to vmcore */
>  static void create_header32(DumpState *s, Error **errp)
>  {
> @@ -1491,6 +1555,42 @@ static int64_t dump_calculate_size(DumpState *s)
>      return total;
>  }
>  
> +static void vmcoreinfo_add_phys_base(DumpState *s)
> +{
> +    uint64_t size, note_head_size, name_size;
> +    char **lines, *physbase = NULL;
> +    uint8_t *newvmci, *vmci;
> +    size_t i;
> +
> +    get_note_sizes(s, s->vmcoreinfo, &note_head_size, &name_size, &size);
> +    note_head_size = ((note_head_size + 3) / 4) * 4;
> +    name_size = ((name_size + 3) / 4) * 4;
> +    vmci = s->vmcoreinfo + note_head_size + name_size;
> +    *(vmci + size) = '\0';
> +    lines = g_strsplit((char *)vmci, "\n", -1);
> +    for (i = 0; lines[i]; i++) {
> +        if (g_str_has_prefix(lines[i], "NUMBER(phys_base)=")) {
> +            goto end;
> +        }
> +    }
> +
> +    physbase = g_strdup_printf("\nNUMBER(phys_base)=%ld",
> +                               s->dump_info.phys_base);
> +    s->vmcoreinfo_size =
> +        ((note_head_size + name_size + size + strlen(physbase) + 3) / 4) * 4;
> +
> +    newvmci = g_malloc(s->vmcoreinfo_size);
> +    memcpy(newvmci, s->vmcoreinfo, note_head_size + name_size + size - 1);
> +    memcpy(newvmci + note_head_size + name_size + size - 1, physbase,
> +           strlen(physbase) + 1);
> +    g_free(s->vmcoreinfo);
> +    s->vmcoreinfo = newvmci;
> +    set_note_desc_size(s, s->vmcoreinfo, size + strlen(physbase));
> +
> +end:
> +    g_strfreev(lines);
> +}
> +
>  static void dump_init(DumpState *s, int fd, bool has_format,
>                        DumpGuestMemoryFormat format, bool paging, bool 
> has_filter,
>                        int64_t begin, int64_t length, Error **errp)
> @@ -1566,6 +1666,39 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>          goto cleanup;
>      }
>  
> +    if (dump_info.has_phys_base) {
> +        s->dump_info.phys_base = dump_info.phys_base;
> +    }
> +    if (dump_info.vmcoreinfo) {
> +        uint64_t addr, size, note_head_size, name_size, desc_size;
> +        int count = sscanf(dump_info.vmcoreinfo, "%" PRIx64 " %" PRIx64,
> +                           &addr, &size);
> +        if (count != 2) {
> +            /* non fatal error */
> +            error_report("Failed to parse vmcoreinfo");
> +        } else {
> +            assert(!s->vmcoreinfo);
> +            s->vmcoreinfo = g_malloc(size);
> +            cpu_physical_memory_read(addr, s->vmcoreinfo, size);
> +
> +            get_note_sizes(s, s->vmcoreinfo,
> +                           &note_head_size, &name_size, &desc_size);
> +            s->vmcoreinfo_size = ((note_head_size + 3) / 4 +
> +                                  (name_size + 3) / 4 +
> +                                  (desc_size + 3) / 4) * 4;
> +            if (s->vmcoreinfo_size > size) {
> +                error_report("Invalid vmcoreinfo header, size mismatch");
> +                g_free(s->vmcoreinfo);
> +                s->vmcoreinfo = NULL;
> +            } else {
> +                if (dump_info.has_phys_base) {
> +                    vmcoreinfo_add_phys_base(s);
> +                }
> +                s->note_size += s->vmcoreinfo_size;
> +            }
> +        }
> +    }
> +
>      /* get memory mapping */
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>