| Message ID | 20240212144736.1933112-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM64: Fixes for FEAT_E2H0 handling | expand |
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
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))); }
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))); > }
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
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
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.
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)
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(-)