| Message ID | 20250225184628.3590671-4-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cputlb: add tlb_flush_other_cpu | expand |
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~
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; }
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(-)