Make cpuid safe and update docs

[sayantn]

Counterproposal to rust-lang/rust#146560

This makes __cpuid, __cpuid_count and __get_cpuid_max safe to use. All of these just call the CPUID instruction, so it is safe to call whenever the CPUID instruction is available (it has defined behavior even if unsupported leaf values are passes). There are cases when it might not be available

• processors lower than i586, which afaik rust doesn't support anymore. The has_cpuid function was removed with this exact motivation, and it is justified
• SGX environments can't use CPUID. Or more accurately, they can't trust the output of CPUID. It should also be mentioned that SGX has been deprecated by intel for quite some time now.

As these are very niche cases we propose to make cpuid safe in general.

Alternatives

Leave it unsafe

This is always an option. But for most use cases using __cpuid is perfectly safe, so it is inconvenient (and more so for projects that ban uses of unsafe).

Introduce a cpuid target feature, and take advantage of target_feature_11

This is what rust-lang/rust#146560 does. As all X86 target features (other than soft_float and x87) were introduced after CPUID, we can add a detection-only target feature cpuid that is implied by all X86 target features. This makes using it in i686 or x86_64 targets safe due to sse being auto-enabled in the targe description, whereas i386 remains unsafe as it doesn't enable sse

[rustbot]

r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[traviscross]

Proposing FCP on this (in fact, even nominating it) is blocked on:

• Prepare for lang FCPs in stdarch team#2007

[traviscross]

@sayantn, if you could please update the PR description with the details of the stabilization being proposed, the motivation, etc., that would be helpful to us in considering this nomination and for proposing FCP.

[joshtriplett]

👍 for this. One clarification:

processors lower than i386

For the avoidance of ambiguity: lower than i586, which is lower than the current 32-bit x86 targets in Rust.

[traviscross]

Thanks @sayantn. Le'ts propose to do it.

@rfcbot fcp merge

[rust-rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @nikomatsakis
• @scottmcm
• @the8472
• @tmandry
• @traviscross

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

We discussed this in the lang team meeting today and the agreement was that if we add support for an older target like i586, we can make this unsafe and require target_feature for the intrinsic on that target. These intrinsics are already platform-specific, so it's natural to have a more target-specific definition that changes the safety of the intrinsic.

@rfcbot reviewed

[nikomatsakis]

@rfcbot reviewed

[sayantn]

Any updates/concerns on this @Amanieu @BurntSushi @the8472 ? Seems like this got more traction than the counterproposal in r-l/r

[rust-rfcbot]

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

[rust-rfcbot]

The final comment period, with a disposition to merge, 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.

This will be merged soon.

[sayantn]

@traviscross @joshtriplett can this be merged now that the RFC is complete?

[traviscross]

can this be merged now that the [FCP] is complete?

Yes; this is OK to go forward as far as lang and libs-api are concerned. It now has an approving review from @tgross35, so it seems this is OK to merge.

[jethrogb]

SGX environments can't use CPUID. Or more accurately, they can't trust the output of CPUID.

The latter sentence is incorrect. The former is correct. Executing the CPUID instruction in SGX results in an #UD exception.

It should also be mentioned that SGX has been deprecated by intel for quite some time now.

This is also incorrect. SGX is not deprecated and Intel's newest processors continue to have SGX.

The SGX target maintainers would've appreciated a heads up for this change.

[tgross35]

In that case we could either panic or return zeros on SGX when this is called, rather than allowing a crash. I expect zeros would be nicer in more cases so code just progresses as if target features aren't available when run on SGX, rather than needing to special case

@jethrogb if one of these sounds reasonable, would you mind putting up a PR?

[jethrogb]

Panicking should be fine, it's not really more or less safe than triggering #UD.

Pretending features aren't available could cause security issues, for example if code is checking whether AES-NI is supported and falling back to an AES implementation that's not constant time.

[sayantn]

That is not a secure implementation, an implementation of AES should always fall back on constant-time implementations (e.g using bitslicing). We cannot do anything about such code -- in general any secure program should behave identically in presence or absence of any target features

Also an alternate approach, can we just #[cfg(not(target_env = "sgx"))]it?