Revert "accel/tcg: Init TCG cflags in vCPU thread handler"

[PATCH] Revert "accel/tcg: Init TCG cflags in vCPU thread handler"

Posted by Peter Maydell 1 year, 6 months ago

Commit a82fd5a4ec24d was intended to be a code cleanup, but
unfortunately it has a bug. It moves the initialization of the
TCG cflags from the "start a new vcpu" function to the
thread handler; this is fine when each vcpu has its own thread,
but when we are doing round-robin of vcpus on a single thread
we end up only initializing the cflags for CPU 0, not for any
of the others.

The most obvious effect of this bug is that running in icount
mode with more than one CPU is broken; typically the guest
hangs shortly after it brings up the secondary CPUs.

This reverts commit a82fd5a4ec24d923ff1e6da128c0fd4a74079d99.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/tcg-accel-ops-mttcg.c | 5 +++--
 accel/tcg/tcg-accel-ops-rr.c    | 7 ++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/tcg-accel-ops-mttcg.c
index ba997f6cfe4..d50239e0e28 100644
--- a/accel/tcg/tcg-accel-ops-mttcg.c
+++ b/accel/tcg/tcg-accel-ops-mttcg.c
@@ -70,8 +70,6 @@ static void *mttcg_cpu_thread_fn(void *arg)
     assert(tcg_enabled());
     g_assert(!icount_enabled());
 
-    tcg_cpu_init_cflags(cpu, current_machine->smp.max_cpus > 1);
-
     rcu_register_thread();
     force_rcu.notifier.notify = mttcg_force_rcu;
     force_rcu.cpu = cpu;
@@ -141,6 +139,9 @@ void mttcg_start_vcpu_thread(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
 
+    g_assert(tcg_enabled());
+    tcg_cpu_init_cflags(cpu, current_machine->smp.max_cpus > 1);
+
     cpu->thread = g_new0(QemuThread, 1);
     cpu->halt_cond = g_malloc0(sizeof(QemuCond));
     qemu_cond_init(cpu->halt_cond);
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index cc8adc23802..1a72149f0e4 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -152,9 +152,7 @@ static void *rr_cpu_thread_fn(void *arg)
     Notifier force_rcu;
     CPUState *cpu = arg;
 
-    g_assert(tcg_enabled());
-    tcg_cpu_init_cflags(cpu, false);
-
+    assert(tcg_enabled());
     rcu_register_thread();
     force_rcu.notify = rr_force_rcu;
     rcu_add_force_rcu_notifier(&force_rcu);
@@ -277,6 +275,9 @@ void rr_start_vcpu_thread(CPUState *cpu)
     static QemuCond *single_tcg_halt_cond;
     static QemuThread *single_tcg_cpu_thread;
 
+    g_assert(tcg_enabled());
+    tcg_cpu_init_cflags(cpu, false);
+
     if (!single_tcg_cpu_thread) {
         cpu->thread = g_new0(QemuThread, 1);
         cpu->halt_cond = g_new0(QemuCond, 1);
-- 
2.25.1

Re: [PATCH] Revert "accel/tcg: Init TCG cflags in vCPU thread handler"

Posted by Philippe Mathieu-Daudé 1 year, 6 months ago

On 21/10/22 18:34, Peter Maydell wrote:
> Commit a82fd5a4ec24d was intended to be a code cleanup, but
> unfortunately it has a bug. It moves the initialization of the
> TCG cflags from the "start a new vcpu" function to the
> thread handler; this is fine when each vcpu has its own thread,
> but when we are doing round-robin of vcpus on a single thread
> we end up only initializing the cflags for CPU 0, not for any
> of the others.
> 
> The most obvious effect of this bug is that running in icount
> mode with more than one CPU is broken; typically the guest
> hangs shortly after it brings up the secondary CPUs.

Ouch, sorry for the annoyance...

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> This reverts commit a82fd5a4ec24d923ff1e6da128c0fd4a74079d99.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops-mttcg.c | 5 +++--
>   accel/tcg/tcg-accel-ops-rr.c    | 7 ++++---
>   2 files changed, 7 insertions(+), 5 deletions(-)

Re: [PATCH] Revert "accel/tcg: Init TCG cflags in vCPU thread handler"

Posted by Richard Henderson 1 year, 6 months ago

On 10/22/22 02:34, Peter Maydell wrote:
> Commit a82fd5a4ec24d was intended to be a code cleanup, but
> unfortunately it has a bug. It moves the initialization of the
> TCG cflags from the "start a new vcpu" function to the
> thread handler; this is fine when each vcpu has its own thread,
> but when we are doing round-robin of vcpus on a single thread
> we end up only initializing the cflags for CPU 0, not for any
> of the others.
> 
> The most obvious effect of this bug is that running in icount
> mode with more than one CPU is broken; typically the guest
> hangs shortly after it brings up the secondary CPUs.
> 
> This reverts commit a82fd5a4ec24d923ff1e6da128c0fd4a74079d99.
> 
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops-mttcg.c | 5 +++--
>   accel/tcg/tcg-accel-ops-rr.c    | 7 ++++---
>   2 files changed, 7 insertions(+), 5 deletions(-)

Thanks, queued to tcg-next.

r~

Re: [PATCH] Revert "accel/tcg: Init TCG cflags in vCPU thread handler"

Posted by Richard Henderson 1 year, 6 months ago

On 10/22/22 02:34, Peter Maydell wrote:
> Commit a82fd5a4ec24d was intended to be a code cleanup, but
> unfortunately it has a bug. It moves the initialization of the
> TCG cflags from the "start a new vcpu" function to the
> thread handler; this is fine when each vcpu has its own thread,
> but when we are doing round-robin of vcpus on a single thread
> we end up only initializing the cflags for CPU 0, not for any
> of the others.
> 
> The most obvious effect of this bug is that running in icount
> mode with more than one CPU is broken; typically the guest
> hangs shortly after it brings up the secondary CPUs.
> 
> This reverts commit a82fd5a4ec24d923ff1e6da128c0fd4a74079d99.
> 
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   accel/tcg/tcg-accel-ops-mttcg.c | 5 +++--
>   accel/tcg/tcg-accel-ops-rr.c    | 7 ++++---
>   2 files changed, 7 insertions(+), 5 deletions(-)

Sorry about that,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~