From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C06AA21B06E80 for ; Fri, 14 Jul 2017 11:21:04 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CF4423E1C46; Fri, 14 Jul 2017 18:22:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CF4423E1C46 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com CF4423E1C46 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-91.phx2.redhat.com [10.3.116.91]) by smtp.corp.redhat.com (Postfix) with ESMTP id 762FA6031D; Fri, 14 Jul 2017 18:22:52 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org, qin.long@intel.com Cc: ting.ye@intel.com, leif.lindholm@linaro.org References: <20170714171913.28524-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <51a36161-0ddc-0a53-24c1-b782883e46cc@redhat.com> Date: Fri, 14 Jul 2017 20:22:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170714171913.28524-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 14 Jul 2017 18:22:54 +0000 (UTC) Subject: Re: [PATCH] CryptoPkg/OpensslLib AARCH64: clear XIP CC flags X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Jul 2017 18:21:05 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/14/17 19:19, Ard Biesheuvel wrote: > Commit 0df6c8c157af ("BaseTools/tools_def AARCH64: avoid SIMD registers > in XIP code") updated the compiler flags used by AARCH64 when building > modules (including BASE libraries) that may execute before the MMU is > enabled. > > This broke the build for OpensslLib/OpensslLibCrypto because the SIMD > register file is shared with the FPU, and since OpenSSL contains some > references to float/double types (which are mostly unused for UEFI btw), > disabling floating point prevents the compiler from building OpenSSL > at all. I'm with you until here. > > When introducing the support for XIP CC flags, we were aware that this > would affect BASE libraries as well, but were not expecting this to > have any performance impact. However, in the case of software crypto, > it makes sense not to needlessly inhibit the compiler's ability to > generate fast code, and even if OpenssLib is a BASE library, it is > guaranteed not to run with the MMU off, so we can create a local > exception, and clear its XIP CC flags for AARCH64. I'm confused by the last paragraph. The fist two paragraphs, and commit 0df6c8c157af explain why SIMD register code gen was disabled (in brief, to work around a GCC bug). Now it turns out that some code in OpenSSL cannot be compiled at all, without us allowing insn generation for SIMD registers (because OpenSSL uses float/double). Such code is unsafe to execute for at least two reasons ((a) with the MMU off it could trigger the same issue that 0df6c8c157af meant to protect against, and (b) because we don't use floating point in UEFI). But, said code in OpenSSL is never executed, so we can allow the potentially invalid code to be produced at least. But why talk about performance then? Do you mean that, in OpenSSL, (a) is not an issue, because the MMU is always on by then, so commit 0df6c8c157af doesn't apply, (b) although we don't use floating point, the integer arithmetic still benefits from SIMD? If so, I think (hope!) that I understand the argument, I'd just like a bit more hand-holding in the structure of the argument. Can you reword the third paragraph as "... another benefit of reenabling SIMD is restoring performance for at least OpenSSL's integer arithmetic, without risk of faults (because OpenSSL never runs with the MMU disabled)". Sorry if I'm being dense. :) Thanks Laszlo > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > Note that this un-breaks the currently broken AARCH64 build for > platforms that have secure boot enabled > > CryptoPkg/Library/OpensslLib/OpensslLib.inf | 11 +++++++++++ > CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf | 11 +++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf b/CryptoPkg/Library/OpensslLib/OpensslLib.inf > index cbabb34bdd7c..1d15da6660b2 100644 > --- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf > +++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf > @@ -580,3 +580,14 @@ [BuildOptions] > RVCT:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS) --library_interface=aeabi_clib99 --diag_suppress=1296,1295,550,1293,111,68,177,223,144,513,188,128,546,1,3017 -JCryptoPkg/Include > XCODE:*_*_IA32_CC_FLAGS = -mmmx -msse -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w > XCODE:*_*_X64_CC_FLAGS = -mmmx -msse -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w > + > + # > + # AARCH64 uses strict alignment and avoids SIMD registers for code that may execute > + # with the MMU off. This involves SEC, PEI_CORE and PEIM modules as well as BASE > + # libraries, given that they may be included into such modules. > + # This library, even though of the BASE type, is never used in such cases, and > + # avoiding the SIMD register file (which is shared with the FPU) prevents the > + # compiler from successfully building some of the OpenSSL source files that > + # use floating point types, so clear the flags here. > + # > + GCC:*_*_AARCH64_CC_XIPFLAGS == > diff --git a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > index 026b551bcafa..6fc8884da492 100644 > --- a/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > +++ b/CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf > @@ -541,3 +541,14 @@ [BuildOptions] > RVCT:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS) --library_interface=aeabi_clib99 --diag_suppress=1296,1295,550,1293,111,68,177,223,144,513,188,128,546,1,3017 -JCryptoPkg/Include > XCODE:*_*_IA32_CC_FLAGS = -mmmx -msse -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w > XCODE:*_*_X64_CC_FLAGS = -mmmx -msse -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w > + > + # > + # AARCH64 uses strict alignment and avoids SIMD registers for code that may execute > + # with the MMU off. This involves SEC, PEI_CORE and PEIM modules as well as BASE > + # libraries, given that they may be included into such modules. > + # This library, even though of the BASE type, is never used in such cases, and > + # avoiding the SIMD register file (which is shared with the FPU) prevents the > + # compiler from successfully building some of the OpenSSL source files that > + # use floating point types, so clear the flags here. > + # > + GCC:*_*_AARCH64_CC_XIPFLAGS == >