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

[Dave Anderson]

----- Original Message -----
> From: qiaonuohan <qiaonuohan at cn.fujitsu.com>
> Subject: Re: [Crash-utility] [PATCH] add support to "virsh
> dump-guest-memory"(qemu memory dump)
> Date: Tue, 21 Aug 2012 14:05:25 +0800
> 
> >>From c2f4d203d1610213ce2af9754d857f7f2192f121 Mon Sep 17 00:00:00 2001
> > From: qiaonuohan <qiaonuohan at cn.fujitsu.com>
> > Date: Tue, 21 Aug 2012 13:35:54 +0800
> > Subject: [PATCH 2/3] support core dump file when kdump is operating
> > 
> 
> > diff --git a/main.c b/main.c
> > index 9dced6e..033bfa1 100755
> > --- a/main.c
> > +++ b/main.c
> > @@ -640,6 +640,8 @@ main_loop(void)
> >          if (!(pc->flags & GDB_INIT)) {
> >  		gdb_session_init();
> >  		show_untrusted_files();
> > +		if (pc->flags2 & QEMU_MEM_DUMP)
> > +			qemu_mem_dump_kdump_backup_region_init();
> >  		if (SADUMP_DUMPFILE())
> >  			sadump_kdump_backup_region_init();
> >  		if (XEN_HYPER_MODE()) {
> 
> This is different from what Dave suggests. He suggests it's useful if
> this conversion is applied also to Xen part. Now the conversion is
> done for vmcore with qemu note only. The previous patch sets
> QEMU_MEM_DUMP if QEMU note exists. To identify Xen's vmcore, some kind
> of the sign that indicates "this vmcore is Xen's" is needed.
> 
> I'm now thinking that this backup region processing should be written
> commonly among crash dump mechanisms running independently of
> kdump. They are now qemu's dump-guest-memory, xen's dump mechanism and
> sadump.

Wait a minute -- I don't understand how "xen's dump mechanism" has anything
to do with this backup region processing?

-> He suggests it's useful if this conversion is applied also to Xen part.

No, when I referred to Xen with respect to the read_kdump() function, I was only 
indicating that there is an "if xen" statement there that translates a Xen
psuedo-physical address into a Xen machine address -- where the incoming "paddr"
argument gets modified by xen_kdump_p2m() before being passed to the common
read_netump() function.  And given that Qiao's patch is:

(1) also changing the incoming "paddr" argument on the fly, and
(2) it's a kdump clone,

then it makes more sense that his check be done in read_kdump() instead of
in read_netdump().

Anyway, now looking at sadump.c, I see that sadump_kdump_backup_region_init()
and Qiao's qemu_mem_dump_kdump_backup_region_init() function are pretty much
equivalent except for:

(1) qemu_mem_dump_kdump_backup_region_init() supports 32-bit ELF vmcores,
    based upon the setting of the nd->flags KDUMP_ELF32/KDUMP_ELF64 bits, and

(2) qemu_mem_dump_kdump_backup_region_init() uses the pre-existing vmcore_data
    structure from netdump.c, and sadump.c uses its own sadump_data structure,
    where both structures have the same thing at the end of the struct: 

  ...
  /* Backup Region, First 640K of System RAM. */
  #define KEXEC_BACKUP_SRC_END    0x0009ffff
          ulong backup_src_start;
          ulong backup_src_size;
          ulonglong backup_offset;
  };

So I agree that they could be a common function, and we could
have the main_loop() function do something like this: 

        if (!(pc->flags & GDB_INIT)) {
                gdb_session_init();
                show_untrusted_files();
+               kdump_backup_region_init()
-               if (SADUMP_DUMPFILE())
-                       sadump_kdump_backup_region_init();
                if (XEN_HYPER_MODE()) {

The common kdump_backup_region_init() could be put in netdump.c, where
it can check for SADUMP_DUMPFILE() or (pc->flags2 & QEMU_MEM_DUMP), and
if either is set, do its thing.  The main issue would be dealing with the
usage of the two different structures.  netdump.c can safely #include
sadump.h, and there would only be a need for a new global function in
sadump.c similar to this one: 

  struct vmcore_data *get_kdump_vmcore_data(void);

but which passes a pointer to the sadump_data structure back to the 
common function in netdump.c.

What do you guys think?

Dave