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

[Andrew Jones]

On Thu, Jun 01, 2017 at 05:03:23PM +0400, 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.
> 
> 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;
> +    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;
>  }
> @@ -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)
> +{
> +    uint64_t note_head_sz;
> +    uint64_t name_sz;
> +    uint64_t desc_sz;
> +
> +    if (s->dump_info.d_class == ELFCLASS64) {
> +        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;

I'd prefer to replace all the + 3 / 4 * 4 stuff with ROUND_UP()

> +    vmci = s->vmcoreinfo + note_head_size + name_size;

How about using another size variable (e.g. sz) = 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;
> +        }
> +    }

I don't think it should be necessary to split the info first. We can
just search the entire info for the substring '\nNUMBER(phys_base)='.

> +
> +    physbase = g_strdup_printf("\nNUMBER(phys_base)=%ld",
> +                               s->dump_info.phys_base);

sz += size

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

How about using g_realloc() instead?

> +    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);
> -- 
> 2.13.0.91.g00982b8dd
> 
>

Thanks,
drew