[PATCH] ARM: Enter CPU in ARM state for cpu_resume

[PATCH] ARM: Enter CPU in ARM state for cpu_resume

[Stephen Boyd]

The standard boot protocol on ARM requires CPUs to be entered in
the ARM state, unless they don't support the ARM instruction set
(see Documentation/arm/Booting). On THUMB2 kernels, we assume
the firmware can determine what state to enter the kernel in, but
some firmwares don't honor the thumb bit. Make the cpu_resume
symbol an ARM symbol, so that firmwares that honor the thumb bit
will enter the kernel in ARM state and firmwares that don't honor
the thumb bit will be able to enter the kernel in ARM state
without more changes.

This fixes a problem reported by Kevin Hilman where the ifc6410 
fails to boot for THUMB2 kernels because the platform's firmware
always enters the kernel in ARM mode from deep idle states.

Reported-by: Kevin Hilman <khilman@linaro.org>
Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 arch/arm/kernel/sleep.S | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/kernel/sleep.S b/arch/arm/kernel/sleep.S
index 76bb3128e135..7c289ebb0b45 100644
--- a/arch/arm/kernel/sleep.S
+++ b/arch/arm/kernel/sleep.S
@@ -118,7 +118,12 @@ ENDPROC(cpu_resume_after_mmu)
 
 	.text
 	.align
+	.arm
 ENTRY(cpu_resume)
+ THUMB(	badr	r9, 1f		)	@ Kernel is entered in ARM.
+ THUMB(	bx	r9		)	@ If this is a Thumb-2 kernel,
+ THUMB(	.thumb			)	@ switch to Thumb now.
+ THUMB(1:			)
 ARM_BE8(setend be)			@ ensure we are in BE mode
 #ifdef CONFIG_ARM_VIRT_EXT
 	bl	__hyp_stub_install_secondary
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] ARM: Enter CPU in ARM state for cpu_resume

[Russell King - ARM Linux]

On Mon, Jun 01, 2015 at 01:22:00PM -0700, Stephen Boyd wrote:
> The standard boot protocol on ARM requires CPUs to be entered in
> the ARM state, unless they don't support the ARM instruction set
> (see Documentation/arm/Booting). On THUMB2 kernels, we assume
> the firmware can determine what state to enter the kernel in, but
> some firmwares don't honor the thumb bit. Make the cpu_resume
> symbol an ARM symbol, so that firmwares that honor the thumb bit
> will enter the kernel in ARM state and firmwares that don't honor
> the thumb bit will be able to enter the kernel in ARM state
> without more changes.
> 
> This fixes a problem reported by Kevin Hilman where the ifc6410 
> fails to boot for THUMB2 kernels because the platform's firmware
> always enters the kernel in ARM mode from deep idle states.

Please do this differently.  The default should be (as we do with
the SMP secondary entry path) to assume that the firmware does the
right thing.

So, if we want an ARM-mode entry point, please use:

+		.arm
+ENTRY(cpu_resume_arm)
+ THUMB(	badr	r9, 1f		)	@ Kernel is entered in ARM.
+ THUMB(	bx	r9		)	@ If this is a Thumb-2 kernel,
+ THUMB(	.thumb			)	@ switch to Thumb now.
+ THUMB(1:			)

Don't forget an ENDPROC() for the new symbol.  Buggy platforms then
use cpu_resume_arm instead of cpu_resume.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: Enter CPU in ARM state for cpu_resume

[Ard Biesheuvel]

On 1 June 2015 at 23:45, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 01, 2015 at 01:22:00PM -0700, Stephen Boyd wrote:
>> The standard boot protocol on ARM requires CPUs to be entered in
>> the ARM state, unless they don't support the ARM instruction set
>> (see Documentation/arm/Booting). On THUMB2 kernels, we assume
>> the firmware can determine what state to enter the kernel in, but
>> some firmwares don't honor the thumb bit. Make the cpu_resume
>> symbol an ARM symbol, so that firmwares that honor the thumb bit
>> will enter the kernel in ARM state and firmwares that don't honor
>> the thumb bit will be able to enter the kernel in ARM state
>> without more changes.
>>
>> This fixes a problem reported by Kevin Hilman where the ifc6410
>> fails to boot for THUMB2 kernels because the platform's firmware
>> always enters the kernel in ARM mode from deep idle states.
>
> Please do this differently.  The default should be (as we do with
> the SMP secondary entry path) to assume that the firmware does the
> right thing.
>
> So, if we want an ARM-mode entry point, please use:
>
> +               .arm
> +ENTRY(cpu_resume_arm)
> + THUMB(        badr    r9, 1f          )       @ Kernel is entered in ARM.
> + THUMB(        bx      r9              )       @ If this is a Thumb-2 kernel,
> + THUMB(        .thumb                  )       @ switch to Thumb now.
> + THUMB(1:                      )
>
> Don't forget an ENDPROC() for the new symbol.  Buggy platforms then
> use cpu_resume_arm instead of cpu_resume.
>

OK, I think that was Stephen intention at first, but I suggested this instead.

The point is that it is safer and more tidy to make these entry points
ARM only throughout, and switch to Thumb2 only if THUMB2_KERNEL. This
way, since all firmwares (except ARMv7-M, but let's disregard that for
now) are known to be able to enter/resume into the kernel in ARM mode,
this is more robust in the face of new platforms and firmware
revisions of existing platforms.

Re: [PATCH] ARM: Enter CPU in ARM state for cpu_resume

[Russell King - ARM Linux]

On Tue, Jun 02, 2015 at 08:18:18AM +0200, Ard Biesheuvel wrote:
> On 1 June 2015 at 23:45, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Please do this differently.  The default should be (as we do with
> > the SMP secondary entry path) to assume that the firmware does the
> > right thing.
> >
> > So, if we want an ARM-mode entry point, please use:
> >
> > +               .arm
> > +ENTRY(cpu_resume_arm)
> > + THUMB(        badr    r9, 1f          )       @ Kernel is entered in ARM.
> > + THUMB(        bx      r9              )       @ If this is a Thumb-2 kernel,
> > + THUMB(        .thumb                  )       @ switch to Thumb now.
> > + THUMB(1:                      )
> >
> > Don't forget an ENDPROC() for the new symbol.  Buggy platforms then
> > use cpu_resume_arm instead of cpu_resume.
> >
> 
> OK, I think that was Stephen intention at first, but I suggested this instead.
> 
> The point is that it is safer and more tidy to make these entry points
> ARM only throughout, and switch to Thumb2 only if THUMB2_KERNEL. This
> way, since all firmwares (except ARMv7-M, but let's disregard that for
> now) are known to be able to enter/resume into the kernel in ARM mode,
> this is more robust in the face of new platforms and firmware
> revisions of existing platforms.

Stop creating random interfaces with differing ways to call them.
Consistency is one of the important things.

We have three interfaces to the kernel:

- Boot
- Secondary CPU
- Resume

Out of those three, boot is the special one, as we have no way to
communicate what mode it is required - so we specify a mode, which
is ARM mode, except for CPUs which have no ARM mode support.

Secondary CPU is not defined in the booting document because it's
not relevant, and it's not relevant for the same reason that the
resume entry point isn't.  These entry points are not at a fixed
location in the kernel image, and the kernel has to communicate that,
along with their entry mode to the firmware.

Therefore, firmware _does_ have the information to discover whether
the address should be called in ARM or Thumb mode, and what's more,
it should just work on those platforms which have no ARM mode support.

If we start forcing these interfaces to be ARM mode only, we then need
the same work-arounds for them as for the boot interface.

In other words, boot has a _valid_ reason to be different, the other
two are very similar in how they should be called.

Besides, I'm not pandering to broken firmware.  We should do the right
thing by default, which is to have sane interfaces, and only work around
stuff where needed.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: Enter CPU in ARM state for cpu_resume

[Ard Biesheuvel]

On 2 June 2015 at 10:34, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jun 02, 2015 at 08:18:18AM +0200, Ard Biesheuvel wrote:
>> On 1 June 2015 at 23:45, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > Please do this differently.  The default should be (as we do with
>> > the SMP secondary entry path) to assume that the firmware does the
>> > right thing.
>> >
>> > So, if we want an ARM-mode entry point, please use:
>> >
>> > +               .arm
>> > +ENTRY(cpu_resume_arm)
>> > + THUMB(        badr    r9, 1f          )       @ Kernel is entered in ARM.
>> > + THUMB(        bx      r9              )       @ If this is a Thumb-2 kernel,
>> > + THUMB(        .thumb                  )       @ switch to Thumb now.
>> > + THUMB(1:                      )
>> >
>> > Don't forget an ENDPROC() for the new symbol.  Buggy platforms then
>> > use cpu_resume_arm instead of cpu_resume.
>> >
>>
>> OK, I think that was Stephen intention at first, but I suggested this instead.
>>
>> The point is that it is safer and more tidy to make these entry points
>> ARM only throughout, and switch to Thumb2 only if THUMB2_KERNEL. This
>> way, since all firmwares (except ARMv7-M, but let's disregard that for
>> now) are known to be able to enter/resume into the kernel in ARM mode,
>> this is more robust in the face of new platforms and firmware
>> revisions of existing platforms.
>
> Stop creating random interfaces with differing ways to call them.
> Consistency is one of the important things.
>

Consistency was actually the point of my suggestion, but I see how the
pre-existence of secondary_startup_arm() and your point regarding
treating broken firmware as the exception may lead to the opposite
conclusion.


> We have three interfaces to the kernel:
>
> - Boot
> - Secondary CPU
> - Resume
>
> Out of those three, boot is the special one, as we have no way to
> communicate what mode it is required - so we specify a mode, which
> is ARM mode, except for CPUs which have no ARM mode support.
>
> Secondary CPU is not defined in the booting document because it's
> not relevant, and it's not relevant for the same reason that the
> resume entry point isn't.  These entry points are not at a fixed
> location in the kernel image, and the kernel has to communicate that,
> along with their entry mode to the firmware.
>
> Therefore, firmware _does_ have the information to discover whether
> the address should be called in ARM or Thumb mode, and what's more,
> it should just work on those platforms which have no ARM mode support.
>
> If we start forcing these interfaces to be ARM mode only, we then need
> the same work-arounds for them as for the boot interface.
>
> In other words, boot has a _valid_ reason to be different, the other
> two are very similar in how they should be called.
>
> Besides, I'm not pandering to broken firmware.  We should do the right
> thing by default, which is to have sane interfaces, and only work around
> stuff where needed.
>
> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.