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

[Dave Anderson]

----- Original Message -----
> 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
> 
> 
> [Text
> Documents:0001-sadump-Fix-invalid-truncation-of-physical-address-to.patch]
> 

Thanks Daisuke -- queued for crash-6.1.0.

Dave

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: text/x-patch
Size: 2314 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20120821/86061618/attachment.bin>