ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

PATCH Superseded Vladimir Murzin 2 participants 12 messages Dec 19, 2017 – Dec 21, 2017 Arm (32-bit)

Share & tools

Vladimir Murzin

author

2017-12-19 10:23 UTC #0 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

With switch to dynamic exception base address setting, VBAR/Hivecs set only for boot CPU, but secondaries stay unaware of that. That might lead to weird effects when trying up to bring up secondaries.

Fixes ad475117d201 ("ARM: 8649/2: nommu: remove Hivecs configuration is asm")

Signed-off-by Vladimir Murzin <[email protected]>

---
 arch/arm/include/asm/memory.h | 9 +++++++++
 arch/arm/kernel/smp.c         | 2 ++
 arch/arm/mm/nommu.c           | 4 ++--
 3 files changed, 13 insertions(+), 2 deletions(-)

arch/arm/include/asm/memory.h +9 -0

index 1f54e4e..65c52ac 100644
@@ -165,6 +165,15 @@ extern unsigned long vectors_base;
 
 #ifndef __ASSEMBLY__
 
+#ifndef CONFIG_MMU
+extern unsigned long setup_vectors_base(void);
+#else
+static inline unsigned long setup_vectors_base(void)
+{
+	return 0;
+}
+#endif
+
 /*
  * Physical vs virtual RAM address space conversion.  These are
  * private definitions which should NOT be used outside memory.h

arch/arm/kernel/smp.c +2 -0

index b4fbf00..248e33f 100644
@@ -379,6 +379,8 @@ asmlinkage void secondary_start_kernel(void)
 
 	cpu_init();
 
+	setup_vectors_base();
+
 	pr_debug("CPU%u: Booted secondary processor\n", cpu);
 
 	preempt_disable();

arch/arm/mm/nommu.c +2 -2

index 885b106..c8beaab 100644
@@ -31,7 +31,7 @@ struct mpu_rgn_info mpu_rgn_info;
 
 #ifdef CONFIG_CPU_CP15
 #ifdef CONFIG_CPU_HIGH_VECTOR
-static unsigned long __init setup_vectors_base(void)
+unsigned long setup_vectors_base(void)
 {
 	unsigned long reg = get_cr();

@@ -58,7 +58,7 @@ static inline bool security_extensions_enabled(void)
 	return 0;
 }
 
-static unsigned long __init setup_vectors_base(void)
+unsigned long setup_vectors_base(void)
 {
 	unsigned long base = 0, reg = get_cr();

Signature · 1 line

2.0.0

afzal mohammed

2017-12-19 11:29 UTC #1 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 5 lines · Vladimir Murzin

With switch to dynamic exception base address setting, VBAR/Hivecs set only for boot CPU, but secondaries stay unaware of that. That might lead to weird effects when trying up to bring up secondaries.
Fixes ad475117d201 ("ARM: 8649/2: nommu: remove Hivecs configuration is asm")

Sorry, it was my incompetence not seeing the secondary CPU's case.

Was the issue observed on Cortex-R ?, and was it occuring with CONFIG_CPU_HIGH_VECTOR enabled or disabled ?

Instead of,

Quoted 12 lines

arch/arm/include/asm/memory.h +8 -0

 
+#ifndef CONFIG_MMU
+extern unsigned long setup_vectors_base(void);
+#else
+static inline unsigned long setup_vectors_base(void)
+{
+	return 0;
+}
+#endif

arch/arm/kernel/smp.c asmlinkage void secondary_start_kernel(void) · line 382

Quoted 2 lines

arch/arm/kernel/smp.c +1 -0
 
+	setup_vectors_base();

how about,

        if (!IS_ENABLED(CONFIG_MMU))
                setup_vectors_base();

That would avoid #ifdef's. Also as w/ MMU, vector base is not setup (always Hivecs), this would make clear that setup_vectors_base() is non-existent on MMU on.

Thanks for the fix.

afzal

Vladimir Murzin

author

2017-12-19 14:44 UTC #2 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 7 lines · afzal mohammed

Hi,
Quoted 4 lines · Vladimir Murzin
With switch to dynamic exception base address setting, VBAR/Hivecs set only for boot CPU, but secondaries stay unaware of that. That might lead to weird effects when trying up to bring up secondaries.

Quoted 6 lines

Quoted 1 line
Fixes ad475117d201 ("ARM: 8649/2: nommu: remove Hivecs configuration is asm")
Sorry, it was my incompetence not seeing the secondary CPU's case.
Was the issue observed on Cortex-R ?, and was it occuring with CONFIG_CPU_HIGH_VECTOR enabled or disabled ?

I caught it when was trying to setup VBAR and after code inspection I noticed that setting of Hivecs were changed as well.

Quoted 20 lines

Instead of,
Quoted 12 lines
arch/arm/include/asm/memory.h +8 -0
 
+#ifndef CONFIG_MMU
+extern unsigned long setup_vectors_base(void);
+#else
+static inline unsigned long setup_vectors_base(void)
+{
+	return 0;
+}
+#endif
arch/arm/kernel/smp.c asmlinkage void secondary_start_kernel(void) · line 382
Quoted 2 lines
arch/arm/kernel/smp.c +1 -0
 
+	setup_vectors_base();
how about,

Quoted 6 lines

        if (!IS_ENABLED(CONFIG_MMU))
                setup_vectors_base();
That would avoid #ifdef's. Also as w/ MMU, vector base is not setup (always Hivecs), this would make clear that setup_vectors_base() is non-existent on MMU on.

Works for me, but I went with plain #ifndef.

Vladimir

Quoted 5 lines

Thanks for the fix.
afzal

afzal mohammed

2017-12-20 04:52 UTC #3 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 5 lines · Vladimir Murzin

Quoted 2 lines
Was the issue observed on Cortex-R ?, and was it occuring with CONFIG_CPU_HIGH_VECTOR enabled or disabled ?
I caught it when was trying to setup VBAR and after code inspection I noticed that setting of Hivecs were changed as well.

Thinking again about this, should the Hivecs setting on secondary CPU's be done (till a requirement comes) ?

ARM ARM deprecates using Hivecs setting on ARMv7-R, so this issue might not be hit in practice for R class. While pre-ARMv7, lack of Hivecs setting for secondaries, it seems can affect only ARMv6k (multi-processing support added here ?) and i am making a guess that even if there are ARMv6k with more than one core available, they might not yet have run with MMU disabled to hit this case, probably the reason no one has reported issue for long.

Perhaps, we can avoid configuring Hivecs for secondaries until some one needs it ?

afzal

Vladimir Murzin

author

2017-12-20 09:55 UTC #4 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 13 lines · afzal mohammed

Hi,
Quoted 5 lines · Vladimir Murzin
Quoted 2 lines
Was the issue observed on Cortex-R ?, and was it occuring with CONFIG_CPU_HIGH_VECTOR enabled or disabled ?
I caught it when was trying to setup VBAR and after code inspection I noticed that setting of Hivecs were changed as well.
Thinking again about this, should the Hivecs setting on secondary CPU's be done (till a requirement comes) ?

Quoted 7 lines

ARM ARM deprecates using Hivecs setting on ARMv7-R, so this issue might not be hit in practice for R class. While pre-ARMv7, lack of Hivecs setting for secondaries, it seems can affect only ARMv6k (multi-processing support added here ?) and i am making a guess that even if there are ARMv6k with more than one core available, they might not yet have run with MMU disabled to hit this case, probably the reason no one has reported issue for long.

I've just reported an issue, no? :)

Quoted 3 lines

Perhaps, we can avoid configuring Hivecs for secondaries until some one needs it ?

Well, before ad475117d201, Hivec would be enabled for secondaries via

secondary_startup

	-> __after_proc_init

after that commit it is not true, so it is kind of regression.

Additionally, patch is not about Hivec only, but VBAR as well and TBH I don't follow what is your proposal...

Cheers
Vladimir

Quoted 3 lines

afzal

afzal mohammed

2017-12-20 11:49 UTC #5 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 14 lines · Vladimir Murzin

Quoted 6 lines
Quoted 2 lines
I caught it when was trying to setup VBAR and after code inspection I noticed that setting of Hivecs were changed as well.
Thinking again about this, should the Hivecs setting on secondary CPU's be done (till a requirement comes) ?
Quoted 7 lines
ARM ARM deprecates using Hivecs setting on ARMv7-R, so this issue might not be hit in practice for R class. While pre-ARMv7, lack of Hivecs setting for secondaries, it seems can affect only ARMv6k (multi-processing support added here ?) and i am making a guess that even if there are ARMv6k with more than one core available, they might not yet have run with MMU disabled to hit this case, probably the reason no one has reported issue for long.

Quoted 1 line

I've just reported an issue, no? :)

By reported, i meant whether the lack of this in secondary would cause an issue at run time in any of the platform's. You spotted it by code inspection rather than hitting any issue in practice is what i understood.

Quoted 9 lines

Quoted 2 lines
Perhaps, we can avoid configuring Hivecs for secondaries until some one needs it ?
Well, before ad475117d201, Hivec would be enabled for secondaries via
secondary_startup
	-> __after_proc_init
after that commit it is not true, so it is kind of regression.

Something that was done before that commit not being done later (though unintentionally) per se doesn't count as regression in my opinion. But if any platform really needs to gets this done or misbehaves due to my change, then certainly it has to be counted a regression, which i believe is not the case here.

Quoted 3 lines

Additionally, patch is not about Hivec only, but VBAR as well and TBH I don't follow what is your proposal...

i was referring to the fact that vector remapping can't be done in Cortex-R, as security extension is a requisite for this feature, which Cortex-R don't have on ARMv7.

afzal

afzal mohammed

2017-12-20 12:01 UTC #6 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi Vladimir,

To add, i am not against your patch, just seeing if we can avoid having this change, lesser code ... lesser bugs.

afzal

Vladimir Murzin

author

2017-12-20 12:22 UTC #7 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 4 lines · afzal mohammed

Hi Vladimir,
To add, i am not against your patch, just seeing if we can avoid having this change, lesser code ... lesser bugs.

Look, I hit that issue and without this change the issue does not magically disappear for me - the only way to avoid this change is to propose alternative patch.

I don't want to jump into these "it should not happen" or "nobody" cares" discussions just because it is not true - it happened to me and I do care. However, I do open to discuss technical aspects.

Cheers
Vladimir

Quoted 3 lines

afzal

afzal mohammed

2017-12-21 04:05 UTC #8 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi Vladimir,

Quoted 10 lines · Vladimir Murzin

Quoted 2 lines
To add, i am not against your patch, just seeing if we can avoid having this change, lesser code ... lesser bugs.
Look, I hit that issue and without this change the issue does not magically disappear for me - the only way to avoid this change is to propose alternative patch.
I don't want to jump into these "it should not happen" or "nobody" cares" discussions just because it is not true - it happened to me and I do care. However, I do open to discuss technical aspects.

Okay sir, my reading of your other mail was that you caught this issue by code inspection and not by hitting the issue in practice, seems there was a communication gap. Now i am deducing from your mails you actually have hit some issue because of my change, then your fix is absolutely required, no question about it.

i was trying to find out the specific scenario where the issue would be be hit because of my change. But since you are able to fix the issue in your specific scenario w/ with this fix, no more questions from my side. Some of my comments are made jokingly to keep the spirits high, but at times it is being misread it seems.

afzal

afzal mohammed

2017-12-20 12:12 UTC #9 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

In reply to afzal mohammed

Hi,

Quoted 2 lines · afzal mohammed

Cortex-R, as security extension is a requisite for this feature, which Cortex-R don't have on ARMv7.

Okay seems even ARMv8-R doesn't have security extensions, only ARMv8-M has it as compared to ARMv7's.

afzal

Vladimir Murzin

author

2017-12-20 12:22 UTC #10 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

In reply to afzal mohammed

Quoted 42 lines · afzal mohammed

Hi,
Quoted 14 lines · Vladimir Murzin
Quoted 6 lines
Quoted 2 lines
I caught it when was trying to setup VBAR and after code inspection I noticed that setting of Hivecs were changed as well.
Thinking again about this, should the Hivecs setting on secondary CPU's be done (till a requirement comes) ?
Quoted 7 lines
ARM ARM deprecates using Hivecs setting on ARMv7-R, so this issue might not be hit in practice for R class. While pre-ARMv7, lack of Hivecs setting for secondaries, it seems can affect only ARMv6k (multi-processing support added here ?) and i am making a guess that even if there are ARMv6k with more than one core available, they might not yet have run with MMU disabled to hit this case, probably the reason no one has reported issue for long.
Quoted 1 line
I've just reported an issue, no? :)
By reported, i meant whether the lack of this in secondary would cause an issue at run time in any of the platform's. You spotted it by code inspection rather than hitting any issue in practice is what i understood.
Quoted 9 lines
Quoted 2 lines
Perhaps, we can avoid configuring Hivecs for secondaries until some one needs it ?
Well, before ad475117d201, Hivec would be enabled for secondaries via
secondary_startup
	-> __after_proc_init
after that commit it is not true, so it is kind of regression.
Something that was done before that commit not being done later (though unintentionally) per se doesn't count as regression in my opinion. But if any platform really needs to gets this done or misbehaves due to my change, then certainly it has to be counted a regression, which i believe is not the case here.
Quoted 1 line

Quoted 6 lines

Quoted 2 lines
Additionally, patch is not about Hivec only, but VBAR as well and TBH I don't follow what is your proposal...
i was referring to the fact that vector remapping can't be done in Cortex-R, as security extension is a requisite for this feature, which Cortex-R don't have on ARMv7.

For instance, just think of ARMv7A with 1:1 MMU running in SMP...

Vladimir

Quoted 3 lines

afzal

afzal mohammed

2017-12-21 03:44 UTC #11 lore raw

PATCH 1/1 ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

Hi,

Quoted 8 lines · Vladimir Murzin

Quoted 6 lines
Quoted 2 lines
Additionally, patch is not about Hivec only, but VBAR as well and TBH I don't follow what is your proposal...
i was referring to the fact that vector remapping can't be done in Cortex-R, as security extension is a requisite for this feature, which Cortex-R don't have on ARMv7.
For instance, just think of ARMv7A with 1:1 MMU running in SMP...

Hmm, 3 things,
1. MMU on would not take the path being patched here
2. w/ MMU on, currently it is always Hivecs
3. w/ MMU on & 1:1 mapping & due to pt. 2 above, if there is no memory @0xFFFF0000, suspect things might go wrong.

afzal