| Message ID | 20200916061418.9197-1-ardb@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: arm/sha-neon - avoid ADRL instructions | expand |
On Tue, Sep 15, 2020 at 11:14 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > Remove some occurrences of ADRL in the SHA NEON code adopted from the > OpenSSL project. > > I will leave it to the Clang folks to decide whether this needs to be > backported and how far, but a Cc stable seems reasonable here. > > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Peter Smith <Peter.Smith@arm.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> Thanks Ard: compile+boot tested each combination of: [gcc, clang]x[defconfig, defconfig+CONFIG_THUMB2_KERNEL=y]. Now, if I additionally apply: 1. this series 2. the adr_l series: https://lore.kernel.org/linux-arm-kernel/20200914095706.3985-1-ardb@kernel.org/ 3. unrelated fix for -next #1: https://lore.kernel.org/lkml/20200916200255.1382086-1-ndesaulniers@google.com/ 4. unrelated fix for -next #2: https://lore.kernel.org/linux-mm/20200917001934.2793370-1-ndesaulniers@google.com/ 5. small fixup to 01/12 from #2: https://lore.kernel.org/linux-arm-kernel/CAMj1kXFmF_24d-7W8AWTJR-PCcja7bUdHhYKptGQmsV4vNp=sA@mail.gmail.com/ 6. vfp fixup for thumb+gcc: https://lore.kernel.org/linux-arm-kernel/CAMj1kXHEpPc0MSoMrCxEkyb44AkLM2NJJGtOXLpr6kxBHS0vfA@mail.gmail.com/ 7. disable CONFIG_IWMMXT https://github.com/ClangBuiltLinux/linux/issues/975 Then build with Clang's integrated assembler: $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 LLVM_IAS=1 -j71 I see a bunch of warnings (https://github.com/ClangBuiltLinux/linux/issues/858) which we will fix, but I am able to build and boot. (CONFIG_THUMB2_KERNEL=y has many more issues, so I didn't pursue that further). Either way, these two adrl patches go a long way towards getting Clang to assemble an ARCH=arm kernel; thank you for all of the work that went into them. One thing I noticed was that if I grep for `adrl` with all of the above applied within arch/arm, I do still see two more instances: crypto/sha256-armv4.pl 609: adrl $Ktbl,K256 crypto/sha256-core.S_shipped 2679: adrl r3,K256 Maybe those can be fixed up in patch 01/02 of this series for a v2? I guess in this cover letter, you did specify *some occurrences of ADRL*. It looks like those are guarded by 605 # ifdef __thumb2__ ... 608 # else 609 adrl $Ktbl,K256 So are these always built as thumb2? > > Ard Biesheuvel (2): > crypto: arm/sha256-neon - avoid ADRL pseudo instruction > crypto: arm/sha512-neon - avoid ADRL pseudo instruction > > arch/arm/crypto/sha256-armv4.pl | 4 ++-- > arch/arm/crypto/sha256-core.S_shipped | 4 ++-- > arch/arm/crypto/sha512-armv4.pl | 4 ++-- > arch/arm/crypto/sha512-core.S_shipped | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > -- > 2.17.1 >
On Thu, 17 Sep 2020 at 03:53, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Sep 15, 2020 at 11:14 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > Remove some occurrences of ADRL in the SHA NEON code adopted from the > > OpenSSL project. > > > > I will leave it to the Clang folks to decide whether this needs to be > > backported and how far, but a Cc stable seems reasonable here. > > > > Cc: Nick Desaulniers <ndesaulniers@google.com> > > Cc: Stefan Agner <stefan@agner.ch> > > Cc: Peter Smith <Peter.Smith@arm.com> > > Tested-by: Nick Desaulniers <ndesaulniers@google.com> > Thanks! > Thanks Ard: > compile+boot tested each combination of: > > [gcc, clang]x[defconfig, defconfig+CONFIG_THUMB2_KERNEL=y]. > > Now, if I additionally apply: > 1. this series > 2. the adr_l series: > https://lore.kernel.org/linux-arm-kernel/20200914095706.3985-1-ardb@kernel.org/ > 3. unrelated fix for -next #1: > https://lore.kernel.org/lkml/20200916200255.1382086-1-ndesaulniers@google.com/ > 4. unrelated fix for -next #2: > https://lore.kernel.org/linux-mm/20200917001934.2793370-1-ndesaulniers@google.com/ > 5. small fixup to 01/12 from #2: > https://lore.kernel.org/linux-arm-kernel/CAMj1kXFmF_24d-7W8AWTJR-PCcja7bUdHhYKptGQmsV4vNp=sA@mail.gmail.com/ > 6. vfp fixup for thumb+gcc: > https://lore.kernel.org/linux-arm-kernel/CAMj1kXHEpPc0MSoMrCxEkyb44AkLM2NJJGtOXLpr6kxBHS0vfA@mail.gmail.com/ > 7. disable CONFIG_IWMMXT https://github.com/ClangBuiltLinux/linux/issues/975 > > Then build with Clang's integrated assembler: > $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make LLVM=1 LLVM_IAS=1 -j71 > > I see a bunch of warnings > (https://github.com/ClangBuiltLinux/linux/issues/858) which we will > fix, but I am able to build and boot. (CONFIG_THUMB2_KERNEL=y has > many more issues, so I didn't pursue that further). > > Either way, these two adrl patches go a long way towards getting Clang > to assemble an ARCH=arm kernel; thank you for all of the work that > went into them. > My pleasure :-) > One thing I noticed was that if I grep for `adrl` with all of the > above applied within arch/arm, I do still see two more instances: > > crypto/sha256-armv4.pl > 609: adrl $Ktbl,K256 > > crypto/sha256-core.S_shipped > 2679: adrl r3,K256 > > Maybe those can be fixed up in patch 01/02 of this series for a v2? I > guess in this cover letter, you did specify *some occurrences of > ADRL*. It looks like those are guarded by > 605 # ifdef __thumb2__ > ... > 608 # else > 609 adrl $Ktbl,K256 > > So are these always built as thumb2? > No need. The code in question is never assembled when built as part of the kernel, only when building OpenSSL for user space. It appears upstream has removed this already, but they have also been playing weird games with the license blocks, so I'd prefer fixing the code here rather than pulling the latest version.
On Wed, Sep 16, 2020 at 11:08 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 17 Sep 2020 at 03:53, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > One thing I noticed was that if I grep for `adrl` with all of the > > above applied within arch/arm, I do still see two more instances: > > > > crypto/sha256-armv4.pl > > 609: adrl $Ktbl,K256 > > > > crypto/sha256-core.S_shipped > > 2679: adrl r3,K256 > > > > Maybe those can be fixed up in patch 01/02 of this series for a v2? I > > guess in this cover letter, you did specify *some occurrences of > > ADRL*. It looks like those are guarded by > > 605 # ifdef __thumb2__ > > ... > > 608 # else > > 609 adrl $Ktbl,K256 > > > > So are these always built as thumb2? > > > > No need. The code in question is never assembled when built as part of > the kernel, only when building OpenSSL for user space. It appears > upstream has removed this already, but they have also been playing > weird games with the license blocks, so I'd prefer fixing the code > here rather than pulling the latest version. Oh, like mixing and matching licenses throughout the source itself? Or changing the source license? (I've always wondered if software licenses apply to an entire repository, or were per source file? Could you mix and match licenses throughout your project? Not sure why you'd do that; maybe to make some parts reusable for some other project. But if you could, could you do different sections of a file under different licenses? Again, probably a worthless hypothetical; you could just split up your source files better).
On Fri, 18 Sep 2020 at 22:08, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Sep 16, 2020 at 11:08 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > On Thu, 17 Sep 2020 at 03:53, Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > > > One thing I noticed was that if I grep for `adrl` with all of the > > > above applied within arch/arm, I do still see two more instances: > > > > > > crypto/sha256-armv4.pl > > > 609: adrl $Ktbl,K256 > > > > > > crypto/sha256-core.S_shipped > > > 2679: adrl r3,K256 > > > > > > Maybe those can be fixed up in patch 01/02 of this series for a v2? I > > > guess in this cover letter, you did specify *some occurrences of > > > ADRL*. It looks like those are guarded by > > > 605 # ifdef __thumb2__ > > > ... > > > 608 # else > > > 609 adrl $Ktbl,K256 > > > > > > So are these always built as thumb2? > > > > > > > No need. The code in question is never assembled when built as part of > > the kernel, only when building OpenSSL for user space. It appears > > upstream has removed this already, but they have also been playing > > weird games with the license blocks, so I'd prefer fixing the code > > here rather than pulling the latest version. > > Oh, like mixing and matching licenses throughout the source itself? > Or changing the source license? > > (I've always wondered if software licenses apply to an entire > repository, or were per source file? Could you mix and match licenses > throughout your project? Not sure why you'd do that; maybe to make > some parts reusable for some other project. But if you could, could > you do different sections of a file under different licenses? Again, > probably a worthless hypothetical; you could just split up your source > files better). > https://github.com/openssl/openssl/blob/master/crypto/sha/asm/sha256-armv4.pl
On Wed, Sep 16, 2020 at 09:14:16AM +0300, Ard Biesheuvel wrote: > Remove some occurrences of ADRL in the SHA NEON code adopted from the > OpenSSL project. > > I will leave it to the Clang folks to decide whether this needs to be > backported and how far, but a Cc stable seems reasonable here. > > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Peter Smith <Peter.Smith@arm.com> > > Ard Biesheuvel (2): > crypto: arm/sha256-neon - avoid ADRL pseudo instruction > crypto: arm/sha512-neon - avoid ADRL pseudo instruction > > arch/arm/crypto/sha256-armv4.pl | 4 ++-- > arch/arm/crypto/sha256-core.S_shipped | 4 ++-- > arch/arm/crypto/sha512-armv4.pl | 4 ++-- > arch/arm/crypto/sha512-core.S_shipped | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) All applied. Thanks.