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

[Anshuman Khandual]

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@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mark Brown <broonie@xxxxxxxxxx>
>> Cc: Dave Martin <Dave.Martin@xxxxxxx>
>> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> ---
>> 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
>