Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rtl] Harden lockstep enable against FI #2129

Merged
merged 1 commit into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions dv/uvm/core_ibex/ibex_dv.f
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
${PRJ_DIR}/dv/uvm/core_ibex/common/prim/prim_pkg.sv
${LOWRISC_IP_DIR}/ip/prim/rtl/prim_assert.sv
${LOWRISC_IP_DIR}/ip/prim/rtl/prim_util_pkg.sv
${LOWRISC_IP_DIR}/ip/prim/rtl/prim_count.sv
${LOWRISC_IP_DIR}/ip/prim/rtl/prim_secded_pkg.sv
${LOWRISC_IP_DIR}/ip/prim/rtl/prim_secded_22_16_dec.sv
${LOWRISC_IP_DIR}/ip/prim/rtl/prim_secded_22_16_enc.sv
Expand Down
7 changes: 7 additions & 0 deletions dv/uvm/core_ibex/tb/core_ibex_tb_top.sv
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,13 @@ module core_ibex_tb_top;
run_test();
end

// Manually set unused_assert_connected = 1 to disable the AssertConnected_A assertion for
// prim_count in case lockstep (set by SecureIbex) is enabled. If not disabled, DV fails.
if (SecureIbex) begin : gen_disable_count_check
assign dut.u_ibex_top.gen_lockstep.u_ibex_lockstep.u_rst_shadow_cnt.
unused_assert_connected = 1;
end

// Disable the assertion for onhot check in case WrenCheck (set by SecureIbex) is enabled.
if (SecureIbex) begin : gen_disable_onehot_check
assign dut.u_ibex_top.gen_regfile_ff.register_file_i.gen_wren_check.u_prim_onehot_check.
Expand Down
1 change: 1 addition & 0 deletions ibex_top.core
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ filesets:
- lowrisc:prim:and2
- lowrisc:prim:buf
- lowrisc:prim:clock_mux2
- lowrisc:prim:count
- lowrisc:prim:flop
- lowrisc:prim:ram_1p_scr
- lowrisc:prim:onehot_check
Expand Down
4 changes: 4 additions & 0 deletions rtl/ibex_core.sv
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ module ibex_core import ibex_pkg::*; #(
) (
// Clock and Reset
input logic clk_i,
// Internally generated resets in ibex_lockstep cause IMPERFECTSCH warnings.
// TODO: Remove when upgrading Verilator #2134.
/* verilator lint_off IMPERFECTSCH */
input logic rst_ni,
/* verilator lint_on IMPERFECTSCH */

input logic [31:0] hart_id_i,
input logic [31:0] boot_addr_i,
Expand Down
75 changes: 52 additions & 23 deletions rtl/ibex_lockstep.sv
Original file line number Diff line number Diff line change
Expand Up @@ -120,44 +120,71 @@ module ibex_lockstep import ibex_pkg::*; #(
// - The reset of the shadow core is synchronously released.
// The comparison is started in the following clock cycle.

logic [LockstepOffsetW-1:0] rst_shadow_cnt_d, rst_shadow_cnt_q, rst_shadow_cnt_incr;
// Internally generated resets cause IMPERFECTSCH warnings
/* verilator lint_off IMPERFECTSCH */
logic rst_shadow_set_d, rst_shadow_set_q;
logic rst_shadow_n, enable_cmp_q;
/* verilator lint_on IMPERFECTSCH */
logic [LockstepOffsetW-1:0] rst_shadow_cnt;
logic rst_shadow_cnt_err;
ibex_mubi_t rst_shadow_set_d, rst_shadow_set_q;
logic rst_shadow_n, rst_shadow_set_single_bit;
ibex_mubi_t enable_cmp_d, enable_cmp_q;

// This counter primitive starts counting to LockstepOffset after a system
// reset. The counter value saturates at LockstepOffset.
prim_count #(
nasahlpa marked this conversation as resolved.
Show resolved Hide resolved
.Width (LockstepOffsetW ),
.ResetValue (LockstepOffsetW'(1'b0) )
nasahlpa marked this conversation as resolved.
Show resolved Hide resolved
) u_rst_shadow_cnt (
.clk_i (clk_i ),
.rst_ni (rst_ni ),
.clr_i (1'b0 ),
.set_i (1'b0 ),
.set_cnt_i ('0 ),
.incr_en_i (1'b1 ),
.decr_en_i (1'b0 ),
.step_i (LockstepOffsetW'(1'b1) ),
.cnt_o (rst_shadow_cnt ),
.cnt_next_o ( ),
.err_o (rst_shadow_cnt_err )
);

assign rst_shadow_cnt_incr = rst_shadow_cnt_q + 1'b1;
// When the LockstepOffset counter value is reached, activate the lockstep
// comparison. We do not explicitly check whether rst_shadow_set_q forms a valid
// multibit signal as this value is implicitly checked by the enable_cmp
// comparison below.
assign rst_shadow_set_d =
(rst_shadow_cnt >= LockstepOffsetW'(LockstepOffset - 1)) ? IbexMuBiOn : IbexMuBiOff;

assign rst_shadow_set_d = (rst_shadow_cnt_q == LockstepOffsetW'(LockstepOffset - 1));
assign rst_shadow_cnt_d = rst_shadow_set_d ? rst_shadow_cnt_q : rst_shadow_cnt_incr;
// Enable lockstep comparison.
assign enable_cmp_d = rst_shadow_set_q;

always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
rst_shadow_cnt_q <= '0;
enable_cmp_q <= '0;
end else begin
rst_shadow_cnt_q <= rst_shadow_cnt_d;
enable_cmp_q <= rst_shadow_set_q;
end
end
// This assignment is needed in order to avoid "Warning-IMPERFECTSCH" messages.
// TODO: Remove when updating Verilator #2134.
assign rst_shadow_set_single_bit = rst_shadow_set_q[0];

// The primitives below are used to place size-only constraints in order to prevent
// synthesis optimizations and preserve anchor points for constraining backend tools.
prim_flop #(
.Width(1),
.ResetValue(1'b0)
.Width(IbexMuBiWidth),
.ResetValue(IbexMuBiOff)
) u_prim_rst_shadow_set_flop (
.clk_i (clk_i),
.rst_ni(rst_ni),
.d_i (rst_shadow_set_d),
.q_o (rst_shadow_set_q)
);

prim_flop #(
.Width(IbexMuBiWidth),
.ResetValue(IbexMuBiOff)
) u_prim_enable_cmp_flop (
.clk_i (clk_i),
.rst_ni(rst_ni),
.d_i (enable_cmp_d),
.q_o (enable_cmp_q)
);

prim_clock_mux2 #(
.NoFpgaBufG(1'b1)
) u_prim_rst_shadow_n_mux2 (
.clk0_i(rst_shadow_set_q),
.clk0_i(rst_shadow_set_single_bit),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed, because when doing this:

Suggested change
.clk0_i(rst_shadow_set_single_bit),
.clk0_i(rst_shadow_set_q[0]),

I get the IMPERFECTSCH linting error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the IMPERFECTSCH warning cause CI to fail? If not, I'd be inclined to use the rst_shadow_set_q[0] form. I notice that the warning was removed from Verilator with version 5 (https://verilator.org/guide/latest/warnings.html).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, CI fails because of this IMPERFECTSCH warning.

I would also prefer using rst_shadow_set_q[0], but I couldn't find a way of getting rid of this warning in CI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A solution: upgrade our Verilator dependency :-)

I think we can probably just do that. Would you mind filing an issue to remind us? ("Upgrade Verilator, then remove hack I had to do in ibex_lockstep.sv" or something similar)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, tracked in issue #2131.

.clk1_i(scan_rst_ni),
.sel_i (test_en_i),
.clk_o (rst_shadow_n)
Expand Down Expand Up @@ -458,8 +485,10 @@ module ibex_lockstep import ibex_pkg::*; #(

logic outputs_mismatch;

assign outputs_mismatch = enable_cmp_q & (shadow_outputs_q != core_outputs_q[0]);
assign alert_major_internal_o = outputs_mismatch | shadow_alert_major_internal;
assign outputs_mismatch =
(enable_cmp_q != IbexMuBiOff) & (shadow_outputs_q != core_outputs_q[0]);
assign alert_major_internal_o
= outputs_mismatch | shadow_alert_major_internal | rst_shadow_cnt_err;
assign alert_major_bus_o = shadow_alert_major_bus;
assign alert_minor_o = shadow_alert_minor;

Expand Down
3 changes: 2 additions & 1 deletion rtl/ibex_pkg.sv
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,8 @@ package ibex_pkg;

// Mult-bit signal used for security hardening. For non-secure implementation all bits other than
// the bottom bit are ignored.
typedef logic [3:0] ibex_mubi_t;
parameter int IbexMuBiWidth = 4;
typedef logic [IbexMuBiWidth-1:0] ibex_mubi_t;

// Note that if adjusting these parameters it is assumed the bottom bit is set for On and unset
// for Off. This allows the use of IbexMuBiOn/IbexMuBiOff to work for both secure and non-secure
Expand Down
Loading