cpuid not supported in sgx environment

[yihuang]

cpuid is not supported in sgx environment.

[newpavlov]

Thank you for reporting it! But this approach will mean that it will not be possible to enable HW acceleration via compiler flags. I think it will be better to modify the cpuid-bool crate instead (e.g. on SGX return false if at least on of requested target-features was not enabled).

[jethrogb]

My recommendation is to always use is_x86_feature_detected and to fall back to a local definition in case std is not available.

#[cfg(not(feature = "std"))]
macro_rules! is_x86_feature_detected {
    ($feat:literal) => {
        {
            #[cfg(all(target_vendor="fortanix", target_env="sgx"))] {
                cfg!(target_feature = $feat)
            }
            #[cfg(not(all(target_vendor="fortanix", target_env="sgx")))] {
                cpuid_bool::cpuid_bool!($feat)
            }
        }
    };
}

[newpavlov]

I've published cpuid-bool v0.1.1 which should fix this issue. Can you please do cargo update and check if SHA crates work correctly?

[jethrogb]

That's not sufficient. You should call is_x86_feature_detected when available.

[newpavlov]

@jethrogb
I think it's unnecessary complication at this stage. I don't want to repeat this code in all crates which will depend on cpuid-bool. Plus note that cpuid-bool works a bit differently from is_x86_feature_detected, it caches a boolean result, instead of two u64s, so it's a bit more efficient, since it does not have to recompute flag each time. For SHA crates it does not matter that much, but I think it will be noticeable for AES.

[jethrogb]

I don't want to repeat this code in all crates which will depend on cpuid-bool.

You'll have to. As mentioned previously, CPUID is not supported on some platforms and as such those platforms can't be supported by cpuid_bool. The standard way for Rust platforms to advertise x86 CPU features is std::is_x86_feature_detected. On some platforms this macro may be implemented by CPUID, but on others it can work differently. The only cross-platform way to know whether a feature is enabled is with the macro in std which properly abstracts all the platform differences.

[newpavlov]

What are other x86 targets apart from SGX which do not support CPUID instruction? In your snippet you special-case SGX as well, so I don't see why I have to use is_x86_feature_detected in the current form, while cpuid-bool works without issues and results in a simpler code.

[jethrogb]

In your snippet you special-case SGX as well

I'm only special-casing it in the case std::is_x86_feature_detected is not available. This is no longer necessary with your recent change to cpuid-bool:

#[cfg(not(feature = "std"))]
macro_rules! is_x86_feature_detected {
    ($feat:literal) => {
        cpuid_bool::cpuid_bool!($feat)
    };
}

I don't see why I have to use is_x86_feature_detected in the current form, while cpuid-bool will work without issues.

cpuid-bool won't work because it can't work.

[newpavlov]

cpuid-bool won't work because it can't work.

What? Can you please elaborate? is_x86_feature_detectedis not magic, under the hood it uses code very similar to one used in cpuid-bool.

[jethrogb]

I feel like I'm repeating myself here. cpuid-bool uses the CPUID instruction so it can't work on platforms that don't support it.

is_x86_feature_detected is not magic, under the hood it uses code very similar to one used in cpuid-bool.

It is, in a way, “magic” as the platform is free to change the implementation of is_x86_feature_detected to provide accurate information regardless of how the information is obtained.

And I will repeat my earlier question: what are other x86 targets apart from SGX which do not support CPUID instruction?

I don't know.

[tarcieri]

@jethrogb if I'm understanding right, it sounds like the issue is the Fortanix Rust target (x86_64-fortanix-unknown-sgx) includes a special std implementation with a custom std::is_x86_feature_detected which works correctly inside SGX enclaves. Is that the case?

[jethrogb]

It doesn't today but it likely will in the future.

[newpavlov]

I guess when the Fortranix target will add a custom implementation of the is_x86_feature_detected macro, we could add std feature to cpuid-bool, which would use this macro instead of the custom CPUID-based code. Until then I think we can keep things as-is. But ideally I would prefer for something like RFC 2725 to land, since we need this feature on ARM as well (right now we have to depend on libc).

[newpavlov]

Should be fixed in RustCrypto/utils#68.

We can revisit this problem when Fortranix target will introduce custom is_x86_feature_detected implementation.

[jethrogb]

Why not fix it now? By not fixing it now, you're forcing people to have to deal with this (i.e. updating their dependencies) in the future.

[newpavlov]

Because for the current state of things the problem is "fixed". It is quite possible that before the custom macro implementation we will get rust-lang/rfcs#2725 merged and implemented, so I don't want to make code more complex than necessary.