On Wed, 2010-03-31 at 11:32 -0400, Pekka Enberg wrote:
> On Wed, Mar 31, 2010 at 4:41 PM, Guenter Roeck
> <[email protected]> wrote:
> > Current early_printk code writes into VGA memory space even
> > if CONFIG_VGA_CONSOLE is undefined. This can cause problems
> > if there is no VGA device in the system, especially if the memory
> > is used by another device.
> >
> > Fix problem by redirecting output to early_serial_console
> > if CONFIG_VGA_CONSOLE is undefined.
> >
> > Signed-off-by: Guenter Roeck <[email protected]>
>
> Reviewed-by: Pekka Enberg <[email protected]>
>
What will it take to get this patch into the tree ?
If there are coding style issues or some other unresolved concerns,
please let me know.
Guenter
On 04/05/2010 11:10 AM, Guenter Roeck wrote:
> On Wed, 2010-03-31 at 11:32 -0400, Pekka Enberg wrote:
>> On Wed, Mar 31, 2010 at 4:41 PM, Guenter Roeck
>> <[email protected]> wrote:
>>> Current early_printk code writes into VGA memory space even
>>> if CONFIG_VGA_CONSOLE is undefined. This can cause problems
>>> if there is no VGA device in the system, especially if the memory
>>> is used by another device.
>>>
>>> Fix problem by redirecting output to early_serial_console
>>> if CONFIG_VGA_CONSOLE is undefined.
>>>
>>> Signed-off-by: Guenter Roeck <[email protected]>
>>
>> Reviewed-by: Pekka Enberg <[email protected]>
>>
> What will it take to get this patch into the tree ?
>
> If there are coding style issues or some other unresolved concerns,
> please let me know.
>
You didn't answer my question (c).
I want to know how you ended up with
boot_params.screen_info.orig_video_isVGA == 1 on a system with no VGA,
which seems like it would have resolved this.
I am *not* inclined to add a compile-time test for what should have been
handed with a runtime test already.
-hpa
On Mon, 2010-04-05 at 14:46 -0400, H. Peter Anvin wrote:
> On 04/05/2010 11:10 AM, Guenter Roeck wrote:
> > On Wed, 2010-03-31 at 11:32 -0400, Pekka Enberg wrote:
> >> On Wed, Mar 31, 2010 at 4:41 PM, Guenter Roeck
> >> <[email protected]> wrote:
> >>> Current early_printk code writes into VGA memory space even
> >>> if CONFIG_VGA_CONSOLE is undefined. This can cause problems
> >>> if there is no VGA device in the system, especially if the memory
> >>> is used by another device.
> >>>
> >>> Fix problem by redirecting output to early_serial_console
> >>> if CONFIG_VGA_CONSOLE is undefined.
> >>>
> >>> Signed-off-by: Guenter Roeck <[email protected]>
> >>
> >> Reviewed-by: Pekka Enberg <[email protected]>
> >>
> > What will it take to get this patch into the tree ?
> >
> > If there are coding style issues or some other unresolved concerns,
> > please let me know.
> >
>
> You didn't answer my question (c).
>
> I want to know how you ended up with
> boot_params.screen_info.orig_video_isVGA == 1 on a system with no VGA,
> which seems like it would have resolved this.
>
> I am *not* inclined to add a compile-time test for what should have been
> handed with a runtime test already.
>
Sorry, I thought I did answer it.
The problem is that early_printk() can be called prior to the call to
setup_early_printk(). Since early_console is currently pre-initialized
with early_vga_console, output can be written to VGA memory space even
if there is no VGA controller in the system (and even if
boot_params.screen_info.orig_video_isVGA == 0). This happens for all
early_printk() calls executed prior to the call to setup_early_printk().
I don't mind taking out have_vga_console, if that is the issue. That is
just an optimization resulting in the entire VGA code to be optimized
away if CONFIG_VGA_CONSOLE is not defined. The important part of the
patch is to not pre-initialize early_console with early_vga_console if
CONFIG_VGA_CONSOLE is not defined.
An alternative might be to not pre-initialize early_console at all, and
to modify early_printk() to not do anything if early_console is NULL.
However, that would result in output such as "Kernel alive" to not be
displayed at all, which I assumed would be undesirable.
Guenter
On 04/05/2010 01:02 PM, Guenter Roeck wrote:
>>
>> You didn't answer my question (c).
>>
>> I want to know how you ended up with
>> boot_params.screen_info.orig_video_isVGA == 1 on a system with no VGA,
>> which seems like it would have resolved this.
>>
>> I am *not* inclined to add a compile-time test for what should have been
>> handed with a runtime test already.
>>
> Sorry, I thought I did answer it.
>
You didn't. You still haven't!
> The problem is that early_printk() can be called prior to the call to
> setup_early_printk(). Since early_console is currently pre-initialized
> with early_vga_console, output can be written to VGA memory space even
> if there is no VGA controller in the system (and even if
> boot_params.screen_info.orig_video_isVGA == 0). This happens for all
> early_printk() calls executed prior to the call to setup_early_printk().
If boot_params.screen_info.orig_video_isVGA == 0, at least this bit of
your patch has no effect:
> > - if (!strncmp(buf, "vga", 3) &&
> > + if (have_vga_console && !strncmp(buf, "vga", 3) &&
> > boot_params.screen_info.orig_video_isVGA == 1) {
Now, we have at least two ways to report a non-VGA console at runtime:
boot_params.screen_info.orig_video_isVGA != 1
boot_params.screen_info.orig_video_lines == 0
The former is zero for CGA/MDA/EGA, but early_vga_write() doesn't work
right for MDA at least, so keying on isVGA is probably right.
early_printk() being called before setup_early_printk() is a problem,
and it's not immediately obvious to me how to fix it. We can of course
make early_vga_write() simply return if boot_params.screen_info.isVGA ==
0, of course, but it really is a bigger problem than that in many ways.
-hpa
On Mon, 2010-04-05 at 16:25 -0400, H. Peter Anvin wrote:
> On 04/05/2010 01:02 PM, Guenter Roeck wrote:
> >>
> >> You didn't answer my question (c).
> >>
> >> I want to know how you ended up with
> >> boot_params.screen_info.orig_video_isVGA == 1 on a system with no VGA,
> >> which seems like it would have resolved this.
> >>
> >> I am *not* inclined to add a compile-time test for what should have been
> >> handed with a runtime test already.
> >>
> > Sorry, I thought I did answer it.
> >
>
> You didn't. You still haven't!
>
c) boot_params.screen_info.orig_video_isVGA == 1?
boot_params.screen_info.orig_video_isVGA == 0 in the problem case. As I
tried to explain below, the problem happens before setup_early_printk()
is called, and thus the value of orig_video_isVGA is irrelevant for the
problem case. Not sure how else I can explain it.
> > The problem is that early_printk() can be called prior to the call to
> > setup_early_printk(). Since early_console is currently pre-initialized
> > with early_vga_console, output can be written to VGA memory space even
> > if there is no VGA controller in the system (and even if
> > boot_params.screen_info.orig_video_isVGA == 0). This happens for all
> > early_printk() calls executed prior to the call to setup_early_printk().
>
> If boot_params.screen_info.orig_video_isVGA == 0, at least this bit of
> your patch has no effect:
>
> > > - if (!strncmp(buf, "vga", 3) &&
> > > + if (have_vga_console && !strncmp(buf, "vga", 3) &&
> > > boot_params.screen_info.orig_video_isVGA == 1) {
>
It does; as a result of this part of the patch, the compiler can
optimize all vga related code away. As I said, this is just an
optimization resulting in less code. It is however not important /
relevant from a functional point of view, and I don't mind taking it
out.
> Now, we have at least two ways to report a non-VGA console at runtime:
>
> boot_params.screen_info.orig_video_isVGA != 1
> boot_params.screen_info.orig_video_lines == 0
>
> The former is zero for CGA/MDA/EGA, but early_vga_write() doesn't work
> right for MDA at least, so keying on isVGA is probably right.
>
> early_printk() being called before setup_early_printk() is a problem,
> and it's not immediately obvious to me how to fix it. We can of course
> make early_vga_write() simply return if boot_params.screen_info.isVGA ==
> 0, of course, but it really is a bigger problem than that in many ways.
>
As far as I can see, boot_params.screen_info.orig_video_isVGA is set
early enough during boot, so that should at least solve the immediate
problem. However, it would result in early messages being ignored, which
might not be desirable.
Would you accept a minimized patch like this ?
/* Direct interface for emergencies */
+#ifdef CONFIG_VGA_CONSOLE
static struct console *early_console = &early_vga_console;
+#else
+static struct console *early_console = &early_serial_console;
+#endif
static int __initdata early_console_initialized;
This would prevent the problem while minimizing changes, and at the same
time permit early messages to be written to the serial console.
Guenter
On 04/05/2010 02:04 PM, Guenter Roeck wrote:
>
> Would you accept a minimized patch like this ?
>
> /* Direct interface for emergencies */
> +#ifdef CONFIG_VGA_CONSOLE
> static struct console *early_console = &early_vga_console;
> +#else
> +static struct console *early_console = &early_serial_console;
> +#endif
> static int __initdata early_console_initialized;
>
> This would prevent the problem while minimizing changes, and at the same
> time permit early messages to be written to the serial console.
>
I'm unhappy about it, because *those early messages shouldn't exist in
the first place*. It seems to be an indication that we're invoking
setup_early_printk() too late. The whole playing around with max_xpos
and max_ypos instead of using boot_params.screen_info directly is
particularly bleacherous.
I would at least like to see if the improper invocation of
early_printk() can be avoided.
-hpa
On 04/05/2010 02:11 PM, H. Peter Anvin wrote:
> On 04/05/2010 02:04 PM, Guenter Roeck wrote:
>>
>> Would you accept a minimized patch like this ?
>>
>> /* Direct interface for emergencies */
>> +#ifdef CONFIG_VGA_CONSOLE
>> static struct console *early_console = &early_vga_console;
>> +#else
>> +static struct console *early_console = &early_serial_console;
>> +#endif
>> static int __initdata early_console_initialized;
>>
>> This would prevent the problem while minimizing changes, and at the same
>> time permit early messages to be written to the serial console.
>>
>
> I'm unhappy about it, because *those early messages shouldn't exist in
> the first place*. It seems to be an indication that we're invoking
> setup_early_printk() too late. The whole playing around with max_xpos
> and max_ypos instead of using boot_params.screen_info directly is
> particularly bleacherous.
>
> I would at least like to see if the improper invocation of
> early_printk() can be avoided.
>
Specifically: is this anything other than "Kernel alive\n"?
If it's just "Kernel alive\n" I say drop it on the floor.
-hpa
On Mon, 2010-04-05 at 17:11 -0400, H. Peter Anvin wrote:
> On 04/05/2010 02:04 PM, Guenter Roeck wrote:
> >
> > Would you accept a minimized patch like this ?
> >
> > /* Direct interface for emergencies */
> > +#ifdef CONFIG_VGA_CONSOLE
> > static struct console *early_console = &early_vga_console;
> > +#else
> > +static struct console *early_console = &early_serial_console;
> > +#endif
> > static int __initdata early_console_initialized;
> >
> > This would prevent the problem while minimizing changes, and at the same
> > time permit early messages to be written to the serial console.
> >
>
> I'm unhappy about it, because *those early messages shouldn't exist in
> the first place*. It seems to be an indication that we're invoking
> setup_early_printk() too late. The whole playing around with max_xpos
> and max_ypos instead of using boot_params.screen_info directly is
> particularly bleacherous.
>
> I would at least like to see if the improper invocation of
> early_printk() can be avoided.
>
There are several such invocations.
1) arch/x86/kernel/head_64.S:
ENTRY(early_idt_handler)
...
leaq early_idt_msg(%rip),%rdi
call early_printk
This displays "PANIC: early exception %02lx rip %lx:%lx error %lx cr2 %
lx\n" and subsequently calls dump_stack. The handler is initialized from
x86_64_start_kernel().
2) arch/x86/kernel/head64.c:x86_64_start_kernel():
if (console_loglevel == 10)
early_printk("Kernel alive\n");
3) init/main.c: start_kernel()
printk(KERN_NOTICE "%s", linux_banner);
and
printk(KERN_NOTICE "Kernel command line: %s\n", boot_command_line);
4) arch/x86/kernel/setup.c:setup_arch()
Several.
After that I gave up looking.
Not sure if or how those can be avoided.
Moving setup_early_printk() into x86_64_start_kernel() might be an
option, but that would require much more significant changes.
Guenter
On 04/05/2010 03:12 PM, Guenter Roeck wrote:
>>
>> I'm unhappy about it, because *those early messages shouldn't exist in
>> the first place*. It seems to be an indication that we're invoking
>> setup_early_printk() too late. The whole playing around with max_xpos
>> and max_ypos instead of using boot_params.screen_info directly is
>> particularly bleacherous.
>>
>> I would at least like to see if the improper invocation of
>> early_printk() can be avoided.
>>
> There are several such invocations.
>
> 1) arch/x86/kernel/head_64.S:
> ENTRY(early_idt_handler)
> ...
> leaq early_idt_msg(%rip),%rdi
> call early_printk
>
> This displays "PANIC: early exception %02lx rip %lx:%lx error %lx cr2 %
> lx\n" and subsequently calls dump_stack. The handler is initialized from
> x86_64_start_kernel().
>
> 2) arch/x86/kernel/head64.c:x86_64_start_kernel():
> if (console_loglevel == 10)
> early_printk("Kernel alive\n");
>
> 3) init/main.c: start_kernel()
> printk(KERN_NOTICE "%s", linux_banner);
> and
> printk(KERN_NOTICE "Kernel command line: %s\n", boot_command_line);
>
> 4) arch/x86/kernel/setup.c:setup_arch()
> Several.
>
> After that I gave up looking.
>
> Not sure if or how those can be avoided.
>
> Moving setup_early_printk() into x86_64_start_kernel() might be an
> option, but that would require much more significant changes.
>
Okay... that leaves a few very ugly options.
I think spewing onto a potentially uninitialized (or not even present)
serial port is worse than losing the messages. It seems to be pretty
much a no-brainer to have:
if (boot_params.screen_info.orig_video_isVGA != 1)
return;
... in early_vga_write(), and is something we should do regardless.
Calling early_serial_write() before early_serial_init() is distinctly
not safe... depending on boot conditions you might end up with a glacial
boot.
Therefore, the only sensible way to get the early messages out is really
to push setup_early_printk() as early as possible.
-hpa
On Tue, 2010-04-06 at 16:17 -0400, H. Peter Anvin wrote:
[...]
> Therefore, the only sensible way to get the early messages out is really
> to push setup_early_printk() as early as possible.
>
I agree. I am already testing a patch which does exactly that. I should
have it ready for review either later today or tomorrow.
On the plus side, it shows several additional early messages on the
serial console, including the linux version and the command line.
On the downside, I removed the "Kernel alive" message. I don't think
that really makes a difference, though, since I do not see how
console_loglevel can be 10 or higher by the time the message is supposed
to be generated.
Guenter
Hi Guenter,
I wanted to check where we are at... at the very least we should drop
messages issued before initialization when isVGA != 1.
Since serial ports require initialization, I really don't want to send
messages to the serial port before the port has been initialized, but
obviously it would be good to initialize earlyprintk as early as at all
possible.
-hpa
On Mon, Apr 19, 2010 at 05:02:25PM -0400, H. Peter Anvin wrote:
> Hi Guenter,
>
> I wanted to check where we are at... at the very least we should drop
> messages issued before initialization when isVGA != 1.
>
> Since serial ports require initialization, I really don't want to send
> messages to the serial port before the port has been initialized, but
> obviously it would be good to initialize earlyprintk as early as at all
> possible.
>
Moving setup_early_printk() around may be possible, but I do not know
the code good enough to take that risk. Even trying was a bad idea.
I could submit a patch to add some protection into early_printk(),
to ensure it does not write into VGA memory space if there is no VGA,
and do that without changing current semantics - specifically, still
write into VGA memory space even if setup_early_printk() was not called
yet, but if boot_params.screen_info.orig_video_isVGA is set.
I thought you indicated that you are opposed to changing the code,
and it looks kind of clumsy, so I concluded that it was not worth
trying again. Maybe I misunderstood - let me know.
Guenter
On 04/19/2010 07:21 PM, Guenter Roeck wrote:
> On Mon, Apr 19, 2010 at 05:02:25PM -0400, H. Peter Anvin wrote:
>> Hi Guenter,
>>
>> I wanted to check where we are at... at the very least we should drop
>> messages issued before initialization when isVGA != 1.
>>
>> Since serial ports require initialization, I really don't want to send
>> messages to the serial port before the port has been initialized, but
>> obviously it would be good to initialize earlyprintk as early as at all
>> possible.
>>
> Moving setup_early_printk() around may be possible, but I do not know
> the code good enough to take that risk. Even trying was a bad idea.
>
> I could submit a patch to add some protection into early_printk(),
> to ensure it does not write into VGA memory space if there is no VGA,
> and do that without changing current semantics - specifically, still
> write into VGA memory space even if setup_early_printk() was not called
> yet, but if boot_params.screen_info.orig_video_isVGA is set.
>
> I thought you indicated that you are opposed to changing the code,
> and it looks kind of clumsy, so I concluded that it was not worth
> trying again. Maybe I misunderstood - let me know.
>
> Guenter
I think writing into VGA memory if we know there is a VGA device is
there. I don't want to write to an uninitialized serial port, because
that can cause all kinds of problems.
But yes, dropping output to VGA if there isn't one makes a lot of sense.
-hpa