Re: [PATCH v2 1/2] powerpc/64: Set up a kernel stack for secondaries before cpu_restore()
On Fri, Sep 18, 2020 at 5:21 PM Michael Ellerman wrote:
>
> Hi Jordan,
>
> Jordan Niethe writes:
> > 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().
> >
> > Fixes: 5a61ef74f269 ("powerpc/64s: Support new device tree binding for
> > discovering CPU features")
> > Signed-off-by: Jordan Niethe
> > ---
> > v2: Add more detail to the commit message
> > ---
> > arch/powerpc/kernel/head_64.S | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Unfortunately this breaks booting via kexec.
>
> In that case the secondaries come in to 0x60 and spin until they're
> released by smp_release_cpus(), which is before emergency_stack_init()
> has run. That means they pick up a bad r1 value and crash/get stuck.
>
> I'm not sure what the best solution is.
Would it be simplest to just call smp_release_cpus() after setting up the stack?
>
> I've thought in the past that it would be nicer if the CPU setup didn't
> run until the secondary is told to start (via PACAPROCSTART), ie. more
> the CPU setup call below there.
>
> But that opens the possibility that we run threads with different
> settings of some SPRs until SMP bringup, and if the user has said not to
> start secondaries then possibly for ever. And I haven't though hard
> enough about whether that's actually problematic (running with different
> SPR values).
>
> cheers
>
>
> > 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)
> > + subir1,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)
> > - subir1,r1,STACK_FRAME_OVERHEAD
> > -
> > b __secondary_start
> > #endif /* SMP */
> >
> > --
> > 2.17.1
Re: [PATCH v2 1/2] powerpc/64: Set up a kernel stack for secondaries before cpu_restore()
Hi Jordan,
Jordan Niethe writes:
> 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().
>
> Fixes: 5a61ef74f269 ("powerpc/64s: Support new device tree binding for
> discovering CPU features")
> Signed-off-by: Jordan Niethe
> ---
> v2: Add more detail to the commit message
> ---
> arch/powerpc/kernel/head_64.S | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Unfortunately this breaks booting via kexec.
In that case the secondaries come in to 0x60 and spin until they're
released by smp_release_cpus(), which is before emergency_stack_init()
has run. That means they pick up a bad r1 value and crash/get stuck.
I'm not sure what the best solution is.
I've thought in the past that it would be nicer if the CPU setup didn't
run until the secondary is told to start (via PACAPROCSTART), ie. more
the CPU setup call below there.
But that opens the possibility that we run threads with different
settings of some SPRs until SMP bringup, and if the user has said not to
start secondaries then possibly for ever. And I haven't though hard
enough about whether that's actually problematic (running with different
SPR values).
cheers
> 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)
> + subir1,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)
> - subir1,r1,STACK_FRAME_OVERHEAD
> -
> b __secondary_start
> #endif /* SMP */
>
> --
> 2.17.1
[PATCH v2 1/2] powerpc/64: Set up a kernel stack for secondaries before cpu_restore()
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().
Fixes: 5a61ef74f269 ("powerpc/64s: Support new device tree binding for
discovering CPU features")
Signed-off-by: Jordan Niethe
---
v2: Add more detail to the commit message
---
arch/powerpc/kernel/head_64.S | 8
1 file changed, 4 insertions(+), 4 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)
+ subir1,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)
- subir1,r1,STACK_FRAME_OVERHEAD
-
b __secondary_start
#endif /* SMP */
--
2.17.1
