Re: [Qemu-devel] [PATCH v4 4/9] dump: Add API to create header of vmcore

["Andreas Färber" <afaerber@suse.de>]

Am 28.05.2013 04:50, schrieb qiaonuohan@cn.fujitsu.com:
> From: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> 
> Functions in this patch are used to gather data of header and sub header in
> kdump-compressed format. The following patch will use these functions to gather
> data of header, then cache them into struct DumpState.
> 
> Signed-off-by: Qiao Nuohan <qiaonuohan@cn.fujitsu.com>
> Reviewed-by: Zhang Xiaohe <zhangxh@cn.fujitsu.com>
> ---
>  dump.c                       |  107 ++++++++++++++++++++++++++++++++++++++++++
>  include/sysemu/dump_memory.h |   93 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 9ac66be..3b9d4ca 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -681,6 +681,113 @@ static ram_addr_t get_start_block(DumpState *s)
>      return -1;
>  }
>  
> +static int create_header32(DumpState *s)
> +{
> +    struct  disk_dump_header32 *dh;
> +    struct  kdump_sub_header32 *kh;
> +
> +    /* create common header, the version of kdump-compressed format is 5th */
> +    dh = g_malloc0(sizeof(struct disk_dump_header32));
> +
> +    strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
> +    dh->header_version = 5;
> +    dh->block_size = s->page_size;
> +    dh->sub_hdr_size = sizeof(struct kdump_sub_header32) + s->note_size;
> +    dh->sub_hdr_size = divideup(dh->sub_hdr_size, dh->block_size);
> +    dh->max_mapnr = s->max_mapnr;
> +    dh->nr_cpus = s->nr_cpus;
> +    dh->bitmap_blocks = divideup(s->len_dump_bitmap, s->page_size);
> +
> +    memcpy(&(dh->utsname.machine), "i686", 4);
> +
> +    s->dh = dh;
> +
> +    /* create sub header */
> +    kh = g_malloc0(sizeof(struct kdump_sub_header32));
> +
> +    kh->phys_base = PHYS_BASE;
> +    kh->dump_level = DUMP_LEVEL;
> +
> +    kh->offset_note = DISKDUMP_HEADER_BLOCKS * dh->block_size +
> +                        sizeof(struct kdump_sub_header32);
> +    kh->note_size = s->note_size;
> +
> +    s->kh = kh;
> +
> +    /* get gap between header and sub header */
> +    s->offset_sub_header = DISKDUMP_HEADER_BLOCKS * dh->block_size -
> +                            sizeof(struct disk_dump_header32);
> +
> +    /* get gap between header and dump_bitmap */
> +    s->offset_dump_bitmap = dh->sub_hdr_size * dh->block_size -
> +                            (sizeof(struct kdump_sub_header32) + s->note_size);
> +
> +    /* get offset of page desc */
> +    s->offset_page = (DISKDUMP_HEADER_BLOCKS + dh->sub_hdr_size +
> +                        dh->bitmap_blocks) * dh->block_size;
> +
> +    return 0;
> +}

Function always return 0 - make it void?

> +
> +static int create_header64(DumpState *s)
> +{
> +    struct  disk_dump_header64 *dh;
> +    struct  kdump_sub_header64 *kh;
> +
> +    /* create common header, the version of kdump-compressed format is 5th */
> +    dh = g_malloc0(sizeof(struct disk_dump_header64));
> +
> +    strncpy(dh->signature, KDUMP_SIGNATURE, strlen(KDUMP_SIGNATURE));
> +    dh->header_version = 5;
> +    dh->block_size = s->page_size;
> +    dh->sub_hdr_size = sizeof(struct kdump_sub_header64) + s->note_size;
> +    dh->sub_hdr_size = divideup(dh->sub_hdr_size, dh->block_size);
> +    dh->max_mapnr = s->max_mapnr;
> +    dh->nr_cpus = s->nr_cpus;
> +    dh->bitmap_blocks = divideup(s->len_dump_bitmap, s->page_size);
> +
> +    memcpy(&(dh->utsname.machine), "x86_64", 6);
> +
> +    s->dh = dh;
> +
> +    /* create sub header */
> +    kh = g_malloc0(sizeof(struct kdump_sub_header64));
> +
> +    kh->phys_base = PHYS_BASE;
> +    kh->dump_level = DUMP_LEVEL;
> +
> +    kh->offset_note = DISKDUMP_HEADER_BLOCKS * dh->block_size +
> +                        sizeof(struct kdump_sub_header64);
> +    kh->note_size = s->note_size;
> +
> +    s->kh = kh;
> +
> +    /* get gap between header and sub header */
> +    s->offset_sub_header = DISKDUMP_HEADER_BLOCKS * dh->block_size -
> +                            sizeof(struct disk_dump_header64);
> +
> +    /* get gap between header and dump_bitmap */
> +    s->offset_dump_bitmap = dh->sub_hdr_size * dh->block_size -
> +                            (sizeof(struct kdump_sub_header64) + s->note_size);
> +
> +    /* get offset of page desc */
> +    s->offset_page = (DISKDUMP_HEADER_BLOCKS + dh->sub_hdr_size +
> +                        dh->bitmap_blocks) * dh->block_size;
> +
> +    return 0;
> +}
> +
> +/*
> + * gather data of header and sub header
> + */
> +static int create_header(DumpState *s)
> +{
> +    if (s->dump_info.d_machine == EM_386)
> +        return create_header32(s);
> +    else
> +        return create_header64(s);
> +}

Braces for if and else missing.

Surely EM_386 is not the only 32-bit machine.

> +
>  static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>                       int64_t begin, int64_t length, Error **errp)
>  {
> diff --git a/include/sysemu/dump_memory.h b/include/sysemu/dump_memory.h
> index ce22c05..56e0f40 100644
> --- a/include/sysemu/dump_memory.h
> +++ b/include/sysemu/dump_memory.h
> @@ -18,6 +18,87 @@
>  #include "sysemu/memory_mapping.h"
>  #include "sysemu/dump.h"
>  
> +#define KDUMP_SIGNATURE             "KDUMP   "
> +#define SIG_LEN                     (sizeof(KDUMP_SIGNATURE) - 1)
> +#define DISKDUMP_HEADER_BLOCKS      (1)
> +#define PHYS_BASE                   (0)
> +#define DUMP_LEVEL                  (1)
> +
> +#define divideup(x, y)              (((x) + ((y) - 1)) / (y))
> +
> +struct new_utsname {
> +    char sysname[65];
> +    char nodename[65];
> +    char release[65];
> +    char version[65];
> +    char machine[65];
> +    char domainname[65];
> +};
> +
> +struct disk_dump_header32 {
> +    char signature[SIG_LEN];        /* = "KDUMP   " */
> +    int header_version;             /* Dump header version */
> +    struct new_utsname utsname;     /* copy of system_utsname */
> +    char timestamp[8];              /* Time stamp */
> +    unsigned int status;            /* Above flags */
> +    int block_size;                 /* Size of a block in byte */
> +    int sub_hdr_size;               /* Size of arch dependent header in block */
> +    unsigned int bitmap_blocks;     /* Size of Memory bitmap in block */
> +    unsigned int max_mapnr;         /* = max_mapnr */
> +    unsigned int total_ram_blocks;  /* Number of blocks should be written */
> +    unsigned int device_blocks;     /* Number of total blocks in dump device */
> +    unsigned int written_blocks;    /* Number of written blocks */
> +    unsigned int current_cpu;       /* CPU# which handles dump */
> +    int nr_cpus;                    /* Number of CPUs */
> +    struct task_struct *tasks[0];
> +};
> +
> +struct disk_dump_header64 {
> +    char signature[SIG_LEN];        /* = "KDUMP   " */
> +    int header_version;             /* Dump header version */
> +    struct new_utsname utsname;     /* copy of system_utsname */
> +    char timestamp[20];             /* Time stamp */
> +    unsigned int status;            /* Above flags */
> +    int block_size;                 /* Size of a block in byte */
> +    int sub_hdr_size;               /* Size of arch dependent header in block */
> +    unsigned int bitmap_blocks;     /* Size of Memory bitmap in block */
> +    unsigned int max_mapnr;         /* = max_mapnr */
> +    unsigned int total_ram_blocks;  /* Number of blocks should be written */
> +    unsigned int device_blocks;     /* Number of total blocks in dump device */
> +    unsigned int written_blocks;    /* Number of written blocks */
> +    unsigned int current_cpu;       /* CPU# which handles dump */
> +    int nr_cpus;                    /* Number of CPUs */
> +    struct task_struct *tasks[0];
> +};

If these are headers, shouldn't they be using int32_t / uint32_t just
like the ones below? Also should tasks rather be struct task_struct
tasks[0]? An array of pointers in the file is a bit hard to imagine...

Also do any of these structs need QEMU_PACKED attribute?

> +
> +struct kdump_sub_header32 {
> +    uint32_t phys_base;
> +    uint32_t dump_level;            /* header_version 1 and later */
> +    uint32_t split;                 /* header_version 2 and later */
> +    uint32_t start_pfn;             /* header_version 2 and later */
> +    uint32_t end_pfn;               /* header_version 2 and later */
> +    uint32_t offset_vmcoreinfo;     /* header_version 3 and later */
> +    uint32_t size_vmcoreinfo;       /* header_version 3 and later */
> +    uint32_t offset_note;           /* header_version 4 and later */
> +    uint32_t note_size;             /* header_version 4 and later */
> +    uint32_t offset_eraseinfo;      /* header_version 5 and later */
> +    uint32_t size_eraseinfo;        /* header_version 5 and later */
> +};
> +
> +struct kdump_sub_header64 {
> +    uint64_t phys_base;
> +    uint32_t dump_level;            /* header_version 1 and later */
> +    uint32_t split;                 /* header_version 2 and later */
> +    uint64_t start_pfn;             /* header_version 2 and later */
> +    uint64_t end_pfn;               /* header_version 2 and later */
> +    uint64_t offset_vmcoreinfo;     /* header_version 3 and later */
> +    uint64_t size_vmcoreinfo;       /* header_version 3 and later */
> +    uint64_t offset_note;           /* header_version 4 and later */
> +    uint64_t note_size;             /* header_version 4 and later */
> +    uint64_t offset_eraseinfo;      /* header_version 5 and later */
> +    uint64_t size_eraseinfo;        /* header_version 5 and later */
> +};
> +
>  typedef struct DumpState {
>      ArchDumpInfo dump_info;
>      MemoryMappingList list;
> @@ -35,6 +116,18 @@ typedef struct DumpState {
>      int64_t begin;
>      int64_t length;
>      Error **errp;
> +
> +    int page_size;
> +    unsigned long long max_mapnr;
> +    int nr_cpus;
> +    void *dh;
> +    void *kh;
> +    off_t offset_sub_header;
> +
> +    off_t offset_dump_bitmap;
> +    unsigned long len_dump_bitmap;
> +
> +    off_t offset_page;
>  } DumpState;
>  
>  #endif

Why unsigned long long and unsigned long respectively?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg