[rv_core_ibex, dv] V2(S) sign-off

[GregAC]

Shortly before finishing writing this I realised I need to factor in this PR as well: lowRISC/ibex#2170 which is not yet merged into Ibex but will be part of the M4 RTL. (This has since been merged.)
I'll do a little delta update to this to take it into account.

Commits since ES

OpenTitan Review commit: b79e491 - Edit: this commit is not on master but rather on a branch / fork. The corresponding commit on master is 9f43e67

08f19b0 [rtl,rv_core_ibex] Mask off lower bits of mapping address
092400a [rv_core_ibex] Reduce number of PRINCE rounds in ICache for timing
ad5f7b9 Update lowrisc_ibex to lowRISC/ibex@eea2bf0c
8295903 [bazel,autogen_hjson] Split C and rust header generation
30d7e78 (tag: Earlgrey-PROD-M2-RC5, tag: Earlgrey-PROD-M2) Add the project name to the copyright header
f4c2bb9 Remove trailing whitespaces
6766fc8 [rtl, rv_core_ibex] Add alert assertion for lockstep reset count
b402d1c [rv_core_ibex,lint] Update lint waiver with lockstep reset changes
5639924 Revert "[edn] Move prim_edn_req out of prim"
61a237e [util/reggen] reverse order of substruct generation
fc84846 [reggen,hw] Create index parameter for registers windows
ce648ca [ipgen.pwrmgr] Change core files to vlnv naming and label as virtual
b2e29a5 [rv_core_ibex] Update AscentLint waiver for newly added onehot muxes
818f1fd [rv_core_ibex] Connect refgile onehot checker asserts
3b4e36e [edn] Move prim_edn_req out of prim
de31bdf [reggen] Remove the devmode input
963a500 [doc] Minor tweak to md sanitisation code
ca2b62b [dv, testplan] Replace descr by desc for consistency
1ce014f [hw,ibex,rtl] Change fatal alert to for RW1S register
619ce83 [rv_core_ibex, sival] Extend rv_core_ibex testplan with SiVal tests
1b16ca2 [reggen] Add mubi support SWAccess that sets/clears a reg
59f8142 [doc] Moved badges over to using hosted images
7084320 [doc] rv_core_ibex registers and interfaces now use CMDGEN
7688e71 [reggen] Add initial support for version and cip_id hjson fields
fbd888e Revert "[reggen] Add CIP_IDs and bump all major versions"
0ba10b3 [reggen] Add CIP_IDs and bump all major versions
4e615de [rtl/rv_core_ibex] Fix RTL assertion for CDC

Of these 6 have occurred since the D2S sign-off:

08f19b0 [rtl,rv_core_ibex] Mask off lower bits of mapping address
092400a [rv_core_ibex] Reduce number of PRINCE rounds in ICache for timing
ad5f7b9 Update lowrisc_ibex to lowRISC/ibex@eea2bf0c
8295903 [bazel,autogen_hjson] Split C and rust header generation
30d7e78 (tag: Earlgrey-PROD-M2-RC5, tag: Earlgrey-PROD-M2) Add the project name to the copyright header
f4c2bb9 Remove trailing whitespaces

3 of these effect functionality

08f19b0 [rtl,rv_core_ibex] Mask off lower bits of mapping address
092400a [rv_core_ibex] Reduce number of PRINCE rounds in ICache for timing
ad5f7b9 Update lowrisc_ibex to lowRISC/ibex@eea2bf0c

ad5f7b9 is a lowRISC vendoring update and changes it introduces are covered
below. 092400a is covered by existing tests and 08f19b0 is covered by
pending PR #23532

Of the changes that had occurred at the D2S sign-off only 1ce014f resulted in
a functional change and this is covered by the chip_sw_all_escalation_resets
chip level test. The relevant bit of which had been disabled until 1ce014f fixed the bug, it is
now re-enabled:

opentitan/hw/top_earlgrey/dv/env/seq_lib/chip_sw_all_escalation_resets_vseq.sv

Line 52 in ca6e918

'{"*rv_core_ibex*sw_fatal_err*", TopEarlgreyAlertIdRvCoreIbexFatalSwErr},

Ibex repository commits

Ibex review commit: lowRISC/ibex@5977d4e3
Ibex ES commit: lowRISC/ibex@1120e8dd

The Ibex core itself is vendored in from the Ibex repository. Changes in that
repository that related to the Ibex RTL also need to be considered. Those
changes that effect RTL files are below:

lowRISC/ibex@5977d4e3 [rtl] Guard against false memory responses for secure configurations
lowRISC/ibex@eea2bf0c [rtl] Expose ICacheScrNumPrinceRoundsHalf parameter
lowRISC/ibex@c1139477 Add missing copyright headers
lowRISC/ibex@27dd6b2e [rtl] Update use of prim_count following port changes
lowRISC/ibex@5a8a1a99 [tracer] Fix reporting of load/store data
lowRISC/ibex@8ec0c6f1 [rtl] Harden lockstep enable against FI
lowRISC/ibex@56413ecf [icache] Disable S&P diffusion layer in memory scrambling
lowRISC/ibex@35bbdb7b [rtl] Fix FI vulnerability in RF
lowRISC/ibex@d097c918 [rtl] Avoid name collision in ibex_pmp.sv
lowRISC/ibex@fe84d64d [verilator] Slight refactor in ibex_tracer to avoid BLKSEQ warning
lowRISC/ibex@bac72d96 [ibex_pmp/lint] Declare functions before using them
lowRISC/ibex@1084ac11 [dv] Add asserts to check alerts for memory integrity failures

Of these 3 have occurred since the D2S sign-off:

lowRISC/ibex@5977d4e3 [rtl] Guard against false memory responses for secure configurations
lowRISC/ibex@eea2bf0c [rtl] Expose ICacheScrNumPrinceRoundsHalf parameter
lowRISC/ibex@c1139477 Add missing copyright headers

Only lowRISC/ibex@5977d4e3 has any functional RTL impact. There is no test for
the new security hardening measure it introduces however the functionality it
covers is well exercised by existing tests for normal operating scenarios. A
test should be written to check the new hardening measure.

Of the changes that had occurred at the D2S sign-off lowRISC/ibex@8ec0c6f1 and
lowRISC/ibex@35bbdb7b are security hardening measures which do not have tests
for fault injection, these should be written.

Closed issues

New since D2S sign-off

This issues have been closed since the D2S sign-off

• [rv_core_ibex] Reads using address_translation modes do not work if read is in a different 64kb aligned address #22884
• RTL bug in address translation (lower bits of remap_addr field should be
  ignore), now fixed in
  [rtl,rv_core_ibex] Mask off lower bits of mapping address #23178. The chip level test for
  address translation has a pending PR to add junk to the lower bits of
  remap_addr to cover this case: [sival, rv_core_ibex] Add junk bits to address translation test #23532
• [top_earlgrey] Area optimizations for production #22025
  • Number of Ibex performance counters reduced to improve area
    ([rv_core_ibex,rtl] Reduce number of performance counters #22047). No DV impact as
    we do not test performance counters.
• [rv_core_ibex] V1 Signoff #21023
  • V1 signoff for M2
    [rv_core_ibex] D2S Signoff #20988
  • D2S sign-off for M2
• [rv_core_ibex] Switch single to multi-bit encoding for Lockstep Enable #20779
  • Make lockstep enable multi-bit encoded, usage covered by existing tests
  • No test to corrupt enable and observe alert occurs
• [rv_core_ibex,RAM] Setup violation from the RAMs #19782
  • Timing fix with rv_core_ibex RTL impact was reducing the number of
    scrambling rounds in ICache memory. Existing testing already covers this
• [rv_core_ibex] Double check whether branch-predictor should be enabled #12777
  • Decision was not to enable branch predictor, no DV impact
• [chip-test] chip_sw_rv_core_ibex_lockstep_glitch #21460
  • Old sival related tracking issue, not relevant as the test in question is not suitable for sival

Closed at D2S sign-off

These were also closed at the D2S sign-off and the comments there suffice for the
V2S signoff

• [chip-test] chip_sw_rv_core_ibex_alerts #21461
• [rv_core_ibex] Vendor in latest Ibex #21128
• [rv_core_ibex] Bit-flip in Register File Muxing #20715
• [ip,doc] Fix inaccurate references to reset behavior #20700
• [rv_core_ibex,rtl] Incorrect behavior of SW_FATAL_ERR CSR #19774
• [rv_core_ibex] Consider implementing ECC check across all ways of data RAM in ICache #11403

Open issues

Earlgrey relevant

• [rv_core_ibex] V3 Signoff #22684
  • V3 signoff, not yet achieved
• [rv_core_ibex] D3 Signoff #22647
  • D3 signoff, not yet achieved
• [rv_core_ibex,doc] Typo in documentation #20453
  • Minor documentation issue
• [rv_core_ibex] Improve V2(S) pass rates #18421
  • Still work to do to improve pass rates, but no concerns with current failures
• [chip, dv, cov] Address low coverage of alert_dump and crashdump signals #15427
  • Not done, though this area is already covered by chip-level tests and
    assertions. Extra coverage would bring limited value
• [rv_core_ibex,dv] Integrate the Ibex "core_ibex" tests with dvsim #11794
  • Integrating Ibex regressions into dvsim based flow, not required for M4 or
    V2S
• [rtl/rstmgr] Make cpu and alert info layout known to sw #8945
  • Software/tooling related
• [chip-test] chip_sw_rv_core_ibex_alerts #16838
  • Would give some extra checking around alerts from Ibex but may not add new coverage

Darjeeling specific

• [rv_core_ibex] Suggested Address Translation improvements #20896
• [top_darjeeling] Wrong ePMP reset values #20489

Tagged as future release

• [rv_core_ibex/top, rtl] Regularise use of timer interrupt #18093
• [rv_core_ibex] Consider ICache pinning #14510
• [tlul] How PutFullData vs PutPartialData should be used #1867

Ibex repository issues

A detailed review of issues in the Ibex repository has not been conducted due to
the large number of issues, most of which are not relevant to OpenTitan. A brief
review has been conducted and issues of note are blow

Open Issues

(Issues opened within the last year have been looked at only)

• [dv] Directed test to check PoR values of CSRs ibex#2057
  • PoR value of all CSRs should be checked, not a major concern for M4/V2S
• [dv, fcov] Look at use of U mode in riscv_mem_intg_error_test ibex#2053
  • Inefficient test that requires many iterations to hit certain bins, not a
    major concern for M4/V2S

Closed issues

• Unexpected PoR value of mstatus ibex#2054
  • Bad PoR value for mstatus, now fixed

Regression Results

The latest Ibex regression result (which was against the reviewed commit lowRISC/ibex@5977d4e3) is available at: https://ibex.reports.lowrisc.org/opentitan/ibex-regr-report-06-06-2024.html

Overall pass rate is 92.7%, coverage results are:

Functional	Block	Branch	Statement	Expression	Toggle	FSM	Assertion
93.7%	95.7%	90.3%	95.8%	90.5%	97.3%	100.0%	98.1%

These maintain V2(S) standards.

A representative regression result for ES is https://ibex.reports.lowrisc.org/opentitan/ibex-regr-report-10-05-2023.html (run against ES commit lowRISC/ibex@1120e8dd)
Overall pass rate was 93.3%, coverage results were:

Functional	Block	Branch	Statement	Expression	Toggle	FSM	Assertion
93.6%	94.6%	90.5%	94.6%	91.4%	96.6%	100.0%	98.1%

So coverage has remained very similar to ES.

Failures between the two sets of results have been compared and there are no new signatures in the latest regression.

Summary

Some new minor security hardening features have been added that should be
covered by a test that checks a fault injection triggers an assert:

• lowRISC/ibex@8ec0c6f1
• lowRISC/ibex@35bbdb7b
• lowRISC/ibex@5977d4e3

lowRISC/ibex@8ec0c6f1 and lowRISC/ibex@35bbdb7b utilize existing primitives that are well tested, so only the connectivity that triggers the alert would be an area of concern.
lowRISC/ibex@5977d4e3 relates to Ibex specific micro-architectural concerns so doesn't use standard countermeasures, however the security issue it relates to (lowRISC/ibex#2144) is of low concern and had a software workaround. It has been observed working correctly in ad-hoc testing.

Tests should be written here but I think there's low risk of undiscovered issues so don't think it should gate M4.

Other than these missing tests there is nothing else that would effect V2(S) status.

I feel it would be appropriate to maintain V2(S) here with a waiver that these tests will be written.

[GregAC]

Realised I left two issues under 'follow-up', they're SiVal test tracking issues, just wanted to check what their current status is as it wasn't clear from the issue

[GregAC]

Delta report with most recent PR now merged:

Review commit: lowRISC/ibex@d019dccb4b

New commits:

• lowRISC/ibex@d019dcc
  • DV only change, fixing lint
• lowRISC/ibex@957529c
  • Add new RTL to fusesoc core files required due to changes below
• lowRISC/ibex@21da9b3
  • Vendor in latest OT IP
• lowRISC/ibex@5cea5d6
  • Wire up new alert port from prim_ram_1p_scr
• lowRISC/ibex@0b0b010
  • Add new ports from latest prim_ram_1p_scr (outputs which aren't used in Ibex)

The new prim_ram_1p_scr has new security hardening that uses MUBI encoding for various internal signals: 856e402 which these commits introduce into Ibex.

These internal signals will be well exercised by existing Ibex testing (as well as being tested elsewhere in OpenTitan). One gap is the need for a test to inject faults and check for an alert from the new hardening. As with the other coverage gaps around countermeasures discussed above I think this is minor enough that we can consider it waived for V2(S) and should not gate M4, but should be completed for M5.

Latest regression is here: https://ibex.reports.lowrisc.org/opentitan/ibex-regr-report-07-06-2024.html

94.1% pass rate, coverage is:

Functional	Block	Branch	Statement	Expression	Toggle	FSM	Assertion
93.9%	95.8%	90.4%	95.9%	90.5%	96.8%	100.0%	98.1%

There's no new signatures in the failures.

[rswarbrick]

I've read through the analysis and I think I agree. A "short-term" thing that might be worth doing is to spec out the tests you describe and add them to the testplan. That way, we'll have obviously visible gaps (that we can waive) and I think everything ends up feeling a bit more robust.

Other than that, I'm happy that the current status be considered V2S.

[marnovandermaas]

Just read through this and I'm happy to close this issue once we create another issue under M5 to track:

One gap is the need for a test to inject faults and check for an alert from the new hardening. As with the other coverage gaps around countermeasures discussed above I think this is minor enough that we can consider it waived for V2(S) and should not gate M4, but should be completed for M5.

Do we have to wait for #23532 to be merged to test 08f19b0?

[marnovandermaas]

Created an issue for the test here: #23569
Please edit if you think it needs to be captured differently.

[GregAC]

A "short-term" thing that might be worth doing is to spec out the tests you describe and add them to the testplan.

As the Ibex testplan pretty much just points at the test list, rather than add dummy/placeholder tests to that (that then need to be removed from regressions etc) I've added some issues to cover the tests that describe the functionality:

• [rv_core_ibex,dv] Extending existing chip_sw_rv_core_ibex_lockstep_glitch test to cover lockstep control faults #23572
• [rv_core_ibex] New Ibex tests to cover security hardening #23573
• [dv] Implement spurious memory response test ibex#2175
• [dv] Implement fault injection test for hardening in register file ibex#2174
• [dv] Implement fault injection tests for prim_ram_1p_scr MUBI values ibex#2173

I'll also plan to properly document all of the current failures in the Ibex testbench and explain why they're of low concern, this is tracked by: #18421

[GregAC]

I've looked at the two issues I'd marked as 'follow-up'. One could be closed the other could be investigated in M5 but isn't crucial (not clear it provides any extra new coverage, existing tests may suffice). I've updated the issue to reflect this.

• [chip-test] chip_sw_rv_core_ibex_lockstep_glitch #21460 - Now closed
• [chip-test] chip_sw_rv_core_ibex_alerts #16838 - To consider if it's worth implementing in M5.

[rswarbrick]

If I understand correctly, the required tests are implemented in the Ibex PRs that you linked above. Of course, they haven't landed yet (nor have they been vendored into OpenTitan). We definitely cannot claim V2S until the required testing has actually landed on this side.

For arguing that the "M4 side of things" is solid enough, I suggest that you get the PRs merged on the Ibex side and (possibly manually) run a regression at your end showing that they work correctly. Maybe do that and take a screenshot? Then the only remaining task for this issue (rather than the M4 milestone) is to do the vendoring update.

[GregAC]

@rswarbrick they're not PRs, they're just issues explaining the tests that need to be written (As we don't have an Ibex testplan that exists in the same way as other OpenTitan IPs do so you can't do a doc-only style PR to add un-written tests to the testplan).

There are no outstanding PRs that require merging, the delta report above links the relevant regression. This has been vendored into OpenTitan here: #23575

Would you prefer we leave this open and just move it to the M5 milestone then? I.e. we don't waive the small gaps (documented by the new issues) for V2(S)?

[rswarbrick]

Ah, I see: sorry for misunderstanding. I guess that if we believe there are tests that should eventually be written and haven't yet been written, we probably shouldn't claim V2S status. Maybe the best thing is to:

• Claim V2 (by filing a PR to drop the current V2S to V2 in rv_core_ibex.hjson)
• Keep this issue open, but bumped to the M5 milestone
• Write some tests :-)

[GregAC]

Ok works for me I'll open the PR and move this to M5

[vogelpi]

Thanks for putting this together @GregAC . Regarding the currently untested security countermeasures: we do have the chip_sw_rv_core_ibex_lockstep_glitch TLT to test the lockstep countermeasure. I think the easiest thing to do would be extending this test to also cover the other countermeasures.

[vogelpi]

Here is the PR vendoring Ibex back in once more: #23937
There are just DV changes, some of them in the RTL files but they are not synthesized. In addition, I also wanted to study Greg's regression report here lowRISC/ibex#2187 and then we can do the sign off.

[vogelpi]

Alright, I've now reviewed @GregAC 's regression report in lowRISC/ibex#2187 and can confirm that the observed issues are not concerning but rather well understood testbench / co-simulation issues which are in most cases tedious to fix. There was one RTL bug identified which isn't critical and for which a workaround exists, see lowRISC/ibex#2186.

Delta report compared to #23542 (comment)

Review commit: lowRISC/ibex@6682336

New commits:
lowRISC/ibex@66823369 [dv] Add spurious responses to memory agent
lowRISC/ibex@0e0f27ad [dv] Add riscv_ram_intg_test
lowRISC/ibex@3384bf4c [cosim] Clang lint fix
lowRISC/ibex@e1f2df24 [ci] Bump co-sim version
lowRISC/ibex@470b39a2 [dv] Output warning message on problematic MIP changes
lowRISC/ibex@65a7231a [cosim] Correctly deal with checking top of range memory accesses
lowRISC/ibex@e784d274 [dv] Update testbench to use new 'pre_val' MIP
lowRISC/ibex@39648048 [dv] Fix model mismatches in cases where an access crosses PMP regions
lowRISC/ibex@89f4d867 [dv] Fix exception_stall_instr_cross illegal bins
lowRISC/ibex@2c132113 [dv] Add riscv_rf_ctrl_intg_test
lowRISC/ibex@e2b721d4 [ci] update private CI
lowRISC/ibex@1449ed5e [dv] Add cover points for memory interface behaviour
lowRISC/ibex@604ba343 [dv] Fix race condition in ibex_mem_intf_agent
lowRISC/ibex@d97a0b4a [doc] Fix C++ style guide link in README

The commits contain DV and doc changes only, including many fixes to improve test pass rates.

Latest regression is here: https://ibex.reports.lowrisc.org/opentitan/ibex-regr-report-05-07-2024.html

97.1% pass rate, coverage is:

Functional	Block	Branch	Statement	Expression	Toggle	FSM	Assertion
94.3%	95.9%	90.6%	95.9%	90.8%	97.2%	100.0%	98.1%

There have two new signatures been identified:

• The first one revealed an possible but uncritical RTL bug for which a workaround exists, for details see [rtl] Behaviour of debug single stepping with dummy instruction insertion may be surprising ibex#2186.
• The second one is an old testbench / co-simulation bug which was thought to be fixed [dv] riscv_debug_ebreakmu_test: Core did not enter debug mode after execution of ebreak ibex#1313 - also uncritical.

In OpenTitan, the following commits have been merged in hw/ip/rv_core_ibex since 9f43e67
9a18359 [rv_core_ibex] Waive uncritical AscentLint errors

There is one oustanding PR to re-vendor Ibex with the latest DV changes here: #23937

In terms of issues the state is as follows:

• The issues mentioned in this comment have all been addressed and closed by implementing the additional tests.
• [chip-test] chip_sw_rv_core_ibex_alerts #16838 has been moved to M7 as part of V3.
• [chip-test] chip_sw_rv_core_ibex_lockstep_glitch #21460 has been closed as the mentioned test has already been implemented.

Summary

To summarize, the items identified previously to be taken care of before we can sign off Ibex at V2S have all been taken care of. Pass rates have been improved further and the test failures are well understood. The risk of critical RTL bugs is deemed low.

There has one possible bug been identified which is considered uncritical and for which a software workaround exists, see lowRISC/ibex#2186.

I thus recommend signing of Ibex at V2S once the latest vendor PR #23937 has been merged. What do you think @andreaskurth , @GregAC , @rswarbrick ?

[andreaskurth]

I thus recommend signing of Ibex at V2S once the latest vendor PR #23937 has been merged. What do you think @andreaskurth , @GregAC , @rswarbrick ?

Agreed, thanks for this summary @vogelpi, and thanks @GregAC for your analysis above and all the great work you did on Ibex!

[vogelpi]

Yep, I agree: Great work from @GregAC , thanks for all this!