Re: [PATCH v3 4/4] dump/win_dump: add 32-bit guest Windows support

[Marc-André Lureau]

Hi

On Fri, Mar 25, 2022 at 11:51 PM Viktor Prutyanov
<viktor.prutyanov@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.prutyanov@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

lgtm,
you should update dump-guest-memory doc in hmp-commands.hx

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

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