Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370031 > unrolled thread

Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Borislav Petkov <bp@alien8.de> - 2016-04-02 20:40 +0200
    Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception Andy Lutomirski <luto@amacapital.net> - 2016-04-02 22:20 +0200
      Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Borislav Petkov <bp@alien8.de> - 2016-04-02 22:50 +0200
        Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception Andy Lutomirski <luto@amacapital.net> - 2016-04-02 23:00 +0200
          Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Jan Kara <jack@suse.cz> - 2016-04-04 14:00 +0200
            Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Peter Zijlstra <peterz@infradead.org> - 2016-04-04 14:50 +0200
            Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Arjan van de Ven <arjan@linux.intel.com> - 2016-04-04 17:40 +0200
            Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception Andy Lutomirski <luto@amacapital.net> - 2016-04-04 17:40 +0200
              Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Peter Zijlstra <peterz@infradead.org> - 2016-04-04 18:10 +0200
                Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Borislav Petkov <bp@alien8.de> - 2016-04-04 21:40 +0200
                  Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception Andy Lutomirski <luto@amacapital.net> - 2016-04-04 23:40 +0200
                    Re: [PATCH v5 3/9] x86/head: Move early exception panic code into  early_fixup_exception Borislav Petkov <bp@alien8.de> - 2016-04-04 23:50 +0200

#1370031 — Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception

On Sat, Apr 02, 2016 at 07:01:34AM -0700, Andy Lutomirski wrote:
> This removes a bunch of assembly and adds some C code instead.  It
> changes the actual printouts on both 32-bit and 64-bit kernels, but
> they still seem okay.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/uaccess.h |  2 +-
>  arch/x86/kernel/head_32.S      | 49 +++++-------------------------------------
>  arch/x86/kernel/head_64.S      | 45 ++------------------------------------
>  arch/x86/mm/extable.c          | 29 ++++++++++++++++++++-----
>  4 files changed, 32 insertions(+), 93 deletions(-)

...

> @@ -99,21 +101,38 @@ int __init early_fixup_exception(struct pt_regs *regs, int trapnr)
>  
>  	/* Ignore early NMIs. */
>  	if (trapnr == X86_TRAP_NMI)
> -		return 1;
> +		return;
> +
> +	if (early_recursion_flag > 2)
> +		goto halt_loop;
> +
> +	if (regs->cs != __KERNEL_CS)
> +		goto fail;
>  
>  	e = search_exception_tables(regs->ip);
>  	if (!e)
> -		return 0;
> +		goto fail;
>  
>  	new_ip  = ex_fixup_addr(e);
>  	handler = ex_fixup_handler(e);
>  
>  	/* special handling not supported during early boot */
>  	if (handler != ex_handler_default)
> -		return 0;
> +		goto fail;
>  
>  	regs->ip = new_ip;
> -	return 1;
> +	return;
> +
> +fail:
> +	early_printk("PANIC: early exception 0x%02x IP %lx:%lx error %lx cr2 0x%lx\n",
> +		     (unsigned)trapnr, (unsigned long)regs->cs, regs->ip,
> +		     regs->orig_ax, read_cr2());
> +
> +	show_regs(regs);

To make this even better, it could be something called early_show_regs()
or so and be a simplified version of __show_regs() on both bitness but
which calls early_printk().

This way you'll be able to get out stuff to the console as early as
possible.

Btw, you don't need to dump rIP, CR2, etc in the PANIC message above
since you're going to early_show_regs() anyway.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [next] | [standalone]

#1370043 — Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception

On Sat, Apr 2, 2016 at 11:39 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Apr 02, 2016 at 07:01:34AM -0700, Andy Lutomirski wrote:
>> This removes a bunch of assembly and adds some C code instead.  It
>> changes the actual printouts on both 32-bit and 64-bit kernels, but
>> they still seem okay.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/include/asm/uaccess.h |  2 +-
>>  arch/x86/kernel/head_32.S      | 49 +++++-------------------------------------
>>  arch/x86/kernel/head_64.S      | 45 ++------------------------------------
>>  arch/x86/mm/extable.c          | 29 ++++++++++++++++++++-----
>>  4 files changed, 32 insertions(+), 93 deletions(-)
>
> ...
>
>> @@ -99,21 +101,38 @@ int __init early_fixup_exception(struct pt_regs *regs, int trapnr)
>>
>>       /* Ignore early NMIs. */
>>       if (trapnr == X86_TRAP_NMI)
>> -             return 1;
>> +             return;
>> +
>> +     if (early_recursion_flag > 2)
>> +             goto halt_loop;
>> +
>> +     if (regs->cs != __KERNEL_CS)
>> +             goto fail;
>>
>>       e = search_exception_tables(regs->ip);
>>       if (!e)
>> -             return 0;
>> +             goto fail;
>>
>>       new_ip  = ex_fixup_addr(e);
>>       handler = ex_fixup_handler(e);
>>
>>       /* special handling not supported during early boot */
>>       if (handler != ex_handler_default)
>> -             return 0;
>> +             goto fail;
>>
>>       regs->ip = new_ip;
>> -     return 1;
>> +     return;
>> +
>> +fail:
>> +     early_printk("PANIC: early exception 0x%02x IP %lx:%lx error %lx cr2 0x%lx\n",
>> +                  (unsigned)trapnr, (unsigned long)regs->cs, regs->ip,
>> +                  regs->orig_ax, read_cr2());
>> +
>> +     show_regs(regs);
>
> To make this even better, it could be something called early_show_regs()
> or so and be a simplified version of __show_regs() on both bitness but
> which calls early_printk().
>
> This way you'll be able to get out stuff to the console as early as
> possible.
>
> Btw, you don't need to dump rIP, CR2, etc in the PANIC message above
> since you're going to early_show_regs() anyway.

Given that I this isn't really a regression with my patches (it
probably never worked much better on 32-bit and the regs never would
have shown at all on 64-bit), I propose a different approach: make
printk work earlier.  Something like:

if (early) {
    early_printk(args);
}

or early_vprintk or whatever.

If the cost of a branch mattered, this could be alternative-patched
out later on, but that seems silly.  I also bet that a more sensible
fallback could be created in which printk would try to use an early
console if there's no real console.

--Andy

[toc] | [prev] | [next] | [standalone]

#1370050

On Sat, Apr 02, 2016 at 01:13:37PM -0700, Andy Lutomirski wrote:
> Given that I this isn't really a regression with my patches (it
> probably never worked much better on 32-bit and the regs never would
> have shown at all on 64-bit),

You're right. That thing calls printk *and* early_printk, WTF:

#ifdef CONFIG_EARLY_PRINTK

	call early_printk
	...

	call dump_stack

	...

	call __print_symbol

those last two call printk. Great.

> I propose a different approach: make
> printk work earlier.  Something like:
> 
> if (early) {
>     early_printk(args);
> }
> 
> or early_vprintk or whatever.
> 
> If the cost of a branch mattered, this could be alternative-patched
> out later on, but that seems silly.  I also bet that a more sensible
> fallback could be created in which printk would try to use an early
> console if there's no real console.

So how about this:

printk() does

	vprintk_func = this_cpu_read(printk_func);

and that's

DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default

I guess we can make that function be early_printk-something and once
printk is initialized, we overwrite it with vprintk_default.

Elegant and no need for if branches and alternatives.

Hmmm.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1370053 — Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception

[cc Jan Kara]

On Sat, Apr 2, 2016 at 1:47 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Apr 02, 2016 at 01:13:37PM -0700, Andy Lutomirski wrote:
>> Given that I this isn't really a regression with my patches (it
>> probably never worked much better on 32-bit and the regs never would
>> have shown at all on 64-bit),
>
> You're right. That thing calls printk *and* early_printk, WTF:
>
> #ifdef CONFIG_EARLY_PRINTK
>
>         call early_printk
>         ...
>
>         call dump_stack
>
>         ...
>
>         call __print_symbol
>
> those last two call printk. Great.
>
>> I propose a different approach: make
>> printk work earlier.  Something like:
>>
>> if (early) {
>>     early_printk(args);
>> }
>>
>> or early_vprintk or whatever.
>>
>> If the cost of a branch mattered, this could be alternative-patched
>> out later on, but that seems silly.  I also bet that a more sensible
>> fallback could be created in which printk would try to use an early
>> console if there's no real console.
>
> So how about this:
>
> printk() does
>
>         vprintk_func = this_cpu_read(printk_func);
>
> and that's
>
> DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default
>
> I guess we can make that function be early_printk-something and once
> printk is initialized, we overwrite it with vprintk_default.
>
> Elegant and no need for if branches and alternatives.
>
> Hmmm.

Jan, IIRC you were looking at printk recently-ish.  Any thoughts here?

--Andy

[toc] | [prev] | [next] | [standalone]

#1370581

On Sat 02-04-16 13:58:19, Andy Lutomirski wrote:
> [cc Jan Kara]
> 
> On Sat, Apr 2, 2016 at 1:47 PM, Borislav Petkov <bp@alien8.de> wrote:
> > On Sat, Apr 02, 2016 at 01:13:37PM -0700, Andy Lutomirski wrote:
> >> Given that I this isn't really a regression with my patches (it
> >> probably never worked much better on 32-bit and the regs never would
> >> have shown at all on 64-bit),
> >
> > You're right. That thing calls printk *and* early_printk, WTF:
> >
> > #ifdef CONFIG_EARLY_PRINTK
> >
> >         call early_printk
> >         ...
> >
> >         call dump_stack
> >
> >         ...
> >
> >         call __print_symbol
> >
> > those last two call printk. Great.
> >
> >> I propose a different approach: make
> >> printk work earlier.  Something like:
> >>
> >> if (early) {
> >>     early_printk(args);
> >> }
> >>
> >> or early_vprintk or whatever.
> >>
> >> If the cost of a branch mattered, this could be alternative-patched
> >> out later on, but that seems silly.  I also bet that a more sensible
> >> fallback could be created in which printk would try to use an early
> >> console if there's no real console.
> >
> > So how about this:
> >
> > printk() does
> >
> >         vprintk_func = this_cpu_read(printk_func);
> >
> > and that's
> >
> > DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default
> >
> > I guess we can make that function be early_printk-something and once
> > printk is initialized, we overwrite it with vprintk_default.
> >
> > Elegant and no need for if branches and alternatives.
> >
> > Hmmm.
> 
> Jan, IIRC you were looking at printk recently-ish.  Any thoughts here?

Sounds like a good idea to me. I've also consulted this with Petr Mladek
(added to CC) who is using printk_func per-cpu variable in his
printk-from-NMI patches and he also doesn't see a problem with this.

I was just wondering about one thing - this way we add more early printks
if I understand your intention right. Are we guaranteed that they happen
only from a single CPU? Because currently there is no locking in
early_printk() and thus we can end up writing to early console several
messages in parallel from different CPUs. Not sure what's going to happen
in that case...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1370599

On Mon, Apr 04, 2016 at 01:52:06PM +0200, Jan Kara wrote:
> Sounds like a good idea to me. I've also consulted this with Petr Mladek
> (added to CC) who is using printk_func per-cpu variable in his
> printk-from-NMI patches and he also doesn't see a problem with this.

There's a few printk() variants that do not go through this; which means
they're broken for a number of cases, including the kdb printk
redirection, this NMI stuff etc.

> I was just wondering about one thing - this way we add more early printks
> if I understand your intention right. Are we guaranteed that they happen
> only from a single CPU? Because currently there is no locking in
> early_printk() and thus we can end up writing to early console several
> messages in parallel from different CPUs. Not sure what's going to happen
> in that case...

You get lovely per char interleaving on you serial line ;-)

What I've done in the past was something like the below; that way you
only get the normal task->softirq->irq->nmi nesting, which is mostly
decipherable.

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bfbf284e4218..c4c3269ff104 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1907,17 +1907,36 @@ struct console *early_console;
 asmlinkage __visible void early_printk(const char *fmt, ...)
 {
 	va_list ap;
+	static int print_cpu = -1;
 	char buf[512];
-	int n;
+	int n, cpu;
 
 	if (!early_console)
 		return;
 
+	preempt_disable();
+	cpu = raw_smp_processor_id();
+	for (;;) {
+		if (READ_ONCE(print_cpu) == cpu)
+			break;
+
+		if (READ_ONCE(print_cpu) == -1 &&
+		    cmpxchg(&print_cpu, -1, cpu) == -1) {
+			cpu = -1;
+			break;
+		}
+
+		cpu_relax();
+	}
+
 	va_start(ap, fmt);
 	n = vscnprintf(buf, sizeof(buf), fmt, ap);
 	va_end(ap);
 
 	early_console->write(early_console, buf, n);
+
+	smp_store_release(&print_cpu, cpu);
+	preempt_enable();
 }
 #endif
 

[toc] | [prev] | [next] | [standalone]

#1370701

On 4/4/2016 8:32 AM, Andy Lutomirski wrote:
>
> Adding locking would be easy enough, wouldn't it?
>
> But do any platforms really boot a second CPU before switching to real
> printk?  Given that I see all the smpboot stuff in dmesg, I guess real
> printk happens first.  I admit I haven't actually checked.

adding locking also makes things more fragile in terms of getting the last thing out
before you go down in flaming death....

until it's a proven problem, this early, get the message out at all is more important
than getting it out perfectly, sometimes.

[toc] | [prev] | [next] | [standalone]

#1370703 — Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception

On Apr 4, 2016 4:51 AM, "Jan Kara" <jack@suse.cz> wrote:
>
> On Sat 02-04-16 13:58:19, Andy Lutomirski wrote:
> > [cc Jan Kara]
> >
> > On Sat, Apr 2, 2016 at 1:47 PM, Borislav Petkov <bp@alien8.de> wrote:
> > > On Sat, Apr 02, 2016 at 01:13:37PM -0700, Andy Lutomirski wrote:
> > >> Given that I this isn't really a regression with my patches (it
> > >> probably never worked much better on 32-bit and the regs never would
> > >> have shown at all on 64-bit),
> > >
> > > You're right. That thing calls printk *and* early_printk, WTF:
> > >
> > > #ifdef CONFIG_EARLY_PRINTK
> > >
> > >         call early_printk
> > >         ...
> > >
> > >         call dump_stack
> > >
> > >         ...
> > >
> > >         call __print_symbol
> > >
> > > those last two call printk. Great.
> > >
> > >> I propose a different approach: make
> > >> printk work earlier.  Something like:
> > >>
> > >> if (early) {
> > >>     early_printk(args);
> > >> }
> > >>
> > >> or early_vprintk or whatever.
> > >>
> > >> If the cost of a branch mattered, this could be alternative-patched
> > >> out later on, but that seems silly.  I also bet that a more sensible
> > >> fallback could be created in which printk would try to use an early
> > >> console if there's no real console.
> > >
> > > So how about this:
> > >
> > > printk() does
> > >
> > >         vprintk_func = this_cpu_read(printk_func);
> > >
> > > and that's
> > >
> > > DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default
> > >
> > > I guess we can make that function be early_printk-something and once
> > > printk is initialized, we overwrite it with vprintk_default.
> > >
> > > Elegant and no need for if branches and alternatives.
> > >
> > > Hmmm.
> >
> > Jan, IIRC you were looking at printk recently-ish.  Any thoughts here?
>
> Sounds like a good idea to me. I've also consulted this with Petr Mladek
> (added to CC) who is using printk_func per-cpu variable in his
> printk-from-NMI patches and he also doesn't see a problem with this.
>
> I was just wondering about one thing - this way we add more early printks
> if I understand your intention right. Are we guaranteed that they happen
> only from a single CPU? Because currently there is no locking in
> early_printk() and thus we can end up writing to early console several
> messages in parallel from different CPUs. Not sure what's going to happen
> in that case...

Adding locking would be easy enough, wouldn't it?

But do any platforms really boot a second CPU before switching to real
printk?  Given that I see all the smpboot stuff in dmesg, I guess real
printk happens first.  I admit I haven't actually checked.

--Andy

>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

[toc] | [prev] | [next] | [standalone]

#1370723

On Mon, Apr 04, 2016 at 08:32:21AM -0700, Andy Lutomirski wrote:

> Adding locking would be easy enough, wouldn't it?

See patch in this thread..

> But do any platforms really boot a second CPU before switching to real
> printk? 

I _only_ use early_printk() as printk() is a quagmire of fail :-)

[toc] | [prev] | [next] | [standalone]

#1370836

On Mon, Apr 04, 2016 at 06:00:42PM +0200, Peter Zijlstra wrote:
> On Mon, Apr 04, 2016 at 08:32:21AM -0700, Andy Lutomirski wrote:
> 
> > Adding locking would be easy enough, wouldn't it?
> 
> See patch in this thread..
> 
> > But do any platforms really boot a second CPU before switching to real
> > printk? 
> 
> I _only_ use early_printk() as printk() is a quagmire of fail :-)

And since I'm the king of minimalistic changes... this below
works. However, the problem is that we need to pull up all that
early_param parsing in order to enable the early console, i.e.
"earlyprintk=ttyS0,115200" cmdline parsing.

And we can do all that after having setup the IDT. So I'd need to do
some early dancing with cmdline_find_option_bool(boot_command_line, ...
in asm or so. Need to think about it more.

---
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 1f4422d5c8d0..ad534226653b 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -130,6 +130,13 @@ static void __init copy_bootdata(char *real_mode_data)
 	}
 }
 
+static int _early_printk(const char *fmt, va_list args)
+{
+	early_printk(fmt, args);
+
+	return 0;
+}
+
 asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
 {
 	int i;
@@ -164,6 +171,10 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
 	load_idt((const struct desc_ptr *)&idt_descr);
 
 	copy_bootdata(__va(real_mode_data));
+	parse_early_param();
+	this_cpu_write(printk_func, _early_printk);
+
+	printk("This is a test!\n");
 
 	/*
 	 * Load microcode early on BSP.
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2367ae07eb76..998d6c675549 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -882,6 +882,7 @@ void __init setup_arch(char **cmdline_p)
 	 */
 	__flush_tlb_all();
 #else
+	this_cpu_write(printk_func, vprintk_default);
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 9ccbdf2c1453..97df81c97b2f 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -169,6 +169,7 @@ void __init setup_log_buf(int early);
 __printf(1, 2) void dump_stack_set_arch_desc(const char *fmt, ...);
 void dump_stack_print_info(const char *log_lvl);
 void show_regs_print_info(const char *log_lvl);
+int vprintk_default(const char *fmt, va_list args);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1370912 — Re: [PATCH v5 3/9] x86/head: Move early exception panic code into early_fixup_exception

On Mon, Apr 4, 2016 at 12:38 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Apr 04, 2016 at 06:00:42PM +0200, Peter Zijlstra wrote:
>> On Mon, Apr 04, 2016 at 08:32:21AM -0700, Andy Lutomirski wrote:
>>
>> > Adding locking would be easy enough, wouldn't it?
>>
>> See patch in this thread..
>>
>> > But do any platforms really boot a second CPU before switching to real
>> > printk?
>>
>> I _only_ use early_printk() as printk() is a quagmire of fail :-)
>
> And since I'm the king of minimalistic changes... this below
> works. However, the problem is that we need to pull up all that
> early_param parsing in order to enable the early console, i.e.
> "earlyprintk=ttyS0,115200" cmdline parsing.
>
> And we can do all that after having setup the IDT. So I'd need to do
> some early dancing with cmdline_find_option_bool(boot_command_line, ...
> in asm or so. Need to think about it more.
>

I'd be nervous about moving early param parsing, especially as part of
the same patch.

Could you do it by moving just the earlyprintk stuff a la
fpu__init_parse_early_param()?

--Andy

[toc] | [prev] | [next] | [standalone]

#1370927

On Mon, Apr 04, 2016 at 02:31:46PM -0700, Andy Lutomirski wrote:
> Could you do it by moving just the earlyprintk stuff a la
> fpu__init_parse_early_param()?

Yeah, something like that. I'll play with this more tomorrow. Btw, no
need to make any of this part of your patchset - I'd like early changes
like that to cook longer for obvious reasons anyway...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web