[Qemu-devel] [PATCH v1 0/9] tcg: probe_write() refactorings and watchpoints

[Qemu-devel] [PATCH v1 0/9] tcg: probe_write() refactorings and watchpoints

[David Hildenbrand]

Fix and refactore some things around probe_write() in s390x code. Introduce
probe_write() for CONFIG_USER_ONLY. Finally, prepare the watchpoint logic
for being called from probe_write() and add the call.

This is the first step of some changes necessary to handle fault-safe
access accross multiple pages on s390x. The next step is making
probe_write() return an address, similar to tlb_vaddr_to_host(), and
introduing probe_read().

Cc: Richard Henderson <rth@twiddle.net>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Riku Voipio <riku.voipio@iki.fi>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Aleksandar Markovic <amarkovic@wavecomp.com>
Cc: Aleksandar Rikalo <arikalo@wavecomp.com>
Cc: Cornelia Huck <cohuck@redhat.com>

David Hildenbrand (9):
  s390x/tcg: Use guest_addr_valid() instead of h2g_valid() in
    probe_write_access()
  s390x/tcg: Fix length calculation in probe_write_access()
  tcg: Factor out CONFIG_USER_ONLY probe_write() from s390x code
  tcg: Enforce single page access in probe_write() for !CONFIG_USER_ONLY
  mips/tcg: Call probe_write() for CONFIG_USER_ONLY as well
  hppa/tcg: Call probe_write() also for CONFIG_USER_ONLY
  s390x/tcg: Pass a size to probe_write() in do_csst()
  exec.c: Factor out core logic of check_watchpoint()
  tcg: Check for watchpoints in probe_write()

 accel/tcg/cputlb.c        | 18 ++++++++++++++++++
 accel/tcg/user-exec.c     | 16 ++++++++++++++++
 exec.c                    | 23 +++++++++++++++++------
 include/exec/exec-all.h   |  4 ++--
 include/hw/core/cpu.h     |  2 ++
 target/hppa/op_helper.c   |  2 --
 target/mips/op_helper.c   |  8 +++-----
 target/s390x/mem_helper.c | 13 ++-----------
 8 files changed, 60 insertions(+), 26 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v1 1/9] s390x/tcg: Use guest_addr_valid() instead of h2g_valid() in probe_write_access()

[David Hildenbrand]

If I'm not completely wrong, we are dealing with guest addresses here
and not with host addresses. Use the right check.

Fixes: c5a7392cfb96 ("s390x/tcg: Provide probe_write_access helper")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 91ba2e03d9..7819aca15d 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -2616,7 +2616,7 @@ void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
                         uintptr_t ra)
 {
 #ifdef CONFIG_USER_ONLY
-    if (!h2g_valid(addr) || !h2g_valid(addr + len - 1) ||
+    if (!guest_addr_valid(addr) || !guest_addr_valid(addr + len - 1) ||
         page_check_range(addr, len, PAGE_WRITE) < 0) {
         s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
     }
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 1/9] s390x/tcg: Use guest_addr_valid() instead of h2g_valid() in probe_write_access()

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> If I'm not completely wrong, we are dealing with guest addresses here
> and not with host addresses. Use the right check.
> 
> Fixes: c5a7392cfb96 ("s390x/tcg: Provide probe_write_access helper")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mem_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v1 2/9] s390x/tcg: Fix length calculation in probe_write_access()

[David Hildenbrand]

Hm,, how did that "-" slip in (-TAGRET_PAGE_SIZE would be correct). This
currently makes us exceed one page in a single probe_write() call,
essentially leaving some memory unchecked.

Fixes: c5a7392cfb96 ("s390x/tcg: Provide probe_write_access helper")
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mem_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 7819aca15d..4b43440e89 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -2623,7 +2623,7 @@ void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
 #else
     /* test the actual access, not just any access to the page due to LAP */
     while (len) {
-        const uint64_t pagelen = -(addr | -TARGET_PAGE_MASK);
+        const uint64_t pagelen = -(addr | TARGET_PAGE_MASK);
         const uint64_t curlen = MIN(pagelen, len);
 
         probe_write(env, addr, curlen, cpu_mmu_index(env, false), ra);
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 2/9] s390x/tcg: Fix length calculation in probe_write_access()

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> Hm,, how did that "-" slip in (-TAGRET_PAGE_SIZE would be correct). This
> currently makes us exceed one page in a single probe_write() call,
> essentially leaving some memory unchecked.
> 
> Fixes: c5a7392cfb96 ("s390x/tcg: Provide probe_write_access helper")
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mem_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v1 3/9] tcg: Factor out CONFIG_USER_ONLY probe_write() from s390x code

[David Hildenbrand]

Factor it out, we'll do some further changes/extensions to both
probe_write() implementations soon. Make sure to allow "size = 0".

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/tcg/user-exec.c     | 16 ++++++++++++++++
 include/exec/exec-all.h   |  4 ++--
 target/s390x/mem_helper.c |  7 -------
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 897d1571c4..322d49c9b8 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -188,6 +188,22 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
     g_assert_not_reached();
 }
 
+void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
+                 uintptr_t retaddr)
+{
+    CPUState *cpu = env_cpu(env);
+    CPUClass *cc;
+
+    if (!guest_addr_valid(addr) ||
+        (size > 0 && !guest_addr_valid(addr + size - 1)) ||
+        page_check_range(addr, size, PAGE_WRITE) < 0) {
+        cc = CPU_GET_CLASS(cpu);
+        cc->tlb_fill(cpu, addr, size, MMU_DATA_STORE, MMU_USER_IDX, false,
+                     retaddr);
+        g_assert_not_reached();
+    }
+}
+
 #if defined(__i386__)
 
 #if defined(__NetBSD__)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 135aeaab0d..cbcc85add3 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -260,8 +260,6 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
 void tlb_set_page(CPUState *cpu, target_ulong vaddr,
                   hwaddr paddr, int prot,
                   int mmu_idx, target_ulong size);
-void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
-                 uintptr_t retaddr);
 #else
 static inline void tlb_init(CPUState *cpu)
 {
@@ -312,6 +310,8 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu,
 {
 }
 #endif
+void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
+                 uintptr_t retaddr);
 
 #define CODE_GEN_ALIGN           16 /* must be >= of the size of a icache line */
 
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 4b43440e89..fdff60ce5d 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -2615,12 +2615,6 @@ uint32_t HELPER(cu42)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t m3)
 void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
                         uintptr_t ra)
 {
-#ifdef CONFIG_USER_ONLY
-    if (!guest_addr_valid(addr) || !guest_addr_valid(addr + len - 1) ||
-        page_check_range(addr, len, PAGE_WRITE) < 0) {
-        s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
-    }
-#else
     /* test the actual access, not just any access to the page due to LAP */
     while (len) {
         const uint64_t pagelen = -(addr | TARGET_PAGE_MASK);
@@ -2630,7 +2624,6 @@ void probe_write_access(CPUS390XState *env, uint64_t addr, uint64_t len,
         addr = wrap_address(env, addr + curlen);
         len -= curlen;
     }
-#endif
 }
 
 void HELPER(probe_write_access)(CPUS390XState *env, uint64_t addr, uint64_t len)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 3/9] tcg: Factor out CONFIG_USER_ONLY probe_write() from s390x code

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> Factor it out, we'll do some further changes/extensions to both
> probe_write() implementations soon. Make sure to allow "size = 0".
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/tcg/user-exec.c     | 16 ++++++++++++++++
>  include/exec/exec-all.h   |  4 ++--
>  target/s390x/mem_helper.c |  7 -------
>  3 files changed, 18 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v1 3/9] tcg: Factor out CONFIG_USER_ONLY probe_write() from s390x code

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> +void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
> +                 uintptr_t retaddr)
> +{
> +    CPUState *cpu = env_cpu(env);
> +    CPUClass *cc;
> +
> +    if (!guest_addr_valid(addr) ||
> +        (size > 0 && !guest_addr_valid(addr + size - 1)) ||

I think the interface needs to remain the same between softmmu and linux-user,
and we should not allow size to cross a page here either.  Which also means
that the second line here can go away -- we only need the one test vs the
single page.


r~

Re: [Qemu-devel] [PATCH v1 3/9] tcg: Factor out CONFIG_USER_ONLY probe_write() from s390x code

[David Hildenbrand]

On 23.08.19 17:22, Richard Henderson wrote:
> On 8/23/19 3:07 AM, David Hildenbrand wrote:
>> +void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
>> +                 uintptr_t retaddr)
>> +{
>> +    CPUState *cpu = env_cpu(env);
>> +    CPUClass *cc;
>> +
>> +    if (!guest_addr_valid(addr) ||
>> +        (size > 0 && !guest_addr_valid(addr + size - 1)) ||
> 
> I think the interface needs to remain the same between softmmu and linux-user,
> and we should not allow size to cross a page here either.  Which also means
> that the second line here can go away -- we only need the one test vs the
> single page.

Can do, and add the same g_assert.

Thanks!


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 4/9] tcg: Enforce single page access in probe_write() for !CONFIG_USER_ONLY

[David Hildenbrand]

While the CONFIG_USER_ONLY variant can handle multiple pages (no MMU), the
!CONFIG_USER_ONLY variant can't and won't. We'll want to convert
probe_write() to return a host address (similar to tlb_vaddr_to_host())
soon. This only works on page granularity.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/tcg/cputlb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index bb9897b25a..4b49ccb58a 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1064,6 +1064,8 @@ void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
     uintptr_t index = tlb_index(env, mmu_idx, addr);
     CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
 
+    g_assert(-(addr | TARGET_PAGE_MASK) >= size);
+
     if (!tlb_hit(tlb_addr_write(entry), addr)) {
         /* TLB entry is for a different page */
         if (!VICTIM_TLB_HIT(addr_write, addr)) {
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 4/9] tcg: Enforce single page access in probe_write() for !CONFIG_USER_ONLY

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> While the CONFIG_USER_ONLY variant can handle multiple pages (no MMU), the
> !CONFIG_USER_ONLY variant can't and won't. We'll want to convert
> probe_write() to return a host address (similar to tlb_vaddr_to_host())
> soon. This only works on page granularity.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/tcg/cputlb.c | 2 ++
>  1 file changed, 2 insertions(+)

As I just mentioned in the previous, I think the two implementations should match.

Anyway, the "multiple pages" thing above still means exactly two, since if
there were three pages involved we were only probing two of them, and the third
could still be unmapped.


r~

Re: [Qemu-devel] [PATCH v1 4/9] tcg: Enforce single page access in probe_write() for !CONFIG_USER_ONLY

[Richard Henderson]

On 8/23/19 8:28 AM, Richard Henderson wrote:
> On 8/23/19 3:07 AM, David Hildenbrand wrote:
>> While the CONFIG_USER_ONLY variant can handle multiple pages (no MMU), the
>> !CONFIG_USER_ONLY variant can't and won't. We'll want to convert
>> probe_write() to return a host address (similar to tlb_vaddr_to_host())
>> soon. This only works on page granularity.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  accel/tcg/cputlb.c | 2 ++
>>  1 file changed, 2 insertions(+)
> 
> As I just mentioned in the previous, I think the two implementations should match.
> 
> Anyway, the "multiple pages" thing above still means exactly two, since if
> there were three pages involved we were only probing two of them, and the third
> could still be unmapped.

Oh, but the actual code here, is fine.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v1 5/9] mips/tcg: Call probe_write() for CONFIG_USER_ONLY as well

[David Hildenbrand]

Let's call it also for CONFIG_USER_ONLY. While at it, add a FIXME and get
rid of one local variable.

MIPS code probably needs a bigger refactoring in regards of
ensure_writable_pages(), similar to s390x, so for example, watchpoints
can be handled reliably later. The actually accessed addresses should
be probed only, not full pages.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/mips/op_helper.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index f88a3ab904..221e8bb1d7 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -4536,16 +4536,14 @@ static inline void ensure_writable_pages(CPUMIPSState *env,
                                          int mmu_idx,
                                          uintptr_t retaddr)
 {
-#if !defined(CONFIG_USER_ONLY)
-    target_ulong page_addr;
+    /* FIXME: Probe the actual accesses (pass and use a size) */
     if (unlikely(MSA_PAGESPAN(addr))) {
         /* first page */
         probe_write(env, addr, 0, mmu_idx, retaddr);
         /* second page */
-        page_addr = (addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
-        probe_write(env, page_addr, 0, mmu_idx, retaddr);
+        addr = (addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
+        probe_write(env, addr, 0, mmu_idx, retaddr);
     }
-#endif
 }
 
 void helper_msa_st_b(CPUMIPSState *env, uint32_t wd,
-- 
2.21.0

[Qemu-devel] [PATCH v1 6/9] hppa/tcg: Call probe_write() also for CONFIG_USER_ONLY

[David Hildenbrand]

We now have a variant for CONFIG_USER_ONLY as well.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/hppa/op_helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
index df0f1361ef..f0516e81f1 100644
--- a/target/hppa/op_helper.c
+++ b/target/hppa/op_helper.c
@@ -137,9 +137,7 @@ static void do_stby_e(CPUHPPAState *env, target_ulong addr, target_ureg val,
     default:
         /* Nothing is stored, but protection is checked and the
            cacheline is marked dirty.  */
-#ifndef CONFIG_USER_ONLY
         probe_write(env, addr, 0, cpu_mmu_index(env, 0), ra);
-#endif
         break;
     }
 }
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 6/9] hppa/tcg: Call probe_write() also for CONFIG_USER_ONLY

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> We now have a variant for CONFIG_USER_ONLY as well.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/hppa/op_helper.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH v1 7/9] s390x/tcg: Pass a size to probe_write() in do_csst()

[David Hildenbrand]

... and also call it for CONFIG_USER_ONLY. This function probably will
also need some refactoring in regards to probing, however, we'll have to
come back to that later, once cleaning up the other mem helpers.

The alignment check always makes sure that the write access falls into a
single page.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mem_helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index fdff60ce5d..29fcce426e 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1443,9 +1443,7 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t r3, uint64_t a1,
     }
 
     /* Sanity check writability of the store address.  */
-#ifndef CONFIG_USER_ONLY
-    probe_write(env, a2, 0, mem_idx, ra);
-#endif
+    probe_write(env, a2, 1 << sc, mem_idx, ra);
 
     /*
      * Note that the compare-and-swap is atomic, and the store is atomic,
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 7/9] s390x/tcg: Pass a size to probe_write() in do_csst()

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> ... and also call it for CONFIG_USER_ONLY. This function probably will
> also need some refactoring in regards to probing, however, we'll have to
> come back to that later, once cleaning up the other mem helpers.
> 
> The alignment check always makes sure that the write access falls into a
> single page.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mem_helper.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[David Hildenbrand]

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>
---
 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,
-- 
2.21.0

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 <david@redhat.com>
> ---
>  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

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

[Richard Henderson]

On 8/23/19 4:27 AM, David Hildenbrand wrote:
> 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 <david@redhat.com>
>> ---
>>  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.

Yes, that looks right.

As for your changes to use cpu_loop_exit_restore...  Those are so right that I
didn't even recognize how wrong this code is when I was looking through it the
other day.  Watchpoints must not actually be working at all at the moment, really.

I suspect that we need to use a page flag for this and not use I/O memory at
all.  Or convert to read/write_with_attrs and use magic MemTxResult values, but
that seems sketchier than page flags.  Either way is the only way that we can
get access to the host return address so that we can unwind and return to the
main loop.

But this is a good step, in the right direction.  We'll fix the rest later.

With the MAX,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

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

[David Hildenbrand]

On 23.08.19 18:09, Richard Henderson wrote:
> On 8/23/19 4:27 AM, David Hildenbrand wrote:
>> 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 <david@redhat.com>
>>> ---
>>>  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.
> 
> Yes, that looks right.
> 
> As for your changes to use cpu_loop_exit_restore...  Those are so right that I
> didn't even recognize how wrong this code is when I was looking through it the
> other day.  Watchpoints must not actually be working at all at the moment, really.

We keep finding surprises ... guess we payed the wrong area of QEMU src
code a visit :)

> 
> I suspect that we need to use a page flag for this and not use I/O memory at
> all.  Or convert to read/write_with_attrs and use magic MemTxResult values, but
> that seems sketchier than page flags.  Either way is the only way that we can
> get access to the host return address so that we can unwind and return to the
> main loop.

I wonder if we can pass through the RA from the callers somehow. But I
have to admit, how the whole MemOps (+MemTX) stuff is fits together is
not yet 100% clear to me.

> 
> But this is a good step, in the right direction.  We'll fix the rest later.
> 
> With the MAX,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 

Thanks!

I'll fixup the smaller things and resend next week. I already have
another set of patches (e.g., probe_read() and return void* from
probe_write() lying around). The s390x part is still in the works.

-- 

Thanks,

David / dhildenb

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

[Richard Henderson]

On 8/23/19 4:27 AM, David Hildenbrand wrote:
> 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 <david@redhat.com>
>> ---
>>  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.
> 

I have applied this patch with this fix to tcg-next.
I'm working on fixing the other problems we discovered re watchpoints.


r~

[Qemu-devel] [PATCH v1 9/9] tcg: Check for watchpoints in probe_write()

[David Hildenbrand]

Let's check for write watchpoints. We'll want to do something similar
for probe_read() in the future (once we introduce that).

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 accel/tcg/cputlb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 4b49ccb58a..8382ac2fc2 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1063,6 +1063,7 @@ void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
 {
     uintptr_t index = tlb_index(env, mmu_idx, addr);
     CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
+    target_ulong tlb_addr;
 
     g_assert(-(addr | TARGET_PAGE_MASK) >= size);
 
@@ -1071,8 +1072,23 @@ void probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx,
         if (!VICTIM_TLB_HIT(addr_write, addr)) {
             tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
                      mmu_idx, retaddr);
+            /* TLB resize via tlb_fill may have moved the entry. */
+            entry = tlb_entry(env, mmu_idx, addr);
         }
     }
+
+    if (!size) {
+        return;
+    }
+    tlb_addr = tlb_addr_write(entry);
+
+    /* Watchpoints for this entry only apply if TLB_MMIO was set. */
+    if (tlb_addr & TLB_MMIO) {
+        MemTxAttrs attrs = env_tlb(env)->d[mmu_idx].iotlb[index].attrs;
+
+        cpu_check_watchpoint(env_cpu(env), addr, size, attrs, BP_MEM_WRITE,
+                             retaddr);
+    }
 }
 
 void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 9/9] tcg: Check for watchpoints in probe_write()

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> Let's check for write watchpoints. We'll want to do something similar
> for probe_read() in the future (once we introduce that).
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  accel/tcg/cputlb.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v1 9/9] tcg: Check for watchpoints in probe_write()

[Richard Henderson]

On 8/23/19 3:07 AM, David Hildenbrand wrote:
> @@ -1071,8 +1072,23 @@ void probe_write(CPUArchState *env, target_ulong addr, 
>          if (!VICTIM_TLB_HIT(addr_write, addr)) {
>              tlb_fill(env_cpu(env), addr, size, MMU_DATA_STORE,
>                       mmu_idx, retaddr);
> +            /* TLB resize via tlb_fill may have moved the entry. */
> +            entry = tlb_entry(env, mmu_idx, addr);
>          }
>      }
> +
> +    if (!size) {
> +        return;
> +    }
> +    tlb_addr = tlb_addr_write(entry);
> +
> +    /* Watchpoints for this entry only apply if TLB_MMIO was set. */
> +    if (tlb_addr & TLB_MMIO) {
> +        MemTxAttrs attrs = env_tlb(env)->d[mmu_idx].iotlb[index].attrs;

We need to recompute index above as well, since we use it here.
Fixed up and applied to tcg-next.


r~