[PATCH] tcg: gdbstub: Fix missing breakpoint issue

[PATCH] tcg: gdbstub: Fix missing breakpoint issue

[Changbin Du]

When inserting breakpoints, we need to invalidate related TBs to apply
helper call. This is done by breakpoint_invalidate(). But many users
found the BPs sometimes never hit.

In system mode emulation, the BPs are global in guest but not particular
address space. The issue is that the current implementation only trys to
invalidate TB of paddr corresponding to the target vaddr in current MMU
context. Then some cached TBs continue running without BPs applied.

To fix this issue, we can just invalidate all TBs as what step mode does.
(For old version users, issuing a step command can workaround this problem.)

Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
 exec.c | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/exec.c b/exec.c
index 67e520d18e..9d9fd48519 100644
--- a/exec.c
+++ b/exec.c
@@ -997,36 +997,9 @@ 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)
 {
-    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);
-    }
+    tb_flush(cpu);
 }
 #endif
 
-- 
2.24.0

Re: [PATCH] tcg: gdbstub: Fix missing breakpoint issue

[Changbin Du]

Hello,
Any review?
Thanks!

On Fri, Jan 24, 2020 at 10:17:28AM +0800, Changbin Du wrote:
> When inserting breakpoints, we need to invalidate related TBs to apply
> helper call. This is done by breakpoint_invalidate(). But many users
> found the BPs sometimes never hit.
> 
> In system mode emulation, the BPs are global in guest but not particular
> address space. The issue is that the current implementation only trys to
> invalidate TB of paddr corresponding to the target vaddr in current MMU
> context. Then some cached TBs continue running without BPs applied.
> 
> To fix this issue, we can just invalidate all TBs as what step mode does.
> (For old version users, issuing a step command can workaround this problem.)
> 
> Signed-off-by: Changbin Du <changbin.du@gmail.com>
> ---
>  exec.c | 29 +----------------------------
>  1 file changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 67e520d18e..9d9fd48519 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -997,36 +997,9 @@ 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)
>  {
> -    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);
> -    }
> +    tb_flush(cpu);
>  }
>  #endif
>  
> -- 
> 2.24.0
> 

-- 
Cheers,
Changbin Du

Re: [PATCH] tcg: gdbstub: Fix missing breakpoint issue

[Richard Henderson]

On 1/24/20 2:17 AM, Changbin Du wrote:
> When inserting breakpoints, we need to invalidate related TBs to apply
> helper call. This is done by breakpoint_invalidate(). But many users
> found the BPs sometimes never hit.
> 
> In system mode emulation, the BPs are global in guest but not particular
> address space. The issue is that the current implementation only trys to
> invalidate TB of paddr corresponding to the target vaddr in current MMU
> context. Then some cached TBs continue running without BPs applied.
> 
> To fix this issue, we can just invalidate all TBs as what step mode does.
> (For old version users, issuing a step command can workaround this problem.)
> 
> Signed-off-by: Changbin Du <changbin.du@gmail.com>
> ---
>  exec.c | 29 +----------------------------
>  1 file changed, 1 insertion(+), 28 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 67e520d18e..9d9fd48519 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -997,36 +997,9 @@ 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)

You can't remove this function yet.
You should have seen that xtensa-softmmu no longer builds.

I've cc'd you into Max Filippov's thread that addresses the same problem, and
I'm going to apply his fix for now.


r~

> -{
> -    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)
>  {
> -    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);
> -    }
> +    tb_flush(cpu);
>  }
>  #endif
>  
> 

Re: [PATCH] tcg: gdbstub: Fix missing breakpoint issue

[Changbin Du]

On Wed, Feb 05, 2020 at 11:03:23AM +0000, Richard Henderson wrote:
> On 1/24/20 2:17 AM, Changbin Du wrote:
> > When inserting breakpoints, we need to invalidate related TBs to apply
> > helper call. This is done by breakpoint_invalidate(). But many users
> > found the BPs sometimes never hit.
> > 
> > In system mode emulation, the BPs are global in guest but not particular
> > address space. The issue is that the current implementation only trys to
> > invalidate TB of paddr corresponding to the target vaddr in current MMU
> > context. Then some cached TBs continue running without BPs applied.
> > 
> > To fix this issue, we can just invalidate all TBs as what step mode does.
> > (For old version users, issuing a step command can workaround this problem.)
> > 
> > Signed-off-by: Changbin Du <changbin.du@gmail.com>
> > ---
> >  exec.c | 29 +----------------------------
> >  1 file changed, 1 insertion(+), 28 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index 67e520d18e..9d9fd48519 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -997,36 +997,9 @@ 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)
> 
> You can't remove this function yet.
> You should have seen that xtensa-softmmu no longer builds.
> 
> I've cc'd you into Max Filippov's thread that addresses the same problem, and
> I'm going to apply his fix for now.
> 
> 
> r~
>
Got it, just go ahead with that one. thanks~

-- 
Cheers,
Changbin Du