All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-01  5:35 ` Suraj Jitindar Singh
  0 siblings, 0 replies; 27+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-01  5:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: dgilbert, armbru, david, qemu-ppc, Suraj Jitindar Singh

Add a monitor command "dump-stack" to be used to dump the stack for the
current cpu.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 hmp-commands.hx   | 13 +++++++++++++
 hmp.h             |  1 +
 include/qom/cpu.h | 10 ++++++++++
 monitor.c         | 12 ++++++++++++
 qom/cpu.c         | 10 ++++++++++
 5 files changed, 46 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9b4035965c..965ccdea28 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -862,6 +862,19 @@ ETEXI
     },
 
 STEXI
+@item dump-stack
+@findex dump-stack
+dump stack of the cpu
+ETEXI
+    {
+        .name           = "dump-stack",
+        .args_type      = "",
+        .params         = "",
+        .help           = "dump stack",
+        .cmd            = hmp_dumpstack,
+    },
+
+STEXI
 @item pmemsave @var{addr} @var{size} @var{file}
 @findex pmemsave
 save to disk physical memory dump starting at @var{addr} of size @var{size}.
diff --git a/hmp.h b/hmp.h
index 43617f2646..e6edf1215c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
+void hmp_dumpstack(Monitor *mon, const QDict *qdict);
 void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
 void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 08abcbd3fe..f2e83e9918 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -181,6 +181,7 @@ typedef struct CPUClass {
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
     void (*dump_state)(CPUState *cpu, FILE *, int flags);
+    void (*dump_stack)(CPUState *cpu, FILE *f);
     GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
     void (*dump_statistics)(CPUState *cpu, int flags);
     int64_t (*get_arch_id)(CPUState *cpu);
@@ -568,6 +569,15 @@ enum CPUDumpFlags {
 void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 
 /**
+ * cpu_dump_stack:
+ * @cpu: The CPU whose stack is to be dumped.
+ * @f: If non-null, dump to this stream, else to current print sink.
+ *
+ * Dumps CPU stack.
+ */
+void cpu_dump_stack(CPUState *cpu, FILE *f);
+
+/**
  * cpu_dump_statistics:
  * @cpu: The CPU whose state is to be dumped.
  * @flags: Flags what to dump.
diff --git a/monitor.c b/monitor.c
index 9b5f10b475..dbec2e4376 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_dumpstack(Monitor *mon, const QDict *qdict)
+{
+    CPUState *cs = mon_get_cpu();
+
+    if (!cs) {
+        monitor_printf(mon, "No CPU available\n");
+        return;
+    }
+
+    cpu_dump_stack(cs, NULL);
+}
+
 #ifdef CONFIG_TCG
 static void hmp_info_jit(Monitor *mon, const QDict *qdict)
 {
diff --git a/qom/cpu.c b/qom/cpu.c
index 3c5493c96c..0dc10004f4 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
     }
 }
 
+void cpu_dump_stack(CPUState *cpu, FILE *f)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->dump_stack) {
+        cpu_synchronize_state(cpu);
+        cc->dump_stack(cpu, f);
+    }
+}
+
 void cpu_dump_statistics(CPUState *cpu, int flags)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-01  5:35 ` Suraj Jitindar Singh
  0 siblings, 0 replies; 27+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-01  5:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: david, qemu-ppc, dgilbert, Suraj Jitindar Singh, armbru

Add a monitor command "dump-stack" to be used to dump the stack for the
current cpu.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 hmp-commands.hx   | 13 +++++++++++++
 hmp.h             |  1 +
 include/qom/cpu.h | 10 ++++++++++
 monitor.c         | 12 ++++++++++++
 qom/cpu.c         | 10 ++++++++++
 5 files changed, 46 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9b4035965c..965ccdea28 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -862,6 +862,19 @@ ETEXI
     },
 
 STEXI
+@item dump-stack
+@findex dump-stack
+dump stack of the cpu
+ETEXI
+    {
+        .name           = "dump-stack",
+        .args_type      = "",
+        .params         = "",
+        .help           = "dump stack",
+        .cmd            = hmp_dumpstack,
+    },
+
+STEXI
 @item pmemsave @var{addr} @var{size} @var{file}
 @findex pmemsave
 save to disk physical memory dump starting at @var{addr} of size @var{size}.
diff --git a/hmp.h b/hmp.h
index 43617f2646..e6edf1215c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
 void hmp_cpu(Monitor *mon, const QDict *qdict);
 void hmp_memsave(Monitor *mon, const QDict *qdict);
 void hmp_pmemsave(Monitor *mon, const QDict *qdict);
+void hmp_dumpstack(Monitor *mon, const QDict *qdict);
 void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
 void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
 void hmp_cont(Monitor *mon, const QDict *qdict);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 08abcbd3fe..f2e83e9918 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -181,6 +181,7 @@ typedef struct CPUClass {
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
     void (*dump_state)(CPUState *cpu, FILE *, int flags);
+    void (*dump_stack)(CPUState *cpu, FILE *f);
     GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
     void (*dump_statistics)(CPUState *cpu, int flags);
     int64_t (*get_arch_id)(CPUState *cpu);
@@ -568,6 +569,15 @@ enum CPUDumpFlags {
 void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 
 /**
+ * cpu_dump_stack:
+ * @cpu: The CPU whose stack is to be dumped.
+ * @f: If non-null, dump to this stream, else to current print sink.
+ *
+ * Dumps CPU stack.
+ */
+void cpu_dump_stack(CPUState *cpu, FILE *f);
+
+/**
  * cpu_dump_statistics:
  * @cpu: The CPU whose state is to be dumped.
  * @flags: Flags what to dump.
diff --git a/monitor.c b/monitor.c
index 9b5f10b475..dbec2e4376 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_dumpstack(Monitor *mon, const QDict *qdict)
+{
+    CPUState *cs = mon_get_cpu();
+
+    if (!cs) {
+        monitor_printf(mon, "No CPU available\n");
+        return;
+    }
+
+    cpu_dump_stack(cs, NULL);
+}
+
 #ifdef CONFIG_TCG
 static void hmp_info_jit(Monitor *mon, const QDict *qdict)
 {
diff --git a/qom/cpu.c b/qom/cpu.c
index 3c5493c96c..0dc10004f4 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
     }
 }
 
+void cpu_dump_stack(CPUState *cpu, FILE *f)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->dump_stack) {
+        cpu_synchronize_state(cpu);
+        cc->dump_stack(cpu, f);
+    }
+}
+
 void cpu_dump_statistics(CPUState *cpu, int flags)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
-- 
2.13.6



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-01  5:35   ` Suraj Jitindar Singh
  0 siblings, 0 replies; 27+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-01  5:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: dgilbert, armbru, david, qemu-ppc, Suraj Jitindar Singh

The monitor function dump-stack is used to dump the stack for a cpu.
This can be useful for debugging purposes when the stack cannot be
dumped by another means.

Add a ppc implementation ppc_cpu_dump_stack().
The stack pointer is stored in R1 with the back pointer at offset 0 and
the link register at offset 2.
Also dump the registers from the stack frame if the marker "regshere" is
found.
This only dumps the kernel stack, stopping if a non-kernel address is
found in the stack.

Sample output:
(qemu) dump-stack
sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
	trap : 0x0000000000000700
	pc   : 0xc000000000104490
	msr  : 0x9000000002843003
	lr   : 0xc000000000103ffc
	gpr 0: 0x0000000000000001
	gpr 1: 0xc00000005051f530
	gpr 2: 0xc000000001088200
	gpr 3: 0x0000000000000001
	gpr 4: 0xc000000032d60000
	gpr 5: 0xc0000000014b8f00
	gpr 6: 0x0000000000c835e0
	gpr 7: 0x0000000000000000
	gpr 8: 0x0000000000000000
	gpr 9: 0xc000000032f00000
	gpr10: 0x9000000002803033
	gpr11: 0xc000000000b60f00
	gpr12: 0x0000000000002000
	gpr13: 0xc000000001250000
	gpr14: 0x0000000000000000
	gpr15: 0x0000000000000008
	gpr16: 0x0000000000000000
	gpr17: 0xc00000000114f790
	gpr18: 0x00000000ffffffff
	gpr19: 0xc00000005051f8e8
	gpr20: 0x0000000000000001
	gpr21: 0x0000000000000000
	gpr22: 0x0000000000000001
	gpr23: 0x0000000000000001
	gpr24: 0x0000000000000001
	gpr25: 0xc0000000014b8f70
	gpr26: 0x0000000000000000
	gpr27: 0x0000000000000001
	gpr28: 0x0000000000000001
	gpr29: 0x0000000000000000
	gpr30: 0xc0000000014b8f00
	gpr31: 0xc0000000014b8f00
sp: 0xc00000005051f530 lr: 0x0000000000000000
sp: 0xc00000005051f600 lr: 0xc000000000103ffc
sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
	trap : 0x0000000000000c01
	pc   : 0x00007fffa6c9d8d0
	msr  : 0x900000000280f033
	lr   : 0x0000000010090f40
	gpr 0: 0x0000000000000036
	gpr 1: 0x00007fffa62fdd70
	gpr 2: 0x00007fffa6d57300
	gpr 3: 0x000000000000000d
	gpr 4: 0x000000002000ae80
	gpr 5: 0x0000000000000000
	gpr 6: 0x0000000000000537
	gpr 7: 0x0000000000000000
	gpr 8: 0x000000000000000d
	gpr 9: 0x0000000000000000
	gpr10: 0x0000000000000000
	gpr11: 0x0000000000000000
	gpr12: 0x0000000000000000
	gpr13: 0x00007fffa6306380
	gpr14: 0x0000000000000000
	gpr15: 0x0000000000000001
	gpr16: 0x0000000039ba6928
	gpr17: 0x0000000000000000
	gpr18: 0x0000000000000000
	gpr19: 0x00007fffa6d702f0
	gpr20: 0x00007fffa62fddf0
	gpr21: 0x0000000000000080
	gpr22: 0x0000000000000004
	gpr23: 0x0000000000000000
	gpr24: 0x0000000010ac85c0
	gpr25: 0x0000000000000008
	gpr26: 0x00007fffa62fde10
	gpr27: 0x0000000000000000
	gpr28: 0x0000000000000002
	gpr29: 0x0000000000000000
	gpr30: 0x0000000039ba6900
	gpr31: 0x0000000010ac85c0
sp: 0x00007fffa62fdd70

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 target/ppc/cpu.h                |  1 +
 target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c |  1 +
 3 files changed, 62 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 5e7cf54b2f..28c4dffca1 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
 void ppc_cpu_do_interrupt(CPUState *cpu);
 bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
 void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
+void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
 void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
 hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 8d08625c33..b162998ce7 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
 #undef RFPL
 }
 
+struct ppc_pt_regs {
+        unsigned long gpr[32];
+        unsigned long nip;
+        unsigned long msr;
+        unsigned long orig_gpr3;
+        unsigned long ctr;
+        unsigned long link;
+        unsigned long xer;
+        unsigned long ccr;
+        unsigned long softe;
+        unsigned long trap;
+        unsigned long dar;
+        unsigned long dsisr;
+        unsigned long result;
+};
+
+void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
+{
+#if defined(TARGET_PPC64)
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    uint64_t sp, next_sp, lr, buf[4];
+
+    /* stack pointer stored in r1 */
+    sp = env->gpr[1];
+
+    while (sp && (sp & (0xCUL << 60))) {
+        uint64_t marker = 0UL;
+
+        /* read and print LR */
+        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
+        next_sp = buf[0];
+        lr = buf[2];
+        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
+        sp &= ~(0xCUL << 60);
+
+        /* Does the stackframe contain regs? */
+        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
+        if (marker == 0x7265677368657265) { /* regshere */
+            struct ppc_pt_regs regs;
+            int i;
+
+            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
+
+            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
+            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
+            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
+            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
+            for (i = 0; i < 32; i++)
+                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
+                            regs.gpr[i]);
+        }
+
+        sp = next_sp;
+    }
+
+    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
+#endif
+}
+
 void ppc_cpu_dump_statistics(CPUState *cs, int flags)
 {
 #if defined(DO_PPC_STATISTICS)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 0394a9ddad..3fd24f85cc 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = ppc_cpu_do_interrupt;
     cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
     cc->dump_state = ppc_cpu_dump_state;
+    cc->dump_stack = ppc_cpu_dump_stack;
     cc->dump_statistics = ppc_cpu_dump_statistics;
     cc->set_pc = ppc_cpu_set_pc;
     cc->gdb_read_register = ppc_cpu_gdb_read_register;
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-01  5:35   ` Suraj Jitindar Singh
  0 siblings, 0 replies; 27+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-01  5:35 UTC (permalink / raw)
  To: qemu-devel; +Cc: david, qemu-ppc, dgilbert, Suraj Jitindar Singh, armbru

The monitor function dump-stack is used to dump the stack for a cpu.
This can be useful for debugging purposes when the stack cannot be
dumped by another means.

Add a ppc implementation ppc_cpu_dump_stack().
The stack pointer is stored in R1 with the back pointer at offset 0 and
the link register at offset 2.
Also dump the registers from the stack frame if the marker "regshere" is
found.
This only dumps the kernel stack, stopping if a non-kernel address is
found in the stack.

Sample output:
(qemu) dump-stack
sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
	trap : 0x0000000000000700
	pc   : 0xc000000000104490
	msr  : 0x9000000002843003
	lr   : 0xc000000000103ffc
	gpr 0: 0x0000000000000001
	gpr 1: 0xc00000005051f530
	gpr 2: 0xc000000001088200
	gpr 3: 0x0000000000000001
	gpr 4: 0xc000000032d60000
	gpr 5: 0xc0000000014b8f00
	gpr 6: 0x0000000000c835e0
	gpr 7: 0x0000000000000000
	gpr 8: 0x0000000000000000
	gpr 9: 0xc000000032f00000
	gpr10: 0x9000000002803033
	gpr11: 0xc000000000b60f00
	gpr12: 0x0000000000002000
	gpr13: 0xc000000001250000
	gpr14: 0x0000000000000000
	gpr15: 0x0000000000000008
	gpr16: 0x0000000000000000
	gpr17: 0xc00000000114f790
	gpr18: 0x00000000ffffffff
	gpr19: 0xc00000005051f8e8
	gpr20: 0x0000000000000001
	gpr21: 0x0000000000000000
	gpr22: 0x0000000000000001
	gpr23: 0x0000000000000001
	gpr24: 0x0000000000000001
	gpr25: 0xc0000000014b8f70
	gpr26: 0x0000000000000000
	gpr27: 0x0000000000000001
	gpr28: 0x0000000000000001
	gpr29: 0x0000000000000000
	gpr30: 0xc0000000014b8f00
	gpr31: 0xc0000000014b8f00
sp: 0xc00000005051f530 lr: 0x0000000000000000
sp: 0xc00000005051f600 lr: 0xc000000000103ffc
sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
	trap : 0x0000000000000c01
	pc   : 0x00007fffa6c9d8d0
	msr  : 0x900000000280f033
	lr   : 0x0000000010090f40
	gpr 0: 0x0000000000000036
	gpr 1: 0x00007fffa62fdd70
	gpr 2: 0x00007fffa6d57300
	gpr 3: 0x000000000000000d
	gpr 4: 0x000000002000ae80
	gpr 5: 0x0000000000000000
	gpr 6: 0x0000000000000537
	gpr 7: 0x0000000000000000
	gpr 8: 0x000000000000000d
	gpr 9: 0x0000000000000000
	gpr10: 0x0000000000000000
	gpr11: 0x0000000000000000
	gpr12: 0x0000000000000000
	gpr13: 0x00007fffa6306380
	gpr14: 0x0000000000000000
	gpr15: 0x0000000000000001
	gpr16: 0x0000000039ba6928
	gpr17: 0x0000000000000000
	gpr18: 0x0000000000000000
	gpr19: 0x00007fffa6d702f0
	gpr20: 0x00007fffa62fddf0
	gpr21: 0x0000000000000080
	gpr22: 0x0000000000000004
	gpr23: 0x0000000000000000
	gpr24: 0x0000000010ac85c0
	gpr25: 0x0000000000000008
	gpr26: 0x00007fffa62fde10
	gpr27: 0x0000000000000000
	gpr28: 0x0000000000000002
	gpr29: 0x0000000000000000
	gpr30: 0x0000000039ba6900
	gpr31: 0x0000000010ac85c0
sp: 0x00007fffa62fdd70

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 target/ppc/cpu.h                |  1 +
 target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
 target/ppc/translate_init.inc.c |  1 +
 3 files changed, 62 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 5e7cf54b2f..28c4dffca1 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
 void ppc_cpu_do_interrupt(CPUState *cpu);
 bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
 void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
+void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
 void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
 hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 8d08625c33..b162998ce7 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
 #undef RFPL
 }
 
+struct ppc_pt_regs {
+        unsigned long gpr[32];
+        unsigned long nip;
+        unsigned long msr;
+        unsigned long orig_gpr3;
+        unsigned long ctr;
+        unsigned long link;
+        unsigned long xer;
+        unsigned long ccr;
+        unsigned long softe;
+        unsigned long trap;
+        unsigned long dar;
+        unsigned long dsisr;
+        unsigned long result;
+};
+
+void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
+{
+#if defined(TARGET_PPC64)
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    uint64_t sp, next_sp, lr, buf[4];
+
+    /* stack pointer stored in r1 */
+    sp = env->gpr[1];
+
+    while (sp && (sp & (0xCUL << 60))) {
+        uint64_t marker = 0UL;
+
+        /* read and print LR */
+        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
+        next_sp = buf[0];
+        lr = buf[2];
+        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
+        sp &= ~(0xCUL << 60);
+
+        /* Does the stackframe contain regs? */
+        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
+        if (marker == 0x7265677368657265) { /* regshere */
+            struct ppc_pt_regs regs;
+            int i;
+
+            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
+
+            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
+            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
+            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
+            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
+            for (i = 0; i < 32; i++)
+                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
+                            regs.gpr[i]);
+        }
+
+        sp = next_sp;
+    }
+
+    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
+#endif
+}
+
 void ppc_cpu_dump_statistics(CPUState *cs, int flags)
 {
 #if defined(DO_PPC_STATISTICS)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 0394a9ddad..3fd24f85cc 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = ppc_cpu_do_interrupt;
     cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
     cc->dump_state = ppc_cpu_dump_state;
+    cc->dump_stack = ppc_cpu_dump_stack;
     cc->dump_statistics = ppc_cpu_dump_statistics;
     cc->set_pc = ppc_cpu_set_pc;
     cc->gdb_read_register = ppc_cpu_gdb_read_register;
-- 
2.13.6



^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-01  9:48     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 27+ messages in thread
From: Alexey Kardashevskiy @ 2019-05-01  9:48 UTC (permalink / raw)
  To: Suraj Jitindar Singh, qemu-devel; +Cc: david, qemu-ppc, dgilbert, armbru



On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
> The monitor function dump-stack is used to dump the stack for a cpu.
> This can be useful for debugging purposes when the stack cannot be
> dumped by another means.
> 
> Add a ppc implementation ppc_cpu_dump_stack().
> The stack pointer is stored in R1 with the back pointer at offset 0 and
> the link register at offset 2.
> Also dump the registers from the stack frame if the marker "regshere" is
> found.

Is this a Linux only marker? ABI does not mentioned this.

> This only dumps the kernel stack, stopping if a non-kernel address is
> found in the stack.

Why enforce this limit?

> 
> Sample output:
> (qemu) dump-stack
> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> 	trap : 0x0000000000000700
> 	pc   : 0xc000000000104490
> 	msr  : 0x9000000002843003
> 	lr   : 0xc000000000103ffc
> 	gpr 0: 0x0000000000000001
> 	gpr 1: 0xc00000005051f530
> 	gpr 2: 0xc000000001088200
> 	gpr 3: 0x0000000000000001
> 	gpr 4: 0xc000000032d60000
> 	gpr 5: 0xc0000000014b8f00
> 	gpr 6: 0x0000000000c835e0
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x0000000000000000
> 	gpr 9: 0xc000000032f00000
> 	gpr10: 0x9000000002803033
> 	gpr11: 0xc000000000b60f00
> 	gpr12: 0x0000000000002000
> 	gpr13: 0xc000000001250000
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000008
> 	gpr16: 0x0000000000000000
> 	gpr17: 0xc00000000114f790
> 	gpr18: 0x00000000ffffffff
> 	gpr19: 0xc00000005051f8e8
> 	gpr20: 0x0000000000000001
> 	gpr21: 0x0000000000000000
> 	gpr22: 0x0000000000000001
> 	gpr23: 0x0000000000000001
> 	gpr24: 0x0000000000000001
> 	gpr25: 0xc0000000014b8f70
> 	gpr26: 0x0000000000000000
> 	gpr27: 0x0000000000000001
> 	gpr28: 0x0000000000000001
> 	gpr29: 0x0000000000000000
> 	gpr30: 0xc0000000014b8f00
> 	gpr31: 0xc0000000014b8f00

Looks bulky, using the "info registers" format would make sense here.


> sp: 0xc00000005051f530 lr: 0x0000000000000000
> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> 	trap : 0x0000000000000c01
> 	pc   : 0x00007fffa6c9d8d0
> 	msr  : 0x900000000280f033
> 	lr   : 0x0000000010090f40
> 	gpr 0: 0x0000000000000036
> 	gpr 1: 0x00007fffa62fdd70
> 	gpr 2: 0x00007fffa6d57300
> 	gpr 3: 0x000000000000000d
> 	gpr 4: 0x000000002000ae80
> 	gpr 5: 0x0000000000000000
> 	gpr 6: 0x0000000000000537
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x000000000000000d
> 	gpr 9: 0x0000000000000000
> 	gpr10: 0x0000000000000000
> 	gpr11: 0x0000000000000000
> 	gpr12: 0x0000000000000000
> 	gpr13: 0x00007fffa6306380
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000001
> 	gpr16: 0x0000000039ba6928
> 	gpr17: 0x0000000000000000
> 	gpr18: 0x0000000000000000
> 	gpr19: 0x00007fffa6d702f0
> 	gpr20: 0x00007fffa62fddf0
> 	gpr21: 0x0000000000000080
> 	gpr22: 0x0000000000000004
> 	gpr23: 0x0000000000000000
> 	gpr24: 0x0000000010ac85c0
> 	gpr25: 0x0000000000000008
> 	gpr26: 0x00007fffa62fde10
> 	gpr27: 0x0000000000000000
> 	gpr28: 0x0000000000000002
> 	gpr29: 0x0000000000000000
> 	gpr30: 0x0000000039ba6900
> 	gpr31: 0x0000000010ac85c0
> sp: 0x00007fffa62fdd70
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  target/ppc/cpu.h                |  1 +
>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c |  1 +
>  3 files changed, 62 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5e7cf54b2f..28c4dffca1 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 8d08625c33..b162998ce7 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #undef RFPL
>  }
>  
> +struct ppc_pt_regs {
> +        unsigned long gpr[32];
> +        unsigned long nip;
> +        unsigned long msr;
> +        unsigned long orig_gpr3;
> +        unsigned long ctr;
> +        unsigned long link;
> +        unsigned long xer;
> +        unsigned long ccr;
> +        unsigned long softe;
> +        unsigned long trap;
> +        unsigned long dar;
> +        unsigned long dsisr;
> +        unsigned long result;
> +};
> +
> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> +{
> +#if defined(TARGET_PPC64)
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t sp, next_sp, lr, buf[4];

These are hwaddr really.

> +
> +    /* stack pointer stored in r1 */
> +    sp = env->gpr[1];
> +
> +    while (sp && (sp & (0xCUL << 60))) {
> +        uint64_t marker = 0UL;

sp = ppc_cpu_get_phys_page_debug(cs, sp) | (sp & ~TARGET_PAGE_MASK);

and finish the loop when ppc_cpu_get_phys_page_debug returns -1?

> +
> +        /* read and print LR */
> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);

and s/ & ~(0xCUL << 60)//

> +        next_sp = buf[0];
> +        lr = buf[2];

These two need to be converted from guest endian. For a BE guest, I see:

sp: 0x000000007e582ff0 lr: 0xe4e60a00000000c0
sp: 0xffffffffffffffff lr: 0x0000600000006000
sp: 0x0000600000006000


> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);

HWADDR_PRIx. Or at least PRIx64, otherwise it won't compile on 32bit or
x86 or somewhere else.


> +        sp &= ~(0xCUL << 60);

and remove this line. And now you can dump



> +
> +        /* Does the stackframe contain regs? */
> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));

I suspect the marker needs byteswap as well.

What is that 96?

> +        if (marker == 0x7265677368657265) { /* regshere */
> +            struct ppc_pt_regs regs;
> +            int i;
> +
> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));

and the regs.

What is that 112?

I'd copy from arch/powerpc/include/asm/ptrace.h:

#define STACK_FRAME_OVERHEAD    112     /* size of minimum stack frame */
#define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)

and whatever that 96 is.

Sadly, scripts/update-linux-headers.sh cannot copy
arch/powerpc/include/asm/ptrace.h as kernel's "make headers_install"
does not install it (it installs the "uapi" header which does not have
these symbols) so you'll have to define them.


> +
> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> +            for (i = 0; i < 32; i++)
> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> +                            regs.gpr[i]);
> +        }
> +
> +        sp = next_sp;
> +    }
> +
> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);


and this is "sp: 0x%"HWADDR_PRIx"\n".



> +#endif
> +}
> +
>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>  {
>  #if defined(DO_PPC_STATISTICS)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 0394a9ddad..3fd24f85cc 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = ppc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>      cc->dump_state = ppc_cpu_dump_state;
> +    cc->dump_stack = ppc_cpu_dump_stack;
>      cc->dump_statistics = ppc_cpu_dump_statistics;
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;
> 

-- 
Alexey

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-01  9:48     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 27+ messages in thread
From: Alexey Kardashevskiy @ 2019-05-01  9:48 UTC (permalink / raw)
  To: Suraj Jitindar Singh, qemu-devel; +Cc: armbru, qemu-ppc, dgilbert, david



On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
> The monitor function dump-stack is used to dump the stack for a cpu.
> This can be useful for debugging purposes when the stack cannot be
> dumped by another means.
> 
> Add a ppc implementation ppc_cpu_dump_stack().
> The stack pointer is stored in R1 with the back pointer at offset 0 and
> the link register at offset 2.
> Also dump the registers from the stack frame if the marker "regshere" is
> found.

Is this a Linux only marker? ABI does not mentioned this.

> This only dumps the kernel stack, stopping if a non-kernel address is
> found in the stack.

Why enforce this limit?

> 
> Sample output:
> (qemu) dump-stack
> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> 	trap : 0x0000000000000700
> 	pc   : 0xc000000000104490
> 	msr  : 0x9000000002843003
> 	lr   : 0xc000000000103ffc
> 	gpr 0: 0x0000000000000001
> 	gpr 1: 0xc00000005051f530
> 	gpr 2: 0xc000000001088200
> 	gpr 3: 0x0000000000000001
> 	gpr 4: 0xc000000032d60000
> 	gpr 5: 0xc0000000014b8f00
> 	gpr 6: 0x0000000000c835e0
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x0000000000000000
> 	gpr 9: 0xc000000032f00000
> 	gpr10: 0x9000000002803033
> 	gpr11: 0xc000000000b60f00
> 	gpr12: 0x0000000000002000
> 	gpr13: 0xc000000001250000
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000008
> 	gpr16: 0x0000000000000000
> 	gpr17: 0xc00000000114f790
> 	gpr18: 0x00000000ffffffff
> 	gpr19: 0xc00000005051f8e8
> 	gpr20: 0x0000000000000001
> 	gpr21: 0x0000000000000000
> 	gpr22: 0x0000000000000001
> 	gpr23: 0x0000000000000001
> 	gpr24: 0x0000000000000001
> 	gpr25: 0xc0000000014b8f70
> 	gpr26: 0x0000000000000000
> 	gpr27: 0x0000000000000001
> 	gpr28: 0x0000000000000001
> 	gpr29: 0x0000000000000000
> 	gpr30: 0xc0000000014b8f00
> 	gpr31: 0xc0000000014b8f00

Looks bulky, using the "info registers" format would make sense here.


> sp: 0xc00000005051f530 lr: 0x0000000000000000
> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> 	trap : 0x0000000000000c01
> 	pc   : 0x00007fffa6c9d8d0
> 	msr  : 0x900000000280f033
> 	lr   : 0x0000000010090f40
> 	gpr 0: 0x0000000000000036
> 	gpr 1: 0x00007fffa62fdd70
> 	gpr 2: 0x00007fffa6d57300
> 	gpr 3: 0x000000000000000d
> 	gpr 4: 0x000000002000ae80
> 	gpr 5: 0x0000000000000000
> 	gpr 6: 0x0000000000000537
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x000000000000000d
> 	gpr 9: 0x0000000000000000
> 	gpr10: 0x0000000000000000
> 	gpr11: 0x0000000000000000
> 	gpr12: 0x0000000000000000
> 	gpr13: 0x00007fffa6306380
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000001
> 	gpr16: 0x0000000039ba6928
> 	gpr17: 0x0000000000000000
> 	gpr18: 0x0000000000000000
> 	gpr19: 0x00007fffa6d702f0
> 	gpr20: 0x00007fffa62fddf0
> 	gpr21: 0x0000000000000080
> 	gpr22: 0x0000000000000004
> 	gpr23: 0x0000000000000000
> 	gpr24: 0x0000000010ac85c0
> 	gpr25: 0x0000000000000008
> 	gpr26: 0x00007fffa62fde10
> 	gpr27: 0x0000000000000000
> 	gpr28: 0x0000000000000002
> 	gpr29: 0x0000000000000000
> 	gpr30: 0x0000000039ba6900
> 	gpr31: 0x0000000010ac85c0
> sp: 0x00007fffa62fdd70
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  target/ppc/cpu.h                |  1 +
>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c |  1 +
>  3 files changed, 62 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5e7cf54b2f..28c4dffca1 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 8d08625c33..b162998ce7 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #undef RFPL
>  }
>  
> +struct ppc_pt_regs {
> +        unsigned long gpr[32];
> +        unsigned long nip;
> +        unsigned long msr;
> +        unsigned long orig_gpr3;
> +        unsigned long ctr;
> +        unsigned long link;
> +        unsigned long xer;
> +        unsigned long ccr;
> +        unsigned long softe;
> +        unsigned long trap;
> +        unsigned long dar;
> +        unsigned long dsisr;
> +        unsigned long result;
> +};
> +
> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> +{
> +#if defined(TARGET_PPC64)
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t sp, next_sp, lr, buf[4];

These are hwaddr really.

> +
> +    /* stack pointer stored in r1 */
> +    sp = env->gpr[1];
> +
> +    while (sp && (sp & (0xCUL << 60))) {
> +        uint64_t marker = 0UL;

sp = ppc_cpu_get_phys_page_debug(cs, sp) | (sp & ~TARGET_PAGE_MASK);

and finish the loop when ppc_cpu_get_phys_page_debug returns -1?

> +
> +        /* read and print LR */
> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);

and s/ & ~(0xCUL << 60)//

> +        next_sp = buf[0];
> +        lr = buf[2];

These two need to be converted from guest endian. For a BE guest, I see:

sp: 0x000000007e582ff0 lr: 0xe4e60a00000000c0
sp: 0xffffffffffffffff lr: 0x0000600000006000
sp: 0x0000600000006000


> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);

HWADDR_PRIx. Or at least PRIx64, otherwise it won't compile on 32bit or
x86 or somewhere else.


> +        sp &= ~(0xCUL << 60);

and remove this line. And now you can dump



> +
> +        /* Does the stackframe contain regs? */
> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));

I suspect the marker needs byteswap as well.

What is that 96?

> +        if (marker == 0x7265677368657265) { /* regshere */
> +            struct ppc_pt_regs regs;
> +            int i;
> +
> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));

and the regs.

What is that 112?

I'd copy from arch/powerpc/include/asm/ptrace.h:

#define STACK_FRAME_OVERHEAD    112     /* size of minimum stack frame */
#define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)

and whatever that 96 is.

Sadly, scripts/update-linux-headers.sh cannot copy
arch/powerpc/include/asm/ptrace.h as kernel's "make headers_install"
does not install it (it installs the "uapi" header which does not have
these symbols) so you'll have to define them.


> +
> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> +            for (i = 0; i < 32; i++)
> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> +                            regs.gpr[i]);
> +        }
> +
> +        sp = next_sp;
> +    }
> +
> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);


and this is "sp: 0x%"HWADDR_PRIx"\n".



> +#endif
> +}
> +
>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>  {
>  #if defined(DO_PPC_STATISTICS)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 0394a9ddad..3fd24f85cc 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = ppc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>      cc->dump_state = ppc_cpu_dump_state;
> +    cc->dump_stack = ppc_cpu_dump_stack;
>      cc->dump_statistics = ppc_cpu_dump_statistics;
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;
> 

-- 
Alexey


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-01 10:44   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2019-05-01 10:44 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: qemu-devel, armbru, david, qemu-ppc

* Suraj Jitindar Singh (sjitindarsingh@gmail.com) wrote:
> Add a monitor command "dump-stack" to be used to dump the stack for the
> current cpu.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

OK, and for a debug-only command that's OK for HMP.

> ---
>  hmp-commands.hx   | 13 +++++++++++++
>  hmp.h             |  1 +
>  include/qom/cpu.h | 10 ++++++++++
>  monitor.c         | 12 ++++++++++++
>  qom/cpu.c         | 10 ++++++++++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9b4035965c..965ccdea28 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -862,6 +862,19 @@ ETEXI
>      },
>  
>  STEXI
> +@item dump-stack
> +@findex dump-stack
> +dump stack of the cpu
> +ETEXI
> +    {
> +        .name           = "dump-stack",
> +        .args_type      = "",
> +        .params         = "",
> +        .help           = "dump stack",
> +        .cmd            = hmp_dumpstack,
> +    },
> +
> +STEXI
>  @item pmemsave @var{addr} @var{size} @var{file}
>  @findex pmemsave
>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
> diff --git a/hmp.h b/hmp.h
> index 43617f2646..e6edf1215c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 08abcbd3fe..f2e83e9918 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>      void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
>  /**
> + * cpu_dump_stack:
> + * @cpu: The CPU whose stack is to be dumped.
> + * @f: If non-null, dump to this stream, else to current print sink.
> + *
> + * Dumps CPU stack.
> + */
> +void cpu_dump_stack(CPUState *cpu, FILE *f);
> +
> +/**
>   * cpu_dump_statistics:
>   * @cpu: The CPU whose state is to be dumped.
>   * @flags: Flags what to dump.
> diff --git a/monitor.c b/monitor.c
> index 9b5f10b475..dbec2e4376 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> +{
> +    CPUState *cs = mon_get_cpu();
> +
> +    if (!cs) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    cpu_dump_stack(cs, NULL);
> +}
> +
>  #ifdef CONFIG_TCG
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3c5493c96c..0dc10004f4 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>      }
>  }
>  
> +void cpu_dump_stack(CPUState *cpu, FILE *f)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->dump_stack) {

Can you make this print a message if there's no implementation for the
current CPU type; since you've just done PPC, everyone else will wonder
why the command doesn't work for them.

> +        cpu_synchronize_state(cpu);
> +        cc->dump_stack(cpu, f);

Are you sure you need this 'f' parameter for the file descriptor?

See Markus's series:
  http://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03271.html
where he removes most places that the FILE* is passed.


Dave

> +    }
> +}
> +
>  void cpu_dump_statistics(CPUState *cpu, int flags)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-01 10:44   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2019-05-01 10:44 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: david, qemu-ppc, qemu-devel, armbru

* Suraj Jitindar Singh (sjitindarsingh@gmail.com) wrote:
> Add a monitor command "dump-stack" to be used to dump the stack for the
> current cpu.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

OK, and for a debug-only command that's OK for HMP.

> ---
>  hmp-commands.hx   | 13 +++++++++++++
>  hmp.h             |  1 +
>  include/qom/cpu.h | 10 ++++++++++
>  monitor.c         | 12 ++++++++++++
>  qom/cpu.c         | 10 ++++++++++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9b4035965c..965ccdea28 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -862,6 +862,19 @@ ETEXI
>      },
>  
>  STEXI
> +@item dump-stack
> +@findex dump-stack
> +dump stack of the cpu
> +ETEXI
> +    {
> +        .name           = "dump-stack",
> +        .args_type      = "",
> +        .params         = "",
> +        .help           = "dump stack",
> +        .cmd            = hmp_dumpstack,
> +    },
> +
> +STEXI
>  @item pmemsave @var{addr} @var{size} @var{file}
>  @findex pmemsave
>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
> diff --git a/hmp.h b/hmp.h
> index 43617f2646..e6edf1215c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 08abcbd3fe..f2e83e9918 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>      void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
>  /**
> + * cpu_dump_stack:
> + * @cpu: The CPU whose stack is to be dumped.
> + * @f: If non-null, dump to this stream, else to current print sink.
> + *
> + * Dumps CPU stack.
> + */
> +void cpu_dump_stack(CPUState *cpu, FILE *f);
> +
> +/**
>   * cpu_dump_statistics:
>   * @cpu: The CPU whose state is to be dumped.
>   * @flags: Flags what to dump.
> diff --git a/monitor.c b/monitor.c
> index 9b5f10b475..dbec2e4376 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> +{
> +    CPUState *cs = mon_get_cpu();
> +
> +    if (!cs) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    cpu_dump_stack(cs, NULL);
> +}
> +
>  #ifdef CONFIG_TCG
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3c5493c96c..0dc10004f4 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>      }
>  }
>  
> +void cpu_dump_stack(CPUState *cpu, FILE *f)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->dump_stack) {

Can you make this print a message if there's no implementation for the
current CPU type; since you've just done PPC, everyone else will wonder
why the command doesn't work for them.

> +        cpu_synchronize_state(cpu);
> +        cc->dump_stack(cpu, f);

Are you sure you need this 'f' parameter for the file descriptor?

See Markus's series:
  http://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03271.html
where he removes most places that the FILE* is passed.


Dave

> +    }
> +}
> +
>  void cpu_dump_statistics(CPUState *cpu, int flags)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-02  0:43       ` David Gibson
  0 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2019-05-02  0:43 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: Suraj Jitindar Singh, qemu-devel, qemu-ppc, dgilbert, armbru

[-- Attachment #1: Type: text/plain, Size: 10268 bytes --]

On Wed, May 01, 2019 at 07:48:48PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
> > The monitor function dump-stack is used to dump the stack for a cpu.
> > This can be useful for debugging purposes when the stack cannot be
> > dumped by another means.
> > 
> > Add a ppc implementation ppc_cpu_dump_stack().
> > The stack pointer is stored in R1 with the back pointer at offset 0 and
> > the link register at offset 2.
> > Also dump the registers from the stack frame if the marker "regshere" is
> > found.
> 
> Is this a Linux only marker? ABI does not mentioned this.
> 
> > This only dumps the kernel stack, stopping if a non-kernel address is
> > found in the stack.
> 
> Why enforce this limit?

It's also making a Linux specific assumption about addresses.

> 
> > 
> > Sample output:
> > (qemu) dump-stack
> > sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> > sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> > sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> > sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> > sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> > sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> > sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> > sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> > sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> > sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> > sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> > 	trap : 0x0000000000000700
> > 	pc   : 0xc000000000104490
> > 	msr  : 0x9000000002843003
> > 	lr   : 0xc000000000103ffc
> > 	gpr 0: 0x0000000000000001
> > 	gpr 1: 0xc00000005051f530
> > 	gpr 2: 0xc000000001088200
> > 	gpr 3: 0x0000000000000001
> > 	gpr 4: 0xc000000032d60000
> > 	gpr 5: 0xc0000000014b8f00
> > 	gpr 6: 0x0000000000c835e0
> > 	gpr 7: 0x0000000000000000
> > 	gpr 8: 0x0000000000000000
> > 	gpr 9: 0xc000000032f00000
> > 	gpr10: 0x9000000002803033
> > 	gpr11: 0xc000000000b60f00
> > 	gpr12: 0x0000000000002000
> > 	gpr13: 0xc000000001250000
> > 	gpr14: 0x0000000000000000
> > 	gpr15: 0x0000000000000008
> > 	gpr16: 0x0000000000000000
> > 	gpr17: 0xc00000000114f790
> > 	gpr18: 0x00000000ffffffff
> > 	gpr19: 0xc00000005051f8e8
> > 	gpr20: 0x0000000000000001
> > 	gpr21: 0x0000000000000000
> > 	gpr22: 0x0000000000000001
> > 	gpr23: 0x0000000000000001
> > 	gpr24: 0x0000000000000001
> > 	gpr25: 0xc0000000014b8f70
> > 	gpr26: 0x0000000000000000
> > 	gpr27: 0x0000000000000001
> > 	gpr28: 0x0000000000000001
> > 	gpr29: 0x0000000000000000
> > 	gpr30: 0xc0000000014b8f00
> > 	gpr31: 0xc0000000014b8f00
> 
> Looks bulky, using the "info registers" format would make sense here.
> 
> 
> > sp: 0xc00000005051f530 lr: 0x0000000000000000
> > sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> > sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> > sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> > sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> > sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> > sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> > sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> > sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> > sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> > sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> > sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> > 	trap : 0x0000000000000c01
> > 	pc   : 0x00007fffa6c9d8d0
> > 	msr  : 0x900000000280f033
> > 	lr   : 0x0000000010090f40
> > 	gpr 0: 0x0000000000000036
> > 	gpr 1: 0x00007fffa62fdd70
> > 	gpr 2: 0x00007fffa6d57300
> > 	gpr 3: 0x000000000000000d
> > 	gpr 4: 0x000000002000ae80
> > 	gpr 5: 0x0000000000000000
> > 	gpr 6: 0x0000000000000537
> > 	gpr 7: 0x0000000000000000
> > 	gpr 8: 0x000000000000000d
> > 	gpr 9: 0x0000000000000000
> > 	gpr10: 0x0000000000000000
> > 	gpr11: 0x0000000000000000
> > 	gpr12: 0x0000000000000000
> > 	gpr13: 0x00007fffa6306380
> > 	gpr14: 0x0000000000000000
> > 	gpr15: 0x0000000000000001
> > 	gpr16: 0x0000000039ba6928
> > 	gpr17: 0x0000000000000000
> > 	gpr18: 0x0000000000000000
> > 	gpr19: 0x00007fffa6d702f0
> > 	gpr20: 0x00007fffa62fddf0
> > 	gpr21: 0x0000000000000080
> > 	gpr22: 0x0000000000000004
> > 	gpr23: 0x0000000000000000
> > 	gpr24: 0x0000000010ac85c0
> > 	gpr25: 0x0000000000000008
> > 	gpr26: 0x00007fffa62fde10
> > 	gpr27: 0x0000000000000000
> > 	gpr28: 0x0000000000000002
> > 	gpr29: 0x0000000000000000
> > 	gpr30: 0x0000000039ba6900
> > 	gpr31: 0x0000000010ac85c0
> > sp: 0x00007fffa62fdd70
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  target/ppc/cpu.h                |  1 +
> >  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
> >  target/ppc/translate_init.inc.c |  1 +
> >  3 files changed, 62 insertions(+)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 5e7cf54b2f..28c4dffca1 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
> >  void ppc_cpu_do_interrupt(CPUState *cpu);
> >  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> >  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> > +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
> >  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
> >  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 8d08625c33..b162998ce7 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> >  #undef RFPL
> >  }
> >  
> > +struct ppc_pt_regs {
> > +        unsigned long gpr[32];
> > +        unsigned long nip;
> > +        unsigned long msr;
> > +        unsigned long orig_gpr3;
> > +        unsigned long ctr;
> > +        unsigned long link;
> > +        unsigned long xer;
> > +        unsigned long ccr;
> > +        unsigned long softe;
> > +        unsigned long trap;
> > +        unsigned long dar;
> > +        unsigned long dsisr;
> > +        unsigned long result;
> > +};
> > +
> > +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> > +{
> > +#if defined(TARGET_PPC64)
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    uint64_t sp, next_sp, lr, buf[4];
> 
> These are hwaddr really.
> 
> > +
> > +    /* stack pointer stored in r1 */
> > +    sp = env->gpr[1];
> > +
> > +    while (sp && (sp & (0xCUL << 60))) {
> > +        uint64_t marker = 0UL;
> 
> sp = ppc_cpu_get_phys_page_debug(cs, sp) | (sp & ~TARGET_PAGE_MASK);
> 
> and finish the loop when ppc_cpu_get_phys_page_debug returns -1?
> 
> > +
> > +        /* read and print LR */
> > +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
> 
> and s/ & ~(0xCUL << 60)//
> 
> > +        next_sp = buf[0];
> > +        lr = buf[2];
> 
> These two need to be converted from guest endian. For a BE guest, I see:
> 
> sp: 0x000000007e582ff0 lr: 0xe4e60a00000000c0
> sp: 0xffffffffffffffff lr: 0x0000600000006000
> sp: 0x0000600000006000
> 
> 
> > +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
> 
> HWADDR_PRIx. Or at least PRIx64, otherwise it won't compile on 32bit or
> x86 or somewhere else.
> 
> 
> > +        sp &= ~(0xCUL << 60);
> 
> and remove this line. And now you can dump
> 
> 
> 
> > +
> > +        /* Does the stackframe contain regs? */
> > +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
> 
> I suspect the marker needs byteswap as well.

Yeah, best to use the cpu_ldl() etc wrappers if possible, which
include byteswaps.  Urgh... except this depends on the cpu mode which
complicates things.

> 
> What is that 96?
> 
> > +        if (marker == 0x7265677368657265) { /* regshere */
> > +            struct ppc_pt_regs regs;
> > +            int i;
> > +
> > +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
> 
> and the regs.
> 
> What is that 112?
> 
> I'd copy from arch/powerpc/include/asm/ptrace.h:
> 
> #define STACK_FRAME_OVERHEAD    112     /* size of minimum stack frame */
> #define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)
> 
> and whatever that 96 is.
> 
> Sadly, scripts/update-linux-headers.sh cannot copy
> arch/powerpc/include/asm/ptrace.h as kernel's "make headers_install"
> does not install it (it installs the "uapi" header which does not have
> these symbols) so you'll have to define them.
> 
> 
> > +
> > +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> > +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> > +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> > +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> > +            for (i = 0; i < 32; i++)
> > +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> > +                            regs.gpr[i]);
> > +        }
> > +
> > +        sp = next_sp;
> > +    }
> > +
> > +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
> 
> 
> and this is "sp: 0x%"HWADDR_PRIx"\n".
> 
> 
> 
> > +#endif
> > +}
> > +
> >  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
> >  {
> >  #if defined(DO_PPC_STATISTICS)
> > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> > index 0394a9ddad..3fd24f85cc 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >      cc->do_interrupt = ppc_cpu_do_interrupt;
> >      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
> >      cc->dump_state = ppc_cpu_dump_state;
> > +    cc->dump_stack = ppc_cpu_dump_stack;
> >      cc->dump_statistics = ppc_cpu_dump_statistics;
> >      cc->set_pc = ppc_cpu_set_pc;
> >      cc->gdb_read_register = ppc_cpu_gdb_read_register;
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-02  0:43       ` David Gibson
  0 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2019-05-02  0:43 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: armbru, qemu-ppc, qemu-devel, Suraj Jitindar Singh, dgilbert

[-- Attachment #1: Type: text/plain, Size: 10268 bytes --]

On Wed, May 01, 2019 at 07:48:48PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
> > The monitor function dump-stack is used to dump the stack for a cpu.
> > This can be useful for debugging purposes when the stack cannot be
> > dumped by another means.
> > 
> > Add a ppc implementation ppc_cpu_dump_stack().
> > The stack pointer is stored in R1 with the back pointer at offset 0 and
> > the link register at offset 2.
> > Also dump the registers from the stack frame if the marker "regshere" is
> > found.
> 
> Is this a Linux only marker? ABI does not mentioned this.
> 
> > This only dumps the kernel stack, stopping if a non-kernel address is
> > found in the stack.
> 
> Why enforce this limit?

It's also making a Linux specific assumption about addresses.

> 
> > 
> > Sample output:
> > (qemu) dump-stack
> > sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> > sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> > sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> > sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> > sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> > sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> > sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> > sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> > sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> > sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> > sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> > 	trap : 0x0000000000000700
> > 	pc   : 0xc000000000104490
> > 	msr  : 0x9000000002843003
> > 	lr   : 0xc000000000103ffc
> > 	gpr 0: 0x0000000000000001
> > 	gpr 1: 0xc00000005051f530
> > 	gpr 2: 0xc000000001088200
> > 	gpr 3: 0x0000000000000001
> > 	gpr 4: 0xc000000032d60000
> > 	gpr 5: 0xc0000000014b8f00
> > 	gpr 6: 0x0000000000c835e0
> > 	gpr 7: 0x0000000000000000
> > 	gpr 8: 0x0000000000000000
> > 	gpr 9: 0xc000000032f00000
> > 	gpr10: 0x9000000002803033
> > 	gpr11: 0xc000000000b60f00
> > 	gpr12: 0x0000000000002000
> > 	gpr13: 0xc000000001250000
> > 	gpr14: 0x0000000000000000
> > 	gpr15: 0x0000000000000008
> > 	gpr16: 0x0000000000000000
> > 	gpr17: 0xc00000000114f790
> > 	gpr18: 0x00000000ffffffff
> > 	gpr19: 0xc00000005051f8e8
> > 	gpr20: 0x0000000000000001
> > 	gpr21: 0x0000000000000000
> > 	gpr22: 0x0000000000000001
> > 	gpr23: 0x0000000000000001
> > 	gpr24: 0x0000000000000001
> > 	gpr25: 0xc0000000014b8f70
> > 	gpr26: 0x0000000000000000
> > 	gpr27: 0x0000000000000001
> > 	gpr28: 0x0000000000000001
> > 	gpr29: 0x0000000000000000
> > 	gpr30: 0xc0000000014b8f00
> > 	gpr31: 0xc0000000014b8f00
> 
> Looks bulky, using the "info registers" format would make sense here.
> 
> 
> > sp: 0xc00000005051f530 lr: 0x0000000000000000
> > sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> > sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> > sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> > sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> > sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> > sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> > sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> > sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> > sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> > sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> > sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> > 	trap : 0x0000000000000c01
> > 	pc   : 0x00007fffa6c9d8d0
> > 	msr  : 0x900000000280f033
> > 	lr   : 0x0000000010090f40
> > 	gpr 0: 0x0000000000000036
> > 	gpr 1: 0x00007fffa62fdd70
> > 	gpr 2: 0x00007fffa6d57300
> > 	gpr 3: 0x000000000000000d
> > 	gpr 4: 0x000000002000ae80
> > 	gpr 5: 0x0000000000000000
> > 	gpr 6: 0x0000000000000537
> > 	gpr 7: 0x0000000000000000
> > 	gpr 8: 0x000000000000000d
> > 	gpr 9: 0x0000000000000000
> > 	gpr10: 0x0000000000000000
> > 	gpr11: 0x0000000000000000
> > 	gpr12: 0x0000000000000000
> > 	gpr13: 0x00007fffa6306380
> > 	gpr14: 0x0000000000000000
> > 	gpr15: 0x0000000000000001
> > 	gpr16: 0x0000000039ba6928
> > 	gpr17: 0x0000000000000000
> > 	gpr18: 0x0000000000000000
> > 	gpr19: 0x00007fffa6d702f0
> > 	gpr20: 0x00007fffa62fddf0
> > 	gpr21: 0x0000000000000080
> > 	gpr22: 0x0000000000000004
> > 	gpr23: 0x0000000000000000
> > 	gpr24: 0x0000000010ac85c0
> > 	gpr25: 0x0000000000000008
> > 	gpr26: 0x00007fffa62fde10
> > 	gpr27: 0x0000000000000000
> > 	gpr28: 0x0000000000000002
> > 	gpr29: 0x0000000000000000
> > 	gpr30: 0x0000000039ba6900
> > 	gpr31: 0x0000000010ac85c0
> > sp: 0x00007fffa62fdd70
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  target/ppc/cpu.h                |  1 +
> >  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
> >  target/ppc/translate_init.inc.c |  1 +
> >  3 files changed, 62 insertions(+)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 5e7cf54b2f..28c4dffca1 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
> >  void ppc_cpu_do_interrupt(CPUState *cpu);
> >  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
> >  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> > +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
> >  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
> >  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 8d08625c33..b162998ce7 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> >  #undef RFPL
> >  }
> >  
> > +struct ppc_pt_regs {
> > +        unsigned long gpr[32];
> > +        unsigned long nip;
> > +        unsigned long msr;
> > +        unsigned long orig_gpr3;
> > +        unsigned long ctr;
> > +        unsigned long link;
> > +        unsigned long xer;
> > +        unsigned long ccr;
> > +        unsigned long softe;
> > +        unsigned long trap;
> > +        unsigned long dar;
> > +        unsigned long dsisr;
> > +        unsigned long result;
> > +};
> > +
> > +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> > +{
> > +#if defined(TARGET_PPC64)
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    uint64_t sp, next_sp, lr, buf[4];
> 
> These are hwaddr really.
> 
> > +
> > +    /* stack pointer stored in r1 */
> > +    sp = env->gpr[1];
> > +
> > +    while (sp && (sp & (0xCUL << 60))) {
> > +        uint64_t marker = 0UL;
> 
> sp = ppc_cpu_get_phys_page_debug(cs, sp) | (sp & ~TARGET_PAGE_MASK);
> 
> and finish the loop when ppc_cpu_get_phys_page_debug returns -1?
> 
> > +
> > +        /* read and print LR */
> > +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
> 
> and s/ & ~(0xCUL << 60)//
> 
> > +        next_sp = buf[0];
> > +        lr = buf[2];
> 
> These two need to be converted from guest endian. For a BE guest, I see:
> 
> sp: 0x000000007e582ff0 lr: 0xe4e60a00000000c0
> sp: 0xffffffffffffffff lr: 0x0000600000006000
> sp: 0x0000600000006000
> 
> 
> > +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
> 
> HWADDR_PRIx. Or at least PRIx64, otherwise it won't compile on 32bit or
> x86 or somewhere else.
> 
> 
> > +        sp &= ~(0xCUL << 60);
> 
> and remove this line. And now you can dump
> 
> 
> 
> > +
> > +        /* Does the stackframe contain regs? */
> > +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
> 
> I suspect the marker needs byteswap as well.

Yeah, best to use the cpu_ldl() etc wrappers if possible, which
include byteswaps.  Urgh... except this depends on the cpu mode which
complicates things.

> 
> What is that 96?
> 
> > +        if (marker == 0x7265677368657265) { /* regshere */
> > +            struct ppc_pt_regs regs;
> > +            int i;
> > +
> > +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
> 
> and the regs.
> 
> What is that 112?
> 
> I'd copy from arch/powerpc/include/asm/ptrace.h:
> 
> #define STACK_FRAME_OVERHEAD    112     /* size of minimum stack frame */
> #define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)
> 
> and whatever that 96 is.
> 
> Sadly, scripts/update-linux-headers.sh cannot copy
> arch/powerpc/include/asm/ptrace.h as kernel's "make headers_install"
> does not install it (it installs the "uapi" header which does not have
> these symbols) so you'll have to define them.
> 
> 
> > +
> > +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> > +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> > +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> > +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> > +            for (i = 0; i < 32; i++)
> > +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> > +                            regs.gpr[i]);
> > +        }
> > +
> > +        sp = next_sp;
> > +    }
> > +
> > +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
> 
> 
> and this is "sp: 0x%"HWADDR_PRIx"\n".
> 
> 
> 
> > +#endif
> > +}
> > +
> >  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
> >  {
> >  #if defined(DO_PPC_STATISTICS)
> > diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> > index 0394a9ddad..3fd24f85cc 100644
> > --- a/target/ppc/translate_init.inc.c
> > +++ b/target/ppc/translate_init.inc.c
> > @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >      cc->do_interrupt = ppc_cpu_do_interrupt;
> >      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
> >      cc->dump_state = ppc_cpu_dump_state;
> > +    cc->dump_stack = ppc_cpu_dump_stack;
> >      cc->dump_statistics = ppc_cpu_dump_statistics;
> >      cc->set_pc = ppc_cpu_set_pc;
> >      cc->gdb_read_register = ppc_cpu_gdb_read_register;
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-02  0:44   ` David Gibson
  0 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2019-05-02  0:44 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: qemu-devel, dgilbert, armbru, qemu-ppc

[-- Attachment #1: Type: text/plain, Size: 4324 bytes --]

On Wed, May 01, 2019 at 03:35:21PM +1000, Suraj Jitindar Singh wrote:
> Add a monitor command "dump-stack" to be used to dump the stack for the
> current cpu.

So, you can already get guest backtraces by using the gdbstub
functionality.  I can see some benefit in allowing this more easily
through hmp, but whether it's worth the code size, I'm less certain.

> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  hmp-commands.hx   | 13 +++++++++++++
>  hmp.h             |  1 +
>  include/qom/cpu.h | 10 ++++++++++
>  monitor.c         | 12 ++++++++++++
>  qom/cpu.c         | 10 ++++++++++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9b4035965c..965ccdea28 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -862,6 +862,19 @@ ETEXI
>      },
>  
>  STEXI
> +@item dump-stack
> +@findex dump-stack
> +dump stack of the cpu
> +ETEXI
> +    {
> +        .name           = "dump-stack",
> +        .args_type      = "",
> +        .params         = "",
> +        .help           = "dump stack",
> +        .cmd            = hmp_dumpstack,
> +    },
> +
> +STEXI
>  @item pmemsave @var{addr} @var{size} @var{file}
>  @findex pmemsave
>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
> diff --git a/hmp.h b/hmp.h
> index 43617f2646..e6edf1215c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 08abcbd3fe..f2e83e9918 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>      void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
>  /**
> + * cpu_dump_stack:
> + * @cpu: The CPU whose stack is to be dumped.
> + * @f: If non-null, dump to this stream, else to current print sink.
> + *
> + * Dumps CPU stack.
> + */
> +void cpu_dump_stack(CPUState *cpu, FILE *f);
> +
> +/**
>   * cpu_dump_statistics:
>   * @cpu: The CPU whose state is to be dumped.
>   * @flags: Flags what to dump.
> diff --git a/monitor.c b/monitor.c
> index 9b5f10b475..dbec2e4376 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> +{
> +    CPUState *cs = mon_get_cpu();
> +
> +    if (!cs) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    cpu_dump_stack(cs, NULL);
> +}
> +
>  #ifdef CONFIG_TCG
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3c5493c96c..0dc10004f4 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>      }
>  }
>  
> +void cpu_dump_stack(CPUState *cpu, FILE *f)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->dump_stack) {
> +        cpu_synchronize_state(cpu);
> +        cc->dump_stack(cpu, f);
> +    }
> +}
> +
>  void cpu_dump_statistics(CPUState *cpu, int flags)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-02  0:44   ` David Gibson
  0 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2019-05-02  0:44 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: armbru, qemu-ppc, qemu-devel, dgilbert

[-- Attachment #1: Type: text/plain, Size: 4324 bytes --]

On Wed, May 01, 2019 at 03:35:21PM +1000, Suraj Jitindar Singh wrote:
> Add a monitor command "dump-stack" to be used to dump the stack for the
> current cpu.

So, you can already get guest backtraces by using the gdbstub
functionality.  I can see some benefit in allowing this more easily
through hmp, but whether it's worth the code size, I'm less certain.

> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  hmp-commands.hx   | 13 +++++++++++++
>  hmp.h             |  1 +
>  include/qom/cpu.h | 10 ++++++++++
>  monitor.c         | 12 ++++++++++++
>  qom/cpu.c         | 10 ++++++++++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9b4035965c..965ccdea28 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -862,6 +862,19 @@ ETEXI
>      },
>  
>  STEXI
> +@item dump-stack
> +@findex dump-stack
> +dump stack of the cpu
> +ETEXI
> +    {
> +        .name           = "dump-stack",
> +        .args_type      = "",
> +        .params         = "",
> +        .help           = "dump stack",
> +        .cmd            = hmp_dumpstack,
> +    },
> +
> +STEXI
>  @item pmemsave @var{addr} @var{size} @var{file}
>  @findex pmemsave
>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
> diff --git a/hmp.h b/hmp.h
> index 43617f2646..e6edf1215c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 08abcbd3fe..f2e83e9918 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>      void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
>  /**
> + * cpu_dump_stack:
> + * @cpu: The CPU whose stack is to be dumped.
> + * @f: If non-null, dump to this stream, else to current print sink.
> + *
> + * Dumps CPU stack.
> + */
> +void cpu_dump_stack(CPUState *cpu, FILE *f);
> +
> +/**
>   * cpu_dump_statistics:
>   * @cpu: The CPU whose state is to be dumped.
>   * @flags: Flags what to dump.
> diff --git a/monitor.c b/monitor.c
> index 9b5f10b475..dbec2e4376 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> +{
> +    CPUState *cs = mon_get_cpu();
> +
> +    if (!cs) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    cpu_dump_stack(cs, NULL);
> +}
> +
>  #ifdef CONFIG_TCG
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3c5493c96c..0dc10004f4 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>      }
>  }
>  
> +void cpu_dump_stack(CPUState *cpu, FILE *f)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->dump_stack) {
> +        cpu_synchronize_state(cpu);
> +        cc->dump_stack(cpu, f);
> +    }
> +}
> +
>  void cpu_dump_statistics(CPUState *cpu, int flags)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-02  2:15     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 27+ messages in thread
From: Alexey Kardashevskiy @ 2019-05-02  2:15 UTC (permalink / raw)
  To: David Gibson, Suraj Jitindar Singh; +Cc: armbru, qemu-ppc, qemu-devel, dgilbert



On 02/05/2019 10:44, David Gibson wrote:
> On Wed, May 01, 2019 at 03:35:21PM +1000, Suraj Jitindar Singh wrote:
>> Add a monitor command "dump-stack" to be used to dump the stack for the
>> current cpu.
> 
> So, you can already get guest backtraces by using the gdbstub

Not in the field - this requires QEMU to run with -s which is not
usually the case.

But since we almost always deal with QEMUs run by libvirt and HMP/QMP is
always available, one could write a script doing QMP's
"human-monitor-command x/16g" or "virsh qemu-monitor-command --hmp
x/16g" to read the guest memory and MSR:LE and dump the stack with the
exception frame.


> functionality.  I can see some benefit in allowing this more easily
> through hmp, but whether it's worth the code size, I'm less certain.

It still seems easier than running an external script talking to HMP/QMP
as you would not want to write such script in bash but rather in a
better language which might not be installed on the client machine (like
missing python3 on many RHEL :) ). Thanks,



>>
>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>> ---
>>  hmp-commands.hx   | 13 +++++++++++++
>>  hmp.h             |  1 +
>>  include/qom/cpu.h | 10 ++++++++++
>>  monitor.c         | 12 ++++++++++++
>>  qom/cpu.c         | 10 ++++++++++
>>  5 files changed, 46 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 9b4035965c..965ccdea28 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -862,6 +862,19 @@ ETEXI
>>      },
>>  
>>  STEXI
>> +@item dump-stack
>> +@findex dump-stack
>> +dump stack of the cpu
>> +ETEXI
>> +    {
>> +        .name           = "dump-stack",
>> +        .args_type      = "",
>> +        .params         = "",
>> +        .help           = "dump stack",
>> +        .cmd            = hmp_dumpstack,
>> +    },
>> +
>> +STEXI
>>  @item pmemsave @var{addr} @var{size} @var{file}
>>  @findex pmemsave
>>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
>> diff --git a/hmp.h b/hmp.h
>> index 43617f2646..e6edf1215c 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
>> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>>  void hmp_cont(Monitor *mon, const QDict *qdict);
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 08abcbd3fe..f2e83e9918 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>>                             uint8_t *buf, int len, bool is_write);
>>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
>> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>>      void (*dump_statistics)(CPUState *cpu, int flags);
>>      int64_t (*get_arch_id)(CPUState *cpu);
>> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>>  
>>  /**
>> + * cpu_dump_stack:
>> + * @cpu: The CPU whose stack is to be dumped.
>> + * @f: If non-null, dump to this stream, else to current print sink.
>> + *
>> + * Dumps CPU stack.
>> + */
>> +void cpu_dump_stack(CPUState *cpu, FILE *f);
>> +
>> +/**
>>   * cpu_dump_statistics:
>>   * @cpu: The CPU whose state is to be dumped.
>>   * @flags: Flags what to dump.
>> diff --git a/monitor.c b/monitor.c
>> index 9b5f10b475..dbec2e4376 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>>      }
>>  }
>>  
>> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
>> +{
>> +    CPUState *cs = mon_get_cpu();
>> +
>> +    if (!cs) {
>> +        monitor_printf(mon, "No CPU available\n");
>> +        return;
>> +    }
>> +
>> +    cpu_dump_stack(cs, NULL);
>> +}
>> +
>>  #ifdef CONFIG_TCG
>>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>>  {
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 3c5493c96c..0dc10004f4 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>>      }
>>  }
>>  
>> +void cpu_dump_stack(CPUState *cpu, FILE *f)
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> +
>> +    if (cc->dump_stack) {
>> +        cpu_synchronize_state(cpu);
>> +        cc->dump_stack(cpu, f);
>> +    }
>> +}
>> +
>>  void cpu_dump_statistics(CPUState *cpu, int flags)
>>  {
>>      CPUClass *cc = CPU_GET_CLASS(cpu);
> 

-- 
Alexey

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] monitor: Add dump-stack command
@ 2019-05-02  2:15     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 27+ messages in thread
From: Alexey Kardashevskiy @ 2019-05-02  2:15 UTC (permalink / raw)
  To: David Gibson, Suraj Jitindar Singh; +Cc: qemu-ppc, armbru, dgilbert, qemu-devel



On 02/05/2019 10:44, David Gibson wrote:
> On Wed, May 01, 2019 at 03:35:21PM +1000, Suraj Jitindar Singh wrote:
>> Add a monitor command "dump-stack" to be used to dump the stack for the
>> current cpu.
> 
> So, you can already get guest backtraces by using the gdbstub

Not in the field - this requires QEMU to run with -s which is not
usually the case.

But since we almost always deal with QEMUs run by libvirt and HMP/QMP is
always available, one could write a script doing QMP's
"human-monitor-command x/16g" or "virsh qemu-monitor-command --hmp
x/16g" to read the guest memory and MSR:LE and dump the stack with the
exception frame.


> functionality.  I can see some benefit in allowing this more easily
> through hmp, but whether it's worth the code size, I'm less certain.

It still seems easier than running an external script talking to HMP/QMP
as you would not want to write such script in bash but rather in a
better language which might not be installed on the client machine (like
missing python3 on many RHEL :) ). Thanks,



>>
>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>> ---
>>  hmp-commands.hx   | 13 +++++++++++++
>>  hmp.h             |  1 +
>>  include/qom/cpu.h | 10 ++++++++++
>>  monitor.c         | 12 ++++++++++++
>>  qom/cpu.c         | 10 ++++++++++
>>  5 files changed, 46 insertions(+)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 9b4035965c..965ccdea28 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -862,6 +862,19 @@ ETEXI
>>      },
>>  
>>  STEXI
>> +@item dump-stack
>> +@findex dump-stack
>> +dump stack of the cpu
>> +ETEXI
>> +    {
>> +        .name           = "dump-stack",
>> +        .args_type      = "",
>> +        .params         = "",
>> +        .help           = "dump stack",
>> +        .cmd            = hmp_dumpstack,
>> +    },
>> +
>> +STEXI
>>  @item pmemsave @var{addr} @var{size} @var{file}
>>  @findex pmemsave
>>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
>> diff --git a/hmp.h b/hmp.h
>> index 43617f2646..e6edf1215c 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
>> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>>  void hmp_cont(Monitor *mon, const QDict *qdict);
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 08abcbd3fe..f2e83e9918 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>>                             uint8_t *buf, int len, bool is_write);
>>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
>> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>>      void (*dump_statistics)(CPUState *cpu, int flags);
>>      int64_t (*get_arch_id)(CPUState *cpu);
>> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>>  
>>  /**
>> + * cpu_dump_stack:
>> + * @cpu: The CPU whose stack is to be dumped.
>> + * @f: If non-null, dump to this stream, else to current print sink.
>> + *
>> + * Dumps CPU stack.
>> + */
>> +void cpu_dump_stack(CPUState *cpu, FILE *f);
>> +
>> +/**
>>   * cpu_dump_statistics:
>>   * @cpu: The CPU whose state is to be dumped.
>>   * @flags: Flags what to dump.
>> diff --git a/monitor.c b/monitor.c
>> index 9b5f10b475..dbec2e4376 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>>      }
>>  }
>>  
>> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
>> +{
>> +    CPUState *cs = mon_get_cpu();
>> +
>> +    if (!cs) {
>> +        monitor_printf(mon, "No CPU available\n");
>> +        return;
>> +    }
>> +
>> +    cpu_dump_stack(cs, NULL);
>> +}
>> +
>>  #ifdef CONFIG_TCG
>>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>>  {
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 3c5493c96c..0dc10004f4 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>>      }
>>  }
>>  
>> +void cpu_dump_stack(CPUState *cpu, FILE *f)
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> +
>> +    if (cc->dump_stack) {
>> +        cpu_synchronize_state(cpu);
>> +        cc->dump_stack(cpu, f);
>> +    }
>> +}
>> +
>>  void cpu_dump_statistics(CPUState *cpu, int flags)
>>  {
>>      CPUClass *cc = CPU_GET_CLASS(cpu);
> 

-- 
Alexey


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-02  3:47         ` Alexey Kardashevskiy
  0 siblings, 0 replies; 27+ messages in thread
From: Alexey Kardashevskiy @ 2019-05-02  3:47 UTC (permalink / raw)
  To: David Gibson; +Cc: Suraj Jitindar Singh, qemu-devel, qemu-ppc, dgilbert, armbru



On 02/05/2019 10:43, David Gibson wrote:
> On Wed, May 01, 2019 at 07:48:48PM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
>>> The monitor function dump-stack is used to dump the stack for a cpu.
>>> This can be useful for debugging purposes when the stack cannot be
>>> dumped by another means.
>>>
>>> Add a ppc implementation ppc_cpu_dump_stack().
>>> The stack pointer is stored in R1 with the back pointer at offset 0 and
>>> the link register at offset 2.
>>> Also dump the registers from the stack frame if the marker "regshere" is
>>> found.
>>
>> Is this a Linux only marker? ABI does not mentioned this.
>>
>>> This only dumps the kernel stack, stopping if a non-kernel address is
>>> found in the stack.
>>
>> Why enforce this limit?
> 
> It's also making a Linux specific assumption about addresses.


It does not have to if it used ppc_cpu_get_phys_page_debug(), the only
linux specific left is that "regshere" marker, otherwise it could work
for AIX or FreeBSD (obviously without the exception frame).


> 
>>
>>>
>>> Sample output:
>>> (qemu) dump-stack
>>> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
>>> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
>>> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
>>> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
>>> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
>>> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
>>> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
>>> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
>>> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
>>> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
>>> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
>>> 	trap : 0x0000000000000700
>>> 	pc   : 0xc000000000104490
>>> 	msr  : 0x9000000002843003
>>> 	lr   : 0xc000000000103ffc
>>> 	gpr 0: 0x0000000000000001
>>> 	gpr 1: 0xc00000005051f530
>>> 	gpr 2: 0xc000000001088200
>>> 	gpr 3: 0x0000000000000001
>>> 	gpr 4: 0xc000000032d60000
>>> 	gpr 5: 0xc0000000014b8f00
>>> 	gpr 6: 0x0000000000c835e0
>>> 	gpr 7: 0x0000000000000000
>>> 	gpr 8: 0x0000000000000000
>>> 	gpr 9: 0xc000000032f00000
>>> 	gpr10: 0x9000000002803033
>>> 	gpr11: 0xc000000000b60f00
>>> 	gpr12: 0x0000000000002000
>>> 	gpr13: 0xc000000001250000
>>> 	gpr14: 0x0000000000000000
>>> 	gpr15: 0x0000000000000008
>>> 	gpr16: 0x0000000000000000
>>> 	gpr17: 0xc00000000114f790
>>> 	gpr18: 0x00000000ffffffff
>>> 	gpr19: 0xc00000005051f8e8
>>> 	gpr20: 0x0000000000000001
>>> 	gpr21: 0x0000000000000000
>>> 	gpr22: 0x0000000000000001
>>> 	gpr23: 0x0000000000000001
>>> 	gpr24: 0x0000000000000001
>>> 	gpr25: 0xc0000000014b8f70
>>> 	gpr26: 0x0000000000000000
>>> 	gpr27: 0x0000000000000001
>>> 	gpr28: 0x0000000000000001
>>> 	gpr29: 0x0000000000000000
>>> 	gpr30: 0xc0000000014b8f00
>>> 	gpr31: 0xc0000000014b8f00
>>
>> Looks bulky, using the "info registers" format would make sense here.
>>
>>
>>> sp: 0xc00000005051f530 lr: 0x0000000000000000
>>> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
>>> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
>>> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
>>> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
>>> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
>>> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
>>> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
>>> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
>>> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
>>> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
>>> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
>>> 	trap : 0x0000000000000c01
>>> 	pc   : 0x00007fffa6c9d8d0
>>> 	msr  : 0x900000000280f033
>>> 	lr   : 0x0000000010090f40
>>> 	gpr 0: 0x0000000000000036
>>> 	gpr 1: 0x00007fffa62fdd70
>>> 	gpr 2: 0x00007fffa6d57300
>>> 	gpr 3: 0x000000000000000d
>>> 	gpr 4: 0x000000002000ae80
>>> 	gpr 5: 0x0000000000000000
>>> 	gpr 6: 0x0000000000000537
>>> 	gpr 7: 0x0000000000000000
>>> 	gpr 8: 0x000000000000000d
>>> 	gpr 9: 0x0000000000000000
>>> 	gpr10: 0x0000000000000000
>>> 	gpr11: 0x0000000000000000
>>> 	gpr12: 0x0000000000000000
>>> 	gpr13: 0x00007fffa6306380
>>> 	gpr14: 0x0000000000000000
>>> 	gpr15: 0x0000000000000001
>>> 	gpr16: 0x0000000039ba6928
>>> 	gpr17: 0x0000000000000000
>>> 	gpr18: 0x0000000000000000
>>> 	gpr19: 0x00007fffa6d702f0
>>> 	gpr20: 0x00007fffa62fddf0
>>> 	gpr21: 0x0000000000000080
>>> 	gpr22: 0x0000000000000004
>>> 	gpr23: 0x0000000000000000
>>> 	gpr24: 0x0000000010ac85c0
>>> 	gpr25: 0x0000000000000008
>>> 	gpr26: 0x00007fffa62fde10
>>> 	gpr27: 0x0000000000000000
>>> 	gpr28: 0x0000000000000002
>>> 	gpr29: 0x0000000000000000
>>> 	gpr30: 0x0000000039ba6900
>>> 	gpr31: 0x0000000010ac85c0
>>> sp: 0x00007fffa62fdd70
>>>
>>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>> ---
>>>  target/ppc/cpu.h                |  1 +
>>>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>>>  target/ppc/translate_init.inc.c |  1 +
>>>  3 files changed, 62 insertions(+)
>>>
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index 5e7cf54b2f..28c4dffca1 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>>>  void ppc_cpu_do_interrupt(CPUState *cpu);
>>>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>>>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>>> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>>>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>>>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>>>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index 8d08625c33..b162998ce7 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>>>  #undef RFPL
>>>  }
>>>  
>>> +struct ppc_pt_regs {
>>> +        unsigned long gpr[32];
>>> +        unsigned long nip;
>>> +        unsigned long msr;
>>> +        unsigned long orig_gpr3;
>>> +        unsigned long ctr;
>>> +        unsigned long link;
>>> +        unsigned long xer;
>>> +        unsigned long ccr;
>>> +        unsigned long softe;
>>> +        unsigned long trap;
>>> +        unsigned long dar;
>>> +        unsigned long dsisr;
>>> +        unsigned long result;
>>> +};
>>> +
>>> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
>>> +{
>>> +#if defined(TARGET_PPC64)
>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +    CPUPPCState *env = &cpu->env;
>>> +    uint64_t sp, next_sp, lr, buf[4];
>>
>> These are hwaddr really.
>>
>>> +
>>> +    /* stack pointer stored in r1 */
>>> +    sp = env->gpr[1];
>>> +
>>> +    while (sp && (sp & (0xCUL << 60))) {
>>> +        uint64_t marker = 0UL;
>>
>> sp = ppc_cpu_get_phys_page_debug(cs, sp) | (sp & ~TARGET_PAGE_MASK);
>>
>> and finish the loop when ppc_cpu_get_phys_page_debug returns -1?
>>
>>> +
>>> +        /* read and print LR */
>>> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
>>
>> and s/ & ~(0xCUL << 60)//
>>
>>> +        next_sp = buf[0];
>>> +        lr = buf[2];
>>
>> These two need to be converted from guest endian. For a BE guest, I see:
>>
>> sp: 0x000000007e582ff0 lr: 0xe4e60a00000000c0
>> sp: 0xffffffffffffffff lr: 0x0000600000006000
>> sp: 0x0000600000006000
>>
>>
>>> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
>>
>> HWADDR_PRIx. Or at least PRIx64, otherwise it won't compile on 32bit or
>> x86 or somewhere else.
>>
>>
>>> +        sp &= ~(0xCUL << 60);
>>
>> and remove this line. And now you can dump
>>
>>
>>
>>> +
>>> +        /* Does the stackframe contain regs? */
>>> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
>>
>> I suspect the marker needs byteswap as well.
> 
> Yeah, best to use the cpu_ldl() etc wrappers if possible, which
> include byteswaps.  Urgh... except this depends on the cpu mode which
> complicates things.
> 
>>
>> What is that 96?
>>
>>> +        if (marker == 0x7265677368657265) { /* regshere */
>>> +            struct ppc_pt_regs regs;
>>> +            int i;
>>> +
>>> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
>>
>> and the regs.
>>
>> What is that 112?
>>
>> I'd copy from arch/powerpc/include/asm/ptrace.h:
>>
>> #define STACK_FRAME_OVERHEAD    112     /* size of minimum stack frame */
>> #define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)
>>
>> and whatever that 96 is.
>>
>> Sadly, scripts/update-linux-headers.sh cannot copy
>> arch/powerpc/include/asm/ptrace.h as kernel's "make headers_install"
>> does not install it (it installs the "uapi" header which does not have
>> these symbols) so you'll have to define them.
>>
>>
>>> +
>>> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
>>> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
>>> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
>>> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
>>> +            for (i = 0; i < 32; i++)
>>> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
>>> +                            regs.gpr[i]);
>>> +        }
>>> +
>>> +        sp = next_sp;
>>> +    }
>>> +
>>> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
>>
>>
>> and this is "sp: 0x%"HWADDR_PRIx"\n".
>>
>>
>>
>>> +#endif
>>> +}
>>> +
>>>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>>>  {
>>>  #if defined(DO_PPC_STATISTICS)
>>> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
>>> index 0394a9ddad..3fd24f85cc 100644
>>> --- a/target/ppc/translate_init.inc.c
>>> +++ b/target/ppc/translate_init.inc.c
>>> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>>      cc->do_interrupt = ppc_cpu_do_interrupt;
>>>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>>>      cc->dump_state = ppc_cpu_dump_state;
>>> +    cc->dump_stack = ppc_cpu_dump_stack;
>>>      cc->dump_statistics = ppc_cpu_dump_statistics;
>>>      cc->set_pc = ppc_cpu_set_pc;
>>>      cc->gdb_read_register = ppc_cpu_gdb_read_register;
>>>
>>
> 

-- 
Alexey

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-02  3:47         ` Alexey Kardashevskiy
  0 siblings, 0 replies; 27+ messages in thread
From: Alexey Kardashevskiy @ 2019-05-02  3:47 UTC (permalink / raw)
  To: David Gibson; +Cc: armbru, qemu-ppc, qemu-devel, Suraj Jitindar Singh, dgilbert



On 02/05/2019 10:43, David Gibson wrote:
> On Wed, May 01, 2019 at 07:48:48PM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
>>> The monitor function dump-stack is used to dump the stack for a cpu.
>>> This can be useful for debugging purposes when the stack cannot be
>>> dumped by another means.
>>>
>>> Add a ppc implementation ppc_cpu_dump_stack().
>>> The stack pointer is stored in R1 with the back pointer at offset 0 and
>>> the link register at offset 2.
>>> Also dump the registers from the stack frame if the marker "regshere" is
>>> found.
>>
>> Is this a Linux only marker? ABI does not mentioned this.
>>
>>> This only dumps the kernel stack, stopping if a non-kernel address is
>>> found in the stack.
>>
>> Why enforce this limit?
> 
> It's also making a Linux specific assumption about addresses.


It does not have to if it used ppc_cpu_get_phys_page_debug(), the only
linux specific left is that "regshere" marker, otherwise it could work
for AIX or FreeBSD (obviously without the exception frame).


> 
>>
>>>
>>> Sample output:
>>> (qemu) dump-stack
>>> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
>>> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
>>> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
>>> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
>>> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
>>> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
>>> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
>>> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
>>> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
>>> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
>>> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
>>> 	trap : 0x0000000000000700
>>> 	pc   : 0xc000000000104490
>>> 	msr  : 0x9000000002843003
>>> 	lr   : 0xc000000000103ffc
>>> 	gpr 0: 0x0000000000000001
>>> 	gpr 1: 0xc00000005051f530
>>> 	gpr 2: 0xc000000001088200
>>> 	gpr 3: 0x0000000000000001
>>> 	gpr 4: 0xc000000032d60000
>>> 	gpr 5: 0xc0000000014b8f00
>>> 	gpr 6: 0x0000000000c835e0
>>> 	gpr 7: 0x0000000000000000
>>> 	gpr 8: 0x0000000000000000
>>> 	gpr 9: 0xc000000032f00000
>>> 	gpr10: 0x9000000002803033
>>> 	gpr11: 0xc000000000b60f00
>>> 	gpr12: 0x0000000000002000
>>> 	gpr13: 0xc000000001250000
>>> 	gpr14: 0x0000000000000000
>>> 	gpr15: 0x0000000000000008
>>> 	gpr16: 0x0000000000000000
>>> 	gpr17: 0xc00000000114f790
>>> 	gpr18: 0x00000000ffffffff
>>> 	gpr19: 0xc00000005051f8e8
>>> 	gpr20: 0x0000000000000001
>>> 	gpr21: 0x0000000000000000
>>> 	gpr22: 0x0000000000000001
>>> 	gpr23: 0x0000000000000001
>>> 	gpr24: 0x0000000000000001
>>> 	gpr25: 0xc0000000014b8f70
>>> 	gpr26: 0x0000000000000000
>>> 	gpr27: 0x0000000000000001
>>> 	gpr28: 0x0000000000000001
>>> 	gpr29: 0x0000000000000000
>>> 	gpr30: 0xc0000000014b8f00
>>> 	gpr31: 0xc0000000014b8f00
>>
>> Looks bulky, using the "info registers" format would make sense here.
>>
>>
>>> sp: 0xc00000005051f530 lr: 0x0000000000000000
>>> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
>>> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
>>> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
>>> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
>>> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
>>> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
>>> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
>>> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
>>> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
>>> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
>>> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
>>> 	trap : 0x0000000000000c01
>>> 	pc   : 0x00007fffa6c9d8d0
>>> 	msr  : 0x900000000280f033
>>> 	lr   : 0x0000000010090f40
>>> 	gpr 0: 0x0000000000000036
>>> 	gpr 1: 0x00007fffa62fdd70
>>> 	gpr 2: 0x00007fffa6d57300
>>> 	gpr 3: 0x000000000000000d
>>> 	gpr 4: 0x000000002000ae80
>>> 	gpr 5: 0x0000000000000000
>>> 	gpr 6: 0x0000000000000537
>>> 	gpr 7: 0x0000000000000000
>>> 	gpr 8: 0x000000000000000d
>>> 	gpr 9: 0x0000000000000000
>>> 	gpr10: 0x0000000000000000
>>> 	gpr11: 0x0000000000000000
>>> 	gpr12: 0x0000000000000000
>>> 	gpr13: 0x00007fffa6306380
>>> 	gpr14: 0x0000000000000000
>>> 	gpr15: 0x0000000000000001
>>> 	gpr16: 0x0000000039ba6928
>>> 	gpr17: 0x0000000000000000
>>> 	gpr18: 0x0000000000000000
>>> 	gpr19: 0x00007fffa6d702f0
>>> 	gpr20: 0x00007fffa62fddf0
>>> 	gpr21: 0x0000000000000080
>>> 	gpr22: 0x0000000000000004
>>> 	gpr23: 0x0000000000000000
>>> 	gpr24: 0x0000000010ac85c0
>>> 	gpr25: 0x0000000000000008
>>> 	gpr26: 0x00007fffa62fde10
>>> 	gpr27: 0x0000000000000000
>>> 	gpr28: 0x0000000000000002
>>> 	gpr29: 0x0000000000000000
>>> 	gpr30: 0x0000000039ba6900
>>> 	gpr31: 0x0000000010ac85c0
>>> sp: 0x00007fffa62fdd70
>>>
>>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>> ---
>>>  target/ppc/cpu.h                |  1 +
>>>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>>>  target/ppc/translate_init.inc.c |  1 +
>>>  3 files changed, 62 insertions(+)
>>>
>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>> index 5e7cf54b2f..28c4dffca1 100644
>>> --- a/target/ppc/cpu.h
>>> +++ b/target/ppc/cpu.h
>>> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>>>  void ppc_cpu_do_interrupt(CPUState *cpu);
>>>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>>>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>>> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>>>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>>>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>>>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index 8d08625c33..b162998ce7 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>>>  #undef RFPL
>>>  }
>>>  
>>> +struct ppc_pt_regs {
>>> +        unsigned long gpr[32];
>>> +        unsigned long nip;
>>> +        unsigned long msr;
>>> +        unsigned long orig_gpr3;
>>> +        unsigned long ctr;
>>> +        unsigned long link;
>>> +        unsigned long xer;
>>> +        unsigned long ccr;
>>> +        unsigned long softe;
>>> +        unsigned long trap;
>>> +        unsigned long dar;
>>> +        unsigned long dsisr;
>>> +        unsigned long result;
>>> +};
>>> +
>>> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
>>> +{
>>> +#if defined(TARGET_PPC64)
>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +    CPUPPCState *env = &cpu->env;
>>> +    uint64_t sp, next_sp, lr, buf[4];
>>
>> These are hwaddr really.
>>
>>> +
>>> +    /* stack pointer stored in r1 */
>>> +    sp = env->gpr[1];
>>> +
>>> +    while (sp && (sp & (0xCUL << 60))) {
>>> +        uint64_t marker = 0UL;
>>
>> sp = ppc_cpu_get_phys_page_debug(cs, sp) | (sp & ~TARGET_PAGE_MASK);
>>
>> and finish the loop when ppc_cpu_get_phys_page_debug returns -1?
>>
>>> +
>>> +        /* read and print LR */
>>> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
>>
>> and s/ & ~(0xCUL << 60)//
>>
>>> +        next_sp = buf[0];
>>> +        lr = buf[2];
>>
>> These two need to be converted from guest endian. For a BE guest, I see:
>>
>> sp: 0x000000007e582ff0 lr: 0xe4e60a00000000c0
>> sp: 0xffffffffffffffff lr: 0x0000600000006000
>> sp: 0x0000600000006000
>>
>>
>>> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
>>
>> HWADDR_PRIx. Or at least PRIx64, otherwise it won't compile on 32bit or
>> x86 or somewhere else.
>>
>>
>>> +        sp &= ~(0xCUL << 60);
>>
>> and remove this line. And now you can dump
>>
>>
>>
>>> +
>>> +        /* Does the stackframe contain regs? */
>>> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
>>
>> I suspect the marker needs byteswap as well.
> 
> Yeah, best to use the cpu_ldl() etc wrappers if possible, which
> include byteswaps.  Urgh... except this depends on the cpu mode which
> complicates things.
> 
>>
>> What is that 96?
>>
>>> +        if (marker == 0x7265677368657265) { /* regshere */
>>> +            struct ppc_pt_regs regs;
>>> +            int i;
>>> +
>>> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
>>
>> and the regs.
>>
>> What is that 112?
>>
>> I'd copy from arch/powerpc/include/asm/ptrace.h:
>>
>> #define STACK_FRAME_OVERHEAD    112     /* size of minimum stack frame */
>> #define STACK_FRAME_REGS_MARKER ASM_CONST(0x7265677368657265)
>>
>> and whatever that 96 is.
>>
>> Sadly, scripts/update-linux-headers.sh cannot copy
>> arch/powerpc/include/asm/ptrace.h as kernel's "make headers_install"
>> does not install it (it installs the "uapi" header which does not have
>> these symbols) so you'll have to define them.
>>
>>
>>> +
>>> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
>>> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
>>> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
>>> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
>>> +            for (i = 0; i < 32; i++)
>>> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
>>> +                            regs.gpr[i]);
>>> +        }
>>> +
>>> +        sp = next_sp;
>>> +    }
>>> +
>>> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
>>
>>
>> and this is "sp: 0x%"HWADDR_PRIx"\n".
>>
>>
>>
>>> +#endif
>>> +}
>>> +
>>>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>>>  {
>>>  #if defined(DO_PPC_STATISTICS)
>>> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
>>> index 0394a9ddad..3fd24f85cc 100644
>>> --- a/target/ppc/translate_init.inc.c
>>> +++ b/target/ppc/translate_init.inc.c
>>> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>>>      cc->do_interrupt = ppc_cpu_do_interrupt;
>>>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>>>      cc->dump_state = ppc_cpu_dump_state;
>>> +    cc->dump_stack = ppc_cpu_dump_stack;
>>>      cc->dump_statistics = ppc_cpu_dump_statistics;
>>>      cc->set_pc = ppc_cpu_set_pc;
>>>      cc->gdb_read_register = ppc_cpu_gdb_read_register;
>>>
>>
> 

-- 
Alexey


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-02 13:59     ` Greg Kurz
  0 siblings, 0 replies; 27+ messages in thread
From: Greg Kurz @ 2019-05-02 13:59 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: qemu-devel, david, qemu-ppc, dgilbert, armbru

On Wed,  1 May 2019 15:35:22 +1000
Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:

> The monitor function dump-stack is used to dump the stack for a cpu.
> This can be useful for debugging purposes when the stack cannot be
> dumped by another means.
> 
> Add a ppc implementation ppc_cpu_dump_stack().
> The stack pointer is stored in R1 with the back pointer at offset 0 and
> the link register at offset 2.
> Also dump the registers from the stack frame if the marker "regshere" is
> found.
> This only dumps the kernel stack, stopping if a non-kernel address is
> found in the stack.
> 
> Sample output:
> (qemu) dump-stack
> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> 	trap : 0x0000000000000700
> 	pc   : 0xc000000000104490
> 	msr  : 0x9000000002843003
> 	lr   : 0xc000000000103ffc
> 	gpr 0: 0x0000000000000001
> 	gpr 1: 0xc00000005051f530
> 	gpr 2: 0xc000000001088200
> 	gpr 3: 0x0000000000000001
> 	gpr 4: 0xc000000032d60000
> 	gpr 5: 0xc0000000014b8f00
> 	gpr 6: 0x0000000000c835e0
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x0000000000000000
> 	gpr 9: 0xc000000032f00000
> 	gpr10: 0x9000000002803033
> 	gpr11: 0xc000000000b60f00
> 	gpr12: 0x0000000000002000
> 	gpr13: 0xc000000001250000
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000008
> 	gpr16: 0x0000000000000000
> 	gpr17: 0xc00000000114f790
> 	gpr18: 0x00000000ffffffff
> 	gpr19: 0xc00000005051f8e8
> 	gpr20: 0x0000000000000001
> 	gpr21: 0x0000000000000000
> 	gpr22: 0x0000000000000001
> 	gpr23: 0x0000000000000001
> 	gpr24: 0x0000000000000001
> 	gpr25: 0xc0000000014b8f70
> 	gpr26: 0x0000000000000000
> 	gpr27: 0x0000000000000001
> 	gpr28: 0x0000000000000001
> 	gpr29: 0x0000000000000000
> 	gpr30: 0xc0000000014b8f00
> 	gpr31: 0xc0000000014b8f00
> sp: 0xc00000005051f530 lr: 0x0000000000000000
> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> 	trap : 0x0000000000000c01
> 	pc   : 0x00007fffa6c9d8d0
> 	msr  : 0x900000000280f033
> 	lr   : 0x0000000010090f40
> 	gpr 0: 0x0000000000000036
> 	gpr 1: 0x00007fffa62fdd70
> 	gpr 2: 0x00007fffa6d57300
> 	gpr 3: 0x000000000000000d
> 	gpr 4: 0x000000002000ae80
> 	gpr 5: 0x0000000000000000
> 	gpr 6: 0x0000000000000537
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x000000000000000d
> 	gpr 9: 0x0000000000000000
> 	gpr10: 0x0000000000000000
> 	gpr11: 0x0000000000000000
> 	gpr12: 0x0000000000000000
> 	gpr13: 0x00007fffa6306380
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000001
> 	gpr16: 0x0000000039ba6928
> 	gpr17: 0x0000000000000000
> 	gpr18: 0x0000000000000000
> 	gpr19: 0x00007fffa6d702f0
> 	gpr20: 0x00007fffa62fddf0
> 	gpr21: 0x0000000000000080
> 	gpr22: 0x0000000000000004
> 	gpr23: 0x0000000000000000
> 	gpr24: 0x0000000010ac85c0
> 	gpr25: 0x0000000000000008
> 	gpr26: 0x00007fffa62fde10
> 	gpr27: 0x0000000000000000
> 	gpr28: 0x0000000000000002
> 	gpr29: 0x0000000000000000
> 	gpr30: 0x0000000039ba6900
> 	gpr31: 0x0000000010ac85c0
> sp: 0x00007fffa62fdd70
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  target/ppc/cpu.h                |  1 +
>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c |  1 +
>  3 files changed, 62 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5e7cf54b2f..28c4dffca1 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 8d08625c33..b162998ce7 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #undef RFPL
>  }
>  
> +struct ppc_pt_regs {
> +        unsigned long gpr[32];
> +        unsigned long nip;
> +        unsigned long msr;
> +        unsigned long orig_gpr3;
> +        unsigned long ctr;
> +        unsigned long link;
> +        unsigned long xer;
> +        unsigned long ccr;
> +        unsigned long softe;
> +        unsigned long trap;
> +        unsigned long dar;
> +        unsigned long dsisr;
> +        unsigned long result;
> +};
> +
> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> +{
> +#if defined(TARGET_PPC64)
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t sp, next_sp, lr, buf[4];
> +
> +    /* stack pointer stored in r1 */
> +    sp = env->gpr[1];
> +
> +    while (sp && (sp & (0xCUL << 60))) {

0xCUL << 60 isn't appropriate for 32-bit hosts. Use 0xCULL instead.

> +        uint64_t marker = 0UL;
> +
> +        /* read and print LR */
> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
> +        next_sp = buf[0];
> +        lr = buf[2];
> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
> +        sp &= ~(0xCUL << 60);
> +
> +        /* Does the stackframe contain regs? */
> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
> +        if (marker == 0x7265677368657265) { /* regshere */
> +            struct ppc_pt_regs regs;
> +            int i;
> +
> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
> +
> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> +            for (i = 0; i < 32; i++)
> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> +                            regs.gpr[i]);
> +        }
> +
> +        sp = next_sp;
> +    }
> +
> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
> +#endif
> +}
> +
>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>  {
>  #if defined(DO_PPC_STATISTICS)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 0394a9ddad..3fd24f85cc 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = ppc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>      cc->dump_state = ppc_cpu_dump_state;
> +    cc->dump_stack = ppc_cpu_dump_stack;
>      cc->dump_statistics = ppc_cpu_dump_statistics;
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] ppc: Add dump-stack implementation
@ 2019-05-02 13:59     ` Greg Kurz
  0 siblings, 0 replies; 27+ messages in thread
From: Greg Kurz @ 2019-05-02 13:59 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: armbru, qemu-ppc, qemu-devel, dgilbert, david

On Wed,  1 May 2019 15:35:22 +1000
Suraj Jitindar Singh <sjitindarsingh@gmail.com> wrote:

> The monitor function dump-stack is used to dump the stack for a cpu.
> This can be useful for debugging purposes when the stack cannot be
> dumped by another means.
> 
> Add a ppc implementation ppc_cpu_dump_stack().
> The stack pointer is stored in R1 with the back pointer at offset 0 and
> the link register at offset 2.
> Also dump the registers from the stack frame if the marker "regshere" is
> found.
> This only dumps the kernel stack, stopping if a non-kernel address is
> found in the stack.
> 
> Sample output:
> (qemu) dump-stack
> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> 	trap : 0x0000000000000700
> 	pc   : 0xc000000000104490
> 	msr  : 0x9000000002843003
> 	lr   : 0xc000000000103ffc
> 	gpr 0: 0x0000000000000001
> 	gpr 1: 0xc00000005051f530
> 	gpr 2: 0xc000000001088200
> 	gpr 3: 0x0000000000000001
> 	gpr 4: 0xc000000032d60000
> 	gpr 5: 0xc0000000014b8f00
> 	gpr 6: 0x0000000000c835e0
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x0000000000000000
> 	gpr 9: 0xc000000032f00000
> 	gpr10: 0x9000000002803033
> 	gpr11: 0xc000000000b60f00
> 	gpr12: 0x0000000000002000
> 	gpr13: 0xc000000001250000
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000008
> 	gpr16: 0x0000000000000000
> 	gpr17: 0xc00000000114f790
> 	gpr18: 0x00000000ffffffff
> 	gpr19: 0xc00000005051f8e8
> 	gpr20: 0x0000000000000001
> 	gpr21: 0x0000000000000000
> 	gpr22: 0x0000000000000001
> 	gpr23: 0x0000000000000001
> 	gpr24: 0x0000000000000001
> 	gpr25: 0xc0000000014b8f70
> 	gpr26: 0x0000000000000000
> 	gpr27: 0x0000000000000001
> 	gpr28: 0x0000000000000001
> 	gpr29: 0x0000000000000000
> 	gpr30: 0xc0000000014b8f00
> 	gpr31: 0xc0000000014b8f00
> sp: 0xc00000005051f530 lr: 0x0000000000000000
> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> 	trap : 0x0000000000000c01
> 	pc   : 0x00007fffa6c9d8d0
> 	msr  : 0x900000000280f033
> 	lr   : 0x0000000010090f40
> 	gpr 0: 0x0000000000000036
> 	gpr 1: 0x00007fffa62fdd70
> 	gpr 2: 0x00007fffa6d57300
> 	gpr 3: 0x000000000000000d
> 	gpr 4: 0x000000002000ae80
> 	gpr 5: 0x0000000000000000
> 	gpr 6: 0x0000000000000537
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x000000000000000d
> 	gpr 9: 0x0000000000000000
> 	gpr10: 0x0000000000000000
> 	gpr11: 0x0000000000000000
> 	gpr12: 0x0000000000000000
> 	gpr13: 0x00007fffa6306380
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000001
> 	gpr16: 0x0000000039ba6928
> 	gpr17: 0x0000000000000000
> 	gpr18: 0x0000000000000000
> 	gpr19: 0x00007fffa6d702f0
> 	gpr20: 0x00007fffa62fddf0
> 	gpr21: 0x0000000000000080
> 	gpr22: 0x0000000000000004
> 	gpr23: 0x0000000000000000
> 	gpr24: 0x0000000010ac85c0
> 	gpr25: 0x0000000000000008
> 	gpr26: 0x00007fffa62fde10
> 	gpr27: 0x0000000000000000
> 	gpr28: 0x0000000000000002
> 	gpr29: 0x0000000000000000
> 	gpr30: 0x0000000039ba6900
> 	gpr31: 0x0000000010ac85c0
> sp: 0x00007fffa62fdd70
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  target/ppc/cpu.h                |  1 +
>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c |  1 +
>  3 files changed, 62 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5e7cf54b2f..28c4dffca1 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 8d08625c33..b162998ce7 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #undef RFPL
>  }
>  
> +struct ppc_pt_regs {
> +        unsigned long gpr[32];
> +        unsigned long nip;
> +        unsigned long msr;
> +        unsigned long orig_gpr3;
> +        unsigned long ctr;
> +        unsigned long link;
> +        unsigned long xer;
> +        unsigned long ccr;
> +        unsigned long softe;
> +        unsigned long trap;
> +        unsigned long dar;
> +        unsigned long dsisr;
> +        unsigned long result;
> +};
> +
> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> +{
> +#if defined(TARGET_PPC64)
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t sp, next_sp, lr, buf[4];
> +
> +    /* stack pointer stored in r1 */
> +    sp = env->gpr[1];
> +
> +    while (sp && (sp & (0xCUL << 60))) {

0xCUL << 60 isn't appropriate for 32-bit hosts. Use 0xCULL instead.

> +        uint64_t marker = 0UL;
> +
> +        /* read and print LR */
> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
> +        next_sp = buf[0];
> +        lr = buf[2];
> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
> +        sp &= ~(0xCUL << 60);
> +
> +        /* Does the stackframe contain regs? */
> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
> +        if (marker == 0x7265677368657265) { /* regshere */
> +            struct ppc_pt_regs regs;
> +            int i;
> +
> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
> +
> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> +            for (i = 0; i < 32; i++)
> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> +                            regs.gpr[i]);
> +        }
> +
> +        sp = next_sp;
> +    }
> +
> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
> +#endif
> +}
> +
>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>  {
>  #if defined(DO_PPC_STATISTICS)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 0394a9ddad..3fd24f85cc 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = ppc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>      cc->dump_state = ppc_cpu_dump_state;
> +    cc->dump_stack = ppc_cpu_dump_stack;
>      cc->dump_statistics = ppc_cpu_dump_statistics;
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
  2019-05-02  3:47         ` Alexey Kardashevskiy
  (?)
@ 2019-05-06  3:39         ` David Gibson
  -1 siblings, 0 replies; 27+ messages in thread
From: David Gibson @ 2019-05-06  3:39 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: armbru, qemu-ppc, qemu-devel, Suraj Jitindar Singh, dgilbert

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

On Thu, May 02, 2019 at 01:47:32PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 02/05/2019 10:43, David Gibson wrote:
> > On Wed, May 01, 2019 at 07:48:48PM +1000, Alexey Kardashevskiy wrote:
> >>
> >>
> >> On 01/05/2019 15:35, Suraj Jitindar Singh wrote:
> >>> The monitor function dump-stack is used to dump the stack for a cpu.
> >>> This can be useful for debugging purposes when the stack cannot be
> >>> dumped by another means.
> >>>
> >>> Add a ppc implementation ppc_cpu_dump_stack().
> >>> The stack pointer is stored in R1 with the back pointer at offset 0 and
> >>> the link register at offset 2.
> >>> Also dump the registers from the stack frame if the marker "regshere" is
> >>> found.
> >>
> >> Is this a Linux only marker? ABI does not mentioned this.
> >>
> >>> This only dumps the kernel stack, stopping if a non-kernel address is
> >>> found in the stack.
> >>
> >> Why enforce this limit?
> > 
> > It's also making a Linux specific assumption about addresses.
> 
> It does not have to if it used ppc_cpu_get_phys_page_debug(), the only
> linux specific left is that "regshere" marker, otherwise it could work
> for AIX or FreeBSD (obviously without the exception frame).

Sorry, I thought this was relying on the fact that Linux put its
linear mapping at 0xc0..., but I realized I was misreading what was
just the masking of the top two real mode address bits which is in the
hardware.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
  2019-05-01  5:35 ` Suraj Jitindar Singh
                   ` (3 preceding siblings ...)
  (?)
@ 2019-05-07 11:09 ` Markus Armbruster
  2019-05-08 10:26   ` Dr. David Alan Gilbert
  -1 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2019-05-07 11:09 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: qemu-ppc, qemu-devel, dgilbert, david

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> Add a monitor command "dump-stack" to be used to dump the stack for the
> current cpu.

I guess this is just for debugging.  Correct?

Shouldn't this be "info stack", to match "info registers" and "info
cpustats"?

>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  hmp-commands.hx   | 13 +++++++++++++
>  hmp.h             |  1 +
>  include/qom/cpu.h | 10 ++++++++++
>  monitor.c         | 12 ++++++++++++
>  qom/cpu.c         | 10 ++++++++++
>  5 files changed, 46 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9b4035965c..965ccdea28 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -862,6 +862,19 @@ ETEXI
>      },
>  
>  STEXI
> +@item dump-stack
> +@findex dump-stack
> +dump stack of the cpu
> +ETEXI
> +    {
> +        .name           = "dump-stack",
> +        .args_type      = "",
> +        .params         = "",
> +        .help           = "dump stack",
> +        .cmd            = hmp_dumpstack,
> +    },
> +
> +STEXI
>  @item pmemsave @var{addr} @var{size} @var{file}
>  @findex pmemsave
>  save to disk physical memory dump starting at @var{addr} of size @var{size}.
> diff --git a/hmp.h b/hmp.h
> index 43617f2646..e6edf1215c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 08abcbd3fe..f2e83e9918 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>      void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
>  /**
> + * cpu_dump_stack:
> + * @cpu: The CPU whose stack is to be dumped.
> + * @f: If non-null, dump to this stream, else to current print sink.
> + *
> + * Dumps CPU stack.
> + */
> +void cpu_dump_stack(CPUState *cpu, FILE *f);
> +
> +/**
>   * cpu_dump_statistics:
>   * @cpu: The CPU whose state is to be dumped.
>   * @flags: Flags what to dump.
> diff --git a/monitor.c b/monitor.c
> index 9b5f10b475..dbec2e4376 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> +{
> +    CPUState *cs = mon_get_cpu();
> +
> +    if (!cs) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    cpu_dump_stack(cs, NULL);
> +}
> +
>  #ifdef CONFIG_TCG
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3c5493c96c..0dc10004f4 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
>      }
>  }
>  
> +void cpu_dump_stack(CPUState *cpu, FILE *f)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->dump_stack) {
> +        cpu_synchronize_state(cpu);
> +        cc->dump_stack(cpu, f);
> +    }
> +}

Silently does nothing when the CPU doesn't implement dump_stack().
Matches "info registers" and "info cpustats".  Awful UI, but at least
it's consistently awful.

> +
>  void cpu_dump_statistics(CPUState *cpu, int flags)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] monitor: Add dump-stack command
  2019-05-02  2:15     ` Alexey Kardashevskiy
  (?)
@ 2019-05-07 11:21     ` Markus Armbruster
  -1 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2019-05-07 11:21 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: qemu-devel, qemu-ppc, dgilbert, Suraj Jitindar Singh, David Gibson

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 02/05/2019 10:44, David Gibson wrote:
>> On Wed, May 01, 2019 at 03:35:21PM +1000, Suraj Jitindar Singh wrote:
>>> Add a monitor command "dump-stack" to be used to dump the stack for the
>>> current cpu.
>> 
>> So, you can already get guest backtraces by using the gdbstub
>
> Not in the field - this requires QEMU to run with -s which is not
> usually the case.

If I understand help correctly, you can hot-add a gdbserver with HMP
command gdbserver.  Makes me think ...

> But since we almost always deal with QEMUs run by libvirt and HMP/QMP is
> always available, one could write a script doing QMP's
> "human-monitor-command x/16g" or "virsh qemu-monitor-command --hmp
> x/16g" to read the guest memory and MSR:LE and dump the stack with the
> exception frame.
>
>
>> functionality.  I can see some benefit in allowing this more easily
>> through hmp, but whether it's worth the code size, I'm less certain.

... David's "why not use gdb" should be considered once more.

> It still seems easier than running an external script talking to HMP/QMP
> as you would not want to write such script in bash but rather in a
> better language which might not be installed on the client machine (like
> missing python3 on many RHEL :) ). Thanks,

Your point "while it's possible to trace the stack with nothing but
x/16g and a general purpose programming language, we shouldn't make our
users jump through this hoop" is valid.

However, the new command is actually competing with gdb.  How much of
gdb do we want to build into QEMU to avoid the inconvenience of having
to install gdb and hot-add a gdb server?


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation
  2019-05-01  5:35   ` Suraj Jitindar Singh
                     ` (2 preceding siblings ...)
  (?)
@ 2019-05-07 11:24   ` Markus Armbruster
  -1 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2019-05-07 11:24 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: qemu-ppc, qemu-devel, dgilbert, david

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> The monitor function dump-stack is used to dump the stack for a cpu.
> This can be useful for debugging purposes when the stack cannot be
> dumped by another means.
>
> Add a ppc implementation ppc_cpu_dump_stack().
> The stack pointer is stored in R1 with the back pointer at offset 0 and
> the link register at offset 2.
> Also dump the registers from the stack frame if the marker "regshere" is
> found.
> This only dumps the kernel stack, stopping if a non-kernel address is
> found in the stack.
>
> Sample output:
> (qemu) dump-stack
> sp: 0xc00000007bfc5690 lr: 0xc0000000000974b8
> sp: 0xc00000007bfc56f0 lr: 0xc00000000065aab4
> sp: 0xc00000007bfc5720 lr: 0xc00000000065ab04
> sp: 0xc00000007bfc5740 lr: 0xc0000000000c29b8
> sp: 0xc00000007bfc57b0 lr: 0xc0000000000bc9e8
> sp: 0xc00000007bfc57e0 lr: 0xc0000000000bd584
> sp: 0xc00000007bfc5800 lr: 0xc0000000000bee14
> sp: 0xc00000007bfc5ac0 lr: 0xc0000000000c2100
> sp: 0xc00000007bfc5c60 lr: 0xc000000000029460
> sp: 0xc00000007bfc5ca0 lr: 0xc00000000010b5e8
> sp: 0xc00000007bfc5d00 lr: 0xc000000000105f34
> 	trap : 0x0000000000000700
> 	pc   : 0xc000000000104490
> 	msr  : 0x9000000002843003
> 	lr   : 0xc000000000103ffc
> 	gpr 0: 0x0000000000000001
> 	gpr 1: 0xc00000005051f530
> 	gpr 2: 0xc000000001088200
> 	gpr 3: 0x0000000000000001
> 	gpr 4: 0xc000000032d60000
> 	gpr 5: 0xc0000000014b8f00
> 	gpr 6: 0x0000000000c835e0
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x0000000000000000
> 	gpr 9: 0xc000000032f00000
> 	gpr10: 0x9000000002803033
> 	gpr11: 0xc000000000b60f00
> 	gpr12: 0x0000000000002000
> 	gpr13: 0xc000000001250000
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000008
> 	gpr16: 0x0000000000000000
> 	gpr17: 0xc00000000114f790
> 	gpr18: 0x00000000ffffffff
> 	gpr19: 0xc00000005051f8e8
> 	gpr20: 0x0000000000000001
> 	gpr21: 0x0000000000000000
> 	gpr22: 0x0000000000000001
> 	gpr23: 0x0000000000000001
> 	gpr24: 0x0000000000000001
> 	gpr25: 0xc0000000014b8f70
> 	gpr26: 0x0000000000000000
> 	gpr27: 0x0000000000000001
> 	gpr28: 0x0000000000000001
> 	gpr29: 0x0000000000000000
> 	gpr30: 0xc0000000014b8f00
> 	gpr31: 0xc0000000014b8f00
> sp: 0xc00000005051f530 lr: 0x0000000000000000
> sp: 0xc00000005051f600 lr: 0xc000000000103ffc
> sp: 0xc00000005051f670 lr: 0xc0000000000f60a8
> sp: 0xc00000005051f850 lr: 0xc0000000000f18c0
> sp: 0xc00000005051fa10 lr: 0xc0000000000f5184
> sp: 0xc00000005051fae0 lr: 0xc0000000000ddf54
> sp: 0xc00000005051fb00 lr: 0xc0000000000dab9c
> sp: 0xc00000005051fb90 lr: 0xc0000000000cbf88
> sp: 0xc00000005051fd00 lr: 0xc0000000003e7480
> sp: 0xc00000005051fdb0 lr: 0xc0000000003e7ce4
> sp: 0xc00000005051fe00 lr: 0xc0000000003e7d88
> sp: 0xc00000005051fe20 lr: 0xc00000000000b3a4
> 	trap : 0x0000000000000c01
> 	pc   : 0x00007fffa6c9d8d0
> 	msr  : 0x900000000280f033
> 	lr   : 0x0000000010090f40
> 	gpr 0: 0x0000000000000036
> 	gpr 1: 0x00007fffa62fdd70
> 	gpr 2: 0x00007fffa6d57300
> 	gpr 3: 0x000000000000000d
> 	gpr 4: 0x000000002000ae80
> 	gpr 5: 0x0000000000000000
> 	gpr 6: 0x0000000000000537
> 	gpr 7: 0x0000000000000000
> 	gpr 8: 0x000000000000000d
> 	gpr 9: 0x0000000000000000
> 	gpr10: 0x0000000000000000
> 	gpr11: 0x0000000000000000
> 	gpr12: 0x0000000000000000
> 	gpr13: 0x00007fffa6306380
> 	gpr14: 0x0000000000000000
> 	gpr15: 0x0000000000000001
> 	gpr16: 0x0000000039ba6928
> 	gpr17: 0x0000000000000000
> 	gpr18: 0x0000000000000000
> 	gpr19: 0x00007fffa6d702f0
> 	gpr20: 0x00007fffa62fddf0
> 	gpr21: 0x0000000000000080
> 	gpr22: 0x0000000000000004
> 	gpr23: 0x0000000000000000
> 	gpr24: 0x0000000010ac85c0
> 	gpr25: 0x0000000000000008
> 	gpr26: 0x00007fffa62fde10
> 	gpr27: 0x0000000000000000
> 	gpr28: 0x0000000000000002
> 	gpr29: 0x0000000000000000
> 	gpr30: 0x0000000039ba6900
> 	gpr31: 0x0000000010ac85c0
> sp: 0x00007fffa62fdd70
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  target/ppc/cpu.h                |  1 +
>  target/ppc/translate.c          | 60 +++++++++++++++++++++++++++++++++++++++++
>  target/ppc/translate_init.inc.c |  1 +
>  3 files changed, 62 insertions(+)

The diffstat doesn't look prohibitive.  However, I fear implementations
for more ornery targets such as x86 might take much more code.

>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5e7cf54b2f..28c4dffca1 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1284,6 +1284,7 @@ struct PPCVirtualHypervisorClass {
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> +void ppc_cpu_dump_stack(CPUState *cpu, FILE *f);
>  void ppc_cpu_dump_statistics(CPUState *cpu, int flags);
>  hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int ppc_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 8d08625c33..b162998ce7 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7705,6 +7705,66 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #undef RFPL
>  }
>  
> +struct ppc_pt_regs {
> +        unsigned long gpr[32];
> +        unsigned long nip;
> +        unsigned long msr;
> +        unsigned long orig_gpr3;
> +        unsigned long ctr;
> +        unsigned long link;
> +        unsigned long xer;
> +        unsigned long ccr;
> +        unsigned long softe;
> +        unsigned long trap;
> +        unsigned long dar;
> +        unsigned long dsisr;
> +        unsigned long result;
> +};
> +
> +void ppc_cpu_dump_stack(CPUState *cs, FILE *f)
> +{
> +#if defined(TARGET_PPC64)

Ignorant question: why do you need the #if here?

> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    uint64_t sp, next_sp, lr, buf[4];
> +
> +    /* stack pointer stored in r1 */
> +    sp = env->gpr[1];
> +
> +    while (sp && (sp & (0xCUL << 60))) {
> +        uint64_t marker = 0UL;
> +
> +        /* read and print LR */
> +        cpu_physical_memory_read(sp & ~(0xCUL << 60), buf, sizeof(*buf) * 4);
> +        next_sp = buf[0];
> +        lr = buf[2];
> +        qemu_fprintf(f, "sp: 0x%.16lx lr: 0x%.16lx\n", sp, lr);
> +        sp &= ~(0xCUL << 60);
> +
> +        /* Does the stackframe contain regs? */
> +        cpu_physical_memory_read(sp + 96, &marker, sizeof(marker));
> +        if (marker == 0x7265677368657265) { /* regshere */
> +            struct ppc_pt_regs regs;
> +            int i;
> +
> +            cpu_physical_memory_read(sp + 112, &regs, sizeof(regs));
> +
> +            qemu_fprintf(f, "\ttrap : 0x%.16lx\n", regs.trap);
> +            qemu_fprintf(f, "\tpc   : 0x%.16lx\n", regs.nip);
> +            qemu_fprintf(f, "\tmsr  : 0x%.16lx\n", regs.msr);
> +            qemu_fprintf(f, "\tlr   : 0x%.16lx\n", regs.link);
> +            for (i = 0; i < 32; i++)
> +                qemu_fprintf(f, "\tgpr%2d: 0x%.16lx\n", i,
> +                            regs.gpr[i]);
> +        }
> +
> +        sp = next_sp;
> +    }
> +
> +    qemu_fprintf(f, "sp: 0x%.16lx\n", sp);
> +#endif
> +}
> +
>  void ppc_cpu_dump_statistics(CPUState *cs, int flags)
>  {
>  #if defined(DO_PPC_STATISTICS)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index 0394a9ddad..3fd24f85cc 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10587,6 +10587,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = ppc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = ppc_cpu_exec_interrupt;
>      cc->dump_state = ppc_cpu_dump_state;
> +    cc->dump_stack = ppc_cpu_dump_stack;
>      cc->dump_statistics = ppc_cpu_dump_statistics;
>      cc->set_pc = ppc_cpu_set_pc;
>      cc->gdb_read_register = ppc_cpu_gdb_read_register;



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
  2019-05-07 11:09 ` [Qemu-devel] " Markus Armbruster
@ 2019-05-08 10:26   ` Dr. David Alan Gilbert
  2019-05-08 13:10     ` Markus Armbruster
  0 siblings, 1 reply; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2019-05-08 10:26 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-ppc, qemu-devel, Suraj Jitindar Singh, david

* Markus Armbruster (armbru@redhat.com) wrote:
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> 
> > Add a monitor command "dump-stack" to be used to dump the stack for the
> > current cpu.
> 
> I guess this is just for debugging.  Correct?
> 
> Shouldn't this be "info stack", to match "info registers" and "info
> cpustats"?

Since this is primarily about walking the guests stack frames and not
walking qemu internal data structures, I think it's probably OK to be
a dump-stack rather than an info subcommand.

Dave

> >
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  hmp-commands.hx   | 13 +++++++++++++
> >  hmp.h             |  1 +
> >  include/qom/cpu.h | 10 ++++++++++
> >  monitor.c         | 12 ++++++++++++
> >  qom/cpu.c         | 10 ++++++++++
> >  5 files changed, 46 insertions(+)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 9b4035965c..965ccdea28 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -862,6 +862,19 @@ ETEXI
> >      },
> >  
> >  STEXI
> > +@item dump-stack
> > +@findex dump-stack
> > +dump stack of the cpu
> > +ETEXI
> > +    {
> > +        .name           = "dump-stack",
> > +        .args_type      = "",
> > +        .params         = "",
> > +        .help           = "dump stack",
> > +        .cmd            = hmp_dumpstack,
> > +    },
> > +
> > +STEXI
> >  @item pmemsave @var{addr} @var{size} @var{file}
> >  @findex pmemsave
> >  save to disk physical memory dump starting at @var{addr} of size @var{size}.
> > diff --git a/hmp.h b/hmp.h
> > index 43617f2646..e6edf1215c 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict);
> >  void hmp_cpu(Monitor *mon, const QDict *qdict);
> >  void hmp_memsave(Monitor *mon, const QDict *qdict);
> >  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> > +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
> >  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
> >  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
> >  void hmp_cont(Monitor *mon, const QDict *qdict);
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 08abcbd3fe..f2e83e9918 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -181,6 +181,7 @@ typedef struct CPUClass {
> >      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
> >                             uint8_t *buf, int len, bool is_write);
> >      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> > +    void (*dump_stack)(CPUState *cpu, FILE *f);
> >      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
> >      void (*dump_statistics)(CPUState *cpu, int flags);
> >      int64_t (*get_arch_id)(CPUState *cpu);
> > @@ -568,6 +569,15 @@ enum CPUDumpFlags {
> >  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> >  
> >  /**
> > + * cpu_dump_stack:
> > + * @cpu: The CPU whose stack is to be dumped.
> > + * @f: If non-null, dump to this stream, else to current print sink.
> > + *
> > + * Dumps CPU stack.
> > + */
> > +void cpu_dump_stack(CPUState *cpu, FILE *f);
> > +
> > +/**
> >   * cpu_dump_statistics:
> >   * @cpu: The CPU whose state is to be dumped.
> >   * @flags: Flags what to dump.
> > diff --git a/monitor.c b/monitor.c
> > index 9b5f10b475..dbec2e4376 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon, const QDict *qdict)
> >      }
> >  }
> >  
> > +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> > +{
> > +    CPUState *cs = mon_get_cpu();
> > +
> > +    if (!cs) {
> > +        monitor_printf(mon, "No CPU available\n");
> > +        return;
> > +    }
> > +
> > +    cpu_dump_stack(cs, NULL);
> > +}
> > +
> >  #ifdef CONFIG_TCG
> >  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
> >  {
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 3c5493c96c..0dc10004f4 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags)
> >      }
> >  }
> >  
> > +void cpu_dump_stack(CPUState *cpu, FILE *f)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (cc->dump_stack) {
> > +        cpu_synchronize_state(cpu);
> > +        cc->dump_stack(cpu, f);
> > +    }
> > +}
> 
> Silently does nothing when the CPU doesn't implement dump_stack().
> Matches "info registers" and "info cpustats".  Awful UI, but at least
> it's consistently awful.
> 
> > +
> >  void cpu_dump_statistics(CPUState *cpu, int flags)
> >  {
> >      CPUClass *cc = CPU_GET_CLASS(cpu);
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
  2019-05-08 10:26   ` Dr. David Alan Gilbert
@ 2019-05-08 13:10     ` Markus Armbruster
  2019-05-08 13:15       ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2019-05-08 13:10 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-ppc, qemu-devel, Suraj Jitindar Singh, david

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
>> 
>> > Add a monitor command "dump-stack" to be used to dump the stack for the
>> > current cpu.
>> 
>> I guess this is just for debugging.  Correct?
>> 
>> Shouldn't this be "info stack", to match "info registers" and "info
>> cpustats"?
>
> Since this is primarily about walking the guests stack frames and not
> walking qemu internal data structures, I think it's probably OK to be
> a dump-stack rather than an info subcommand.

Well, "info registers" is also about the guest's state and not QEMU
internal state.  Arguably, so are "info pic", "info tlb", ...

We have a long-standing tradition of using "info" for "pure"
information-retrieving commands.  I rather like that pattern.

Ultimately your choice as the HMP maintainer, of course.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
  2019-05-08 13:10     ` Markus Armbruster
@ 2019-05-08 13:15       ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 27+ messages in thread
From: Dr. David Alan Gilbert @ 2019-05-08 13:15 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-ppc, qemu-devel, Suraj Jitindar Singh, david

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> >> 
> >> > Add a monitor command "dump-stack" to be used to dump the stack for the
> >> > current cpu.
> >> 
> >> I guess this is just for debugging.  Correct?
> >> 
> >> Shouldn't this be "info stack", to match "info registers" and "info
> >> cpustats"?
> >
> > Since this is primarily about walking the guests stack frames and not
> > walking qemu internal data structures, I think it's probably OK to be
> > a dump-stack rather than an info subcommand.
> 
> Well, "info registers" is also about the guest's state and not QEMU
> internal state.  Arguably, so are "info pic", "info tlb", ...

When doing an 'info registers' you don't have to interpret or parse 
much guest state, you just print it, and it's guest state that's
held separately (similarly a separate piece of state for info pic, info
tlb etc).  You might check the register state a little when you
decide which bits to print or how to format them, but that's about as
far as it goes.  So each of the 'info's (or query for qmp) correspond
to one logical chunk of qemu and/or guest state.

Printing a stack is a much hairier thing, with knowledge of guest
stack layout and potentially some heuristics.

> We have a long-standing tradition of using "info" for "pure"
> information-retrieving commands.  I rather like that pattern.
> 
> Ultimately your choice as the HMP maintainer, of course.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
  2019-05-01  5:35 ` Suraj Jitindar Singh
                   ` (4 preceding siblings ...)
  (?)
@ 2019-06-21  0:51 ` Suraj Jitindar Singh
  2019-06-24  8:57   ` Markus Armbruster
  -1 siblings, 1 reply; 27+ messages in thread
From: Suraj Jitindar Singh @ 2019-06-21  0:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: david, qemu-ppc, dgilbert, armbru

On Wed, 2019-05-01 at 15:35 +1000, Suraj Jitindar Singh wrote:
> Add a monitor command "dump-stack" to be used to dump the stack for
> the
> current cpu.

To summarise the discussion which occured on this patch,

- It looks like it's ok to duplicate this functionality as it provides
an easier method to achieve this in the field and also for development.
- It's ok for this to remain as a separate command and to not place it
as a subcommand under info.

I'll rework based on the comments on 2/2 of the series and resend.

Thanks,
Suraj

> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  hmp-commands.hx   | 13 +++++++++++++
>  hmp.h             |  1 +
>  include/qom/cpu.h | 10 ++++++++++
>  monitor.c         | 12 ++++++++++++
>  qom/cpu.c         | 10 ++++++++++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 9b4035965c..965ccdea28 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -862,6 +862,19 @@ ETEXI
>      },
>  
>  STEXI
> +@item dump-stack
> +@findex dump-stack
> +dump stack of the cpu
> +ETEXI
> +    {
> +        .name           = "dump-stack",
> +        .args_type      = "",
> +        .params         = "",
> +        .help           = "dump stack",
> +        .cmd            = hmp_dumpstack,
> +    },
> +
> +STEXI
>  @item pmemsave @var{addr} @var{size} @var{file}
>  @findex pmemsave
>  save to disk physical memory dump starting at @var{addr} of size
> @var{size}.
> diff --git a/hmp.h b/hmp.h
> index 43617f2646..e6edf1215c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -51,6 +51,7 @@ void hmp_announce_self(Monitor *mon, const QDict
> *qdict);
>  void hmp_cpu(Monitor *mon, const QDict *qdict);
>  void hmp_memsave(Monitor *mon, const QDict *qdict);
>  void hmp_pmemsave(Monitor *mon, const QDict *qdict);
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_write(Monitor *mon, const QDict *qdict);
>  void hmp_ringbuf_read(Monitor *mon, const QDict *qdict);
>  void hmp_cont(Monitor *mon, const QDict *qdict);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 08abcbd3fe..f2e83e9918 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -181,6 +181,7 @@ typedef struct CPUClass {
>      int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
>                             uint8_t *buf, int len, bool is_write);
>      void (*dump_state)(CPUState *cpu, FILE *, int flags);
> +    void (*dump_stack)(CPUState *cpu, FILE *f);
>      GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
>      void (*dump_statistics)(CPUState *cpu, int flags);
>      int64_t (*get_arch_id)(CPUState *cpu);
> @@ -568,6 +569,15 @@ enum CPUDumpFlags {
>  void cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>  
>  /**
> + * cpu_dump_stack:
> + * @cpu: The CPU whose stack is to be dumped.
> + * @f: If non-null, dump to this stream, else to current print sink.
> + *
> + * Dumps CPU stack.
> + */
> +void cpu_dump_stack(CPUState *cpu, FILE *f);
> +
> +/**
>   * cpu_dump_statistics:
>   * @cpu: The CPU whose state is to be dumped.
>   * @flags: Flags what to dump.
> diff --git a/monitor.c b/monitor.c
> index 9b5f10b475..dbec2e4376 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1299,6 +1299,18 @@ static void hmp_info_registers(Monitor *mon,
> const QDict *qdict)
>      }
>  }
>  
> +void hmp_dumpstack(Monitor *mon, const QDict *qdict)
> +{
> +    CPUState *cs = mon_get_cpu();
> +
> +    if (!cs) {
> +        monitor_printf(mon, "No CPU available\n");
> +        return;
> +    }
> +
> +    cpu_dump_stack(cs, NULL);
> +}
> +
>  #ifdef CONFIG_TCG
>  static void hmp_info_jit(Monitor *mon, const QDict *qdict)
>  {
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 3c5493c96c..0dc10004f4 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -230,6 +230,16 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int
> flags)
>      }
>  }
>  
> +void cpu_dump_stack(CPUState *cpu, FILE *f)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->dump_stack) {
> +        cpu_synchronize_state(cpu);
> +        cc->dump_stack(cpu, f);
> +    }
> +}
> +
>  void cpu_dump_statistics(CPUState *cpu, int flags)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command
  2019-06-21  0:51 ` Suraj Jitindar Singh
@ 2019-06-24  8:57   ` Markus Armbruster
  0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2019-06-24  8:57 UTC (permalink / raw)
  To: Suraj Jitindar Singh; +Cc: qemu-ppc, qemu-devel, dgilbert, david

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> On Wed, 2019-05-01 at 15:35 +1000, Suraj Jitindar Singh wrote:
>> Add a monitor command "dump-stack" to be used to dump the stack for
>> the
>> current cpu.
>
> To summarise the discussion which occured on this patch,
>
> - It looks like it's ok to duplicate this functionality as it provides
> an easier method to achieve this in the field and also for development.

By "duplicate", do you mean "one copy in gdb, one copy in QEMU"?

The question "why can't we simply hot-add a gdb server and use gdb?" has
not been answered as far as I can tell.

If the answer is "we can, but we find duplicating the functionality in
QEMU more convenient", then the next question is "okay, but is the
convenience worth the additional code?".  For PPC, the additional code
is fairly small.  What about more ornery targets like x86_64?  This
hasn't been answered, either.

> - It's ok for this to remain as a separate command and to not place it
> as a subcommand under info.

I strongly prefer "info FOO" for "pure" information-retrieving
commands.  But I'm not the maintainer anymore.

> I'll rework based on the comments on 2/2 of the series and resend.


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2019-06-24  9:05 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-01  5:35 [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command Suraj Jitindar Singh
2019-05-01  5:35 ` Suraj Jitindar Singh
2019-05-01  5:35 ` [Qemu-devel] [PATCH 2/2] ppc: Add dump-stack implementation Suraj Jitindar Singh
2019-05-01  5:35   ` Suraj Jitindar Singh
2019-05-01  9:48   ` Alexey Kardashevskiy
2019-05-01  9:48     ` Alexey Kardashevskiy
2019-05-02  0:43     ` David Gibson
2019-05-02  0:43       ` David Gibson
2019-05-02  3:47       ` Alexey Kardashevskiy
2019-05-02  3:47         ` Alexey Kardashevskiy
2019-05-06  3:39         ` David Gibson
2019-05-02 13:59   ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2019-05-02 13:59     ` Greg Kurz
2019-05-07 11:24   ` [Qemu-devel] " Markus Armbruster
2019-05-01 10:44 ` [Qemu-devel] [PATCH 1/2] monitor: Add dump-stack command Dr. David Alan Gilbert
2019-05-01 10:44   ` Dr. David Alan Gilbert
2019-05-02  0:44 ` David Gibson
2019-05-02  0:44   ` David Gibson
2019-05-02  2:15   ` [Qemu-devel] [Qemu-ppc] " Alexey Kardashevskiy
2019-05-02  2:15     ` Alexey Kardashevskiy
2019-05-07 11:21     ` Markus Armbruster
2019-05-07 11:09 ` [Qemu-devel] " Markus Armbruster
2019-05-08 10:26   ` Dr. David Alan Gilbert
2019-05-08 13:10     ` Markus Armbruster
2019-05-08 13:15       ` Dr. David Alan Gilbert
2019-06-21  0:51 ` Suraj Jitindar Singh
2019-06-24  8:57   ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.