Skip to content

Conversation

@vogelpi
Copy link
Contributor

@vogelpi vogelpi commented Nov 28, 2025

This interface adds considerable design and verification complexity without substantial gain: The two crypto primitives serialized by this interface (unmasked AES core inside CSRNG, unmasked SHA3 inside ENTROPY_SRC) amount to less than 40 kGE together. Much more logic than this got removed through the CSRNG restructuring effort.

At the same time, the interface is the main source of back pressure inside the ENTROPY_SRC thereby introducing the need for additional logic to aborb this back pressure. For this reason, the interface was anyway disabled for Darjeeling which uses a high-rate noise source.

This commit removes the interface completely from the top levels and the two involved hardware block IPs CSRNG and ENTROPY_SRC.

This resolves #28819.

This PR is based on top of #28804 and will be rebased once the PR is merged. Please only review the last commit.

@vogelpi vogelpi requested review from a team as code owners November 28, 2025 13:17
@vogelpi vogelpi force-pushed the remove-cs-aes-halt-if branch 2 times, most recently from c3a0ac9 to 8d3e178 Compare November 28, 2025 13:24
@vogelpi vogelpi force-pushed the remove-cs-aes-halt-if branch 3 times, most recently from ec5e4ef to 7a20c20 Compare November 28, 2025 18:46
Copy link
Contributor

@glaserf glaserf 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 this PR which really removes some further complexity from the entropy complex. This looks good to me, I only have some very minor comments.

I also checked out your branch to search the repo for any leftovers of the AES halt interface. I only found the following (link to master such that it won't die if you rebase your branch):

Verify that when AES_HALT is set during a generate command that no request is sent to the AES block.

@vogelpi vogelpi force-pushed the remove-cs-aes-halt-if branch from 7a20c20 to 077d3a9 Compare December 2, 2025 17:49
@vogelpi
Copy link
Contributor Author

vogelpi commented Dec 2, 2025

Thanks for your reviews @glaserf and @phani-lowrisc , I've now addressed your comments.

This interface adds considerable design and verification complexity
without substantial gain: The two crypto primitives serialized by this
interface (unmasked AES core inside CSRNG, unmasked SHA3 inside
ENTROPY_SRC) amount to less than 40 kGE together. Much more logic than
this got removed through the CSRNG restructuring effort.

At the same time, the interface is the main source of back pressure
inside the ENTROPY_SRC thereby introducing the need for additional logic
to aborb this back pressure. For this reason, the interface was anyway
disabled for Darjeeling which uses a high-rate noise source.

This commit removes the interface completely from the top levels and the
two involved hardware block IPs CSRNG and ENTROPY_SRC in order to reduce
complexity.

This resolves lowRISC#28819.

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
This is a follow-up of 357d41a .

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
@vogelpi vogelpi force-pushed the remove-cs-aes-halt-if branch from 077d3a9 to 027ef83 Compare December 2, 2025 23:22
@Razer6
Copy link
Member

Razer6 commented Dec 4, 2025

@vogelpi Does this have impact on the size of the distribution FIFO of Darjeeling? Giving that it's entropy source runs at a different rate?

@vogelpi
Copy link
Contributor Author

vogelpi commented Dec 4, 2025

@vogelpi Does this have impact on the size of the distribution FIFO of Darjeeling? Giving that it's entropy source runs at a different rate?

No it doesn't. Because Darjeeling did disable this interface (with a parameter) for the exact same reason. When disabling the parameter, I lowered the depth of the FIFO from 26 to 11 entries.

@vogelpi
Copy link
Contributor Author

vogelpi commented Dec 4, 2025

CHANGE AUTHORIZED: hw/ip/csrng/data/csrng.hjson
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_block_encrypt.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_core.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/data/entropy_src.hjson
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src_core.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src_enable_delay.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src_pkg.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

This PR reduces overall complexity by removing a feature which has no clear benefit. It has been discussed in the Silicon and Integrated WG meeting on 2025-11-25. This is fine.

@Razer6
Copy link
Member

Razer6 commented Dec 5, 2025

CHANGE AUTHORIZED: hw/ip/csrng/data/csrng.hjson
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_block_encrypt.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_core.sv
CHANGE AUTHORIZED: hw/ip/csrng/rtl/csrng_ctr_drbg.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/data/entropy_src.hjson
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src_core.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src_enable_delay.sv
CHANGE AUTHORIZED: hw/ip/entropy_src/rtl/entropy_src_pkg.sv
CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

This PR reduces overall complexity by removing a feature which has no clear benefit. It has been discussed in the Silicon and Integrated WG meeting on 2025-11-25. This is fine.

@Razer6 Razer6 added this pull request to the merge queue Dec 5, 2025
Merged via the queue into lowRISC:master with commit 3663d93 Dec 5, 2025
47 of 48 checks passed
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.

[csrng, entropy_src] Consider removing the CS AES halt interface

4 participants