[PATCH 0/2] flush CPU TB cache in breakpoint_invalidate

[PATCH 0/2] flush CPU TB cache in breakpoint_invalidate

[Max Filippov]

Hello,

this series changes breakpoint_invalidate to unconditionally do
tb_flush and then changes remaining users of tb_invalidate_phys_addr
to do the same and removes tb_invalidate_phys_addr.

Changes RFC->v1:
- do tb_flush in breakpoint_invalidate unconditionally
- add tb_invalidate_phys_addr cleanup.

Max Filippov (2):
  exec: flush CPU TB cache in breakpoint_invalidate
  exec: drop tb_invalidate_phys_addr

 exec.c                     | 44 ++++++++------------------------------
 include/exec/exec-all.h    |  3 ---
 target/xtensa/dbg_helper.c | 19 +++-------------
 3 files changed, 12 insertions(+), 54 deletions(-)

-- 
2.20.1

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

[Max Filippov]

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 <jcmvbkbc@gmail.com>
---
Changes RFC->v1:
- do tb_flush in breakpoint_invalidate unconditionally

 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
 
-- 
2.20.1

[PATCH 2/2] exec: drop tb_invalidate_phys_addr

[Max Filippov]

The only remaining user of tb_invalidate_phys_addr is target/xtensa
instruction breakpoint code and it is better to use tb_flush there.

Drop tb_invalidate_phys_addr implementations and declarations.
Use tb_flush in xtensa IBREAK helpers.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 exec.c                     | 29 ++---------------------------
 include/exec/exec-all.h    |  3 ---
 target/xtensa/dbg_helper.c | 19 +++----------------
 3 files changed, 5 insertions(+), 46 deletions(-)

diff --git a/exec.c b/exec.c
index 1709b760edc1..4d20fc005520 100644
--- a/exec.c
+++ b/exec.c
@@ -983,38 +983,13 @@ const char *parse_cpu_option(const char *cpu_option)
 }
 
 #if defined(CONFIG_USER_ONLY)
-void tb_invalidate_phys_addr(target_ulong addr)
+static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
     mmap_lock();
-    tb_invalidate_phys_page_range(addr, addr + 1);
+    tb_invalidate_phys_page_range(pc, pc + 1);
     mmap_unlock();
 }
-
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-    tb_invalidate_phys_addr(pc);
-}
 #else
-void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
-{
-    ram_addr_t ram_addr;
-    MemoryRegion *mr;
-    hwaddr l = 1;
-
-    if (!tcg_enabled()) {
-        return;
-    }
-
-    RCU_READ_LOCK_GUARD();
-    mr = address_space_translate(as, addr, &addr, &l, false, attrs);
-    if (!(memory_region_is_ram(mr)
-          || memory_region_is_romd(mr))) {
-        return;
-    }
-    ram_addr = memory_region_get_ram_addr(mr) + addr;
-    tb_invalidate_phys_page_range(ram_addr, ram_addr + 1);
-}
-
 static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
     /*
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index d85e610e85b9..585fe7ff430c 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -449,10 +449,7 @@ static inline uint32_t curr_cflags(void)
 
 /* TranslationBlock invalidate API */
 #if defined(CONFIG_USER_ONLY)
-void tb_invalidate_phys_addr(target_ulong addr);
 void tb_invalidate_phys_range(target_ulong start, target_ulong end);
-#else
-void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs);
 #endif
 void tb_flush(CPUState *cpu);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
diff --git a/target/xtensa/dbg_helper.c b/target/xtensa/dbg_helper.c
index be1f81107b43..2481dc326fba 100644
--- a/target/xtensa/dbg_helper.c
+++ b/target/xtensa/dbg_helper.c
@@ -33,19 +33,6 @@
 #include "exec/exec-all.h"
 #include "exec/address-spaces.h"
 
-static void tb_invalidate_virtual_addr(CPUXtensaState *env, uint32_t vaddr)
-{
-    uint32_t paddr;
-    uint32_t page_size;
-    unsigned access;
-    int ret = xtensa_get_physical_addr(env, false, vaddr, 2, 0,
-                                       &paddr, &page_size, &access);
-    if (ret == 0) {
-        tb_invalidate_phys_addr(&address_space_memory, paddr,
-                                MEMTXATTRS_UNSPECIFIED);
-    }
-}
-
 void HELPER(wsr_ibreakenable)(CPUXtensaState *env, uint32_t v)
 {
     uint32_t change = v ^ env->sregs[IBREAKENABLE];
@@ -53,7 +40,8 @@ void HELPER(wsr_ibreakenable)(CPUXtensaState *env, uint32_t v)
 
     for (i = 0; i < env->config->nibreak; ++i) {
         if (change & (1 << i)) {
-            tb_invalidate_virtual_addr(env, env->sregs[IBREAKA + i]);
+            tb_flush(env_cpu(env));
+            break;
         }
     }
     env->sregs[IBREAKENABLE] = v & ((1 << env->config->nibreak) - 1);
@@ -62,8 +50,7 @@ void HELPER(wsr_ibreakenable)(CPUXtensaState *env, uint32_t v)
 void HELPER(wsr_ibreaka)(CPUXtensaState *env, uint32_t i, uint32_t v)
 {
     if (env->sregs[IBREAKENABLE] & (1 << i) && env->sregs[IBREAKA + i] != v) {
-        tb_invalidate_virtual_addr(env, env->sregs[IBREAKA + i]);
-        tb_invalidate_virtual_addr(env, v);
+        tb_flush(env_cpu(env));
     }
     env->sregs[IBREAKA + i] = v;
 }
-- 
2.20.1

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 <jcmvbkbc@gmail.com>
> ---
> Changes RFC->v1:
> - do tb_flush in breakpoint_invalidate unconditionally

Unlike Paolo, I don't think this is a good idea.

If I was going to change anything here, I'd change this to not use
cpu_get_phys_page_attrs_debug but using the caching available from the actual
cputlb, using cc->tlb_fill() in probe mode -- something akin to probe_access(),
but not returning a host address, nor handling watchpoints nor notdirty.

This would help flushing too much by distinguishing different tbs for the same
virtual address mapping to a different physical address.


r~

Re: [PATCH 2/2] exec: drop tb_invalidate_phys_addr

[Richard Henderson]

On 11/27/19 10:06 PM, Max Filippov wrote:
> The only remaining user of tb_invalidate_phys_addr is target/xtensa
> instruction breakpoint code and it is better to use tb_flush there.
> 
> Drop tb_invalidate_phys_addr implementations and declarations.
> Use tb_flush in xtensa IBREAK helpers.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  exec.c                     | 29 ++---------------------------
>  include/exec/exec-all.h    |  3 ---
>  target/xtensa/dbg_helper.c | 19 +++----------------
>  3 files changed, 5 insertions(+), 46 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Though perhaps split in half, so the xtensa patch comes first.


r~

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

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> 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 <jcmvbkbc@gmail.com>
>> ---
>> Changes RFC->v1:
>> - do tb_flush in breakpoint_invalidate unconditionally
>
> Unlike Paolo, I don't think this is a good idea.

We previously had a general tb_flush during the MTTCG implementation as
a temporary fix. It was changed back in 406bc339b0 and it would be nice
to minimise the flushing of code if we can. While most interactive users
aren't going to notice the temporary slow down it would suck for any
automated gdb scripting.

>
> If I was going to change anything here, I'd change this to not use
> cpu_get_phys_page_attrs_debug but using the caching available from the actual
> cputlb, using cc->tlb_fill() in probe mode -- something akin to probe_access(),
> but not returning a host address, nor handling watchpoints nor notdirty.
>
> This would help flushing too much by distinguishing different tbs for the same
> virtual address mapping to a different physical address.
>
>
> r~


-- 
Alex Bennée

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 <jcmvbkbc@gmail.com>
> ---
> 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
>  
> 

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

[Max Filippov]

On Wed, Feb 5, 2020 at 3:00 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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 <jcmvbkbc@gmail.com>
> > ---
> > 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.

I don't see how this can fix this issue: it's not the list of TBs that
we want to
invalidate, it's the TBs that get associated with new virtual addresses that
are currently causing the issue, right?

>  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.

Will do.

-- 
Thanks.
-- Max