Re: [Crash-utility] [PATCH] add support to "virsh dump-guest-memory"(qemu memory dump)

From: Dave Anderson <anderson(a)redhat.com&gt; Subject: Re: [Crash-utility] [PATCH] add support to "virsh dump-guest-memory"(qemu memory dump) Date: Mon, 20 Aug 2012 15:47:16 -0400 > ----- Original Message ----- >> At 2012-8-20 16:32, qiaonuohan wrote: >> > >> > I have modified the patches, and they are based on crash 6.0.9. >> >> I find some mistakes in my former patches, please ignore them and refer >> to the attachments of this mail. >> >> -- >> -- >> Regards >> Qiao Nuohan > > The patch is looking better, but a few issues still remain to be > cleaned up. > > I'm wondering about the correctness of this addition to read_netdump() > for 32-bit dumpfiles: > > int > read_netdump(int fd, void *bufptr, int cnt, ulong addr, > physaddr_t paddr) > { > off_t offset; > struct pt_load_segment *pls; > int i; > > + if (nd->flags & QEMU_MEM_DUMP_KDUMP_BACKUP && > + paddr >= nd->backup_src_start && > + paddr < nd->backup_src_start + nd->backup_src_size) { > + ulong orig_paddr; > + > + orig_paddr = paddr; > + paddr += nd->backup_offset - nd->backup_src_start; > + > + if (CRASHDEBUG(1)) > + error(INFO, > + "qemu_mem_dump: kdump backup region: %#lx => %#lx\n", > + orig_paddr, paddr); > + } > > The incoming "paddr" parameter is type physaddr_t, which is declared as: > > typedef uint64_t physaddr_t; > > I see that you've pretty much copied the "if" statement from read_sadump(), > but I'm not sure whether SADUMP supports 32-bit dumpfiles? > SADUMP supports 32-bit dumpfiles, so this is a bug. Thanks for pointing out this. > Since nd->backup_src_start is a physical address, maybe it should be > declared as a physaddr_t as well? Or if the incoming paddr is 4GB or > greater, then perhaps it shouldn't be checked against nd->backup_src_start. > In other words, I'm not sure what happens when you do this: > > paddr >= nd->backup_src_start > > When running on a 32-bit machine, does paddr get truncated to a 32-bit value, > or does nd->backup_src_start get promoted to a 64-bit value? > At least next test case on RHEL5.8 32-bit machines shows truncation in 32-bit direction. #include <stdio.h> #include <stdint.h> int main(int argc, char **argv) { uint64_t u = (1ULL << 32); unsigned long i = (unsigned long)-1; if (u >= i) printf("%#llx >= %#x\n", u, i); else printf("not %#llx >= %#x\n", u, i); if (u < i) printf("%#llx < %#x\n", u, i); else printf("not %#llx < %#x\n", u, i); return 0; } [hat@vm-x86 test]$ gcc ./test.c -o test; ./test not 0x100000000 >= 0xffffffff 0x100000000 < 0xffffffff Please apply the attached patch. Thanks. HATAYAMA, Daisuke [Text Documents:0001-sadump-Fix-invalid-truncation-of-physical-address-to.patch]