Pay attention to impebreak.

This required updating debug_defines.h, which caused a few other small
cleanups as well.

Change-Id: I3c2cb418d7eff3093d7664c5563b2af5e8b530eb

4 files changed, 95 insertions, 64 deletions

diff --git a/src/target/riscv/debug_defines.h b/src/target/riscv/debug_defines.h
index 6e88a55..6067dbe 100644
--- a/src/target/riscv/debug_defines.h
+++ b/src/target/riscv/debug_defines.h
@@ -173,12 +173,6 @@
 #define CSR_DCSR_EBREAKM_LENGTH             1
 #define CSR_DCSR_EBREAKM                    (0x1U << CSR_DCSR_EBREAKM_OFFSET)
 /*
-* When 1, {\tt ebreak} instructions in Hypervisor Mode enter Debug Mode.
- */
-#define CSR_DCSR_EBREAKH_OFFSET             14
-#define CSR_DCSR_EBREAKH_LENGTH             1
-#define CSR_DCSR_EBREAKH                    (0x1U << CSR_DCSR_EBREAKH_OFFSET)
-/*
 * When 1, {\tt ebreak} instructions in Supervisor Mode enter Debug Mode.
  */
 #define CSR_DCSR_EBREAKS_OFFSET             13
@@ -207,9 +201,10 @@
 /*
 * 0: Increment counters as usual.
 *
-* 1: Don't increment any counters while in Debug Mode.  This includes
-* the {\tt cycle} and {\tt instret} CSRs. This is preferred for most
-* debugging scenarios.
+* 1: Don't increment any counters while in Debug Mode or on {\tt
+* ebreak} instructions that cause entry into Debug Mode.  These
+* counters include the {\tt cycle} and {\tt instret} CSRs. This is
+* preferred for most debugging scenarios.
 *
 * An implementation may choose not to support writing to this bit.
 * The debugger must read back the value it writes to check whether
@@ -312,9 +307,9 @@
 *
 * This bit is only writable from Debug Mode.
  */
-#define CSR_TDATA1_HMODE_OFFSET             XLEN-5
-#define CSR_TDATA1_HMODE_LENGTH             1
-#define CSR_TDATA1_HMODE                    (0x1ULL << CSR_TDATA1_HMODE_OFFSET)
+#define CSR_TDATA1_DMODE_OFFSET             XLEN-5
+#define CSR_TDATA1_DMODE_LENGTH             1
+#define CSR_TDATA1_DMODE                    (0x1ULL << CSR_TDATA1_DMODE_OFFSET)
 /*
 * Trigger-specific data.
  */
@@ -390,7 +385,7 @@
 * 0: Raise a breakpoint exception. (Used when software wants to use
 * the trigger module without an external debugger attached.)
 *
-* 1: Enter Debug Mode. (Only supported when \Fhmode is 1.)
+* 1: Enter Debug Mode. (Only supported when \Fdmode is 1.)
 *
 * 2: Start tracing.
 *
@@ -532,7 +527,7 @@
 * 0: Raise a breakpoint exception. (Used when software wants to use the
 * trigger module without an external debugger attached.)
 *
-* 1: Enter Debug Mode. (Only supported when \Fhmode is 1.)
+* 1: Enter Debug Mode. (Only supported when \Fdmode is 1.)
 *
 * 2: Start tracing.
 *
@@ -549,6 +544,30 @@
 #define CSR_ICOUNT_ACTION                   (0x3fULL << CSR_ICOUNT_ACTION_OFFSET)
 #define DMI_DMSTATUS                        0x11
 /*
+* If 1, then there is an implicit {\tt ebreak} instruction at the
+* non-existent word immediately after the Program Buffer. This saves
+* the debugger from having to write the {\tt ebreak} itself, and
+* allows the Program Buffer to be one word smaller.
+*
+* This must be 1 when \Fprogbufsize is 1.
+ */
+#define DMI_DMSTATUS_IMPEBREAK_OFFSET       22
+#define DMI_DMSTATUS_IMPEBREAK_LENGTH       1
+#define DMI_DMSTATUS_IMPEBREAK              (0x1U << DMI_DMSTATUS_IMPEBREAK_OFFSET)
+/*
+* Gets set if the Debug Module was accessed incorrectly.
+*
+* 0 (none): No error.
+*
+* 1 (badaddr): There was an access to an unimplemented Debug Module
+* address.
+*
+* 7 (other): An access failed for another reason.
+ */
+#define DMI_DMSTATUS_DMERR_OFFSET           18
+#define DMI_DMSTATUS_DMERR_LENGTH           3
+#define DMI_DMSTATUS_DMERR                  (0x7U << DMI_DMSTATUS_DMERR_OFFSET)
+/*
 * This field is 1 when all currently selected harts have acknowledged the previous \Fresumereq.
  */
 #define DMI_DMSTATUS_ALLRESUMEACK_OFFSET    17
@@ -629,6 +648,13 @@
 #define DMI_DMSTATUS_AUTHBUSY_OFFSET        6
 #define DMI_DMSTATUS_AUTHBUSY_LENGTH        1
 #define DMI_DMSTATUS_AUTHBUSY               (0x1U << DMI_DMSTATUS_AUTHBUSY_OFFSET)
+/*
+* 0: \Rdevtreeaddrzero--\Rdevtreeaddrthree hold information which
+* is not relevant to the Device Tree.
+*
+* 1: \Rdevtreeaddrzero--\Rdevtreeaddrthree registers hold the address of the
+* Device Tree.
+ */
 #define DMI_DMSTATUS_DEVTREEVALID_OFFSET    4
 #define DMI_DMSTATUS_DEVTREEVALID_LENGTH    1
 #define DMI_DMSTATUS_DEVTREEVALID           (0x1U << DMI_DMSTATUS_DEVTREEVALID_OFFSET)
@@ -654,7 +680,6 @@
 *
 * Writing 1 or 0 has no effect on a hart which is already halted, but
 * the bit should be cleared to 0 before the hart is resumed.
-* Setting both \Fhaltreq and \Fresumereq leads to undefined behavior.
 *
 * Writes apply to the new value of \Fhartsel and \Fhasel.
  */
@@ -664,7 +689,8 @@
 /*
 * Resume request signal for all currently selected harts. When set to 1,
 * each selected hart will resume if it is currently halted.
-* Setting both \Fhaltreq and \Fresumereq leads to undefined behavior.
+*
+* This bit is ignored while \Fhaltreq is set.
 *
 * Writes apply to the new value of \Fhartsel and \Fhasel.
  */
@@ -710,11 +736,12 @@
 #define DMI_DMCONTROL_HARTSEL               (0x3ffU << DMI_DMCONTROL_HARTSEL_OFFSET)
 /*
 * This bit controls the reset signal from the DM to the rest of the
-* system. To perform a system reset the debugger writes 1,
+* system. The signal should reset every part of the system, including
+* every hart, except for the DM and any logic required to access the
+* DM.
+* To perform a system reset the debugger writes 1,
 * and then writes 0
-* to deassert the reset. This bit must not reset the Debug Module
-* registers. What it does reset is platform-specific (it may
-* reset nothing).
+* to deassert the reset.
  */
 #define DMI_DMCONTROL_NDMRESET_OFFSET       1
 #define DMI_DMCONTROL_NDMRESET_LENGTH       1
@@ -778,7 +805,7 @@
 * shadowing the {\tt data} registers.
 *
 * If \Fdataaccess is 1: Signed address of RAM where the {\tt data}
-* registers are shadowed.
+* registers are shadowed, to be used to access relative to \Rzero.
  */
 #define DMI_HARTINFO_DATAADDR_OFFSET        0
 #define DMI_HARTINFO_DATAADDR_LENGTH        12
@@ -891,13 +918,10 @@
 #define DMI_ABSTRACTCS                      0x16
 /*
 * Size of the Program Buffer, in 32-bit words. Valid sizes are 0 - 16.
-*
-* TODO: Explain what can be done with each size of the buffer, to suggest
-* why you would want more or less words.
  */
-#define DMI_ABSTRACTCS_PROGSIZE_OFFSET      24
-#define DMI_ABSTRACTCS_PROGSIZE_LENGTH      5
-#define DMI_ABSTRACTCS_PROGSIZE             (0x1fU << DMI_ABSTRACTCS_PROGSIZE_OFFSET)
+#define DMI_ABSTRACTCS_PROGBUFSIZE_OFFSET   24
+#define DMI_ABSTRACTCS_PROGBUFSIZE_LENGTH   5
+#define DMI_ABSTRACTCS_PROGBUFSIZE          (0x1fU << DMI_ABSTRACTCS_PROGBUFSIZE_OFFSET)
 /*
 * 1: An abstract command is currently being executed.
 *
@@ -1013,24 +1037,22 @@
 *
 * 4: 128-bit
 *
-* If an unsupported system bus access size is written here,
-* the DM may not perform the access, or may perform the access
-* with any access size.
+* If an unsupported system bus access size is written here, the DM
+* does not perform the access and sberror is set to 3.
  */
 #define DMI_SBCS_SBACCESS_OFFSET            17
 #define DMI_SBCS_SBACCESS_LENGTH            3
 #define DMI_SBCS_SBACCESS                   (0x7U << DMI_SBCS_SBACCESS_OFFSET)
 /*
-* When 1, the internal address value (used by the system bus master)
-* is incremented by the access size (in bytes) selected in \Fsbaccess
-* after every system bus access.
+* When 1, {\tt sbaddress} is incremented by the access size (in
+* bytes) selected in \Fsbaccess after every system bus access.
  */
 #define DMI_SBCS_SBAUTOINCREMENT_OFFSET     16
 #define DMI_SBCS_SBAUTOINCREMENT_LENGTH     1
 #define DMI_SBCS_SBAUTOINCREMENT            (0x1U << DMI_SBCS_SBAUTOINCREMENT_OFFSET)
 /*
-* When 1, every read from \Rsbdatazero automatically triggers a system
-* bus read at the new address.
+* When 1, every read from \Rsbdatazero automatically triggers a
+* system bus read at the (possibly auto-incremented) address.
  */
 #define DMI_SBCS_SBAUTOREAD_OFFSET          15
 #define DMI_SBCS_SBAUTOREAD_LENGTH          1
@@ -1052,8 +1074,7 @@
 *
 * 4: The system bus master was busy when one of the
 * {\tt sbaddress} or {\tt sbdata} registers was written,
-* or the {\tt sbdata} register was read when it had
-* stale data.
+* or \Rsbdatazero was read when it had stale data.
  */
 #define DMI_SBCS_SBERROR_OFFSET             12
 #define DMI_SBCS_SBERROR_LENGTH             3
@@ -1097,54 +1118,54 @@
 #define DMI_SBCS_SBACCESS8                  (0x1U << DMI_SBCS_SBACCESS8_OFFSET)
 #define DMI_SBADDRESS0                      0x39
 /*
-* Accesses bits 31:0 of the internal address.
+* Accesses bits 31:0 of the physical address in {\tt sbaddress}.
  */
 #define DMI_SBADDRESS0_ADDRESS_OFFSET       0
 #define DMI_SBADDRESS0_ADDRESS_LENGTH       32
 #define DMI_SBADDRESS0_ADDRESS              (0xffffffffU << DMI_SBADDRESS0_ADDRESS_OFFSET)
 #define DMI_SBADDRESS1                      0x3a
 /*
-* Accesses bits 63:32 of the internal address (if the system address
-* bus is that wide).
+* Accesses bits 63:32 of the physical address in {\tt sbaddress} (if
+* the system address bus is that wide).
  */
 #define DMI_SBADDRESS1_ADDRESS_OFFSET       0
 #define DMI_SBADDRESS1_ADDRESS_LENGTH       32
 #define DMI_SBADDRESS1_ADDRESS              (0xffffffffU << DMI_SBADDRESS1_ADDRESS_OFFSET)
 #define DMI_SBADDRESS2                      0x3b
 /*
-* Accesses bits 95:64 of the internal address (if the system address
-* bus is that wide).
+* Accesses bits 95:64 of the physical address in {\tt sbaddress} (if
+* the system address bus is that wide).
  */
 #define DMI_SBADDRESS2_ADDRESS_OFFSET       0
 #define DMI_SBADDRESS2_ADDRESS_LENGTH       32
 #define DMI_SBADDRESS2_ADDRESS              (0xffffffffU << DMI_SBADDRESS2_ADDRESS_OFFSET)
 #define DMI_SBDATA0                         0x3c
 /*
-* Accesses bits 31:0 of the internal data.
+* Accesses bits 31:0 of {\tt sbdata}.
  */
 #define DMI_SBDATA0_DATA_OFFSET             0
 #define DMI_SBDATA0_DATA_LENGTH             32
 #define DMI_SBDATA0_DATA                    (0xffffffffU << DMI_SBDATA0_DATA_OFFSET)
 #define DMI_SBDATA1                         0x3d
 /*
-* Accesses bits 63:32 of the internal data (if the system bus is
-* that wide).
+* Accesses bits 63:32 of {\tt sbdata} (if the system bus is that
+* wide).
  */
 #define DMI_SBDATA1_DATA_OFFSET             0
 #define DMI_SBDATA1_DATA_LENGTH             32
 #define DMI_SBDATA1_DATA                    (0xffffffffU << DMI_SBDATA1_DATA_OFFSET)
 #define DMI_SBDATA2                         0x3e
 /*
-* Accesses bits 95:64 of the internal data (if the system bus is
-* that wide).
+* Accesses bits 95:64 of {\tt sbdata} (if the system bus is that
+* wide).
  */
 #define DMI_SBDATA2_DATA_OFFSET             0
 #define DMI_SBDATA2_DATA_LENGTH             32
 #define DMI_SBDATA2_DATA                    (0xffffffffU << DMI_SBDATA2_DATA_OFFSET)
 #define DMI_SBDATA3                         0x3f
 /*
-* Accesses bits 127:96 of the internal data (if the system bus is
-* that wide).
+* Accesses bits 127:96 of {\tt sbdata} (if the system bus is that
+* wide).
  */
 #define DMI_SBDATA3_DATA_OFFSET             0
 #define DMI_SBDATA3_DATA_LENGTH             32
@@ -1188,6 +1209,9 @@
 * 0: Don't do the operation specified by \Fwrite.
 *
 * 1: Do the operation specified by \Fwrite.
+*
+* This bit can be used to just execute the Program Buffer without
+* having to worry about placing valid values into \Fsize or \Fregno.
  */
 #define AC_ACCESS_REGISTER_TRANSFER_OFFSET  17
 #define AC_ACCESS_REGISTER_TRANSFER_LENGTH  1
@@ -1223,8 +1247,9 @@
 /*
 * Contains the privilege level the hart was operating in when Debug
 * Mode was entered. The encoding is described in Table
-* \ref{tab:privlevel}. A user can write this value to change the
-* hart's privilege level when exiting Debug Mode.
+* \ref{tab:privlevel}, and matches the privilege level encoding from
+* the RISC-V Privileged ISA Specification. A user can write this
+* value to change the hart's privilege level when exiting Debug Mode.
  */
 #define VIRT_PRIV_PRV_OFFSET                0
 #define VIRT_PRIV_PRV_LENGTH                2
diff --git a/src/target/riscv/program.c b/src/target/riscv/program.c
index b4f2b55..f02e4ea 100644
--- a/src/target/riscv/program.c
+++ b/src/target/riscv/program.c
@@ -279,9 +279,10 @@ int riscv_program_fence(struct riscv_program *p)
 
 int riscv_program_ebreak(struct riscv_program *p)
 {
-	if (p->instruction_count == riscv_debug_buffer_size(p->target)) {
-		// TODO: Check for impebreak bit.
-		// There's an implicit ebreak here, so no need for us to add one.
+	struct target *target = p->target;
+	RISCV_INFO(r);
+	if (p->instruction_count == riscv_debug_buffer_size(p->target) &&
+			r->impebreak) {
 		return ERROR_OK;
 	}
 	return riscv_program_insert(p, ebreak());
diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c
index 5495516..b05110c 100644
--- a/src/target/riscv/riscv-013.c
+++ b/src/target/riscv/riscv-013.c
@@ -137,7 +137,7 @@ typedef struct {
 	/* Number of abstract command data registers. */
 	unsigned datacount;
 	/* Number of words in the Program Buffer. */
-	unsigned progsize;
+	unsigned progbufsize;
 	/* Number of Program Buffer registers. */
 	/* Number of words in Debug RAM. */
 	uint64_t tselect;
@@ -200,6 +200,7 @@ static void decode_dmi(char *text, unsigned address, unsigned data)
 		{ DMI_DMCONTROL, DMI_DMCONTROL_NDMRESET, "ndmreset" },
 		{ DMI_DMCONTROL, DMI_DMCONTROL_DMACTIVE, "dmactive" },
 
+		{ DMI_DMSTATUS, DMI_DMSTATUS_IMPEBREAK, "impebreak" },
 		{ DMI_DMSTATUS, DMI_DMSTATUS_ALLRESUMEACK, "allresumeack" },
 		{ DMI_DMSTATUS, DMI_DMSTATUS_ANYRESUMEACK, "anyresumeack" },
 		{ DMI_DMSTATUS, DMI_DMSTATUS_ALLNONEXISTENT, "allnonexistent" },
@@ -215,7 +216,7 @@ static void decode_dmi(char *text, unsigned address, unsigned data)
 		{ DMI_DMSTATUS, DMI_DMSTATUS_DEVTREEVALID, "devtreevalid" },
 		{ DMI_DMSTATUS, DMI_DMSTATUS_VERSION, "version" },
 
-		{ DMI_ABSTRACTCS, DMI_ABSTRACTCS_PROGSIZE, "progsize" },
+		{ DMI_ABSTRACTCS, DMI_ABSTRACTCS_PROGBUFSIZE, "progbufsize" },
 		{ DMI_ABSTRACTCS, DMI_ABSTRACTCS_BUSY, "busy" },
 		{ DMI_ABSTRACTCS, DMI_ABSTRACTCS_CMDERR, "cmderr" },
 		{ DMI_ABSTRACTCS, DMI_ABSTRACTCS_DATACOUNT, "datacount" },
@@ -910,7 +911,7 @@ static int init_target(struct command_context *cmd_ctx,
 		return ERROR_FAIL;
 	riscv013_info_t *info = get_info(target);
 
-	info->progsize = -1;
+	info->progbufsize = -1;
 	info->progbuf_addr = -1;
 	info->data_size = -1;
 	info->data_addr = -1;
@@ -1043,10 +1044,12 @@ static int examine(struct target *target)
 	// Check that abstract data registers are accessible.
 	uint32_t abstractcs = dmi_read(target, DMI_ABSTRACTCS);
 	info->datacount = get_field(abstractcs, DMI_ABSTRACTCS_DATACOUNT);
-	info->progsize = get_field(abstractcs, DMI_ABSTRACTCS_PROGSIZE);
+	info->progbufsize = get_field(abstractcs, DMI_ABSTRACTCS_PROGBUFSIZE);
 
 	/* Before doing anything else we must first enumerate the harts. */
 	RISCV_INFO(r);
+	r->impebreak = get_field(dmstatus, DMI_DMSTATUS_IMPEBREAK);
+
 	int original_coreid = target->coreid;
 	for (int i = 0; i < RISCV_MAX_HARTS; ++i) {
 		/* Fake being a non-RTOS targeted to this core so we can see if
@@ -1080,7 +1083,7 @@ static int examine(struct target *target)
 
 		/* Without knowing anything else we can at least mess with the
 		 * program buffer. */
-		r->debug_buffer_size[i] = info->progsize;
+		r->debug_buffer_size[i] = info->progbufsize;
 
 		int result = register_read_abstract(target, NULL, GDB_REGNO_S0, 64);
 		if (result == ERROR_OK) {
@@ -1768,16 +1771,16 @@ static enum riscv_halt_reason riscv013_halt_reason(struct target *target)
 void riscv013_write_debug_buffer(struct target *target, unsigned index, riscv_insn_t data)
 {
 	RISCV013_INFO(info);
-	if (index >= info->progsize)
-		return dmi_write(target, DMI_DATA0 + index - info->progsize, data);
+	if (index >= info->progbufsize)
+		return dmi_write(target, DMI_DATA0 + index - info->progbufsize, data);
 	return dmi_write(target, DMI_PROGBUF0 + index, data);
 }
 
 riscv_insn_t riscv013_read_debug_buffer(struct target *target, unsigned index)
 {
 	RISCV013_INFO(info);
-	if (index >= info->progsize)
-		return dmi_read(target, DMI_DATA0 + index - info->progsize);
+	if (index >= info->progbufsize)
+		return dmi_read(target, DMI_DATA0 + index - info->progbufsize);
 	return dmi_read(target, DMI_PROGBUF0 + index);
 }
 
@@ -1835,7 +1838,6 @@ static void riscv013_on_step_or_resume(struct target *target, bool step)
 	uint64_t dcsr = riscv_get_register(target, GDB_REGNO_DCSR);
 	dcsr = set_field(dcsr, CSR_DCSR_STEP, step);
 	dcsr = set_field(dcsr, CSR_DCSR_EBREAKM, 1);
-	dcsr = set_field(dcsr, CSR_DCSR_EBREAKH, 1);
 	dcsr = set_field(dcsr, CSR_DCSR_EBREAKS, 1);
 	dcsr = set_field(dcsr, CSR_DCSR_EBREAKU, 1);
 	riscv_set_register(target, GDB_REGNO_DCSR, dcsr);
diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h
index e7b0ea2..f19f06c 100644
--- a/src/target/riscv/riscv.h
+++ b/src/target/riscv/riscv.h
@@ -78,6 +78,9 @@ typedef struct {
 	/* This avoids invalidating the register cache too often. */
 	bool registers_initialized;
 
+	/* This hart contains an implicit ebreak at the end of the program buffer. */
+	bool impebreak;
+
 	/* Helper functions that target the various RISC-V debug spec
 	 * implementations. */
 	riscv_reg_t (*get_register)(struct target *, int hartid, int regid);