Skip to content

Commit

Permalink
Bug 1608327 - Fix freebl arm NEON code use on tier3 platforms. r=jcj
Browse files Browse the repository at this point in the history
Despite the code having runtime detection of NEON and crypto extensions,
the optimized code using those instructions is disabled at build time on
platforms where the compiler doesn't enable NEON by default of with the
flags it's given for the caller code.

In the case of gcm, this goes as far as causing a build error.

What is needed is for the optimized code to be enabled in every case,
letting the caller code choose whether to use that code based on the
existing runtime checks.

But this can't be simply done either, because those optimized parts of
the code need to be built with NEON enabled, unconditionally, but that
is not compatible with platforms using the softfloat ABI. For those,
we need to use the softfp ABI, which is compatible. However, the softfp
ABI is not compatible with the hardfp ABI, so we also can't
unconditionally use the softfp ABI, so we do so only when the compiler
targets the softfloat ABI, which confusingly enough is advertized via
the `__SOFTFP__` define.

Differential Revision: https://phabricator.services.mozilla.com/D59451

--HG--
extra : moz-landing-system : lando
  • Loading branch information
glandium committed Jan 14, 2020
1 parent 552234e commit 332047c
Show file tree
Hide file tree
Showing 6 changed files with 25 additions and 16 deletions.
8 changes: 6 additions & 2 deletions lib/freebl/Makefile
Expand Up @@ -765,8 +765,12 @@ $(OBJDIR)/$(PROG_PREFIX)intel-gcm-wrap$(OBJ_SUFFIX): CFLAGS += -mssse3
endif

ifeq ($(CPU_ARCH),arm)
$(OBJDIR)/$(PROG_PREFIX)aes-armv8$(OBJ_SUFFIX): CFLAGS += -march=armv8-a -mfpu=crypto-neon-fp-armv8
$(OBJDIR)/$(PROG_PREFIX)gcm-arm32-neon$(OBJ_SUFFIX): CFLAGS += -mfpu=neon
# When the compiler uses the softfloat ABI, we want to use the compatible softfp ABI when
# enabling NEON for these objects.
# Confusingly, __SOFTFP__ is the name of the define for the softfloat ABI, not for the softfp ABI.
USES_SOFTFLOAT_ABI := $(shell $(CC) -o - -E -dM - $(CFLAGS) < /dev/null | grep __SOFTFP__ > /dev/null && echo 1)
$(OBJDIR)/$(PROG_PREFIX)aes-armv8$(OBJ_SUFFIX): CFLAGS += -march=armv8-a -mfpu=crypto-neon-fp-armv8$(if $(USES_SOFTFLOAT_ABI), -mfloat-abi=softfp)
$(OBJDIR)/$(PROG_PREFIX)gcm-arm32-neon$(OBJ_SUFFIX): CFLAGS += -mfpu=neon$(if $(USES_SOFTFLOAT_ABI), -mfloat-abi=softfp)
endif
ifeq ($(CPU_ARCH),aarch64)
$(OBJDIR)/$(PROG_PREFIX)aes-armv8$(OBJ_SUFFIX): CFLAGS += -march=armv8-a+crypto
Expand Down
2 changes: 1 addition & 1 deletion lib/freebl/aes-armv8.c
Expand Up @@ -8,7 +8,7 @@
#if ((defined(__clang__) || \
(defined(__GNUC__) && defined(__GNUC_MINOR__) && \
(__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ > 8)))) && \
(defined(__ARM_NEON) || defined(__ARM_NEON__)))
defined(IS_LITTLE_ENDIAN))

#ifndef __ARM_FEATURE_CRYPTO
#error "Compiler option is invalid"
Expand Down
17 changes: 13 additions & 4 deletions lib/freebl/freebl.gyp
Expand Up @@ -158,10 +158,12 @@
'<(DEPTH)/exports.gyp:nss_exports'
],
'cflags': [
'-mfpu=neon'
'-mfpu=neon',
'<@(softfp_cflags)',
],
'cflags_mozilla': [
'-mfpu=neon'
'-mfpu=neon',
'<@(softfp_cflags)',
]
},
{
Expand Down Expand Up @@ -211,11 +213,13 @@
[ 'target_arch=="arm"', {
'cflags': [
'-march=armv8-a',
'-mfpu=crypto-neon-fp-armv8'
'-mfpu=crypto-neon-fp-armv8',
'<@(softfp_cflags)',
],
'cflags_mozilla': [
'-march=armv8-a',
'-mfpu=crypto-neon-fp-armv8'
'-mfpu=crypto-neon-fp-armv8',
'<@(softfp_cflags)',
],
}, 'target_arch=="arm64" or target_arch=="aarch64"', {
'cflags': [
Expand Down Expand Up @@ -567,6 +571,11 @@
}, {
'have_int128_support%': 0,
}],
[ 'target_arch=="arm"', {
# When the compiler uses the softfloat ABI, we want to use the compatible softfp ABI when enabling NEON for these objects.
# Confusingly, __SOFTFP__ is the name of the define for the softfloat ABI, not for the softfp ABI.
'softfp_cflags': '<!(${CC:-cc} -o - -E -dM - ${CFLAGS} < /dev/null | grep __SOFTFP__ > /dev/null && echo -mfloat-abi=softfp || true)',
}],
],
}
}
4 changes: 2 additions & 2 deletions lib/freebl/gcm-arm32-neon.c
Expand Up @@ -11,7 +11,7 @@
#include "secerr.h"
#include "prtypes.h"

#if defined(__ARM_NEON__) || defined(__ARM_NEON)
#if defined(IS_LITTLE_ENDIAN)

#include <arm_neon.h>

Expand Down Expand Up @@ -199,4 +199,4 @@ gcm_HashZeroX_hw(gcmHashContext *ghash)
return SECSuccess;
}

#endif /* __ARM_NEON__ || __ARM_NEON */
#endif /* IS_LITTLE_ENDIAN */
7 changes: 2 additions & 5 deletions lib/freebl/gcm.c
Expand Up @@ -21,11 +21,8 @@
#if defined(__aarch64__) && defined(IS_LITTLE_ENDIAN) && \
(defined(__clang__) || defined(__GNUC__) && __GNUC__ > 6)
#define USE_ARM_GCM
#elif defined(__arm__) && defined(IS_LITTLE_ENDIAN) && \
(defined(__ARM_NEON__) || defined(__ARM_NEON))
/* We don't test on big endian platform, so disable this on big endian.
* Also, we don't check whether compiler support NEON well, so this uses
* that compiler uses -mfpu=neon only. */
#elif defined(__arm__) && defined(IS_LITTLE_ENDIAN)
/* We don't test on big endian platform, so disable this on big endian. */
#define USE_ARM_GCM
#endif

Expand Down
3 changes: 1 addition & 2 deletions lib/freebl/rijndael.c
Expand Up @@ -20,8 +20,7 @@
#include "gcm.h"
#include "mpi.h"

#if (!defined(IS_LITTLE_ENDIAN) && !defined(NSS_X86_OR_X64)) || \
(defined(__arm__) && !defined(__ARM_NEON) && !defined(__ARM_NEON__))
#if !defined(IS_LITTLE_ENDIAN) && !defined(NSS_X86_OR_X64)
// not test yet on big endian platform of arm
#undef USE_HW_AES
#endif
Expand Down

0 comments on commit 332047c

Please sign in to comment.