[PATCH v2 00/10] accel: Only include qdev-realized vCPUs in global &cpus_queue

[PATCH v2 00/10] accel: Only include qdev-realized vCPUs in global &cpus_queue

[Igor Mammedov]

Changelog:
  * drop wire/unwire hooks patches
  * drop unrealize related patches
  * include fixed up patches from
       [PATCH 0/6] tcg: fix qemu crash when add assert_cpu_is_self() is enabled
        and cleanups related to cpu->created check
        https://patchew.org/QEMU/20250129134436.1240740-1-imammedo@redhat.com/
    as it's related to the topic (well, modulo bsd cleanup)
  * CI mostly green modulo rust failure on Ubuntu
     https://gitlab.com/imammedo/qemu/-/pipelines/1660855467

The goal of this series is to expose vCPUs in a stable state
to the accelerators, in particular the QDev 'REALIZED' step.

To do this we split out cpu_index assignment into a separate step,
and move call cpu_list_add() to the end of CPU realize stage.

I expect these changes to allow CPUState::cpu_index clarifications
and simplifications, but this will be addressed (and commented) in
a separate series.

As result, the series also
 * fix regression intoroduced by
      30933c4fb4f3d ("tcg/cputlb: remove other-cpu capability from TLB flushing")
   for deatials see 'tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()'
 * drops no longer needed workaround 'cpu->check' due to vCPU being exposed
   too early in cpus_queue.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Richard Henderson <richard.henderson@linaro.org>
CC: "Philippe Mathieu-Daudé" <philmd@linaro.org>
CC: Alex Bennée <alex.bennee@linaro.org>

Igor Mammedov (7):
  bsd-user: drop not longer used target_reset_cpu()
  loongarch: reset vcpu after it's created
  m68k: reset vcpu after it's created
  tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()
  Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"
  tcg: drop cpu->created check
  cpus: expose only realized vCPUs to global &cpus_queue

Philippe Mathieu-Daudé (3):
  accel/tcg: Simplify use of &first_cpu in rr_cpu_thread_fn()
  accel/kvm: Assert vCPU is created when calling kvm_dirty_ring_reap*()
  accel/kvm: Remove unreachable assertion in kvm_dirty_ring_reap*()

 bsd-user/aarch64/target_arch_cpu.h |  5 ---
 bsd-user/arm/target_arch_cpu.h     |  4 ---
 bsd-user/i386/target_arch_cpu.h    |  5 ---
 bsd-user/riscv/target_arch_cpu.h   |  4 ---
 bsd-user/x86_64/target_arch_cpu.h  |  5 ---
 include/hw/core/cpu.h              |  6 ++++
 accel/kvm/kvm-all.c                |  9 ------
 accel/tcg/cputlb.c                 | 49 +++++++++++++++++++++---------
 accel/tcg/tcg-accel-ops-rr.c       | 13 +++++---
 cpu-common.c                       | 23 ++++++++------
 cpu-target.c                       |  2 +-
 hw/core/cpu-common.c               |  2 ++
 target/loongarch/cpu.c             |  2 +-
 target/m68k/cpu.c                  |  2 +-
 14 files changed, 68 insertions(+), 63 deletions(-)

-- 
2.43.0

[PATCH v2 01/10] bsd-user: drop not longer used target_reset_cpu()

[Igor Mammedov]

target_reset_cpu() static inlines have no user,
remove them.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Warner Losh <imp@bsdimp.com>
CC: Kyle Evans <kevans@freebsd.org>
---
 bsd-user/aarch64/target_arch_cpu.h | 5 -----
 bsd-user/arm/target_arch_cpu.h     | 4 ----
 bsd-user/i386/target_arch_cpu.h    | 5 -----
 bsd-user/riscv/target_arch_cpu.h   | 4 ----
 bsd-user/x86_64/target_arch_cpu.h  | 5 -----
 5 files changed, 23 deletions(-)

diff --git a/bsd-user/aarch64/target_arch_cpu.h b/bsd-user/aarch64/target_arch_cpu.h
index 87fbf6d677..46a448e93f 100644
--- a/bsd-user/aarch64/target_arch_cpu.h
+++ b/bsd-user/aarch64/target_arch_cpu.h
@@ -181,9 +181,4 @@ static inline void target_cpu_clone_regs(CPUARMState *env, target_ulong newsp)
     pstate_write(env, 0);
 }
 
-static inline void target_cpu_reset(CPUArchState *env)
-{
-}
-
-
 #endif /* TARGET_ARCH_CPU_H */
diff --git a/bsd-user/arm/target_arch_cpu.h b/bsd-user/arm/target_arch_cpu.h
index bc2eaa0bf4..b9583b0f92 100644
--- a/bsd-user/arm/target_arch_cpu.h
+++ b/bsd-user/arm/target_arch_cpu.h
@@ -206,8 +206,4 @@ static inline void target_cpu_clone_regs(CPUARMState *env, target_ulong newsp)
     env->regs[0] = 0;
 }
 
-static inline void target_cpu_reset(CPUArchState *env)
-{
-}
-
 #endif /* TARGET_ARCH_CPU_H */
diff --git a/bsd-user/i386/target_arch_cpu.h b/bsd-user/i386/target_arch_cpu.h
index 5d4c931dec..371e702799 100644
--- a/bsd-user/i386/target_arch_cpu.h
+++ b/bsd-user/i386/target_arch_cpu.h
@@ -194,9 +194,4 @@ static inline void target_cpu_clone_regs(CPUX86State *env, target_ulong newsp)
     env->regs[R_EAX] = 0;
 }
 
-static inline void target_cpu_reset(CPUArchState *env)
-{
-    cpu_reset(env_cpu(env));
-}
-
 #endif /* TARGET_ARCH_CPU_H */
diff --git a/bsd-user/riscv/target_arch_cpu.h b/bsd-user/riscv/target_arch_cpu.h
index ef92f00480..d3cc5adbf4 100644
--- a/bsd-user/riscv/target_arch_cpu.h
+++ b/bsd-user/riscv/target_arch_cpu.h
@@ -141,8 +141,4 @@ static inline void target_cpu_clone_regs(CPURISCVState *env, target_ulong newsp)
     env->gpr[xT0] = 0;
 }
 
-static inline void target_cpu_reset(CPUArchState *env)
-{
-}
-
 #endif /* TARGET_ARCH_CPU_H */
diff --git a/bsd-user/x86_64/target_arch_cpu.h b/bsd-user/x86_64/target_arch_cpu.h
index f82042e30a..8ec5c65fab 100644
--- a/bsd-user/x86_64/target_arch_cpu.h
+++ b/bsd-user/x86_64/target_arch_cpu.h
@@ -169,9 +169,4 @@ static inline void target_cpu_clone_regs(CPUX86State *env, target_ulong newsp)
     env->regs[R_EAX] = 0;
 }
 
-static inline void target_cpu_reset(CPUArchState *env)
-{
-    cpu_reset(env_cpu(env));
-}
-
 #endif /* TARGET_ARCH_CPU_H */
-- 
2.43.0

Re: [PATCH v2 01/10] bsd-user: drop not longer used target_reset_cpu()

[Alex Bennée]

Igor Mammedov <imammedo@redhat.com> writes:

> target_reset_cpu() static inlines have no user,
> remove them.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Warner Losh <imp@bsdimp.com>
> CC: Kyle Evans <kevans@freebsd.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 01/10] bsd-user: drop not longer used target_reset_cpu()

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

On Fri, Feb 7, 2025 at 9:21 AM Igor Mammedov <imammedo@redhat.com> wrote:

> target_reset_cpu() static inlines have no user,
> remove them.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Warner Losh <imp@bsdimp.com>
> CC: Kyle Evans <kevans@freebsd.org>
> ---
>  bsd-user/aarch64/target_arch_cpu.h | 5 -----
>  bsd-user/arm/target_arch_cpu.h     | 4 ----
>  bsd-user/i386/target_arch_cpu.h    | 5 -----
>  bsd-user/riscv/target_arch_cpu.h   | 4 ----
>  bsd-user/x86_64/target_arch_cpu.h  | 5 -----
>  5 files changed, 23 deletions(-)
>

Reviewed-by: Warner Losh <imp@bsdimp.com>

Warner


> diff --git a/bsd-user/aarch64/target_arch_cpu.h
> b/bsd-user/aarch64/target_arch_cpu.h
> index 87fbf6d677..46a448e93f 100644
> --- a/bsd-user/aarch64/target_arch_cpu.h
> +++ b/bsd-user/aarch64/target_arch_cpu.h
> @@ -181,9 +181,4 @@ static inline void target_cpu_clone_regs(CPUARMState
> *env, target_ulong newsp)
>      pstate_write(env, 0);
>  }
>
> -static inline void target_cpu_reset(CPUArchState *env)
> -{
> -}
> -
> -
>  #endif /* TARGET_ARCH_CPU_H */
> diff --git a/bsd-user/arm/target_arch_cpu.h
> b/bsd-user/arm/target_arch_cpu.h
> index bc2eaa0bf4..b9583b0f92 100644
> --- a/bsd-user/arm/target_arch_cpu.h
> +++ b/bsd-user/arm/target_arch_cpu.h
> @@ -206,8 +206,4 @@ static inline void target_cpu_clone_regs(CPUARMState
> *env, target_ulong newsp)
>      env->regs[0] = 0;
>  }
>
> -static inline void target_cpu_reset(CPUArchState *env)
> -{
> -}
> -
>  #endif /* TARGET_ARCH_CPU_H */
> diff --git a/bsd-user/i386/target_arch_cpu.h
> b/bsd-user/i386/target_arch_cpu.h
> index 5d4c931dec..371e702799 100644
> --- a/bsd-user/i386/target_arch_cpu.h
> +++ b/bsd-user/i386/target_arch_cpu.h
> @@ -194,9 +194,4 @@ static inline void target_cpu_clone_regs(CPUX86State
> *env, target_ulong newsp)
>      env->regs[R_EAX] = 0;
>  }
>
> -static inline void target_cpu_reset(CPUArchState *env)
> -{
> -    cpu_reset(env_cpu(env));
> -}
> -
>  #endif /* TARGET_ARCH_CPU_H */
> diff --git a/bsd-user/riscv/target_arch_cpu.h
> b/bsd-user/riscv/target_arch_cpu.h
> index ef92f00480..d3cc5adbf4 100644
> --- a/bsd-user/riscv/target_arch_cpu.h
> +++ b/bsd-user/riscv/target_arch_cpu.h
> @@ -141,8 +141,4 @@ static inline void target_cpu_clone_regs(CPURISCVState
> *env, target_ulong newsp)
>      env->gpr[xT0] = 0;
>  }
>
> -static inline void target_cpu_reset(CPUArchState *env)
> -{
> -}
> -
>  #endif /* TARGET_ARCH_CPU_H */
> diff --git a/bsd-user/x86_64/target_arch_cpu.h
> b/bsd-user/x86_64/target_arch_cpu.h
> index f82042e30a..8ec5c65fab 100644
> --- a/bsd-user/x86_64/target_arch_cpu.h
> +++ b/bsd-user/x86_64/target_arch_cpu.h
> @@ -169,9 +169,4 @@ static inline void target_cpu_clone_regs(CPUX86State
> *env, target_ulong newsp)
>      env->regs[R_EAX] = 0;
>  }
>
> -static inline void target_cpu_reset(CPUArchState *env)
> -{
> -    cpu_reset(env_cpu(env));
> -}
> -
>  #endif /* TARGET_ARCH_CPU_H */
> --
> 2.43.0
>
>

[-- Attachment #2: Type: text/html, Size: 3982 bytes --]

[PATCH v2 02/10] loongarch: reset vcpu after it's created

[Igor Mammedov]

Reseting vcpu before its thread is created, caused various issues in the past
for other targets. It doesn't cause issues for loongarch at the moment but
to be consistent with the rest of targets, move reset during realize time
after qemu_init_vcpu().

That basically prevents reset being run when when vCPU is in incositent state
(i.e. accelerator hasn't initialized vCPU yet).

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Song Gao <gaosong@loongson.cn>
---
 target/loongarch/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index e91f4a5239..15018d43ae 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -634,8 +634,8 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
 
     loongarch_cpu_register_gdb_regs_for_features(cs);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     lacc->parent_realize(dev, errp);
 }
-- 
2.43.0

Re: [PATCH v2 02/10] loongarch: reset vcpu after it's created

[bibo mao]

On 2025/2/8 上午12:20, Igor Mammedov wrote:
> Reseting vcpu before its thread is created, caused various issues in the past
> for other targets. It doesn't cause issues for loongarch at the moment but
> to be consistent with the rest of targets, move reset during realize time
> after qemu_init_vcpu().
> 
> That basically prevents reset being run when when vCPU is in incositent state
> (i.e. accelerator hasn't initialized vCPU yet).
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index e91f4a5239..15018d43ae 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -634,8 +634,8 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
>   
>       loongarch_cpu_register_gdb_regs_for_features(cs);
>   
> -    cpu_reset(cs);
>       qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>   
>       lacc->parent_realize(dev, errp);
>   }
>
yes, that is actually a problem in kvm mode. vCPU thread should be 
created and kvm_fd for the vCPU need valid before cpu_reset().

Reviewed-by: Bibo Mao <maobibo@loongson.cn>

Re: [PATCH v2 02/10] loongarch: reset vcpu after it's created

[Philippe Mathieu-Daudé]

On 7/2/25 17:20, Igor Mammedov wrote:
> Reseting vcpu before its thread is created, caused various issues in the past
> for other targets. It doesn't cause issues for loongarch at the moment but
> to be consistent with the rest of targets, move reset during realize time
> after qemu_init_vcpu().
> 
> That basically prevents reset being run when when vCPU is in incositent state

"inconsistent"

> (i.e. accelerator hasn't initialized vCPU yet).
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

Cc: qemu-stable@nongnu.org

> ---
> CC: Song Gao <gaosong@loongson.cn>
> ---
>   target/loongarch/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index e91f4a5239..15018d43ae 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -634,8 +634,8 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
>   
>       loongarch_cpu_register_gdb_regs_for_features(cs);
>   
> -    cpu_reset(cs);
>       qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>   
>       lacc->parent_realize(dev, errp);
>   }

[PATCH v2 03/10] m68k: reset vcpu after it's created

[Igor Mammedov]

Reseting vcpu before its thread is created, caused various issues in the past
for other targets. It doesn't cause issues for m68k at the moment but
to be consistent with the rest of targets, move reset during realize time
after qemu_init_vcpu().

That basically prevents reset being run when when vCPU is in incositent state
(i.e. accelerator hasn't initialized vCPU yet).

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Laurent Vivier <laurent@vivier.eu>
---
 target/m68k/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 41dfdf5804..3fd2663fb0 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -334,8 +334,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
 
     m68k_cpu_init_gdb(cpu);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
 }
-- 
2.43.0

Re: [PATCH v2 03/10] m68k: reset vcpu after it's created

[Alex Bennée]

Igor Mammedov <imammedo@redhat.com> writes:

> Reseting vcpu before its thread is created, caused various issues in the past
> for other targets. It doesn't cause issues for m68k at the moment but
> to be consistent with the rest of targets, move reset during realize time
> after qemu_init_vcpu().
>
> That basically prevents reset being run when when vCPU is in incositent state
> (i.e. accelerator hasn't initialized vCPU yet).
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 03/10] m68k: reset vcpu after it's created

[Philippe Mathieu-Daudé]

On 7/2/25 17:20, Igor Mammedov wrote:
> Reseting vcpu before its thread is created, caused various issues in the past
> for other targets. It doesn't cause issues for m68k at the moment but
> to be consistent with the rest of targets, move reset during realize time
> after qemu_init_vcpu().
> 
> That basically prevents reset being run when when vCPU is in incositent state

(typo)

> (i.e. accelerator hasn't initialized vCPU yet).
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Cc: qemu-stable@nongnu.org
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> ---
> CC: Laurent Vivier <laurent@vivier.eu>
> ---
>   target/m68k/cpu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH v2 04/10] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Igor Mammedov]

that will enable assert_cpu_is_self when QEMU is configured with
   --enable-debug
without need for manual patching DEBUG_TLB_GATE define.

Need to manually path DEBUG_TLB_GATE define to enable assert,
let regression caused by [1] creep in unnoticed.

1) 30933c4fb4f3d ("tcg/cputlb: remove other-cpu capability from TLB flushing")
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Suggested-by: Alex Bennée <alex.bennee@linaro.org>
---
v2: revert DEBUG_TLB_GATE/DEBUG_TLB_LOG_GATE to 0 as it used to be
---
 accel/tcg/cputlb.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b4ccf0cdcb..7380b29da3 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -74,11 +74,8 @@
     } \
 } while (0)
 
-#define assert_cpu_is_self(cpu) do {                              \
-        if (DEBUG_TLB_GATE) {                                     \
-            g_assert(!(cpu)->created || qemu_cpu_is_self(cpu));   \
-        }                                                         \
-    } while (0)
+#define assert_cpu_is_self(cpu)                             \
+    tcg_debug_assert(!(cpu)->created || qemu_cpu_is_self(cpu))
 
 /* run_on_cpu_data.target_ptr should always be big enough for a
  * vaddr even on 32 bit builds
-- 
2.43.0

Re: [PATCH v2 04/10] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Alex Bennée]

Igor Mammedov <imammedo@redhat.com> writes:

> that will enable assert_cpu_is_self when QEMU is configured with
>    --enable-debug
> without need for manual patching DEBUG_TLB_GATE define.
>
> Need to manually path DEBUG_TLB_GATE define to enable assert,
> let regression caused by [1] creep in unnoticed.
>
> 1) 30933c4fb4f3d ("tcg/cputlb: remove other-cpu capability from TLB flushing")
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Alex Bennée <alex.bennee@linaro.org>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 05/10] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Igor Mammedov]

1)
This reverts commit 30933c4fb4f3df95ae44c4c3c86a5df049852c01.
  ("tcg/cputlb: remove other-cpu capability from TLB flushing")

The commit caused a regression which went unnoticed due to
affected being disabled by default (DEBUG_TLB_GATE 0)
Previous patch switched to using tcg_debug_assert() so that
at least on debug builds assert_cpu_is_self() path would be exercised.

And that lead to exposing regression introduced by [1] with abort during tests.
to reproduce:
  $ configure  --target-list=x86_64-softmmu --enable-debug
  $ make && ./qemu-system-x86_64

  accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx:
    Assertion `!(cpu)->created || qemu_cpu_is_self(cpu)' failed.

which is triggered by usage outside of cpu thread:
    x86_cpu_new -> ... ->
      x86_cpu_realizefn -> cpu_reset -> ... ->
          tcg_cpu_reset_hold

Drop offending commit for now, until a propper fix that doesn't break
'make check' is available.

PS:
fixup g_memdup() checkpatch error s/g_memdup/g_memdup2/

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
I'll leave it upto TCG folz to fix it up propperly.

CC: npiggin@gmail.com
CC: BALATON Zoltan <balaton@eik.bme.hu>

---
 accel/tcg/cputlb.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 7380b29da3..3d1d7d2409 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -416,9 +416,12 @@ void tlb_flush_by_mmuidx(CPUState *cpu, uint16_t idxmap)
 {
     tlb_debug("mmu_idx: 0x%" PRIx16 "\n", idxmap);
 
-    assert_cpu_is_self(cpu);
-
-    tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(idxmap));
+    if (cpu->created && !qemu_cpu_is_self(cpu)) {
+        async_run_on_cpu(cpu, tlb_flush_by_mmuidx_async_work,
+                         RUN_ON_CPU_HOST_INT(idxmap));
+    } else {
+        tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(idxmap));
+    }
 }
 
 void tlb_flush(CPUState *cpu)
@@ -607,12 +610,28 @@ void tlb_flush_page_by_mmuidx(CPUState *cpu, vaddr addr, uint16_t idxmap)
 {
     tlb_debug("addr: %016" VADDR_PRIx " mmu_idx:%" PRIx16 "\n", addr, idxmap);
 
-    assert_cpu_is_self(cpu);
-
     /* This should already be page aligned */
     addr &= TARGET_PAGE_MASK;
 
-    tlb_flush_page_by_mmuidx_async_0(cpu, addr, idxmap);
+    if (qemu_cpu_is_self(cpu)) {
+        tlb_flush_page_by_mmuidx_async_0(cpu, addr, idxmap);
+    } else if (idxmap < TARGET_PAGE_SIZE) {
+        /*
+         * Most targets have only a few mmu_idx.  In the case where
+         * we can stuff idxmap into the low TARGET_PAGE_BITS, avoid
+         * allocating memory for this operation.
+         */
+        async_run_on_cpu(cpu, tlb_flush_page_by_mmuidx_async_1,
+                         RUN_ON_CPU_TARGET_PTR(addr | idxmap));
+    } else {
+        TLBFlushPageByMMUIdxData *d = g_new(TLBFlushPageByMMUIdxData, 1);
+
+        /* Otherwise allocate a structure, freed by the worker.  */
+        d->addr = addr;
+        d->idxmap = idxmap;
+        async_run_on_cpu(cpu, tlb_flush_page_by_mmuidx_async_2,
+                         RUN_ON_CPU_HOST_PTR(d));
+    }
 }
 
 void tlb_flush_page(CPUState *cpu, vaddr addr)
@@ -775,8 +794,6 @@ void tlb_flush_range_by_mmuidx(CPUState *cpu, vaddr addr,
 {
     TLBFlushRangeData d;
 
-    assert_cpu_is_self(cpu);
-
     /*
      * If all bits are significant, and len is small,
      * this devolves to tlb_flush_page.
@@ -797,7 +814,14 @@ void tlb_flush_range_by_mmuidx(CPUState *cpu, vaddr addr,
     d.idxmap = idxmap;
     d.bits = bits;
 
-    tlb_flush_range_by_mmuidx_async_0(cpu, d);
+    if (qemu_cpu_is_self(cpu)) {
+        tlb_flush_range_by_mmuidx_async_0(cpu, d);
+    } else {
+        /* Otherwise allocate a structure, freed by the worker.  */
+        TLBFlushRangeData *p = g_memdup2(&d, sizeof(d));
+        async_run_on_cpu(cpu, tlb_flush_range_by_mmuidx_async_1,
+                         RUN_ON_CPU_HOST_PTR(p));
+    }
 }
 
 void tlb_flush_page_bits_by_mmuidx(CPUState *cpu, vaddr addr,
-- 
2.43.0

Re: [PATCH v2 05/10] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Alex Bennée]

Igor Mammedov <imammedo@redhat.com> writes:

> 1)
> This reverts commit 30933c4fb4f3df95ae44c4c3c86a5df049852c01.
>   ("tcg/cputlb: remove other-cpu capability from TLB flushing")
>
> The commit caused a regression which went unnoticed due to
> affected being disabled by default (DEBUG_TLB_GATE 0)
> Previous patch switched to using tcg_debug_assert() so that
> at least on debug builds assert_cpu_is_self() path would be exercised.
>
> And that lead to exposing regression introduced by [1] with abort during tests.
> to reproduce:
>   $ configure  --target-list=x86_64-softmmu --enable-debug
>   $ make && ./qemu-system-x86_64
>
>   accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx:
>     Assertion `!(cpu)->created || qemu_cpu_is_self(cpu)' failed.
>
> which is triggered by usage outside of cpu thread:
>     x86_cpu_new -> ... ->
>       x86_cpu_realizefn -> cpu_reset -> ... ->
>           tcg_cpu_reset_hold

If the reset case is the only one I don't think we need to revert the
rest of the changes as only tlb_flush needs calling. How about something
like:

--8<---------------cut here---------------start------------->8---
cputlb: introduce tlb_flush_other_cpu for reset use

The commit 30933c4fb4 (tcg/cputlb: remove other-cpu capability from
TLB flushing) introduced a regression that only shows up when
--enable-debug-tcg is used. The main use case of tlb_flush outside of
the current_cpu context is for handling reset and CPU creation. Rather
than revert the commit introduce a new helper and tweak the
documentation to make it clear where it should be used.

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

3 files changed, 26 insertions(+), 5 deletions(-)
include/exec/exec-all.h   | 20 ++++++++++++++++----
accel/tcg/cputlb.c        |  9 +++++++++
accel/tcg/tcg-accel-ops.c |  2 +-

modified   include/exec/exec-all.h
@@ -64,12 +64,24 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, vaddr addr);
  * tlb_flush:
  * @cpu: CPU whose TLB should be flushed
  *
- * Flush the entire TLB for the specified CPU. Most CPU architectures
- * allow the implementation to drop entries from the TLB at any time
- * so this is generally safe. If more selective flushing is required
- * use one of the other functions for efficiency.
+ * Flush the entire TLB for the specified current CPU.
+ *
+ * Most CPU architectures allow the implementation to drop entries
+ * from the TLB at any time so this is generally safe. If more
+ * selective flushing is required use one of the other functions for
+ * efficiency.
  */
 void tlb_flush(CPUState *cpu);
+/**
+ * tlb_flush_other_cpu:
+ * @cpu: CPU whose TLB should be flushed
+ *
+ * Flush the entire TLB for a specified CPU. For cross vCPU flushes
+ * you shuld be using a more selective function. This is really only
+ * used for flushing CPUs being reset from outside their current
+ * context.
+ */
+void tlb_flush_other_cpu(CPUState *cpu);
 /**
  * tlb_flush_all_cpus_synced:
  * @cpu: src CPU of the flush
modified   accel/tcg/cputlb.c
@@ -414,6 +414,15 @@ void tlb_flush(CPUState *cpu)
     tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
 }
 
+void tlb_flush_other_cpu(CPUState *cpu)
+{
+    g_assert(!qemu_cpu_is_self(cpu));
+
+    async_run_on_cpu(cpu,
+                     tlb_flush_by_mmuidx_async_work,
+                     RUN_ON_CPU_HOST_INT(ALL_MMUIDX_BITS));
+}
+
 void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *src_cpu, uint16_t idxmap)
 {
     const run_on_cpu_func fn = tlb_flush_by_mmuidx_async_work;
modified   accel/tcg/tcg-accel-ops.c
@@ -85,7 +85,7 @@ static void tcg_cpu_reset_hold(CPUState *cpu)
 {
     tcg_flush_jmp_cache(cpu);
 
-    tlb_flush(cpu);
+    tlb_flush_other_cpu(cpu);
 }
 
--8<---------------cut here---------------end--------------->8---

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 05/10] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Igor Mammedov]

On Tue, 25 Feb 2025 12:42:24 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > 1)
> > This reverts commit 30933c4fb4f3df95ae44c4c3c86a5df049852c01.
> >   ("tcg/cputlb: remove other-cpu capability from TLB flushing")
> >
> > The commit caused a regression which went unnoticed due to
> > affected being disabled by default (DEBUG_TLB_GATE 0)
> > Previous patch switched to using tcg_debug_assert() so that
> > at least on debug builds assert_cpu_is_self() path would be exercised.
> >
> > And that lead to exposing regression introduced by [1] with abort during tests.
> > to reproduce:
> >   $ configure  --target-list=x86_64-softmmu --enable-debug
> >   $ make && ./qemu-system-x86_64
> >
> >   accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx:
> >     Assertion `!(cpu)->created || qemu_cpu_is_self(cpu)' failed.
> >
> > which is triggered by usage outside of cpu thread:
> >     x86_cpu_new -> ... ->
> >       x86_cpu_realizefn -> cpu_reset -> ... ->
> >           tcg_cpu_reset_hold  
> 
> If the reset case is the only one I don't think we need to revert the
> rest of the changes as only tlb_flush needs calling. How about something
> like:
> 
> --8<---------------cut here---------------start------------->8---
> cputlb: introduce tlb_flush_other_cpu for reset use

while that works for my reproducer it's not sufficient,
'make check' is some tests fails anyway

ex:

10/378 qemu:qtest+qtest-x86_64 / qtest-x86_64/ahci-test ERROR 13.47s killed by signal 6 SIGABRT
stderr:
qemu-system-x86_64: qemu/accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx: Assertion `qemu_cpu_is_self(cpu)' failed.
Broken pipe
qemu/tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)



> 
> The commit 30933c4fb4 (tcg/cputlb: remove other-cpu capability from
> TLB flushing) introduced a regression that only shows up when
> --enable-debug-tcg is used. The main use case of tlb_flush outside of
> the current_cpu context is for handling reset and CPU creation. Rather
> than revert the commit introduce a new helper and tweak the
> documentation to make it clear where it should be used.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> 3 files changed, 26 insertions(+), 5 deletions(-)
> include/exec/exec-all.h   | 20 ++++++++++++++++----
> accel/tcg/cputlb.c        |  9 +++++++++
> accel/tcg/tcg-accel-ops.c |  2 +-
> 
> modified   include/exec/exec-all.h
> @@ -64,12 +64,24 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, vaddr addr);
>   * tlb_flush:
>   * @cpu: CPU whose TLB should be flushed
>   *
> - * Flush the entire TLB for the specified CPU. Most CPU architectures
> - * allow the implementation to drop entries from the TLB at any time
> - * so this is generally safe. If more selective flushing is required
> - * use one of the other functions for efficiency.
> + * Flush the entire TLB for the specified current CPU.
> + *
> + * Most CPU architectures allow the implementation to drop entries
> + * from the TLB at any time so this is generally safe. If more
> + * selective flushing is required use one of the other functions for
> + * efficiency.
>   */
>  void tlb_flush(CPUState *cpu);
> +/**
> + * tlb_flush_other_cpu:
> + * @cpu: CPU whose TLB should be flushed
> + *
> + * Flush the entire TLB for a specified CPU. For cross vCPU flushes
> + * you shuld be using a more selective function. This is really only
> + * used for flushing CPUs being reset from outside their current
> + * context.
> + */
> +void tlb_flush_other_cpu(CPUState *cpu);
>  /**
>   * tlb_flush_all_cpus_synced:
>   * @cpu: src CPU of the flush
> modified   accel/tcg/cputlb.c
> @@ -414,6 +414,15 @@ void tlb_flush(CPUState *cpu)
>      tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
>  }
>  
> +void tlb_flush_other_cpu(CPUState *cpu)
> +{
> +    g_assert(!qemu_cpu_is_self(cpu));
> +
> +    async_run_on_cpu(cpu,
> +                     tlb_flush_by_mmuidx_async_work,
> +                     RUN_ON_CPU_HOST_INT(ALL_MMUIDX_BITS));
> +}
> +
>  void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *src_cpu, uint16_t idxmap)
>  {
>      const run_on_cpu_func fn = tlb_flush_by_mmuidx_async_work;
> modified   accel/tcg/tcg-accel-ops.c
> @@ -85,7 +85,7 @@ static void tcg_cpu_reset_hold(CPUState *cpu)
>  {
>      tcg_flush_jmp_cache(cpu);
>  
> -    tlb_flush(cpu);
> +    tlb_flush_other_cpu(cpu);
>  }
>  
> --8<---------------cut here---------------end--------------->8---
> 

Re: [PATCH v2 05/10] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Igor Mammedov]

On Tue, 25 Feb 2025 18:19:03 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 25 Feb 2025 12:42:24 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > Igor Mammedov <imammedo@redhat.com> writes:
> >   
> > > 1)
> > > This reverts commit 30933c4fb4f3df95ae44c4c3c86a5df049852c01.
> > >   ("tcg/cputlb: remove other-cpu capability from TLB flushing")
> > >
> > > The commit caused a regression which went unnoticed due to
> > > affected being disabled by default (DEBUG_TLB_GATE 0)
> > > Previous patch switched to using tcg_debug_assert() so that
> > > at least on debug builds assert_cpu_is_self() path would be exercised.
> > >
> > > And that lead to exposing regression introduced by [1] with abort during tests.
> > > to reproduce:
> > >   $ configure  --target-list=x86_64-softmmu --enable-debug
> > >   $ make && ./qemu-system-x86_64
> > >
> > >   accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx:
> > >     Assertion `!(cpu)->created || qemu_cpu_is_self(cpu)' failed.
> > >
> > > which is triggered by usage outside of cpu thread:
> > >     x86_cpu_new -> ... ->
> > >       x86_cpu_realizefn -> cpu_reset -> ... ->
> > >           tcg_cpu_reset_hold    
> > 
> > If the reset case is the only one I don't think we need to revert the
> > rest of the changes as only tlb_flush needs calling. How about something
> > like:
> > 
> > --8<---------------cut here---------------start------------->8---
> > cputlb: introduce tlb_flush_other_cpu for reset use  
> 
> while that works for my reproducer it's not sufficient,
> 'make check' is some tests fails anyway
> 
> ex:
> 
> 10/378 qemu:qtest+qtest-x86_64 / qtest-x86_64/ahci-test ERROR 13.47s killed by signal 6 SIGABRT
> stderr:
> qemu-system-x86_64: qemu/accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx: Assertion `qemu_cpu_is_self(cpu)' failed.
> Broken pipe
> qemu/tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)

here is v3 rebased on top of the patch

https://gitlab.com/imammedo/qemu/-/commits/qemu_cpu_cond_v3?ref_type=heads

it seems that reset path is not the only one that relied on 'cpu->created' workaround


> 
> 
> 
> > 
> > The commit 30933c4fb4 (tcg/cputlb: remove other-cpu capability from
> > TLB flushing) introduced a regression that only shows up when
> > --enable-debug-tcg is used. The main use case of tlb_flush outside of
> > the current_cpu context is for handling reset and CPU creation. Rather
> > than revert the commit introduce a new helper and tweak the
> > documentation to make it clear where it should be used.
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > 
> > 3 files changed, 26 insertions(+), 5 deletions(-)
> > include/exec/exec-all.h   | 20 ++++++++++++++++----
> > accel/tcg/cputlb.c        |  9 +++++++++
> > accel/tcg/tcg-accel-ops.c |  2 +-
> > 
> > modified   include/exec/exec-all.h
> > @@ -64,12 +64,24 @@ void tlb_flush_page_all_cpus_synced(CPUState *src, vaddr addr);
> >   * tlb_flush:
> >   * @cpu: CPU whose TLB should be flushed
> >   *
> > - * Flush the entire TLB for the specified CPU. Most CPU architectures
> > - * allow the implementation to drop entries from the TLB at any time
> > - * so this is generally safe. If more selective flushing is required
> > - * use one of the other functions for efficiency.
> > + * Flush the entire TLB for the specified current CPU.
> > + *
> > + * Most CPU architectures allow the implementation to drop entries
> > + * from the TLB at any time so this is generally safe. If more
> > + * selective flushing is required use one of the other functions for
> > + * efficiency.
> >   */
> >  void tlb_flush(CPUState *cpu);
> > +/**
> > + * tlb_flush_other_cpu:
> > + * @cpu: CPU whose TLB should be flushed
> > + *
> > + * Flush the entire TLB for a specified CPU. For cross vCPU flushes
> > + * you shuld be using a more selective function. This is really only
> > + * used for flushing CPUs being reset from outside their current
> > + * context.
> > + */
> > +void tlb_flush_other_cpu(CPUState *cpu);
> >  /**
> >   * tlb_flush_all_cpus_synced:
> >   * @cpu: src CPU of the flush
> > modified   accel/tcg/cputlb.c
> > @@ -414,6 +414,15 @@ void tlb_flush(CPUState *cpu)
> >      tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
> >  }
> >  
> > +void tlb_flush_other_cpu(CPUState *cpu)
> > +{
> > +    g_assert(!qemu_cpu_is_self(cpu));
> > +
> > +    async_run_on_cpu(cpu,
> > +                     tlb_flush_by_mmuidx_async_work,
> > +                     RUN_ON_CPU_HOST_INT(ALL_MMUIDX_BITS));
> > +}
> > +
> >  void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *src_cpu, uint16_t idxmap)
> >  {
> >      const run_on_cpu_func fn = tlb_flush_by_mmuidx_async_work;
> > modified   accel/tcg/tcg-accel-ops.c
> > @@ -85,7 +85,7 @@ static void tcg_cpu_reset_hold(CPUState *cpu)
> >  {
> >      tcg_flush_jmp_cache(cpu);
> >  
> > -    tlb_flush(cpu);
> > +    tlb_flush_other_cpu(cpu);
> >  }
> >  
> > --8<---------------cut here---------------end--------------->8---
> >   
> 

[PATCH v2 06/10] tcg: drop cpu->created check

[Igor Mammedov]

previous commits fixed 2 remaining cases where vcpu might
have had 'cpu->created == false' during 1st vcpu reset (at realize time)
that leads to call chain
      tcg_cpu_reset_hold() => tlb_flush_by_mmuidx()

remove not need anymore check, with cpu->created always being true.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Nicholas Piggin <npiggin@gmail.com>
---
 accel/tcg/cputlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 3d1d7d2409..6ccb173960 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -75,7 +75,7 @@
 } while (0)
 
 #define assert_cpu_is_self(cpu)                             \
-    tcg_debug_assert(!(cpu)->created || qemu_cpu_is_self(cpu))
+    tcg_debug_assert(qemu_cpu_is_self(cpu))
 
 /* run_on_cpu_data.target_ptr should always be big enough for a
  * vaddr even on 32 bit builds
@@ -416,7 +416,7 @@ void tlb_flush_by_mmuidx(CPUState *cpu, uint16_t idxmap)
 {
     tlb_debug("mmu_idx: 0x%" PRIx16 "\n", idxmap);
 
-    if (cpu->created && !qemu_cpu_is_self(cpu)) {
+    if (!qemu_cpu_is_self(cpu)) {
         async_run_on_cpu(cpu, tlb_flush_by_mmuidx_async_work,
                          RUN_ON_CPU_HOST_INT(idxmap));
     } else {
-- 
2.43.0

[PATCH v2 07/10] accel/tcg: Simplify use of &first_cpu in rr_cpu_thread_fn()

[Igor Mammedov]

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

Let vCPUs wait for themselves being ready first, then other ones.
This allows the first thread to starts without the global vcpu
queue (thus &first_cpu) being populated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 accel/tcg/tcg-accel-ops-rr.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index 028b385af9..d9eadd5ec4 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -197,16 +197,19 @@ static void *rr_cpu_thread_fn(void *arg)
     qemu_guest_random_seed_thread_part2(cpu->random_seed);
 
     /* wait for initial kick-off after machine start */
-    while (first_cpu->stopped) {
-        qemu_cond_wait_bql(first_cpu->halt_cond);
+    while (cpu->stopped) {
+        CPUState *iter_cpu;
+
+        qemu_cond_wait_bql(cpu->halt_cond);
 
         /* process any pending work */
-        CPU_FOREACH(cpu) {
-            current_cpu = cpu;
-            qemu_wait_io_event_common(cpu);
+        CPU_FOREACH(iter_cpu) {
+            current_cpu = iter_cpu;
+            qemu_wait_io_event_common(iter_cpu);
         }
     }
 
+    g_assert(first_cpu);
     rr_start_kick_timer();
 
     cpu = first_cpu;
-- 
2.43.0

Re: [PATCH v2 07/10] accel/tcg: Simplify use of &first_cpu in rr_cpu_thread_fn()

[Alex Bennée]

Igor Mammedov <imammedo@redhat.com> writes:

> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Let vCPUs wait for themselves being ready first, then other ones.
> This allows the first thread to starts without the global vcpu
> queue (thus &first_cpu) being populated.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 08/10] cpus: expose only realized vCPUs to global &cpus_queue

[Igor Mammedov]

cpu_list_add() was doing 2 distinct things:
- assign some index to vCPU
- add unrealized (thus in inconsistent state) vCPU to &cpus_queue

Code using CPU_FOREACH() macro would iterate over possibly
unrealized vCPUs, often dealt with special casing.

Instead of working around of vCPU existence in cpus_queue,
split out cpu_index assignment from cpu_list_add(),
and move the later to the end of realize stage,
right before vCPU is let run.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Yanan Wang <wangyanan55@huawei.com>
CC: Zhao Liu <zhao1.liu@intel.com>
---
 include/hw/core/cpu.h |  6 ++++++
 cpu-common.c          | 23 ++++++++++++++---------
 cpu-target.c          |  2 +-
 hw/core/cpu-common.c  |  2 ++
 4 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index fb397cdfc5..c338fd31bd 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -750,6 +750,12 @@ bool cpu_virtio_is_big_endian(CPUState *cpu);
 
 #endif /* CONFIG_USER_ONLY */
 
+/**
+ * cpu_auto_assign_cpu_index:
+ * @cpu: The CPU to be assigned a cpu_index
+ */
+void cpu_auto_assign_cpu_index(CPUState *cpu);
+
 /**
  * cpu_list_add:
  * @cpu: The CPU to be added to the list of CPUs.
diff --git a/cpu-common.c b/cpu-common.c
index 4248b2d727..92f3d00e56 100644
--- a/cpu-common.c
+++ b/cpu-common.c
@@ -71,15 +71,7 @@ int cpu_get_free_index(void)
     return max_cpu_index;
 }
 
-CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
-static unsigned int cpu_list_generation_id;
-
-unsigned int cpu_list_generation_id_get(void)
-{
-    return cpu_list_generation_id;
-}
-
-void cpu_list_add(CPUState *cpu)
+void cpu_auto_assign_cpu_index(CPUState *cpu)
 {
     static bool cpu_index_auto_assigned;
 
@@ -91,6 +83,19 @@ void cpu_list_add(CPUState *cpu)
     } else {
         assert(!cpu_index_auto_assigned);
     }
+}
+
+CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
+static unsigned int cpu_list_generation_id;
+
+unsigned int cpu_list_generation_id_get(void)
+{
+    return cpu_list_generation_id;
+}
+
+void cpu_list_add(CPUState *cpu)
+{
+    QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
     QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
     cpu_list_generation_id++;
 }
diff --git a/cpu-target.c b/cpu-target.c
index 667688332c..0c86c18a50 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -142,7 +142,7 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
     }
 
     /* Wait until cpu initialization complete before exposing cpu. */
-    cpu_list_add(cpu);
+    cpu_auto_assign_cpu_index(cpu);
 
 #ifdef CONFIG_USER_ONLY
     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index cb79566cc5..c29737e5e3 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -211,6 +211,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    cpu_list_add(cpu);
+
     if (dev->hotplugged) {
         cpu_synchronize_post_init(cpu);
         cpu_resume(cpu);
-- 
2.43.0

Re: [PATCH v2 08/10] cpus: expose only realized vCPUs to global &cpus_queue

[Philippe Mathieu-Daudé]

On 7/2/25 17:20, Igor Mammedov wrote:
> cpu_list_add() was doing 2 distinct things:
> - assign some index to vCPU
> - add unrealized (thus in inconsistent state) vCPU to &cpus_queue
> 
> Code using CPU_FOREACH() macro would iterate over possibly
> unrealized vCPUs, often dealt with special casing.
> 
> Instead of working around of vCPU existence in cpus_queue,
> split out cpu_index assignment from cpu_list_add(),

Better split 2 distinct changes in 2 patches for clarity.

> and move the later to the end of realize stage,
> right before vCPU is let run.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: Yanan Wang <wangyanan55@huawei.com>
> CC: Zhao Liu <zhao1.liu@intel.com>
> ---
>   include/hw/core/cpu.h |  6 ++++++
>   cpu-common.c          | 23 ++++++++++++++---------
>   cpu-target.c          |  2 +-
>   hw/core/cpu-common.c  |  2 ++
>   4 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index fb397cdfc5..c338fd31bd 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -750,6 +750,12 @@ bool cpu_virtio_is_big_endian(CPUState *cpu);
>   
>   #endif /* CONFIG_USER_ONLY */
>   
> +/**
> + * cpu_auto_assign_cpu_index:
> + * @cpu: The CPU to be assigned a cpu_index
> + */
> +void cpu_auto_assign_cpu_index(CPUState *cpu);
> +
>   /**
>    * cpu_list_add:
>    * @cpu: The CPU to be added to the list of CPUs.
> diff --git a/cpu-common.c b/cpu-common.c
> index 4248b2d727..92f3d00e56 100644
> --- a/cpu-common.c
> +++ b/cpu-common.c
> @@ -71,15 +71,7 @@ int cpu_get_free_index(void)
>       return max_cpu_index;
>   }
>   
> -CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> -static unsigned int cpu_list_generation_id;
> -
> -unsigned int cpu_list_generation_id_get(void)
> -{
> -    return cpu_list_generation_id;
> -}
> -
> -void cpu_list_add(CPUState *cpu)
> +void cpu_auto_assign_cpu_index(CPUState *cpu)
>   {
>       static bool cpu_index_auto_assigned;
>   
> @@ -91,6 +83,19 @@ void cpu_list_add(CPUState *cpu)
>       } else {
>           assert(!cpu_index_auto_assigned);
>       }
> +}
> +
> +CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> +static unsigned int cpu_list_generation_id;
> +
> +unsigned int cpu_list_generation_id_get(void)
> +{
> +    return cpu_list_generation_id;
> +}
> +
> +void cpu_list_add(CPUState *cpu)
> +{
> +    QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
>       QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
>       cpu_list_generation_id++;
>   }
> diff --git a/cpu-target.c b/cpu-target.c
> index 667688332c..0c86c18a50 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -142,7 +142,7 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
>       }
>   
>       /* Wait until cpu initialization complete before exposing cpu. */
> -    cpu_list_add(cpu);
> +    cpu_auto_assign_cpu_index(cpu);
>   
>   #ifdef CONFIG_USER_ONLY
>       assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index cb79566cc5..c29737e5e3 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -211,6 +211,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>           }
>       }
>   
> +    cpu_list_add(cpu);
> +
>       if (dev->hotplugged) {
>           cpu_synchronize_post_init(cpu);
>           cpu_resume(cpu);

Re: [PATCH v2 08/10] cpus: expose only realized vCPUs to global &cpus_queue

[Igor Mammedov]

On Wed, 26 Feb 2025 08:16:52 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 7/2/25 17:20, Igor Mammedov wrote:
> > cpu_list_add() was doing 2 distinct things:
> > - assign some index to vCPU
> > - add unrealized (thus in inconsistent state) vCPU to &cpus_queue
> > 
> > Code using CPU_FOREACH() macro would iterate over possibly
> > unrealized vCPUs, often dealt with special casing.
> > 
> > Instead of working around of vCPU existence in cpus_queue,
> > split out cpu_index assignment from cpu_list_add(),  
> 
> Better split 2 distinct changes in 2 patches for clarity.


Will do it later, once folks decide how to fix broken TCG reset path.

do you mean:
 #1 - introduce  cpu_auto_assign_cpu_index()
 #2 - move cpu_list_add() to later stage but keep cpu_auto_assign_cpu_index()
      where it's now?


> 
> > and move the later to the end of realize stage,
> > right before vCPU is let run.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > CC: Yanan Wang <wangyanan55@huawei.com>
> > CC: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   include/hw/core/cpu.h |  6 ++++++
> >   cpu-common.c          | 23 ++++++++++++++---------
> >   cpu-target.c          |  2 +-
> >   hw/core/cpu-common.c  |  2 ++
> >   4 files changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index fb397cdfc5..c338fd31bd 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -750,6 +750,12 @@ bool cpu_virtio_is_big_endian(CPUState *cpu);
> >   
> >   #endif /* CONFIG_USER_ONLY */
> >   
> > +/**
> > + * cpu_auto_assign_cpu_index:
> > + * @cpu: The CPU to be assigned a cpu_index
> > + */
> > +void cpu_auto_assign_cpu_index(CPUState *cpu);
> > +
> >   /**
> >    * cpu_list_add:
> >    * @cpu: The CPU to be added to the list of CPUs.
> > diff --git a/cpu-common.c b/cpu-common.c
> > index 4248b2d727..92f3d00e56 100644
> > --- a/cpu-common.c
> > +++ b/cpu-common.c
> > @@ -71,15 +71,7 @@ int cpu_get_free_index(void)
> >       return max_cpu_index;
> >   }
> >   
> > -CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> > -static unsigned int cpu_list_generation_id;
> > -
> > -unsigned int cpu_list_generation_id_get(void)
> > -{
> > -    return cpu_list_generation_id;
> > -}
> > -
> > -void cpu_list_add(CPUState *cpu)
> > +void cpu_auto_assign_cpu_index(CPUState *cpu)
> >   {
> >       static bool cpu_index_auto_assigned;
> >   
> > @@ -91,6 +83,19 @@ void cpu_list_add(CPUState *cpu)
> >       } else {
> >           assert(!cpu_index_auto_assigned);
> >       }
> > +}
> > +
> > +CPUTailQ cpus_queue = QTAILQ_HEAD_INITIALIZER(cpus_queue);
> > +static unsigned int cpu_list_generation_id;
> > +
> > +unsigned int cpu_list_generation_id_get(void)
> > +{
> > +    return cpu_list_generation_id;
> > +}
> > +
> > +void cpu_list_add(CPUState *cpu)
> > +{
> > +    QEMU_LOCK_GUARD(&qemu_cpu_list_lock);
> >       QTAILQ_INSERT_TAIL_RCU(&cpus_queue, cpu, node);
> >       cpu_list_generation_id++;
> >   }
> > diff --git a/cpu-target.c b/cpu-target.c
> > index 667688332c..0c86c18a50 100644
> > --- a/cpu-target.c
> > +++ b/cpu-target.c
> > @@ -142,7 +142,7 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >       }
> >   
> >       /* Wait until cpu initialization complete before exposing cpu. */
> > -    cpu_list_add(cpu);
> > +    cpu_auto_assign_cpu_index(cpu);
> >   
> >   #ifdef CONFIG_USER_ONLY
> >       assert(qdev_get_vmsd(DEVICE(cpu)) == NULL ||
> > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> > index cb79566cc5..c29737e5e3 100644
> > --- a/hw/core/cpu-common.c
> > +++ b/hw/core/cpu-common.c
> > @@ -211,6 +211,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> >           }
> >       }
> >   
> > +    cpu_list_add(cpu);
> > +
> >       if (dev->hotplugged) {
> >           cpu_synchronize_post_init(cpu);
> >           cpu_resume(cpu);  
> 

Re: [PATCH v2 08/10] cpus: expose only realized vCPUs to global &cpus_queue

[Philippe Mathieu-Daudé]

On 3/3/25 14:09, Igor Mammedov wrote:
> On Wed, 26 Feb 2025 08:16:52 +0100
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
>> On 7/2/25 17:20, Igor Mammedov wrote:
>>> cpu_list_add() was doing 2 distinct things:
>>> - assign some index to vCPU
>>> - add unrealized (thus in inconsistent state) vCPU to &cpus_queue
>>>
>>> Code using CPU_FOREACH() macro would iterate over possibly
>>> unrealized vCPUs, often dealt with special casing.
>>>
>>> Instead of working around of vCPU existence in cpus_queue,
>>> split out cpu_index assignment from cpu_list_add(),
>>
>> Better split 2 distinct changes in 2 patches for clarity.
> 
> 
> Will do it later, once folks decide how to fix broken TCG reset path.
> 
> do you mean:
>   #1 - introduce  cpu_auto_assign_cpu_index()
>   #2 - move cpu_list_add() to later stage but keep cpu_auto_assign_cpu_index()
>        where it's now?

Exactly, thanks!

[PATCH v2 09/10] accel/kvm: Assert vCPU is created when calling kvm_dirty_ring_reap*()

[Igor Mammedov]

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

Previous commits made sure vCPUs are realized before accelerators
(such KVM) use them. Ensure that by asserting the vCPU is created,
no need to return.

For more context, see commit 56adee407fc ("kvm: dirty-ring: Fix race
with vcpu creation").

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: kvm@vger.kernel.org
CC: peterx@redhat.com
---
 accel/kvm/kvm-all.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c65b790433..cb56d120a9 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -831,13 +831,11 @@ static uint32_t kvm_dirty_ring_reap_one(KVMState *s, CPUState *cpu)
     uint32_t count = 0, fetch = cpu->kvm_fetch_index;
 
     /*
-     * It's possible that we race with vcpu creation code where the vcpu is
+     * It's not possible that we race with vcpu creation code where the vcpu is
      * put onto the vcpus list but not yet initialized the dirty ring
-     * structures.  If so, skip it.
+     * structures.
      */
-    if (!cpu->created) {
-        return 0;
-    }
+    assert(cpu->created);
 
     assert(dirty_gfns && ring_size);
     trace_kvm_dirty_ring_reap_vcpu(cpu->cpu_index);
-- 
2.43.0

[PATCH v2 10/10] accel/kvm: Remove unreachable assertion in kvm_dirty_ring_reap*()

[Igor Mammedov]

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

Previous commit passed all our CI tests, this assertion being
never triggered. Remove it as dead code.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: kvm@vger.kernel.org
---
 accel/kvm/kvm-all.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index cb56d120a9..814b1a53eb 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -830,13 +830,6 @@ static uint32_t kvm_dirty_ring_reap_one(KVMState *s, CPUState *cpu)
     uint32_t ring_size = s->kvm_dirty_ring_size;
     uint32_t count = 0, fetch = cpu->kvm_fetch_index;
 
-    /*
-     * It's not possible that we race with vcpu creation code where the vcpu is
-     * put onto the vcpus list but not yet initialized the dirty ring
-     * structures.
-     */
-    assert(cpu->created);
-
     assert(dirty_gfns && ring_size);
     trace_kvm_dirty_ring_reap_vcpu(cpu->cpu_index);
 
-- 
2.43.0

Re: [PATCH v2 00/10] accel: Only include qdev-realized vCPUs in global &cpus_queue

[Igor Mammedov]

On Fri,  7 Feb 2025 17:20:38 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> Changelog:
>   * drop wire/unwire hooks patches
>   * drop unrealize related patches
>   * include fixed up patches from
>        [PATCH 0/6] tcg: fix qemu crash when add assert_cpu_is_self() is enabled
>         and cleanups related to cpu->created check
>         https://patchew.org/QEMU/20250129134436.1240740-1-imammedo@redhat.com/
>     as it's related to the topic (well, modulo bsd cleanup)
>   * CI mostly green modulo rust failure on Ubuntu
>      https://gitlab.com/imammedo/qemu/-/pipelines/1660855467
> 
> The goal of this series is to expose vCPUs in a stable state
> to the accelerators, in particular the QDev 'REALIZED' step.
> 
> To do this we split out cpu_index assignment into a separate step,
> and move call cpu_list_add() to the end of CPU realize stage.
> 
> I expect these changes to allow CPUState::cpu_index clarifications
> and simplifications, but this will be addressed (and commented) in
> a separate series.
> 
> As result, the series also
>  * fix regression intoroduced by
>       30933c4fb4f3d ("tcg/cputlb: remove other-cpu capability from TLB flushing")
>    for deatials see 'tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()'
>  * drops no longer needed workaround 'cpu->check' due to vCPU being exposed
>    too early in cpus_queue.

Richard,
gentle ping,
can you pick it up is it looks reasonable.
 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Richard Henderson <richard.henderson@linaro.org>
> CC: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> CC: Alex Bennée <alex.bennee@linaro.org>
> 
> Igor Mammedov (7):
>   bsd-user: drop not longer used target_reset_cpu()
>   loongarch: reset vcpu after it's created
>   m68k: reset vcpu after it's created
>   tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()
>   Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"
>   tcg: drop cpu->created check
>   cpus: expose only realized vCPUs to global &cpus_queue
> 
> Philippe Mathieu-Daudé (3):
>   accel/tcg: Simplify use of &first_cpu in rr_cpu_thread_fn()
>   accel/kvm: Assert vCPU is created when calling kvm_dirty_ring_reap*()
>   accel/kvm: Remove unreachable assertion in kvm_dirty_ring_reap*()
> 
>  bsd-user/aarch64/target_arch_cpu.h |  5 ---
>  bsd-user/arm/target_arch_cpu.h     |  4 ---
>  bsd-user/i386/target_arch_cpu.h    |  5 ---
>  bsd-user/riscv/target_arch_cpu.h   |  4 ---
>  bsd-user/x86_64/target_arch_cpu.h  |  5 ---
>  include/hw/core/cpu.h              |  6 ++++
>  accel/kvm/kvm-all.c                |  9 ------
>  accel/tcg/cputlb.c                 | 49 +++++++++++++++++++++---------
>  accel/tcg/tcg-accel-ops-rr.c       | 13 +++++---
>  cpu-common.c                       | 23 ++++++++------
>  cpu-target.c                       |  2 +-
>  hw/core/cpu-common.c               |  2 ++
>  target/loongarch/cpu.c             |  2 +-
>  target/m68k/cpu.c                  |  2 +-
>  14 files changed, 68 insertions(+), 63 deletions(-)
>