Re: [PATCH 1/2] exec: flush CPU TB cache in breakpoint_invalidate

[Richard Henderson]

On 11/27/19 10:06 PM, Max Filippov wrote:
> When a breakpoint is inserted at location for which there's currently no
> virtual to physical translation no action is taken on CPU TB cache. If a
> TB for that virtual address already exists but is not visible ATM the
> breakpoint won't be hit next time an instruction at that address will be
> executed.
> 
> Flush entire CPU TB cache in breakpoint_invalidate to force
> re-translation of all TBs for the breakpoint address.
> 
> This change fixes the following scenario:
> - linux user application is running
> - a breakpoint is inserted from QEMU gdbstub for a user address that is
>   not currently present in the target CPU TLB
> - an instruction at that address is executed, but the external debugger
>   doesn't get control.
> 
> Signed-off-by: Max Filippov <address@hidden>
> ---
> Changes RFC->v1:
> - do tb_flush in breakpoint_invalidate unconditionally

I know I had reservations about this, but we now have two patches on list that
fix the problem in this way.

What I would *like* is for each CPUBreakpoint to maintain a list of the TBs to
which it has been applied, so that each can be invalidated.  Our current
management of breakpoints are IMO sloppy.

That said, I don't really have time to work on cleaning this up myself in the
short term, and this is fixing a real bug.  Therefore, I am going to queue this
to tcg-next.

I would still like patch 2/2 to be split, and that can probably go through an
xtensa branch.


r~

> 
>  exec.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index ffdb5185353b..1709b760edc1 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1017,14 +1017,13 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr 
> addr, MemTxAttrs attrs)
>  
>  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), attrs);
> -    }
> +    /*
> +     * There may not be a virtual to physical translation for the pc
> +     * right now, but there may exist cached TB for this pc.
> +     * Flush the whole TB cache to force re-translation of such TBs.
> +     * This is heavyweight, but we're debugging anyway.
> +     */
> +    tb_flush(cpu);
>  }
>  #endif
>  
>