Richard Henderson <richard.hender...@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.ben...@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
