* [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C
2017-03-04 9:56 [PATCH 1/2] x86/head_64.S: Rename start_cpu() Borislav Petkov
@ 2017-03-04 9:56 ` Borislav Petkov
2017-03-07 9:11 ` Ingo Molnar
2017-03-07 9:06 ` [PATCH 1/2] x86/head_64.S: Rename start_cpu() Ingo Molnar
2017-03-07 13:01 ` [tip:x86/boot] x86/boot/64: " tip-bot for Borislav Petkov
2 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2017-03-04 9:56 UTC (permalink / raw)
To: X86 ML; +Cc: LKML
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C
2017-03-04 9:56 ` [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C Borislav Petkov
@ 2017-03-07 9:11 ` Ingo Molnar
2017-03-07 10:40 ` Borislav Petkov
0 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2017-03-07 9:11 UTC (permalink / raw)
To: Borislav Petkov; +Cc: X86 ML, LKML, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C
2017-03-07 9:11 ` Ingo Molnar
@ 2017-03-07 10:40 ` Borislav Petkov
0 siblings, 0 replies; 8+ messages in thread
From: Borislav Petkov @ 2017-03-07 10:40 UTC (permalink / raw)
To: Ingo Molnar; +Cc: X86 ML, LKML, Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/head_64.S: Rename start_cpu()
2017-03-04 9:56 [PATCH 1/2] x86/head_64.S: Rename start_cpu() Borislav Petkov
2017-03-04 9:56 ` [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C Borislav Petkov
@ 2017-03-07 9:06 ` Ingo Molnar
2017-03-07 10:38 ` Borislav Petkov
2017-03-07 13:01 ` [tip:x86/boot] x86/boot/64: " tip-bot for Borislav Petkov
2 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2017-03-07 9:06 UTC (permalink / raw)
To: Borislav Petkov; +Cc: X86 ML, LKML, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/head_64.S: Rename start_cpu()
2017-03-07 9:06 ` [PATCH 1/2] x86/head_64.S: Rename start_cpu() Ingo Molnar
@ 2017-03-07 10:38 ` Borislav Petkov
2017-03-07 12:56 ` Ingo Molnar
0 siblings, 1 reply; 8+ messages in thread
From: Borislav Petkov @ 2017-03-07 10:38 UTC (permalink / raw)
To: Ingo Molnar; +Cc: X86 ML, LKML, Thomas Gleixner
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] x86/head_64.S: Rename start_cpu()
2017-03-07 10:38 ` Borislav Petkov
@ 2017-03-07 12:56 ` Ingo Molnar
0 siblings, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2017-03-07 12:56 UTC (permalink / raw)
To: Borislav Petkov; +Cc: X86 ML, LKML, Thomas Gleixner
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip:x86/boot] x86/boot/64: Rename start_cpu()
2017-03-04 9:56 [PATCH 1/2] x86/head_64.S: Rename start_cpu() Borislav Petkov
2017-03-04 9:56 ` [PATCH 2/2] x86/head_64.S: Pass struct boot_params' virtual address to C Borislav Petkov
2017-03-07 9:06 ` [PATCH 1/2] x86/head_64.S: Rename start_cpu() Ingo Molnar
@ 2017-03-07 13:01 ` tip-bot for Borislav Petkov
2 siblings, 0 replies; 8+ messages in thread
From: tip-bot for Borislav Petkov @ 2017-03-07 13:01 UTC (permalink / raw)
To: linux-tip-commits; +Cc: mingo, hpa, bp, peterz, tglx, linux-kernel, torvalds
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
^ permalink raw reply related [flat|nested] 8+ messages in thread