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

[Dave Anderson]

----- Original Message -----
> At 2012-8-8 21:26, Dave Anderson wrote:
> >
> >
> > ----- Original Message -----
> >> At 2012-8-8 2:59, Dave Anderson wrote:
> >>
> >>   >
> >>   >  Anyway, it was surprising to note is that the two dumpfiles can already 
> >>   >  can be read with no problem by the crash utility -- with no additional
> >>   >  patches to support it.  The crash utility just thinks that it's a kdump
> >>   >  with some unknown "QEMU" ELF notes:
> >>
> >> That's right. The formats are extremely similar, except the qemu note
> >> section.
> >>
> >>   >  I should have suggested moving one of the currently-existing pc->flags bits into
> >>   >  pc->flags2, and keeping all the dumpfile types in pc->flags. Or at least create a
> >>
> >> I think it's a better choice. But encountering a new type of dump file,
> >> creating a patch used to move bits in pc->flags can easily put things
> >> into a mess. Why not use a new flag only used to keep dumpfile types?
> >
> > Yes, that's probably a better idea.  When the next "real" dumpfile type
> > comes along, perhaps we can go that route.
> 
> I see. And I reworked the patches to treat qemu memory dump as kdump.
> >
> >> >  But -- it seems that we can just leave the dumpfile type as a "KDUMP_ELF32/KDUMP_ELF64",
> >> >  and then based upon the existence of a "QEMU" note, just set a QEMU_MEM_DUMP pc->flags2
> >> >  bit that can be used everywhere that you're interested in accessing the "QEMU"
> >> >  notes/registers section?  Wouldn't that be far simpler?
> >>
> >> Treat it as a kdump files seems cool to me. The only problem is I still
> >> need to distinguish kdump and qemu memory dump, not only using qemu note
> >> section, but also the first 640K is special(when doing qemu memory dump,
> >> the second kernel is working).
> >
> > Right, and you will be able to set the new QEMU_MEM_DUMP flag very early
> > on during the dumpfile determination phase so that later you will have
> > that knowledge at hand later on when you want to deal with the notes.
> >
> > With respect to the "second-kernel" issue, I would presume that you would
> > have to use the currently-existing "--machdep phys_base=<physaddr>" capability
> > for x86_64?
> 
> Yes, that's right. To see dump image from the 2nd kernel's view, 
> phys_base should be set.
> 
> P.S.
> I will suspend the work for about one week because of personal affairs.

Hello Qiao,

Here are my comments on your latest patch.

First, I have a couple problems with your check_elf_note() implementation.
It gets called regardless whether it's a QEMU-generated dumpfile or not.
You call the function here in is_netdump():

                if ((load32->p_offset & (MIN_PAGE_SIZE-1)) &&
                    (load32->p_align == 0)) {
                        tmp_flags |= KDUMP_ELF32;
                        if (check_elf_note(eheader, fd, file, 0))
                                pc->flags2 |= QEMU_MEM_DUMP;
                } else


               if ((load64->p_offset & (MIN_PAGE_SIZE-1)) &&
                    (load64->p_align == 0)) {
                        tmp_flags |= KDUMP_ELF64;
                        if (check_elf_note(eheader, fd, file, 1))
                                pc->flags2 |= QEMU_MEM_DUMP;
                } else

That forces *all* non-QEMU netdumps and kdumps to run through the
complete check_elf_note() function for no reason at all.

But more importantly, the check_elf_note() is a huge duplication
of effort.  It completely parses the ELF header looking for ELF
notes, and when it finds a "QEMU" note string:

                if (STREQ(name, "QEMU")) {
                        qemu_memory_dump = TRUE;
                        break;
                }

it ends up returning TRUE back to is_netdump(), which sets the 
QEMU_MEM_DUMP flag.

But then later on, is_netdump() calls either dump_Elf32_Nhdr()
or dump_Elf64_Nhdr for every ELF note.  And in those two functions
you've added this:

               qemu_info = STRNEQ(buf, "QEMU"); 

in order to decide whether to display the register data.

But given that you are again just checking for the "QEMU" string in 
those two functions above, then check_elf_note() is completely
unnecessary.  Just set the QEMU_MEM_DUMP flag in those two locations
in dump_Elf32_Nhdr() and dump_Elf64_Nhdr() -- that's what those two 
function are there for.

And lastly, the output of "crash -d1" and "help -n" is really kind
of ugly.  Plus this looks to be a bug in both dump_Elf32_Nhdr() 
and dump_Elf64_Nhdr() where you've got:

        if (CRASHDEBUG(1) & qemu_info) { 

It should be "&&" so that "help -n" does not display the register
data unless CRASHDEBUG(x) is turned on.

But speaking to the "ugliness" issue, it really is confusing to
see the "normal" PT_NOTE raw data display interspersed with your 
new QEMU register translation.  

How about separating it into a unique display?  Maybe something
like "help -q" could do something like:

 crash> help q
 CPU 0:
   <register data>
 CPU 1:
   <register data>
 ...

Thanks,
  Dave