proposal: x/sys/cpu

[aead]

This is a proposal for unifying the CPU detection for /x/crypto similar to the standard lib internal/cpu.

Current situation

Currently different crypto implementations detect CPU features differently:

• ChaCha20Poly1305 uses a mixture of Go and x64 asm.
• Blake2{b,s} uses plain asm and undocumented runtime functionality.
• Argon2 also uses plain asm and a pending CL for argon2 AVX2 support wants to add AVX2 detection in plain asm.

This has lead to asm code which was copied over and over again. Usually we try to avoid asm code if possible.

Why not use internal/cpu and the linker

This CL tried to use the linker to reuse internal/cpu. But this approach does not work since x/crypto must also work with older Go releases and causes a performance regression at the moment.

Possible Solution

One possible solution would be an internal package (e.g. x/crypto/internal/cpu) which contains functionality to detect CPU/platform specific features similar to internal/cpu in the standard library.
This package would be vendored into the standard library because chacha20poly1305 is used by crypto/tls.
Using the linker approach showing in CL-106336 it's not required that internal/cpu duplicates x/crypto/internal/cpu. Instead we can use //go:linkname to instruct the linker to bind vars in internal/cpu to x/crypto/internal/cpu. This way both cpu packages only have to export the same API but the CPU feature detection (asm code) would be only in x/crypto/internal/cpu.

So this proposal would:

1. Create a new package x/crypto/internal/cpu.
2. Copy the CPU feature detection code from internal/cpu to x/crypto/internal/cpu.
3. Remove the the CPU feature detection code from internal/cpu but keep the export API.
4. Change all crypto primitive implementations to use x/crypto/internal/cpu.
5. Vendor x/crypto/internal/cpu into standard library (required by chacha20poly1305)
6. Use the //go:linkname directive to bind the internal/cpu API to x/crypto/internal/cpu.

Remark

While it possible to extract the package x/crypto/internal/cpu to x/internal/cpu using the same approach described above it's not clear whether this is needed. AFAIK only x/crypto uses asm code depending on the CPU/platform features.

Ref: #24828
Also related: #24813

/cc @ianlancetaylor @tklauser @FiloSottile @gtank

[bradfitz]

Sounds good to me. Thanks for filing this. The whole CPU detection situation has been getting increasingly messy.

I would also be fine with putting a cpu package somewhere not even internal. Crypto code often needs to do CPU-specific things at runtime, but there's no inherent reason it should be scoped privately to just x/crypto/... with a x/crypto/internal restriction. I'd even be fine with putting it in the standard library somewhere (CPU flags are somewhat stable and don't get added as fast as, say, Linux system calls.)

But I'm not sure what the point of even using internal at all is, if any assembly anywhere on GitHub can access whatever it wants via //go:linkname unsafeness anyway.

Maybe we just put it in x/arch/cpu?

[aead]

A non-internal cpu package in the standard library would be much cleaner since we can avoid sharing a internal package between standard library and other repositories (like x/crypto) and can also replace other internal packages like crypto/internal/cipherhw. I agree with @bradfitz here.
However a package like x/arch/cpu would not completely solve the issue with x/crypto since such a package would only be available using (at least) Go1.11. So custom CPU feature detection would still be needed for x/crypto.

[FiloSottile]

Can't we make x/arch/cpu and vendor it in both x/crypto and the standard library?

[aead]

Oh, yes - my bad. Didn't recognize the x in x/arch/cpu. Yes, I think that approach would work @FiloSottile

[tklauser]

Thanks for the proposal @aead. Creating x/arch/cpu and vendoring it where nedded sounds reasonable to me.

[gtank]

Thanks for proposing this. Can't come soon enough - I literally wrote another piece of CPU-detection code bound for x/crypto last night. x/arch/cpu sounds reasonable to me.

[ianlancetaylor]

Sorry for the bike shedding, but right now x/arch contains tools used to develop for the architecture. Adding a library that executes at run time is an uneasy fit. Another option would be x/sys/cpu.

[bradfitz]

but right now x/arch contains tools used to develop for the architecture. Adding
library that executes at run time is an uneasy fit.

That wouldn't bother me. Almost every x/ repo is a mix of tools and packages.

But I'm also fine with x/sys.

[aead]

Okay, I can submit a first proposal for a cpu package at x/sys as x/sys/cpu.

[gopherbot]

Change https://golang.org/cl/107015 mentions this issue: x/sys: new package cpu

[AlexRouSg]

Just wanted to point out that intel has a cpuid package with a more complete x86 cpu feature list and it's BSD 3-Clause licensed. https://github.com/intel-go/cpuid