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.
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
[hmac,sw] Workaround after hang #24839
[hmac,sw] Workaround after hang #24839
Changes from all commits
ec4c9d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am no expert on this but maybe it would be cleaner to allow passing a NULL pointer here in which case the function will not try save the context in case the hardware needs to be recovered? This would allow calling the function without context in case we just need HMAC to be idle (See second call below). Maybe @jadephilipoom can give some guidance here, we may use a different approach in the cryptolib compared to other code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could simply return
OTCRYPTO_RECOV_ERR
here and exit, and then look for that case in the function that calls this one and run therecover_hw_after_stop()
operation there. Then you wouldn't need to passctx
to this function at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do so, the function will stop here and won't execute this remaining piece of code?
I think we should execute those read and write operations after the HW has been recovered or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to add a return here? Can the
status
value here get overwritten when checking thehmac_done
bit below?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exact comparison will fail to trigger if
CRYPTO_STATUS_DEBUG
is set on the command line, since then every instance ofOTCRYPTO_RECOV_ERR
includes the line number on which the macro was called. It would be more robust to usestatus_extract
and compare the resultingcode
:opentitan/sw/device/lib/base/status.h
Line 221 in da20a58
It's just a tool for test debugging, so maybe not the most critical, but could make debugging harder. I'd be okay with not blocking the PR and creating an issue to fix this. There's more background on how cryptolib handles status codes here: https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/crypto/impl/status.h