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
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,
--
2.43.0On 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?
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,
>
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,
> >
>
© 2016 - 2025 Red Hat, Inc.