1:25 a.m.
Hello Dave,
I made this patch to make crash can analyze core dump file created by
"virsh dump-guest-memory"(I will call it "qemu memory dump" below).
In
my test(guest OS: RHEL6.2 x86 & x86_64), the patch works well with dump
files created by "qemu memory dump".
However, after some investigation, I think I need to discuss further
works with you.
The core dump created by qemu memory dump is similar to kdump. The
distinctness only focuses on note sections. The former one gets
note sections with a name called "QEMU".
1. Some registers' information stored in "CORE" note sections, needed
by crash, also stores in "QEMU" note sections. I think it's not
reasonable to replace them. What do you think?
2. Other registers which are only stored in "QEMU" note sections are
not directly used in crash. I will continue investigating the use of
these registers. And if you give some suggestion, it will be helpful.
--
--
Regards
Qiao Nuohan
3:43 a.m.
From: qiaonuohan <qiaonuohan(a)cn.fujitsu.com>
Subject: [Crash-utility] [PATCH] add support to "virsh dump-guest-memory"(qemu
memory dump)
Date: Fri, 20 Jul 2012 14:25:58 +0800
Hello Dave,
I made this patch to make crash can analyze core dump file created by
"virsh dump-guest-memory"(I will call it "qemu memory dump" below).
In
my test(guest OS: RHEL6.2 x86 & x86_64), the patch works well with
dump
files created by "qemu memory dump".
However, after some investigation, I think I need to discuss further
works with you.
The core dump created by qemu memory dump is similar to kdump. The
distinctness only focuses on note sections. The former one gets
note sections with a name called "QEMU".
1. Some registers' information stored in "CORE" note sections, needed
by crash, also stores in "QEMU" note sections. I think it's not
reasonable to replace them. What do you think?
2. Other registers which are only stored in "QEMU" note sections are
not directly used in crash. I will continue investigating the use of
these registers. And if you give some suggestion, it will be helpful.
qemu memory dump can be triggered when guest machine is in the kdump
2nd kernel. kdump moves the first 640kB in the reserved area, so then
in order to show users the 1st kernel's view, crash needs to interpret
read requests to the first 640kB properly. See sadump implementation,
which does this so seems helpful.
I also think it useful if we can see dump image from the 2nd kernel's
view. Because we can pass phys_base value via command-line, this
amounts to how to calculate phys_base for the 2nd kernel, and this
needs some meaningful 2nd kernel's address that points at
straight-mapping region, and this is IDT value, which is also included
in QEMU values.
Even if there's no commands that explicitly use QEMU values, it's
useful at least to be able to dump them. Users that know what each
value means can take advantage of them in some senses; at least IDT
value is needed in the above case. Though I don't know the best place
to add the feature. help -D is natural?
Thanks.
HATAYAMA, Daisuke
9:20 p.m.
Hello Dave,
The patch has been modified. For every bit of flag has been occupied, I
use flag2 to indicate qemu memory dump.
According to HATAYAMA's suggest, I made another two patch. The 0002
patch is used to deal with the reserved 640KB area. And the 0003 is
used to display register in qemu note section.
Now, I am still working on "see dump image from the 2nd kernel's
view"(See HATAYAMA's mail). As HATAYAMA suggested, only hint message
about calculating phys_base is needed. Then what do think? Is it
suitable add some brief explanation in program_usage_info?
BTW, the following files are what you want, including the core of
RHEL6.2 x86_64 & RHEL6.2 i386
https://skydrive.live.com/redir?resid=62F21E6013CBFCB9!111&authkey=!A...
https://skydrive.live.com/redir?resid=62F21E6013CBFCB9!112&authkey=!A...
https://skydrive.live.com/redir?resid=62F21E6013CBFCB9!113&authkey=!A...
https://skydrive.live.com/redir?resid=62F21E6013CBFCB9!114&authkey=!A...
--
--
Regards
Qiao Nuohan
3:03 a.m.
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?
new macro to replace all the usages of "(pc->flags&
MEMORY_SOURCES)".
My mistake, Qian -- sorry about that...
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).
Dave
--
--
Regards
Qiao Nuohan
8:26 a.m.
----- 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.
> 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?
Dave
2:50 a.m.
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.
Dave
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
--
--
Regards
Qiao Nuohan
1:46 p.m.
----- 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
3:32 a.m.
Hello Dave,
I have modified the patches, and they are based on crash 6.0.9.
--
--
Regards
Qiao Nuohan
2:47 p.m.
----- 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?
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?
In any case, whatever you decide, please move the complete "if" statement
above from read_netdump() into read_kdump(). It makes much more sense for
it to be located in read_kdump(), where for example, the incoming "paddr"
is also modified for Xen ELF core files. You can also use the "paddr_in"
that is declared there instead of the 32-bit-invalid "ulong orig_paddr"
that you currently use within your "if" statement.
Lastly, "help -n" should show the new QEMU_MEM_DUMP_KDUMP_BACKUP
vmcore_data.flag, as well as the 3 new "backup_xxx" fields that
you have added to the vmcore_data structure.
Other than that, it looks pretty good.
Thanks,
Dave
8:29 p.m.
From: Dave Anderson <anderson(a)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@vm-x86 test]$ gcc ./test.c -o test; ./test
not 0x100000000 >= 0xffffffff
0x100000000 < 0xffffffff
Please apply the attached patch.
Thanks.
HATAYAMA, Daisuke
10:09 a.m.
----- Original Message -----
From: Dave Anderson <anderson(a)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@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
1:05 a.m.
At 2012-8-21 3:47, Dave Anderson wrote:
The patch is looking better, but a few issues still remain to be
cleaned up.
The patches have been modified as you recommended. And the third patch
also adds some fields to vmcore_data, so "help -n" is modified here
once again.
--
--
Regards
Qiao Nuohan
1:32 a.m.
From: qiaonuohan <qiaonuohan(a)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(a)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.
Thanks.
HATAYAMA, Daisuke
8:43 a.m.
----- Original Message -----
From: qiaonuohan <qiaonuohan(a)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(a)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
10:18 a.m.
----- Original Message -----
At 2012-8-21 3:47, Dave Anderson wrote:
> The patch is looking better, but a few issues still remain to be
> cleaned up.
The patches have been modified as you recommended. And the third patch
also adds some fields to vmcore_data, so "help -n" is modified here
once again.
--
--
Regards
Qiao Nuohan
Thanks for the updated patch. But I am going to defer it until we
first come to an agreement about how to best consolidate the commonality
between the sadump and qemu-mem-dump formats.
I'd like to accept Daisuke's "backup_src_start" bug-fix, but for a new
common init function, it would seem best that these fields should be
the same:
diff --git a/sadump.h b/sadump.h
index 64c2630..29dce06 100644
--- a/sadump.h
+++ b/sadump.h
@@ -204,7 +204,7 @@ struct sadump_data {
/* Backup Region, First 640K of System RAM. */
#define KEXEC_BACKUP_SRC_END 0x0009ffff
- ulong backup_src_start;
+ ulonglong backup_src_start;
ulong backup_src_size;
ulonglong backup_offset;
};
--
whereas your patch has this:
diff --git a/netdump.h b/netdump.h
index 2e296ad..4a6d661 100644
--- a/netdump.h
+++ b/netdump.h
@@ -71,6 +71,11 @@ struct vmcore_data {
struct xen_kdump_data *xen_kdump_data;
void *vmcoreinfo;
uint size_vmcoreinfo;
+/* Backup Region, First 640K of System RAM. */
+#define KEXEC_BACKUP_SRC_END 0x0009ffff
+ ulong backup_src_start;
+ ulong backup_src_size;
+ ulong backup_offset;
};
Can you change your patch so that they are the same?
And would you like to work on creating the new common
kdump_backup_region_init() function that can handle both
vmcore_data and sadump_data structures?
Thanks,
Dave
3:55 a.m.
At 2012-8-21 23:18, Dave Anderson wrote:
Can you change your patch so that they are the same?
And would you like to work on creating the new common
kdump_backup_region_init() function that can handle both
vmcore_data and sadump_data structures?
Please refer to the attachments. The first two are used to support qemu
memory dump file. And I made the 4th patch to rewrite function
kdump_backup_region_init() based on HATAYAMA's patch(the 3rd patch).
--
--
Regards
Qiao Nuohan
3:20 p.m.
----- Original Message -----
At 2012-8-21 23:18, Dave Anderson wrote:
> Can you change your patch so that they are the same?
>
> And would you like to work on creating the new common
> kdump_backup_region_init() function that can handle both
> vmcore_data and sadump_data structures?
>
Please refer to the attachments. The first two are used to support qemu
memory dump file. And I made the 4th patch to rewrite function
kdump_backup_region_init() based on HATAYAMA's patch(the 3rd patch).
--
--
Regards
Qiao Nuohan
Hello Qiao,
Thanks for consolidating the common SADUMP/QEMU_MEM_DUMP code
into kdump_backup_region_init(). The patch works well, at least
on the two sample QEMU_MEM_DUMP dumpfiles I have. Did you also
verify this patch on an SADUMP dumpfile? (I don't have one
available).
I'd like to make a few small changes/additions that don't affect the
functionality of the patch, but are more for aesthetics/maintainability.
You do *not* have to resubmit the patch again!
FYI, here's what I'm changing:
You created new get_vmcore_data() and get_sadump_data() functions
to get pointers to the vmcore_data and sadump_data structures, but
you have them returning a void *:
void *
get_vmcore_data(void)
{
return nd;
}
void *
get_sadump_data(void)
{
return sd;
}
But your get_vmcore_data() is essentially a duplicate of the existing
get_kdump_vmcore_data(), which returns a pointer to the same structure:
struct vmcore_data *
get_kdump_vmcore_data(void)
{
if (!VMCORE_VALID() || !KDUMP_DUMPFILE())
return NULL;
return &vmcore_data;
}
Anyway, in kdump_backup_region_init(), you call the two functions
to set "vd":
void *vd;
...
if (SADUMP_DUMPFILE()) {
vd = get_sadump_data();
...
} else if (pc->flags2 & QEMU_MEM_DUMP) {
vd = get_vmcore_data();
And then you have constructs such as these where you have to cast "vd"
as the relevant structure:
if (SADUMP_DUMPFILE()) {
((struct sadump_data *)vd)->flags |=
SADUMP_KDUMP_BACKUP;
((struct sadump_data *)vd)->backup_src_start =
backup_src_start;
((struct sadump_data *)vd)->backup_src_size =
backup_src_size;
((struct sadump_data *)vd)->backup_offset =
backup_offset;
} else if (pc->flags2 & QEMU_MEM_DUMP) {
((struct vmcore_data *)vd)->flags |=
QEMU_MEM_DUMP_KDUMP_BACKUP;
((struct vmcore_data *)vd)->backup_src_start =
backup_src_start;
((struct vmcore_data *)vd)->backup_src_size =
backup_src_size;
((struct vmcore_data *)vd)->backup_offset =
backup_offset;
}
Kind of ugly, no?
So I'm going to do it like this, so that the constructs above can
be expressed more clearly:
struct vmcore_data *vd;
struct sadump_data *sd;
...
if (SADUMP_DUMPFILE()) {
sd = get_sadump_data();
...
} else if (pc->flags2 & QEMU_MEM_DUMP) {
vd = get_kdump_vmcore_data();
....
if (SADUMP_DUMPFILE()) {
sd->flags |= SADUMP_KDUMP_BACKUP;
sd->backup_src_start = backup_src_start;
sd->backup_src_size = backup_src_size;
sd->backup_offset = backup_offset;
} else if (pc->flags2 & QEMU_MEM_DUMP) {
vd->flags |= QEMU_MEM_DUMP_KDUMP_BACKUP;
vd->backup_src_start = backup_src_start;
vd->backup_src_size = backup_src_size;
vd->backup_offset = backup_offset;
}
Also, I don't like this declaration/naming convention:
int archflag; /* indicate the arch of core file: 1 -> 32bit */
and then all of the subsequent "if (archflag)" checks in the function.
Since it's not really an "arch", but rather whether it's a 32-bit
dumpfile,
I've changed its name to "is_32_bit" to clarify the subsequent usage.
I also updated the "help" help page itself, and changed the option from
"help -q" to "help -r". That way, if in the future somebody wants to
dump
the registers from some other dumpfile type, it can be done from there.
Anyway, thanks again for all the work you've put into this effort.
Dave
8:28 p.m.
At 2012-8-23 4:20, Dave Anderson wrote:
I also updated the "help" help page itself, and changed the
option from
"help -q" to "help -r". That way, if in the future somebody wants to
dump
the registers from some other dumpfile type, it can be done from there.
Add an info here. The information displayed by "help -q" not only
includes registers.
<cut>
+ netdump_print(" version:%08lx size:%08lx\n",
+ ptr->version, ptr->size);
<cut>
If you think it is not suitable here, deleting it will be fine.
--
--
Regards
Qiao Nuohan
7:45 a.m.
----- Original Message -----
At 2012-8-23 4:20, Dave Anderson wrote:
> I also updated the "help" help page itself, and changed the option from
> "help -q" to "help -r". That way, if in the future somebody
wants to dump
> the registers from some other dumpfile type, it can be done from there.
Add an info here. The information displayed by "help -q" not only
includes registers.
<cut>
+ netdump_print(" version:%08lx size:%08lx\n",
+ ptr->version, ptr->size);
<cut>
If you think it is not suitable here, deleting it will be fine.
OK, I'll just enclose it with CRASHDEBUG(1) in case there's ever a need
to see it.
BTW, did you or Daisuke get a chance to verify this patch with an SADUMP dumpfile?
Dave
2:34 a.m.
At 2012-8-23 20:45, Dave Anderson wrote:
BTW, did you or Daisuke get a chance to verify this patch with an
SADUMP dumpfile?
I have checked the patch with a sadump dumpfile which contains kdump
backup region.
The attachment is some display when analyzing my sadump dumpfile.
Please check it.
--
--
Regards
Qiao Nuohan
7:51 a.m.
----- Original Message -----
At 2012-8-23 20:45, Dave Anderson wrote:
> BTW, did you or Daisuke get a chance to verify this patch with an
> SADUMP dumpfile?
I have checked the patch with a sadump dumpfile which contains kdump
backup region.
The attachment is some display when analyzing my sadump dumpfile.
Please check it.
--
--
Regards
Qiao Nuohan
Thanks Qiao -- queued for crash-6.1.0.
Dave
4327
days inactive
4362
days old
devel@lists.crash-utility.osci.io
21 comments
3 participants
participants (3)
-
Dave Anderson -
HATAYAMA Daisuke -
qiaonuohan