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

[Borislav Petkov]

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andy Lutomirski]

On Mon, Apr 4, 2016 at 12:38 PM, Borislav Petkov  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Borislav Petkov]

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 *)_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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Peter Zijlstra]

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 :-)

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Arjan van de Ven]

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.



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andy Lutomirski]

On Apr 4, 2016 4:51 AM, "Jan Kara"  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  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 
> SUSE Labs, CR

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Peter Zijlstra]

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(_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(_cpu, cpu);
+   preempt_enable();
 }
 #endif
 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Kara]

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  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 
SUSE Labs, CR

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andy Lutomirski]

[cc Jan Kara]

On Sat, Apr 2, 2016 at 1:47 PM, Borislav Petkov  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Borislav Petkov]

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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Andy Lutomirski]

On Sat, Apr 2, 2016 at 11:39 AM, Borislav Petkov  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 
>> ---
>>  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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Borislav Petkov]

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 
> ---
>  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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel