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

[aead]

@AlexRouSg Thanks for the hint. I'm aware of this. However the standard library and the x/* repositories cannot depend on third party packages.

[tklauser]

Sorry for bike shedding as well, but we could argue similarly that x/sys is related to functionality provided by the operating system. So cpu doesn't quite fit there either. Would it be worthwhile creating a golang.org/x/cpu repository especially for this or would that be too much bike shedding?

In any case, I'm fine with either x/arch, x/sys or x/cpu as long as the functionality ends up in a non-internal package :)

[AlexRouSg]

@aead Maybe ask intel if they want to contribute to a x/* package? Seems like a lot of duplicated work if the new x/* is going to contain the entire x86 list. Or is it just going to contain the popular features or features the std and other x/* packages use?

[aead]

Hm, while both x/arch/cpu and x/sys/cpu sounds reasonable to me x/cpu as package which contains the whole CPU related functionality looks a bit odd compared to other x/* packages. AFAICS there three different approaches for the cpu package:

1. A package x/*/cpu providing the functionally for all supported architectures (like x86/amd64, arm, s390x, ...). The API would look quite similar to the current internal/cpu package in the standard library. Personally I don't like that approach because cpu would export functionality for e.g. ARM-64 on x64.
2. Basically the same approach as 1. but (as shown in the CL) cpu would only export functionality for your current platform. The adv. here would be that cpu.HasSSE2 would not even compile if used in an *_arm.go file. So cpu basically exports only things related to your current platform. The downside would be that for example godoc.org would only show the documentation for godoc's CPU platform (IIRC amd64).
3. The cpu package contains some basic information about the CPU like e.g. register/address size while e.g.g the CPU features are located under x*/cpu/x86, x*/cpu/arm, ... This approach would also justify making cpu a top-level package under x/.

[aead]

@AlexRouSg
AFAIK x/*/cpu will only contain a subset of cpu features. Please notice that it will not only contain x86/x64 features but also arm, s390x, a.s.o. More features (like AVX-512) can be added - if required - later on. The functionality is already there: internal/cpu in the standard library.

[randall77]

@TocarIP

[randall77]

I do like how the internal/cpu package is organized, so I would pick option 1.
It allows you to use code like this:

    blocksize := 16
    if runtime.GOARCH=="arm64" && cpu.Arm64.Foo {
        blocksize = 64
    }

This wouldn't compile under option 2.
Option 3 is ok, but it looks just like option 1 except replace Arm64 with arm64 in the code above.

[bradfitz]

Option 1 also has the advantage of nice, consistent godoc for everybody, without tweaking environment variables and/or URL parameters.

Edit: oh, @aead already mentioned that.

[TocarIP]

I also like option 1. It allows for something like:

if cpu.Arm64.FastFOO || cpu.x86.FastFOO {
     FOO()
} else { // FOO is slow,so use different algorithm
   BAR() 
}

[martisch]

Could we just move internal/cpu to be public in the std lib or would e.g. a update every 6 months be considered to slow for introduction of new features?

The advantage i see is we have one place for crypto/runtime/std lib to source flags going into the future and one way to e.g. mask flags for all tests. Seems possible to still consider this for 1.11.

[aead]

@martisch Making cpu public in the standard library does not really solve the problem this proposal tries to address. Currently all packages in the standard library can access internal/cpu already. The problem is that packages outside of std lib which must work for older releases too - like x/crypto - cannot assume that such a package is there. So while this would work for >= Go1.11 it does not work for e.g. Go1.10.
There are basically two options (without doing the custom CPU feature detection in external packages):

• Move a cpu package into the standard library and drop support / backward "guarantee" for x/* repositories at some point.
• Create a cpu package under /x/* and vendor it into the standard lib - like already done for e.g. chacha20poly1305.

[aead]

Didn't thought about the "fast-branch" use case where implementations are build not only on certain archs - like in a _arm.go, _amd64.go,... file. IIRC that is relatively common in the runtime?!. Agree that this makes 1./3. more useful.

[martisch]

I think x/ supports the last 2-3 versions of go (correct me if i am wrong). So what i was thinking is we could make cpu public in standard lib now. In the mean time vendor a copy into x/crypto/internal which will be removed again in 2-3 versions and from then on x/crypto can rely on using the standard lib version. Not sure if that corresponds to @aead second option "Create a cpu package under /x/* and vendor it into the standard lib - like already done for e.g. chacha20poly1305.". The downside is that any new feature flags (AVX512) can take months/years to be usable by crypto with full backwards compatibility.

One problem i see with vendoring x/*/cpu into std lib as internal/cpu is that the runtime was recently changed to use internal/cpu and init is not used anymore for initialization but runtime calls a custom init function early before even map code is initialized to init internal/cpu. So there would need to be some new code around initializing if triggered by runtime and some triggered through normal init chain if in x/*/cpu. Would seem better to me then to keep runtime cpu feature flag detection separate again and just initialize internal/cpu through normal init when used in std lib or x/*/cpu such that there is one common clean initialization path.

[aead]

I'm not completely sure about x/* backward compatibility, but for example x/net/http2 contains also files for eg. Go1.6 - (@bradfitz)? If we agree on supporting just the latest n Go releases for x/* than putting a cpu package into the standard library may be an option.

The downside is that any new feature flags (AVX512) can take months/years to be usable by crypto with full backwards compatibility.

Usually - if we write special assembly code - we should avoid raw BYTE / WORD encoding.
(It makes reading the asm even worse). So I guess this is not a big issue since CLs which add new instructions to the assembler can also add feature detection to */cpu. So whenever new instructions for a certain CPU feature are added we can ensure that they are also available by cpu. That way backward compatible code can be added to x/* within half a year / Go std-lib dev cycle.

Would seem better to me then to keep runtime cpu feature flag detection separate again and just initialize internal/cpu through normal init when used in std lib or x/*/cpu such that there is one common clean initialization path.

Completely agree here 👍
If we move cpu into the standard lib we should take care that all code - including runtime can use it.

[bradfitz]

x/* is only tested for the past two releases (currently Go 1.9 and Go 1.10), and anything older than that is not explicitly supported.

We remove support for older things when it becomes inconvenient, but we don't proactively break older versions just because we can.

[tmthrgd]

If I understand correctly, the main disadvantage of making internal/cpu public is

[...] that current x/* code can only switch to the cpu package immediately if we share/maintain a internal copy of cpu in the x/ repository.

but that needn't be true. I'd like to suggest an alternative approach, similar to some other suggestions:

• Promote internal/cpu to a public standard library package for go1.11.
• Move all of x/cryptos existing feature detection code to a really minimal x/crypto/internal/cpu and hide it beind !go.11 build flags.
• Add a really small stub to x/crypto/internal/cpu guarded by go1.11 that just references the standard library package.

This deduplicates code within x/crypto, and between it and the standard library, while not requiring any more feature detection routines be added.

---

The AT_HWCAP entries in the auxv vector are a bigger problem. I guess we'll probably need some hack that lets x/sys/cpu reach into internal/cpu, or runtime, somehow. Ugh.

@ianlancetaylor The approach would essentially force the runtime to expose the auxv vector in a way bound by the go1 compatibility guarantee, which doesn't seem cleaner than exposing internal/cpu.

[ianlancetaylor]

@tmthrgd When I say "some hack" I specifically mean some mechanism that does not invoke the Go 1 compatibility guarantee. As the Go 1 guarantee only operates at the source code level, there are a couple of such hacks available.

On the other hand promoting internal/cpu to be a visible standard library package would invoke the Go 1 compatibility guarantee. Our experience with the syscall package suggests that that would be less than ideal in the long run.

[gopherbot]

Change https://golang.org/cl/110355 mentions this issue: crypto/{blake2b,blake2s,argon2,chacha20poly1305}: replace CPU feature detection

[gopherbot]

Change https://golang.org/cl/110796 mentions this issue: crypto/chacha20poly1305: correct AVX2 feature detection

[gopherbot]

Change https://golang.org/cl/177897 mentions this issue: all: remove x/sys/cpu and replace with internal/cpu