[hmac, dv] Update coverage exclusion file

[andrea-caforio]

This commit removes the coverage exclusions that were introduced as part of the padding cancellation bug #23936 that was fixed in the previous commit. New exclusions are added that cover some impossible conditions and branches as identified in #24692.

[martin-velay]

I think everything mentioned here #24692 has been taken over. Thanks Andrea ! What does the code coverage report say ?

[andrea-caforio]

I think everything mentioned here #24692 has been taken over. Thanks Andrea ! What does the code coverage report say ?

These are the percentages with the new exclusions and the padding cancellation fix, there is definitely an improvement but I don't have the exact numbers for the Master branch. I'll run a regression for the Master branch over night to have a full comparison.

[martin-velay]

According to the V3 signoff doc, we should reach 100% for:

Line, toggle, fsm (state & transition), branch and assertion code coverage has reached 100%.

Can you check if this is the case in your report?

[andrea-caforio]

According to the V3 signoff doc, we should reach 100% for:

Line, toggle, fsm (state & transition), branch and assertion code coverage has reached 100%.

Can you check if this is the case in your report?

There are only 3 lines which appear uncovered but the branches that cause them are in the exclusion file because they are unreachable.

• opentitan/hw/ip/prim/rtl/prim_sha2_pad.sv

  Line 133 in db75ea1

  shaf_rdata_o = '0;

• opentitan/hw/ip/prim/rtl/prim_sha2_pad.sv

  Line 317 in db75ea1

  st_d = StLenHi;

• opentitan/hw/ip/prim/rtl/prim_sha2_pad.sv

  Line 330 in db75ea1

  st_d = StLenLo;

[andrea-caforio]

This is the only branch not covered:

opentitan/hw/ip/prim/rtl/prim_sha2.sv

Line 362 in 162a845

fifo_st_d = FifoIdle;

[martin-velay]

Which is alright as that's the default case. But I wonder why the UNR is not excluding it.

[andrea-caforio]

Which is alright as that's the default case. But I wonder why the UNR is not excluding it.

It might be because the UNR file is out of date. I'm looking at the coverage report log and there are many warnings of the form:

Warning-[UCAPI-EL-EIVI] Invalid Branch Vector
/home/dev/src/hw/ip/hmac/dv/cov/hmac_unr_excl.el, 512
  Ignoring invalid vector ID '2' of branch '16' in module 
  'tb.dut.u_prim_sha2_512.gen_multimode_logic.u_prim_sha2_multimode' specified
  in the exclude file. This situation may occur if some 
  out-dated/inappropriate exclusion file is used.
  Please regenerate the exclude file.

[martin-velay]

Yes, we definitely need an updated version of the UNR !

[vogelpi]

Thanks for doing this @andrea-caforio . I see the PR is inline with the recommendations in #24692 . But to be honest, I believe we should modify the RTL instead of adding coverage waivers for quite a few of these exclusions. Let's discuss in the issue first.

[martin-velay]

Hi @andrea-caforio and @vogelpi,
After fixing some DV issues in the TB, the FSM transition StLenHi -> StIdle from the file prim_sha2_pad.sv was not covered anymore. I have spend some time to try to cover it again. But it turns out that this transition is visible on some rare occasions and after the transition, the StIdle state is there only on very short time (not even a clock cycle) as it goes quickly into StLenLo. I tried to wait until we are StLenHi and then trigger a reset as this action would have bring the sha_en_i to its reset value which 0 and would result in having the state transitioning from StLenHi -> StIdle. But after having added an assertion I noticed that this is almost impossible as the StLenHi state is never longer than 1 clock cycle.
Do you see any way of changing the RTL easily to help me to cover this or would you be happy to exclude this transition hole?

[andrea-caforio]

Hi @andrea-caforio and @vogelpi, After fixing some DV issues in the TB, the FSM transition StLenHi -> StIdle from the file prim_sha2_pad.sv was not covered anymore. I have spend some time to try to cover it again. But it turns out that this transition is visible on some rare occasions and after the transition, the StIdle state is there only on very short time (not even a clock cycle) as it goes quickly into StLenLo. I tried to wait until we are StLenHi and then trigger a reset as this action would have bring the sha_en_i to its reset value which 0 and would result in having the state transitioning from StLenHi -> StIdle. But after having added an assertion I noticed that this is almost impossible as the StLenHi state is never longer than 1 clock cycle. Do you see any way of changing the RTL easily to help me to cover this or would you be happy to exclude this transition hole?

Thank you for investigating this. One possibility could be to disallow the transition StLenHi -> StIdle altogether by changing the FSM, but this might lead to runaway errors later, where the effects of sha_en_i == 0 are deferred and not immediate.

What is the reason this transition was covered before you fixed the DV issues?

[martin-velay]

Unfortunately, I don't have the answer why the transition was seen before and why it's not anymore, it looks very odd.

[vogelpi]

This is strange @martin-velay . I think we should not change the RTL here nor should we exclude this. It's obviously reachable and we have covered this before. Being in he StIdleSt for less than one clock cycle doesn't really make sense unless it's done via the reset.

But the fact that this transition was covered before suggests that we can cover this not by playing with the reset. What is needed is the sha_en_i signal going low while we're in the StLenHi state. This signal is ultimately driven by the register interface (i.e., software). What makes things complicated is that for a software write access to go though and update the register, we must either see a reg_hash_done or a reg_hash_stop pulse. The former is coming out of hmac_core (probably harder to control) while the latter is again software driven. What you probably need is software providing a stop command followed by an access setting sha_en back to 0. Does this sound reasonable?

I really wouldn't modify the RTL here because there is a risk of leaving prim_sha2_pad in StLenHi forever which would be critical.

[martin-velay]

What you probably need is software providing a stop command followed by an access setting sha_en back to 0. Does this sound reasonable?

I don't know why this signal is never longer than 1 clock cycle. Here was the the assertion I added into the RTL file to try to find it out, which never failed during a full regression:

ASSERT(DEBUG_MVy, st_d == StLenHi |=> st_d != StLenHi)

I am not sure I understand how your proposal may help, but let's talk about it on Monday in the office. Otherwise maybe I can also perform backdoor register write in a directed test to cover this transition. But I agree with you changing the RTL is not something I'd go for neither.

[andrea-caforio]

I ran a full regression based on Master. Two more ->StIdle transitions are uncovered.

@martin-velay I remember these transitions were covered by #25568. Maybe the latest changes in DV have nullified it.

[andrea-caforio]

@vogelpi @martin-velay

I updated the coverage exclusion file with the two missing else statements that were discussed in #24692.
Including the three FSM default states that should eventually be excluded when the UNR file is regenerated (see this comment), hmac, prim_sha2_32, prim_sha2 and prim_sha2_pad are now at 100% line, FSM and branch coverage in the VCS regression.

[martin-velay]

Excellent! I feel that D3/V3 are getting close 😍
BTW, I have also updated this file on the master to add some assertions, can you rebase please @andrea-caforio

[vogelpi]

LGTM, thanks @andrea-caforio !