[cryptolib,silicon_creator] Deduplicate ibex random function #28646
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pamaury
requested review from
AlexJones0,
Razer6,
alees24,
cfrantz,
johannheyszl,
moidx and
nasahlpa
and removed request for
a team
nasahlpa
approved these changes
Nov 5, 2025
| while (!(abs_mmio_read32(ibex_base() + RV_CORE_IBEX_RND_STATUS_REG_OFFSET) & | ||
| 1)) { | ||
| } | ||
| return ibex_rnd32_read(); |
There was a problem hiding this comment.
Thanks, this looks correct to me.
Similar to the code before, ibex_rnd32_read() first checks whether randomness is available and then reads it.
This commit makes two changes: - the ibex_rnd32_read() function is moved from cryptolib to silicon_creator, the users are fixed - the rnd_uint32() function is changed to return either mcycle OR rnd32(), depending on the boot stage and OTP setting. Regarding the second change, it was previoulsy returning `mcycle + rnd32()` but not waiting for random data depending on an OTP setting. There is no good reason for that because if it waits for data from the EDN, all the health checks are already done by the csrng, meaning that adding `mcycle` does not add any value to a random variable. Also if the OTP is *not* set, it causes a read to known-invalid register using the fact that it will be 0 in this cases. Signed-off-by: Amaury Pouly <[email protected]>
The code is technically incorrect: for an empty vector, accessing the first element is undefined behaviour and this can abort if the c++ library is compiled with assertions. On the other hand, data() is well-defined for an empty vector (it just cannot be dereferenced). Signed-off-by: Amaury Pouly <[email protected]>
Signed-off-by: Amaury Pouly <[email protected]>
The code conditionally compiles from inline version of the code on the RV platform, and otherwise just declares some external symbol. This makes the code very difficult to test and mock because the OT_PLATFORM_RV32 define is only true when compiling for RISC-V but we do want to test some of this code on the host in a unittest. Since the initial motivation for this was probably just to make it inline, and we have since then enabled LTO, there is no realy reason to do this anymore. This commit moves the device implementation to ibex.c so that the header always just defines the prototypes. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
cl_deduplicate_ibex
branch
from
ebda1d5 to
382c894
Compare
|
So fixing the unittest turned into a rabbit hole: I discovered that the Annoyingly, it's very hard to create atomic commits in this situation. I have tried to split them as much as I could to make review easier. |
7 tasks
pamaury
force-pushed
the
cl_deduplicate_ibex
branch
2 times, most recently
from
91c3225 to
79cb0a2
Compare
In order to properly test the rnd library, introduce a mock for the ibex library. This allows to better test the rnd code without knowing the specific implementation. Signed-off-by: Amaury Pouly <[email protected]>
Properly test the mcycles and rnd32 functions. Signed-off-by: Amaury Pouly <[email protected]>
Now that the ibex library has a proper mock, we can more thorougly test the uart library. Signed-off-by: Amaury Pouly <[email protected]>
Currently the library exports a very generic method to search for a bitmask in the DT and it requires users to manually poke at the pwrmgr registers. This is not really in the spirit of the silicon_creator libraries and it can be misused. This commit makes the search function internal and create a specific method to enable the watch reset request, which is the only user currently. Signed-off-by: Amaury Pouly <[email protected]>
This pwrmgr library is used in the watchdog unittest so it is useful to create a mock to abstract from the details of how the pwrmgr works. Signed-off-by: Amaury Pouly <[email protected]>
pamaury
force-pushed
the
cl_deduplicate_ibex
branch
from
d516312 to
573173c
Compare
|
thanks re |
|
I am aware of #28666 but this is a somewhat independent issue: the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes two changes:
Regarding the second change, it was previoulsy returning
mcycle + rnd32()but not waiting for random data depending on an OTP setting. There is no good reason for that because if it waits for data from the EDN, all the health checks are already done by the csrng, meaning that addingmcycledoes not add any value to a random variable. Also if the OTP is not set, it causes a read to known-invalid register using the fact that it will be 0 in this cases.Partially addresses 28645
Unfortunately, while doing this change, I realized that the unittest situation of several libraries in the
silicon_creatorcode is not ideal. This fundamentally stems from the fact thatibex.cis mocked by making calls to the OS's clocks. It also exports some clock symbols which are unrelated but turn out to be used by other unittests! I decided to properly mockibex.cusing the Google test library. This turned into a bit of rabbit hole because once I removed thekClock*symbols fromibex_host.c, several unittest did not compile anymore and also required proper mocking.