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

[HATAYAMA Daisuke]

From: Dave Anderson <anderson at redhat.com>
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 at vm-x86 test]$ gcc ./test.c -o test; ./test
not 0x100000000 >= 0xffffffff
0x100000000 < 0xffffffff

Please apply the attached patch.

Thanks.
HATAYAMA, Daisuke
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sadump-Fix-invalid-truncation-of-physical-address-to.patch
Type: text/x-patch
Size: 2314 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20120821/b77bb388/attachment.bin>