[PATCH] accel: Fix qtest deadlock during unplug

[PATCH] accel: Fix qtest deadlock during unplug

[Fabiano Rosas]

This is a revert of one hunk of commit d5e33b5f8f ("accel: make all
calls to qemu_process_cpu_events look the same"). It regressed
device-plug-test on ppc64. Run this in a loop and it deadlocks before
50 iterations:

QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/device-plug-test -p
/ppc64/device-plug/spapr-cpu-unplug-request

The deadlocked stacks are:
T0:
 #0  in sigtimedwait
 #1  in sigwait
 #2  in dummy_cpu_thread_fn (arg=0x558ed4db8eb0) at ../accel/dummy-cpus.c:52

T1:
 #2  in qemu_thread_join (thread=0x55e3803dfc60) at ../util/qemu-thread-posix.c:554
 #3  in cpu_remove_sync (cpu=0x558ed4db8eb0) at ../system/cpus.c:633
 #4  in ppc_cpu_unrealize (dev=0x558ed4db8eb0) at ../target/ppc/cpu_init.c:6967

What the test does is to queue a cpu unplug request to be executed
during system reset. So we end up with two cpu_exit() calls affecting
the dummy loop, one via pause_all_cpus() and another via
cpu_remove_sync().

Moving qemu_process_cpu_events() to the top of the loop has made the
release of the halt_cond + the read of cpu->unplug not happen
atomically regarding the BQL anymore.

One cpu_exit() call will cause qemu_process_cpu_events() to make
progress, the BQL be release and the pending SIG_IPI to be consumed by
sigwait(). But since the BQL is unlocked, the second qemu_cpu_kick()
invocation can execute entirely while the BQL is unlocked and issue:

i) another broadcast on halt_cond, which will be queued and,
ii) another signal, which will be discarded

After the sigwait() returns and qemu_process_cpu_events() executes
again in the next loop iteration, it exits right away due to the cond
already being posted, but the sigwait() call for that loop won't see
any signal. The thread cannot be joined at this point so there's a
deadlock.

Since the dummy_cpu loop is so simple, I think the best way to fix
this is to revert that part of the change and move
qemu_process_cpu_events() back to the end of the loop, where it will
be within the same BQL locking window as the cpu->unplug check.

Fixes: d5e33b5f8f ("accel: make all calls to qemu_process_cpu_events look the same")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
CI run: https://gitlab.com/farosas/qemu/-/pipelines/2861429046
---
 accel/dummy-cpus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
index 5752f6302c..225a47c31f 100644
--- a/accel/dummy-cpus.c
+++ b/accel/dummy-cpus.c
@@ -43,7 +43,6 @@ static void *dummy_cpu_thread_fn(void *arg)
     qemu_guest_random_seed_thread_part2(cpu->random_seed);
 
     do {
-        qemu_process_cpu_events(cpu);
         bql_unlock();
 #ifndef _WIN32
         do {
@@ -58,6 +57,7 @@ static void *dummy_cpu_thread_fn(void *arg)
         qemu_sem_wait(&cpu->sem);
 #endif
         bql_lock();
+        qemu_process_cpu_events(cpu);
     } while (!cpu->unplug);
 
     bql_unlock();
-- 
2.53.0

Re: [PATCH] accel: Fix qtest deadlock during unplug

[Philippe Mathieu-Daudé]

Hi Fabiano,

On 2026-09-18 16:00, Fabiano Rosas wrote:
> This is a revert of one hunk of commit d5e33b5f8f ("accel: make all
> calls to qemu_process_cpu_events look the same"). It regressed
> device-plug-test on ppc64. Run this in a loop and it deadlocks before
> 50 iterations:
> 
> QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/device-plug-test -p
> /ppc64/device-plug/spapr-cpu-unplug-request
> 
> The deadlocked stacks are:
> T0:
>   #0  in sigtimedwait
>   #1  in sigwait
>   #2  in dummy_cpu_thread_fn (arg=0x558ed4db8eb0) at ../accel/dummy-cpus.c:52
> 
> T1:
>   #2  in qemu_thread_join (thread=0x55e3803dfc60) at ../util/qemu-thread-posix.c:554
>   #3  in cpu_remove_sync (cpu=0x558ed4db8eb0) at ../system/cpus.c:633
>   #4  in ppc_cpu_unrealize (dev=0x558ed4db8eb0) at ../target/ppc/cpu_init.c:6967
> 
> What the test does is to queue a cpu unplug request to be executed
> during system reset. So we end up with two cpu_exit() calls affecting
> the dummy loop, one via pause_all_cpus() and another via
> cpu_remove_sync().

I don't understand why we have these few per-target cpu_remove_sync()
calls. IMO it should only be called by core vcpu accel scheduler layer.

> 
> Moving qemu_process_cpu_events() to the top of the loop has made the
> release of the halt_cond + the read of cpu->unplug not happen
> atomically regarding the BQL anymore.
> 
> One cpu_exit() call will cause qemu_process_cpu_events() to make
> progress, the BQL be release and the pending SIG_IPI to be consumed by
> sigwait(). But since the BQL is unlocked, the second qemu_cpu_kick()
> invocation can execute entirely while the BQL is unlocked and issue:
> 
> i) another broadcast on halt_cond, which will be queued and,
> ii) another signal, which will be discarded
> 
> After the sigwait() returns and qemu_process_cpu_events() executes
> again in the next loop iteration, it exits right away due to the cond
> already being posted, but the sigwait() call for that loop won't see
> any signal. The thread cannot be joined at this point so there's a
> deadlock.
> 
> Since the dummy_cpu loop is so simple, I think the best way to fix
> this is to revert that part of the change and move
> qemu_process_cpu_events() back to the end of the loop, where it will
> be within the same BQL locking window as the cpu->unplug check.
> 
> Fixes: d5e33b5f8f ("accel: make all calls to qemu_process_cpu_events look the same")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/2861429046
> ---
>   accel/dummy-cpus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
> index 5752f6302c..225a47c31f 100644
> --- a/accel/dummy-cpus.c
> +++ b/accel/dummy-cpus.c
> @@ -43,7 +43,6 @@ static void *dummy_cpu_thread_fn(void *arg)
>       qemu_guest_random_seed_thread_part2(cpu->random_seed);
>   
>       do {
> -        qemu_process_cpu_events(cpu);
>           bql_unlock();
>   #ifndef _WIN32
>           do {
> @@ -58,6 +57,7 @@ static void *dummy_cpu_thread_fn(void *arg)
>           qemu_sem_wait(&cpu->sem);
>   #endif
>           bql_lock();
> +        qemu_process_cpu_events(cpu);
>       } while (!cpu->unplug);
>   
>       bql_unlock();

Re: [PATCH] accel: Fix qtest deadlock during unplug

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> Hi Fabiano,
>
> On 2026-09-18 16:00, Fabiano Rosas wrote:
>> This is a revert of one hunk of commit d5e33b5f8f ("accel: make all
>> calls to qemu_process_cpu_events look the same"). It regressed
>> device-plug-test on ppc64. Run this in a loop and it deadlocks before
>> 50 iterations:
>> 
>> QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/device-plug-test -p
>> /ppc64/device-plug/spapr-cpu-unplug-request
>> 
>> The deadlocked stacks are:
>> T0:
>>   #0  in sigtimedwait
>>   #1  in sigwait
>>   #2  in dummy_cpu_thread_fn (arg=0x558ed4db8eb0) at ../accel/dummy-cpus.c:52
>> 
>> T1:
>>   #2  in qemu_thread_join (thread=0x55e3803dfc60) at ../util/qemu-thread-posix.c:554
>>   #3  in cpu_remove_sync (cpu=0x558ed4db8eb0) at ../system/cpus.c:633
>>   #4  in ppc_cpu_unrealize (dev=0x558ed4db8eb0) at ../target/ppc/cpu_init.c:6967
>> 
>> What the test does is to queue a cpu unplug request to be executed
>> during system reset. So we end up with two cpu_exit() calls affecting
>> the dummy loop, one via pause_all_cpus() and another via
>> cpu_remove_sync().
>
> I don't understand why we have these few per-target cpu_remove_sync()
> calls. IMO it should only be called by core vcpu accel scheduler layer.
>

TLDR: ppc introduced it at the machine level (spapr_cpu_core.c) to handle
cpu hot-unplug. It later got moved into cpu unrealize.

Let's look at the history:

1) cpu_remove_sync() was introduced to deal with hot-unplug failures.
2c579042e3 (cpu: Add a sync version of cpu_remove(), 2016-05-12)

2) The cpu->unplug logic is due to the possibility of requesting a cpu
unplug, leaving the cpu object in the list and then later plugging the
cpu again and re-using that object (I'm not sure, looks more like a
comment from KVM point of view).
4c055ab54f (cpu: Reclaim vCPU objects, 2016-05-12)

3) For x86, it seems cpu_remove_sync() is called just for regular
cleanup.
c884776e9d (target-i386: Add x86_cpu_unrealizefn(), 2016-06-24)

3) The _sync version got later turned into the default with the removal
of the async one.
dbadee4ff4 (cpus: join thread when removing a vCPU, 2018-01-30)

I'm confused about two aspects of this:

1) Checking cpu->unplug in the run loops in general because it seems to
make most of cpu_can_run() redundant since cpu->stop is also set during
cpu_remove_sync(). It's not clear to me what sort of concurrency can
keep the loop going once cpu->unplug=true.

To me it looks like we could very well move the cpu->unplug check into
cpu_can_run(), move qemu_process_cpu_events(cpu) back to the end of the
loop and merge both (where there are two) calls to cpu_can_run(). E.g.:

    do {
        bql_unlock();
        excp = tcg_cpu_exec(cpu);
        qemu_process_cpu_events(cpu);
        bql_lock();
    } while (cpu_can_run());

2) Not all targets set cpu->unplug, but the check happens for all. What
exactly stops their vcpus threads? And, obviously, is it safe to now use
cpu->unplug.

Re: [PATCH] accel: Fix qtest deadlock during unplug

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> This is a revert of one hunk of commit d5e33b5f8f ("accel: make all
> calls to qemu_process_cpu_events look the same"). It regressed
> device-plug-test on ppc64. Run this in a loop and it deadlocks before
> 50 iterations:
>
> QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/device-plug-test -p
> /ppc64/device-plug/spapr-cpu-unplug-request
>
> The deadlocked stacks are:
> T0:
>  #0  in sigtimedwait
>  #1  in sigwait
>  #2  in dummy_cpu_thread_fn (arg=0x558ed4db8eb0) at ../accel/dummy-cpus.c:52
>
> T1:
>  #2  in qemu_thread_join (thread=0x55e3803dfc60) at ../util/qemu-thread-posix.c:554
>  #3  in cpu_remove_sync (cpu=0x558ed4db8eb0) at ../system/cpus.c:633
>  #4  in ppc_cpu_unrealize (dev=0x558ed4db8eb0) at ../target/ppc/cpu_init.c:6967
>
> What the test does is to queue a cpu unplug request to be executed
> during system reset. So we end up with two cpu_exit() calls affecting
> the dummy loop, one via pause_all_cpus() and another via
> cpu_remove_sync().
>
> Moving qemu_process_cpu_events() to the top of the loop has made the
> release of the halt_cond + the read of cpu->unplug not happen
> atomically regarding the BQL anymore.
>
> One cpu_exit() call will cause qemu_process_cpu_events() to make
> progress, the BQL be release and the pending SIG_IPI to be consumed by
> sigwait(). But since the BQL is unlocked, the second qemu_cpu_kick()
> invocation can execute entirely while the BQL is unlocked and issue:
>
> i) another broadcast on halt_cond, which will be queued and,
> ii) another signal, which will be discarded
>
> After the sigwait() returns and qemu_process_cpu_events() executes
> again in the next loop iteration, it exits right away due to the cond
> already being posted, but the sigwait() call for that loop won't see
> any signal. The thread cannot be joined at this point so there's a
> deadlock.
>
> Since the dummy_cpu loop is so simple, I think the best way to fix
> this is to revert that part of the change and move
> qemu_process_cpu_events() back to the end of the loop, where it will
> be within the same BQL locking window as the cpu->unplug check.
>
> Fixes: d5e33b5f8f ("accel: make all calls to qemu_process_cpu_events look the same")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/2861429046
> ---
>  accel/dummy-cpus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c
> index 5752f6302c..225a47c31f 100644
> --- a/accel/dummy-cpus.c
> +++ b/accel/dummy-cpus.c
> @@ -43,7 +43,6 @@ static void *dummy_cpu_thread_fn(void *arg)
>      qemu_guest_random_seed_thread_part2(cpu->random_seed);
>  
>      do {
> -        qemu_process_cpu_events(cpu);
>          bql_unlock();
>  #ifndef _WIN32
>          do {
> @@ -58,6 +57,7 @@ static void *dummy_cpu_thread_fn(void *arg)
>          qemu_sem_wait(&cpu->sem);
>  #endif
>          bql_lock();
> +        qemu_process_cpu_events(cpu);
>      } while (!cpu->unplug);
>  
>      bql_unlock();

+ppc and mshv folks

@Chinmay, just to make you aware that there's a broken test for ppc

@Shivang, we're discussing about cpu_remove_sync() down in this thread,
maybe that's of interest to you. Philippe is suggesting we could maybe
move the call up a layer.

@Doru, Magnus, for your awareness. This bug is about the dummy
cpu thread that qtest and xen use, but the pattern of coming out of
qemu_process_cpu_events() and unlocking the BQL is present in mshv as
well.

Re: [PATCH] accel: Fix qtest deadlock during unplug

[Shivang Upadhyay]

On Wed, 2026-09-23 at 10:05 -0300, Fabiano Rosas wrote:
> 
> @Shivang, we're discussing about cpu_remove_sync() down in this
> thread,
> maybe that's of interest to you. Philippe is suggesting we could
> maybe
> move the call up a layer.

Hi Fabiano, 
 I checked the dummp_cpu way to working, I can see you changes makes
sense here. I couldn't understand much from Philippe's suggestion. Do
we want to handle the case for thread waiting on sigwait, inside
cpu_remove_sync ?

Regards.
~Shivang.

Re: [PATCH] accel: Fix qtest deadlock during unplug

[Fabiano Rosas]

Shivang Upadhyay <shivangu@linux.ibm.com> writes:

> On Wed, 2026-09-23 at 10:05 -0300, Fabiano Rosas wrote:
>> 
>> @Shivang, we're discussing about cpu_remove_sync() down in this
>> thread,
>> maybe that's of interest to you. Philippe is suggesting we could
>> maybe
>> move the call up a layer.
>
> Hi Fabiano, 
>  I checked the dummp_cpu way to working, I can see you changes makes
> sense here. I couldn't understand much from Philippe's suggestion. Do
> we want to handle the case for thread waiting on sigwait, inside
> cpu_remove_sync ?
>

The point was simply to call cpu_remove_sync() from another spot. I
think he made the same suggestion to you here:
https://lore.kernel.org/r/bb91d62a-f15a-42e5-813d-5ec7e6d92479@oss.qualcomm.com

While researching what would it take to do it, I got concerned about the
effects that setting cpu->unplug could have for targets that currently
do not set it, but nonetheless use it as the cpu loop exit
condition. That question is still open, I haven't got time to look
deeper into it yet.

If you're interested in that area, you could give it a try. As I
mentioned in the other email, the unplug logic was added back then to
improve the usability of spapr cpu hot-unplug so it could be pertinent
to anyone working on ppc.