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

[csrng/rtl] Make the main SM error test signal fatal #23657

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

h-filali
Copy link

The main SM error test signal is currently the only test signal where the actual error would be fatal but the test signal is not. This commit make adds the err_code_test_bit for the main SM error to fatal_loc_events to make the main SM error test fatal.

@h-filali h-filali requested a review from vogelpi June 13, 2024 09:07
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 for flagging this @h-filali . This looks good to me!

The fix is very small and fully aligns EDN, CSRNG and ENTROPY_SRC with respect to error signaling. I think we should absorb this change but we'll need to discuss this with other leads.

@vogelpi
Copy link
Contributor

vogelpi commented Jun 13, 2024

CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_core.sv

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

Is there a reason not to use main_sm_err_sum here? That way, we don't have the magic "21" index in two different places.

@h-filali
Copy link
Author

Is there a reason not to use main_sm_err_sum here? That way, we don't have the magic "21" index in two different places.

Yes, there is a reason. main_sm_err_sum causes a loop, since it is set to high by the error signal that comes out of the main SM. And if main_sm_err_sum is high, it in turn causes the main SM error signal to go high. There is a comment above the lines I changed which tries to explain this. Should I maybe make the comment more clear?

@rswarbrick
Copy link
Contributor

rswarbrick commented Jun 13, 2024

Oh, now I understand the comment above. Maybe it would be worth tweaking it slightly to be something like this?

"Rather than using man_err_sum for the main state machine error, we use the error code test bit (index 21), which avoids causing a loop."

But this looks sensible otherwise. Thanks for the response.

UPDATE: ... and now I've read the comment before. Maybe the right fix is change that last sentence to something like:

"To include the state machine error at all, we use the error code test bit (index 21) directly."

@rswarbrick
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_core.sv

The main SM error test signal is currently the only test signal where
the actual error would be fatal but the test signal is not.
This commit make adds the err_code_test_bit for the main SM error to
fatal_loc_events to make the main SM error test fatal.

Signed-off-by: Hakim Filali <[email protected]>
@h-filali h-filali force-pushed the csrng-make-sm-err-test-fatal branch from 1420243 to 959542f Compare June 13, 2024 11:50
@h-filali
Copy link
Author

@rswarbrick I updated the comment following your suggestion. Thanks for your feedback!

Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@vogelpi vogelpi merged commit b447bf7 into lowRISC:master Jun 13, 2024
2 checks passed
@vogelpi vogelpi mentioned this pull request Jun 25, 2024
@h-filali h-filali deleted the csrng-make-sm-err-test-fatal branch October 7, 2024 14:11
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.

3 participants