Hi
On Wed, Apr 6, 2022 at 11:00 AM Marc-André Lureau
<marcandre.lur...@redhat.com> wrote:
>
> Hi
>
> On Fri, Mar 25, 2022 at 11:51 PM Viktor Prutyanov
> <viktor.prutya...@redhat.com> wrote:
> >
> > Before this patch, 'dump-guest-memory -w' was accepting only 64-bit
> > dump header provided by guest through vmcoreinfo and thus was unable
> > to produce 32-bit guest Windows dump. So, add 32-bit guest Windows
> > dumping support.
> >
> > Signed-off-by: Viktor Prutyanov <viktor.prutya...@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>
> lgtm,
> you should update dump-guest-memory doc in hmp-commands.hx
Thank you for this note, I will update the doc.
>
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
Best regards,
Viktor Prutyanov
> > ---
> > dump/win_dump.c | 245 +++++++++++++++++++++++++++++-------------------
> > 1 file changed, 149 insertions(+), 96 deletions(-)
> >
> > diff --git a/dump/win_dump.c b/dump/win_dump.c
> > index df3b432ca5..fda32da036 100644
> > --- a/dump/win_dump.c
> > +++ b/dump/win_dump.c
> > @@ -24,18 +24,24 @@
> > #include "hw/misc/vmcoreinfo.h"
> > #include "win_dump.h"
> >
> > -#define WIN_DUMP_PTR_SIZE sizeof(uint64_t)
> > +static size_t win_dump_ptr_size(bool x64)
> > +{
> > + return x64 ? sizeof(uint64_t) : sizeof(uint32_t);
> > +}
> >
> > -#define _WIN_DUMP_FIELD(f) (h->f)
> > +#define _WIN_DUMP_FIELD(f) (x64 ? h->x64.f : h->x32.f)
> > #define WIN_DUMP_FIELD(field) _WIN_DUMP_FIELD(field)
> >
> > -#define _WIN_DUMP_FIELD_PTR(f) ((void *)&h->f)
> > +#define _WIN_DUMP_FIELD_PTR(f) (x64 ? (void *)&h->x64.f : (void
> > *)&h->x32.f)
> > #define WIN_DUMP_FIELD_PTR(field) _WIN_DUMP_FIELD_PTR(field)
> >
> > -#define _WIN_DUMP_FIELD_SIZE(f) sizeof(h->f)
> > +#define _WIN_DUMP_FIELD_SIZE(f) (x64 ? sizeof(h->x64.f) : sizeof(h->x32.f))
> > #define WIN_DUMP_FIELD_SIZE(field) _WIN_DUMP_FIELD_SIZE(field)
> >
> > -#define WIN_DUMP_CTX_SIZE sizeof(WinContext64)
> > +static size_t win_dump_ctx_size(bool x64)
> > +{
> > + return x64 ? sizeof(WinContext64) : sizeof(WinContext32);
> > +}
> >
> > static size_t write_run(uint64_t base_page, uint64_t page_count,
> > int fd, Error **errp)
> > @@ -71,7 +77,7 @@ static size_t write_run(uint64_t base_page, uint64_t
> > page_count,
> > return total;
> > }
> >
> > -static void write_runs(DumpState *s, WinDumpHeader64 *h, Error **errp)
> > +static void write_runs(DumpState *s, WinDumpHeader *h, bool x64, Error
> > **errp)
> > {
> > uint64_t BasePage, PageCount;
> > Error *local_err = NULL;
> > @@ -88,22 +94,24 @@ static void write_runs(DumpState *s, WinDumpHeader64
> > *h, Error **errp)
> > }
> > }
> >
> > -static int cpu_read_ptr(CPUState *cpu, uint64_t addr, uint64_t *ptr)
> > +static int cpu_read_ptr(bool x64, CPUState *cpu, uint64_t addr, uint64_t
> > *ptr)
> > {
> > int ret;
> > + uint32_t ptr32;
> > uint64_t ptr64;
> >
> > - ret = cpu_memory_rw_debug(cpu, addr, &ptr64, WIN_DUMP_PTR_SIZE, 0);
> > + ret = cpu_memory_rw_debug(cpu, addr, x64 ? (void *)&ptr64 : (void
> > *)&ptr32,
> > + win_dump_ptr_size(x64), 0);
> >
> > - *ptr = ptr64;
> > + *ptr = x64 ? ptr64 : ptr32;
> >
> > return ret;
> > }
> >
> > -static void patch_mm_pfn_database(WinDumpHeader64 *h, Error **errp)
> > +static void patch_mm_pfn_database(WinDumpHeader *h, bool x64, Error **errp)
> > {
> > if (cpu_memory_rw_debug(first_cpu,
> > - WIN_DUMP_FIELD(KdDebuggerDataBlock) +
> > KDBG_MM_PFN_DATABASE_OFFSET64,
> > + WIN_DUMP_FIELD(KdDebuggerDataBlock) +
> > KDBG_MM_PFN_DATABASE_OFFSET,
> > WIN_DUMP_FIELD_PTR(PfnDatabase),
> > WIN_DUMP_FIELD_SIZE(PfnDatabase), 0)) {
> > error_setg(errp, "win-dump: failed to read MmPfnDatabase");
> > @@ -111,13 +119,12 @@ static void patch_mm_pfn_database(WinDumpHeader64 *h,
> > Error **errp)
> > }
> > }
> >
> > -static void patch_bugcheck_data(WinDumpHeader64 *h, Error **errp)
> > +static void patch_bugcheck_data(WinDumpHeader *h, bool x64, Error **errp)
> > {
> > uint64_t KiBugcheckData;
> >
> > - if (cpu_read_ptr(first_cpu,
> > - WIN_DUMP_FIELD(KdDebuggerDataBlock) +
> > - KDBG_KI_BUGCHECK_DATA_OFFSET64,
> > + if (cpu_read_ptr(x64, first_cpu,
> > + WIN_DUMP_FIELD(KdDebuggerDataBlock) +
> > KDBG_KI_BUGCHECK_DATA_OFFSET,
> > &KiBugcheckData)) {
> > error_setg(errp, "win-dump: failed to read KiBugcheckData");
> > return;
> > @@ -142,30 +149,34 @@ static void patch_bugcheck_data(WinDumpHeader64 *h,
> > Error **errp)
> > /*
> > * This routine tries to correct mistakes in crashdump header.
> > */
> > -static void patch_header(WinDumpHeader64 *h)
> > +static void patch_header(WinDumpHeader *h, bool x64)
> > {
> > Error *local_err = NULL;
> >
> > - h->RequiredDumpSpace = sizeof(WinDumpHeader64) +
> > - (h->PhysicalMemoryBlock.NumberOfPages << TARGET_PAGE_BITS);
> > - h->PhysicalMemoryBlock.unused = 0;
> > - h->unused1 = 0;
> > + if (x64) {
> > + h->x64.RequiredDumpSpace = sizeof(WinDumpHeader64) +
> > + (h->x64.PhysicalMemoryBlock.NumberOfPages << TARGET_PAGE_BITS);
> > + h->x64.PhysicalMemoryBlock.unused = 0;
> > + h->x64.unused1 = 0;
> > + } else {
> > + h->x32.RequiredDumpSpace = sizeof(WinDumpHeader32) +
> > + (h->x32.PhysicalMemoryBlock.NumberOfPages << TARGET_PAGE_BITS);
> > + }
> >
> > - patch_mm_pfn_database(h, &local_err);
> > + patch_mm_pfn_database(h, x64, &local_err);
> > if (local_err) {
> > warn_report_err(local_err);
> > local_err = NULL;
> > }
> > - patch_bugcheck_data(h, &local_err);
> > + patch_bugcheck_data(h, x64, &local_err);
> > if (local_err) {
> > warn_report_err(local_err);
> > }
> > }
> >
> > -static void check_header(WinDumpHeader64 *h, Error **errp)
> > +static void check_header(WinDumpHeader *h, bool *x64, Error **errp)
> > {
> > const char Signature[] = "PAGE";
> > - const char ValidDump[] = "DU64";
> >
> > if (memcmp(h->Signature, Signature, sizeof(h->Signature))) {
> > error_setg(errp, "win-dump: invalid header, expected '%.4s',"
> > @@ -173,14 +184,17 @@ static void check_header(WinDumpHeader64 *h, Error
> > **errp)
> > return;
> > }
> >
> > - if (memcmp(h->ValidDump, ValidDump, sizeof(h->ValidDump))) {
> > - error_setg(errp, "win-dump: invalid header, expected '%.4s',"
> > - " got '%.4s'", ValidDump, h->ValidDump);
> > - return;
> > + if (!memcmp(h->ValidDump, "DUMP", sizeof(h->ValidDump))) {
> > + *x64 = false;
> > + } else if (!memcmp(h->ValidDump, "DU64", sizeof(h->ValidDump))) {
> > + *x64 = true;
> > + } else {
> > + error_setg(errp, "win-dump: invalid header, expected 'DUMP' or
> > 'DU64',"
> > + " got '%.4s'", h->ValidDump);
> > }
> > }
> >
> > -static void check_kdbg(WinDumpHeader64 *h, Error **errp)
> > +static void check_kdbg(WinDumpHeader *h, bool x64, Error **errp)
> > {
> > const char OwnerTag[] = "KDBG";
> > char read_OwnerTag[4];
> > @@ -189,7 +203,7 @@ static void check_kdbg(WinDumpHeader64 *h, Error **errp)
> >
> > try_again:
> > if (cpu_memory_rw_debug(first_cpu,
> > - KdDebuggerDataBlock + KDBG_OWNER_TAG_OFFSET64,
> > + KdDebuggerDataBlock + KDBG_OWNER_TAG_OFFSET,
> > (uint8_t *)&read_OwnerTag, sizeof(read_OwnerTag), 0)) {
> > error_setg(errp, "win-dump: failed to read OwnerTag");
> > return;
> > @@ -214,15 +228,19 @@ try_again:
> > }
> > }
> >
> > - h->KdDebuggerDataBlock = KdDebuggerDataBlock;
> > + if (x64) {
> > + h->x64.KdDebuggerDataBlock = KdDebuggerDataBlock;
> > + } else {
> > + h->x32.KdDebuggerDataBlock = KdDebuggerDataBlock;
> > + }
> > }
> >
> > struct saved_context {
> > - WinContext64 ctx;
> > + WinContext ctx;
> > uint64_t addr;
> > };
> >
> > -static void patch_and_save_context(WinDumpHeader64 *h,
> > +static void patch_and_save_context(WinDumpHeader *h, bool x64,
> > struct saved_context *saved_ctx,
> > Error **errp)
> > {
> > @@ -232,15 +250,15 @@ static void patch_and_save_context(WinDumpHeader64 *h,
> > CPUState *cpu;
> > int i = 0;
> >
> > - if (cpu_read_ptr(first_cpu,
> > - KdDebuggerDataBlock + KDBG_KI_PROCESSOR_BLOCK_OFFSET64,
> > + if (cpu_read_ptr(x64, first_cpu,
> > + KdDebuggerDataBlock + KDBG_KI_PROCESSOR_BLOCK_OFFSET,
> > &KiProcessorBlock)) {
> > error_setg(errp, "win-dump: failed to read KiProcessorBlock");
> > return;
> > }
> >
> > if (cpu_memory_rw_debug(first_cpu,
> > - KdDebuggerDataBlock + KDBG_OFFSET_PRCB_CONTEXT_OFFSET64,
> > + KdDebuggerDataBlock + KDBG_OFFSET_PRCB_CONTEXT_OFFSET,
> > (uint8_t *)&OffsetPrcbContext, sizeof(OffsetPrcbContext), 0)) {
> > error_setg(errp, "win-dump: failed to read OffsetPrcbContext");
> > return;
> > @@ -251,17 +269,17 @@ static void patch_and_save_context(WinDumpHeader64 *h,
> > CPUX86State *env = &x86_cpu->env;
> > uint64_t Prcb;
> > uint64_t Context;
> > - WinContext64 ctx;
> > + WinContext ctx;
> >
> > - if (cpu_read_ptr(first_cpu,
> > - KiProcessorBlock + i * WIN_DUMP_PTR_SIZE,
> > + if (cpu_read_ptr(x64, first_cpu,
> > + KiProcessorBlock + i * win_dump_ptr_size(x64),
> > &Prcb)) {
> > error_setg(errp, "win-dump: failed to read"
> > " CPU #%d PRCB location", i);
> > return;
> > }
> >
> > - if (cpu_read_ptr(first_cpu,
> > + if (cpu_read_ptr(x64, first_cpu,
> > Prcb + OffsetPrcbContext,
> > &Context)) {
> > error_setg(errp, "win-dump: failed to read"
> > @@ -271,56 +289,88 @@ static void patch_and_save_context(WinDumpHeader64 *h,
> >
> > saved_ctx[i].addr = Context;
> >
> > - ctx = (WinContext64){
> > - .ContextFlags = WIN_CTX64_ALL,
> > - .MxCsr = env->mxcsr,
> > -
> > - .SegEs = env->segs[0].selector,
> > - .SegCs = env->segs[1].selector,
> > - .SegSs = env->segs[2].selector,
> > - .SegDs = env->segs[3].selector,
> > - .SegFs = env->segs[4].selector,
> > - .SegGs = env->segs[5].selector,
> > - .EFlags = cpu_compute_eflags(env),
> > -
> > - .Dr0 = env->dr[0],
> > - .Dr1 = env->dr[1],
> > - .Dr2 = env->dr[2],
> > - .Dr3 = env->dr[3],
> > - .Dr6 = env->dr[6],
> > - .Dr7 = env->dr[7],
> > -
> > - .Rax = env->regs[R_EAX],
> > - .Rbx = env->regs[R_EBX],
> > - .Rcx = env->regs[R_ECX],
> > - .Rdx = env->regs[R_EDX],
> > - .Rsp = env->regs[R_ESP],
> > - .Rbp = env->regs[R_EBP],
> > - .Rsi = env->regs[R_ESI],
> > - .Rdi = env->regs[R_EDI],
> > - .R8 = env->regs[8],
> > - .R9 = env->regs[9],
> > - .R10 = env->regs[10],
> > - .R11 = env->regs[11],
> > - .R12 = env->regs[12],
> > - .R13 = env->regs[13],
> > - .R14 = env->regs[14],
> > - .R15 = env->regs[15],
> > -
> > - .Rip = env->eip,
> > - .FltSave = {
> > + if (x64) {
> > + ctx.x64 = (WinContext64){
> > + .ContextFlags = WIN_CTX64_ALL,
> > .MxCsr = env->mxcsr,
> > - },
> > - };
> > +
> > + .SegEs = env->segs[0].selector,
> > + .SegCs = env->segs[1].selector,
> > + .SegSs = env->segs[2].selector,
> > + .SegDs = env->segs[3].selector,
> > + .SegFs = env->segs[4].selector,
> > + .SegGs = env->segs[5].selector,
> > + .EFlags = cpu_compute_eflags(env),
> > +
> > + .Dr0 = env->dr[0],
> > + .Dr1 = env->dr[1],
> > + .Dr2 = env->dr[2],
> > + .Dr3 = env->dr[3],
> > + .Dr6 = env->dr[6],
> > + .Dr7 = env->dr[7],
> > +
> > + .Rax = env->regs[R_EAX],
> > + .Rbx = env->regs[R_EBX],
> > + .Rcx = env->regs[R_ECX],
> > + .Rdx = env->regs[R_EDX],
> > + .Rsp = env->regs[R_ESP],
> > + .Rbp = env->regs[R_EBP],
> > + .Rsi = env->regs[R_ESI],
> > + .Rdi = env->regs[R_EDI],
> > + .R8 = env->regs[8],
> > + .R9 = env->regs[9],
> > + .R10 = env->regs[10],
> > + .R11 = env->regs[11],
> > + .R12 = env->regs[12],
> > + .R13 = env->regs[13],
> > + .R14 = env->regs[14],
> > + .R15 = env->regs[15],
> > +
> > + .Rip = env->eip,
> > + .FltSave = {
> > + .MxCsr = env->mxcsr,
> > + },
> > + };
> > + } else {
> > + ctx.x32 = (WinContext32){
> > + .ContextFlags = WIN_CTX32_FULL | WIN_CTX_DBG,
> > +
> > + .SegEs = env->segs[0].selector,
> > + .SegCs = env->segs[1].selector,
> > + .SegSs = env->segs[2].selector,
> > + .SegDs = env->segs[3].selector,
> > + .SegFs = env->segs[4].selector,
> > + .SegGs = env->segs[5].selector,
> > + .EFlags = cpu_compute_eflags(env),
> > +
> > + .Dr0 = env->dr[0],
> > + .Dr1 = env->dr[1],
> > + .Dr2 = env->dr[2],
> > + .Dr3 = env->dr[3],
> > + .Dr6 = env->dr[6],
> > + .Dr7 = env->dr[7],
> > +
> > + .Eax = env->regs[R_EAX],
> > + .Ebx = env->regs[R_EBX],
> > + .Ecx = env->regs[R_ECX],
> > + .Edx = env->regs[R_EDX],
> > + .Esp = env->regs[R_ESP],
> > + .Ebp = env->regs[R_EBP],
> > + .Esi = env->regs[R_ESI],
> > + .Edi = env->regs[R_EDI],
> > +
> > + .Eip = env->eip,
> > + };
> > + }
> >
> > if (cpu_memory_rw_debug(first_cpu, Context,
> > - &saved_ctx[i].ctx, WIN_DUMP_CTX_SIZE, 0)) {
> > + &saved_ctx[i].ctx, win_dump_ctx_size(x64), 0)) {
> > error_setg(errp, "win-dump: failed to save CPU #%d context",
> > i);
> > return;
> > }
> >
> > if (cpu_memory_rw_debug(first_cpu, Context,
> > - &ctx, WIN_DUMP_CTX_SIZE, 1)) {
> > + &ctx, win_dump_ctx_size(x64), 1)) {
> > error_setg(errp, "win-dump: failed to write CPU #%d context",
> > i);
> > return;
> > }
> > @@ -329,14 +379,14 @@ static void patch_and_save_context(WinDumpHeader64 *h,
> > }
> > }
> >
> > -static void restore_context(WinDumpHeader64 *h,
> > +static void restore_context(WinDumpHeader *h, bool x64,
> > struct saved_context *saved_ctx)
> > {
> > int i;
> >
> > for (i = 0; i < WIN_DUMP_FIELD(NumberProcessors); i++) {
> > if (cpu_memory_rw_debug(first_cpu, saved_ctx[i].addr,
> > - &saved_ctx[i].ctx, WIN_DUMP_CTX_SIZE, 1)) {
> > + &saved_ctx[i].ctx, win_dump_ctx_size(x64), 1)) {
> > warn_report("win-dump: failed to restore CPU #%d context", i);
> > }
> > }
> > @@ -344,25 +394,28 @@ static void restore_context(WinDumpHeader64 *h,
> >
> > void create_win_dump(DumpState *s, Error **errp)
> > {
> > - WinDumpHeader64 *h = (WinDumpHeader64 *)(s->guest_note +
> > - VMCOREINFO_ELF_NOTE_HDR_SIZE);
> > + WinDumpHeader *h = (void *)(s->guest_note +
> > VMCOREINFO_ELF_NOTE_HDR_SIZE);
> > X86CPU *first_x86_cpu = X86_CPU(first_cpu);
> > uint64_t saved_cr3 = first_x86_cpu->env.cr[3];
> > struct saved_context *saved_ctx = NULL;
> > Error *local_err = NULL;
> > + bool x64;
> > + size_t hdr_size;
> >
> > - if (s->guest_note_size != sizeof(WinDumpHeader64) +
> > - VMCOREINFO_ELF_NOTE_HDR_SIZE) {
> > + 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;
> > }
> >
> > - check_header(h, &local_err);
> > + check_header(h, &x64, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > return;
> > }
> >
> > + hdr_size = x64 ? sizeof(WinDumpHeader64) : sizeof(WinDumpHeader32);
> > +
> > /*
> > * Further access to kernel structures by virtual addresses
> > * should be made from system context.
> > @@ -370,13 +423,13 @@ void create_win_dump(DumpState *s, Error **errp)
> >
> > first_x86_cpu->env.cr[3] = WIN_DUMP_FIELD(DirectoryTableBase);
> >
> > - check_kdbg(h, &local_err);
> > + check_kdbg(h, x64, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > goto out_cr3;
> > }
> >
> > - patch_header(h);
> > + patch_header(h, x64);
> >
> > saved_ctx = g_new(struct saved_context,
> > WIN_DUMP_FIELD(NumberProcessors));
> >
> > @@ -385,7 +438,7 @@ void create_win_dump(DumpState *s, Error **errp)
> > * to determine if the system-saved context is valid
> > */
> >
> > - patch_and_save_context(h, saved_ctx, &local_err);
> > + patch_and_save_context(h, x64, saved_ctx, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > goto out_free;
> > @@ -393,20 +446,20 @@ void create_win_dump(DumpState *s, Error **errp)
> >
> > s->total_size = WIN_DUMP_FIELD(RequiredDumpSpace);
> >
> > - s->written_size = qemu_write_full(s->fd, h, sizeof(*h));
> > - if (s->written_size != sizeof(*h)) {
> > + s->written_size = qemu_write_full(s->fd, h, hdr_size);
> > + if (s->written_size != hdr_size) {
> > error_setg(errp, QERR_IO_ERROR);
> > goto out_restore;
> > }
> >
> > - write_runs(s, h, &local_err);
> > + write_runs(s, h, x64, &local_err);
> > if (local_err) {
> > error_propagate(errp, local_err);
> > goto out_restore;
> > }
> >
> > out_restore:
> > - restore_context(h, saved_ctx);
> > + restore_context(h, x64, saved_ctx);
> > out_free:
> > g_free(saved_ctx);
> > out_cr3:
> > --
> > 2.35.1
> >
>
