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

[Scott Wood <scottwood@freescale.com>]

On Tue, 2015-08-18 at 14:51 +1000, Michael Ellerman wrote:
> 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.

I'm not a fan of it as it isn't distinguishing from a non-underscore version, 
isn't there for namespacing reasons, and it's not even a private 
implementation detail -- it's part of the interface with kexec tools.  I'll 
change it if you want, though.

> > 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)

It stands for "no kexec", which is true if that value is not overwritten.

>  means "unknown", and
> have kexec-tools write "yes" to indicate yes. I realise that's not 100% 
> bullet

That is what I implemented (other than "1" versus "yes").

> > 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?

That would return "false" for non-book3e which isn't correct.

If it has to be done with a single expression, then it'd be:

        return !IS_ENABLED(CONFIG_PPC_FSL_BOOK3E) || booted_from_kexec == 1;

-Scott