exec.c: simplify the breakpoint invalidation logic

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(-)

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