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

crypto-intrinsics #730

Closed
wants to merge 2 commits into from
Closed

crypto-intrinsics #730

wants to merge 2 commits into from

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Feb 6, 2022

With the forthcoming stabilization of inline assembly, I thought we could start putting together a crate which provides wrappers for assembly instructions which are useful for cryptography but lack proper core::arch wrappers.

More importantly, using inline assembly allows us to provide a sort of black box which LLVM will not interfere with, which is problematic using anything else besides ASM. For example, it's otherwise not possible to correctly emit CMOV instructions on x86 platforms with LLVM because the x86-cmov-conversion pass which will rewrite them as branches. For more details, see:

https://dsprenkels.com/cmov-conversion.html

@tarcieri
Copy link
Member Author

tarcieri commented Feb 6, 2022

ADX and MULX are also good candidates, and particularly problematic to emit outside of inline assembly due to LLVM bugs:

rust-lang/stdarch#666

Namely LLVM isn't aware of the relevant flags for these instructions and either clobbers them or doesn't properly emit them.

@tarcieri tarcieri marked this pull request as draft February 6, 2022 18:58
@tarcieri tarcieri force-pushed the crypto-intrinsics branch 3 times, most recently from f6acf38 to 1b16dc8 Compare February 6, 2022 19:05
Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Interesting. Is it feasible for those intrinsics to be added to core::arch eventually? Also I wonder about fragility of flags used with ADX/MULX. If we are to chain several calls to such intrinsics, are we sure that compiler can not reorder some intrinsics clobbering the flags between those calls?

crypto-intrinsics/src/x86_64.rs Show resolved Hide resolved
@tarcieri tarcieri force-pushed the crypto-intrinsics branch 2 times, most recently from 2811bcd to a1b0fba Compare February 6, 2022 19:14
@tarcieri
Copy link
Member Author

tarcieri commented Feb 6, 2022

Is it feasible for those intrinsics to be added to core::arch eventually?

I think the only way would be if they used inline assembly themselves. In fact I requested as much and they seemed skeptical.

If we are to chain several calls to such intrinsics, are we sure that compiler can not reorder some intrinsics clobbering the flags between those calls?

I think it still relies on careful inspection of the generated assembly, but really the important thing IMO is that LLVM does not insert additional instructions that clobber the flags, as otherwise I think the data dependencies should take care of reordering which would affect the flags.

///
/// This function requires support for the Intel BMI2 (Bit Manipulation
/// Instruction Set 2) extension to the x86 instruction set.
#[target_feature(enable = "bmi2")]
Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to add this for MULX, as it requires BMI2.

I tried to do the same thing with ADX, but unfortunately:

error[E0658]: the target feature `adx` is currently unstable
  --> crypto-intrinsics/src/x86_64.rs:26:18
   |
26 | #[target_feature(enable = "adx")]
   |                  ^^^^^^^^^^^^^^

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 looks like a PR to stabilize ADX stalled some time in 2019: rust-lang/rust#60109

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened a PR to stabilize the ADX target feature here: rust-lang/rust#93745

dbl/src/lib.rs Show resolved Hide resolved
@tarcieri tarcieri changed the title [WIP] crypto-intrinsics crypto-intrinsics Feb 26, 2022
Now that inline assembly has been stabilized in Rust 1.59, I thought we
could start putting together a crate which provides wrappers for
assembly instructions which are useful for cryptography but lack proper
`core::arch` wrappers.

More importantly, using inline assembly allows us to provide a sort of
black box which LLVM will not interfere with, which is problematic using
anything else besides ASM. For example, it's otherwise not possible to
correctly emit CMOV instructions on x86 platforms with LLVM because the
`x86-cmov-conversion` pass which will rewrite them as branches. For
more details, see:

https://dsprenkels.com/cmov-conversion.html
Bumping the clippy version to 1.59 exposed some new warnings
@tarcieri tarcieri marked this pull request as ready for review February 26, 2022 17:30
@tarcieri
Copy link
Member Author

Removed draft/WIP and slimmed the initial implementation down to just cmovz and cmovnz on x86 and x86_64 targets.

Namely I removed all ADX/MULX support as we need a solution which supports carry chains emitted as a single asm! block in order to ensure flags don't get clobbered by LLVM (which is the whole point of using ADX/MULX at all).

Potentially in the future we can add some more of the CMOVcc family, and try to come up with something like a macro solution for ADX/MULX chains which emits an asm! macro with a fully formed set of assembly instructions that comprise the entire carry chain.

If it winds up this crate only implements CMOV though, I've gone ahead and snagged the cmov crate as well, and we could potentially rename this crate to that.

@tarcieri
Copy link
Member Author

tarcieri commented Feb 26, 2022

I'm going to open a separate PR for a simple cmov crate, since that's the only functionality currently exposed. That might be a better place to start to keep things simple.

@newpavlov
Copy link
Member

Can you update the CI workflow with cargo cache and minimal versions job? The latter is less needed since the crate does no have any dependencies, but I guess it's better to keep workflows consistent with each other.

keywords = ["crypto", "cmov", "intrinsics"]
readme = "README.md"
edition = "2018" # Can't bump to 2021 due to pre-1.56 MSRV crates in the same workspace
rust-version = "1.59"
Copy link
Member

@newpavlov newpavlov Feb 26, 2022

Choose a reason for hiding this comment

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

Wouldn't it be better to use 2021 edition and the separate workspace trick for now? It's already done for the inout crate.

Copy link
Member Author

@tarcieri tarcieri Feb 26, 2022

Choose a reason for hiding this comment

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

That approach is really annoying from a maintenance perspective, since each crate gets its own Cargo.lock and therefore needs its own dependabot config (although that's admittedly not really a problem here).

I'd rather attempt the "delete the toplevel Cargo.toml" trick if we were to do that, but this crate is so simple it doesn't really derive any benefits from being 2021 edition.

@tarcieri
Copy link
Member Author

Here's a cmov-only crate as an alternative. I'm liking this as a reasonable place to start:

#741

@tarcieri
Copy link
Member Author

Closing in favor of #741

@tarcieri tarcieri closed this Feb 26, 2022
@tarcieri tarcieri deleted the crypto-intrinsics branch February 26, 2022 18:36
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.

2 participants