[csrng/rtl] csrng move cmd checks into the cmd stage #22906
Merged
Conversation
This file contains 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
h-filali
force-pushed
the
csrng-move-cmd-checks
branch
from
005e96e
to
d2eeed2
Compare
vogelpi
approved these changes
Apr 30, 2024
| @@ -47,7 +47,7 @@ This process repeats until the `glen` field value has been matched. | |||
| Because each request is pipelined, requests from other `cmd_stage` blocks can be processed before the original generate command is completely done. | |||
| This provides some interleaving of commands since a generate command can be programmed to take a very long time. | |||
|
|
|||
| When sending an unsupported or illegal command, `CS_MAIN_SM_ALERT` will be triggered, but there will be no status response or indication of which app the error occurred in. | |||
| When sending an unsupported or illegal command, `CMD_STAGE_INVALID_ACMD_ALERT` will be triggered, but there will be no status response or indication of which app the error occurred in. | |||
There was a problem hiding this comment.
I think the status response is still sent out on the corresponding app interface...
There was a problem hiding this comment.
I renamed the alert because it doesn't come from the main SM anymore. I think maybe we can discuss this in person.
| // Here we send an illegal command to CSRNG to check that cs_main_sm_alert is triggered. | ||
| // Sending an illegal command does not get a response from CSRNG. | ||
| // Here we send an illegal command to CSRNG to check that CMD_STAGE_INVALID_ACMD_ALERT is | ||
| // triggered. Sending an illegal command does not get a response from CSRNG. |
There was a problem hiding this comment.
Don't we ack the command and flag an error in the status signal?
h-filali
force-pushed
the
csrng-move-cmd-checks
branch
2 times, most recently
from
0c82155
to
95da1c1
Compare
|
@vogelpi thanks for reviewing. I rebased and addressed your comments. Let's discuss some points in person and then commence from there. |
h-filali
force-pushed
the
csrng-move-cmd-checks
branch
5 times, most recently
from
2b16016
to
8ee1c9d
Compare
Having the command checks in the main SM doesn't make a lot of sense. This causes a lot of issues including a hang condition inside the cmd stage in case of generate commands with cmd len greater than 1. For this reason it is best to not even start processing commands that violate our checks. Signed-off-by: Hakim Filali <[email protected]>
h-filali
force-pushed
the
csrng-move-cmd-checks
branch
from
8ee1c9d
to
fc67229
Compare
vogelpi
requested review from
matutem and
timothytrippel
and removed request for
a team
|
Thanks for rebasing @h-filali . I am merging this and for addressing the feedback on the implementation. The remaining comments relate to code comments and we can discuss this next week. I am merging this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Please see the commit message for a description.
I ran a regression with 0.1 multiplier and a few more tests than usual are failing (~5 more than expected).
I didn't have time to debug all of them but I think this PR is ready to at least be looked at.
depends on #22883