| Message ID | 1513679029-5915-1-git-send-email-vladimir.murzin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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 >
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
Hi Vladimir, To add, i am not against your patch, just seeing if we can avoid having this change, lesser code ... lesser bugs. afzal
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
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 >
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 >
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
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
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();
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(-)