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.mari...@arm.com>
>> Cc: Will Deacon <w...@kernel.org>
>> Cc: Mark Brown <broo...@kernel.org>
>> Cc: Dave Martin <dave.mar...@arm.com>
>> Cc: Ard Biesheuvel <a...@kernel.org>
>> Cc: Mark Rutland <mark.rutl...@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poul...@arm.com>
>> Cc: linux-arm-ker...@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khand...@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
>
