[Qemu-devel] [PATCH v3 0/2] Qemu: gdbstub: fix vCont

[Qemu-devel] [PATCH v3 0/2] Qemu: gdbstub: fix vCont

[Claudio Imbrenda]

This small patchset fixes the incorrect behaviour of the vCont command
in the gdb stub. 

The first patch, as suggested be Paolo, refactors some code. The most
visible change is that it moves vm_start to cpus.c 

The second one fixes the incorrect behaviour of the vCont command.
Previously, continuing or stepping a single thread (CPU) caused all
other CPUs to be started too, whereas the GDB specification clearly
states that without a default action all threads not explicitly
mentioned in the command should stay stopped.

So if the Qemu gdbstub receives a  vCont;c:1  packet, no other CPU
should be restarted except the first, and when a  vCont;s:1  is
received, the first CPU should be stepped without restarting the others.
With this patchset Qemu now behaves as expected.

See here for reference material about the packets: 
https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html
https://sourceware.org/gdb/onlinedocs/gdb/Packets.html

v2 -> v3
* removed resume_some_vcpus
* cleared up the code and simplified the implementation in light of the 
  clarification in the specification of the vCont packet

Claudio Imbrenda (2):
  move vm_start to cpus.c
  gdbstub: Fix vCont behaviour

 cpus.c                     |  44 ++++++++++-
 gdbstub.c                  | 189 ++++++++++++++++++++++++++++++++++-----------
 hw/i386/kvmvapic.c         |   2 +
 include/sysemu/cpus.h      |   1 +
 include/sysemu/sysemu.h    |   2 +
 target-s390x/misc_helper.c |   2 +
 vl.c                       |  32 +-------
 7 files changed, 195 insertions(+), 77 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v3 1/2] move vm_start to cpus.c

[Claudio Imbrenda]

This patch:

* moves vm_start to cpus.c .
* exports qemu_vmstop_requested, since it's needed by vm_start .
* extracts vm_prepare_start from vm_start; it does what vm_start did,
  except restarting the cpus. vm_start now calls vm_prepare_start.
* moves the call to qemu_clock_enable away from resume_all_vcpus, and
  add an explicit call to it before each instance of resume_all_vcpus
  in the code.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 cpus.c                     | 44 +++++++++++++++++++++++++++++++++++++++++++-
 hw/i386/kvmvapic.c         |  2 ++
 include/sysemu/cpus.h      |  1 +
 include/sysemu/sysemu.h    |  2 ++
 target-s390x/misc_helper.c |  2 ++
 vl.c                       | 32 +++-----------------------------
 6 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/cpus.c b/cpus.c
index cfd5cdc..34667fa 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1260,7 +1260,6 @@ void resume_all_vcpus(void)
 {
     CPUState *cpu;
 
-    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
     CPU_FOREACH(cpu) {
         cpu_resume(cpu);
     }
@@ -1396,6 +1395,49 @@ int vm_stop(RunState state)
     return do_vm_stop(state);
 }
 
+/**
+ * Prepare for (re)starting the VM.
+ * Returns -1 if the vCPUs are not to be restarted (e.g. if they are already
+ * running or in case of an error condition), 0 otherwise.
+ */
+int vm_prepare_start(void)
+{
+    RunState requested;
+    int res = 0;
+
+    qemu_vmstop_requested(&requested);
+    if (runstate_is_running() && requested == RUN_STATE__MAX) {
+        return -1;
+    }
+
+    /* Ensure that a STOP/RESUME pair of events is emitted if a
+     * vmstop request was pending.  The BLOCK_IO_ERROR event, for
+     * example, according to documentation is always followed by
+     * the STOP event.
+     */
+    if (runstate_is_running()) {
+        qapi_event_send_stop(&error_abort);
+        res = -1;
+    } else {
+        replay_enable_events();
+        cpu_enable_ticks();
+        runstate_set(RUN_STATE_RUNNING);
+        vm_state_notify(1, RUN_STATE_RUNNING);
+    }
+
+    /* XXX: is it ok to send this even before actually resuming the CPUs? */
+    qapi_event_send_resume(&error_abort);
+    return res;
+}
+
+void vm_start(void)
+{
+    if (!vm_prepare_start()) {
+        qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
+        resume_all_vcpus();
+    }
+}
+
 /* does a state transition even if the VM is already stopped,
    current state is forgotten forever */
 int vm_stop_force_state(RunState state)
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 74a549b..3101860 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -446,6 +446,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
         abort();
     }
 
+    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
     resume_all_vcpus();
 
     if (!kvm_enabled()) {
@@ -686,6 +687,7 @@ static void vapic_write(void *opaque, hwaddr addr, uint64_t data,
             pause_all_vcpus();
             patch_byte(cpu, env->eip - 2, 0x66);
             patch_byte(cpu, env->eip - 1, 0x90);
+            qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
             resume_all_vcpus();
         }
 
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 3728a1e..5fa074b 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -5,6 +5,7 @@
 bool qemu_in_vcpu_thread(void);
 void qemu_init_cpu_loop(void);
 void resume_all_vcpus(void);
+void resume_some_vcpus(CPUState **cpus);
 void pause_all_vcpus(void);
 void cpu_stop_current(void);
 void cpu_ticks_init(void);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 66c6f15..ebe8b76 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -37,6 +37,7 @@ void vm_state_notify(int running, RunState state);
 #define VMRESET_REPORT   true
 
 void vm_start(void);
+int vm_prepare_start(void);
 int vm_stop(RunState state);
 int vm_stop_force_state(RunState state);
 
@@ -60,6 +61,7 @@ void qemu_register_powerdown_notifier(Notifier *notifier);
 void qemu_system_debug_request(void);
 void qemu_system_vmstop_request(RunState reason);
 void qemu_system_vmstop_request_prepare(void);
+bool qemu_vmstop_requested(RunState *r);
 int qemu_shutdown_requested_get(void);
 int qemu_reset_requested_get(void);
 void qemu_system_killed(int signal, pid_t pid);
diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c
index 4df2ec6..2b74710 100644
--- a/target-s390x/misc_helper.c
+++ b/target-s390x/misc_helper.c
@@ -133,6 +133,7 @@ static int modified_clear_reset(S390CPU *cpu)
     s390_crypto_reset();
     scc->load_normal(CPU(cpu));
     cpu_synchronize_all_post_reset();
+    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
     resume_all_vcpus();
     return 0;
 }
@@ -152,6 +153,7 @@ static int load_normal_reset(S390CPU *cpu)
     scc->initial_cpu_reset(CPU(cpu));
     scc->load_normal(CPU(cpu));
     cpu_synchronize_all_post_reset();
+    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
     resume_all_vcpus();
     return 0;
 }
diff --git a/vl.c b/vl.c
index 4ec8120..e2dbc45 100644
--- a/vl.c
+++ b/vl.c
@@ -746,7 +746,7 @@ StatusInfo *qmp_query_status(Error **errp)
     return info;
 }
 
-static bool qemu_vmstop_requested(RunState *r)
+bool qemu_vmstop_requested(RunState *r)
 {
     qemu_mutex_lock(&vmstop_lock);
     *r = vmstop_requested;
@@ -767,34 +767,6 @@ void qemu_system_vmstop_request(RunState state)
     qemu_notify_event();
 }
 
-void vm_start(void)
-{
-    RunState requested;
-
-    qemu_vmstop_requested(&requested);
-    if (runstate_is_running() && requested == RUN_STATE__MAX) {
-        return;
-    }
-
-    /* Ensure that a STOP/RESUME pair of events is emitted if a
-     * vmstop request was pending.  The BLOCK_IO_ERROR event, for
-     * example, according to documentation is always followed by
-     * the STOP event.
-     */
-    if (runstate_is_running()) {
-        qapi_event_send_stop(&error_abort);
-    } else {
-        replay_enable_events();
-        cpu_enable_ticks();
-        runstate_set(RUN_STATE_RUNNING);
-        vm_state_notify(1, RUN_STATE_RUNNING);
-        resume_all_vcpus();
-    }
-
-    qapi_event_send_resume(&error_abort);
-}
-
-
 /***********************************************************/
 /* real time host monotonic timer */
 
@@ -1914,6 +1886,7 @@ static bool main_loop_should_exit(void)
     if (qemu_reset_requested()) {
         pause_all_vcpus();
         qemu_system_reset(VMRESET_REPORT);
+        qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
         resume_all_vcpus();
         if (!runstate_check(RUN_STATE_RUNNING) &&
                 !runstate_check(RUN_STATE_INMIGRATE)) {
@@ -1925,6 +1898,7 @@ static bool main_loop_should_exit(void)
         qemu_system_reset(VMRESET_SILENT);
         notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
         wakeup_reason = QEMU_WAKEUP_REASON_NONE;
+        qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
         resume_all_vcpus();
         qapi_event_send_wakeup(&error_abort);
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 1/2] move vm_start to cpus.c

[Paolo Bonzini]

On 28/10/2016 19:15, Claudio Imbrenda wrote:
> * moves the call to qemu_clock_enable away from resume_all_vcpus, and
>   add an explicit call to it before each instance of resume_all_vcpus
>   in the code.

This change adds useless duplication, and isn't matched by a similar
change to pause_all_vcpus.  You need to justify it; I suppose it is
because the next patch will not call resume_all_cpus?

Most of the callers of pause_all_vcpus/resume_all_vcpus don't let timers
run, so the clock need not be disabled and enabled.  Maybe the right
places to call qemu_clock_enable are cpu_disable_ticks and
cpu_enable_ticks?  That should work for you.

In that case, please make the first patch the qemu_clock_enable
movement; the second patch the introduction of vm_prepare_start; the
third patch the gdbstub change.

> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index 3728a1e..5fa074b 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -5,6 +5,7 @@
>  bool qemu_in_vcpu_thread(void);
>  void qemu_init_cpu_loop(void);
>  void resume_all_vcpus(void);
> +void resume_some_vcpus(CPUState **cpus);
>  void pause_all_vcpus(void);
>  void cpu_stop_current(void);
>  void cpu_ticks_init(void);

This function doesn't exist.

Paolo

Re: [Qemu-devel] [PATCH v3 1/2] move vm_start to cpus.c

[Claudio Imbrenda]

On 25/01/17 11:21, Paolo Bonzini wrote:
> 
> 
> On 28/10/2016 19:15, Claudio Imbrenda wrote:
>> * moves the call to qemu_clock_enable away from resume_all_vcpus, and
>>   add an explicit call to it before each instance of resume_all_vcpus
>>   in the code.
> 
> This change adds useless duplication, and isn't matched by a similar
> change to pause_all_vcpus.  You need to justify it; I suppose it is
> because the next patch will not call resume_all_cpus?

Actually it is because the next patch used to call resume_all_vcpus, and
we didn't want it to restart the clock too. But then I ended up not
using resume_all_vcpus.

And now I have actually no idea why we didn't want to restart the clock.
Maybe we should? After all some CPUs are running. I can patch
gdb_continue_partial to check if any CPU was actually restarted, and if
so start the clock.

So in the end it should still be correct, but the changes would be kept
small.

> Most of the callers of pause_all_vcpus/resume_all_vcpus don't let timers
> run, so the clock need not be disabled and enabled.  Maybe the right
> places to call qemu_clock_enable are cpu_disable_ticks and
> cpu_enable_ticks?  That should work for you.
> 
> In that case, please make the first patch the qemu_clock_enable
> movement; the second patch the introduction of vm_prepare_start; the
> third patch the gdbstub change.
> 
>> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
>> index 3728a1e..5fa074b 100644
>> --- a/include/sysemu/cpus.h
>> +++ b/include/sysemu/cpus.h
>> @@ -5,6 +5,7 @@
>>  bool qemu_in_vcpu_thread(void);
>>  void qemu_init_cpu_loop(void);
>>  void resume_all_vcpus(void);
>> +void resume_some_vcpus(CPUState **cpus);
>>  void pause_all_vcpus(void);
>>  void cpu_stop_current(void);
>>  void cpu_ticks_init(void);
> 
> This function doesn't exist.

oops.


Claudio

Re: [Qemu-devel] [PATCH v3 1/2] move vm_start to cpus.c

[Paolo Bonzini]

On 25/01/2017 18:53, Claudio Imbrenda wrote:
> On 25/01/17 11:21, Paolo Bonzini wrote:
>>
>>
>> On 28/10/2016 19:15, Claudio Imbrenda wrote:
>>> * moves the call to qemu_clock_enable away from resume_all_vcpus, and
>>>   add an explicit call to it before each instance of resume_all_vcpus
>>>   in the code.
>>
>> This change adds useless duplication, and isn't matched by a similar
>> change to pause_all_vcpus.  You need to justify it; I suppose it is
>> because the next patch will not call resume_all_cpus?
> 
> Actually it is because the next patch used to call resume_all_vcpus, and
> we didn't want it to restart the clock too. But then I ended up not
> using resume_all_vcpus.
> 
> And now I have actually no idea why we didn't want to restart the clock.
> Maybe we should? After all some CPUs are running. I can patch
> gdb_continue_partial to check if any CPU was actually restarted, and if
> so start the clock.
> 
> So in the end it should still be correct, but the changes would be kept
> small.

Yeah, this sounds like a good solution.  Thanks!

Paolo

>> Most of the callers of pause_all_vcpus/resume_all_vcpus don't let timers
>> run, so the clock need not be disabled and enabled.  Maybe the right
>> places to call qemu_clock_enable are cpu_disable_ticks and
>> cpu_enable_ticks?  That should work for you.
>>
>> In that case, please make the first patch the qemu_clock_enable
>> movement; the second patch the introduction of vm_prepare_start; the
>> third patch the gdbstub change.
>>
>>> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
>>> index 3728a1e..5fa074b 100644
>>> --- a/include/sysemu/cpus.h
>>> +++ b/include/sysemu/cpus.h
>>> @@ -5,6 +5,7 @@
>>>  bool qemu_in_vcpu_thread(void);
>>>  void qemu_init_cpu_loop(void);
>>>  void resume_all_vcpus(void);
>>> +void resume_some_vcpus(CPUState **cpus);
>>>  void pause_all_vcpus(void);
>>>  void cpu_stop_current(void);
>>>  void cpu_ticks_init(void);
>>
>> This function doesn't exist.
> 
> oops.
> 
> 
> Claudio
> 

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

[Claudio Imbrenda]

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 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 <imbrenda@linux.vnet.ibm.com>
---
 gdbstub.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 142 insertions(+), 47 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index b2e1b79..9bb548f 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -386,6 +386,45 @@ static inline void gdb_continue(GDBState *s)
 #endif
 }
 
+/*
+ * Resume execution, per CPU actions. For user-more emulation it's
+ * equivalent to gdb_continue .
+ */
+static int gdb_continue_partial(GDBState *s, char *newstates)
+{
+    int res = 0;
+#ifdef CONFIG_USER_ONLY
+    s->running_state = 1;
+#else
+    CPUState *cpu;
+
+    if (!runstate_needs_reset()) {
+        if (vm_prepare_start()) {
+            return 0;
+        }
+
+        CPU_FOREACH(cpu) {
+            switch (newstates[cpu_index(cpu) - 1]) {
+            case 0:
+            case 1:
+                break; /* nothing to do here */
+            case 's':
+                cpu_single_step(cpu, sstep_flags);
+                cpu_resume(cpu);
+                break;
+            case 'c':
+                cpu_resume(cpu);
+                break;
+            default:
+                res = -1;
+                break;
+            }
+        }
+    }
+#endif
+    return res;
+}
+
 static void put_buffer(GDBState *s, const uint8_t *buf, int len)
 {
 #ifdef CONFIG_USER_ONLY
@@ -784,6 +823,102 @@ 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)
+{
+    int res, idx, signal = 0;
+    char cur_action;
+    char *newstates;
+    unsigned long tmp;
+    CPUState *cpu;
+
+    /* uninitialised CPUs stay 0 */
+    newstates = g_new0(char, max_cpus);
+
+    /* mark valid CPUs with 1 */
+    CPU_FOREACH(cpu) {
+        newstates[cpu_index(cpu) - 1] = 1;
+    }
+
+    /*
+     * 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 = tolower(*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 specification. special values: (none), -1 = all; 0 = any */
+        if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) {
+            for (idx = 0; idx < max_cpus; idx++) {
+                if (newstates[idx] == 1) {
+                    newstates[idx] = cur_action;
+                }
+            }
+            if (*p == ':') {
+                p += 3;
+            }
+        } else if (*p == ':') {
+            p++;
+            if (qemu_strtoul(p, &p, 16, &tmp)) {
+                res = -22;
+                break;
+            }
+            idx = tmp;
+            /* 0 means any thread, so we pick the first valid CPU */
+            if (!idx) {
+                CPU_FOREACH(cpu) {
+                    idx = cpu_index(cpu);
+                    break;
+                }
+            }
+
+            /* invalid CPU specified */
+            if (!idx || idx > max_cpus || !newstates[idx - 1]) {
+                res = -22;
+                break;
+            }
+            /* only use if no previous match occourred */
+            if (newstates[idx - 1] == 1) {
+                newstates[idx - 1] = cur_action;
+            }
+        }
+    }
+    if (!res) {
+        s->signal = signal;
+        gdb_continue_partial(s, newstates);
+    }
+
+    g_free(newstates);
+
+    return res;
+}
+
 static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
     CPUState *cpu;
@@ -829,60 +964,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 {
-- 
1.9.1

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

[Paolo Bonzini]

On 28/10/2016 19:15, Claudio Imbrenda wrote:
> 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 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 <imbrenda@linux.vnet.ibm.com>
> ---
>  gdbstub.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 142 insertions(+), 47 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index b2e1b79..9bb548f 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -386,6 +386,45 @@ static inline void gdb_continue(GDBState *s)
>  #endif
>  }
>  
> +/*
> + * Resume execution, per CPU actions. For user-more emulation it's
> + * equivalent to gdb_continue .
> + */
> +static int gdb_continue_partial(GDBState *s, char *newstates)
> +{
> +    int res = 0;
> +#ifdef CONFIG_USER_ONLY
> +    s->running_state = 1;
> +#else
> +    CPUState *cpu;
> +
> +    if (!runstate_needs_reset()) {
> +        if (vm_prepare_start()) {
> +            return 0;
> +        }
> +
> +        CPU_FOREACH(cpu) {
> +            switch (newstates[cpu_index(cpu) - 1]) {
> +            case 0:
> +            case 1:
> +                break; /* nothing to do here */
> +            case 's':
> +                cpu_single_step(cpu, sstep_flags);
> +                cpu_resume(cpu);
> +                break;
> +            case 'c':
> +                cpu_resume(cpu);
> +                break;
> +            default:
> +                res = -1;
> +                break;
> +            }
> +        }
> +    }
> +#endif
> +    return res;
> +}
> +
>  static void put_buffer(GDBState *s, const uint8_t *buf, int len)
>  {
>  #ifdef CONFIG_USER_ONLY
> @@ -784,6 +823,102 @@ 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)
> +{
> +    int res, idx, signal = 0;
> +    char cur_action;
> +    char *newstates;
> +    unsigned long tmp;
> +    CPUState *cpu;
> +
> +    /* uninitialised CPUs stay 0 */
> +    newstates = g_new0(char, max_cpus);
> +
> +    /* mark valid CPUs with 1 */
> +    CPU_FOREACH(cpu) {
> +        newstates[cpu_index(cpu) - 1] = 1;
> +    }
> +
> +    /*
> +     * 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.
> +     */

Please use ENOTSUP for unknown/unsupported and any other negative errno
value for invalid/incorrect parameters.  This simplifies the code a bit
in this function, as we know that qemu_strtoul only returns -EINVAL or
-ERANGE.

> +    res = 0;
> +    while (*p) {
> +        if (*p != ';') {

Use *p++ here.

> +            res = -1;
> +            break;

If you change all returns with error to a "goto out", you can avoid the
"if (!res)" at the end of the loop.

> +        }
> +        p++; /* skip the ; */
> +
> +        /* unknown/invalid/unsupported command */
> +        if (*p != 'C' && *p != 'S' && *p != 'c' && *p != 's') {
> +            res = -1;
> +            break;
> +        }
> +        cur_action = tolower(*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++;
> +        }

Maybe:

	cur_action = *p++;
	if (cur_action == 'C' || cur_action == 'S') {
	    cur_action = tolower(cur_action);
	    res = qemu_strtoul(p, &p, 16, &tmp);
            if (res < 0) {
                goto out;
            }
	} else if (cur_action != 'c' && cur_action != 's') {
	    res = -ENOTSUP;
	    goto out;
	}

What's the meaning of "signal" for system emulation?

> +        /* thread specification. special values: (none), -1 = all; 0 = any */
> +        if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) {
> +            for (idx = 0; idx < max_cpus; idx++) {
> +                if (newstates[idx] == 1) {
> +                    newstates[idx] = cur_action;
> +                }
> +            }
> +            if (*p == ':') {
> +                p += 3;
> +            }

Please place this "if" before the "for", to match what you do in the
"else if" case.

> +        } else if (*p == ':') {
> +            p++;
> +            if (qemu_strtoul(p, &p, 16, &tmp)) {
> +                res = -22;
> +                break;
> +            }

As suggested above, you can just assign the result of qemu_strtoul to res.

> +            idx = tmp;
> +            /* 0 means any thread, so we pick the first valid CPU */
> +            if (!idx) {
> +                CPU_FOREACH(cpu) {
> +                    idx = cpu_index(cpu);
> +                    break;
> +                }

Can you use first_cpu here perhaps?

> +            }
> +
> +            /* invalid CPU specified */
> +            if (!idx || idx > max_cpus || !newstates[idx - 1]) {
> +                res = -22;
> +                break;
> +            }
> +            /* only use if no previous match occourred */
> +            if (newstates[idx - 1] == 1) {
> +                newstates[idx - 1] = cur_action;
> +            }
> +        }
> +    }
> +    if (!res) {
> +        s->signal = signal;
> +        gdb_continue_partial(s, newstates);
> +    }
> +

This would be the placement of the "out:" label.

Thanks,

Paolo

> +    g_free(newstates);
> +
> +    return res;
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -829,60 +964,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 {
> 

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

[Claudio Imbrenda]

I applied all your suggestions


Claudio

Re: [Qemu-devel] [PATCH v3 0/2] Qemu: gdbstub: fix vCont

[Claudio Imbrenda]

Hi Paolo, I was wondering if you had seen this new version of the vCont
patchset, which I sent around last month -- is there any more work to do
or things to fix? or is it going to be upstreamed after 2.8?
(btw I just rebased it on 2.8.0-rc2 and it applied cleanly)

please don't keep me in suspense :)

thanks!


Claudio


On 28/10/16 19:15, Claudio Imbrenda wrote:
> This small patchset fixes the incorrect behaviour of the vCont command
> in the gdb stub. 
> 
> The first patch, as suggested be Paolo, refactors some code. The most
> visible change is that it moves vm_start to cpus.c 
> 
> The second one fixes the incorrect behaviour of the vCont command.
> Previously, continuing or stepping a single thread (CPU) caused all
> other CPUs to be started too, whereas the GDB specification clearly
> states that without a default action all threads not explicitly
> mentioned in the command should stay stopped.
> 
> So if the Qemu gdbstub receives a  vCont;c:1  packet, no other CPU
> should be restarted except the first, and when a  vCont;s:1  is
> received, the first CPU should be stepped without restarting the others.
> With this patchset Qemu now behaves as expected.
> 
> See here for reference material about the packets: 
> https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html
> https://sourceware.org/gdb/onlinedocs/gdb/Packets.html
> 
> v2 -> v3
> * removed resume_some_vcpus
> * cleared up the code and simplified the implementation in light of the 
>   clarification in the specification of the vCont packet
> 
> Claudio Imbrenda (2):
>   move vm_start to cpus.c
>   gdbstub: Fix vCont behaviour
> 
>  cpus.c                     |  44 ++++++++++-
>  gdbstub.c                  | 189 ++++++++++++++++++++++++++++++++++-----------
>  hw/i386/kvmvapic.c         |   2 +
>  include/sysemu/cpus.h      |   1 +
>  include/sysemu/sysemu.h    |   2 +
>  target-s390x/misc_helper.c |   2 +
>  vl.c                       |  32 +-------
>  7 files changed, 195 insertions(+), 77 deletions(-)
> 

Re: [Qemu-devel] [PATCH v3 0/2] Qemu: gdbstub: fix vCont

[Claudio Imbrenda]

Hello,

just a respectful poke to remind about this patchset :)

Claudio

Re: [Qemu-devel] [PATCH v3 0/2] Qemu: gdbstub: fix vCont

[Christian Borntraeger]

On 01/12/2017 05:45 PM, Claudio Imbrenda wrote:
> Hello,
> 
> just a respectful poke to remind about this patchset :)

Can you maybe resend/rebase the patch set? The target-s390x folder has moved.

Re: [Qemu-devel] [PATCH v3 0/2] Qemu: gdbstub: fix vCont

[Paolo Bonzini]

On 25/01/2017 11:05, Christian Borntraeger wrote:
> On 01/12/2017 05:45 PM, Claudio Imbrenda wrote:
>> Hello,
>>
>> just a respectful poke to remind about this patchset :)
> 
> Can you maybe resend/rebase the patch set? The target-s390x folder has moved.

I was reviewing it now. :)

Paolo