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

Fix aes_shift_rows_fwd and aes_shift_rows_inv functions from latest vector spec. #281

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

mlawson-tt
Copy link
Contributor

This PR updates the aes_shift_rows_fwd and aes_shift_rows_inv functions to match the versions defined in the latest vector crypto spec after this fix: riscv/riscv-crypto@a19ae20.

These functions are not used in any scalar instructions and are not currently called anywhere, so no existing functionality should be affected, but this change is necessary for the upcoming vector crypto implementation and ensures the versions in the scalar spec match the vector crypto spec (as the Sail versions are used directly in the scalar spec).

@mlawson-tt
Copy link
Contributor Author

Fixes #280.

Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

Ideally we would never have added them in the first place if they're unused (did an old version of the scalar spec want them?..), but since they're here and will be used by vector crypto we should fix them and leave them here for it to use once added rather than introduce the churn of removing them and re-adding them with vector crypto. So these changes make sense to land from my POV, and carry zero risk given they're in dead code (and when vector crypto comes into the tree it'll be responsible for testing these as part of its instruction implementations).

@github-actions
Copy link

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 9d851a6. ± Comparison against base commit 5c12d94.

@ptomsich
Copy link
Collaborator

ptomsich commented Jul 3, 2023

@charmitro Patch #234 uses these functions, but (from what I understand) Zvkned works correctly with the current implementation. Could you review and report back?

Copy link

@charmitro charmitro left a comment

Choose a reason for hiding this comment

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

If vector-dev branch isn't to be merged soon with master. Would it be wise for this to get cherry-picked or similar to vector-dev as it seems that it's only going to be needed for my #234 ?

@mlawson-tt
Copy link
Contributor Author

@charmitro I was actually building your branch to test against anyway, so I can just merge this into your branch if it makes more sense. I just targeted master because these functions are showing up in the scalar crypto spec now, and that spec includes this file directly, so merging into master would let the scalar spec pick up the change so that the vector spec and scalar spec don't disagree on these functions (even though these functions aren't used in any scalar instructions). Seems like it'd be nice for these functions to not show up in the scalar spec at all since they're not used, but I imagine it may be a bigger pain to try remove them than to just keep including this file directly.

@Timmmm
Copy link
Collaborator

Timmmm commented Dec 16, 2024

Looks like this hasn't been merged yet, and I agree with @jrtc27 we probably shouldn't have these functions if they aren't used. However since they are there we may as well fix them. I'll merge this in a few days if nobody objects.

@Timmmm Timmmm self-assigned this Dec 16, 2024
@Timmmm Timmmm added the will be merged Scheduled to be merged in a few days if nobody objects label Dec 16, 2024
@Timmmm Timmmm merged commit 3707d0b into riscv:master Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will be merged Scheduled to be merged in a few days if nobody objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants