Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 13/02/2024 14:21, Marc Zyngier wrote:
> On Tue, 13 Feb 2024 11:14:37 +0000,
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>>
>> Hi
>>
>> On 12.02.2024 15:47, Marc Zyngier wrote:
>>> We handle ID_AA64MMFR4_EL1.E2H0 being 0 as NV1 being present.
>>> However, this is only true if FEAT_NV is implemented.
>>>
>>> Add the required check to has_nv1(), avoiding spuriously advertising
>>> NV1 on HW that doesn't have NV at all.
>>>
>>> Fixes: da9af5071b25 ("arm64: cpufeature: Detect HCR_EL2.NV1 being RES0")
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>
>> This patch in turn introduces the following warning during boot
>> (observed on today's linux-next):
>>
>> CPU: All CPU(s) started at EL2
>> CPU features: detected: 32-bit EL0 Support
>> CPU features: detected: 32-bit EL1 Support
>> CPU features: detected: CRC32 instructions
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1 at arch/arm64/kernel/cpufeature.c:3369
>> this_cpu_has_cap+0x18/0x70
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc4-next-20240213 #8014
>> Hardware name: Khadas VIM3 (DT)
>> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> pc : this_cpu_has_cap+0x18/0x70
>> lr : has_nv1+0x24/0xcc
>> ...
>> Call trace:
>>    this_cpu_has_cap+0x18/0x70
>>    update_cpu_capabilities+0x50/0x134
>>    setup_system_features+0x30/0x120
>>    smp_cpus_done+0x48/0xb4
>>    smp_init+0x7c/0x8c
>>    kernel_init_freeable+0x18c/0x4e4
>>    kernel_init+0x20/0x1d8
>>    ret_from_fork+0x10/0x20
>> irq event stamp: 2846
>> hardirqs last  enabled at (2845): [<ffff80008012cf5c>]
>> console_unlock+0x164/0x190
>> hardirqs last disabled at (2846): [<ffff80008123a078>] el1_dbg+0x24/0x8c
>> softirqs last  enabled at (2842): [<ffff800080010a60>]
>> __do_softirq+0x4a0/0x4e8
>> softirqs last disabled at (2827): [<ffff8000800169b0>]
>> ____do_softirq+0x10/0x1c
>> ---[ end trace 0000000000000000 ]---
>> alternatives: applying system-wide alternatives
> 
> This is nothing short of embarrassing. It looks like I somehow managed
> to drop CONFIG_PREEMPT from my test config, making it impossible to
> identify these issues. Apologies for that.
> 
> The following patch fixes it for me. Could you please give it a go?
> 
> Thanks,
> 
> 	M.
> 
>  From cd75279d3b6c387c13972b61c486a203d9652e97 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Tue, 13 Feb 2024 13:37:57 +0000
> Subject: [PATCH] arm64: cpufeatures: Fix FEAT_NV check when checking for
>   FEAT_NV1
> 
> Using this_cpu_has_cap() has the potential to go wrong when
> used system-wide on a preemptible kernel. Instead, use the
> __system_matches_cap() helper when checking for FEAT_NV in the
> FEAT_NV1 probing helper.
> 
> Fixes: 3673d01a2f55 ("arm64: cpufeatures: Only check for NV1 if NV is present")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>   arch/arm64/kernel/cpufeature.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 3421b684d340..f309fd542c20 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1812,7 +1812,7 @@ static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope)
>   		{}
>   	};
>   
> -	return (this_cpu_has_cap(ARM64_HAS_NESTED_VIRT) &&
> +	return (__system_matches_cap(ARM64_HAS_NESTED_VIRT) &&

Even though this change now uses SYSTEM scope for NESTED_VIRT, that is 
the correct choice. Ideally, we should use the scope that was passed 
into "has_v1", but as we have seen SCOPE_LOCAL is not safe. SYSTEM
schope works fine as both NV1 and NESTED_VIRT are SYSTEM scope. The only 
time we run them SCOPE_LOCAL is for hotplugged in CPUs, at which point 
the SYSTEM wide caps are finalized and will use the right value.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>



>   		!(has_cpuid_feature(entry, scope) ||
>   		  is_midr_in_range_list(read_cpuid_id(), nv1_ni_list)));
>   }