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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160805161733.sob3i6fnb6y7kmce@treble>
Date:	Fri, 5 Aug 2016 11:17:33 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Nilay Vaish <nilayvaish@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>, x86 <x86@...nel.org>,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Brian Gerst <brgerst@...il.com>,
	Kees Cook <keescook@...omium.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Byungchul Park <byungchul.park@....com>
Subject: Re: [PATCH v2 04/44] x86/asm/head: use a common function for
 starting CPUs

On Fri, Aug 05, 2016 at 10:41:15AM -0500, Nilay Vaish wrote:
> On 4 August 2016 at 17:22, Josh Poimboeuf <jpoimboe@...hat.com> 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@...hat.com>
> > ---
> >  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