Re: [PATCH V3] arm64/cpuinfo: Define HWCAP name arrays per their actual bit definitions

[Will Deacon <will@kernel.org>]

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?

> @@ -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(...),
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.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel