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

[Laszlo Ersek <lersek@redhat.com>]

On 07/14/17 20:41, Ard Biesheuvel wrote:
> 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.
> 

Thanks! For completeness, it would be great if you could work this into
the commit message before pushing the patch.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Cheers,
Laszlo