Messages in this thread |  | | Subject | Re: [PATCH V3] arm64/cpuinfo: Define HWCAP name arrays per their actual bit definitions | From | Anshuman Khandual <> | Date | Tue, 8 Sep 2020 10:43:12 +0530 |
| |
On 09/07/2020 05:46 PM, Will Deacon wrote: > On Mon, Aug 17, 2020 at 05:34:23PM +0530, Anshuman Khandual wrote: >> HWCAP name arrays (hwcap_str, compat_hwcap_str, compat_hwcap2_str) that are >> scanned for /proc/cpuinfo are detached from their bit definitions making it >> vulnerable and difficult to correlate. It is also bit problematic because >> during /proc/cpuinfo dump these arrays get traversed sequentially assuming >> they reflect and match actual HWCAP bit sequence, to test various features >> for a given CPU. This redefines name arrays per their HWCAP bit definitions >> . It also warns after detecting any feature which is not expected on arm64. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Mark Brown <broonie@kernel.org> >> Cc: Dave Martin <Dave.Martin@arm.com> >> Cc: Ard Biesheuvel <ardb@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> This applies on 5.9-rc1 >> >> Mark, since the patch has changed I have dropped your Acked-by: tag. Are you >> happy to give a new one ? >> >> Changes in V3: >> >> - Moved name arrays to (arch/arm64/kernel/cpuinfo.c) to prevent a build warning >> - Replaced string values with NULL for all compat features not possible on arm64 >> - Changed compat_hwcap_str[] iteration on size as some NULL values are expected >> - Warn once after detecting any feature on arm64 that is not expected >> >> Changes in V2: (https://patchwork.kernel.org/patch/11533755/) >> >> - Defined COMPAT_KERNEL_HWCAP[2] and updated the name arrays per Mark >> - Updated the commit message as required >> >> Changes in V1: (https://patchwork.kernel.org/patch/11532945/) >> >> arch/arm64/include/asm/hwcap.h | 9 +++ >> arch/arm64/kernel/cpuinfo.c | 172 ++++++++++++++++++++++------------------- >> 2 files changed, 100 insertions(+), 81 deletions(-) > > [...] > >> + [KERNEL_HWCAP_FP] = "fp", >> + [KERNEL_HWCAP_ASIMD] = "asimd", >> + [KERNEL_HWCAP_EVTSTRM] = "evtstrm", >> + [KERNEL_HWCAP_AES] = "aes", > > It would be nice if the cap and the string were generated by the same > macro, along the lines of: > > #define KERNEL_HWCAP(c) [KERNEL_HWCAP_##c] = #c, > > Does making the constants mixed case break anything, or is it just really > churny to do?
Currently all existing HWCAP feature strings are lower case, above change will make them into upper case instead. I could not find a method to force convert #c into lower case constant strings in the macro definition. Would not changing the HWCAP string case here, break user interface ?
> >> @@ -166,9 +167,18 @@ static int c_show(struct seq_file *m, void *v) >> seq_puts(m, "Features\t:"); >> if (compat) { >> #ifdef CONFIG_COMPAT >> - for (j = 0; compat_hwcap_str[j]; j++) >> - if (compat_elf_hwcap & (1 << j)) >> + for (j = 0; j < ARRAY_SIZE(compat_hwcap_str); j++) { >> + if (compat_elf_hwcap & (1 << j)) { >> + /* >> + * Warn once if any feature should not >> + * have been present on arm64 platform. >> + */ >> + if (WARN_ON_ONCE(!compat_hwcap_str[j])) >> + continue; >> + >> seq_printf(m, " %s", compat_hwcap_str[j]); >> + } >> + } >> >> for (j = 0; compat_hwcap2_str[j]; j++) > > Hmm, I find this pretty confusing now as compat_hwcap_str is not NULL > terminated and must be traversed with a loop bounded by ARRAY_SIZE(...),
Right. Thats because unlike before, it can now have some intermediate NULL entries. Hence NULL sentinel based traversal wont be possible any more.
> whereas compat_hwcap2_str *is* NULL terminated and is traversed until you > hit the sentinel. > > I think hwcap_str, compat_hwcap_str and compat_hwcap2_str should be > identical in this regard.
Sure, will make the traversal based on ARRAY_SIZE() for all three arrays here, to make that uniform.
> > Will >
|  |