Re: [Qemu-devel] [PATCH v2 2/8] exec: Factor out core logic of check_watchpoint()

[Philippe Mathieu-Daudé]

Hi Richard, David,

On 8/29/19 1:16 AM, Richard Henderson wrote:
> From: David Hildenbrand <address@hidden>
> 
> We want to perform the same checks in probe_write() to trigger a cpu
> exit before doing any modifications. We'll have to pass a PC.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
> Message-Id: <address@hidden>
> [rth: Use vaddr for len, like other watchpoint functions;
> Move user-only stub to static inline.]
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  include/hw/core/cpu.h |  7 +++++++
>  exec.c                | 26 ++++++++++++++++++--------
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 6de688059d..7bd8bed5b2 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -1091,6 +1091,11 @@ static inline void 
> cpu_watchpoint_remove_by_ref(CPUState *cpu,
>  static inline void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
>  {
>  }
> +
> +static inline void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
> +                                        MemTxAttrs atr, int fl, uintptr_t ra)
> +{
> +}
>  #else
>  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                            int flags, CPUWatchpoint **watchpoint);
> @@ -1098,6 +1103,8 @@ int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
>                            vaddr len, int flags);
>  void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint);
>  void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
> +void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
> +                          MemTxAttrs attrs, int flags, uintptr_t ra);
>  #endif
>  
>  /**
> diff --git a/exec.c b/exec.c
> index 31fb75901f..cb6f5763dc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2789,11 +2789,10 @@ static const MemoryRegionOps notdirty_mem_ops = {
>  };
>  
>  /* Generate a debug exception if a watchpoint has been hit.  */
> -static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
> flags)
> +void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
> +                          MemTxAttrs attrs, int flags, uintptr_t ra)
>  {
> -    CPUState *cpu = current_cpu;
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> -    target_ulong vaddr;
>      CPUWatchpoint *wp;
>  
>      assert(tcg_enabled());
> @@ -2804,17 +2803,17 @@ static void check_watchpoint(int offset, int len, 
> MemTxAttrs attrs, int flags)
>          cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
>          return;
>      }
> -    vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
> -    vaddr = cc->adjust_watchpoint_address(cpu, vaddr, len);
> +
> +    addr = cc->adjust_watchpoint_address(cpu, addr, len);
>      QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> -        if (cpu_watchpoint_address_matches(wp, vaddr, len)
> +        if (cpu_watchpoint_address_matches(wp, addr, len)
>              && (wp->flags & flags)) {
>              if (flags == BP_MEM_READ) {
>                  wp->flags |= BP_WATCHPOINT_HIT_READ;
>              } else {
>                  wp->flags |= BP_WATCHPOINT_HIT_WRITE;
>              }
> -            wp->hitaddr = vaddr;
> +            wp->hitaddr = MAX(addr, wp->vaddr);

When is addr > wp->vaddr?

>              wp->hitattrs = attrs;
>              if (!cpu->watchpoint_hit) {
>                  if (wp->flags & BP_CPU &&
> @@ -2829,11 +2828,14 @@ static void check_watchpoint(int offset, int len, 
> MemTxAttrs attrs, int flags)
>                  if (wp->flags & BP_STOP_BEFORE_ACCESS) {
>                      cpu->exception_index = EXCP_DEBUG;
>                      mmap_unlock();
> -                    cpu_loop_exit(cpu);
> +                    cpu_loop_exit_restore(cpu, ra);
>                  } else {
>                      /* Force execution of one insn next time.  */
>                      cpu->cflags_next_tb = 1 | curr_cflags();
>                      mmap_unlock();
> +                    if (ra) {
> +                        cpu_restore_state(cpu, ra, true);
> +                    }
>                      cpu_loop_exit_noexc(cpu);
>                  }
>              }
> @@ -2843,6 +2845,14 @@ static void check_watchpoint(int offset, int len, 
> MemTxAttrs attrs, int flags)
>      }
>  }
>  
> +static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
> flags)
> +{
> +    CPUState *cpu = current_cpu;
> +    vaddr addr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
> +
> +    cpu_check_watchpoint(cpu, addr, len, attrs, flags, 0);
> +}
> +
>  /* Watchpoint access routines.  Watchpoints are inserted using TLB tricks,
>     so these check for a hit then pass through to the normal out-of-line
>     phys routines.  */
>