[Qemu-devel] [PATCH 00/10] Implement ARM architectural watchpoints

[Qemu-devel] [PATCH 00/10] Implement ARM architectural watchpoints

[Peter Maydell]

These patches implement support for the ARM architecturally
defined watchpoints, for ARMv8 and ARMv7 CPUs.

The first three patches improve the generic exec.c watchpoint
code to provide some features that ARM watchpoints require.

Patch 4 is a cleanup that makes an ad-hoc per-target registered
callback function a proper QOM method (Andreas, I cc'd you on
this series in case you wanted to review this one.)

Patches 5..10 do the actual work. There are a couple of
slight corner cases we don't currently implement:
 * LDRT/STRT from kernel mode are supposed to trigger watchpoints
   set to fire on user mode accesses. This would require some
   effort to plumb the mmu_idx through into the watchpoint-check
   callbacks in exec.c, and Linux doesn't use it.
 * the cache-invalidate instruction DC IVAC strictly speaking should
   trigger watchpoints, but we currently just NOP that so it won't.

but all the functionality Linux uses works fine.

Tested: v7 kernel, v8 kernel with 64-bit gdbserver, v8 kernel
with 32-bit gdbserver using the compat interface. (A kernel
bug means 32-bit compat will incorrectly report watchpoint hits
to gdb as SIGTRAP. A proposed fix for this is here:
https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=fixes/core&id=27d7ff273c2aad37b28f6ff0cab2cfa35b51e648
and will presumably make its way into upstream kernels.)

Peter Maydell (10):
  exec.c: Relax restrictions on watchpoint length and alignment
  exec.c: Provide full set of dummy wp remove functions in user-mode
  exec.c: Record watchpoint fault address and direction
  cpu-exec: Make debug_excp_handler a QOM CPU method
  target-arm: Implement setting of watchpoints
  target-arm: Move extended_addresses_enabled() to internals.h
  target-arm: Implement handling of fired watchpoints
  target-arm: Set DBGDSCR.MOE for debug exceptions taken to AArch32
  target-arm: Remove comment about MDSCR_EL1 being dummy implementation
  target-arm: Implement minimal DBGVCR, OSDLR_EL1, MDCCSR_EL0

 cpu-exec.c              |  13 +---
 exec.c                  |  61 +++++++++++----
 include/exec/exec-all.h |   4 -
 include/qom/cpu.h       |  10 ++-
 linux-user/main.c       |   3 +-
 qom/cpu.c               |   5 ++
 target-arm/cpu.c        |   3 +
 target-arm/cpu.h        |   2 +
 target-arm/helper.c     | 202 +++++++++++++++++++++++++++++++++++++++++++-----
 target-arm/internals.h  |  30 +++++++
 target-arm/machine.c    |   3 +
 target-arm/op_helper.c  | 188 ++++++++++++++++++++++++++++++++++++++++++++
 target-i386/cpu.c       |   6 +-
 target-i386/cpu.h       |   2 +-
 target-i386/helper.c    |   5 +-
 target-lm32/cpu.c       |   2 +-
 target-lm32/cpu.h       |   2 +-
 target-lm32/helper.c    |   5 +-
 target-xtensa/cpu.c     |   2 +-
 target-xtensa/cpu.h     |   2 +-
 target-xtensa/helper.c  |   5 +-
 21 files changed, 491 insertions(+), 64 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 01/10] exec.c: Relax restrictions on watchpoint length and alignment

[Peter Maydell]

The current implementation of watchpoints requires that they
have a power of 2 length which is not greater than TARGET_PAGE_SIZE
and that their address is a multiple of their length. Watchpoints
on ARM don't fit these restrictions, so change the implementation
so they can be relaxed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c            | 44 +++++++++++++++++++++++++++++++-------------
 include/qom/cpu.h |  2 +-
 linux-user/main.c |  3 +--
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/exec.c b/exec.c
index 5f9857c..1f3f4a5 100644
--- a/exec.c
+++ b/exec.c
@@ -547,12 +547,10 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
 int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
                           int flags, CPUWatchpoint **watchpoint)
 {
-    vaddr len_mask = ~(len - 1);
     CPUWatchpoint *wp;
 
-    /* sanity checks: allow power-of-2 lengths, deny unaligned watchpoints */
-    if ((len & (len - 1)) || (addr & ~len_mask) ||
-            len == 0 || len > TARGET_PAGE_SIZE) {
+    /* 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;
@@ -560,7 +558,7 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
     wp = g_malloc(sizeof(*wp));
 
     wp->vaddr = addr;
-    wp->len_mask = len_mask;
+    wp->len = len;
     wp->flags = flags;
 
     /* keep all GDB-injected watchpoints in front */
@@ -581,11 +579,10 @@ int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
 int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
                           int flags)
 {
-    vaddr len_mask = ~(len - 1);
     CPUWatchpoint *wp;
 
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        if (addr == wp->vaddr && len_mask == wp->len_mask
+        if (addr == wp->vaddr && len == wp->len
                 && flags == (wp->flags & ~BP_WATCHPOINT_HIT)) {
             cpu_watchpoint_remove_by_ref(cpu, wp);
             return 0;
@@ -615,6 +612,27 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
         }
     }
 }
+
+/* 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 cpu_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);
+}
+
 #endif
 
 /* Add a breakpoint.  */
@@ -826,7 +844,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
     /* Make accesses to pages with watchpoints go via the
        watchpoint trap routines.  */
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
+        if (cpu_watchpoint_address_matches(wp, vaddr, TARGET_PAGE_SIZE)) {
             /* Avoid trapping reads of pages with a write breakpoint. */
             if ((prot & PAGE_WRITE) || (wp->flags & BP_MEM_READ)) {
                 iotlb = PHYS_SECTION_WATCH + paddr;
@@ -1590,7 +1608,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
 };
 
 /* Generate a debug exception if a watchpoint has been hit.  */
-static void check_watchpoint(int offset, int len_mask, int flags)
+static void check_watchpoint(int offset, int len, int flags)
 {
     CPUState *cpu = current_cpu;
     CPUArchState *env = cpu->env_ptr;
@@ -1608,8 +1626,8 @@ static void check_watchpoint(int offset, int len_mask, int flags)
     }
     vaddr = (cpu->mem_io_vaddr & TARGET_PAGE_MASK) + offset;
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        if ((vaddr == (wp->vaddr & len_mask) ||
-             (vaddr & wp->len_mask) == wp->vaddr) && (wp->flags & flags)) {
+        if (cpu_watchpoint_address_matches(wp, vaddr, len)
+            && (wp->flags & flags)) {
             wp->flags |= BP_WATCHPOINT_HIT;
             if (!cpu->watchpoint_hit) {
                 cpu->watchpoint_hit = wp;
@@ -1635,7 +1653,7 @@ static void check_watchpoint(int offset, int len_mask, int flags)
 static uint64_t watch_mem_read(void *opaque, hwaddr addr,
                                unsigned size)
 {
-    check_watchpoint(addr & ~TARGET_PAGE_MASK, ~(size - 1), BP_MEM_READ);
+    check_watchpoint(addr & ~TARGET_PAGE_MASK, size, BP_MEM_READ);
     switch (size) {
     case 1: return ldub_phys(&address_space_memory, addr);
     case 2: return lduw_phys(&address_space_memory, addr);
@@ -1647,7 +1665,7 @@ static uint64_t watch_mem_read(void *opaque, hwaddr addr,
 static void watch_mem_write(void *opaque, hwaddr addr,
                             uint64_t val, unsigned size)
 {
-    check_watchpoint(addr & ~TARGET_PAGE_MASK, ~(size - 1), BP_MEM_WRITE);
+    check_watchpoint(addr & ~TARGET_PAGE_MASK, size, BP_MEM_WRITE);
     switch (size) {
     case 1:
         stb_phys(&address_space_memory, addr, val);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 1aafbf5..7c06f37 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -169,7 +169,7 @@ typedef struct CPUBreakpoint {
 
 typedef struct CPUWatchpoint {
     vaddr vaddr;
-    vaddr len_mask;
+    vaddr len;
     int flags; /* BP_* */
     QTAILQ_ENTRY(CPUWatchpoint) entry;
 } CPUWatchpoint;
diff --git a/linux-user/main.c b/linux-user/main.c
index 472a16d..483eb3f 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3458,8 +3458,7 @@ CPUArchState *cpu_copy(CPUArchState *env)
         cpu_breakpoint_insert(new_cpu, bp->pc, bp->flags, NULL);
     }
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
-        cpu_watchpoint_insert(new_cpu, wp->vaddr, (~wp->len_mask) + 1,
-                              wp->flags, NULL);
+        cpu_watchpoint_insert(new_cpu, wp->vaddr, wp->len, wp->flags, NULL);
     }
 #endif
 
-- 
1.9.1

[Qemu-devel] [PATCH 02/10] exec.c: Provide full set of dummy wp remove functions in user-mode

[Peter Maydell]

We already provide dummy versions of the cpu_watchpoint_insert
and cpu_watchpoint_remove_all functions when CONFIG_USER_ONLY
is defined. Complete the set by providing cpu_watchpoint_remove
and cpu_watchpoint_remove_by_ref as well.

This allows target-* code using these functions to avoid
some ifdeffery.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/exec.c b/exec.c
index 1f3f4a5..82a90e7 100644
--- a/exec.c
+++ b/exec.c
@@ -537,6 +537,16 @@ void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
 {
 }
 
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len,
+                          int flags)
+{
+    return -ENOSYS;
+}
+
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint)
+{
+}
+
 int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
                           int flags, CPUWatchpoint **watchpoint)
 {
-- 
1.9.1

[Qemu-devel] [PATCH 03/10] exec.c: Record watchpoint fault address and direction

[Peter Maydell]

When we check whether we've hit a watchpoint we know the address
that we were attempting to access and whether it was a read or a
write. Record this information in the CPUWatchpoint struct so that
target-specific code can report it to the guest.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c            | 7 ++++++-
 include/qom/cpu.h | 6 +++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 82a90e7..57376c0 100644
--- a/exec.c
+++ b/exec.c
@@ -1638,7 +1638,12 @@ static void check_watchpoint(int offset, int len, int flags)
     QTAILQ_FOREACH(wp, &cpu->watchpoints, entry) {
         if (cpu_watchpoint_address_matches(wp, vaddr, len)
             && (wp->flags & flags)) {
-            wp->flags |= BP_WATCHPOINT_HIT;
+            if (flags == BP_MEM_READ) {
+                wp->flags |= BP_WATCHPOINT_HIT_READ;
+            } else {
+                wp->flags |= BP_WATCHPOINT_HIT_WRITE;
+            }
+            wp->hitaddr = vaddr;
             if (!cpu->watchpoint_hit) {
                 cpu->watchpoint_hit = wp;
                 tb_check_watchpoint(cpu);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7c06f37..c325774 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -170,6 +170,7 @@ typedef struct CPUBreakpoint {
 typedef struct CPUWatchpoint {
     vaddr vaddr;
     vaddr len;
+    vaddr hitaddr;
     int flags; /* BP_* */
     QTAILQ_ENTRY(CPUWatchpoint) entry;
 } CPUWatchpoint;
@@ -622,9 +623,12 @@ void cpu_single_step(CPUState *cpu, int enabled);
 #define BP_MEM_WRITE          0x02
 #define BP_MEM_ACCESS         (BP_MEM_READ | BP_MEM_WRITE)
 #define BP_STOP_BEFORE_ACCESS 0x04
-#define BP_WATCHPOINT_HIT     0x08
+/* 0x08 currently unused */
 #define BP_GDB                0x10
 #define BP_CPU                0x20
+#define BP_WATCHPOINT_HIT_READ 0x40
+#define BP_WATCHPOINT_HIT_WRITE 0x80
+#define BP_WATCHPOINT_HIT (BP_WATCHPOINT_HIT_READ | BP_WATCHPOINT_HIT_WRITE)
 
 int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
                           CPUBreakpoint **breakpoint);
-- 
1.9.1

[Qemu-devel] [PATCH 04/10] cpu-exec: Make debug_excp_handler a QOM CPU method

[Peter Maydell]

Make the debug_excp_handler target specific hook into a QOM
CPU method.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu-exec.c              | 13 +++----------
 include/exec/exec-all.h |  4 ----
 include/qom/cpu.h       |  2 ++
 qom/cpu.c               |  5 +++++
 target-i386/cpu.c       |  6 +++---
 target-i386/cpu.h       |  2 +-
 target-i386/helper.c    |  5 +++--
 target-lm32/cpu.c       |  2 +-
 target-lm32/cpu.h       |  2 +-
 target-lm32/helper.c    |  5 +++--
 target-xtensa/cpu.c     |  2 +-
 target-xtensa/cpu.h     |  2 +-
 target-xtensa/helper.c  |  5 +++--
 13 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index c6aad74..82c7358 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -295,16 +295,10 @@ static inline TranslationBlock *tb_find_fast(CPUArchState *env)
     return tb;
 }
 
-static CPUDebugExcpHandler *debug_excp_handler;
-
-void cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
-{
-    debug_excp_handler = handler;
-}
-
 static void cpu_handle_debug_exception(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUWatchpoint *wp;
 
     if (!cpu->watchpoint_hit) {
@@ -312,9 +306,8 @@ static void cpu_handle_debug_exception(CPUArchState *env)
             wp->flags &= ~BP_WATCHPOINT_HIT;
         }
     }
-    if (debug_excp_handler) {
-        debug_excp_handler(env);
-    }
+
+    cc->debug_excp_handler(cpu);
 }
 
 /* main execution loop */
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5e5d86e..421a142 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -356,10 +356,6 @@ static inline tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong
 tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr);
 #endif
 
-typedef void (CPUDebugExcpHandler)(CPUArchState *env);
-
-void cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);
-
 /* vl.c */
 extern int singlestep;
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index c325774..370b3eb 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -95,6 +95,7 @@ struct TranslationBlock;
  * @get_phys_page_debug: Callback for obtaining a physical address.
  * @gdb_read_register: Callback for letting GDB read a register.
  * @gdb_write_register: Callback for letting GDB write a register.
+ * @debug_excp_handler: Callback for handling debug exceptions.
  * @vmsd: State description for migration.
  * @gdb_num_core_regs: Number of core registers accessible to GDB.
  * @gdb_core_xml_file: File name for core registers GDB XML description.
@@ -134,6 +135,7 @@ typedef struct CPUClass {
     hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
     int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg);
     int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
+    void (*debug_excp_handler)(CPUState *cpu);
 
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
                             int cpuid, void *opaque);
diff --git a/qom/cpu.c b/qom/cpu.c
index b32dd0a..ba8b402 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -202,6 +202,10 @@ static bool cpu_common_virtio_is_big_endian(CPUState *cpu)
     return target_words_bigendian();
 }
 
+static void cpu_common_debug_excp_handler(CPUState *cpu)
+{
+}
+
 void cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
                     int flags)
 {
@@ -340,6 +344,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->gdb_read_register = cpu_common_gdb_read_register;
     k->gdb_write_register = cpu_common_gdb_write_register;
     k->virtio_is_big_endian = cpu_common_virtio_is_big_endian;
+    k->debug_excp_handler = cpu_common_debug_excp_handler;
     dc->realize = cpu_common_realizefn;
     /*
      * Reason: CPUs still need special care by board code: wiring up
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index fa811a0..9a6f34b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2835,9 +2835,6 @@ static void x86_cpu_initfn(Object *obj)
     if (tcg_enabled() && !inited) {
         inited = 1;
         optimize_flags_init();
-#ifndef CONFIG_USER_ONLY
-        cpu_set_debug_excp_handler(breakpoint_handler);
-#endif
     }
 }
 
@@ -2934,6 +2931,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->vmsd = &vmstate_x86_cpu;
 #endif
     cc->gdb_num_core_regs = CPU_NB_REGS * 2 + 25;
+#ifndef CONFIG_USER_ONLY
+    cc->debug_excp_handler = breakpoint_handler;
+#endif
 }
 
 static const TypeInfo x86_cpu_type_info = {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 3460b12..71b505f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1121,7 +1121,7 @@ static inline int hw_breakpoint_len(unsigned long dr7, int index)
 void hw_breakpoint_insert(CPUX86State *env, int index);
 void hw_breakpoint_remove(CPUX86State *env, int index);
 bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update);
-void breakpoint_handler(CPUX86State *env);
+void breakpoint_handler(CPUState *cs);
 
 /* will be suppressed */
 void cpu_x86_update_cr0(CPUX86State *env, uint32_t new_cr0);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 30cb0d0..28fefe0 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1011,9 +1011,10 @@ bool check_hw_breakpoints(CPUX86State *env, bool force_dr6_update)
     return hit_enabled;
 }
 
-void breakpoint_handler(CPUX86State *env)
+void breakpoint_handler(CPUState *cs)
 {
-    CPUState *cs = CPU(x86_env_get_cpu(env));
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
     CPUBreakpoint *bp;
 
     if (cs->watchpoint_hit) {
diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
index c5c20d7..419d664 100644
--- a/target-lm32/cpu.c
+++ b/target-lm32/cpu.c
@@ -158,7 +158,6 @@ static void lm32_cpu_initfn(Object *obj)
     if (tcg_enabled() && !tcg_initialized) {
         tcg_initialized = true;
         lm32_translate_init();
-        cpu_set_debug_excp_handler(lm32_debug_excp_handler);
     }
 }
 
@@ -273,6 +272,7 @@ static void lm32_cpu_class_init(ObjectClass *oc, void *data)
     cc->vmsd = &vmstate_lm32_cpu;
 #endif
     cc->gdb_num_core_regs = 32 + 7;
+    cc->debug_excp_handler = lm32_debug_excp_handler;
 }
 
 static void lm32_register_cpu_type(const LM32CPUInfo *info)
diff --git a/target-lm32/cpu.h b/target-lm32/cpu.h
index 70600aa..0dab6e8 100644
--- a/target-lm32/cpu.h
+++ b/target-lm32/cpu.h
@@ -211,7 +211,7 @@ void lm32_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 void lm32_translate_init(void);
 void cpu_lm32_set_phys_msb_ignore(CPULM32State *env, int value);
 void QEMU_NORETURN raise_exception(CPULM32State *env, int index);
-void lm32_debug_excp_handler(CPULM32State *env);
+void lm32_debug_excp_handler(CPUState *cs);
 void lm32_breakpoint_insert(CPULM32State *env, int index, target_ulong address);
 void lm32_breakpoint_remove(CPULM32State *env, int index);
 void lm32_watchpoint_insert(CPULM32State *env, int index, target_ulong address,
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index 1bca196..ad724ae 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -125,9 +125,10 @@ static bool check_watchpoints(CPULM32State *env)
     return false;
 }
 
-void lm32_debug_excp_handler(CPULM32State *env)
+void lm32_debug_excp_handler(CPUState *cs)
 {
-    CPUState *cs = CPU(lm32_env_get_cpu(env));
+    LM32CPU *cpu = LM32_CPU(cs);
+    CPULM32State *env = &cpu->env;
     CPUBreakpoint *bp;
 
     if (cs->watchpoint_hit) {
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index 9d8801b..936d526 100644
--- a/target-xtensa/cpu.c
+++ b/target-xtensa/cpu.c
@@ -119,7 +119,6 @@ static void xtensa_cpu_initfn(Object *obj)
     if (tcg_enabled() && !tcg_inited) {
         tcg_inited = true;
         xtensa_translate_init();
-        cpu_set_debug_excp_handler(xtensa_breakpoint_handler);
     }
 }
 
@@ -151,6 +150,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_unaligned_access = xtensa_cpu_do_unaligned_access;
     cc->get_phys_page_debug = xtensa_cpu_get_phys_page_debug;
 #endif
+    cc->debug_excp_handler = xtensa_breakpoint_handler;
     dc->vmsd = &vmstate_xtensa_cpu;
 }
 
diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
index d797d26..9cf5275 100644
--- a/target-xtensa/cpu.h
+++ b/target-xtensa/cpu.h
@@ -390,7 +390,7 @@ static inline CPUXtensaState *cpu_init(const char *cpu_model)
 }
 
 void xtensa_translate_init(void);
-void xtensa_breakpoint_handler(CPUXtensaState *env);
+void xtensa_breakpoint_handler(CPUState *cs);
 int cpu_xtensa_exec(CPUXtensaState *s);
 void xtensa_register_core(XtensaConfigList *node);
 void check_interrupts(CPUXtensaState *s);
diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
index 94dcd94..6671e40 100644
--- a/target-xtensa/helper.c
+++ b/target-xtensa/helper.c
@@ -79,9 +79,10 @@ static uint32_t check_hw_breakpoints(CPUXtensaState *env)
     return 0;
 }
 
-void xtensa_breakpoint_handler(CPUXtensaState *env)
+void xtensa_breakpoint_handler(CPUState *cs)
 {
-    CPUState *cs = CPU(xtensa_env_get_cpu(env));
+    XtensaCPU *cpu = XTENSA_CPU(cs);
+    CPUXtensaState *env = &cpu->env;
 
     if (cs->watchpoint_hit) {
         if (cs->watchpoint_hit->flags & BP_CPU) {
-- 
1.9.1

[Qemu-devel] [PATCH 05/10] target-arm: Implement setting of watchpoints

[Peter Maydell]

Implement support for setting QEMU watchpoints based on the
values the guest writes to the ARM architected watchpoint
registers. (We do not yet report the firing of the watchpoints
to the guest, so they will just be ignored.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.c       |   2 +
 target-arm/cpu.h       |   2 +
 target-arm/helper.c    | 135 +++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/internals.h |  10 ++++
 target-arm/machine.c   |   3 ++
 5 files changed, 149 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 8199f32..3d12656 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -172,6 +172,8 @@ static void arm_cpu_reset(CPUState *s)
         kvm_arm_reset_vcpu(cpu);
     }
 #endif
+
+    hw_watchpoint_update_all(cpu);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 8098b8d..2218127 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -322,6 +322,8 @@ typedef struct CPUARMState {
     int eabi;
 #endif
 
+    struct CPUWatchpoint *cpu_watchpoint[16];
+
     CPU_COMMON
 
     /* These fields after the common ones so they are preserved on reset.  */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 2a77c97..406b9bc 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2223,6 +2223,131 @@ static const ARMCPRegInfo debug_lpae_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+void hw_watchpoint_update(ARMCPU *cpu, int n)
+{
+    CPUARMState *env = &cpu->env;
+    vaddr len = 0;
+    vaddr wvr = env->cp15.dbgwvr[n];
+    uint64_t wcr = env->cp15.dbgwcr[n];
+    int mask;
+    int flags = BP_CPU | BP_STOP_BEFORE_ACCESS;
+
+    if (env->cpu_watchpoint[n]) {
+        cpu_watchpoint_remove_by_ref(CPU(cpu), env->cpu_watchpoint[n]);
+        env->cpu_watchpoint[n] = NULL;
+    }
+
+    if (!extract64(wcr, 0, 1)) {
+        /* E bit clear : watchpoint disabled */
+        return;
+    }
+
+    switch (extract64(wcr, 3, 2)) {
+    case 0:
+        /* LSC 00 is reserved and must behave as if the wp is disabled */
+        return;
+    case 1:
+        flags |= BP_MEM_READ;
+        break;
+    case 2:
+        flags |= BP_MEM_WRITE;
+        break;
+    case 3:
+        flags |= BP_MEM_ACCESS;
+        break;
+    }
+
+    /* Attempts to use both MASK and BAS fields simultaneously are
+     * CONSTRAINED UNPREDICTABLE; we opt to ignore BAS in this case,
+     * thus generating a watchpoint for every byte in the masked region.
+     */
+    mask = extract64(wcr, 24, 4);
+    if (mask == 1 || mask == 2) {
+        /* Reserved values of MASK; we must act as if the mask value was
+         * some non-reserved value, or as if the watchpoint were disabled.
+         * We choose the latter.
+         */
+        return;
+    } else if (mask) {
+        /* Watchpoint covers an aligned area up to 2GB in size */
+        len = 1ULL << mask;
+        /* If masked bits in WVR are not zero it's CONSTRAINED UNPREDICTABLE
+         * whether the watchpoint fires when the unmasked bits match; we opt
+         * to generate the exceptions.
+         */
+        wvr &= (len - 1);
+    } else {
+        /* Watchpoint covers bytes defined by the byte address select bits */
+        int bas = extract64(wcr, 5, 8);
+        int basstart;
+
+        if (bas == 0) {
+            /* This must act as if the watchpoint is disabled */
+            return;
+        }
+
+        if (extract64(wvr, 2, 1)) {
+            /* Deprecated case of an only 4-aligned address. BAS[7:4] are
+             * ignored, and BAS[3:0] define which bytes to watch.
+             */
+            bas &= 0xf;
+        }
+        /* The BAS bits are supposed to be programmed to indicate a contiguous
+         * range of bytes. Otherwise it is CONSTRAINED UNPREDICTABLE whether
+         * we fire for each byte in the word/doubleword addressed by the WVR.
+         * We choose to ignore any non-zero bits after the first range of 1s.
+         */
+        basstart = ctz32(bas);
+        len = cto32(bas >> basstart);
+        wvr += basstart;
+    }
+
+    cpu_watchpoint_insert(CPU(cpu), wvr, len, flags,
+                          &env->cpu_watchpoint[n]);
+}
+
+void hw_watchpoint_update_all(ARMCPU *cpu)
+{
+    int i;
+    CPUARMState *env = &cpu->env;
+
+    /* Completely clear out existing QEMU watchpoints and our array, to
+     * avoid possible stale entries following migration load.
+     */
+    cpu_watchpoint_remove_all(CPU(cpu), BP_CPU);
+    memset(env->cpu_watchpoint, 0, sizeof(env->cpu_watchpoint));
+
+    for (i = 0; i < ARRAY_SIZE(cpu->env.cpu_watchpoint); i++) {
+        hw_watchpoint_update(cpu, i);
+    }
+}
+
+static void dbgwvr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    int i = ri->crm;
+
+    /* Bits [63:49] are hardwired to the value of bit [48]; that is, the
+     * register reads and behaves as if values written are sign extended.
+     * Bits [1:0] are RES0.
+     */
+    value = sextract64(value, 0, 49) & ~3ULL;
+
+    raw_write(env, ri, value);
+    hw_watchpoint_update(cpu, i);
+}
+
+static void dbgwcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    int i = ri->crm;
+
+    raw_write(env, ri, value);
+    hw_watchpoint_update(cpu, i);
+}
+
 static void define_debug_regs(ARMCPU *cpu)
 {
     /* Define v7 and v8 architectural debug registers.
@@ -2274,12 +2399,16 @@ static void define_debug_regs(ARMCPU *cpu)
             { .name = "DBGWVR", .state = ARM_CP_STATE_BOTH,
               .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 6,
               .access = PL1_RW,
-              .fieldoffset = offsetof(CPUARMState, cp15.dbgwvr[i]) },
+              .fieldoffset = offsetof(CPUARMState, cp15.dbgwvr[i]),
+              .writefn = dbgwvr_write, .raw_writefn = raw_write
+            },
             { .name = "DBGWCR", .state = ARM_CP_STATE_BOTH,
               .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 7,
               .access = PL1_RW,
-              .fieldoffset = offsetof(CPUARMState, cp15.dbgwcr[i]) },
-               REGINFO_SENTINEL
+              .fieldoffset = offsetof(CPUARMState, cp15.dbgwcr[i]),
+              .writefn = dbgwcr_write, .raw_writefn = raw_write
+            },
+            REGINFO_SENTINEL
         };
         define_arm_cp_regs(cpu, dbgregs);
     }
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 53c2e3c..22f382c 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -296,4 +296,14 @@ static inline uint32_t syn_swstep(int same_el, int isv, int ex)
         | (isv << 24) | (ex << 6) | 0x22;
 }
 
+/* Update a QEMU watchpoint based on the information the guest has set in the
+ * DBGWCR<n>_EL1 and DBGWVR<n>_EL1 registers.
+ */
+void hw_watchpoint_update(ARMCPU *cpu, int n);
+/* Update the QEMU watchpoints for every guest watchpoint. This does a
+ * complete delete-and-reinstate of the QEMU watchpoint list and so is
+ * suitable for use after migration or on reset.
+ */
+void hw_watchpoint_update_all(ARMCPU *cpu);
+
 #endif
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 3bcc7cc..8dfe87c 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -2,6 +2,7 @@
 #include "hw/boards.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
+#include "internals.h"
 
 static bool vfp_needed(void *opaque)
 {
@@ -213,6 +214,8 @@ static int cpu_post_load(void *opaque, int version_id)
         }
     }
 
+    hw_watchpoint_update_all(cpu);
+
     return 0;
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 05/10] target-arm: Implement setting of watchpoints

[Richard Henderson]

On 08/29/2014 04:21 AM, Peter Maydell wrote:
> +        /* Watchpoint covers an aligned area up to 2GB in size */
> +        len = 1ULL << mask;
> +        /* If masked bits in WVR are not zero it's CONSTRAINED UNPREDICTABLE
> +         * whether the watchpoint fires when the unmasked bits match; we opt
> +         * to generate the exceptions.
> +         */
> +        wvr &= (len - 1);

This looks funny... address being clipped to length?
Surely it's ~(len - 1)...


r~

Re: [Qemu-devel] [PATCH 05/10] target-arm: Implement setting of watchpoints

[Peter Maydell]

On 29 August 2014 17:42, Richard Henderson <rth@twiddle.net> wrote:
> On 08/29/2014 04:21 AM, Peter Maydell wrote:
>> +        /* Watchpoint covers an aligned area up to 2GB in size */
>> +        len = 1ULL << mask;
>> +        /* If masked bits in WVR are not zero it's CONSTRAINED UNPREDICTABLE
>> +         * whether the watchpoint fires when the unmasked bits match; we opt
>> +         * to generate the exceptions.
>> +         */
>> +        wvr &= (len - 1);
>
> This looks funny... address being clipped to length?
> Surely it's ~(len - 1)...

Nice catch. Linux doesn't actually use this bit of the watchpoint
functionality, which is why I didn't notice.

-- PMM

[Qemu-devel] [PATCH 06/10] target-arm: Move extended_addresses_enabled() to internals.h

[Peter Maydell]

Move the utility function extended_addresses_enabled() into
internals.h; we're going to need to call it from op_helper.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c    | 11 -----------
 target-arm/internals.h | 11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 406b9bc..7963807 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -304,17 +304,6 @@ void init_cpreg_list(ARMCPU *cpu)
     g_list_free(keys);
 }
 
-/* Return true if extended addresses are enabled.
- * This is always the case if our translation regime is 64 bit,
- * but depends on TTBCR.EAE for 32 bit.
- */
-static inline bool extended_addresses_enabled(CPUARMState *env)
-{
-    return arm_el_is_aa64(env, 1)
-        || ((arm_feature(env, ARM_FEATURE_LPAE)
-             && (env->cp15.c2_control & TTBCR_EAE)));
-}
-
 static void dacr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 22f382c..1d788b0 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -142,6 +142,17 @@ static inline void update_spsel(CPUARMState *env, uint32_t imm)
     aarch64_restore_sp(env, cur_el);
 }
 
+/* Return true if extended addresses are enabled.
+ * This is always the case if our translation regime is 64 bit,
+ * but depends on TTBCR.EAE for 32 bit.
+ */
+static inline bool extended_addresses_enabled(CPUARMState *env)
+{
+    return arm_el_is_aa64(env, 1)
+        || ((arm_feature(env, ARM_FEATURE_LPAE)
+             && (env->cp15.c2_control & TTBCR_EAE)));
+}
+
 /* Valid Syndrome Register EC field values */
 enum arm_exception_class {
     EC_UNCATEGORIZED          = 0x00,
-- 
1.9.1

[Qemu-devel] [PATCH 07/10] target-arm: Implement handling of fired watchpoints

[Peter Maydell]

Implement the ARM debug exception handler for dealing with
fired watchpoints.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.c       |   1 +
 target-arm/helper.c    |   7 +-
 target-arm/internals.h |   9 +++
 target-arm/op_helper.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 204 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 3d12656..b2df26d 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1053,6 +1053,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
 #endif
     cc->gdb_num_core_regs = 26;
     cc->gdb_core_xml_file = "arm-core.xml";
+    cc->debug_excp_handler = arm_debug_excp_handler;
 }
 
 static void cpu_register(const ARMCPUInfo *info)
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 7963807..40dde9c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2343,14 +2343,18 @@ static void define_debug_regs(ARMCPU *cpu)
      * These are just dummy implementations for now.
      */
     int i;
-    int wrps, brps;
+    int wrps, brps, ctx_cmps;
     ARMCPRegInfo dbgdidr = {
         .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 0,
         .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,
     };
 
+    /* Note that all these register fields hold "number of Xs minus 1". */
     brps = extract32(cpu->dbgdidr, 24, 4);
     wrps = extract32(cpu->dbgdidr, 28, 4);
+    ctx_cmps = extract32(cpu->dbgdidr, 20, 4);
+
+    assert(ctx_cmps <= brps);
 
     /* The DBGDIDR and ID_AA64DFR0_EL1 define various properties
      * of the debug registers such as number of breakpoints;
@@ -2359,6 +2363,7 @@ static void define_debug_regs(ARMCPU *cpu)
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         assert(extract32(cpu->id_aa64dfr0, 12, 4) == brps);
         assert(extract32(cpu->id_aa64dfr0, 20, 4) == wrps);
+        assert(extract32(cpu->id_aa64dfr0, 28, 4) == ctx_cmps);
     }
 
     define_one_arm_cp_reg(cpu, &dbgdidr);
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 1d788b0..64751a0 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -307,6 +307,12 @@ static inline uint32_t syn_swstep(int same_el, int isv, int ex)
         | (isv << 24) | (ex << 6) | 0x22;
 }
 
+static inline uint32_t syn_watchpoint(int same_el, int cm, int wnr)
+{
+    return (EC_WATCHPOINT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
+        | (cm << 8) | (wnr << 6) | 0x22;
+}
+
 /* Update a QEMU watchpoint based on the information the guest has set in the
  * DBGWCR<n>_EL1 and DBGWVR<n>_EL1 registers.
  */
@@ -317,4 +323,7 @@ void hw_watchpoint_update(ARMCPU *cpu, int n);
  */
 void hw_watchpoint_update_all(ARMCPU *cpu);
 
+/* Callback function for when a watchpoint or breakpoint triggers. */
+void arm_debug_excp_handler(CPUState *cs);
+
 #endif
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index fe40358..b956216 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -456,6 +456,194 @@ illegal_return:
     }
 }
 
+/* Return true if the linked breakpoint entry lbn passes its checks */
+static bool linked_bp_matches(ARMCPU *cpu, int lbn)
+{
+    CPUARMState *env = &cpu->env;
+    uint64_t bcr = env->cp15.dbgbcr[lbn];
+    int brps = extract32(cpu->dbgdidr, 24, 4);
+    int ctx_cmps = extract32(cpu->dbgdidr, 20, 4);
+    int bt;
+    uint32_t contextidr;
+
+    /* Links to unimplemented or non-context aware breakpoints are
+     * CONSTRAINED UNPREDICTABLE: either behave as if disabled, or
+     * as if linked to an UNKNOWN context-aware breakpoint (in which
+     * case DBGWCR<n>_EL1.LBN must indicate that breakpoint).
+     * We choose the former.
+     */
+    if (lbn > brps || lbn < (brps - ctx_cmps)) {
+        return false;
+    }
+
+    bcr = env->cp15.dbgbcr[lbn];
+
+    if (extract64(bcr, 0, 1) == 0) {
+        /* Linked breakpoint disabled : generate no events */
+        return false;
+    }
+
+    bt = extract64(bcr, 20, 4);
+
+    /* We match the whole register even if this is AArch32 using the
+     * short descriptor format (in which case it holds both PROCID and ASID),
+     * since we don't implement the optional v7 context ID masking.
+     */
+    contextidr = extract64(env->cp15.contextidr_el1, 0, 32);
+
+    switch (bt) {
+    case 3: /* linked context ID match */
+        if (arm_current_pl(env) > 1) {
+            /* Context matches never fire in EL2 or (AArch64) EL3 */
+            return false;
+        }
+        return (contextidr == extract64(env->cp15.dbgbvr[lbn], 0, 32));
+    case 5: /* linked address mismatch (reserved in AArch64) */
+    case 9: /* linked VMID match (reserved if no EL2) */
+    case 11: /* linked context ID and VMID match (reserved if no EL2) */
+    default:
+        /* Links to Unlinked context breakpoints must generate no
+         * events; we choose to do the same for reserved values too.
+         */
+        return false;
+    }
+
+    return false;
+}
+
+static bool wp_matches(ARMCPU *cpu, int n)
+{
+    CPUARMState *env = &cpu->env;
+    uint64_t wcr = env->cp15.dbgwcr[n];
+    int pac, hmc, ssc, wt, lbn;
+    /* TODO: check against CPU security state when we implement TrustZone */
+    bool is_secure = false;
+
+    if (!env->cpu_watchpoint[n]
+        || !(env->cpu_watchpoint[n]->flags & BP_WATCHPOINT_HIT)) {
+        return false;
+    }
+
+    /* The WATCHPOINT_HIT flag guarantees us that the watchpoint is
+     * enabled and that the address and access type match; check the
+     * remaining fields, including linked breakpoints.
+     * Note that some combinations of {PAC, HMC SSC} are reserved and
+     * must act either like some valid combination or as if the watchpoint
+     * were disabled. We choose the former, and use this together with
+     * the fact that EL3 must always be Secure and EL2 must always be
+     * Non-Secure to simplify the code slightly compared to the full
+     * table in the ARM ARM.
+     */
+    pac = extract64(wcr, 1, 2);
+    hmc = extract64(wcr, 13, 1);
+    ssc = extract64(wcr, 14, 2);
+
+    switch (ssc) {
+    case 0:
+        break;
+    case 1:
+    case 3:
+        if (is_secure) {
+            return false;
+        }
+        break;
+    case 2:
+        if (!is_secure) {
+            return false;
+        }
+        break;
+    }
+
+    /* TODO: this is not strictly correct because the LDRT/STRT/LDT/STT
+     * "unprivileged access" instructions should match watchpoints as if
+     * they were accesses done at EL0, even if the CPU is at EL1 or higher.
+     * Implementing this would require reworking the core watchpoint code
+     * to plumb the mmu_idx through to this point. Luckily Linux does not
+     * rely on this behaviour currently.
+     */
+    switch (arm_current_pl(env)) {
+    case 3:
+    case 2:
+        if (!hmc) {
+            return false;
+        }
+        break;
+    case 1:
+        if (extract32(pac, 0, 1) == 0) {
+            return false;
+        }
+        break;
+    case 0:
+        if (extract32(pac, 1, 1) == 0) {
+            return false;
+        }
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    wt = extract64(wcr, 20, 1);
+    lbn = extract64(wcr, 16, 4);
+
+    if (wt && !linked_bp_matches(cpu, lbn)) {
+        return false;
+    }
+
+    return true;
+}
+
+static bool check_watchpoints(ARMCPU *cpu)
+{
+    CPUARMState *env = &cpu->env;
+    int n;
+
+    /* If watchpoints are disabled globally or we can't take debug
+     * exceptions here then watchpoint firings are ignored.
+     */
+    if (extract32(env->cp15.mdscr_el1, 15, 1) == 0
+        || !arm_generate_debug_exceptions(env)) {
+        return false;
+    }
+
+    for (n = 0; n < ARRAY_SIZE(env->cpu_watchpoint); n++) {
+        if (wp_matches(cpu, n)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+void arm_debug_excp_handler(CPUState *cs)
+{
+    /* Called by core code when a watchpoint or breakpoint fires;
+     * need to check which one and raise the appropriate exception.
+     */
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    CPUWatchpoint *wp_hit = cs->watchpoint_hit;
+
+    if (wp_hit) {
+        if (wp_hit->flags & BP_CPU) {
+            cs->watchpoint_hit = NULL;
+            if (check_watchpoints(cpu)) {
+                bool wnr = (wp_hit->flags & BP_WATCHPOINT_HIT_WRITE) != 0;
+                bool same_el = arm_debug_target_el(env) == arm_current_pl(env);
+
+                env->exception.syndrome = syn_watchpoint(same_el, 0, wnr);
+                if (extended_addresses_enabled(env)) {
+                    env->exception.fsr = (1 << 9) | 0x22;
+                } else {
+                    env->exception.fsr = 0x2;
+                }
+                env->exception.vaddress = wp_hit->hitaddr;
+                raise_exception(env, EXCP_DATA_ABORT);
+            } else {
+                cpu_resume_from_signal(cs, NULL);
+            }
+        }
+    }
+}
+
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
    The only way to do that in TCG is a conditional branch, which clobbers
    all our temporaries.  For now implement these as helper functions.  */
-- 
1.9.1

[Qemu-devel] [PATCH 08/10] target-arm: Set DBGDSCR.MOE for debug exceptions taken to AArch32

[Peter Maydell]

For debug exceptions taken to AArch32 we have to set the
DBGDSCR.MOE (Method Of Entry) bits; we can identify the
kind of debug exception from the information in
exception.syndrome.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 40dde9c..ef1eac9 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3563,11 +3563,37 @@ void arm_cpu_do_interrupt(CPUState *cs)
     uint32_t mask;
     int new_mode;
     uint32_t offset;
+    uint32_t moe;
 
     assert(!IS_M(env));
 
     arm_log_exception(cs->exception_index);
 
+    /* If this is a debug exception we must update the DBGDSCR.MOE bits */
+    switch (env->exception.syndrome >> ARM_EL_EC_SHIFT) {
+    case EC_BREAKPOINT:
+    case EC_BREAKPOINT_SAME_EL:
+        moe = 1;
+        break;
+    case EC_WATCHPOINT:
+    case EC_WATCHPOINT_SAME_EL:
+        moe = 10;
+        break;
+    case EC_AA32_BKPT:
+        moe = 3;
+        break;
+    case EC_VECTORCATCH:
+        moe = 5;
+        break;
+    default:
+        moe = 0;
+        break;
+    }
+
+    if (moe) {
+        env->cp15.mdscr_el1 = deposit64(env->cp15.mdscr_el1, 2, 4, moe);
+    }
+
     /* TODO: Vectored interrupt controller.  */
     switch (cs->exception_index) {
     case EXCP_UDEF:
-- 
1.9.1

[Qemu-devel] [PATCH 09/10] target-arm: Remove comment about MDSCR_EL1 being dummy implementation

[Peter Maydell]

MDSCR_EL1 has actual functionality now; remove the out of date
comment that claims it is a dummy implementation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index ef1eac9..6ae3c50 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2188,9 +2188,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
       .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
     { .name = "DBGDSAR", .cp = 14, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
-    /* Dummy implementation of monitor debug system control register:
-     * we don't support debug. (The 32-bit alias is DBGDSCRext.)
-     */
+    /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
     { .name = "MDSCR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 2, .opc2 = 2,
       .access = PL1_RW,
-- 
1.9.1

[Qemu-devel] [PATCH 10/10] target-arm: Implement minimal DBGVCR, OSDLR_EL1, MDCCSR_EL0

[Peter Maydell]

Implement debug registers DBGVCR, OSDLR_EL1 and MDCCSR_EL0
(as dummy or limited-functionality). 32 bit Linux kernels will
access these at startup so they are required for breakpoints
and watchpoints to be supported.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 6ae3c50..2945828 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2194,10 +2194,29 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
       .access = PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1),
       .resetvalue = 0 },
+    /* MDCCSR_EL0, aka DBGDSCRint. This is a read-only mirror of MDSCR_EL1.
+     * We don't implement the configurable EL0 access.
+     */
+    { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
+      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
+      .type = ARM_CP_NO_MIGRATE,
+      .access = PL1_R,
+      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1),
+      .resetfn = arm_cp_reset_ignore },
     /* We define a dummy WI OSLAR_EL1, because Linux writes to it. */
     { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
       .access = PL1_W, .type = ARM_CP_NOP },
+    /* Dummy OSDLR_EL1: 32-bit Linux will read this */
+    { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH,
+      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4,
+      .access = PL1_RW, .type = ARM_CP_NOP },
+    /* Dummy DBGVCR: Linux wants to clear this on startup, but we don't
+     * implement vector catch debug events yet.
+     */
+    { .name = "DBGVCR",
+      .cp = 14, .opc1 = 0, .crn = 0, .crm = 7, .opc2 = 0,
+      .access = PL1_RW, .type = ARM_CP_NOP },
     REGINFO_SENTINEL
 };
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 00/10] Implement ARM architectural watchpoints

[Richard Henderson]

On 08/29/2014 04:21 AM, Peter Maydell wrote:
> Peter Maydell (10):
>   exec.c: Relax restrictions on watchpoint length and alignment
>   exec.c: Provide full set of dummy wp remove functions in user-mode
>   exec.c: Record watchpoint fault address and direction
>   cpu-exec: Make debug_excp_handler a QOM CPU method
>   target-arm: Implement setting of watchpoints
>   target-arm: Move extended_addresses_enabled() to internals.h
>   target-arm: Implement handling of fired watchpoints
>   target-arm: Set DBGDSCR.MOE for debug exceptions taken to AArch32
>   target-arm: Remove comment about MDSCR_EL1 being dummy implementation
>   target-arm: Implement minimal DBGVCR, OSDLR_EL1, MDCCSR_EL0

Patches 1-3:
Reviewed-by: Richard Henderson <rth@twiddle.net>

The rest I looked through and only saw one funny bit.
In particular I didn't match up the implementation to the ARM.


r~

Re: [Qemu-devel] [PATCH 00/10] Implement ARM architectural watchpoints

[Peter Maydell]

On 29 August 2014 17:49, Richard Henderson <rth@twiddle.net> wrote:
> On 08/29/2014 04:21 AM, Peter Maydell wrote:
>> Peter Maydell (10):
>>   exec.c: Relax restrictions on watchpoint length and alignment
>>   exec.c: Provide full set of dummy wp remove functions in user-mode
>>   exec.c: Record watchpoint fault address and direction
>>   cpu-exec: Make debug_excp_handler a QOM CPU method
>>   target-arm: Implement setting of watchpoints
>>   target-arm: Move extended_addresses_enabled() to internals.h
>>   target-arm: Implement handling of fired watchpoints
>>   target-arm: Set DBGDSCR.MOE for debug exceptions taken to AArch32
>>   target-arm: Remove comment about MDSCR_EL1 being dummy implementation
>>   target-arm: Implement minimal DBGVCR, OSDLR_EL1, MDCCSR_EL0
>
> Patches 1-3:
> Reviewed-by: Richard Henderson <rth@twiddle.net>
>
> The rest I looked through and only saw one funny bit.
> In particular I didn't match up the implementation to the ARM.

Thanks. In the absence of further review from anybody
else I'm going to fix up the minor bug you spotted in
patch 5 and apply to target-arm.next.

-- PMM