[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[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")
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
 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(-)

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 1f54e4e..65c52ac 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -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
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index b4fbf00..248e33f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -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();
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index 885b106..c8beaab 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -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();
 
-- 
2.0.0

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[afzal mohammed]

Hi,

On Tue, Dec 19, 2017 at 10:23:49AM +0000, Vladimir Murzin wrote:
> 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,

> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h

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

> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c

> +	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

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[Vladimir Murzin]

Hi,

On 19/12/17 11:29, afzal mohammed wrote:
> Hi,
> 
> On Tue, Dec 19, 2017 at 10:23:49AM +0000, Vladimir Murzin wrote:
>> 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 ?

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

> 
> Instead of,
> 
>> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> 
>> +#ifndef CONFIG_MMU
>> +extern unsigned long setup_vectors_base(void);
>> +#else
>> +static inline unsigned long setup_vectors_base(void)
>> +{
>> +	return 0;
>> +}
>> +#endif
> 
>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> 
>> +	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.

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

Vladimir

> 
> Thanks for the fix.
> 
> afzal
> 

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[afzal mohammed]

Hi,

On Tue, Dec 19, 2017 at 02:44:01PM +0000, Vladimir Murzin wrote:

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

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[Vladimir Murzin]

Hi,

On 20/12/17 04:52, afzal mohammed wrote:
> Hi,
> 
> On Tue, Dec 19, 2017 at 02:44:01PM +0000, Vladimir Murzin wrote:
> 
>>> 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.

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

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

> 
> afzal
> 

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[afzal mohammed]

Hi,

On Wed, Dec 20, 2017 at 09:55:00AM +0000, Vladimir Murzin wrote:

> >> 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.
> 
> 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.

> > 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.

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

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[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.

afzal

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[Vladimir Murzin]

Hi,

On 20/12/17 12:01, afzal mohammed wrote:
> 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

> 
> afzal
> 

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[afzal mohammed]

Hi Vladimir,

On Wed, Dec 20, 2017 at 12:22:38PM +0000, Vladimir Murzin wrote:

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

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[afzal mohammed]

Hi,

On Wed, Dec 20, 2017 at 05:19:24PM +0530, afzal mohammed wrote:

> 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

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[Vladimir Murzin]

On 20/12/17 11:49, afzal mohammed wrote:
> Hi,
> 
> On Wed, Dec 20, 2017 at 09:55:00AM +0000, Vladimir Murzin wrote:
> 
>>>> 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.
>>
>> 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.
> 
>>> 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.
> 
>>
>> 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

> 
> afzal
> 

[PATCH] ARM: NOMMU: Setup VBAR/Hivecs for secondaries cores

[afzal mohammed]

Hi,

On Wed, Dec 20, 2017 at 12:22:16PM +0000, Vladimir Murzin wrote:

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