Closed Bug 1629414 Opened 4 years ago Closed 4 years ago

PPC64: VMX vs. VSX vector instructions

Categories

(NSS :: Libraries, defect, P3)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mozilla, Assigned: jcj, NeedInfo)

Details

Attachments

(1 file)

Bug 1629414 - Guard USE_PPC_CRYPTO and VSX types with __VSX__ and __ALTIVEC__
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:4.5) Goanna/20170101 PaleMoon/28.9.0.1

Steps to reproduce:

While compiling on Linux/PPC970 (Apple G5) the compiler stops and complains that there is VSX code but no command line argument "-mvsx".

The file "nss/lib/freebl/altivec-types.h" defines code for using the SIMD registers of the PPC-architecture.

Power ISA v.2.03 Altivec / VMX POWER6/PPC970 (Apple G5)
Power ISA v.2.06 VSX POWER7
Power ISA v.2.07 VSX-2 POWER8
Power ISA v.3.0 VSX-3 POWER9

The data type "long long" defined in "altivec-types.h" belongs to the VSX instruction set. This breaks compilation on "Power ISA v.2.03" systems.

I disabled these two lines and compiled NSS successfully on a PPC970 CPU.
This means there are currently no other VSX instructions in the code (otherwise the compiler would have complained.)

Be inspired by
/usr/lib/gcc/powerpc64-linux-gnu/7/include/altivec.h
I suggest encapsulating these two instructions like this:

#ifdef VSX
/* VSX - Power ISA v.2.06 - POWER7 CPU and later */
typedef __vector unsigned long long vec_u64;
typedef __vector signed long long vec_s64;
#endif

This way the newer VSX instructions are available if the source code is compiled with the option "-mvsx" (gcc and clang compilers).

Thank you for reading this.

Sorry, the underscores got lost. It is :

#ifdef __VSX__
/* VSX - Power ISA v.2.06 - POWER7 CPU and later */
typedef __vector unsigned long long vec_u64;
typedef __vector signed long long vec_s64;
#endif

Resetting severity to default of --.

Thanks for raising this. PPC is not one of our officially supported platforms... but that said, USE_PPC_CRYPTO is basically our stand-in for __VSX__, and I'm not sure which is best to use here. I'm going to put up a patch that guards using it to match the rest, and maybe give that a shot, and if not, we can introduce the __VSX__ flag.

Assignee: nobody → jjones
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3

This avoids build errors on non-VSX architectures even when not compiling
the POWER accelerated code.

Can you try out the attached patch?

Flags: needinfo?(mozilla)

I successfully tried the patch on PPC970.

But "gcm.h" first includes "altivec-types.h" and defines "USE_PPC_CRYPTO" about 10 lines of code later. This way the VSX instructions in the "altivec-types.h" (with the patch from J.C. Jones) will never be enabled, will they?

Furthermore I would expect the code in "gcm.h" to define "USE_PPC_CRYPTO" only if the compiler was called with "-mvsx".
Maybe the code should also check for "-mcpu=power8" or "-mcpu=power9" as only these CPUs have special crypto hardware(?).

Flags: needinfo?(mozilla)

You're correct, I'll update the patch here to use VSX directly.

Attachment #9142478 - Attachment description: Bug 1629414 - Guard VSX types with USE_PPC_CRYPTO → Bug 1629414 - Guard VSX types with __VSX__

Please see the updated patch, see if that works. Thanks for understanding the delay in my responses, but we'll get this one way or another into NSS 3.53.

See Comment 8 :)

Flags: needinfo?(mozilla)

The patch looks OK.
May I suggest do modify it a little?

  1. I suggest to define "USE_PPC_CRYPTO" only if "VSX" is defined. This way "gcm-ppc.c" and other code has only to check for "USE_PPC_CRYPTO".
  2. If the code is compiled for POWER4 there is no Altivec engine at all. Thus the compiler will complain about the altivec types. Because of this I added another test for "ALTIVEC".
    There may still be a problem for POWER7 CPUs as I don't know if they have the crypto hardware.
Power ISA v.2.03        Altivec / VMX   POWER6/PPC970
Power ISA v.2.06        VSX             POWER7
Power ISA v.2.07        VSX-2           POWER8
Power ISA v.3.0         VSX-3           POWER9 

diff -r 92058f185316 lib/freebl/altivec-types.h
--- a/lib/freebl/altivec-types.h        Tue Apr 07 10:51:00 2020 -0700
+++ b/lib/freebl/altivec-types.h        Mon May 04 22:47:57 2020 +0200
@@ -16,8 +16,10 @@
 typedef __vector signed short vec_s16;
 typedef __vector unsigned int vec_u32;
 typedef __vector signed int vec_s32;
+#ifdef __VSX__
 typedef __vector unsigned long long vec_u64;
 typedef __vector signed long long vec_s64;
+#endif
 typedef __vector float vec_f;
 
 #endif
diff -r 92058f185316 lib/freebl/gcm.h
--- a/lib/freebl/gcm.h  Tue Apr 07 10:51:00 2020 -0700
+++ b/lib/freebl/gcm.h  Mon May 04 22:47:57 2020 +0200
@@ -31,7 +31,7 @@
 #include <arm_neon.h>
 #endif
 
-#if defined(__powerpc64__) && !defined(NSS_DISABLE_ALTIVEC)
+#if defined(__powerpc64__) && !defined(NSS_DISABLE_ALTIVEC) && defined(__ALTIVEC__)
 #include "altivec-types.h"
 
 /* The ghash freebl test tries to use this in C++, and gcc defines conflict. */
@@ -45,9 +45,12 @@
  * PPC CRYPTO requires at least gcc 8 or clang. The LE check is purely
  * because it's only been tested on LE. If you're interested in BE,
  * please send a patch.
+ * Only use PPC CRYPTO if the compiler is allowed to use VSX instructions
+ * otherwise it compiles for older hardware, e.g. PPC970 or POWER6.
  */
 #if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 8)) && \
-    defined(IS_LITTLE_ENDIAN)
+    defined(IS_LITTLE_ENDIAN) && \
+    defined(__VSX__)
 #define USE_PPC_CRYPTO
 #endif

In addition to this there is the following problem:
Compiling on PPC970 the compilation stops and gcc complains that it has to deal with altivec/vmx code but there was no "-maltivec" on the gcc command line. I looked at the failing gcc instruction and added "-maltivec" by hand and the compilation worked.
But this may be my fault as I forgot to specify something to make?

Thank you.

Flags: needinfo?(mozilla)

Thanks! So curious note, your local copy doesn't have Bug 1613238's sha2 vector acceleration, which changes the files around a little bit versus your patch. But your point about only defining USE_PPC_CRYPTO if __VSX__ is noted. Since we apply -mvsx to all those files, it sure looks like we're requiring VSX and ALTIVEC for PPC crypto anyway. I'll adjust the patch to more look like your proposal and post it shortly.

That said, maltivec is defined in Gyp for gcm-aes-ppc_c_lib, gcm-sha512-ppc_c_lib, and gcm-sha512-nodepend-ppc_c_lib. In the Makefile it's defined for gcm, aes, and sha512. Which build system were you using, and what file had that complaint?

Flags: needinfo?(mozilla)
Attachment #9142478 - Attachment description: Bug 1629414 - Guard VSX types with __VSX__ → Bug 1629414 - Guard USE_PPC_CRYPTO and VSX types with __VSX__ and __ALTIVEC__

My repository was created about the time I reported this bug. May be some days earlier.

Which build system were you using, and what file had that complaint?

NSS is included in the palemoon web browser (www.palemoon.org; a fork of Firefox that still includes XUL).
While compiling 28.9.0.2 I had 2 problems:

  1. not guarded vector instructions
  2. there were these three compiler instructions that needed "-maltivec" added. I did this manually at the end of the instructions and everything went well. Sorry, but I don't know how to identify the NSS version included in palemoon.
/usr/bin/gcc -std=gnu99 -o /usr/src/ralf/pmbuild_28.9.0.2/security/nss/lib/freebl/gcm.o -c -std=c99 -O2 -gdwarf-2 -fPIC  -m64 -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_LOWHASH -DNSS_NO_INIT_SUPPORT -DHAVE_INT128_SUPPORT -DMP_API_COMPATIBLE -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nspr -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nspr -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nss -I/usr/src/ralf/pmbuild_28.9.0.2/dist/private/nss -Impi -Iecl -Iverified -DNSS_ENABLE_TLS13_SHORT_HEADERS gcm.c -maltivec

/usr/bin/gcc -std=gnu99 -o /usr/src/ralf/pmbuild_28.9.0.2/security/nss/lib/freebl/hmacct.o -c -std=c99 -O2 -gdwarf-2 -fPIC  -m64 -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_LOWHASH -DNSS_NO_INIT_SUPPORT -DHAVE_INT128_SUPPORT -DMP_API_COMPATIBLE -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nspr -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nspr -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nss -I/usr/src/ralf/pmbuild_28.9.0.2/dist/private/nss -Impi -Iecl -Iverified -DNSS_ENABLE_TLS13_SHORT_HEADERS hmacct.c -maltivec

/usr/bin/gcc -std=gnu99 -o /usr/src/ralf/pmbuild_28.9.0.2/security/nss/lib/freebl/rijndael.o -c -std=c99 -O2 -gdwarf-2 -fPIC  -m64 -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DNSS_USE_64 -DFREEBL_LOWHASH -DNSS_NO_INIT_SUPPORT -DHAVE_INT128_SUPPORT -DMP_API_COMPATIBLE -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nspr -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nspr -I/usr/src/ralf/pmbuild_28.9.0.2/dist/include/nss -I/usr/src/ralf/pmbuild_28.9.0.2/dist/private/nss -Impi -Iecl -Iverified -DNSS_ENABLE_TLS13_SHORT_HEADERS rijndael.c -maltivec

My original post about this problem is here: https://forum.palemoon.org/viewtopic.php?f=62&t=24153
But the folks at palemoon.org told me to report upstream.

Thank you for helping.

Flags: needinfo?(mozilla)

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Assignee: jjones → nobody
Severity: normal → S3
Status: ASSIGNED → NEW

Looks like NSS 3.48: https://github.com/MoonchildProductions/UXP/blob/221627575b56e8bb85e8329400da99138f2f67c1/security/nss/lib/util/nssutil.h#L22

It's hard to tell whether our build changes would get the -maltivec where it needs to go or not. I'd suggest we land this patch pretty much as-is (with review) and have you try building the new NSS directly, see if it works?

Assignee: nobody → jjones
Status: NEW → ASSIGNED

^ see Comment 14 :) Have a good weekend!

Flags: needinfo?(mozilla)

I did a complete new checkout and switched to a tagged revision:

hg checkout NSS_3_52_1_RTM
hg update
hg install

I tried a build and the compiler complained about missing "-maltivec" and "-mvsx". This was expected.

./build.sh  --gcc --with-nspr=/usr/src/ralf/nspr/dist/include/nspr:/usr/src/ralf/nspr/dist/libninja: Entering directory `/usr/src/ralf/nss2/out/Debug'
[122/997] CC obj/lib/freebl/freebl_static.rijndael.o
FAILED: obj/lib/freebl/freebl_static.rijndael.o 
gcc -MMD -MF obj/lib/freebl/freebl_static.rijndael.o.d '-DSHLIB_SUFFIX="so"' '-DSHLIB_PREFIX="lib"' '-DSHLIB_VERSION="3"' '-DSOFTOKEN_SHLIB_VERSION="3"' -DRIJNDAEL_INCLUDE_TABLES -DMP_API_COMPATIBLE -DKRML_VERIFIED_UINT128 -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_USE_64 -DSEED_ONLY_DEV_URANDOM -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSQL_MEASURE_USE_TEMP_DIR -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_DBM -DNSS_DISABLE_LIBPKIX -DDEBUG -I../../lib/freebl/mpi -I../../lib/freebl/ecl -I../../lib/freebl/verified -I../../lib/freebl/verified/kremlin/include -I../../lib/freebl/verified/kremlin/kremlib/dist/minimal -I../../lib/freebl/deprecated -I/usr/src/ralf/nspr/dist/include/nspr -I/usr/src/ralf/dist/private/nss -I/usr/src/ralf/dist/public/nss -fPIC -pipe -ffunction-sections -fdata-sections -Werror -Wall -Wshadow -O0 -g -gdwarf-2 -std=c99  -c ../../lib/freebl/rijndael.c -o obj/lib/freebl/freebl_static.rijndael.o
In file included from ../../lib/freebl/altivec-types.h:11:0,
                 from ../../lib/freebl/ppc-crypto.h:9,
                 from ../../lib/freebl/gcm.h:35,
                 from ../../lib/freebl/rijndael.c:20:
/usr/lib/gcc/powerpc64-linux-gnu/7/include/altivec.h:34:2: error: #error Use the "-maltivec" flag to enable PowerPC AltiVec support
 #error Use the "-maltivec" flag to enable PowerPC AltiVec support

After that I downloaded the "raw patch" and saved it as patch.diff

   https://phabricator.services.mozilla.com/D72014#change-HTQTumMGxIhM

Afterwards I did

rm -rf /usr/src/ralf/dist
make clean

patch -p1 < patch.diff 

./build.sh  --gcc --with-nspr=/usr/src/ralf/nspr/dist/include/nspr:/usr/src/ralf/nspr/dist/lib
ninja: Entering directory `/usr/src/ralf/nss2/out/Debug'
[270/1146] CC obj/lib/freebl/deprecated/freebl_static.seed.o
FAILED: obj/lib/freebl/deprecated/freebl_static.seed.o 
gcc -MMD -MF obj/lib/freebl/deprecated/freebl_static.seed.o.d '-DSHLIB_SUFFIX="so"' '-DSHLIB_PREFIX="lib"' '-DSHLIB_VERSION="3"' '-DSOFTOKEN_SHLIB_VERSION="3"' -DRIJNDAEL_INCLUDE_TABLES -DMP_API_COMPATIBLE -DKRML_VERIFIED_UINT128 -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_USE_64 -DSEED_ONLY_DEV_URANDOM -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSQL_MEASURE_USE_TEMP_DIR -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_DBM -DNSS_DISABLE_LIBPKIX -DDEBUG -I../../lib/freebl/mpi -I../../lib/freebl/ecl -I../../lib/freebl/verified -I../../lib/freebl/verified/kremlin/include -I../../lib/freebl/verified/kremlin/kremlib/dist/minimal -I../../lib/freebl/deprecated -I/usr/src/ralf/nspr/dist/include/nspr -I/usr/src/ralf/dist/private/nss -I/usr/src/ralf/dist/public/nss -fPIC -pipe -ffunction-sections -fdata-sections -Werror -Wall -Wshadow -O0 -g -gdwarf-2 -std=c99  -c ../../lib/freebl/deprecated/seed.c -o obj/lib/freebl/deprecated/freebl_static.seed.o
gcc: error: ../../lib/freebl/deprecated/seed.c: Datei oder Verzeichnis nicht gefunden
gcc: fatal error: no input files

The directory "deprecated" seems missing and the source files were still in lib/freebl so I added a link.

pushd .
cd lib/freebl
ln -s . deprecated
popd
make clean
rm -rf /usr/src/ralf/dist

./build.sh  --gcc --with-nspr=/usr/src/ralf/nspr/dist/include/nspr:/usr/src/ralf/nspr/dist/lib

ninja: Entering directory `/usr/src/ralf/nss2/out/Debug'
[344/1146] CC obj/lib/pk11wrap/pk11wrap.pk11load.o
FAILED: obj/lib/pk11wrap/pk11wrap.pk11load.o 
gcc -MMD -MF obj/lib/pk11wrap/pk11wrap.pk11load.o.d '-DSHLIB_SUFFIX="so"' '-DSHLIB_PREFIX="lib"' '-DNSS_SHLIB_VERSION="3"' '-DSOFTOKEN_SHLIB_VERSION="3"' -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_USE_64 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -DSQL_MEASURE_USE_TEMP_DIR -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_DBM -DNSS_DISABLE_LIBPKIX -DDEBUG -I/usr/src/ralf/nspr/dist/include/nspr -I/usr/src/ralf/dist/private/nss -I/usr/src/ralf/dist/public/nss -fPIC -pipe -ffunction-sections -fdata-sections -Werror -Wall -Wshadow -O0 -g -gdwarf-2 -std=c99  -c ../../lib/pk11wrap/pk11load.c -o obj/lib/pk11wrap/pk11wrap.pk11load.o
../../lib/pk11wrap/pk11load.c:358:24: error: expected ‘,’ or ‘;’ before ‘SHLIB_VERSION’
     SHLIB_PREFIX "nss" SHLIB_VERSION "." SHLIB_SUFFIX;
                        ^~~~~~~~~~~~~
[346/1146] CC obj/lib/pk11wrap/pk11wrap.pk11merge.o
ninja: build stopped: subcommand failed.

Because the compiler did not complain about "-maltivec" and "-mvsx" anymore the patch seems to work.

For the other errors I currently have no idea. (Did I handle Mercurial (hg) right?)

@ J.C. Jones: Thank you very much for helping so far.

Absolutely! Thanks for your help, too.

The commands I use for this are:

hg clone https://hg.mozilla.org/projects/nss
cd nss
hg update NSS_3_52_1_RTM
curl -L "https://phabricator.services.mozilla.com/D72014?download=true" | hg import - -m "test commit"
./build.sh --with-nspr=~/hg/dist/lib:~/hg/dist/include/nspr

The thing with the missing directory is because of the two hg checkout (update is an alias) commands, I think, stranding changes from trunk when what you wanted was just NSS_3_52_1_RTM. hg status would have shown lots of untracked changes, I think, needing a hg revert --all.

Anyway, I think those compile errors are because of the half-merged state your repo was in. Feel free to try as above, but I'm guessing this patch will fix the issue nonetheless. :)

Happy Friday!

https://hg.mozilla.org/projects/nss/rev/c7a1c91cd9befb627eb908965ae3a611b381c7ad

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: