Re: [Qemu-devel] [PATCH v1 8/9] exec.c: Factor out core logic of check_watchpoint()

[David Hildenbrand]

On 23.08.19 12:07, David Hildenbrand wrote:
> 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>
> ---
>  exec.c                | 23 +++++++++++++++++------
>  include/hw/core/cpu.h |  2 ++
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 1df966d17a..d233a4250b 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2810,12 +2810,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 vaddr, int 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());
> @@ -2826,7 +2824,7 @@ 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);
>      QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
>          if (cpu_watchpoint_address_matches(wp, vaddr, len)
> @@ -2851,11 +2849,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);
>                  }
>              }
> @@ -2865,6 +2866,16 @@ static void check_watchpoint(int offset, int len, 
> MemTxAttrs attrs, int flags)
>      }
>  }
>  
> +/* Generate a debug exception if a watchpoint has been hit.  */
> +static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int 
> flags)
> +{
> +    CPUState *cpu = current_cpu;
> +    vaddr vaddr;
> +
> +    vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
> +    cpu_check_watchpoint(cpu, vaddr, 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.  */
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 77fca95a40..3a2d76b32c 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -1070,6 +1070,8 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, 
> vaddr pc, int mask)
>      return false;
>  }
>  
> +void cpu_check_watchpoint(CPUState *cpu, vaddr vaddr, int len,
> +                          MemTxAttrs attrs, int flags, uintptr_t ra);
>  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                            int flags, CPUWatchpoint **watchpoint);
>  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
> 

As we will have bigger accesses with probe_write(), we should do

diff --git a/exec.c b/exec.c
index d233a4250b..4f8cc62a5f 100644
--- a/exec.c
+++ b/exec.c
@@ -2834,7 +2834,7 @@ void cpu_check_watchpoint(CPUState *cpu, vaddr
vaddr, int len,
             } else {
                 wp->flags |= BP_WATCHPOINT_HIT_WRITE;
             }
-            wp->hitaddr = vaddr;
+            wp->hitaddr = MAX(vaddr, wp->vaddr);
             wp->hitattrs = attrs;
             if (!cpu->watchpoint_hit) {
                 if (wp->flags & BP_CPU &&

I guess, to make sure we actually indicate the watchpoint.

-- 

Thanks,

David / dhildenb