| Message ID | 20161206145104.16048-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 6 December 2016 at 14:51, Alex Bennée <alex.bennee@linaro.org> wrote: > A bug (1647683) was reported showing a crash when removing > breakpoints. The reproducer was bisected to 3359baad when tb_flush was > finally made thread safe. While in MTTCG the locking in > breakpoint_invalidate should have prevented any problems currently > tb_lock() is a NOP for system emulation. > > On top of it all the invalidation code was special-cased between user > and system emulation which was ugly. As we now have a safe tb_flush() > we might as well use that when breakpoints and added and removed. This > is the same thing we do for single-stepping and as this is during > debugging it isn't a major performance concern. > > Reported-by: Julian Brown > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > exec.c | 31 ++++--------------------------- > 1 file changed, 4 insertions(+), 27 deletions(-) > > diff --git a/exec.c b/exec.c > index 3d867f1..e991221 100644 > --- a/exec.c > +++ b/exec.c > @@ -685,29 +685,6 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) > } > > #if defined(CONFIG_USER_ONLY) > -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) > -{ > - mmap_lock(); > - tb_lock(); > - tb_invalidate_phys_page_range(pc, pc + 1, 0); > - tb_unlock(); > - mmap_unlock(); > -} > -#else > -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) > -{ > - MemTxAttrs attrs; > - hwaddr phys = cpu_get_phys_page_attrs_debug(cpu, pc, &attrs); > - int asidx = cpu_asidx_from_attrs(cpu, attrs); > - if (phys != -1) { > - /* Locks grabbed by tb_invalidate_phys_addr */ > - tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as, > - phys | (pc & ~TARGET_PAGE_MASK)); > - } > -} > -#endif > - > -#if defined(CONFIG_USER_ONLY) > void cpu_watchpoint_remove_all(CPUState *cpu, int mask) > > { > @@ -839,7 +816,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, > QTAILQ_INSERT_TAIL(&cpu->breakpoints, bp, entry); > } > > - breakpoint_invalidate(cpu, pc); > + tb_flush(cpu); > > if (breakpoint) { > *breakpoint = bp; > @@ -855,6 +832,7 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) > QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) { > if (bp->pc == pc && bp->flags == flags) { > cpu_breakpoint_remove_by_ref(cpu, bp); > + tb_flush(cpu); > return 0; > } > } > @@ -865,9 +843,6 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) > void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint) > { > QTAILQ_REMOVE(&cpu->breakpoints, breakpoint, entry); > - > - breakpoint_invalidate(cpu, breakpoint->pc); > - > g_free(breakpoint); > } > > @@ -881,6 +856,8 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) > cpu_breakpoint_remove_by_ref(cpu, bp); > } > } > + > + tb_flush(cpu); > } > > /* enable or disable single step mode. EXCP_DEBUG is returned by the I think this is the wrong direction. We only need to invalidate the TBs for the specific location the breakpoint is at. Even if we for the moment want to apply a big hammer of doing a full tb flush on breakpoint to fix this issue for this release, we shouldn't drop the indirection through breakpoint_invalidate(), because then we can't go back to invalidating just the parts we need easily later. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 6 December 2016 at 14:51, Alex Bennée <alex.bennee@linaro.org> wrote: >> A bug (1647683) was reported showing a crash when removing >> breakpoints. The reproducer was bisected to 3359baad when tb_flush was >> finally made thread safe. While in MTTCG the locking in >> breakpoint_invalidate should have prevented any problems currently >> tb_lock() is a NOP for system emulation. >> >> On top of it all the invalidation code was special-cased between user >> and system emulation which was ugly. As we now have a safe tb_flush() >> we might as well use that when breakpoints and added and removed. This >> is the same thing we do for single-stepping and as this is during >> debugging it isn't a major performance concern. >> >> Reported-by: Julian Brown >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> exec.c | 31 ++++--------------------------- >> 1 file changed, 4 insertions(+), 27 deletions(-) >> >> diff --git a/exec.c b/exec.c >> index 3d867f1..e991221 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -685,29 +685,6 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) >> } >> >> #if defined(CONFIG_USER_ONLY) >> -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) >> -{ >> - mmap_lock(); >> - tb_lock(); >> - tb_invalidate_phys_page_range(pc, pc + 1, 0); >> - tb_unlock(); >> - mmap_unlock(); >> -} >> -#else >> -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) >> -{ >> - MemTxAttrs attrs; >> - hwaddr phys = cpu_get_phys_page_attrs_debug(cpu, pc, &attrs); >> - int asidx = cpu_asidx_from_attrs(cpu, attrs); >> - if (phys != -1) { >> - /* Locks grabbed by tb_invalidate_phys_addr */ >> - tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as, >> - phys | (pc & ~TARGET_PAGE_MASK)); >> - } >> -} >> -#endif >> - >> -#if defined(CONFIG_USER_ONLY) >> void cpu_watchpoint_remove_all(CPUState *cpu, int mask) >> >> { >> @@ -839,7 +816,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, >> QTAILQ_INSERT_TAIL(&cpu->breakpoints, bp, entry); >> } >> >> - breakpoint_invalidate(cpu, pc); >> + tb_flush(cpu); >> >> if (breakpoint) { >> *breakpoint = bp; >> @@ -855,6 +832,7 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) >> QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) { >> if (bp->pc == pc && bp->flags == flags) { >> cpu_breakpoint_remove_by_ref(cpu, bp); >> + tb_flush(cpu); >> return 0; >> } >> } >> @@ -865,9 +843,6 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) >> void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint) >> { >> QTAILQ_REMOVE(&cpu->breakpoints, breakpoint, entry); >> - >> - breakpoint_invalidate(cpu, breakpoint->pc); >> - >> g_free(breakpoint); >> } >> >> @@ -881,6 +856,8 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) >> cpu_breakpoint_remove_by_ref(cpu, bp); >> } >> } >> + >> + tb_flush(cpu); >> } >> >> /* enable or disable single step mode. EXCP_DEBUG is returned by the > > I think this is the wrong direction. We only need to invalidate > the TBs for the specific location the breakpoint is at. > Even if we for the moment want to apply a big hammer of doing > a full tb flush on breakpoint to fix this issue for this release, > we shouldn't drop the indirection through breakpoint_invalidate(), > because then we can't go back to invalidating just the parts we need > easily later. Why would we? It's not like fresh code generation is particularly slow, especially in a debugging situation when you've just stopped the machine. The self-modifying-code paths make more sense to be partial in their invalidations although I've never really measured quite how pathalogical running a JIT inside QEMU is. -- Alex Bennée
On 6 December 2016 at 15:14, Alex Bennée <alex.bennee@linaro.org> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> I think this is the wrong direction. We only need to invalidate >> the TBs for the specific location the breakpoint is at. >> Even if we for the moment want to apply a big hammer of doing >> a full tb flush on breakpoint to fix this issue for this release, >> we shouldn't drop the indirection through breakpoint_invalidate(), >> because then we can't go back to invalidating just the parts we need >> easily later. > > Why would we? It's not like fresh code generation is particularly slow, > especially in a debugging situation when you've just stopped the > machine. Because a wholescale tb_flush() is a "this is probably wrong" flag, and a great way to hide bugs. It also makes the gdbstub more invasive than it strictly has to be. We have less than 10 calls to tb_flush() in the whole system and I think they could all use examination to see whether they're really correct. thanks -- PMM
> I think this is the wrong direction. We only need to invalidate > the TBs for the specific location the breakpoint is at. > Even if we for the moment want to apply a big hammer of doing > a full tb flush on breakpoint to fix this issue for this release, > we shouldn't drop the indirection through breakpoint_invalidate(), > because then we can't go back to invalidating just the parts we need > easily later. I agree with Peter, but still this patch should be fine for 2.8 and we can revert it when MTTCG locking is merged. Paolo
diff --git a/exec.c b/exec.c index 3d867f1..e991221 100644 --- a/exec.c +++ b/exec.c @@ -685,29 +685,6 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) } #if defined(CONFIG_USER_ONLY) -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) -{ - mmap_lock(); - tb_lock(); - tb_invalidate_phys_page_range(pc, pc + 1, 0); - tb_unlock(); - mmap_unlock(); -} -#else -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) -{ - MemTxAttrs attrs; - hwaddr phys = cpu_get_phys_page_attrs_debug(cpu, pc, &attrs); - int asidx = cpu_asidx_from_attrs(cpu, attrs); - if (phys != -1) { - /* Locks grabbed by tb_invalidate_phys_addr */ - tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as, - phys | (pc & ~TARGET_PAGE_MASK)); - } -} -#endif - -#if defined(CONFIG_USER_ONLY) void cpu_watchpoint_remove_all(CPUState *cpu, int mask) { @@ -839,7 +816,7 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags, QTAILQ_INSERT_TAIL(&cpu->breakpoints, bp, entry); } - breakpoint_invalidate(cpu, pc); + tb_flush(cpu); if (breakpoint) { *breakpoint = bp; @@ -855,6 +832,7 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) { if (bp->pc == pc && bp->flags == flags) { cpu_breakpoint_remove_by_ref(cpu, bp); + tb_flush(cpu); return 0; } } @@ -865,9 +843,6 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags) void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *breakpoint) { QTAILQ_REMOVE(&cpu->breakpoints, breakpoint, entry); - - breakpoint_invalidate(cpu, breakpoint->pc); - g_free(breakpoint); } @@ -881,6 +856,8 @@ void cpu_breakpoint_remove_all(CPUState *cpu, int mask) cpu_breakpoint_remove_by_ref(cpu, bp); } } + + tb_flush(cpu); } /* enable or disable single step mode. EXCP_DEBUG is returned by the
A bug (1647683) was reported showing a crash when removing breakpoints. The reproducer was bisected to 3359baad when tb_flush was finally made thread safe. While in MTTCG the locking in breakpoint_invalidate should have prevented any problems currently tb_lock() is a NOP for system emulation. On top of it all the invalidation code was special-cased between user and system emulation which was ugly. As we now have a safe tb_flush() we might as well use that when breakpoints and added and removed. This is the same thing we do for single-stepping and as this is during debugging it isn't a major performance concern. Reported-by: Julian Brown Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- exec.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) -- 2.10.2