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

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~

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

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~