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); */
