[PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

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 <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Jones <pjones@redhat.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Ingo Molnar]

* Matt Fleming <matt@console-pimps.org> wrote:

> From: Matt Fleming <matt.fleming@intel.com>
> 
> 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 <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Jones <pjones@redhat.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Peter Jones]

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Ingo Molnar]

* Peter Jones <pjones@redhat.com> 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Ingo Molnar]

* Matthew Garrett <mjg59@srcf.ucam.org> 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Peter Jones]

On Thu, Oct 10, 2013 at 07:45:21PM +0200, Ingo Molnar wrote:
> 
> * Peter Jones <pjones@redhat.com> 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Geert Uytterhoeven]

On Thu, Oct 10, 2013 at 8:09 PM, Peter Jones <pjones@redhat.com> 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 -- geert@linux-m68k.org

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Matt Fleming]

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Ingo Molnar]

* Matt Fleming <matt@console-pimps.org> 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[H. Peter Anvin]

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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Matt Fleming]

On Mon, 14 Oct, at 09:57:26AM, Ingo Molnar wrote:
> 
> * H. Peter Anvin <hpa@zytor.com> 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

Re: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

[Ingo Molnar]

* Matt Fleming <matt@console-pimps.org> wrote:

> On Mon, 14 Oct, at 09:57:26AM, Ingo Molnar wrote:
> > 
> > * H. Peter Anvin <hpa@zytor.com> 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 <mingo@kernel.org>

Thanks,

	Ingo