Re: [PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

Re: [PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Will Deacon]

Hi Ard,

On Sat, Nov 12, 2016 at 01:32:33PM +0100, Ard Biesheuvel wrote:
> This integrates both the accelerated scalar and the NEON implementations
> of SHA-224/256 as well as SHA-384/512 from the OpenSSL project.
> 
> Relative performance compared to the respective generic C versions:
> 
>                  |  SHA256-scalar  | SHA256-NEON* |  SHA512  |
>      ------------+-----------------+--------------+----------+
>      Cortex-A53  |      1.63x      |     1.63x    |   2.34x  |
>      Cortex-A57  |      1.43x      |     1.59x    |   1.95x  |
>      Cortex-A73  |      1.26x      |     1.56x    |     ?    |
> 
> The core crypto code was authored by Andy Polyakov of the OpenSSL
> project, in collaboration with whom the upstream code was adapted so
> that this module can be built from the same version of sha512-armv8.pl.
> 
> The version in this patch was taken from OpenSSL commit
> 
>    866e505e0d66 sha/asm/sha512-armv8.pl: add NEON version of SHA256.
> 
> * The core SHA algorithm is fundamentally sequential, but there is a
>   secondary transformation involved, called the schedule update, which
>   can be performed independently. The NEON version of SHA-224/SHA-256
>   only implements this part of the algorithm using NEON instructions,
>   the sequential part is always done using scalar instructions.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v3: at Will's request, the generated assembly files are now included
>     as .S_shipped files, for which generic build rules are defined
>     already. Note that this has caused issues in the past with
>     patchwork, so for Herbert's convenience, the patch can be pulled
>     from http://git.kernel.org/cgit/linux/kernel/git/ardb/linux.git,
>     branch arm64-sha256 (based on today's cryptodev)

Thanks.

Looking at the generated code, I see references to __ARMEB__ and __ILP32__.
The former is probably a bug, whilst the second is not required. There are
also some commented out instructions, which is weird.

Will

[PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Will Deacon]

Hi Ard,

On Sat, Nov 12, 2016 at 01:32:33PM +0100, Ard Biesheuvel wrote:
> This integrates both the accelerated scalar and the NEON implementations
> of SHA-224/256 as well as SHA-384/512 from the OpenSSL project.
> 
> Relative performance compared to the respective generic C versions:
> 
>                  |  SHA256-scalar  | SHA256-NEON* |  SHA512  |
>      ------------+-----------------+--------------+----------+
>      Cortex-A53  |      1.63x      |     1.63x    |   2.34x  |
>      Cortex-A57  |      1.43x      |     1.59x    |   1.95x  |
>      Cortex-A73  |      1.26x      |     1.56x    |     ?    |
> 
> The core crypto code was authored by Andy Polyakov of the OpenSSL
> project, in collaboration with whom the upstream code was adapted so
> that this module can be built from the same version of sha512-armv8.pl.
> 
> The version in this patch was taken from OpenSSL commit
> 
>    866e505e0d66 sha/asm/sha512-armv8.pl: add NEON version of SHA256.
> 
> * The core SHA algorithm is fundamentally sequential, but there is a
>   secondary transformation involved, called the schedule update, which
>   can be performed independently. The NEON version of SHA-224/SHA-256
>   only implements this part of the algorithm using NEON instructions,
>   the sequential part is always done using scalar instructions.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v3: at Will's request, the generated assembly files are now included
>     as .S_shipped files, for which generic build rules are defined
>     already. Note that this has caused issues in the past with
>     patchwork, so for Herbert's convenience, the patch can be pulled
>     from http://git.kernel.org/cgit/linux/kernel/git/ardb/linux.git,
>     branch arm64-sha256 (based on today's cryptodev)

Thanks.

Looking at the generated code, I see references to __ARMEB__ and __ILP32__.
The former is probably a bug, whilst the second is not required. There are
also some commented out instructions, which is weird.

Will

Re: [PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Andy Polyakov]

> (+ Andy)
> 
> ...
>>
>> Looking at the generated code, I see references to __ARMEB__ and
> __ILP32__.
>> The former is probably a bug,

Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__! Apparently I
simply went on assuming that it would be __ARMEB__/__ARMEL__ even in
64-bit case. As Ard mentioned, there is a number of modules that can be
compiled for either 32- or 64-bit case, and supposedly that's where this
blunder stems from. Will fix... As for it being actually tested. No, I
don't have big-endian AArch64 setup, and big-endian segments in AArch64
are based on experience with endian neutrality elsewhere. I.e. it's
based on observations what it takes to achieve the neutrality elsewhere,
e.g. ARM, MIPS, PPC, and then exercising equivalent approach even here.
Modulo fact that I apparently got macros wrong :-(

>> whilst the second is not required.

Note that references to __ILP32__ are also inside #ifndef
__KERNEL__/#endif, i.e. they won't be even evaluated when compiled for
kernel. Or in other words it's shared code artefact just like #if[n]def
__KERNEL__ itself. You either disregard it or remove altogether, but
then code becomes specific and not shared :-)

>> There are
>> also some commented out instructions, which is weird.

Commented instructions denote those that are moved into next round in
order to improve instruction scheduling on either of processors. In
other words it means that you should spot it further below [in generated
code]. Yes, it's weird, but it helps me to remember which instructions
are moved. On side note it's not uncommon that instruction scheduling is
result of compromise. In general I attempt to schedule instruction for
near-optimal performance on *multiple* processors, but sometimes you
have to make compromises. One is mentioned in commentary section for
this very module, the Apple A7 vs. Cortex-A53 thing. Well, it's not like
the commented instruction you're observing is the only result of the
compromise, it's merely an example of how said compromises can manifest
themselves, as somewhat weird irregularities in the code, possibly with
commented instructions.

[PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Andy Polyakov]

> (+ Andy)
> 
> ...
>>
>> Looking at the generated code, I see references to __ARMEB__ and
> __ILP32__.
>> The former is probably a bug,

Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__! Apparently I
simply went on assuming that it would be __ARMEB__/__ARMEL__ even in
64-bit case. As Ard mentioned, there is a number of modules that can be
compiled for either 32- or 64-bit case, and supposedly that's where this
blunder stems from. Will fix... As for it being actually tested. No, I
don't have big-endian AArch64 setup, and big-endian segments in AArch64
are based on experience with endian neutrality elsewhere. I.e. it's
based on observations what it takes to achieve the neutrality elsewhere,
e.g. ARM, MIPS, PPC, and then exercising equivalent approach even here.
Modulo fact that I apparently got macros wrong :-(

>> whilst the second is not required.

Note that references to __ILP32__ are also inside #ifndef
__KERNEL__/#endif, i.e. they won't be even evaluated when compiled for
kernel. Or in other words it's shared code artefact just like #if[n]def
__KERNEL__ itself. You either disregard it or remove altogether, but
then code becomes specific and not shared :-)

>> There are
>> also some commented out instructions, which is weird.

Commented instructions denote those that are moved into next round in
order to improve instruction scheduling on either of processors. In
other words it means that you should spot it further below [in generated
code]. Yes, it's weird, but it helps me to remember which instructions
are moved. On side note it's not uncommon that instruction scheduling is
result of compromise. In general I attempt to schedule instruction for
near-optimal performance on *multiple* processors, but sometimes you
have to make compromises. One is mentioned in commentary section for
this very module, the Apple A7 vs. Cortex-A53 thing. Well, it's not like
the commented instruction you're observing is the only result of the
compromise, it's merely an example of how said compromises can manifest
themselves, as somewhat weird irregularities in the code, possibly with
commented instructions.

Re: [PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Ard Biesheuvel]

On 13 November 2016 at 15:12, Andy Polyakov <appro@openssl.org> wrote:
>> (+ Andy)
>>
>> ...
>>>
>>> Looking at the generated code, I see references to __ARMEB__ and
>> __ILP32__.
>>> The former is probably a bug,
>
> Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__!

Indeed:

$ aarch64-linux-gnu-gcc -dM -E  - <<<""   |grep AARCH
#define __AARCH64_CMODEL_SMALL__ 1
#define __AARCH64EL__ 1

$ aarch64-linux-gnu-gcc -dM -E -mbig-endian - <<<""   |grep AARCH
#define __AARCH64_CMODEL_SMALL__ 1
#define __AARCH64EB__ 1

-- 
Ard.

[PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Ard Biesheuvel]

On 13 November 2016 at 15:12, Andy Polyakov <appro@openssl.org> wrote:
>> (+ Andy)
>>
>> ...
>>>
>>> Looking at the generated code, I see references to __ARMEB__ and
>> __ILP32__.
>>> The former is probably a bug,
>
> Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__!

Indeed:

$ aarch64-linux-gnu-gcc -dM -E  - <<<""   |grep AARCH
#define __AARCH64_CMODEL_SMALL__ 1
#define __AARCH64EL__ 1

$ aarch64-linux-gnu-gcc -dM -E -mbig-endian - <<<""   |grep AARCH
#define __AARCH64_CMODEL_SMALL__ 1
#define __AARCH64EB__ 1

-- 
Ard.

Re: [PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Andy Polyakov]

>>> (+ Andy)
>>>
>>> ...
>>>>
>>>> Looking at the generated code, I see references to __ARMEB__ and
>>> __ILP32__.
>>>> The former is probably a bug,
>>
>> Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__!
> 
> Indeed:
> 
> $ aarch64-linux-gnu-gcc -dM -E  - <<<""   |grep AARCH
> #define __AARCH64_CMODEL_SMALL__ 1
> #define __AARCH64EL__ 1
> 
> $ aarch64-linux-gnu-gcc -dM -E -mbig-endian - <<<""   |grep AARCH
> #define __AARCH64_CMODEL_SMALL__ 1
> #define __AARCH64EB__ 1

As it turns out it wasn't really an overlook, at least not in OpenSSL
context, as I force __ARMEB__/__ARMEL__ in 64-bit build through
arm_arch.h, which in turn is shared between 32- and 64-bit builds. But
shared code should be preferred to be self-sufficient. It doesn't mean
that it won't be fixed, only that it wasn't really a bug in OpenSSL context.

[PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Andy Polyakov]

>>> (+ Andy)
>>>
>>> ...
>>>>
>>>> Looking at the generated code, I see references to __ARMEB__ and
>>> __ILP32__.
>>>> The former is probably a bug,
>>
>> Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__!
> 
> Indeed:
> 
> $ aarch64-linux-gnu-gcc -dM -E  - <<<""   |grep AARCH
> #define __AARCH64_CMODEL_SMALL__ 1
> #define __AARCH64EL__ 1
> 
> $ aarch64-linux-gnu-gcc -dM -E -mbig-endian - <<<""   |grep AARCH
> #define __AARCH64_CMODEL_SMALL__ 1
> #define __AARCH64EB__ 1

As it turns out it wasn't really an overlook, at least not in OpenSSL
context, as I force __ARMEB__/__ARMEL__ in 64-bit build through
arm_arch.h, which in turn is shared between 32- and 64-bit builds. But
shared code should be preferred to be self-sufficient. It doesn't mean
that it won't be fixed, only that it wasn't really a bug in OpenSSL context.

Re: [PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Andy Polyakov]

>>>>> Looking at the generated code, I see references to __ARMEB__ and
>>>> __ILP32__.
>>>>> The former is probably a bug,
>>>
>>> Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__!
>>
>> Indeed:
>>
>> $ aarch64-linux-gnu-gcc -dM -E  - <<<""   |grep AARCH
>> #define __AARCH64_CMODEL_SMALL__ 1
>> #define __AARCH64EL__ 1
>>
>> $ aarch64-linux-gnu-gcc -dM -E -mbig-endian - <<<""   |grep AARCH
>> #define __AARCH64_CMODEL_SMALL__ 1
>> #define __AARCH64EB__ 1
> 
> As it turns out it wasn't really an overlook, at least not in OpenSSL
> context, as I force __ARMEB__/__ARMEL__ in 64-bit build through
> arm_arch.h, which in turn is shared between 32- and 64-bit builds. But
> shared code should be preferred to be self-sufficient. It doesn't mean
> that it won't be fixed, only that it wasn't really a bug in OpenSSL context.
> 

https://github.com/openssl/openssl/pull/1914.

[PATCH v3] crypto: arm64/sha2: integrate OpenSSL implementations of SHA256/SHA512

[Andy Polyakov]

>>>>> Looking at the generated code, I see references to __ARMEB__ and
>>>> __ILP32__.
>>>>> The former is probably a bug,
>>>
>>> Oh! You mean that it should be __AARCH64EB__/__AARCH64EL__!
>>
>> Indeed:
>>
>> $ aarch64-linux-gnu-gcc -dM -E  - <<<""   |grep AARCH
>> #define __AARCH64_CMODEL_SMALL__ 1
>> #define __AARCH64EL__ 1
>>
>> $ aarch64-linux-gnu-gcc -dM -E -mbig-endian - <<<""   |grep AARCH
>> #define __AARCH64_CMODEL_SMALL__ 1
>> #define __AARCH64EB__ 1
> 
> As it turns out it wasn't really an overlook, at least not in OpenSSL
> context, as I force __ARMEB__/__ARMEL__ in 64-bit build through
> arm_arch.h, which in turn is shared between 32- and 64-bit builds. But
> shared code should be preferred to be self-sufficient. It doesn't mean
> that it won't be fixed, only that it wasn't really a bug in OpenSSL context.
> 

https://github.com/openssl/openssl/pull/1914.