[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 at 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 at 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 at 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