| Message ID | 1476100224-19760-3-git-send-email-imbrenda@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/10/2016 13:50, Claudio Imbrenda wrote: > + /* > + * XXX vm_start also calls qemu_vmstop_requested(&requested); here, is > + * it actually important? it's static in vl.c > + */ Yes, it is, :) and so is qapi_event_send_resume (which is automatically generated in qapi-event.c). You can make qemu_vmstop_requested non-static, but the right thing to do is: 1) move vm_start to cpus.c 2) move qemu_clock_enable from resume_all_cpus to vm_start 3) extract vm_start_noresume out of vm_start and call it here. Another suggestion is to extract the whole handling of vCont into a separate function; not just gdb_continue_partial. And because the logic for actions is quite complex: > + if (def == 0) { > + for (cx = 0; scpus && scpus[cx]; cx++) { > + cpu_single_step(scpus[cx], sstep_flags); > + cpu_resume(scpus[cx]); > + } > + for (cx = 0; ccpus && ccpus[cx]; cx++) { > + cpu_resume(ccpus[cx]); > + } > + } else if (def == 'c' || def == 'C') { > + for (cx = 0; scpus && scpus[cx]; cx++) { > + cpu_single_step(scpus[cx], sstep_flags); > + } > + CPU_FOREACH(cpu) { > + cpu_resume(cpu); > + } > + } else if (def == 's' || def == 'S') { > + CPU_FOREACH(cpu) { > + cpu_single_step(cpu, sstep_flags); > + } > + for (cx = 0; ccpus && ccpus[cx]; cx++) { > + cpu_single_step(cpu, 0); > + } > + CPU_FOREACH(cpu) { > + cpu_resume(cpu); > + } it looks better with a couple helper functions: if (def == 's' || def == 'S') { /* single-step all CPUs but ccpus */ gdb_single_step_cpus(NULL, sstep_flags); gdb_single_step_cpus(ccpus, 0); resume_all_cpus(); } else { gdb_single_step_cpus(scpus, sstep_flags); if (def == 0) { gdb_resume_cpus(scpus); gdb_resume_cpus(ccpus); } else { resume_all_cpus(); } } Also: > + if (*p == ':') { > + if (broadcast_action) { > + res = -22; > + break; > + } > + p++; > + if ((p[0] == '-') && (p[1] == '1')) { > + if (broadcast_action || scnt || ccnt) { You can't get here with broadcast_action != 0, it's checked above. You're also not implementing this on user-mode emulation, but you're not documenting this anywhere. Perhaps user-mode emulation should not support vCont at all unless gdbstub is moved to a separate thread altogether? Finally, some more stylistic choices: 1) use g_new instead of g_malloc. 2) the handling of res is very messy. What do -1 and -22 mean? 3) Please do error checking on qemu_strtoul Thanks, Paolo > + cpu_enable_ticks(); > + runstate_set(RUN_STATE_RUNNING); > + vm_state_notify(1, RUN_STATE_RUNNING); > + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); > + if (def == 0) { > + for (cx = 0; scpus && scpus[cx]; cx++) { > + cpu_single_step(scpus[cx], sstep_flags); > + cpu_resume(scpus[cx]); > + } > + for (cx = 0; ccpus && ccpus[cx]; cx++) { > + cpu_resume(ccpus[cx]); > + } > + } else if (def == 'c' || def == 'C') { > + for (cx = 0; scpus && scpus[cx]; cx++) { > + cpu_single_step(scpus[cx], sstep_flags); > + } > + CPU_FOREACH(cpu) { > + cpu_resume(cpu); > + } > + } else if (def == 's' || def == 'S') { > + CPU_FOREACH(cpu) { > + cpu_single_step(cpu, sstep_flags); > + } > + for (cx = 0; ccpus && ccpus[cx]; cx++) { > + cpu_single_step(cpu, 0); > + } > + CPU_FOREACH(cpu) { > + cpu_resume(cpu); > + } > + } else { > + res = -1; > + } > + /* > + * XXX vm_start also calls qapi_event_send_resume(&error_abort); here, > + * is it actually important? moreover I can't find where it's defined, > + * and calling it here yields a compiler error. > + */ > + /* qapi_event_send_resume(&error_abort); */
> > + if (def == 0) { > > + for (cx = 0; scpus && scpus[cx]; cx++) { > > + cpu_single_step(scpus[cx], sstep_flags); > > + cpu_resume(scpus[cx]); > > + } > > + for (cx = 0; ccpus && ccpus[cx]; cx++) { > > + cpu_resume(ccpus[cx]); > > + } > > + } else if (def == 'c' || def == 'C') { > > + for (cx = 0; scpus && scpus[cx]; cx++) { > > + cpu_single_step(scpus[cx], sstep_flags); > > + } > > + CPU_FOREACH(cpu) { > > + cpu_resume(cpu); > > + } > > + } else if (def == 's' || def == 'S') { > > + CPU_FOREACH(cpu) { > > + cpu_single_step(cpu, sstep_flags); > > + } > > + for (cx = 0; ccpus && ccpus[cx]; cx++) { > > + cpu_single_step(cpu, 0); This looks suspicious > > + } > > + CPU_FOREACH(cpu) { > > + cpu_resume(cpu); > > + } Claudio, did you have a look at how s->c_cpu is used later on? I remember that we have to take care of some query reply packages. David
On 12/10/16 15:15, David Hildenbrand wrote: >>> + for (cx = 0; ccpus && ccpus[cx]; cx++) { >>> + cpu_single_step(cpu, 0); > > This looks suspicious why? we set all cpus to single step, since that is the default, and then we clear the single-step property from all CPUs that should be restarted in normal mode, then we restart all CPUs. Those in single-step will indeed only perform one single step, the others will run freely (at least until the first single-step CPU stops again). >>> + } >>> + CPU_FOREACH(cpu) { >>> + cpu_resume(cpu); >>> + } > > Claudio, did you have a look at how s->c_cpu is used later on? I remember that we > have to take care of some query reply packages. yes, that's set by the H packet and used by the c,s,m,etc packets. vCont ignores it and doesn't change it (see here https://sourceware.org/gdb/onlinedocs/gdb/Packets.html )
On 12/10/2016 15:55, Claudio Imbrenda wrote: >>>> >>> + for (cx = 0; ccpus && ccpus[cx]; cx++) { >>>> >>> + cpu_single_step(cpu, 0); >> > >> > This looks suspicious > why? we set all cpus to single step, since that is the default, and then > we clear the single-step property from all CPUs that should be restarted > in normal mode, then we restart all CPUs. Those in single-step will > indeed only perform one single step, the others will run freely (at > least until the first single-step CPU stops again). > Yeah, it makes sense. Paolo
On Wed, Oct 12, 2016 at 03:55:18PM +0200, Claudio Imbrenda wrote: > On 12/10/16 15:15, David Hildenbrand wrote: > >>> + for (cx = 0; ccpus && ccpus[cx]; cx++) { > >>> + cpu_single_step(cpu, 0); > > > > This looks suspicious > > why? we set all cpus to single step, since that is the default, and then > we clear the single-step property from all CPUs that should be restarted > in normal mode, then we restart all CPUs. Those in single-step will > indeed only perform one single step, the others will run freely (at > least until the first single-step CPU stops again). actually I was more concerned about calling it on "cpu" in a loop. GDB will: - single step one thread only (stopping all other) - use vCont as default. So this means quite some ioctls on every step with some VCPUs. I doubt that it will really be a problem (e.g. for GDB single stepping instead of setting breakpoints when returning froma function), but still I want to have it said. (we actually only need 1 ioctl but call quite a lot). > > >>> + } > >>> + CPU_FOREACH(cpu) { > >>> + cpu_resume(cpu); > >>> + } > > > > Claudio, did you have a look at how s->c_cpu is used later on? I remember that we > > have to take care of some query reply packages. > > yes, that's set by the H packet and used by the c,s,m,etc packets. vCont > ignores it and doesn't change it > (see here https://sourceware.org/gdb/onlinedocs/gdb/Packets.html ) I remember something different (also having to do with clients detaching and re-attaching). Will have a look at the code when I have time.
----- Original Message ----- > From: "David Hildenbrand" <dahi@daveh.de> > To: "Claudio Imbrenda" <imbrenda@linux.vnet.ibm.com> > Cc: "David Hildenbrand" <dahi@daveh.de>, "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org > Sent: Wednesday, October 12, 2016 8:38:15 PM > Subject: Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: Fix vCont behaviour > > On Wed, Oct 12, 2016 at 03:55:18PM +0200, Claudio Imbrenda wrote: > > On 12/10/16 15:15, David Hildenbrand wrote: > > >>> + for (cx = 0; ccpus && ccpus[cx]; cx++) { > > >>> + cpu_single_step(cpu, 0); > > > > > > This looks suspicious > > > > why? we set all cpus to single step, since that is the default, and then > > we clear the single-step property from all CPUs that should be restarted > > in normal mode, then we restart all CPUs. Those in single-step will > > indeed only perform one single step, the others will run freely (at > > least until the first single-step CPU stops again). > > actually I was more concerned about calling it on "cpu" in a loop. And we all missed that it should have been ccpus[cx], not cpu. :) Paolo
diff --git a/gdbstub.c b/gdbstub.c index ecea8c4..0ce9c83 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -386,6 +386,65 @@ static inline void gdb_continue(GDBState *s) #endif } +/* Resume execution, per CPU actions. */ +static int gdb_continue_partial(GDBState *s, char def, CPUState **scpus, + CPUState **ccpus) +{ + int res = 0; +#ifdef CONFIG_USER_ONLY + s->running_state = 1; +#else + CPUState *cpu; + int cx; + + if (!runstate_needs_reset()) { + /* + * XXX vm_start also calls qemu_vmstop_requested(&requested); here, is + * it actually important? it's static in vl.c + */ + cpu_enable_ticks(); + runstate_set(RUN_STATE_RUNNING); + vm_state_notify(1, RUN_STATE_RUNNING); + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); + if (def == 0) { + for (cx = 0; scpus && scpus[cx]; cx++) { + cpu_single_step(scpus[cx], sstep_flags); + cpu_resume(scpus[cx]); + } + for (cx = 0; ccpus && ccpus[cx]; cx++) { + cpu_resume(ccpus[cx]); + } + } else if (def == 'c' || def == 'C') { + for (cx = 0; scpus && scpus[cx]; cx++) { + cpu_single_step(scpus[cx], sstep_flags); + } + CPU_FOREACH(cpu) { + cpu_resume(cpu); + } + } else if (def == 's' || def == 'S') { + CPU_FOREACH(cpu) { + cpu_single_step(cpu, sstep_flags); + } + for (cx = 0; ccpus && ccpus[cx]; cx++) { + cpu_single_step(cpu, 0); + } + CPU_FOREACH(cpu) { + cpu_resume(cpu); + } + } else { + res = -1; + } + /* + * XXX vm_start also calls qapi_event_send_resume(&error_abort); here, + * is it actually important? moreover I can't find where it's defined, + * and calling it here yields a compiler error. + */ + /* qapi_event_send_resume(&error_abort); */ + } +#endif + return res; +} + static void put_buffer(GDBState *s, const uint8_t *buf, int len) { #ifdef CONFIG_USER_ONLY @@ -829,62 +888,114 @@ 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; + CPUState **s_cpus, **c_cpus; + CPUState *curcpu; + int idx, signal, broadcast_action = 0; + int scnt, ccnt; + char default_action = 0; + char cur_action = 0; + unsigned long tmp; p += 4; if (*p == '?') { put_packet(s, "vCont;c;C;s;S"); break; } + if (*p != ';') { + goto unknown_command; + } + + for (idx = scnt = 0; p[idx]; idx++) { + if (p[idx] == ';') { + scnt++; + } + } + s_cpus = g_malloc((scnt + 1) * sizeof(*s_cpus)); + c_cpus = g_malloc((scnt + 1) * sizeof(*c_cpus)); + scnt = ccnt = 0; + signal = 0; + + /* + * For us: vCont[;action[:thread-id]]... + * where action can be one of c s C<sig> S<sig> + */ res = 0; - res_signal = 0; - res_thread = 0; while (*p) { - int action, signal; - - if (*p++ != ';') { - res = 0; + if (*p != ';') { + res = -1; 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; + p++; /* skip the ; */ + + if (*p == 'C' || *p == 'S' || *p == 'c' || *p == 's') { + cur_action = *p; + if (*p == 'C' || *p == 'S') { + (void) qemu_strtoul(p + 1, &p, 16, &tmp); + signal = gdb_signal_to_target(tmp); + } else { + p++; + } + /* thread specified. special values: -1 = all, 0 = any */ + if (*p == ':') { + if (broadcast_action) { + res = -22; + break; + } + p++; + if ((p[0] == '-') && (p[1] == '1')) { + if (broadcast_action || scnt || ccnt) { + res = -22; + break; + } + broadcast_action = cur_action; + p += 2; + continue; + } + (void) qemu_strtoul(p, &p, 16, &tmp); + idx = tmp; + /* 0 means any thread, so we pick the first */ + idx = idx ? idx : 1; + curcpu = find_cpu(idx); + if (!curcpu) { + res = -22; + break; + } + if (cur_action == 's' || cur_action == 'S') { + s_cpus[scnt++] = curcpu; + } else { + c_cpus[ccnt++] = curcpu; + } + } else { /* no thread specified */ + if (default_action != 0) { + res = -22; /* error */ + break; + } + default_action = cur_action; } - } else if (action != 'c' && action != 's') { - res = 0; + } else { /* unknown/invalid/unsupported command */ + res = -1; 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; - } } 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; + g_free(s_cpus); + g_free(c_cpus); + if (res == -1) { + goto unknown_command; } - if (res == 's') { - cpu_single_step(s->c_cpu, sstep_flags); - } - s->signal = res_signal; - gdb_continue(s); - return RS_IDLE; + put_packet(s, "E22"); + break; } - break; + s->signal = signal; + s_cpus[scnt] = c_cpus[ccnt] = NULL; + if (broadcast_action) { + gdb_continue_partial(s, broadcast_action, NULL, NULL); + } else { + gdb_continue_partial(s, default_action, s_cpus, c_cpus); + } + g_free(s_cpus); + g_free(c_cpus); + return RS_IDLE; } else { goto unknown_command; }
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: * adds an additional helper function to selectively restart only some CPUs * completely rewrites the vCont parsing code Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> --- gdbstub.c | 189 +++++++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 150 insertions(+), 39 deletions(-)