| Message ID | 1425556302-3370-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.
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---------------------------
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; >
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)
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(-)