Re: [PATCH v2 05/10] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 25 Feb 2025 18:19:03 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 25 Feb 2025 12:42:24 +0000
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > Igor Mammedov <imammedo@redhat.com> writes:
> >   
> > > 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 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    
> > 
> > If the reset case is the only one I don't think we need to revert the
> > rest of the changes as only tlb_flush needs calling. How about something
> > like:
> > 
> > --8<---------------cut here---------------start------------->8---
> > cputlb: introduce tlb_flush_other_cpu for reset use  
> 
> while that works for my reproducer it's not sufficient,
> 'make check' is some tests fails anyway
> 
> ex:
> 
> 10/378 qemu:qtest+qtest-x86_64 / qtest-x86_64/ahci-test ERROR 13.47s killed by signal 6 SIGABRT
> stderr:
> qemu-system-x86_64: qemu/accel/tcg/cputlb.c:419: tlb_flush_by_mmuidx: Assertion `qemu_cpu_is_self(cpu)' failed.
> Broken pipe
> qemu/tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)

here is v3 rebased on top of the patch

https://gitlab.com/imammedo/qemu/-/commits/qemu_cpu_cond_v3?ref_type=heads

it seems that reset path is not the only one that relied on 'cpu->created' workaround


> 
> 
> 
> > 
> > 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>
> > 
> > 3 files changed, 26 insertions(+), 5 deletions(-)
> > include/exec/exec-all.h   | 20 ++++++++++++++++----
> > accel/tcg/cputlb.c        |  9 +++++++++
> > accel/tcg/tcg-accel-ops.c |  2 +-
> > 
> > modified   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
> > modified   accel/tcg/cputlb.c
> > @@ -414,6 +414,15 @@ void tlb_flush(CPUState *cpu)
> >      tlb_flush_by_mmuidx(cpu, ALL_MMUIDX_BITS);
> >  }
> >  
> > +void tlb_flush_other_cpu(CPUState *cpu)
> > +{
> > +    g_assert(!qemu_cpu_is_self(cpu));
> > +
> > +    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;
> > modified   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);
> >  }
> >  
> > --8<---------------cut here---------------end--------------->8---
> >   
>