Re: [PATCH] CryptoPkg/OpensslLib AARCH64: clear XIP CC flags

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 14 July 2017 at 19:22, Laszlo Ersek <lersek@redhat.com> 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.

-- 
Ard.