[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] gdbstub: Fix vCont behaviour
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] gdbstub: Fix vCont behaviour |
Date: |
Fri, 27 Jan 2017 17:33:24 +0000 |
User-agent: |
mu4e 0.9.19; emacs 25.1.91.4 |
Claudio Imbrenda <address@hidden> writes:
> 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.
OK, please CC me when you send it out.
>
>>> +
>>> +/*
>>> + * 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
>>
--
Alex Bennée