Skip to content

Commit

Permalink
[hmac] Error Handling: Discard Msg if sha_en:=0
Browse files Browse the repository at this point in the history
First version of HMAC silently discarded the incoming message if the
message FIFO was full. The reasone was the FIFO was connected to the
register interface, which doesn't have back-pressure mechanism. So, the
consequences of the behavior was the hash digest could be wrong if the
feeder sends the data faster than what HMAC can consume.

Livestream mode was introduced to address the issue above. With
`prim_packer` module and the window feature in reggen, now the message
FIFO is connected through TL-UL window port not the register interface
port. Window port is another TL-UL port that can back-pressure the
requester (yes it has ready signal now).

The consequences? Now the feeder (software or DMA later) can be stuck at
requesting new data until HMAC consumes previous data. It becomes
important that the HMAC IP now shouldn't hang, which has a chance to
create a hang condition to the entire SoC system.

One particular case was what @cindychip found in lowRISC#523. She tried to send
a message into the HMAC message FIFO while HMAC is turned off, aka
sha_en := 0. This case, HMAC accepts the message and let the message
FIFO full and eventually back-pressuring the host.

As the host is back-pressured, it cannot clear the hang condition!
(maybe watchdog eventually can do) So, the design is changed to clear
this issue. Now HMAC discards the incoming message if HMAC is not
enabled.

One more step taken in this issue is to create `ERR_CODE` register. It
might be OK to create another interrupt line named as
`new_msg_sha_disabled` or something similar. It, however, could be
beneficial to combine any error message into one error message code and
let the software knows what was the reason after getting the alert/
interrupt. So, HMAC stores error code while generating the interrupt.

It also adds a logic to detect the condition of the software that sets
`hash_start` when `sha_en` is 0. This ensures the earliest catch of
wrong software behavior.

Next PR will also merge `fifo_full` interrupt into `hmac_err`.
  • Loading branch information
Eunchan Kim committed Oct 23, 2019
1 parent a40dc02 commit 14a4312
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 11 deletions.
16 changes: 16 additions & 0 deletions hw/ip/hmac/doc/hmac.hjson
Expand Up @@ -14,6 +14,9 @@
{ name: "fifo_full",
desc: "Message FIFO full condition"
}
{ name: "hmac_err",
desc: "HMAC error occurred. ERR_CODE register shows which error occurred"
}
],
alert_list: [
],
Expand Down Expand Up @@ -98,6 +101,19 @@
}
]
}
{ name: "ERR_CODE",
desc: "HMAC Error Code",
swaccess: "ro",
hwaccess: "hwo",
fields: [
{ bits: "31:0",
name: "err_code",
desc: '''If error interrupt occurs, this register has information of error cause.
Please take a look at `hw/ip/hmac/rtl/hmac_pkg.sv:err_code_e enum type.
'''
}
]
}
{ name: "WIPE_SECRET",
desc: '''Randomize internal secret registers.

Expand Down
71 changes: 60 additions & 11 deletions hw/ip/hmac/rtl/hmac.sv
Expand Up @@ -12,7 +12,8 @@ module hmac (
output tlul_pkg::tl_d2h_t tl_o,

output logic intr_hmac_done_o,
output logic intr_fifo_full_o
output logic intr_fifo_full_o,
output logic intr_hmac_err_o
);

import hmac_pkg::*;
Expand Down Expand Up @@ -73,6 +74,7 @@ module hmac (

logic reg_hash_start;
logic sha_hash_start;
logic hash_start; // Valid hash_start_signal
logic reg_hash_process;
logic sha_hash_process;

Expand All @@ -82,6 +84,9 @@ module hmac (
logic [63:0] message_length;
logic [63:0] sha_message_length;

err_code_e err_code;
logic err_valid;

sha_word_t [7:0] digest;

// Connect registers ========================================================
Expand Down Expand Up @@ -112,6 +117,13 @@ module hmac (
assign reg_hash_start = reg2hw.cmd.hash_start.qe & reg2hw.cmd.hash_start.q;
assign reg_hash_process = reg2hw.cmd.hash_process.qe & reg2hw.cmd.hash_process.q;

// Error code register
assign hw2reg.err_code.de = err_valid;
assign hw2reg.err_code.d = err_code;

// Control signals ==========================================================
assign hash_start = reg_hash_start & sha_en;

// Interrupts ===============================================================
logic fifo_full_d;
always_ff @(posedge clk_i or negedge rst_ni) begin
Expand All @@ -122,8 +134,8 @@ module hmac (
logic fifo_full_event;
assign fifo_full_event = fifo_full & !fifo_full_d;

logic [1:0] event_intr;
assign event_intr = {fifo_full_event, reg_hash_done};
logic [2:0] event_intr;
assign event_intr = {err_valid, fifo_full_event, reg_hash_done};

// instantiate interrupt hardware primitive
prim_intr_hw #(.Width(1)) intr_hw_hmac_done (
Expand All @@ -146,6 +158,16 @@ module hmac (
.hw2reg_intr_state_d_o (hw2reg.intr_state.fifo_full.d),
.intr_o (intr_fifo_full_o)
);
prim_intr_hw #(.Width(1)) intr_hw_hmac_err (
.event_intr_i (event_intr[2]),
.reg2hw_intr_enable_q_i (reg2hw.intr_enable.hmac_err.q),
.reg2hw_intr_test_q_i (reg2hw.intr_test.hmac_err.q),
.reg2hw_intr_test_qe_i (reg2hw.intr_test.hmac_err.qe),
.reg2hw_intr_state_q_i (reg2hw.intr_state.hmac_err.q),
.hw2reg_intr_state_de_o (hw2reg.intr_state.hmac_err.de),
.hw2reg_intr_state_d_o (hw2reg.intr_state.hmac_err.d),
.intr_o (intr_hmac_err_o)
);
// Instances ================================================================

// TODO: Revise logic to assert rvalid only after grant asserted.
Expand Down Expand Up @@ -173,7 +195,7 @@ module hmac (
.clk_i,
.rst_ni,

.wvalid (fifo_wvalid),
.wvalid (fifo_wvalid & sha_en),
.wready (fifo_wready),
.wdata (fifo_wdata),

Expand Down Expand Up @@ -226,7 +248,7 @@ module hmac (
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) begin
message_length <= '0;
end else if (reg_hash_start) begin // or reg_hash_start
end else if (hash_start) begin
message_length <= '0;
end else if (reg_fifo_wvalid && fifo_wready && !hmac_fifo_wsel) begin
message_length <= message_length + 64'(wmask_ones);
Expand Down Expand Up @@ -254,7 +276,7 @@ module hmac (
.clk_i,
.rst_ni,

.valid_i (msg_write),
.valid_i (msg_write & sha_en),
.data_i (msg_fifo_wdata_endian),
.mask_i (msg_fifo_wmask_endian),
.ready_o (packer_ready),
Expand All @@ -280,7 +302,7 @@ module hmac (

.hmac_en,

.reg_hash_start,
.reg_hash_start (hash_start),
.reg_hash_process (packer_flush_done), // Trigger after all msg written
.hash_done (reg_hash_done),
.sha_hash_start,
Expand Down Expand Up @@ -341,8 +363,35 @@ module hmac (
.devmode_i (1'b1)
);

//===========================================================================
// Assertions, Assumptions, and Coverpoints
// HMAC Error Handling ======================================================
logic msg_push_sha_disabled, hash_start_sha_disabled;
assign msg_push_sha_disabled = msg_write & ~sha_en;
assign hash_start_sha_disabled = reg_hash_start & ~sha_en;

// Update ERR_CODE register and interrupt only when no pending interrupt.
// This ensures only the first event of the series of events can be seen to sw.
// It is recommended that the software reads ERR_CODE register when interrupt
// is pending to avoid any race conditions.
assign err_valid = ~reg2hw.intr_state.hmac_err.q &
( msg_push_sha_disabled | hash_start_sha_disabled );

always_comb begin
err_code = NoError;
case (1'b1)
msg_push_sha_disabled: begin
err_code = SwPushMsgWhenShaDisabled;
end
hash_start_sha_disabled: begin
err_code = SwHashStartWhenShaDisabled;
end

default: begin
err_code = NoError;
end
endcase
end

// Assertions, Assumptions, and Coverpoints =================================
//
// HMAC assumes TL-UL mask is byte-aligned.
`ifndef VERILATOR
Expand Down Expand Up @@ -370,7 +419,7 @@ module hmac (
logic initiated;
always_ff @(posedge clk_i or negedge rst_ni) begin
if (!rst_ni) initiated <= 1'b0;
else if (reg_hash_start) initiated <= 1'b1;
else if (hash_start) initiated <= 1'b1;
else if (reg_hash_process) initiated <= 1'b0;
end

Expand All @@ -379,7 +428,7 @@ module hmac (
`ASSERT(ValidWriteAssert, msg_fifo_req |-> !in_process, clk_i, !rst_ni)

// `hash_process` shall be toggle and paired with `hash_start`.
`ASSERT(ValidHashStartAssert, reg_hash_start |-> !initiated, clk_i, !rst_ni)
`ASSERT(ValidHashStartAssert, hash_start |-> !initiated, clk_i, !rst_ni)
`ASSERT(ValidHashProcessAssert, reg_hash_process |-> initiated, clk_i, !rst_ni)

// between `hash_done` and `hash_start`, message FIFO should be empty
Expand Down
6 changes: 6 additions & 0 deletions hw/ip/hmac/rtl/hmac_pkg.sv
Expand Up @@ -83,4 +83,10 @@ package hmac_pkg;
calc_w = w_0 + sum0 + w_9 + sum1;
endfunction : calc_w

typedef enum logic [31:0] {
NoError = 32'h 0000_0000,
SwPushMsgWhenShaDisabled = 32'h 0000_0001,
SwHashStartWhenShaDisabled = 32'h 0000_0002
} err_code_e;

endpackage : hmac_pkg

0 comments on commit 14a4312

Please sign in to comment.