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.

Sign up for GitHub

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

Jump to bottom

keccak: add asm feature; use cpufeatures on aarch64 #24

Merged
merged 2 commits into from
Nov 13, 2022
Merged

keccak: add asm feature; use cpufeatures on aarch64 #24

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2af43c0
keccak: add `asm` feature; use `cpufeatures` on `aarch64`
tarcieri Nov 12, 2022
8e40212
keccak: expose `f1600_asm` in public API
tarcieri Nov 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion keccak/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,9 @@ categories = ["cryptography", "no-std"]
readme = "README.md"

[features]
asm = [] # Use optimized assembly when available (currently only ARMv8)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this perhaps be a cfg! attribute rather than a crate feature?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I am not sure. Application developers would probably want to have it enabled by default without any additional steps from users. It's a slightly different situation from the aes crate where configuration flags are used mostly for testing backends.

no_unroll = [] # Do no unroll loops for binary size reduction
simd = [] # Use core::simd (WARNING: requires Nigthly)
simd = [] # Use core::simd (WARNING: requires Nigthly)

[target.'cfg(target_arch = "aarch64")'.dependencies]
cpufeatures = "0.2"
Comment on lines +21 to +22
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit unfortunate this is a hard dependency on aarch64, although I couldn't figure out how to gate it better

Copy link
Member

Choose a reason for hiding this comment

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

What about gating on feature or configuration flag inside the cfg statement?

Copy link
Member Author

@tarcieri tarcieri Nov 13, 2022 •
edited

Choose a reason for hiding this comment

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

This doesn't work:

[target.'cfg(all(target_arch = "aarch64", feature = "asm"))'.dependencies]
cpufeatures = "0.2"

It prints this warning:

warning: Found feature = ... in target.'cfg(...)'.dependencies. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

This however, seems to work:

[target.'cfg(all(keccak_asm, target_arch = "aarch64"))'.dependencies]
cpufeatures = "0.2"

So that's a possible argument for using a cfg attribute for gating rather than a Cargo feature.

17 changes: 14 additions & 3 deletions keccak/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,13 @@ use core::{

#[rustfmt::skip]
mod unroll;

#[cfg(all(target_arch = "aarch64", feature = "asm"))]
mod aarch64_sha3;

#[cfg(all(target_arch = "aarch64", feature = "asm"))]
cpufeatures::new!(armv8_sha3_intrinsics, "sha3");

const PLEN: usize = 25;

const RHO: [u32; 24] = [
Expand Down Expand Up @@ -145,11 +150,17 @@ impl_keccak!(f200, u8);
impl_keccak!(f400, u16);
impl_keccak!(f800, u32);

#[cfg(not(all(target_arch = "aarch64", target_feature = "sha3")))]
#[cfg(not(all(target_arch = "aarch64", feature = "asm")))]
impl_keccak!(f1600, u64);

#[cfg(all(target_arch = "aarch64", target_feature = "sha3"))]
pub use aarch64_sha3::keccak_f1600 as f1600;
#[cfg(all(target_arch = "aarch64", feature = "asm"))]
pub fn f1600(state: &mut [u64; PLEN]) {
if armv8_sha3_intrinsics::get() {
Copy link
Member Author

Choose a reason for hiding this comment

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

@newpavlov there's not really a way to make this interface work with an init token. Does it seem ok to you?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's not ideal, but should be fine. One potential alternative is to expose an unsafe gated f1600_aarch64_asm function and do the switch inside sha3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that works. I can update the PR.

Copy link
Member

@newpavlov newpavlov Nov 13, 2022 •
edited

Choose a reason for hiding this comment

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

I am not saying a separate function is a better approach. Just floating an idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems nice to have to me, especially for the sha3 use case.

It's unsafe and #[target_feature(enable = "sha3")]-gated, which should prevent casual misuse.

aarch64_sha3::keccak_f1600(state)
} else {
keccak_p(state, u64::KECCAK_F_ROUND_COUNT);
}
}

#[cfg(feature = "simd")]
/// SIMD implementations for Keccak-f1600 sponge function
Expand Down