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

Tracking issue for core::arch::{x86, x86_64}::has_cpuid #60123

Closed
gnzlbg opened this issue Apr 19, 2019 · 61 comments
Closed

Tracking issue for core::arch::{x86, x86_64}::has_cpuid #60123

gnzlbg opened this issue Apr 19, 2019 · 61 comments
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 19, 2019

This issue tracks the stabilization of the has_cpuid intrinsic.

The "mini-RFC" is part of the stabilization PR: rust-lang/stdarch#730

cc @rust-lang/libs @alexcrichton

@jonas-schievink jonas-schievink added A-SIMD Area: SIMD (Single Instruction Multiple Data) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Apr 19, 2019
@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 19, 2019
@alexcrichton
Copy link
Member

@rfcbot fcp merge

There's a longer description of what's being stablilized on the PR, so looking for an official sign-off now!

@rfcbot
Copy link

rfcbot commented Apr 19, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 19, 2019
@briansmith
Copy link
Contributor

In what circumstance would somebody call has_cpuid()? Does Rust support any x86 or amd64 targets that don't have CPUID? I've never even considered the possibility that CPUID would be unavailable.

The original RFC mentioned me as somebody who might be interested in this, but really what I'm interested in is the actual CPUID and related intrinsics.

@nagisa
Copy link
Member

nagisa commented Apr 19, 2019

Yes. SGX target is notable modern target which does not support CPUID.

@briansmith
Copy link
Contributor

Yes. SGX target is notable modern target which does not support CPUID.

Right. However, in practice we need to know "Is this SGX?" not just vaguely "Is CPUID available?" In particular, if the target is SGX then we'll assume certain features are available that we wouldn't normally assume are available by default otherwise.

FWIW, I have code that uses CPUID extensively on all non-SGX x86 and amd64 targets, but I still don't have any use for this function, AFAICT, because it doesn't provide information useful to make a decision. (I mention this because I was mentioned in the other RFC that motivated this.)

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 20, 2019

Yes. SGX target is notable modern target which does not support CPUID.

Heh, I wonder what result the EFLAGS.ID-based CPUID detection procedure returns inside an enclave.
I suspect it's still going to be true.

On a similar note, if you are calling CPUID inside a VMX-based hypervisor, it will be rerouted to the hypervisor and the hypervisor can do whatever it wants in theory, including eating your laundry :)

@dtolnay
Copy link
Member

dtolnay commented Apr 20, 2019

Registering Brian's point above in #60123 (comment). Do we know of someone with a use case that benefits from this function? How did it come to be added originally?

FWIW, I have code that uses CPUID extensively on all non-SGX x86 and amd64 targets, but I still don't have any use for this function, AFAICT, because it doesn't provide information useful to make a decision.

@rfcbot concern not useful

@Centril
Copy link
Contributor

Centril commented Apr 20, 2019

@alexcrichton Please cancel the FCP and include T-Lang. (I've noted this on many occasions -- exposing intrinsics adds new fundamental abilities that cannot be done in Rust code otherwise and all such decisions needs T-Lang approval)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 20, 2019

@briansmith

In what circumstance would somebody call has_cpuid()?

When you want to know whether the hardware you are running on has the cpuid instruction available, so that you can know whether using cpuid is safe or not.

Does Rust support any x86 or amd64 targets that don't have CPUID?

Many. CPUID was introduced with the Pentium and i486-SL architectures, and Rust does support both i486 and i386 targets - we even ship some i386 targets with rustup like i386-apple-ios although more are available via cargo xbuild (e.g. i386 linux targets).

I've never even considered the possibility that CPUID would be unavailable.

Lucky you I guess. As the mini-RFC mentions, all mainstream compilers do CPUID detection in their CPUID intrinsics. i386 and i486 are modern enough that all modern toolchains can support it without many issues, but old enough to predate the introduction of the CPUID instruction.


@nagisa

SGX target is notable modern target which does not support CPUID.

On SGX doing x86_64 hardware feature detection via the CPUID instruction is not something you can do. Still, has_cpuid returning false would prevent any code in std from trying to do that.

Heh, I wonder what result the EFLAGS.ID-based CPUID detection procedure returns inside an enclave. I suspect it's still going to be true.

Yeah, the current implementation is incorrect for SGX targets. That target was added relatively recently, and none of the people working on SGX has run into this (cc @jethrogb), (EDIT: that is incorrect, we do support SGX targets, see next comment) I suspect because as @briansmith mentions std::detect is not a very useful module inside an SGX enclave. Knowing whether CPUID available is not enough. What you want to know is whether you are actually in an SGX enclave or not.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 20, 2019

That target was added relatively recently, and none of the people working on SGX has run into this (cc @jethrogb), I suspect because as @briansmith mentions std::detect is not a very useful module inside an SGX enclave

Wait, this is wrong, the current implementation does properly support SGX and it does the obvious thing that was suggested above already:

https://github.com/rust-lang-nursery/stdsimd/blob/master/crates/core_arch/src/x86/cpuid.rs#L87

@jethrogb
Copy link
Contributor

The motivation for this intrinsic is still unclear to me. Specifically, for SGX, feature detection might work when using is_x86_feature_detected even though has_cpuid returns false. Everyone should be using std::is_x86_feature_detected as much as possible such that feature detection need only be implemented once.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 20, 2019

Specifically, for SGX, feature detection might work when using is_x86_feature_detected even though has_cpuid returns false.

I've commented on that issue, since I think the claim is not 100% precise, but puting us back on topic: implementing run-time feature detection for SGX is orthogonal from being able to tell whether a CPU supports the CPUID instruction or not.

For SGX, has_cpuid returns false, and that is correct AFAICT (asm!("cpuid" ) fails there).

Everyone should be using std::is_x86_feature_detected as much as possible such that feature detection need only be implemented once.

If your application is #![no_std] you can't do that - we'd need to force everyone to use std to make that true. For most targets (x86 is one of them), run-time feature detection requires OS support. If you don't have an OS, there are many ways of implementing run-time feature detection that are incompatible with each other depending on what your application is doing (your application becomes the OS), so we can't provide something that works for everybody in #![no_std], which is why people can build std::detect from crates.io, and configure it for their use case if they can't use std.

@briansmith
Copy link
Contributor

Rust does support both i486 and i386 targets - we even ship some i386 targets with rustup like i386-apple-ios although more are available via cargo xbuild (e.g. i386 linux targets).

OK, then what we really need to know is why CPUID isn't available: because the target is too old, or because SGX, because the toolchain has been configured to recommend/require only static compile-time feature detection, or for some other reason.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 21, 2019 via email

@briansmith
Copy link
Contributor

If you need to know any of that (where one of the things you mention can’t happen,

What is the thing that can't, and will never, happen?

and two are the same, so there are only two cases) has_cpuid is one of the building blocks you can use to query that.

I can get all the same information without has_cpuid, except for the thing that hasn't been implemented yet (target explicitly configured to disable dynamic feature probing).

But the std cpuid APIs are unsafe, and std only needs to know whether they are safe to call.

I don't know what this means. If you're saying that the current implementation of libstd needs this for some reason, then libstd can already use it without it being stable.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 21, 2019

because the toolchain has been configured to recommend/require only static compile-time feature detection

If you don't want to do run-time feature detection, you can just not use is_x86_feature_detected!. If you only want to do compile-time feature detection, cfg(target_feature) has always done that.

because the target is too old,

You can run i386 binaries on skylake, which does have CPUID. What does knowing that the target is i386 and does not have CPUID tell you about the hardware the binary actually runs on ?

I can get all the same information without has_cpuid,

How can you tell whether the hardware an i386 or i486 binary runs on supports the CPUID instruction without has_cpuid on stable Rust? Can you show an example ?

@briansmith
Copy link
Contributor

OK, I didn't realize that this was doing runtime detection (using EFLAGS bit 21 to detect whether CPUID is available) vs. determining it statically.

A few things I noticed:

  1. One of the references cited in the implementation for this suggested to avoid doing this detection in C because compilers often stomp on EFLAGS when doing inline assembly, yet the implementation is in inline assembly. If it were up to me, I would move it out of Rust and into a standalone assembly module to be safe.

  2. The implementation of the runtime detection this feature does is conditional on #[cfg(not(target_feature = "sse"))], but that seems wrong for i586 targets. i586 targets shouldn't (AFAICT) should be presumed to support CPUID since it was available on (AFAICT) all Pentiums. Changing the implementation to avoid the EFLAGS-based conditional logic for 586 and later seems like a good idea to me.

  3. There are no i386 or i486 targets at https://forge.rust-lang.org/platform-support.html because i386-apple-ios. I think every machine running i386-apple-ios can actually execute CPUID just fine. So, I'm not sure any (officially) supported target actually benefits from this feature.

The runtime detection is conditional on #[cfg(not(target_feature = "sse"))] but SSE is the one of the minimum features my code needs, and every SSE-having CPU has CPUID, I think. This is one reason I don't need this function, the other being that I already need to do different things for SGX specifically. I hope this clarifies my statement about why it isn't useful for me. However, it may be useful to people targeting 386 and 486. (I doubt the reliability of LLVM-generated code running on a 386 or 486 though, due to general lack of attention.)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Apr 22, 2019

One of the references cited in the implementation for this suggested to avoid doing this detection in C because compilers often stomp on EFLAGS when doing inline assembly, yet the implementation is in inline assembly. If it were up to me, I would move it out of Rust and into a standalone assembly module to be safe.

Iff this is the case with the current implementation, please fill a bug in rust-lang/rust about it since that would mean that the implementation of asm! is broken.

The implementation of the runtime detection this feature does is conditional on #[cfg(not(target_feature = "sse"))], but that seems wrong for i586 targets.

Rust i686 targets do have SSE, what they don't have is SSE2. Doing better on i586 is possible (rust-lang/stdarch#497), PRs welcome.

@dtolnay
Copy link
Member

dtolnay commented May 8, 2019

The runtime detection is conditional on #[cfg(not(target_feature = "sse"))] but SSE is the one of the minimum features my code needs, and every SSE-having CPU has CPUID, I think. This is one reason I don't need this function, the other being that I already need to do different things for SGX specifically. I hope this clarifies my statement about why it isn't useful for me. However, it may be useful to people targeting 386 and 486.

Can someone confirm that this is the entire intended target audience for this function, or are there other reasons someone might want to call it outside of 386 and 486?

It's strange to me that the motivation in rust-lang/stdarch#730 refers to @briansmith but Brian won't need this function. I am still trying to work out whether anyone else would ever want to call this.

How does this typically work in C++? The PR mentions that neither clang nor gcc expose this functionality. What do people do instead?

If there is an implementation of has_cpuid available through crates.io, do we know of anyone calling it?

@dtolnay
Copy link
Member

dtolnay commented May 8, 2019

@Centril:

Please cancel the FCP and include T-Lang. (I've noted this on many occasions -- exposing intrinsics adds new fundamental abilities that cannot be done in Rust code otherwise and all such decisions needs T-Lang approval)

Could you take a look at rust-lang/stdarch#730 and let me know whether you still would like T-lang involved? This isn't an intrinsic in the sense of extern "rust-intrinsic" -- this is a library function implemented using target_env / target_arch / target_feature / asm.

@Centril
Copy link
Contributor

Centril commented May 8, 2019

This isn't an intrinsic in the sense of extern "rust-intrinsic" -- this is a library function implemented using target_env / target_arch / target_feature / asm.

As far as I can tell, the function is implemented using asm!(...), an unstable language feature. As far as I know, it cannot be implemented using stable Rust (linking to assembly files doesn't count for the standard library cause then you could add all sorts of behaviors to the operational semantics...). As such, it requires language team sign-off. (Not that I would be opposed to adding this or anything)

@dtolnay
Copy link
Member

dtolnay commented May 8, 2019

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented May 8, 2019

@dtolnay proposal cancelled.

@workingjubilee workingjubilee added I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 15, 2024
@workingjubilee
Copy link
Member

@RalfJung

Where can I read more about the plan here -- how would one write is_x86_feature_detected!("avx") in no_std with this proposal?

As briansmith has noted, you cannot do so without additional asm! or cfg mechanisms.

What would it take to make the existing macros available in core, why is that not viable?

It would be fine if we're willing to just do so. As far as I understand, I think we've avoided doing so out of a reluctance to include what is target-specific code in core.

What are the trade-offs around letting people infer target features from raw CPUID information -- a new stable language-level guarantee? (As such I think this also needs t-lang involvement.)

The CPUID information, when accessed from userspace, constitutes a kind of special calling convention with the kernel, as the kernel is allowed to pick the answers. It is not exactly "raw", in that sense, unless accessed from the privileged context, where one is allowed to see the true answers (and change them).

@workingjubilee
Copy link
Member

T-lang has already made some decisions re: target-features that implicitly relate to CPUID: #73631 (comment)

@workingjubilee
Copy link
Member

cc @rust-lang/libs-api

I am requesting that libs-api advance a new FCP to remove has_cpuid and close this issue. Note that I am asking only for a judgement regarding has_cpuid(): I believe this should not reflect negatively or positively on @briansmith's alternative proposal, which I believe is a new (and possibly better) feature. Rather, asm! is now stable, and as @briansmith notes, this intrinsic essentially does not provide an answer to the "is it 'safe' to call CPUID and rely on its results?" question.

...Note that key detail: it is never possible to simply invoke CPUID with impunity, due to the existence of cpuid_fault, so we should not stabilize this function with this signature if we expect the following code to be written:

if has_cpuid() {
    // SAFETY: see previous line
    let cpuid_result = unsafe { __cpuid(some_leaf) };
    // the rest of the code may never be reached because of a fault,
    // and our fate is decided by the platform's fault handlers
}

I don't think a function that returns a bool for which true is "sure, you can call that unsafe fn safely, but it may abort, or do whatever the fault handler picks, or return, whatever lol" is something we should really bother with. The CPUID instruction invokes something we should regard as arbitrary magic that essentially says whatever the architecture and OS (or hypervisor!) wants it to say, and has arbitrary effects on control flow based on the microarchitecture.

One can make the argument that because these behaviors are relatively well-specified (for the CPUs that have CPUID, anyways), that this is "sound" nonetheless, because amb(x, y, z) is a safe function in Rust, so it is allowed to have a function that is, basically

if cpu_chosen() {
   jump_to(cpu_specific_code)
} else if os_chosen() {
   jump_to(os_specific_code)
} else {
   let eax, ebx, ecx, edx;
   asm!(..);
   CpuidResult { eax, ebx, ecx, edx }
}

But if we are arguing that, then we should just have the courage to make __cpuid() a safe function on x86-64 CPUs, and make this function reachable only on 32-bit x86. Except even that wouldn't apply, due to SGX existing. In truth, the problem is this: Intel is quite clearly willing to alter the deal. We can only pray they do not change it further!

Let us render unto asm! what is asm!'s, and unto cfg what is cfg's.

@briansmith
Copy link
Contributor

But if we are arguing that, then we should just have the courage to make __cpuid() a safe function on x86-64 CPUs, and make this function reachable only on 32-bit x86. Except even that wouldn't apply, due to SGX existing. In truth, the problem is this: Intel is quite clearly willing to alter the deal. We can only pray they do not change it further!

You usually cannot rely on what CPUID reports when you are operating in a kernel context or in other special contexts (probably signal handlers). This, combined with the SGX and i586 complexity, is why I suggested that this be made a target feature.

@Amanieu
Copy link
Member

Amanieu commented Jun 18, 2024

@rfcbot fcp close

We don't have any targets for i386/i486 (which don't support CPUID) and we're happy to let SGX targets figure out feature detection on their own. As such, let's just remove has_cpuid.

@rfcbot
Copy link

rfcbot commented Jun 18, 2024

Team member @Amanieu has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Jun 18, 2024
@Amanieu Amanieu removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Jun 18, 2024
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 18, 2024
@rfcbot
Copy link

rfcbot commented Jun 18, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@joshtriplett
Copy link
Member

Speaking for myself here: I'd be in favor of having is_x86_feature_detected be available in core, ideally in a fully functional way.

@jethrogb
Copy link
Contributor

I'd be in favor of having is_x86_feature_detected be available in core, ideally in a fully functional way.

The "fully functional" part isn't possible on SGX, where platform support is needed for feature detection. So if the definition is moved to core, there needs to be a way for std to plug-in functionality.

@RalfJung
Copy link
Member

I assume it would only exist in core for targets where that is possible, i.e., not the SGX targets.

@jethrogb
Copy link
Contributor

It seems pretty strange to me to have target-specific (beyond arch) items in core.

@workingjubilee
Copy link
Member

I mean, it seems pretty strange to me to have architectures which can't actually execute their ISA.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 28, 2024
@rfcbot
Copy link

rfcbot commented Jun 28, 2024

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@workingjubilee
Copy link
Member

false

@workingjubilee workingjubilee closed this as not planned Won't fix, can't repro, duplicate, stale Jun 29, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-SIMD Area: SIMD (Single Instruction Multiple Data) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests