[PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format

[PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format

[Stephen Brennan]

Hello all,

This is the third version of the kdump patch series, the first and
second revisions being visible at [1] and [2] respectively. You can see
the history and motivation for the patch series described in the cover
letter of [2].

Thank you for your continued feedback and review.
Stephen

Changes from v2 to v3:
- Rather than use "reassembled" flag in QMP API, represent each kdump
  format "kdump-X" with a new enumerator "kdump-raw-X". (The HMP
  interface retains the -R option)
- Return an error if the file descriptor passed in is not seekable, yet
  the requested dump format is kdump-raw-*

Changes from v1 to v2:
- Keep the default as the flattened format
- Add QMP / HMP arguments for "reassembled"

[1]: https://lore.kernel.org/qemu-devel/20230717163855.7383-1-stephen.s.brennan@oracle.com/
[2]: https://lore.kernel.org/qemu-devel/20230914010315.945705-1-stephen.s.brennan@oracle.com/

Stephen Brennan (3):
  dump: Pass DumpState to write_ functions
  dump: Allow directly outputting raw kdump format
  dump: Add command interface for kdump-raw formats

 dump/dump-hmp-cmds.c  | 21 +++++++--
 dump/dump.c           | 99 +++++++++++++++++++++++++++++++------------
 hmp-commands.hx       |  9 +++-
 include/sysemu/dump.h |  3 +-
 qapi/dump.json        | 24 +++++++++--
 5 files changed, 119 insertions(+), 37 deletions(-)

-- 
2.39.3

[PATCH v3 qemu 1/3] dump: Pass DumpState to write_ functions

[Stephen Brennan]

For the next patch, we need a reference to DumpState when writing data.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 dump/dump.c           | 40 ++++++++++++++++++++--------------------
 include/sysemu/dump.h |  2 +-
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index d4ef713cd0..74071a1565 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -809,7 +809,7 @@ static void create_vmcore(DumpState *s, Error **errp)
     dump_end(s, errp);
 }
 
-static int write_start_flat_header(int fd)
+static int write_start_flat_header(DumpState *s)
 {
     MakedumpfileHeader *mh;
     int ret = 0;
@@ -824,7 +824,7 @@ static int write_start_flat_header(int fd)
     mh->version = cpu_to_be64(VERSION_FLAT_HEADER);
 
     size_t written_size;
-    written_size = qemu_write_full(fd, mh, MAX_SIZE_MDF_HEADER);
+    written_size = qemu_write_full(s->fd, mh, MAX_SIZE_MDF_HEADER);
     if (written_size != MAX_SIZE_MDF_HEADER) {
         ret = -1;
     }
@@ -833,7 +833,7 @@ static int write_start_flat_header(int fd)
     return ret;
 }
 
-static int write_end_flat_header(int fd)
+static int write_end_flat_header(DumpState *s)
 {
     MakedumpfileDataHeader mdh;
 
@@ -841,7 +841,7 @@ static int write_end_flat_header(int fd)
     mdh.buf_size = END_FLAG_FLAT_HEADER;
 
     size_t written_size;
-    written_size = qemu_write_full(fd, &mdh, sizeof(mdh));
+    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
     if (written_size != sizeof(mdh)) {
         return -1;
     }
@@ -849,7 +849,7 @@ static int write_end_flat_header(int fd)
     return 0;
 }
 
-static int write_buffer(int fd, off_t offset, const void *buf, size_t size)
+static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size)
 {
     size_t written_size;
     MakedumpfileDataHeader mdh;
@@ -857,12 +857,12 @@ static int write_buffer(int fd, off_t offset, const void *buf, size_t size)
     mdh.offset = cpu_to_be64(offset);
     mdh.buf_size = cpu_to_be64(size);
 
-    written_size = qemu_write_full(fd, &mdh, sizeof(mdh));
+    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
     if (written_size != sizeof(mdh)) {
         return -1;
     }
 
-    written_size = qemu_write_full(fd, buf, size);
+    written_size = qemu_write_full(s->fd, buf, size);
     if (written_size != size) {
         return -1;
     }
@@ -982,7 +982,7 @@ static void create_header32(DumpState *s, Error **errp)
 #endif
     dh->status = cpu_to_dump32(s, status);
 
-    if (write_buffer(s->fd, 0, dh, size) < 0) {
+    if (write_buffer(s, 0, dh, size) < 0) {
         error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
@@ -1012,7 +1012,7 @@ static void create_header32(DumpState *s, Error **errp)
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump32(s, s->note_size);
 
-    if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
+    if (write_buffer(s, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
         error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
@@ -1027,7 +1027,7 @@ static void create_header32(DumpState *s, Error **errp)
     if (*errp) {
         goto out;
     }
-    if (write_buffer(s->fd, offset_note, s->note_buf,
+    if (write_buffer(s, offset_note, s->note_buf,
                      s->note_size) < 0) {
         error_setg(errp, "dump: failed to write notes");
         goto out;
@@ -1093,7 +1093,7 @@ static void create_header64(DumpState *s, Error **errp)
 #endif
     dh->status = cpu_to_dump32(s, status);
 
-    if (write_buffer(s->fd, 0, dh, size) < 0) {
+    if (write_buffer(s, 0, dh, size) < 0) {
         error_setg(errp, "dump: failed to write disk dump header");
         goto out;
     }
@@ -1123,7 +1123,7 @@ static void create_header64(DumpState *s, Error **errp)
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump64(s, s->note_size);
 
-    if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS *
+    if (write_buffer(s, DISKDUMP_HEADER_BLOCKS *
                      block_size, kh, size) < 0) {
         error_setg(errp, "dump: failed to write kdump sub header");
         goto out;
@@ -1139,7 +1139,7 @@ static void create_header64(DumpState *s, Error **errp)
         goto out;
     }
 
-    if (write_buffer(s->fd, offset_note, s->note_buf,
+    if (write_buffer(s, offset_note, s->note_buf,
                      s->note_size) < 0) {
         error_setg(errp, "dump: failed to write notes");
         goto out;
@@ -1204,7 +1204,7 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
     while (old_offset < new_offset) {
         /* calculate the offset and write dump_bitmap */
         offset_bitmap1 = s->offset_dump_bitmap + old_offset;
-        if (write_buffer(s->fd, offset_bitmap1, buf,
+        if (write_buffer(s, offset_bitmap1, buf,
                          bitmap_bufsize) < 0) {
             return -1;
         }
@@ -1212,7 +1212,7 @@ static int set_dump_bitmap(uint64_t last_pfn, uint64_t pfn, bool value,
         /* dump level 1 is chosen, so 1st and 2nd bitmap are same */
         offset_bitmap2 = s->offset_dump_bitmap + s->len_dump_bitmap +
                          old_offset;
-        if (write_buffer(s->fd, offset_bitmap2, buf,
+        if (write_buffer(s, offset_bitmap2, buf,
                          bitmap_bufsize) < 0) {
             return -1;
         }
@@ -1380,7 +1380,7 @@ out:
 static void prepare_data_cache(DataCache *data_cache, DumpState *s,
                                off_t offset)
 {
-    data_cache->fd = s->fd;
+    data_cache->state = s;
     data_cache->data_size = 0;
     data_cache->buf_size = 4 * dump_bitmap_get_bufsize(s);
     data_cache->buf = g_malloc0(data_cache->buf_size);
@@ -1399,11 +1399,11 @@ static int write_cache(DataCache *dc, const void *buf, size_t size,
     /*
      * if flag_sync is set, synchronize data in dc->buf into vmcore.
      * otherwise check if the space is enough for caching data in buf, if not,
-     * write the data in dc->buf to dc->fd and reset dc->buf
+     * write the data in dc->buf to dc->state->fd and reset dc->buf
      */
     if ((!flag_sync && dc->data_size + size > dc->buf_size) ||
         (flag_sync && dc->data_size > 0)) {
-        if (write_buffer(dc->fd, dc->offset, dc->buf, dc->data_size) < 0) {
+        if (write_buffer(dc->state, dc->offset, dc->buf, dc->data_size) < 0) {
             return -1;
         }
 
@@ -1644,7 +1644,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
      *  +------------------------------------------+
      */
 
-    ret = write_start_flat_header(s->fd);
+    ret = write_start_flat_header(s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to write start flat header");
         return;
@@ -1665,7 +1665,7 @@ static void create_kdump_vmcore(DumpState *s, Error **errp)
         return;
     }
 
-    ret = write_end_flat_header(s->fd);
+    ret = write_end_flat_header(s);
     if (ret < 0) {
         error_setg(errp, "dump: failed to write end flat header");
         return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 7008d43d04..e27af8fb34 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -137,7 +137,7 @@ typedef struct QEMU_PACKED KdumpSubHeader64 {
 } KdumpSubHeader64;
 
 typedef struct DataCache {
-    int fd;             /* fd of the file where to write the cached data */
+    DumpState *state;   /* dump state related to this data */
     uint8_t *buf;       /* buffer for cached data */
     size_t buf_size;    /* size of the buf */
     size_t data_size;   /* size of cached data in buf */
-- 
2.39.3

[PATCH v3 qemu 2/3] dump: Allow directly outputting raw kdump format

[Stephen Brennan]

The flattened format (currently output by QEMU) is used by makedumpfile
only when it is outputting a vmcore to a file which is not seekable. The
flattened format functions essentially as a set of instructions of the
form "seek to the given offset, then write the given bytes out".

The flattened format can be reconstructed using makedumpfile -R, or
makedumpfile-R.pl, but it is a slow process because it requires copying
the entire vmcore. The flattened format can also be directly read by
crash, but still, it requires a lengthy reassembly phase.

To sum up, the flattened format is not an ideal one: it should only be
used on files which are actually not seekable. This is the exact
strategy which makedumpfile uses, as seen in the implementation of
"write_buffer()" in makedumpfile [1]. However, QEMU has always used the
flattened format. For compatibility it is best not to change the default
output format without warning. So, add a flag to DumpState which changes
the output to use the normal (i.e. raw) format. This flag will be added
to the QMP and HMP commands in the next change.

[1]: https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 dump/dump.c           | 32 +++++++++++++++++++++++++-------
 include/sysemu/dump.h |  1 +
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 74071a1565..10aa2c79e0 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -814,6 +814,10 @@ static int write_start_flat_header(DumpState *s)
     MakedumpfileHeader *mh;
     int ret = 0;
 
+    if (s->kdump_raw) {
+        return 0;
+    }
+
     QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
     mh = g_malloc0(MAX_SIZE_MDF_HEADER);
 
@@ -837,6 +841,10 @@ static int write_end_flat_header(DumpState *s)
 {
     MakedumpfileDataHeader mdh;
 
+    if (s->kdump_raw) {
+        return 0;
+    }
+
     mdh.offset = END_FLAG_FLAT_HEADER;
     mdh.buf_size = END_FLAG_FLAT_HEADER;
 
@@ -853,13 +861,21 @@ static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size
 {
     size_t written_size;
     MakedumpfileDataHeader mdh;
+    loff_t seek_loc;
 
-    mdh.offset = cpu_to_be64(offset);
-    mdh.buf_size = cpu_to_be64(size);
+    if (s->kdump_raw) {
+        seek_loc = lseek(s->fd, offset, SEEK_SET);
+        if (seek_loc == (off_t) -1) {
+            return -1;
+        }
+    } else {
+        mdh.offset = cpu_to_be64(offset);
+        mdh.buf_size = cpu_to_be64(size);
 
-    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
-    if (written_size != sizeof(mdh)) {
-        return -1;
+        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
+        if (written_size != sizeof(mdh)) {
+            return -1;
+        }
     }
 
     written_size = qemu_write_full(s->fd, buf, size);
@@ -1775,7 +1791,8 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
 
 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)
+                      int64_t begin, int64_t length, bool kdump_raw,
+                      Error **errp)
 {
     ERRP_GUARD();
     VMCoreInfoState *vmci = vmcoreinfo_find();
@@ -1786,6 +1803,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     s->has_format = has_format;
     s->format = format;
     s->written_size = 0;
+    s->kdump_raw = kdump_raw;
 
     /* kdump-compressed is conflict with paging and filter */
     if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
@@ -2168,7 +2186,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     dump_state_prepare(s);
 
     dump_init(s, fd, has_format, format, paging, has_begin,
-              begin, length, errp);
+              begin, length, false, errp);
     if (*errp) {
         qatomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index e27af8fb34..d702854853 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -157,6 +157,7 @@ typedef struct DumpState {
     MemoryMappingList list;
     bool resume;
     bool detached;
+    bool kdump_raw;
     hwaddr memory_offset;
     int fd;
 
-- 
2.39.3

[PATCH v3 qemu 3/3] dump: Add command interface for kdump-raw formats

[Stephen Brennan]

The QMP dump API represents the dump format as an enumeration. Add three
new enumerators, one for each supported kdump compression, each named
"kdump-raw-*".

For the HMP command line, rather than adding a new flag corresponding to
each format, it seems more human-friendly to add a single flag "-R" to
switch the kdump formats to "raw" mode. The choice of "-R" also
correlates nicely to the "makedumpfile -R" option, which would serve to
reassemble a flattened vmcore.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 dump/dump-hmp-cmds.c | 21 +++++++++++++++++----
 dump/dump.c          | 31 ++++++++++++++++++++++++++++++-
 hmp-commands.hx      |  9 +++++++--
 qapi/dump.json       | 24 ++++++++++++++++++++----
 4 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
index b038785fee..b428ec33df 100644
--- a/dump/dump-hmp-cmds.c
+++ b/dump/dump-hmp-cmds.c
@@ -19,6 +19,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
     bool paging = qdict_get_try_bool(qdict, "paging", false);
     bool zlib = qdict_get_try_bool(qdict, "zlib", false);
     bool lzo = qdict_get_try_bool(qdict, "lzo", false);
+    bool raw = qdict_get_try_bool(qdict, "raw", false);
     bool snappy = qdict_get_try_bool(qdict, "snappy", false);
     const char *file = qdict_get_str(qdict, "filename");
     bool has_begin = qdict_haskey(qdict, "begin");
@@ -40,16 +41,28 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
         dump_format = DUMP_GUEST_MEMORY_FORMAT_WIN_DMP;
     }
 
-    if (zlib) {
-        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+    if (zlib && raw) {
+        if (raw) {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB;
+        } else {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+        }
     }
 
     if (lzo) {
-        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+        if (raw) {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO;
+        } else {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+        }
     }
 
     if (snappy) {
-        dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+        if (raw) {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY;
+        } else {
+            dump_format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+        }
     }
 
     if (has_begin) {
diff --git a/dump/dump.c b/dump/dump.c
index 10aa2c79e0..55cb6af20b 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -2090,6 +2090,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     int fd = -1;
     DumpState *s;
     bool detach_p = false;
+    bool kdump_raw = false;
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         error_setg(errp, "Dump not allowed during incoming migration.");
@@ -2103,6 +2104,27 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
+    /* externally, we represent kdump-raw-* as separate formats, but internally
+     * they are handled the same, except for the "raw" flag */
+    if (has_format) {
+        switch (format) {
+            case DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB:
+                format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB;
+                kdump_raw = true;
+                break;
+            case DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO:
+                format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO;
+                kdump_raw = true;
+                break;
+            case DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY:
+                format = DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY;
+                kdump_raw = true;
+                break;
+            default:
+                break;
+        }
+    }
+
     /*
      * kdump-compressed format need the whole memory dumped, so paging or
      * filter is not supported here.
@@ -2166,6 +2188,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         error_setg(errp, QERR_INVALID_PARAMETER, "protocol");
         return;
     }
+    if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (loff_t) -1) {
+        error_setg(errp, "kdump-raw formats require a seekable file");
+        return;
+    }
 
     if (!dump_migration_blocker) {
         error_setg(&dump_migration_blocker,
@@ -2186,7 +2212,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     dump_state_prepare(s);
 
     dump_init(s, fd, has_format, format, paging, has_begin,
-              begin, length, false, errp);
+              begin, length, kdump_raw, errp);
     if (*errp) {
         qatomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
@@ -2214,15 +2240,18 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
 
     /* kdump-zlib is always available */
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_ZLIB);
+    QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_ZLIB);
 
     /* add new item if kdump-lzo is available */
 #ifdef CONFIG_LZO
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_LZO);
+    QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_LZO);
 #endif
 
     /* add new item if kdump-snappy is available */
 #ifdef CONFIG_SNAPPY
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_SNAPPY);
+    QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
 #endif
 
     if (win_dump_available(NULL)) {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2cbd0f77a0..69eaa1d6a6 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1085,14 +1085,16 @@ ERST
 
     {
         .name       = "dump-guest-memory",
-        .args_type  = "paging:-p,detach:-d,windmp:-w,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:l?,length:l?",
-        .params     = "[-p] [-d] [-z|-l|-s|-w] filename [begin length]",
+        .args_type  = "paging:-p,detach:-d,windmp:-w,zlib:-z,lzo:-l,snappy:-s,raw:-R,filename:F,begin:l?,length:l?",
+        .params     = "[-p] [-d] [-z|-l|-s|-w] [-R] filename [begin length]",
         .help       = "dump guest memory into file 'filename'.\n\t\t\t"
                       "-p: do paging to get guest's memory mapping.\n\t\t\t"
                       "-d: return immediately (do not wait for completion).\n\t\t\t"
                       "-z: dump in kdump-compressed format, with zlib compression.\n\t\t\t"
                       "-l: dump in kdump-compressed format, with lzo compression.\n\t\t\t"
                       "-s: dump in kdump-compressed format, with snappy compression.\n\t\t\t"
+                      "-R: when using kdump (-z, -l, -s), use raw rather than makedumpfile-flattened\n\t\t\t"
+                      "    format\n\t\t\t"
                       "-w: dump in Windows crashdump format (can be used instead of ELF-dump converting),\n\t\t\t"
                       "    for Windows x86 and x64 guests with vmcoreinfo driver only.\n\t\t\t"
                       "begin: the starting physical address.\n\t\t\t"
@@ -1115,6 +1117,9 @@ SRST
     dump in kdump-compressed format, with lzo compression.
   ``-s``
     dump in kdump-compressed format, with snappy compression.
+  ``-R``
+    when using kdump (-z, -l, -s), use raw rather than makedumpfile-flattened
+    format
   ``-w``
     dump in Windows crashdump format (can be used instead of ELF-dump converting),
     for Windows x64 guests with vmcoreinfo driver only
diff --git a/qapi/dump.json b/qapi/dump.json
index 4ae1f722a9..5cbc237ad9 100644
--- a/qapi/dump.json
+++ b/qapi/dump.json
@@ -15,11 +15,23 @@
 #
 # @elf: elf format
 #
-# @kdump-zlib: kdump-compressed format with zlib-compressed
+# @kdump-zlib: makedumpfile flattened, kdump-compressed format with zlib
+#     compression
 #
-# @kdump-lzo: kdump-compressed format with lzo-compressed
+# @kdump-lzo: makedumpfile flattened, kdump-compressed format with lzo
+#     compression
 #
-# @kdump-snappy: kdump-compressed format with snappy-compressed
+# @kdump-snappy: makedumpfile flattened, kdump-compressed format with snappy
+#     compression
+#
+# @kdump-raw-zlib: raw assembled kdump-compressed format with zlib compression
+#     (since 8.2)
+#
+# @kdump-raw-lzo: raw assembled kdump-compressed format with lzo compression
+#     (since 8.2)
+#
+# @kdump-raw-snappy: raw assembled kdump-compressed format with snappy
+#     compression (since 8.2)
 #
 # @win-dmp: Windows full crashdump format, can be used instead of ELF
 #     converting (since 2.13)
@@ -27,7 +39,11 @@
 # Since: 2.0
 ##
 { 'enum': 'DumpGuestMemoryFormat',
-  'data': [ 'elf', 'kdump-zlib', 'kdump-lzo', 'kdump-snappy', 'win-dmp' ] }
+  'data': [
+      'elf',
+      'kdump-zlib', 'kdump-lzo', 'kdump-snappy',
+      'kdump-raw-zlib', 'kdump-raw-lzo', 'kdump-raw-snappy',
+      'win-dmp' ] }
 
 ##
 # @dump-guest-memory:
-- 
2.39.3

Re: [PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format

[Marc-André Lureau]

Hi

On Tue, Sep 19, 2023 at 3:32 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> Hello all,
>
> This is the third version of the kdump patch series, the first and
> second revisions being visible at [1] and [2] respectively. You can see
> the history and motivation for the patch series described in the cover
> letter of [2].
>
> Thank you for your continued feedback and review.
> Stephen
>
> Changes from v2 to v3:
> - Rather than use "reassembled" flag in QMP API, represent each kdump
>   format "kdump-X" with a new enumerator "kdump-raw-X". (The HMP
>   interface retains the -R option)
> - Return an error if the file descriptor passed in is not seekable, yet
>   the requested dump format is kdump-raw-*
>
> Changes from v1 to v2:
> - Keep the default as the flattened format
> - Add QMP / HMP arguments for "reassembled"
>
> [1]: https://lore.kernel.org/qemu-devel/20230717163855.7383-1-stephen.s.brennan@oracle.com/
> [2]: https://lore.kernel.org/qemu-devel/20230914010315.945705-1-stephen.s.brennan@oracle.com/
>
> Stephen Brennan (3):
>   dump: Pass DumpState to write_ functions
>   dump: Allow directly outputting raw kdump format
>   dump: Add command interface for kdump-raw formats
>
>  dump/dump-hmp-cmds.c  | 21 +++++++--
>  dump/dump.c           | 99 +++++++++++++++++++++++++++++++------------
>  hmp-commands.hx       |  9 +++-
>  include/sysemu/dump.h |  3 +-
>  qapi/dump.json        | 24 +++++++++--
>  5 files changed, 119 insertions(+), 37 deletions(-)
>
> --
> 2.39.3
>

For the series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format

[Daniel P. Berrangé]

On Tue, Sep 19, 2023 at 11:53:49AM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 19, 2023 at 3:32 AM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
> >
> > Hello all,
> >
> > This is the third version of the kdump patch series, the first and
> > second revisions being visible at [1] and [2] respectively. You can see
> > the history and motivation for the patch series described in the cover
> > letter of [2].
> >
> > Thank you for your continued feedback and review.
> > Stephen
> >
> > Changes from v2 to v3:
> > - Rather than use "reassembled" flag in QMP API, represent each kdump
> >   format "kdump-X" with a new enumerator "kdump-raw-X". (The HMP
> >   interface retains the -R option)
> > - Return an error if the file descriptor passed in is not seekable, yet
> >   the requested dump format is kdump-raw-*
> >
> > Changes from v1 to v2:
> > - Keep the default as the flattened format
> > - Add QMP / HMP arguments for "reassembled"
> >
> > [1]: https://lore.kernel.org/qemu-devel/20230717163855.7383-1-stephen.s.brennan@oracle.com/
> > [2]: https://lore.kernel.org/qemu-devel/20230914010315.945705-1-stephen.s.brennan@oracle.com/
> >
> > Stephen Brennan (3):
> >   dump: Pass DumpState to write_ functions
> >   dump: Allow directly outputting raw kdump format
> >   dump: Add command interface for kdump-raw formats
> >
> >  dump/dump-hmp-cmds.c  | 21 +++++++--
> >  dump/dump.c           | 99 +++++++++++++++++++++++++++++++------------
> >  hmp-commands.hx       |  9 +++-
> >  include/sysemu/dump.h |  3 +-
> >  qapi/dump.json        | 24 +++++++++--
> >  5 files changed, 119 insertions(+), 37 deletions(-)
> >
> > --
> > 2.39.3
> >
> 
> For the series:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format

[Stephen Brennan]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Tue, Sep 19, 2023 at 3:32 AM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> Hello all,
>>
>> This is the third version of the kdump patch series, the first and
>> second revisions being visible at [1] and [2] respectively. You can see
>> the history and motivation for the patch series described in the cover
>> letter of [2].
>>
>> Thank you for your continued feedback and review.
>> Stephen
>>
>> Changes from v2 to v3:
>> - Rather than use "reassembled" flag in QMP API, represent each kdump
>>   format "kdump-X" with a new enumerator "kdump-raw-X". (The HMP
>>   interface retains the -R option)
>> - Return an error if the file descriptor passed in is not seekable, yet
>>   the requested dump format is kdump-raw-*
>>
>> Changes from v1 to v2:
>> - Keep the default as the flattened format
>> - Add QMP / HMP arguments for "reassembled"
>>
>> [1]: https://lore.kernel.org/qemu-devel/20230717163855.7383-1-stephen.s.brennan@oracle.com/
>> [2]: https://lore.kernel.org/qemu-devel/20230914010315.945705-1-stephen.s.brennan@oracle.com/
>>
>> Stephen Brennan (3):
>>   dump: Pass DumpState to write_ functions
>>   dump: Allow directly outputting raw kdump format
>>   dump: Add command interface for kdump-raw formats
>>
>>  dump/dump-hmp-cmds.c  | 21 +++++++--
>>  dump/dump.c           | 99 +++++++++++++++++++++++++++++++------------
>>  hmp-commands.hx       |  9 +++-
>>  include/sysemu/dump.h |  3 +-
>>  qapi/dump.json        | 24 +++++++++--
>>  5 files changed, 119 insertions(+), 37 deletions(-)
>>
>> --
>> 2.39.3
>>
>
> For the series:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> -- 
> Marc-André Lureau

Hello,

Will this be queued for version 8.2? I see here[1] that the feature
freeze is November 7. I'm not really familiar with QEMU development so I
didn't know if there was a "linux-next" equivalent I could check.

[1]: https://wiki.qemu.org/Planning/8.2

Thanks,
Stephen

Re: [PATCH v3 qemu 0/3] Allow dump-guest-memory to output standard kdump format

[Marc-André Lureau]

Hi

On Thu, Oct 26, 2023 at 2:44 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
> Hello,
>
> Will this be queued for version 8.2? I see here[1] that the feature
> freeze is November 7. I'm not really familiar with QEMU development so I
> didn't know if there was a "linux-next" equivalent I could check.
>
> [1]: https://wiki.qemu.org/Planning/8.2

I'll prepare a PR.

thanks

-- 
Marc-André Lureau

Re: [PATCH v3 qemu 2/3] dump: Allow directly outputting raw kdump format

[Marc-André Lureau]

Hi Stephen

On Tue, Sep 19, 2023 at 3:32 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> The flattened format (currently output by QEMU) is used by makedumpfile
> only when it is outputting a vmcore to a file which is not seekable. The
> flattened format functions essentially as a set of instructions of the
> form "seek to the given offset, then write the given bytes out".
>
> The flattened format can be reconstructed using makedumpfile -R, or
> makedumpfile-R.pl, but it is a slow process because it requires copying
> the entire vmcore. The flattened format can also be directly read by
> crash, but still, it requires a lengthy reassembly phase.
>
> To sum up, the flattened format is not an ideal one: it should only be
> used on files which are actually not seekable. This is the exact
> strategy which makedumpfile uses, as seen in the implementation of
> "write_buffer()" in makedumpfile [1]. However, QEMU has always used the
> flattened format. For compatibility it is best not to change the default
> output format without warning. So, add a flag to DumpState which changes
> the output to use the normal (i.e. raw) format. This flag will be added
> to the QMP and HMP commands in the next change.
>
> [1]: https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
> ---
>  dump/dump.c           | 32 +++++++++++++++++++++++++-------
>  include/sysemu/dump.h |  1 +
>  2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 74071a1565..10aa2c79e0 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -814,6 +814,10 @@ static int write_start_flat_header(DumpState *s)
>      MakedumpfileHeader *mh;
>      int ret = 0;
>
> +    if (s->kdump_raw) {
> +        return 0;
> +    }
> +
>      QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
>      mh = g_malloc0(MAX_SIZE_MDF_HEADER);
>
> @@ -837,6 +841,10 @@ static int write_end_flat_header(DumpState *s)
>  {
>      MakedumpfileDataHeader mdh;
>
> +    if (s->kdump_raw) {
> +        return 0;
> +    }
> +
>      mdh.offset = END_FLAG_FLAT_HEADER;
>      mdh.buf_size = END_FLAG_FLAT_HEADER;
>
> @@ -853,13 +861,21 @@ static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size
>  {
>      size_t written_size;
>      MakedumpfileDataHeader mdh;
> +    loff_t seek_loc;

Any reason to use loff_t over off_t here? It fails to compile on win32
for ex. I can touch on PR commit otherwise.

>
> -    mdh.offset = cpu_to_be64(offset);
> -    mdh.buf_size = cpu_to_be64(size);
> +    if (s->kdump_raw) {
> +        seek_loc = lseek(s->fd, offset, SEEK_SET);
> +        if (seek_loc == (off_t) -1) {
> +            return -1;
> +        }
> +    } else {
> +        mdh.offset = cpu_to_be64(offset);
> +        mdh.buf_size = cpu_to_be64(size);
>
> -    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
> -    if (written_size != sizeof(mdh)) {
> -        return -1;
> +        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
> +        if (written_size != sizeof(mdh)) {
> +            return -1;
> +        }
>      }
>
>      written_size = qemu_write_full(s->fd, buf, size);
> @@ -1775,7 +1791,8 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
>
>  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)
> +                      int64_t begin, int64_t length, bool kdump_raw,
> +                      Error **errp)
>  {
>      ERRP_GUARD();
>      VMCoreInfoState *vmci = vmcoreinfo_find();
> @@ -1786,6 +1803,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      s->has_format = has_format;
>      s->format = format;
>      s->written_size = 0;
> +    s->kdump_raw = kdump_raw;
>
>      /* kdump-compressed is conflict with paging and filter */
>      if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> @@ -2168,7 +2186,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>      dump_state_prepare(s);
>
>      dump_init(s, fd, has_format, format, paging, has_begin,
> -              begin, length, errp);
> +              begin, length, false, errp);
>      if (*errp) {
>          qatomic_set(&s->status, DUMP_STATUS_FAILED);
>          return;
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index e27af8fb34..d702854853 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -157,6 +157,7 @@ typedef struct DumpState {
>      MemoryMappingList list;
>      bool resume;
>      bool detached;
> +    bool kdump_raw;
>      hwaddr memory_offset;
>      int fd;
>
> --
> 2.39.3
>


-- 
Marc-André Lureau

Re: [PATCH v3 qemu 2/3] dump: Allow directly outputting raw kdump format

[Stephen Brennan]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi Stephen
>
> On Tue, Sep 19, 2023 at 3:32 AM Stephen Brennan
> <stephen.s.brennan@oracle.com> wrote:
>>
>> The flattened format (currently output by QEMU) is used by makedumpfile
>> only when it is outputting a vmcore to a file which is not seekable. The
>> flattened format functions essentially as a set of instructions of the
>> form "seek to the given offset, then write the given bytes out".
>>
>> The flattened format can be reconstructed using makedumpfile -R, or
>> makedumpfile-R.pl, but it is a slow process because it requires copying
>> the entire vmcore. The flattened format can also be directly read by
>> crash, but still, it requires a lengthy reassembly phase.
>>
>> To sum up, the flattened format is not an ideal one: it should only be
>> used on files which are actually not seekable. This is the exact
>> strategy which makedumpfile uses, as seen in the implementation of
>> "write_buffer()" in makedumpfile [1]. However, QEMU has always used the
>> flattened format. For compatibility it is best not to change the default
>> output format without warning. So, add a flag to DumpState which changes
>> the output to use the normal (i.e. raw) format. This flag will be added
>> to the QMP and HMP commands in the next change.
>>
>> [1]: https://github.com/makedumpfile/makedumpfile/blob/f23bb943568188a2746dbf9b6692668f5a2ac3b6/makedumpfile.c#L5008-L5040
>>
>> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
>> ---
>>  dump/dump.c           | 32 +++++++++++++++++++++++++-------
>>  include/sysemu/dump.h |  1 +
>>  2 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 74071a1565..10aa2c79e0 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -814,6 +814,10 @@ static int write_start_flat_header(DumpState *s)
>>      MakedumpfileHeader *mh;
>>      int ret = 0;
>>
>> +    if (s->kdump_raw) {
>> +        return 0;
>> +    }
>> +
>>      QEMU_BUILD_BUG_ON(sizeof *mh > MAX_SIZE_MDF_HEADER);
>>      mh = g_malloc0(MAX_SIZE_MDF_HEADER);
>>
>> @@ -837,6 +841,10 @@ static int write_end_flat_header(DumpState *s)
>>  {
>>      MakedumpfileDataHeader mdh;
>>
>> +    if (s->kdump_raw) {
>> +        return 0;
>> +    }
>> +
>>      mdh.offset = END_FLAG_FLAT_HEADER;
>>      mdh.buf_size = END_FLAG_FLAT_HEADER;
>>
>> @@ -853,13 +861,21 @@ static int write_buffer(DumpState *s, off_t offset, const void *buf, size_t size
>>  {
>>      size_t written_size;
>>      MakedumpfileDataHeader mdh;
>> +    loff_t seek_loc;
>
> Any reason to use loff_t over off_t here? It fails to compile on win32
> for ex. I can touch on PR commit otherwise.

I think that was an oversight on my part: lseek() uses off_t.

I see that qemu is compiled with _FILE_OFFSET_BITS=64 so off_t should be
64 bits even on 32-bit architectures. So this should be off_t. I believe
the compile error would also happen in qmp_dump_guest_memory() where I
have:

    if (kdump_raw && lseek(fd, 0, SEEK_CUR) == (loff_t) -1) {

If it's easiest for you to tweak it in the PR, please do. Otherwise I
can respin the series replacing loff_t with off_t.

Thank you,
Stephen

>>
>> -    mdh.offset = cpu_to_be64(offset);
>> -    mdh.buf_size = cpu_to_be64(size);
>> +    if (s->kdump_raw) {
>> +        seek_loc = lseek(s->fd, offset, SEEK_SET);
>> +        if (seek_loc == (off_t) -1) {
>> +            return -1;
>> +        }
>> +    } else {
>> +        mdh.offset = cpu_to_be64(offset);
>> +        mdh.buf_size = cpu_to_be64(size);
>>
>> -    written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
>> -    if (written_size != sizeof(mdh)) {
>> -        return -1;
>> +        written_size = qemu_write_full(s->fd, &mdh, sizeof(mdh));
>> +        if (written_size != sizeof(mdh)) {
>> +            return -1;
>> +        }
>>      }
>>
>>      written_size = qemu_write_full(s->fd, buf, size);
>> @@ -1775,7 +1791,8 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
>>
>>  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)
>> +                      int64_t begin, int64_t length, bool kdump_raw,
>> +                      Error **errp)
>>  {
>>      ERRP_GUARD();
>>      VMCoreInfoState *vmci = vmcoreinfo_find();
>> @@ -1786,6 +1803,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>>      s->has_format = has_format;
>>      s->format = format;
>>      s->written_size = 0;
>> +    s->kdump_raw = kdump_raw;
>>
>>      /* kdump-compressed is conflict with paging and filter */
>>      if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> @@ -2168,7 +2186,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
>>      dump_state_prepare(s);
>>
>>      dump_init(s, fd, has_format, format, paging, has_begin,
>> -              begin, length, errp);
>> +              begin, length, false, errp);
>>      if (*errp) {
>>          qatomic_set(&s->status, DUMP_STATUS_FAILED);
>>          return;
>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index e27af8fb34..d702854853 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -157,6 +157,7 @@ typedef struct DumpState {
>>      MemoryMappingList list;
>>      bool resume;
>>      bool detached;
>> +    bool kdump_raw;
>>      hwaddr memory_offset;
>>      int fd;
>>
>> --
>> 2.39.3
>>
>
>
> -- 
> Marc-André Lureau