[Qemu-devel] [PATCH v1 0/2] Qemu: gdbstub: fix vCont and single-step

[Qemu-devel] [PATCH v1 0/2] Qemu: gdbstub: fix vCont and single-step

[Claudio Imbrenda]

This small patchset fixes two bugs that affect the gdb stub.

The first one is 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/onlinedocs/gdb/Packets.html

The second bug causes single-step mode not to work any longer, it was
introduced with commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 . This
bug causes all s (single-step) commands to behave like c (continue)
commands.

Claudio Imbrenda (2):
  gdbstub: Fix single-step
  gdbstub: Fix vCont behaviour

 gdbstub.c | 189 +++++++++++++++++++++++++++++++++++++++++++++++++-------------
 kvm-all.c |   1 +
 2 files changed, 151 insertions(+), 39 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v1 1/2] gdbstub: Fix single-step

[Claudio Imbrenda]

Commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 introduced a bug that
causes single-step in the gdbstub to not work, at least in kvm. CPUs
that are supposed to single-step will instead run normally.

This small patch fixes the problem. (tested on s390x)

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 kvm-all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kvm-all.c b/kvm-all.c
index efb5fe3..46b8dcd 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2237,6 +2237,7 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
         data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
     }
     kvm_arch_update_guest_debug(cpu, &data.dbg);
+    data.cpu = cpu;
 
     run_on_cpu(cpu, kvm_invoke_set_guest_debug, &data);
     return data.err;
-- 
1.9.1

[Qemu-devel] [PATCH v1 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:
* adds an additional helper function to selectively restart only some
  CPUs
* completely rewrites the vCont parsing code

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 gdbstub.c | 189 +++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 150 insertions(+), 39 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index ecea8c4..0ce9c83 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -386,6 +386,65 @@ static inline void gdb_continue(GDBState *s)
 #endif
 }
 
+/* Resume execution, per CPU actions. */
+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
+    CPUState *cpu;
+    int cx;
+
+    if (!runstate_needs_reset()) {
+        /*
+         * XXX vm_start also calls qemu_vmstop_requested(&requested); here, is
+         * it actually important? it's static in vl.c
+         */
+        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); */
+    }
+#endif
+    return res;
+}
+
 static void put_buffer(GDBState *s, const uint8_t *buf, int len)
 {
 #ifdef CONFIG_USER_ONLY
@@ -829,62 +888,114 @@ 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;
+            CPUState **s_cpus, **c_cpus;
+            CPUState *curcpu;
+            int idx, signal, broadcast_action = 0;
+            int scnt, ccnt;
+            char default_action = 0;
+            char cur_action = 0;
+            unsigned long tmp;
 
             p += 4;
             if (*p == '?') {
                 put_packet(s, "vCont;c;C;s;S");
                 break;
             }
+            if (*p != ';') {
+                goto unknown_command;
+            }
+
+            for (idx = scnt = 0; p[idx]; idx++) {
+                if (p[idx] == ';') {
+                    scnt++;
+                }
+            }
+            s_cpus = g_malloc((scnt + 1) * sizeof(*s_cpus));
+            c_cpus = g_malloc((scnt + 1) * sizeof(*c_cpus));
+            scnt = ccnt = 0;
+            signal = 0;
+
+            /*
+             * For us: vCont[;action[:thread-id]]...
+             * where action can be one of c s C<sig> S<sig>
+             */
             res = 0;
-            res_signal = 0;
-            res_thread = 0;
             while (*p) {
-                int action, signal;
-
-                if (*p++ != ';') {
-                    res = 0;
+                if (*p != ';') {
+                    res = -1;
                     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;
+                p++; /* skip the ; */
+
+                if (*p == 'C' || *p == 'S' || *p == 'c' || *p == 's') {
+                    cur_action = *p;
+                    if (*p == 'C' || *p == 'S') {
+                        (void) qemu_strtoul(p + 1, &p, 16, &tmp);
+                        signal = gdb_signal_to_target(tmp);
+                    } else {
+                        p++;
+                    }
+                    /* thread specified. special values: -1 = all, 0 = any */
+                    if (*p == ':') {
+                        if (broadcast_action) {
+                            res = -22;
+                            break;
+                        }
+                        p++;
+                        if ((p[0] == '-') && (p[1] == '1')) {
+                            if (broadcast_action || scnt || ccnt) {
+                                res = -22;
+                                break;
+                            }
+                            broadcast_action = cur_action;
+                            p += 2;
+                            continue;
+                        }
+                        (void) qemu_strtoul(p, &p, 16, &tmp);
+                        idx = tmp;
+                        /* 0 means any thread, so we pick the first */
+                        idx = idx ? idx : 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 */
+                            break;
+                        }
+                        default_action = cur_action;
                     }
-                } else if (action != 'c' && action != 's') {
-                    res = 0;
+                } else { /* unknown/invalid/unsupported command */
+                    res = -1;
                     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;
-                }
             }
             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;
+                g_free(s_cpus);
+                g_free(c_cpus);
+                if (res == -1) {
+                    goto unknown_command;
                 }
-                if (res == 's') {
-                    cpu_single_step(s->c_cpu, sstep_flags);
-                }
-                s->signal = res_signal;
-                gdb_continue(s);
-                return RS_IDLE;
+                put_packet(s, "E22");
+                break;
             }
-            break;
+            s->signal = signal;
+            s_cpus[scnt] = c_cpus[ccnt] = NULL;
+            if (broadcast_action) {
+                gdb_continue_partial(s, broadcast_action, NULL, NULL);
+            } else {
+                gdb_continue_partial(s, default_action, s_cpus, c_cpus);
+            }
+            g_free(s_cpus);
+            g_free(c_cpus);
+            return RS_IDLE;
         } else {
             goto unknown_command;
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 1/2] gdbstub: Fix single-step

[Christian Borntraeger]

On 10/10/2016 01:50 PM, Claudio Imbrenda wrote:
> Commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 introduced a bug that
> causes single-step in the gdbstub to not work, at least in kvm. CPUs
> that are supposed to single-step will instead run normally.
> 
> This small patch fixes the problem. (tested on s390x)
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> ---
>  kvm-all.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index efb5fe3..46b8dcd 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2237,6 +2237,7 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>          data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>      }
>      kvm_arch_update_guest_debug(cpu, &data.dbg);
> +    data.cpu = cpu;
> 
>      run_on_cpu(cpu, kvm_invoke_set_guest_debug, &data);
>      return data.err;
> 

Adding Alex and Paolo to cc.

Re: [Qemu-devel] [PATCH v1 1/2] gdbstub: Fix single-step

[Paolo Bonzini]

On 10/10/2016 14:51, Christian Borntraeger wrote:
> On 10/10/2016 01:50 PM, Claudio Imbrenda wrote:
>> Commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 introduced a bug that
>> causes single-step in the gdbstub to not work, at least in kvm. CPUs
>> that are supposed to single-step will instead run normally.
>>
>> This small patch fixes the problem. (tested on s390x)
>>
>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>> ---
>>  kvm-all.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index efb5fe3..46b8dcd 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -2237,6 +2237,7 @@ int kvm_update_guest_debug(CPUState *cpu, unsigned long reinject_trap)
>>          data.dbg.control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
>>      }
>>      kvm_arch_update_guest_debug(cpu, &data.dbg);
>> +    data.cpu = cpu;
>>
>>      run_on_cpu(cpu, kvm_invoke_set_guest_debug, &data);
>>      return data.err;
>>
> 
> Adding Alex and Paolo to cc.

The patch works but it also makes sense to use the new CPUState*
argument of kvm_invoke_set_guest_debug.  This way the
kvm_set_guest_debug_data struct doesn't need the "cpu" field anymore.

Paolo

[PATCH] kvm-all: don't use stale dbg_data->cpu

[Alex Bennée]

The changes to run_on_cpu and friends mean that all helpers are passed
the CPUState of vCPU they are running on. The conversion missed the
field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
introduced bugs.

Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 kvm-all.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index efb5fe3..3dcce16 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
 
 struct kvm_set_guest_debug_data {
     struct kvm_guest_debug dbg;
-    CPUState *cpu;
     int err;
 };
 
-static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
+static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
 {
     struct kvm_set_guest_debug_data *dbg_data = data;
 
-    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
+    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
                                    &dbg_data->dbg);
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH] kvm-all: don't use stale dbg_data->cpu

[Alex Bennée]

The changes to run_on_cpu and friends mean that all helpers are passed
the CPUState of vCPU they are running on. The conversion missed the
field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
introduced bugs.

Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 kvm-all.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index efb5fe3..3dcce16 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
 
 struct kvm_set_guest_debug_data {
     struct kvm_guest_debug dbg;
-    CPUState *cpu;
     int err;
 };
 
-static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
+static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
 {
     struct kvm_set_guest_debug_data *dbg_data = data;
 
-    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
+    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
                                    &dbg_data->dbg);
 }
 
-- 
2.9.3

Re: [PATCH] kvm-all: don't use stale dbg_data->cpu

[Paolo Bonzini]

On 10/10/2016 17:46, Alex Bennée wrote:
> The changes to run_on_cpu and friends mean that all helpers are passed
> the CPUState of vCPU they are running on. The conversion missed the
> field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
> introduced bugs.
> 
> Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  kvm-all.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index efb5fe3..3dcce16 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
>  
>  struct kvm_set_guest_debug_data {
>      struct kvm_guest_debug dbg;
> -    CPUState *cpu;
>      int err;
>  };
>  
> -static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
> +static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
>  {
>      struct kvm_set_guest_debug_data *dbg_data = data;
>  
> -    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
> +    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
>                                     &dbg_data->dbg);
>  }
>  
> 

Queued, thanks - or if Christian wants to send a pull request himself,
he can go ahead.

Paolo

Re: [Qemu-devel] [PATCH] kvm-all: don't use stale dbg_data->cpu

[Paolo Bonzini]

On 10/10/2016 17:46, Alex Bennée wrote:
> The changes to run_on_cpu and friends mean that all helpers are passed
> the CPUState of vCPU they are running on. The conversion missed the
> field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
> introduced bugs.
> 
> Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  kvm-all.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index efb5fe3..3dcce16 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
>  
>  struct kvm_set_guest_debug_data {
>      struct kvm_guest_debug dbg;
> -    CPUState *cpu;
>      int err;
>  };
>  
> -static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
> +static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
>  {
>      struct kvm_set_guest_debug_data *dbg_data = data;
>  
> -    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
> +    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
>                                     &dbg_data->dbg);
>  }
>  
> 

Queued, thanks - or if Christian wants to send a pull request himself,
he can go ahead.

Paolo

Re: [PATCH] kvm-all: don't use stale dbg_data->cpu

[Christian Borntraeger]

On 10/10/2016 06:39 PM, Paolo Bonzini wrote:
> 
> 
> On 10/10/2016 17:46, Alex Bennée wrote:
>> The changes to run_on_cpu and friends mean that all helpers are passed
>> the CPUState of vCPU they are running on. The conversion missed the
>> field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
>> introduced bugs.
>>
>> Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  kvm-all.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index efb5fe3..3dcce16 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
>>  
>>  struct kvm_set_guest_debug_data {
>>      struct kvm_guest_debug dbg;
>> -    CPUState *cpu;
>>      int err;
>>  };
>>  
>> -static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
>> +static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
>>  {
>>      struct kvm_set_guest_debug_data *dbg_data = data;
>>  
>> -    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
>> +    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
>>                                     &dbg_data->dbg);
>>  }
>>  
>>
> 
> Queued, thanks - or if Christian wants to send a pull request himself,
> he can go ahead.

Lets wait for Claudio to verify and then it can go via your tree.

PS: Can you also have a look at Claudios 2nd patch?

Re: [Qemu-devel] [PATCH] kvm-all: don't use stale dbg_data->cpu

[Christian Borntraeger]

On 10/10/2016 06:39 PM, Paolo Bonzini wrote:
> 
> 
> On 10/10/2016 17:46, Alex Bennée wrote:
>> The changes to run_on_cpu and friends mean that all helpers are passed
>> the CPUState of vCPU they are running on. The conversion missed the
>> field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
>> introduced bugs.
>>
>> Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  kvm-all.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index efb5fe3..3dcce16 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
>>  
>>  struct kvm_set_guest_debug_data {
>>      struct kvm_guest_debug dbg;
>> -    CPUState *cpu;
>>      int err;
>>  };
>>  
>> -static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
>> +static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
>>  {
>>      struct kvm_set_guest_debug_data *dbg_data = data;
>>  
>> -    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
>> +    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
>>                                     &dbg_data->dbg);
>>  }
>>  
>>
> 
> Queued, thanks - or if Christian wants to send a pull request himself,
> he can go ahead.

Lets wait for Claudio to verify and then it can go via your tree.

PS: Can you also have a look at Claudios 2nd patch?

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

[Paolo Bonzini]

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

Re: [PATCH] kvm-all: don't use stale dbg_data->cpu

[Claudio Imbrenda]

Tested-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>

On 10/10/16 17:46, Alex Bennée wrote:
> The changes to run_on_cpu and friends mean that all helpers are passed
> the CPUState of vCPU they are running on. The conversion missed the
> field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
> introduced bugs.
> 
> Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  kvm-all.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index efb5fe3..3dcce16 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
> 
>  struct kvm_set_guest_debug_data {
>      struct kvm_guest_debug dbg;
> -    CPUState *cpu;
>      int err;
>  };
> 
> -static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
> +static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
>  {
>      struct kvm_set_guest_debug_data *dbg_data = data;
> 
> -    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
> +    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
>                                     &dbg_data->dbg);
>  }
> 

Re: [Qemu-devel] [PATCH] kvm-all: don't use stale dbg_data->cpu

[Claudio Imbrenda]

Tested-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>

On 10/10/16 17:46, Alex Bennée wrote:
> The changes to run_on_cpu and friends mean that all helpers are passed
> the CPUState of vCPU they are running on. The conversion missed the
> field in commit e0eeb4a21a3ca4b296220ce4449d8acef9de9049 which
> introduced bugs.
> 
> Reported-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  kvm-all.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index efb5fe3..3dcce16 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -2215,15 +2215,14 @@ int kvm_sw_breakpoints_active(CPUState *cpu)
> 
>  struct kvm_set_guest_debug_data {
>      struct kvm_guest_debug dbg;
> -    CPUState *cpu;
>      int err;
>  };
> 
> -static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
> +static void kvm_invoke_set_guest_debug(CPUState *cpu, void *data)
>  {
>      struct kvm_set_guest_debug_data *dbg_data = data;
> 
> -    dbg_data->err = kvm_vcpu_ioctl(dbg_data->cpu, KVM_SET_GUEST_DEBUG,
> +    dbg_data->err = kvm_vcpu_ioctl(cpu, KVM_SET_GUEST_DEBUG,
>                                     &dbg_data->dbg);
>  }
> 

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

[David Hildenbrand]

> > +        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);

This looks suspicious

> > +            }
> > +            CPU_FOREACH(cpu) {
> > +                cpu_resume(cpu);
> > +            }
 
Claudio, did you have a look at how s->c_cpu is used later on? I remember that we
have to take care of some query reply packages.

David

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

[Claudio Imbrenda]

On 12/10/16 15:15, David Hildenbrand wrote:
>>> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
>>> +                cpu_single_step(cpu, 0);
> 
> This looks suspicious

why? we set all cpus to single step, since that is the default, and then
we clear the single-step property from all CPUs that should be restarted
in normal mode, then we restart all CPUs. Those in single-step will
indeed only perform one single step, the others will run freely (at
least until the first single-step CPU stops again).

>>> +            }
>>> +            CPU_FOREACH(cpu) {
>>> +                cpu_resume(cpu);
>>> +            }
> 
> Claudio, did you have a look at how s->c_cpu is used later on? I remember that we
> have to take care of some query reply packages.

yes, that's set by the H packet and used by the c,s,m,etc packets. vCont
ignores it and doesn't change it
(see here https://sourceware.org/gdb/onlinedocs/gdb/Packets.html )

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

[Paolo Bonzini]

On 12/10/2016 15:55, Claudio Imbrenda wrote:
>>>> >>> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
>>>> >>> +                cpu_single_step(cpu, 0);
>> > 
>> > This looks suspicious
> why? we set all cpus to single step, since that is the default, and then
> we clear the single-step property from all CPUs that should be restarted
> in normal mode, then we restart all CPUs. Those in single-step will
> indeed only perform one single step, the others will run freely (at
> least until the first single-step CPU stops again).
> 

Yeah, it makes sense.

Paolo

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

[David Hildenbrand]

On Wed, Oct 12, 2016 at 03:55:18PM +0200, Claudio Imbrenda wrote:
> On 12/10/16 15:15, David Hildenbrand wrote:
> >>> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
> >>> +                cpu_single_step(cpu, 0);
> > 
> > This looks suspicious
> 
> why? we set all cpus to single step, since that is the default, and then
> we clear the single-step property from all CPUs that should be restarted
> in normal mode, then we restart all CPUs. Those in single-step will
> indeed only perform one single step, the others will run freely (at
> least until the first single-step CPU stops again).

actually I was more concerned about calling it on "cpu" in a loop.

GDB will:
- single step one thread only (stopping all other)
- use vCont

as default. So this means quite some ioctls on every step with some VCPUs.
I doubt that it will really be a problem (e.g. for GDB single stepping
instead of setting breakpoints when returning froma function), but still I
want to have it said. (we actually only need 1 ioctl but call quite a lot).

> 
> >>> +            }
> >>> +            CPU_FOREACH(cpu) {
> >>> +                cpu_resume(cpu);
> >>> +            }
> > 
> > Claudio, did you have a look at how s->c_cpu is used later on? I remember that we
> > have to take care of some query reply packages.
> 
> yes, that's set by the H packet and used by the c,s,m,etc packets. vCont
> ignores it and doesn't change it
> (see here https://sourceware.org/gdb/onlinedocs/gdb/Packets.html )

I remember something different (also having to do with clients detaching and
re-attaching). Will have a look at the code when I have time.

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

[Paolo Bonzini]

----- Original Message -----
> From: "David Hildenbrand" <dahi@daveh.de>
> To: "Claudio Imbrenda" <imbrenda@linux.vnet.ibm.com>
> Cc: "David Hildenbrand" <dahi@daveh.de>, "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Sent: Wednesday, October 12, 2016 8:38:15 PM
> Subject: Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: Fix vCont behaviour
> 
> On Wed, Oct 12, 2016 at 03:55:18PM +0200, Claudio Imbrenda wrote:
> > On 12/10/16 15:15, David Hildenbrand wrote:
> > >>> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
> > >>> +                cpu_single_step(cpu, 0);
> > > 
> > > This looks suspicious
> > 
> > why? we set all cpus to single step, since that is the default, and then
> > we clear the single-step property from all CPUs that should be restarted
> > in normal mode, then we restart all CPUs. Those in single-step will
> > indeed only perform one single step, the others will run freely (at
> > least until the first single-step CPU stops again).
> 
> actually I was more concerned about calling it on "cpu" in a loop.

And we all missed that it should have been ccpus[cx], not cpu. :)

Paolo