[RFC] exec: flush CPU TB cache when breakpoint address translation fails

[RFC] exec: flush CPU TB cache when breakpoint address translation fails

[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 when a breakpoint is set for a virtual address
that cannot be translated to physical address.

This change fixes the following workflow:
- 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>
---
 exec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/exec.c b/exec.c
index ffdb5185353b..918945f8097e 100644
--- a/exec.c
+++ b/exec.c
@@ -1024,6 +1024,8 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
         /* Locks grabbed by tb_invalidate_phys_addr */
         tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
                                 phys | (pc & ~TARGET_PAGE_MASK), attrs);
+    } else {
+        tb_flush(cpu);
     }
 }
 #endif
-- 
2.20.1

Re: [RFC] exec: flush CPU TB cache when breakpoint address translation fails

[Paolo Bonzini]

On 26/11/19 23:26, 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 when a breakpoint is set for a virtual address
> that cannot be translated to physical address.
> 
> This change fixes the following workflow:
> - 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.

That's quite heavyweight, but perhaps since it's debugging we might as
well _always_ just flush the TB cache?

In any case, it deserves a comment in the code.

Paolo

Re: [RFC] exec: flush CPU TB cache when breakpoint address translation fails

[Alex Bennée]

Max Filippov <jcmvbkbc@gmail.com> writes:

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

So the userspace has run once but is currently paged out?

>
> Flush entire CPU TB cache when a breakpoint is set for a virtual address
> that cannot be translated to physical address.
>
> This change fixes the following workflow:
> - 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>
> ---
>  exec.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index ffdb5185353b..918945f8097e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1024,6 +1024,8 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>          /* Locks grabbed by tb_invalidate_phys_addr */
>          tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
>                                  phys | (pc & ~TARGET_PAGE_MASK), attrs);
> +    } else {
> +        tb_flush(cpu);
>      }
>  }
>  #endif


-- 
Alex Bennée

Re: [RFC] exec: flush CPU TB cache when breakpoint address translation fails

[Max Filippov]

On Wed, Nov 27, 2019 at 11:06 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> Max Filippov <jcmvbkbc@gmail.com> writes:
>
> > 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.
>
> So the userspace has run once but is currently paged out?

Yes, but not necessarily paged out, just not in the CPU TLB.
Or it has run to completion and when you start it next time
it gets loaded to the same physical pages.

-- 
Thanks.
-- Max

Re: [RFC] exec: flush CPU TB cache when breakpoint address translation fails

[Richard Henderson]

On 11/26/19 10:26 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 when a breakpoint is set for a virtual address
> that cannot be translated to physical address.
> 
> This change fixes the following workflow:
> - 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>

As a short-term fix this works.  I'm willing to apply this to 5.0 with cc:
stable so that as a bug fix this gets pulled back to 4.2.1.  Paolo's request
for a comment is en pointe, so I'll wait for a version 2.

However, while running in system mode with a decent sized amount of guest ram,
I see 1-2M tbs live at once.  We don't really want to throw all of those away.

I think it would be better to put the tbs into some sort of data structure that
indexes the tbs by their virtual address.  I have no strong feelings about what
sort of data structure would be best, just something better than a linear
search of all of the tbs.  :-)


r~