[PATCH 1/2] x86/head_64.S: Rename start_cpu()

[PATCH 1/2] x86/head_64.S: Rename start_cpu()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

It doesn't really start a CPU but does a far jump to C code. So call it
that. Eliminate the unconditional JMP to it from secondary_startup_64()
but make the jump to C code piece part of secondary_startup_64()
instead.

Also, it doesn't need to be a global symbol either so make it a local
label. One less needlessly global symbol in the symbol table.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/head_64.S | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index b467b14b03eb..ac9d327d2e42 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -269,10 +269,8 @@ ENTRY(secondary_startup_64)
 	/* rsi is pointer to real mode structure with interesting info.
 	   pass it to C */
 	movq	%rsi, %rdi
-	jmp	start_cpu
-ENDPROC(secondary_startup_64)
 
-ENTRY(start_cpu)
+.Ljump_to_C_code:
 	/*
 	 * 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
@@ -305,7 +303,7 @@ ENTRY(start_cpu)
 	pushq	%rax		# target address in negative space
 	lretq
 .Lafter_lret:
-ENDPROC(start_cpu)
+ENDPROC(secondary_startup_64)
 
 #include "verify_cpu.S"
 
@@ -313,11 +311,11 @@ ENDPROC(start_cpu)
 /*
  * 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() via start_cpu().
+ * start_secondary() via .Ljump_to_C_code.
  */
 ENTRY(start_cpu0)
 	movq	initial_stack(%rip), %rsp
-	jmp	start_cpu
+	jmp	.Ljump_to_C_code
 ENDPROC(start_cpu0)
 #endif
 
-- 
2.11.0

[PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

... so that callees don't have to convert it and can use it directly.
Simplifies C code a bit. Cleanup comments and formatting in the
vicinity, while at it.

Also, document what phys_base really is.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/head64.c  |  8 ++++----
 arch/x86/kernel/head_64.S | 10 +++++++---
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 54a2372f5dbb..17ee20a9b4e4 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -118,7 +118,7 @@ static unsigned long get_cmd_line_ptr(void)
 
 static void __init copy_bootdata(char *real_mode_data)
 {
-	char * command_line;
+	char *command_line;
 	unsigned long cmd_line_ptr;
 
 	memcpy(&boot_params, real_mode_data, sizeof boot_params);
@@ -130,7 +130,7 @@ static void __init copy_bootdata(char *real_mode_data)
 	}
 }
 
-asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
+asmlinkage __visible void __init x86_64_start_kernel(char *real_mode_data)
 {
 	int i;
 
@@ -163,7 +163,7 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
 		set_intr_gate(i, early_idt_handler_array[i]);
 	load_idt((const struct desc_ptr *)&idt_descr);
 
-	copy_bootdata(__va(real_mode_data));
+	copy_bootdata(real_mode_data);
 
 	/*
 	 * Load microcode early on BSP.
@@ -180,7 +180,7 @@ void __init x86_64_start_reservations(char *real_mode_data)
 {
 	/* version is always not zero if it is copied */
 	if (!boot_params.hdr.version)
-		copy_bootdata(__va(real_mode_data));
+		copy_bootdata(real_mode_data);
 
 	x86_early_init_platform_quirks();
 
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index ac9d327d2e42..506de36293bc 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -266,9 +266,12 @@ ENTRY(secondary_startup_64)
 	movl	initial_gs+4(%rip),%edx
 	wrmsr
 
-	/* rsi is pointer to real mode structure with interesting info.
-	   pass it to C */
-	movq	%rsi, %rdi
+	/*
+	 * %rsi is pointer to real mode struct boot_params with interesting info.
+	 * Pass its virtual address to C.
+	 */
+	movq	$__PAGE_OFFSET_BASE, %rdi
+	addq	%rsi, %rdi
 
 .Ljump_to_C_code:
 	/*
@@ -487,6 +490,7 @@ early_gdt_descr:
 early_gdt_descr_base:
 	.quad	INIT_PER_CPU_VAR(gdt_page)
 
+/* The physical address the kernel is loaded at. */
 ENTRY(phys_base)
 	/* This must match the first entry in level2_kernel_pgt */
 	.quad   0x0000000000000000
-- 
2.11.0

Re: [PATCH 1/2] x86/head_64.S: Rename start_cpu()

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> It doesn't really start a CPU but does a far jump to C code. So call it
> that. Eliminate the unconditional JMP to it from secondary_startup_64()
> but make the jump to C code piece part of secondary_startup_64()
> instead.
> 
> Also, it doesn't need to be a global symbol either so make it a local
> label. One less needlessly global symbol in the symbol table.
> 
> No functionality change.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kernel/head_64.S | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index b467b14b03eb..ac9d327d2e42 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -269,10 +269,8 @@ ENTRY(secondary_startup_64)
>  	/* rsi is pointer to real mode structure with interesting info.
>  	   pass it to C */
>  	movq	%rsi, %rdi
> -	jmp	start_cpu
> -ENDPROC(secondary_startup_64)
>  
> -ENTRY(start_cpu)
> +.Ljump_to_C_code:
>  	/*
>  	 * 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
> @@ -305,7 +303,7 @@ ENTRY(start_cpu)
>  	pushq	%rax		# target address in negative space
>  	lretq
>  .Lafter_lret:
> -ENDPROC(start_cpu)
> +ENDPROC(secondary_startup_64)
>  
>  #include "verify_cpu.S"
>  
> @@ -313,11 +311,11 @@ ENDPROC(start_cpu)
>  /*
>   * 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() via start_cpu().
> + * start_secondary() via .Ljump_to_C_code.
>   */
>  ENTRY(start_cpu0)
>  	movq	initial_stack(%rip), %rsp
> -	jmp	start_cpu
> +	jmp	.Ljump_to_C_code
>  ENDPROC(start_cpu0)
>  #endif

Wouldn't this be slightly more readable:

	jmp	.L_jump_to_C_code

? (Note the extra underscore in the symbol name)

The local labels syntax is silly, I always end up looking twice to make sense of 
'Ljump' or 'Lwhatever' ;-)

We cannot do anything about that - but we can name our symbols to work it around. 
But the price is the extra underscore in the symbol name...

Thanks,

	Ingo

Re: [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> ... so that callees don't have to convert it and can use it directly.
> Simplifies C code a bit. Cleanup comments and formatting in the
> vicinity, while at it.
> 
> Also, document what phys_base really is.
> 
> No functionality change.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kernel/head64.c  |  8 ++++----
>  arch/x86/kernel/head_64.S | 10 +++++++---
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index 54a2372f5dbb..17ee20a9b4e4 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -118,7 +118,7 @@ static unsigned long get_cmd_line_ptr(void)
>  
>  static void __init copy_bootdata(char *real_mode_data)
>  {
> -	char * command_line;
> +	char *command_line;
>  	unsigned long cmd_line_ptr;
>  
>  	memcpy(&boot_params, real_mode_data, sizeof boot_params);
> @@ -130,7 +130,7 @@ static void __init copy_bootdata(char *real_mode_data)
>  	}
>  }
>  
> -asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
> +asmlinkage __visible void __init x86_64_start_kernel(char *real_mode_data)
>  {
>  	int i;
>  
> @@ -163,7 +163,7 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
>  		set_intr_gate(i, early_idt_handler_array[i]);
>  	load_idt((const struct desc_ptr *)&idt_descr);
>  
> -	copy_bootdata(__va(real_mode_data));
> +	copy_bootdata(real_mode_data);
>  
>  	/*
>  	 * Load microcode early on BSP.
> @@ -180,7 +180,7 @@ void __init x86_64_start_reservations(char *real_mode_data)
>  {
>  	/* version is always not zero if it is copied */
>  	if (!boot_params.hdr.version)
> -		copy_bootdata(__va(real_mode_data));
> +		copy_bootdata(real_mode_data);
>  
>  	x86_early_init_platform_quirks();
>  
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index ac9d327d2e42..506de36293bc 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -266,9 +266,12 @@ ENTRY(secondary_startup_64)
>  	movl	initial_gs+4(%rip),%edx
>  	wrmsr
>  
> -	/* rsi is pointer to real mode structure with interesting info.
> -	   pass it to C */
> -	movq	%rsi, %rdi
> +	/*
> +	 * %rsi is pointer to real mode struct boot_params with interesting info.
> +	 * Pass its virtual address to C.
> +	 */
> +	movq	$__PAGE_OFFSET_BASE, %rdi
> +	addq	%rsi, %rdi

The updated comments and other details are fine, but I'm not sure about the __va() 
change: the patch essentially open codes __va() in assembly - which would make any 
changes to __va() [for whatever reason] more difficult, right?

Right now we don't seem to have a single such line of assembly, so changes to 
__va() can be done in C alone.

Thanks,

	Ingo

Re: [PATCH 1/2] x86/head_64.S: Rename start_cpu()

[Borislav Petkov]

On Tue, Mar 07, 2017 at 10:06:38AM +0100, Ingo Molnar wrote:
> The local labels syntax is silly, I always end up looking twice to make sense of 
> 'Ljump' or 'Lwhatever' ;-)
> 
> We cannot do anything about that - but we can name our symbols to work it around. 
> But the price is the extra underscore in the symbol name...

Sure, but other labels in this file don't have the preceding "_".

Perhaps we should agree on a convention for the local labels naming in
our asm and adhere to it...

Hmm.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C

[Borislav Petkov]

On Tue, Mar 07, 2017 at 10:11:40AM +0100, Ingo Molnar wrote:
> The updated comments and other details are fine, but I'm not sure about the __va() 
> change: the patch essentially open codes __va() in assembly - which would make any 
> changes to __va() [for whatever reason] more difficult, right?
> 
> Right now we don't seem to have a single such line of assembly, so changes to 
> __va() can be done in C alone.

Ok, that's a fair point.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 1/2] x86/head_64.S: Rename start_cpu()

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Tue, Mar 07, 2017 at 10:06:38AM +0100, Ingo Molnar wrote:
> > The local labels syntax is silly, I always end up looking twice to make sense of 
> > 'Ljump' or 'Lwhatever' ;-)
> > 
> > We cannot do anything about that - but we can name our symbols to work it around. 
> > But the price is the extra underscore in the symbol name...
> 
> Sure, but other labels in this file don't have the preceding "_".

Yeah, and that would make it all inconsistent - fair enough.

Ok, I'll apply your patch as-is.

> Perhaps we should agree on a convention for the local labels naming in
> our asm and adhere to it...

Too much churn I suspect, plus we'd generate extra churn when people inevitably 
get it wrong.

Thanks,

	Ingo

[tip:x86/boot] x86/boot/64: Rename start_cpu()

[tip-bot for Borislav Petkov]

Commit-ID:  79d243a042155b4421a06faaac15d775a133e6c8
Gitweb:     http://git.kernel.org/tip/79d243a042155b4421a06faaac15d775a133e6c8
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Sat, 4 Mar 2017 10:56:10 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 7 Mar 2017 13:57:25 +0100

x86/boot/64: Rename start_cpu()

It doesn't really start a CPU but does a far jump to C code. So call it
that. Eliminate the unconditional JMP to it from secondary_startup_64()
but make the jump to C code piece part of secondary_startup_64()
instead.

Also, it doesn't need to be a global symbol either so make it a local
label. One less needlessly global symbol in the symbol table.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170304095611.11355-1-bp@alien8.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/head_64.S | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index b467b14..ac9d327 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -269,10 +269,8 @@ ENTRY(secondary_startup_64)
 	/* rsi is pointer to real mode structure with interesting info.
 	   pass it to C */
 	movq	%rsi, %rdi
-	jmp	start_cpu
-ENDPROC(secondary_startup_64)
 
-ENTRY(start_cpu)
+.Ljump_to_C_code:
 	/*
 	 * 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
@@ -305,7 +303,7 @@ ENTRY(start_cpu)
 	pushq	%rax		# target address in negative space
 	lretq
 .Lafter_lret:
-ENDPROC(start_cpu)
+ENDPROC(secondary_startup_64)
 
 #include "verify_cpu.S"
 
@@ -313,11 +311,11 @@ ENDPROC(start_cpu)
 /*
  * 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() via start_cpu().
+ * start_secondary() via .Ljump_to_C_code.
  */
 ENTRY(start_cpu0)
 	movq	initial_stack(%rip), %rsp
-	jmp	start_cpu
+	jmp	.Ljump_to_C_code
 ENDPROC(start_cpu0)
 #endif