[PATCH 0/8] EFI framebuffer support for ARM and arm64

[PATCH 0/8] EFI framebuffer support for ARM and arm64

[Ard Biesheuvel]

This series adds support to ARM and arm64 for using the kernel's EFI
framebuffer driver to drive EFI Graphics Output Protocol (GOP) based
framebuffers.

This involves refactoring some of the existing x86 code so it can be reused
by ARM and arm64, and wiring it up both in the UEFI stub and in the core
kernel into the existing UEFI infrastructure for ARM and arm64.

Ard Biesheuvel (8):
  efi: make install_configuration_table() boot service usable
  efi: libstub: move Graphics Output Protocol handling to generic code
  efi/x86: libstub: move to generic GOP code
  efi/x86: efifb: move DMI based quirks handling out of generic code
  efi: efifb: use builtin_platform_driver and drop unused includes
  efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub
  efi/arm*: efifb: expose efifb platform device if GOP is available
  efi/arm: populate screen_info based on data provided by the UEFI stub

 arch/arm/include/asm/efi.h                |  20 +-
 arch/arm/kernel/efi.c                     |  24 ++
 arch/arm/kernel/setup.c                   |   3 +-
 arch/arm64/include/asm/efi.h              |  11 +-
 arch/arm64/kernel/efi.c                   |   3 +
 arch/arm64/kernel/image.h                 |   1 +
 arch/x86/boot/compressed/eboot.c          | 308 +----------------
 arch/x86/boot/compressed/eboot.h          |  74 ----
 arch/x86/include/asm/efi.h                |   7 +
 arch/x86/kernel/sysfb_efi.c               |  15 +
 drivers/firmware/efi/arm-init.c           |  25 ++
 drivers/firmware/efi/libstub/Makefile     |   2 +-
 drivers/firmware/efi/libstub/arm-stub.c   |  23 ++
 drivers/firmware/efi/libstub/arm32-stub.c |  38 +++
 drivers/firmware/efi/libstub/gop.c        | 354 ++++++++++++++++++++
 drivers/video/fbdev/Kconfig               |   2 +-
 drivers/video/fbdev/efifb.c               |  21 +-
 include/linux/efi.h                       |  89 ++++-
 18 files changed, 615 insertions(+), 405 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/gop.c

-- 
1.9.1

[PATCH 1/8] efi: make install_configuration_table() boot service usable

[Ard Biesheuvel]

This patch redeclares efi_boot_services_t::install_configuration_table
as a function pointer so that the boot service is callable as a function.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 include/linux/efi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index e747eb08b2be..8ba6eb7863aa 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -283,7 +283,7 @@ typedef struct {
 	void *register_protocol_notify;
 	void *locate_handle;
 	void *locate_device_path;
-	void *install_configuration_table;
+	efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
 	void *load_image;
 	void *start_image;
 	void *exit;
-- 
1.9.1

Re: [PATCH 1/8] efi: make install_configuration_table() boot service usable

[Matt Fleming]

On Thu, 10 Mar, at 12:40:01PM, Ard Biesheuvel wrote:
> This patch redeclares efi_boot_services_t::install_configuration_table
> as a function pointer so that the boot service is callable as a function.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  include/linux/efi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
 
Any chance you could fold this patch into the one that actually uses
install_configuration_table() on ARM? That way it's clear to see where
the users are.

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index e747eb08b2be..8ba6eb7863aa 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -283,7 +283,7 @@ typedef struct {
>  	void *register_protocol_notify;
>  	void *locate_handle;
>  	void *locate_device_path;
> -	void *install_configuration_table;
> +	efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
>  	void *load_image;
>  	void *start_image;
>  	void *exit;
> -- 
> 1.9.1
> 

Re: [PATCH 1/8] efi: make install_configuration_table() boot service usable

[Ard Biesheuvel]

On 18 March 2016 at 11:59, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Thu, 10 Mar, at 12:40:01PM, Ard Biesheuvel wrote:
>> This patch redeclares efi_boot_services_t::install_configuration_table
>> as a function pointer so that the boot service is callable as a function.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  include/linux/efi.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Any chance you could fold this patch into the one that actually uses
> install_configuration_table() on ARM? That way it's clear to see where
> the users are.
>

Sure. Patch #2 actually does the same for locate_handle()


>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index e747eb08b2be..8ba6eb7863aa 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -283,7 +283,7 @@ typedef struct {
>>       void *register_protocol_notify;
>>       void *locate_handle;
>>       void *locate_device_path;
>> -     void *install_configuration_table;
>> +     efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
>>       void *load_image;
>>       void *start_image;
>>       void *exit;
>> --
>> 1.9.1
>>

[PATCH 2/8] efi: libstub: move Graphics Output Protocol handling to generic code

[Ard Biesheuvel]

The Graphics Output Protocol code executes in the stub, so create a generic
version based on the x86 version in libstub so that we can move all archs
to it in subsequent patches. The new source file gop.c is added to the
libstub build for all architectures, but is not actually included in any of
the final images, since this patch does not wire it up yet.

Note that the generic version uses slightly different ways of casting the
protocol methods and some other variables to the correct types, since such
method calls are not loosely typed on ARM and arm64 as they are on x86.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/include/asm/efi.h            |   4 +-
 arch/arm64/include/asm/efi.h          |   4 +-
 arch/x86/boot/compressed/eboot.h      |  74 ----
 arch/x86/include/asm/efi.h            |   5 +
 drivers/firmware/efi/libstub/Makefile |   2 +-
 drivers/firmware/efi/libstub/gop.c    | 354 ++++++++++++++++++++
 include/linux/efi.h                   |  87 ++++-
 7 files changed, 451 insertions(+), 79 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72deb87..daebcfe6c35a 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -59,7 +59,9 @@ void efi_virtmap_unload(void);
 
 /* arch specific definitions used by the stub code */
 
-#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
+#define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
+#define __efi_call_early(f, ...)	f(__VA_ARGS__)
+#define efi_is_64bit()			(0)
 
 /*
  * A reasonable upper bound for the uncompressed kernel size is 32 MBytes,
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a696c9cb..e3d707131bba 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -50,7 +50,9 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
 #define MAX_FDT_OFFSET	SZ_512M
 
-#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
+#define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
+#define __efi_call_early(f, ...)	f(__VA_ARGS__)
+#define efi_is_64bit()			(1)
 
 #define EFI_ALLOC_ALIGN		SZ_64K
 
diff --git a/arch/x86/boot/compressed/eboot.h b/arch/x86/boot/compressed/eboot.h
index d487e727f1ec..c0223f1a89d7 100644
--- a/arch/x86/boot/compressed/eboot.h
+++ b/arch/x86/boot/compressed/eboot.h
@@ -11,80 +11,6 @@
 
 #define DESC_TYPE_CODE_DATA	(1 << 0)
 
-#define EFI_CONSOLE_OUT_DEVICE_GUID    \
-	EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4, 0x9a, 0x46, 0x0, 0x90, 0x27, \
-		  0x3f, 0xc1, 0x4d)
-
-#define PIXEL_RGB_RESERVED_8BIT_PER_COLOR		0
-#define PIXEL_BGR_RESERVED_8BIT_PER_COLOR		1
-#define PIXEL_BIT_MASK					2
-#define PIXEL_BLT_ONLY					3
-#define PIXEL_FORMAT_MAX				4
-
-struct efi_pixel_bitmask {
-	u32 red_mask;
-	u32 green_mask;
-	u32 blue_mask;
-	u32 reserved_mask;
-};
-
-struct efi_graphics_output_mode_info {
-	u32 version;
-	u32 horizontal_resolution;
-	u32 vertical_resolution;
-	int pixel_format;
-	struct efi_pixel_bitmask pixel_information;
-	u32 pixels_per_scan_line;
-} __packed;
-
-struct efi_graphics_output_protocol_mode_32 {
-	u32 max_mode;
-	u32 mode;
-	u32 info;
-	u32 size_of_info;
-	u64 frame_buffer_base;
-	u32 frame_buffer_size;
-} __packed;
-
-struct efi_graphics_output_protocol_mode_64 {
-	u32 max_mode;
-	u32 mode;
-	u64 info;
-	u64 size_of_info;
-	u64 frame_buffer_base;
-	u64 frame_buffer_size;
-} __packed;
-
-struct efi_graphics_output_protocol_mode {
-	u32 max_mode;
-	u32 mode;
-	unsigned long info;
-	unsigned long size_of_info;
-	u64 frame_buffer_base;
-	unsigned long frame_buffer_size;
-} __packed;
-
-struct efi_graphics_output_protocol_32 {
-	u32 query_mode;
-	u32 set_mode;
-	u32 blt;
-	u32 mode;
-};
-
-struct efi_graphics_output_protocol_64 {
-	u64 query_mode;
-	u64 set_mode;
-	u64 blt;
-	u64 mode;
-};
-
-struct efi_graphics_output_protocol {
-	void *query_mode;
-	unsigned long set_mode;
-	unsigned long blt;
-	struct efi_graphics_output_protocol_mode *mode;
-};
-
 struct efi_uga_draw_protocol_32 {
 	u32 get_mode;
 	u32 set_mode;
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 08b1f2f6ea50..7d5187b66108 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -199,6 +199,11 @@ __pure const struct efi_config *__efi_early(void);
 #define efi_call_early(f, ...)						\
 	__efi_early()->call(__efi_early()->f, __VA_ARGS__);
 
+#define __efi_call_early(f, ...)					\
+	__efi_early()->call((unsigned long)f, __VA_ARGS__);
+
+#define efi_is_64bit()		__efi_early()->is64
+
 extern bool efi_reboot_required(void);
 
 #else
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index ad077944aa0e..595f5920a740 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -24,7 +24,7 @@ GCOV_PROFILE			:= n
 KASAN_SANITIZE			:= n
 UBSAN_SANITIZE			:= n
 
-lib-y				:= efi-stub-helper.o
+lib-y				:= efi-stub-helper.o gop.o
 
 # include the stub's generic dependencies from lib/ when building for ARM/arm64
 arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
new file mode 100644
index 000000000000..932742e4cf23
--- /dev/null
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -0,0 +1,354 @@
+/* -----------------------------------------------------------------------
+ *
+ *   Copyright 2011 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/efi.h>
+#include <linux/screen_info.h>
+#include <asm/efi.h>
+#include <asm/setup.h>
+
+static void find_bits(unsigned long mask, u8 *pos, u8 *size)
+{
+	u8 first, len;
+
+	first = 0;
+	len = 0;
+
+	if (mask) {
+		while (!(mask & 0x1)) {
+			mask = mask >> 1;
+			first++;
+		}
+
+		while (mask & 0x1) {
+			mask = mask >> 1;
+			len++;
+		}
+	}
+
+	*pos = first;
+	*size = len;
+}
+
+static void
+setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line,
+		 struct efi_pixel_bitmask pixel_info, int pixel_format)
+{
+	if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) {
+		si->lfb_depth = 32;
+		si->lfb_linelength = pixels_per_scan_line * 4;
+		si->red_size = 8;
+		si->red_pos = 0;
+		si->green_size = 8;
+		si->green_pos = 8;
+		si->blue_size = 8;
+		si->blue_pos = 16;
+		si->rsvd_size = 8;
+		si->rsvd_pos = 24;
+	} else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) {
+		si->lfb_depth = 32;
+		si->lfb_linelength = pixels_per_scan_line * 4;
+		si->red_size = 8;
+		si->red_pos = 16;
+		si->green_size = 8;
+		si->green_pos = 8;
+		si->blue_size = 8;
+		si->blue_pos = 0;
+		si->rsvd_size = 8;
+		si->rsvd_pos = 24;
+	} else if (pixel_format == PIXEL_BIT_MASK) {
+		find_bits(pixel_info.red_mask, &si->red_pos, &si->red_size);
+		find_bits(pixel_info.green_mask, &si->green_pos,
+			  &si->green_size);
+		find_bits(pixel_info.blue_mask, &si->blue_pos, &si->blue_size);
+		find_bits(pixel_info.reserved_mask, &si->rsvd_pos,
+			  &si->rsvd_size);
+		si->lfb_depth = si->red_size + si->green_size +
+			si->blue_size + si->rsvd_size;
+		si->lfb_linelength = (pixels_per_scan_line * si->lfb_depth) / 8;
+	} else {
+		si->lfb_depth = 4;
+		si->lfb_linelength = si->lfb_width / 2;
+		si->red_size = 0;
+		si->red_pos = 0;
+		si->green_size = 0;
+		si->green_pos = 0;
+		si->blue_size = 0;
+		si->blue_pos = 0;
+		si->rsvd_size = 0;
+		si->rsvd_pos = 0;
+	}
+}
+
+static efi_status_t
+__gop_query32(efi_system_table_t *sys_table_arg,
+	      struct efi_graphics_output_protocol_32 *gop32,
+	      struct efi_graphics_output_mode_info **info,
+	      unsigned long *size, u64 *fb_base)
+{
+	struct efi_graphics_output_protocol_mode_32 *mode;
+	efi_graphics_output_protocol_query_mode query_mode;
+	efi_status_t status;
+	unsigned long m;
+
+	m = gop32->mode;
+	mode = (struct efi_graphics_output_protocol_mode_32 *)m;
+	query_mode = (void *)(unsigned long)gop32->query_mode;
+
+	status = __efi_call_early(query_mode, (void *)gop32, mode->mode, size,
+				  info);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	*fb_base = mode->frame_buffer_base;
+	return status;
+}
+
+static efi_status_t
+setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
+            efi_guid_t *proto, unsigned long size, void **gop_handle)
+{
+	struct efi_graphics_output_protocol_32 *gop32, *first_gop;
+	unsigned long nr_gops;
+	u16 width, height;
+	u32 pixels_per_scan_line;
+	u32 ext_lfb_base;
+	u64 fb_base;
+	struct efi_pixel_bitmask pixel_info;
+	int pixel_format;
+	efi_status_t status = EFI_NOT_FOUND;
+	u32 *handles = (u32 *)(unsigned long)gop_handle;
+	int i;
+
+	first_gop = NULL;
+	gop32 = NULL;
+
+	nr_gops = size / sizeof(u32);
+	for (i = 0; i < nr_gops; i++) {
+		struct efi_graphics_output_mode_info *info = NULL;
+		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
+		bool conout_found = false;
+		void *dummy = NULL;
+		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
+		u64 current_fb_base;
+
+		status = efi_call_early(handle_protocol, h,
+					proto, (void **)&gop32);
+		if (status != EFI_SUCCESS)
+			continue;
+
+		status = efi_call_early(handle_protocol, h,
+					&conout_proto, &dummy);
+		if (status == EFI_SUCCESS)
+			conout_found = true;
+
+		status = __gop_query32(sys_table_arg, gop32, &info, &size,
+				       &current_fb_base);
+		if (status == EFI_SUCCESS && (!first_gop || conout_found)) {
+			/*
+			 * Systems that use the UEFI Console Splitter may
+			 * provide multiple GOP devices, not all of which are
+			 * backed by real hardware. The workaround is to search
+			 * for a GOP implementing the ConOut protocol, and if
+			 * one isn't found, to just fall back to the first GOP.
+			 */
+			width = info->horizontal_resolution;
+			height = info->vertical_resolution;
+			pixel_format = info->pixel_format;
+			pixel_info = info->pixel_information;
+			pixels_per_scan_line = info->pixels_per_scan_line;
+			fb_base = current_fb_base;
+
+			/*
+			 * Once we've found a GOP supporting ConOut,
+			 * don't bother looking any further.
+			 */
+			first_gop = gop32;
+			if (conout_found)
+				break;
+		}
+	}
+
+	/* Did we find any GOPs? */
+	if (!first_gop)
+		goto out;
+
+	/* EFI framebuffer */
+	si->orig_video_isVGA = VIDEO_TYPE_EFI;
+
+	si->lfb_width = width;
+	si->lfb_height = height;
+	si->lfb_base = fb_base;
+
+	ext_lfb_base = (u64)(unsigned long)fb_base >> 32;
+	if (ext_lfb_base) {
+		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
+		si->ext_lfb_base = ext_lfb_base;
+	}
+
+	si->pages = 1;
+
+	setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
+
+	si->lfb_size = si->lfb_linelength * si->lfb_height;
+
+	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
+out:
+	return status;
+}
+
+static efi_status_t
+__gop_query64(efi_system_table_t *sys_table_arg,
+	      struct efi_graphics_output_protocol_64 *gop64,
+	      struct efi_graphics_output_mode_info **info,
+	      unsigned long *size, u64 *fb_base)
+{
+	struct efi_graphics_output_protocol_mode_64 *mode;
+	efi_graphics_output_protocol_query_mode query_mode;
+	efi_status_t status;
+	unsigned long m;
+
+	m = gop64->mode;
+	mode = (struct efi_graphics_output_protocol_mode_64 *)m;
+	query_mode = (void *)(unsigned long)gop64->query_mode;
+
+	status = __efi_call_early(query_mode, (void *)gop64, mode->mode, size,
+				  info);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	*fb_base = mode->frame_buffer_base;
+	return status;
+}
+
+static efi_status_t
+setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
+	    efi_guid_t *proto, unsigned long size, void **gop_handle)
+{
+	struct efi_graphics_output_protocol_64 *gop64, *first_gop;
+	unsigned long nr_gops;
+	u16 width, height;
+	u32 pixels_per_scan_line;
+	u32 ext_lfb_base;
+	u64 fb_base;
+	struct efi_pixel_bitmask pixel_info;
+	int pixel_format;
+	efi_status_t status = EFI_NOT_FOUND;
+	u64 *handles = (u64 *)(unsigned long)gop_handle;
+	int i;
+
+	first_gop = NULL;
+	gop64 = NULL;
+
+	nr_gops = size / sizeof(u64);
+	for (i = 0; i < nr_gops; i++) {
+		struct efi_graphics_output_mode_info *info = NULL;
+		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
+		bool conout_found = false;
+		void *dummy = NULL;
+		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
+		u64 current_fb_base;
+
+		status = efi_call_early(handle_protocol, h,
+					proto, (void **)&gop64);
+		if (status != EFI_SUCCESS)
+			continue;
+
+		status = efi_call_early(handle_protocol, h,
+					&conout_proto, &dummy);
+		if (status == EFI_SUCCESS)
+			conout_found = true;
+
+		status = __gop_query64(sys_table_arg, gop64, &info, &size,
+				       &current_fb_base);
+		if (status == EFI_SUCCESS && (!first_gop || conout_found)) {
+			/*
+			 * Systems that use the UEFI Console Splitter may
+			 * provide multiple GOP devices, not all of which are
+			 * backed by real hardware. The workaround is to search
+			 * for a GOP implementing the ConOut protocol, and if
+			 * one isn't found, to just fall back to the first GOP.
+			 */
+			width = info->horizontal_resolution;
+			height = info->vertical_resolution;
+			pixel_format = info->pixel_format;
+			pixel_info = info->pixel_information;
+			pixels_per_scan_line = info->pixels_per_scan_line;
+			fb_base = current_fb_base;
+
+			/*
+			 * Once we've found a GOP supporting ConOut,
+			 * don't bother looking any further.
+			 */
+			first_gop = gop64;
+			if (conout_found)
+				break;
+		}
+	}
+
+	/* Did we find any GOPs? */
+	if (!first_gop)
+		goto out;
+
+	/* EFI framebuffer */
+	si->orig_video_isVGA = VIDEO_TYPE_EFI;
+
+	si->lfb_width = width;
+	si->lfb_height = height;
+	si->lfb_base = fb_base;
+
+	ext_lfb_base = (u64)(unsigned long)fb_base >> 32;
+	if (ext_lfb_base) {
+		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
+		si->ext_lfb_base = ext_lfb_base;
+	}
+
+	si->pages = 1;
+
+	setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
+
+	si->lfb_size = si->lfb_linelength * si->lfb_height;
+
+	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
+out:
+	return status;
+}
+
+/*
+ * See if we have Graphics Output Protocol
+ */
+efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
+			   struct screen_info *si, efi_guid_t *proto,
+			   unsigned long size)
+{
+	efi_status_t status;
+	void **gop_handle = NULL;
+
+	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
+				size, (void **)&gop_handle);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	status = efi_call_early(locate_handle,
+				EFI_LOCATE_BY_PROTOCOL,
+				proto, NULL, &size, gop_handle);
+	if (status != EFI_SUCCESS)
+		goto free_handle;
+
+	if (efi_is_64bit()) {
+		status = setup_gop64(sys_table_arg, si, proto, size,
+				     gop_handle);
+	} else {
+		status = setup_gop32(sys_table_arg, si, proto, size,
+				     gop_handle);
+	}
+
+free_handle:
+	efi_call_early(free_pool, gop_handle);
+	return status;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8ba6eb7863aa..fe9e728aa083 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -21,6 +21,7 @@
 #include <linux/pfn.h>
 #include <linux/pstore.h>
 #include <linux/reboot.h>
+#include <linux/screen_info.h>
 
 #include <asm/page.h>
 
@@ -43,7 +44,6 @@ typedef u16 efi_char16_t;		/* UNICODE character */
 typedef u64 efi_physical_addr_t;
 typedef void *efi_handle_t;
 
-
 typedef struct {
 	u8 b[16];
 } efi_guid_t;
@@ -281,7 +281,8 @@ typedef struct {
 	efi_status_t (*handle_protocol)(efi_handle_t, efi_guid_t *, void **);
 	void *__reserved;
 	void *register_protocol_notify;
-	void *locate_handle;
+	efi_status_t (*locate_handle)(int, efi_guid_t *, void *,
+				      unsigned long *, efi_handle_t *);
 	void *locate_device_path;
 	efi_status_t (*install_configuration_table)(efi_guid_t *, void *);
 	void *load_image;
@@ -603,6 +604,10 @@ void efi_native_runtime_setup(void);
 	EFI_GUID(0x3152bca5, 0xeade, 0x433d, \
 		 0x86, 0x2e, 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
 
+#define EFI_CONSOLE_OUT_DEVICE_GUID \
+	EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4, \
+		 0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;
@@ -1155,6 +1160,80 @@ struct efi_simple_text_output_protocol {
 	void *test_string;
 };
 
+#define PIXEL_RGB_RESERVED_8BIT_PER_COLOR		0
+#define PIXEL_BGR_RESERVED_8BIT_PER_COLOR		1
+#define PIXEL_BIT_MASK					2
+#define PIXEL_BLT_ONLY					3
+#define PIXEL_FORMAT_MAX				4
+
+struct efi_pixel_bitmask {
+	u32 red_mask;
+	u32 green_mask;
+	u32 blue_mask;
+	u32 reserved_mask;
+};
+
+struct efi_graphics_output_mode_info {
+	u32 version;
+	u32 horizontal_resolution;
+	u32 vertical_resolution;
+	int pixel_format;
+	struct efi_pixel_bitmask pixel_information;
+	u32 pixels_per_scan_line;
+} __packed;
+
+struct efi_graphics_output_protocol_mode_32 {
+	u32 max_mode;
+	u32 mode;
+	u32 info;
+	u32 size_of_info;
+	u64 frame_buffer_base;
+	u32 frame_buffer_size;
+} __packed;
+
+struct efi_graphics_output_protocol_mode_64 {
+	u32 max_mode;
+	u32 mode;
+	u64 info;
+	u64 size_of_info;
+	u64 frame_buffer_base;
+	u64 frame_buffer_size;
+} __packed;
+
+struct efi_graphics_output_protocol_mode {
+	u32 max_mode;
+	u32 mode;
+	unsigned long info;
+	unsigned long size_of_info;
+	u64 frame_buffer_base;
+	unsigned long frame_buffer_size;
+} __packed;
+
+struct efi_graphics_output_protocol_32 {
+	u32 query_mode;
+	u32 set_mode;
+	u32 blt;
+	u32 mode;
+};
+
+struct efi_graphics_output_protocol_64 {
+	u64 query_mode;
+	u64 set_mode;
+	u64 blt;
+	u64 mode;
+};
+
+struct efi_graphics_output_protocol {
+	unsigned long query_mode;
+	unsigned long set_mode;
+	unsigned long blt;
+	struct efi_graphics_output_protocol_mode *mode;
+};
+
+typedef efi_status_t (*efi_graphics_output_protocol_query_mode)(
+	struct efi_graphics_output_protocol *, u32, unsigned long *,
+	struct efi_graphics_output_mode_info **);
+
 extern struct list_head efivar_sysfs_list;
 
 static inline void
@@ -1291,5 +1370,9 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
 
 efi_status_t efi_parse_options(char *cmdline);
 
+efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
+			   struct screen_info *si, efi_guid_t *proto,
+			   unsigned long size);
+
 bool efi_runtime_disabled(void);
 #endif /* _LINUX_EFI_H */
-- 
1.9.1

Re: [PATCH 2/8] efi: libstub: move Graphics Output Protocol handling to generic code

[Matt Fleming]

On Thu, 10 Mar, at 12:40:02PM, Ard Biesheuvel wrote:
> The Graphics Output Protocol code executes in the stub, so create a generic
> version based on the x86 version in libstub so that we can move all archs
> to it in subsequent patches. The new source file gop.c is added to the
> libstub build for all architectures, but is not actually included in any of
> the final images, since this patch does not wire it up yet.
> 
> Note that the generic version uses slightly different ways of casting the
> protocol methods and some other variables to the correct types, since such
> method calls are not loosely typed on ARM and arm64 as they are on x86.
 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/include/asm/efi.h            |   4 +-
>  arch/arm64/include/asm/efi.h          |   4 +-
>  arch/x86/boot/compressed/eboot.h      |  74 ----
>  arch/x86/include/asm/efi.h            |   5 +
>  drivers/firmware/efi/libstub/Makefile |   2 +-
>  drivers/firmware/efi/libstub/gop.c    | 354 ++++++++++++++++++++
>  include/linux/efi.h                   |  87 ++++-
>  7 files changed, 451 insertions(+), 79 deletions(-)
 
I can totally appreciate why you did the gop code move in stages, but
I'd prefer to see the move (deletion from arch/x86 and addition to
drivers/firmware/efi) done as a single patch.

The reason is that git will produce a very nice diff stat that shows
the number of lines that were simply moved from one file to another
without modification.

Which makes it much easier to see which lines *did* change.

As a very quick and dirty example (but this is not what I'm suggesting
you do because it'd be too many changes in one patch) I merged this
patch with the next one, which produces the following diff stat. The
important point are the insertions,

 : $ git show --shortstat --summary -C10% e63c02a98c3c
 :
 : 8 files changed, 133 insertions(+), 1606 deletions(-)
 :  copy arch/x86/boot/compressed/eboot.c => drivers/firmware/efi/libstub/gop.c (20%)

> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index e0eea72deb87..daebcfe6c35a 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -59,7 +59,9 @@ void efi_virtmap_unload(void);
>  
>  /* arch specific definitions used by the stub code */
>  
> -#define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
> +#define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
> +#define __efi_call_early(f, ...)	f(__VA_ARGS__)
> +#define efi_is_64bit()			(0)
  
Is there a specific reason that these can't be boolean? I know some
people hate them, but I've always been a pretty big fan when the
circumstances are right. Plus it is bool on x86. 

[PATCH 3/8] efi/x86: libstub: move to generic GOP code

[Ard Biesheuvel]

Switch to the generic Graphics Output Protocol handling in libstub, and
remove the now unused code from arch/x86/boot/compressed/eboot.c

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/x86/boot/compressed/eboot.c | 308 +-------------------
 1 file changed, 1 insertion(+), 307 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 583d539a4197..52fef606bc54 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -571,312 +571,6 @@ free_handle:
 	efi_call_early(free_pool, pci_handle);
 }
 
-static void
-setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line,
-		 struct efi_pixel_bitmask pixel_info, int pixel_format)
-{
-	if (pixel_format == PIXEL_RGB_RESERVED_8BIT_PER_COLOR) {
-		si->lfb_depth = 32;
-		si->lfb_linelength = pixels_per_scan_line * 4;
-		si->red_size = 8;
-		si->red_pos = 0;
-		si->green_size = 8;
-		si->green_pos = 8;
-		si->blue_size = 8;
-		si->blue_pos = 16;
-		si->rsvd_size = 8;
-		si->rsvd_pos = 24;
-	} else if (pixel_format == PIXEL_BGR_RESERVED_8BIT_PER_COLOR) {
-		si->lfb_depth = 32;
-		si->lfb_linelength = pixels_per_scan_line * 4;
-		si->red_size = 8;
-		si->red_pos = 16;
-		si->green_size = 8;
-		si->green_pos = 8;
-		si->blue_size = 8;
-		si->blue_pos = 0;
-		si->rsvd_size = 8;
-		si->rsvd_pos = 24;
-	} else if (pixel_format == PIXEL_BIT_MASK) {
-		find_bits(pixel_info.red_mask, &si->red_pos, &si->red_size);
-		find_bits(pixel_info.green_mask, &si->green_pos,
-			  &si->green_size);
-		find_bits(pixel_info.blue_mask, &si->blue_pos, &si->blue_size);
-		find_bits(pixel_info.reserved_mask, &si->rsvd_pos,
-			  &si->rsvd_size);
-		si->lfb_depth = si->red_size + si->green_size +
-			si->blue_size + si->rsvd_size;
-		si->lfb_linelength = (pixels_per_scan_line * si->lfb_depth) / 8;
-	} else {
-		si->lfb_depth = 4;
-		si->lfb_linelength = si->lfb_width / 2;
-		si->red_size = 0;
-		si->red_pos = 0;
-		si->green_size = 0;
-		si->green_pos = 0;
-		si->blue_size = 0;
-		si->blue_pos = 0;
-		si->rsvd_size = 0;
-		si->rsvd_pos = 0;
-	}
-}
-
-static efi_status_t
-__gop_query32(struct efi_graphics_output_protocol_32 *gop32,
-	      struct efi_graphics_output_mode_info **info,
-	      unsigned long *size, u64 *fb_base)
-{
-	struct efi_graphics_output_protocol_mode_32 *mode;
-	efi_status_t status;
-	unsigned long m;
-
-	m = gop32->mode;
-	mode = (struct efi_graphics_output_protocol_mode_32 *)m;
-
-	status = efi_early->call(gop32->query_mode, gop32,
-				 mode->mode, size, info);
-	if (status != EFI_SUCCESS)
-		return status;
-
-	*fb_base = mode->frame_buffer_base;
-	return status;
-}
-
-static efi_status_t
-setup_gop32(struct screen_info *si, efi_guid_t *proto,
-	    unsigned long size, void **gop_handle)
-{
-	struct efi_graphics_output_protocol_32 *gop32, *first_gop;
-	unsigned long nr_gops;
-	u16 width, height;
-	u32 pixels_per_scan_line;
-	u32 ext_lfb_base;
-	u64 fb_base;
-	struct efi_pixel_bitmask pixel_info;
-	int pixel_format;
-	efi_status_t status;
-	u32 *handles = (u32 *)(unsigned long)gop_handle;
-	int i;
-
-	first_gop = NULL;
-	gop32 = NULL;
-
-	nr_gops = size / sizeof(u32);
-	for (i = 0; i < nr_gops; i++) {
-		struct efi_graphics_output_mode_info *info = NULL;
-		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
-		bool conout_found = false;
-		void *dummy = NULL;
-		u32 h = handles[i];
-		u64 current_fb_base;
-
-		status = efi_call_early(handle_protocol, h,
-					proto, (void **)&gop32);
-		if (status != EFI_SUCCESS)
-			continue;
-
-		status = efi_call_early(handle_protocol, h,
-					&conout_proto, &dummy);
-		if (status == EFI_SUCCESS)
-			conout_found = true;
-
-		status = __gop_query32(gop32, &info, &size, &current_fb_base);
-		if (status == EFI_SUCCESS && (!first_gop || conout_found)) {
-			/*
-			 * Systems that use the UEFI Console Splitter may
-			 * provide multiple GOP devices, not all of which are
-			 * backed by real hardware. The workaround is to search
-			 * for a GOP implementing the ConOut protocol, and if
-			 * one isn't found, to just fall back to the first GOP.
-			 */
-			width = info->horizontal_resolution;
-			height = info->vertical_resolution;
-			pixel_format = info->pixel_format;
-			pixel_info = info->pixel_information;
-			pixels_per_scan_line = info->pixels_per_scan_line;
-			fb_base = current_fb_base;
-
-			/*
-			 * Once we've found a GOP supporting ConOut,
-			 * don't bother looking any further.
-			 */
-			first_gop = gop32;
-			if (conout_found)
-				break;
-		}
-	}
-
-	/* Did we find any GOPs? */
-	if (!first_gop)
-		goto out;
-
-	/* EFI framebuffer */
-	si->orig_video_isVGA = VIDEO_TYPE_EFI;
-
-	si->lfb_width = width;
-	si->lfb_height = height;
-	si->lfb_base = fb_base;
-
-	ext_lfb_base = (u64)(unsigned long)fb_base >> 32;
-	if (ext_lfb_base) {
-		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
-		si->ext_lfb_base = ext_lfb_base;
-	}
-
-	si->pages = 1;
-
-	setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
-
-	si->lfb_size = si->lfb_linelength * si->lfb_height;
-
-	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
-out:
-	return status;
-}
-
-static efi_status_t
-__gop_query64(struct efi_graphics_output_protocol_64 *gop64,
-	      struct efi_graphics_output_mode_info **info,
-	      unsigned long *size, u64 *fb_base)
-{
-	struct efi_graphics_output_protocol_mode_64 *mode;
-	efi_status_t status;
-	unsigned long m;
-
-	m = gop64->mode;
-	mode = (struct efi_graphics_output_protocol_mode_64 *)m;
-
-	status = efi_early->call(gop64->query_mode, gop64,
-				 mode->mode, size, info);
-	if (status != EFI_SUCCESS)
-		return status;
-
-	*fb_base = mode->frame_buffer_base;
-	return status;
-}
-
-static efi_status_t
-setup_gop64(struct screen_info *si, efi_guid_t *proto,
-	    unsigned long size, void **gop_handle)
-{
-	struct efi_graphics_output_protocol_64 *gop64, *first_gop;
-	unsigned long nr_gops;
-	u16 width, height;
-	u32 pixels_per_scan_line;
-	u32 ext_lfb_base;
-	u64 fb_base;
-	struct efi_pixel_bitmask pixel_info;
-	int pixel_format;
-	efi_status_t status;
-	u64 *handles = (u64 *)(unsigned long)gop_handle;
-	int i;
-
-	first_gop = NULL;
-	gop64 = NULL;
-
-	nr_gops = size / sizeof(u64);
-	for (i = 0; i < nr_gops; i++) {
-		struct efi_graphics_output_mode_info *info = NULL;
-		efi_guid_t conout_proto = EFI_CONSOLE_OUT_DEVICE_GUID;
-		bool conout_found = false;
-		void *dummy = NULL;
-		u64 h = handles[i];
-		u64 current_fb_base;
-
-		status = efi_call_early(handle_protocol, h,
-					proto, (void **)&gop64);
-		if (status != EFI_SUCCESS)
-			continue;
-
-		status = efi_call_early(handle_protocol, h,
-					&conout_proto, &dummy);
-		if (status == EFI_SUCCESS)
-			conout_found = true;
-
-		status = __gop_query64(gop64, &info, &size, &current_fb_base);
-		if (status == EFI_SUCCESS && (!first_gop || conout_found)) {
-			/*
-			 * Systems that use the UEFI Console Splitter may
-			 * provide multiple GOP devices, not all of which are
-			 * backed by real hardware. The workaround is to search
-			 * for a GOP implementing the ConOut protocol, and if
-			 * one isn't found, to just fall back to the first GOP.
-			 */
-			width = info->horizontal_resolution;
-			height = info->vertical_resolution;
-			pixel_format = info->pixel_format;
-			pixel_info = info->pixel_information;
-			pixels_per_scan_line = info->pixels_per_scan_line;
-			fb_base = current_fb_base;
-
-			/*
-			 * Once we've found a GOP supporting ConOut,
-			 * don't bother looking any further.
-			 */
-			first_gop = gop64;
-			if (conout_found)
-				break;
-		}
-	}
-
-	/* Did we find any GOPs? */
-	if (!first_gop)
-		goto out;
-
-	/* EFI framebuffer */
-	si->orig_video_isVGA = VIDEO_TYPE_EFI;
-
-	si->lfb_width = width;
-	si->lfb_height = height;
-	si->lfb_base = fb_base;
-
-	ext_lfb_base = (u64)(unsigned long)fb_base >> 32;
-	if (ext_lfb_base) {
-		si->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
-		si->ext_lfb_base = ext_lfb_base;
-	}
-
-	si->pages = 1;
-
-	setup_pixel_info(si, pixels_per_scan_line, pixel_info, pixel_format);
-
-	si->lfb_size = si->lfb_linelength * si->lfb_height;
-
-	si->capabilities |= VIDEO_CAPABILITY_SKIP_QUIRKS;
-out:
-	return status;
-}
-
-/*
- * See if we have Graphics Output Protocol
- */
-static efi_status_t setup_gop(struct screen_info *si, efi_guid_t *proto,
-			      unsigned long size)
-{
-	efi_status_t status;
-	void **gop_handle = NULL;
-
-	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
-				size, (void **)&gop_handle);
-	if (status != EFI_SUCCESS)
-		return status;
-
-	status = efi_call_early(locate_handle,
-				EFI_LOCATE_BY_PROTOCOL,
-				proto, NULL, &size, gop_handle);
-	if (status != EFI_SUCCESS)
-		goto free_handle;
-
-	if (efi_early->is64)
-		status = setup_gop64(si, proto, size, gop_handle);
-	else
-		status = setup_gop32(si, proto, size, gop_handle);
-
-free_handle:
-	efi_call_early(free_pool, gop_handle);
-	return status;
-}
-
 static efi_status_t
 setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 {
@@ -1038,7 +732,7 @@ void setup_graphics(struct boot_params *boot_params)
 				EFI_LOCATE_BY_PROTOCOL,
 				&graphics_proto, NULL, &size, gop_handle);
 	if (status == EFI_BUFFER_TOO_SMALL)
-		status = setup_gop(si, &graphics_proto, size);
+		status = efi_setup_gop(NULL, si, &graphics_proto, size);
 
 	if (status != EFI_SUCCESS) {
 		size = 0;
-- 
1.9.1

[PATCH 4/8] efi/x86: efifb: move DMI based quirks handling out of generic code

[Ard Biesheuvel]

The efifb quirks handling based on DMI identification of the platform is
specific to x86, so move it to x86 arch code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/x86/include/asm/efi.h  |  2 ++
 arch/x86/kernel/sysfb_efi.c | 15 +++++++++++++++
 drivers/video/fbdev/efifb.c | 15 ++++-----------
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 7d5187b66108..790adbc3a64a 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -154,6 +154,8 @@ static inline bool efi_runtime_supported(void)
 extern struct console early_efi_console;
 extern void parse_efi_setup(u64 phys_addr, u32 data_len);
 
+extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
+
 #ifdef CONFIG_EFI_MIXED
 extern void efi_thunk_runtime_setup(void);
 extern efi_status_t efi_thunk_set_virtual_address_map(
diff --git a/arch/x86/kernel/sysfb_efi.c b/arch/x86/kernel/sysfb_efi.c
index b285d4e8c68e..e21a8a7ddcff 100644
--- a/arch/x86/kernel/sysfb_efi.c
+++ b/arch/x86/kernel/sysfb_efi.c
@@ -68,6 +68,21 @@ struct efifb_dmi_info efifb_dmi_list[] = {
 	[M_UNKNOWN] = { NULL, 0, 0, 0, 0, OVERRIDE_NONE }
 };
 
+void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
+{
+	int i;
+
+	for (i = 0; i < M_UNKNOWN; i++) {
+		if (efifb_dmi_list[i].base != 0 &&
+		    !strcmp(opt, efifb_dmi_list[i].optname)) {
+			si->lfb_base = efifb_dmi_list[i].base;
+			si->lfb_linelength = efifb_dmi_list[i].stride;
+			si->lfb_width = efifb_dmi_list[i].width;
+			si->lfb_height = efifb_dmi_list[i].height;
+		}
+	}
+}
+
 #define choose_value(dmivalue, fwvalue, field, flags) ({	\
 		typeof(fwvalue) _ret_ = fwvalue;		\
 		if ((flags) & (field))				\
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 95d293b7445a..dd594369b8a6 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -8,6 +8,7 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/efi.h>
 #include <linux/errno.h>
 #include <linux/fb.h>
 #include <linux/platform_device.h>
@@ -15,7 +16,7 @@
 #include <linux/dmi.h>
 #include <linux/pci.h>
 #include <video/vga.h>
-#include <asm/sysfb.h>
+#include <asm/efi.h>
 
 static bool request_mem_succeeded = false;
 
@@ -85,21 +86,13 @@ static struct fb_ops efifb_ops = {
 static int efifb_setup(char *options)
 {
 	char *this_opt;
-	int i;
 
 	if (options && *options) {
 		while ((this_opt = strsep(&options, ",")) != NULL) {
 			if (!*this_opt) continue;
 
-			for (i = 0; i < M_UNKNOWN; i++) {
-				if (efifb_dmi_list[i].base != 0 &&
-				    !strcmp(this_opt, efifb_dmi_list[i].optname)) {
-					screen_info.lfb_base = efifb_dmi_list[i].base;
-					screen_info.lfb_linelength = efifb_dmi_list[i].stride;
-					screen_info.lfb_width = efifb_dmi_list[i].width;
-					screen_info.lfb_height = efifb_dmi_list[i].height;
-				}
-			}
+			efifb_setup_from_dmi(&screen_info, this_opt);
+
 			if (!strncmp(this_opt, "base:", 5))
 				screen_info.lfb_base = simple_strtoul(this_opt+5, NULL, 0);
 			else if (!strncmp(this_opt, "stride:", 7))
-- 
1.9.1

Re: [PATCH 4/8] efi/x86: efifb: move DMI based quirks handling out of generic code

[Matt Fleming]

(Cc'ing efifb maintainer and sysfb_efi.c author)

On Thu, 10 Mar, at 12:40:04PM, Ard Biesheuvel wrote:
> The efifb quirks handling based on DMI identification of the platform is
> specific to x86, so move it to x86 arch code.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/x86/include/asm/efi.h  |  2 ++
>  arch/x86/kernel/sysfb_efi.c | 15 +++++++++++++++
>  drivers/video/fbdev/efifb.c | 15 ++++-----------
>  3 files changed, 21 insertions(+), 11 deletions(-)
 
I've left the patch content intact below. This looks like a worthwhile
change to me. David, Peter, any comments?

> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 7d5187b66108..790adbc3a64a 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -154,6 +154,8 @@ static inline bool efi_runtime_supported(void)
>  extern struct console early_efi_console;
>  extern void parse_efi_setup(u64 phys_addr, u32 data_len);
>  
> +extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
> +
>  #ifdef CONFIG_EFI_MIXED
>  extern void efi_thunk_runtime_setup(void);
>  extern efi_status_t efi_thunk_set_virtual_address_map(
> diff --git a/arch/x86/kernel/sysfb_efi.c b/arch/x86/kernel/sysfb_efi.c
> index b285d4e8c68e..e21a8a7ddcff 100644
> --- a/arch/x86/kernel/sysfb_efi.c
> +++ b/arch/x86/kernel/sysfb_efi.c
> @@ -68,6 +68,21 @@ struct efifb_dmi_info efifb_dmi_list[] = {
>  	[M_UNKNOWN] = { NULL, 0, 0, 0, 0, OVERRIDE_NONE }
>  };
>  
> +void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
> +{
> +	int i;
> +
> +	for (i = 0; i < M_UNKNOWN; i++) {
> +		if (efifb_dmi_list[i].base != 0 &&
> +		    !strcmp(opt, efifb_dmi_list[i].optname)) {
> +			si->lfb_base = efifb_dmi_list[i].base;
> +			si->lfb_linelength = efifb_dmi_list[i].stride;
> +			si->lfb_width = efifb_dmi_list[i].width;
> +			si->lfb_height = efifb_dmi_list[i].height;
> +		}
> +	}
> +}
> +
>  #define choose_value(dmivalue, fwvalue, field, flags) ({	\
>  		typeof(fwvalue) _ret_ = fwvalue;		\
>  		if ((flags) & (field))				\
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 95d293b7445a..dd594369b8a6 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/efi.h>
>  #include <linux/errno.h>
>  #include <linux/fb.h>
>  #include <linux/platform_device.h>
> @@ -15,7 +16,7 @@
>  #include <linux/dmi.h>
>  #include <linux/pci.h>
>  #include <video/vga.h>
> -#include <asm/sysfb.h>
> +#include <asm/efi.h>
>  
>  static bool request_mem_succeeded = false;
>  
> @@ -85,21 +86,13 @@ static struct fb_ops efifb_ops = {
>  static int efifb_setup(char *options)
>  {
>  	char *this_opt;
> -	int i;
>  
>  	if (options && *options) {
>  		while ((this_opt = strsep(&options, ",")) != NULL) {
>  			if (!*this_opt) continue;
>  
> -			for (i = 0; i < M_UNKNOWN; i++) {
> -				if (efifb_dmi_list[i].base != 0 &&
> -				    !strcmp(this_opt, efifb_dmi_list[i].optname)) {
> -					screen_info.lfb_base = efifb_dmi_list[i].base;
> -					screen_info.lfb_linelength = efifb_dmi_list[i].stride;
> -					screen_info.lfb_width = efifb_dmi_list[i].width;
> -					screen_info.lfb_height = efifb_dmi_list[i].height;
> -				}
> -			}
> +			efifb_setup_from_dmi(&screen_info, this_opt);
> +
>  			if (!strncmp(this_opt, "base:", 5))
>  				screen_info.lfb_base = simple_strtoul(this_opt+5, NULL, 0);
>  			else if (!strncmp(this_opt, "stride:", 7))
> -- 
> 1.9.1
> 

Re: [PATCH 4/8] efi/x86: efifb: move DMI based quirks handling out of generic code

[Peter Jones]

On Fri, Mar 18, 2016 at 10:50:07AM +0000, Matt Fleming wrote:
> (Cc'ing efifb maintainer and sysfb_efi.c author)
> 
> On Thu, 10 Mar, at 12:40:04PM, Ard Biesheuvel wrote:
> > The efifb quirks handling based on DMI identification of the platform is
> > specific to x86, so move it to x86 arch code.
> > 
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  arch/x86/include/asm/efi.h  |  2 ++
> >  arch/x86/kernel/sysfb_efi.c | 15 +++++++++++++++
> >  drivers/video/fbdev/efifb.c | 15 ++++-----------
> >  3 files changed, 21 insertions(+), 11 deletions(-)
>  
> I've left the patch content intact below. This looks like a worthwhile
> change to me. David, Peter, any comments?

It looks right to me as well:

Acked-By: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> 
> > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> > index 7d5187b66108..790adbc3a64a 100644
> > --- a/arch/x86/include/asm/efi.h
> > +++ b/arch/x86/include/asm/efi.h
> > @@ -154,6 +154,8 @@ static inline bool efi_runtime_supported(void)
> >  extern struct console early_efi_console;
> >  extern void parse_efi_setup(u64 phys_addr, u32 data_len);
> >  
> > +extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
> > +
> >  #ifdef CONFIG_EFI_MIXED
> >  extern void efi_thunk_runtime_setup(void);
> >  extern efi_status_t efi_thunk_set_virtual_address_map(
> > diff --git a/arch/x86/kernel/sysfb_efi.c b/arch/x86/kernel/sysfb_efi.c
> > index b285d4e8c68e..e21a8a7ddcff 100644
> > --- a/arch/x86/kernel/sysfb_efi.c
> > +++ b/arch/x86/kernel/sysfb_efi.c
> > @@ -68,6 +68,21 @@ struct efifb_dmi_info efifb_dmi_list[] = {
> >  	[M_UNKNOWN] = { NULL, 0, 0, 0, 0, OVERRIDE_NONE }
> >  };
> >  
> > +void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < M_UNKNOWN; i++) {
> > +		if (efifb_dmi_list[i].base != 0 &&
> > +		    !strcmp(opt, efifb_dmi_list[i].optname)) {
> > +			si->lfb_base = efifb_dmi_list[i].base;
> > +			si->lfb_linelength = efifb_dmi_list[i].stride;
> > +			si->lfb_width = efifb_dmi_list[i].width;
> > +			si->lfb_height = efifb_dmi_list[i].height;
> > +		}
> > +	}
> > +}
> > +
> >  #define choose_value(dmivalue, fwvalue, field, flags) ({	\
> >  		typeof(fwvalue) _ret_ = fwvalue;		\
> >  		if ((flags) & (field))				\
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 95d293b7445a..dd594369b8a6 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -8,6 +8,7 @@
> >  
> >  #include <linux/module.h>
> >  #include <linux/kernel.h>
> > +#include <linux/efi.h>
> >  #include <linux/errno.h>
> >  #include <linux/fb.h>
> >  #include <linux/platform_device.h>
> > @@ -15,7 +16,7 @@
> >  #include <linux/dmi.h>
> >  #include <linux/pci.h>
> >  #include <video/vga.h>
> > -#include <asm/sysfb.h>
> > +#include <asm/efi.h>
> >  
> >  static bool request_mem_succeeded = false;
> >  
> > @@ -85,21 +86,13 @@ static struct fb_ops efifb_ops = {
> >  static int efifb_setup(char *options)
> >  {
> >  	char *this_opt;
> > -	int i;
> >  
> >  	if (options && *options) {
> >  		while ((this_opt = strsep(&options, ",")) != NULL) {
> >  			if (!*this_opt) continue;
> >  
> > -			for (i = 0; i < M_UNKNOWN; i++) {
> > -				if (efifb_dmi_list[i].base != 0 &&
> > -				    !strcmp(this_opt, efifb_dmi_list[i].optname)) {
> > -					screen_info.lfb_base = efifb_dmi_list[i].base;
> > -					screen_info.lfb_linelength = efifb_dmi_list[i].stride;
> > -					screen_info.lfb_width = efifb_dmi_list[i].width;
> > -					screen_info.lfb_height = efifb_dmi_list[i].height;
> > -				}
> > -			}
> > +			efifb_setup_from_dmi(&screen_info, this_opt);
> > +
> >  			if (!strncmp(this_opt, "base:", 5))
> >  				screen_info.lfb_base = simple_strtoul(this_opt+5, NULL, 0);
> >  			else if (!strncmp(this_opt, "stride:", 7))
> > -- 
> > 1.9.1
> > 

-- 
        Peter

[PATCH 5/8] efi: efifb: use builtin_platform_driver and drop unused includes

[Ard Biesheuvel]

Since efifb can only be built directly into the kernel, drop the module
specific includes and definitions. Drop some other includes we don't need
as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/video/fbdev/efifb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index dd594369b8a6..f4c045c0051c 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -6,15 +6,12 @@
  *
  */
 
-#include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/efi.h>
 #include <linux/errno.h>
 #include <linux/fb.h>
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
-#include <linux/dmi.h>
-#include <linux/pci.h>
 #include <video/vga.h>
 #include <asm/efi.h>
 
@@ -331,5 +328,4 @@ static struct platform_driver efifb_driver = {
 	.remove = efifb_remove,
 };
 
-module_platform_driver(efifb_driver);
-MODULE_LICENSE("GPL");
+builtin_platform_driver(efifb_driver);
-- 
1.9.1

Re: [PATCH 5/8] efi: efifb: use builtin_platform_driver and drop unused includes

[Matt Fleming]

Cc'ing Peter.

On Thu, 10 Mar, at 12:40:05PM, Ard Biesheuvel wrote:
> Since efifb can only be built directly into the kernel, drop the module
> specific includes and definitions. Drop some other includes we don't need
> as well.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/video/fbdev/efifb.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index dd594369b8a6..f4c045c0051c 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -6,15 +6,12 @@
>   *
>   */
>  
> -#include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/efi.h>
>  #include <linux/errno.h>
>  #include <linux/fb.h>
>  #include <linux/platform_device.h>
>  #include <linux/screen_info.h>
> -#include <linux/dmi.h>
> -#include <linux/pci.h>
>  #include <video/vga.h>
>  #include <asm/efi.h>
>  
> @@ -331,5 +328,4 @@ static struct platform_driver efifb_driver = {
>  	.remove = efifb_remove,
>  };
>  
> -module_platform_driver(efifb_driver);
> -MODULE_LICENSE("GPL");
> +builtin_platform_driver(efifb_driver);
> -- 
> 1.9.1
> 

Re: [PATCH 5/8] efi: efifb: use builtin_platform_driver and drop unused includes

[Peter Jones]

On Thu, Mar 10, 2016 at 12:40:05PM +0700, Ard Biesheuvel wrote:
> Since efifb can only be built directly into the kernel, drop the module
> specific includes and definitions. Drop some other includes we don't need
> as well.

Yeah; this didn't exist when this code was initially written, and it
just never got converted.  Thanks.

Acked-By: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/video/fbdev/efifb.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index dd594369b8a6..f4c045c0051c 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -6,15 +6,12 @@
>   *
>   */
>  
> -#include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/efi.h>
>  #include <linux/errno.h>
>  #include <linux/fb.h>
>  #include <linux/platform_device.h>
>  #include <linux/screen_info.h>
> -#include <linux/dmi.h>
> -#include <linux/pci.h>
>  #include <video/vga.h>
>  #include <asm/efi.h>
>  
> @@ -331,5 +328,4 @@ static struct platform_driver efifb_driver = {
>  	.remove = efifb_remove,
>  };
>  
> -module_platform_driver(efifb_driver);
> -MODULE_LICENSE("GPL");
> +builtin_platform_driver(efifb_driver);
> -- 
> 1.9.1
> 

-- 
        Peter

[PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ard Biesheuvel]

This enables the EFI stub side of handling the EFI Graphics Output
Protocol. It involves invoking the generic GOP code to discover the GOP
handle that also drives the console, and using its metadata to populate
the screen_info structure.

For arm64, the stub and the kernel proper live in the same executable, so
we can simply access screen_info directly.

For ARM, we need to allocate a struct screen_info in the stub, and install
it as a configuration table so that the core kernel can go and fetch it to
copy its contents into the real screen_info structure.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/include/asm/efi.h                | 12 +++++++
 arch/arm/kernel/setup.c                   |  3 +-
 arch/arm64/include/asm/efi.h              |  3 ++
 arch/arm64/kernel/efi.c                   |  3 ++
 arch/arm64/kernel/image.h                 |  1 +
 drivers/firmware/efi/libstub/arm-stub.c   | 23 ++++++++++++
 drivers/firmware/efi/libstub/arm32-stub.c | 38 ++++++++++++++++++++
 7 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index daebcfe6c35a..6329b5be1eca 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -63,6 +63,18 @@ void efi_virtmap_unload(void);
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
 #define efi_is_64bit()			(0)
 
+struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg);
+void free_screen_info(efi_system_table_t *sys_table, struct screen_info *si);
+
+/*
+ * This GUID is used to pass to the kernel proper the struct screen_info
+ * structure that was populated by the stub based on the GOP protocol instance
+ * associated with ConOut
+ */
+#define LINUX_ARM32_SCREEN_INFO_TABLE_GUID \
+	EFI_GUID(0xe03fc20a, 0x85dc, 0x406e, \
+		 0xb9, 0xe, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95)
+
 /*
  * A reasonable upper bound for the uncompressed kernel size is 32 MBytes,
  * so we will reserve that amount of memory. We have no easy way to tell what
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 7d0cba6f1cc5..149a7787b40f 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -881,7 +881,8 @@ static void __init request_standard_resources(const struct machine_desc *mdesc)
 		request_resource(&ioport_resource, &lp2);
 }
 
-#if defined(CONFIG_VGA_CONSOLE) || defined(CONFIG_DUMMY_CONSOLE)
+#if defined(CONFIG_VGA_CONSOLE) || defined(CONFIG_DUMMY_CONSOLE) || \
+    defined(CONFIG_EFI)
 struct screen_info screen_info = {
  .orig_video_lines	= 30,
  .orig_video_cols	= 80,
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index e3d707131bba..a6c14f29b970 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -54,6 +54,9 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
 #define efi_is_64bit()			(1)
 
+#define alloc_screen_info(x...)		&screen_info
+#define free_screen_info(x...)
+
 #define EFI_ALLOC_ALIGN		SZ_64K
 
 /*
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index b6abc852f2a1..6f4b3ea359db 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -17,6 +17,9 @@
 
 #include <asm/efi.h>
 
+/* we will fill this structure from the stub, so don't put it in .bss */
+struct screen_info screen_info __section(.data);
+
 int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 {
 	pteval_t prot_val;
diff --git a/arch/arm64/kernel/image.h b/arch/arm64/kernel/image.h
index 5ff892f40a0a..fa52ebdfc93e 100644
--- a/arch/arm64/kernel/image.h
+++ b/arch/arm64/kernel/image.h
@@ -115,6 +115,7 @@ __efistub___memset		= KALLSYMS_HIDE(__pi_memset);
 __efistub__text			= KALLSYMS_HIDE(_text);
 __efistub__end			= KALLSYMS_HIDE(_end);
 __efistub__edata		= KALLSYMS_HIDE(_edata);
+__efistub_screen_info		= KALLSYMS_HIDE(screen_info);
 
 #endif
 
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 4deb3e7faa0e..94bd51cfb71e 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -147,6 +147,25 @@ void efi_char16_printk(efi_system_table_t *sys_table_arg,
 	out->output_string(out, str);
 }
 
+static struct screen_info *setup_graphics(efi_system_table_t *sys_table_arg)
+{
+	efi_guid_t gop_proto = EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID;
+	efi_status_t status;
+	unsigned long size;
+	void **gop_handle = NULL;
+	struct screen_info *si = NULL;
+
+	size = 0;
+	status = efi_call_early(locate_handle, EFI_LOCATE_BY_PROTOCOL,
+				&gop_proto, NULL, &size, gop_handle);
+	if (status == EFI_BUFFER_TOO_SMALL) {
+		si = alloc_screen_info(sys_table_arg);
+		if (!si)
+			return NULL;
+		efi_setup_gop(sys_table_arg, si, &gop_proto, size);
+	}
+	return si;
+}
 
 /*
  * This function handles the architcture specific differences between arm and
@@ -185,6 +204,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
 	unsigned long reserve_size = 0;
+	struct screen_info *si;
 
 	/* Check if we were booted by the EFI firmware */
 	if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
@@ -233,6 +253,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 			__nokaslr = true;
 	}
 
+	si = setup_graphics(sys_table);
+
 	status = handle_kernel_image(sys_table, image_addr, &image_size,
 				     &reserve_addr,
 				     &reserve_size,
@@ -305,6 +327,7 @@ fail_free_image:
 	efi_free(sys_table, image_size, *image_addr);
 	efi_free(sys_table, reserve_size, reserve_addr);
 fail_free_cmdline:
+	free_screen_info(sys_table, si);
 	efi_free(sys_table, cmdline_size, (unsigned long)cmdline_ptr);
 fail:
 	return EFI_ERROR;
diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
index 495ebd657e38..0fbe00f13186 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -9,6 +9,44 @@
 #include <linux/efi.h>
 #include <asm/efi.h>
 
+static const efi_guid_t screen_info_guid = LINUX_ARM32_SCREEN_INFO_TABLE_GUID;
+
+struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg)
+{
+	struct screen_info *si;
+	efi_status_t status;
+
+	/*
+	 * Unlike on arm64, where we can directly fill out the screen_info
+	 * structure from the stub, we need to allocate a buffer to hold
+	 * its contents while we hand over to the kernel proper from the
+	 * decompressor.
+	 */
+	status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
+				sizeof(*si), (void **)&si);
+
+	if (status != EFI_SUCCESS)
+		return NULL;
+
+	status = efi_call_early(install_configuration_table,
+				(efi_guid_t *)&screen_info_guid, si);
+	if (status == EFI_SUCCESS)
+		return si;
+
+	efi_call_early(free_pool, si);
+	return NULL;
+}
+
+void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si)
+{
+	if (!si)
+		return;
+
+	efi_call_early(install_configuration_table,
+		       (efi_guid_t *)&screen_info_guid, NULL);
+	efi_call_early(free_pool, si);
+}
+
 efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
 				 unsigned long *image_addr,
 				 unsigned long *image_size,
-- 
1.9.1

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
> index 495ebd657e38..0fbe00f13186 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -9,6 +9,44 @@
>  #include <linux/efi.h>
>  #include <asm/efi.h>
>  
> +static const efi_guid_t screen_info_guid = LINUX_ARM32_SCREEN_INFO_TABLE_GUID;
> +
> +struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg)
> +{
> +	struct screen_info *si;
> +	efi_status_t status;
> +
> +	/*
> +	 * Unlike on arm64, where we can directly fill out the screen_info
> +	 * structure from the stub, we need to allocate a buffer to hold
> +	 * its contents while we hand over to the kernel proper from the
> +	 * decompressor.
> +	 */
> +	status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> +				sizeof(*si), (void **)&si);
> +
> +	if (status != EFI_SUCCESS)
> +		return NULL;
> +
> +	status = efi_call_early(install_configuration_table,
> +				(efi_guid_t *)&screen_info_guid, si);
> +	if (status == EFI_SUCCESS)
> +		return si;
> +
> +	efi_call_early(free_pool, si);
> +	return NULL;
> +}
> +
> +void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si)
> +{
> +	if (!si)
> +		return;
> +
> +	efi_call_early(install_configuration_table,
> +		       (efi_guid_t *)&screen_info_guid, NULL);
> +	efi_call_early(free_pool, si);
> +}
> +
>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>  				 unsigned long *image_addr,
>  				 unsigned long *image_size,

So screen_info_guid should probably not be a 'const': you have to cast it away 
anyway, adding artificial linebreaks and uglifying the code. It's also a bad 
practice to cast away const-ness, it hinders move-consts-to-readonly-sections 
efforts.

Otherwise the series looks mostly good to me. Time for new v4.6 merges is short, 
but if Matt acks it I can still take it into tip:efi/core.

Thanks,

	Ingo

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ard Biesheuvel]

On 10 March 2016 at 15:25, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
>> index 495ebd657e38..0fbe00f13186 100644
>> --- a/drivers/firmware/efi/libstub/arm32-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
>> @@ -9,6 +9,44 @@
>>  #include <linux/efi.h>
>>  #include <asm/efi.h>
>>
>> +static const efi_guid_t screen_info_guid = LINUX_ARM32_SCREEN_INFO_TABLE_GUID;
>> +
>> +struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg)
>> +{
>> +     struct screen_info *si;
>> +     efi_status_t status;
>> +
>> +     /*
>> +      * Unlike on arm64, where we can directly fill out the screen_info
>> +      * structure from the stub, we need to allocate a buffer to hold
>> +      * its contents while we hand over to the kernel proper from the
>> +      * decompressor.
>> +      */
>> +     status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
>> +                             sizeof(*si), (void **)&si);
>> +
>> +     if (status != EFI_SUCCESS)
>> +             return NULL;
>> +
>> +     status = efi_call_early(install_configuration_table,
>> +                             (efi_guid_t *)&screen_info_guid, si);
>> +     if (status == EFI_SUCCESS)
>> +             return si;
>> +
>> +     efi_call_early(free_pool, si);
>> +     return NULL;
>> +}
>> +
>> +void free_screen_info(efi_system_table_t *sys_table_arg, struct screen_info *si)
>> +{
>> +     if (!si)
>> +             return;
>> +
>> +     efi_call_early(install_configuration_table,
>> +                    (efi_guid_t *)&screen_info_guid, NULL);
>> +     efi_call_early(free_pool, si);
>> +}
>> +
>>  efi_status_t handle_kernel_image(efi_system_table_t *sys_table,
>>                                unsigned long *image_addr,
>>                                unsigned long *image_size,
>
> So screen_info_guid should probably not be a 'const': you have to cast it away
> anyway, adding artificial linebreaks and uglifying the code. It's also a bad
> practice to cast away const-ness, it hinders move-consts-to-readonly-sections
> efforts.
>

The problem here is that the UEFI spec never uses const qualifiers in
its APIs for by-ref parameters that are obviously never modified by
the caller, such as these GUIDs. This is __init code regardless
(either due to the fact that it is linked into the stub rather than
the core kernel, or because we __init-ize it using objcopy) so I don't
care deeply, and I am happy to remove it.

> Otherwise the series looks mostly good to me. Time for new v4.6 merges is short,
> but if Matt acks it I can still take it into tip:efi/core.
>

Thank you. I am perfectly happy exercising some more caution here,
though, considering that our past experiences involving reshuffling
stub code for x86 were not a huge success (and broke Linus's laptop,
among others')

I was very careful not to introduce cross-object data symbol
references (which were the culprit before), and I tested both i386 and
x86_64 to the extent feasible to me, but we'd need to confirm that
those things are still fully correct before queuing this.

Thanks,
Ard.

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> > So screen_info_guid should probably not be a 'const': you have to cast it away
> > anyway, adding artificial linebreaks and uglifying the code. It's also a bad
> > practice to cast away const-ness, it hinders move-consts-to-readonly-sections
> > efforts.
> 
> The problem here is that the UEFI spec never uses const qualifiers in
> its APIs for by-ref parameters that are obviously never modified by
> the caller, such as these GUIDs. [...]

Ah, ok. Two related thoughts came up:

1)

While I was looking at this code and was asking myself why the EFI runtime is 
generally invoked via a relatively fragile, non-type-checking vararg construct.

Wouldn't you be better off by explicitly defining all the API variants, and then 
internally calling the EFI runtime?

That would neatly solve such const artifacts as well.

So instead of:


+       status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
+                               sizeof(*si), (void **)&si);

we could have something like:

	status = efi_early__allocate_pool(EFI_RUNTIME_SERVICES_DATA, sizeof(*si), &si);

	...

	efi_early__free_pool(si);


i.e. it would look a lot more like a properly distributed, typed, structured 
family of C function APIs, instead of this single central bastard of an ioctl() 
interface.

There's over 100 invocations of the EFI runtime in the Linux kernel, I think it 
would be worth the effort. The wrapper inlines should be mostly trivial.

That would also add an opportunity to actually document most of these calls.

2)

Another suggestion: would it make sense to unify the 'EFI' and 'EFI early' calls - 
is there any deep reason why they are invoked via separate names? Why not use a 
single namespace:

	efi__allocate_pool()
	efi__free_pool()

and have a 'current EFI configuration' pointer internaly that can be switched from 
the early to the later variant during bootup. The various typed API wrappers would 
use this pointer.

Thanks,

	Ingo

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ard Biesheuvel]

On 10 March 2016 at 16:03, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> > So screen_info_guid should probably not be a 'const': you have to cast it away
>> > anyway, adding artificial linebreaks and uglifying the code. It's also a bad
>> > practice to cast away const-ness, it hinders move-consts-to-readonly-sections
>> > efforts.
>>
>> The problem here is that the UEFI spec never uses const qualifiers in
>> its APIs for by-ref parameters that are obviously never modified by
>> the caller, such as these GUIDs. [...]
>
> Ah, ok. Two related thoughts came up:
>
> 1)
>
> While I was looking at this code and was asking myself why the EFI runtime is
> generally invoked via a relatively fragile, non-type-checking vararg construct.
>
> Wouldn't you be better off by explicitly defining all the API variants, and then
> internally calling the EFI runtime?
>
> That would neatly solve such const artifacts as well.
>
> So instead of:
>
>
> +       status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> +                               sizeof(*si), (void **)&si);
>
> we could have something like:
>
>         status = efi_early__allocate_pool(EFI_RUNTIME_SERVICES_DATA, sizeof(*si), &si);
>
>         ...
>
>         efi_early__free_pool(si);
>
>
> i.e. it would look a lot more like a properly distributed, typed, structured
> family of C function APIs, instead of this single central bastard of an ioctl()
> interface.
>
> There's over 100 invocations of the EFI runtime in the Linux kernel, I think it
> would be worth the effort. The wrapper inlines should be mostly trivial.
>
> That would also add an opportunity to actually document most of these calls.
>

If only. The ARM and arm64 wrappers actually simply resolve to
correctly typed calls. While I am not an expert on the x86 side of
things, I think the vararg stuff is needed to be able to perform the
thunking required to do function calls using the MS ABI for x86_64.

As far as documentation is concerned, all of these functions and
protocol methods are documented in the UEFI spec, which is freely
accessible. It may be somewhat redundant to have our own documentation
for them.

> 2)
>
> Another suggestion: would it make sense to unify the 'EFI' and 'EFI early' calls -
> is there any deep reason why they are invoked via separate names? Why not use a
> single namespace:
>
>         efi__allocate_pool()
>         efi__free_pool()
>
> and have a 'current EFI configuration' pointer internaly that can be switched from
> the early to the later variant during bootup. The various typed API wrappers would
> use this pointer.
>

Actually, having statically initialized pointer variables is more of a
concern. The nice thing about plain function calls is that they can
usually be resolved at link time to relative branches on most
architectures, rather than require GOT fixups or other magic to be
able to execute in the UEFI environment.

Thanks,
Ard.

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 10 March 2016 at 16:03, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> >> > So screen_info_guid should probably not be a 'const': you have to cast it away
> >> > anyway, adding artificial linebreaks and uglifying the code. It's also a bad
> >> > practice to cast away const-ness, it hinders move-consts-to-readonly-sections
> >> > efforts.
> >>
> >> The problem here is that the UEFI spec never uses const qualifiers in
> >> its APIs for by-ref parameters that are obviously never modified by
> >> the caller, such as these GUIDs. [...]
> >
> > Ah, ok. Two related thoughts came up:
> >
> > 1)
> >
> > While I was looking at this code and was asking myself why the EFI runtime is
> > generally invoked via a relatively fragile, non-type-checking vararg construct.
> >
> > Wouldn't you be better off by explicitly defining all the API variants, and then
> > internally calling the EFI runtime?
> >
> > That would neatly solve such const artifacts as well.
> >
> > So instead of:
> >
> >
> > +       status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
> > +                               sizeof(*si), (void **)&si);
> >
> > we could have something like:
> >
> >         status = efi_early__allocate_pool(EFI_RUNTIME_SERVICES_DATA, sizeof(*si), &si);
> >
> >         ...
> >
> >         efi_early__free_pool(si);
> >
> >
> > i.e. it would look a lot more like a properly distributed, typed, structured
> > family of C function APIs, instead of this single central bastard of an ioctl()
> > interface.
> >
> > There's over 100 invocations of the EFI runtime in the Linux kernel, I think it
> > would be worth the effort. The wrapper inlines should be mostly trivial.
> >
> > That would also add an opportunity to actually document most of these calls.
> >
> 
> If only. The ARM and arm64 wrappers actually simply resolve to
> correctly typed calls. While I am not an expert on the x86 side of
> things, I think the vararg stuff is needed to be able to perform the
> thunking required to do function calls using the MS ABI for x86_64.

So at least the efi_early calls seem to just be using a function pointer:

  arch/x86/include/asm/efi.h:     __efi_early()->call(__efi_early()->f, __VA_ARGS__);

Furthermore, my suggestion would work with arbitrarily structured thunking: my 
suggestion was to put a typed C layer in there - and the layer itself could then 
call the vararg construct internally. It's a C type demuxing, only a syntactic 
effort, it does not change any real call signature.

> As far as documentation is concerned, all of these functions and
> protocol methods are documented in the UEFI spec, which is freely
> accessible. It may be somewhat redundant to have our own documentation
> for them.

I mean a properly split up typed C interface 'documents itself' to a large degree 
- while with opaque varargs bugs can slip in more easily.

This is really an obvious argument ... it's the well known 'why ioctl()s are bad' 
API argument.

It has no bearing on this series obviously, it's a cleanup suggestion that could 
be done separately.

Thanks,

	Ingo

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Ard Biesheuvel]

On 10 March 2016 at 16:25, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> On 10 March 2016 at 16:03, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> >
>> > * Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> >
>> >> > So screen_info_guid should probably not be a 'const': you have to cast it away
>> >> > anyway, adding artificial linebreaks and uglifying the code. It's also a bad
>> >> > practice to cast away const-ness, it hinders move-consts-to-readonly-sections
>> >> > efforts.
>> >>
>> >> The problem here is that the UEFI spec never uses const qualifiers in
>> >> its APIs for by-ref parameters that are obviously never modified by
>> >> the caller, such as these GUIDs. [...]
>> >
>> > Ah, ok. Two related thoughts came up:
>> >
>> > 1)
>> >
>> > While I was looking at this code and was asking myself why the EFI runtime is
>> > generally invoked via a relatively fragile, non-type-checking vararg construct.
>> >
>> > Wouldn't you be better off by explicitly defining all the API variants, and then
>> > internally calling the EFI runtime?
>> >
>> > That would neatly solve such const artifacts as well.
>> >
>> > So instead of:
>> >
>> >
>> > +       status = efi_call_early(allocate_pool, EFI_RUNTIME_SERVICES_DATA,
>> > +                               sizeof(*si), (void **)&si);
>> >
>> > we could have something like:
>> >
>> >         status = efi_early__allocate_pool(EFI_RUNTIME_SERVICES_DATA, sizeof(*si), &si);
>> >
>> >         ...
>> >
>> >         efi_early__free_pool(si);
>> >
>> >
>> > i.e. it would look a lot more like a properly distributed, typed, structured
>> > family of C function APIs, instead of this single central bastard of an ioctl()
>> > interface.
>> >
>> > There's over 100 invocations of the EFI runtime in the Linux kernel, I think it
>> > would be worth the effort. The wrapper inlines should be mostly trivial.
>> >
>> > That would also add an opportunity to actually document most of these calls.
>> >
>>
>> If only. The ARM and arm64 wrappers actually simply resolve to
>> correctly typed calls. While I am not an expert on the x86 side of
>> things, I think the vararg stuff is needed to be able to perform the
>> thunking required to do function calls using the MS ABI for x86_64.
>
> So at least the efi_early calls seem to just be using a function pointer:
>
>   arch/x86/include/asm/efi.h:     __efi_early()->call(__efi_early()->f, __VA_ARGS__);
>

Indeed. I think the efi_early struct is a separate structure that is
set up by the early boot code, and has a number of function pointers
of EFI boot services copied into it. Matt should know the details.

> Furthermore, my suggestion would work with arbitrarily structured thunking: my
> suggestion was to put a typed C layer in there - and the layer itself could then
> call the vararg construct internally. It's a C type demuxing, only a syntactic
> effort, it does not change any real call signature.
>

I would welcome any improvement in this regard. Happy to contribute as well.

>> As far as documentation is concerned, all of these functions and
>> protocol methods are documented in the UEFI spec, which is freely
>> accessible. It may be somewhat redundant to have our own documentation
>> for them.
>
> I mean a properly split up typed C interface 'documents itself' to a large degree
> - while with opaque varargs bugs can slip in more easily.
>
> This is really an obvious argument ... it's the well known 'why ioctl()s are bad'
> API argument.
>
> It has no bearing on this series obviously, it's a cleanup suggestion that could
> be done separately.
>

I will take this up with Matt. There is lots of room for improvement
and consolidation between x86 on the one hand and ARM/arm64 on the
other hand in other ways as well.

Thanks,
Ard.

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Matt Fleming]

On Thu, 10 Mar, at 05:25:08PM, Ard Biesheuvel wrote:
> 
> Indeed. I think the efi_early struct is a separate structure that is
> set up by the early boot code, and has a number of function pointers
> of EFI boot services copied into it. Matt should know the details.
 
If you look at where the efi_early stuff came from originally, you'll
see it was merged as part of the mixed mode code for running 64-bit
kernels on 32-bit EFI,

commit 54b52d872680
Author: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date:   Fri Jan 10 15:27:14 2014 +0000

    x86/efi: Build our own EFI services pointer table
    
    It's not possible to dereference the EFI System table directly when
    booting a 64-bit kernel on a 32-bit EFI firmware because the size of
    pointers don't match.
    
    In preparation for supporting the above use case, build a list of
    function pointers on boot so that callers don't have to worry about
    converting pointer sizes through multiple levels of indirection.
    
    Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>


You basically cannot ever dereference a pointer in that mode, it's
crazy.
 
The subsequent efi_early() wrapper came when some of the EFI boot stub
code was migrated out of arch/x86 into drivers/firmware/efi/libstub.

I'm not suggesting it's the most beautiful code in the world, far from
it. It is a massive eyesore that has resulted in type bugs in the past
because of all the casting.

I've just never come up with a cleanup patch series that I was happy
with.

> > Furthermore, my suggestion would work with arbitrarily structured thunking: my
> > suggestion was to put a typed C layer in there - and the layer itself could then
> > call the vararg construct internally. It's a C type demuxing, only a syntactic
> > effort, it does not change any real call signature.
> >
> 
> I would welcome any improvement in this regard. Happy to contribute as well.
 
The only way I think a typed C layer would be maintainable is if it
was automatically generated. We're already suffering from the
multitude of different bitness functions, i.e. __setup_efi_pci32() vs
__setup_efi_pci64().

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Matt Fleming]

On Thu, 10 Mar, at 03:36:24PM, Ard Biesheuvel wrote:
> 
> Thank you. I am perfectly happy exercising some more caution here,
> though, considering that our past experiences involving reshuffling
> stub code for x86 were not a huge success (and broke Linus's laptop,
> among others')
> 
> I was very careful not to introduce cross-object data symbol
> references (which were the culprit before), and I tested both i386 and
> x86_64 to the extent feasible to me, but we'd need to confirm that
> those things are still fully correct before queuing this.

I agree 100% with Ard. It's far too late in the development cycle to
be introducing cross-architecture changes, and in particular, changes
to the code driving the early boot console.

I haven't reviewed the patches yet, but this is all v4.7 material.

Re: [PATCH 6/8] efi/arm*: libstub: wire up GOP handling into the ARM UEFI stub

[Matt Fleming]

On Thu, 10 Mar, at 12:40:06PM, Ard Biesheuvel wrote:
> This enables the EFI stub side of handling the EFI Graphics Output
> Protocol. It involves invoking the generic GOP code to discover the GOP
> handle that also drives the console, and using its metadata to populate
> the screen_info structure.
> 
> For arm64, the stub and the kernel proper live in the same executable, so
> we can simply access screen_info directly.
> 
> For ARM, we need to allocate a struct screen_info in the stub, and install
> it as a configuration table so that the core kernel can go and fetch it to
> copy its contents into the real screen_info structure.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/include/asm/efi.h                | 12 +++++++
>  arch/arm/kernel/setup.c                   |  3 +-
>  arch/arm64/include/asm/efi.h              |  3 ++
>  arch/arm64/kernel/efi.c                   |  3 ++
>  arch/arm64/kernel/image.h                 |  1 +
>  drivers/firmware/efi/libstub/arm-stub.c   | 23 ++++++++++++
>  drivers/firmware/efi/libstub/arm32-stub.c | 38 ++++++++++++++++++++
>  7 files changed, 82 insertions(+), 1 deletion(-)
 
I'd really like to see the ARM and arm64-specific parts split into
separate patches, and then the ARM part merged with patch 8.

The main reason I'm being so strict about this is because the ARM use
of InstallConfigurationTable() should be a shining example of how to
use that scheme for any future developers.

So that if someone wants to use it to pass other data we can say:
"Look at how we did it for ARM and screen_info. Use that as your
template." 

Of course, if we do get more users we need to think about providing a
real API around InstallConfigurationTable(), but that's for
future-Matt and future-Ard to worry about.

> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index daebcfe6c35a..6329b5be1eca 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -63,6 +63,18 @@ void efi_virtmap_unload(void);
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
>  #define efi_is_64bit()			(0)
>  
> +struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg);
> +void free_screen_info(efi_system_table_t *sys_table, struct screen_info *si);
> +
> +/*
> + * This GUID is used to pass to the kernel proper the struct screen_info
> + * structure that was populated by the stub based on the GOP protocol instance
> + * associated with ConOut
> + */
> +#define LINUX_ARM32_SCREEN_INFO_TABLE_GUID \
> +	EFI_GUID(0xe03fc20a, 0x85dc, 0x406e, \
> +		 0xb9, 0xe, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95)
> +
>  /*
>   * A reasonable upper bound for the uncompressed kernel size is 32 MBytes,
>   * so we will reserve that amount of memory. We have no easy way to tell what

This should go into include/linux/efi.h. We already have precedent for
Linux-specific GUIDs there. See LINUX_EFI_CRASH_GUID.

[PATCH 7/8] efi/arm*: efifb: expose efifb platform device if GOP is available

[Ard Biesheuvel]

This allows the efifb driver to be built for ARM and arm64, and adds the
registration of a "efi-framebuffer" platform device if the GOP code in the
stub has populated screen_info appropriately.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/include/asm/efi.h      |  4 ++++
 arch/arm64/include/asm/efi.h    |  4 ++++
 drivers/firmware/efi/arm-init.c | 19 +++++++++++++++++++
 drivers/video/fbdev/Kconfig     |  2 +-
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 6329b5be1eca..9b6d441b08fd 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -66,6 +66,10 @@ void efi_virtmap_unload(void);
 struct screen_info *alloc_screen_info(efi_system_table_t *sys_table_arg);
 void free_screen_info(efi_system_table_t *sys_table, struct screen_info *si);
 
+static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
+{
+}
+
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
  * structure that was populated by the stub based on the GOP protocol instance
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index a6c14f29b970..00e9d7099a8f 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -57,6 +57,10 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 #define alloc_screen_info(x...)		&screen_info
 #define free_screen_info(x...)
 
+static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
+{
+}
+
 #define EFI_ALLOC_ALIGN		SZ_64K
 
 /*
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 9e15d571b53c..eca9b4f826ee 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -17,6 +17,8 @@
 #include <linux/mm_types.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
+#include <linux/platform_device.h>
+#include <linux/screen_info.h>
 
 #include <asm/efi.h>
 
@@ -206,4 +208,21 @@ void __init efi_init(void)
 	memblock_mark_nomap(params.mmap & PAGE_MASK,
 			    PAGE_ALIGN(params.mmap_size +
 				       (params.mmap & ~PAGE_MASK)));
+
+	if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI &&
+	    memblock_is_map_memory(screen_info.lfb_base))
+		memblock_mark_nomap(screen_info.lfb_base, screen_info.lfb_size);
+}
+
+static int __init register_gop_device(void)
+{
+	void *pd;
+
+	if (screen_info.orig_video_isVGA != VIDEO_TYPE_EFI)
+		return 0;
+
+	/* the efifb driver accesses screen_info directly, no need to pass it */
+	pd = platform_device_register_simple("efi-framebuffer", 0, NULL, 0);
+	return PTR_ERR_OR_ZERO(pd);
 }
+subsys_initcall(register_gop_device);
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 8ea45a5cd806..ccbaa25aad76 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -761,7 +761,7 @@ config FB_VESA
 
 config FB_EFI
 	bool "EFI-based Framebuffer Support"
-	depends on (FB = y) && X86 && EFI
+	depends on (FB = y) && !IA64 && EFI
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
-- 
1.9.1

[PATCH 8/8] efi/arm: populate screen_info based on data provided by the UEFI stub

[Ard Biesheuvel]

Unlike on arm64, where we can simply access the screen_info struct directly,
ARM requires an intermediate step to get the information discovered by the
GOP code in the UEFI stub into the screen_info struct.

So retrieve the dedicated config table we invented for this purpose, and
put its contents into the core kernel's copy of struct screen_info.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/kernel/efi.c           | 24 ++++++++++++++++++++
 drivers/firmware/efi/arm-init.c |  6 +++++
 2 files changed, 30 insertions(+)

diff --git a/arch/arm/kernel/efi.c b/arch/arm/kernel/efi.c
index ff8a9d8acfac..8d1119259654 100644
--- a/arch/arm/kernel/efi.c
+++ b/arch/arm/kernel/efi.c
@@ -6,11 +6,35 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt)     "efi: " fmt
+
 #include <linux/efi.h>
+#include <linux/screen_info.h>
 #include <asm/efi.h>
 #include <asm/mach/map.h>
 #include <asm/mmu_context.h>
 
+static const efi_guid_t __initconst si_tbl = LINUX_ARM32_SCREEN_INFO_TABLE_GUID;
+
+void __init efi_find_screen_info(efi_config_table_t *tbl, u32 num_tables)
+{
+	struct screen_info *si;
+
+	while (num_tables--) {
+		if (efi_guidcmp(tbl->guid, si_tbl) == 0) {
+			si = early_memremap_ro(tbl->table, sizeof(*si));
+			if (!si) {
+				pr_warn("Failed to remap screen_info config table\n");
+				return;
+			}
+			screen_info = *si;
+			early_memunmap(si, sizeof(*si));
+			break;
+		}
+		tbl++;
+	}
+}
+
 int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
 {
 	struct map_desc desc = {
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index eca9b4f826ee..689bf65f8ddf 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -112,6 +112,12 @@ static int __init uefi_init(void)
 	retval = efi_config_parse_tables(config_tables, efi.systab->nr_tables,
 					 sizeof(efi_config_table_t), NULL);
 
+	if (IS_ENABLED(CONFIG_ARM)) {
+		extern void efi_find_screen_info(efi_config_table_t *, u32);
+
+		efi_find_screen_info(config_tables, efi.systab->nr_tables);
+	}
+
 	early_memunmap(config_tables, table_size);
 out:
 	early_memunmap(efi.systab,  sizeof(efi_system_table_t));
-- 
1.9.1

Re: [PATCH 8/8] efi/arm: populate screen_info based on data provided by the UEFI stub

[Matt Fleming]

On Thu, 10 Mar, at 12:40:08PM, Ard Biesheuvel wrote:
> Unlike on arm64, where we can simply access the screen_info struct directly,
> ARM requires an intermediate step to get the information discovered by the
> GOP code in the UEFI stub into the screen_info struct.
> 
> So retrieve the dedicated config table we invented for this purpose, and
> put its contents into the core kernel's copy of struct screen_info.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/kernel/efi.c           | 24 ++++++++++++++++++++
>  drivers/firmware/efi/arm-init.c |  6 +++++
>  2 files changed, 30 insertions(+)
 
[...]

> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
> index eca9b4f826ee..689bf65f8ddf 100644
> --- a/drivers/firmware/efi/arm-init.c
> +++ b/drivers/firmware/efi/arm-init.c
> @@ -112,6 +112,12 @@ static int __init uefi_init(void)
>  	retval = efi_config_parse_tables(config_tables, efi.systab->nr_tables,
>  					 sizeof(efi_config_table_t), NULL);
>  
> +	if (IS_ENABLED(CONFIG_ARM)) {
> +		extern void efi_find_screen_info(efi_config_table_t *, u32);
> +
> +		efi_find_screen_info(config_tables, efi.systab->nr_tables);
> +	}
> +
>  	early_memunmap(config_tables, table_size);
>  out:
>  	early_memunmap(efi.systab,  sizeof(efi_system_table_t));

It's unfortunate that this is the first example of putting
arch-specific things in arm-init.c. Is there anyway we can move it out
into arch/arm?

If not, because efi_find_screen_info() uses all standard data types
and function calls, I wouldn't be opposed to just sticking this in
efi_config_parse_tables() like we do for EFI_PROPERTIES_TABLE. It's
certainly nice to have all config table code in one place.

Also, it would save ARM the effort of looping over the config tables
twice.

Re: [PATCH 8/8] efi/arm: populate screen_info based on data provided by the UEFI stub

[Ard Biesheuvel]

On 18 March 2016 at 12:53, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Thu, 10 Mar, at 12:40:08PM, Ard Biesheuvel wrote:
>> Unlike on arm64, where we can simply access the screen_info struct directly,
>> ARM requires an intermediate step to get the information discovered by the
>> GOP code in the UEFI stub into the screen_info struct.
>>
>> So retrieve the dedicated config table we invented for this purpose, and
>> put its contents into the core kernel's copy of struct screen_info.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  arch/arm/kernel/efi.c           | 24 ++++++++++++++++++++
>>  drivers/firmware/efi/arm-init.c |  6 +++++
>>  2 files changed, 30 insertions(+)
>
> [...]
>
>> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
>> index eca9b4f826ee..689bf65f8ddf 100644
>> --- a/drivers/firmware/efi/arm-init.c
>> +++ b/drivers/firmware/efi/arm-init.c
>> @@ -112,6 +112,12 @@ static int __init uefi_init(void)
>>       retval = efi_config_parse_tables(config_tables, efi.systab->nr_tables,
>>                                        sizeof(efi_config_table_t), NULL);
>>
>> +     if (IS_ENABLED(CONFIG_ARM)) {
>> +             extern void efi_find_screen_info(efi_config_table_t *, u32);
>> +
>> +             efi_find_screen_info(config_tables, efi.systab->nr_tables);
>> +     }
>> +
>>       early_memunmap(config_tables, table_size);
>>  out:
>>       early_memunmap(efi.systab,  sizeof(efi_system_table_t));
>
> It's unfortunate that this is the first example of putting
> arch-specific things in arm-init.c. Is there anyway we can move it out
> into arch/arm?
>
> If not, because efi_find_screen_info() uses all standard data types
> and function calls, I wouldn't be opposed to just sticking this in
> efi_config_parse_tables() like we do for EFI_PROPERTIES_TABLE. It's
> certainly nice to have all config table code in one place.
>
> Also, it would save ARM the effort of looping over the config tables
> twice.

I'd actually prefer that, especially since -at some point- we may
expose the stub-to-kernel interface as an ABI, for things like Xen
dom0 that expose a 'sanitized' EFI environment, and don't boot the
dom0 kernel via the stub. There, this config table would be the only
way to populate screen_info, and the fact that arm64 *may* do it
directly becomes an implementation detail of the stub.

So in v2, I will rename the GUID to drop the ARMxx part, and move its
handling into the generic code.

-- 
Ard.