[otbn] Add proper error tracking to otbn_core_model
This turned out to be a bit painful: if you want to update more than
one SystemVerilog variable, you need to put the logic in a task, but
then verilator notices that you're being called from an always_ff
block and tells you to use non-blocking assignments (not
unreasonably). It also complains if you mix blocking and non-blocking
assignments. If you still need to inspect the return code from DPI
functions, you might try to change this pattern:
retcode = call_dpi(arg1, arg2);
unique case (retcode)
...
endcase
to this one:
unique case (call_dpi(arg1, arg2))
...
endcase
but that doesn't work because call_dpi(..) gets evaluated (with
side-effects) for each branch of the case statement until a match is
found.
So I gave up, packing the status bits that I cared about into an
integer and just doing the stateful updates in the C++ code.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/otbn/dv/model/otbn_core_model.sv b/hw/ip/otbn/dv/model/otbn_core_model.sv
index 3f36fa7..a62a829 100644
--- a/hw/ip/otbn/dv/model/otbn_core_model.sv
+++ b/hw/ip/otbn/dv/model/otbn_core_model.sv
@@ -35,30 +35,32 @@
input logic start_i, // start the operation
output logic done_o, // operation done
- input logic [ImemAddrWidth-1:0] start_addr_i // start byte address in IMEM
-);
+ input logic [ImemAddrWidth-1:0] start_addr_i, // start byte address in IMEM
+ output bit err_o // something went wrong
+);
import "DPI-C" function chandle otbn_model_init();
import "DPI-C" function void otbn_model_destroy(chandle handle);
import "DPI-C" context function
- int otbn_model_start(chandle model,
- string imem_scope,
- int imem_size,
- string dmem_scope,
- int dmem_size,
- int start_addr);
- import "DPI-C" function int otbn_model_step(chandle model);
- import "DPI-C" context function
- int otbn_model_load_dmem(chandle model,
- string dmem_scope,
- int dmem_size);
- import "DPI-C" context function
- int otbn_model_check(chandle model,
- string dmem_scope,
- int dmem_size,
- string design_scope);
+ int unsigned otbn_model_step(chandle model,
+ string imem_scope,
+ int unsigned imem_size,
+ string dmem_scope,
+ int unsigned dmem_size,
+ string design_scope,
+ logic start_i,
+ int unsigned start_addr,
+ int unsigned status);
+ localparam ImemSizeWords = ImemSizeByte / 4;
+ localparam DmemSizeWords = DmemSizeByte / (WLEN / 8);
+
+ `ASSERT_INIT(StartAddr32_A, ImemAddrWidth <= 32);
+ logic [31:0] start_addr_32;
+ assign start_addr_32 = {{32 - ImemAddrWidth{1'b0}}, start_addr_i};
+
+ // Create and destroy an object through which we can talk to the ISS
chandle model_handle;
initial begin
model_handle = otbn_model_init();
@@ -69,73 +71,47 @@
model_handle = 0;
end
+ // A packed "status" value. This gets assigned by DPI function calls that need to update both
+ // whether we're running and also error flags at the same time. The contents are magic simulation
+ // values, so get initialized before reset (to avoid stopping the simulation before it even
+ // starts).
+ int unsigned status = 0;
- localparam ImemSizeWords = ImemSizeByte / 4;
- localparam DmemSizeWords = DmemSizeByte / (WLEN / 8);
+ // 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.
+ bit failed_cmp, failed_step, running;
+ assign {failed_cmp, failed_step, running} = status[2:0];
- `ASSERT_INIT(StartAddr32_A, ImemAddrWidth <= 32);
- logic [31:0] start_addr_32;
- assign start_addr_32 = {{32 - ImemAddrWidth{1'b0}}, start_addr_i};
-
- // The control loop for the model. We track whether we're currently running in the running
- // variable. The step_iss function is run every cycle when not in reset. It steps the ISS if
- // necessary and returns the new value for running.
- function automatic bit step_iss(bit running);
- int retcode;
- bit new_run = running;
-
- // If start_i is asserted, start again (regardless of whether we're currently running).
- if (start_i) begin
- retcode = otbn_model_start(model_handle,
- ImemScope, ImemSizeWords,
- DmemScope, DmemSizeWords,
- start_addr_32);
- unique case (retcode)
- 0: new_run = 1'b1;
- // Something went wrong. Assume we didn't manage to start.
- default: new_run = 1'b0;
- endcase
- end
-
- // Otherwise, if we aren't currently running then there's nothing more to do.
- if (!new_run) begin
- return 1'b0;
- end
-
- // We are running. Step by one instruction.
- retcode = otbn_model_step(model_handle);
- unique case (retcode)
- 0: new_run = 1'b1;
- 1: begin
- // The model has just finished running. If this is a standalone model (which we can tell
- // because DesignScope is empty), write the ISS's DMEM contents back to the simulation.
- // Otherwise, check the ISS and simulation agree (TODO: We don't do error handling properly
- // at the moment; for now, the C++ code just prints error messages to stderr).
- if (DesignScope.len() == 0) begin
- void'(otbn_model_load_dmem(model_handle, DmemScope, DmemSizeWords));
- end else begin
- void'(otbn_model_check(model_handle, DmemScope, DmemSizeWords, DesignScope));
- end
- new_run = 1'b0;
- end
- // Something went wrong. Assume we've stopped.
- default: new_run = 1'b0;
- endcase
- return new_run;
- endfunction
-
- bit running, running_r;
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
- running <= 1'b0;
- running_r <= 1'b0;
+ // Clear status (stop running, and forget any errors)
+ status <= 0;
end else begin
- running <= step_iss(running);
- running_r <= running;
+ if (start_i | running) begin
+ status <= otbn_model_step(model_handle,
+ ImemScope, ImemSizeWords,
+ DmemScope, DmemSizeWords,
+ DesignScope,
+ start_i, start_addr_32,
+ status);
+ end else begin
+ // If we're not running and we're not being told to start, there's nothing to do.
+ end
end
end
- // Track negedges of running and expose that as a "done" output.
+ // Track negedges of running_q and expose that as a "done" output.
+ bit running_r = 1'b0;
+ always_ff @(posedge clk_i or negedge rst_ni) begin
+ if (!rst_ni) begin
+ running_r <= 1'b0;
+ end else begin
+ running_r <= running;
+ end
+ end
assign done_o = running_r & ~running;
// If DesignScope is not empty, we have a design to check. Bind a copy of otbn_rf_snooper into
@@ -148,4 +124,6 @@
bind otbn_rf_bignum_ff otbn_rf_snooper #(.Width (256), .Depth (32)) u_snooper (.rf (rf));
end endgenerate
+ assign err_o = failed_step | failed_cmp;
+
endmodule
diff --git a/hw/ip/otbn/dv/model/otbn_model.cc b/hw/ip/otbn/dv/model/otbn_model.cc
index 06275c9..371e244 100644
--- a/hw/ip/otbn/dv/model/otbn_model.cc
+++ b/hw/ip/otbn/dv/model/otbn_model.cc
@@ -19,6 +19,44 @@
extern "C" int simutil_verilator_set_mem(int index, const svBitVecVal *val);
extern "C" int otbn_rf_peek(int index, svBitVecVal *val);
+#define RUNNING_BIT (1U << 0)
+#define FAILED_STEP_BIT (1U << 1)
+#define FAILED_CMP_BIT (1U << 2)
+
+// The main entry point to the OTBN model, exported from here and used in
+// otbn_core_model.sv.
+//
+// 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
+//
+// 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
+// will return immediately (but wastes a DPI call).
+//
+// If the model is running and start_i is false, otbn_model_step steps the ISS
+// by a single cycle. If something goes wrong, it will set failed_step to true
+// and running to false.
+//
+// If nothing goes wrong, but the ISS finishes its run, we still set running to
+// false, but also do the post-run task. If design_scope is non-empty, it
+// should be the scope of an RTL implementation. In that case, we compare
+// register and memory contents with that implementation, printing to stderr
+// and setting the failed_cmp bit if there are any mismatches. If design_scope
+// is the empty string, we grab the contents of DMEM from the ISS and inject
+// them into the memory at dmem_scope.
+//
+// If start_i is true, we start the model at start_addr and then step once (as
+// described above).
+extern "C" unsigned otbn_model_step(ISSWrapper *model, const char *imem_scope,
+ unsigned imem_words, const char *dmem_scope,
+ unsigned dmem_words,
+ const char *design_scope, svLogic start_i,
+ unsigned start_addr, unsigned status);
+
// Use simutil_verilator_get_mem to read data one word at a time from the given
// scope and collect the results up in a vector of uint8_t values.
static std::vector<uint8_t> get_sim_memory(const char *scope, size_t num_words,
@@ -149,13 +187,13 @@
// Start a new run with the model, writing IMEM/DMEM and jumping to the given
// start address. Returns 0 on success; -1 on failure.
-extern "C" int otbn_model_start(ISSWrapper *model, const char *imem_scope,
- int imem_words, const char *dmem_scope,
- int dmem_words, int start_addr) {
+static int start_model(ISSWrapper *model, const char *imem_scope,
+ unsigned imem_words, const char *dmem_scope,
+ unsigned dmem_words, unsigned start_addr) {
assert(model);
assert(imem_words >= 0);
assert(dmem_words >= 0);
- assert(start_addr >= 0 && start_addr < (imem_words * 4));
+ assert(start_addr < (imem_words * 4));
std::string ifname(model->make_tmp_path("imem"));
std::string dfname(model->make_tmp_path("dmem"));
@@ -182,7 +220,7 @@
// Step once in the model. Returns 1 if the model has finished, 0 if not and -1
// on failure.
-extern "C" int otbn_model_step(ISSWrapper *model) {
+static int step_model(ISSWrapper *model) {
assert(model);
try {
return model->step() ? 1 : 0;
@@ -194,8 +232,8 @@
// Grab contents of dmem from the model and load it back into the RTL. Returns
// 0 on success; -1 on failure.
-extern "C" int otbn_model_load_dmem(ISSWrapper *model, const char *dmem_scope,
- int dmem_words) {
+static int load_dmem(ISSWrapper *model, const char *dmem_scope,
+ unsigned dmem_words) {
assert(model);
std::string dfname(model->make_tmp_path("dmem_out"));
try {
@@ -211,10 +249,9 @@
// Grab contents of dmem from the model and compare it with the RTL.
// Prints messages to stderr on failure or mismatch. Returns true on
// success; false on mismatch. Throws a std::runtime_error on failure.
-static bool otbn_model_check_dmem(ISSWrapper *model, const char *dmem_scope,
- int dmem_words) {
+static bool check_dmem(ISSWrapper *model, const char *dmem_scope,
+ unsigned dmem_words) {
assert(model);
- assert(dmem_words >= 0);
size_t dmem_bytes = dmem_words * 32;
std::string dfname(model->make_tmp_path("dmem_out"));
@@ -281,8 +318,7 @@
// Compare contents of ISS registers with those from the design. Prints
// messages to stderr on failure or mismatch. Returns true on success; false on
// mismatch. Throws a std::runtime_error on failure.
-static bool otbn_model_check_regs(ISSWrapper *model,
- const std::string &design_scope) {
+static bool check_regs(ISSWrapper *model, const std::string &design_scope) {
assert(model);
std::string base_scope =
@@ -337,25 +373,25 @@
return good;
}
-// Check model against RTL when a run has finished. Prints messages to
-// stderr on failure or mismatch. Returns 1 for a match, 0 for a
-// mismatch, -1 for some other failure.
-extern "C" int otbn_model_check(ISSWrapper *model, const char *dmem_scope,
- int dmem_words, const char *design_scope) {
+// Check model against RTL when a run has finished. Prints messages to stderr
+// on failure or mismatch. Returns 1 for a match, 0 for a mismatch, -1 for some
+// other failure.
+int check_model(ISSWrapper *model, const char *dmem_scope, unsigned dmem_words,
+ const char *design_scope) {
assert(model);
assert(dmem_words >= 0);
assert(design_scope);
bool good = true;
try {
- good &= otbn_model_check_dmem(model, dmem_scope, dmem_words);
+ good &= check_dmem(model, dmem_scope, dmem_words);
} catch (const std::exception &err) {
std::cerr << "Failed to check DMEM: " << err.what() << "\n";
return -1;
}
try {
- good &= otbn_model_check_regs(model, design_scope);
+ good &= check_regs(model, design_scope);
} catch (const std::exception &err) {
std::cerr << "Failed to check registers: " << err.what() << "\n";
return -1;
@@ -363,3 +399,80 @@
return good ? 1 : 0;
}
+
+extern "C" unsigned otbn_model_step(ISSWrapper *model, const char *imem_scope,
+ unsigned imem_words, const char *dmem_scope,
+ unsigned dmem_words,
+ const char *design_scope, svLogic start_i,
+ unsigned start_addr, unsigned status) {
+ assert(model && imem_scope && dmem_scope && design_scope);
+
+ // Start the model if requested
+ if (start_i) {
+ switch (start_model(model, imem_scope, imem_words, dmem_scope, dmem_words,
+ start_addr)) {
+ case 0:
+ // All good
+ status |= RUNNING_BIT;
+ break;
+
+ default:
+ // Something went wrong.
+ return (status & ~RUNNING_BIT) | FAILED_STEP_BIT;
+ }
+ }
+
+ // If the model isn't running, there's nothing more to do.
+ if (!(status & RUNNING_BIT))
+ return status;
+
+ // Step the model once
+ switch (step_model(model)) {
+ case 0:
+ // Still running: no change
+ break;
+
+ case 1:
+ // Finished
+ status = status & ~RUNNING_BIT;
+ break;
+
+ default:
+ // Something went wrong
+ return (status & ~RUNNING_BIT) | FAILED_STEP_BIT;
+ }
+
+ // If we're still running, there's nothing more to do.
+ if (status & RUNNING_BIT)
+ return status;
+
+ // If we've stopped running, either load DMEM or check against the RTL.
+ if (design_scope[0] == '\0') {
+ switch (load_dmem(model, dmem_scope, dmem_words)) {
+ case 0:
+ // Success
+ break;
+
+ default:
+ // Failed to load DMEM
+ return (status & ~RUNNING_BIT) | FAILED_STEP_BIT;
+ }
+ } else {
+ switch (check_model(model, dmem_scope, dmem_words, design_scope)) {
+ case 1:
+ // Match (success)
+ break;
+
+ case 0:
+ // Mismatch
+ status |= FAILED_CMP_BIT;
+ break;
+
+ default:
+ // Something went wrong
+ return (status & ~RUNNING_BIT) | FAILED_STEP_BIT;
+ }
+ }
+
+ return status;
+}
diff --git a/hw/ip/otbn/dv/verilator/otbn_top_sim.sv b/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
index 6bab4cc..a5a108e 100644
--- a/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
+++ b/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
@@ -198,7 +198,9 @@
.start_i ( otbn_start ),
.done_o ( otbn_model_done ),
- .start_addr_i ( ImemStartAddr )
+ .start_addr_i ( ImemStartAddr ),
+
+ .err_o ()
);
always_ff @(posedge IO_CLK or negedge IO_RST_N) begin
diff --git a/hw/ip/otbn/rtl/otbn.sv b/hw/ip/otbn/rtl/otbn.sv
index fc49a67..133f561 100644
--- a/hw/ip/otbn/rtl/otbn.sv
+++ b/hw/ip/otbn/rtl/otbn.sv
@@ -441,7 +441,9 @@
.start_i (start),
.done_o (done),
- .start_addr_i (start_addr)
+ .start_addr_i (start_addr),
+
+ .err_o ()
);
end else begin : gen_impl_rtl
otbn_core #(
