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

[entropy_src] Fix handling of backpressure in the hardware pipeline #21846

Closed
wants to merge 4 commits into from

Conversation

h-filali
Copy link

@h-filali h-filali commented Mar 5, 2024

Copied from #21799

Now that the the noise source is no longer disabled upon the esrng FIFO filling up, the hardware pipeline is no longer handling backpressure from the conditioner appropriately. This becomes very obvious with #21787 which fixes the max rate test as well as the CS AES halt interface (a main source of backpressure in the entropy source).

This PR contains a couple of commits to counteract this:

  1. The FIFO controls are reworked and the entropy drop point is moved to before the postht FIFO to and the sample counter is updated to keep the number of samples going into the conditioner constant, independent of samples being dropped. Even though this behavior should be fine in practice, it's very hard to verify this with our DV environment as we don't (want / can't) accurately model the internals of the pipeline in the scoreboard.
  2. As an alternative, new FIFO - the distribution FIFO - is added between postht, precon, observe and bypass FIFOs. The purpose of this FIFO is to absorb backpressure from the conditioner/CS AES halt interface such that we don't have to drop samples and can keep verifying the design. The FIFO has a parametrizable depth to adjust it to pessimistic conditions if needed. With a 32-bit, 2-deep FIFO things work already pretty well. The max rate passes again with a rate of almost 90%.

This is resolves #21686 and is related to #20953.

What's still required and not yet part of this PR:

  • I need to investigate whether dropping entropy from at the input of the bypass FIFO is desirable / okay in bypass mode.
  • I need to investigate whether there is enough time between the health test done pulse and the pulse to trigger the conditioner such that last tested samples can flow from the postht FIFO through the distr FIFO and precon FIFO into the conditioner. This is critical to keep the number of bits in the conditioner fixed (a spec requirement).
  • There seems to be a kind of a deadlock situation when re-enabling the module after having used the conditioner: as the conditioner might still have some data from before the disable inside, the delayed-enable module doesn't enable the noise source, so no bits are coming in. No bits coming in means the conditioner will never run to produce the done pulse the delayed enable module is waiting for... One way out of this is to enable the module in Firmware Override: Extract and Insert mode and push in some fake randomness to manually operate the conditioner.

Me chipping in with some comments

For the first of the three opens above I would argue that it's fine dropping in front of the bypass FIFO, since if the configuration is not FIPS compliant we don't really care about dropping data (as long as it's not dropped unnecessarily).
If the bypass configuration should be FIPS compliant, then dropping at the bypass FIFO is effectively the same as dropping at the esfinal FIFO.
IMO this is a question of efficiency, so ideally drops should only happen when the esfinal FIFO is full.

With regards to the firmware override mode I would like to add that I think we need to be 100 percent sure that we are not dropping any entropy before the observe FIFO. Here is my reasoning for this.
This could be done like @andreaskurth said:
"In a top-level test that runs validation/min entropy estimation on Ibex, check that no entropy is dropped within a 4 kibit window. (This test can be added in a later, DV-focused milestone.)"
We should make sure that that none of the FIFOs before the observe FIFO drop entropy in fw_ov_mode.

Another thing to consider would be this. To model the back pressure closer to reality we might want to consider reducing the max rate from the noise source instead of reducing the number of cycles until AES from CSRNG is done and the SHA can start it's operation.

@vogelpi I think we can merge this PR (which I stole from you) now and create some issues/follow up PRs at some point in the future, what do you think?

This commit cleans up and documents the control signals for the main
FIFOs of the pipeline including the esrng, esbit, postht, precon and
esfinal FIFOs. Most of the changes simplify the code but don't alter
the behavior of the design as the used FIFO primitives already
implement the logic to not accept pushes when full internally.

However, there are some important changes that are necessary:
1. The esrng FIFO handles no backpressure anymore. This wasn't spec
   compliant.
2. The sample drop point in case of backpressure is moved to after the
   health tests in case we are not in firmware overide insert mode
   and the window counter controls are updated. This means
   in case of backpressure, samples are tested but they're not pushed
   into the postht FIFO (or the esbit FIFO in case of single-bit mode).
   The window timer doesn't increment to keep the number of samples
   ending up in the conditioner fixed, independent of backpressure.
   This is required by the spec.

Signed-off-by: Pirmin Vogel <[email protected]>
This commit switches all instances of prim_fifo_sync to use hardened
counters for the pointers.

Signed-off-by: Pirmin Vogel <[email protected]>
This commit adds a 32-bit wide distribution FIFO of configurable depth.
The FIFO is added between the postht FIFO, the observe FIFO, the bypass
FIFO and the precon FIFO. Its main purpose is to buffer entropy bits
while the conditioner is busy such that we don't have to drop entropy
bits from the hardware pipeline.

Dropping entropy bits is not a big issue per se during normal operation
as it's allowed by the spec (when done after the health tests and in a
way such that number of samples going into the conditioner is fixed).
Also, under normal operating conditions, noise source samples arrive at
very low rate and dropping bits should not be needed.

However, verifying that the `correct` entropy bits are dropped is hard
and seems impossible for our current DV environment as it requires to
very accurately model the hardware pipeline which is undesirable. Thus,
the safest approach is to add this new distribution FIFO and tune its
depth parameter to handle potential backpressure from the conditioner
such that dropping bits is not necessary.

Signed-off-by: Pirmin Vogel <[email protected]>
This is useful to reduce the backpressure in the pipeline as this leads
to entropy bits being dropped eventually which the scoreboard cannot
handle at the moment.

Signed-off-by: Pirmin Vogel <[email protected]>
@h-filali h-filali requested review from a team as code owners March 5, 2024 18:11
@h-filali h-filali requested review from marnovandermaas, moidx, andreaskurth and vogelpi and removed request for a team, marnovandermaas and moidx March 5, 2024 18:11
Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

Thanks @h-filali. I checked the diff to #21799 and I think all our comments are addressed. From my side we can merge this (or let @vogelpi) pick your fixes to his branch and merge PR #21799. We should then follow up with issues and separate PRs on the open points in #21799, which are less urgent and shouldn't block this PR IMO.

@vogelpi
Copy link
Contributor

vogelpi commented Mar 6, 2024

Thanks @h-filali for your work on this. I've taken most of your changes and re-included them into the original PR.

@andreaskurth
Copy link
Contributor

Closing as this has been integrated into #21799.

@h-filali h-filali deleted the origin/es-add-distr-fifo 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.

[entropy_src] Determine when and what FIFOs can drop entropy
3 participants