search

lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.59k stars 782 forks source link

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

Closed GregAC closed 4 months ago

GregAC commented 5 months ago

Shortly before finishing writing this I realised I need to factor in this PR as well: https://github.com/lowRISC/ibex/pull/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: b79e491e33 - Edit: this commit is not on master but rather on a branch / fork. The corresponding commit on master is 9f43e67210314c47f7ec991bba4d82f49c281e4c

08f19b0ed7 [rtl,rv_core_ibex] Mask off lower bits of mapping address 092400ac01 [rv_core_ibex] Reduce number of PRINCE rounds in ICache for timing ad5f7b947b Update lowrisc_ibex to lowRISC/ibex@eea2bf0c 8295903d7e [bazel,autogen_hjson] Split C and rust header generation 30d7e787c7 (tag: Earlgrey-PROD-M2-RC5, tag: Earlgrey-PROD-M2) Add the project name to the copyright header f4c2bb9030 Remove trailing whitespaces 6766fc8a0f [rtl, rv_core_ibex] Add alert assertion for lockstep reset count b402d1cc7a [rv_core_ibex,lint] Update lint waiver with lockstep reset changes 56399241e7 Revert "[edn] Move prim_edn_req out of prim" 61a237e197 [util/reggen] reverse order of substruct generation fc8484601e [reggen,hw] Create index parameter for registers windows ce648ca68e [ipgen.pwrmgr] Change core files to vlnv naming and label as virtual b2e29a57a1 [rv_core_ibex] Update AscentLint waiver for newly added onehot muxes 818f1fd870 [rv_core_ibex] Connect refgile onehot checker asserts 3b4e36e01c [edn] Move prim_edn_req out of prim de31bdf1c2 [reggen] Remove the devmode input 963a5006cc [doc] Minor tweak to md sanitisation code ca2b62b28a [dv, testplan] Replace descr by desc for consistency 1ce014fd67 [hw,ibex,rtl] Change fatal alert to for RW1S register 619ce83526 [rv_core_ibex, sival] Extend rv_core_ibex testplan with SiVal tests 1b16ca2122 [reggen] Add mubi support SWAccess that sets/clears a reg 59f8142826 [doc] Moved badges over to using hosted images 70843207c1 [doc] rv_core_ibex registers and interfaces now use CMDGEN 7688e714e8 [reggen] Add initial support for version and cip_id hjson fields fbd888eea8 Revert "[reggen] Add CIP_IDs and bump all major versions" 0ba10b3cd3 [reggen] Add CIP_IDs and bump all major versions 4e615dedac [rtl/rv_core_ibex] Fix RTL assertion for CDC

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

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

3 of these effect functionality

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

ad5f7b947b is a lowRISC vendoring update and changes it introduces are covered below. 092400ac01 is covered by existing tests and 08f19b0ed7 is covered by pending PR https://github.com/lowRISC/opentitan/pull/23532

Of the changes that had occurred at the D2S sign-off only 1ce014fd67 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 1ce014fd67 fixed the bug, it is now re-enabled: https://github.com/lowRISC/opentitan/blob/ca6e918cee40e3fe69b76e75b9caaef7023028f8/hw/top_earlgrey/dv/env/seq_lib/chip_sw_all_escalation_resets_vseq.sv#L52

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

Closed at D2S sign-off

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

Open issues

Earlgrey relevant

Darjeeling specific

Tagged as future release

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)

Closed issues

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 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 (https://github.com/lowRISC/ibex/issues/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 commented 5 months ago

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 commented 5 months ago

Delta report with most recent PR now merged:

Review commit: lowrisc/ibex@d019dccb4b

New commits:

The new prim_ram_1p_scr has new security hardening that uses MUBI encoding for various internal signals: https://github.com/lowRISC/opentitan/commit/856e402f740d40dbdb06013320f1697670154fc0 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 commented 5 months ago

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 commented 5 months ago

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 https://github.com/lowRISC/opentitan/pull/23532 to be merged to test https://github.com/lowRISC/opentitan/commit/08f19b0ed76c9f2329d54707e2abd108442a08c7?

marnovandermaas commented 5 months ago

Created an issue for the test here: https://github.com/lowRISC/opentitan/issues/23569 Please edit if you think it needs to be captured differently.

GregAC commented 5 months ago

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:

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: https://github.com/lowRISC/opentitan/issues/18421

GregAC commented 5 months ago

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.

rswarbrick commented 5 months ago

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 commented 5 months ago

@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: https://github.com/lowRISC/opentitan/pull/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 commented 5 months ago

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:

GregAC commented 5 months ago

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

vogelpi commented 5 months ago

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 commented 4 months ago

Here is the PR vendoring Ibex back in once more: https://github.com/lowRISC/opentitan/pull/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 https://github.com/lowRISC/ibex/issues/2187 and then we can do the sign off.

vogelpi commented 4 months ago

Alright, I've now reviewed @GregAC 's regression report in https://github.com/lowRISC/ibex/issues/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 https://github.com/lowRISC/ibex/issues/2186.

Delta report compared to https://github.com/lowRISC/opentitan/issues/23542#issuecomment-2154629103

Review commit: lowRISC/Ibex@668233699df9ec2a40413e69e0de0a5b10185980

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:

In OpenTitan, the following commits have been merged in hw/ip/rv_core_ibex since 9f43e67210314c47f7ec991bba4d82f49c281e4c 9a18359abb [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:

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 commented 4 months ago

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 commented 4 months ago

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