Re: [PATCH V2] arm64/cpuinfo: Move HWCAP name arrays alongside their bit definitions

[Anshuman Khandual]

On 05/14/2020 01:06 PM, Will Deacon wrote:
> On Thu, May 14, 2020 at 07:14:58AM +0530, Anshuman Khandual wrote:
>> On 05/13/2020 08:34 PM, Dave Martin wrote:
>>> On Thu, May 07, 2020 at 06:59:10PM +0530, Anshuman Khandual wrote:
>>>> diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
>>>> index 0f00265248b5..589ac02e1ddd 100644
>>>> --- a/arch/arm64/include/asm/hwcap.h
>>>> +++ b/arch/arm64/include/asm/hwcap.h
>>>> @@ -8,18 +8,27 @@
>>>>  #include <uapi/asm/hwcap.h>
>>>>  #include <asm/cpufeature.h>
>>>>  
>>>> +#define COMPAT_HWCAP_SWP	(1 << 0)
>>>>  #define COMPAT_HWCAP_HALF	(1 << 1)
>>>>  #define COMPAT_HWCAP_THUMB	(1 << 2)
>>>> +#define COMPAT_HWCAP_26BIT	(1 << 3)
>>>>  #define COMPAT_HWCAP_FAST_MULT	(1 << 4)
>>>> +#define COMPAT_HWCAP_FPA	(1 << 5)
>>>>  #define COMPAT_HWCAP_VFP	(1 << 6)
>>>>  #define COMPAT_HWCAP_EDSP	(1 << 7)
>>>> +#define COMPAT_HWCAP_JAVA	(1 << 8)
>>>> +#define COMPAT_HWCAP_IWMMXT	(1 << 9)
>>>> +#define COMPAT_HWCAP_CRUNCH	(1 << 10)
>>>> +#define COMPAT_HWCAP_THUMBEE	(1 << 11)
>>>>  #define COMPAT_HWCAP_NEON	(1 << 12)
>>>>  #define COMPAT_HWCAP_VFPv3	(1 << 13)
>>>> +#define COMPAT_HWCAP_VFPV3D16	(1 << 14)
>>>>  #define COMPAT_HWCAP_TLS	(1 << 15)
>>>>  #define COMPAT_HWCAP_VFPv4	(1 << 16)
>>>>  #define COMPAT_HWCAP_IDIVA	(1 << 17)
>>>>  #define COMPAT_HWCAP_IDIVT	(1 << 18)
>>>>  #define COMPAT_HWCAP_IDIV	(COMPAT_HWCAP_IDIVA|COMPAT_HWCAP_IDIVT)
>>>> +#define COMPAT_HWCAP_VFPD32	(1 << 19)
>>>>  #define COMPAT_HWCAP_LPAE	(1 << 20)
>>>>  #define COMPAT_HWCAP_EVTSTRM	(1 << 21)
>>>
>>> With the possible exception of SWP (does the swp emulation allow us to
>>> report this as supported?), I think all these weren't mentioned because
>>> they aren't included in ARMv8 and so can never be reported.
>>>
>>> If we find ourselves reporting them, there's a bug somewhere.
>>>
>>> So, can we just default all obsolete string entries to NULL?
>>>
>>> When generating the cpuinfo strings we could WARN and just emit an empty
>>> string for that hwcap.
>>
>> All these above will be a change in the existing user visible behavior on
>> the system and this patch never intended to create one.
> 
> Why is it a change? We've never reported e.g. "java" on an arm64 kernel, so

We already have "java" defined in existing compat_hwcap_str[] array even
though it might never get set in compat_elf_hwcap. AFAICS, compat_elf_hwcap
will have the following capabilities set (at the most).

Via COMPAT_ELF_HWCAP_DEFAULT

01.  COMPAT_HWCAP_HALF
02.  COMPAT_HWCAP_THUMB
03.  COMPAT_HWCAP_FAST_MULT
04.  COMPAT_HWCAP_EDSP
05.  COMPAT_HWCAP_TLS
06.  COMPAT_HWCAP_IDIV
07.  COMPAT_HWCAP_LPAE

Via setup_elf_hwcaps(compat_elf_hwcaps) <-- setup_cpu_features()

8.  COMPAT_HWCAP_NEON
9.  COMPAT_HWCAP_VFPv4
10. COMPAT_HWCAP_VFP
11. COMPAT_HWCAP_VFPv3

Via arch_timer_set_evtstrm_feature()

12. COMPAT_HWCAP_EVTSTRM

The code exists for "java" string to be displayed with /proc/cpuinfo but it
may never get triggered as compat_elf_hwcap will never have JAVA capability
unless there is a bug as you had rightly mentioned.

The current patch preserves status quo without fixing the underlying problem
("java" should have been dropped from possible /proc/cpuinfo display strings
as it can never happen). It tries to formalize what is already being done.

arch/arm64/kernel/cpuinfo.c

static int c_show(struct seq_file *m, void *v)
{
	....
#ifdef CONFIG_COMPAT
	for (j = 0; compat_hwcap_str[j]; j++)
		if (compat_elf_hwcap & (1 << j))
		seq_printf(m, " %s", compat_hwcap_str[j]);
	....
#endif
	....
}

When compat_hwcap_str[j] == "java" just define COMPAT_HWCAP_JAVA as (1 << j).

> I agree with Dave that we shouldn't be adding this string. If it /ever/ ends
> up in userspace it's because something has gone horribly wrong. NULL would
> be much better. Couldn't you achieve that by simply omitting these entries?
> e.g. deleting things like:
> 
> 	[COMPAT_KERNEL_HWCAP(JAVA)]     = "java",
> 
> completely (including the COMPAT_HWCAP_JAVA definition)?

Right, dropping these above will fix the existing underlying problem. But the
only point here is should that be done in this patch (which keeps everything
as is) or with a separate patch fixing an already existing problem.

> 
>> Hence, I will just defer this to maintainers on whether we should change
>> existing /proc/cpuinfo output (including non-practically-possible ones on
>> ARMv8) or even treat swap emulation as SWP.
> 
> SWP is fine because we emulate it and so userspace can use it. Removing that
> *would* be a change in behaviour. I don't think the compat ABI is broken
> here, so please don't change it without good reason.

Understood, this patch was not trying to change the compat ABI. SWP just gets
carried over as is like before.