Cortex-M3: improved core exception handling

This updates three aspects of debugger/exception interactions:

 - Save the user's "vector_catch" setting, and restore it after reset.
   Previously, it was obliterated (rather annoyingly) each time.

 - Don't catch BusFault and HardFault exceptions unless the user says
   to do so.  Target firmware may need to handle them.

 - Don't modify SHCSR to prevent escalating BusFault to HardFault.
   Target firmware may expect to handle it as a HardFault.

Those simplifications fix several bugs.  In one annoying case, OpenOCD
would cause the target to lock up on ome faults which triggered after
the debugger disconnected.

NOTE:  a known remaining issue is that OpenOCD can still leave DEMCR
set after an otherwise-clean OpenOCD shutdown.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

3 files changed, 33 insertions, 23 deletions

diff --git a/NEWS b/NEWS
index b1696065..a8b2b44b 100644
--- a/NEWS
+++ b/NEWS
@@ -34,6 +34,8 @@ Target Layer:
 		- watchpoint support
 	Cortex-M3
 		- Exposed DWT registers like cycle counter
+		- vector_catch settings not clobbered by resets
+		- no longer interferes with firmware's fault handling
 	ETM, ETB
 		- "trigger_percent" command moved ETM --> ETB
 		- "etm trigger_debug" command added
diff --git a/src/target/armv7m.h b/src/target/armv7m.h
index ac559b9e..86caae21 100644
--- a/src/target/armv7m.h
+++ b/src/target/armv7m.h
@@ -106,9 +106,14 @@ struct armv7m_common
 	int exception_number;
 	struct swjdp_common swjdp_info;
 
+	uint32_t demcr;
+
 	/* Direct processor core register read and writes */
-	int (*load_core_reg_u32)(struct target *target, enum armv7m_regtype type, uint32_t num, uint32_t *value);
-	int (*store_core_reg_u32)(struct target *target, enum armv7m_regtype type, uint32_t num, uint32_t value);
+	int (*load_core_reg_u32)(struct target *target,
+		enum armv7m_regtype type, uint32_t num, uint32_t *value);
+	int (*store_core_reg_u32)(struct target *target,
+		enum armv7m_regtype type, uint32_t num, uint32_t value);
+
 	/* register cache to processor synchronization */
 	int (*read_core_reg)(struct target *target, unsigned num);
 	int (*write_core_reg)(struct target *target, unsigned num);
diff --git a/src/target/cortex_m3.c b/src/target/cortex_m3.c
index c6b1bb2d..48f81148 100644
--- a/src/target/cortex_m3.c
+++ b/src/target/cortex_m3.c
@@ -185,11 +185,12 @@ static int cortex_m3_endreset_event(struct target *target)
 	int i;
 	uint32_t dcb_demcr;
 	struct cortex_m3_common *cortex_m3 = target_to_cm3(target);
+	struct armv7m_common *armv7m = &cortex_m3->armv7m;
 	struct swjdp_common *swjdp = &cortex_m3->armv7m.swjdp_info;
 	struct cortex_m3_fp_comparator *fp_list = cortex_m3->fp_comparator_list;
 	struct cortex_m3_dwt_comparator *dwt_list = cortex_m3->dwt_comparator_list;
 
-	/* FIXME handling of DEMCR clobbers vector_catch config ... */
+	/* REVISIT The four debug monitor bits are currently ignored... */
 	mem_ap_read_atomic_u32(swjdp, DCB_DEMCR, &dcb_demcr);
 	LOG_DEBUG("DCB_DEMCR = 0x%8.8" PRIx32 "",dcb_demcr);
 
@@ -204,21 +205,14 @@ static int cortex_m3_endreset_event(struct target *target)
 	/* clear any interrupt masking */
 	cortex_m3_write_debug_halt_mask(target, 0, C_MASKINTS);
 
-	/* Enable trace and DWT; trap hard and bus faults.
+	/* Enable features controlled by ITM and DWT blocks, and catch only
+	 * the vectors we were told to pay attention to.
 	 *
-	 * REVISIT why trap those two?  And why trash the vector_catch
-	 * config, instead of preserving it?  Catching HARDERR and BUSERR
-	 * will interfere with code that handles those itself...
+	 * Target firmware is responsible for all fault handling policy
+	 * choices *EXCEPT* explicitly scripted overrides like "vector_catch"
+	 * or manual updates to the NVIC SHCSR and CCR registers.
 	 */
-	mem_ap_write_u32(swjdp, DCB_DEMCR, TRCENA | VC_HARDERR | VC_BUSERR);
-
-	/* Monitor bus faults as such (instead of as generic HARDERR), but
-	 * leave memory management and usage faults disabled.
-	 *
-	 * REVISIT setting BUSFAULTENA interferes with code which relies
-	 * on the default setting.  Why do it?
-	 */
-	mem_ap_write_u32(swjdp, NVIC_SHCSR, SHCSR_BUSFAULTENA);
+	mem_ap_write_u32(swjdp, DCB_DEMCR, TRCENA | armv7m->demcr);
 
 	/* Paranoia: evidently some (early?) chips don't preserve all the
 	 * debug state (including FBP, DWT, etc) across reset...
@@ -533,7 +527,7 @@ static int cortex_m3_soft_reset_halt(struct target *target)
 	uint32_t dcb_dhcsr = 0;
 	int retval, timeout = 0;
 
-	/* Enter debug state on reset; see end_reset_event() */
+	/* Enter debug state on reset; restore DEMCR in endreset_event() */
 	mem_ap_write_u32(swjdp, DCB_DEMCR,
 			TRCENA | VC_HARDERR | VC_BUSERR | VC_CORERESET);
 
@@ -782,14 +776,15 @@ static int cortex_m3_assert_reset(struct target *target)
 
 		/* clear C_HALT in dhcsr reg */
 		cortex_m3_write_debug_halt_mask(target, 0, C_HALT);
-
-		/* Enter debug state on reset, cf. end_reset_event() */
-		mem_ap_write_u32(swjdp, DCB_DEMCR,
-				TRCENA | VC_HARDERR | VC_BUSERR);
 	}
 	else
 	{
-		/* Enter debug state on reset, cf. end_reset_event() */
+		/* Halt in debug on reset; endreset_event() restores DEMCR.
+		 *
+		 * REVISIT catching BUSERR presumably helps to defend against
+		 * bad vector table entries.  Should this include MMERR or
+		 * other flags too?
+		 */
 		mem_ap_write_atomic_u32(swjdp, DCB_DEMCR,
 				TRCENA | VC_HARDERR | VC_BUSERR | VC_CORERESET);
 	}
@@ -1938,12 +1933,20 @@ COMMAND_HANDLER(handle_cortex_m3_vector_catch_command)
 			}
 		}
 write:
+		/* For now, armv7m->demcr only stores vector catch flags. */
+		armv7m->demcr = catch;
+
 		demcr &= ~0xffff;
 		demcr |= catch;
 
-		/* write, but don't assume it stuck */
+		/* write, but don't assume it stuck (why not??) */
 		mem_ap_write_u32(swjdp, DCB_DEMCR, demcr);
 		mem_ap_read_atomic_u32(swjdp, DCB_DEMCR, &demcr);
+
+		/* FIXME be sure to clear DEMCR on clean server shutdown.
+		 * Otherwise the vector catch hardware could fire when there's
+		 * no debugger hooked up, causing much confusion...
+		 */
 	}
 
 	for (unsigned i = 0; i < ARRAY_SIZE(vec_ids); i++)