[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

Add patch to fix how NSS check if we're on a Big Endian machine.

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
---
Pending here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1606119
---
 ...6119-Fix-PPC-HW-Crypto-build-failure.patch | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch

diff --git a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
new file mode 100644
index 0000000000..0b891b5ebc
--- /dev/null
+++ b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
@@ -0,0 +1,37 @@
+From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00 2001
+From: Giulio Benetti <giulio.benetti@benettiengineering.com>
+Date: Fri, 27 Dec 2019 17:41:04 +0100
+Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
+
+Only Big Endian Altivec functions are used, not Little Endian ones, so
+let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
+instead of IS_LITTLE_ENDIAN.
+
+Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
+---
+ nss/lib/freebl/gcm.h | 6 +++---
+ 1 file changed, 3 insertions(+), 3 deletions(-)
+
+diff --git a/nss/lib/freebl/gcm.h b/nss/lib/freebl/gcm.h
+index 571b9ec55..de8b51db8 100644
+--- a/nss/lib/freebl/gcm.h
++++ b/nss/lib/freebl/gcm.h
+@@ -41,12 +41,12 @@
+ #endif
+ 
+ /*
+- * PPC CRYPTO requires at least gcc 5 or clang. The LE check is purely
+- * because it's only been tested on LE. If you're interested in BE,
++ * PPC CRYPTO requires at least gcc 5 or clang. The BE check is purely
++ * because it's only been tested on BE. If you're interested in LE,
+  * please send a patch.
+  */
+ #if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 5)) && \
+-    defined(IS_LITTLE_ENDIAN)
++    defined(IS_BIG_ENDIAN)
+ #define USE_PPC_CRYPTO
+ #endif
+ 
+-- 
+2.20.1
+
-- 
2.20.1

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

On 12/27/19 5:54 PM, Giulio Benetti wrote:
> Add patch to fix how NSS check if we're on a Big Endian machine.

Fixes:
http://autobuild.buildroot.net/results/e42/e42005d1bb70b3f2741eb77242b2fd8dbf2a55d6/

> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> ---
> Pending here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1606119
> ---
>   ...6119-Fix-PPC-HW-Crypto-build-failure.patch | 37 +++++++++++++++++++
>   1 file changed, 37 insertions(+)
>   create mode 100644 package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> 
> diff --git a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> new file mode 100644
> index 0000000000..0b891b5ebc
> --- /dev/null
> +++ b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> @@ -0,0 +1,37 @@
> +From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00 2001
> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
> +Date: Fri, 27 Dec 2019 17:41:04 +0100
> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
> +
> +Only Big Endian Altivec functions are used, not Little Endian ones, so
> +let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
> +instead of IS_LITTLE_ENDIAN.
> +
> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> +---
> + nss/lib/freebl/gcm.h | 6 +++---
> + 1 file changed, 3 insertions(+), 3 deletions(-)
> +
> +diff --git a/nss/lib/freebl/gcm.h b/nss/lib/freebl/gcm.h
> +index 571b9ec55..de8b51db8 100644
> +--- a/nss/lib/freebl/gcm.h
> ++++ b/nss/lib/freebl/gcm.h
> +@@ -41,12 +41,12 @@
> + #endif
> +
> + /*
> +- * PPC CRYPTO requires at least gcc 5 or clang. The LE check is purely
> +- * because it's only been tested on LE. If you're interested in BE,
> ++ * PPC CRYPTO requires at least gcc 5 or clang. The BE check is purely
> ++ * because it's only been tested on BE. If you're interested in LE,
> +  * please send a patch.
> +  */
> + #if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 5)) && \
> +-    defined(IS_LITTLE_ENDIAN)
> ++    defined(IS_BIG_ENDIAN)
> + #define USE_PPC_CRYPTO
> + #endif
> +
> +--
> +2.20.1
> +
> 

-- 
Giulio Benetti
Benetti Engineering sas

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Thomas Petazzoni]

On Fri, 27 Dec 2019 17:54:27 +0100
Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:

> diff --git a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> new file mode 100644
> index 0000000000..0b891b5ebc
> --- /dev/null
> +++ b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> @@ -0,0 +1,37 @@
> +From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00 2001
> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
> +Date: Fri, 27 Dec 2019 17:41:04 +0100
> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
> +
> +Only Big Endian Altivec functions are used, not Little Endian ones, so
> +let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
> +instead of IS_LITTLE_ENDIAN.
> +
> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>

I don't understand the reasoning here. From a quick look, the undefined
symbols reported in the build log come from the file
./nss/lib/freebl/gcm-ppc.c.

This file is included in the build in nss/lib/freebl/Makefile if
CPU_ARCH=ppc.

However CPU_ARCH=ppc is only set in nss/coreconf/Linux.mk as follows:

ifeq (,$(filter-out ppc64 ppc64le,$(OS_TEST)))
        CPU_ARCH        = ppc
ifeq ($(USE_64),1)
        ARCHFLAG        = -m64
endif

In libnss.mk, we pass OS_TEST=$(LIBNSS_ARCH), which basically is the
value of $(BR2_ARCH), which is powerpc64 or powerpc64le in Buildroot.

So, regardless of whether we are big endian PPC64 or little endian
PPC64, the gcm-ppc.c file will not be included in the build.

Am I missing something ? Could you explain a bit better how you came to
the conclusion you have in this patch ?

Thanks!


Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

Hi Thomas,

On 12/31/19 6:07 PM, Thomas Petazzoni wrote:
> On Fri, 27 Dec 2019 17:54:27 +0100
> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
> 
>> diff --git a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>> new file mode 100644
>> index 0000000000..0b891b5ebc
>> --- /dev/null
>> +++ b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>> @@ -0,0 +1,37 @@
>> +From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00 2001
>> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
>> +Date: Fri, 27 Dec 2019 17:41:04 +0100
>> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
>> +
>> +Only Big Endian Altivec functions are used, not Little Endian ones, so
>> +let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
>> +instead of IS_LITTLE_ENDIAN.
>> +
>> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> 
> I don't understand the reasoning here. From a quick look, the undefined
> symbols reported in the build log come from the file
> ./nss/lib/freebl/gcm-ppc.c.
> 
> This file is included in the build in nss/lib/freebl/Makefile if
> CPU_ARCH=ppc.
> 
> However CPU_ARCH=ppc is only set in nss/coreconf/Linux.mk as follows:
> 
> ifeq (,$(filter-out ppc64 ppc64le,$(OS_TEST)))
>          CPU_ARCH        = ppc
> ifeq ($(USE_64),1)
>          ARCHFLAG        = -m64
> endif
> 
> In libnss.mk, we pass OS_TEST=$(LIBNSS_ARCH), which basically is the
> value of $(BR2_ARCH), which is powerpc64 or powerpc64le in Buildroot.
> 
> So, regardless of whether we are big endian PPC64 or little endian
> PPC64, the gcm-ppc.c file will not be included in the build.
> 
> Am I missing something ? Could you explain a bit better how you came to
> the conclusion you have in this patch ?

The point is that gcm-ppc.c gets compiled, but inside it there's an #if 
defined(USE_PPC_CRYPTO) on top of the file, that is defined in gsm.h 
only #if __powerpc64__ and LITTLE_ENDIAN. But inside gcm-ppc.c they use 
_be() Altivec functions. Basically they have inverted LITTLE_ENDIAN with 
BIB_ENDIAN, since they use Big Endian only Altivec functions in 
gcm-ppc.c so the only wrong piece of code is in gcm.h, where 
USE_PPC_CRYPTO is defined.

Need only to invert the condition by which USE_PPC_CRYPTO is defined: 
BIG_ENDIAN instead of LITTLE_ENDIAN.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> 
> Thanks!
> 
> 
> Thomas
> 

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Vincent Fazio]

Gang,

Just going to toss my $.02 in...

On 12/31/19 3:24 PM, Giulio Benetti wrote:
> Hi Thomas,
>
> On 12/31/19 6:07 PM, Thomas Petazzoni wrote:
>> On Fri, 27 Dec 2019 17:54:27 +0100
>> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
>>
>>> diff --git 
>>> a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch 
>>> b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>>> new file mode 100644
>>> index 0000000000..0b891b5ebc
>>> --- /dev/null
>>> +++ 
>>> b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>>> @@ -0,0 +1,37 @@
>>> +From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00 2001
>>> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
>>> +Date: Fri, 27 Dec 2019 17:41:04 +0100
>>> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
>>> +
>>> +Only Big Endian Altivec functions are used, not Little Endian ones, so
>>> +let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
>>> +instead of IS_LITTLE_ENDIAN.
>>> +
>>> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
>>
>> I don't understand the reasoning here. From a quick look, the undefined
>> symbols reported in the build log come from the file
>> ./nss/lib/freebl/gcm-ppc.c.
>>
>> This file is included in the build in nss/lib/freebl/Makefile if
>> CPU_ARCH=ppc.
>>
>> However CPU_ARCH=ppc is only set in nss/coreconf/Linux.mk as follows:
>>
>> ifeq (,$(filter-out ppc64 ppc64le,$(OS_TEST)))
>> ???????? CPU_ARCH??????? = ppc
>> ifeq ($(USE_64),1)
>> ???????? ARCHFLAG??????? = -m64
>> endif
>>
>> In libnss.mk, we pass OS_TEST=$(LIBNSS_ARCH), which basically is the
>> value of $(BR2_ARCH), which is powerpc64 or powerpc64le in Buildroot.
>>
>> So, regardless of whether we are big endian PPC64 or little endian
>> PPC64, the gcm-ppc.c file will not be included in the build.
>>
>> Am I missing something ? Could you explain a bit better how you came to
>> the conclusion you have in this patch ?
>
> The point is that gcm-ppc.c gets compiled, but inside it there's an 
> #if defined(USE_PPC_CRYPTO) on top of the file, that is defined in 
> gsm.h only #if __powerpc64__ and LITTLE_ENDIAN. But inside gcm-ppc.c 
> they use _be() Altivec functions. Basically they have inverted 
> LITTLE_ENDIAN with BIB_ENDIAN, since they use Big Endian only Altivec 
> functions in gcm-ppc.c so the only wrong piece of code is in gcm.h, 
> where USE_PPC_CRYPTO is defined.
>
I assume you mean this in gcm_HashMult_hw:

 ??????? /* clang needs the following cast away from const; maybe a bug 
in 7.0.0 */
 ??????? v = (vec_u64)vec_xl_be(0, (unsigned char *)buf);

It should be fine to call endian specific functions if you know how the 
data is stored.

> Need only to invert the condition by which USE_PPC_CRYPTO is defined: 
> BIG_ENDIAN instead of LITTLE_ENDIAN.
>
> Best regards

Based on the below commit, it was tested on a POWER8, presumably in LE 
mode, so I'd be hesitant to switch any endian checks:
https://hg.mozilla.org/projects/nss/rev/3d7e509d6d20ecd607a28fa6ce42e4ffd9c51443

The bigger issue may be that 'vec_xl_be' wasn't introduced until GCC 8 
and the toolchain used in the failed build is GCC 7, so that function is 
not available in altivec.h:
https://github.com/gcc-mirror/gcc/commit/f7b0548e5eba54b637977e3df4d4daf0cabe474d

So if we stick with nss-3.48, the package should likely depend on 
BR2_TOOLCHAIN_GCC_AT_LEAST_8.

-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Vincent Fazio]

On 12/31/19 4:49 PM, Vincent Fazio wrote:
> Gang,
>
> Just going to toss my $.02 in...
>
> On 12/31/19 3:24 PM, Giulio Benetti wrote:
>> Hi Thomas,
>>
>> On 12/31/19 6:07 PM, Thomas Petazzoni wrote:
>>> On Fri, 27 Dec 2019 17:54:27 +0100
>>> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
>>>
>>>> diff --git 
>>>> a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch 
>>>> b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch 
>>>>
>>>> new file mode 100644
>>>> index 0000000000..0b891b5ebc
>>>> --- /dev/null
>>>> +++ 
>>>> b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch 
>>>>
>>>> @@ -0,0 +1,37 @@
>>>> +From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00 
>>>> 2001
>>>> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
>>>> +Date: Fri, 27 Dec 2019 17:41:04 +0100
>>>> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
>>>> +
>>>> +Only Big Endian Altivec functions are used, not Little Endian 
>>>> ones, so
>>>> +let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
>>>> +instead of IS_LITTLE_ENDIAN.
>>>> +
>>>> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
>>>
>>> I don't understand the reasoning here. From a quick look, the undefined
>>> symbols reported in the build log come from the file
>>> ./nss/lib/freebl/gcm-ppc.c.
>>>
>>> This file is included in the build in nss/lib/freebl/Makefile if
>>> CPU_ARCH=ppc.
>>>
>>> However CPU_ARCH=ppc is only set in nss/coreconf/Linux.mk as follows:
>>>
>>> ifeq (,$(filter-out ppc64 ppc64le,$(OS_TEST)))
>>> ???????? CPU_ARCH??????? = ppc
>>> ifeq ($(USE_64),1)
>>> ???????? ARCHFLAG??????? = -m64
>>> endif
>>>
>>> In libnss.mk, we pass OS_TEST=$(LIBNSS_ARCH), which basically is the
>>> value of $(BR2_ARCH), which is powerpc64 or powerpc64le in Buildroot.
>>>
>>> So, regardless of whether we are big endian PPC64 or little endian
>>> PPC64, the gcm-ppc.c file will not be included in the build.
>>>
>>> Am I missing something ? Could you explain a bit better how you came to
>>> the conclusion you have in this patch ?
>>
>> The point is that gcm-ppc.c gets compiled, but inside it there's an 
>> #if defined(USE_PPC_CRYPTO) on top of the file, that is defined in 
>> gsm.h only #if __powerpc64__ and LITTLE_ENDIAN. But inside gcm-ppc.c 
>> they use _be() Altivec functions. Basically they have inverted 
>> LITTLE_ENDIAN with BIB_ENDIAN, since they use Big Endian only Altivec 
>> functions in gcm-ppc.c so the only wrong piece of code is in gcm.h, 
>> where USE_PPC_CRYPTO is defined.
>>
> I assume you mean this in gcm_HashMult_hw:
>
> ??????? /* clang needs the following cast away from const; maybe a bug 
> in 7.0.0 */
> ??????? v = (vec_u64)vec_xl_be(0, (unsigned char *)buf);
>
> It should be fine to call endian specific functions if you know how 
> the data is stored.
>
>> Need only to invert the condition by which USE_PPC_CRYPTO is defined: 
>> BIG_ENDIAN instead of LITTLE_ENDIAN.
>>
>> Best regards
>
> Based on the below commit, it was tested on a POWER8, presumably in LE 
> mode, so I'd be hesitant to switch any endian checks:
> https://hg.mozilla.org/projects/nss/rev/3d7e509d6d20ecd607a28fa6ce42e4ffd9c51443 
>
>
> The bigger issue may be that 'vec_xl_be' wasn't introduced until GCC 8 
> and the toolchain used in the failed build is GCC 7, so that function 
> is not available in altivec.h:
> https://github.com/gcc-mirror/gcc/commit/f7b0548e5eba54b637977e3df4d4daf0cabe474d 
>
>
> So if we stick with nss-3.48, the package should likely depend on 
> BR2_TOOLCHAIN_GCC_AT_LEAST_8.
Or don't utilize PPC hardware accelerated instructions by undefining 
USE_PPC_CRYPTO... which is probably the saner option :-). This would 
give the semblance that the compile worked as expected since the hashing 
function will now be available, but Thomas' concern re CPU_ARCH and the 
future inclusion of ppc specific source is still valid.


-- 
Vincent Fazio
Embedded Software Engineer - Linux
Extreme Engineering Solutions, Inc
http://www.xes-inc.com

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

Hi Vincent,


On 1/1/20 12:31 AM, Vincent Fazio wrote:
> 
> On 12/31/19 4:49 PM, Vincent Fazio wrote:
>> Gang,
>>
>> Just going to toss my $.02 in...
>>
>> On 12/31/19 3:24 PM, Giulio Benetti wrote:
>>> Hi Thomas,
>>>
>>> On 12/31/19 6:07 PM, Thomas Petazzoni wrote:
>>>> On Fri, 27 Dec 2019 17:54:27 +0100
>>>> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
>>>>
>>>>> diff --git
>>>>> a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>>>>> b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>>>>>
>>>>> new file mode 100644
>>>>> index 0000000000..0b891b5ebc
>>>>> --- /dev/null
>>>>> +++
>>>>> b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
>>>>>
>>>>> @@ -0,0 +1,37 @@
>>>>> +From 09b3776a924736049693a118d5a8d883e8c794ca Mon Sep 17 00:00:00
>>>>> 2001
>>>>> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
>>>>> +Date: Fri, 27 Dec 2019 17:41:04 +0100
>>>>> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
>>>>> +
>>>>> +Only Big Endian Altivec functions are used, not Little Endian
>>>>> ones, so
>>>>> +let's change check if USE_PPC_CRYPTO by changing to IS_BIG_ENDIAN
>>>>> +instead of IS_LITTLE_ENDIAN.
>>>>> +
>>>>> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
>>>>
>>>> I don't understand the reasoning here. From a quick look, the undefined
>>>> symbols reported in the build log come from the file
>>>> ./nss/lib/freebl/gcm-ppc.c.
>>>>
>>>> This file is included in the build in nss/lib/freebl/Makefile if
>>>> CPU_ARCH=ppc.
>>>>
>>>> However CPU_ARCH=ppc is only set in nss/coreconf/Linux.mk as follows:
>>>>
>>>> ifeq (,$(filter-out ppc64 ppc64le,$(OS_TEST)))
>>>>  ???????? CPU_ARCH??????? = ppc
>>>> ifeq ($(USE_64),1)
>>>>  ???????? ARCHFLAG??????? = -m64
>>>> endif
>>>>
>>>> In libnss.mk, we pass OS_TEST=$(LIBNSS_ARCH), which basically is the
>>>> value of $(BR2_ARCH), which is powerpc64 or powerpc64le in Buildroot.
>>>>
>>>> So, regardless of whether we are big endian PPC64 or little endian
>>>> PPC64, the gcm-ppc.c file will not be included in the build.
>>>>
>>>> Am I missing something ? Could you explain a bit better how you came to
>>>> the conclusion you have in this patch ?
>>>
>>> The point is that gcm-ppc.c gets compiled, but inside it there's an
>>> #if defined(USE_PPC_CRYPTO) on top of the file, that is defined in
>>> gsm.h only #if __powerpc64__ and LITTLE_ENDIAN. But inside gcm-ppc.c
>>> they use _be() Altivec functions. Basically they have inverted
>>> LITTLE_ENDIAN with BIB_ENDIAN, since they use Big Endian only Altivec
>>> functions in gcm-ppc.c so the only wrong piece of code is in gcm.h,
>>> where USE_PPC_CRYPTO is defined.
>>>
>> I assume you mean this in gcm_HashMult_hw:

Yes

>>
>>  ??????? /* clang needs the following cast away from const; maybe a bug
>> in 7.0.0 */
>>  ??????? v = (vec_u64)vec_xl_be(0, (unsigned char *)buf);
>>
>> It should be fine to call endian specific functions if you know how
>> the data is stored.

I've completely understood vec_xl_be() wrong.

>>
>>> Need only to invert the condition by which USE_PPC_CRYPTO is defined:
>>> BIG_ENDIAN instead of LITTLE_ENDIAN.
>>>
>>> Best regards
>>
>> Based on the below commit, it was tested on a POWER8, presumably in LE
>> mode, so I'd be hesitant to switch any endian checks:
>> https://hg.mozilla.org/projects/nss/rev/3d7e509d6d20ecd607a28fa6ce42e4ffd9c51443

Right.

>>
>>
>> The bigger issue may be that 'vec_xl_be' wasn't introduced until GCC 8
>> and the toolchain used in the failed build is GCC 7, so that function
>> is not available in altivec.h:
>> https://github.com/gcc-mirror/gcc/commit/f7b0548e5eba54b637977e3df4d4daf0cabe474d

Ok

>>
>>
>> So if we stick with nss-3.48, the package should likely depend on
>> BR2_TOOLCHAIN_GCC_AT_LEAST_8.

This ^^^ is too restrictive, but...

> Or don't utilize PPC hardware accelerated instructions by undefining
> USE_PPC_CRYPTO... which is probably the saner option :-). This would
> give the semblance that the compile worked as expected since the hashing
> function will now be available, but Thomas' concern re CPU_ARCH and the
> future inclusion of ppc specific source is still valid.

...this is a good idea. So with gcc version < 8.x let's disable 
USE_PPC_CRYPTO and with gcc version >= 8.x let's enable it only if 
LITTLE_ENDIAN. So at this point I would change my upstream patch to do 
that in NSS, since there's already a gcc version check but only against 
version 5.x(>=) that instead needs to be >= 8.x.

Thanks for all the explanations!

Best regards
and
happy new year! For me at least :-)
-- 
Giulio Benetti
Benetti Engineering sas

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Yann E. MORIN]

Giulio, Vincent, All,

On 2020-01-01 00:44 +0100, Giulio Benetti spake thusly:
> On 1/1/20 12:31 AM, Vincent Fazio wrote:
> >On 12/31/19 4:49 PM, Vincent Fazio wrote:
[--SNIP--]
> >> ??????? /* clang needs the following cast away from const; maybe a bug
> >>in 7.0.0 */
> >> ??????? v = (vec_u64)vec_xl_be(0, (unsigned char *)buf);
> >>
> >>It should be fine to call endian specific functions if you know how
> >>the data is stored.
[--SNIP--]
> >>Based on the below commit, it was tested on a POWER8, presumably in LE
> >>mode, so I'd be hesitant to switch any endian checks:
> >>https://hg.mozilla.org/projects/nss/rev/3d7e509d6d20ecd607a28fa6ce42e4ffd9c51443
[--SNIP--]
> >>The bigger issue may be that 'vec_xl_be' wasn't introduced until GCC 8
> >>and the toolchain used in the failed build is GCC 7, so that function
> >>is not available in altivec.h:
> >>https://github.com/gcc-mirror/gcc/commit/f7b0548e5eba54b637977e3df4d4daf0cabe474d

Thanks Vincent for digging all this info, very informative. :-)

> ...this is a good idea. So with gcc version < 8.x let's disable
> USE_PPC_CRYPTO and with gcc version >= 8.x let's enable it only if
> LITTLE_ENDIAN. So at this point I would change my upstream patch to do that
> in NSS, since there's already a gcc version check but only against version
> 5.x(>=) that instead needs to be >= 8.x.

Yes, that sounds like a good plan. I've marked this patch as changes
requested in ptachwork in the meantime.

Thanks! :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Thomas Petazzoni]

On Wed, 1 Jan 2020 00:44:09 +0100
Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:

> >> So if we stick with nss-3.48, the package should likely depend on
> >> BR2_TOOLCHAIN_GCC_AT_LEAST_8.  
> 
> This ^^^ is too restrictive, but...
> 
> > Or don't utilize PPC hardware accelerated instructions by undefining
> > USE_PPC_CRYPTO... which is probably the saner option :-). This would
> > give the semblance that the compile worked as expected since the hashing
> > function will now be available, but Thomas' concern re CPU_ARCH and the
> > future inclusion of ppc specific source is still valid.  
> 
> ...this is a good idea. So with gcc version < 8.x let's disable 
> USE_PPC_CRYPTO and with gcc version >= 8.x let's enable it only if 
> LITTLE_ENDIAN. So at this point I would change my upstream patch to do 
> that in NSS, since there's already a gcc version check but only against 
> version 5.x(>=) that instead needs to be >= 8.x.

As I said in my previous e-mail, I also don't see how it can work with
the current OS_TEST value that we pass in libnss.mk, which is never set
to ppc64 or ppc64le as the libnss build system expects.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

Hi Thomas,

On 1/1/20 4:32 PM, Thomas Petazzoni wrote:
> On Wed, 1 Jan 2020 00:44:09 +0100
> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
> 
>>>> So if we stick with nss-3.48, the package should likely depend on
>>>> BR2_TOOLCHAIN_GCC_AT_LEAST_8.
>>
>> This ^^^ is too restrictive, but...
>>
>>> Or don't utilize PPC hardware accelerated instructions by undefining
>>> USE_PPC_CRYPTO... which is probably the saner option :-). This would
>>> give the semblance that the compile worked as expected since the hashing
>>> function will now be available, but Thomas' concern re CPU_ARCH and the
>>> future inclusion of ppc specific source is still valid.
>>
>> ...this is a good idea. So with gcc version < 8.x let's disable
>> USE_PPC_CRYPTO and with gcc version >= 8.x let's enable it only if
>> LITTLE_ENDIAN. So at this point I would change my upstream patch to do
>> that in NSS, since there's already a gcc version check but only against
>> version 5.x(>=) that instead needs to be >= 8.x.
> 
> As I said in my previous e-mail, I also don't see how it can work with
> the current OS_TEST value that we pass in libnss.mk, which is never set
> to ppc64 or ppc64le as the libnss build system expects.

You're right, the problem is that it doesn't compile gcm-ppc.c because 
Buildroot sets OS_TEST to powerpc64le in this case but libnss expects 
ppc64le, so I'm testing a patch where I remap:
BR2_powerpc => ppc
BR2_powerpc64 => ppc64
BR2_powerpc64le => ppc64le

And I'm going to add a patch too to modify minimum gcc version required 
from 5.x to 8.x, because as Vincent pointed vec_xst_be() has been 
introduced only in version 8.x.
This way we avoid BR2_TOOLCHAIN_GCC_AT_LEAST_8 and later I send it upstream.

Thank you and
Best regards
-- 
Giulio Benetti
Benetti Engineering sas

[Buildroot] [PATCH v2] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

libnss expects OS_TEST to be set to ppc or ppc64 or ppc64le instead of
powerpc, powerpc64, powerpc64le. At the moment gcm.h header checks if
__powerpc64__ is defined, but Makefile expects OS_TEST to be set to
ppc64 or ppc64le to build gcm-ppc.c. This way we end with having gcm
prototypes defined, but without implementation compiled. So remap
OS_TEST to what libnss expects depending on BR2_powerpc* enabled. This
way Makefile will build gcm-ppc.c too, that contains gcm-ppc function
implementations using Altivec. But add also a patch that modify
minimum gcc version required for using Altivec functions from 5.x to 8.x
since some Altivec intrinsics have been instroduced only in 8.x(i.e.
vec_xst_be()).

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
---
V1 -> V2:
* changed completely approach
Pending upstream:
https://bugzilla.mozilla.org/show_bug.cgi?id=1606119
---
 ...6119-Fix-PPC-HW-Crypto-build-failure.patch | 35 +++++++++++++++++++
 package/libnss/libnss.mk                      |  6 ++++
 2 files changed, 41 insertions(+)
 create mode 100644 package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch

diff --git a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
new file mode 100644
index 0000000000..2439eb1625
--- /dev/null
+++ b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
@@ -0,0 +1,35 @@
+From ebf185f8e48b5aec622dc949cef1b19b0a7669ef Mon Sep 17 00:00:00 2001
+From: Giulio Benetti <giulio.benetti@benettiengineering.com>
+Date: Wed, 1 Jan 2020 12:54:45 +0100
+Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
+
+All Altivec *_be() functions are supported from gcc version 8.x not 5.x
+so modify gcc version check that at the moment cause build failure due
+to missing Altivec *_be() functions.
+
+Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
+---
+ nss/lib/freebl/gcm.h | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/nss/lib/freebl/gcm.h b/nss/lib/freebl/gcm.h
+index 571b9ec55..aa4dee824 100644
+--- a/nss/lib/freebl/gcm.h
++++ b/nss/lib/freebl/gcm.h
+@@ -41,11 +41,11 @@
+ #endif
+ 
+ /*
+- * PPC CRYPTO requires at least gcc 5 or clang. The LE check is purely
++ * 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.
+  */
+-#if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 5)) && \
++#if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 8)) && \
+     defined(IS_LITTLE_ENDIAN)
+ #define USE_PPC_CRYPTO
+ #endif
+-- 
+2.20.1
+
diff --git a/package/libnss/libnss.mk b/package/libnss/libnss.mk
index 9349276a90..23dfdb11e4 100644
--- a/package/libnss/libnss.mk
+++ b/package/libnss/libnss.mk
@@ -39,6 +39,12 @@ endif
 
 ifeq ($(BR2_aarch64_be),y)
 LIBNSS_ARCH = aarch64
+else ifeq ($(BR2_powerpc),y)
+LIBNSS_ARCH = ppc
+else ifeq ($(BR2_powerpc64),y)
+LIBNSS_ARCH = ppc64
+else ifeq ($(BR2_powerpc64le),y)
+LIBNSS_ARCH = ppc64le
 else
 LIBNSS_ARCH = $(ARCH)
 endif
-- 
2.20.1

[Buildroot] [PATCH v2] package/libnss: fix build failure due to HW PPC Crypto bug

[Yann E. MORIN]

Giulio, All,

On 2020-01-01 17:58 +0100, Giulio Benetti spake thusly:
> libnss expects OS_TEST to be set to ppc or ppc64 or ppc64le instead of
> powerpc, powerpc64, powerpc64le. At the moment gcm.h header checks if
> __powerpc64__ is defined, but Makefile expects OS_TEST to be set to
> ppc64 or ppc64le to build gcm-ppc.c. This way we end with having gcm
> prototypes defined, but without implementation compiled. So remap
> OS_TEST to what libnss expects depending on BR2_powerpc* enabled. This
> way Makefile will build gcm-ppc.c too, that contains gcm-ppc function
> implementations using Altivec. But add also a patch that modify
> minimum gcc version required for using Altivec functions from 5.x to 8.x
> since some Altivec intrinsics have been instroduced only in 8.x(i.e.
> vec_xst_be()).
> 
> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>

Applied to master, after doing a few tweaks:

  - move the arch setting to kconfig (like recently done in e3159cad71)
  - add the build failure
  - add pointer to upstream bug report and patch sybmission
  - reformat and reword commit log

Pushed now, thanks.

Regards,
Yann E. MORIN.

> ---
> V1 -> V2:
> * changed completely approach
> Pending upstream:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1606119
> ---
>  ...6119-Fix-PPC-HW-Crypto-build-failure.patch | 35 +++++++++++++++++++
>  package/libnss/libnss.mk                      |  6 ++++
>  2 files changed, 41 insertions(+)
>  create mode 100644 package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> 
> diff --git a/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> new file mode 100644
> index 0000000000..2439eb1625
> --- /dev/null
> +++ b/package/libnss/0005-Bug-1606119-Fix-PPC-HW-Crypto-build-failure.patch
> @@ -0,0 +1,35 @@
> +From ebf185f8e48b5aec622dc949cef1b19b0a7669ef Mon Sep 17 00:00:00 2001
> +From: Giulio Benetti <giulio.benetti@benettiengineering.com>
> +Date: Wed, 1 Jan 2020 12:54:45 +0100
> +Subject: [PATCH] Bug 1606119 - Fix PPC HW Crypto build failure
> +
> +All Altivec *_be() functions are supported from gcc version 8.x not 5.x
> +so modify gcc version check that at the moment cause build failure due
> +to missing Altivec *_be() functions.
> +
> +Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> +---
> + nss/lib/freebl/gcm.h | 4 ++--
> + 1 file changed, 2 insertions(+), 2 deletions(-)
> +
> +diff --git a/nss/lib/freebl/gcm.h b/nss/lib/freebl/gcm.h
> +index 571b9ec55..aa4dee824 100644
> +--- a/nss/lib/freebl/gcm.h
> ++++ b/nss/lib/freebl/gcm.h
> +@@ -41,11 +41,11 @@
> + #endif
> + 
> + /*
> +- * PPC CRYPTO requires at least gcc 5 or clang. The LE check is purely
> ++ * 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.
> +  */
> +-#if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 5)) && \
> ++#if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 8)) && \
> +     defined(IS_LITTLE_ENDIAN)
> + #define USE_PPC_CRYPTO
> + #endif
> +-- 
> +2.20.1
> +
> diff --git a/package/libnss/libnss.mk b/package/libnss/libnss.mk
> index 9349276a90..23dfdb11e4 100644
> --- a/package/libnss/libnss.mk
> +++ b/package/libnss/libnss.mk
> @@ -39,6 +39,12 @@ endif
>  
>  ifeq ($(BR2_aarch64_be),y)
>  LIBNSS_ARCH = aarch64
> +else ifeq ($(BR2_powerpc),y)
> +LIBNSS_ARCH = ppc
> +else ifeq ($(BR2_powerpc64),y)
> +LIBNSS_ARCH = ppc64
> +else ifeq ($(BR2_powerpc64le),y)
> +LIBNSS_ARCH = ppc64le
>  else
>  LIBNSS_ARCH = $(ARCH)
>  endif
> -- 
> 2.20.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2] package/libnss: fix build failure due to HW PPC Crypto bug

[Thomas Petazzoni]

Hello Giulio,

Thanks for the patch. One comment below.

On Wed,  1 Jan 2020 17:58:11 +0100
Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:

>  ifeq ($(BR2_aarch64_be),y)
>  LIBNSS_ARCH = aarch64
> +else ifeq ($(BR2_powerpc),y)
> +LIBNSS_ARCH = ppc
> +else ifeq ($(BR2_powerpc64),y)
> +LIBNSS_ARCH = ppc64
> +else ifeq ($(BR2_powerpc64le),y)
> +LIBNSS_ARCH = ppc64le
>  else
>  LIBNSS_ARCH = $(ARCH)
>  endif

Shouldn't we globally review the values that libnss wants for the
OS_TEST variable (which we set to $(LIBNSS_ARCH)) and see if other
architectures need similar tweaks ?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

Hi Thomas,

On 1/2/20 10:06 AM, Thomas Petazzoni wrote:
> Hello Giulio,
> 
> Thanks for the patch. One comment below.
> 
> On Wed,  1 Jan 2020 17:58:11 +0100
> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
> 
>>   ifeq ($(BR2_aarch64_be),y)
>>   LIBNSS_ARCH = aarch64
>> +else ifeq ($(BR2_powerpc),y)
>> +LIBNSS_ARCH = ppc
>> +else ifeq ($(BR2_powerpc64),y)
>> +LIBNSS_ARCH = ppc64
>> +else ifeq ($(BR2_powerpc64le),y)
>> +LIBNSS_ARCH = ppc64le
>>   else
>>   LIBNSS_ARCH = $(ARCH)
>>   endif
> 
> Shouldn't we globally review the values that libnss wants for the
> OS_TEST variable (which we set to $(LIBNSS_ARCH)) and see if other
> architectures need similar tweaks ?

Absolutely yes, going to do that.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

[Buildroot] [PATCH v2] package/libnss: fix build failure due to HW PPC Crypto bug

[Giulio Benetti]

On 1/2/20 6:19 PM, Giulio Benetti wrote:
> Hi Thomas,
> 
> On 1/2/20 10:06 AM, Thomas Petazzoni wrote:
>> Hello Giulio,
>>
>> Thanks for the patch. One comment below.
>>
>> On Wed,  1 Jan 2020 17:58:11 +0100
>> Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
>>
>>>    ifeq ($(BR2_aarch64_be),y)
>>>    LIBNSS_ARCH = aarch64
>>> +else ifeq ($(BR2_powerpc),y)
>>> +LIBNSS_ARCH = ppc
>>> +else ifeq ($(BR2_powerpc64),y)
>>> +LIBNSS_ARCH = ppc64
>>> +else ifeq ($(BR2_powerpc64le),y)
>>> +LIBNSS_ARCH = ppc64le
>>>    else
>>>    LIBNSS_ARCH = $(ARCH)
>>>    endif
>>
>> Shouldn't we globally review the values that libnss wants for the
>> OS_TEST variable (which we set to $(LIBNSS_ARCH)) and see if other
>> architectures need similar tweaks ?

It turns out that all specific architectures checked in 
coreconf/Linux.mk are already correct(after changing powerpc* to ppc*). 
All the others are directly copied from OS_TEST to OS_ARCH in 
coreconf/Linux.mk:
https://hg.mozilla.org/projects/nss/file/tip/coreconf/Linux.mk#l93
and OS_ARCH is the variable NSS uses internally.

So it seems that at the moment there is nothing more to do about this.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> Absolutely yes, going to do that.
> 
> Best regards
>