[PATCH 0/3] ARM/PSCI: Fix THUMB2_KERNEL entry points

[PATCH 0/3] ARM/PSCI: Fix THUMB2_KERNEL entry points

[Florian Fainelli]

Hi all,

While playing with THUMB2_KERNEL on ARCH_BRCMSTB, several issues came up
which are addressed by these 3 patches.

The THUMB() assembler macro is a no-op unless CONFIG_THUMB2_KERNEL so
using it unconditionally for CONFIG_ARM should not cause a problem
AFAICT.

Those patches can all be independently picked up by their respective
maintainers and don't depent on one another.

Thank you!

Florian Fainelli (3):
  firmware/psci: Fix cpu_resume entry points with THUMB2_KERNEL
  ARM: psci: Fix secondary core boot with THUMB2_KERNEL
  soc: bcm: brcmstb: Fix re-entry point with a THUMB2_KERNEL

 arch/arm/kernel/psci_smp.c          |  4 ++--
 drivers/firmware/psci.c             | 18 +++++++++++++++---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c |  2 +-
 3 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/3] firmware/psci: Fix cpu_resume entry points with THUMB2_KERNEL

[Florian Fainelli]

When THUMB2_KERNEL is enabled, we would be failing to resume from an
idle or system suspend call where the reentry point is set to
cpu_resume() because that function is in Thumb2. Utilize
cpu_resume_arm() for ARM 32-bit kernels which takes care of the mode
switching for us.

Fixes: 8b6f2499ac45 ("ARM: 8511/1: ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware")
Fixes: faf7ec4a92c0 ("drivers: firmware: psci: add system suspend support")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/firmware/psci.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index c80ec1d03274..f8b154384483 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -400,9 +400,14 @@ int psci_cpu_init_idle(unsigned int cpu)
 static int psci_suspend_finisher(unsigned long index)
 {
 	u32 *state = __this_cpu_read(psci_power_state);
+	unsigned long reentry;
 
-	return psci_ops.cpu_suspend(state[index - 1],
-				    __pa_symbol(cpu_resume));
+#ifdef CONFIG_ARM
+	reentry = __pa_symbol(cpu_resume_arm);
+#else
+	reentry = __pa_symbol(cpu_resume);
+#endif
+	return psci_ops.cpu_suspend(state[index - 1], reentry);
 }
 
 int psci_cpu_suspend_enter(unsigned long index)
@@ -437,8 +442,15 @@ CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops);
 
 static int psci_system_suspend(unsigned long unused)
 {
+	unsigned long reentry;
+
+#ifdef CONFIG_ARM
+	reentry = __pa_symbol(cpu_resume_arm);
+#else
+	reentry = __pa_symbol(cpu_resume);
+#endif
 	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
-			      __pa_symbol(cpu_resume), 0, 0);
+			      reentry, 0, 0);
 }
 
 static int psci_system_suspend_enter(suspend_state_t state)
-- 
2.17.1

[PATCH 1/3] firmware/psci: Fix cpu_resume entry points with THUMB2_KERNEL

[Mark Rutland]

On Thu, Sep 27, 2018 at 12:27:09PM -0700, Florian Fainelli wrote:
> When THUMB2_KERNEL is enabled, we would be failing to resume from an
> idle or system suspend call where the reentry point is set to
> cpu_resume() because that function is in Thumb2. Utilize
> cpu_resume_arm() for ARM 32-bit kernels which takes care of the mode
> switching for us.

Looking at the PSCI spec, if bit[0] of the entry point address is set,
the CPU should be placed into thumb state.

So either there's a FW bug here, or perhaps we're stripping bit[0] when
we do the __pa_symbol() dance.

Which firmware have you seen this with?

Thanks,
Mark.

> Fixes: 8b6f2499ac45 ("ARM: 8511/1: ARM64: kernel: PSCI: move PSCI idle management code to drivers/firmware")
> Fixes: faf7ec4a92c0 ("drivers: firmware: psci: add system suspend support")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/firmware/psci.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index c80ec1d03274..f8b154384483 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -400,9 +400,14 @@ int psci_cpu_init_idle(unsigned int cpu)
>  static int psci_suspend_finisher(unsigned long index)
>  {
>  	u32 *state = __this_cpu_read(psci_power_state);
> +	unsigned long reentry;
>  
> -	return psci_ops.cpu_suspend(state[index - 1],
> -				    __pa_symbol(cpu_resume));
> +#ifdef CONFIG_ARM
> +	reentry = __pa_symbol(cpu_resume_arm);
> +#else
> +	reentry = __pa_symbol(cpu_resume);
> +#endif
> +	return psci_ops.cpu_suspend(state[index - 1], reentry);
>  }
>  
>  int psci_cpu_suspend_enter(unsigned long index)
> @@ -437,8 +442,15 @@ CPUIDLE_METHOD_OF_DECLARE(psci, "psci", &psci_cpuidle_ops);
>  
>  static int psci_system_suspend(unsigned long unused)
>  {
> +	unsigned long reentry;
> +
> +#ifdef CONFIG_ARM
> +	reentry = __pa_symbol(cpu_resume_arm);
> +#else
> +	reentry = __pa_symbol(cpu_resume);
> +#endif
>  	return invoke_psci_fn(PSCI_FN_NATIVE(1_0, SYSTEM_SUSPEND),
> -			      __pa_symbol(cpu_resume), 0, 0);
> +			      reentry, 0, 0);
>  }
>  
>  static int psci_system_suspend_enter(suspend_state_t state)
> -- 
> 2.17.1
> 

[PATCH 1/3] firmware/psci: Fix cpu_resume entry points with THUMB2_KERNEL

[Florian Fainelli]

On 09/28/2018 06:47 AM, Mark Rutland wrote:
> On Thu, Sep 27, 2018 at 12:27:09PM -0700, Florian Fainelli wrote:
>> When THUMB2_KERNEL is enabled, we would be failing to resume from an
>> idle or system suspend call where the reentry point is set to
>> cpu_resume() because that function is in Thumb2. Utilize
>> cpu_resume_arm() for ARM 32-bit kernels which takes care of the mode
>> switching for us.
> 
> Looking at the PSCI spec, if bit[0] of the entry point address is set,
> the CPU should be placed into thumb state.
> 
> So either there's a FW bug here, or perhaps we're stripping bit[0] when
> we do the __pa_symbol() dance.
> 
> Which firmware have you seen this with?

This is a custom implementation use with ARCH_BRCMSTB, the same way I
managed to miss it during code review, I missed it again here, sorry
about that and thanks for pointing me in the right direction.
-- 
Florian

[PATCH 2/3] ARM: psci: Fix secondary core boot with THUMB2_KERNEL

[Florian Fainelli]

When THUMB2_KERNEL is enabled, we would be setting the secondary core's
entry point to secondary_startup() which is already Thumb2 code, utilize
secondary_startup_arm() which takes care of doing the mode switching for
us.

Fixes: 05774088391c ("arm: introduce psci_smp_ops")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/kernel/psci_smp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
index cb3fcaeb2233..547a11b065d2 100644
--- a/arch/arm/kernel/psci_smp.c
+++ b/arch/arm/kernel/psci_smp.c
@@ -47,13 +47,13 @@
  *
  */
 
-extern void secondary_startup(void);
+extern void secondary_startup_arm(void);
 
 static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	if (psci_ops.cpu_on)
 		return psci_ops.cpu_on(cpu_logical_map(cpu),
-					virt_to_idmap(&secondary_startup));
+					virt_to_idmap(&secondary_startup_arm));
 	return -ENODEV;
 }
 
-- 
2.17.1

[PATCH 2/3] ARM: psci: Fix secondary core boot with THUMB2_KERNEL

[Mark Rutland]

On Thu, Sep 27, 2018 at 12:27:10PM -0700, Florian Fainelli wrote:
> When THUMB2_KERNEL is enabled, we would be setting the secondary core's
> entry point to secondary_startup() which is already Thumb2 code, utilize
> secondary_startup_arm() which takes care of doing the mode switching for
> us.

As with the first patch, a call to CPU_ON with bit[0] set in the entry
point *should* place the CPU into thumb state, so I'd like to know which
FW you've seen this bug with.

Thanks,
Mark.

> 
> Fixes: 05774088391c ("arm: introduce psci_smp_ops")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm/kernel/psci_smp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
> index cb3fcaeb2233..547a11b065d2 100644
> --- a/arch/arm/kernel/psci_smp.c
> +++ b/arch/arm/kernel/psci_smp.c
> @@ -47,13 +47,13 @@
>   *
>   */
>  
> -extern void secondary_startup(void);
> +extern void secondary_startup_arm(void);
>  
>  static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
>  	if (psci_ops.cpu_on)
>  		return psci_ops.cpu_on(cpu_logical_map(cpu),
> -					virt_to_idmap(&secondary_startup));
> +					virt_to_idmap(&secondary_startup_arm));
>  	return -ENODEV;
>  }
>  
> -- 
> 2.17.1
> 

[PATCH 3/3] soc: bcm: brcmstb: Fix re-entry point with a THUMB2_KERNEL

[Florian Fainelli]

When the kernel is built with CONFIG_THUMB2_KERNEL we would set the
kernel's resume entry point to be a function that is already built as
Thumb-2 code while the boot agent doing the resume is in ARM mode, so
this does not work. There is a header label defined: cpu_resume_arm
which we can use to do the switching for us.

Fixes: 0b741b8234c8 ("soc: bcm: brcmstb: Add support for S2/S3/S5 suspend states (ARM)")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index a5577dd5eb08..8ee06347447c 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -404,7 +404,7 @@ noinline int brcmstb_pm_s3_finish(void)
 {
 	struct brcmstb_s3_params *params = ctrl.s3_params;
 	dma_addr_t params_pa = ctrl.s3_params_pa;
-	phys_addr_t reentry = virt_to_phys(&cpu_resume);
+	phys_addr_t reentry = virt_to_phys(&cpu_resume_arm);
 	enum bsp_initiate_command cmd;
 	u32 flags;
 
-- 
2.17.1

[PATCH 3/3] soc: bcm: brcmstb: Fix re-entry point with a THUMB2_KERNEL

[Florian Fainelli]

On Thu, 27 Sep 2018 12:27:11 -0700, Florian Fainelli <f.fainelli@gmail.com> wrote:
> When the kernel is built with CONFIG_THUMB2_KERNEL we would set the
> kernel's resume entry point to be a function that is already built as
> Thumb-2 code while the boot agent doing the resume is in ARM mode, so
> this does not work. There is a header label defined: cpu_resume_arm
> which we can use to do the switching for us.
> 
> Fixes: 0b741b8234c8 ("soc: bcm: brcmstb: Add support for S2/S3/S5 suspend states (ARM)")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Applied to drivers/next, thanks!
--
Florian

[PATCH 0/3] ARM/PSCI: Fix THUMB2_KERNEL entry points

[Robin Murphy]

On 2018-09-27 8:27 PM, Florian Fainelli wrote:
> Hi all,
> 
> While playing with THUMB2_KERNEL on ARCH_BRCMSTB, several issues came up
> which are addressed by these 3 patches.

Hmmm, PSCI looks to explicitly support Thumb entrypoints ("T32 support" 
in section 6.4.3 of DEN0022D), so these changes smell a little of 
papering over a more fundamental problem, which is presumably either 
that Thumb symbols are not being resolved correctly, or that the 
firmware you're using has a bug.

Robin.

> The THUMB() assembler macro is a no-op unless CONFIG_THUMB2_KERNEL so
> using it unconditionally for CONFIG_ARM should not cause a problem
> AFAICT.
> 
> Those patches can all be independently picked up by their respective
> maintainers and don't depent on one another.
> 
> Thank you!
> 
> Florian Fainelli (3):
>    firmware/psci: Fix cpu_resume entry points with THUMB2_KERNEL
>    ARM: psci: Fix secondary core boot with THUMB2_KERNEL
>    soc: bcm: brcmstb: Fix re-entry point with a THUMB2_KERNEL
> 
>   arch/arm/kernel/psci_smp.c          |  4 ++--
>   drivers/firmware/psci.c             | 18 +++++++++++++++---
>   drivers/soc/bcm/brcmstb/pm/pm-arm.c |  2 +-
>   3 files changed, 18 insertions(+), 6 deletions(-)
> 

[PATCH 0/3] ARM/PSCI: Fix THUMB2_KERNEL entry points

[Florian Fainelli]

On 09/28/2018 01:08 AM, Robin Murphy wrote:
> On 2018-09-27 8:27 PM, Florian Fainelli wrote:
>> Hi all,
>>
>> While playing with THUMB2_KERNEL on ARCH_BRCMSTB, several issues came up
>> which are addressed by these 3 patches.
> 
> Hmmm, PSCI looks to explicitly support Thumb entrypoints ("T32 support"
> in section 6.4.3 of DEN0022D), so these changes smell a little of
> papering over a more fundamental problem, which is presumably either
> that Thumb symbols are not being resolved correctly, or that the
> firmware you're using has a bug.

Yes, I clearly missed that during the internal review of our
implementation and missed it again prior to submitting those patches,
shame on me.

The last patch is valid because it addresses a problem on 32-bit only
platforms which do not have a PSCI firmware at that point.

Thanks!
-- 
Florian