Re: [Qemu-devel] [RFC 22/48] cpu: hook plugin vcpu events

["Alex Bennée" <alex.bennee@linaro.org>]

Emilio G. Cota <cota@braap.org> writes:

> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  cpus.c    | 10 ++++++++++
>  exec.c    |  2 ++
>  qom/cpu.c |  2 ++
>  3 files changed, 14 insertions(+)
>
> diff --git a/cpus.c b/cpus.c
> index 28e39f045a..3efe89354d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -43,6 +43,7 @@
>  #include "exec/exec-all.h"
>
>  #include "qemu/thread.h"
> +#include "qemu/plugin.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/qtest.h"
>  #include "qemu/main-loop.h"
> @@ -1322,12 +1323,21 @@ static void qemu_tcg_rr_wait_io_event(CPUState *cpu)
>
>  static void qemu_wait_io_event(CPUState *cpu)
>  {
> +    bool asleep = false;
> +

slept?

>      g_assert(cpu_mutex_locked(cpu));
>      g_assert(!qemu_mutex_iothread_locked());
>
>      while (cpu_thread_is_idle(cpu)) {
> +        if (!asleep) {
> +            asleep = true;
> +            qemu_plugin_vcpu_idle_cb(cpu);
> +        }
>          qemu_cond_wait(&cpu->halt_cond, &cpu->lock);
>      }
> +    if (asleep) {
> +        qemu_plugin_vcpu_resume_cb(cpu);
> +    }

I wonder if having two hooks is too much? What might a plugin want to do
before we go into idle sleep?

It feels like we are exposing too much of the guts of TCG to the plugin
here as wait_io could be for any number of internal reasons other than
the actual emulation blocking for IO through a WFI or something. If a
plugin really wants to track such things shouldn't it be hooking to the
guest sleep points?

If idle sleeps really are that important maybe we could just report our
sleep time on resume - so a single hook but passing a bit more
information?

>
>  #ifdef _WIN32
>      /* Eat dummy APC queued by qemu_cpu_kick_thread.  */
> diff --git a/exec.c b/exec.c
> index cd171adb93..71fc76f55e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -967,6 +967,8 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
>      }
>      tlb_init(cpu);
>
> +    qemu_plugin_vcpu_init_hook(cpu);
> +
>  #ifndef CONFIG_USER_ONLY
>      if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>          vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu);
> diff --git a/qom/cpu.c b/qom/cpu.c
> index d1e6ecae03..062817c03b 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -32,6 +32,7 @@
>  #include "hw/boards.h"
>  #include "hw/qdev-properties.h"
>  #include "trace-root.h"
> +#include "qemu/plugin.h"
>
>  CPUInterruptHandler cpu_interrupt_handler;
>
> @@ -353,6 +354,7 @@ static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
>      CPUState *cpu = CPU(dev);
>      /* NOTE: latest generic point before the cpu is fully unrealized */
>      trace_fini_vcpu(cpu);
> +    qemu_plugin_vcpu_exit_hook(cpu);
>      cpu_exec_unrealizefn(cpu);
>  }


--
Alex Bennée