Re: [RFC,14/17] powerpc/book3e-64/kexec: Enable SMP release

[Michael Ellerman <mpe@ellerman.id.au>]

On Sat, 2015-18-07 at 20:08:51 UTC, Scott Wood wrote:
> booted_from_exec is similar to __run_at_load, except that it is set for
              ^
	      missing k.

Also do you mind using __booted_from_kexec to keep the naming similar to the
other variables down there, and also make it clear it's low level guts.

I see you asked for them to be removed on the original patch but all the other
vars in there are named that way.

> regular kexec as well as kdump.
> 
> The flag is needed because the SMP release mechanism for FSL book3e is
> different from when booting with normal hardware.  In theory we could
> simulate the normal spin table mechanism, but not at the addresses
> U-Boot put in the device tree -- so there'd need to be even more
> communication between the kernel and kexec to set that up.  Since
> there's already a similar flag being set (for kdump only), this seemed
> like a reasonable approach.

Yeah I guess it is. Obviously it'd be nicer if we didn't have to do it though.

> 
> Unlike __run_at_kexec in http://patchwork.ozlabs.org/patch/257657/
> ("book3e/kexec/kdump: introduce a kexec kernel flag"), this flag is at
> a fixed address for ABI stability, and actually gets set properly in
> the kdump case (i.e. on the crash kernel, not on the crashing kernel).
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
> This depends on the kexec-tools patch "ppc64: Add a flag to tell the
> kernel it's booting from kexec":
> http://lists.infradead.org/pipermail/kexec/2015-July/014048.html
> ---
>  arch/powerpc/include/asm/smp.h    |  1 +
>  arch/powerpc/kernel/head_64.S     | 15 +++++++++++++++
>  arch/powerpc/kernel/setup_64.c    | 14 +++++++++++++-
>  arch/powerpc/platforms/85xx/smp.c | 16 ++++++++++++----
>  4 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index 825663c..f9245df 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -197,6 +197,7 @@ extern void generic_secondary_thread_init(void);
>  extern unsigned long __secondary_hold_spinloop;
>  extern unsigned long __secondary_hold_acknowledge;
>  extern char __secondary_hold;
> +extern u32 booted_from_kexec;
>  
>  extern void __early_start(void);
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index 1b77956..ae2d6b5 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -91,6 +91,21 @@ __secondary_hold_spinloop:
>  __secondary_hold_acknowledge:
>  	.llong	0x0
>  
> +	/* Do not move this variable as kexec-tools knows about it. */
> +	. = 0x58
> +	.globl	booted_from_kexec
> +booted_from_kexec:
> +	/*
> +	 * "nkxc" -- not (necessarily) from kexec by default
> +	 *
> +	 * This flag is set to 1 by a loader if the kernel is being
> +	 * booted by kexec.  Older kexec-tools don't know about this
> +	 * flag, so platforms other than fsl-book3e should treat a value
> +	 * of "nkxc" as inconclusive.  fsl-book3e relies on this to
> +	 * know how to release secondary cpus.
> +	 */
> +	.long	0x6e6b7863

Couldn't we say that "nkxc" (whatever that stands for) means "unknown", and
have kexec-tools write "yes" to indicate yes. I realise that's not 100% bullet
proof, but it seems like it would be good enough. And it would mean we could
use the flag on other platforms if we ever want to.

Also "nkxc" ? "bfkx" ?

> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 505ec2c..baeddcc 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -340,11 +340,23 @@ void early_setup_secondary(void)
>  #endif /* CONFIG_SMP */
>  
>  #if defined(CONFIG_SMP) || defined(CONFIG_KEXEC)
> +static bool use_spinloop(void)
> +{
> +#ifdef CONFIG_PPC_FSL_BOOK3E
> +	return booted_from_kexec == 1;
> +#else
> +	return true;
> +#endif

Ugh, more ifdefs.

What about:

	return IS_ENABLED(CONFIG_PPC_FSL_BOOK3E) && (booted_from_kexec == 1);

If that works, I haven't checked. It's slightly less ugly?

> +}
> +
>  void smp_release_cpus(void)
>  {
>  	unsigned long *ptr;
>  	int i;
>  
> +	if (!use_spinloop())
> +		return;
> +
>  	DBG(" -> smp_release_cpus()\n");
>  
>  	/* All secondary cpus are spinning on a common spinloop, release them
> @@ -524,7 +536,7 @@ void __init setup_system(void)
>  	 * Freescale Book3e parts spin in a loop provided by firmware,
>  	 * so smp_release_cpus() does nothing for them
>  	 */
> -#if defined(CONFIG_SMP) && !defined(CONFIG_PPC_FSL_BOOK3E)
> +#if defined(CONFIG_SMP)

Can you make that just #ifdef CONFIG_SMP.

>  	/* Release secondary cpus out of their spinloops at 0x60 now that
>  	 * we can map physical -> logical CPU ids
>  	 */

cheers