Re: [PATCH v3 1/2] powerpc/64: Set up a kernel stack for secondaries before cpu_restore()

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 22/09/2020 à 07:53, Jordan Niethe a écrit :
> Currently in generic_secondary_smp_init(), cur_cpu_spec->cpu_restore()
> is called before a stack has been set up in r1. This was previously fine
> as the cpu_restore() functions were implemented in assembly and did not
> use a stack. However commit 5a61ef74f269 ("powerpc/64s: Support new
> device tree binding for discovering CPU features") used
> __restore_cpu_cpufeatures() as the cpu_restore() function for a
> device-tree features based cputable entry. This is a C function and
> hence uses a stack in r1.
> 
> generic_secondary_smp_init() is entered on the secondary cpus via the
> primary cpu using the OPAL call opal_start_cpu(). In OPAL, each hardware
> thread has its own stack. The OPAL call is ran in the primary's hardware
> thread. During the call, a job is scheduled on a secondary cpu that will
> start executing at the address of generic_secondary_smp_init().  Hence
> the value that will be left in r1 when the secondary cpu enters the
> kernel is part of that secondary cpu's individual OPAL stack. This means
> that __restore_cpu_cpufeatures() will write to that OPAL stack. This is
> not horribly bad as each hardware thread has its own stack and the call
> that enters the kernel from OPAL never returns, but it is still wrong
> and should be corrected.
> 
> Create the temp kernel stack before calling cpu_restore().
> 
> As noted by mpe, for a kexec boot, the secondary CPUs are released from
> the spin loop at address 0x60 by smp_release_cpus() and then jump to
> generic_secondary_smp_init(). The call to smp_release_cpus() is in
> setup_arch(), and it comes before the call to emergency_stack_init().
> emergency_stack_init() allocates an emergency stack in the PACA for each
> CPU.  This address in the PACA is what is used to set up the temp kernel
> stack in generic_secondary_smp_init(). Move releasing the secondary CPUs
> to after the PACAs have been allocated an emergency stack, otherwise the
> PACA stack pointer will contain garbage and hence the temp kernel stack
> created from it will be broken.
> 
> Fixes: 5a61ef74f269 ("powerpc/64s: Support new device tree binding for discovering CPU features")
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: Add more detail to the commit message
> v3: Release secondary CPUs after the emergency stack is created
> ---
>   arch/powerpc/kernel/head_64.S      | 8 ++++----
>   arch/powerpc/kernel/setup-common.c | 6 ++++--
>   2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index 0e05a9a47a4b..4b7f4c6c2600 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -420,6 +420,10 @@ generic_secondary_common_init:
>   	/* From now on, r24 is expected to be logical cpuid */
>   	mr	r24,r5
>   
> +	/* Create a temp kernel stack for use before relocation is on.	*/
> +	ld	r1,PACAEMERGSP(r13)
> +	subi	r1,r1,STACK_FRAME_OVERHEAD
> +
>   	/* See if we need to call a cpu state restore handler */
>   	LOAD_REG_ADDR(r23, cur_cpu_spec)
>   	ld	r23,0(r23)
> @@ -448,10 +452,6 @@ generic_secondary_common_init:
>   	sync				/* order paca.run and cur_cpu_spec */
>   	isync				/* In case code patching happened */
>   
> -	/* Create a temp kernel stack for use before relocation is on.	*/
> -	ld	r1,PACAEMERGSP(r13)
> -	subi	r1,r1,STACK_FRAME_OVERHEAD
> -
>   	b	__secondary_start
>   #endif /* SMP */
>   
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 808ec9fab605..fff714e36b37 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -919,8 +919,6 @@ void __init setup_arch(char **cmdline_p)
>   
>   	/* On BookE, setup per-core TLB data structures. */
>   	setup_tlb_core_data();
> -
> -	smp_release_cpus();
>   #endif
>   
>   	/* Print various info about the machine that has been gathered so far. */
> @@ -944,6 +942,10 @@ void __init setup_arch(char **cmdline_p)
>   	exc_lvl_early_init();
>   	emergency_stack_init();
>   
> +#ifdef CONFIG_SMP
> +	smp_release_cpus();
> +#endif

Are you sure you need that #ifdef ?

In asm/smp.h, we have:

#if defined(CONFIG_PPC64) && (defined(CONFIG_SMP) || 
defined(CONFIG_KEXEC_CORE))
extern void smp_release_cpus(void);
#else
static inline void smp_release_cpus(void) { };
#endif


> +
>   	initmem_init();
>   
>   	early_memtest(min_low_pfn << PAGE_SHIFT, max_low_pfn << PAGE_SHIFT);
> 

Christophe