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

Add support for x86_64-unknown-uefi #787

Closed
wants to merge 1 commit into from

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Feb 6, 2019

Right now, ring contains a lot of long cfg clauses to determine which RNG implementation to use or if the crate should be no_std. This change simplifies them, mainly by using the fact that std in only ever used in tests or if use_heap is enabled. It also avoids using the confusing windows and unix attributes, and now makes all target decisions based on target_os.

This also fixes a bug arising from the fact that #[cfg(windows)] != #[cfg(target_os = "windows")]. Some custom targets base themselves off of the windows family of LLVM, but these targets do not have any of the Windows OS services.

The remaining issues blocking use of x86_64-unknown-uefi are:

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

6 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.1%) to 91.791% when pulling 76fdbb4 on josephlr:cfg into dd5f7fe on briansmith:master.

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

Please share the instructions for how I can run the ring tests in a simulated UEFI environment.

no_std
)]
// std is only used in tests or when gated with #[cfg(use_heap)]
#![cfg_attr(not(any(test, feature = "use_heap")), no_std)]
Copy link
Owner

Choose a reason for hiding this comment

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

The comment here isn't necessary since it's just duplicating the code, so let's remove it.

@@ -100,14 +100,14 @@ impl sealed::Sealed for SystemRandom {}
target_os = "macos",
target_os = "ios",
target_os = "fuchsia",
windows
target_os = "windows"
Copy link
Owner

Choose a reason for hiding this comment

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

How should fill_impl be implemented for UEFI targets? Please provide an implementation.

not(target_os = "macos"),
not(target_os = "ios"),
not(target_os = "fuchsia"),
not(target_os = "windows"),
Copy link
Owner

Choose a reason for hiding this comment

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

I am planning to switch this to a whitelist approach because I want to avoid using /dev/urandom whenever practical. So I think that while this may work right now, it will soon (in a matter of days) stop working.

@@ -215,7 +215,7 @@ mod urandom {

#[cfg(target_os = "redox")]
static RANDOM_PATH: &str = "rand:";
#[cfg(unix)]
#[cfg(not(target_os = "redox"))]
Copy link
Owner

Choose a reason for hiding this comment

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

What was wrong with cfg(unix) here? It seems better to me.

@briansmith
Copy link
Owner

Generally, I am happy to help people port ring to new environments, but to minimize the maintenance burden I ask that ports be reasonably complete and easy for me to test.

In the documentation for ring::rand, please add a note about how ring::rand is implemented for UEFI targets, similar to the other ones.

According to https://github.com/rust-osdev/uefi-rs/blob/master/.travis.yml, it seems like it is relatively straightforward to add UEFI as a target to Travis CI. Please do so.

@briansmith
Copy link
Owner

Also, thanks for the PR!

@josephlr
Copy link
Contributor Author

josephlr commented Feb 6, 2019

@briansmith I was thinking to have this PR only simplify the cfg attributes. I was goint to add support for UEFI in a seperate PR. Do you want me to just do both in this PR?

Also, UEFI (like SGX) has to use hardware RNG, so actually completing the port will have to wait on some of the material from #775

@josephlr
Copy link
Contributor Author

josephlr commented Feb 6, 2019

It looks like #738 adds RDRAND support should I factor that change out and add it here, or wait for that PR to land?

@briansmith
Copy link
Owner

Do you want me to just do both in this PR?

I would like to have everything together in one PR, with a separate commit for each separate thing being changed. I will probably merge them all at once.

Please add the contribution statement at https://github.com/briansmith/ring#contributing at the end of each commit's commit message. Is Google the copyright holder for these changes?

@josephlr
Copy link
Contributor Author

josephlr commented Feb 6, 2019

I would like to have everything together in one PR, with a separate commit for each separate thing being changed. I will probably merge them all at once.

Sounds good, I'll have this commit, rdrand support, and UEFI support

Please add the contribution statement at https://github.com/briansmith/ring#contributing at the end of each commit's commit message. Is Google the copyright holder for these changes?

Will do, and yes Google is still the copyright holder. I can go though a process to transfer copyright to you (if you prefer), but the projects we contribute to are normally Copyright The [project_name] Authors, avoiding any need for approval or rights transfer. This is mostly what OpenSSL/Chromium/Linux/etc... do.

I can change the Copyright 2015-2016 Brian Smith. statements to be Copyright The Ring Authors. Would that work with you?

@briansmith
Copy link
Owner

Regarding RDRAND, I am sad about the idea of relying on that, although right now I don't have a better suggestion. It would be great if you could help review the PR that adds RDRAND. It is fine by me if you want to rebase this on top of the SGX PR.

@briansmith
Copy link
Owner

Assigning the copyright would be best. Otherwise, in other files, we have Portions Copyright 2019 Google, Inc.

@josephlr josephlr changed the title Simplify conditional compilation Add support for x86_64-unknown-uefi Feb 6, 2019
@briansmith
Copy link
Owner

I'm currently planning for ring 0.17 to switch away from spin back to std::sync::Once for x86_64 targets in cpu.rs, which may have a negative impact here. If/when we do that, we'll still be open to accepting a PR for getting the -uefi target working, if something is needed.

@briansmith briansmith closed this Feb 12, 2020
@briansmith briansmith reopened this Feb 12, 2020
@josephlr
Copy link
Contributor Author

This will be fine. I think bare-metal x86 targets shouldn't need spinlocks. All we have to do is check CPUID, and that operation is idempotent. So we should just be able to use atomic load/store, where we might run cpuid twice, but that seems fine.

For reference, getrandom has to check CPUID on bare-metal x86 targets. This is our spinlock-free implementation of the check.

@josephlr
Copy link
Contributor Author

Closing this as the issues here are best solved generically for all bare-metal x86 targets.

@josephlr josephlr closed this May 22, 2020
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