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

[Claudio Imbrenda]

On 06/02/17 11:00, Paolo Bonzini wrote:
> 
> 
> On 27/01/2017 19:11, Claudio Imbrenda wrote:
>> +    /* mark valid CPUs with 1 */
>> +    CPU_FOREACH(cpu) {
>> +        newstates[cpu_index(cpu) - 1] = 1;
>> +    }
> 
> Sorry I didn't notice this before: CPU indices are zero-based in QEMU,
> so you are probably overwriting newstates[-1].  I can adjust it myself,
> but can you please double check?

they are zero based in the struct, but the already existing cpu_index
function (include/exec/gdbstub.h) does this:

static inline int cpu_index(CPUState *cpu)
{
#if defined(CONFIG_USER_ONLY)
    return cpu->host_tid;
#else
    return cpu->cpu_index + 1;
#endif
}


maybe that can just become  newstates[cpu->cpu_index] = 1  ?
(since we're not in CONFIG_USER_ONLY anyway)


> Paolo
> 
>> +
>> +    /*
>> +     * 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 = -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);
>> +            }
>> +
>> +            /* invalid CPU specified */
>> +            if (!idx || idx > max_cpus || !newstates[idx - 1]) {
>> +                res = -EINVAL;
>> +                goto out;
>> +            }
>> +            /* only use if no previous match occourred */
>> +            if (newstates[idx - 1] == 1) {
>> +                newstates[idx - 1] = cur_action;
>> +            }
>> +        }
>