crypto/{aes,internal/cipherhw,tls}: use common internal/cpu in place of cipherhw #24766
Conversation
populated from the internal/cpu package, hasGCMAsm is true if the x86 processor has AES-NI and CLMUL-NI/PCMUL
they won't be populated with the correct values until after all the init funcs are called from internal/cpu ; use functions or the values directly inside existing functions
… delete cipherhw package and references
merge latest from upstream
googlebot
added
the
cla: yes
|
This PR (HEAD: 7b7165d) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/go/+/105695 to see it. Tip: You can toggle comments from me using the |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Brad Fitzpatrick: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Brad Fitzpatrick: Patch Set 1: (2 comments) Nice cleanup, thanks. This looks more complete than the TODO below suggests. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
This PR (HEAD: 9eb8467) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/go/+/105695 to see it. Tip: You can toggle comments from me using the |
|
Message from Brad Fitzpatrick: Patch Set 2: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Gobot Gobot: Patch Set 2: TryBots beginning. Status page: https://farmer.golang.org/try?commit=8b65ccf1 Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Brad Fitzpatrick: Patch Set 2: R=mike.munday Mike, can you review for the s390x bits? LGTM, at least, but I haven't tested it. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Anit Gandhi: Patch Set 2: (3 comments)
Thanks for the quick review! Yeah I just forgot to remove the TODO message :) Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Gobot Gobot: Patch Set 2: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Michael Munday: Patch Set 2: (12 comments) A couple of changes are needed to get it to compile (s/S390x/S390X/) but, other than some nits I've commented on, this LGTM. Thanks for doing this! Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
This PR (HEAD: f960e99) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/go/+/105695 to see it. Tip: You can toggle comments from me using the |
|
This PR (HEAD: 4adc088) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/go/+/105695 to see it. Tip: You can toggle comments from me using the |
|
Message from Anit Gandhi: Patch Set 4: (12 comments) Thanks for the quick turnaround on the review Michael! Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Brad Fitzpatrick: Patch Set 4: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Gobot Gobot: Patch Set 4: TryBots beginning. Status page: https://farmer.golang.org/try?commit=53dd76f2 Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Gobot Gobot: Patch Set 4: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Anit Gandhi: Patch Set 5: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Gerrit Bot: Uploaded patch set 6: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Anit Gandhi: Patch Set 6: (1 comment) Just as a note in case it wasn't clear, I'm waiting for this new AES-GCM ARM64 CL to be merged (https://go-review.googlesource.com/c/go/+/107298) and I'll re-base and update this CL accordingly to factor in the separation of plain AES support vs AES+PMULL support. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Gerrit Bot: Uploaded patch set 7: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
gopherbot
force-pushed
the
master
branch
2 times, most recently
from
a8a60ac
to
87412a1
Compare
this aligns it with the amd64 and arm64 implementations: the creation of the base cipher.Block only requires aes hardware support
the optimized s390x implementation uses either KMCTR+KIMD OR KMA ; KM is always required
1. Add back HasKMC and hasKMCTR since the optimized CBC and CTR s390x implementations rely on that. Possible TODO is to add a fallback path for generic CTR and CBC 2. reduce key storage to 32 bytes (previous was based on a 256 bytes instead of correct bits)
|
This PR (HEAD: f2a49db) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/go/+/105695 to see it. Tip: You can toggle comments from me using the |
|
Message from Anit Gandhi: Patch Set 8: I've updated the CL with a few small fixes; please disregard my previous message, as the separation of AES support vs AES+PMULL support is now done for amd64, arm64, and s390x in this CL. Technically, it's still dependent on https://go-review.googlesource.com/c/go/+/107298 since that brings the AES-GCM updates to arm64. Only caveat is that if anyone uses Go with this CL but without 107928, the TLS performance will be awful since it'll prioritize the generic, non-optimized implementation over the chacha20-poly1305 one. I think this one can still be reviewed and merged independently though. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
merge from upstream
|
This PR (HEAD: bf8cbdf) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/#/c/go/+/105695 to see it. Tip: You can toggle comments from me using the |
|
Message from Brad Fitzpatrick: Patch Set 9:
That sounds like a pretty nasty caveat. Can you make this just a clean-up CL that doesn't have performance implications? Then we can merge this and not worry about CL 107928 needing to happen in the same release. Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
Message from Anit Gandhi: Patch Set 9:
Agreed/understood. Small clarification (sorry for the poor wording on this previously) - it's anyone with an arm64, with the AES and PMULL CPU features that would face the performance penalty. That's because of this line (https://go-review.googlesource.com/c/go/+/105695/9/src/crypto/tls/common.go#924). But your point is still valid, this should updated to be separate. Since the rest of the cleanup in this CL is done, would it be acceptable to replace that line with Please don’t reply on this GitHub thread. Visit golang.org/cl/105695. |
|
/comments off |
…of cipherhw When the internal/cpu package was introduced, the AES package still used the custom crypto/internal/cipherhw package for amd64 and s390x. This change removes that package entirely in favor of directly referencing the cpu feature flags set and exposed by the internal/cpu package. In addition, 5 new flags have been added to the internal/cpu s390x struct for detecting various cipher message (KM) features. Change-Id: I77cdd8bc1b04ab0e483b21bf1879b5801a4ba5f4 GitHub-Last-Rev: a611e3e GitHub-Pull-Request: #24766 Reviewed-on: https://go-review.googlesource.com/105695 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
…of cipherhw When the internal/cpu package was introduced, the AES package still used the custom crypto/internal/cipherhw package for amd64 and s390x. This change removes that package entirely in favor of directly referencing the cpu feature flags set and exposed by the internal/cpu package. In addition, 5 new flags have been added to the internal/cpu s390x struct for detecting various cipher message (KM) features. Change-Id: I77cdd8bc1b04ab0e483b21bf1879b5801a4ba5f4 GitHub-Last-Rev: a611e3ecb1f480dcbfce3cb0c8c9e4058f56c1a4 GitHub-Pull-Request: golang/go#24766 Reviewed-on: https://go-review.googlesource.com/105695 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
…of cipherhw When the internal/cpu package was introduced, the AES package still used the custom crypto/internal/cipherhw package for amd64 and s390x. This change removes that package entirely in favor of directly referencing the cpu feature flags set and exposed by the internal/cpu package. In addition, 5 new flags have been added to the internal/cpu s390x struct for detecting various cipher message (KM) features. Change-Id: I77cdd8bc1b04ab0e483b21bf1879b5801a4ba5f4 GitHub-Last-Rev: a611e3e GitHub-Pull-Request: golang#24766 Reviewed-on: https://go-review.googlesource.com/105695 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
…of cipherhw When the internal/cpu package was introduced, the AES package still used the custom crypto/internal/cipherhw package for amd64 and s390x. This change removes that package entirely in favor of directly referencing the cpu feature flags set and exposed by the internal/cpu package. In addition, 5 new flags have been added to the internal/cpu s390x struct for detecting various cipher message (KM) features. Change-Id: I77cdd8bc1b04ab0e483b21bf1879b5801a4ba5f4 GitHub-Last-Rev: a611e3ecb1f480dcbfce3cb0c8c9e4058f56c1a4 GitHub-Pull-Request: golang/go#24766 Reviewed-on: https://go-review.googlesource.com/105695 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
…of cipherhw When the internal/cpu package was introduced, the AES package still used the custom crypto/internal/cipherhw package for amd64 and s390x. This change removes that package entirely in favor of directly referencing the cpu feature flags set and exposed by the internal/cpu package. In addition, 5 new flags have been added to the internal/cpu s390x struct for detecting various cipher message (KM) features. Change-Id: I77cdd8bc1b04ab0e483b21bf1879b5801a4ba5f4 GitHub-Last-Rev: a611e3ecb1f480dcbfce3cb0c8c9e4058f56c1a4 GitHub-Pull-Request: golang/go#24766 Reviewed-on: https://go-review.googlesource.com/105695 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
…of cipherhw When the internal/cpu package was introduced, the AES package still used the custom crypto/internal/cipherhw package for amd64 and s390x. This change removes that package entirely in favor of directly referencing the cpu feature flags set and exposed by the internal/cpu package. In addition, 5 new flags have been added to the internal/cpu s390x struct for detecting various cipher message (KM) features. Change-Id: I77cdd8bc1b04ab0e483b21bf1879b5801a4ba5f4 GitHub-Last-Rev: a611e3ecb1f480dcbfce3cb0c8c9e4058f56c1a4 GitHub-Pull-Request: golang/go#24766 Reviewed-on: https://go-review.googlesource.com/105695 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
When the internal/cpu package was introduced, the AES package still used
the custom crypto/internal/cipherhw package for amd64 and s390x. This
change removes that package entirely in favor of directly referencing the
cpu feature flags set and exposed by the internal/cpu package. In
addition, 5 new flags have been added to the internal/cpu s390x struct
for detecting various cipher message (KM) features.