Re: [PATCH v2 04/44] x86/asm/head: use a common function for starting CPUs

[Josh Poimboeuf]

On Fri, Aug 05, 2016 at 10:41:15AM -0500, Nilay Vaish wrote:
> On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > There are two different pieces of code for starting a CPU: start_cpu0()
> > and the end of secondary_startup_64().  They're identical except for the
> > stack setup.  Combine the common parts into a shared start_cpu()
> > function.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > ---
> >  arch/x86/kernel/head_64.S | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > index aa10a53..8822c20 100644
> > --- a/arch/x86/kernel/head_64.S
> > +++ b/arch/x86/kernel/head_64.S
> > @@ -264,13 +264,15 @@ ENTRY(secondary_startup_64)
> >         movl    $MSR_GS_BASE,%ecx
> >         movl    initial_gs(%rip),%eax
> >         movl    initial_gs+4(%rip),%edx
> > -       wrmsr
> > +       wrmsr
> >
> >         /* rsi is pointer to real mode structure with interesting info.
> >            pass it to C */
> >         movq    %rsi, %rdi
> > -
> > -       /* Finally jump to run C code and to be on real kernel address
> > +
> > +ENTRY(start_cpu)
> > +       /*
> > +        * Jump to run C code and to be on a real kernel address.
> >          * Since we are running on identity-mapped space we have to jump
> >          * to the full 64bit address, this is only possible as indirect
> >          * jump.  In addition we need to ensure %cs is set so we make this
> > @@ -307,15 +309,11 @@ ENDPROC(secondary_startup_64)
> >  /*
> >   * Boot CPU0 entry point. It's called from play_dead(). Everything has been set
> >   * up already except stack. We just set up stack here. Then call
> > - * start_secondary().
> > + * start_secondary() via start_cpu().
> >   */
> >  ENTRY(start_cpu0)
> > -       movq initial_stack(%rip),%rsp
> > -       movq    initial_code(%rip),%rax
> > -       pushq   $0              # fake return address to stop unwinder
> > -       pushq   $__KERNEL_CS    # set correct cs
> > -       pushq   %rax            # target address in negative space
> > -       lretq
> > +       movq    initial_stack(%rip), %rsp
> > +       jmp     start_cpu
> >  ENDPROC(start_cpu0)
> >  #endif
> >
> 
> I have small suggestion here.  To me jumping from start_cpu0 into the
> middle of secondary_startup_64 just seems strange.  May be we can
> define separate ENTRY and ENDPROC pair for start_cpu and jump there
> from start_cpu0 and also from secondary_startup_64.

Yeah, that might be better.  But then again, it would also be strange to
add a jump at the end of secondary_startup_64, when it could instead
just fall through.

Maybe I should do as you suggest, but instead of the jump, add a comment
that it falls through to start_cpu()?

-- 
Josh