Ouh, yes please do it. This is the right change. Thanks Kazu. --Alexey
On Thu, Mar 7, 2024 at 12:12 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
On 2024/03/05 10:44, Alexey Makhalov wrote:
> There are several versions of debug.guest format. Current version of
> the code is able to parse only version 4.
>
> Improve parser to support other known versions. Split data structures
> on sub-structures and introduce a helper functions to calculate a gap
> between them based on the version number. Implement additional data
> structure (struct mainmeminfo_old) and logic specifically for original
> (version 1) format support.
>
> Signed-off-by: Alexey Makhalov <alexey.makhalov@xxxxxxxxxxxx>
This warning is emitted:
$ make clean ; make warn
cc -c -g -DX86_64 -DLZO -DSNAPPY -DZSTD -DGDB_10_2 vmware_guestdump.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security
vmware_guestdump.c: In function 'is_vmware_guestdump':
vmware_guestdump.c:290:1: warning: label 'unrecognized' defined but not used [-Wunused-label]
unrecognized:
^~~~~~~~~~~~
Is it OK to just remove this label and the following code?
--- a/vmware_guestdump.c
+++ b/vmware_guestdump.c
@@ -286,11 +286,6 @@ is_vmware_guestdump(char *filename)
vmss.memoffset = 0;
vmss.num_vcpus = hdr.num_vcpus;
return TRUE;
-
-unrecognized:
- if (CRASHDEBUG(1))
- error(INFO, LOGPRX"Unrecognized debug.guest file.\n");
- return FALSE;
}
int
If so, we can do it when applying,
Acked-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
Thanks,
Kazu
> ---
> vmware_guestdump.c | 316 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 229 insertions(+), 87 deletions(-)
>
> diff --git a/vmware_guestdump.c b/vmware_guestdump.c
> index 5be26c8..5c7ee4d 100644
> --- a/vmware_guestdump.c
> +++ b/vmware_guestdump.c
> @@ -2,6 +2,8 @@
> * vmware_guestdump.c
> *
> * Copyright (c) 2020 VMware, Inc.
> + * Copyright (c) 2024 Broadcom. All Rights Reserved. The term "Broadcom"
> + * refers to Broadcom Inc. and/or its subsidiaries.
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -13,7 +15,7 @@
> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> * GNU General Public License for more details.
> *
> - * Author: Alexey Makhalov <amakhalov@xxxxxxxxxx>
> + * Author: Alexey Makhalov <alexey.makhalov@xxxxxxxxxxxx>
> */
>
> #include "defs.h"
> @@ -21,20 +23,31 @@
>
> #define LOGPRX "vmw: "
>
> -#define GUESTDUMP_VERSION 4
> -#define GUESTDUMP_MAGIC1 1
> -#define GUESTDUMP_MAGIC2 0
> -
> +/*
> + * debug.guest file layout
> + * 00000000: guest dump header, it includes:
> + * 1. Version (4 bytes) \
> + * 2. Number of Virtual CPUs (4 bytes) } - struct guestdumpheader
> + * 3. Reserved gap
> + * 4. Main Memory information - struct mainmeminfo{,_old}
> + * (use get_vcpus_offset() to get total size of guestdumpheader)
> + * vcpus_offset: ---------\
> + * 1. struct vcpu_state1 \
> + * 2. reserved gap } num_vcpus times
> + * 3. struct vcpu_state2 /
> + * 4. 4KB of reserved data /
> + * --------/
> + *
> + */
> struct guestdumpheader {
> uint32_t version;
> uint32_t num_vcpus;
> - uint8_t magic1;
> - uint8_t reserved1;
> - uint32_t cpu_vendor;
> - uint64_t magic2;
> +} __attribute__((packed)) hdr;
> +
> +struct mainmeminfo {
> uint64_t last_addr;
> uint64_t memsize_in_pages;
> - uint32_t reserved2;
> + uint32_t reserved1;
> uint32_t mem_holes;
> struct memhole {
> uint64_t ppn;
> @@ -42,14 +55,36 @@ struct guestdumpheader {
> } holes[2];
> } __attribute__((packed));
>
> -struct vcpu_state {
> +/* Used by version 1 only */
> +struct mainmeminfo_old {
> + uint64_t last_addr;
> + uint32_t memsize_in_pages;
> + uint32_t reserved1;
> + uint32_t mem_holes;
> + struct memhole1 {
> + uint32_t ppn;
> + uint32_t pages;
> + } holes[2];
> + /* There are additional fields, see get_vcpus_offset() calculation. */
> +} __attribute__((packed));
> +
> +/* First half of vcpu_state */
> +struct vcpu_state1 {
> uint32_t cr0;
> uint64_t cr2;
> uint64_t cr3;
> uint64_t cr4;
> uint64_t reserved1[10];
> uint64_t idt_base;
> - uint16_t reserved2[21];
> +} __attribute__((packed));
> +
> +/*
> + * Unused fields between vcpu_state1 and vcpu_state2 swill be skipped.
> + * See get_vcpu_gapsize() calculation.
> + */
> +
> +/* Second half of vcpu_state */
> +struct vcpu_state2 {
> struct x86_64_pt_regs {
> uint64_t r15;
> uint64_t r14;
> @@ -76,9 +111,41 @@ struct vcpu_state {
> uint8_t reserved3[65];
> } __attribute__((packed));
>
> +/*
> + * Returns the size of the guest dump header.
> + */
> +static inline long
> +get_vcpus_offset(uint32_t version, int mem_holes)
> +{
> + switch (version) {
> + case 1: /* ESXi 6.7 and older */
> + return sizeof(struct guestdumpheader) + 13 + sizeof(struct mainmeminfo_old) +
> + (mem_holes == -1 ? 0 : 8 * mem_holes + 4);
> + case 3: /* ESXi 6.8 */
> + return sizeof(struct guestdumpheader) + 14 + sizeof(struct mainmeminfo);
> + case 4: /* ESXi 7.0 */
> + case 5: /* ESXi 8.0 */
> + return sizeof(struct guestdumpheader) + 14 + sizeof(struct mainmeminfo);
> + case 6: /* ESXi 8.0u2 */
> + return sizeof(struct guestdumpheader) + 15 + sizeof(struct mainmeminfo);
> +
> + }
> + return 0;
> +}
> +
> +/*
> + * Returns the size of reserved (unused) fields in the middle of vcpu_state structure.
> + */
> +static inline long
> +get_vcpu_gapsize(uint32_t version)
> +{
> + if (version < 4)
> + return 45;
> + return 42;
> +}
>
> /*
> - * vmware_guestdump is extension to vmware_vmss with ability to debug
> + * vmware_guestdump is an extension to the vmware_vmss with ability to debug
> * debug.guest and debug.vmem files.
> *
> * debug.guest.gz and debug.vmem.gz can be obtained using following
> @@ -86,73 +153,136 @@ struct vcpu_state {
> * monitor.mini-suspend_on_panic = TRUE
> * monitor.suspend_on_triplefault = TRUE
> *
> - * guestdump (debug.guest) is simplified version of *.vmss which does
> - * not contain full VM state, but minimal guest state, such as memory
> + * guestdump (debug.guest) is a simplified version of the *.vmss which does
> + * not contain a full VM state, but minimal guest state, such as a memory
> * layout and CPUs state, needed for debugger. is_vmware_guestdump()
> * and vmware_guestdump_init() functions parse guestdump header and
> - * populate vmss data structure (from vmware_vmss.c). As result, all
> + * populate vmss data structure (from vmware_vmss.c). In result, all
> * handlers (except mempry_dump) from vmware_vmss.c can be reused.
> *
> - * debug.guest does not have dedicated header magic or signature for
> - * its format. To probe debug.guest we need to perform header fields
> - * and file size validity. In addition, check for the filename
> - * extension, which must be ".guest".
> + * debug.guest does not have a dedicated header magic or file format signature
> + * To probe debug.guest we need to perform series of validations. In addition,
> + * we check for the filename extension, which must be ".guest".
> */
> -
> int
> is_vmware_guestdump(char *filename)
> {
> - struct guestdumpheader hdr;
> + struct mainmeminfo mmi;
> + long vcpus_offset;
> FILE *fp;
> - uint64_t filesize, holes_sum = 0;
> + uint64_t filesize, expected_filesize, holes_sum = 0;
> int i;
>
> if (strcmp(filename + strlen(filename) - 6, ".guest"))
> return FALSE;
>
> - if ((fp = fopen(filename, "r")) == NULL) {
> + if ((fp = fopen(filename, "r")) == NULL) {
> error(INFO, LOGPRX"Failed to open '%s': [Error %d] %s\n",
> - filename, errno, strerror(errno));
> + filename, errno, strerror(errno));
> return FALSE;
> - }
> + }
>
> if (fread(&hdr, sizeof(struct guestdumpheader), 1, fp) != 1) {
> error(INFO, LOGPRX"Failed to read '%s' from file '%s': [Error %d] %s\n",
> - "guestdumpheader", filename, errno, strerror(errno));
> + "guestdumpheader", filename, errno, strerror(errno));
> + fclose(fp);
> + return FALSE;
> + }
> +
> + vcpus_offset = get_vcpus_offset(hdr.version, -1 /* Unknown yet, adjust it later */);
> +
> + if (!vcpus_offset) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Not supported version %d\n", hdr.version);
> fclose(fp);
> return FALSE;
> }
>
> + if (hdr.version == 1) {
> + struct mainmeminfo_old tmp;
> + if (fseek(fp, vcpus_offset - sizeof(struct mainmeminfo_old), SEEK_SET) == -1) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Failed to fseek '%s': [Error %d] %s\n",
> + filename, errno, strerror(errno));
> + fclose(fp);
> + return FALSE;
> + }
> +
> + if (fread(&tmp, sizeof(struct mainmeminfo_old), 1, fp) != 1) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Failed to read '%s' from file '%s': [Error %d] %s\n",
> + "mainmeminfo_old", filename, errno, strerror(errno));
> + fclose(fp);
> + return FALSE;
> + }
> + mmi.last_addr = tmp.last_addr;
> + mmi.memsize_in_pages = tmp.memsize_in_pages;
> + mmi.mem_holes = tmp.mem_holes;
> + mmi.holes[0].ppn = tmp.holes[0].ppn;
> + mmi.holes[0].pages = tmp.holes[0].pages;
> + mmi.holes[1].ppn = tmp.holes[1].ppn;
> + mmi.holes[1].pages = tmp.holes[1].pages;
> + /* vcpu_offset adjustment for mem_holes is required only for version 1. */
> + vcpus_offset = get_vcpus_offset(hdr.version, mmi.mem_holes);
> + } else {
> + if (fseek(fp, vcpus_offset - sizeof(struct mainmeminfo), SEEK_SET) == -1) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Failed to fseek '%s': [Error %d] %s\n",
> + filename, errno, strerror(errno));
> + fclose(fp);
> + return FALSE;
> + }
> +
> + if (fread(&mmi, sizeof(struct mainmeminfo), 1, fp) != 1) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Failed to read '%s' from file '%s': [Error %d] %s\n",
> + "mainmeminfo", filename, errno, strerror(errno));
> + fclose(fp);
> + return FALSE;
> + }
> + }
> if (fseek(fp, 0L, SEEK_END) == -1) {
> - error(INFO, LOGPRX"Failed to fseek '%s': [Error %d] %s\n",
> - filename, errno, strerror(errno));
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Failed to fseek '%s': [Error %d] %s\n",
> + filename, errno, strerror(errno));
> fclose(fp);
> return FALSE;
> }
> filesize = ftell(fp);
> fclose(fp);
>
> - if (hdr.mem_holes > 2)
> - goto unrecognized;
> + if (mmi.mem_holes > 2) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Unexpected mmi.mem_holes value %d\n",
> + mmi.mem_holes);
> + return FALSE;
> + }
>
> - for (i = 0; i < hdr.mem_holes; i++) {
> + for (i = 0; i < mmi.mem_holes; i++) {
> /* hole start page */
> - vmss.regions[i].startpagenum = hdr.holes[i].ppn;
> + vmss.regions[i].startpagenum = mmi.holes[i].ppn;
> /* hole end page */
> - vmss.regions[i].startppn = hdr.holes[i].ppn + hdr.holes[i].pages;
> - holes_sum += hdr.holes[i].pages;
> + vmss.regions[i].startppn = mmi.holes[i].ppn + mmi.holes[i].pages;
> + holes_sum += mmi.holes[i].pages;
> + }
> +
> + if ((mmi.last_addr + 1) != ((mmi.memsize_in_pages + holes_sum) << VMW_PAGE_SHIFT)) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Memory size check failed\n");
> + return FALSE;
> }
>
> - if (hdr.version != GUESTDUMP_VERSION ||
> - hdr.magic1 != GUESTDUMP_MAGIC1 ||
> - hdr.magic2 != GUESTDUMP_MAGIC2 ||
> - (hdr.last_addr + 1) != ((hdr.memsize_in_pages + holes_sum) << VMW_PAGE_SHIFT) ||
> - filesize != sizeof(struct guestdumpheader) +
> - hdr.num_vcpus * (sizeof (struct vcpu_state) + VMW_PAGE_SIZE))
> - goto unrecognized;
> + expected_filesize = vcpus_offset + hdr.num_vcpus * (sizeof(struct vcpu_state1) +
> + get_vcpu_gapsize(hdr.version) + sizeof(struct vcpu_state2) + VMW_PAGE_SIZE);
> + if (filesize != expected_filesize) {
> + if (CRASHDEBUG(1))
> + error(INFO, LOGPRX"Incorrect file size: %d != %d\n",
> + filesize, expected_filesize);
> + return FALSE;
> + }
>
> - vmss.memsize = hdr.memsize_in_pages << VMW_PAGE_SHIFT;
> - vmss.regionscount = hdr.mem_holes + 1;
> + vmss.memsize = mmi.memsize_in_pages << VMW_PAGE_SHIFT;
> + vmss.regionscount = mmi.mem_holes + 1;
> vmss.memoffset = 0;
> vmss.num_vcpus = hdr.num_vcpus;
> return TRUE;
> @@ -169,7 +299,8 @@ vmware_guestdump_init(char *filename, FILE *ofp)
> FILE *fp = NULL;
> int i, result = TRUE;
> char *vmem_filename = NULL;
> - struct vcpu_state vs;
> + struct vcpu_state1 vs1;
> + struct vcpu_state2 vs2;
> char *p;
>
> if (!machine_type("X86") && !machine_type("X86_64")) {
> @@ -180,14 +311,14 @@ vmware_guestdump_init(char *filename, FILE *ofp)
> goto exit;
> }
>
> - if ((fp = fopen(filename, "r")) == NULL) {
> + if ((fp = fopen(filename, "r")) == NULL) {
> error(INFO, LOGPRX"Failed to open '%s': [Error %d] %s\n",
> filename, errno, strerror(errno));
> result = FALSE;
> goto exit;
> - }
> + }
>
> - if (fseek(fp, sizeof(struct guestdumpheader), SEEK_SET) == -1) {
> + if (fseek(fp, get_vcpus_offset(hdr.version, vmss.regionscount - 1), SEEK_SET) == -1) {
> error(INFO, LOGPRX"Failed to fseek '%s': [Error %d] %s\n",
> filename, errno, strerror(errno));
> result = FALSE;
> @@ -203,7 +334,19 @@ vmware_guestdump_init(char *filename, FILE *ofp)
> }
>
> for (i = 0; i < vmss.num_vcpus; i++) {
> - if (fread(&vs, sizeof(struct vcpu_state), 1, fp) != 1) {
> + if (fread(&vs1, sizeof(struct vcpu_state1), 1, fp) != 1) {
> + error(INFO, LOGPRX"Failed to read '%s' from file '%s': [Error %d] %s\n",
> + "vcpu_state", filename, errno, strerror(errno));
> + result = FALSE;
> + goto exit;
> + }
> + if (fseek(fp, get_vcpu_gapsize(hdr.version), SEEK_CUR) == -1) {
> + error(INFO, LOGPRX"Failed to read '%s' from file '%s': [Error %d] %s\n",
> + "vcpu_state", filename, errno, strerror(errno));
> + result = FALSE;
> + goto exit;
> + }
> + if (fread(&vs2, sizeof(struct vcpu_state2), 1, fp) != 1) {
> error(INFO, LOGPRX"Failed to read '%s' from file '%s': [Error %d] %s\n",
> "vcpu_state", filename, errno, strerror(errno));
> result = FALSE;
> @@ -217,29 +360,29 @@ vmware_guestdump_init(char *filename, FILE *ofp)
> }
> vmss.vcpu_regs[i] = 0;
>
> - vmss.regs64[i]->rax = vs.regs64.rax;
> - vmss.regs64[i]->rcx = vs.regs64.rcx;
> - vmss.regs64[i]->rdx = vs.regs64.rdx;
> - vmss.regs64[i]->rbx = vs.regs64.rbx;
> - vmss.regs64[i]->rbp = vs.regs64.rbp;
> - vmss.regs64[i]->rsp = vs.regs64.rsp;
> - vmss.regs64[i]->rsi = vs.regs64.rsi;
> - vmss.regs64[i]->rdi = vs.regs64.rdi;
> - vmss.regs64[i]->r8 = vs.regs64.r8;
> - vmss.regs64[i]->r9 = vs.regs64.r9;
> - vmss.regs64[i]->r10 = vs.regs64.r10;
> - vmss.regs64[i]->r11 = vs.regs64.r11;
> - vmss.regs64[i]->r12 = vs.regs64.r12;
> - vmss.regs64[i]->r13 = vs.regs64.r13;
> - vmss.regs64[i]->r14 = vs.regs64.r14;
> - vmss.regs64[i]->r15 = vs.regs64.r15;
> - vmss.regs64[i]->idtr = vs.idt_base;
> - vmss.regs64[i]->cr[0] = vs.cr0;
> - vmss.regs64[i]->cr[2] = vs.cr2;
> - vmss.regs64[i]->cr[3] = vs.cr3;
> - vmss.regs64[i]->cr[4] = vs.cr4;
> - vmss.regs64[i]->rip = vs.regs64.rip;
> - vmss.regs64[i]->rflags = vs.regs64.eflags;
> + vmss.regs64[i]->rax = vs2.regs64.rax;
> + vmss.regs64[i]->rcx = vs2.regs64.rcx;
> + vmss.regs64[i]->rdx = vs2.regs64.rdx;
> + vmss.regs64[i]->rbx = vs2.regs64.rbx;
> + vmss.regs64[i]->rbp = vs2.regs64.rbp;
> + vmss.regs64[i]->rsp = vs2.regs64.rsp;
> + vmss.regs64[i]->rsi = vs2.regs64.rsi;
> + vmss.regs64[i]->rdi = vs2.regs64.rdi;
> + vmss.regs64[i]->r8 = vs2.regs64.r8;
> + vmss.regs64[i]->r9 = vs2.regs64.r9;
> + vmss.regs64[i]->r10 = vs2.regs64.r10;
> + vmss.regs64[i]->r11 = vs2.regs64.r11;
> + vmss.regs64[i]->r12 = vs2.regs64.r12;
> + vmss.regs64[i]->r13 = vs2.regs64.r13;
> + vmss.regs64[i]->r14 = vs2.regs64.r14;
> + vmss.regs64[i]->r15 = vs2.regs64.r15;
> + vmss.regs64[i]->idtr = vs1.idt_base;
> + vmss.regs64[i]->cr[0] = vs1.cr0;
> + vmss.regs64[i]->cr[2] = vs1.cr2;
> + vmss.regs64[i]->cr[3] = vs1.cr3;
> + vmss.regs64[i]->cr[4] = vs1.cr4;
> + vmss.regs64[i]->rip = vs2.regs64.rip;
> + vmss.regs64[i]->rflags = vs2.regs64.eflags;
>
> vmss.vcpu_regs[i] = REGS_PRESENT_ALL;
> }
> @@ -268,9 +411,9 @@ vmware_guestdump_init(char *filename, FILE *ofp)
> fprintf(ofp, LOGPRX"vmem file: %s\n\n", vmem_filename);
>
> if (CRASHDEBUG(1)) {
> - vmware_guestdump_memory_dump(ofp);
> - dump_registers_for_vmss_dump();
> - }
> + vmware_guestdump_memory_dump(ofp);
> + dump_registers_for_vmss_dump();
> + }
>
> exit:
> if (fp)
> @@ -296,24 +439,23 @@ exit:
> int
> vmware_guestdump_memory_dump(FILE *ofp)
> {
> + uint64_t holes_sum = 0;
> + unsigned i;
> +
> fprintf(ofp, "vmware_guestdump:\n");
> fprintf(ofp, " Header: version=%d num_vcpus=%llu\n",
> - GUESTDUMP_VERSION, (ulonglong)vmss.num_vcpus);
> + hdr.version, (ulonglong)vmss.num_vcpus);
> fprintf(ofp, "Total memory: %llu\n", (ulonglong)vmss.memsize);
>
> - if (vmss.regionscount > 1) {
> - uint64_t holes_sum = 0;
> - unsigned i;
>
> - fprintf(ofp, "Memory regions[%d]:\n", vmss.regionscount);
> - fprintf(ofp, " [0x%016x-", 0);
> - for (i = 0; i < vmss.regionscount - 1; i++) {
> - fprintf(ofp, "0x%016llx]\n", (ulonglong)vmss.regions[i].startpagenum << VMW_PAGE_SHIFT);
> - fprintf(ofp, " [0x%016llx-", (ulonglong)vmss.regions[i].startppn << VMW_PAGE_SHIFT);
> - holes_sum += vmss.regions[i].startppn - vmss.regions[i].startpagenum;
> - }
> - fprintf(ofp, "0x%016llx]\n", (ulonglong)vmss.memsize + (holes_sum << VMW_PAGE_SHIFT));
> + fprintf(ofp, "Memory regions[%d]:\n", vmss.regionscount);
> + fprintf(ofp, " [0x%016x-", 0);
> + for (i = 0; i < vmss.regionscount - 1; i++) {
> + fprintf(ofp, "0x%016llx]\n", (ulonglong)vmss.regions[i].startpagenum << VMW_PAGE_SHIFT);
> + fprintf(ofp, " [0x%016llx-", (ulonglong)vmss.regions[i].startppn << VMW_PAGE_SHIFT);
> + holes_sum += vmss.regions[i].startppn - vmss.regions[i].startpagenum;
> }
> + fprintf(ofp, "0x%016llx]\n", (ulonglong)vmss.memsize + (holes_sum << VMW_PAGE_SHIFT));
>
> return TRUE;
> }
-- Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/ Contribution Guidelines: https://github.com/crash-utility/crash/wiki
