Re: [PATCH 5/6] Revert "tcg/cputlb: remove other-cpu capability from TLB flushing"

[Igor Mammedov]

On Wed, 29 Jan 2025 19:33:30 +0100 (CET)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Wed, 29 Jan 2025, Igor Mammedov wrote:
> > 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 moved switched to using tcg_debug_assert() so that  
> 
> The verb "moved" not needed and left from editing?
yep, I'll fix it up in case of respin


> 
> Regards,
> BALATON Zoltan
> 
> > 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
> >
> > Drop offending commit for now, until a propper fix that doesn't break
> > 'make check' is available.
> >
> > PS:
> > fixup g_memdup() checkpatch error s/g_memdup/g_memdup2/
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > I'll leave it upto TCG folz to fix it up propperly.
> >
> > CC: npiggin@gmail.com
> > CC: richard.henderson@linaro.org
> > ---
> > accel/tcg/cputlb.c | 42 +++++++++++++++++++++++++++++++++---------
> > 1 file changed, 33 insertions(+), 9 deletions(-)
> >
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 71207d6dbf..db1713b3ca 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -416,9 +416,12 @@ void tlb_flush_by_mmuidx(CPUState *cpu, uint16_t 
> > idxmap)
> > {
> >     tlb_debug("mmu_idx: 0x%" PRIx16 "\n", idxmap);
> >
> > -    assert_cpu_is_self(cpu);
> > -
> > -    tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(idxmap));
> > +    if (cpu->created && !qemu_cpu_is_self(cpu)) {
> > +        async_run_on_cpu(cpu, tlb_flush_by_mmuidx_async_work,
> > +                         RUN_ON_CPU_HOST_INT(idxmap));
> > +    } else {
> > +        tlb_flush_by_mmuidx_async_work(cpu, RUN_ON_CPU_HOST_INT(idxmap));
> > +    }
> > }
> >
> > void tlb_flush(CPUState *cpu)
> > @@ -607,12 +610,28 @@ void tlb_flush_page_by_mmuidx(CPUState *cpu, vaddr 
> > addr, uint16_t idxmap)
> > {
> >     tlb_debug("addr: %016" VADDR_PRIx " mmu_idx:%" PRIx16 "\n", addr, 
> > idxmap);
> >
> > -    assert_cpu_is_self(cpu);
> > -
> >     /* This should already be page aligned */
> >     addr &= TARGET_PAGE_MASK;
> >
> > -    tlb_flush_page_by_mmuidx_async_0(cpu, addr, idxmap);
> > +    if (qemu_cpu_is_self(cpu)) {
> > +        tlb_flush_page_by_mmuidx_async_0(cpu, addr, idxmap);
> > +    } else if (idxmap < TARGET_PAGE_SIZE) {
> > +        /*
> > +         * Most targets have only a few mmu_idx.  In the case where
> > +         * we can stuff idxmap into the low TARGET_PAGE_BITS, avoid
> > +         * allocating memory for this operation.
> > +         */
> > +        async_run_on_cpu(cpu, tlb_flush_page_by_mmuidx_async_1,
> > +                         RUN_ON_CPU_TARGET_PTR(addr | idxmap));
> > +    } else {
> > +        TLBFlushPageByMMUIdxData *d = g_new(TLBFlushPageByMMUIdxData, 1);
> > +
> > +        /* Otherwise allocate a structure, freed by the worker.  */
> > +        d->addr = addr;
> > +        d->idxmap = idxmap;
> > +        async_run_on_cpu(cpu, tlb_flush_page_by_mmuidx_async_2,
> > +                         RUN_ON_CPU_HOST_PTR(d));
> > +    }
> > }
> >
> > void tlb_flush_page(CPUState *cpu, vaddr addr)
> > @@ -775,8 +794,6 @@ void tlb_flush_range_by_mmuidx(CPUState *cpu, vaddr 
> > addr,
> > {
> >     TLBFlushRangeData d;
> >
> > -    assert_cpu_is_self(cpu);
> > -
> >     /*
> >      * If all bits are significant, and len is small,
> >      * this devolves to tlb_flush_page.
> > @@ -797,7 +814,14 @@ void tlb_flush_range_by_mmuidx(CPUState *cpu, vaddr 
> > addr,
> >     d.idxmap = idxmap;
> >     d.bits = bits;
> >
> > -    tlb_flush_range_by_mmuidx_async_0(cpu, d);
> > +    if (qemu_cpu_is_self(cpu)) {
> > +        tlb_flush_range_by_mmuidx_async_0(cpu, d);
> > +    } else {
> > +        /* Otherwise allocate a structure, freed by the worker.  */
> > +        TLBFlushRangeData *p = g_memdup2(&d, sizeof(d));
> > +        async_run_on_cpu(cpu, tlb_flush_range_by_mmuidx_async_1,
> > +                         RUN_ON_CPU_HOST_PTR(p));
> > +    }
> > }
> >
> > void tlb_flush_page_bits_by_mmuidx(CPUState *cpu, vaddr addr,
> >  
>