| Message ID | 20191007085312.27912-1-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly | expand |
Hi Suzuki, On Mon, Oct 07, 2019 at 09:53:12AM +0100, Suzuki K Poulose wrote: > We set the compat_elf_hwcap bits unconditionally on arm64 to > include the VFP and NEON support. However, the FP/SIMD unit > is optional on Arm v8 and thus could be missing. We already > handle this properly in the kernel, but still advertise to > the COMPAT applications that the VFP is available. Fix this > to make sure we only advertise when we really have them. Why didn't we find this in testing? Should be able to throw an armel Debian filesystem at the fastmodel in this configuration, no? > Fixes: commit 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD") nit: please drop "commit" from this tag. > Cc: Will Deacon <will@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > arch/arm64/kernel/cpufeature.c | 37 +++++++++++++++++++++++++++++++--- > 1 file changed, 34 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index 9323bcc40a58..9a28ba10dc03 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -32,9 +32,7 @@ static unsigned long elf_hwcap __read_mostly; > #define COMPAT_ELF_HWCAP_DEFAULT \ > (COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\ > COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\ > - COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\ > - COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\ > - COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV|\ > + COMPAT_HWCAP_TLS|COMPAT_HWCAP_IDIV|\ > COMPAT_HWCAP_LPAE) > unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT; > unsigned int compat_elf_hwcap2 __read_mostly; > @@ -1589,6 +1587,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = { > .match_list = list, \ > } > > +#define HWCAP_CAP_MATCH(match, cap_type, cap) \ > + { \ > + __HWCAP_CAP(#cap, cap_type, cap) \ > + .matches = match, \ > + } Do you actually need this, or can you use HWCAP_MULTI_CAP instead to check the mvfr1 fields? > + > #ifdef CONFIG_ARM64_PTR_AUTH > static const struct arm64_cpu_capabilities ptr_auth_hwcap_addr_matches[] = { > { > @@ -1662,8 +1666,35 @@ static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = { > {}, > }; > > +#ifdef CONFIG_COMPAT > +static bool compat_has_neon(const struct arm64_cpu_capabilities *cap, int scope) > +{ > + /* > + * Check that all of MVFR1_EL1.{SIMDSP, SIMDInt, SIMDLS} are available, > + * in line with that of arm32 as in vfp_init(). We make sure that the > + * check is future proof, by making sure value is non-zero. > + */ > + u32 mvfr1; > + > + WARN_ON(scope == SCOPE_LOCAL_CPU && preemptible()); > + if (scope == SCOPE_SYSTEM) > + mvfr1 = read_sanitised_ftr_reg(SYS_MVFR1_EL1); > + else > + mvfr1 = read_sysreg_s(SYS_MVFR1_EL1); > + > + return (mvfr1 & (0xf << MVFR1_SIMDSP_SHIFT)) && > + (mvfr1 & (0xf << MVFR1_SIMDINT_SHIFT)) && > + (mvfr1 & (0xf << MVFR1_SIMDLS_SHIFT)); If you can't use HWCAP_MULTI_CAP, can you use cpuid_feature_extract_field() instead of hardcoding shifts of 0xf? Will
Hi Will, On 07/10/2019 16:16, Will Deacon wrote: > Hi Suzuki, > > On Mon, Oct 07, 2019 at 09:53:12AM +0100, Suzuki K Poulose wrote: >> We set the compat_elf_hwcap bits unconditionally on arm64 to >> include the VFP and NEON support. However, the FP/SIMD unit >> is optional on Arm v8 and thus could be missing. We already >> handle this properly in the kernel, but still advertise to >> the COMPAT applications that the VFP is available. Fix this >> to make sure we only advertise when we really have them. > > Why didn't we find this in testing? Should be able to throw an armel > Debian filesystem at the fastmodel in this configuration, no? Apologies for this mistake. I didn't test it with the armel rootfs. Back then it was only tested to make sure we don't set the HWCAP. I have now tested this on an armel based initramfs and have uncovered other issues. Patches to follow in the next version. > >> Fixes: commit 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD") > > nit: please drop "commit" from this tag. Sure > >> Cc: Will Deacon <will@kernel.org> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >> --- >> arch/arm64/kernel/cpufeature.c | 37 +++++++++++++++++++++++++++++++--- >> 1 file changed, 34 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index 9323bcc40a58..9a28ba10dc03 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -32,9 +32,7 @@ static unsigned long elf_hwcap __read_mostly; >> #define COMPAT_ELF_HWCAP_DEFAULT \ >> (COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\ >> COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\ >> - COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\ >> - COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\ >> - COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV|\ >> + COMPAT_HWCAP_TLS|COMPAT_HWCAP_IDIV|\ >> COMPAT_HWCAP_LPAE) >> unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT; >> unsigned int compat_elf_hwcap2 __read_mostly; >> @@ -1589,6 +1587,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = { >> .match_list = list, \ >> } >> >> +#define HWCAP_CAP_MATCH(match, cap_type, cap) \ >> + { \ >> + __HWCAP_CAP(#cap, cap_type, cap) \ >> + .matches = match, \ >> + } > > Do you actually need this, or can you use HWCAP_MULTI_CAP instead to check > the mvfr1 fields? MULTT_CAP gives us ( cap1 || cap2 ||... ) and we want (cap1 && cap2 &&...) > >> + >> #ifdef CONFIG_ARM64_PTR_AUTH >> static const struct arm64_cpu_capabilities ptr_auth_hwcap_addr_matches[] = { >> { >> @@ -1662,8 +1666,35 @@ static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = { >> {}, >> }; >> >> +#ifdef CONFIG_COMPAT >> +static bool compat_has_neon(const struct arm64_cpu_capabilities *cap, int scope) >> +{ >> + /* >> + * Check that all of MVFR1_EL1.{SIMDSP, SIMDInt, SIMDLS} are available, >> + * in line with that of arm32 as in vfp_init(). We make sure that the >> + * check is future proof, by making sure value is non-zero. >> + */ >> + u32 mvfr1; >> + >> + WARN_ON(scope == SCOPE_LOCAL_CPU && preemptible()); >> + if (scope == SCOPE_SYSTEM) >> + mvfr1 = read_sanitised_ftr_reg(SYS_MVFR1_EL1); >> + else >> + mvfr1 = read_sysreg_s(SYS_MVFR1_EL1); >> + >> + return (mvfr1 & (0xf << MVFR1_SIMDSP_SHIFT)) && >> + (mvfr1 & (0xf << MVFR1_SIMDINT_SHIFT)) && >> + (mvfr1 & (0xf << MVFR1_SIMDLS_SHIFT)); > > If you can't use HWCAP_MULTI_CAP, can you use cpuid_feature_extract_field() > instead of hardcoding shifts of 0xf? Sure, that sounds better. will do that. Suzuki
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 9323bcc40a58..9a28ba10dc03 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -32,9 +32,7 @@ static unsigned long elf_hwcap __read_mostly; #define COMPAT_ELF_HWCAP_DEFAULT \ (COMPAT_HWCAP_HALF|COMPAT_HWCAP_THUMB|\ COMPAT_HWCAP_FAST_MULT|COMPAT_HWCAP_EDSP|\ - COMPAT_HWCAP_TLS|COMPAT_HWCAP_VFP|\ - COMPAT_HWCAP_VFPv3|COMPAT_HWCAP_VFPv4|\ - COMPAT_HWCAP_NEON|COMPAT_HWCAP_IDIV|\ + COMPAT_HWCAP_TLS|COMPAT_HWCAP_IDIV|\ COMPAT_HWCAP_LPAE) unsigned int compat_elf_hwcap __read_mostly = COMPAT_ELF_HWCAP_DEFAULT; unsigned int compat_elf_hwcap2 __read_mostly; @@ -1589,6 +1587,12 @@ static const struct arm64_cpu_capabilities arm64_features[] = { .match_list = list, \ } +#define HWCAP_CAP_MATCH(match, cap_type, cap) \ + { \ + __HWCAP_CAP(#cap, cap_type, cap) \ + .matches = match, \ + } + #ifdef CONFIG_ARM64_PTR_AUTH static const struct arm64_cpu_capabilities ptr_auth_hwcap_addr_matches[] = { { @@ -1662,8 +1666,35 @@ static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = { {}, }; +#ifdef CONFIG_COMPAT +static bool compat_has_neon(const struct arm64_cpu_capabilities *cap, int scope) +{ + /* + * Check that all of MVFR1_EL1.{SIMDSP, SIMDInt, SIMDLS} are available, + * in line with that of arm32 as in vfp_init(). We make sure that the + * check is future proof, by making sure value is non-zero. + */ + u32 mvfr1; + + WARN_ON(scope == SCOPE_LOCAL_CPU && preemptible()); + if (scope == SCOPE_SYSTEM) + mvfr1 = read_sanitised_ftr_reg(SYS_MVFR1_EL1); + else + mvfr1 = read_sysreg_s(SYS_MVFR1_EL1); + + return (mvfr1 & (0xf << MVFR1_SIMDSP_SHIFT)) && + (mvfr1 & (0xf << MVFR1_SIMDINT_SHIFT)) && + (mvfr1 & (0xf << MVFR1_SIMDLS_SHIFT)); +} +#endif + static const struct arm64_cpu_capabilities compat_elf_hwcaps[] = { #ifdef CONFIG_COMPAT + HWCAP_CAP_MATCH(compat_has_neon, CAP_COMPAT_HWCAP, COMPAT_HWCAP_NEON), + HWCAP_CAP(SYS_MVFR1_EL1, MVFR1_SIMDFMAC_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFPv4), + /* Arm v8 mandates MVFR0.FPDP == {0, 2}. So, piggy back on this for the presence of VFP support */ + HWCAP_CAP(SYS_MVFR0_EL1, MVFR0_FPDP_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFP), + HWCAP_CAP(SYS_MVFR0_EL1, MVFR0_FPDP_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP, COMPAT_HWCAP_VFPv3), HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_AES_SHIFT, FTR_UNSIGNED, 2, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_PMULL), HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_AES_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_AES), HWCAP_CAP(SYS_ID_ISAR5_EL1, ID_ISAR5_SHA1_SHIFT, FTR_UNSIGNED, 1, CAP_COMPAT_HWCAP2, COMPAT_HWCAP2_SHA1),
We set the compat_elf_hwcap bits unconditionally on arm64 to include the VFP and NEON support. However, the FP/SIMD unit is optional on Arm v8 and thus could be missing. We already handle this properly in the kernel, but still advertise to the COMPAT applications that the VFP is available. Fix this to make sure we only advertise when we really have them. Fixes: commit 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD") Cc: Will Deacon <will@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- arch/arm64/kernel/cpufeature.c | 37 +++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)