| Message ID | 1487255507-106654-10-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > > When GDB issues a "vCont", QEMU was not handling it correctly when > multiple VCPUs are active. > For vCont, for each thread (VCPU), it can be specified whether to > single step, continue or stop that thread. The default is to stop a > thread. <snip> > > +/** > + * gdb_handle_vcont - Parses and handles a vCont packet. > + * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if there is > + * a format error, 0 on success. > + */ > +static int gdb_handle_vcont(GDBState *s, const char *p) > +{ > + int res, idx, signal = 0; > + char cur_action; > + char *newstates; > + unsigned long tmp; > + CPUState *cpu; > +#ifdef CONFIG_USER_ONLY > + int max_cpus = 1; /* global variable max_cpus exists only in system mode */ > + > + CPU_FOREACH(cpu) { > + max_cpus = max_cpus <= cpu->cpu_index ? cpu->cpu_index + 1 : max_cpus; > + } > +#endif > + /* uninitialised CPUs stay 0 */ > + newstates = g_new0(char, max_cpus); > + > + /* mark valid CPUs with 1 */ > + CPU_FOREACH(cpu) { > + newstates[cpu->cpu_index] = 1; > + } > + > + /* > + * res keeps track of what error we are returning, with -ENOTSUP meaning > + * that the command is unknown or unsupported, thus returning an empty > + * packet, while -EINVAL and -ERANGE cause an E22 packet, due to invalid, > + * or incorrect parameters passed. > + */ > + res = 0; > + while (*p) { > + if (*p++ != ';') { > + res = -ENOTSUP; > + goto out; > + } > + > + cur_action = *p++; > + if (cur_action == 'C' || cur_action == 'S') { > + cur_action = tolower(cur_action); > + res = qemu_strtoul(p + 1, &p, 16, &tmp); > + if (res) { > + goto out; > + } > + signal = gdb_signal_to_target(tmp); > + } else if (cur_action != 'c' && cur_action != 's') { > + /* unknown/invalid/unsupported command */ > + res = -ENOTSUP; > + goto out; > + } > + /* thread specification. special values: (none), -1 = all; 0 = any */ > + if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) { > + if (*p == ':') { > + p += 3; > + } > + for (idx = 0; idx < max_cpus; idx++) { > + if (newstates[idx] == 1) { > + newstates[idx] = cur_action; > + } > + } > + } else if (*p == ':') { > + p++; > + res = qemu_strtoul(p, &p, 16, &tmp); > + if (res) { > + goto out; > + } > + idx = tmp; > + /* 0 means any thread, so we pick the first valid CPU */ > + if (!idx) { > + idx = cpu_index(first_cpu); > + } > + > + /* > + * If we are in user mode, the thread specified is actually a > + * thread id, and not an index. We need to find the actual > + * CPU first, and only then we can use its index. > + */ > + cpu = find_cpu(idx); > + /* invalid CPU/thread specified */ > + if (!idx || !cpu) { > + res = -EINVAL; > + goto out; > + } This fails on a packet like vCont;C04:0;c where we do find a cpu but it happens to have a internal cpu_index of 0. I'm sending a patch. -- Alex Bennée
On 2017-02-16 15:31, Paolo Bonzini wrote: > From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > > When GDB issues a "vCont", QEMU was not handling it correctly when > multiple VCPUs are active. > For vCont, for each thread (VCPU), it can be specified whether to > single step, continue or stop that thread. The default is to stop a > thread. > However, when (for example) "vCont;s:2" is issued, all VCPUs continue > to run, although all but VCPU nr 2 are to be stopped. > > This patch completely rewrites the vCont parsing code. > > Please note that this improvement only works in system emulation mode, > when in userspace emulation mode the old behaviour is preserved. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > Message-Id: <1487092068-16562-3-git-send-email-imbrenda@linux.vnet.ibm.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > gdbstub.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 162 insertions(+), 47 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 755a8e3..9911153 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -387,6 +387,60 @@ static inline void gdb_continue(GDBState *s) > #endif > } > > +/* > + * Resume execution, per CPU actions. For user-mode emulation it's > + * equivalent to gdb_continue. > + */ > +static int gdb_continue_partial(GDBState *s, char *newstates) > +{ > + CPUState *cpu; > + int res = 0; > +#ifdef CONFIG_USER_ONLY > + /* > + * This is not exactly accurate, but it's an improvement compared to the > + * previous situation, where only one CPU would be single-stepped. > + */ > + CPU_FOREACH(cpu) { > + if (newstates[cpu->cpu_index] == 's') { > + cpu_single_step(cpu, sstep_flags); > + } > + } > + s->running_state = 1; > +#else > + int flag = 0; > + > + if (!runstate_needs_reset()) { > + if (vm_prepare_start()) { > + return 0; > + } > + > + CPU_FOREACH(cpu) { > + switch (newstates[cpu->cpu_index]) { > + case 0: > + case 1: > + break; /* nothing to do here */ > + case 's': > + cpu_single_step(cpu, sstep_flags); > + cpu_resume(cpu); > + flag = 1; > + break; > + case 'c': > + cpu_resume(cpu); > + flag = 1; > + break; > + default: > + res = -1; > + break; > + } > + } > + } > + if (flag) { > + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); > + } > +#endif > + return res; > +} > + > static void put_buffer(GDBState *s, const uint8_t *buf, int len) > { > #ifdef CONFIG_USER_ONLY > @@ -785,6 +839,107 @@ static int is_query_packet(const char *p, const char *query, char separator) > (p[query_len] == '\0' || p[query_len] == separator); > } > > +/** > + * gdb_handle_vcont - Parses and handles a vCont packet. > + * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if there is > + * a format error, 0 on success. > + */ > +static int gdb_handle_vcont(GDBState *s, const char *p) > +{ > + int res, idx, signal = 0; > + char cur_action; > + char *newstates; > + unsigned long tmp; > + CPUState *cpu; > +#ifdef CONFIG_USER_ONLY > + int max_cpus = 1; /* global variable max_cpus exists only in system mode */ > + > + CPU_FOREACH(cpu) { > + max_cpus = max_cpus <= cpu->cpu_index ? cpu->cpu_index + 1 : max_cpus; > + } > +#endif > + /* uninitialised CPUs stay 0 */ > + newstates = g_new0(char, max_cpus); > + > + /* mark valid CPUs with 1 */ > + CPU_FOREACH(cpu) { > + newstates[cpu->cpu_index] = 1; > + } > + > + /* > + * res keeps track of what error we are returning, with -ENOTSUP meaning > + * that the command is unknown or unsupported, thus returning an empty > + * packet, while -EINVAL and -ERANGE cause an E22 packet, due to invalid, > + * or incorrect parameters passed. > + */ > + res = 0; > + while (*p) { > + if (*p++ != ';') { > + res = -ENOTSUP; > + goto out; > + } > + > + cur_action = *p++; > + if (cur_action == 'C' || cur_action == 'S') { > + cur_action = tolower(cur_action); > + res = qemu_strtoul(p + 1, &p, 16, &tmp); > + if (res) { > + goto out; > + } > + signal = gdb_signal_to_target(tmp); > + } else if (cur_action != 'c' && cur_action != 's') { > + /* unknown/invalid/unsupported command */ > + res = -ENOTSUP; > + goto out; > + } > + /* thread specification. special values: (none), -1 = all; 0 = any */ > + if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) { > + if (*p == ':') { > + p += 3; > + } > + for (idx = 0; idx < max_cpus; idx++) { > + if (newstates[idx] == 1) { > + newstates[idx] = cur_action; > + } > + } > + } else if (*p == ':') { > + p++; > + res = qemu_strtoul(p, &p, 16, &tmp); > + if (res) { > + goto out; > + } > + idx = tmp; > + /* 0 means any thread, so we pick the first valid CPU */ > + if (!idx) { > + idx = cpu_index(first_cpu); > + } > + > + /* > + * If we are in user mode, the thread specified is actually a > + * thread id, and not an index. We need to find the actual > + * CPU first, and only then we can use its index. > + */ > + cpu = find_cpu(idx); > + /* invalid CPU/thread specified */ > + if (!idx || !cpu) { > + res = -EINVAL; > + goto out; > + } > + /* only use if no previous match occourred */ > + if (newstates[cpu->cpu_index] == 1) { > + newstates[cpu->cpu_index] = cur_action; > + } > + } > + } > + s->signal = signal; > + gdb_continue_partial(s, newstates); > + > +out: > + g_free(newstates); > + > + return res; > +} > + > static int gdb_handle_packet(GDBState *s, const char *line_buf) > { > CPUState *cpu; > @@ -830,60 +985,20 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > return RS_IDLE; > case 'v': > if (strncmp(p, "Cont", 4) == 0) { > - int res_signal, res_thread; > - > p += 4; > if (*p == '?') { > put_packet(s, "vCont;c;C;s;S"); > break; > } > - res = 0; > - res_signal = 0; > - res_thread = 0; > - while (*p) { > - int action, signal; > - > - if (*p++ != ';') { > - res = 0; > - break; > - } > - action = *p++; > - signal = 0; > - if (action == 'C' || action == 'S') { > - signal = gdb_signal_to_target(strtoul(p, (char **)&p, 16)); > - if (signal == -1) { > - signal = 0; > - } > - } else if (action != 'c' && action != 's') { > - res = 0; > - break; > - } > - thread = 0; > - if (*p == ':') { > - thread = strtoull(p+1, (char **)&p, 16); > - } > - action = tolower(action); > - if (res == 0 || (res == 'c' && action == 's')) { > - res = action; > - res_signal = signal; > - res_thread = thread; > - } > - } > + > + res = gdb_handle_vcont(s, p); > + > if (res) { > - if (res_thread != -1 && res_thread != 0) { > - cpu = find_cpu(res_thread); > - if (cpu == NULL) { > - put_packet(s, "E22"); > - break; > - } > - s->c_cpu = cpu; > - } > - if (res == 's') { > - cpu_single_step(s->c_cpu, sstep_flags); > + if ((res == -EINVAL) || (res == -ERANGE)) { > + put_packet(s, "E22"); > + break; > } > - s->signal = res_signal; > - gdb_continue(s); > - return RS_IDLE; > + goto unknown_command; > } > break; > } else { > Seems like no one is doing guest debugging with kvm on x86 except me, and I'm only doing it too infrequently now: This one broke that use case for SMP guests long ago. How was it tested? To reproduce the bug: set up an x86-64 guest kernel with > 1 core, break on some prominent syscall entry (e.g. sys_execve), continue the guest on hit and it will quickly lock up, even after disabling the breakpoint again. Kernel version doesn't matter (was my first guess), gdb is 7.7.50.20140604-cvs (OpenSUSE) here. Jan
On 2018-02-17 09:56, Jan Kiszka wrote: > On 2017-02-16 15:31, Paolo Bonzini wrote: >> From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> >> >> When GDB issues a "vCont", QEMU was not handling it correctly when >> multiple VCPUs are active. >> For vCont, for each thread (VCPU), it can be specified whether to >> single step, continue or stop that thread. The default is to stop a >> thread. >> However, when (for example) "vCont;s:2" is issued, all VCPUs continue >> to run, although all but VCPU nr 2 are to be stopped. >> >> This patch completely rewrites the vCont parsing code. >> >> Please note that this improvement only works in system emulation mode, >> when in userspace emulation mode the old behaviour is preserved. >> >> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> >> Message-Id: <1487092068-16562-3-git-send-email-imbrenda@linux.vnet.ibm.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> gdbstub.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 162 insertions(+), 47 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 755a8e3..9911153 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -387,6 +387,60 @@ static inline void gdb_continue(GDBState *s) >> #endif >> } >> >> +/* >> + * Resume execution, per CPU actions. For user-mode emulation it's >> + * equivalent to gdb_continue. >> + */ >> +static int gdb_continue_partial(GDBState *s, char *newstates) >> +{ >> + CPUState *cpu; >> + int res = 0; >> +#ifdef CONFIG_USER_ONLY >> + /* >> + * This is not exactly accurate, but it's an improvement compared to the >> + * previous situation, where only one CPU would be single-stepped. >> + */ >> + CPU_FOREACH(cpu) { >> + if (newstates[cpu->cpu_index] == 's') { >> + cpu_single_step(cpu, sstep_flags); >> + } >> + } >> + s->running_state = 1; >> +#else >> + int flag = 0; >> + >> + if (!runstate_needs_reset()) { >> + if (vm_prepare_start()) { >> + return 0; >> + } >> + >> + CPU_FOREACH(cpu) { >> + switch (newstates[cpu->cpu_index]) { >> + case 0: >> + case 1: >> + break; /* nothing to do here */ >> + case 's': >> + cpu_single_step(cpu, sstep_flags); >> + cpu_resume(cpu); >> + flag = 1; >> + break; >> + case 'c': >> + cpu_resume(cpu); >> + flag = 1; >> + break; >> + default: >> + res = -1; >> + break; >> + } >> + } >> + } >> + if (flag) { >> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >> + } >> +#endif >> + return res; >> +} >> + >> static void put_buffer(GDBState *s, const uint8_t *buf, int len) >> { >> #ifdef CONFIG_USER_ONLY >> @@ -785,6 +839,107 @@ static int is_query_packet(const char *p, const char *query, char separator) >> (p[query_len] == '\0' || p[query_len] == separator); >> } >> >> +/** >> + * gdb_handle_vcont - Parses and handles a vCont packet. >> + * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if there is >> + * a format error, 0 on success. >> + */ >> +static int gdb_handle_vcont(GDBState *s, const char *p) >> +{ >> + int res, idx, signal = 0; >> + char cur_action; >> + char *newstates; >> + unsigned long tmp; >> + CPUState *cpu; >> +#ifdef CONFIG_USER_ONLY >> + int max_cpus = 1; /* global variable max_cpus exists only in system mode */ >> + >> + CPU_FOREACH(cpu) { >> + max_cpus = max_cpus <= cpu->cpu_index ? cpu->cpu_index + 1 : max_cpus; >> + } >> +#endif >> + /* uninitialised CPUs stay 0 */ >> + newstates = g_new0(char, max_cpus); >> + >> + /* mark valid CPUs with 1 */ >> + CPU_FOREACH(cpu) { >> + newstates[cpu->cpu_index] = 1; >> + } >> + >> + /* >> + * res keeps track of what error we are returning, with -ENOTSUP meaning >> + * that the command is unknown or unsupported, thus returning an empty >> + * packet, while -EINVAL and -ERANGE cause an E22 packet, due to invalid, >> + * or incorrect parameters passed. >> + */ >> + res = 0; >> + while (*p) { >> + if (*p++ != ';') { >> + res = -ENOTSUP; >> + goto out; >> + } >> + >> + cur_action = *p++; >> + if (cur_action == 'C' || cur_action == 'S') { >> + cur_action = tolower(cur_action); >> + res = qemu_strtoul(p + 1, &p, 16, &tmp); >> + if (res) { >> + goto out; >> + } >> + signal = gdb_signal_to_target(tmp); >> + } else if (cur_action != 'c' && cur_action != 's') { >> + /* unknown/invalid/unsupported command */ >> + res = -ENOTSUP; >> + goto out; >> + } >> + /* thread specification. special values: (none), -1 = all; 0 = any */ >> + if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) { >> + if (*p == ':') { >> + p += 3; >> + } >> + for (idx = 0; idx < max_cpus; idx++) { >> + if (newstates[idx] == 1) { >> + newstates[idx] = cur_action; >> + } >> + } >> + } else if (*p == ':') { >> + p++; >> + res = qemu_strtoul(p, &p, 16, &tmp); >> + if (res) { >> + goto out; >> + } >> + idx = tmp; >> + /* 0 means any thread, so we pick the first valid CPU */ >> + if (!idx) { >> + idx = cpu_index(first_cpu); >> + } >> + >> + /* >> + * If we are in user mode, the thread specified is actually a >> + * thread id, and not an index. We need to find the actual >> + * CPU first, and only then we can use its index. >> + */ >> + cpu = find_cpu(idx); >> + /* invalid CPU/thread specified */ >> + if (!idx || !cpu) { >> + res = -EINVAL; >> + goto out; >> + } >> + /* only use if no previous match occourred */ >> + if (newstates[cpu->cpu_index] == 1) { >> + newstates[cpu->cpu_index] = cur_action; >> + } >> + } >> + } >> + s->signal = signal; >> + gdb_continue_partial(s, newstates); >> + >> +out: >> + g_free(newstates); >> + >> + return res; >> +} >> + >> static int gdb_handle_packet(GDBState *s, const char *line_buf) >> { >> CPUState *cpu; >> @@ -830,60 +985,20 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> return RS_IDLE; >> case 'v': >> if (strncmp(p, "Cont", 4) == 0) { >> - int res_signal, res_thread; >> - >> p += 4; >> if (*p == '?') { >> put_packet(s, "vCont;c;C;s;S"); >> break; >> } >> - res = 0; >> - res_signal = 0; >> - res_thread = 0; >> - while (*p) { >> - int action, signal; >> - >> - if (*p++ != ';') { >> - res = 0; >> - break; >> - } >> - action = *p++; >> - signal = 0; >> - if (action == 'C' || action == 'S') { >> - signal = gdb_signal_to_target(strtoul(p, (char **)&p, 16)); >> - if (signal == -1) { >> - signal = 0; >> - } >> - } else if (action != 'c' && action != 's') { >> - res = 0; >> - break; >> - } >> - thread = 0; >> - if (*p == ':') { >> - thread = strtoull(p+1, (char **)&p, 16); >> - } >> - action = tolower(action); >> - if (res == 0 || (res == 'c' && action == 's')) { >> - res = action; >> - res_signal = signal; >> - res_thread = thread; >> - } >> - } >> + >> + res = gdb_handle_vcont(s, p); >> + >> if (res) { >> - if (res_thread != -1 && res_thread != 0) { >> - cpu = find_cpu(res_thread); >> - if (cpu == NULL) { >> - put_packet(s, "E22"); >> - break; >> - } >> - s->c_cpu = cpu; >> - } >> - if (res == 's') { >> - cpu_single_step(s->c_cpu, sstep_flags); >> + if ((res == -EINVAL) || (res == -ERANGE)) { >> + put_packet(s, "E22"); >> + break; >> } >> - s->signal = res_signal; >> - gdb_continue(s); >> - return RS_IDLE; >> + goto unknown_command; >> } >> break; >> } else { >> > > Seems like no one is doing guest debugging with kvm on x86 except me, > and I'm only doing it too infrequently now: This one broke that use case > for SMP guests long ago. How was it tested? > > To reproduce the bug: set up an x86-64 guest kernel with > 1 core, break > on some prominent syscall entry (e.g. sys_execve), continue the guest on > hit and it will quickly lock up, even after disabling the breakpoint > again. Kernel version doesn't matter (was my first guess), gdb is > 7.7.50.20140604-cvs (OpenSUSE) here. > I case it helps: GNU gdb (GDB) 7.7.50.20140604-cvs Copyright (C) 2014 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-unknown-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from vmlinux...done. (gdb) tar rem :1234 Remote debugging using :1234 0xffffffff81719d51 in native_safe_halt () at ../arch/x86/include/asm/irqflags.h:54 warning: Source file is more recent than executable. 54 asm volatile("hlt": : :"memory"); (gdb) c Continuing. ^C Program received signal SIGINT, Interrupt. 0xffffffff81719d51 in native_safe_halt () at ../arch/x86/include/asm/irqflags.h:54 54 asm volatile("hlt": : :"memory"); (gdb) set debug remote 1 (gdb) b sys_execve Sending packet: $Hg4#e3...Ack Packet received: OK Sending packet: $g#67...Ack Packet received: c023ef390088ffff000000000000000044be080000c9ffff00f70000000000000100000000000000c023ef390088ffff0000000000000000c8be080000c9ffff0100000000000000010000000000000010be080000c9ffff089a0082ffffffffc023ef390088ffffc023ef390088ffff0300000000000000c023ef390088ffff519d7181ffffffff0602000010000000180000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f03000000000000000000000000000000000000000000000000000000000000000000000000000000000000ff000000ffffffffffffffffffffffff00ffffff70617273655f616e645f657865637574414d4500000000000000000000000000000000000000000031000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000801f0000 Sending packet: $mffffffff811a3800,40#f3...Ack Packet received: 5b5d415ce9aa65ebffe8a2e265004883ec10488974240848891424e84c620000488b14244889c7488b742408e8cdfdffff48984883c410c3e873e26500554489 Sending packet: $mffffffff811a3809,1#c9...Ack Packet received: e8 Breakpoint 1 at 0xffffffff811a3809: file ../fs/exec.c, line 1923. (gdb) c Continuing. Sending packet: $Z0,ffffffff811a3809,1#12...Ack Packet received: OK Packet Z0 (software-breakpoint) is supported Sending packet: $vCont;c#a8...Ack Packet received: T05thread:01; Sending packet: $g#67...Ack Packet received: 3b0000000000000058bfc00000c9ffff60f1d015ff7f0000e0f073000000000090b673000000000090436b00000000003b0000000000000030bfc00000c9ffff40bd73000000000060b37300000000000000000000000000000000000000000000a852370088ffff00000000000000000000000000000000000000000000000009381a81ffffffff0202000010000000180000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f03000000000000000000000000000000000000000000000000000000000000ff000000000000000000000000ffffff00000000000000000000000000000000ff000000000000000000000000ffffff000000000000000000ff00ffffffffff000000000000000031000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000801f0000 Sending packet: $Hg4#e3...Ack Packet received: OK Sending packet: $g#67...Ack Packet received: c023ef390088ffff000000000000000001000000000000004013da3f0088ffff0100000000000000c023ef390088ffff0000000000000000c8be080000c9ffff0100000000000000010000000000000080bd080000c9ffff88af0383ffffffffc023ef390088ffffc023ef390088ffff0300000000000000c023ef390088ffff519d7181ffffffff0602000010000000180000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f0300000001000000000000000000000000000000000000000000000000000000000000000000000000ffffffffffff000000000000000000000000000000000000000000000000000000000000000000000000ff00000000000000ff00000074726163743d2f646275732d7666732d00000000008064400000000000000000fefdfdfdfdfded3f0000000000000000000000000000f03f0000000000000000000000000000944000000000000000000000000000008e400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a01f0000 Sending packet: $z0,ffffffff811a3809,1#32...Ack Packet received: OK Sending packet: $Hg1#e0...Ack Packet received: OK Sending packet: $mffffc90000c0bf30,8#83...Ack Packet received: ae180081ffffffff Sending packet: $mffffc90000c0bf30,8#83...Ack Packet received: ae180081ffffffff Breakpoint 1, SyS_execve (filename=7029648, argv=7583376, envp=7598304) at ../fs/exec.c:1923 warning: Source file is more recent than executable. (gdb) c Continuing. Sending packet: $vCont;s:1#23...Ack Packet received: T05thread:01; Sending packet: $g#67...Ack Packet received: 0020000000c9ffff46000000000000000c3fc03f0088ffff8287ac5600000000f1a60381fffffffff1a60381ffffffff000000006622ab56383fc03f0088ffff01000000000000000100000000000000d83ec03f0088ffff88af0383ffffffff18bec13f0088ffff98bec13f0088ffff18bfc13f0088ffff0a180000000000004da70381ffffffff4600000010000000180000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f03000000000000000000000000000000000000000000000000000000000000ff000000000000000000000000ffffff00000000000000000000000000000000ff000000000000000000000000ffffff000000000000000000ff00ffffffffff000000000000000031000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000801f0000 Sending packet: $Z0,ffffffff811a3809,1#12...Ack Packet received: OK Sending packet: $vCont;c#a8...Ack Packet received: T05thread:01; Sending packet: $g#67...Ack Packet received: 3b0000000000000058bfc00000c9ffff60f1d015ff7f0000e0f073000000000090b673000000000090436b00000000003b0000000000000030bfc00000c9ffff40bd73000000000060b37300000000000000000000000000000000000000000000a852370088ffff00000000000000000000000000000000000000000000000009381a81ffffffff0203000010000000180000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f03000000000000000000000000000000000000000000000000000000000000ff000000000000000000000000ffffff00000000000000000000000000000000ff000000000000000000000000ffffff000000000000000000ff00ffffffffff000000000000000031000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000801f0000 Sending packet: $Hg4#e3...Ack Packet received: OK Sending packet: $g#67...Ack Packet received: c023ef390088ffff000000000000000001000000000000004013da3f0088ffff0100000000000000c023ef390088ffff0000000000000000c8be080000c9ffff0100000000000000010000000000000080bd080000c9ffff88af0383ffffffffc023ef390088ffffc023ef390088ffff0300000000000000c023ef390088ffff519d7181ffffffff0602000010000000180000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f0300000001000000000000000000000000000000000000000000000000000000000000000000000000ffffffffffff000000000000000000000000000000000000000000000000000000000000000000000000ff00000000000000ff00000074726163743d2f646275732d7666732d00000000008064400000000000000000fefdfdfdfdfded3f0000000000000000000000000000f03f0000000000000000000000000000944000000000000000000000000000008e400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a01f0000 Sending packet: $z0,ffffffff811a3809,1#32...Ack Packet received: OK Sending packet: $Hg1#e0...Ack Packet received: OK Sending packet: $mffffc90000c0bf30,8#83...Ack Packet received: ae180081ffffffff Sending packet: $mffffc90000c0bf30,8#83...Ack Packet received: ae180081ffffffff Breakpoint 1, SyS_execve (filename=7029648, argv=7583376, envp=7598304) at ../fs/exec.c:1923 (gdb) c Continuing. Sending packet: $vCont;s:1#23...Ack ...and now the guest is dead. I can still interrupt it, but it's otherwise not working properly. Jan
Jan Kiszka <jan.kiszka@web.de> writes: > On 2018-02-17 09:56, Jan Kiszka wrote: >> On 2017-02-16 15:31, Paolo Bonzini wrote: >>> From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> >>> >>> When GDB issues a "vCont", QEMU was not handling it correctly when >>> multiple VCPUs are active. >>> For vCont, for each thread (VCPU), it can be specified whether to >>> single step, continue or stop that thread. The default is to stop a >>> thread. >>> However, when (for example) "vCont;s:2" is issued, all VCPUs continue >>> to run, although all but VCPU nr 2 are to be stopped. >>> >>> This patch completely rewrites the vCont parsing code. >>> >>> Please note that this improvement only works in system emulation mode, >>> when in userspace emulation mode the old behaviour is preserved. >>> >>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> >>> Message-Id: <1487092068-16562-3-git-send-email-imbrenda@linux.vnet.ibm.com> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> gdbstub.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++-------------- >>> 1 file changed, 162 insertions(+), 47 deletions(-) >>> <snip> >> >> Seems like no one is doing guest debugging with kvm on x86 except me, >> and I'm only doing it too infrequently now: This one broke that use case >> for SMP guests long ago. How was it tested? >> >> To reproduce the bug: set up an x86-64 guest kernel with > 1 core, break >> on some prominent syscall entry (e.g. sys_execve), continue the guest on >> hit and it will quickly lock up, even after disabling the breakpoint >> again. Kernel version doesn't matter (was my first guess), gdb is >> 7.7.50.20140604-cvs (OpenSUSE) here. I thought I fixed this with 5a6a1ad181c658b810041d852b290ac836965aca FWIW I do periodically test ARM TCG and KVM guest debug using: tests/guest-debug/test-gdbstub.py But we are missing a nice integration to get an appropriate guest image to automate this process. If we can fix that we should be able to turn on the test as part of make check. -- Alex Bennée
On 2018-02-17 14:27, Alex Bennée wrote: > > Jan Kiszka <jan.kiszka@web.de> writes: > >> On 2018-02-17 09:56, Jan Kiszka wrote: >>> On 2017-02-16 15:31, Paolo Bonzini wrote: >>>> From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> >>>> >>>> When GDB issues a "vCont", QEMU was not handling it correctly when >>>> multiple VCPUs are active. >>>> For vCont, for each thread (VCPU), it can be specified whether to >>>> single step, continue or stop that thread. The default is to stop a >>>> thread. >>>> However, when (for example) "vCont;s:2" is issued, all VCPUs continue >>>> to run, although all but VCPU nr 2 are to be stopped. >>>> >>>> This patch completely rewrites the vCont parsing code. >>>> >>>> Please note that this improvement only works in system emulation mode, >>>> when in userspace emulation mode the old behaviour is preserved. >>>> >>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> >>>> Message-Id: <1487092068-16562-3-git-send-email-imbrenda@linux.vnet.ibm.com> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>>> --- >>>> gdbstub.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++++-------------- >>>> 1 file changed, 162 insertions(+), 47 deletions(-) >>>> > <snip> >>> >>> Seems like no one is doing guest debugging with kvm on x86 except me, >>> and I'm only doing it too infrequently now: This one broke that use case >>> for SMP guests long ago. How was it tested? >>> >>> To reproduce the bug: set up an x86-64 guest kernel with > 1 core, break >>> on some prominent syscall entry (e.g. sys_execve), continue the guest on >>> hit and it will quickly lock up, even after disabling the breakpoint >>> again. Kernel version doesn't matter (was my first guess), gdb is >>> 7.7.50.20140604-cvs (OpenSUSE) here. > > I thought I fixed this with 5a6a1ad181c658b810041d852b290ac836965aca > > FWIW I do periodically test ARM TCG and KVM guest debug using: > > tests/guest-debug/test-gdbstub.py > > But we are missing a nice integration to get an appropriate guest image > to automate this process. If we can fix that we should be able to turn > on the test as part of make check. > If that test above is extended with more interesting setups, that should be enough. E.g., you can reproduce this issue by running qemu with -smp 4 and the following test modifications. diff --git a/tests/guest-debug/test-gdbstub.py b/tests/guest-debug/test-gdbstub.py index 31ba6c943a..a55782fa9a 100644 --- a/tests/guest-debug/test-gdbstub.py +++ b/tests/guest-debug/test-gdbstub.py @@ -15,6 +15,7 @@ def report(cond, msg): print ("PASS: %s" % (msg)) else: print ("FAIL: %s" % (msg)) + global failcount failcount += 1 @@ -33,6 +34,7 @@ def check_break(sym_name): bp = gdb.Breakpoint(sym_name) gdb.execute("c") + gdb.execute("c 100") # hopefully we came back end_pc = gdb.parse_and_eval('$pc') @@ -138,12 +140,12 @@ def run_test(): # Can't set this up until we are in the kernel proper # if we make it to run_init_process we've over-run and # one of the tests failed - print ("Setup catch-all for run_init_process") - cbp = CatchBreakpoint("run_init_process") - cpb2 = CatchBreakpoint("try_to_run_init_process") + #print ("Setup catch-all for run_init_process") + #cbp = CatchBreakpoint("run_init_process") + #cpb2 = CatchBreakpoint("try_to_run_init_process") print ("Checking Normal breakpoint works") - break_ok = check_break("wait_for_completion") + break_ok = check_break("SyS_execve") report(break_ok, "break @ wait_for_completion") print ("Checking watchpoint works") With -smp 1, check_break succeeds. Jan
On Sat, 17 Feb 2018 10:07:38 +0100 Jan Kiszka <jan.kiszka@web.de> wrote: [...] > > Seems like no one is doing guest debugging with kvm on x86 except > > me, and I'm only doing it too infrequently now: This one broke that > > use case for SMP guests long ago. How was it tested? > > > > To reproduce the bug: set up an x86-64 guest kernel with > 1 core, > > break on some prominent syscall entry (e.g. sys_execve), continue > > the guest on hit and it will quickly lock up, even after disabling > > the breakpoint again. Kernel version doesn't matter (was my first > > guess), gdb is 7.7.50.20140604-cvs (OpenSUSE) here. [...] > Sending packet: $Hg1#e0...Ack > Packet received: OK > Sending packet: $mffffc90000c0bf30,8#83...Ack > Packet received: ae180081ffffffff > Sending packet: $mffffc90000c0bf30,8#83...Ack > Packet received: ae180081ffffffff > Breakpoint 1, SyS_execve (filename=7029648, argv=7583376, > envp=7598304) at ../fs/exec.c:1923 (gdb) c > Continuing. > Sending packet: $vCont;s:1#23...Ack > > ...and now the guest is dead. I can still interrupt it, but it's > otherwise not working properly. I tried, but I could not reproduce this bug neither on s390x nor on amd64. in both cases I used recent enough versions of qemu so that they have the patch you mentioned, and qemu was started with several cpus (I tried 64 on s390x, and 4 on amd64). in particular on amd64: host: 4.4.0-112-generic (ubuntu 16.04) QEMU version 2.10.0 (vanilla from git) gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1 I also used the gdb script you provided but everything worked both with -smp 1 and with -smp 4 the only issue I had was that I had to disable kaslr in the guest to be able to do anything, but that does not seem to be the problem you have. my primary suspicion at this point would be an issue in KVM, and not in qemu. have you tried running it without KVM? what is the version of qemu and kernel in the host? Claudio
On 2018-02-19 19:15, Claudio Imbrenda wrote: > On Sat, 17 Feb 2018 10:07:38 +0100 > Jan Kiszka <jan.kiszka@web.de> wrote: > > [...] > >>> Seems like no one is doing guest debugging with kvm on x86 except >>> me, and I'm only doing it too infrequently now: This one broke that >>> use case for SMP guests long ago. How was it tested? >>> >>> To reproduce the bug: set up an x86-64 guest kernel with > 1 core, >>> break on some prominent syscall entry (e.g. sys_execve), continue >>> the guest on hit and it will quickly lock up, even after disabling >>> the breakpoint again. Kernel version doesn't matter (was my first >>> guess), gdb is 7.7.50.20140604-cvs (OpenSUSE) here. > > [...] > >> Sending packet: $Hg1#e0...Ack >> Packet received: OK >> Sending packet: $mffffc90000c0bf30,8#83...Ack >> Packet received: ae180081ffffffff >> Sending packet: $mffffc90000c0bf30,8#83...Ack >> Packet received: ae180081ffffffff >> Breakpoint 1, SyS_execve (filename=7029648, argv=7583376, >> envp=7598304) at ../fs/exec.c:1923 (gdb) c >> Continuing. >> Sending packet: $vCont;s:1#23...Ack >> >> ...and now the guest is dead. I can still interrupt it, but it's >> otherwise not working properly. > > I tried, but I could not reproduce this bug neither on s390x nor on > amd64. in both cases I used recent enough versions of qemu so that > they have the patch you mentioned, and qemu was started with several > cpus (I tried 64 on s390x, and 4 on amd64). > > in particular on amd64: > host: 4.4.0-112-generic (ubuntu 16.04) > QEMU version 2.10.0 (vanilla from git) > gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1 > > I also used the gdb script you provided but everything worked both with > -smp 1 and with -smp 4 > > the only issue I had was that I had to disable kaslr in the guest to be > able to do anything, but that does not seem to be the problem you have. > > my primary suspicion at this point would be an issue in KVM, and not > in qemu. have you tried running it without KVM? what is the version of > qemu and kernel in the host? Yes, debugging works without KVM. But that is a completely different setup for various reasons (no parallelism, no guest-visible modifications by soft-breakpoints and continuations). But I would not exclude that the issue is caused by KVM, and your patch just surfaced it. Jan
diff --git a/gdbstub.c b/gdbstub.c index 755a8e3..9911153 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -387,6 +387,60 @@ static inline void gdb_continue(GDBState *s) #endif } +/* + * Resume execution, per CPU actions. For user-mode emulation it's + * equivalent to gdb_continue. + */ +static int gdb_continue_partial(GDBState *s, char *newstates) +{ + CPUState *cpu; + int res = 0; +#ifdef CONFIG_USER_ONLY + /* + * This is not exactly accurate, but it's an improvement compared to the + * previous situation, where only one CPU would be single-stepped. + */ + CPU_FOREACH(cpu) { + if (newstates[cpu->cpu_index] == 's') { + cpu_single_step(cpu, sstep_flags); + } + } + s->running_state = 1; +#else + int flag = 0; + + if (!runstate_needs_reset()) { + if (vm_prepare_start()) { + return 0; + } + + CPU_FOREACH(cpu) { + switch (newstates[cpu->cpu_index]) { + case 0: + case 1: + break; /* nothing to do here */ + case 's': + cpu_single_step(cpu, sstep_flags); + cpu_resume(cpu); + flag = 1; + break; + case 'c': + cpu_resume(cpu); + flag = 1; + break; + default: + res = -1; + break; + } + } + } + if (flag) { + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); + } +#endif + return res; +} + static void put_buffer(GDBState *s, const uint8_t *buf, int len) { #ifdef CONFIG_USER_ONLY @@ -785,6 +839,107 @@ static int is_query_packet(const char *p, const char *query, char separator) (p[query_len] == '\0' || p[query_len] == separator); } +/** + * gdb_handle_vcont - Parses and handles a vCont packet. + * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if there is + * a format error, 0 on success. + */ +static int gdb_handle_vcont(GDBState *s, const char *p) +{ + int res, idx, signal = 0; + char cur_action; + char *newstates; + unsigned long tmp; + CPUState *cpu; +#ifdef CONFIG_USER_ONLY + int max_cpus = 1; /* global variable max_cpus exists only in system mode */ + + CPU_FOREACH(cpu) { + max_cpus = max_cpus <= cpu->cpu_index ? cpu->cpu_index + 1 : max_cpus; + } +#endif + /* uninitialised CPUs stay 0 */ + newstates = g_new0(char, max_cpus); + + /* mark valid CPUs with 1 */ + CPU_FOREACH(cpu) { + newstates[cpu->cpu_index] = 1; + } + + /* + * res keeps track of what error we are returning, with -ENOTSUP meaning + * that the command is unknown or unsupported, thus returning an empty + * packet, while -EINVAL and -ERANGE cause an E22 packet, due to invalid, + * or incorrect parameters passed. + */ + res = 0; + while (*p) { + if (*p++ != ';') { + res = -ENOTSUP; + goto out; + } + + cur_action = *p++; + if (cur_action == 'C' || cur_action == 'S') { + cur_action = tolower(cur_action); + res = qemu_strtoul(p + 1, &p, 16, &tmp); + if (res) { + goto out; + } + signal = gdb_signal_to_target(tmp); + } else if (cur_action != 'c' && cur_action != 's') { + /* unknown/invalid/unsupported command */ + res = -ENOTSUP; + goto out; + } + /* thread specification. special values: (none), -1 = all; 0 = any */ + if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) { + if (*p == ':') { + p += 3; + } + for (idx = 0; idx < max_cpus; idx++) { + if (newstates[idx] == 1) { + newstates[idx] = cur_action; + } + } + } else if (*p == ':') { + p++; + res = qemu_strtoul(p, &p, 16, &tmp); + if (res) { + goto out; + } + idx = tmp; + /* 0 means any thread, so we pick the first valid CPU */ + if (!idx) { + idx = cpu_index(first_cpu); + } + + /* + * If we are in user mode, the thread specified is actually a + * thread id, and not an index. We need to find the actual + * CPU first, and only then we can use its index. + */ + cpu = find_cpu(idx); + /* invalid CPU/thread specified */ + if (!idx || !cpu) { + res = -EINVAL; + goto out; + } + /* only use if no previous match occourred */ + if (newstates[cpu->cpu_index] == 1) { + newstates[cpu->cpu_index] = cur_action; + } + } + } + s->signal = signal; + gdb_continue_partial(s, newstates); + +out: + g_free(newstates); + + return res; +} + static int gdb_handle_packet(GDBState *s, const char *line_buf) { CPUState *cpu; @@ -830,60 +985,20 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) return RS_IDLE; case 'v': if (strncmp(p, "Cont", 4) == 0) { - int res_signal, res_thread; - p += 4; if (*p == '?') { put_packet(s, "vCont;c;C;s;S"); break; } - res = 0; - res_signal = 0; - res_thread = 0; - while (*p) { - int action, signal; - - if (*p++ != ';') { - res = 0; - break; - } - action = *p++; - signal = 0; - if (action == 'C' || action == 'S') { - signal = gdb_signal_to_target(strtoul(p, (char **)&p, 16)); - if (signal == -1) { - signal = 0; - } - } else if (action != 'c' && action != 's') { - res = 0; - break; - } - thread = 0; - if (*p == ':') { - thread = strtoull(p+1, (char **)&p, 16); - } - action = tolower(action); - if (res == 0 || (res == 'c' && action == 's')) { - res = action; - res_signal = signal; - res_thread = thread; - } - } + + res = gdb_handle_vcont(s, p); + if (res) { - if (res_thread != -1 && res_thread != 0) { - cpu = find_cpu(res_thread); - if (cpu == NULL) { - put_packet(s, "E22"); - break; - } - s->c_cpu = cpu; - } - if (res == 's') { - cpu_single_step(s->c_cpu, sstep_flags); + if ((res == -EINVAL) || (res == -ERANGE)) { + put_packet(s, "E22"); + break; } - s->signal = res_signal; - gdb_continue(s); - return RS_IDLE; + goto unknown_command; } break; } else {