* [PATCH 0/4] Update cpu feature extraction @ 2016-01-26 9:23 Vladimir Murzin 2016-01-26 9:23 ` [PATCH 1/4] ARM: fix demoting HWCAP_SWP Vladimir Murzin ` (4 more replies) 0 siblings, 5 replies; 13+ messages in thread From: Vladimir Murzin @ 2016-01-26 9:23 UTC (permalink / raw) To: linux-arm-kernel Hi, This has been started as a fix in cpu feature extractor, but Suzuki suggested syncing-up the whole cpu feature handling with the recent updates on arm64 side [1,2]. I've kept fixes separately, so they can go as the are in case there is concern on updating cpu feature handling. [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/455568 [2] https://lkml.org/lkml/2015/11/18/570 Thanks! Vladimir Murzin (4): ARM: fix demoting HWCAP_SWP ARM: fix cpu feature extracting helper ARM: introduce cpu feature helper for unsigned values ARM: use proper helper while extracting cpu features arch/arm/include/asm/cputype.h | 16 +++++++++++++--- arch/arm/include/asm/smp_plat.h | 4 ++-- arch/arm/kernel/setup.c | 34 ++++++++++++++++++++-------------- arch/arm/kernel/thumbee.c | 4 +--- arch/arm/mm/mmu.c | 2 +- 5 files changed, 37 insertions(+), 23 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] ARM: fix demoting HWCAP_SWP 2016-01-26 9:23 [PATCH 0/4] Update cpu feature extraction Vladimir Murzin @ 2016-01-26 9:23 ` Vladimir Murzin 2016-01-27 9:34 ` Ard Biesheuvel 2016-01-26 9:23 ` [PATCH 2/4] ARM: fix cpu feature extracting helper Vladimir Murzin ` (3 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2016-01-26 9:23 UTC (permalink / raw) To: linux-arm-kernel Commit b8c9592 "ARM: 8318/1: treat CPU feature register fields as signed quantities" accidentally altered cpuid register used to demote HWCAP_SWP. ARM ARM says that SyncPrim_instrs bits in ID_ISAR3 should be used with SynchPrim_instrs_frac from ID_ISAR4. So, follow this rule. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm/kernel/setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 7d0cba6f..fde041b 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -510,7 +510,7 @@ static void __init elf_hwcap_fixup(void) */ if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && - cpuid_feature_extract(CPUID_EXT_ISAR3, 20) >= 3)) + cpuid_feature_extract(CPUID_EXT_ISAR4, 20) >= 3)) elf_hwcap &= ~HWCAP_SWP; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/4] ARM: fix demoting HWCAP_SWP 2016-01-26 9:23 ` [PATCH 1/4] ARM: fix demoting HWCAP_SWP Vladimir Murzin @ 2016-01-27 9:34 ` Ard Biesheuvel 0 siblings, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2016-01-27 9:34 UTC (permalink / raw) To: linux-arm-kernel On 26 January 2016 at 10:23, Vladimir Murzin <vladimir.murzin@arm.com> wrote: > Commit b8c9592 "ARM: 8318/1: treat CPU feature register fields as signed > quantities" accidentally altered cpuid register used to demote > HWCAP_SWP. > ARM ARM says that SyncPrim_instrs bits in ID_ISAR3 should be used with > SynchPrim_instrs_frac from ID_ISAR4. So, follow this rule. > > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Apologies for breaking that ... > --- > arch/arm/kernel/setup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index 7d0cba6f..fde041b 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -510,7 +510,7 @@ static void __init elf_hwcap_fixup(void) > */ > if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || > (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && > - cpuid_feature_extract(CPUID_EXT_ISAR3, 20) >= 3)) > + cpuid_feature_extract(CPUID_EXT_ISAR4, 20) >= 3)) > elf_hwcap &= ~HWCAP_SWP; > } > > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] ARM: fix cpu feature extracting helper 2016-01-26 9:23 [PATCH 0/4] Update cpu feature extraction Vladimir Murzin 2016-01-26 9:23 ` [PATCH 1/4] ARM: fix demoting HWCAP_SWP Vladimir Murzin @ 2016-01-26 9:23 ` Vladimir Murzin 2016-01-27 9:35 ` Ard Biesheuvel 2016-01-26 9:23 ` [PATCH 3/4] ARM: introduce cpu feature helper for unsigned values Vladimir Murzin ` (2 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2016-01-26 9:23 UTC (permalink / raw) To: linux-arm-kernel Commit b8c9592 "ARM: 8318/1: treat CPU feature register fields as signed quantities" introduced helper to extract signed quantities of 4-bit blocks. However, with a current code feature with value 0b1000 isn't rejected as negative. So fix the "if" condition. Reported-by: Jonathan Brawn <Jon.Brawn@arm.com> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm/include/asm/cputype.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h index b23c6c8..1ee94c7 100644 --- a/arch/arm/include/asm/cputype.h +++ b/arch/arm/include/asm/cputype.h @@ -276,7 +276,7 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u32 features, int feature = (features >> field) & 15; /* feature registers are signed values */ - if (feature > 8) + if (feature > 7) feature -= 16; return feature; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] ARM: fix cpu feature extracting helper 2016-01-26 9:23 ` [PATCH 2/4] ARM: fix cpu feature extracting helper Vladimir Murzin @ 2016-01-27 9:35 ` Ard Biesheuvel 0 siblings, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2016-01-27 9:35 UTC (permalink / raw) To: linux-arm-kernel On 26 January 2016 at 10:23, Vladimir Murzin <vladimir.murzin@arm.com> wrote: > Commit b8c9592 "ARM: 8318/1: treat CPU feature register fields as signed > quantities" introduced helper to extract signed quantities of 4-bit > blocks. However, with a current code feature with value 0b1000 isn't > rejected as negative. So fix the "if" condition. > > Reported-by: Jonathan Brawn <Jon.Brawn@arm.com> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/include/asm/cputype.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h > index b23c6c8..1ee94c7 100644 > --- a/arch/arm/include/asm/cputype.h > +++ b/arch/arm/include/asm/cputype.h > @@ -276,7 +276,7 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u32 features, > int feature = (features >> field) & 15; > > /* feature registers are signed values */ > - if (feature > 8) > + if (feature > 7) > feature -= 16; > > return feature; > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] ARM: introduce cpu feature helper for unsigned values 2016-01-26 9:23 [PATCH 0/4] Update cpu feature extraction Vladimir Murzin 2016-01-26 9:23 ` [PATCH 1/4] ARM: fix demoting HWCAP_SWP Vladimir Murzin 2016-01-26 9:23 ` [PATCH 2/4] ARM: fix cpu feature extracting helper Vladimir Murzin @ 2016-01-26 9:23 ` Vladimir Murzin 2016-01-26 9:23 ` [PATCH 4/4] ARM: use proper helper while extracting cpu features Vladimir Murzin 2016-01-28 11:13 ` [PATCH 0/4] Update cpu feature extraction Vladimir Murzin 4 siblings, 0 replies; 13+ messages in thread From: Vladimir Murzin @ 2016-01-26 9:23 UTC (permalink / raw) To: linux-arm-kernel Some of the CPU feature bits have unsigned values and need to be treated accordingly to avoid errors. Add the new cpu feature helper for such cases. Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm/include/asm/cputype.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h index 1ee94c7..d93ced5 100644 --- a/arch/arm/include/asm/cputype.h +++ b/arch/arm/include/asm/cputype.h @@ -270,8 +270,15 @@ static inline int cpu_is_pj4(void) #define cpu_is_pj4() 0 #endif -static inline int __attribute_const__ cpuid_feature_extract_field(u32 features, - int field) + +static inline unsigned int __attribute_const__ +cpuid_feature_extract_unsigned_field(u32 features, int field) +{ + return (features >> field) & 15; +} + +static inline int __attribute_const__ +cpuid_feature_extract_field(u32 features, int field) { int feature = (features >> field) & 15; @@ -285,4 +292,7 @@ static inline int __attribute_const__ cpuid_feature_extract_field(u32 features, #define cpuid_feature_extract(reg, field) \ cpuid_feature_extract_field(read_cpuid_ext(reg), field) +#define cpuid_feature_extract_unsigned(reg, field) \ + cpuid_feature_extract_unsigned_field(read_cpuid_ext(reg), field) + #endif -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: use proper helper while extracting cpu features 2016-01-26 9:23 [PATCH 0/4] Update cpu feature extraction Vladimir Murzin ` (2 preceding siblings ...) 2016-01-26 9:23 ` [PATCH 3/4] ARM: introduce cpu feature helper for unsigned values Vladimir Murzin @ 2016-01-26 9:23 ` Vladimir Murzin 2016-01-27 9:44 ` Ard Biesheuvel 2016-01-28 11:13 ` [PATCH 0/4] Update cpu feature extraction Vladimir Murzin 4 siblings, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2016-01-26 9:23 UTC (permalink / raw) To: linux-arm-kernel Update current users of cpu feature helpers to use the proper helper depends on how feature bits should be handled. We follow the arm64 scheme [1] (slightly rephrased): We have three types of fields: a) A precise value or a value from which you derive some precise value. b) Fields defining the presence of a feature (1, 2, 3). These would always be positive since the absence of such feature would mean a value of 0 c) Fields defining the absence of a feature by setting 0xf. These are usually fields that were initial RAZ and turned to -1. So we can treat (a) and (b) as unsigned permanently and (c) as as signed, [1] https://lkml.org/lkml/2015/11/19/549 Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> --- arch/arm/include/asm/smp_plat.h | 4 ++-- arch/arm/kernel/setup.c | 34 ++++++++++++++++++++-------------- arch/arm/kernel/thumbee.c | 4 +--- arch/arm/mm/mmu.c | 2 +- 4 files changed, 24 insertions(+), 20 deletions(-) diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h index f908071..03bbe02 100644 --- a/arch/arm/include/asm/smp_plat.h +++ b/arch/arm/include/asm/smp_plat.h @@ -49,7 +49,7 @@ static inline int tlb_ops_need_broadcast(void) if (!is_smp()) return 0; - return ((read_cpuid_ext(CPUID_EXT_MMFR3) >> 12) & 0xf) < 2; + return cpuid_feature_extract_unsigned(CPUID_EXT_MMFR3, 12) < 2; } #endif @@ -61,7 +61,7 @@ static inline int cache_ops_need_broadcast(void) if (!is_smp()) return 0; - return ((read_cpuid_ext(CPUID_EXT_MMFR3) >> 12) & 0xf) < 1; + return cpuid_feature_extract_unsigned(CPUID_EXT_MMFR3, 12) < 1; } #endif diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index fde041b..e696553 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -257,11 +257,15 @@ static int __get_cpu_architecture(void) /* Revised CPUID format. Read the Memory Model Feature * Register 0 and check for VMSAv7 or PMSAv7 */ unsigned int mmfr0 = read_cpuid_ext(CPUID_EXT_MMFR0); - if ((mmfr0 & 0x0000000f) >= 0x00000003 || - (mmfr0 & 0x000000f0) >= 0x00000030) + unsigned int block; +#ifdef CONFIG_MMU + block = cpuid_feature_extract_unsigned_field(mmfr0, 0); +#else + block = cpuid_feature_extract_unsigned_field(mmfr0, 4); +#endif + if (mmfr0 >= 3) cpu_arch = CPU_ARCH_ARMv7; - else if ((mmfr0 & 0x0000000f) == 0x00000002 || - (mmfr0 & 0x000000f0) == 0x00000020) + else if (mmfr0 == 2) cpu_arch = CPU_ARCH_ARMv6; else cpu_arch = CPU_ARCH_UNKNOWN; @@ -446,41 +450,41 @@ static inline void patch_aeabi_idiv(void) { } static void __init cpuid_init_hwcaps(void) { - int block; + unsigned int block; u32 isar5; if (cpu_architecture() < CPU_ARCH_ARMv7) return; - block = cpuid_feature_extract(CPUID_EXT_ISAR0, 24); + block = cpuid_feature_extract_unsigned(CPUID_EXT_ISAR0, 24); if (block >= 2) elf_hwcap |= HWCAP_IDIVA; if (block >= 1) elf_hwcap |= HWCAP_IDIVT; /* LPAE implies atomic ldrd/strd instructions */ - block = cpuid_feature_extract(CPUID_EXT_MMFR0, 0); + block = cpuid_feature_extract_unsigned(CPUID_EXT_MMFR0, 0); if (block >= 5) elf_hwcap |= HWCAP_LPAE; /* check for supported v8 Crypto instructions */ isar5 = read_cpuid_ext(CPUID_EXT_ISAR5); - block = cpuid_feature_extract_field(isar5, 4); + block = cpuid_feature_extract_unsigned_field(isar5, 4); if (block >= 2) elf_hwcap2 |= HWCAP2_PMULL; if (block >= 1) elf_hwcap2 |= HWCAP2_AES; - block = cpuid_feature_extract_field(isar5, 8); + block = cpuid_feature_extract_unsigned_field(isar5, 8); if (block >= 1) elf_hwcap2 |= HWCAP2_SHA1; - block = cpuid_feature_extract_field(isar5, 12); + block = cpuid_feature_extract_unsigned_field(isar5, 12); if (block >= 1) elf_hwcap2 |= HWCAP2_SHA2; - block = cpuid_feature_extract_field(isar5, 16); + block = cpuid_feature_extract_unsigned_field(isar5, 16); if (block >= 1) elf_hwcap2 |= HWCAP2_CRC32; } @@ -488,6 +492,7 @@ static void __init cpuid_init_hwcaps(void) static void __init elf_hwcap_fixup(void) { unsigned id = read_cpuid_id(); + unsigned int block; /* * HWCAP_TLS is available only on 1136 r1p0 and later, @@ -508,9 +513,10 @@ static void __init elf_hwcap_fixup(void) * avoid advertising SWP; it may not be atomic with * multiprocessing cores. */ - if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || - (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && - cpuid_feature_extract(CPUID_EXT_ISAR4, 20) >= 3)) + block = cpuid_feature_extract_unsigned(CPUID_EXT_ISAR3, 12); + + if (block > 1 || (block == 1 && + cpuid_feature_extract_unsigned(CPUID_EXT_ISAR4, 20) >= 3)) elf_hwcap &= ~HWCAP_SWP; } diff --git a/arch/arm/kernel/thumbee.c b/arch/arm/kernel/thumbee.c index 8ff8dbf..b5cac80 100644 --- a/arch/arm/kernel/thumbee.c +++ b/arch/arm/kernel/thumbee.c @@ -62,14 +62,12 @@ static struct notifier_block thumbee_notifier_block = { static int __init thumbee_init(void) { - unsigned long pfr0; unsigned int cpu_arch = cpu_architecture(); if (cpu_arch < CPU_ARCH_ARMv7) return 0; - pfr0 = read_cpuid_ext(CPUID_EXT_PFR0); - if ((pfr0 & 0x0000f000) != 0x00001000) + if (cpuid_feature_extract_unsigned(CPUID_EXT_PFR0, 12) != 1) return 0; pr_info("ThumbEE CPU extension supported.\n"); diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index 434d76f..06723a9 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -572,7 +572,7 @@ static void __init build_mem_type_table(void) * in the Short-descriptor translation table format descriptors. */ if (cpu_arch == CPU_ARCH_ARMv7 && - (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xF) >= 4) { + (cpuid_feature_extract_unsigned(CPUID_EXT_MMFR0, 0) >= 4)) { user_pmd_table |= PMD_PXNTABLE; } #endif -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: use proper helper while extracting cpu features 2016-01-26 9:23 ` [PATCH 4/4] ARM: use proper helper while extracting cpu features Vladimir Murzin @ 2016-01-27 9:44 ` Ard Biesheuvel 0 siblings, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2016-01-27 9:44 UTC (permalink / raw) To: linux-arm-kernel On 26 January 2016 at 10:23, Vladimir Murzin <vladimir.murzin@arm.com> wrote: > Update current users of cpu feature helpers to use the proper helper > depends on how feature bits should be handled. We follow the arm64 > scheme [1] (slightly rephrased): > > We have three types of fields: > > a) A precise value or a value from which you derive some precise > value. > b) Fields defining the presence of a feature (1, 2, 3). These would > always be positive since the absence of such feature would mean a > value of 0 > c) Fields defining the absence of a feature by setting 0xf. These are > usually fields that were initial RAZ and turned to -1. > > So we can treat (a) and (b) as unsigned permanently and (c) as as > signed, > > [1] https://lkml.org/lkml/2015/11/19/549 > > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> I do feel that it would be very useful to document that nature of those fields in the ARM ARM. If it is the intention to expose incremental functionality, the values should be documented as such, e.g., AES[7:4] >= 1 means AES instructions supported, >= 2 means AES and PMULL instructions supported. Without classifying the fields in such a way, there is no way to be compatible with future extensions. In this example, it would perhaps be safer to assume no such instructions are supported for values outside of the documented range. -- Ard. > --- > arch/arm/include/asm/smp_plat.h | 4 ++-- > arch/arm/kernel/setup.c | 34 ++++++++++++++++++++-------------- > arch/arm/kernel/thumbee.c | 4 +--- > arch/arm/mm/mmu.c | 2 +- > 4 files changed, 24 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h > index f908071..03bbe02 100644 > --- a/arch/arm/include/asm/smp_plat.h > +++ b/arch/arm/include/asm/smp_plat.h > @@ -49,7 +49,7 @@ static inline int tlb_ops_need_broadcast(void) > if (!is_smp()) > return 0; > > - return ((read_cpuid_ext(CPUID_EXT_MMFR3) >> 12) & 0xf) < 2; > + return cpuid_feature_extract_unsigned(CPUID_EXT_MMFR3, 12) < 2; > } > #endif > > @@ -61,7 +61,7 @@ static inline int cache_ops_need_broadcast(void) > if (!is_smp()) > return 0; > > - return ((read_cpuid_ext(CPUID_EXT_MMFR3) >> 12) & 0xf) < 1; > + return cpuid_feature_extract_unsigned(CPUID_EXT_MMFR3, 12) < 1; > } > #endif > > diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c > index fde041b..e696553 100644 > --- a/arch/arm/kernel/setup.c > +++ b/arch/arm/kernel/setup.c > @@ -257,11 +257,15 @@ static int __get_cpu_architecture(void) > /* Revised CPUID format. Read the Memory Model Feature > * Register 0 and check for VMSAv7 or PMSAv7 */ > unsigned int mmfr0 = read_cpuid_ext(CPUID_EXT_MMFR0); > - if ((mmfr0 & 0x0000000f) >= 0x00000003 || > - (mmfr0 & 0x000000f0) >= 0x00000030) > + unsigned int block; > +#ifdef CONFIG_MMU > + block = cpuid_feature_extract_unsigned_field(mmfr0, 0); > +#else > + block = cpuid_feature_extract_unsigned_field(mmfr0, 4); > +#endif > + if (mmfr0 >= 3) > cpu_arch = CPU_ARCH_ARMv7; > - else if ((mmfr0 & 0x0000000f) == 0x00000002 || > - (mmfr0 & 0x000000f0) == 0x00000020) > + else if (mmfr0 == 2) > cpu_arch = CPU_ARCH_ARMv6; > else > cpu_arch = CPU_ARCH_UNKNOWN; > @@ -446,41 +450,41 @@ static inline void patch_aeabi_idiv(void) { } > > static void __init cpuid_init_hwcaps(void) > { > - int block; > + unsigned int block; > u32 isar5; > > if (cpu_architecture() < CPU_ARCH_ARMv7) > return; > > - block = cpuid_feature_extract(CPUID_EXT_ISAR0, 24); > + block = cpuid_feature_extract_unsigned(CPUID_EXT_ISAR0, 24); > if (block >= 2) > elf_hwcap |= HWCAP_IDIVA; > if (block >= 1) > elf_hwcap |= HWCAP_IDIVT; > > /* LPAE implies atomic ldrd/strd instructions */ > - block = cpuid_feature_extract(CPUID_EXT_MMFR0, 0); > + block = cpuid_feature_extract_unsigned(CPUID_EXT_MMFR0, 0); > if (block >= 5) > elf_hwcap |= HWCAP_LPAE; > > /* check for supported v8 Crypto instructions */ > isar5 = read_cpuid_ext(CPUID_EXT_ISAR5); > > - block = cpuid_feature_extract_field(isar5, 4); > + block = cpuid_feature_extract_unsigned_field(isar5, 4); > if (block >= 2) > elf_hwcap2 |= HWCAP2_PMULL; > if (block >= 1) > elf_hwcap2 |= HWCAP2_AES; > > - block = cpuid_feature_extract_field(isar5, 8); > + block = cpuid_feature_extract_unsigned_field(isar5, 8); > if (block >= 1) > elf_hwcap2 |= HWCAP2_SHA1; > > - block = cpuid_feature_extract_field(isar5, 12); > + block = cpuid_feature_extract_unsigned_field(isar5, 12); > if (block >= 1) > elf_hwcap2 |= HWCAP2_SHA2; > > - block = cpuid_feature_extract_field(isar5, 16); > + block = cpuid_feature_extract_unsigned_field(isar5, 16); > if (block >= 1) > elf_hwcap2 |= HWCAP2_CRC32; > } > @@ -488,6 +492,7 @@ static void __init cpuid_init_hwcaps(void) > static void __init elf_hwcap_fixup(void) > { > unsigned id = read_cpuid_id(); > + unsigned int block; > > /* > * HWCAP_TLS is available only on 1136 r1p0 and later, > @@ -508,9 +513,10 @@ static void __init elf_hwcap_fixup(void) > * avoid advertising SWP; it may not be atomic with > * multiprocessing cores. > */ > - if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || > - (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && > - cpuid_feature_extract(CPUID_EXT_ISAR4, 20) >= 3)) > + block = cpuid_feature_extract_unsigned(CPUID_EXT_ISAR3, 12); > + > + if (block > 1 || (block == 1 && > + cpuid_feature_extract_unsigned(CPUID_EXT_ISAR4, 20) >= 3)) > elf_hwcap &= ~HWCAP_SWP; > } > > diff --git a/arch/arm/kernel/thumbee.c b/arch/arm/kernel/thumbee.c > index 8ff8dbf..b5cac80 100644 > --- a/arch/arm/kernel/thumbee.c > +++ b/arch/arm/kernel/thumbee.c > @@ -62,14 +62,12 @@ static struct notifier_block thumbee_notifier_block = { > > static int __init thumbee_init(void) > { > - unsigned long pfr0; > unsigned int cpu_arch = cpu_architecture(); > > if (cpu_arch < CPU_ARCH_ARMv7) > return 0; > > - pfr0 = read_cpuid_ext(CPUID_EXT_PFR0); > - if ((pfr0 & 0x0000f000) != 0x00001000) > + if (cpuid_feature_extract_unsigned(CPUID_EXT_PFR0, 12) != 1) > return 0; > > pr_info("ThumbEE CPU extension supported.\n"); > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c > index 434d76f..06723a9 100644 > --- a/arch/arm/mm/mmu.c > +++ b/arch/arm/mm/mmu.c > @@ -572,7 +572,7 @@ static void __init build_mem_type_table(void) > * in the Short-descriptor translation table format descriptors. > */ > if (cpu_arch == CPU_ARCH_ARMv7 && > - (read_cpuid_ext(CPUID_EXT_MMFR0) & 0xF) >= 4) { > + (cpuid_feature_extract_unsigned(CPUID_EXT_MMFR0, 0) >= 4)) { > user_pmd_table |= PMD_PXNTABLE; > } > #endif > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/4] Update cpu feature extraction 2016-01-26 9:23 [PATCH 0/4] Update cpu feature extraction Vladimir Murzin ` (3 preceding siblings ...) 2016-01-26 9:23 ` [PATCH 4/4] ARM: use proper helper while extracting cpu features Vladimir Murzin @ 2016-01-28 11:13 ` Vladimir Murzin 2016-01-28 17:01 ` Russell King - ARM Linux 4 siblings, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2016-01-28 11:13 UTC (permalink / raw) To: linux-arm-kernel On 26/01/16 09:23, Vladimir Murzin wrote: > Hi, > > This has been started as a fix in cpu feature extractor, but Suzuki suggested > syncing-up the whole cpu feature handling with the recent updates on arm64 > side [1,2]. > > I've kept fixes separately, so they can go as the are in case there is concern > on updating cpu feature handling. > > [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/455568 > [2] https://lkml.org/lkml/2015/11/18/570 > Russell, is it OK for patch tracker? Cheers Vladimir > Thanks! > > Vladimir Murzin (4): > ARM: fix demoting HWCAP_SWP > ARM: fix cpu feature extracting helper > ARM: introduce cpu feature helper for unsigned values > ARM: use proper helper while extracting cpu features > > arch/arm/include/asm/cputype.h | 16 +++++++++++++--- > arch/arm/include/asm/smp_plat.h | 4 ++-- > arch/arm/kernel/setup.c | 34 ++++++++++++++++++++-------------- > arch/arm/kernel/thumbee.c | 4 +--- > arch/arm/mm/mmu.c | 2 +- > 5 files changed, 37 insertions(+), 23 deletions(-) > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/4] Update cpu feature extraction 2016-01-28 11:13 ` [PATCH 0/4] Update cpu feature extraction Vladimir Murzin @ 2016-01-28 17:01 ` Russell King - ARM Linux 2016-01-28 17:14 ` Ard Biesheuvel 2016-01-29 10:36 ` Vladimir Murzin 0 siblings, 2 replies; 13+ messages in thread From: Russell King - ARM Linux @ 2016-01-28 17:01 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jan 28, 2016 at 11:13:10AM +0000, Vladimir Murzin wrote: > On 26/01/16 09:23, Vladimir Murzin wrote: > > Hi, > > > > This has been started as a fix in cpu feature extractor, but Suzuki suggested > > syncing-up the whole cpu feature handling with the recent updates on arm64 > > side [1,2]. > > > > I've kept fixes separately, so they can go as the are in case there is concern > > on updating cpu feature handling. > > > > [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/455568 > > [2] https://lkml.org/lkml/2015/11/18/570 > > > > Russell, is it OK for patch tracker? I'd really like to hear that someone has validated these changes or at least someone in ARM Ltd such as Will or Catalin has reviewed them; I remember the subject of whether certain fields are signed or unsigned being almost a personal opinion - we used to treat all fields as unsigned, but that was declared to be wrong, so we then went to treating them all as signed fields... See: commit b8c9592b4a6c93211c8163888a97880d608503b5 Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> Date: Thu Mar 19 19:03:25 2015 +0100 ARM: 8318/1: treat CPU feature register fields as signed quantities The various CPU feature registers consist of 4-bit blocks that represent signed quantities, whose positive values represent incremental features, and whose negative values are reserved. To improve forward compatibility, update the feature detection code to take possible future higher values into account, but ignore negative values. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> For instance: - sync_prim = ((read_cpuid_ext(CPUID_EXT_ISAR3) >> 8) & 0xf0) | - ((read_cpuid_ext(CPUID_EXT_ISAR4) >> 20) & 0x0f); - if (sync_prim >= 0x13) + if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || + (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && + cpuid_feature_extract(CPUID_EXT_ISAR3, 20) >= 3)) elf_hwcap &= ~HWCAP_SWP; (let's ignore the ISAR4 vs ISAR3 problem). What if ISAR4 20:23 have bit 23 set? In the original code, that would be treated as being ">= 3" but in the new code, that's less than zero, so would fail the test. I think we had much the same questions back in March 2015 when this was last discussed, when I raised the issue of there being a documentation bug in that the documentation doesn't make it clear how reserved 4-bit CPU ID fields should be intepreted. What I'd like to see is some kind of positive concensus on this (and a doc update); what I don't want to do is to apply these patches and then get another round of patches converting some of them back to signed tests, and then later another set of patches doing the reverse, because that seems to be what's now happening. -- RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/4] Update cpu feature extraction 2016-01-28 17:01 ` Russell King - ARM Linux @ 2016-01-28 17:14 ` Ard Biesheuvel 2016-01-29 10:36 ` Vladimir Murzin 1 sibling, 0 replies; 13+ messages in thread From: Ard Biesheuvel @ 2016-01-28 17:14 UTC (permalink / raw) To: linux-arm-kernel On 28 January 2016 at 18:01, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Jan 28, 2016 at 11:13:10AM +0000, Vladimir Murzin wrote: >> On 26/01/16 09:23, Vladimir Murzin wrote: >> > Hi, >> > >> > This has been started as a fix in cpu feature extractor, but Suzuki suggested >> > syncing-up the whole cpu feature handling with the recent updates on arm64 >> > side [1,2]. >> > >> > I've kept fixes separately, so they can go as the are in case there is concern >> > on updating cpu feature handling. >> > >> > [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/455568 >> > [2] https://lkml.org/lkml/2015/11/18/570 >> > >> >> Russell, is it OK for patch tracker? > > I'd really like to hear that someone has validated these changes or at > least someone in ARM Ltd such as Will or Catalin has reviewed them; I > remember the subject of whether certain fields are signed or unsigned > being almost a personal opinion - we used to treat all fields as > unsigned, but that was declared to be wrong, so we then went to treating > them all as signed fields... See: > > commit b8c9592b4a6c93211c8163888a97880d608503b5 > Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Date: Thu Mar 19 19:03:25 2015 +0100 > > ARM: 8318/1: treat CPU feature register fields as signed quantities > > The various CPU feature registers consist of 4-bit blocks that > represent signed quantities, whose positive values represent > incremental features, and whose negative values are reserved. > > To improve forward compatibility, update the feature detection > code to take possible future higher values into account, but > ignore negative values. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > For instance: > > - sync_prim = ((read_cpuid_ext(CPUID_EXT_ISAR3) >> 8) & 0xf0) | > - ((read_cpuid_ext(CPUID_EXT_ISAR4) >> 20) & 0x0f); > - if (sync_prim >= 0x13) > + if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || > + (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && > + cpuid_feature_extract(CPUID_EXT_ISAR3, 20) >= 3)) > elf_hwcap &= ~HWCAP_SWP; > > (let's ignore the ISAR4 vs ISAR3 problem). What if ISAR4 20:23 have > bit 23 set? In the original code, that would be treated as being ">= 3" > but in the new code, that's less than zero, so would fail the test. > > I think we had much the same questions back in March 2015 when this was > last discussed, when I raised the issue of there being a documentation > bug in that the documentation doesn't make it clear how reserved 4-bit > CPU ID fields should be intepreted. > > What I'd like to see is some kind of positive concensus on this (and > a doc update); what I don't want to do is to apply these patches and > then get another round of patches converting some of them back to > signed tests, and then later another set of patches doing the reverse, > because that seems to be what's now happening. > Indeed. As I commented to one of the other patches, having any kind of less-than/greater-than relation does not make any sense if the ARM ARM defines '0 means A, 1 means B, all other values reserved' since the whole point of the comparisons is to infer A or B (or both) from other values than 0 or 1. My understanding at the time was that these fields are signed 4-bit quantities, and so each positive value n implies all of [0, n-1] as well. If this turns out not to be the case, we should simply not infer the presence of features based on reserved-yet-greater-than-some-documented values, and back out my changes to that effect as well. -- Ard. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/4] Update cpu feature extraction 2016-01-28 17:01 ` Russell King - ARM Linux 2016-01-28 17:14 ` Ard Biesheuvel @ 2016-01-29 10:36 ` Vladimir Murzin 2016-04-19 11:39 ` Vladimir Murzin 1 sibling, 1 reply; 13+ messages in thread From: Vladimir Murzin @ 2016-01-29 10:36 UTC (permalink / raw) To: linux-arm-kernel On 28/01/16 17:01, Russell King - ARM Linux wrote: > On Thu, Jan 28, 2016 at 11:13:10AM +0000, Vladimir Murzin wrote: >> On 26/01/16 09:23, Vladimir Murzin wrote: >>> Hi, >>> >>> This has been started as a fix in cpu feature extractor, but Suzuki suggested >>> syncing-up the whole cpu feature handling with the recent updates on arm64 >>> side [1,2]. >>> >>> I've kept fixes separately, so they can go as the are in case there is concern >>> on updating cpu feature handling. >>> >>> [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/455568 >>> [2] https://lkml.org/lkml/2015/11/18/570 >>> >> >> Russell, is it OK for patch tracker? > > I'd really like to hear that someone has validated these changes or at > least someone in ARM Ltd such as Will or Catalin has reviewed them; I > remember the subject of whether certain fields are signed or unsigned > being almost a personal opinion - we used to treat all fields as > unsigned, but that was declared to be wrong, so we then went to treating > them all as signed fields... See: > > commit b8c9592b4a6c93211c8163888a97880d608503b5 > Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Date: Thu Mar 19 19:03:25 2015 +0100 > > ARM: 8318/1: treat CPU feature register fields as signed quantities > > The various CPU feature registers consist of 4-bit blocks that > represent signed quantities, whose positive values represent > incremental features, and whose negative values are reserved. > > To improve forward compatibility, update the feature detection > code to take possible future higher values into account, but > ignore negative values. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > For instance: > > - sync_prim = ((read_cpuid_ext(CPUID_EXT_ISAR3) >> 8) & 0xf0) | > - ((read_cpuid_ext(CPUID_EXT_ISAR4) >> 20) & 0x0f); > - if (sync_prim >= 0x13) > + if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || > + (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && > + cpuid_feature_extract(CPUID_EXT_ISAR3, 20) >= 3)) > elf_hwcap &= ~HWCAP_SWP; > > (let's ignore the ISAR4 vs ISAR3 problem). What if ISAR4 20:23 have > bit 23 set? In the original code, that would be treated as being ">= 3" > but in the new code, that's less than zero, so would fail the test. > > I think we had much the same questions back in March 2015 when this was > last discussed, when I raised the issue of there being a documentation > bug in that the documentation doesn't make it clear how reserved 4-bit > CPU ID fields should be intepreted. > > What I'd like to see is some kind of positive concensus on this (and > a doc update); what I don't want to do is to apply these patches and > then get another round of patches converting some of them back to > signed tests, and then later another set of patches doing the reverse, > because that seems to be what's now happening. > It is fair point. Since this topic is quite fuzzy and discussion can take some time I'd be happy if only fixes (the first two patches) can find their way to mainline independent of the rest of the series. Is it OK for patches 1/4 and 2/4 to go in patch tracker? Thanks Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/4] Update cpu feature extraction 2016-01-29 10:36 ` Vladimir Murzin @ 2016-04-19 11:39 ` Vladimir Murzin 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Murzin @ 2016-04-19 11:39 UTC (permalink / raw) To: linux-arm-kernel On 29/01/16 10:36, Vladimir Murzin wrote: > On 28/01/16 17:01, Russell King - ARM Linux wrote: >> On Thu, Jan 28, 2016 at 11:13:10AM +0000, Vladimir Murzin wrote: >>> On 26/01/16 09:23, Vladimir Murzin wrote: >>>> Hi, >>>> >>>> This has been started as a fix in cpu feature extractor, but Suzuki suggested >>>> syncing-up the whole cpu feature handling with the recent updates on arm64 >>>> side [1,2]. >>>> >>>> I've kept fixes separately, so they can go as the are in case there is concern >>>> on updating cpu feature handling. >>>> >>>> [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/455568 >>>> [2] https://lkml.org/lkml/2015/11/18/570 >>>> >>> >>> Russell, is it OK for patch tracker? >> >> I'd really like to hear that someone has validated these changes or at >> least someone in ARM Ltd such as Will or Catalin has reviewed them; I >> remember the subject of whether certain fields are signed or unsigned >> being almost a personal opinion - we used to treat all fields as >> unsigned, but that was declared to be wrong, so we then went to treating >> them all as signed fields... See: >> >> commit b8c9592b4a6c93211c8163888a97880d608503b5 >> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Date: Thu Mar 19 19:03:25 2015 +0100 >> >> ARM: 8318/1: treat CPU feature register fields as signed quantities >> >> The various CPU feature registers consist of 4-bit blocks that >> represent signed quantities, whose positive values represent >> incremental features, and whose negative values are reserved. >> >> To improve forward compatibility, update the feature detection >> code to take possible future higher values into account, but >> ignore negative values. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >> >> For instance: >> >> - sync_prim = ((read_cpuid_ext(CPUID_EXT_ISAR3) >> 8) & 0xf0) | >> - ((read_cpuid_ext(CPUID_EXT_ISAR4) >> 20) & 0x0f); >> - if (sync_prim >= 0x13) >> + if (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) > 1 || >> + (cpuid_feature_extract(CPUID_EXT_ISAR3, 12) == 1 && >> + cpuid_feature_extract(CPUID_EXT_ISAR3, 20) >= 3)) >> elf_hwcap &= ~HWCAP_SWP; >> >> (let's ignore the ISAR4 vs ISAR3 problem). What if ISAR4 20:23 have >> bit 23 set? In the original code, that would be treated as being ">= 3" >> but in the new code, that's less than zero, so would fail the test. >> >> I think we had much the same questions back in March 2015 when this was >> last discussed, when I raised the issue of there being a documentation >> bug in that the documentation doesn't make it clear how reserved 4-bit >> CPU ID fields should be intepreted. >> >> What I'd like to see is some kind of positive concensus on this (and >> a doc update); what I don't want to do is to apply these patches and >> then get another round of patches converting some of them back to >> signed tests, and then later another set of patches doing the reverse, >> because that seems to be what's now happening. >> > > It is fair point. Since this topic is quite fuzzy and discussion can > take some time I'd be happy if only fixes (the first two patches) can > find their way to mainline independent of the rest of the series. > > Is it OK for patches 1/4 and 2/4 to go in patch tracker? > I've just uploaded patches 1/4 and 2/4 to patch tracker with assumption they are OK, please, let me know in case it's wrong. Cheers Vladimir > Thanks > Vladimir > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-04-19 11:39 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-26 9:23 [PATCH 0/4] Update cpu feature extraction Vladimir Murzin 2016-01-26 9:23 ` [PATCH 1/4] ARM: fix demoting HWCAP_SWP Vladimir Murzin 2016-01-27 9:34 ` Ard Biesheuvel 2016-01-26 9:23 ` [PATCH 2/4] ARM: fix cpu feature extracting helper Vladimir Murzin 2016-01-27 9:35 ` Ard Biesheuvel 2016-01-26 9:23 ` [PATCH 3/4] ARM: introduce cpu feature helper for unsigned values Vladimir Murzin 2016-01-26 9:23 ` [PATCH 4/4] ARM: use proper helper while extracting cpu features Vladimir Murzin 2016-01-27 9:44 ` Ard Biesheuvel 2016-01-28 11:13 ` [PATCH 0/4] Update cpu feature extraction Vladimir Murzin 2016-01-28 17:01 ` Russell King - ARM Linux 2016-01-28 17:14 ` Ard Biesheuvel 2016-01-29 10:36 ` Vladimir Murzin 2016-04-19 11:39 ` Vladimir Murzin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.