[PATCH] dump: enhance win_dump_available to report properly

[PATCH] dump: enhance win_dump_available to report properly

[Nikolai Barybin]

QMP query-dump-guest-memory-capability reports win dump as available for
any x86 VM, which is false.

This patch implements proper query of vmcoreinfo and calculation of
guest note size. Based on that we can surely report whether win dump
available or not.

For further reference one may review this libvirt discussion:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
[PATCH 0/4] Allow xml-configured coredump format on VM crash

Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
---
 dump/win_dump.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/dump/win_dump.c b/dump/win_dump.c
index 3162e8bd48..4bb1b28e63 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -14,14 +14,74 @@
 #include "qemu/error-report.h"
 #include "exec/cpu-defs.h"
 #include "hw/core/cpu.h"
+#include "hw/misc/vmcoreinfo.h"
 #include "qemu/win_dump_defs.h"
 #include "win_dump.h"
 #include "cpu.h"
+#include "elf.h"
 
 #if defined(TARGET_X86_64)
 
+#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
+    ((DIV_ROUND_UP((hdr_size), 4) +                     \
+      DIV_ROUND_UP((name_size), 4) +                    \
+      DIV_ROUND_UP((desc_size), 4)) * 4)
+
 bool win_dump_available(Error **errp)
 {
+    uint64_t addr, note_head_size, name_size, desc_size;
+    uint32_t size;
+    uint16_t guest_format;
+    uint8_t *guest_note = NULL;
+    size_t guest_note_size = 0;
+    VMCoreInfoState *vmci = vmcoreinfo_find();
+    ArchDumpInfo dump_info = {};
+    GuestPhysBlockList blocks = {};
+    int ret;
+
+    if (!vmci || !vmci->has_vmcoreinfo)
+        return false;
+
+    ret = cpu_get_dump_info(&dump_info, &blocks);
+    if (ret < 0)
+        return false;
+
+    guest_format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
+    if (guest_format != FW_CFG_VMCOREINFO_FORMAT_ELF)
+        return false;
+
+    size = le32_to_cpu(vmci->vmcoreinfo.size);
+    addr = le64_to_cpu(vmci->vmcoreinfo.paddr);
+    note_head_size = dump_info.d_class == ELFCLASS64 ?
+        sizeof(Elf64_Nhdr) : sizeof(Elf32_Nhdr);
+
+    guest_note = g_malloc(size + 1);
+    cpu_physical_memory_read(addr, guest_note, size);
+    if (dump_info.d_class == ELFCLASS64) {
+        const Elf64_Nhdr *hdr = (void *)guest_note;
+        if (dump_info.d_endian == ELFDATA2LSB) {
+            name_size = cpu_to_le64(hdr->n_namesz);
+            desc_size = cpu_to_le64(hdr->n_descsz);
+        } else {
+            name_size = cpu_to_be64(hdr->n_namesz);
+            desc_size = cpu_to_be64(hdr->n_descsz);
+        }
+    } else {
+        const Elf32_Nhdr *hdr = (void *)guest_note;
+        if (dump_info.d_endian == ELFDATA2LSB) {
+            name_size = cpu_to_le32(hdr->n_namesz);
+            desc_size = cpu_to_le32(hdr->n_descsz);
+        } else {
+            name_size = cpu_to_be32(hdr->n_namesz);
+            desc_size = cpu_to_be32(hdr->n_descsz);
+        }
+    }
+
+    guest_note_size = ELF_NOTE_SIZE(note_head_size, name_size, desc_size);
+    if (guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64 &&
+        guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32)
+        return false;
+
     return true;
 }
 
-- 
2.47.3

Re: [PATCH] dump: enhance win_dump_available to report properly

[Denis V. Lunev]

On 7/23/25 19:04, Nikolai Barybin wrote:
> QMP query-dump-guest-memory-capability reports win dump as available for
> any x86 VM, which is false.
>
> This patch implements proper query of vmcoreinfo and calculation of
> guest note size. Based on that we can surely report whether win dump
> available or not.
>
> For further reference one may review this libvirt discussion:
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>
> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
> ---
>   dump/win_dump.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
>
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index 3162e8bd48..4bb1b28e63 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -14,14 +14,74 @@
>   #include "qemu/error-report.h"
>   #include "exec/cpu-defs.h"
>   #include "hw/core/cpu.h"
> +#include "hw/misc/vmcoreinfo.h"
>   #include "qemu/win_dump_defs.h"
>   #include "win_dump.h"
>   #include "cpu.h"
> +#include "elf.h"
>   
>   #if defined(TARGET_X86_64)
>   
> +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
> +    ((DIV_ROUND_UP((hdr_size), 4) +                     \
> +      DIV_ROUND_UP((name_size), 4) +                    \
> +      DIV_ROUND_UP((desc_size), 4)) * 4)
> +
>   bool win_dump_available(Error **errp)
>   {
> +    uint64_t addr, note_head_size, name_size, desc_size;
> +    uint32_t size;
> +    uint16_t guest_format;
> +    uint8_t *guest_note = NULL;
> +    size_t guest_note_size = 0;
> +    VMCoreInfoState *vmci = vmcoreinfo_find();
> +    ArchDumpInfo dump_info = {};
> +    GuestPhysBlockList blocks = {};
> +    int ret;
> +
> +    if (!vmci || !vmci->has_vmcoreinfo)
> +        return false;
> +
> +    ret = cpu_get_dump_info(&dump_info, &blocks);
> +    if (ret < 0)
> +        return false;
> +
> +    guest_format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
> +    if (guest_format != FW_CFG_VMCOREINFO_FORMAT_ELF)
> +        return false;
> +
> +    size = le32_to_cpu(vmci->vmcoreinfo.size);
> +    addr = le64_to_cpu(vmci->vmcoreinfo.paddr);
> +    note_head_size = dump_info.d_class == ELFCLASS64 ?
> +        sizeof(Elf64_Nhdr) : sizeof(Elf32_Nhdr);
> +
> +    guest_note = g_malloc(size + 1);
> +    cpu_physical_memory_read(addr, guest_note, size);
> +    if (dump_info.d_class == ELFCLASS64) {
> +        const Elf64_Nhdr *hdr = (void *)guest_note;
> +        if (dump_info.d_endian == ELFDATA2LSB) {
> +            name_size = cpu_to_le64(hdr->n_namesz);
> +            desc_size = cpu_to_le64(hdr->n_descsz);
> +        } else {
> +            name_size = cpu_to_be64(hdr->n_namesz);
> +            desc_size = cpu_to_be64(hdr->n_descsz);
> +        }
> +    } else {
> +        const Elf32_Nhdr *hdr = (void *)guest_note;
> +        if (dump_info.d_endian == ELFDATA2LSB) {
> +            name_size = cpu_to_le32(hdr->n_namesz);
> +            desc_size = cpu_to_le32(hdr->n_descsz);
> +        } else {
> +            name_size = cpu_to_be32(hdr->n_namesz);
> +            desc_size = cpu_to_be32(hdr->n_descsz);
> +        }
> +    }
> +
> +    guest_note_size = ELF_NOTE_SIZE(note_head_size, name_size, desc_size);
> +    if (guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64 &&
> +        guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32)
> +        return false;
> +
>       return true;
>   }
>   
please start from ./scripts/check-patch.pl

Re: [PATCH] dump: enhance win_dump_available to report properly

[Denis V. Lunev]

On 7/23/25 19:04, Nikolai Barybin wrote:
> QMP query-dump-guest-memory-capability reports win dump as available for
> any x86 VM, which is false.
>
> This patch implements proper query of vmcoreinfo and calculation of
> guest note size. Based on that we can surely report whether win dump
> available or not.
>
> For further reference one may review this libvirt discussion:
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>
> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
> ---
>   dump/win_dump.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
>
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index 3162e8bd48..4bb1b28e63 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -14,14 +14,74 @@
>   #include "qemu/error-report.h"
>   #include "exec/cpu-defs.h"
>   #include "hw/core/cpu.h"
> +#include "hw/misc/vmcoreinfo.h"
>   #include "qemu/win_dump_defs.h"
>   #include "win_dump.h"
>   #include "cpu.h"
> +#include "elf.h"
>   
>   #if defined(TARGET_X86_64)
>   
> +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
> +    ((DIV_ROUND_UP((hdr_size), 4) +                     \
> +      DIV_ROUND_UP((name_size), 4) +                    \
> +      DIV_ROUND_UP((desc_size), 4)) * 4)
> +
>   bool win_dump_available(Error **errp)
>   {
> +    uint64_t addr, note_head_size, name_size, desc_size;
> +    uint32_t size;
> +    uint16_t guest_format;
> +    uint8_t *guest_note = NULL;
> +    size_t guest_note_size = 0;
> +    VMCoreInfoState *vmci = vmcoreinfo_find();
> +    ArchDumpInfo dump_info = {};
> +    GuestPhysBlockList blocks = {};
> +    int ret;
> +
> +    if (!vmci || !vmci->has_vmcoreinfo)
> +        return false;
> +
> +    ret = cpu_get_dump_info(&dump_info, &blocks);
This will not work IMHO. cpu_get_dump_info tries to operate
with a real guest memory, which is not provided. This means
that guest executable is impossible to find.

For me this is looking like Windows dump could not be enabled
after the patch.

> +    if (ret < 0)
> +        return false;
> +
> +    guest_format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
> +    if (guest_format != FW_CFG_VMCOREINFO_FORMAT_ELF)
> +        return false;
> +
> +    size = le32_to_cpu(vmci->vmcoreinfo.size);
> +    addr = le64_to_cpu(vmci->vmcoreinfo.paddr);
> +    note_head_size = dump_info.d_class == ELFCLASS64 ?
> +        sizeof(Elf64_Nhdr) : sizeof(Elf32_Nhdr);

note_head_size assignment could go to if below saving conditional

> +
> +    guest_note = g_malloc(size + 1);
leaked at the end of the call

> +    cpu_physical_memory_read(addr, guest_note, size);
> +    if (dump_info.d_class == ELFCLASS64) {
> +        const Elf64_Nhdr *hdr = (void *)guest_note;
> +        if (dump_info.d_endian == ELFDATA2LSB) {
> +            name_size = cpu_to_le64(hdr->n_namesz);
> +            desc_size = cpu_to_le64(hdr->n_descsz);
you are going to use these values as integers below,
thus it is required to do conversion from the guest
endianity to the local one, i.e. le64_to_cpu


> +        } else {
> +            name_size = cpu_to_be64(hdr->n_namesz);
> +            desc_size = cpu_to_be64(hdr->n_descsz);
same as above

> +        }
> +    } else {
> +        const Elf32_Nhdr *hdr = (void *)guest_note;
> +        if (dump_info.d_endian == ELFDATA2LSB) {
> +            name_size = cpu_to_le32(hdr->n_namesz);
> +            desc_size = cpu_to_le32(hdr->n_descsz);
same as above

> +        } else {
> +            name_size = cpu_to_be32(hdr->n_namesz);
> +            desc_size = cpu_to_be32(hdr->n_descsz);
same as above

> +        }
> +    }
> +
> +    guest_note_size = ELF_NOTE_SIZE(note_head_size, name_size, desc_size);
> +    if (guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64 &&
> +        guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32)
> +        return false;
> +
personally I prefer
return guest_note_size == VMCOREINFO_WIN_DUMP_NOTE_SIZE64 || 
guest_note_size == VMCOREINFO_WIN_DUMP_NOTE_SIZE32;

>       return true;
>   }
>   

Re: [PATCH] dump: enhance win_dump_available to report properly

[Denis V. Lunev]

On 7/23/25 19:31, Denis V. Lunev wrote:
> On 7/23/25 19:04, Nikolai Barybin wrote:
>> QMP query-dump-guest-memory-capability reports win dump as available for
>> any x86 VM, which is false.
>>
>> This patch implements proper query of vmcoreinfo and calculation of
>> guest note size. Based on that we can surely report whether win dump
>> available or not.
>>
>> For further reference one may review this libvirt discussion:
>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB 
>>
>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>>
>> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
>> ---
>>   dump/win_dump.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/dump/win_dump.c b/dump/win_dump.c
>> index 3162e8bd48..4bb1b28e63 100644
>> --- a/dump/win_dump.c
>> +++ b/dump/win_dump.c
>> @@ -14,14 +14,74 @@
>>   #include "qemu/error-report.h"
>>   #include "exec/cpu-defs.h"
>>   #include "hw/core/cpu.h"
>> +#include "hw/misc/vmcoreinfo.h"
>>   #include "qemu/win_dump_defs.h"
>>   #include "win_dump.h"
>>   #include "cpu.h"
>> +#include "elf.h"
>>     #if defined(TARGET_X86_64)
>>   +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
>> +    ((DIV_ROUND_UP((hdr_size), 4) +                     \
>> +      DIV_ROUND_UP((name_size), 4) +                    \
>> +      DIV_ROUND_UP((desc_size), 4)) * 4)
>> +
>>   bool win_dump_available(Error **errp)
>>   {
>> +    uint64_t addr, note_head_size, name_size, desc_size;
>> +    uint32_t size;
>> +    uint16_t guest_format;
>> +    uint8_t *guest_note = NULL;
>> +    size_t guest_note_size = 0;
>> +    VMCoreInfoState *vmci = vmcoreinfo_find();
>> +    ArchDumpInfo dump_info = {};
>> +    GuestPhysBlockList blocks = {};
>> +    int ret;
>> +
>> +    if (!vmci || !vmci->has_vmcoreinfo)
>> +        return false;
>> +
>> +    ret = cpu_get_dump_info(&dump_info, &blocks);
> This will not work IMHO. cpu_get_dump_info tries to operate
> with a real guest memory, which is not provided. This means
> that guest executable is impossible to find.
>
> For me this is looking like Windows dump could not be enabled
> after the patch.
>
Correction: current code will not work on 32bit guest with
more than 4Gb RAM on i386. More problems are definitely
possible on other platforms.

Den

Re: [PATCH] dump: enhance win_dump_available to report properly

[Daniel P. Berrangé]

On Wed, Jul 23, 2025 at 08:04:02PM +0300, Nikolai Barybin wrote:
> QMP query-dump-guest-memory-capability reports win dump as available for
> any x86 VM, which is false.
> 
> This patch implements proper query of vmcoreinfo and calculation of
> guest note size. Based on that we can surely report whether win dump
> available or not.
> 
> For further reference one may review this libvirt discussion:
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
> 
> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
> ---
>  dump/win_dump.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/dump/win_dump.c b/dump/win_dump.c
> index 3162e8bd48..4bb1b28e63 100644
> --- a/dump/win_dump.c
> +++ b/dump/win_dump.c
> @@ -14,14 +14,74 @@
>  #include "qemu/error-report.h"
>  #include "exec/cpu-defs.h"
>  #include "hw/core/cpu.h"
> +#include "hw/misc/vmcoreinfo.h"
>  #include "qemu/win_dump_defs.h"
>  #include "win_dump.h"
>  #include "cpu.h"
> +#include "elf.h"
>  
>  #if defined(TARGET_X86_64)
>  
> +#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
> +    ((DIV_ROUND_UP((hdr_size), 4) +                     \
> +      DIV_ROUND_UP((name_size), 4) +                    \
> +      DIV_ROUND_UP((desc_size), 4)) * 4)
> +
>  bool win_dump_available(Error **errp)
>  {
> +    uint64_t addr, note_head_size, name_size, desc_size;
> +    uint32_t size;
> +    uint16_t guest_format;
> +    uint8_t *guest_note = NULL;
> +    size_t guest_note_size = 0;
> +    VMCoreInfoState *vmci = vmcoreinfo_find();
> +    ArchDumpInfo dump_info = {};
> +    GuestPhysBlockList blocks = {};
> +    int ret;
> +
> +    if (!vmci || !vmci->has_vmcoreinfo)
> +        return false;
> +
> +    ret = cpu_get_dump_info(&dump_info, &blocks);
> +    if (ret < 0)
> +        return false;
> +
> +    guest_format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
> +    if (guest_format != FW_CFG_VMCOREINFO_FORMAT_ELF)
> +        return false;
> +
> +    size = le32_to_cpu(vmci->vmcoreinfo.size);
> +    addr = le64_to_cpu(vmci->vmcoreinfo.paddr);
> +    note_head_size = dump_info.d_class == ELFCLASS64 ?
> +        sizeof(Elf64_Nhdr) : sizeof(Elf32_Nhdr);
> +
> +    guest_note = g_malloc(size + 1);
> +    cpu_physical_memory_read(addr, guest_note, size);
> +    if (dump_info.d_class == ELFCLASS64) {
> +        const Elf64_Nhdr *hdr = (void *)guest_note;
> +        if (dump_info.d_endian == ELFDATA2LSB) {
> +            name_size = cpu_to_le64(hdr->n_namesz);
> +            desc_size = cpu_to_le64(hdr->n_descsz);
> +        } else {
> +            name_size = cpu_to_be64(hdr->n_namesz);
> +            desc_size = cpu_to_be64(hdr->n_descsz);
> +        }
> +    } else {
> +        const Elf32_Nhdr *hdr = (void *)guest_note;
> +        if (dump_info.d_endian == ELFDATA2LSB) {
> +            name_size = cpu_to_le32(hdr->n_namesz);
> +            desc_size = cpu_to_le32(hdr->n_descsz);
> +        } else {
> +            name_size = cpu_to_be32(hdr->n_namesz);
> +            desc_size = cpu_to_be32(hdr->n_descsz);
> +        }
> +    }
> +
> +    guest_note_size = ELF_NOTE_SIZE(note_head_size, name_size, desc_size);

It feels like there is overlap between what this method has to do upto
here, with what the existing 'dump_init' has to do. Any possibility to
have a common helper to share logic ?

> +    if (guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64 &&
> +        guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32)
> +        return false;

This dupes a check in create_win_dump, but  misses the extra sanity
check from check_header. I think we should move the guest_note_size
check out of 'create_win_dump' and into 'check_header', then call
that from this code.


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 :|

[PATCH] dump: enhance win_dump_available to report properly

[Nikolai Barybin]

QMP query-dump-guest-memory-capability reports win dump as available for
any x86 VM, which is false.

This patch implements proper query of vmcoreinfo and calculation of
guest note size. Based on that we can surely report whether win dump
available or not.

To perform this I suggest to split dump_init() into dump_preinit() and
dump_init_complete() to avoid exausting copypaste in
win_dump_available().

For further reference one may review this libvirt discussion:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
[PATCH 0/4] Allow xml-configured coredump format on VM crash

Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
---
During first series discussion Den mentions that that code will not work
on 32bit guest with more than 4Gb RAM on i386.

This issue required even more code duplication between dump_init() and
win_dump_available() which we'd like to avoid as mentioned by Daniel.

Hence I present 2nd version of this fix:
 - split dump_init() into dump_preinit() and dump_init_complete()
 - pass pre-inited dump structure with calculated guest note size to
   win_dump_available()
 - call header check and guest note size check inside
   win_dump_available()
---
 dump/dump.c     | 129 ++++++++++++++++++++++++++++++++------------------------
 dump/win_dump.c |  23 ++++++++--
 dump/win_dump.h |   2 +-
 3 files changed, 95 insertions(+), 59 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 15bbcc0c6192822cf920fcb7d60eb7d2cfad0952..19341fa42feef4d1c50dbb3a892ded59a3468d20 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1777,10 +1777,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
     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, bool kdump_raw,
-                      Error **errp)
+static void dump_preinit(DumpState *s, Error **errp)
 {
     ERRP_GUARD();
     VMCoreInfoState *vmci = vmcoreinfo_find();
@@ -1788,16 +1785,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     int nr_cpus;
     int ret;
 
-    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) {
-        assert(!paging && !has_filter);
-    }
-
     if (runstate_is_running()) {
         vm_stop(RUN_STATE_SAVE_VM);
         s->resume = true;
@@ -1814,41 +1801,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         nr_cpus++;
     }
 
-    s->fd = fd;
-    if (has_filter && !length) {
-        error_setg(errp, "parameter 'length' expects a non-zero size");
-        goto cleanup;
-    }
-    s->filter_area_begin = begin;
-    s->filter_area_length = length;
-
-    /* First index is 0, it's the special null name */
-    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
-    /*
-     * Allocate the null name, due to the clearing option set to true
-     * it will be 0.
-     */
-    g_array_set_size(s->string_table_buf, 1);
-
     memory_mapping_list_init(&s->list);
-
     guest_phys_blocks_init(&s->guest_phys_blocks);
     guest_phys_blocks_append(&s->guest_phys_blocks);
-    s->total_size = dump_calculate_size(s);
-#ifdef DEBUG_DUMP_GUEST_MEMORY
-    fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
-#endif
 
-    /* it does not make sense to dump non-existent memory */
-    if (!s->total_size) {
-        error_setg(errp, "dump: no guest memory to dump");
-        goto cleanup;
-    }
-
-    /* get dump info: endian, class and architecture.
-     * If the target architecture is not supported, cpu_get_dump_info() will
-     * return -1.
-     */
     ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
     if (ret < 0) {
         error_setg(errp,
@@ -1906,6 +1862,56 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         }
     }
 
+    s->nr_cpus = nr_cpus;
+    return;
+cleanup:
+    dump_cleanup(s);
+}
+
+static void dump_init_complete(DumpState *s, int fd, bool has_format,
+                               DumpGuestMemoryFormat format, bool paging,
+                               bool has_filter, int64_t begin, int64_t length,
+                               bool kdump_raw, Error **errp)
+{
+    ERRP_GUARD();
+
+    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) {
+        assert(!paging && !has_filter);
+    }
+
+    s->fd = fd;
+    if (has_filter && !length) {
+        error_setg(errp, "parameter 'length' expects a non-zero size");
+        goto cleanup;
+    }
+    s->filter_area_begin = begin;
+    s->filter_area_length = length;
+
+    /* First index is 0, it's the special null name */
+    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
+    /*
+     * Allocate the null name, due to the clearing option set to true
+     * it will be 0.
+     */
+    g_array_set_size(s->string_table_buf, 1);
+
+    s->total_size = dump_calculate_size(s);
+#ifdef DEBUG_DUMP_GUEST_MEMORY
+    fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
+#endif
+
+    /* it does not make sense to dump non-existent memory */
+    if (!s->total_size) {
+        error_setg(errp, "dump: no guest memory to dump");
+        goto cleanup;
+    }
+
     /* get memory mapping */
     if (paging) {
         qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, errp);
@@ -1916,8 +1922,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         qemu_get_guest_simple_memory_mapping(&s->list, &s->guest_phys_blocks);
     }
 
-    s->nr_cpus = nr_cpus;
-
     get_max_mapnr(s);
 
     uint64_t tmp;
@@ -2147,11 +2151,6 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
     }
 #endif
 
-    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
-        && !win_dump_available(errp)) {
-        return;
-    }
-
     if (strstart(protocol, "fd:", &p)) {
         fd = monitor_get_fd(monitor_cur(), p, errp);
         if (fd == -1) {
@@ -2190,9 +2189,19 @@ void qmp_dump_guest_memory(bool paging, const char *protocol,
 
     s = &dump_state_global;
     dump_state_prepare(s);
+    dump_preinit(s, errp);
+    if (*errp) {
+        qatomic_set(&s->status, DUMP_STATUS_FAILED);
+        return;
+    }
+
+    if (has_format && format == DUMP_GUEST_MEMORY_FORMAT_WIN_DMP
+        && !win_dump_available(s, errp)) {
+        return;
+    }
 
-    dump_init(s, fd, has_format, format, paging, has_begin,
-              begin, length, kdump_raw, errp);
+    dump_init_complete(s, fd, has_format, format, paging, has_begin,
+                       begin, length, kdump_raw, errp);
     if (*errp) {
         qatomic_set(&s->status, DUMP_STATUS_FAILED);
         return;
@@ -2215,6 +2224,14 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
                                   g_new0(DumpGuestMemoryCapability, 1);
     DumpGuestMemoryFormatList **tail = &cap->formats;
 
+    DumpState *s = &dump_state_global;
+    dump_state_prepare(s);
+    dump_preinit(s, errp);
+    if (*errp) {
+        qatomic_set(&s->status, DUMP_STATUS_FAILED);
+        return cap;
+    }
+
     /* elf is always available */
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
 
@@ -2234,9 +2251,11 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
     QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
 #endif
 
-    if (win_dump_available(NULL)) {
+    if (win_dump_available(s, NULL)) {
         QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_WIN_DMP);
     }
 
+    dump_cleanup(s);
+    qatomic_set(&s->status, DUMP_STATUS_NONE);
     return cap;
 }
diff --git a/dump/win_dump.c b/dump/win_dump.c
index 3162e8bd48771af8b0e1b1571e40d9a0d888409c..d42427feb22e9cec282129fef4d207342c28205e 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -20,8 +20,25 @@
 
 #if defined(TARGET_X86_64)
 
-bool win_dump_available(Error **errp)
+static bool check_header(WinDumpHeader *h, bool *x64, Error **errp);
+
+bool win_dump_available(DumpState *s, Error **errp)
 {
+    WinDumpHeader *h = (void *)(s->guest_note + VMCOREINFO_ELF_NOTE_HDR_SIZE);
+    Error *local_err = NULL;
+    bool x64 = true;
+
+    if (s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE32 &&
+            s->guest_note_size != VMCOREINFO_WIN_DUMP_NOTE_SIZE64) {
+        error_setg(errp, "win-dump: invalid vmcoreinfo note size");
+        return false;
+    }
+
+    if (!check_header(h, &x64, &local_err)) {
+        error_propagate(errp, local_err);
+        return false;
+    }
+
     return true;
 }
 
@@ -480,7 +497,7 @@ out_cr3:
 
 #else /* !TARGET_X86_64 */
 
-bool win_dump_available(Error **errp)
+bool win_dump_available(DumpState *s, Error **errp)
 {
     error_setg(errp, "Windows dump is only available for x86-64");
 
@@ -489,7 +506,7 @@ bool win_dump_available(Error **errp)
 
 void create_win_dump(DumpState *s, Error **errp)
 {
-    win_dump_available(errp);
+    win_dump_available(s, errp);
 }
 
 #endif
diff --git a/dump/win_dump.h b/dump/win_dump.h
index 9d6cfa47c566503b50db11e773c01381540443e9..62e19c252738e36ac304f92ae834d3be2e4aafa4 100644
--- a/dump/win_dump.h
+++ b/dump/win_dump.h
@@ -14,7 +14,7 @@
 #include "system/dump.h"
 
 /* Check Windows dump availability for the current target */
-bool win_dump_available(Error **errp);
+bool win_dump_available(DumpState *s, Error **errp);
 
 void create_win_dump(DumpState *s, Error **errp);
 

---
base-commit: e771ba98de25c9f43959f79fc7099cf7fbba44cc
change-id: 20250827-enhance-win-dump-avalaible-v2-4325b03f894d

Best regards,
-- 
Nikolai Barybin <nikolai.barybin@virtuozzo.com>

Re: [PATCH] dump: enhance win_dump_available to report properly

[Daniel P. Berrangé]

On Wed, Aug 27, 2025 at 04:15:27PM +0300, Nikolai Barybin wrote:
> QMP query-dump-guest-memory-capability reports win dump as available for
> any x86 VM, which is false.
> 
> This patch implements proper query of vmcoreinfo and calculation of
> guest note size. Based on that we can surely report whether win dump
> available or not.
> 
> To perform this I suggest to split dump_init() into dump_preinit() and
> dump_init_complete() to avoid exausting copypaste in
> win_dump_available().
> 
> For further reference one may review this libvirt discussion:
> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
> [PATCH 0/4] Allow xml-configured coredump format on VM crash
> 
> Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com>
> ---
> During first series discussion Den mentions that that code will not work
> on 32bit guest with more than 4Gb RAM on i386.
> 
> This issue required even more code duplication between dump_init() and
> win_dump_available() which we'd like to avoid as mentioned by Daniel.
> 
> Hence I present 2nd version of this fix:
>  - split dump_init() into dump_preinit() and dump_init_complete()
>  - pass pre-inited dump structure with calculated guest note size to
>    win_dump_available()
>  - call header check and guest note size check inside
>    win_dump_available()
> ---
>  dump/dump.c     | 129 ++++++++++++++++++++++++++++++++------------------------
>  dump/win_dump.c |  23 ++++++++--
>  dump/win_dump.h |   2 +-
>  3 files changed, 95 insertions(+), 59 deletions(-)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 15bbcc0c6192822cf920fcb7d60eb7d2cfad0952..19341fa42feef4d1c50dbb3a892ded59a3468d20 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1777,10 +1777,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
>      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, bool kdump_raw,
> -                      Error **errp)
> +static void dump_preinit(DumpState *s, Error **errp)
>  {
>      ERRP_GUARD();
>      VMCoreInfoState *vmci = vmcoreinfo_find();
> @@ -1788,16 +1785,6 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      int nr_cpus;
>      int ret;
>  
> -    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) {
> -        assert(!paging && !has_filter);
> -    }
> -
>      if (runstate_is_running()) {
>          vm_stop(RUN_STATE_SAVE_VM);
>          s->resume = true;
> @@ -1814,41 +1801,10 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          nr_cpus++;
>      }
>  
> -    s->fd = fd;
> -    if (has_filter && !length) {
> -        error_setg(errp, "parameter 'length' expects a non-zero size");
> -        goto cleanup;
> -    }
> -    s->filter_area_begin = begin;
> -    s->filter_area_length = length;
> -
> -    /* First index is 0, it's the special null name */
> -    s->string_table_buf = g_array_new(FALSE, TRUE, 1);
> -    /*
> -     * Allocate the null name, due to the clearing option set to true
> -     * it will be 0.
> -     */
> -    g_array_set_size(s->string_table_buf, 1);
> -
>      memory_mapping_list_init(&s->list);
> -
>      guest_phys_blocks_init(&s->guest_phys_blocks);
>      guest_phys_blocks_append(&s->guest_phys_blocks);
> -    s->total_size = dump_calculate_size(s);
> -#ifdef DEBUG_DUMP_GUEST_MEMORY
> -    fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
> -#endif
>  
> -    /* it does not make sense to dump non-existent memory */
> -    if (!s->total_size) {
> -        error_setg(errp, "dump: no guest memory to dump");
> -        goto cleanup;
> -    }
> -
> -    /* get dump info: endian, class and architecture.
> -     * If the target architecture is not supported, cpu_get_dump_info() will
> -     * return -1.
> -     */
>      ret = cpu_get_dump_info(&s->dump_info, &s->guest_phys_blocks);
>      if (ret < 0) {
>          error_setg(errp,
> @@ -1906,6 +1862,56 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>          }
>      }
>  
> +    s->nr_cpus = nr_cpus;
> +    return;
> +cleanup:
> +    dump_cleanup(s);
> +}

The 'dump_cleanup' call is unsafe.

In qmp_query_dump_guest_memory_capability we initialize 's' using
'dump_state_prepare' which just zero's the struct, aside from
the 'status' field.

Meanwhile 'dump_cleanup' will unconditionally do:

    close(s->fd);

and 'fd' will be 0, as in stdin, so we break any usage of stdin
that QEMU has. Then some other unlucky part of QEMU will open a
FD and get given FD == 0, making things potentially even worse.

We need 'dump_state_prepare' to set 's->fd = -1', and in
dump_cleanup we should check for s->fd != -1, and after
closing it, must set it back to '-1'.

In fact, I think even the existing dump code is broken in
this respect, and so this should likely be a separate fix
we can send to stable.

I think the 'migrate_del_blocker' call in dump_cleanup
is potentially unsafe too, as it might try to delete a
blocker that is not registered.


> @@ -2215,6 +2224,14 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
>                                    g_new0(DumpGuestMemoryCapability, 1);
>      DumpGuestMemoryFormatList **tail = &cap->formats;
>  
> +    DumpState *s = &dump_state_global;
> +    dump_state_prepare(s);

This is unsafe.

Consider we have 2 distinct monitor clients. One client is
in the middle of a dump operation, when another client calls
query-dump-guest-memory-capability - the latter will scribble
over state used by the former. We must use a local stack
allocated DumpState in this query command, instead of
dump_state_global.

> +    dump_preinit(s, errp);
> +    if (*errp) {
> +        qatomic_set(&s->status, DUMP_STATUS_FAILED);
> +        return cap;
> +    }
> +
>      /* elf is always available */
>      QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_ELF);
>  
> @@ -2234,9 +2251,11 @@ DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
>      QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_KDUMP_RAW_SNAPPY);
>  #endif
>  
> -    if (win_dump_available(NULL)) {
> +    if (win_dump_available(s, NULL)) {
>          QAPI_LIST_APPEND(tail, DUMP_GUEST_MEMORY_FORMAT_WIN_DMP);
>      }
>  
> +    dump_cleanup(s);
> +    qatomic_set(&s->status, DUMP_STATUS_NONE);
>      return cap;
>  }

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] dump: enhance win_dump_available to report properly

[Nikolai Barybin]

[-- Attachment #1: Type: text/plain, Size: 3799 bytes --]

27.08.2025 20:14, Daniel P. Berrangé wrote:

> On Wed, Aug 27, 2025 at 04:15:27PM +0300, Nikolai Barybin wrote:
>> QMP query-dump-guest-memory-capability reports win dump as available for
>> any x86 VM, which is false.
>>
>> This patch implements proper query of vmcoreinfo and calculation of
>> guest note size. Based on that we can surely report whether win dump
>> available or not.
>>
>> To perform this I suggest to split dump_init() into dump_preinit() and
>> dump_init_complete() to avoid exausting copypaste in
>> win_dump_available().
>>
>> For further reference one may review this libvirt discussion:
>> https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB/#HJ3JRLWLGN3IKIC22OQ3PMZ4J3EFG5XB
>> [PATCH 0/4] Allow xml-configured coredump format on VM crash
>>
>> Signed-off-by: Nikolai Barybin<nikolai.barybin@virtuozzo.com>
>> ---
>> During first series discussion Den mentions that that code will not work
>> on 32bit guest with more than 4Gb RAM on i386.
>>
>> This issue required even more code duplication between dump_init() and
>> win_dump_available() which we'd like to avoid as mentioned by Daniel.
>>
>> Hence I present 2nd version of this fix:
>>   - split dump_init() into dump_preinit() and dump_init_complete()
>>   - pass pre-inited dump structure with calculated guest note size to
>>     win_dump_available()
>>   - call header check and guest note size check inside
>>     win_dump_available()
>> ---
>>   dump/dump.c     | 129 ++++++++++++++++++++++++++++++++------------------------
>>   dump/win_dump.c |  23 ++++++++--
>>   dump/win_dump.h |   2 +-
>>   3 files changed, 95 insertions(+), 59 deletions(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 15bbcc0c6192822cf920fcb7d60eb7d2cfad0952..19341fa42feef4d1c50dbb3a892ded59a3468d20 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -1777,10 +1777,7 @@ static void vmcoreinfo_update_phys_base(DumpState *s)
>>       g_strfreev(lines);
>>   }
>>   [...]
>> +    s->nr_cpus = nr_cpus;
>> +    return;
>> +cleanup:
>> +    dump_cleanup(s);
>> +}
> The 'dump_cleanup' call is unsafe.
>
> In qmp_query_dump_guest_memory_capability we initialize 's' using
> 'dump_state_prepare' which just zero's the struct, aside from
> the 'status' field.
>
> Meanwhile 'dump_cleanup' will unconditionally do:
>
>      close(s->fd);
>
> and 'fd' will be 0, as in stdin, so we break any usage of stdin
> that QEMU has. Then some other unlucky part of QEMU will open a
> FD and get given FD == 0, making things potentially even worse.
>
> We need 'dump_state_prepare' to set 's->fd = -1', and in
> dump_cleanup we should check for s->fd != -1, and after
> closing it, must set it back to '-1'.
>
> In fact, I think even the existing dump code is broken in
> this respect, and so this should likely be a separate fix
> we can send to stable.
>
> I think the 'migrate_del_blocker' call in dump_cleanup
> is potentially unsafe too, as it might try to delete a
> blocker that is not registered.

I'm not sure about that. Dump blocker variable is defined as global 
static and is zeroed by default:

static Error *dump_migration_blocker;

And even if we tried to delete unregistered blocker it would do nothing:

migrate_del_blocker(&dump_migration_blocker);

void migrate_del_blocker(Error **reasonp)
{
     if (*reasonp) { <--- NULL-ptr check
         for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
             migration_blockers[mode] = 
g_slist_remove(migration_blockers[mode],
                                                       *reasonp);
         }
         error_free(*reasonp);
         *reasonp = NULL;
     }
}


But maybe I'm losing something, correct me if I'm wrong.

> [...]
> With regards,
> Daniel

[-- Attachment #2: Type: text/html, Size: 5174 bytes --]