Re: [PATCH] crypto: arm64/crc32 - detect crc32 support in assembler

[Ard Biesheuvel]

On 1 February 2017 at 13:58, Alexander Graf <agraf@xxxxxxx> wrote:
> On 02/01/2017 10:43 AM, Ard Biesheuvel wrote:
>>
>> On 1 February 2017 at 09:07, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> wrote:
>>>
>>> On 27 January 2017 at 10:52, Will Deacon <will.deacon@xxxxxxx> wrote:
>>>>
>>>> On Fri, Jan 27, 2017 at 10:43:16AM +0000, Ard Biesheuvel wrote:
>>>>>
>>>>> On 27 January 2017 at 10:40, Matthias Brugger <mbrugger@xxxxxxxx>
>>>>> wrote:
>>>>>>
>>>>>> Older compilers may not be able to detect the crc32 extended cpu type.
>>>>>
>>>>> What do you mean 'detect'? Could you describe the failure in more
>>>>> detail
>>>>> please?
>>>>>
>>>>>> Anyway only inline assembler code is used, which gets passed to the
>>>>>> assembler. This patch moves the crc detection to the assembler.
>>>>>>
>>>>>> Suggested-by: Alexander Graf <agraf@xxxxxxx>
>>>>>> Signed-off-by: Matthias Brugger <mbrugger@xxxxxxxx>
>>>>>> ---
>>>>>>   arch/arm64/crypto/Makefile      | 2 --
>>>>>>   arch/arm64/crypto/crc32-arm64.c | 3 +++
>>>>>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/crypto/Makefile b/arch/arm64/crypto/Makefile
>>>>>> index aa8888d7b744..0d779dac75cd 100644
>>>>>> --- a/arch/arm64/crypto/Makefile
>>>>>> +++ b/arch/arm64/crypto/Makefile
>>>>>> @@ -48,8 +48,6 @@ CFLAGS_aes-glue-ce.o  := -DUSE_V8_CRYPTO_EXTENSIONS
>>>>>>
>>>>>>   obj-$(CONFIG_CRYPTO_CRC32_ARM64) += crc32-arm64.o
>>>>>>
>>>>>> -CFLAGS_crc32-arm64.o   := -mcpu=generic+crc
>>>>>> -
>>>>>>   $(obj)/aes-glue-%.o: $(src)/aes-glue.c FORCE
>>>>>>          $(call if_changed_rule,cc_o_c)
>>>>>>
>>>>>> diff --git a/arch/arm64/crypto/crc32-arm64.c
>>>>>> b/arch/arm64/crypto/crc32-arm64.c
>>>>>> index 6a37c3c6b11d..10f5dd075323 100644
>>>>>> --- a/arch/arm64/crypto/crc32-arm64.c
>>>>>> +++ b/arch/arm64/crypto/crc32-arm64.c
>>>>>> @@ -29,6 +29,9 @@ MODULE_AUTHOR("Yazen Ghannam
>>>>>> <yazen.ghannam@xxxxxxxxxx>");
>>>>>>   MODULE_DESCRIPTION("CRC32 and CRC32C using optional ARMv8
>>>>>> instructions");
>>>>>>   MODULE_LICENSE("GPL v2");
>>>>>>
>>>>>> +/* Request crc extension capabilities from the assembler */
>>>>>> +asm(".arch_extension crc");
>>>>>> +
>>>>>
>>>>> Will should confirm, but I think this is a recent feature in GAS for
>>>>> AArch64, so this may break older toolchains as well.
>>>>
>>>> Yes, the .arch_extension directive isn't universally supported by
>>>> AArch64
>>>> gas so we can't rely on it unconditionally. The best bet is to check for
>>>> the support and, if it's not present, then disable whatever feature
>>>> relies
>>>> on it. See the lseinstr variable in Makefile.
>>>>
>>> Actually, this driver has become somewhat redundant now that we have
>>> an alternative that combines an implementation based on 64x64
>>> polynomial multiplication with an implementation based on the CRC32
>>> instructions.
>>>
>>> I will propose a patch that makes the latter usable when only the
>>> CRC32 instructions are supported.
>>
>> ... although you still haven't explained what the actual problem is
>> that you are trying to solve.
>
>
>
>
> The problem is that in Leap 42.2 (as well as SLES12 SP2) we have a 4.8 based
> system compiler, but recent binutils. That means that while our assembler is
> happy to work with crc instructions, passing the -mcpu parameter to gcc
> fails because gcc isn't aware of the flavor yet.
>
> That in turn means that we want to tell the assembler about feature
> requirements rather than the compiler. Fortunately the ".arch_extension"
> primitive allows you to do so.
>
> As far as checking for availability of it goes, I agree that it'd be nice to
> check if ".arch_extension" is supported. But so would be to check if
> -mcpu=generic+crc is supported. IMHO this patch doesn't make the current
> non-checking situation any worse. But of course checking is always nicer
> than not checking :)
>

Well, I am pretty sure binutils v2.25 and older are in wider use than
GCC v4.8 and older, which means the runtime test would disable the CRC
module altogether for a lot of users.

However, as I mentioned, we can also remove this driver once the PMULL
based one is updated.