[otbn] Remove enable_i input from otbn_core_model
The previous code had a race in the initial blocks when used in
otbn.sv. The obvious way to fix this in the SystemVerilog code would
be to call otbn_model_init() just before we first call
otbn_model_step. Unfortunately, you can't do that (for Verilator, at
least) because that ends up mixing blocking and non-blocking
assignments.
Rather than think hard about how to do this properly in Verilog, we
can just put in a simple layer of indirection in the C++ and only
start the subprocess when we see start_i for the first time.
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 126bfcb..527dcba 100644
--- a/hw/ip/otbn/dv/model/otbn_core_model.sv
+++ b/hw/ip/otbn/dv/model/otbn_core_model.sv
@@ -34,10 +34,6 @@
input logic clk_i,
input logic rst_ni,
- // Enable the model. This signal is a "runtime parameter", it is only honored if set in an initial
- // block.
- input logic enable_i,
-
input logic start_i, // start the operation
output bit done_o, // operation done
@@ -79,21 +75,13 @@
`endif
// Create and destroy an object through which we can talk to the ISS.
- // If enable_i is not set, do not initialize the ISS (and, by consequence, do not start the Python
- // process).
- chandle model_handle = chandle_null;
+ chandle model_handle;
initial begin
- if (enable_i) begin
- model_handle = otbn_model_init();
- assert(model_handle != chandle_null);
- $display("OTBN: Model enabled.");
- end
+ model_handle = otbn_model_init();
+ assert(model_handle != chandle_null);
end
final begin
- if (model_handle != chandle_null) begin
- otbn_model_destroy(model_handle);
- model_handle = chandle_null;
- end
+ otbn_model_destroy(model_handle);
end
// A packed "status" value. This gets assigned by DPI function calls that need to update both
@@ -115,7 +103,7 @@
bit [31:0] raw_err_bits_d, raw_err_bits_q;
bit unused_raw_err_bits;
always_ff @(posedge clk_i or negedge rst_ni) begin
- if (!rst_ni || !enable_i) begin
+ if (!rst_ni) begin
// Clear status (stop running, and forget any errors)
status <= 0;
raw_err_bits_q <= 0;
diff --git a/hw/ip/otbn/dv/model/otbn_model.cc b/hw/ip/otbn/dv/model/otbn_model.cc
index ae361ad..0cc4b5f 100644
--- a/hw/ip/otbn/dv/model/otbn_model.cc
+++ b/hw/ip/otbn/dv/model/otbn_model.cc
@@ -16,6 +16,32 @@
#include "otbn_trace_checker.h"
#include "sv_scoped.h"
+namespace {
+// An extremely thin wrapper around ISSWrapper. The point is that we want to
+// create the model in an initial block in the SystemVerilog simulation, but
+// might not actually want to spawn the ISS. To handle that in a non-racy
+// way, the most convenient thing is to spawn the ISS on the first call to
+// otbn_model_step.
+struct OtbnModel {
+ public:
+ bool ensure() {
+ if (!iss) {
+ try {
+ iss.reset(new ISSWrapper());
+ } catch (const std::runtime_error &err) {
+ std::cerr << "Error when constructing ISS wrapper: " << err.what()
+ << "\n";
+ return false;
+ }
+ }
+ assert(iss);
+ return true;
+ }
+
+ std::unique_ptr<ISSWrapper> iss;
+};
+} // namespace
+
extern "C" {
// DPI imports, implemented in SystemVerilog
int simutil_get_mem(int index, svBitVecVal *val);
@@ -64,7 +90,7 @@
//
// 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,
+extern "C" unsigned otbn_model_step(OtbnModel *model, const char *imem_scope,
unsigned imem_words, const char *dmem_scope,
unsigned dmem_words,
const char *design_scope, svLogic start_i,
@@ -187,20 +213,13 @@
word_size);
}
-extern "C" ISSWrapper *otbn_model_init() {
- try {
- return new ISSWrapper();
- } catch (const std::runtime_error &err) {
- std::cerr << "Error when constructing ISS wrapper: " << err.what() << "\n";
- return nullptr;
- }
-}
+extern "C" OtbnModel *otbn_model_init() { return new OtbnModel; }
-extern "C" void otbn_model_destroy(ISSWrapper *model) { delete model; }
+extern "C" void otbn_model_destroy(OtbnModel *model) { delete model; }
// Start a new run with the model, writing IMEM/DMEM and jumping to the given
// start address. Returns 0 on success; -1 on failure.
-static int start_model(ISSWrapper *model, const char *imem_scope,
+static int start_model(OtbnModel *model, const char *imem_scope,
unsigned imem_words, const char *dmem_scope,
unsigned dmem_words, unsigned start_addr) {
assert(model);
@@ -208,8 +227,13 @@
assert(dmem_words >= 0);
assert(start_addr < (imem_words * 4));
- std::string ifname(model->make_tmp_path("imem"));
- std::string dfname(model->make_tmp_path("dmem"));
+ if (!model->ensure())
+ return -1;
+ assert(model->iss);
+ ISSWrapper &iss = *model->iss;
+
+ std::string ifname(iss.make_tmp_path("imem"));
+ std::string dfname(iss.make_tmp_path("dmem"));
try {
dump_memory_to_file(dfname, dmem_scope, dmem_words, 32);
@@ -220,9 +244,9 @@
}
try {
- model->load_d(dfname);
- model->load_i(ifname);
- model->start(start_addr);
+ iss.load_d(dfname);
+ iss.load_i(ifname);
+ iss.start(start_addr);
} catch (const std::runtime_error &err) {
std::cerr << "Error when starting ISS: " << err.what() << "\n";
return -1;
@@ -234,12 +258,17 @@
// Step once in the model. Returns 1 if the model has finished, 0 if not and -1
// on failure. If gen_trace is true, pass trace entries to the trace checker.
// If the model has finished, writes otbn.ERR_CODE to *err_code.
-static int step_model(ISSWrapper *model, bool gen_trace, uint32_t *err_code) {
+static int step_model(OtbnModel *model, bool gen_trace, uint32_t *err_code) {
assert(model);
assert(err_code);
+ if (!model->ensure())
+ return -1;
+ assert(model->iss);
+ ISSWrapper &iss = *model->iss;
+
try {
- std::pair<bool, uint32_t> ret = model->step(gen_trace);
+ std::pair<bool, uint32_t> ret = iss.step(gen_trace);
if (ret.first) {
*err_code = ret.second;
return 1;
@@ -254,12 +283,18 @@
// Grab contents of dmem from the model and load it back into the RTL. Returns
// 0 on success; -1 on failure.
-static int load_dmem(ISSWrapper *model, const char *dmem_scope,
+static int load_dmem(OtbnModel *model, const char *dmem_scope,
unsigned dmem_words) {
assert(model);
- std::string dfname(model->make_tmp_path("dmem_out"));
+ if (!model->iss) {
+ std::cerr << "Cannot load dmem from OTBN model: ISS has not started.\n";
+ return -1;
+ }
+ ISSWrapper &iss = *model->iss;
+
+ std::string dfname(iss.make_tmp_path("dmem_out"));
try {
- model->dump_d(dfname);
+ iss.dump_d(dfname);
load_memory_from_file(dfname, dmem_scope, dmem_words, 32);
} catch (const std::exception &err) {
std::cerr << "Error when loading dmem from ISS: " << err.what() << "\n";
@@ -271,14 +306,12 @@
// 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 check_dmem(ISSWrapper *model, const char *dmem_scope,
+static bool check_dmem(ISSWrapper &iss, const char *dmem_scope,
unsigned dmem_words) {
- assert(model);
-
size_t dmem_bytes = dmem_words * 32;
- std::string dfname(model->make_tmp_path("dmem_out"));
+ std::string dfname(iss.make_tmp_path("dmem_out"));
- model->dump_d(dfname);
+ iss.dump_d(dfname);
std::vector<uint8_t> iss_data = read_vector_from_file(dfname, dmem_bytes);
assert(iss_data.size() == dmem_bytes);
@@ -388,9 +421,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 check_regs(ISSWrapper *model, const std::string &design_scope) {
- assert(model);
-
+static bool check_regs(ISSWrapper &iss, const std::string &design_scope) {
std::string base_scope =
design_scope +
".u_otbn_rf_base.gen_rf_base_ff.u_otbn_rf_base_inner.u_snooper";
@@ -402,7 +433,7 @@
std::array<uint32_t, 32> iss_gprs;
std::array<ISSWrapper::u256_t, 32> iss_wdrs;
- model->get_regs(&iss_gprs, &iss_wdrs);
+ iss.get_regs(&iss_gprs, &iss_wdrs);
bool good = true;
@@ -451,16 +482,13 @@
// Compare contents of ISS call stack 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 check_call_stack(ISSWrapper *model,
- const std::string &design_scope) {
- assert(model);
-
+static bool check_call_stack(ISSWrapper &iss, const std::string &design_scope) {
std::string call_stack_snooper_scope =
design_scope + ".u_otbn_rf_base.u_call_stack_snooper";
auto rtl_call_stack = get_stack<uint32_t>(call_stack_snooper_scope);
- auto iss_call_stack = model->get_call_stack();
+ auto iss_call_stack = iss.get_call_stack();
bool good = true;
@@ -493,32 +521,38 @@
// 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,
+int check_model(OtbnModel *model, const char *dmem_scope, unsigned dmem_words,
const char *design_scope) {
assert(model);
assert(dmem_words >= 0);
assert(design_scope);
+ if (!model->iss) {
+ std::cerr << "Cannot check OTBN model: ISS has not started.\n";
+ return -1;
+ }
+ ISSWrapper &iss = *model->iss;
+
bool good = true;
good &= OtbnTraceChecker::get().Finish();
try {
- good &= check_dmem(model, dmem_scope, dmem_words);
+ good &= check_dmem(iss, dmem_scope, dmem_words);
} catch (const std::exception &err) {
std::cerr << "Failed to check DMEM: " << err.what() << "\n";
return -1;
}
try {
- good &= check_regs(model, design_scope);
+ good &= check_regs(iss, design_scope);
} catch (const std::exception &err) {
std::cerr << "Failed to check registers: " << err.what() << "\n";
return -1;
}
try {
- good &= check_call_stack(model, design_scope);
+ good &= check_call_stack(iss, design_scope);
} catch (const std::exception &err) {
std::cerr << "Failed to check call stack: " << err.what() << "\n";
return -1;
@@ -537,7 +571,7 @@
}
}
-extern "C" unsigned otbn_model_step(ISSWrapper *model, const char *imem_scope,
+extern "C" unsigned otbn_model_step(OtbnModel *model, const char *imem_scope,
unsigned imem_words, const char *dmem_scope,
unsigned dmem_words,
const char *design_scope, svLogic start_i,
diff --git a/hw/ip/otbn/dv/uvm/tb.sv b/hw/ip/otbn/dv/uvm/tb.sv
index b9324fb..bfa711f 100644
--- a/hw/ip/otbn/dv/uvm/tb.sv
+++ b/hw/ip/otbn/dv/uvm/tb.sv
@@ -82,8 +82,6 @@
.clk_i (model_if.clk_i),
.rst_ni (model_if.rst_ni),
- .enable_i (1'b1),
-
.start_i (model_if.start),
.done_o (model_if.done),
.start_addr_i (model_if.start_addr),
diff --git a/hw/ip/otbn/dv/verilator/otbn_top_sim.sv b/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
index ef1cbef..d6f639b 100644
--- a/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
+++ b/hw/ip/otbn/dv/verilator/otbn_top_sim.sv
@@ -221,8 +221,6 @@
.clk_i ( IO_CLK ),
.rst_ni ( IO_RST_N ),
- .enable_i ( 1'b1 ),
-
.start_i ( otbn_start ),
.done_o ( otbn_model_done ),
diff --git a/hw/ip/otbn/rtl/otbn.sv b/hw/ip/otbn/rtl/otbn.sv
index 6b4b0a8..031d731 100644
--- a/hw/ip/otbn/rtl/otbn.sv
+++ b/hw/ip/otbn/rtl/otbn.sv
@@ -507,8 +507,6 @@
.clk_i,
.rst_ni,
- .enable_i (otbn_use_model),
-
.start_i (start_model),
.done_o (done_model),
