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 0BCC02095D8F2 for ; Fri, 14 Jul 2017 12:57:37 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 346B7EE56F; Fri, 14 Jul 2017 19:59:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 346B7EE56F Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 346B7EE56F 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 C6F8760A9F; Fri, 14 Jul 2017 19:59:24 +0000 (UTC) To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , qlong , "Ye, Ting" , Leif Lindholm References: <20170714171913.28524-1-ard.biesheuvel@linaro.org> <51a36161-0ddc-0a53-24c1-b782883e46cc@redhat.com> From: Laszlo Ersek Message-ID: <8302b453-66c2-9cbe-7554-b0697bef278a@redhat.com> Date: Fri, 14 Jul 2017 21:59:23 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 14 Jul 2017 19:59:26 +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 19:57:37 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/14/17 20:41, Ard Biesheuvel wrote: > On 14 July 2017 at 19:22, Laszlo Ersek wrote: >> 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. :) > > No, not at all, I just was a bit terse in the description. > > Before the mentioned commit, XIP CC flags was set to -mstrict-align, > which is required for modules that may execute with the MMU off. This > has nothing to do with SIMD or FP. > > When we added -mgeneral-regs-only to that, we broke the OpensslLib > build, which drew my attention to the fact that it, being a BASE > library, is also built with -mstrict-align, and /this/ is the > potential performance concern. > > So clearing XIP CC flags serves two distinct purposes: > - removing -mgeneral-regs-only to unbreak the build > - removing -mstrict-align to let the compiler generate faster code. > Thanks! For completeness, it would be great if you could work this into the commit message before pushing the patch. Acked-by: Laszlo Ersek Cheers, Laszlo