| Message ID | 20190501053522.10967-1-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] monitor: Add dump-stack command | expand |
* 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
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);
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); >
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);
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?
* 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
"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.
* 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
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);
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.
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);
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(+)