Re: [PATCH v11 3/5] x86/earlyprintk: Allocate early log_buf as early as possible

[Borislav Petkov <bp@suse.de>]

On Tue, Jun 09, 2015 at 06:58:52PM +0300, Andy Shevchenko wrote:
> On Tue, 2015-06-09 at 17:11 +0600, Alexander Kuleshov wrote:
> > This patch moves call of the early setup_log_buf from the
> > arch/x86/kernel/setup.c to the arch/x86/kernel/head{32,64}.c
> > and updates log_buf with the earlyprintk messages.
> > 
> 
> Didn't mention lockdep_init() change. Regarding to what Borislav told
> you how do you handle double call of lockdep_init()?
> 
> 
> > We need to do it because without it we will not see earlyprintk
> > messages in the kernel log.
> > 
> > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> > ---
> >  arch/x86/kernel/head32.c | 6 ++++++
> >  arch/x86/kernel/head64.c | 5 +++++
> >  arch/x86/kernel/setup.c  | 3 ---
> >  kernel/printk/printk.c   | 5 +++++
> >  4 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/head32.c b/arch/x86/kernel/head32.c
> > index 92e8b5f..f28d10f 100644
> > --- a/arch/x86/kernel/head32.c
> > +++ b/arch/x86/kernel/head32.c
> > @@ -32,2 +32,2 @@ static void __init i386_default_early_setup(void)
> >  asmlinkage __visible void __init i386_start_kernel(void)
> >  {
> > 	setup_builtin_cmdline();
> > +
> > +	lockdep_init();
> > +
> > +	/* Allocate early log buffer */
> > +	setup_log_buf(1);
> > +
> >  	cr4_init_shadow();
> >  	sanitize_boot_params(&boot_params);
> >  
> > diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> > index 1e5f064..53662d2 100644
> > --- a/arch/x86/kernel/head64.c
> > +++ b/arch/x86/kernel/head64.c
> > @@ -174,0 +174,0 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
> > 
> >  	setup_builtin_cmdline();
> >  
> > +	lockdep_init();
> > +
> > +	/* Allocate early log buffer */
> > +	setup_log_buf(1);
> > +
> >  	/*
> >  	 * Load microcode early on BSP.
> >  	 */
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 0aeee0a..edfdb6a 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -1146,9 +1146,6 @@ void __init setup_arch(char **cmdline_p)
> >  	if (init_ohci1394_dma_early)
> >  		init_ohci1394_dma_on_all_controllers();
> >  #endif
> > -	/* Allocate bigger log buffer */
> > -	setup_log_buf(1);
> > -
> >  	reserve_initrd();
> >  
> >  #if defined(CONFIG_ACPI) && defined(CONFIG_BLK_DEV_INITRD)
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index c099b08..d76a032 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1913,6 +1913,7 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
> >  	va_list ap;
> >  	char buf[512];
> >  	int n;
> > +	printk_func_t vprintk_func;
> 
> Rearrange this to be upper a bit, for example before char buf[].
> 
> >  
> >  	if (!early_console)
> >  		return;
> > @@ -1922,7 +1923,10 @@ asmlinkage __visible void early_printk(const char *fmt, ...)
> >  	va_end(ap);
> >  
> >  	early_console->write(early_console, buf, n);
> > +
> > +	/* Store earlyprintk messages in the log_buf */
> > +	vprintk_func = this_cpu_read(printk_func);
> > +	vprintk_func(fmt, ap);
> 
> Shouldn't be 
> 
> va_start(ap, fmt);
> …
> vprintk_func(…);
> va_end(ap);
> 
> ?
> 
> Also, when you print the same message to the serial and to kernel
> buffer, do you avoid duplication? Your early_printk messages needs flag
> LOG_NOCONS if I understand correctly.

Calling printk_func that early - which basically is vprintk_default
- I'm not sure, TBH. He pulls setup_log_buf() up but I'm not sure
everything would be kosher that early in the boot. And I don't know the
whole "fun" of printk() to make an informed decision here.

Let me add akpm. Andrew, see above (not snipping it). Do you see any
problems with snatching that printk_func per-cpu pointer that early and
and using it to print to log_buf so that early_printk() calls very early
in the boot are visible in dmesg?

Thanks.

-- 
Regards/Gruss,
    Boris.

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