-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Entropy Stack update #153
base: ot-earlgrey-9.2.0
Are you sure you want to change the base?
Entropy Stack update #153
Conversation
fc891c7
to
5b61a89
Compare
5b61a89
to
53d3431
Compare
53d3431
to
5de8cd8
Compare
Updated EDN: GENERATE command header was not parsed in |
It's going to take me a while to mentally digest all of the changes in this PR, so a review from me might take a bit, but I don't want to delay this too much. Maybe we could occasionally do review meetings where we go over the code together? |
|
Some progress since last draft:
|
5de8cd8
to
b5013e5
Compare
It might be worth splitting this PR once entropy test are passing. Entropy tests depend on so many IPs that is difficult to predict the proper set of IPs to update for now. |
b5013e5
to
057b9fe
Compare
This departs from QEMU's fifo8 implementation, but enables to use the OT FIFO from a const context. Signed-off-by: Emmanuel Blot <[email protected]>
…blic Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
057b9fe
to
1540612
Compare
|
c5f0586
to
cf2d9cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with the EDN and CSRNG and so can't provide a thorough review on the implementation specifics (I did find the CSRNG changes easier to review though); I've left some comments on things I noticed going over the code. On the whole the changes all look great, however, and it's nice to see more EDN/CSRNG tests passing.
Aside: it is surprising to me to see entropy_src_kat_test
failing, since I have it noted as passing - I went and ran it on master
on OpenTitan and to my surprise it is failing! Going back to an older OpenTitan commit 32b1337496
and running ./bazelisk.sh test //sw/device/tests:entropy_src_kat_test_sim_qemu_rom_with_fake_keys
it clearly is passing, so there has been a regression at some point, though not in this PR (so it shouldn't block merging). I would guess that the implementation of this test changed during the Multitop top-level-test porting efforts, in a way that is incompatible with the QEMU model (or maybe QEMU was bumped since then and a previous change broke this)?
This enables runnning applications built for Verilator simulator on QEMU. Signed-off-by: Emmanuel Blot <[email protected]>
…xecution. Tracing execution massively slows down OTBN execution. Add a new option to trace execution, while leaving logfile option to only report execution errors when trace logging is not enabled. Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
OT_SENSOR IP is no longer present on Darjeeling Signed-off-by: Emmanuel Blot <[email protected]>
OT Sensor IP is top-specific Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
A new command should never be emitted if there is a previous one on going. Remove useless command buffer wipes. Signed-off-by: Emmanuel Blot <[email protected]>
EDN_ENABLE is not a boolean, but a multibit boolean. Signed-off-by: Emmanuel Blot <[email protected]>
…mentation Signed-off-by: Emmanuel Blot <[email protected]>
…icit class name QEMU assumes that the class name should be built from the instance name, however this is not even used constantly across QEMU files. Only when abstract classes are used this auto naming scheme is applied. Redefine two similar macros (from the original ones) to provide the actual object class name. Signed-off-by: Emmanuel Blot <[email protected]>
This naming better follows other OT class naming conventions. Signed-off-by: Emmanuel Blot <[email protected]>
…ble implementation Signed-off-by: Emmanuel Blot <[email protected]>
Upon reset the data will be invalid with a new EDN request pending. Signed-off-by: Emmanuel Blot <[email protected]>
1. inverted detection of a previous EDN connection 2. the filler_fn function now tracks whether an EDN is connected or not, the instantiation status was a left over from the initial implementation Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
When an ELF file is used to store an app in the flash image, and not binary physical file exists, the binary data is automatically generated but no match should be attempted between the ELF metadata and the generated binary content. Signed-off-by: Emmanuel Blot <[email protected]>
The documented rules 1. ENTROPY_SRC may only be disabled if CSRNG is disabled. 3. Once disabled, CSRNG may only be re-enabled after ENTROPY_SRC has been disabled and re-enabled. no longer apply: remove the related checks. Signed-off-by: Emmanuel Blot <[email protected]>
They had not been updated when registers had. Signed-off-by: Emmanuel Blot <[email protected]>
FIPS status should be reset after each entropy packet is delivered to the OTBN core. Signed-off-by: Emmanuel Blot <[email protected]>
cf2d9cc
to
b77932f
Compare
#153 (comment) not yet addressed (TBD next week). |
It turns out that the generation identifier is no longer needed with the latest Entropy Src IP. |
b77932f
to
22f23c1
Compare
Get rid of the generation identifier. This mechanism had initially be implemented to verify early EarlGrey entropy source constraint, quoting: "CSRNG may only be enabled if ENTROPY_SRC is enabled. Once disabled, CSRNG may only be re-enabled after ENTROPY_SRC has been disabled and re-enabled." This constraint has been removed in recent EarlGrey implementations while Darjeeling implementation did not follow this constraint, as it did not make use of the entropy source IP. Generation number is therefore removed; get_random_generation API is no longer needed, neither is the genid argument of get_random_values. Error management can be simplified accordingly. Signed-off-by: Emmanuel Blot <[email protected]>
Signed-off-by: Emmanuel Blot <[email protected]>
22f23c1
to
3838be9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thank you! I'm not familiar with the hardware enough to thoroughly review all the EDN changes but the code all seems reasonable and more tests are passing, so this is definitely an improvement.
This update contains mostly EDN IP update which has been heavily reworked based on HW simulation (Verilator traces) rather than on EDN documentation, as it has been done for the initial implementation.
It also contains several updates and fixes for the CSRNG and EntropySrc, but those IPs still require some updates, which will be delivered in a forth coming PR.