[otbn] Report errors properly from ISS to RTL
Most of the plumbing is pretty obvious. The only challenging bit is
getting multiple values back from the C++ model wrapper to the SV code
in otbn_core_model.sv. We were already passing 3 flags as the bottom 3
bits of a uint32_t. Now we pass a 16-bit error code as the top 3 bits.
If we ever need more than that many bits, we might have to do
something cleverer with out parameters, but I thought the quick and
dirty solution was probably best for now.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/otbn/dv/model/iss_wrapper.cc b/hw/ip/otbn/dv/model/iss_wrapper.cc
index b71fe98..910741e 100644
--- a/hw/ip/otbn/dv/model/iss_wrapper.cc
+++ b/hw/ip/otbn/dv/model/iss_wrapper.cc
@@ -189,6 +189,35 @@
return strtoul(buf, nullptr, 16);
}
+// Read through trace output (in the lines argument) to pick up any write to
+// the named CSR register. If there is no such write, return default_val.
+static uint32_t read_ext_reg(const std::string ®_name,
+ const std::vector<std::string> &lines,
+ uint32_t default_val) {
+ // We're interested in lines that show an update to the external register
+ // called reg_name. These look something like this:
+ //
+ // otbn.$REG_NAME &= ~ 0x000001 (from HW) (now 0x000000)
+ //
+ // The \n picks up the newline that we expect at the end of each line.
+ std::regex re("\\s*otbn\\." + reg_name + ".*0x([0-9a-f]{8})\\)\n");
+ std::smatch match;
+
+ uint32_t val = default_val;
+
+ for (const auto &line : lines) {
+ if (std::regex_match(line, match, re)) {
+ // Ahah! We have a match. We have captured exactly 8 hex digits, so know
+ // that we can safely parse them to a uint32_t without risking a parse
+ // failure or overflow.
+ assert(match.size() == 2);
+ val = (uint32_t)strtoul(match[1].str().c_str(), nullptr, 16);
+ }
+ }
+
+ return val;
+}
+
ISSWrapper::ISSWrapper() : tmpdir(new TmpDir()) {
std::string model_path(find_otbn_model());
@@ -293,10 +322,15 @@
run_command(oss.str(), nullptr);
}
-bool ISSWrapper::step() {
+std::pair<bool, uint32_t> ISSWrapper::step() {
std::vector<std::string> lines;
run_command("step\n", &lines);
- return saw_busy_cleared(lines);
+
+ // The busy flag is bit 0 of the STATUS register, so is cleared on this cycle
+ // if we see a write that sets the value to an even number.
+ bool done = (read_ext_reg("STATUS", lines, 1) & 1) == 0;
+ uint32_t err_code = done ? read_ext_reg("ERR_CODE", lines, 0) : 0;
+ return std::make_pair(done, err_code);
}
void ISSWrapper::get_regs(std::array<uint32_t, 32> *gprs,
@@ -471,38 +505,3 @@
fflush(child_write_file);
return read_child_response(dst);
}
-
-bool ISSWrapper::saw_busy_cleared(std::vector<std::string> &lines) const {
- // We're interested in lines that show an update to otbn.STATUS. These look
- // something like this:
- //
- // otbn.STATUS &= ~ 0x000001 (from HW) (now 0x000000)
- //
- // The \n picks up the newline that we expect at the end of each line.
- std::regex re("\\s*otbn\\.STATUS.*0x[0-9]+\\)\n");
-
- bool is_cleared = false;
-
- for (const auto &line : lines) {
- if (std::regex_match(line, re)) {
- // Ahah! We have a match. At this point, we can cheat because we happen
- // to know that the the busy bit is bit zero, so we just need to check
- // whether the last character of the hex constant is even. Since the
- // regex has a fixed number (2) of characters after the hex constant, we
- // can just count back from the end of the string.
- char last_digit = (&line.back())[-2];
-
- int as_num;
- if ('0' <= last_digit && last_digit <= '9') {
- as_num = last_digit - '0';
- } else {
- assert('a' <= last_digit && last_digit <= 'f');
- as_num = 10 + (last_digit - 'a');
- }
-
- is_cleared = !(as_num & 1);
- }
- }
-
- return is_cleared;
-}
diff --git a/hw/ip/otbn/dv/model/iss_wrapper.h b/hw/ip/otbn/dv/model/iss_wrapper.h
index 0e76258..475a599 100644
--- a/hw/ip/otbn/dv/model/iss_wrapper.h
+++ b/hw/ip/otbn/dv/model/iss_wrapper.h
@@ -38,7 +38,7 @@
// Run simulation for a single cycle. Return true if it is now
// finished (ECALL or error).
- bool step();
+ std::pair<bool, uint32_t> step();
// Read contents of the register file
void get_regs(std::array<uint32_t, 32> *gprs, std::array<u256_t, 32> *wdrs);
@@ -61,10 +61,6 @@
// value and dst argument behave as for read_child_response.
bool run_command(const std::string &cmd, std::vector<std::string> *dst) const;
- // Return true if lines contain a line that shows otbn.STATUS having
- // its busy flag cleared.
- bool saw_busy_cleared(std::vector<std::string> &lines) const;
-
pid_t child_pid;
FILE *child_write_file;
FILE *child_read_file;
diff --git a/hw/ip/otbn/dv/model/otbn_core_model.sv b/hw/ip/otbn/dv/model/otbn_core_model.sv
index 8d3f324..fcfafdc 100644
--- a/hw/ip/otbn/dv/model/otbn_core_model.sv
+++ b/hw/ip/otbn/dv/model/otbn_core_model.sv
@@ -41,6 +41,8 @@
input logic start_i, // start the operation
output bit done_o, // operation done
+ output err_code_e err_code_o, // valid when done_o is asserted
+
input logic [ImemAddrWidth-1:0] start_addr_i, // start byte address in IMEM
output bit err_o // something went wrong
@@ -101,11 +103,17 @@
// Extract particular bits of the status value.
//
- // running: The ISS is currently running
- // failed_step: Something went wrong when trying to start or step the ISS.
- // failed_cmp: The consistency check at the end of run failed.
+ // [0] running: The ISS is currently running
+ // [1] failed_step: Something went wrong when trying to start or step the ISS.
+ // [2] failed_cmp: The consistency check at the end of run failed.
+ // [31:16] raw_err_code: The code to expose as the ERR_CODE register
+ //
+
bit failed_cmp, failed_step, running;
+ bit [15:0] raw_err_code;
assign {failed_cmp, failed_step, running} = status[2:0];
+ assign raw_err_code = status[31:16];
+ assign err_code_o = err_code_e'(raw_err_code);
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni || !enable_i) begin
diff --git a/hw/ip/otbn/dv/model/otbn_model.cc b/hw/ip/otbn/dv/model/otbn_model.cc
index 5a8baab..77f62ed 100644
--- a/hw/ip/otbn/dv/model/otbn_model.cc
+++ b/hw/ip/otbn/dv/model/otbn_model.cc
@@ -31,6 +31,7 @@
#define RUNNING_BIT (1U << 0)
#define FAILED_STEP_BIT (1U << 1)
#define FAILED_CMP_BIT (1U << 2)
+#define STATUS_MASK ((1U << 3) - 1)
// The main entry point to the OTBN model, exported from here and used in
// otbn_core_model.sv.
@@ -38,9 +39,10 @@
// This communicates state with otbn_core_model.sv through the status
// parameter, which has the following bits:
//
-// Bit 0: running True if the model is currently running
-// Bit 1: failed_step Something failed when trying to start/step ISS
-// Bit 2: failed_cmp Consistency check at end of run failed
+// Bit 0: running True if the model is currently running
+// Bit 1: failed_step Something failed when trying to start/step ISS
+// Bit 2: failed_cmp Consistency check at end of run failed
+// Bits 31:16: err_code Error code to report on negedge of running
//
// The otbn_model_step function should only be called when either the model is
// running (bit 0 of status) or when start_i is asserted. At other times, it
@@ -227,11 +229,19 @@
}
// Step once in the model. Returns 1 if the model has finished, 0 if not and -1
-// on failure.
-static int step_model(ISSWrapper *model) {
+// on failure. If the model has finished, writes otbn.ERR_CODE to *err_code.
+static int step_model(ISSWrapper *model, uint32_t *err_code) {
assert(model);
+ assert(err_code);
+
try {
- return model->step() ? 1 : 0;
+ std::pair<bool, uint32_t> ret = model->step();
+ if (ret.first) {
+ *err_code = ret.second;
+ return 1;
+ } else {
+ return 0;
+ }
} catch (const std::runtime_error &err) {
std::cerr << "Error when stepping ISS: " << err.what() << "\n";
return -1;
@@ -517,6 +527,9 @@
unsigned start_addr, unsigned status) {
assert(model && imem_scope && dmem_scope && design_scope);
+ // Clear out any ERR_CODE from a previous cycle
+ status &= STATUS_MASK;
+
// Start the model if requested
if (start_i) {
switch (start_model(model, imem_scope, imem_words, dmem_scope, dmem_words,
@@ -537,14 +550,15 @@
return status;
// Step the model once
- switch (step_model(model)) {
+ uint32_t err_code;
+ switch (step_model(model, &err_code)) {
case 0:
// Still running: no change
break;
case 1:
// Finished
- status = status & ~RUNNING_BIT;
+ status = (status & ~RUNNING_BIT) | (err_code << 16);
break;
default:
diff --git a/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py b/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py
index ca43c11..e585b2e 100644
--- a/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py
+++ b/hw/ip/otbn/dv/otbnsim/sim/ext_regs.py
@@ -18,7 +18,7 @@
self.new_value = new_value
def trace(self) -> str:
- return ("otbn.{} {} {:#08x}{} (now {:#08x})"
+ return ("otbn.{} {} {:#010x}{} (now {:#010x})"
.format(self.name,
self.op,
self.written,
diff --git a/hw/ip/otbn/dv/verilator/otbn_top_sim.sv b/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
index fedbbb0..d4b3205 100644
--- a/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
+++ b/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
@@ -18,9 +18,10 @@
localparam int ImemAddrWidth = prim_util_pkg::vbits(ImemSizeByte);
localparam int DmemAddrWidth = prim_util_pkg::vbits(DmemSizeByte);
- logic otbn_done_d, otbn_done_q;
- logic otbn_start;
- logic otbn_start_done;
+ logic otbn_done_d, otbn_done_q;
+ err_code_e otbn_err_code_d, otbn_err_code_q;
+ logic otbn_start;
+ logic otbn_start_done;
// Instruction memory (IMEM) signals
logic imem_req;
@@ -45,31 +46,31 @@
.ImemSizeByte ( ImemSizeByte ),
.DmemSizeByte ( DmemSizeByte )
) u_otbn_core (
- .clk_i ( IO_CLK ),
- .rst_ni ( IO_RST_N ),
+ .clk_i ( IO_CLK ),
+ .rst_ni ( IO_RST_N ),
- .start_i ( otbn_start ),
- .done_o ( otbn_done_d ),
+ .start_i ( otbn_start ),
+ .done_o ( otbn_done_d ),
- .err_code_o ( ),
+ .err_code_o ( otbn_err_code_d ),
- .start_addr_i ( ImemStartAddr ),
+ .start_addr_i ( ImemStartAddr ),
- .imem_req_o ( imem_req ),
- .imem_addr_o ( imem_addr ),
- .imem_wdata_o ( ),
- .imem_rdata_i ( imem_rdata ),
- .imem_rvalid_i ( imem_rvalid ),
- .imem_rerror_i ( imem_rerror ),
+ .imem_req_o ( imem_req ),
+ .imem_addr_o ( imem_addr ),
+ .imem_wdata_o ( ),
+ .imem_rdata_i ( imem_rdata ),
+ .imem_rvalid_i ( imem_rvalid ),
+ .imem_rerror_i ( imem_rerror ),
- .dmem_req_o ( dmem_req ),
- .dmem_write_o ( dmem_write ),
- .dmem_addr_o ( dmem_addr ),
- .dmem_wdata_o ( dmem_wdata ),
- .dmem_wmask_o ( dmem_wmask ),
- .dmem_rdata_i ( dmem_rdata ),
- .dmem_rvalid_i ( dmem_rvalid ),
- .dmem_rerror_i ( dmem_rerror )
+ .dmem_req_o ( dmem_req ),
+ .dmem_write_o ( dmem_write ),
+ .dmem_addr_o ( dmem_addr ),
+ .dmem_wdata_o ( dmem_wdata ),
+ .dmem_wmask_o ( dmem_wmask ),
+ .dmem_rdata_i ( dmem_rdata ),
+ .dmem_rvalid_i ( dmem_rvalid ),
+ .dmem_rerror_i ( dmem_rerror )
);
bind otbn_core otbn_trace_intf #(.ImemAddrWidth, .DmemAddrWidth) i_otbn_trace_intf (.*);
@@ -82,6 +83,7 @@
otbn_start <= 1'b0;
otbn_start_done <= 1'b0;
otbn_done_q <= 1'b0;
+ otbn_err_code_q <= ErrCodeNoError;
end else begin
if (!otbn_start_done) begin
otbn_start <= 1'b1;
@@ -91,6 +93,7 @@
end
otbn_done_q <= otbn_done_d;
+ otbn_err_code_q <= otbn_err_code_d;
end
end
@@ -186,8 +189,9 @@
localparam string DmemScope = "..u_dmem.u_mem.gen_generic.u_impl_generic";
localparam string DesignScope = "..u_otbn_core";
- logic otbn_model_done;
- bit otbn_model_err;
+ logic otbn_model_done;
+ err_code_e otbn_model_err_code;
+ bit otbn_model_err;
otbn_core_model #(
.DmemSizeByte ( DmemSizeByte ),
@@ -206,22 +210,32 @@
.start_addr_i ( ImemStartAddr ),
+ .err_code_o ( otbn_model_err_code ),
+
.err_o ( otbn_model_err )
);
- bit done_mismatch_latched;
+ bit done_mismatch_latched, err_code_mismatch_latched;
bit model_err_latched;
always_ff @(posedge IO_CLK or negedge IO_RST_N) begin
if (!IO_RST_N) begin
- done_mismatch_latched <= 1'b0;
- model_err_latched <= 1'b0;
+ done_mismatch_latched <= 1'b0;
+ err_code_mismatch_latched <= 1'b0;
+ model_err_latched <= 1'b0;
end else begin
if (otbn_done_q != otbn_model_done) begin
$display("ERROR: At time %0t, otbn_done_q != otbn_model_done (%0d != %0d).",
$time, otbn_done_q, otbn_model_done);
done_mismatch_latched <= 1'b1;
end
+ if (otbn_done_q && otbn_model_done) begin
+ if (otbn_err_code_q != otbn_model_err_code) begin
+ $display("ERROR: At time %0t, otbn_err_code != otbn_model_err_code (%0d != %0d).",
+ $time, otbn_err_code_q, otbn_model_err_code);
+ err_code_mismatch_latched <= 1'b1;
+ end
+ end
model_err_latched <= model_err_latched | otbn_model_err;
end
end
@@ -253,7 +267,7 @@
export "DPI-C" function otbn_err_get;
function automatic bit otbn_err_get();
- return model_err_latched | done_mismatch_latched;
+ return model_err_latched | done_mismatch_latched | err_code_mismatch_latched;
endfunction
endmodule
diff --git a/hw/ip/otbn/rtl/otbn.sv b/hw/ip/otbn/rtl/otbn.sv
index de59d21..b1b0a9d 100644
--- a/hw/ip/otbn/rtl/otbn.sv
+++ b/hw/ip/otbn/rtl/otbn.sv
@@ -471,6 +471,8 @@
.start_i (start_model),
.done_o (done_model),
+ .err_code_o (err_code_model),
+
.start_addr_i (start_addr),
.err_o ()
