From: Matt Fleming <[email protected]>
It's incredibly difficult to diagnose early EFI boot issues without
special hardware because earlyprintk=vga doesn't work on EFI systems.
Add support for writing to the EFI framebuffer, via earlyprintk=efi,
which will actually give users a chance of providing debug output.
Cc: H. Peter Anvin <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Jones <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
Documentation/kernel-parameters.txt | 8 +-
arch/x86/Kconfig.debug | 9 ++
arch/x86/include/asm/efi.h | 2 +
arch/x86/kernel/early_printk.c | 7 ++
arch/x86/platform/efi/Makefile | 1 +
arch/x86/platform/efi/early_printk.c | 166 +++++++++++++++++++++++++++++++++++
6 files changed, 190 insertions(+), 3 deletions(-)
create mode 100644 arch/x86/platform/efi/early_printk.c
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index fcbb736..c07cb09 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -847,6 +847,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
earlyprintk= [X86,SH,BLACKFIN,ARM]
earlyprintk=vga
+ earlyprintk=efi
earlyprintk=xen
earlyprintk=serial[,ttySn[,baudrate]]
earlyprintk=serial[,0x...[,baudrate]]
@@ -860,7 +861,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Append ",keep" to not disable it when the real console
takes over.
- Only vga or serial or usb debug port at a time.
+ Only one of vga, efi, serial, or usb debug port can
+ be used at a time.
Currently only ttyS0 and ttyS1 may be specified by
name. Other I/O ports may be explicitly specified
@@ -874,8 +876,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Interaction with the standard serial driver is not
very good.
- The VGA output is eventually overwritten by the real
- console.
+ The VGA and EFI output is eventually overwritten by
+ the real console.
The xen output can only be used by Xen PV guests.
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 78d91af..d05df92 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -59,6 +59,15 @@ config EARLY_PRINTK_DBGP
with klogd/syslogd or the X server. You should normally N here,
unless you want to debug such a crash. You need usb debug device.
+config EARLY_PRINTK_EFI
+ bool "Early printk via EFI framebuffer"
+ depends on EFI && EARLY_PRINTK && FONT_SUPPORT
+ ---help---
+ Write kernel log output directly into the EFI framebuffer.
+
+ This is useful for kernel debugging when your machine crashes very
+ early before the console code is initialized.
+
config X86_PTDUMP
bool "Export kernel pagetable layout to userspace via debugfs"
depends on DEBUG_KERNEL
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 0062a01..65c6e6e 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -109,6 +109,8 @@ static inline bool efi_is_native(void)
return IS_ENABLED(CONFIG_X86_64) == efi_enabled(EFI_64BIT);
}
+extern struct console early_efi_console;
+
#else
/*
* IF EFI is not configured, have the EFI calls return -ENOSYS.
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index d15f575..6d3d200 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -17,6 +17,8 @@
#include <asm/mrst.h>
#include <asm/pgtable.h>
#include <linux/usb/ehci_def.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
/* Simple VGA output */
#define VGABASE (__ISA_IO_base + 0xb8000)
@@ -234,6 +236,11 @@ static int __init setup_early_printk(char *buf)
early_console_register(&early_hsu_console, keep);
}
#endif
+#ifdef CONFIG_EARLY_PRINTK_EFI
+ if (!strncmp(buf, "efi", 3))
+ early_console_register(&early_efi_console, keep);
+#endif
+
buf++;
}
return 0;
diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
index 6db1cc4..b7b0b35 100644
--- a/arch/x86/platform/efi/Makefile
+++ b/arch/x86/platform/efi/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_EFI) += efi.o efi_$(BITS).o efi_stub_$(BITS).o
obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
+obj-$(CONFIG_EARLY_PRINTK_EFI) += early_printk.o
diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
new file mode 100644
index 0000000..bfe597b
--- /dev/null
+++ b/arch/x86/platform/efi/early_printk.c
@@ -0,0 +1,166 @@
+/*
+ * Copyright (C) 2013 Intel Corporation; author Matt Fleming
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#include <linux/console.h>
+#include <linux/font.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <asm/setup.h>
+
+static const struct font_desc *font;
+
+static void early_efi_clear_line(int y)
+{
+ unsigned long base, *dst;
+ u16 len;
+
+ base = boot_params.screen_info.lfb_base;
+ len = boot_params.screen_info.lfb_linelength;
+
+ dst = early_ioremap(base + (y * len), len);
+ if (!dst)
+ return;
+
+ memset(dst, 0, len);
+ early_iounmap(dst, len);
+}
+
+static __init void early_efi_clear_screen(void)
+{
+ struct screen_info *si;
+ int y;
+
+ si = &boot_params.screen_info;
+ for (y = 0; y < si->lfb_height; y++)
+ early_efi_clear_line(y);
+}
+
+static void early_efi_write_char(void *dst, char c, int h)
+{
+ const u8 *src;
+ u32 fgcolor = 0xaaaaaa;
+ u8 s8;
+ int m;
+
+ src = font->data + (c * font->height);
+ s8 = *(src + h);
+
+ for (m = 0; m < 8; m++) {
+ u32 val, mask = 0;
+
+ if ((s8 >> (7 - m)) & 1)
+ mask = 0xffffffff;
+
+ val = mask & fgcolor;
+ memcpy(dst, &val, 4);
+ dst += 4;
+ }
+}
+
+static uint32_t efi_x, efi_y;
+
+static __init void
+early_efi_write(struct console *con, const char *str, unsigned int num)
+{
+ struct screen_info *si;
+ unsigned long base;
+ unsigned int len;
+ const char *s;
+ void *dst;
+
+ base = boot_params.screen_info.lfb_base;
+ si = &boot_params.screen_info;
+ len = si->lfb_linelength;
+
+ while (num) {
+ unsigned int linemax;
+ int h, count = 0;
+
+ for (s = str; *s && *s != '\n'; s++) {
+ if (count == num)
+ break;
+ count++;
+ }
+
+ linemax = (si->lfb_width - efi_x) / font->width;
+ if (count > linemax)
+ count = linemax;
+
+ for (h = 0; h < font->height; h++) {
+ unsigned int n, x;
+
+ dst = early_ioremap(base + (efi_y + h) * len, len);
+ if (!dst)
+ return;
+
+ s = str;
+ n = count;
+ x = efi_x;
+
+ while (n-- > 0) {
+ early_efi_write_char(dst + x * 4, *s++, h);
+ x += font->width;
+ }
+
+ early_iounmap(dst, len);
+ }
+
+ num -= count;
+ efi_x += count * font->width;
+ str += count;
+
+ if (num > 0 && *s == '\n') {
+ efi_x = 0;
+ efi_y += font->height;
+ str++;
+ num--;
+ }
+
+ if (efi_x >= si->lfb_width) {
+ efi_x = 0;
+ efi_y += font->height;
+ }
+
+ if (efi_y + font->height >= si->lfb_height) {
+ early_efi_clear_screen();
+ efi_y = 0;
+ }
+ }
+}
+
+static __init int early_efi_setup(struct console *con, char *options)
+{
+ struct screen_info *si;
+ u16 xres, yres;
+
+ si = &boot_params.screen_info;
+ xres = si->lfb_width;
+ yres = si->lfb_height;
+
+ /*
+ * early_efi_write_char() implicitly assumes a framebuffer with
+ * 32-bits per pixel.
+ */
+ if (si->lfb_depth != 32)
+ return -ENODEV;
+
+ font = get_default_font(xres, yres, ~(u32)0, ~(u32)0);
+ if (!font)
+ return -ENODEV;
+
+ early_efi_clear_screen();
+
+ return 0;
+}
+
+struct console early_efi_console = {
+ .name = "earlyefi",
+ .write = early_efi_write,
+ .setup = early_efi_setup,
+ .flags = CON_PRINTBUFFER,
+ .index = -1,
+};
--
1.8.1.4
* Matt Fleming <[email protected]> wrote:
> From: Matt Fleming <[email protected]>
>
> It's incredibly difficult to diagnose early EFI boot issues without
> special hardware because earlyprintk=vga doesn't work on EFI systems.
>
> Add support for writing to the EFI framebuffer, via earlyprintk=efi,
> which will actually give users a chance of providing debug output.
>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Peter Jones <[email protected]>
> Signed-off-by: Matt Fleming <[email protected]>
Very nice patch!
Some (mostly minor) nits:
> ---
> Documentation/kernel-parameters.txt | 8 +-
> arch/x86/Kconfig.debug | 9 ++
> arch/x86/include/asm/efi.h | 2 +
> arch/x86/kernel/early_printk.c | 7 ++
> arch/x86/platform/efi/Makefile | 1 +
> arch/x86/platform/efi/early_printk.c | 166 +++++++++++++++++++++++++++++++++++
> 6 files changed, 190 insertions(+), 3 deletions(-)
> create mode 100644 arch/x86/platform/efi/early_printk.c
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index fcbb736..c07cb09 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -847,6 +847,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>
> earlyprintk= [X86,SH,BLACKFIN,ARM]
> earlyprintk=vga
> + earlyprintk=efi
> earlyprintk=xen
> earlyprintk=serial[,ttySn[,baudrate]]
> earlyprintk=serial[,0x...[,baudrate]]
> @@ -860,7 +861,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> Append ",keep" to not disable it when the real console
> takes over.
>
> - Only vga or serial or usb debug port at a time.
> + Only one of vga, efi, serial, or usb debug port can
> + be used at a time.
>
> Currently only ttyS0 and ttyS1 may be specified by
> name. Other I/O ports may be explicitly specified
> @@ -874,8 +876,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> Interaction with the standard serial driver is not
> very good.
>
> - The VGA output is eventually overwritten by the real
> - console.
> + The VGA and EFI output is eventually overwritten by
> + the real console.
>
> The xen output can only be used by Xen PV guests.
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 78d91af..d05df92 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -59,6 +59,15 @@ config EARLY_PRINTK_DBGP
> with klogd/syslogd or the X server. You should normally N here,
> unless you want to debug such a crash. You need usb debug device.
>
> +config EARLY_PRINTK_EFI
> + bool "Early printk via EFI framebuffer"
s/via the EFI framebuffer
?
> + depends on EFI && EARLY_PRINTK && FONT_SUPPORT
> + ---help---
> + Write kernel log output directly into the EFI framebuffer.
> +
> + This is useful for kernel debugging when your machine crashes very
> + early before the console code is initialized.
> +
> config X86_PTDUMP
> bool "Export kernel pagetable layout to userspace via debugfs"
> depends on DEBUG_KERNEL
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 0062a01..65c6e6e 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -109,6 +109,8 @@ static inline bool efi_is_native(void)
> return IS_ENABLED(CONFIG_X86_64) == efi_enabled(EFI_64BIT);
> }
>
> +extern struct console early_efi_console;
> +
> #else
> /*
> * IF EFI is not configured, have the EFI calls return -ENOSYS.
> diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
> index d15f575..6d3d200 100644
> --- a/arch/x86/kernel/early_printk.c
> +++ b/arch/x86/kernel/early_printk.c
> @@ -17,6 +17,8 @@
> #include <asm/mrst.h>
> #include <asm/pgtable.h>
> #include <linux/usb/ehci_def.h>
> +#include <linux/efi.h>
> +#include <asm/efi.h>
Given that linux/efi.h does not include asm/efi.h (in purpose I suppose),
asm/efi.h could include linux/efi.h and avoid the duplication thusly.
>
> /* Simple VGA output */
> #define VGABASE (__ISA_IO_base + 0xb8000)
> @@ -234,6 +236,11 @@ static int __init setup_early_printk(char *buf)
> early_console_register(&early_hsu_console, keep);
> }
> #endif
> +#ifdef CONFIG_EARLY_PRINTK_EFI
> + if (!strncmp(buf, "efi", 3))
> + early_console_register(&early_efi_console, keep);
> +#endif
> +
> buf++;
> }
> return 0;
> diff --git a/arch/x86/platform/efi/Makefile b/arch/x86/platform/efi/Makefile
> index 6db1cc4..b7b0b35 100644
> --- a/arch/x86/platform/efi/Makefile
> +++ b/arch/x86/platform/efi/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_EFI) += efi.o efi_$(BITS).o efi_stub_$(BITS).o
> obj-$(CONFIG_ACPI_BGRT) += efi-bgrt.o
> +obj-$(CONFIG_EARLY_PRINTK_EFI) += early_printk.o
> diff --git a/arch/x86/platform/efi/early_printk.c b/arch/x86/platform/efi/early_printk.c
> new file mode 100644
> index 0000000..bfe597b
> --- /dev/null
> +++ b/arch/x86/platform/efi/early_printk.c
> @@ -0,0 +1,166 @@
> +/*
> + * Copyright (C) 2013 Intel Corporation; author Matt Fleming
> + *
> + * This file is part of the Linux kernel, and is made available under
> + * the terms of the GNU General Public License version 2.
> + */
> +
> +#include <linux/console.h>
> +#include <linux/font.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <asm/setup.h>
Does this want to include asm/efi.h, to make sure the definition of
early_efi_console matches?
> +static const struct font_desc *font;
> +
> +static void early_efi_clear_line(int y)
> +{
> + unsigned long base, *dst;
> + u16 len;
> +
> + base = boot_params.screen_info.lfb_base;
> + len = boot_params.screen_info.lfb_linelength;
> +
> + dst = early_ioremap(base + (y * len), len);
that really wants to be:
dst = early_ioremap(base + y*len, len);
as the precedence is clear.
Btw., could we perhaps remap the whole framebuffer at init time, or is it
too large? If early_ioremap() fails for whatever reason then that will
emit a WARN_ON(), which will recurse in a fairly nasty way ...
> + if (!dst)
> + return;
> +
> + memset(dst, 0, len);
> + early_iounmap(dst, len);
> +}
> +
> +static __init void early_efi_clear_screen(void)
> +{
> + struct screen_info *si;
> + int y;
> +
> + si = &boot_params.screen_info;
> + for (y = 0; y < si->lfb_height; y++)
> + early_efi_clear_line(y);
Looks a bit superfluous to introduce 'si' just for that single use?
> +}
> +
> +static void early_efi_write_char(void *dst, char c, int h)
> +{
> + const u8 *src;
> + u32 fgcolor = 0xaaaaaa;
That's RGB grey, right? Why not 0xffffff for a very visible white?
> + u8 s8;
> + int m;
> +
> + src = font->data + (c * font->height);
here too the multiplication does not need parantheses.
> + s8 = *(src + h);
We normally only care about the ASCII range, but doesn't 'c' want to be
'unsigned char', to make sure we do the right thing, should anyone use
above 0x7f characters in printk, accidentally or intentionally?
> +
> + for (m = 0; m < 8; m++) {
> + u32 val, mask = 0;
> +
> + if ((s8 >> (7 - m)) & 1)
> + mask = 0xffffffff;
> +
> + val = mask & fgcolor;
Hm, because this is really about 32-bit pixel framebuffer, is that mask
code a _really_ roundabout way of saying:
const u32 color_grey = 0x00aaaaaa;
const u32 color_black = 0x00000000;
...
if ((s8 >> (7 - m)) & 1)
pixel = color_grey;
else
pixel = color_black;
*dst = pixel;
?
> + memcpy(dst, &val, 4);
Also, why not pass in dst as 'u32 *' and replace the memcpy with:
*dst = val;
?
> + dst += 4;
and this with:
dst++;
?
> + }
> +}
> +
> +static uint32_t efi_x, efi_y;
These globals might make sense at the top of the file.
Also, in kernel code we prefer to write 'u32' or 'unsigned int', not
uint32_t - like you do in many other places within this patch.
> +
> +static __init void
> +early_efi_write(struct console *con, const char *str, unsigned int num)
> +{
> + struct screen_info *si;
> + unsigned long base;
> + unsigned int len;
> + const char *s;
> + void *dst;
> +
> + base = boot_params.screen_info.lfb_base;
> + si = &boot_params.screen_info;
> + len = si->lfb_linelength;
> +
> + while (num) {
> + unsigned int linemax;
> + int h, count = 0;
> +
> + for (s = str; *s && *s != '\n'; s++) {
> + if (count == num)
> + break;
> + count++;
> + }
> +
> + linemax = (si->lfb_width - efi_x) / font->width;
> + if (count > linemax)
> + count = linemax;
> +
> + for (h = 0; h < font->height; h++) {
> + unsigned int n, x;
> +
> + dst = early_ioremap(base + (efi_y + h) * len, len);
So, this applies to other functions as well but it's visible here too:
some of the line/height geometry types are unsigned, others are signed:
'x' is unsigned but 'h' is signed.
It's not consistent - for performance and consistency reasons it might be
good to just standardize all geometry on unsigned int or so?
(Assuming negative numbers are never used, which appears to be the case.)
> + if (!dst)
> + return;
> +
> + s = str;
> + n = count;
> + x = efi_x;
> +
> + while (n-- > 0) {
> + early_efi_write_char(dst + x * 4, *s++, h);
> + x += font->width;
Would be nice to put the s++ side effect into a separate line, it wasn't
obvious to me at first glance.
Also, multiplications are nicer written without the spaces:
early_efi_write_char(dst + x*4, *s++, h);
> + }
> +
> + early_iounmap(dst, len);
> + }
> +
> + num -= count;
> + efi_x += count * font->width;
> + str += count;
> +
> + if (num > 0 && *s == '\n') {
> + efi_x = 0;
> + efi_y += font->height;
btw., it's a fixed-width, fixed-height font, right?
> + str++;
> + num--;
> + }
> +
> + if (efi_x >= si->lfb_width) {
> + efi_x = 0;
> + efi_y += font->height;
> + }
> +
> + if (efi_y + font->height >= si->lfb_height) {
> + early_efi_clear_screen();
> + efi_y = 0;
So, if I understand it correctly this clears the screen and starts at the
top when we scroll off the bottom, right?
That might make the recovery of oopses hard when the number of log lines
is unlucky.
Would scrolling a few lines up instead, via a well-calculated memcpy and
memset be doable?
> + }
> + }
> +}
> +
> +static __init int early_efi_setup(struct console *con, char *options)
> +{
> + struct screen_info *si;
> + u16 xres, yres;
> +
> + si = &boot_params.screen_info;
> + xres = si->lfb_width;
> + yres = si->lfb_height;
> +
> + /*
> + * early_efi_write_char() implicitly assumes a framebuffer with
> + * 32-bits per pixel.
> + */
> + if (si->lfb_depth != 32)
> + return -ENODEV;
Is a non-32-bit framebuffer a possibility? If yes then it might be nice to
emit an informative printk() here, so that users who try to enable EFI
early-printk can at least see why it's not working. (Assuming they get to
look at regular printk output, on a safe/working kernel.)
Also, if non-32-bit framebuffers are common, it would be rather easy to
generalize early_efi_write_char() framebuffer depth multiples of eight
(24, 16):
- white is -1 masked to the pixel width
- black is zero
- the memory step granularity is the number of bytes in a pixel
> +
> + font = get_default_font(xres, yres, ~(u32)0, ~(u32)0);
Any reason why it's not:
font = get_default_font(xres, yres, -1, -1);
?
> + if (!font)
> + return -ENODEV;
> +
> + early_efi_clear_screen();
Assuming we implement scrolling above, here too it might make sense to
scroll up the framebuffer - if the crash is early enough then some
firmware and boot stub info might still be present in the framebuffer?
> +
> + return 0;
> +}
> +
> +struct console early_efi_console = {
> + .name = "earlyefi",
> + .write = early_efi_write,
> + .setup = early_efi_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> +};
Thanks,
Ingo
On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
>
> Is a non-32-bit framebuffer a possibility? If yes then it might be nice to
> emit an informative printk() here, so that users who try to enable EFI
> early-printk can at least see why it's not working. (Assuming they get to
> look at regular printk output, on a safe/working kernel.)
Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
they're define like:
typedef struct {
UINT32 RedMask;
UINT32 GreenMask;
UINT32 BlueMask;
UINT32 ReservedMask;
} EFI_PIXEL_BITMASK;
--
Peter
* Peter Jones <[email protected]> wrote:
> On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
> >
> > Is a non-32-bit framebuffer a possibility? If yes then it might be nice to
> > emit an informative printk() here, so that users who try to enable EFI
> > early-printk can at least see why it's not working. (Assuming they get to
> > look at regular printk output, on a safe/working kernel.)
>
> Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
> they're define like:
>
> typedef struct {
> UINT32 RedMask;
> UINT32 GreenMask;
> UINT32 BlueMask;
> UINT32 ReservedMask;
> } EFI_PIXEL_BITMASK;
Hm, that structure does not show up anywhere in the kernel that I can see.
How are those mask values to be interpreted? As regular bitmasks? Are bits
in the masks set to 1 consecutively, starting from bit 0?
Also, the main question would be, what is the typical value for
si->lfb_depth. 32 on almost all EFI systems? All around the map? Depends
on what graphics state the EFI bootloader passes us?
Thanks,
Ingo
On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote:
> Also, the main question would be, what is the typical value for
> si->lfb_depth. 32 on almost all EFI systems? All around the map? Depends
> on what graphics state the EFI bootloader passes us?
Microsoft require that it be 32, so in practice I suspect it'll always
be 32. It's possible that Itanium decides to be different.
--
Matthew Garrett | [email protected]
On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote:
>
> * Peter Jones <[email protected]> wrote:
>
> > On Thu, Oct 10, 2013 at 07:28:44PM +0200, Ingo Molnar wrote:
> > >
> > > Is a non-32-bit framebuffer a possibility? If yes then it might be nice to
> > > emit an informative printk() here, so that users who try to enable EFI
> > > early-printk can at least see why it's not working. (Assuming they get to
> > > look at regular printk output, on a safe/working kernel.)
> >
> > Not really - the spec allows RGBx, BGRx, and for custom bit masks, but
> > they're define like:
> >
> > typedef struct {
> > UINT32 RedMask;
> > UINT32 GreenMask;
> > UINT32 BlueMask;
> > UINT32 ReservedMask;
> > } EFI_PIXEL_BITMASK;
>
> Hm, that structure does not show up anywhere in the kernel that I can see.
It's the thing being interpretted in arch/x86/boot/compressed/eboot.c in
setup_gop() in the code that looks like:
if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) {
si->lfb_depth = 32;
si->lfb_linelength = pixels_per_scan_line * 4;
...
} else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) {
...
} else if (pixel_format == PIXEL_BIT_MASK) {
find_bits(pixel_info.red_mask, &si->red_pos, &si->red_size);
...
...
> How are those mask values to be interpreted? As regular bitmasks? Are bits
> in the masks set to 1 consecutively, starting from bit 0?
So, the spec actually has some sample code in it:
INTN
GetPixelElementSize (
IN EFI_PIXEL_BITMASK *PixelBits
)
{
INTN HighestPixel = -1;
INTN BluePixel;
INTN RedPixel;
INTN GreenPixel;
INTN RsvdPixel;
BluePixel = FindHighestSetBit (PixelBits->BlueMask);
RedPixel = FindHighestSetBit (PixelBits->RedMask);
GreenPixel = FindHighestSetBit (PixelBits->GreenMask);
RsvdPixel = FindHighestSetBit (PixelBits->ReservedMask);
HighestPixel = max (BluePixel, RedPixel);
HighestPixel = max (HighestPixel, GreenPixel);
HighestPixel = max (HighestPixel, RsvdPixel);
return HighestPixel;
}
EFI_PHYSICAL_ADDRESS NewPixelAddress;
EFI_PHYSICAL_ADDRESS CurrentPixelAddress;
EFI_GRAPHICS_OUTPUT_MODE_INFORMATION OutputInfo;
INTN PixelElementSize;
switch (OutputInfo.PixelFormat) {
case PixelBitMask:
PixelElementSize = GetPixelElementSize (&OutputInfo.PixelInformation);
break;
case PixelBlueGreenRedReserved8BitPerColor:
case PixelRedGreenBlueReserved8BitPerColor:
PixelElementSize = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
break;
}
Which makes this painfully clear.
> Also, the main question would be, what is the typical value for
> si->lfb_depth. 32 on almost all EFI systems? All around the map? Depends
> on what graphics state the EFI bootloader passes us?
Yes, 32 on almost all systems that implement a framebuffer console at
all.
--
Peter
* Matthew Garrett <[email protected]> wrote:
> On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote:
>
> > Also, the main question would be, what is the typical value for
> > si->lfb_depth. 32 on almost all EFI systems? All around the map? Depends
> > on what graphics state the EFI bootloader passes us?
>
> Microsoft require that it be 32, so in practice I suspect it'll always
> be 32. [...]
Ok, cool - and it's a rare sight: Microsoft standing up against bloat and
complexity.
> [...] It's possible that Itanium decides to be different.
That would be a feature!
Thanks,
Ingo
On Thu, Oct 10, 2013 at 8:09 PM, Peter Jones <[email protected]> wrote:
> INTN
> GetPixelElementSize (
> IN EFI_PIXEL_BITMASK *PixelBits
> )
> {
> INTN HighestPixel = -1;
> INTN BluePixel;
> INTN RedPixel;
> INTN GreenPixel;
> INTN RsvdPixel;
> BluePixel = FindHighestSetBit (PixelBits->BlueMask);
> RedPixel = FindHighestSetBit (PixelBits->RedMask);
> GreenPixel = FindHighestSetBit (PixelBits->GreenMask);
> RsvdPixel = FindHighestSetBit (PixelBits->ReservedMask);
> HighestPixel = max (BluePixel, RedPixel);
> HighestPixel = max (HighestPixel, GreenPixel);
> HighestPixel = max (HighestPixel, RsvdPixel);
> return HighestPixel;
> }
> EFI_PHYSICAL_ADDRESS NewPixelAddress;
> EFI_PHYSICAL_ADDRESS CurrentPixelAddress;
> EFI_GRAPHICS_OUTPUT_MODE_INFORMATION OutputInfo;
> INTN PixelElementSize;
>
> switch (OutputInfo.PixelFormat) {
> case PixelBitMask:
> PixelElementSize = GetPixelElementSize (&OutputInfo.PixelInformation);
So this can be less than 32.
> break;
> case PixelBlueGreenRedReserved8BitPerColor:
> case PixelRedGreenBlueReserved8BitPerColor:
> PixelElementSize = sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL);
> break;
> }
>
> Which makes this painfully clear.
>
>> Also, the main question would be, what is the typical value for
>> si->lfb_depth. 32 on almost all EFI systems? All around the map? Depends
>> on what graphics state the EFI bootloader passes us?
>
> Yes, 32 on almost all systems that implement a framebuffer console at
> all.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 10 Oct, at 07:28:44PM, Ingo Molnar wrote:
> Btw., could we perhaps remap the whole framebuffer at init time, or is it
> too large? If early_ioremap() fails for whatever reason then that will
> emit a WARN_ON(), which will recurse in a fairly nasty way ...
The framebuffer memory will be quite large, so I don't think it makes
sense to map it all this early, because it's likely we'll run out of
fixmap entries.
> > + if (!dst)
> > + return;
> > +
> > + memset(dst, 0, len);
> > + early_iounmap(dst, len);
> > +}
> > +
> > +static __init void early_efi_clear_screen(void)
> > +{
> > + struct screen_info *si;
> > + int y;
> > +
> > + si = &boot_params.screen_info;
> > + for (y = 0; y < si->lfb_height; y++)
> > + early_efi_clear_line(y);
>
> Looks a bit superfluous to introduce 'si' just for that single use?
I did this to reduce the length of the "for (y = 0..." line.
> > +}
> > +
> > +static void early_efi_write_char(void *dst, char c, int h)
> > +{
> > + const u8 *src;
> > + u32 fgcolor = 0xaaaaaa;
>
> That's RGB grey, right? Why not 0xffffff for a very visible white?
The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
picked this value.
> > + u8 s8;
> > + int m;
> > +
> > + src = font->data + (c * font->height);
>
> here too the multiplication does not need parantheses.
>
> > + s8 = *(src + h);
>
> We normally only care about the ASCII range, but doesn't 'c' want to be
> 'unsigned char', to make sure we do the right thing, should anyone use
> above 0x7f characters in printk, accidentally or intentionally?
Yeah, this should be unsigned.
> > +
> > + for (m = 0; m < 8; m++) {
> > + u32 val, mask = 0;
> > +
> > + if ((s8 >> (7 - m)) & 1)
> > + mask = 0xffffffff;
> > +
> > + val = mask & fgcolor;
>
> Hm, because this is really about 32-bit pixel framebuffer, is that mask
> code a _really_ roundabout way of saying:
>
> const u32 color_grey = 0x00aaaaaa;
> const u32 color_black = 0x00000000;
> ...
>
> if ((s8 >> (7 - m)) & 1)
> pixel = color_grey;
> else
> pixel = color_black;
>
> *dst = pixel;
>
> ?
>
> > + memcpy(dst, &val, 4);
>
> Also, why not pass in dst as 'u32 *' and replace the memcpy with:
>
> *dst = val;
>
> ?
>
> > + dst += 4;
>
> and this with:
>
> dst++;
>
> ?
Yeah, that's a nice cleanup.
> > + }
> > +
> > + early_iounmap(dst, len);
> > + }
> > +
> > + num -= count;
> > + efi_x += count * font->width;
> > + str += count;
> > +
> > + if (num > 0 && *s == '\n') {
> > + efi_x = 0;
> > + efi_y += font->height;
>
> btw., it's a fixed-width, fixed-height font, right?
Correct.
> > + str++;
> > + num--;
> > + }
> > +
> > + if (efi_x >= si->lfb_width) {
> > + efi_x = 0;
> > + efi_y += font->height;
> > + }
> > +
> > + if (efi_y + font->height >= si->lfb_height) {
> > + early_efi_clear_screen();
> > + efi_y = 0;
>
> So, if I understand it correctly this clears the screen and starts at the
> top when we scroll off the bottom, right?
>
> That might make the recovery of oopses hard when the number of log lines
> is unlucky.
>
> Would scrolling a few lines up instead, via a well-calculated memcpy and
> memset be doable?
Yeah we can do that. I thought about this originally but decided against
it because I figured it would complicate the code unnecessarily. But it
turned out to be fairly trivial.
> > + if (!font)
> > + return -ENODEV;
> > +
> > + early_efi_clear_screen();
>
> Assuming we implement scrolling above, here too it might make sense to
> scroll up the framebuffer - if the crash is early enough then some
> firmware and boot stub info might still be present in the framebuffer?
It's possible that some info will be in the framebuffer, but we can't
begin writing immediately after the boot stub info because we don't know
the last xy coordinates the firmware wrote to.
But yeah, leaving it intact and beginning our output from the last line
of the framebuffer makes more sense than clearing the screen entirely.
--
Matt Fleming, Intel Open Source Technology Center
* Matt Fleming <[email protected]> wrote:
> On Thu, 10 Oct, at 07:28:44PM, Ingo Molnar wrote:
> > Btw., could we perhaps remap the whole framebuffer at init time, or is it
> > too large? If early_ioremap() fails for whatever reason then that will
> > emit a WARN_ON(), which will recurse in a fairly nasty way ...
>
> The framebuffer memory will be quite large, so I don't think it makes
> sense to map it all this early, because it's likely we'll run out of
> fixmap entries.
Fair enough.
> > > +static __init void early_efi_clear_screen(void)
> > > +{
> > > + struct screen_info *si;
> > > + int y;
> > > +
> > > + si = &boot_params.screen_info;
> > > + for (y = 0; y < si->lfb_height; y++)
> > > + early_efi_clear_line(y);
> >
> > Looks a bit superfluous to introduce 'si' just for that single use?
>
> I did this to reduce the length of the "for (y = 0..." line.
But that line looks fine with that included. If it goes slightly above 80
chars that's still OK IMHO.
> > > +static void early_efi_write_char(void *dst, char c, int h)
> > > +{
> > > + const u8 *src;
> > > + u32 fgcolor = 0xaaaaaa;
> >
> > That's RGB grey, right? Why not 0xffffff for a very visible white?
>
> The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
> picked this value.
The VGA code should be changed to white too I suspect ;-)
> > > + if (efi_y + font->height >= si->lfb_height) {
> > > + early_efi_clear_screen();
> > > + efi_y = 0;
> >
> > So, if I understand it correctly this clears the screen and starts at
> > the top when we scroll off the bottom, right?
> >
> > That might make the recovery of oopses hard when the number of log
> > lines is unlucky.
> >
> > Would scrolling a few lines up instead, via a well-calculated memcpy
> > and memset be doable?
>
> Yeah we can do that. I thought about this originally but decided against
> it because I figured it would complicate the code unnecessarily. But it
> turned out to be fairly trivial.
Cool!
> > > + if (!font)
> > > + return -ENODEV;
> > > +
> > > + early_efi_clear_screen();
> >
> > Assuming we implement scrolling above, here too it might make sense to
> > scroll up the framebuffer - if the crash is early enough then some
> > firmware and boot stub info might still be present in the framebuffer?
>
> It's possible that some info will be in the framebuffer, but we can't
> begin writing immediately after the boot stub info because we don't know
> the last xy coordinates the firmware wrote to.
>
> But yeah, leaving it intact and beginning our output from the last line
> of the framebuffer makes more sense than clearing the screen entirely.
Especially with scrolling it should not matter where the previous info is
on the screen: if we start with a scroll event then we can make some space
at the bottom and start our printout there, or so.
Thanks,
Ingo
On 10/12/2013 10:49 AM, Ingo Molnar wrote:
>
>>>> +static void early_efi_write_char(void *dst, char c, int h)
>>>> +{
>>>> + const u8 *src;
>>>> + u32 fgcolor = 0xaaaaaa;
>>>
>>> That's RGB grey, right? Why not 0xffffff for a very visible white?
>>
>> The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
>> picked this value.
>
> The VGA code should be changed to white too I suspect ;-)
>
For compatibility with the classical text console we use light grey as
the default color and a double-stroked font (which was necessary on the
CGA display since it didn't have enough bandwidth to handle
single-stroked fonts well). The problem with changing that to white is
that you end up with a mismatch between the earlyprintk console and the
"real" console, *or* we change the behavior of everyone's consoles...
-hpa
* H. Peter Anvin <[email protected]> wrote:
> On 10/12/2013 10:49 AM, Ingo Molnar wrote:
> >
> >>>> +static void early_efi_write_char(void *dst, char c, int h)
> >>>> +{
> >>>> + const u8 *src;
> >>>> + u32 fgcolor = 0xaaaaaa;
> >>>
> >>> That's RGB grey, right? Why not 0xffffff for a very visible white?
> >>
> >> The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
> >> picked this value.
> >
> > The VGA code should be changed to white too I suspect ;-)
> >
>
> For compatibility with the classical text console we use light grey as
> the default color and a double-stroked font (which was necessary on the
> CGA display since it didn't have enough bandwidth to handle
> single-stroked fonts well). The problem with changing that to white is
> that you end up with a mismatch between the earlyprintk console and the
> "real" console, *or* we change the behavior of everyone's consoles...
Btw., I have no problem at all with making the early console look separate
and making it clear when we switch to the 'real' console.
earlyprintk is a debug method, and more information can only be good.
Thanks,
Ingo
On Mon, 14 Oct, at 09:57:26AM, Ingo Molnar wrote:
>
> * H. Peter Anvin <[email protected]> wrote:
>
> > On 10/12/2013 10:49 AM, Ingo Molnar wrote:
> > >
> > >>>> +static void early_efi_write_char(void *dst, char c, int h)
> > >>>> +{
> > >>>> + const u8 *src;
> > >>>> + u32 fgcolor = 0xaaaaaa;
> > >>>
> > >>> That's RGB grey, right? Why not 0xffffff for a very visible white?
> > >>
> > >> The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
> > >> picked this value.
> > >
> > > The VGA code should be changed to white too I suspect ;-)
> > >
> >
> > For compatibility with the classical text console we use light grey as
> > the default color and a double-stroked font (which was necessary on the
> > CGA display since it didn't have enough bandwidth to handle
> > single-stroked fonts well). The problem with changing that to white is
> > that you end up with a mismatch between the earlyprintk console and the
> > "real" console, *or* we change the behavior of everyone's consoles...
>
> Btw., I have no problem at all with making the early console look separate
> and making it clear when we switch to the 'real' console.
>
> earlyprintk is a debug method, and more information can only be good.
Have we reached a consensus here? I've no strong opinion either way on
the colour of the text.
--
Matt Fleming, Intel Open Source Technology Center
* Matt Fleming <[email protected]> wrote:
> On Mon, 14 Oct, at 09:57:26AM, Ingo Molnar wrote:
> >
> > * H. Peter Anvin <[email protected]> wrote:
> >
> > > On 10/12/2013 10:49 AM, Ingo Molnar wrote:
> > > >
> > > >>>> +static void early_efi_write_char(void *dst, char c, int h)
> > > >>>> +{
> > > >>>> + const u8 *src;
> > > >>>> + u32 fgcolor = 0xaaaaaa;
> > > >>>
> > > >>> That's RGB grey, right? Why not 0xffffff for a very visible white?
> > > >>
> > > >> The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
> > > >> picked this value.
> > > >
> > > > The VGA code should be changed to white too I suspect ;-)
> > > >
> > >
> > > For compatibility with the classical text console we use light grey as
> > > the default color and a double-stroked font (which was necessary on the
> > > CGA display since it didn't have enough bandwidth to handle
> > > single-stroked fonts well). The problem with changing that to white is
> > > that you end up with a mismatch between the earlyprintk console and the
> > > "real" console, *or* we change the behavior of everyone's consoles...
> >
> > Btw., I have no problem at all with making the early console look separate
> > and making it clear when we switch to the 'real' console.
> >
> > earlyprintk is a debug method, and more information can only be good.
>
> Have we reached a consensus here? I've no strong opinion either way on
> the colour of the text.
I'd suggest we go white and see whether anyone complains legitimately ...
In any case:
Acked-by: Ingo Molnar <[email protected]>
Thanks,
Ingo