target/aarch64: fix watchpoint management

The early documentation for armv8a report the debug register WFAR
as containing the address of the instruction that triggered the
watchpoint. More recent documentation report the register EDWAR as
containing the data memory address that triggered the watchpoint.

The name of macros CPUV8_DBG_WFAR0 and CPUV8_DBG_WFAR1 is not
correct as they point to the debug register EDWAR, so reading such
register returns directly the data memory address that triggered
the watchpoint. The code incorrectly passes this address value to
the function armv8_dpm_report_wfar(); this function is supposed to
adjust the PC value, decrementing it to remove the effects of the
CPU pipeline. This pipeline offset, that has no meaning on the
value in EDWAR, caused commit 651b861d5d5f ("target/aarch64: Add
watchpoint support") to add back the offset while comparing the
address with the watchpoint enabled.

The upper 32 bits of EDWAR are not valid in aarch32 mode and have
to be ignored.

Rename CPUV8_DBG_WFAR0/1 as CPUV8_DBG_EDWAR0/1.
Remove the function armv8_dpm_report_wfar().
Remove the offset while searching the matching watchpoint.
Ignore the upper 32 bits of EDWAR in aarch32 mode.
Fix a comment and the LOG text.

Change-Id: I7cbdbeb766fa18e31cc72be098ca2bc501877ed1
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Reviewed-on: http://openocd.zylin.com/6205
Tested-by: jenkins
Reviewed-by: Liming Sun <limings@nvidia.com>

diff --git a/src/target/aarch64.c b/src/target/aarch64.c
index e6eb2cafd0e5f2be46cb56f20f21099747bb553f..612924f5a7b76b771313ea6897bd584413af1c97 100644 (file)
--- a/src/target/aarch64.c
+++ b/src/target/aarch64.c
@@ -988,25 +988,26 @@ static int aarch64_debug_entry(struct target *target)
        /* Examine debug reason */
        armv8_dpm_report_dscr(dpm, dscr);
 
-       /* save address of instruction that triggered the watchpoint? */
+       /* save the memory address that triggered the watchpoint */
        if (target->debug_reason == DBG_REASON_WATCHPOINT) {
                uint32_t tmp;
-               uint64_t wfar = 0;
 
                retval = mem_ap_read_atomic_u32(armv8->debug_ap,
-                               armv8->debug_base + CPUV8_DBG_WFAR1,
-                               &tmp);
+                               armv8->debug_base + CPUV8_DBG_EDWAR0, &tmp);
                if (retval != ERROR_OK)
                        return retval;
-               wfar = tmp;
-               wfar = (wfar << 32);
-               retval = mem_ap_read_atomic_u32(armv8->debug_ap,
-                               armv8->debug_base + CPUV8_DBG_WFAR0,
-                               &tmp);
-               if (retval != ERROR_OK)
-                       return retval;
-               wfar |= tmp;
-               armv8_dpm_report_wfar(&armv8->dpm, wfar);
+               target_addr_t edwar = tmp;
+
+               /* EDWAR[63:32] has unknown content in aarch32 state */
+               if (core_state == ARM_STATE_AARCH64) {
+                       retval = mem_ap_read_atomic_u32(armv8->debug_ap,
+                                       armv8->debug_base + CPUV8_DBG_EDWAR1, &tmp);
+                       if (retval != ERROR_OK)
+                               return retval;
+                       edwar |= ((target_addr_t)tmp) << 32;
+               }
+
+               armv8->dpm.wp_addr = edwar;
        }
 
        retval = armv8_dpm_read_current_registers(&armv8->dpm);
@@ -1853,7 +1854,7 @@ int aarch64_hit_watchpoint(struct target *target,
 
        struct armv8_common *armv8 = target_to_armv8(target);
 
-       uint64_t exception_address;
+       target_addr_t exception_address;
        struct watchpoint *wp;
 
        exception_address = armv8->dpm.wp_addr;
@@ -1861,23 +1862,11 @@ int aarch64_hit_watchpoint(struct target *target,
        if (exception_address == 0xFFFFFFFF)
                return ERROR_FAIL;
 
-       /**********************************************************/
-       /* see if a watchpoint address matches a value read from  */
-       /* the EDWAR register. Testing shows that on some ARM CPUs*/
-       /* the EDWAR value needs to have 8 added to it so we add  */
-       /* that check as well not sure if that is a core bug)     */
-       /**********************************************************/
-       for (exception_address = armv8->dpm.wp_addr; exception_address <= (armv8->dpm.wp_addr + 8);
-               exception_address += 8) {
-               for (wp = target->watchpoints; wp; wp = wp->next) {
-                       if ((exception_address >= wp->address) && (exception_address < (wp->address + wp->length))) {
-                               *hit_watchpoint = wp;
-                               if (exception_address != armv8->dpm.wp_addr)
-                                       LOG_DEBUG("watchpoint hit required EDWAR to be increased by 8");
-                               return ERROR_OK;
-                       }
+       for (wp = target->watchpoints; wp; wp = wp->next)
+               if (exception_address >= wp->address && exception_address < (wp->address + wp->length)) {
+                       *hit_watchpoint = wp;
+                       return ERROR_OK;
                }
-       }
 
        return ERROR_FAIL;
 }

diff --git a/src/target/armv8.c b/src/target/armv8.c
index e209e8896a0a5108d36d03b954143453b4376e1a..0c5cf346e566e02724438973f6682ffa583c06b1 100644 (file)
--- a/src/target/armv8.c
+++ b/src/target/armv8.c
@@ -1169,7 +1169,7 @@ int armv8_arch_state(struct target *target)
                armv8_show_fault_registers(target);
 
        if (target->debug_reason == DBG_REASON_WATCHPOINT)
-               LOG_USER("Watchpoint triggered at PC " TARGET_ADDR_FMT, armv8->dpm.wp_addr);
+               LOG_USER("Watchpoint triggered at " TARGET_ADDR_FMT, armv8->dpm.wp_addr);
 
        return ERROR_OK;
 }

diff --git a/src/target/armv8.h b/src/target/armv8.h
index 978b2ad4a1832eb6f024f2528413b5b270c5f165..f09f3abfdbebee0d47a05460eb07513ce0263046 100644 (file)
--- a/src/target/armv8.h
+++ b/src/target/armv8.h
@@ -257,8 +257,8 @@ static inline bool is_armv8(struct armv8_common *armv8)
 
 #define CPUV8_DBG_EDESR                0x20
 #define CPUV8_DBG_EDECR                0x24
-#define CPUV8_DBG_WFAR0                0x30
-#define CPUV8_DBG_WFAR1                0x34
+#define CPUV8_DBG_EDWAR0       0x30
+#define CPUV8_DBG_EDWAR1       0x34
 #define CPUV8_DBG_DSCR         0x088
 #define CPUV8_DBG_DRCR         0x090
 #define CPUV8_DBG_ECCR         0x098

diff --git a/src/target/armv8_dpm.c b/src/target/armv8_dpm.c
index a7b4f1d386c039cd73ec3260b856fe8250c8bdfc..6ef8c33de2d07fa6e34559fbab98cab54d4e92f9 100644 (file)
--- a/src/target/armv8_dpm.c
+++ b/src/target/armv8_dpm.c
@@ -1279,27 +1279,6 @@ static int dpmv8_remove_watchpoint(struct target *target, struct watchpoint *wp)
        return retval;
 }
 
-void armv8_dpm_report_wfar(struct arm_dpm *dpm, uint64_t addr)
-{
-       switch (dpm->arm->core_state) {
-               case ARM_STATE_ARM:
-               case ARM_STATE_AARCH64:
-                       addr -= 8;
-                       break;
-               case ARM_STATE_THUMB:
-               case ARM_STATE_THUMB_EE:
-                       addr -= 4;
-                       break;
-               case ARM_STATE_JAZELLE:
-                       /* ?? */
-                       break;
-               default:
-                       LOG_DEBUG("Unknown core_state");
-                       break;
-       }
-       dpm->wp_addr = addr;
-}
-
 /*
  * Handle exceptions taken in debug state. This happens mostly for memory
  * accesses that violated a MMU policy. Taking an exception while in debug

diff --git a/src/target/armv8_dpm.h b/src/target/armv8_dpm.h
index a6cade345e169d7fc6cb3bece4b9a230cc337d96..c30b04ffa64736666e4ad8dc9cc41cc154f651eb 100644 (file)
--- a/src/target/armv8_dpm.h
+++ b/src/target/armv8_dpm.h
@@ -38,8 +38,6 @@ int armv8_dpm_modeswitch(struct arm_dpm *dpm, enum arm_mode mode);
 
 int armv8_dpm_write_dirty_registers(struct arm_dpm *dpm, bool bpwp);
 
-void armv8_dpm_report_wfar(struct arm_dpm *dpm, uint64_t wfar);
-
 /* DSCR bits; see ARMv7a arch spec section C10.3.1.
  * Not all v7 bits are valid in v6.
  */