Re: [PATCH v6 03/16] cpus: prepare new CpusAccel cpu accelerator interface

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> The new interface starts unused, will start being used by the
> next patches.
>
> It provides methods for each accelerator to start a vcpu, kick a vcpu,
> synchronize state, get cpu virtual clock and elapsed ticks.
>
> In qemu_wait_io_event, make it clear that APC is used only for HAX
> on Windows.
>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
<snip>
>  
>      /* signal CPU creation */
> -    cpu->created = true;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      do {
> @@ -482,8 +582,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>      } while (!cpu->unplug || cpu_can_run(cpu));
>  
>      qemu_kvm_destroy_vcpu(cpu);
> -    cpu->created = false;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_destroyed(cpu);
>      qemu_mutex_unlock_iothread();
>      rcu_unregister_thread();
>      return NULL;
> @@ -511,8 +610,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>      sigaddset(&waitset, SIG_IPI);
>  
>      /* signal CPU creation */
> -    cpu->created = true;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      do {
> @@ -660,8 +758,7 @@ static void deal_with_unplugged_cpus(void)
>      CPU_FOREACH(cpu) {
>          if (cpu->unplug && !cpu_can_run(cpu)) {
>              qemu_tcg_destroy_vcpu(cpu);
> -            cpu->created = false;
> -            qemu_cond_signal(&qemu_cpu_cond);
> +            cpu_thread_signal_destroyed(cpu);
>              break;
>          }
>      }
> @@ -688,9 +785,8 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
>      qemu_thread_get_self(cpu->thread);
>  
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu->created = true;
>      cpu->can_do_io = 1;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      /* wait for initial kick-off after machine start */
> @@ -800,11 +896,9 @@ static void *qemu_hax_cpu_thread_fn(void *arg)
>      qemu_thread_get_self(cpu->thread);
>  
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu->created = true;
>      current_cpu = cpu;
> -
>      hax_init_vcpu(cpu);
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      do {
> @@ -843,8 +937,7 @@ static void *qemu_hvf_cpu_thread_fn(void *arg)
>      hvf_init_vcpu(cpu);
>  
>      /* signal CPU creation */
> -    cpu->created = true;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      do {
> @@ -858,8 +951,7 @@ static void *qemu_hvf_cpu_thread_fn(void *arg)
>      } while (!cpu->unplug || cpu_can_run(cpu));
>  
>      hvf_vcpu_destroy(cpu);
> -    cpu->created = false;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_destroyed(cpu);
>      qemu_mutex_unlock_iothread();
>      rcu_unregister_thread();
>      return NULL;
> @@ -884,8 +976,7 @@ static void *qemu_whpx_cpu_thread_fn(void *arg)
>      }
>  
>      /* signal CPU creation */
> -    cpu->created = true;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      do {
> @@ -902,8 +993,7 @@ static void *qemu_whpx_cpu_thread_fn(void *arg)
>      } while (!cpu->unplug || cpu_can_run(cpu));
>  
>      whpx_destroy_vcpu(cpu);
> -    cpu->created = false;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_destroyed(cpu);
>      qemu_mutex_unlock_iothread();
>      rcu_unregister_thread();
>      return NULL;
> @@ -936,10 +1026,9 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>      qemu_thread_get_self(cpu->thread);
>  
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu->created = true;
>      cpu->can_do_io = 1;
>      current_cpu = cpu;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_created(cpu);
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      /* process any pending work */
> @@ -980,14 +1069,13 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>      } while (!cpu->unplug || cpu_can_run(cpu));
>  
>      qemu_tcg_destroy_vcpu(cpu);
> -    cpu->created = false;
> -    qemu_cond_signal(&qemu_cpu_cond);
> +    cpu_thread_signal_destroyed(cpu);
>      qemu_mutex_unlock_iothread();
>      rcu_unregister_thread();
>      return NULL;

I suspect this clean-up could be a separate patch.

<snip>
>  
> +/* signal CPU creation */
> +void cpu_thread_signal_created(CPUState *cpu)
> +{
> +    cpu->created = true;
> +    qemu_cond_signal(&qemu_cpu_cond);
> +}
> +
> +/* signal CPU destruction */
> +void cpu_thread_signal_destroyed(CPUState *cpu)
> +{
> +    cpu->created = false;
> +    qemu_cond_signal(&qemu_cpu_cond);
> +}
> +
> +
>  static bool all_vcpus_paused(void)
>  {
>      CPUState *cpu;
> @@ -1163,9 +1269,6 @@ void cpu_remove_sync(CPUState *cpu)
>      qemu_mutex_lock_iothread();
>  }
>  
> -/* For temporary buffers for forming a name */
> -#define VCPU_THREAD_NAME_SIZE 16
> -

Lets kill this rather than move it around. The thread functions could
more cleanly do:

    g_autoptr(GString) thread_name = g_string_new(NULL);
    ...
    g_string_printf(thread_name, "CPU %d/DUMMY", cpu->cpu_index);
    qemu_thread_create(..., thread_name->str, ...);


>  static void qemu_tcg_init_vcpu(CPUState *cpu)
>  {
>      char thread_name[VCPU_THREAD_NAME_SIZE];
> @@ -1286,6 +1389,13 @@ static void qemu_whpx_start_vcpu(CPUState *cpu)
>  #endif
>  }
>  
> +void cpus_register_accel(const CpusAccel *ca)
> +{
> +    assert(ca != NULL);
> +    assert(ca->create_vcpu_thread != NULL); /* mandatory */
> +    cpus_accel = ca;
> +}
> +
>  static void qemu_dummy_start_vcpu(CPUState *cpu)
>  {
>      char thread_name[VCPU_THREAD_NAME_SIZE];
> @@ -1316,7 +1426,10 @@ void qemu_init_vcpu(CPUState *cpu)
>          cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory);
>      }
>  
> -    if (kvm_enabled()) {
> +    if (cpus_accel) {
> +        /* accelerator already implements the CpusAccel interface */
> +        cpus_accel->create_vcpu_thread(cpu);
> +    } else if (kvm_enabled()) {
>          qemu_kvm_start_vcpu(cpu);
>      } else if (hax_enabled()) {
>          qemu_hax_start_vcpu(cpu);
> diff --git a/stubs/cpu-synchronize-state.c b/stubs/cpu-synchronize-state.c
> new file mode 100644
> index 0000000000..d9211da66c
> --- /dev/null
> +++ b/stubs/cpu-synchronize-state.c
> @@ -0,0 +1,9 @@
> +#include "qemu/osdep.h"
> +#include "sysemu/hw_accel.h"
> +
> +void cpu_synchronize_state(CPUState *cpu)
> +{
> +}
> +void cpu_synchronize_post_init(CPUState *cpu)
> +{
> +}
> diff --git a/stubs/cpus-get-virtual-clock.c b/stubs/cpus-get-virtual-clock.c
> new file mode 100644
> index 0000000000..fd447d53f3
> --- /dev/null
> +++ b/stubs/cpus-get-virtual-clock.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +#include "sysemu/cpu-timers.h"
> +#include "qemu/main-loop.h"
> +
> +int64_t cpus_get_virtual_clock(void)
> +{
> +    return cpu_get_clock();
> +}
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 57fec4f8c8..54d4a3f6d4 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -5,6 +5,7 @@ stub_ss.add(files('blockdev-close-all-bdrv-states.c'))
>  stub_ss.add(files('change-state-handler.c'))
>  stub_ss.add(files('cmos.c'))
>  stub_ss.add(files('cpu-get-clock.c'))
> +stub_ss.add(files('cpus-get-virtual-clock.c'))
>  stub_ss.add(files('qemu-timer-notify-cb.c'))
>  stub_ss.add(files('icount.c'))
>  stub_ss.add(files('dump.c'))
> @@ -45,6 +46,7 @@ stub_ss.add(files('vmgenid.c'))
>  stub_ss.add(files('vmstate.c'))
>  stub_ss.add(files('vm-stop.c'))
>  stub_ss.add(files('win32-kbd-hook.c'))
> +stub_ss.add(files('cpu-synchronize-state.c'))
>  if have_system
>    stub_ss.add(files('semihost.c'))
>  endif
> diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> index db51e68f25..50b325c65b 100644
> --- a/util/qemu-timer.c
> +++ b/util/qemu-timer.c
> @@ -635,13 +635,7 @@ int64_t qemu_clock_get_ns(QEMUClockType type)
>          return get_clock();
>      default:
>      case QEMU_CLOCK_VIRTUAL:
> -        if (icount_enabled()) {
> -            return icount_get();
> -        } else if (qtest_enabled()) { /* for qtest_clock_warp */
> -            return qtest_get_virtual_clock();
> -        } else {
> -            return cpu_get_clock();
> -        }
> +        return cpus_get_virtual_clock();
>      case QEMU_CLOCK_HOST:
>          return REPLAY_CLOCK(REPLAY_CLOCK_HOST, get_clock_realtime());
>      case QEMU_CLOCK_VIRTUAL_RT:

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée