[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Ard Biesheuvel]

When running the 32-bit ARM kernel on ARMv8 capable bare metal (e.g.,
32-bit Android userland and kernel on a Cortex-A53), or as a KVM guest
on a 64-bit host, we should advertise the availability of the Crypto
instructions, so that userland libraries such as OpenSSL may use them.
(Support for the v8 Crypto instructions in the 32-bit build was added
to OpenSSL more than six months ago)

This adds the ID feature bit detection, and sets elf_hwcap2 accordingly.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v2:
- drop redundant architecture check -> accessing ID_ISAR5 should be safe
  even on v7-M, and even if we don't expect to find crypto features there
- add comment regarding pmull->aes

 arch/arm/kernel/setup.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index e55408e96559..482bdfa24daa 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -375,7 +375,7 @@ void __init early_print(const char *str, ...)
 
 static void __init cpuid_init_hwcaps(void)
 {
-	unsigned int divide_instrs, vmsa;
+	unsigned int divide_instrs, vmsa, isar5;
 
 	if (cpu_architecture() < CPU_ARCH_ARMv7)
 		return;
@@ -393,6 +393,20 @@ static void __init cpuid_init_hwcaps(void)
 	vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
 	if (vmsa >= 5)
 		elf_hwcap |= HWCAP_LPAE;
+
+	/* check for supported v8 Crypto instructions */
+	isar5 = read_cpuid_ext(CPUID_EXT_ISAR5);
+
+	switch ((isar5 >> 4) & 0xf) {
+	case 2:	elf_hwcap2 |= HWCAP2_PMULL;	/* pmull implies aes */
+	case 1:	elf_hwcap2 |= HWCAP2_AES;
+	}
+	if (((isar5 >> 8) & 0xf) == 1)
+		elf_hwcap2 |= HWCAP2_SHA1;
+	if (((isar5 >> 12) & 0xf) == 1)
+		elf_hwcap2 |= HWCAP2_SHA2;
+	if (((isar5 >> 16) & 0xf) == 1)
+		elf_hwcap2 |= HWCAP2_CRC32;
 }
 
 static void __init elf_hwcap_fixup(void)
-- 
1.8.3.2

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Russell King - ARM Linux]

On Thu, Mar 05, 2015 at 12:51:42PM +0100, Ard Biesheuvel wrote:
> @@ -393,6 +393,20 @@ static void __init cpuid_init_hwcaps(void)
>  	vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
>  	if (vmsa >= 5)
>  		elf_hwcap |= HWCAP_LPAE;
> +
> +	/* check for supported v8 Crypto instructions */
> +	isar5 = read_cpuid_ext(CPUID_EXT_ISAR5);
> +
> +	switch ((isar5 >> 4) & 0xf) {
> +	case 2:	elf_hwcap2 |= HWCAP2_PMULL;	/* pmull implies aes */
> +	case 1:	elf_hwcap2 |= HWCAP2_AES;
> +	}
> +	if (((isar5 >> 8) & 0xf) == 1)
> +		elf_hwcap2 |= HWCAP2_SHA1;
> +	if (((isar5 >> 12) & 0xf) == 1)
> +		elf_hwcap2 |= HWCAP2_SHA2;
> +	if (((isar5 >> 16) & 0xf) == 1)
> +		elf_hwcap2 |= HWCAP2_CRC32;

Reading through the ARMv7 ARM, the ISAR registers seem to work in a way
that "feature >= N" is sufficient to test for something (in other words,
the feature revision bits build on previous instructions added by older
revisions of that feature.)

Hence why PMULL implies AES - that follows the same pattern.  So, I wonder
if those == should all be >=, and there should be a "default:" before
"case 2:" with the zero case handled separately.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Ard Biesheuvel]

On 9 March 2015 at 19:06, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Mar 05, 2015 at 12:51:42PM +0100, Ard Biesheuvel wrote:
>> @@ -393,6 +393,20 @@ static void __init cpuid_init_hwcaps(void)
>>       vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
>>       if (vmsa >= 5)
>>               elf_hwcap |= HWCAP_LPAE;
>> +
>> +     /* check for supported v8 Crypto instructions */
>> +     isar5 = read_cpuid_ext(CPUID_EXT_ISAR5);
>> +
>> +     switch ((isar5 >> 4) & 0xf) {
>> +     case 2: elf_hwcap2 |= HWCAP2_PMULL;     /* pmull implies aes */
>> +     case 1: elf_hwcap2 |= HWCAP2_AES;
>> +     }
>> +     if (((isar5 >> 8) & 0xf) == 1)
>> +             elf_hwcap2 |= HWCAP2_SHA1;
>> +     if (((isar5 >> 12) & 0xf) == 1)
>> +             elf_hwcap2 |= HWCAP2_SHA2;
>> +     if (((isar5 >> 16) & 0xf) == 1)
>> +             elf_hwcap2 |= HWCAP2_CRC32;
>
> Reading through the ARMv7 ARM, the ISAR registers seem to work in a way
> that "feature >= N" is sufficient to test for something (in other words,
> the feature revision bits build on previous instructions added by older
> revisions of that feature.)
>

There was quite some discussion about that when we implemented this
for arm64. Perhaps Steve wants to elaborate on the details, because I
don't remember them exactly.

> Hence why PMULL implies AES - that follows the same pattern.  So, I wonder
> if those == should all be >=, and there should be a "default:" before
> "case 2:" with the zero case handled separately.
>

Quoting from my version of the ARM ARM (DDI0487A_d):

AES, bits [7:4]
Indicates whether AES instructions are implemented in AArch32.
0000 No AES instructions implemented.
0001 AESE, AESD, AESMC, and AESIMC implemented.
0010 As for 0001 , plus PMULL/PMULL2 instructions operating on 64-bit
data quantities.
All other values are reserved.

Even though they are obviously not bitfields, 'reserved' to me
suggests that it is still incorrect to assume both PMULL and AES
capability for values not listed yet > 0b0010.
The same applies to the other blocks: all other values are reserved.

-- 
Ard.

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Ard Biesheuvel]

On 9 March 2015 at 19:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 March 2015 at 19:06, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Thu, Mar 05, 2015 at 12:51:42PM +0100, Ard Biesheuvel wrote:
>>> @@ -393,6 +393,20 @@ static void __init cpuid_init_hwcaps(void)
>>>       vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
>>>       if (vmsa >= 5)
>>>               elf_hwcap |= HWCAP_LPAE;
>>> +
>>> +     /* check for supported v8 Crypto instructions */
>>> +     isar5 = read_cpuid_ext(CPUID_EXT_ISAR5);
>>> +
>>> +     switch ((isar5 >> 4) & 0xf) {
>>> +     case 2: elf_hwcap2 |= HWCAP2_PMULL;     /* pmull implies aes */
>>> +     case 1: elf_hwcap2 |= HWCAP2_AES;
>>> +     }
>>> +     if (((isar5 >> 8) & 0xf) == 1)
>>> +             elf_hwcap2 |= HWCAP2_SHA1;
>>> +     if (((isar5 >> 12) & 0xf) == 1)
>>> +             elf_hwcap2 |= HWCAP2_SHA2;
>>> +     if (((isar5 >> 16) & 0xf) == 1)
>>> +             elf_hwcap2 |= HWCAP2_CRC32;
>>
>> Reading through the ARMv7 ARM, the ISAR registers seem to work in a way
>> that "feature >= N" is sufficient to test for something (in other words,
>> the feature revision bits build on previous instructions added by older
>> revisions of that feature.)
>>
>
> There was quite some discussion about that when we implemented this
> for arm64. Perhaps Steve wants to elaborate on the details, because I
> don't remember them exactly.
>
>> Hence why PMULL implies AES - that follows the same pattern.  So, I wonder
>> if those == should all be >=, and there should be a "default:" before
>> "case 2:" with the zero case handled separately.
>>
>
> Quoting from my version of the ARM ARM (DDI0487A_d):
>
> AES, bits [7:4]
> Indicates whether AES instructions are implemented in AArch32.
> 0000 No AES instructions implemented.
> 0001 AESE, AESD, AESMC, and AESIMC implemented.
> 0010 As for 0001 , plus PMULL/PMULL2 instructions operating on 64-bit
> data quantities.
> All other values are reserved.
>
> Even though they are obviously not bitfields, 'reserved' to me
> suggests that it is still incorrect to assume both PMULL and AES
> capability for values not listed yet > 0b0010.
> The same applies to the other blocks: all other values are reserved.
>

Replying to self: looking at the arm64 code, it seems the 4-bit block
are signed quantities, and the negative valiues are reserved.
This means only signed higher values should be considered incremental,
and values with bit 4 set should be ignored.

However, I can't find anything about it in the ARMv8 ARM.

If you prefer, I can rework the patch to duplicate the compat code in
arm64: at least they will be the same then.

------------------------>8---------------------------
features = read_cpuid(ID_ISAR5_EL1);
block = (features >> 4) & 0xf;
if (!(block & 0x8)) {
switch (block) {
default:
case 2:
compat_elf_hwcap2 |= COMPAT_HWCAP2_PMULL;
case 1:
compat_elf_hwcap2 |= COMPAT_HWCAP2_AES;
case 0:
break;
}
}

block = (features >> 8) & 0xf;
if (block && !(block & 0x8))
compat_elf_hwcap2 |= COMPAT_HWCAP2_SHA1;

block = (features >> 12) & 0xf;
if (block && !(block & 0x8))
compat_elf_hwcap2 |= COMPAT_HWCAP2_SHA2;

block = (features >> 16) & 0xf;
if (block && !(block & 0x8))
compat_elf_hwcap2 |= COMPAT_HWCAP2_CRC32;
------------------------>8---------------------------

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Russell King - ARM Linux]

On Mon, Mar 09, 2015 at 07:38:01PM +0100, Ard Biesheuvel wrote:
> Replying to self: looking at the arm64 code, it seems the 4-bit block
> are signed quantities, and the negative valiues are reserved.
> This means only signed higher values should be considered incremental,
> and values with bit 4 set should be ignored.

ITYM bit 3.

On 32-bit, we've assumed that they're unsigned:

        /* LPAE implies atomic ldrd/strd instructions */
        vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
        if (vmsa >= 5)
                elf_hwcap |= HWCAP_LPAE;

        sync_prim = ((read_cpuid_ext(CPUID_EXT_ISAR3) >> 8) & 0xf0) |
                    ((read_cpuid_ext(CPUID_EXT_ISAR4) >> 20) & 0x0f);
        if (sync_prim >= 0x13)
                elf_hwcap &= ~HWCAP_SWP;

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Ard Biesheuvel]

On 10 March 2015 at 11:15, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Mar 09, 2015 at 07:38:01PM +0100, Ard Biesheuvel wrote:
>> Replying to self: looking at the arm64 code, it seems the 4-bit block
>> are signed quantities, and the negative valiues are reserved.
>> This means only signed higher values should be considered incremental,
>> and values with bit 4 set should be ignored.
>
> ITYM bit 3.
>

Sorry, yes, the top bit.

> On 32-bit, we've assumed that they're unsigned:
>
>         /* LPAE implies atomic ldrd/strd instructions */
>         vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
>         if (vmsa >= 5)
>                 elf_hwcap |= HWCAP_LPAE;
>
>         sync_prim = ((read_cpuid_ext(CPUID_EXT_ISAR3) >> 8) & 0xf0) |
>                     ((read_cpuid_ext(CPUID_EXT_ISAR4) >> 20) & 0x0f);
>         if (sync_prim >= 0x13)
>                 elf_hwcap &= ~HWCAP_SWP;
>

[PATCH v2] ARM: advertise availability of v8 Crypto instructions

[Russell King - ARM Linux]

On Mon, Mar 09, 2015 at 07:12:46PM +0100, Ard Biesheuvel wrote:
> On 9 March 2015 at 19:06, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Mar 05, 2015 at 12:51:42PM +0100, Ard Biesheuvel wrote:
> >> @@ -393,6 +393,20 @@ static void __init cpuid_init_hwcaps(void)
> >>       vmsa = (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xf) >> 0;
> >>       if (vmsa >= 5)
> >>               elf_hwcap |= HWCAP_LPAE;
> >> +
> >> +     /* check for supported v8 Crypto instructions */
> >> +     isar5 = read_cpuid_ext(CPUID_EXT_ISAR5);
> >> +
> >> +     switch ((isar5 >> 4) & 0xf) {
> >> +     case 2: elf_hwcap2 |= HWCAP2_PMULL;     /* pmull implies aes */
> >> +     case 1: elf_hwcap2 |= HWCAP2_AES;
> >> +     }
> >> +     if (((isar5 >> 8) & 0xf) == 1)
> >> +             elf_hwcap2 |= HWCAP2_SHA1;
> >> +     if (((isar5 >> 12) & 0xf) == 1)
> >> +             elf_hwcap2 |= HWCAP2_SHA2;
> >> +     if (((isar5 >> 16) & 0xf) == 1)
> >> +             elf_hwcap2 |= HWCAP2_CRC32;
> >
> > Reading through the ARMv7 ARM, the ISAR registers seem to work in a way
> > that "feature >= N" is sufficient to test for something (in other words,
> > the feature revision bits build on previous instructions added by older
> > revisions of that feature.)
> >
> 
> There was quite some discussion about that when we implemented this
> for arm64. Perhaps Steve wants to elaborate on the details, because I
> don't remember them exactly.
> 
> > Hence why PMULL implies AES - that follows the same pattern.  So, I wonder
> > if those == should all be >=, and there should be a "default:" before
> > "case 2:" with the zero case handled separately.
> >
> 
> Quoting from my version of the ARM ARM (DDI0487A_d):
> 
> AES, bits [7:4]
> Indicates whether AES instructions are implemented in AArch32.
> 0000 No AES instructions implemented.
> 0001 AESE, AESD, AESMC, and AESIMC implemented.
> 0010 As for 0001 , plus PMULL/PMULL2 instructions operating on 64-bit
> data quantities.
> All other values are reserved.
> 
> Even though they are obviously not bitfields, 'reserved' to me
> suggests that it is still incorrect to assume both PMULL and AES
> capability for values not listed yet > 0b0010.
> The same applies to the other blocks: all other values are reserved.

The use of "reserved" in this manner isn't actually detailed in the
glossary, which makes it a little hard to work out exactly what is
meant here (that's a documentation bug).

Is it "ARM Ltd reserves the right to add further values which augment
the previous values" or is it "ARM Ltd reserve the right to add
further values with completely different behaviours."

Really, the ARM ARM needs to be improved in this area.  It needs to
contain a clear statement about how the bitfields are intended to work.
The purpose of the CPUID stuff is to allow software to adapt to the
features of the CPU, but without a clearly defined behaviour for the
"unknown values", moving a piece of software to a newer CPU which
implements a superset of the features of a previous CPU will lead it
to fail (because the CPUID fields could now be in the reserved ranges)
if strict tests were applied for exact field values.

I'm not talking about moving a piece of binary software: I'm talking
there more about the maintanence burden of having to go through the
entire program checking for any references to CPUID fields, and checking
and updating every single one.

The current CPUID situation is which encourages exact precise tests is,
IMHO, not well thought out.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.