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

rdrand: Add 32-bit x86 support #134

Merged
merged 1 commit into from
Feb 20, 2020
Merged

rdrand: Add 32-bit x86 support #134

merged 1 commit into from
Feb 20, 2020

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Feb 19, 2020

The code in rdrand.rs is mostly straightforward. We just switch between using core::arch::x86_64::_rdrand64_step and core::arch::x86::_rdrand32_step. By having WORD_SIZE differ between x86_64 and x86, most of the code can remain the same while not losing any efficiency (as would be the case if we used _rdrand32_step on both platforms).

Similar to what we were already doing on x86_64, we now always test the RDRAND implementation on all x86 targets. I also improved our testing coverage, we now run tests on the following targets:

  • x86_64-unknown-linux-gnu
  • x86_64-unknown-linux-musl
  • i686-unknown-linux-gnu
  • i686-unknown-linux-musl

This is possible as 64-bit Linux supports running 32-bit binaries.

Remaining questions

Right now we just assume that we will always have CPUID. This is true for all currently supported Rust x86 targets (as CPUID has been around for ~27 years). However, in an ideal world, we would use has_cpuid before invoking __cpuid, but that function isn't stable (and will likely never be stable).

Is it fine to just assume CPUID is always supported (on non-SGX targets)? Or should we do something else to handle the case of CPUID not existing.

@josephlr josephlr changed the base branch from master to 0.2 February 19, 2020 08:52
@josephlr josephlr force-pushed the x32 branch 3 times, most recently from a97b5bd to 0b6d683 Compare February 19, 2020 18:09
Also add tests for 32-bit x86
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Sounds okay to me.

I'm not aware that we should support pre-Pentium targets. I guess if anyone feels differently they should make an argument for why we should.

@josephlr
Copy link
Member Author

I'm not aware that we should support pre-Pentium targets. I guess if anyone feels differently they should make an argument for why we should.

Ya, it would be nice if we had a way to detect CPUID support based on the build target, but that doesn't work consistently across arches. Something like #[cfg(target_feature = "tsc")] should work in theory, but most x86/x86_64 targets don't explicitly have +tsc.

Given that only suuuuper old targets don't have CPUID, I think it's fine if they get a UD exception when running this crate. I'll be shocked if this ever happens.

@josephlr josephlr merged commit 2e39004 into rust-random:0.2 Feb 20, 2020
@josephlr josephlr deleted the x32 branch February 21, 2020 07:17
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.

3 participants