[PATCH 0/4] cputlb: add tlb_flush_other_cpu

[PATCH 0/4] cputlb: add tlb_flush_other_cpu

[Alex Bennée]

This is based on one of the fixes from Igor's series:

  20250207162048.1890669-1-imammedo@redhat.com

Rather than reverting the patch I cleaned up a couple of cases where
we either didn't need to do a flush or we could trivially defer to
when the CPU started running. For the other cases I introduced
tlb_flush_other_cpu() for users like reset and post migration. Any
other users wanting to do a cross-vcpu blat everything function like
tlb_flush() are almost certainly wanting to use a different more
targeted helper which still supports queuing async work.

Alex.

Alex Bennée (3):
  target/ppc: drop ppc_tlb_invalidate_all from cpu_reset
  target/hppa: defer hppa_ptlbe until CPU starts running
  cputlb: introduce tlb_flush_other_cpu for reset use

Igor Mammedov (1):
  tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

 include/exec/exec-all.h   | 20 ++++++++++++++++----
 accel/tcg/cputlb.c        | 18 +++++++++++++-----
 accel/tcg/tcg-accel-ops.c |  2 +-
 cpu-target.c              |  2 +-
 target/hppa/cpu.c         | 10 +++++++++-
 target/i386/machine.c     |  2 +-
 target/ppc/cpu_init.c     |  3 ---
 7 files changed, 41 insertions(+), 16 deletions(-)

-- 
2.39.5

[PATCH 1/4] target/ppc: drop ppc_tlb_invalidate_all from cpu_reset

[Alex Bennée]

The vCPU parent already triggers a tb_flush so this is un-needed:

  #0  tlb_flush_other_cpu (cpu=0x555556df8630) at ../../accel/tcg/cputlb.c:419
  #1  0x0000555555ee38c9 in tcg_cpu_reset_hold (cpu=0x555556df8630) at ../../accel/tcg/tcg-accel-ops.c:88
  #2  0x0000555555bc29e5 in cpu_exec_reset_hold (cpu=0x555556df8630) at ../../system/cpus.c:208
  #3  0x00005555558932c3 in cpu_common_reset_hold (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../hw/core/cpu-common.c:139
  #4  0x0000555555d480b1 in ppc_cpu_reset_hold (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../target/ppc/cpu_init.c:7200
  #5  0x0000555555ef28f0 in resettable_phase_hold (obj=0x555556df8630, opaque=0x0, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:162
  #6  0x0000555555ef24f4 in resettable_assert_reset (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:58
  #7  0x0000555555ef244c in resettable_reset (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:45
  #8  0x0000555555eef525 in device_cold_reset (dev=0x555556df8630) at ../../hw/core/qdev.c:239
  #9  0x00005555558931ab in cpu_reset (cpu=0x555556df8630) at ../../hw/core/cpu-common.c:114
  #10 0x0000555555d1ec6b in ppce500_cpu_reset (opaque=0x555556df8630) at ../../hw/ppc/e500.c:785
  #11 0x000055555595c410 in legacy_reset_hold (obj=0x555556e6bbc0, type=RESET_TYPE_COLD) at ../../hw/core/reset.c:76
  #12 0x0000555555ef28f0 in resettable_phase_hold (obj=0x555556e6bbc0, opaque=0x0, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:162

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/ppc/cpu_init.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 062a6e85fb..f987b75c4f 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7242,9 +7242,6 @@ static void ppc_cpu_reset_hold(Object *obj, ResetType type)
     if (tcg_enabled()) {
         cpu_breakpoint_remove_all(cs, BP_CPU);
         cpu_watchpoint_remove_all(cs, BP_CPU);
-        if (env->mmu_model != POWERPC_MMU_REAL) {
-            ppc_tlb_invalidate_all(env);
-        }
         pmu_mmcr01a_updated(env);
     }
 
-- 
2.39.5

[PATCH 2/4] target/hppa: defer hppa_ptlbe until CPU starts running

[Alex Bennée]

Since 30933c4fb4 (tcg/cputlb: remove other-cpu capability from TLB flushing)
we don't expect non-CPU callers to the tlb_flush() code. Normally I
would drop the call anyway as the common cpu_reset() code will call
tlb_flush anyway. However as the flush function does more than that,
and is called from helpers instead defer it with an async_run_on_cpu.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/hppa/cpu.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 5655677431..b631af381c 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -168,6 +168,14 @@ void hppa_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
 
     cpu_loop_exit(cs);
 }
+
+static void hppa_clear_ptlbe(CPUState *cpu, run_on_cpu_data opaque)
+{
+    CPUHPPAState *env = (CPUHPPAState *) opaque.host_ptr;
+    hppa_ptlbe(env);
+}
+
+
 #endif /* CONFIG_USER_ONLY */
 
 static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -191,7 +199,7 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
 
         cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                         hppa_cpu_alarm_timer, cpu);
-        hppa_ptlbe(&cpu->env);
+        async_run_on_cpu(cs, hppa_clear_ptlbe, RUN_ON_CPU_HOST_PTR(&cpu->env));
     }
 #endif
 
-- 
2.39.5

[PATCH 3/4] cputlb: introduce tlb_flush_other_cpu for reset use

[Alex Bennée]

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>

---
v2
  - appraently reset can come from both cpu context and outside
  - add cpu_common_post_load fixes
---
 include/exec/exec-all.h   | 20 ++++++++++++++++----
 accel/tcg/cputlb.c        | 11 +++++++++++
 accel/tcg/tcg-accel-ops.c |  2 +-
 cpu-target.c              |  2 +-
 target/i386/machine.c     |  2 +-
 5 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index d9045c9ac4..cf030001ca 100644
--- a/include/exec/exec-all.h
+++ b/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
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ad158050a1..fc16a576f0 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -417,6 +417,17 @@ void tlb_flush(CPUState *cpu)
     tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
 }
 
+void tlb_flush_other_cpu(CPUState *cpu)
+{
+    if (qemu_cpu_is_self(cpu)) {
+        tlb_flush(cpu);
+    } else {
+        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;
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 6e3f1fa92b..e85d317d34 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/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);
 }
 
 /* mask must never be zero, except for A20 change call */
diff --git a/cpu-target.c b/cpu-target.c
index 667688332c..8eb1633c02 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -56,7 +56,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
     /* 0x01 was CPU_INTERRUPT_EXIT. This line can be removed when the
        version_id is increased. */
     cpu->interrupt_request &= ~0x01;
-    tlb_flush(cpu);
+    tlb_flush_other_cpu(cpu);
 
     /* loadvm has just updated the content of RAM, bypassing the
      * usual mechanisms that ensure we flush TBs for writes to
diff --git a/target/i386/machine.c b/target/i386/machine.c
index d9d4f25d1a..e66f46758a 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -401,7 +401,7 @@ static int cpu_post_load(void *opaque, int version_id)
         env->dr[7] = dr7 & ~(DR7_GLOBAL_BP_MASK | DR7_LOCAL_BP_MASK);
         cpu_x86_update_dr7(env, dr7);
     }
-    tlb_flush(cs);
+    tlb_flush_other_cpu(cs);
     return 0;
 }
 
-- 
2.39.5

[PATCH 4/4] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Alex Bennée]

From: Igor Mammedov <imammedo@redhat.com>

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>
Message-Id: <20250207162048.1890669-5-imammedo@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 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 fc16a576f0..65b04b1055 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -73,11 +73,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.39.5

Re: [PATCH 1/4] target/ppc: drop ppc_tlb_invalidate_all from cpu_reset

[Richard Henderson]

On 2/25/25 10:46, Alex Bennée wrote:
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 062a6e85fb..f987b75c4f 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7242,9 +7242,6 @@ static void ppc_cpu_reset_hold(Object *obj, ResetType type)
>       if (tcg_enabled()) {
>           cpu_breakpoint_remove_all(cs, BP_CPU);
>           cpu_watchpoint_remove_all(cs, BP_CPU);
> -        if (env->mmu_model != POWERPC_MMU_REAL) {
> -            ppc_tlb_invalidate_all(env);
> -        }
>           pmu_mmcr01a_updated(env);
>       }
>   

Nack.  This is flushing the thing emulating the on-chip hardware, not the softmmu tlb.


r~

Re: [PATCH 2/4] target/hppa: defer hppa_ptlbe until CPU starts running

[Richard Henderson]

On 2/25/25 10:46, Alex Bennée wrote:
> @@ -191,7 +199,7 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>   
>           cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                           hppa_cpu_alarm_timer, cpu);
> -        hppa_ptlbe(&cpu->env);
> +        async_run_on_cpu(cs, hppa_clear_ptlbe, RUN_ON_CPU_HOST_PTR(&cpu->env));

Nack, this is emulation of hardware, not softmmu.


r~

Re: [PATCH 2/4] target/hppa: defer hppa_ptlbe until CPU starts running

[Richard Henderson]

On 2/25/25 11:33, Richard Henderson wrote:
> On 2/25/25 10:46, Alex Bennée wrote:
>> @@ -191,7 +199,7 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>>           cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>                                           hppa_cpu_alarm_timer, cpu);
>> -        hppa_ptlbe(&cpu->env);
>> +        async_run_on_cpu(cs, hppa_clear_ptlbe, RUN_ON_CPU_HOST_PTR(&cpu->env));
> 
> Nack, this is emulation of hardware, not softmmu.

Hmm.  I see what you're thinking about though: this function, after resetting the data 
structures associated with the hardware emulation, also calls the softmmu flush.

If we absolutely need to do so, I suppose delaying the hardware emulation flush to the 
work queue isn't the worst solution.  This is where the hppa patch is more correct than 
the ppc patch which completely eliminated the hardware emulation flush.


r~

Re: [PATCH 3/4] cputlb: introduce tlb_flush_other_cpu for reset use

[Richard Henderson]

On 2/25/25 10:46, Alex Bennée wrote:
> 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>
> 
> ---
> v2
>    - appraently reset can come from both cpu context and outside
>    - add cpu_common_post_load fixes
> ---
>   include/exec/exec-all.h   | 20 ++++++++++++++++----
>   accel/tcg/cputlb.c        | 11 +++++++++++
>   accel/tcg/tcg-accel-ops.c |  2 +-
>   cpu-target.c              |  2 +-
>   target/i386/machine.c     |  2 +-
>   5 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index d9045c9ac4..cf030001ca 100644
> --- a/include/exec/exec-all.h
> +++ b/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
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index ad158050a1..fc16a576f0 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -417,6 +417,17 @@ void tlb_flush(CPUState *cpu)
>       tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
>   }
>   
> +void tlb_flush_other_cpu(CPUState *cpu)
> +{
> +    if (qemu_cpu_is_self(cpu)) {
> +        tlb_flush(cpu);
> +    } else {
> +        async_run_on_cpu(cpu,
> +                         tlb_flush_by_mmuidx_async_work,
> +                         RUN_ON_CPU_HOST_INT(ALL_MMUIDX_BITS));
> +    }
> +}

I'm not convinced this is necessary.

> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index 6e3f1fa92b..e85d317d34 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/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);
>   }

I would really like to believe that at this point, hold phase, the cpu is *not* running. 
Therefore it is safe to zero out the softmmu tlb data structures.

>   
>   /* mask must never be zero, except for A20 change call */
> diff --git a/cpu-target.c b/cpu-target.c
> index 667688332c..8eb1633c02 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -56,7 +56,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
>       /* 0x01 was CPU_INTERRUPT_EXIT. This line can be removed when the
>          version_id is increased. */
>       cpu->interrupt_request &= ~0x01;
> -    tlb_flush(cpu);
> +    tlb_flush_other_cpu(cpu);

Likewise, in post_load, the cpu is *not* running.

> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index d9d4f25d1a..e66f46758a 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -401,7 +401,7 @@ static int cpu_post_load(void *opaque, int version_id)
>           env->dr[7] = dr7 & ~(DR7_GLOBAL_BP_MASK | DR7_LOCAL_BP_MASK);
>           cpu_x86_update_dr7(env, dr7);
>       }
> -    tlb_flush(cs);
> +    tlb_flush_other_cpu(cs);
>       return 0;

Likewise.


r~

Re: [PATCH 4/4] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Richard Henderson]

On 2/25/25 10:46, Alex Bennée wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> 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>
> Message-Id: <20250207162048.1890669-5-imammedo@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   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 fc16a576f0..65b04b1055 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -73,11 +73,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))

I think this check is just wrong or incomplete.

The intent here is to check that we're not attempting to modify the softmmu tlb 
asynchronously while a cpu is running.

(0) A synchronous flush to the current cpu is (obviously?) ok.
(1) A flush to a cpu that is not yet created is (or should be) a no-op.

Not checked here are any of the other reasons a flush might be ok:

(2) The system as a whole is stopped, on the way in from migration/vmload.
(3) The cpu is offline, on the way in from poweroff/reset.

If we decide that {1, 2, 3} are too complicated to check, then perhaps the solution to 
queue flushes to the cpu's workqueue is the appropriate solution.  But so far all I see is 
that we have an incomplete check, and no ready explanation for why that check can't be 
improved.


r~

Re: [PATCH 4/4] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Richard Henderson]

On 2/25/25 12:02, Richard Henderson wrote:
> Not checked here are any of the other reasons a flush might be ok:
> 
> (2) The system as a whole is stopped, on the way in from migration/vmload.
> (3) The cpu is offline, on the way in from poweroff/reset.
(4) Running in round-robin mode, so there is *never* a race between cpus.

Anything else I've forgotten?


r~

Re: [PATCH 4/4] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Igor Mammedov]

On Tue, 25 Feb 2025 12:02:02 -0800
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 2/25/25 10:46, Alex Bennée wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > 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>
> > Message-Id: <20250207162048.1890669-5-imammedo@redhat.com>
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >   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 fc16a576f0..65b04b1055 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -73,11 +73,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))  
> 
> I think this check is just wrong or incomplete.

the point of the path is to bring out check out of ifdef limbo.
Whether it's correct or not it's up to another patch to fix.


> The intent here is to check that we're not attempting to modify the softmmu tlb 
> asynchronously while a cpu is running.
> 
> (0) A synchronous flush to the current cpu is (obviously?) ok.
> (1) A flush to a cpu that is not yet created is (or should be) a no-op.

my another patch that was touching the check

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

is trying to remove (abusing)usage of cpu->created
which should be used only for syncing main loop and
a to be created vcpu thread.
The creation of vcpu is not really complete yet by
this point so it depends on luck (being nop).

End goal from my side is to get rid of users that
use cpu->created as workaround to move from one
incomplete vcpu state to another still incomplete state.

We can drop the check after reset/postload paths are
fixed to schedule async flush.
 
> Not checked here are any of the other reasons a flush might be ok:
> 
> (2) The system as a whole is stopped, on the way in from migration/vmload.
> (3) The cpu is offline, on the way in from poweroff/reset.
> 
> If we decide that {1, 2, 3} are too complicated to check, then perhaps the solution to 
> queue flushes to the cpu's workqueue is the appropriate solution.  But so far all I see is 
> that we have an incomplete check, and no ready explanation for why that check can't be 
> improved.
> 
> 
> r~
> 

Re: [PATCH 4/4] tcg:tlb: use tcg_debug_assert() in assert_cpu_is_self()

[Igor Mammedov]

On Tue, 25 Feb 2025 12:04:40 -0800
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 2/25/25 12:02, Richard Henderson wrote:
> > Not checked here are any of the other reasons a flush might be ok:
> > 
> > (2) The system as a whole is stopped, on the way in from migration/vmload.
> > (3) The cpu is offline, on the way in from poweroff/reset.  
> (4) Running in round-robin mode, so there is *never* a race between cpus.
> 
> Anything else I've forgotten?

looking at x86 reset path
 * we have 2 resets per vcpu from main loop,
   when vcpu is created (at realize time and at system_reset time).
   this probably are nop for tcg as you said.
   (it likely applies to all targets)

 * And also a reset triggered by IPI (run in vcpu thread),
   which likely should do flush to clear whatever context
   vcpu had before reset.

Re: [PATCH 3/4] cputlb: introduce tlb_flush_other_cpu for reset use

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 2/25/25 10:46, Alex Bennée wrote:
>> 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>
>> ---
>> v2
>>    - appraently reset can come from both cpu context and outside
>>    - add cpu_common_post_load fixes
>> ---
>>   include/exec/exec-all.h   | 20 ++++++++++++++++----
>>   accel/tcg/cputlb.c        | 11 +++++++++++
>>   accel/tcg/tcg-accel-ops.c |  2 +-
>>   cpu-target.c              |  2 +-
>>   target/i386/machine.c     |  2 +-
>>   5 files changed, 30 insertions(+), 7 deletions(-)
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index d9045c9ac4..cf030001ca 100644
>> --- a/include/exec/exec-all.h
>> +++ b/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
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index ad158050a1..fc16a576f0 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -417,6 +417,17 @@ void tlb_flush(CPUState *cpu)
>>       tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
>>   }
>>   +void tlb_flush_other_cpu(CPUState *cpu)
>> +{
>> +    if (qemu_cpu_is_self(cpu)) {
>> +        tlb_flush(cpu);
>> +    } else {
>> +        async_run_on_cpu(cpu,
>> +                         tlb_flush_by_mmuidx_async_work,
>> +                         RUN_ON_CPU_HOST_INT(ALL_MMUIDX_BITS));
>> +    }
>> +}
>
> I'm not convinced this is necessary.

I guess we want something like:


/* tlb_reset() - reset the TLB when the CPU is not running
 * cs: the cpu
 *
 * Only to be used when the CPU is definitely not running
 */

void tlb_reset(CPUState *cs) {
     g_assert(cs->cpu_stopped);

    for (i = 0; i < NB_MMU_MODES; i++) {
        tlb_mmu_flush_locked(&cpu->neg.tlb.d[i], &cpu->neg.tlb.f[i]);
    }
}

?

>
>> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
>> index 6e3f1fa92b..e85d317d34 100644
>> --- a/accel/tcg/tcg-accel-ops.c
>> +++ b/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);
>>   }
>
> I would really like to believe that at this point, hold phase, the cpu
> is *not* running. Therefore it is safe to zero out the softmmu tlb
> data structures.
>
>>     /* mask must never be zero, except for A20 change call */
>> diff --git a/cpu-target.c b/cpu-target.c
>> index 667688332c..8eb1633c02 100644
>> --- a/cpu-target.c
>> +++ b/cpu-target.c
>> @@ -56,7 +56,7 @@ static int cpu_common_post_load(void *opaque, int version_id)
>>       /* 0x01 was CPU_INTERRUPT_EXIT. This line can be removed when the
>>          version_id is increased. */
>>       cpu->interrupt_request &= ~0x01;
>> -    tlb_flush(cpu);
>> +    tlb_flush_other_cpu(cpu);
>
> Likewise, in post_load, the cpu is *not* running.
>
>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>> index d9d4f25d1a..e66f46758a 100644
>> --- a/target/i386/machine.c
>> +++ b/target/i386/machine.c
>> @@ -401,7 +401,7 @@ static int cpu_post_load(void *opaque, int version_id)
>>           env->dr[7] = dr7 & ~(DR7_GLOBAL_BP_MASK | DR7_LOCAL_BP_MASK);
>>           cpu_x86_update_dr7(env, dr7);
>>       }
>> -    tlb_flush(cs);
>> +    tlb_flush_other_cpu(cs);
>>       return 0;
>
> Likewise.
>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/4] cputlb: introduce tlb_flush_other_cpu for reset use

[Richard Henderson]

On 2/26/25 06:29, Alex Bennée wrote:
> I guess we want something like:
> 
> 
> /* tlb_reset() - reset the TLB when the CPU is not running
>   * cs: the cpu
>   *
>   * Only to be used when the CPU is definitely not running
>   */
> 
> void tlb_reset(CPUState *cs) {
>       g_assert(cs->cpu_stopped);
> 
>      for (i = 0; i < NB_MMU_MODES; i++) {
>          tlb_mmu_flush_locked(&cpu->neg.tlb.d[i], &cpu->neg.tlb.f[i]);
>      }
> }
> 
> ?

I like the name, and the separate assert.
I'm not convinced skipping the tlb resize and (especially) accounting is a good idea.

I suspect that the tlb_flush_by_mmuidx_async_work should be split vs its 
assert_cpu_is_self, and you just should use that.  I'll note that tcg_cpu_reset_hold and 
tlb_flush_by_mmuidx_async_work both call tcg_flush_jmp_cache, so we've got a double-flush 
in there.

If you don't want to use tlb_flush_by_mmuidx_async_work, I think you need at minimum

- take the lock
- tlb_window_reset()
- honor and update cpu->neg.tlb.c.dirty


r~

Re: [PATCH 1/4] target/ppc: drop ppc_tlb_invalidate_all from cpu_reset

[Nicholas Piggin]

On Wed Feb 26, 2025 at 4:46 AM AEST, Alex Bennée wrote:
> The vCPU parent already triggers a tb_flush so this is un-needed:
>
>   #0  tlb_flush_other_cpu (cpu=0x555556df8630) at ../../accel/tcg/cputlb.c:419
>   #1  0x0000555555ee38c9 in tcg_cpu_reset_hold (cpu=0x555556df8630) at ../../accel/tcg/tcg-accel-ops.c:88
>   #2  0x0000555555bc29e5 in cpu_exec_reset_hold (cpu=0x555556df8630) at ../../system/cpus.c:208
>   #3  0x00005555558932c3 in cpu_common_reset_hold (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../hw/core/cpu-common.c:139
>   #4  0x0000555555d480b1 in ppc_cpu_reset_hold (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../target/ppc/cpu_init.c:7200
>   #5  0x0000555555ef28f0 in resettable_phase_hold (obj=0x555556df8630, opaque=0x0, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:162
>   #6  0x0000555555ef24f4 in resettable_assert_reset (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:58
>   #7  0x0000555555ef244c in resettable_reset (obj=0x555556df8630, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:45
>   #8  0x0000555555eef525 in device_cold_reset (dev=0x555556df8630) at ../../hw/core/qdev.c:239
>   #9  0x00005555558931ab in cpu_reset (cpu=0x555556df8630) at ../../hw/core/cpu-common.c:114
>   #10 0x0000555555d1ec6b in ppce500_cpu_reset (opaque=0x555556df8630) at ../../hw/ppc/e500.c:785
>   #11 0x000055555595c410 in legacy_reset_hold (obj=0x555556e6bbc0, type=RESET_TYPE_COLD) at ../../hw/core/reset.c:76
>   #12 0x0000555555ef28f0 in resettable_phase_hold (obj=0x555556e6bbc0, opaque=0x0, type=RESET_TYPE_COLD) at ../../hw/core/resettable.c:162
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/ppc/cpu_init.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 062a6e85fb..f987b75c4f 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7242,9 +7242,6 @@ static void ppc_cpu_reset_hold(Object *obj, ResetType type)
>      if (tcg_enabled()) {
>          cpu_breakpoint_remove_all(cs, BP_CPU);
>          cpu_watchpoint_remove_all(cs, BP_CPU);
> -        if (env->mmu_model != POWERPC_MMU_REAL) {
> -            ppc_tlb_invalidate_all(env);
> -        }

This invalidates architectural TLBs (software-loaded TLBs) as well, so I 
don't think we can get rid of it.

Thanks,
Nick

Re: [PATCH 2/4] target/hppa: defer hppa_ptlbe until CPU starts running

[Nicholas Piggin]

On Wed Feb 26, 2025 at 5:38 AM AEST, Richard Henderson wrote:
> On 2/25/25 11:33, Richard Henderson wrote:
>> On 2/25/25 10:46, Alex Bennée wrote:
>>> @@ -191,7 +199,7 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>>>           cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>>                                           hppa_cpu_alarm_timer, cpu);
>>> -        hppa_ptlbe(&cpu->env);
>>> +        async_run_on_cpu(cs, hppa_clear_ptlbe, RUN_ON_CPU_HOST_PTR(&cpu->env));
>> 
>> Nack, this is emulation of hardware, not softmmu.
>
> Hmm.  I see what you're thinking about though: this function, after resetting the data 
> structures associated with the hardware emulation, also calls the softmmu flush.
>
> If we absolutely need to do so, I suppose delaying the hardware emulation flush to the 
> work queue isn't the worst solution.  This is where the hppa patch is more correct than 
> the ppc patch which completely eliminated the hardware emulation flush.

Could we expose a function that performs the hardware state reset,
and leave the TCG flushing to the TCG CPU reset?

Thanks,
Nick

Re: [PATCH 2/4] target/hppa: defer hppa_ptlbe until CPU starts running

[Alex Bennée]

"Nicholas Piggin" <npiggin@gmail.com> writes:

> On Wed Feb 26, 2025 at 5:38 AM AEST, Richard Henderson wrote:
>> On 2/25/25 11:33, Richard Henderson wrote:
>>> On 2/25/25 10:46, Alex Bennée wrote:
>>>> @@ -191,7 +199,7 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>>>>           cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>>>>                                           hppa_cpu_alarm_timer, cpu);
>>>> -        hppa_ptlbe(&cpu->env);
>>>> +        async_run_on_cpu(cs, hppa_clear_ptlbe, RUN_ON_CPU_HOST_PTR(&cpu->env));
>>> 
>>> Nack, this is emulation of hardware, not softmmu.
>>
>> Hmm.  I see what you're thinking about though: this function, after resetting the data 
>> structures associated with the hardware emulation, also calls the softmmu flush.
>>
>> If we absolutely need to do so, I suppose delaying the hardware emulation flush to the 
>> work queue isn't the worst solution.  This is where the hppa patch is more correct than 
>> the ppc patch which completely eliminated the hardware emulation flush.
>
> Could we expose a function that performs the hardware state reset,
> and leave the TCG flushing to the TCG CPU reset?

I did consider that - but the function is also called from helpers at
which point you also do need to flush the softmmu TLB. However someone
with better knowledge of the architecture might be able to do a slightly
more refined flush (individual pages or mmuidxs) in those cases.

>
> Thanks,
> Nick

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro