8:56 a.m.
----- "Dave Anderson" <anderson(a)redhat.com> wrote:
----- "Hu Tao" <hutao(a)cn.fujitsu.com> wrote:
> Hi Dave,
>
> There is a bug on get_be_long() that causes high 32 bits truncated.
> As a result, we get wrong registers values from dump file. Patch 1
> fixes this.
Good catch!
> Once we can get right cpu registers values, it's better to use the
> sp/ip for backtracing the active task. This can show a more accurate
> backtrace, not including those invalid frames beyond sp. pathes 2 and
> 3 do this on kvmdump case(virsh dump).
>
> To verify: run that km_probe.c test module on a x86_64 system, then
> `echo q > /proc/sysrq-trigger' to trigger the kprobe which does
> looping in post_handler. Then vrish dump then crash.
However, I'm wondering whether you actually tested this with a
*crashed* system's dumpfile, and not just with a *live* system dump with
a contrived set of circumstances. And if so, what differences, if any,
did you see with the backtraces of the task that either oops'd or called
panic(), as well as those of the other active tasks that were brought down
by IP interrupt?
Anyway, I'll give this a thorough testing with a set of sample
dumpfiles that I have on hand.
Actually, the patch fails miserably on SMP dumpfiles with segmentation
during initialization.
And now looking at your patch, I'm wondering whether you even tested this
with an SMP system?
The change to qemu_load() here:
@@ -904,6 +906,9 @@ qemu_load (const struct qemu_device_loader *devices, uint32_t
required_features,
if (feof (fp) || ferror (fp))
break;
+ if (STREQ(d->vtbl->name, "cpu"))
+ result->dx86 = d;
+
if (sec == QEMU_VM_SECTION_END || sec == QEMU_VM_SECTION_FULL)
result->features |= features;
}
That function cycles through the "cpu" devices for *each* cpu
in the system, so this patch will store the device of the last cpu
device it encounters. So in an SMP machine, it will store the
device for the highest cpu only, right?
And then there's this change to get_kvmdump_regs():
@@ -310,7 +311,11 @@ kvmdump_memory_dump(FILE *ofp)
void
get_kvmdump_regs(struct bt_info *bt, ulong *pc, ulong *sp)
{
- machdep->get_stack_frame(bt, pc, sp);
+ if (is_task_active(bt->task)) {
+ *sp = device_list->dx86->regs[R_ESP];
+ *pc = device_list->dx86->eip;
+ } else
+ machdep->get_stack_frame(bt, pc, sp);
}
is_task_active() returns TRUE for all active tasks in an SMP system,
not just the panic task. So it would seem you're going to pass
back the same registers for all active tasks?
And what's the point of this change to kernel.c?
diff --git a/kernel.c b/kernel.c
index e399099..2627020 100755
--- a/kernel.c
+++ b/kernel.c
@@ -16,6 +16,7 @@
*/
#include "defs.h"
+#include "qemu-load.h"
#include "xen_hyper_defs.h"
#include <elf.h>
Also, the change to main.c is unnecessary -- there are dozens of malloc'd
memory areas in the program -- so why go to the bother of free'ing
just this one prior to exiting?
Anyway, instead of saving the device list, I suggest you do something
like storing the per-cpu IP/SP values in a separate data structure that
can possibly be used as an alternative source for register values for
"live dumps" -- and possibly for crashing systems if usable starting
hooks cannot be determined in the traditional manner. I had thought
of doing something like that in the past, but when I looked at the
register values, I must have run into the get_be_long() issue?
Dave
7:36 p.m.
New subject: [PATCH] bug on get_be_long() and improvement of bt
On Mon, Oct 18, 2010 at 09:56:41AM -0400, Dave Anderson wrote:
----- "Dave Anderson" <anderson(a)redhat.com> wrote:
> ----- "Hu Tao" <hutao(a)cn.fujitsu.com> wrote:
>
> > Hi Dave,
> >
> > There is a bug on get_be_long() that causes high 32 bits truncated.
> > As a result, we get wrong registers values from dump file. Patch 1
> > fixes this.
>
> Good catch!
>
> > Once we can get right cpu registers values, it's better to use the
> > sp/ip for backtracing the active task. This can show a more accurate
> > backtrace, not including those invalid frames beyond sp. pathes 2 and
> > 3 do this on kvmdump case(virsh dump).
> >
> > To verify: run that km_probe.c test module on a x86_64 system, then
> > `echo q > /proc/sysrq-trigger' to trigger the kprobe which does
> > looping in post_handler. Then vrish dump then crash.
>
> However, I'm wondering whether you actually tested this with a
> *crashed* system's dumpfile, and not just with a *live* system dump with
> a contrived set of circumstances. And if so, what differences, if any,
> did you see with the backtraces of the task that either oops'd or called
> panic(), as well as those of the other active tasks that were brought down
> by IP interrupt?
>
> Anyway, I'll give this a thorough testing with a set of sample
> dumpfiles that I have on hand.
Actually, the patch fails miserably on SMP dumpfiles with segmentation
during initialization.
And now looking at your patch, I'm wondering whether you even tested this
with an SMP system?
My fault. I will do more tests and improve the patch.
* test with a SMP system
* test with a live dump
* test with system panic/oops
* test with x86/x86_64
The change to qemu_load() here:
@@ -904,6 +906,9 @@ qemu_load (const struct qemu_device_loader *devices, uint32_t
required_features,
if (feof (fp) || ferror (fp))
break;
+ if (STREQ(d->vtbl->name, "cpu"))
+ result->dx86 = d;
+
if (sec == QEMU_VM_SECTION_END || sec == QEMU_VM_SECTION_FULL)
result->features |= features;
}
That function cycles through the "cpu" devices for *each* cpu
in the system, so this patch will store the device of the last cpu
device it encounters. So in an SMP machine, it will store the
device for the highest cpu only, right?
And then there's this change to get_kvmdump_regs():
@@ -310,7 +311,11 @@ kvmdump_memory_dump(FILE *ofp)
void
get_kvmdump_regs(struct bt_info *bt, ulong *pc, ulong *sp)
{
- machdep->get_stack_frame(bt, pc, sp);
+ if (is_task_active(bt->task)) {
+ *sp = device_list->dx86->regs[R_ESP];
+ *pc = device_list->dx86->eip;
+ } else
+ machdep->get_stack_frame(bt, pc, sp);
}
is_task_active() returns TRUE for all active tasks in an SMP system,
not just the panic task. So it would seem you're going to pass
back the same registers for all active tasks?
And what's the point of this change to kernel.c?
diff --git a/kernel.c b/kernel.c
index e399099..2627020 100755
--- a/kernel.c
+++ b/kernel.c
@@ -16,6 +16,7 @@
*/
#include "defs.h"
+#include "qemu-load.h"
#include "xen_hyper_defs.h"
#include <elf.h>
Also, the change to main.c is unnecessary -- there are dozens of malloc'd
memory areas in the program -- so why go to the bother of free'ing
just this one prior to exiting?
Anyway, instead of saving the device list, I suggest you do something
like storing the per-cpu IP/SP values in a separate data structure that
can possibly be used as an alternative source for register values for
"live dumps" -- and possibly for crashing systems if usable starting
hooks cannot be determined in the traditional manner. I had thought
of doing something like that in the past, but when I looked at the
register values, I must have run into the get_be_long() issue?
Sounds reasonable. Thanks for suggestions.
--
Thanks,
Hu Tao
4:05 a.m.
New subject: [PATCH] bug on get_be_long() and improvement of bt
Hi Dave,
These are updated patches tested with SMP system and panic task.
When testing a x86 guest, I found another bug about reading cpu
registers from dumpfile. Qemu simulated system is x86_64
(qemu-system-x86_64), guest OS is x86. When crash reads cpu registers
from dumpfile, it uses cpu_load_32(), this will read gp registers by
get_be_long(fp, 32), that is, treate them as 32bits. But in fact,
qemu-system-x86_64 saves 64bits for each of them(although guest OS
uses only lower 32 bits). As a result, crash gets wrong cpu gp
register values.
Is there any way we can know from dumpfile that these gp
registers(and those similar registers) are 32bits or 64bits?
--
Thanks,
Hu Tao
2:51 a.m.
New subject: [PATCH] bug on get_be_long() and improvement of bt
And one more patch adding command `cpu' to show cpu registers.
--
Thanks,
Hu Tao
8 a.m.
New subject: [PATCH] bug on get_be_long() and improvement of bt
----- "Hu Tao" <hutao(a)cn.fujitsu.com> wrote:
And one more patch adding command `cpu' to show cpu registers.
I had considered a "regs" command in the past, but given that they
are not always available in the various dumpfiles, and if implemented
should be done for all architectures, and quite frankly they're often
not that interesting because in, say ELF core dumps, they're typically
redundantly displayed in the backtrace exception frame.
Since this is KVM-specific and x86/x86_64-specific, for now I will
fold your patch into "help -n", which dumps dumpfile-specific data.
We can revisit making it a new command if it can be made more
generally used.
Thanks,
Dave
5001
days inactive
5003
days old
devel@lists.crash-utility.osci.io
4 comments
2 participants
participants (2)
-
Dave Anderson -
Hu Tao