Re: [Qemu-devel] [PATCH v2 2/2] gdbstub: Fix vCont behaviour

[Claudio Imbrenda]

On 27/01/17 18:07, Alex Bennée wrote:
> 
> Claudio Imbrenda <address@hidden> writes:
> 
>> 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 some additional helper functions to selectively restart only
>>   some CPUs
>> * 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 <address@hidden>
>> ---
>>  gdbstub.c | 226 
>> +++++++++++++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 179 insertions(+), 47 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index ecea8c4..ea42afa 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -386,6 +386,61 @@ static inline void gdb_continue(GDBState *s)
>>  #endif
>>  }
>>
>> +static void gdb_single_step_cpus(CPUState **scpus, int flags)
>> +{
>> +    CPUState *cpu;
>> +    int cx;
>> +
>> +    if (!scpus) {
>> +        CPU_FOREACH(cpu) {
>> +            cpu_single_step(cpu, flags);
>> +        }
>> +    } else {
>> +        for (cx = 0; scpus[cx]; cx++) {
>> +            cpu_single_step(scpus[cx], flags);
>> +        }
>> +    }
>> +}
> 
> I missed this last time but as gdb_single_step_cpus() is only called for
> system emulation it breaks the linux-user compilation with:
> 
>   error: ‘gdb_single_step_cpus’ defined but not used [-Werror=unused-function]

that function is not there any longer in the patch I sent yesterday (v6)

I'm about to send a v7 to update the description of the first patch.

>> +
>> +/*
>> + * Resume execution, per CPU actions. For user-more emulation it's
>> + * equivalent to gdb_continue .
>> + */
>> +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
>> +    if (!runstate_needs_reset()) {
>> +        if (vm_prepare_start()) {
>> +            return 0;
>> +        }
>> +
>> +        if (def == 0) {
>> +            gdb_single_step_cpus(scpus, sstep_flags);
>> +            /* CPUs not in scpus or ccpus stay stopped */
>> +            qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>> +            resume_some_vcpus(scpus);
>> +            resume_some_vcpus(ccpus);
>> +        } else if (def == 'c' || def == 'C') {
>> +            gdb_single_step_cpus(scpus, sstep_flags);
>> +            qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>> +            resume_all_vcpus();
>> +        } else if (def == 's' || def == 'S') {
>> +            gdb_single_step_cpus(NULL, sstep_flags);
>> +            gdb_single_step_cpus(ccpus, 0);
>> +            qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>> +            resume_all_vcpus();
>> +        } else {
>> +            res = -1;
>> +        }
>> +    }
>> +#endif
>> +    return res;
>> +}
>> +
>>  static void put_buffer(GDBState *s, const uint8_t *buf, int len)
>>  {
>>  #ifdef CONFIG_USER_ONLY
>> @@ -784,6 +839,123 @@ 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 -1 if a command is unsupported, -22 if there is a format error,
>> + *         0 on success.
>> + */
>> +static int gdb_handle_vcont(GDBState *s, const char *p)
>> +{
>> +    CPUState **s_cpus, **c_cpus;
>> +    CPUState *curcpu;
>> +    int res, idx, signal, scnt, ccnt;
>> +    char cur_action, default_action, broadcast_action;
>> +    unsigned long tmp;
>> +
>> +    /*
>> +     * For us: vCont[;action[:thread-id]]...
>> +     * where action can be one of c s C<sig> S<sig>
>> +     */
>> +    for (idx = scnt = 0; p[idx]; idx++) {
>> +        if (p[idx] == ';') {
>> +            scnt++;
>> +        }
>> +    }
>> +    s_cpus = g_new(CPUState *, scnt + 1);
>> +    c_cpus = g_new(CPUState *, scnt + 1);
>> +    scnt = ccnt = signal = 0;
>> +    default_action = broadcast_action = 0;
>> +
>> +    /*
>> +     * res keeps track of what error we are returning, with -1 meaning
>> +     * that the command is unknown or unsupported, and thus returning
>> +     * an empty packet, while -22 returns an E22 packet due to
>> +     * invalid or incorrect parameters passed.
>> +     */
>> +    res = 0;
>> +    while (*p) {
>> +        if (*p != ';') {
>> +            res = -1;
>> +            break;
>> +        }
>> +        p++; /* skip the ; */
>> +
>> +        /* unknown/invalid/unsupported command */
>> +        if (*p != 'C' && *p != 'S' && *p != 'c' && *p != 's') {
>> +            res = -1;
>> +            break;
>> +        }
>> +        cur_action = *p;
>> +        if (*p == 'C' || *p == 'S') {
>> +            if (qemu_strtoul(p + 1, &p, 16, &tmp)) {
>> +                res = -22;
>> +                break;
>> +            }
>> +            signal = gdb_signal_to_target(tmp);
>> +        } else {
>> +            p++;
>> +        }
>> +        /* thread specified. special values: -1 = all, 0 = any */
>> +        if (*p == ':') {
>> +            /*
>> +             * cannot specify an action for a single thread when an action
>> +             * was already specified for all threads
>> +             */
>> +            if (broadcast_action) {
>> +                res = -22;
>> +                break;
>> +            }
>> +            p++;
>> +            if ((p[0] == '-') && (p[1] == '1')) {
>> +                /*
>> +                 * specifying an action for all threads when some threads
>> +                 * already have actions.
>> +                 */
>> +                if (scnt || ccnt) {
>> +                    res = -22;
>> +                    break;
>> +                }
>> +                broadcast_action = cur_action;
>> +                p += 2;
>> +                continue;
>> +            }
>> +            if (qemu_strtoul(p, &p, 16, &tmp)) {
>> +                res = -22;
>> +                break;
>> +            }
>> +            /* 0 means any thread, so we pick the first */
>> +            idx = tmp ? tmp : 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, can't specify multiple defaults! */
>> +                break;
>> +            }
>> +            default_action = cur_action;
>> +        }
>> +    }
>> +    if (!res) {
>> +        s->signal = signal;
>> +        s_cpus[scnt] = c_cpus[ccnt] = NULL;
>> +        default_action = broadcast_action ? broadcast_action : 
>> default_action;
>> +        gdb_continue_partial(s, default_action, s_cpus, c_cpus);
>> +    }
>> +
>> +    g_free(s_cpus);
>> +    g_free(c_cpus);
>> +
>> +    return res;
>> +}
>> +
>>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>>  {
>>      CPUState *cpu;
>> @@ -829,60 +1001,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 == -22) {
>> +                    put_packet(s, "E22");
>> +                    break;
>>                  }
>> -                s->signal = res_signal;
>> -                gdb_continue(s);
>> -                return RS_IDLE;
>> +                goto unknown_command;
>>              }
>>              break;
>>          } else {
> 
> 
> --
> Alex Bennée
>