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

[Qemu-devel] [PATCH v2 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/onlinedocs/gdb/Packets.html

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

 cpus.c                     |  61 +++++++++++-
 gdbstub.c                  | 226 +++++++++++++++++++++++++++++++++++----------
 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, 249 insertions(+), 77 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 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.
* adds resume_some_vcpus, to selectively restart only some CPUs.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
---
 cpus.c                     | 61 +++++++++++++++++++++++++++++++++++++++++++++-
 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, 70 insertions(+), 30 deletions(-)

diff --git a/cpus.c b/cpus.c
index 31204bb..c102624 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1260,12 +1260,28 @@ void resume_all_vcpus(void)
 {
     CPUState *cpu;
 
-    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
     CPU_FOREACH(cpu) {
         cpu_resume(cpu);
     }
 }
 
+/**
+ * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated
+ * array of CPUState *; if the parameter is NULL, all CPUs are resumed.
+ */
+void resume_some_vcpus(CPUState **cpus)
+{
+    int idx;
+
+    if (!cpus) {
+        resume_all_vcpus();
+        return;
+    }
+    for (idx = 0; cpus[idx]; idx++) {
+        cpu_resume(cpus[idx]);
+    }
+}
+
 void cpu_remove(CPUState *cpu)
 {
     cpu->stop = true;
@@ -1396,6 +1412,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 ef2c50b..ac301d6 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 f3abd99..42addbb 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 */
 
@@ -1910,6 +1882,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)) {
@@ -1921,6 +1894,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 v2 1/2] move vm_start to cpus.c

[Christian Borntraeger]

On 10/14/2016 01:53 PM, Claudio Imbrenda wrote:
> 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.
> * adds resume_some_vcpus, to selectively restart only some CPUs.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>

CCing Paolo. (please also have a look at patch2)

> ---
>  cpus.c                     | 61 +++++++++++++++++++++++++++++++++++++++++++++-
>  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, 70 insertions(+), 30 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 31204bb..c102624 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1260,12 +1260,28 @@ void resume_all_vcpus(void)
>  {
>      CPUState *cpu;
> 
> -    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>      CPU_FOREACH(cpu) {
>          cpu_resume(cpu);
>      }
>  }
> 
> +/**
> + * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated
> + * array of CPUState *; if the parameter is NULL, all CPUs are resumed.
> + */
> +void resume_some_vcpus(CPUState **cpus)
> +{
> +    int idx;
> +
> +    if (!cpus) {
> +        resume_all_vcpus();
> +        return;
> +    }
> +    for (idx = 0; cpus[idx]; idx++) {
> +        cpu_resume(cpus[idx]);
> +    }
> +}
> +
>  void cpu_remove(CPUState *cpu)
>  {
>      cpu->stop = true;
> @@ -1396,6 +1412,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 ef2c50b..ac301d6 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 f3abd99..42addbb 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 */
> 
> @@ -1910,6 +1882,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)) {
> @@ -1921,6 +1894,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);
>      }
> 

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

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> 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.

Can you be a bit more explicit about this? Shouldn't CPU time still
accrue even if you are only starting some of them?

I'd prefer making resume_all_vcpus() a private function called from
resume_some_vcpus() which can then make the QEMU_CLOCK_VIRTUAL decision
in one place - with a comment.

> * adds resume_some_vcpus, to selectively restart only some CPUs.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
> ---
>  cpus.c                     | 61 +++++++++++++++++++++++++++++++++++++++++++++-
>  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, 70 insertions(+), 30 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 31204bb..c102624 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1260,12 +1260,28 @@ void resume_all_vcpus(void)
>  {
>      CPUState *cpu;
>
> -    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>      CPU_FOREACH(cpu) {
>          cpu_resume(cpu);
>      }
>  }
>
> +/**
> + * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated
> + * array of CPUState *; if the parameter is NULL, all CPUs are resumed.
> + */
> +void resume_some_vcpus(CPUState **cpus)
> +{
> +    int idx;
> +
> +    if (!cpus) {
> +        resume_all_vcpus();
> +        return;
> +    }
> +    for (idx = 0; cpus[idx]; idx++) {
> +        cpu_resume(cpus[idx]);
> +    }
> +}
> +
>  void cpu_remove(CPUState *cpu)
>  {
>      cpu->stop = true;
> @@ -1396,6 +1412,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 ef2c50b..ac301d6 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 f3abd99..42addbb 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 */
>
> @@ -1910,6 +1882,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)) {
> @@ -1921,6 +1894,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);
>      }


--
Alex Bennée

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

[Claudio Imbrenda]

On 27/01/17 17:31, Alex Bennée wrote:
> 
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:
> 
>> 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.
> 
> Can you be a bit more explicit about this? Shouldn't CPU time still
> accrue even if you are only starting some of them?

This is what's happening in the newest version of this patch, which I
sent around yesterday, although I forgot to update the patch
description; I'll send a fixed version ASAP.

> I'd prefer making resume_all_vcpus() a private function called from

resume_all_vcpus is already being used in other files too, doesn't make
sense to make it private now.

> resume_some_vcpus() which can then make the QEMU_CLOCK_VIRTUAL decision
> in one place - with a comment.
> 
>> * adds resume_some_vcpus, to selectively restart only some CPUs.
>>
>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>> ---
>>  cpus.c                     | 61 +++++++++++++++++++++++++++++++++++++++++++++-
>>  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, 70 insertions(+), 30 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 31204bb..c102624 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -1260,12 +1260,28 @@ void resume_all_vcpus(void)
>>  {
>>      CPUState *cpu;
>>
>> -    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>>      CPU_FOREACH(cpu) {
>>          cpu_resume(cpu);
>>      }
>>  }
>>
>> +/**
>> + * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated
>> + * array of CPUState *; if the parameter is NULL, all CPUs are resumed.
>> + */
>> +void resume_some_vcpus(CPUState **cpus)
>> +{
>> +    int idx;
>> +
>> +    if (!cpus) {
>> +        resume_all_vcpus();
>> +        return;
>> +    }
>> +    for (idx = 0; cpus[idx]; idx++) {
>> +        cpu_resume(cpus[idx]);
>> +    }
>> +}
>> +
>>  void cpu_remove(CPUState *cpu)
>>  {
>>      cpu->stop = true;
>> @@ -1396,6 +1412,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 ef2c50b..ac301d6 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 f3abd99..42addbb 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 */
>>
>> @@ -1910,6 +1882,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)) {
>> @@ -1921,6 +1894,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);
>>      }
> 
> 
> --
> Alex Bennée
> 

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

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On 27/01/17 17:31, Alex Bennée wrote:
>>
>> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:
>>
>>> 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.
>>
>> Can you be a bit more explicit about this? Shouldn't CPU time still
>> accrue even if you are only starting some of them?
>
> This is what's happening in the newest version of this patch, which I
> sent around yesterday, although I forgot to update the patch
> description; I'll send a fixed version ASAP.
>
>> I'd prefer making resume_all_vcpus() a private function called from
>
> resume_all_vcpus is already being used in other files too, doesn't make
> sense to make it private now.

Yeah I would rename the call-sites across QEMU. Perhaps just one entry
point of rename_vcpus() which does the right thing given a list or NULL.
But pushing the details of restarting the timer to the call sites just
sounds like a way of it getting missed next time someone adds a resume
somewhere.

>
>> resume_some_vcpus() which can then make the QEMU_CLOCK_VIRTUAL decision
>> in one place - with a comment.
>>
>>> * adds resume_some_vcpus, to selectively restart only some CPUs.
>>>
>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>>> ---
>>>  cpus.c                     | 61 +++++++++++++++++++++++++++++++++++++++++++++-
>>>  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, 70 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 31204bb..c102624 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -1260,12 +1260,28 @@ void resume_all_vcpus(void)
>>>  {
>>>      CPUState *cpu;
>>>
>>> -    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>>>      CPU_FOREACH(cpu) {
>>>          cpu_resume(cpu);
>>>      }
>>>  }
>>>
>>> +/**
>>> + * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated
>>> + * array of CPUState *; if the parameter is NULL, all CPUs are resumed.
>>> + */
>>> +void resume_some_vcpus(CPUState **cpus)
>>> +{
>>> +    int idx;
>>> +
>>> +    if (!cpus) {
>>> +        resume_all_vcpus();
>>> +        return;
>>> +    }
>>> +    for (idx = 0; cpus[idx]; idx++) {
>>> +        cpu_resume(cpus[idx]);
>>> +    }
>>> +}
>>> +
>>>  void cpu_remove(CPUState *cpu)
>>>  {
>>>      cpu->stop = true;
>>> @@ -1396,6 +1412,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 ef2c50b..ac301d6 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 f3abd99..42addbb 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 */
>>>
>>> @@ -1910,6 +1882,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)) {
>>> @@ -1921,6 +1894,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);
>>>      }
>>
>>
>> --
>> Alex Bennée
>>


--
Alex Bennée

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

[Claudio Imbrenda]

On 27/01/17 18:05, Alex Bennée wrote:
> 
> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:
> 
>> On 27/01/17 17:31, Alex Bennée wrote:
>>>
>>> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:
>>>
>>>> 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.
>>>
>>> Can you be a bit more explicit about this? Shouldn't CPU time still
>>> accrue even if you are only starting some of them?
>>
>> This is what's happening in the newest version of this patch, which I
>> sent around yesterday, although I forgot to update the patch
>> description; I'll send a fixed version ASAP.
>>
>>> I'd prefer making resume_all_vcpus() a private function called from
>>
>> resume_all_vcpus is already being used in other files too, doesn't make
>> sense to make it private now.
> 
> Yeah I would rename the call-sites across QEMU. Perhaps just one entry
> point of rename_vcpus() which does the right thing given a list or NULL.
> But pushing the details of restarting the timer to the call sites just

this is not happening any longer in the patch I sent yesterday (v6).
the only thing happening now in the first patch is moving vm_start and
splitting it. clocks are not affected, no further code changes are needed.

> sounds like a way of it getting missed next time someone adds a resume
> somewhere.
> 
>>
>>> resume_some_vcpus() which can then make the QEMU_CLOCK_VIRTUAL decision
>>> in one place - with a comment.
>>>
>>>> * adds resume_some_vcpus, to selectively restart only some CPUs.
>>>>
>>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
>>>> ---
>>>>  cpus.c                     | 61 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>  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, 70 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/cpus.c b/cpus.c
>>>> index 31204bb..c102624 100644
>>>> --- a/cpus.c
>>>> +++ b/cpus.c
>>>> @@ -1260,12 +1260,28 @@ void resume_all_vcpus(void)
>>>>  {
>>>>      CPUState *cpu;
>>>>
>>>> -    qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
>>>>      CPU_FOREACH(cpu) {
>>>>          cpu_resume(cpu);
>>>>      }
>>>>  }
>>>>
>>>> +/**
>>>> + * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated
>>>> + * array of CPUState *; if the parameter is NULL, all CPUs are resumed.
>>>> + */
>>>> +void resume_some_vcpus(CPUState **cpus)
>>>> +{
>>>> +    int idx;
>>>> +
>>>> +    if (!cpus) {
>>>> +        resume_all_vcpus();
>>>> +        return;
>>>> +    }
>>>> +    for (idx = 0; cpus[idx]; idx++) {
>>>> +        cpu_resume(cpus[idx]);
>>>> +    }
>>>> +}
>>>> +
>>>>  void cpu_remove(CPUState *cpu)
>>>>  {
>>>>      cpu->stop = true;
>>>> @@ -1396,6 +1412,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 ef2c50b..ac301d6 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 f3abd99..42addbb 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 */
>>>>
>>>> @@ -1910,6 +1882,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)) {
>>>> @@ -1921,6 +1894,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);
>>>>      }
>>>
>>>
>>> --
>>> Alex Bennée
>>>
> 
> 
> --
> Alex Bennée
> 

[Qemu-devel] [PATCH v2 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 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 <imbrenda@linux.vnet.ibm.com>
---
 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);
+        }
+    }
+}
+
+/*
+ * 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 {
-- 
1.9.1

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

[Christian Borntraeger]

On 10/14/2016 01:53 PM, 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:
> * 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 <imbrenda@linux.vnet.ibm.com>

Any gdbstub expert in the house?

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

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

[Pedro Alves]

On 10/26/2016 08:56 AM, Christian Borntraeger wrote:
> On 10/14/2016 01:53 PM, 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.

Or more accurately, to leave it stopped as it was.

>> However, when (for example) "vCont;s:2" is issued, all VCPUs continue
>> to run, although all but VCPU nr 2 are to be stopped.

> 
> Any gdbstub expert in the house?

I'm not a qemu gdbstub expert, but FYI, seeing this reminded me to push
to gdb's master a (getting old) gdb patch that clarifies how vCont actions
should be interpreted:

 https://sourceware.org/ml/gdb-patches/2016-02/msg00493.html

It's already live at:

 https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html

(The "already running" case is for non-stop mode, which qemu
probably doesn't support.)

Thanks,
Pedro Alves

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

[Claudio Imbrenda]

On 27/10/16 13:40, Pedro Alves wrote:
> I'm not a qemu gdbstub expert, but FYI, seeing this reminded me to push
> to gdb's master a (getting old) gdb patch that clarifies how vCont actions
> should be interpreted:
> 
>  https://sourceware.org/ml/gdb-patches/2016-02/msg00493.html
> 
> It's already live at:
> 
>  https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html
> 
> (The "already running" case is for non-stop mode, which qemu
> probably doesn't support.)

>From the new specifications I seem to understand that if I specify a
default as first action, then no further actions will be considered,
since it matches all threads?

e.g. vCont;c;s:1  will ignore the s:1 because the c was specified
earlier and it matches all threads, and therefore it's the leftmost
match? do I understand correctly?

Or does the default only match any thread without an explicit match?
(which is how I interpreted the old spec)

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

[Pedro Alves]

On 10/28/2016 02:35 PM, Claudio Imbrenda wrote:
> On 27/10/16 13:40, Pedro Alves wrote:
>> I'm not a qemu gdbstub expert, but FYI, seeing this reminded me to push
>> to gdb's master a (getting old) gdb patch that clarifies how vCont actions
>> should be interpreted:
>>
>>  https://sourceware.org/ml/gdb-patches/2016-02/msg00493.html
>>
>> It's already live at:
>>
>>  https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html
>>
>> (The "already running" case is for non-stop mode, which qemu
>> probably doesn't support.)
> 
> From the new specifications I seem to understand that if I specify a
> default as first action, then no further actions will be considered,
> since it matches all threads?

Right.

> 
> e.g. vCont;c;s:1  will ignore the s:1 because the c was specified
> earlier and it matches all threads, and therefore it's the leftmost
> match? do I understand correctly?

Yes.

Note that GDB never ever sent a vCont like that.  It's always put
the default action last.  So in practice, it doesn't change
any behavior.  OTOH, stubs can be simpler, given there's no need to
special-case the default action.

(Also, with this definition we can safely break a big vCont packet
in multiple smaller ones, in non-stop mode:

 vCont;s:1;c == vCont;s:1 + vCont;c
 vCont;c;s:1 == vCont;c + vCont;s:1
 vCont;s:1;s:2;s:3;s:3;s:4;...s:N;c == vCont;s:1;s:2;s:3 + vCont;s:4;...s:N;c

 etc.
)

> 
> Or does the default only match any thread without an explicit match?
> (which is how I interpreted the old spec)

Maybe I should just remove all mentions of a "default" from the text?

Like:

diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo
index d636a16..df548dc 100644
--- c/gdb/doc/gdb.texinfo
+++ w/gdb/doc/gdb.texinfo
@@ -35538,9 +35538,8 @@ syntax described in @ref{thread-id syntax}.  If multiprocess
 extensions (@pxref{multiprocess extensions}) are supported, actions
 can be specified to match all threads in a process by using the
 @samp{p@var{pid}.-1} form of the @var{thread-id}.  An action with no
-@var{thread-id} is called the default action and matches all threads.
-Specifying multiple default actions is an error; specifying no actions
-is also an error.
+@var{thread-id} matches all threads.  Specifying no actions is an
+error.

I.e., go from this:

~~~
For each inferior thread, the leftmost action with a matching
thread-id is applied. Threads that don’t match any action remain in
their current state. Thread IDs are specified using the syntax
described in thread-id syntax. If multiprocess extensions (see
multiprocess extensions) are supported, actions can be specified to
match all threads in a process by using the ‘ppid.-1’ form of the
thread-id. An action with no thread-id is called the default action
and matches all threads. Specifying multiple default actions is an
error; specifying no actions is also an error.
~~~

To this:

~~~
For each inferior thread, the leftmost action with a matching
thread-id is applied. Threads that don’t match any action remain in
their current state. Thread IDs are specified using the syntax
described in thread-id syntax. If multiprocess extensions (see
multiprocess extensions) are supported, actions can be specified to
match all threads in a process by using the ‘ppid.-1’ form of the
thread-id. An action with no thread-id matches all threads. Specifying
no actions is an error.
~~~

Would that help ?

Thanks,
Pedro Alves

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

[Claudio Imbrenda]

On 28/10/16 16:01, Pedro Alves wrote:
>> From the new specifications I seem to understand that if I specify a
>> default as first action, then no further actions will be considered,
>> since it matches all threads?
> 
> Right.

ok, that's the way I understand the new specs, which is not how I had
understood the old specs (hence my questions), so I think your
documentation patch was really needed.

> Note that GDB never ever sent a vCont like that.  It's always put
> the default action last.  So in practice, it doesn't change
> any behavior.  OTOH, stubs can be simpler, given there's no need to
> special-case the default action.

ok, I admit I never looked deeply into what GDB sends :)

> (Also, with this definition we can safely break a big vCont packet
> in multiple smaller ones, in non-stop mode:
> 
>  vCont;s:1;c == vCont;s:1 + vCont;c
>  vCont;c;s:1 == vCont;c + vCont;s:1
>  vCont;s:1;s:2;s:3;s:3;s:4;...s:N;c == vCont;s:1;s:2;s:3 + vCont;s:4;...s:N;c

that makes sense, although I never looked into it since Qemu doesn't
support non-stop mode

>> Or does the default only match any thread without an explicit match?
>> (which is how I interpreted the old spec)
> 
> Maybe I should just remove all mentions of a "default" from the text?

[..]

> Like:
> 
> diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo
> index d636a16..df548dc 100644
> --- c/gdb/doc/gdb.texinfo
> +++ w/gdb/doc/gdb.texinfo
> @@ -35538,9 +35538,8 @@ syntax described in @ref{thread-id syntax}.  If multiprocess
>  extensions (@pxref{multiprocess extensions}) are supported, actions
>  can be specified to match all threads in a process by using the
>  @samp{p@var{pid}.-1} form of the @var{thread-id}.  An action with no
> -@var{thread-id} is called the default action and matches all threads.
> -Specifying multiple default actions is an error; specifying no actions
> -is also an error.
> +@var{thread-id} matches all threads.  Specifying no actions is an
> +error.

> Would that help ?

that makes it even more clear (and simple).
On the other hand, now expressing more than one default is not
considered an error any longer (although the extra defaults serve no
purpose); in the end it wouldn't break backwards compatibility, so why not.

Basically a "default" e.g. "s" is the same as specifying "s:-1". This
does make the implementation of the stub easier.


thanks a lot for your prompt and detailed reply!

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

[Pedro Alves]

Closing the loop here:

On 10/28/2016 03:25 PM, Claudio Imbrenda wrote:

>> Maybe I should just remove all mentions of a "default" from the text?

...

>> Would that help ?
> 
> that makes it even more clear (and simple).
> On the other hand, now expressing more than one default is not
> considered an error any longer (although the extra defaults serve no
> purpose); in the end it wouldn't break backwards compatibility, so why not.
> 
> Basically a "default" e.g. "s" is the same as specifying "s:-1". This
> does make the implementation of the stub easier.

That gdb doc simplification is in master now, and live at the same url.

> thanks a lot for your prompt and detailed reply!

Np!  Thank you too.

-- 
Pedro Alves

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

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> 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 <imbrenda@linux.vnet.ibm.com>
> ---
>  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);
> +        }
> +    }
> +}
> +
> +/*
> + * 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);

FWIW glib also provides a g_ptr_array_new() which may marginally cut
down on your boiler plate.

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

It would be nice if the parse function could be teased out a bit more so
we could add some unit tests, especially if GDB packets are going to
continue to become more complex.

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

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

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> 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 <imbrenda@linux.vnet.ibm.com>
> ---
>  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]

> +
> +/*
> + * 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

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 <imbrenda@linux.vnet.ibm.com> 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 <imbrenda@linux.vnet.ibm.com>
>> ---
>>  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
> 

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

[Alex Bennée]

Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On 27/01/17 18:07, Alex Bennée wrote:
>>
>> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> 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 <imbrenda@linux.vnet.ibm.com>
>>> ---
>>>  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