[PATCH-for-8.0] softmmu: Extract watchpoint API from physmem.c

Philippe Mathieu-Daudé posted 1 patch 1 year, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221209141254.68662-1-philmd@linaro.org
Maintainers: Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, Richard Henderson <richard.henderson@linaro.org>
MAINTAINERS           |   1 +
include/hw/core/cpu.h |   2 +-
softmmu/meson.build   |   3 +-
softmmu/physmem.c     | 191 ------------------------------------
softmmu/watchpoint.c  | 220 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 224 insertions(+), 193 deletions(-)
create mode 100644 softmmu/watchpoint.c
[PATCH-for-8.0] softmmu: Extract watchpoint API from physmem.c
Posted by Philippe Mathieu-Daudé 1 year, 4 months ago
The watchpoint API is specific to TCG system emulation.

Move it to a new compile unit. The inlined stubs are used
for user-mode and non-TCG accelerators.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS           |   1 +
 include/hw/core/cpu.h |   2 +-
 softmmu/meson.build   |   3 +-
 softmmu/physmem.c     | 191 ------------------------------------
 softmmu/watchpoint.c  | 220 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 224 insertions(+), 193 deletions(-)
 create mode 100644 softmmu/watchpoint.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6966490c94..979c3e2c3f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -120,6 +120,7 @@ M: Richard Henderson <richard.henderson@linaro.org>
 R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
 F: softmmu/cpus.c
+F: softmmu/watchpoint.c
 F: cpus-common.c
 F: page-vary.c
 F: page-vary-common.c
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8830546121..b490cc80d8 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -943,7 +943,7 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
     return false;
 }
 
-#ifdef CONFIG_USER_ONLY
+#if !defined(CONFIG_TCG) || defined(CONFIG_USER_ONLY)
 static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
                                         int flags, CPUWatchpoint **watchpoint)
 {
diff --git a/softmmu/meson.build b/softmmu/meson.build
index 3272af1f31..918166cdc0 100644
--- a/softmmu/meson.build
+++ b/softmmu/meson.build
@@ -8,7 +8,8 @@ specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: [files(
 )])
 
 specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: [files(
-  'icount.c'
+  'icount.c',
+  'watchpoint.c',
 )])
 
 softmmu_ss.add(files(
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 1b606a3002..1a9e4aa1cf 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -780,197 +780,6 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
     return cpu->cpu_ases[asidx].as;
 }
 
-/* Add a watchpoint.  */
-int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
-                          int flags, CPUWatchpoint **watchpoint)
-{
-    CPUWatchpoint *wp;
-    vaddr in_page;
-
-    /* forbid ranges which are empty or run off the end of the address space */
-    if (len == 0 || (addr + len - 1) < addr) {
-        error_report("tried to set invalid watchpoint at %"
-                     VADDR_PRIx ", len=%" VADDR_PRIu, addr, len);
-        return -EINVAL;
-    }
-    wp = g_malloc(sizeof(*wp));
-
-    wp->vaddr = addr;
-    wp->len = len;
-    wp->flags = flags;
-
-    /* keep all GDB-injected watchpoints in front */
-    if (flags & BP_GDB) {
-        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
-    } else {
-        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
-    }
-
-    in_page = -(addr | TARGET_PAGE_MASK);
-    if (len <= in_page) {
-        tlb_flush_page(cpu, addr);
-    } else {
-        tlb_flush(cpu);
-    }
-
-    if (watchpoint)
-        *watchpoint = wp;
-    return 0;
-}
-
-/* Remove a specific watchpoint.  */
-int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
-                          int flags)
-{
-    CPUWatchpoint *wp;
-
-    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        if (addr == wp->vaddr && len == wp->len
-                && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
-            cpu_watchpoint_remove_by_ref(cpu, wp);
-            return 0;
-        }
-    }
-    return -ENOENT;
-}
-
-/* Remove a specific watchpoint by reference.  */
-void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint)
-{
-    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
-
-    tlb_flush_page(cpu, watchpoint->vaddr);
-
-    g_free(watchpoint);
-}
-
-/* Remove all matching watchpoints.  */
-void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
-{
-    CPUWatchpoint *wp, *next;
-
-    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
-        if (wp->flags & mask) {
-            cpu_watchpoint_remove_by_ref(cpu, wp);
-        }
-    }
-}
-
-#ifdef CONFIG_TCG
-/* Return true if this watchpoint address matches the specified
- * access (ie the address range covered by the watchpoint overlaps
- * partially or completely with the address range covered by the
- * access).
- */
-static inline bool watchpoint_address_matches(CPUWatchpoint *wp,
-                                              vaddr addr, vaddr len)
-{
-    /* We know the lengths are non-zero, but a little caution is
-     * required to avoid errors in the case where the range ends
-     * exactly at the top of the address space and so addr + len
-     * wraps round to zero.
-     */
-    vaddr wpend = wp->vaddr + wp->len - 1;
-    vaddr addrend = addr + len - 1;
-
-    return !(addr > wpend || wp->vaddr > addrend);
-}
-
-/* Return flags for watchpoints that match addr + prot.  */
-int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len)
-{
-    CPUWatchpoint *wp;
-    int ret = 0;
-
-    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        if (watchpoint_address_matches(wp, addr, len)) {
-            ret |= wp->flags;
-        }
-    }
-    return ret;
-}
-
-/* Generate a debug exception if a watchpoint has been hit.  */
-void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
-                          MemTxAttrs attrs, int flags, uintptr_t ra)
-{
-    CPUClass *cc = CPU_GET_CLASS(cpu);
-    CPUWatchpoint *wp;
-
-    assert(tcg_enabled());
-    if (cpu->watchpoint_hit) {
-        /*
-         * We re-entered the check after replacing the TB.
-         * Now raise the debug interrupt so that it will
-         * trigger after the current instruction.
-         */
-        qemu_mutex_lock_iothread();
-        cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
-        qemu_mutex_unlock_iothread();
-        return;
-    }
-
-    if (cc->tcg_ops->adjust_watchpoint_address) {
-        /* this is currently used only by ARM BE32 */
-        addr = cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
-    }
-    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        if (watchpoint_address_matches(wp, addr, len)
-            && (wp->flags & flags)) {
-            if (replay_running_debug()) {
-                /*
-                 * replay_breakpoint reads icount.
-                 * Force recompile to succeed, because icount may
-                 * be read only at the end of the block.
-                 */
-                if (!cpu->can_do_io) {
-                    /* Force execution of one insn next time.  */
-                    cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu);
-                    cpu_loop_exit_restore(cpu, ra);
-                }
-                /*
-                 * Don't process the watchpoints when we are
-                 * in a reverse debugging operation.
-                 */
-                replay_breakpoint();
-                return;
-            }
-            if (flags == BP_MEM_READ) {
-                wp->flags |= BP_WATCHPOINT_HIT_READ;
-            } else {
-                wp->flags |= BP_WATCHPOINT_HIT_WRITE;
-            }
-            wp->hitaddr = MAX(addr, wp->vaddr);
-            wp->hitattrs = attrs;
-
-            if (wp->flags & BP_CPU && cc->tcg_ops->debug_check_watchpoint &&
-                !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
-                wp->flags &= ~BP_WATCHPOINT_HIT;
-                continue;
-            }
-            cpu->watchpoint_hit = wp;
-
-            mmap_lock();
-            /* This call also restores vCPU state */
-            tb_check_watchpoint(cpu, ra);
-            if (wp->flags & BP_STOP_BEFORE_ACCESS) {
-                cpu->exception_index = EXCP_DEBUG;
-                mmap_unlock();
-                cpu_loop_exit(cpu);
-            } else {
-                /* Force execution of one insn next time.  */
-                cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu);
-                mmap_unlock();
-                cpu_loop_exit_noexc(cpu);
-            }
-        } else {
-            wp->flags &= ~BP_WATCHPOINT_HIT;
-        }
-    }
-}
-
-#endif /* CONFIG_TCG */
-
 /* Called from RCU critical section */
 static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
 {
diff --git a/softmmu/watchpoint.c b/softmmu/watchpoint.c
new file mode 100644
index 0000000000..279129dd1c
--- /dev/null
+++ b/softmmu/watchpoint.c
@@ -0,0 +1,220 @@
+/*
+ * CPU watchpoints
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "exec/exec-all.h"
+#include "exec/translate-all.h"
+#include "sysemu/tcg.h"
+#include "sysemu/replay.h"
+#include "hw/core/tcg-cpu-ops.h"
+#include "hw/core/cpu.h"
+
+/* Add a watchpoint.  */
+int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
+                          int flags, CPUWatchpoint **watchpoint)
+{
+    CPUWatchpoint *wp;
+    vaddr in_page;
+
+    /* forbid ranges which are empty or run off the end of the address space */
+    if (len == 0 || (addr + len - 1) < addr) {
+        error_report("tried to set invalid watchpoint at %"
+                     VADDR_PRIx ", len=%" VADDR_PRIu, addr, len);
+        return -EINVAL;
+    }
+    wp = g_malloc(sizeof(*wp));
+
+    wp->vaddr = addr;
+    wp->len = len;
+    wp->flags = flags;
+
+    /* keep all GDB-injected watchpoints in front */
+    if (flags & BP_GDB) {
+        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
+    } else {
+        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
+    }
+
+    in_page = -(addr | TARGET_PAGE_MASK);
+    if (len <= in_page) {
+        tlb_flush_page(cpu, addr);
+    } else {
+        tlb_flush(cpu);
+    }
+
+    if (watchpoint) {
+        *watchpoint = wp;
+    }
+    return 0;
+}
+
+/* Remove a specific watchpoint.  */
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
+                          int flags)
+{
+    CPUWatchpoint *wp;
+
+    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
+        if (addr == wp->vaddr && len == wp->len
+                && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
+            cpu_watchpoint_remove_by_ref(cpu, wp);
+            return 0;
+        }
+    }
+    return -ENOENT;
+}
+
+/* Remove a specific watchpoint by reference.  */
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint)
+{
+    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
+
+    tlb_flush_page(cpu, watchpoint->vaddr);
+
+    g_free(watchpoint);
+}
+
+/* Remove all matching watchpoints.  */
+void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
+{
+    CPUWatchpoint *wp, *next;
+
+    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
+        if (wp->flags & mask) {
+            cpu_watchpoint_remove_by_ref(cpu, wp);
+        }
+    }
+}
+
+/*
+ * Return true if this watchpoint address matches the specified
+ * access (ie the address range covered by the watchpoint overlaps
+ * partially or completely with the address range covered by the
+ * access).
+ */
+static inline bool watchpoint_address_matches(CPUWatchpoint *wp,
+                                              vaddr addr, vaddr len)
+{
+    /*
+     * We know the lengths are non-zero, but a little caution is
+     * required to avoid errors in the case where the range ends
+     * exactly at the top of the address space and so addr + len
+     * wraps round to zero.
+     */
+    vaddr wpend = wp->vaddr + wp->len - 1;
+    vaddr addrend = addr + len - 1;
+
+    return !(addr > wpend || wp->vaddr > addrend);
+}
+
+/* Return flags for watchpoints that match addr + prot.  */
+int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len)
+{
+    CPUWatchpoint *wp;
+    int ret = 0;
+
+    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
+        if (watchpoint_address_matches(wp, addr, len)) {
+            ret |= wp->flags;
+        }
+    }
+    return ret;
+}
+
+/* Generate a debug exception if a watchpoint has been hit.  */
+void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
+                          MemTxAttrs attrs, int flags, uintptr_t ra)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+    CPUWatchpoint *wp;
+
+    assert(tcg_enabled());
+    if (cpu->watchpoint_hit) {
+        /*
+         * We re-entered the check after replacing the TB.
+         * Now raise the debug interrupt so that it will
+         * trigger after the current instruction.
+         */
+        qemu_mutex_lock_iothread();
+        cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
+        qemu_mutex_unlock_iothread();
+        return;
+    }
+
+    if (cc->tcg_ops->adjust_watchpoint_address) {
+        /* this is currently used only by ARM BE32 */
+        addr = cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
+    }
+    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
+        if (watchpoint_address_matches(wp, addr, len)
+            && (wp->flags & flags)) {
+            if (replay_running_debug()) {
+                /*
+                 * replay_breakpoint reads icount.
+                 * Force recompile to succeed, because icount may
+                 * be read only at the end of the block.
+                 */
+                if (!cpu->can_do_io) {
+                    /* Force execution of one insn next time.  */
+                    cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ
+                                          | curr_cflags(cpu);
+                    cpu_loop_exit_restore(cpu, ra);
+                }
+                /*
+                 * Don't process the watchpoints when we are
+                 * in a reverse debugging operation.
+                 */
+                replay_breakpoint();
+                return;
+            }
+            if (flags == BP_MEM_READ) {
+                wp->flags |= BP_WATCHPOINT_HIT_READ;
+            } else {
+                wp->flags |= BP_WATCHPOINT_HIT_WRITE;
+            }
+            wp->hitaddr = MAX(addr, wp->vaddr);
+            wp->hitattrs = attrs;
+
+            if (wp->flags & BP_CPU && cc->tcg_ops->debug_check_watchpoint &&
+                !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
+                wp->flags &= ~BP_WATCHPOINT_HIT;
+                continue;
+            }
+            cpu->watchpoint_hit = wp;
+
+            mmap_lock();
+            /* This call also restores vCPU state */
+            tb_check_watchpoint(cpu, ra);
+            if (wp->flags & BP_STOP_BEFORE_ACCESS) {
+                cpu->exception_index = EXCP_DEBUG;
+                mmap_unlock();
+                cpu_loop_exit(cpu);
+            } else {
+                /* Force execution of one insn next time.  */
+                cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ
+                                      | curr_cflags(cpu);
+                mmap_unlock();
+                cpu_loop_exit_noexc(cpu);
+            }
+        } else {
+            wp->flags &= ~BP_WATCHPOINT_HIT;
+        }
+    }
+}
-- 
2.38.1


Re: [PATCH-for-8.0] softmmu: Extract watchpoint API from physmem.c
Posted by Richard Henderson 1 year, 3 months ago
On 12/9/22 06:12, Philippe Mathieu-Daudé wrote:
> The watchpoint API is specific to TCG system emulation.
> 
> Move it to a new compile unit. The inlined stubs are used
> for user-mode and non-TCG accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   MAINTAINERS           |   1 +
>   include/hw/core/cpu.h |   2 +-
>   softmmu/meson.build   |   3 +-
>   softmmu/physmem.c     | 191 ------------------------------------
>   softmmu/watchpoint.c  | 220 ++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 224 insertions(+), 193 deletions(-)
>   create mode 100644 softmmu/watchpoint.c

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

r~

Re: [PATCH-for-8.0] softmmu: Extract watchpoint API from physmem.c
Posted by Philippe Mathieu-Daudé 1 year, 3 months ago
ping?

On 9/12/22 15:12, Philippe Mathieu-Daudé wrote:
> The watchpoint API is specific to TCG system emulation.
> 
> Move it to a new compile unit. The inlined stubs are used
> for user-mode and non-TCG accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   MAINTAINERS           |   1 +
>   include/hw/core/cpu.h |   2 +-
>   softmmu/meson.build   |   3 +-
>   softmmu/physmem.c     | 191 ------------------------------------
>   softmmu/watchpoint.c  | 220 ++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 224 insertions(+), 193 deletions(-)
>   create mode 100644 softmmu/watchpoint.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6966490c94..979c3e2c3f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -120,6 +120,7 @@ M: Richard Henderson <richard.henderson@linaro.org>
>   R: Paolo Bonzini <pbonzini@redhat.com>
>   S: Maintained
>   F: softmmu/cpus.c
> +F: softmmu/watchpoint.c
>   F: cpus-common.c
>   F: page-vary.c
>   F: page-vary-common.c
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 8830546121..b490cc80d8 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -943,7 +943,7 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
>       return false;
>   }
>   
> -#ifdef CONFIG_USER_ONLY
> +#if !defined(CONFIG_TCG) || defined(CONFIG_USER_ONLY)
>   static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                                           int flags, CPUWatchpoint **watchpoint)
>   {
> diff --git a/softmmu/meson.build b/softmmu/meson.build
> index 3272af1f31..918166cdc0 100644
> --- a/softmmu/meson.build
> +++ b/softmmu/meson.build
> @@ -8,7 +8,8 @@ specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: [files(
>   )])
>   
>   specific_ss.add(when: ['CONFIG_SOFTMMU', 'CONFIG_TCG'], if_true: [files(
> -  'icount.c'
> +  'icount.c',
> +  'watchpoint.c',
>   )])
>   
>   softmmu_ss.add(files(
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 1b606a3002..1a9e4aa1cf 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -780,197 +780,6 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>       return cpu->cpu_ases[asidx].as;
>   }
>   
> -/* Add a watchpoint.  */
> -int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
> -                          int flags, CPUWatchpoint **watchpoint)
> -{
> -    CPUWatchpoint *wp;
> -    vaddr in_page;
> -
> -    /* forbid ranges which are empty or run off the end of the address space */
> -    if (len == 0 || (addr + len - 1) < addr) {
> -        error_report("tried to set invalid watchpoint at %"
> -                     VADDR_PRIx ", len=%" VADDR_PRIu, addr, len);
> -        return -EINVAL;
> -    }
> -    wp = g_malloc(sizeof(*wp));
> -
> -    wp->vaddr = addr;
> -    wp->len = len;
> -    wp->flags = flags;
> -
> -    /* keep all GDB-injected watchpoints in front */
> -    if (flags & BP_GDB) {
> -        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
> -    } else {
> -        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
> -    }
> -
> -    in_page = -(addr | TARGET_PAGE_MASK);
> -    if (len <= in_page) {
> -        tlb_flush_page(cpu, addr);
> -    } else {
> -        tlb_flush(cpu);
> -    }
> -
> -    if (watchpoint)
> -        *watchpoint = wp;
> -    return 0;
> -}
> -
> -/* Remove a specific watchpoint.  */
> -int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
> -                          int flags)
> -{
> -    CPUWatchpoint *wp;
> -
> -    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> -        if (addr == wp->vaddr && len == wp->len
> -                && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
> -            cpu_watchpoint_remove_by_ref(cpu, wp);
> -            return 0;
> -        }
> -    }
> -    return -ENOENT;
> -}
> -
> -/* Remove a specific watchpoint by reference.  */
> -void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint)
> -{
> -    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
> -
> -    tlb_flush_page(cpu, watchpoint->vaddr);
> -
> -    g_free(watchpoint);
> -}
> -
> -/* Remove all matching watchpoints.  */
> -void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
> -{
> -    CPUWatchpoint *wp, *next;
> -
> -    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
> -        if (wp->flags & mask) {
> -            cpu_watchpoint_remove_by_ref(cpu, wp);
> -        }
> -    }
> -}
> -
> -#ifdef CONFIG_TCG
> -/* Return true if this watchpoint address matches the specified
> - * access (ie the address range covered by the watchpoint overlaps
> - * partially or completely with the address range covered by the
> - * access).
> - */
> -static inline bool watchpoint_address_matches(CPUWatchpoint *wp,
> -                                              vaddr addr, vaddr len)
> -{
> -    /* We know the lengths are non-zero, but a little caution is
> -     * required to avoid errors in the case where the range ends
> -     * exactly at the top of the address space and so addr + len
> -     * wraps round to zero.
> -     */
> -    vaddr wpend = wp->vaddr + wp->len - 1;
> -    vaddr addrend = addr + len - 1;
> -
> -    return !(addr > wpend || wp->vaddr > addrend);
> -}
> -
> -/* Return flags for watchpoints that match addr + prot.  */
> -int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len)
> -{
> -    CPUWatchpoint *wp;
> -    int ret = 0;
> -
> -    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> -        if (watchpoint_address_matches(wp, addr, len)) {
> -            ret |= wp->flags;
> -        }
> -    }
> -    return ret;
> -}
> -
> -/* Generate a debug exception if a watchpoint has been hit.  */
> -void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
> -                          MemTxAttrs attrs, int flags, uintptr_t ra)
> -{
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -    CPUWatchpoint *wp;
> -
> -    assert(tcg_enabled());
> -    if (cpu->watchpoint_hit) {
> -        /*
> -         * We re-entered the check after replacing the TB.
> -         * Now raise the debug interrupt so that it will
> -         * trigger after the current instruction.
> -         */
> -        qemu_mutex_lock_iothread();
> -        cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
> -        qemu_mutex_unlock_iothread();
> -        return;
> -    }
> -
> -    if (cc->tcg_ops->adjust_watchpoint_address) {
> -        /* this is currently used only by ARM BE32 */
> -        addr = cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
> -    }
> -    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> -        if (watchpoint_address_matches(wp, addr, len)
> -            && (wp->flags & flags)) {
> -            if (replay_running_debug()) {
> -                /*
> -                 * replay_breakpoint reads icount.
> -                 * Force recompile to succeed, because icount may
> -                 * be read only at the end of the block.
> -                 */
> -                if (!cpu->can_do_io) {
> -                    /* Force execution of one insn next time.  */
> -                    cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu);
> -                    cpu_loop_exit_restore(cpu, ra);
> -                }
> -                /*
> -                 * Don't process the watchpoints when we are
> -                 * in a reverse debugging operation.
> -                 */
> -                replay_breakpoint();
> -                return;
> -            }
> -            if (flags == BP_MEM_READ) {
> -                wp->flags |= BP_WATCHPOINT_HIT_READ;
> -            } else {
> -                wp->flags |= BP_WATCHPOINT_HIT_WRITE;
> -            }
> -            wp->hitaddr = MAX(addr, wp->vaddr);
> -            wp->hitattrs = attrs;
> -
> -            if (wp->flags & BP_CPU && cc->tcg_ops->debug_check_watchpoint &&
> -                !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
> -                wp->flags &= ~BP_WATCHPOINT_HIT;
> -                continue;
> -            }
> -            cpu->watchpoint_hit = wp;
> -
> -            mmap_lock();
> -            /* This call also restores vCPU state */
> -            tb_check_watchpoint(cpu, ra);
> -            if (wp->flags & BP_STOP_BEFORE_ACCESS) {
> -                cpu->exception_index = EXCP_DEBUG;
> -                mmap_unlock();
> -                cpu_loop_exit(cpu);
> -            } else {
> -                /* Force execution of one insn next time.  */
> -                cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu);
> -                mmap_unlock();
> -                cpu_loop_exit_noexc(cpu);
> -            }
> -        } else {
> -            wp->flags &= ~BP_WATCHPOINT_HIT;
> -        }
> -    }
> -}
> -
> -#endif /* CONFIG_TCG */
> -
>   /* Called from RCU critical section */
>   static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
>   {
> diff --git a/softmmu/watchpoint.c b/softmmu/watchpoint.c
> new file mode 100644
> index 0000000000..279129dd1c
> --- /dev/null
> +++ b/softmmu/watchpoint.c
> @@ -0,0 +1,220 @@
> +/*
> + * CPU watchpoints
> + *
> + *  Copyright (c) 2003 Fabrice Bellard
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "exec/exec-all.h"
> +#include "exec/translate-all.h"
> +#include "sysemu/tcg.h"
> +#include "sysemu/replay.h"
> +#include "hw/core/tcg-cpu-ops.h"
> +#include "hw/core/cpu.h"
> +
> +/* Add a watchpoint.  */
> +int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
> +                          int flags, CPUWatchpoint **watchpoint)
> +{
> +    CPUWatchpoint *wp;
> +    vaddr in_page;
> +
> +    /* forbid ranges which are empty or run off the end of the address space */
> +    if (len == 0 || (addr + len - 1) < addr) {
> +        error_report("tried to set invalid watchpoint at %"
> +                     VADDR_PRIx ", len=%" VADDR_PRIu, addr, len);
> +        return -EINVAL;
> +    }
> +    wp = g_malloc(sizeof(*wp));
> +
> +    wp->vaddr = addr;
> +    wp->len = len;
> +    wp->flags = flags;
> +
> +    /* keep all GDB-injected watchpoints in front */
> +    if (flags & BP_GDB) {
> +        QTAILQ_INSERT_HEAD(&cpu->watchpoints, wp, entry);
> +    } else {
> +        QTAILQ_INSERT_TAIL(&cpu->watchpoints, wp, entry);
> +    }
> +
> +    in_page = -(addr | TARGET_PAGE_MASK);
> +    if (len <= in_page) {
> +        tlb_flush_page(cpu, addr);
> +    } else {
> +        tlb_flush(cpu);
> +    }
> +
> +    if (watchpoint) {
> +        *watchpoint = wp;
> +    }
> +    return 0;
> +}
> +
> +/* Remove a specific watchpoint.  */
> +int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
> +                          int flags)
> +{
> +    CPUWatchpoint *wp;
> +
> +    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> +        if (addr == wp->vaddr && len == wp->len
> +                && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
> +            cpu_watchpoint_remove_by_ref(cpu, wp);
> +            return 0;
> +        }
> +    }
> +    return -ENOENT;
> +}
> +
> +/* Remove a specific watchpoint by reference.  */
> +void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint)
> +{
> +    QTAILQ_REMOVE(&cpu->watchpoints, watchpoint, entry);
> +
> +    tlb_flush_page(cpu, watchpoint->vaddr);
> +
> +    g_free(watchpoint);
> +}
> +
> +/* Remove all matching watchpoints.  */
> +void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
> +{
> +    CPUWatchpoint *wp, *next;
> +
> +    QTAILQ_FOREACH_SAFE(wp, &cpu->watchpoints, entry, next) {
> +        if (wp->flags & mask) {
> +            cpu_watchpoint_remove_by_ref(cpu, wp);
> +        }
> +    }
> +}
> +
> +/*
> + * Return true if this watchpoint address matches the specified
> + * access (ie the address range covered by the watchpoint overlaps
> + * partially or completely with the address range covered by the
> + * access).
> + */
> +static inline bool watchpoint_address_matches(CPUWatchpoint *wp,
> +                                              vaddr addr, vaddr len)
> +{
> +    /*
> +     * We know the lengths are non-zero, but a little caution is
> +     * required to avoid errors in the case where the range ends
> +     * exactly at the top of the address space and so addr + len
> +     * wraps round to zero.
> +     */
> +    vaddr wpend = wp->vaddr + wp->len - 1;
> +    vaddr addrend = addr + len - 1;
> +
> +    return !(addr > wpend || wp->vaddr > addrend);
> +}
> +
> +/* Return flags for watchpoints that match addr + prot.  */
> +int cpu_watchpoint_address_matches(CPUState *cpu, vaddr addr, vaddr len)
> +{
> +    CPUWatchpoint *wp;
> +    int ret = 0;
> +
> +    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> +        if (watchpoint_address_matches(wp, addr, len)) {
> +            ret |= wp->flags;
> +        }
> +    }
> +    return ret;
> +}
> +
> +/* Generate a debug exception if a watchpoint has been hit.  */
> +void cpu_check_watchpoint(CPUState *cpu, vaddr addr, vaddr len,
> +                          MemTxAttrs attrs, int flags, uintptr_t ra)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    CPUWatchpoint *wp;
> +
> +    assert(tcg_enabled());
> +    if (cpu->watchpoint_hit) {
> +        /*
> +         * We re-entered the check after replacing the TB.
> +         * Now raise the debug interrupt so that it will
> +         * trigger after the current instruction.
> +         */
> +        qemu_mutex_lock_iothread();
> +        cpu_interrupt(cpu, CPU_INTERRUPT_DEBUG);
> +        qemu_mutex_unlock_iothread();
> +        return;
> +    }
> +
> +    if (cc->tcg_ops->adjust_watchpoint_address) {
> +        /* this is currently used only by ARM BE32 */
> +        addr = cc->tcg_ops->adjust_watchpoint_address(cpu, addr, len);
> +    }
> +    QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
> +        if (watchpoint_address_matches(wp, addr, len)
> +            && (wp->flags & flags)) {
> +            if (replay_running_debug()) {
> +                /*
> +                 * replay_breakpoint reads icount.
> +                 * Force recompile to succeed, because icount may
> +                 * be read only at the end of the block.
> +                 */
> +                if (!cpu->can_do_io) {
> +                    /* Force execution of one insn next time.  */
> +                    cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ
> +                                          | curr_cflags(cpu);
> +                    cpu_loop_exit_restore(cpu, ra);
> +                }
> +                /*
> +                 * Don't process the watchpoints when we are
> +                 * in a reverse debugging operation.
> +                 */
> +                replay_breakpoint();
> +                return;
> +            }
> +            if (flags == BP_MEM_READ) {
> +                wp->flags |= BP_WATCHPOINT_HIT_READ;
> +            } else {
> +                wp->flags |= BP_WATCHPOINT_HIT_WRITE;
> +            }
> +            wp->hitaddr = MAX(addr, wp->vaddr);
> +            wp->hitattrs = attrs;
> +
> +            if (wp->flags & BP_CPU && cc->tcg_ops->debug_check_watchpoint &&
> +                !cc->tcg_ops->debug_check_watchpoint(cpu, wp)) {
> +                wp->flags &= ~BP_WATCHPOINT_HIT;
> +                continue;
> +            }
> +            cpu->watchpoint_hit = wp;
> +
> +            mmap_lock();
> +            /* This call also restores vCPU state */
> +            tb_check_watchpoint(cpu, ra);
> +            if (wp->flags & BP_STOP_BEFORE_ACCESS) {
> +                cpu->exception_index = EXCP_DEBUG;
> +                mmap_unlock();
> +                cpu_loop_exit(cpu);
> +            } else {
> +                /* Force execution of one insn next time.  */
> +                cpu->cflags_next_tb = 1 | CF_LAST_IO | CF_NOIRQ
> +                                      | curr_cflags(cpu);
> +                mmap_unlock();
> +                cpu_loop_exit_noexc(cpu);
> +            }
> +        } else {
> +            wp->flags &= ~BP_WATCHPOINT_HIT;
> +        }
> +    }
> +}


Re: [PATCH-for-8.0] softmmu: Extract watchpoint API from physmem.c
Posted by Philippe Mathieu-Daudé 1 year, 4 months ago
+Peter/Laurent

On 9/12/22 15:12, Philippe Mathieu-Daudé wrote:
> The watchpoint API is specific to TCG system emulation.
> 
> Move it to a new compile unit. The inlined stubs are used
> for user-mode and non-TCG accelerators.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   MAINTAINERS           |   1 +
>   include/hw/core/cpu.h |   2 +-
>   softmmu/meson.build   |   3 +-
>   softmmu/physmem.c     | 191 ------------------------------------
>   softmmu/watchpoint.c  | 220 ++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 224 insertions(+), 193 deletions(-)
>   create mode 100644 softmmu/watchpoint.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6966490c94..979c3e2c3f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -120,6 +120,7 @@ M: Richard Henderson <richard.henderson@linaro.org>
>   R: Paolo Bonzini <pbonzini@redhat.com>
>   S: Maintained
>   F: softmmu/cpus.c
> +F: softmmu/watchpoint.c
>   F: cpus-common.c
>   F: page-vary.c
>   F: page-vary-common.c
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 8830546121..b490cc80d8 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -943,7 +943,7 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
>       return false;
>   }
>   
> -#ifdef CONFIG_USER_ONLY
> +#if !defined(CONFIG_TCG) || defined(CONFIG_USER_ONLY)
>   static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
>                                           int flags, CPUWatchpoint **watchpoint)

I guess remember Peter saying watchpoints could be supported in 
user-mode but I'm not sure... Anyhow if so this can still be done on top of
this patch, which removes 200 LoC from physmem.c which is the 2000 LoC.