Re: [Qemu-devel] [PATCH v6 10/11] dump: Make kdump-compressed format available for 'dump-guest-memory'

[Laszlo Ersek]

comments below

On 01/05/14 08:27, Qiao Nuohan wrote:
> Make monitor command 'dump-guest-memory' be able to dump in kdump-compressed
> format. The command's usage:
> 
>   dump [-p] protocol [begin] [length] [format]
> 
> 'format' is used to specified the format of vmcore and can be:
> 1. 'elf': ELF format, without compression
> 2. 'kdump-zlib': kdump-compressed format, with zlib-compressed
> 3. 'kdump-lzo': kdump-compressed format, with lzo-compressed
> 4. 'kdump-snappy': kdump-compressed format, with snappy-compressed
> And without 'format' being set, it is same as 'elf'.
> 
> Note:
>   1. The kdump-compressed format is readable only with the crash utility and
>      makedumpfile, and it can be smaller than the ELF format because of the
>      compression support.
>   2. The kdump-compressed format is the 6th edition.
> 
> Signed-off-by: Qiao Nuohan <address@hidden>
> ---
>  dump.c           |  163 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hmp-commands.hx  |   12 +++-
>  hmp.c            |   23 +++++++-
>  qapi-schema.json |   22 +++++++-
>  qmp-commands.hx  |    6 +-
>  5 files changed, 212 insertions(+), 14 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 848957c..b4e79ff 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1398,6 +1398,70 @@ out:
>      return ret;
>  }
>  
> +static int create_kdump_vmcore(DumpState *s)
> +{
> +    int ret;
> +
> +    /*
> +     * the kdump-compressed format is:
> +     *                                               File offset
> +     *  +------------------------------------------+ 0x0
> +     *  |    main header (struct disk_dump_header) |
> +     *  |------------------------------------------+ block 1
> +     *  |    sub header (struct kdump_sub_header)  |

Also including the "note".

... Come to think of it, "note_buf" could actually be local to
create_headerXX(), no need to add it to DumpState.

> +     *  |------------------------------------------+ block 2
> +     *  |            1st-dump_bitmap               |
> +     *  |------------------------------------------+ block 2 + X blocks
> +     *  |            2nd-dump_bitmap               | (aligned by block)
> +     *  |------------------------------------------+ block 2 + 2 * X blocks
> +     *  |  page desc for pfn 0 (struct page_desc)  | (aligned by block)
> +     *  |  page desc for pfn 1 (struct page_desc)  |
> +     *  |                    :                     |
> +     *  |  page desc for pfn Z (struct page_desc)  |
> +     *  |------------------------------------------| (not aligned by block)
> +     *  |         page data (pfn 0)                |
> +     *  |         page data (pfn 1)                |
> +     *  |                        :                 |
> +     *  |         page data (pfn Z)                |
> +     *  +------------------------------------------+ offset_eraseinfo

I think this "offset_eraseinfo" is a bit misleading, because we never
set that field in KdumpSubHeaderXX to nonzero, and we also never write
the trailing portion here.

Maybe a zero value of "offset_eraseinfo" implies presicely that the
portion marked below as ":" is never written, but still
"offset_eraseinfo" (== 0) doesn't match the end of the page data.

Also, using the same Z as final subscript for "page data" as for "page
desc" is misleading. There can be fewer, equal number of, and more page
data entries than page desc entries. We'll have fewer page data entries
if the guest has at least two zero pages. We'll have more page data
entries if the guest has no zero pages (because we dump exactly one zero
page data entry).

> +     *  |                    :                     |
> +     *  +------------------------------------------+
> +     */
> +
> +    if (s->flag_flatten) {
> +        ret = write_start_flat_header(s->fd);
> +        if (ret < 0) {
> +            return -1;
> +        }
> +    }
> +
> +    ret = write_dump_header(s);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    ret = write_dump_bitmap(s);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    ret = write_dump_pages(s);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    if (s->flag_flatten) {
> +        ret = write_end_flat_header(s->fd);
> +        if (ret < 0) {
> +            return -1;
> +        }
> +    }
> +
> +    dump_completed(s);
> +
> +    return 0;
> +}
> +
>  static ram_addr_t get_start_block(DumpState *s)
>  {
>      GuestPhysBlock *block;
> @@ -1426,7 +1490,27 @@ static ram_addr_t get_start_block(DumpState *s)
>      return -1;
>  }
>  
> -static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
> +static bool use_flatten_format(int fd)
> +{
> +    if (lseek(fd, 0, SEEK_SET) < 0) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void get_max_mapnr(DumpState *s)
> +{
> +    MemoryMapping *memory_mapping;
> +
> +    QTAILQ_FOREACH(memory_mapping, &s->list.head, next) {
> +        s->max_mapnr = paddr_to_pfn(memory_mapping->phys_addr +
> +                        memory_mapping->length, s->page_shift);
> +    }
> +}

I'm a bit confused about the difference between DumpState.num_dumpable
and DumpState.max_mapnr.

DumpState.max_mapnr:
- defined as the highest gpfn, plus 1
- used to fill the "DiskDumpHeaderXX.max_mapnr" field (truncated to 32
  bits),
- used to fill the "KdumpSubHeaderXX.max_mapnr_64" field,
- used to determine "DumpState.len_dump_bitmap", which in turn
  determines:
  - start offset of the 2nd bitmap dump
  - DiskDumpHeaderXX.bitmap_blocks, which in turn determines:
    - "DumpState.offset_page", ie. start offset of page_descs

DumpState.num_dumpable:
- number of existent guest page frames (<= DumpState.max_mapnr)
- number of entries in the page desc area,
- number of entries in the page data area

OK. I got confused for a second but these two sets of things are
independent.

"crash" can bit-index the bitmap with the absolute gpfn. By summing the
1-bits up to there, it can derive the subscript into the page desc
array. Seems good.

> +
> +static int dump_init(DumpState *s, int fd, bool has_format,
> +                     DumpGuestMemoryFormat format, bool paging, bool 
> has_filter,
>                       int64_t begin, int64_t length, Error **errp)
>  {
>      CPUState *cpu;
> @@ -1494,6 +1578,44 @@ static int dump_init(DumpState *s, int fd, bool 
> paging, bool has_filter,
>          qemu_get_guest_simple_memory_mapping(&s->list, 
> &s->guest_phys_blocks);
>      }
>  
> +    /* init for kdump-compressed format */
> +    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> +        switch (format) {
> +        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB:
> +            s->flag_compress = DUMP_DH_COMPRESSED_ZLIB;
> +            break;
> +
> +        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO:
> +            s->flag_compress = DUMP_DH_COMPRESSED_LZO;
> +            break;
> +
> +        case DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY:
> +            s->flag_compress = DUMP_DH_COMPRESSED_SNAPPY;
> +            break;
> +
> +        default:
> +            s->flag_compress = 0;

Not really necessary since DumpState is allocated with g_malloc0(), but
doesn't hurt.

> +        }
> +
> +        /*
> +         * check to see if fd is available to seek.
> +         * if not, flatten format is used to avoid seek
> +         */
> +        s->flag_flatten = use_flatten_format(fd);

use_flatten_format() will mis-report an lseek() IO error on an otherwise
seekable regular file or block device, but such IO errors should be
detected later on anyway. fstat() + S_ISREG()/S_ISBLK() would be
cleaner, but it's not very important.

> +
> +        s->nr_cpus = nr_cpus;
> +        s->page_size = PAGE_SIZE;
> +        s->page_shift = ffs(s->page_size) - 1;
> +
> +        get_max_mapnr(s);
> +
> +        size_t tmp;
> +        tmp = DIV_ROUND_UP(DIV_ROUND_UP(s->max_mapnr, CHAR_BIT), 
> s->page_size);
> +        s->len_dump_bitmap = tmp * s->page_size;

"tmp" and "s->len_dump_bitmap" are "size_t", which can be 32-bit, and
"s->max_mapnr" is "uint64_t". But we discussed earlier how much guest
memory it would take to overflow this, so I think it's fine.

> +
> +        return 0;

OK I think this covers all new fields in DumpState.

> +    }
> +
>      if (s->has_filter) {
>          memory_mapping_filter(&s->list, s->begin, s->length);
>      }
> @@ -1553,8 +1675,9 @@ cleanup:
>  }
>  
>  void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
> -                           int64_t begin, bool has_length, int64_t length,
> -                           Error **errp)
> +                           int64_t begin, bool has_length,
> +                           int64_t length, bool has_format,
> +                           DumpGuestMemoryFormat format, Error **errp)
>  {
>      const char *p;
>      int fd = -1;
> @@ -1569,6 +1692,27 @@ void qmp_dump_guest_memory(bool paging, const char 
> *file, bool has_begin,
>          error_set(errp, QERR_MISSING_PARAMETER, "begin");
>          return;
>      }
> +    /* kdump-compressed format doesn't support paging or filter */
> +    if ((has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) &&
> +        (paging || has_begin || has_length)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +        return;
> +    }

I'd prefer a more verbose explanation with error_setg() -- the error
should state what the comment says -- but it's not a blocker for me.

And, this check here is *really* important. I wish it were reflected in
a handful of asserts() across the rest of the code (I sought to call out
those spots).

> +
> +    /* check whether lzo/snappy is supported */
> +#ifndef CONFIG_LZO
> +    if (format == DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "format",
> +                  "supported format(kdump-lzo is not available now)");
> +    }
> +#endif

In theory you should look at "has_format" first. In practice I think if
the format is absent then "format" will have value 0
(DUMP_GUEST_MEMORY_FORMAT_ELF), so the check is OK as-is.

The error message needs at least a space character before the left
paren, but error_setg() would be preferable I think. (Others should
chime in too of course...)

> +
> +#ifndef CONFIG_SNAPPY
> +    if (format == DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "format",
> +                  "supported format(kdump-snappy is not available now)");
> +    }
> +#endif

Ditto.

>  
>  #if !defined(WIN32)
>      if (strstart(file, "fd:", &p)) {
> @@ -1594,14 +1738,21 @@ void qmp_dump_guest_memory(bool paging, const char 
> *file, bool has_begin,
>  
>      s = g_malloc0(sizeof(DumpState));
>  
> -    ret = dump_init(s, fd, paging, has_begin, begin, length, errp);
> +    ret = dump_init(s, fd, has_format, format, paging, has_begin,
> +                    begin, length, errp);
>      if (ret < 0) {
>          g_free(s);
>          return;
>      }
>  
> -    if (create_vmcore(s) < 0 && !error_is_set(s->errp)) {
> -        error_set(errp, QERR_IO_ERROR);
> +    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> +        if (create_kdump_vmcore(s) < 0 && !error_is_set(s->errp)) {
> +            error_set(errp, QERR_IO_ERROR);
> +        }
> +    } else {
> +        if (create_vmcore(s) < 0 && !error_is_set(s->errp)) {
> +            error_set(errp, QERR_IO_ERROR);
> +        }
>      }

On error some stuff may be leaked -- there are error paths when
dump_cleanup() is never called, for example:

dump_init()
  guest_phys_blocks_append()
create_kdump_vmcore()
  write_start_flat_header()
    return -1

I'm not sure if this has been possible in the create_vmcore() branch
too. It seems unlikely, because that branch tends to call dump_error()
on all (most?) error paths.

>  
>      g_free(s);
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index ebe8e78..3856bb4 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -993,17 +993,19 @@ ETEXI
>  
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:-p,filename:F,begin:i?,length:i?",
> -        .params     = "[-p] filename [begin] [length]",
> +        .args_type  = "paging:-p,filename:F,begin:i?,length:i?,format:s?",
> +        .params     = "[-p] filename [begin] [length] [format]",
>          .help       = "dump guest memory to file"
>                        "\n\t\t\t begin(optional): the starting physical 
> address"
> -                      "\n\t\t\t length(optional): the memory size, in bytes",
> +                      "\n\t\t\t length(optional): the memory size, in bytes"
> +                      "\n\t\t\t format(optional): the format of guest memory 
> dump,"
> +                      "\n\t\t\t it can be 
> elf|kdump-zlib|kdump-lzo|kdump-snappy",
>          .mhandler.cmd = hmp_dump_guest_memory,
>      },
>  
>  
>  STEXI
> address@hidden dump-guest-memory [-p] @var{protocol} @var{begin} @var{length}
> address@hidden dump-guest-memory [-p] @var{protocol} @var{begin} @var{length} 
> @var{format}
>  @findex dump-guest-memory
>  Dump guest memory to @var{protocol}. The file can be processed with crash or
>  gdb.
> @@ -1013,6 +1015,8 @@ gdb.
>              specified with length together.
>      length: the memory size, in bytes. It's optional, and should be specified
>              with begin together.
> +    format: the format of guest memory dump. It's optional, and can be
> +            elf|kdump-zlib|kdump-lzo|kdump-snappy
>  ETEXI
>  
>      {

I would document that non-elf formats conflict with both paging and
filtering (ie. begin/length)

However... I'm afraid the compression facility is simply unaccessible
via HMP. Namely:

- If you specify both "begin" and "length", then the parameter
combination check will (correctly!) reject compressed formats.

- If you omit "begin" and/or "length", then the HMP parser will choke on
the compression format, trying to parse it (eg. "kdump-zlib") as the
first integer (begin or length) that you would have wanted to omit.

This is a limitation of the HMP parser, see monitor_parse_command():

        case 'i':
        case 'l':
        case 'M':
            {
                int64_t val;

                while (qemu_isspace(*p))
                    p++;
                if (*typestr == '?' || *typestr == '.') {
                    if (*typestr == '?') {
                        if (*p == '\0') {
                            typestr++;
                            break;
                        }
                    } else {
                        if (*p == '.') {
                            p++;
                            while (qemu_isspace(*p))
                                p++;
                        } else {
                            typestr++;
                            break;
                        }
                    }
                    typestr++;
                }
                if (get_expr(mon, &val, &p))
                    goto fail;

There are only two possibilities to break out of the 'i' branch and
avoid blowing up in get_expr():

- (*typestr == '?' && *p == '\0'), that is, the parameter (ie. "begin",
"length") is optional, and there's nothing else (except whitespace) on
the HMP command line. This is not our case, because the command line
still has "kdump-zlib".

- (*typestr == '.' && *p != '.') -- This would require replacing the
"i?" specifiers in "args_type" with "i.", and passing such arguments for
"begin" and "length" on the HMP command line that don't start with a
dot. For example:

  (qemu) dump-guest-memory 0 1000000 kdump-zlib

In this case "begin" and "length" would be ignored due to the "i."
specifier. They would only be parsed with

  (qemu) dump-guest-memory .0 .1000000 kdump-zlib

However, "i." would break existing HMP command lines, like

  (qemu) dump-guest-memory 0 1000000

so "i." won't fly.

("i." is BTW only ever used with do_ioport_read(), for the case when the
port to read works as an index-register on write.)


I think we simply shouldn't try to make this functionality available
over HMP.

> diff --git a/hmp.c b/hmp.c
> index 32ee285..9bd62b8 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1307,9 +1307,12 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
> *qdict)
>      const char *file = qdict_get_str(qdict, "filename");
>      bool has_begin = qdict_haskey(qdict, "begin");
>      bool has_length = qdict_haskey(qdict, "length");
> +    bool has_format = qdict_haskey(qdict, "format");
>      int64_t begin = 0;
>      int64_t length = 0;
> +    const char *format = NULL;
>      char *prot;
> +    enum DumpGuestMemoryFormat dump_format;
>  
>      if (has_begin) {
>          begin = qdict_get_int(qdict, "begin");
> @@ -1317,11 +1320,29 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
> *qdict)
>      if (has_length) {
>          length = qdict_get_int(qdict, "length");
>      }
> +    if (has_format) {
> +        format = qdict_get_str(qdict, "format");
> +    }
> +
> +    if (strcmp(format, "elf") == 0) {

This results in a SIGSEGV when the format has not been specified
("format" is initialized to NULL in its definition, and when "qdict" has
no "format" key (--> has_format==false) then "format" is left at NULL).

But, if we drop HMP support for the feature, this goes away too.

> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
> +    } else if (strcmp(format, "kdump-zlib") == 0) {
> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
> +    } else if (strcmp(format, "kdump-lzo") == 0) {
> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
> +    } else if (strcmp(format, "kdump-snappy") == 0) {
> +        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
> +    } else {
> +        error_set(&errp, QERR_INVALID_PARAMETER_VALUE,
> +                  "format", "elf|kdump-zlib|kdump-lzo|kdump-snappy");
> +        hmp_handle_error(mon, &errp);
> +        return;
> +    }
>  
>      prot = g_strconcat("file:", file, NULL);
>  
>      qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
> -                          &errp);
> +                          has_format, dump_format, &errp);
>      hmp_handle_error(mon, &errp);
>      g_free(prot);
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c3c939c..19b2b23 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2676,6 +2676,24 @@
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @DumpGuestMemoryFormat:
> +#
> +# An enumeration of guest-memory-dump's format.
> +#
> +# @elf: elf format
> +#
> +# @kdump-zlib: kdump-compressed format with zlib-compressed
> +#
> +# @kdump-lzo: kdump-compressed format with zlib-compressed

should say "lzo-compressed" rather than "zlib-compressed"

> +#
> +# @kdump-snappy: kdump-compressed format with zlib-compressed

should say "snappy-compressed" rather than "zlib-compressed"

> +#
> +# Since: 1.8
> +##

1.8 will never exist, we should say 2.0

> +{ 'enum': 'DumpGuestMemoryFormat',
> +  'data': [ 'elf', 'kdump-zlib', 'kdump-lzo', 'kdump-snappy' ] }
> +
> +##
>  # @dump-guest-memory
>  #
>  # Dump guest's memory to vmcore. It is a synchronous operation that can take
> @@ -2711,13 +2729,15 @@
>  #          want to dump all guest's memory, please specify the start @begin
>  #          and @length
>  #
> +# @format: #optional if specified, the format of guest memory dump. (since 
> 1.8)
> +#

since 2.0

>  # Returns: nothing on success
>  #
>  # Since: 1.2
>  ##
>  { 'command': 'dump-guest-memory',
>    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
> -            '*length': 'int' } }
> +            '*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
>  
>  ##
>  # @netdev_add:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fba15cd..3de9e7d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -791,8 +791,8 @@ EQMP
>  
>      {
>          .name       = "dump-guest-memory",
> -        .args_type  = "paging:b,protocol:s,begin:i?,end:i?",
> -        .params     = "-p protocol [begin] [length]",
> +        .args_type  = "paging:b,protocol:s,begin:i?,length:i?,format:s?",

Adding "format' is OK, but we should not rename "end" to "length".

> +        .params     = "-p protocol [begin] [length] [format]",
>          .help       = "dump guest memory to file",
>          .user_print = monitor_user_noop,
>          .mhandler.cmd_new = qmp_marshal_input_dump_guest_memory,
> @@ -813,6 +813,8 @@ Arguments:
>             with length together (json-int)
>  - "length": the memory size, in bytes. It's optional, and should be specified
>              with begin together (json-int)
> +- "format": the format of guest memory dump. It's optional, and can be
> +            elf|kdump-zlib|kdump-lzo|kdump-snappy (json-string)
>  
>  Example:
>  
> 

I think this patch should be respun in order to drop HMP support
completely. The rest is of varying importance; most of it shouldn't be
too hard to address.

Thanks
Laszlo