[PATCH 0/6] tcg: fix qemu crash when add assert_cpu_is_self() is enabled and cleanups related to cpu->created check

[PATCH 0/6] tcg: fix qemu crash when add assert_cpu_is_self() is enabled and cleanups related to cpu->created check

[Igor Mammedov]

1st 3 patches are cleanups around cpu_reset logic
4th patch enables assert_cpu_is_self() macro on --enable-debug builds
5th drops offending patch
    (since my understanding of TCG is close to nill, so I'd leave it up to
     TCG folks to fix if needed properly) 
6th is removing no longer needed cpu->created check, since
    by the time it's invoked, cpu->created == true

CC: alex.bennee@linaro.org
CC: richard.henderson@linaro.org
CC: pbonzini@redhat.com 
CC: imp@bsdimp.com
CC: kevans@freebsd.org
CC: gaosong@loongson.cn
CC: laurent@vivier.eu

Igor Mammedov (6):
  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

 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 ---
 accel/tcg/cputlb.c                 | 53 +++++++++++++++++++++---------
 target/loongarch/cpu.c             |  2 +-
 target/m68k/cpu.c                  |  2 +-
 8 files changed, 39 insertions(+), 41 deletions(-)

-- 
2.43.0

[PATCH 1/6] 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>
---
 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

[PATCH 2/6] 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().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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

[PATCH 3/6] 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().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 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

[PATCH 4/6] 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>
---
 accel/tcg/cputlb.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b4ccf0cdcb..71207d6dbf 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -61,8 +61,8 @@
 #  define DEBUG_TLB_LOG_GATE 0
 # endif
 #else
-# define DEBUG_TLB_GATE 0
-# define DEBUG_TLB_LOG_GATE 0
+# define DEBUG_TLB_GATE 1
+# define DEBUG_TLB_LOG_GATE 1
 #endif
 
 #define tlb_debug(fmt, ...) do { \
@@ -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 4/6] 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>
> ---
>  accel/tcg/cputlb.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index b4ccf0cdcb..71207d6dbf 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -61,8 +61,8 @@
>  #  define DEBUG_TLB_LOG_GATE 0
>  # endif
>  #else
> -# define DEBUG_TLB_GATE 0
> -# define DEBUG_TLB_LOG_GATE 0
> +# define DEBUG_TLB_GATE 1
> +# define DEBUG_TLB_LOG_GATE 1
>  #endif

This shouldn't be changed because it still gates the much more expensive
tlb_debug() which hasn't yet been converted to trace points.

>  
>  #define tlb_debug(fmt, ...) do { \
> @@ -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))
>

This is fine.


>  /* run_on_cpu_data.target_ptr should always be big enough for a
>   * vaddr even on 32 bit builds

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH v2 4/6] 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

[PATCH 5/6] 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 moved 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: richard.henderson@linaro.org
---
 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 71207d6dbf..db1713b3ca 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 5/6] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[BALATON Zoltan]

On Wed, 29 Jan 2025, Igor Mammedov wrote:
> 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 moved switched to using tcg_debug_assert() so that

The verb "moved" not needed and left from editing?

Regards,
BALATON Zoltan

> 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: richard.henderson@linaro.org
> ---
> 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 71207d6dbf..db1713b3ca 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,
>

Re: [PATCH 5/6] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Igor Mammedov]

On Wed, 29 Jan 2025 19:33:30 +0100 (CET)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Wed, 29 Jan 2025, Igor Mammedov wrote:
> > 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 moved switched to using tcg_debug_assert() so that  
> 
> The verb "moved" not needed and left from editing?
yep, I'll fix it up in case of respin


> 
> Regards,
> BALATON Zoltan
> 
> > 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: richard.henderson@linaro.org
> > ---
> > 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 71207d6dbf..db1713b3ca 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,
> >  
> 

[PATCH 6/6] 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>
---
 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 db1713b3ca..f4f3965518 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