Skip to contentSkip to conversation

ARM64: Fixes for FEAT_E2H0 handling

PATCH Applied Marc Zyngier 4 participants 10 messages Feb 12, 2024 – Feb 15, 2024

Subsystems (3)
lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

cover ARM64: Fixes for FEAT_E2H0 handling

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(-)
Signature · 1 line
2.39.2
author maintainer
#1 lore raw

PATCH 1/2 arm64: cpufeatures: Add missing ID_AA64MMFR4_EL1 to __read_sysreg_by_encoding()

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")
---
 arch/arm64/kernel/cpufeature.c | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/kernel/cpufeature.c +1 -0
index 0f29ac43c7a2..2f8958f27e9e 100644
@@ -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);
Signature · 1 line
2.39.2
author maintainer
#2 lore raw

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

In reply to Marc Zyngier

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")
---
 arch/arm64/kernel/cpufeature.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm64/kernel/cpufeature.c +3 -2
index 2f8958f27e9e..3421b684d340 100644
@@ -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)
Signature · 1 line
2.39.2
#3 lore raw

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

Hi

Quoted 8 lines · Marc Zyngier

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")

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

Quoted 10 lines
---
  arch/arm64/kernel/cpufeature.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
arch/arm64/kernel/cpufeature.c +0 -0
index 2f8958f27e9e..3421b684d340 100644
@@ -1812,8 +1812,9 @@ static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope)
  		{}

arch/arm64/kernel/cpufeature.c static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope) · line 1820

Quoted 10 lines
arch/arm64/kernel/cpufeature.c +3 -2
  	};
 
-	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

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author maintainer
#4 lore raw

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

Quoted 48 lines · Marek Szyprowski

Hi

Quoted 8 lines · Marc Zyngier

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")

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 ]---
Quoted 1 line

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 <[email protected]>
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")
---
 arch/arm64/kernel/cpufeature.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/cpufeature.c +1 -1
index 3421b684d340..f309fd542c20 100644
@@ -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)));
 }
Signature · 5 lines
2.39.2


-- 
Without deviation from the norm, progress is not possible.
reviewer Reviewed-by
#5 lore raw

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

Quoted 85 lines · Marc Zyngier
Quoted 48 lines · Marek Szyprowski

Hi

Quoted 8 lines · Marc Zyngier

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")

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 ]---
Quoted 1 line

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 <[email protected]>
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")
---
  arch/arm64/kernel/cpufeature.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/cpufeature.c +0 -0
index 3421b684d340..f309fd542c20 100644

arch/arm64/kernel/cpufeature.c static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope) · line 1815

Quoted 5 lines
arch/arm64/kernel/cpufeature.c +1 -1
@@ -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.

Quoted 3 lines
arch/arm64/kernel/cpufeature.c +0 -0
  		!(has_cpuid_feature(entry, scope) ||
  		  is_midr_in_range_list(read_cpuid_id(), nv1_ni_list)));
  }
#6 lore raw

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

In reply to Marc Zyngier
Quoted 47 lines · Marc Zyngier
Quoted 44 lines · Marek Szyprowski
Quoted 8 lines · Marc Zyngier

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")

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 ]---
Quoted 1 line

alternatives: applying system-wide alternatives

Quoted 5 lines

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.

Quoted 23 lines

From cd75279d3b6c387c13972b61c486a203d9652e97 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
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")
---
  arch/arm64/kernel/cpufeature.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kernel/cpufeature.c +0 -0
index 3421b684d340..f309fd542c20 100644

arch/arm64/kernel/cpufeature.c static bool has_nv1(const struct arm64_cpu_capabilities *entry, int scope) · line 1818

Quoted 8 lines
arch/arm64/kernel/cpufeature.c +1 -1
@@ -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

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
#7 lore raw

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

In reply to Marc Zyngier
Quoted 6 lines · Marc Zyngier

From cd75279d3b6c387c13972b61c486a203d9652e97 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Tue, 13 Feb 2024 13:37:57 +0000
Subject: [PATCH] arm64: cpufeatures: Fix FEAT_NV check when checking for
FEAT_NV1

Quoted 8 lines

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")

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

Signature · 2 lines
Thanks,
Oliver
author maintainer
#8 lore raw

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

Quoted 13 lines · Oliver Upton
Quoted 11 lines · Marc Zyngier

From cd75279d3b6c387c13972b61c486a203d9652e97 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
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.

Quoted 8 lines
Quoted 3 lines
Fixes 3673d01a2f55 ("arm64: cpufeatures: Only check for NV1 if NV is present")

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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Applied
#9 lore raw
In reply to Marc Zyngier
Quoted 10 lines · Marc Zyngier

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