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 feature detection on FreeBSD/aarch64 (WIP) #611

Merged
merged 1 commit into from Dec 12, 2018

Conversation

valpackett
Copy link
Contributor

@valpackett valpackett commented Dec 8, 2018

On AArch64, FreeBSD does not currently provide HWCAP in the ELF auxiliary vector.
Reads of the ID registers are trapped though, and they are accessible in userspace.

  • The register reading code could work on other OSes and even bare metal, so it's separated out into its own module.
  • Seems like it could also be used on Linux.
  • TODO: The OS-dependent code should trap SIGILL before calling the reading function, in order to work on older OS versions that didn't expose the registers. I guess it's fine™ to expect that to work.
  • TODO: The OS-dependent code should run the reading function on all cores (via thread affinity) and take a union of features, because there are weird SoCs where cores have unequal features. On FreeBSD, the register view is the same on each core

References:

@lu-zero
Copy link
Contributor

lu-zero commented Dec 8, 2018

How old are we talking regarding old oses? Trapping signals is quite bad for libraries.

@valpackett
Copy link
Contributor Author

valpackett commented Dec 8, 2018

@lu-zero well, older than FreeBSD 12 (which is currently in release candidate phase). The number of people who use Rust + FreeBSD + AArch64 is currently quite small, especially considering that rustc currently doesn't build on FreeBSD/aarch64 without a very recent patch that hasn't landed yet :) So it should be fine to ignore that.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'm trying to verify the implementation against online docs but I'm getting lost pretty quickly unfortunately. This looks though like it's based on another implementation perhaps?

stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 8, 2018 via email

@valpackett
Copy link
Contributor Author

valpackett commented Dec 8, 2018

I’d like to review this with calm tomorrow afternoon

Of course :)

a CPU that “doesn’t know” the instruction being executed

Every aarch64 CPU knows mrs with these arguments, they're just privileged instructions, and most (?) OSes trap them to allow them to succeed in userspace.

I can see how trying to detect the SIMD/etc instructions themselves via signal handling can be unreliable, yes.

Would it be possible to get CI set up for aarch64-FreeBSD?

I have a public buildbot..

@valpackett
Copy link
Contributor Author

Update: no need to read the values on different cores, the kernel already presents the same view of the registers on all cores https://reviews.freebsd.org/D17137#393947 So this is good as is, pretty much. I can add some code comments about this I guess.

@alexcrichton
Copy link
Member

FWIW I personally think that we shouldn't try to trap instructions here, if FreeBSD handles this and "makes the instruction work" that's fine by me and if we're not compatible with older versions that's probably ok for now too

stdsimd/arch/detect/mod.rs Outdated Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Outdated Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Outdated Show resolved Hide resolved
cfg_if! {
if #[cfg(all(target_arch = "aarch64", any(target_os = "freebsd")))] {
#[path = "os/aarch64.rs"]
mod aarch64;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer to move this into the cfg_if! below:

cfg_if! { 
   ...
   } else if #[cfg(target_os = "freebsd")] {
        #[cfg(target_arch = "aarch64")]
        #[path = ...]
        mod aarch64;
        #[path = "os/freebsd/mod.rs"]
        mod os;
   } 
   ...
}

When we need to use aarch64 from somewhere else, we can figure out how to do that, but there is no need to do so here.

Copy link

@parched parched left a comment

Choose a reason for hiding this comment

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

Nice work @myfreeweb, I just have a few minor comments.

stdsimd/arch/detect/os/aarch64.rs Outdated Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Outdated Show resolved Hide resolved
stdsimd/arch/detect/os/aarch64.rs Show resolved Hide resolved
@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 10, 2018

The register reading code could work on other OSes and even bare metal, so it's separated out into its own module.

So I think this is a good idea. We do the same for x86 where we have it as its own "OS", I think it is worth it to do this for aarch64 as well. Since this is part of libstd , it is unlikely that people will use it directly on bare metal in the near future, but there is some interest to do this with x86 as well and there are some ways to do that (e.g. split those into different no_std crates that people can include without pulling in std, portability check, etc.).

Seems like it could also be used on Linux.

IIRC msr was a privileged instruction on Linux, but maybe I remember wrong?

TODO: The OS-dependent code should run the reading function on all cores (via thread affinity) and take a union of features, because there are weird SoCs where cores have unequal features. On FreeBSD, the register view is the same on each core

stdsimd is used in libstd, so it requires OS support, we do have some workarounds over some specific known CPU bugs, but we don't have any workarounds that assume an "adversary" OS that tries to break your process.

On Linux, we ask the OS "What is this process allowed to do?" via the OS interfaces (e.g. auxiliary vectors). If Linux decides to migrate the process to a different core, it has to make sure that the core is at least as capable as the one we are running on because we can cache the answer to the question. If the FreeBSD way of querying this information is via the msr register, then they have to make sure about that too.

When the OS fails, all bets are off anyways. For example, see A tale of big.LITTLE gone wrong, where Samsung Exynos aarch64 LITTLE cores had atomic support but the big cores did not (usually big cores are "better" than little cores, and the OS assumed that all SoCs were like this). So the Apps would start in LITTLE cores, do run-time feature detection, detect that they can use atomics, cache that, start using them, and some time after the OS would move them to a big core and the process would SIGILL because the big cores did not support atomics. Working around "bugs" like these here feels like trying to do too much: we'd need OS APIs that allow us to query the set of potential cores the process might be moved to, APIs to query the features of these cores, and then select the minimum subset which the process supports, which might be very sub-optimal if the OS never decides to move the process to a different core.

TODO: The OS-dependent code should trap SIGILL before calling the reading function, in order to work on older OS versions that didn't expose the registers. I guess it's fine™ to expect that to work.

What's the recommended FreeBSD way of handling this ? We don't support Windows+aarch64 but the recommended approach to do run-time feature detection there is to "try to use an intrinsic, and use SEH to handle the error if its not available" (see #120 (comment)) . I agree with @lu-zero and @alexcrichton that we should avoid this if possible.

@parched
Copy link

parched commented Dec 10, 2018

Seems like it could also be used on Linux.

IIRC msr was a privileged instruction on Linux, but maybe I remember wrong?

Yes this works on Linux now too, since version 4.11.

Whether MRS is privileged depends on what register is being read, i.e., only ones ending with '_EL0' can possibly be read directly from userspace. EL1 ones like these cause an exception and must be emulated by the kernel if it wants (normally it will abort the process with SIGILL or similar).

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 10, 2018

Yes this works on Linux now too, since version 4.11.

Cool! We should explore using this in Linux then, it is way easier than using auxiliary vectors.

@valpackett
Copy link
Contributor Author

If the FreeBSD way of querying this information is via the msr register, then they have to make sure about that too

Yep, the emulated msr only returns features supported by all cores. (Looks like you replied before I updated the original post.)

see A tale of big.LITTLE gone wrong

That's where I linked with the "weird SoCs where cores have unequal features" text :)

What's the recommended FreeBSD way of handling this ?

Similar to Windows, I guess. For an API that's literally "run a privileged instruction and expect it to be emulated", not much you can do other than trapping SIGILL.

But we can just ignore this. The FreeBSD/aarch64 userbase is small enough that "just upgrade your OS to >=12" is fine :)

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 12, 2018

But we can just ignore this. The FreeBSD/aarch64 userbase is small enough that "just upgrade your OS to >=12" is fine :)

We can leave adding support for FreeBSD == 11 to "future work". If someone complains, I'd guess we can just disable this with a config flag or by querying the FreeBSD version somehow later.

This is looking good, just a couple of nits open (don't forget to run cargo fmt --all since that is the only CI failure)!

@valpackett
Copy link
Contributor Author

Updated with comments and suggested cfg.

cargo fmt --all

The CI rustfmt failure is:

error: component 'rustfmt' for target 'x86_64-unknown-linux-gnu' is unavailable for download

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

LGTM

@gnzlbg gnzlbg merged commit 28eff1a into rust-lang:master Dec 12, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 12, 2018

Thank you a lot @myfreeweb !

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.

None yet

5 participants