[v2,2/8] exec: Factor out core logic of check_watchpoint()

From: David Hildenbrand <david@redhat.com>

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 <david@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20190823100741.9621-9-david@redhat.com>
[rth: Use vaddr for len, like other watchpoint functions;
Move user-only stub to static inline.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/cpu.h |  7 +++++++
 exec.c                | 26 ++++++++++++++++++--------
 2 files changed, 25 insertions(+), 8 deletions(-)

Hi Richard, David,

On 8/29/19 1:16 AM, Richard Henderson wrote:
> From: David Hildenbrand <david@redhat.com>
> 
> 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 <david@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20190823100741.9621-9-david@redhat.com>
> [rth: Use vaddr for len, like other watchpoint functions;
> Move user-only stub to static inline.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  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.  */
>

On 8/29/19 10:26 AM, Philippe Mathieu-Daudé wrote:
>> -            wp->hitaddr = vaddr;
>> +            wp->hitaddr = MAX(addr, wp->vaddr);
> 
> When is addr > wp->vaddr?

Both the watchpoint and the access are arbitrary ranges.

  wp:    [ 1000               - 1008 ]
  store:     [ 1002 - 1004 ]

  wp:               [ 1004    - 1008 ]
  store: [ 1000               - 1008 ]

The old code would, for the first case, return 1002 and not the 1000 of the
watch point, which seems reasonable.  For the second case, we would set 1000,
an address outside of the watchpoint.

David's change makes sure that the address signaled is inside the watchpoint.
I.e. leaving the first case unchanged and making the second  set 1004.

It seems very reasonable to me.


r~

On 8/30/19 3:21 AM, Richard Henderson wrote:
> On 8/29/19 10:26 AM, Philippe Mathieu-Daudé wrote:
>>> -            wp->hitaddr = vaddr;
>>> +            wp->hitaddr = MAX(addr, wp->vaddr);
>>
>> When is addr > wp->vaddr?
> 
> Both the watchpoint and the access are arbitrary ranges.
> 
>   wp:    [ 1000               - 1008 ]
>   store:     [ 1002 - 1004 ]
> 
>   wp:               [ 1004    - 1008 ]
>   store: [ 1000               - 1008 ]
> 
> The old code would, for the first case, return 1002 and not the 1000 of the
> watch point, which seems reasonable.  For the second case, we would set 1000,
> an address outside of the watchpoint.
> 
> David's change makes sure that the address signaled is inside the watchpoint.
> I.e. leaving the first case unchanged and making the second  set 1004.
> 
> It seems very reasonable to me.

Thanks for the very clear explanation :)

If you have time, few lines of comment around would be very appreciated...

Now that it is clearer:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Regards,

Phil.