linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling
@ 2024-02-12 14:47 Marc Zyngier
  2024-02-12 14:47 ` [PATCH 1/2] arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding() Marc Zyngier
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Marc Zyngier @ 2024-02-12 14:47 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel, kvm
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Catalin Marinas, Will Deacon, Marek Szyprowski

As the FEAT_E2H0 handling series has made it to -next, some of its
shortcomings are becoming apparent:

- A missing ID register in __read_sysreg_by_encoding() is causing CPU
  hotplug to explode (reported by Marek)

- NV1 is getting advertised on HW that doesn't have FEAT_NV, which is
  fairly harmless, but still annoying

These fixes should directly apply on the feat_e2h0 branch that Oliver
has pushed to kvmarm/next.

Marc Zyngier (2):
  arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to
    __read_sysreg_by_encoding()
  arm64: cpufeatures: Only check for NV1 if NV is present

 arch/arm64/kernel/cpufeature.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.39.2


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding()
  2024-02-12 14:47 [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Marc Zyngier
@ 2024-02-12 14:47 ` Marc Zyngier
  2024-02-12 14:47 ` [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present Marc Zyngier
  2024-02-12 18:05 ` [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Oliver Upton
  2 siblings, 0 replies; 10+ messages in thread
From: Marc Zyngier @ 2024-02-12 14:47 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel, kvm
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Catalin Marinas, Will Deacon, Marek Szyprowski

When triggering a CPU hotplug scenario, we reparse the CPU feature
with SCOPE_LOCAL_CPU, for which we use __read_sysreg_by_encoding()
to get the HW value for this CPU.

As it turns out, we're missing the handling for ID_AA64MMFR4_EL1,
and trigger a BUG(). Funnily enough, Marek isn't completely happy
about that.

Add the damn register to the list.

Fixes: 805bb61f8279 ("arm64: cpufeature: Add ID_AA64MMFR4_EL1 handling")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kernel/cpufeature.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0f29ac43c7a2..2f8958f27e9e 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1456,6 +1456,7 @@ u64 __read_sysreg_by_encoding(u32 sys_id)
 	read_sysreg_case(SYS_ID_AA64MMFR1_EL1);
 	read_sysreg_case(SYS_ID_AA64MMFR2_EL1);
 	read_sysreg_case(SYS_ID_AA64MMFR3_EL1);
+	read_sysreg_case(SYS_ID_AA64MMFR4_EL1);
 	read_sysreg_case(SYS_ID_AA64ISAR0_EL1);
 	read_sysreg_case(SYS_ID_AA64ISAR1_EL1);
 	read_sysreg_case(SYS_ID_AA64ISAR2_EL1);
-- 
2.39.2


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-12 14:47 [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Marc Zyngier
  2024-02-12 14:47 ` [PATCH 1/2] arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding() Marc Zyngier
@ 2024-02-12 14:47 ` Marc Zyngier
  2024-02-13 11:14   ` Marek Szyprowski
  2024-02-12 18:05 ` [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Oliver Upton
  2 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2024-02-12 14:47 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel, kvm
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Catalin Marinas, Will Deacon, Marek Szyprowski

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>
---
 arch/arm64/kernel/cpufeature.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 2f8958f27e9e..3421b684d340 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1812,8 +1812,9 @@ static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope)
 		{}
 	};
 
-	return !(has_cpuid_feature(entry, scope) ||
-		 is_midr_in_range_list(read_cpuid_id(), nv1_ni_list));
+	return (this_cpu_has_cap(ARM64_HAS_NESTED_VIRT) &&
+		!(has_cpuid_feature(entry, scope) ||
+		  is_midr_in_range_list(read_cpuid_id(), nv1_ni_list)));
 }
 
 #if defined(ID_AA64MMFR0_EL1_TGRAN_LPA2) && defined(ID_AA64MMFR0_EL1_TGRAN_2_SUPPORTED_LPA2)
-- 
2.39.2


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling
  2024-02-12 14:47 [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Marc Zyngier
  2024-02-12 14:47 ` [PATCH 1/2] arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding() Marc Zyngier
  2024-02-12 14:47 ` [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present Marc Zyngier
@ 2024-02-12 18:05 ` Oliver Upton
  2 siblings, 0 replies; 10+ messages in thread
From: Oliver Upton @ 2024-02-12 18:05 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm, Marc Zyngier
  Cc: Oliver Upton, Will Deacon, Suzuki K Poulose, Catalin Marinas,
	Marek Szyprowski, Zenghui Yu, James Morse

On Mon, 12 Feb 2024 14:47:34 +0000, Marc Zyngier wrote:
> As the FEAT_E2H0 handling series has made it to -next, some of its
> shortcomings are becoming apparent:
> 
> - A missing ID register in __read_sysreg_by_encoding() is causing CPU
>   hotplug to explode (reported by Marek)
> 
> - NV1 is getting advertised on HW that doesn't have FEAT_NV, which is
>   fairly harmless, but still annoying
> 
> [...]

Applied to kvmarm/next, thanks!

[1/2] arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding()
      https://git.kernel.org/kvmarm/kvmarm/c/87b8cf2387c5
[2/2] arm64: cpufeatures: Only check for NV1 if NV is present
      https://git.kernel.org/kvmarm/kvmarm/c/3673d01a2f55

--
Best,
Oliver

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-12 14:47 ` [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present Marc Zyngier
@ 2024-02-13 11:14   ` Marek Szyprowski
  2024-02-13 14:21     ` Marc Zyngier
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2024-02-13 11:14 UTC (permalink / raw)
  To: Marc Zyngier, kvmarm, linux-arm-kernel, kvm
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Catalin Marinas, Will Deacon

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


> ---
>   arch/arm64/kernel/cpufeature.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 2f8958f27e9e..3421b684d340 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1812,8 +1812,9 @@ static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope)
>   		{}
>   	};
>   
> -	return !(has_cpuid_feature(entry, scope) ||
> -		 is_midr_in_range_list(read_cpuid_id(), nv1_ni_list));
> +	return (this_cpu_has_cap(ARM64_HAS_NESTED_VIRT) &&
> +		!(has_cpuid_feature(entry, scope) ||
> +		  is_midr_in_range_list(read_cpuid_id(), nv1_ni_list)));
>   }
>   
>   #if defined(ID_AA64MMFR0_EL1_TGRAN_LPA2) && defined(ID_AA64MMFR0_EL1_TGRAN_2_SUPPORTED_LPA2)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-13 11:14   ` Marek Szyprowski
@ 2024-02-13 14:21     ` Marc Zyngier
  2024-02-13 14:54       ` Suzuki K Poulose
                         ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Marc Zyngier @ 2024-02-13 14:21 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: kvmarm, linux-arm-kernel, kvm, James Morse, Suzuki K Poulose,
	Oliver Upton, Zenghui Yu, Catalin Marinas, Will Deacon

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) &&
 		!(has_cpuid_feature(entry, scope) ||
 		  is_midr_in_range_list(read_cpuid_id(), nv1_ni_list)));
 }
-- 
2.39.2


-- 
Without deviation from the norm, progress is not possible.

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-13 14:21     ` Marc Zyngier
@ 2024-02-13 14:54       ` Suzuki K Poulose
  2024-02-13 20:06       ` Marek Szyprowski
  2024-02-15  2:02       ` Oliver Upton
  2 siblings, 0 replies; 10+ messages in thread
From: Suzuki K Poulose @ 2024-02-13 14:54 UTC (permalink / raw)
  To: Marc Zyngier, Marek Szyprowski
  Cc: kvmarm, linux-arm-kernel, kvm, James Morse, Oliver Upton,
	Zenghui Yu, Catalin Marinas, Will Deacon

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)));
>   }


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-13 14:21     ` Marc Zyngier
  2024-02-13 14:54       ` Suzuki K Poulose
@ 2024-02-13 20:06       ` Marek Szyprowski
  2024-02-15  2:02       ` Oliver Upton
  2 siblings, 0 replies; 10+ messages in thread
From: Marek Szyprowski @ 2024-02-13 20:06 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvmarm, linux-arm-kernel, kvm, James Morse, Suzuki K Poulose,
	Oliver Upton, Zenghui Yu, Catalin Marinas, Will Deacon

On 13.02.2024 15:21, Marc Zyngier wrote:
> On Tue, 13 Feb 2024 11:14:37 +0000,
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> 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?

Works fine for me.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

>  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) &&
>   		!(has_cpuid_feature(entry, scope) ||
>   		  is_midr_in_range_list(read_cpuid_id(), nv1_ni_list)));
>   }

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-13 14:21     ` Marc Zyngier
  2024-02-13 14:54       ` Suzuki K Poulose
  2024-02-13 20:06       ` Marek Szyprowski
@ 2024-02-15  2:02       ` Oliver Upton
  2024-02-15 15:19         ` Marc Zyngier
  2 siblings, 1 reply; 10+ messages in thread
From: Oliver Upton @ 2024-02-15  2:02 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Marek Szyprowski, kvmarm, linux-arm-kernel, kvm, James Morse,
	Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon

On Tue, Feb 13, 2024 at 02:21:48PM +0000, Marc Zyngier wrote:
> 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>

b4 wasn't very happy with grabbing this patch in reply to a series
(probably user error), but I've picked this up for kvmarm/next.

https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?id=9aa030cee1c45d6e962f6bf22ba63d4aff2b1644

-- 
Thanks,
Oliver

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present
  2024-02-15  2:02       ` Oliver Upton
@ 2024-02-15 15:19         ` Marc Zyngier
  0 siblings, 0 replies; 10+ messages in thread
From: Marc Zyngier @ 2024-02-15 15:19 UTC (permalink / raw)
  To: Oliver Upton
  Cc: Marek Szyprowski, kvmarm, linux-arm-kernel, kvm, James Morse,
	Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon

On Thu, 15 Feb 2024 02:02:24 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Feb 13, 2024 at 02:21:48PM +0000, Marc Zyngier wrote:
> > 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>
> 
> b4 wasn't very happy with grabbing this patch in reply to a series
> (probably user error), but I've picked this up for kvmarm/next.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?id=9aa030cee1c45d6e962f6bf22ba63d4aff2b1644

Ah, sorry, I should have posted as a proper patch instead of an inline
patch. Thanks for picking it up despite that.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-02-15 17:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-12 14:47 [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Marc Zyngier
2024-02-12 14:47 ` [PATCH 1/2] arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding() Marc Zyngier
2024-02-12 14:47 ` [PATCH 2/2] arm64: cpufeatures: Only check for NV1 if NV is present Marc Zyngier
2024-02-13 11:14   ` Marek Szyprowski
2024-02-13 14:21     ` Marc Zyngier
2024-02-13 14:54       ` Suzuki K Poulose
2024-02-13 20:06       ` Marek Szyprowski
2024-02-15  2:02       ` Oliver Upton
2024-02-15 15:19         ` Marc Zyngier
2024-02-12 18:05 ` [PATCH 0/2] ARM64: Fixes for FEAT_E2H0 handling Oliver Upton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).