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

[Jordan Niethe]

On Tue, Sep 22, 2020 at 3:59 PM Christophe Leroy
<christophe.leroy at csgroup.eu> wrote:
>
>
>
> 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 at 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 ?
Thanks, you are right, should not be necessary.
>
> 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