[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),