----- Original Message ----- > From: qiaonuohan <qiaonuohan@xxxxxxxxxxxxxx> > Subject: Re: [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@xxxxxxxxxxxxxx> > > 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 -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility