From c9456c0ad1f3a54295bd1fe9c0a69098b57aae10 Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 9 Mar 2024 21:29:15 +0900 Subject: [PATCH] detect/x86_64: Reduce scope of allow(dead_code) --- src/imp/atomic128/detect/common.rs | 2 ++ src/imp/atomic128/detect/x86_64.rs | 53 ++++++++++++++++-------------- src/imp/atomic128/x86_64.rs | 16 +++++---- 3 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/imp/atomic128/detect/common.rs b/src/imp/atomic128/detect/common.rs index b87caa35..a8cc0580 100644 --- a/src/imp/atomic128/detect/common.rs +++ b/src/imp/atomic128/detect/common.rs @@ -89,6 +89,7 @@ impl CpuInfo { /// Whether CMPXCHG16B is available const HAS_CMPXCHG16B: u32 = 1; /// Whether VMOVDQA is atomic + #[cfg(target_feature = "sse")] const HAS_VMOVDQA_ATOMIC: u32 = 2; #[cfg(any( @@ -99,6 +100,7 @@ impl CpuInfo { pub(crate) fn has_cmpxchg16b(self) -> bool { self.test(CpuInfo::HAS_CMPXCHG16B) } + #[cfg(target_feature = "sse")] #[inline] pub(crate) fn has_vmovdqa_atomic(self) -> bool { self.test(CpuInfo::HAS_VMOVDQA_ATOMIC) diff --git a/src/imp/atomic128/detect/x86_64.rs b/src/imp/atomic128/detect/x86_64.rs index 80eefed5..a47ed9d0 100644 --- a/src/imp/atomic128/detect/x86_64.rs +++ b/src/imp/atomic128/detect/x86_64.rs @@ -2,7 +2,7 @@ // Adapted from https://github.com/rust-lang/stdarch. -#![cfg_attr(any(not(target_feature = "sse"), portable_atomic_sanitize_thread), allow(dead_code))] +#![cfg_attr(portable_atomic_sanitize_thread, allow(dead_code))] // Miri doesn't support inline assembly used in __cpuid: https://github.com/rust-lang/miri/issues/932 // SGX doesn't support CPUID: https://github.com/rust-lang/stdarch/blob/a0c30f3e3c75adcd6ee7efc94014ebcead61c507/crates/core_arch/src/x86/cpuid.rs#L102-L105 @@ -13,7 +13,7 @@ include!("common.rs"); #[cfg(not(portable_atomic_no_asm))] use core::arch::asm; -use core::arch::x86_64::{CpuidResult, _xgetbv}; +use core::arch::x86_64::CpuidResult; // Workaround for https://github.com/rust-lang/rust/issues/101346 // It is not clear if our use cases are affected, but we implement this just in case. @@ -45,8 +45,8 @@ unsafe fn __cpuid(leaf: u32) -> CpuidResult { } // https://en.wikipedia.org/wiki/CPUID -const VENDOR_ID_INTEL: [u8; 12] = *b"GenuineIntel"; -const VENDOR_ID_AMD: [u8; 12] = *b"AuthenticAMD"; +const _VENDOR_ID_INTEL: [u8; 12] = *b"GenuineIntel"; +const _VENDOR_ID_AMD: [u8; 12] = *b"AuthenticAMD"; unsafe fn _vendor_id() -> [u8; 12] { // https://github.com/rust-lang/stdarch/blob/a0c30f3e3c75adcd6ee7efc94014ebcead61c507/crates/std_detect/src/detect/os/x86.rs#L40-L59 @@ -59,9 +59,6 @@ unsafe fn _vendor_id() -> [u8; 12] { #[cold] fn _detect(info: &mut CpuInfo) { - // SAFETY: Calling `_vendor_id`` is safe because the CPU has `cpuid` support. - let vendor_id = unsafe { _vendor_id() }; - // SAFETY: Calling `__cpuid`` is safe because the CPU has `cpuid` support. let proc_info_ecx = unsafe { __cpuid(0x0000_0001_u32).ecx }; @@ -70,20 +67,29 @@ fn _detect(info: &mut CpuInfo) { info.set(CpuInfo::HAS_CMPXCHG16B); } - // VMOVDQA is atomic on Intel and AMD CPUs with AVX. - // See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688 for details. - if vendor_id == VENDOR_ID_INTEL || vendor_id == VENDOR_ID_AMD { - // https://github.com/rust-lang/stdarch/blob/a0c30f3e3c75adcd6ee7efc94014ebcead61c507/crates/std_detect/src/detect/os/x86.rs#L131-L224 - let cpu_xsave = test(proc_info_ecx, 26); - if cpu_xsave { - let cpu_osxsave = test(proc_info_ecx, 27); - if cpu_osxsave { - // SAFETY: Calling `_xgetbv`` is safe because the CPU has `xsave` support - // and OS has set `osxsave`. - let xcr0 = unsafe { _xgetbv(0) }; - let os_avx_support = xcr0 & 6 == 6; - if os_avx_support && test(proc_info_ecx, 28) { - info.set(CpuInfo::HAS_VMOVDQA_ATOMIC); + // We only use VMOVDQA when SSE is enabled. See atomic_load_vmovdqa() in atomic128/x86_64.rs for more. + #[cfg(target_feature = "sse")] + { + use core::arch::x86_64::_xgetbv; + + // SAFETY: Calling `vendor_id`` is safe because the CPU has `cpuid` support. + let vendor_id = unsafe { _vendor_id() }; + + // VMOVDQA is atomic on Intel and AMD CPUs with AVX. + // See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688 for details. + if vendor_id == _VENDOR_ID_INTEL || vendor_id == _VENDOR_ID_AMD { + // https://github.com/rust-lang/stdarch/blob/a0c30f3e3c75adcd6ee7efc94014ebcead61c507/crates/std_detect/src/detect/os/x86.rs#L131-L224 + let cpu_xsave = test(proc_info_ecx, 26); + if cpu_xsave { + let cpu_osxsave = test(proc_info_ecx, 27); + if cpu_osxsave { + // SAFETY: Calling `_xgetbv`` is safe because the CPU has `xsave` support + // and OS has set `osxsave`. + let xcr0 = unsafe { _xgetbv(0) }; + let os_avx_support = xcr0 & 6 == 6; + if os_avx_support && test(proc_info_ecx, 28) { + info.set(CpuInfo::HAS_VMOVDQA_ATOMIC); + } } } } @@ -99,15 +105,14 @@ fn _detect(info: &mut CpuInfo) { )] #[cfg(test)] mod tests { - #[cfg(not(portable_atomic_test_outline_atomics_detect_false))] use super::*; - #[cfg(not(portable_atomic_test_outline_atomics_detect_false))] #[test] + #[cfg_attr(portable_atomic_test_outline_atomics_detect_false, ignore)] fn test_cpuid() { assert_eq!(std::is_x86_feature_detected!("cmpxchg16b"), detect().has_cmpxchg16b()); let vendor_id = unsafe { _vendor_id() }; - if vendor_id == VENDOR_ID_INTEL || vendor_id == VENDOR_ID_AMD { + if vendor_id == _VENDOR_ID_INTEL || vendor_id == _VENDOR_ID_AMD { assert_eq!(std::is_x86_feature_detected!("avx"), detect().has_vmovdqa_atomic()); } else { assert!(!detect().has_vmovdqa_atomic()); diff --git a/src/imp/atomic128/x86_64.rs b/src/imp/atomic128/x86_64.rs index 5950f0b1..29180c60 100644 --- a/src/imp/atomic128/x86_64.rs +++ b/src/imp/atomic128/x86_64.rs @@ -20,6 +20,7 @@ mod fallback; #[cfg(not(portable_atomic_no_outline_atomics))] #[cfg(not(target_env = "sgx"))] +#[cfg_attr(not(target_feature = "sse"), cfg(not(target_feature = "cmpxchg16b")))] #[path = "detect/x86_64.rs"] mod detect; @@ -131,8 +132,11 @@ unsafe fn cmpxchg16b(dst: *mut u128, old: u128, new: u128) -> (u128, bool) { // // Refs: https://www.felixcloutier.com/x86/movdqa:vmovdqa32:vmovdqa64 // -// Do not use vector registers on targets such as x86_64-unknown-none unless SSE is explicitly enabled. -// https://doc.rust-lang.org/nightly/rustc/platform-support/x86_64-unknown-none.html +// Use cfg(target_feature = "sse") here -- SSE is included in the x86_64 +// baseline and is always available, but the SSE target feature is disabled for +// use cases such as kernels and firmware that should not use vector registers. +// So, do not use vector registers unless SSE target feature is enabled. +// See also https://doc.rust-lang.org/nightly/rustc/platform-support/x86_64-unknown-none.html. #[cfg(not(any(portable_atomic_no_outline_atomics, target_env = "sgx")))] #[cfg(target_feature = "sse")] #[target_feature(enable = "avx")] @@ -208,7 +212,7 @@ macro_rules! load_store_detect { { // Check CMPXCHG16B first to prevent mixing atomic and non-atomic access. if cpuid.has_cmpxchg16b() { - // We do not use vector registers on targets such as x86_64-unknown-none unless SSE is explicitly enabled. + // We only use VMOVDQA when SSE is enabled. See atomic_load_vmovdqa() for more. #[cfg(target_feature = "sse")] { if cpuid.has_vmovdqa_atomic() { @@ -238,8 +242,7 @@ macro_rules! load_store_detect { #[inline] unsafe fn atomic_load(src: *mut u128, _order: Ordering) -> u128 { - // Do not use vector registers on targets such as x86_64-unknown-none unless SSE is explicitly enabled. - // https://doc.rust-lang.org/nightly/rustc/platform-support/x86_64-unknown-none.html + // We only use VMOVDQA when SSE is enabled. See atomic_load_vmovdqa() for more. // SGX doesn't support CPUID. #[cfg(all( any(target_feature = "cmpxchg16b", portable_atomic_target_feature = "cmpxchg16b"), @@ -317,8 +320,7 @@ unsafe fn atomic_load_cmpxchg16b(src: *mut u128) -> u128 { #[inline] unsafe fn atomic_store(dst: *mut u128, val: u128, order: Ordering) { - // Do not use vector registers on targets such as x86_64-unknown-none unless SSE is explicitly enabled. - // https://doc.rust-lang.org/nightly/rustc/platform-support/x86_64-unknown-none.html + // We only use VMOVDQA when SSE is enabled. See atomic_load_vmovdqa() for more. // SGX doesn't support CPUID. #[cfg(all( any(target_feature = "cmpxchg16b", portable_atomic_target_feature = "cmpxchg16b"),