linux-kernel - Re: [PATCH v6 3/3] x86/earlyprintk: setup earlyprintk as early as possible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 12 May 2015 20:58:47 +0300
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Alexander Kuleshov <kuleshovmail@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Borislav Petkov <bp@...e.de>,
	Mark Rustad <mark.d.rustad@...el.com>,
	Yinghai Lu <yinghai@...nel.org>
Subject: Re: [PATCH v6 3/3] x86/earlyprintk: setup earlyprintk as early as
 possible

On Tue, 2015-05-12 at 22:26 +0600, Alexander Kuleshov wrote:
> 2015-05-12 17:19 GMT+06:00 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>:
> >> +/* used by arch/x86/kernel/head{32,64}.c */
> >> +int __init setup_early_serial_console(void);
> >> +
> >
> > Actually, have you investigated how this works on the other supported
> > architectures? It might be better to align this with them.
> 
> Yes. In other architetures, earlyprintk setup occurs through 'early_param',
> as it in the x86 now.

So, which means that instead of this proposal (in a hackish way, since
it half-working solution) maybe better to reconsider how you may handle
early_param?

> 
> >
> > What about other cases like that described in setup_early_printk()?
> >
> >> +
> >> +     arg = strstr(boot_command_line, "earlyprintk=");
> >> +     /* += strlen("earlyprintk"); */
> >> +     arg += 12;
> >> +
> >> +     return setup_early_printk(arg);
> >> +#endif
> >
> > So, the logic of this function seems broken. I don't get why you have to
> > check the contents of earlyprintk parameter.
> >
> 
> Because for now we can setup only serial console, for other we need ioremap
> which is not enabled for this moment. Here we just try to find serial console
> and setup it, if another argument passed to the 'earlyprintk', it will
> be parsed in the
> 'setup_arch'.

Even for EFI case?

So, you might redesign that to somehow test if the setup_early_printk()
is called in early_param() context or even earlier and depending on that
do initializations regarding to possibilities, though I have no idea how
to this in clean way.

Currently you have two places where you check the content of the
parameter, which is not okay from my p.o.v.

> 
> >>
> >> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> >> index 38da21c..06fcc1b 100644
> >> --- a/arch/x86/kernel/head64.c
> >> +++ b/arch/x86/kernel/head64.c
> >> @@ -173,6 +173,11 @@ asmlinkage __visible void __init x86_64_start_kernel(char * real_mode_data)
> >>       copy_bootdata(__va(real_mode_data));
> >
> >
> >>       setup_builtin_cmdline();
> >>
> >> +     setup_early_serial_console();
> >
> > Those two can be grouped in the same way like in previous change (see
> > above).
> >
> 
> I'm not sure that I understand this. Can you please, explain what did
> you mean here.

It's about style. Just make empty line before setup_builtin_cmdline()
instead of doing this in between two setup_ functions.

> 
> Thank you.


-- 
Andy Shevchenko <andriy.shevchenko@...el.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/