* [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S @ 2011-09-07 16:00 Dave Martin 2011-09-12 16:52 ` Dave Martin 0 siblings, 1 reply; 5+ messages in thread From: Dave Martin @ 2011-09-07 16:00 UTC (permalink / raw) To: linux-arm-kernel At secondary_startup, the MSR CPSR_cxsf, #0xd3 is not compatible with Thumb-2 and also unmasks asynchronous aborts (CPSR.A bit forced to zero -- this is probably unintentional). Any remotely sane bootloader should be putting each secondary CPU in the appropriate state _before_ entering the kernel anyway. Otherwise, disabling interrupts on entry to the kernel isn't going to fix it. Therefore this patch just removes the MSR instruction. Signed-off-by: Dave Martin <dave.martin@linaro•org> --- I make assumptions about the bootloader in this patch. If someone with Tegra knowledge can please comment and/or test, that would be much appreciated, thanks. arch/arm/mach-tegra/headsmp.S | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/arch/arm/mach-tegra/headsmp.S b/arch/arm/mach-tegra/headsmp.S index b5349b2..6ec4790 100644 --- a/arch/arm/mach-tegra/headsmp.S +++ b/arch/arm/mach-tegra/headsmp.S @@ -48,7 +48,6 @@ ENTRY(v7_invalidate_l1) ENDPROC(v7_invalidate_l1) ENTRY(tegra_secondary_startup) - msr cpsr_fsxc, #0xd3 bl v7_invalidate_l1 mrc p15, 0, r0, c0, c0, 5 and r0, r0, #15 -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S 2011-09-07 16:00 [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S Dave Martin @ 2011-09-12 16:52 ` Dave Martin 2011-09-14 1:46 ` Colin Cross 0 siblings, 1 reply; 5+ messages in thread From: Dave Martin @ 2011-09-12 16:52 UTC (permalink / raw) To: linux-arm-kernel Colin, can you comment on this? On Wed, Sep 07, 2011 at 05:00:53PM +0100, Dave Martin wrote: > At secondary_startup, the MSR CPSR_cxsf, #0xd3 is not compatible > with Thumb-2 and also unmasks asynchronous aborts (CPSR.A bit > forced to zero -- this is probably unintentional). > > Any remotely sane bootloader should be putting each secondary CPU > in the appropriate state _before_ entering the kernel anyway. > Otherwise, disabling interrupts on entry to the kernel isn't going > to fix it. > > Therefore this patch just removes the MSR instruction. > > Signed-off-by: Dave Martin <dave.martin@linaro•org> > --- > > I make assumptions about the bootloader in this patch. If someone with > Tegra knowledge can please comment and/or test, that would be much > appreciated, thanks. > > arch/arm/mach-tegra/headsmp.S | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-tegra/headsmp.S b/arch/arm/mach-tegra/headsmp.S > index b5349b2..6ec4790 100644 > --- a/arch/arm/mach-tegra/headsmp.S > +++ b/arch/arm/mach-tegra/headsmp.S > @@ -48,7 +48,6 @@ ENTRY(v7_invalidate_l1) > ENDPROC(v7_invalidate_l1) > > ENTRY(tegra_secondary_startup) > - msr cpsr_fsxc, #0xd3 > bl v7_invalidate_l1 > mrc p15, 0, r0, c0, c0, 5 > and r0, r0, #15 My rationale here is that the CPU boots straight into the correct mode -- if there is any boot code before we get here, then it should already have established the correct mode, otherwise we're likely to get problems which "MSR" by itself isn't going to fix -- such as stray interrupts for example. If that feels unsafe however, we should still to be able to write cpsid aif, #SVC_MODE (which is the compact v6/v7-compatible way to set all the interrupt mask bits and get into a specific mode) Cheers ---Dave ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S 2011-09-12 16:52 ` Dave Martin @ 2011-09-14 1:46 ` Colin Cross 2011-09-14 9:15 ` Dave Martin 0 siblings, 1 reply; 5+ messages in thread From: Colin Cross @ 2011-09-14 1:46 UTC (permalink / raw) To: linux-arm-kernel On Mon, Sep 12, 2011 at 9:52 AM, Dave Martin <dave.martin@linaro•org> wrote: > Colin, can you comment on this? > > On Wed, Sep 07, 2011 at 05:00:53PM +0100, Dave Martin wrote: >> At secondary_startup, the MSR CPSR_cxsf, #0xd3 is not compatible >> with Thumb-2 and also unmasks asynchronous aborts (CPSR.A bit >> forced to zero -- this is probably unintentional). >> >> Any remotely sane bootloader should be putting each secondary CPU >> in the appropriate state _before_ entering the kernel anyway. >> Otherwise, disabling interrupts on entry to the kernel isn't going >> to fix it. >> >> Therefore this patch just removes the MSR instruction. >> >> Signed-off-by: Dave Martin <dave.martin@linaro•org> >> --- >> >> I make assumptions about the bootloader in this patch. ?If someone with >> Tegra knowledge can please comment and/or test, that would be much >> appreciated, thanks. >> >> ?arch/arm/mach-tegra/headsmp.S | ? ?1 - >> ?1 files changed, 0 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/mach-tegra/headsmp.S b/arch/arm/mach-tegra/headsmp.S >> index b5349b2..6ec4790 100644 >> --- a/arch/arm/mach-tegra/headsmp.S >> +++ b/arch/arm/mach-tegra/headsmp.S >> @@ -48,7 +48,6 @@ ENTRY(v7_invalidate_l1) >> ?ENDPROC(v7_invalidate_l1) >> >> ?ENTRY(tegra_secondary_startup) >> - ? ? msr ? ? cpsr_fsxc, #0xd3 >> ? ? ? ? ?bl ? ? ?v7_invalidate_l1 >> ? ? ? mrc ? ? p15, 0, r0, c0, c0, 5 >> ? ? ? ? ?and ?r0, r0, #15 > > My rationale here is that the CPU boots straight into the > correct mode -- if there is any boot code before we get here, > then it should already have established the correct mode, otherwise > we're likely to get problems which "MSR" by itself isn't going to > fix -- such as stray interrupts for example. > > If that feels unsafe however, we should still to be able to write > > ? ? ? ?cpsid ? aif, #SVC_MODE > > (which is the compact v6/v7-compatible way to set all the interrupt > mask bits and get into a specific mode) Tegra boots with one cpu enabled, and the other in reset. The cpu needs to be initialized from scratch by the kernel. Does cpsid aif, #SVC_MODE compile to the same instruction as the msr instruction? ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S 2011-09-14 1:46 ` Colin Cross @ 2011-09-14 9:15 ` Dave Martin 2011-09-21 16:14 ` Colin Cross 0 siblings, 1 reply; 5+ messages in thread From: Dave Martin @ 2011-09-14 9:15 UTC (permalink / raw) To: linux-arm-kernel On Tue, Sep 13, 2011 at 06:46:48PM -0700, Colin Cross wrote: > On Mon, Sep 12, 2011 at 9:52 AM, Dave Martin <dave.martin@linaro•org> wrote: > > Colin, can you comment on this? > > > > On Wed, Sep 07, 2011 at 05:00:53PM +0100, Dave Martin wrote: > >> At secondary_startup, the MSR CPSR_cxsf, #0xd3 is not compatible > >> with Thumb-2 and also unmasks asynchronous aborts (CPSR.A bit > >> forced to zero -- this is probably unintentional). > >> > >> Any remotely sane bootloader should be putting each secondary CPU > >> in the appropriate state _before_ entering the kernel anyway. > >> Otherwise, disabling interrupts on entry to the kernel isn't going > >> to fix it. > >> > >> Therefore this patch just removes the MSR instruction. > >> > >> Signed-off-by: Dave Martin <dave.martin@linaro•org> > >> --- > >> > >> I make assumptions about the bootloader in this patch. ?If someone with > >> Tegra knowledge can please comment and/or test, that would be much > >> appreciated, thanks. > >> > >> ?arch/arm/mach-tegra/headsmp.S | ? ?1 - > >> ?1 files changed, 0 insertions(+), 1 deletions(-) > >> > >> diff --git a/arch/arm/mach-tegra/headsmp.S b/arch/arm/mach-tegra/headsmp.S > >> index b5349b2..6ec4790 100644 > >> --- a/arch/arm/mach-tegra/headsmp.S > >> +++ b/arch/arm/mach-tegra/headsmp.S > >> @@ -48,7 +48,6 @@ ENTRY(v7_invalidate_l1) > >> ?ENDPROC(v7_invalidate_l1) > >> > >> ?ENTRY(tegra_secondary_startup) > >> - ? ? msr ? ? cpsr_fsxc, #0xd3 > >> ? ? ? ? ?bl ? ? ?v7_invalidate_l1 > >> ? ? ? mrc ? ? p15, 0, r0, c0, c0, 5 > >> ? ? ? ? ?and ?r0, r0, #15 > > > > My rationale here is that the CPU boots straight into the > > correct mode -- if there is any boot code before we get here, > > then it should already have established the correct mode, otherwise > > we're likely to get problems which "MSR" by itself isn't going to > > fix -- such as stray interrupts for example. > > > > If that feels unsafe however, we should still to be able to write > > > > ? ? ? ?cpsid ? aif, #SVC_MODE > > > > (which is the compact v6/v7-compatible way to set all the interrupt > > mask bits and get into a specific mode) > > Tegra boots with one cpu enabled, and the other in reset. The cpu > needs to be initialized from scratch by the kernel. Does cpsid aif, > #SVC_MODE compile to the same instruction as the msr instruction? CPUs power on in SVC mode with all interrupts masked, so it should be safe to assume that the CPU is already in this state (this is actually _required_ by Documentation/arm/Booting). If the CPU booted with a random mode and random interrupt masks, no system could boot reliably... However, it's harmless to set the state explicitly if we want to be extra safe. My assumption is that the unmasking of asynchronous aborts done by msr CPSR_cxsf, #0xd3 is unintentional -- i.e., this is a copy-paste of code which would have been correct prior to ARMv6 (when there was no separate 'A' interrupt class). cps is a distinct instruction, introduced in ARMv6. The advantages are that it can be used to control the A bit, and doesn't clobber the T bit (or other CPSR bits). Since mach-tegra doesn't apply to any platform older than ARMv6, use of this instruction seems reasonable. cpsid aif disables all three kinds of interrupt. With regard to Thumb-2 kernels, what code is responsible for performing the jump to tegra_secondary_startup? If the jump is done by making an exception return from some firmware, you may find you hit tegra_secondary_starup in ARM state, and everything will go horribly wrong... It could be worth making the secondary kernel entry point ARM code unconditionally (just as is now done for the primary kernel entry point). Cheers ---Dave ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S 2011-09-14 9:15 ` Dave Martin @ 2011-09-21 16:14 ` Colin Cross 0 siblings, 0 replies; 5+ messages in thread From: Colin Cross @ 2011-09-21 16:14 UTC (permalink / raw) To: linux-arm-kernel On Wed, Sep 14, 2011 at 2:15 AM, Dave Martin <dave.martin@linaro•org> wrote: <snip> > With regard to Thumb-2 kernels, what code is responsible for > performing the jump to tegra_secondary_startup? arch/arm/mach-tegra/platsmp.c puts the physical address of tegra_secondary_startup into the reset vector, and then brings the cpu out of reset. As far as I know, it is always in ARM mode, but it is possible that it comes through ROM code that ends with a bx, allowing it to be entered in Thumb mode. > If the jump is done by making an exception return from some firmware, > you may find you hit tegra_secondary_starup in ARM state, and > everything will go horribly wrong... > > It could be worth making the secondary kernel entry point ARM code > unconditionally (just as is now done for the primary kernel entry > point). That seems safest. > Cheers > ---Dave > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-09-21 16:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-09-07 16:00 [PATCH] ARM: tegra: Remove redundant change to the CPSR in headsmp.S Dave Martin 2011-09-12 16:52 ` Dave Martin 2011-09-14 1:46 ` Colin Cross 2011-09-14 9:15 ` Dave Martin 2011-09-21 16:14 ` Colin Cross
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox