Skip to content
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

[testutils] make entropy_testutils work for darjeeling #26269

Merged
merged 4 commits into from
Feb 14, 2025

Conversation

nbdd0121
Copy link
Contributor

This involves the entropy_src specific part of to a new entropy_src_testutils.

Fix #26193

@nbdd0121 nbdd0121 requested a review from pamaury February 12, 2025 18:09
@nbdd0121 nbdd0121 requested a review from a team as a code owner February 12, 2025 18:10
@moidx moidx requested review from jadephilipoom and vogelpi and removed request for a team February 12, 2025 19:18
@nbdd0121 nbdd0121 requested a review from AlexJones0 February 13, 2025 11:48

#define MODULE_ID MAKE_MODULE_ID('e', 'n', 'y')

status_t entropy_testutils_entropy_src_init(void) {
#ifdef HAS_ENTROPY_SRC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of an open question: should this function be moved to the entropy_src testutils? I would expect that users of the entropy complex that don't specifically care of the entropy source will use entropy_testutils_auto_mode_init or entropy_testutils_boot_mode_init (which call this function anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There're at least 2 tests that I know that needs this for earlgrey while also exists in integrated_dev, which is why I decided to keep it entropy_testutils.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let me reformulate the question: what is this function supposed to achieve that cannot be done by using one of the other initialization functions? It seems its purpose is to initialize specifically the entropy_src so maybe those tests should be switch to of the more general init functions? Can you point to those tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's ast_clk_rst_inputs.c which uses this function, but taking another look it seems that master and integrated_dev has diverged a bit, so probably having conditional compilation inside itself might make more sense.

Another example is entropy_src_csrng_test which mostly tests CSRNG.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on that one:

  • entropy_src_csrng_test really just tests CSRNG and for this the entropy_src needs to be configured, too. On Darjeeling, we'll simply not configure ENTROPY_SRC.
  • ast_clk_rst_inputs tests some features of AST including whether it consumes entropy from EDN (not affected by this code) and whether it outputs raw entropy. The latter is checked by looking at the fill status of the observe FIFO inside ENTROPY_SRC. For Darjeeling the test needs to adapted in a way to check that CSRNG can be reseeded with "raw" entropy from AST.

Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nbdd0121 for working on this. The changes all make sense to me. I agree with @pamaury and you on the open discussion point. This function should probably also be moved and then we need to adapt the tests for Darjeeling.


#define MODULE_ID MAKE_MODULE_ID('e', 'n', 'y')

status_t entropy_testutils_entropy_src_init(void) {
#ifdef HAS_ENTROPY_SRC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on that one:

  • entropy_src_csrng_test really just tests CSRNG and for this the entropy_src needs to be configured, too. On Darjeeling, we'll simply not configure ENTROPY_SRC.
  • ast_clk_rst_inputs tests some features of AST including whether it consumes entropy from EDN (not affected by this code) and whether it outputs raw entropy. The latter is checked by looking at the fill status of the observe FIFO inside ENTROPY_SRC. For Darjeeling the test needs to adapted in a way to check that CSRNG can be reseeded with "raw" entropy from AST.

Copy link
Contributor

@jadephilipoom jadephilipoom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, also agree that moving the entropy_src specific function makes sense.

This initializes entropy src with default configuration.
`entropy_testutils_config_default` is removed.

The idea is that this function will become a no-op for darjeeling when
the user only needs the entropy src to be initialized but doesn't need
special config.

The old `entropy_testutils_config_default`, for most users, are
immediately passed to initialize entropy_src, with only a single user
tweaking it. This user is updated to just explicitly list all config,
and other users are updated to call the new initialization function
instead.

Signed-off-by: Gary Guo <[email protected]>
…_check

This function is still needed for darjeeling, but it doesn't have an
entropy_src block and DIF cannot be used. This function is updated to
implicitly check for the entropy_src internally, so it can work for both
earlgrey and darjeeling.

Signed-off-by: Gary Guo <[email protected]>
There's a part of entropy_testutils that is dedicated to entropy_src,
rather than the entropy generation as a whole. This is now moved to
entropy_src_testutils, allowing tops that does not have entropy_src to
continue to use entropy_testutils.

Signed-off-by: Gary Guo <[email protected]>
@nbdd0121 nbdd0121 merged commit 8594ab8 into lowRISC:master Feb 14, 2025
34 of 40 checks passed
@nbdd0121 nbdd0121 deleted the entropy branch February 14, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[multitop] Abstract entropy/EDN
4 participants