[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec.c: simplify the breakpoint invalidation lo
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH] exec.c: simplify the breakpoint invalidation logic |
Date: |
Tue, 6 Dec 2016 14:54:20 +0000 |
On 6 December 2016 at 14:51, Alex Bennée <address@hidden> 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 <address@hidden>
> ---
> 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