[otbn,rtl] Avoid a 1-cycle glitch in locking_o
The locking_o signal used to be computed as follows:
assign locking_o = (state_d == OtbnStateLocked) & ~secure_wipe_running_i;
This used to use state_q, but 48e4bb6 changed to state_d to move it a
cycle earlier. Of course, this doesn't work properly because you end
up with a single cycle "glitch" where locking_o is asserted before the
secure wipe starts.
The problem is that you can't just assert secure_wipe_running_o in the
start/stop controller a cycle earlier because you end up with a
combinational loop (I tried!). This commit switches the start/stop
controller to a sort of req/ack interface where the controller asks to
start a secure wipe and the start/stop controller responds once it's
done.
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
diff --git a/hw/ip/otbn/dv/tracer/rtl/otbn_trace_if.sv b/hw/ip/otbn/dv/tracer/rtl/otbn_trace_if.sv
index f024dfc..4fb9cbf 100644
--- a/hw/ip/otbn/dv/tracer/rtl/otbn_trace_if.sv
+++ b/hw/ip/otbn/dv/tracer/rtl/otbn_trace_if.sv
@@ -82,7 +82,8 @@
input logic [1:0][otbn_pkg::SideloadKeyWidth-1:0] sideload_key_shares_i,
- input logic secure_wipe_running
+ input logic start_secure_wipe,
+ input logic secure_wipe_done
);
import otbn_pkg::*;
@@ -310,16 +311,14 @@
assign flags_read_data[i_fg] = u_otbn_alu_bignum.flags_q[i_fg];
end
- // Detect negative edges of the secure wipe signal
- logic secure_wipe_running_r, secure_wipe_done;
+ logic secure_wipe_running;
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
- secure_wipe_running_r <= 0;
+ secure_wipe_running <= 0;
end else begin
- secure_wipe_running_r <= secure_wipe_running;
+ secure_wipe_running <= start_secure_wipe | (secure_wipe_running & ~secure_wipe_done);
end
end
- assign secure_wipe_done = secure_wipe_running_r & ~secure_wipe_running;
endinterface
diff --git a/hw/ip/otbn/dv/tracer/rtl/otbn_tracer.sv b/hw/ip/otbn/dv/tracer/rtl/otbn_tracer.sv
index f0b2cee..95b1e06 100644
--- a/hw/ip/otbn/dv/tracer/rtl/otbn_tracer.sv
+++ b/hw/ip/otbn/dv/tracer/rtl/otbn_tracer.sv
@@ -207,10 +207,10 @@
end
end
if (SecWipeEn) begin
- if (otbn_trace.secure_wipe_running) begin
- output_trace(WipeInProgressPrefix, "");
- end else if (otbn_trace.secure_wipe_done) begin
+ if (otbn_trace.secure_wipe_done) begin
output_trace(WipeCompletePrefix, "");
+ end else if (otbn_trace.secure_wipe_running) begin
+ output_trace(WipeInProgressPrefix, "");
end
end
endfunction
diff --git a/hw/ip/otbn/rtl/otbn_controller.sv b/hw/ip/otbn/rtl/otbn_controller.sv
index cbaddd8..5233085 100644
--- a/hw/ip/otbn/rtl/otbn_controller.sv
+++ b/hw/ip/otbn/rtl/otbn_controller.sv
@@ -136,8 +136,8 @@
input logic rnd_fips_err_i,
// Secure Wipe
- input logic secure_wipe_running_i,
output logic start_secure_wipe_o,
+ input logic secure_wipe_done_i,
input logic sec_wipe_zero_i,
input logic state_reset_i,
@@ -285,6 +285,15 @@
logic [4:0] insn_bignum_rd_addr_a_q, insn_bignum_rd_addr_b_q, insn_bignum_wr_addr_q;
+ logic secure_wipe_running_q;
+ always_ff @(posedge clk_i or negedge rst_ni) begin
+ if (!rst_ni) begin
+ secure_wipe_running_q <= 1'b0;
+ end else begin
+ secure_wipe_running_q <= start_secure_wipe_o | (secure_wipe_running_q & ~secure_wipe_done_i);
+ end
+ end
+
// Stall a cycle on loads to allow load data writeback to happen the following cycle. Stall not
// required on stores as there is no response to deal with.
assign mem_stall = lsu_load_req_raw;
@@ -310,8 +319,8 @@
assign executing = (state_q == OtbnStateRun) ||
(state_q == OtbnStateStall);
- assign locking_o = (state_d == OtbnStateLocked) & ~secure_wipe_running_i;
- assign start_secure_wipe_o = executing & (done_complete | err) & ~secure_wipe_running_i;
+ assign locking_o = (state_d == OtbnStateLocked) & ~(start_secure_wipe_o | secure_wipe_running_q);
+ assign start_secure_wipe_o = executing & (done_complete | err) & ~secure_wipe_running_q;
assign jump_or_branch = (insn_valid_i &
(insn_dec_shared_i.branch_insn | insn_dec_shared_i.jump_insn));
diff --git a/hw/ip/otbn/rtl/otbn_core.sv b/hw/ip/otbn/rtl/otbn_core.sv
index a2ec496..d7d3341 100644
--- a/hw/ip/otbn/rtl/otbn_core.sv
+++ b/hw/ip/otbn/rtl/otbn_core.sv
@@ -219,7 +219,7 @@
logic [31:0] insn_cnt;
logic start_secure_wipe;
- logic secure_wipe_running;
+ logic secure_wipe_done;
logic sec_wipe_wdr_d, sec_wipe_wdr_q;
logic sec_wipe_wdr_urnd_d, sec_wipe_wdr_urnd_q;
@@ -266,8 +266,8 @@
.urnd_reseed_ack_i (urnd_reseed_ack),
.urnd_advance_o (urnd_advance_start_stop_control),
- .start_secure_wipe_i (start_secure_wipe),
- .secure_wipe_running_o(secure_wipe_running),
+ .start_secure_wipe_i (start_secure_wipe),
+ .secure_wipe_done_o (secure_wipe_done),
.done_o,
.sec_wipe_wdr_o (sec_wipe_wdr_d),
@@ -486,9 +486,9 @@
.rnd_fips_err_i (rnd_fips_err),
// Secure wipe
- .secure_wipe_running_i(secure_wipe_running),
- .start_secure_wipe_o (start_secure_wipe),
- .sec_wipe_zero_i (sec_wipe_zero),
+ .start_secure_wipe_o (start_secure_wipe),
+ .secure_wipe_done_i (secure_wipe_done),
+ .sec_wipe_zero_i (sec_wipe_zero),
.state_reset_i(state_reset),
.insn_cnt_o (insn_cnt),
diff --git a/hw/ip/otbn/rtl/otbn_start_stop_control.sv b/hw/ip/otbn/rtl/otbn_start_stop_control.sv
index 1757a50..5064ce8 100644
--- a/hw/ip/otbn/rtl/otbn_start_stop_control.sv
+++ b/hw/ip/otbn/rtl/otbn_start_stop_control.sv
@@ -42,7 +42,7 @@
output logic urnd_advance_o,
input logic start_secure_wipe_i,
- output logic secure_wipe_running_o,
+ output logic secure_wipe_done_o,
output logic done_o,
output logic sec_wipe_wdr_o,
@@ -103,7 +103,7 @@
sec_wipe_mod_urnd_o = 1'b0;
sec_wipe_zero_o = 1'b0;
addr_cnt_inc = 1'b0;
- secure_wipe_running_o = 1'b0;
+ secure_wipe_done_o = 1'b0;
state_error_o = 1'b0;
unique case (state_q)
@@ -142,7 +142,6 @@
addr_cnt_inc = 1'b1;
sec_wipe_wdr_o = 1'b1;
sec_wipe_wdr_urnd_o = 1'b1;
- secure_wipe_running_o = 1'b1;
if (addr_cnt_q == 5'b11111) begin
state_d = OtbnStartStopSecureWipeAccModBaseUrnd;
end
@@ -151,7 +150,6 @@
// addr_cnt_q wraps around to 0 when first moving to this state, and we need to
// supress writes to the zero register and the call stack.
OtbnStartStopSecureWipeAccModBaseUrnd: begin
- secure_wipe_running_o = 1'b1;
urnd_advance_o = 1'b1;
addr_cnt_inc = 1'b1;
// The first two clock cycles are used to write random data to accumulator and modulus.
@@ -167,7 +165,6 @@
// Writing zeros to the accumulator, modulus and the registers.
// Resetting stack
OtbnStartStopSecureWipeAllZero: begin
- secure_wipe_running_o = 1'b1;
sec_wipe_zero_o = (addr_cnt_q == 5'b00000);
sec_wipe_wdr_o = 1'b1;
sec_wipe_base_o = (addr_cnt_q > 5'b00001);
@@ -178,6 +175,7 @@
end
OtbnStartStopSecureWipeComplete: begin
urnd_advance_o = 1'b1;
+ secure_wipe_done_o = 1'b1;
state_d = should_lock_d ? OtbnStartStopStateLocked : OtbnStartStopStateHalt;
end
OtbnStartStopStateLocked: begin
