[hmac] Code coverage exclusions

[martin-velay]

Description

Some code coverage holes should be added to the exclusion file:

• hmac_core.sv:
  • Branch line 192 → the default case cannot be reached as all the sel_rdata possible values are explicitly specified and covered. This should be excluded.
  • Branch line 209-225 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This non-existing “else” should be excluded.
• prim_sha2.sv:
  • Branch line 124-132 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This non-existing “else” should be excluded.
• prim_sha2_pad.sv:
  • Line 120 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.
  • Line 206 → this requires to trigger hash_start or hash_continue while SHA core is disabled and also while the state machine is in Idle mode. This is outside the specification and won’t be easy to trigger, so we may as well exclude it.
  • Line 251 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.
  • Line 251 and 257 when shaf_rready_i is equal to 0 → The StPad80 state gets entered through the StFifoReceive state, for which hash_start_i has to be 1. Outside of prim_sha2_pad, in prim_sha2, hash_start_i == 1 causes the FIFO FSM to enter the FifoLoadFromFifo state. In this state, prim_sha2 sets prim_sha2_pad’s shaf_rready_i input to 1 (through update_w_from_fifo) whenever shaf_rvalid is set. shaf_rvalid is driven by prim_sha2_pad to constant 1 in the StPad80 state. Thus shaf_rready_i == 1’b0 cannot occur and should be excluded.
  • Line 357 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.
  • Branch line 101-148 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.
  • Branch line 202-326
    • StPad80 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.
    • StLenHi and StLenLo → For the reason described above, shaf_rready_i == 1’b0 cannot occur in the StLenHi and StLenLo states. This should be excluded.
  • Branch line 348-357 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.

[vogelpi]

Thanks @martin-velay for putting this together. I've started to review this when reviewing @andrea-caforio 's PR here: #25696

hmac_core.sv:

• Branch line 192 → the default case cannot be reached as all the sel_rdata possible values are explicitly specified and covered. This should be excluded.

This one is also related to sel_rdata being a 2-bit signal (4 options) with only 3 values being defined. The default cannot be reached in simulation but synthesis won't understand this and generate the default assignment which is another, unused mux input. We should change the RTL here. And use another mux input for the default case for synthesis OR make the last statement the default (sel_rdata == SelFifo).

• Branch line 209-225 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This non-existing “else” should be excluded.

Please note that the non-existing else here is not referring to the digest size but to the hmac_en_i and sel_msglen. I agree it cannot be covered but how we usually handle this is to modify the RTL accordingly instead of adding a coverage waiver.

prim_sha2.sv:

• Branch line 124-132 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This non-existing “else” should be excluded.

I don't see an a non-existing else here. Can you please clarify what is meant?

I'll need to go through the other exclusions later.

[vogelpi]

prim_sha2_pad.sv

• Line 120 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.

This can easily be reformulated in the RTL without making anything less clear:

end else if ((digest_mode_flag_q == SHA2_384) || (digest_mode_flag_q == SHA2_512)) begin

to

end else begin // SHA2_384/SHA2_512

• Line 206 → this requires to trigger hash_start or hash_continue while SHA core is disabled and also while the state machine is in Idle mode. This is outside the specification and won’t be easy to trigger, so we may as well exclude it.

I think I agree on that one.

Line 251 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.

Reformulate RTL to just check for SHA2_256.

Line 251 and 257 when shaf_rready_i is equal to 0 → The StPad80 state gets entered through the StFifoReceive state, for which hash_start_i has to be 1. Outside of prim_sha2_pad, in prim_sha2, hash_start_i == 1 causes the FIFO FSM to enter the FifoLoadFromFifo state. In this state, prim_sha2 sets prim_sha2_pad’s shaf_rready_i input to 1 (through update_w_from_fifo) whenever shaf_rvalid is set. shaf_rvalid is driven by prim_sha2_pad to constant 1 in the StPad80 state. Thus shaf_rready_i == 1’b0 cannot occur and should be excluded.

I agree on that one.

• Line 357 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.

Reformulate RTL to just check for SHA2_256.

* Branch line 101-148  → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.

Reformulate RTL to just check for SHA2_256. Remove the default all-zero mux input for LenHi, LenLo. For Pad80 use a default which is equal to the branches above, i.e.:

          unique case (message_length_i[4:3])
            2'b 00:  shaf_rdata_o = 64'h 0000_0000_8000_0000;
            2'b 01:  shaf_rdata_o = {32'h 0000_0000, fifo_rdata_i.data[31:24], 24'h 8000_00};
            2'b 10:  shaf_rdata_o = {32'h 0000_0000, fifo_rdata_i.data[31:16], 16'h 8000};
            2'b 11:  shaf_rdata_o = {32'h 0000_0000, fifo_rdata_i.data[31: 8],  8'h 80};
            default: shaf_rdata_o = 64'h 0000_0000_8000_0000;

* Branch line 202-326
  
  * StPad80 →  any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.
  * StLenHi and StLenLo → For the reason described above, shaf_rready_i == 1’b0 cannot occur in the StLenHi and StLenLo states. This should be excluded.

Agreed!

* Branch line 348-357 → any other value than SHA2_256/384/512 is not supposed to be programmed, otherwise an interrupt is raised. This should be excluded.

Reformulate RTL to just check for SHA2_256 and MultimodeEn. Actually, I don't understand how this is not covered currently. Is the case 1 || 1 for (digest_mode_flag_q == SHA2_384) || (digest_mode_flag_q == SHA2_512) not covered or which case isn't covered?