Re: [PATCH v4 31/54] accel/qtest: Implement a portable qtest accelerator

[Thomas Huth]

On 27/09/2022 13.06, Bin Meng wrote:
> From: Xuzhou Cheng <xuzhou.cheng@windriver.com>
>
> Currently signal SIGIPI [=SIGUSR1] is used to kick the dummy CPU
> when qtest accelerator is used. However SIGUSR1 is unsupported on
> Windows. To support Windows, we add a QemuSemaphore CPUState::sem
> to kick the dummy CPU instead.
>
> As a result of this, the POSIX implementation via signal is no
> longer needed and can use the same path as Windows.
>
> Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Replace signal by the semaphore on posix too

This does not work - it breaks running with KVM on Linux. When I add this 
patch, I get:

$ ./qemu-system-x86_64  -machine pc   -smp 4 -enable-kvm
qemu-system-x86_64: ../../devel/qemu/util/qemu-thread-posix.c:86: 
qemu_mutex_lock_impl: Assertion `mutex->initialized' failed.
Aborted (core dumped)

I think "SIG_IPI" is used for kicking a CPU out of the kernel mode there, so 
you cannot simply replace it wrt KVM.

 Thomas


>   include/hw/core/cpu.h   |  1 +
>   accel/dummy-cpus.c      | 15 ++-------------
>   softmmu/cpus.c          | 10 +---------
>   accel/meson.build       |  1 +
>   accel/qtest/meson.build |  1 +
>   5 files changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 500503da13..2f46c37dc1 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -326,6 +326,7 @@ struct CPUState {
>   #ifdef _WIN32
>       HANDLE hThread;
>   #endif
> +    QemuSemaphore sem;
>       int thread_id;
>       bool running, has_waiter;
>       struct QemuCond *halt_cond;
> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
> index 10429fdfb2..3769d3db0a 100644
> --- a/accel/dummy-cpus.c
> +++ b/accel/dummy-cpus.c
> @@ -21,8 +21,6 @@
>   static void *dummy_cpu_thread_fn(void *arg)
>   {
>       CPUState *cpu = arg;
> -    sigset_t waitset;
> -    int r;
>   
>       rcu_register_thread();
>   
> @@ -32,23 +30,13 @@ static void *dummy_cpu_thread_fn(void *arg)
>       cpu->can_do_io = 1;
>       current_cpu = cpu;
>   
> -    sigemptyset(&waitset);
> -    sigaddset(&waitset, SIG_IPI);
> -
>       /* signal CPU creation */
>       cpu_thread_signal_created(cpu);
>       qemu_guest_random_seed_thread_part2(cpu->random_seed);
>   
>       do {
>           qemu_mutex_unlock_iothread();
> -        do {
> -            int sig;
> -            r = sigwait(&waitset, &sig);
> -        } while (r == -1 && (errno == EAGAIN || errno == EINTR));
> -        if (r == -1) {
> -            perror("sigwait");
> -            exit(1);
> -        }
> +        qemu_sem_wait(&cpu->sem);
>           qemu_mutex_lock_iothread();
>           qemu_wait_io_event(cpu);
>       } while (!cpu->unplug);
> @@ -67,6 +55,7 @@ void dummy_start_vcpu_thread(CPUState *cpu)
>       qemu_cond_init(cpu->halt_cond);
>       snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/DUMMY",
>                cpu->cpu_index);
> +    qemu_sem_init(&cpu->sem, 0);
>       qemu_thread_create(cpu->thread, thread_name, dummy_cpu_thread_fn, cpu,
>                          QEMU_THREAD_JOINABLE);
>   }
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 23b30484b2..2a992d0d5f 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -437,19 +437,11 @@ void qemu_wait_io_event(CPUState *cpu)
>   
>   void cpus_kick_thread(CPUState *cpu)
>   {
> -#ifndef _WIN32
> -    int err;
> -
>       if (cpu->thread_kicked) {
>           return;
>       }
>       cpu->thread_kicked = true;
> -    err = pthread_kill(cpu->thread->thread, SIG_IPI);
> -    if (err && err != ESRCH) {
> -        fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
> -        exit(1);
> -    }
> -#endif
> +    qemu_sem_post(&cpu->sem);
>   }
>   
>   void qemu_cpu_kick(CPUState *cpu)
> diff --git a/accel/meson.build b/accel/meson.build
> index b9a963cf80..b21c85dc0a 100644
> --- a/accel/meson.build
> +++ b/accel/meson.build
> @@ -17,4 +17,5 @@ dummy_ss.add(files(
>   ))
>   
>   specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true: dummy_ss)
> +specific_ss.add_all(when: ['CONFIG_WIN32'], if_true: dummy_ss)
>   specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss)
> diff --git a/accel/qtest/meson.build b/accel/qtest/meson.build
> index 4c65600293..a4876fc0f2 100644
> --- a/accel/qtest/meson.build
> +++ b/accel/qtest/meson.build
> @@ -1,2 +1,3 @@
>   qtest_module_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'],
>                       if_true: files('qtest.c'))
> +qtest_module_ss.add(when: ['CONFIG_WIN32'], if_true: files('qtest.c'))