[PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Lukas Wunner]

Use LocateHandleBuffer instead of the LocateHandle + AllocatePool +
LocateHandle combo to save a bit on code.

Usage of LocateHandle for UGA and GOP has been present in the efistub
since its introduction with commit 291f36325f9f ("x86, efi: EFI boot
stub support"). It was subsequently inherited by the PCI ROM retrieval
with commit dd5fc854de5f ("EFI: Stash ROMs if they're not in the PCI
BAR").

One valid reason to not use LocateHandleBuffer would be if a specific
memory type is required for the buffer, and particularly if the buffer
needs to persist across ExitBootServices. The spec does not mandate
which type is used, edk2 seems to default to EfiBootServicesData.
In the three use cases modified by this commit (UGA, GOP, PCI), the
buffer is freed before ExitBootServices is called. Hence the memory
type is irrelevant.

No functional change intended.

Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---

The patch can be reviewed somewhat more comfortably with green/red
highlighting on GitHub:
https://github.com/l1k/linux/commit/2ff1fc10c84f

It depends on two patches which are already queued in Matt's tree for 4.9:

"x86/efi: Optimize away setup_gop32/64 if unused"
https://patchwork.kernel.org/patch/9315763/

"x86/efi: Allow invocation of arbitrary boot services"
https://patchwork.kernel.org/patch/9293371/

 arch/x86/boot/compressed/eboot.c        | 90 ++++++++++-----------------------
 drivers/firmware/efi/libstub/arm-stub.c | 17 ++++---
 drivers/firmware/efi/libstub/gop.c      | 29 +++--------
 include/linux/efi.h                     |  5 +-
 4 files changed, 47 insertions(+), 94 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 447a6a2..87c8d31 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -340,13 +340,12 @@ free_struct:
 
 static void
 setup_efi_pci32(struct boot_params *params, void **pci_handle,
-		unsigned long size)
+		unsigned long nr_pci)
 {
 	efi_pci_io_protocol_32 *pci = NULL;
 	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
 	u32 *handles = (u32 *)(unsigned long)pci_handle;
 	efi_status_t status;
-	unsigned long nr_pci;
 	struct setup_data *data;
 	int i;
 
@@ -355,7 +354,6 @@ setup_efi_pci32(struct boot_params *params, void **pci_handle,
 	while (data && data->next)
 		data = (struct setup_data *)(unsigned long)data->next;
 
-	nr_pci = size / sizeof(u32);
 	for (i = 0; i < nr_pci; i++) {
 		struct pci_setup_rom *rom = NULL;
 		u32 h = handles[i];
@@ -447,13 +445,12 @@ free_struct:
 
 static void
 setup_efi_pci64(struct boot_params *params, void **pci_handle,
-		unsigned long size)
+		unsigned long nr_pci)
 {
 	efi_pci_io_protocol_64 *pci = NULL;
 	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
 	u64 *handles = (u64 *)(unsigned long)pci_handle;
 	efi_status_t status;
-	unsigned long nr_pci;
 	struct setup_data *data;
 	int i;
 
@@ -462,7 +459,6 @@ setup_efi_pci64(struct boot_params *params, void **pci_handle,
 	while (data && data->next)
 		data = (struct setup_data *)(unsigned long)data->next;
 
-	nr_pci = size / sizeof(u64);
 	for (i = 0; i < nr_pci; i++) {
 		struct pci_setup_rom *rom = NULL;
 		u64 h = handles[i];
@@ -502,53 +498,38 @@ setup_efi_pci64(struct boot_params *params, void **pci_handle,
 static void setup_efi_pci(struct boot_params *params)
 {
 	efi_status_t status;
-	void **pci_handle = NULL;
+	void **pci_handle;
 	efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
-	unsigned long size = 0;
+	unsigned long nr_pci;
 
-	status = efi_call_early(locate_handle,
+	status = efi_call_early(locate_handle_buffer,
 				EFI_LOCATE_BY_PROTOCOL,
-				&pci_proto, NULL, &size, pci_handle);
+				&pci_proto, NULL, &nr_pci, &pci_handle);
 
-	if (status == EFI_BUFFER_TOO_SMALL) {
-		status = efi_call_early(allocate_pool,
-					EFI_LOADER_DATA,
-					size, (void **)&pci_handle);
-
-		if (status != EFI_SUCCESS) {
-			efi_printk(sys_table, "Failed to alloc mem for pci_handle\n");
-			return;
-		}
-
-		status = efi_call_early(locate_handle,
-					EFI_LOCATE_BY_PROTOCOL, &pci_proto,
-					NULL, &size, pci_handle);
-	}
+	if (status == EFI_OUT_OF_RESOURCES)
+		efi_printk(sys_table, "Failed to alloc mem for pci_handle\n");
 
 	if (status != EFI_SUCCESS)
-		goto free_handle;
+		return;
 
 	if (efi_early->is64)
-		setup_efi_pci64(params, pci_handle, size);
+		setup_efi_pci64(params, pci_handle, nr_pci);
 	else
-		setup_efi_pci32(params, pci_handle, size);
+		setup_efi_pci32(params, pci_handle, nr_pci);
 
-free_handle:
 	efi_call_early(free_pool, pci_handle);
 }
 
 static efi_status_t
-setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
+setup_uga32(void **uga_handle, unsigned long nr_ugas, u32 *width, u32 *height)
 {
 	struct efi_uga_draw_protocol *uga = NULL, *first_uga;
 	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
-	unsigned long nr_ugas;
 	u32 *handles = (u32 *)uga_handle;;
 	efi_status_t status = EFI_INVALID_PARAMETER;
 	int i;
 
 	first_uga = NULL;
-	nr_ugas = size / sizeof(u32);
 	for (i = 0; i < nr_ugas; i++) {
 		efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
 		u32 w, h, depth, refresh;
@@ -583,17 +564,15 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
 }
 
 static efi_status_t
-setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height)
+setup_uga64(void **uga_handle, unsigned long nr_ugas, u32 *width, u32 *height)
 {
 	struct efi_uga_draw_protocol *uga = NULL, *first_uga;
 	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
-	unsigned long nr_ugas;
 	u64 *handles = (u64 *)uga_handle;;
 	efi_status_t status = EFI_INVALID_PARAMETER;
 	int i;
 
 	first_uga = NULL;
-	nr_ugas = size / sizeof(u64);
 	for (i = 0; i < nr_ugas; i++) {
 		efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
 		u32 w, h, depth, refresh;
@@ -631,30 +610,18 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height)
  * See if we have Universal Graphics Adapter (UGA) protocol
  */
 static efi_status_t setup_uga(struct screen_info *si, efi_guid_t *uga_proto,
-			      unsigned long size)
+			      unsigned long nr_ugas, void **uga_handle)
 {
 	efi_status_t status;
 	u32 width, height;
-	void **uga_handle = NULL;
-
-	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
-				size, (void **)&uga_handle);
-	if (status != EFI_SUCCESS)
-		return status;
-
-	status = efi_call_early(locate_handle,
-				EFI_LOCATE_BY_PROTOCOL,
-				uga_proto, NULL, &size, uga_handle);
-	if (status != EFI_SUCCESS)
-		goto free_handle;
 
 	height = 0;
 	width = 0;
 
 	if (efi_early->is64)
-		status = setup_uga64(uga_handle, size, &width, &height);
+		status = setup_uga64(uga_handle, nr_ugas, &width, &height);
 	else
-		status = setup_uga32(uga_handle, size, &width, &height);
+		status = setup_uga32(uga_handle, nr_ugas, &width, &height);
 
 	if (!width && !height)
 		goto free_handle;
@@ -686,27 +653,26 @@ void setup_graphics(struct boot_params *boot_params)
 	struct screen_info *si;
 	efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
 	efi_status_t status;
-	unsigned long size;
-	void **gop_handle = NULL;
-	void **uga_handle = NULL;
+	unsigned long nr;
+	void **gop_handle;
+	void **uga_handle;
 
 	si = &boot_params->screen_info;
 	memset(si, 0, sizeof(*si));
 
-	size = 0;
-	status = efi_call_early(locate_handle,
+	status = efi_call_early(locate_handle_buffer,
 				EFI_LOCATE_BY_PROTOCOL,
-				&graphics_proto, NULL, &size, gop_handle);
-	if (status == EFI_BUFFER_TOO_SMALL)
-		status = efi_setup_gop(NULL, si, &graphics_proto, size);
+				&graphics_proto, NULL, &nr, &gop_handle);
+	if (status == EFI_SUCCESS)
+		status = efi_setup_gop(NULL, si, &graphics_proto, nr,
+				       gop_handle);
 
 	if (status != EFI_SUCCESS) {
-		size = 0;
-		status = efi_call_early(locate_handle,
+		status = efi_call_early(locate_handle_buffer,
 					EFI_LOCATE_BY_PROTOCOL,
-					&uga_proto, NULL, &size, uga_handle);
-		if (status == EFI_BUFFER_TOO_SMALL)
-			setup_uga(si, &uga_proto, size);
+					&uga_proto, NULL, &nr, &uga_handle);
+		if (status == EFI_SUCCESS)
+			setup_uga(si, &uga_proto, nr, uga_handle);
 	}
 }
 
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 993aa56..27fd62d 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -173,18 +173,19 @@ 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;
+	unsigned long nr;
+	void **gop_handle;
 	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) {
+	status = efi_call_early(locate_handle_buffer, EFI_LOCATE_BY_PROTOCOL,
+				&gop_proto, NULL, &nr, &gop_handle);
+	if (status == EFI_SUCCESS) {
 		si = alloc_screen_info(sys_table_arg);
-		if (!si)
+		if (!si) {
+			efi_call_early(free_pool, gop_handle);
 			return NULL;
-		efi_setup_gop(sys_table_arg, si, &gop_proto, size);
+		}
+		efi_setup_gop(sys_table_arg, si, &gop_proto, nr, gop_handle);
 	}
 	return si;
 }
diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
index 932742e..a2dca32 100644
--- a/drivers/firmware/efi/libstub/gop.c
+++ b/drivers/firmware/efi/libstub/gop.c
@@ -111,10 +111,9 @@ __gop_query32(efi_system_table_t *sys_table_arg,
 
 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)
+	    efi_guid_t *proto, unsigned long nr_gops, 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;
@@ -128,7 +127,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	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;
@@ -136,6 +134,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
 		void *dummy = NULL;
 		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
 		u64 current_fb_base;
+		unsigned long size;
 
 		status = efi_call_early(handle_protocol, h,
 					proto, (void **)&gop32);
@@ -228,10 +227,9 @@ __gop_query64(efi_system_table_t *sys_table_arg,
 
 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)
+	    efi_guid_t *proto, unsigned long nr_gops, 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;
@@ -245,7 +243,6 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 	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;
@@ -253,6 +250,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
 		void *dummy = NULL;
 		efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
 		u64 current_fb_base;
+		unsigned long size;
 
 		status = efi_call_early(handle_protocol, h,
 					proto, (void **)&gop64);
@@ -324,31 +322,18 @@ out:
  */
 efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
 			   struct screen_info *si, efi_guid_t *proto,
-			   unsigned long size)
+			   unsigned long nr_gops, void **gop_handle)
 {
 	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,
+		status = setup_gop64(sys_table_arg, si, proto, nr_gops,
 				     gop_handle);
 	} else {
-		status = setup_gop32(sys_table_arg, si, proto, size,
+		status = setup_gop32(sys_table_arg, si, proto, nr_gops,
 				     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 4ae1a1a..52fe49d 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -304,7 +304,8 @@ typedef struct {
 	void *close_protocol;
 	void *open_protocol_information;
 	void *protocols_per_handle;
-	void *locate_handle_buffer;
+	efi_status_t (*locate_handle_buffer)(int, efi_guid_t *, void *,
+					     unsigned long *, efi_handle_t **);
 	efi_status_t (*locate_protocol)(efi_guid_t *, void *, void **);
 	void *install_multiple_protocol_interfaces;
 	void *uninstall_multiple_protocol_interfaces;
@@ -1424,7 +1425,7 @@ 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);
+			   unsigned long nr_gops, void **gop_handle);
 
 bool efi_runtime_disabled(void);
 extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
-- 
2.9.3

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Ard Biesheuvel]

On 8 September 2016 at 01:02, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> Use LocateHandleBuffer instead of the LocateHandle + AllocatePool +
> LocateHandle combo to save a bit on code.
>
> Usage of LocateHandle for UGA and GOP has been present in the efistub
> since its introduction with commit 291f36325f9f ("x86, efi: EFI boot
> stub support"). It was subsequently inherited by the PCI ROM retrieval
> with commit dd5fc854de5f ("EFI: Stash ROMs if they're not in the PCI
> BAR").
>
> One valid reason to not use LocateHandleBuffer would be if a specific
> memory type is required for the buffer, and particularly if the buffer
> needs to persist across ExitBootServices. The spec does not mandate
> which type is used, edk2 seems to default to EfiBootServicesData.
> In the three use cases modified by this commit (UGA, GOP, PCI), the
> buffer is freed before ExitBootServices is called. Hence the memory
> type is irrelevant.
>
> No functional change intended.
>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
>

What is the oldest UEFI version we claim to support? For ARM, this is
not an issue, but it appears that LocateHandleBuffer () was introduced
in v1.10

> The patch can be reviewed somewhat more comfortably with green/red
> highlighting on GitHub:
> https://github.com/l1k/linux/commit/2ff1fc10c84f
>
> It depends on two patches which are already queued in Matt's tree for 4.9:
>
> "x86/efi: Optimize away setup_gop32/64 if unused"
> https://patchwork.kernel.org/patch/9315763/
>
> "x86/efi: Allow invocation of arbitrary boot services"
> https://patchwork.kernel.org/patch/9293371/
>
>  arch/x86/boot/compressed/eboot.c        | 90 ++++++++++-----------------------
>  drivers/firmware/efi/libstub/arm-stub.c | 17 ++++---
>  drivers/firmware/efi/libstub/gop.c      | 29 +++--------
>  include/linux/efi.h                     |  5 +-
>  4 files changed, 47 insertions(+), 94 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 447a6a2..87c8d31 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -340,13 +340,12 @@ free_struct:
>
>  static void
>  setup_efi_pci32(struct boot_params *params, void **pci_handle,
> -               unsigned long size)
> +               unsigned long nr_pci)
>  {
>         efi_pci_io_protocol_32 *pci = NULL;
>         efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
>         u32 *handles = (u32 *)(unsigned long)pci_handle;
>         efi_status_t status;
> -       unsigned long nr_pci;
>         struct setup_data *data;
>         int i;
>
> @@ -355,7 +354,6 @@ setup_efi_pci32(struct boot_params *params, void **pci_handle,
>         while (data && data->next)
>                 data = (struct setup_data *)(unsigned long)data->next;
>
> -       nr_pci = size / sizeof(u32);
>         for (i = 0; i < nr_pci; i++) {
>                 struct pci_setup_rom *rom = NULL;
>                 u32 h = handles[i];
> @@ -447,13 +445,12 @@ free_struct:
>
>  static void
>  setup_efi_pci64(struct boot_params *params, void **pci_handle,
> -               unsigned long size)
> +               unsigned long nr_pci)
>  {
>         efi_pci_io_protocol_64 *pci = NULL;
>         efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
>         u64 *handles = (u64 *)(unsigned long)pci_handle;
>         efi_status_t status;
> -       unsigned long nr_pci;
>         struct setup_data *data;
>         int i;
>
> @@ -462,7 +459,6 @@ setup_efi_pci64(struct boot_params *params, void **pci_handle,
>         while (data && data->next)
>                 data = (struct setup_data *)(unsigned long)data->next;
>
> -       nr_pci = size / sizeof(u64);
>         for (i = 0; i < nr_pci; i++) {
>                 struct pci_setup_rom *rom = NULL;
>                 u64 h = handles[i];
> @@ -502,53 +498,38 @@ setup_efi_pci64(struct boot_params *params, void **pci_handle,
>  static void setup_efi_pci(struct boot_params *params)
>  {
>         efi_status_t status;
> -       void **pci_handle = NULL;
> +       void **pci_handle;
>         efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
> -       unsigned long size = 0;
> +       unsigned long nr_pci;
>
> -       status = efi_call_early(locate_handle,
> +       status = efi_call_early(locate_handle_buffer,
>                                 EFI_LOCATE_BY_PROTOCOL,
> -                               &pci_proto, NULL, &size, pci_handle);
> +                               &pci_proto, NULL, &nr_pci, &pci_handle);
>
> -       if (status == EFI_BUFFER_TOO_SMALL) {
> -               status = efi_call_early(allocate_pool,
> -                                       EFI_LOADER_DATA,
> -                                       size, (void **)&pci_handle);
> -
> -               if (status != EFI_SUCCESS) {
> -                       efi_printk(sys_table, "Failed to alloc mem for pci_handle\n");
> -                       return;
> -               }
> -
> -               status = efi_call_early(locate_handle,
> -                                       EFI_LOCATE_BY_PROTOCOL, &pci_proto,
> -                                       NULL, &size, pci_handle);
> -       }
> +       if (status == EFI_OUT_OF_RESOURCES)
> +               efi_printk(sys_table, "Failed to alloc mem for pci_handle\n");
>
>         if (status != EFI_SUCCESS)
> -               goto free_handle;
> +               return;
>
>         if (efi_early->is64)
> -               setup_efi_pci64(params, pci_handle, size);
> +               setup_efi_pci64(params, pci_handle, nr_pci);
>         else
> -               setup_efi_pci32(params, pci_handle, size);
> +               setup_efi_pci32(params, pci_handle, nr_pci);
>
> -free_handle:
>         efi_call_early(free_pool, pci_handle);
>  }
>
>  static efi_status_t
> -setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
> +setup_uga32(void **uga_handle, unsigned long nr_ugas, u32 *width, u32 *height)
>  {
>         struct efi_uga_draw_protocol *uga = NULL, *first_uga;
>         efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
> -       unsigned long nr_ugas;
>         u32 *handles = (u32 *)uga_handle;;
>         efi_status_t status = EFI_INVALID_PARAMETER;
>         int i;
>
>         first_uga = NULL;
> -       nr_ugas = size / sizeof(u32);
>         for (i = 0; i < nr_ugas; i++) {
>                 efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
>                 u32 w, h, depth, refresh;
> @@ -583,17 +564,15 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height)
>  }
>
>  static efi_status_t
> -setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height)
> +setup_uga64(void **uga_handle, unsigned long nr_ugas, u32 *width, u32 *height)
>  {
>         struct efi_uga_draw_protocol *uga = NULL, *first_uga;
>         efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
> -       unsigned long nr_ugas;
>         u64 *handles = (u64 *)uga_handle;;
>         efi_status_t status = EFI_INVALID_PARAMETER;
>         int i;
>
>         first_uga = NULL;
> -       nr_ugas = size / sizeof(u64);
>         for (i = 0; i < nr_ugas; i++) {
>                 efi_guid_t pciio_proto = EFI_PCI_IO_PROTOCOL_GUID;
>                 u32 w, h, depth, refresh;
> @@ -631,30 +610,18 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height)
>   * See if we have Universal Graphics Adapter (UGA) protocol
>   */
>  static efi_status_t setup_uga(struct screen_info *si, efi_guid_t *uga_proto,
> -                             unsigned long size)
> +                             unsigned long nr_ugas, void **uga_handle)
>  {
>         efi_status_t status;
>         u32 width, height;
> -       void **uga_handle = NULL;
> -
> -       status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> -                               size, (void **)&uga_handle);
> -       if (status != EFI_SUCCESS)
> -               return status;
> -
> -       status = efi_call_early(locate_handle,
> -                               EFI_LOCATE_BY_PROTOCOL,
> -                               uga_proto, NULL, &size, uga_handle);
> -       if (status != EFI_SUCCESS)
> -               goto free_handle;
>
>         height = 0;
>         width = 0;
>
>         if (efi_early->is64)
> -               status = setup_uga64(uga_handle, size, &width, &height);
> +               status = setup_uga64(uga_handle, nr_ugas, &width, &height);
>         else
> -               status = setup_uga32(uga_handle, size, &width, &height);
> +               status = setup_uga32(uga_handle, nr_ugas, &width, &height);
>
>         if (!width && !height)
>                 goto free_handle;
> @@ -686,27 +653,26 @@ void setup_graphics(struct boot_params *boot_params)
>         struct screen_info *si;
>         efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
>         efi_status_t status;
> -       unsigned long size;
> -       void **gop_handle = NULL;
> -       void **uga_handle = NULL;
> +       unsigned long nr;
> +       void **gop_handle;
> +       void **uga_handle;
>
>         si = &boot_params->screen_info;
>         memset(si, 0, sizeof(*si));
>
> -       size = 0;
> -       status = efi_call_early(locate_handle,
> +       status = efi_call_early(locate_handle_buffer,
>                                 EFI_LOCATE_BY_PROTOCOL,
> -                               &graphics_proto, NULL, &size, gop_handle);
> -       if (status == EFI_BUFFER_TOO_SMALL)
> -               status = efi_setup_gop(NULL, si, &graphics_proto, size);
> +                               &graphics_proto, NULL, &nr, &gop_handle);
> +       if (status == EFI_SUCCESS)
> +               status = efi_setup_gop(NULL, si, &graphics_proto, nr,
> +                                      gop_handle);
>
>         if (status != EFI_SUCCESS) {
> -               size = 0;
> -               status = efi_call_early(locate_handle,
> +               status = efi_call_early(locate_handle_buffer,
>                                         EFI_LOCATE_BY_PROTOCOL,
> -                                       &uga_proto, NULL, &size, uga_handle);
> -               if (status == EFI_BUFFER_TOO_SMALL)
> -                       setup_uga(si, &uga_proto, size);
> +                                       &uga_proto, NULL, &nr, &uga_handle);
> +               if (status == EFI_SUCCESS)
> +                       setup_uga(si, &uga_proto, nr, uga_handle);
>         }
>  }
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index 993aa56..27fd62d 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -173,18 +173,19 @@ 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;
> +       unsigned long nr;
> +       void **gop_handle;
>         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) {
> +       status = efi_call_early(locate_handle_buffer, EFI_LOCATE_BY_PROTOCOL,
> +                               &gop_proto, NULL, &nr, &gop_handle);
> +       if (status == EFI_SUCCESS) {
>                 si = alloc_screen_info(sys_table_arg);
> -               if (!si)
> +               if (!si) {
> +                       efi_call_early(free_pool, gop_handle);
>                         return NULL;
> -               efi_setup_gop(sys_table_arg, si, &gop_proto, size);
> +               }
> +               efi_setup_gop(sys_table_arg, si, &gop_proto, nr, gop_handle);
>         }
>         return si;
>  }
> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
> index 932742e..a2dca32 100644
> --- a/drivers/firmware/efi/libstub/gop.c
> +++ b/drivers/firmware/efi/libstub/gop.c
> @@ -111,10 +111,9 @@ __gop_query32(efi_system_table_t *sys_table_arg,
>
>  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)
> +           efi_guid_t *proto, unsigned long nr_gops, 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;
> @@ -128,7 +127,6 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
>         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;
> @@ -136,6 +134,7 @@ setup_gop32(efi_system_table_t *sys_table_arg, struct screen_info *si,
>                 void *dummy = NULL;
>                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
>                 u64 current_fb_base;
> +               unsigned long size;
>
>                 status = efi_call_early(handle_protocol, h,
>                                         proto, (void **)&gop32);
> @@ -228,10 +227,9 @@ __gop_query64(efi_system_table_t *sys_table_arg,
>
>  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)
> +           efi_guid_t *proto, unsigned long nr_gops, 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;
> @@ -245,7 +243,6 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
>         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;
> @@ -253,6 +250,7 @@ setup_gop64(efi_system_table_t *sys_table_arg, struct screen_info *si,
>                 void *dummy = NULL;
>                 efi_handle_t h = (efi_handle_t)(unsigned long)handles[i];
>                 u64 current_fb_base;
> +               unsigned long size;
>
>                 status = efi_call_early(handle_protocol, h,
>                                         proto, (void **)&gop64);
> @@ -324,31 +322,18 @@ out:
>   */
>  efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
>                            struct screen_info *si, efi_guid_t *proto,
> -                          unsigned long size)
> +                          unsigned long nr_gops, void **gop_handle)
>  {
>         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,
> +               status = setup_gop64(sys_table_arg, si, proto, nr_gops,
>                                      gop_handle);
>         } else {
> -               status = setup_gop32(sys_table_arg, si, proto, size,
> +               status = setup_gop32(sys_table_arg, si, proto, nr_gops,
>                                      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 4ae1a1a..52fe49d 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -304,7 +304,8 @@ typedef struct {
>         void *close_protocol;
>         void *open_protocol_information;
>         void *protocols_per_handle;
> -       void *locate_handle_buffer;
> +       efi_status_t (*locate_handle_buffer)(int, efi_guid_t *, void *,
> +                                            unsigned long *, efi_handle_t **);
>         efi_status_t (*locate_protocol)(efi_guid_t *, void *, void **);
>         void *install_multiple_protocol_interfaces;
>         void *uninstall_multiple_protocol_interfaces;
> @@ -1424,7 +1425,7 @@ 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);
> +                          unsigned long nr_gops, void **gop_handle);
>
>  bool efi_runtime_disabled(void);
>  extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
> --
> 2.9.3
>

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Lukas Wunner]

On Fri, Sep 09, 2016 at 11:52:32AM +0100, Ard Biesheuvel wrote:
> On 8 September 2016 at 01:02, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> > Use LocateHandleBuffer instead of the LocateHandle + AllocatePool +
> > LocateHandle combo to save a bit on code.
> >
> > Usage of LocateHandle for UGA and GOP has been present in the efistub
> > since its introduction with commit 291f36325f9f ("x86, efi: EFI boot
> > stub support"). It was subsequently inherited by the PCI ROM retrieval
> > with commit dd5fc854de5f ("EFI: Stash ROMs if they're not in the PCI
> > BAR").
> >
> > One valid reason to not use LocateHandleBuffer would be if a specific
> > memory type is required for the buffer, and particularly if the buffer
> > needs to persist across ExitBootServices. The spec does not mandate
> > which type is used, edk2 seems to default to EfiBootServicesData.
> > In the three use cases modified by this commit (UGA, GOP, PCI), the
> > buffer is freed before ExitBootServices is called. Hence the memory
> > type is irrelevant.
> >
> > No functional change intended.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> > ---
> >
> 
> What is the oldest UEFI version we claim to support? For ARM, this is
> not an issue, but it appears that LocateHandleBuffer () was introduced
> in v1.10

That's a fair point. I couldn't find any older specs than 1.10, only found
some sources saying the 1.02 spec was pulled for legal reasons.

I did notice the 1.10 spec says "The LocateHandleBuffer() is a new version
of LocateHandle() that allocates the required buffer for the caller."

Note the word "new".

According to Wikipedia, "Intel's first Itanium workstations and servers,
released in 2000, implemented EFI 1.02. Hewlett-Packard's first Itanium 2
systems, released in 2002, implemented EFI 1.10".

Since this code is only used by x86 and arm (not ia64), were x86_32
systems with EFI < 1.10 actually shipped?

I vaguely recall that Apple was among the first vendors adopting EFI
for x86 in 2005, but I could be wrong.

Thanks,

Lukas

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Matt Fleming]

On Fri, 09 Sep, at 01:59:51PM, Lukas Wunner wrote:
> 
> I vaguely recall that Apple was among the first vendors adopting EFI
> for x86 in 2005, but I could be wrong.

I'm fairly sure my old Macbook 2.1 is EFI 1.10, yeah.

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Ard Biesheuvel]

On 3 October 2016 at 12:32, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Fri, 09 Sep, at 01:59:51PM, Lukas Wunner wrote:
>>
>> I vaguely recall that Apple was among the first vendors adopting EFI
>> for x86 in 2005, but I could be wrong.
>
> I'm fairly sure my old Macbook 2.1 is EFI 1.10, yeah.

Indeed. But the Macs are a little weird in this respect, it does not
necessarily mean they don't implement LocateHandleBuffer(). Since
Lukas uses a Mac himself (IIRC), I'd assume it works on his system and
so their UEFI does implement this boot service. That does not
necessarily tell us anything about other Macs, though.

So I'm a bit on the fence here: we could mandate a more recent version
of UEFI if the Mac is the only 'older' implementation we intend to
support, but it is not a very clean approach imo, and this particular
patch, while useful, does not use a critical piece of functionality
that is only implemented by newer spec revisions.

I am leaning towards not taking the patch, although we could revisit
this in the future.

-- 
Ard.

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Lukas Wunner]

On Tue, Oct 04, 2016 at 01:56:42PM +0100, Ard Biesheuvel wrote:
> On 3 October 2016 at 12:32, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > On Fri, 09 Sep, at 01:59:51PM, Lukas Wunner wrote:
> >>
> >> I vaguely recall that Apple was among the first vendors adopting EFI
> >> for x86 in 2005, but I could be wrong.
> >
> > I'm fairly sure my old Macbook 2.1 is EFI 1.10, yeah.
> 
> Indeed. But the Macs are a little weird in this respect, it does not
> necessarily mean they don't implement LocateHandleBuffer(). Since
> Lukas uses a Mac himself (IIRC), I'd assume it works on his system and
> so their UEFI does implement this boot service. That does not
> necessarily tell us anything about other Macs, though.

I have a 2012 MacBook Pro, but there's a simple trick to find out if
even the earliest Intel Macs of 2006 supported LocateHandleBuffer:
By disassembling the bootloader that shipped with OS X 10.6 (2009).
This release still officially supported the very first Intel Macs.
If it unconditionally calls LocateHandleBuffer, then it must have
been supported from the very beginning.

Did a quick search for instruction FF 90 38 01 00 00, which means
"call qword [ds:rax+0x138]", where 138 is the LocateHandleBuffer
offset in the 64-bit boot services table.  And sure enough:

5147         lea        rdi, qword [ds:0x278d0]         ; boot services table
514e         lea        rdx, qword [ss:rbp+0xffffffffffffffa8]
5152         xor        r8d, r8d                        ; SearchKey
5155         lea        r9, qword [ss:rbp+0xffffffffffffff98] ; NoHandles
5159         mov        ecx, 0x2                        ; ByProtocol
515e         mov        rax, qword [ds:rdi]
5161         mov        qword [ss:rsp-0x8+arg_10], rdx  ; Buffer
5166         lea        rdx, qword [ds:0x27a00]         ; GUID
516d         call       qword [ds:rax+0x138]            ; LocateHandleBuffer

The 32-bit version looks the same except for the offset (rax+0xa8) and
calling convention.

So I'm pretty confident this patch works on all Macs, the question is
were there any non-Mac x86 machines which might lack LocateHandleBuffer?

The EFI 1.10 spec merely says "The LocateHandleBuffer() is a new version
of LocateHandle() that allocates the required buffer for the caller."

But new since when?  Was this added with 1.10 or did it already exist
in earlier versions?  The 1.10 spec dates back to 2002 and according
to Wikipedia, only Itaniums used EFI back then.

If LocateHandleBuffer was indeed missing in earlier specs, was the
layout of the boot services table the same as in 1.10 and the function
pointer was just marked reserved, or was the layout completely different?
If the latter, then we don't support earlier versions anyway.

The Graphics Output Protocol was added post 1.10, so it should be safe
to use LocateHandleBuffer at least for GOP.  The two other protocols,
UGA and EFI_PCI_IO_PROTOCOL, were already present in 1.10.

I tried to find the 1.02 spec using the Wayback Machine:
https://web.archive.org/web/20021218163928/http://developer.intel.com/technology/efi/download.htm

The download required entering an e-mail address and the user was then
redirected to an URL which is missing in the Wayback Machine:
https://web.archive.org/web/http://developer.intel.com/technology/efi/EFISpec_v102.htm

I couldn't find older specs than 1.10 with Google either, so it looks
like it's gone from today's web.  Thus, on September 12 I reached out
by e-mail to Vincent Zimmer who had been working on EFI since its early
days (see his blog at http://vzimmer.blogspot.com).  Unfortunately I
never got a reply.

My guess is that pre-1.10 versions were only ever shipped with Itanium
and that using LocateHandleBuffer is therefore safe on x86.  But to know
for sure it would be necessary to find someone who still is in possession
of earlier specs and knows what shipped when.

Thanks,

Lukas

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Matt Fleming]

On Tue, 04 Oct, at 01:56:42PM, Ard Biesheuvel wrote:
> 
> I am leaning towards not taking the patch, although we could revisit
> this in the future.

Let's do this. It's only a cleanup after all.

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Ard Biesheuvel]

On 4 October 2016 at 17:53, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> On Tue, Oct 04, 2016 at 01:56:42PM +0100, Ard Biesheuvel wrote:
>> On 3 October 2016 at 12:32, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>> > On Fri, 09 Sep, at 01:59:51PM, Lukas Wunner wrote:
>> >>
>> >> I vaguely recall that Apple was among the first vendors adopting EFI
>> >> for x86 in 2005, but I could be wrong.
>> >
>> > I'm fairly sure my old Macbook 2.1 is EFI 1.10, yeah.
>>
>> Indeed. But the Macs are a little weird in this respect, it does not
>> necessarily mean they don't implement LocateHandleBuffer(). Since
>> Lukas uses a Mac himself (IIRC), I'd assume it works on his system and
>> so their UEFI does implement this boot service. That does not
>> necessarily tell us anything about other Macs, though.
>
> I have a 2012 MacBook Pro, but there's a simple trick to find out if
> even the earliest Intel Macs of 2006 supported LocateHandleBuffer:
> By disassembling the bootloader that shipped with OS X 10.6 (2009).
> This release still officially supported the very first Intel Macs.
> If it unconditionally calls LocateHandleBuffer, then it must have
> been supported from the very beginning.
>
> Did a quick search for instruction FF 90 38 01 00 00, which means
> "call qword [ds:rax+0x138]", where 138 is the LocateHandleBuffer
> offset in the 64-bit boot services table.  And sure enough:
>
> 5147         lea        rdi, qword [ds:0x278d0]         ; boot services table
> 514e         lea        rdx, qword [ss:rbp+0xffffffffffffffa8]
> 5152         xor        r8d, r8d                        ; SearchKey
> 5155         lea        r9, qword [ss:rbp+0xffffffffffffff98] ; NoHandles
> 5159         mov        ecx, 0x2                        ; ByProtocol
> 515e         mov        rax, qword [ds:rdi]
> 5161         mov        qword [ss:rsp-0x8+arg_10], rdx  ; Buffer
> 5166         lea        rdx, qword [ds:0x27a00]         ; GUID
> 516d         call       qword [ds:rax+0x138]            ; LocateHandleBuffer
>
> The 32-bit version looks the same except for the offset (rax+0xa8) and
> calling convention.
>
> So I'm pretty confident this patch works on all Macs, the question is
> were there any non-Mac x86 machines which might lack LocateHandleBuffer?
>
> The EFI 1.10 spec merely says "The LocateHandleBuffer() is a new version
> of LocateHandle() that allocates the required buffer for the caller."
>
> But new since when?  Was this added with 1.10 or did it already exist
> in earlier versions?  The 1.10 spec dates back to 2002 and according
> to Wikipedia, only Itaniums used EFI back then.
>
> If LocateHandleBuffer was indeed missing in earlier specs, was the
> layout of the boot services table the same as in 1.10 and the function
> pointer was just marked reserved, or was the layout completely different?
> If the latter, then we don't support earlier versions anyway.
>
> The Graphics Output Protocol was added post 1.10, so it should be safe
> to use LocateHandleBuffer at least for GOP.  The two other protocols,
> UGA and EFI_PCI_IO_PROTOCOL, were already present in 1.10.
>
> I tried to find the 1.02 spec using the Wayback Machine:
> https://web.archive.org/web/20021218163928/http://developer.intel.com/technology/efi/download.htm
>
> The download required entering an e-mail address and the user was then
> redirected to an URL which is missing in the Wayback Machine:
> https://web.archive.org/web/http://developer.intel.com/technology/efi/EFISpec_v102.htm
>
> I couldn't find older specs than 1.10 with Google either, so it looks
> like it's gone from today's web.  Thus, on September 12 I reached out
> by e-mail to Vincent Zimmer who had been working on EFI since its early
> days (see his blog at http://vzimmer.blogspot.com).  Unfortunately I
> never got a reply.
>

Thanks for the archeological background :-) Matt and I deal with
Vincent on a regular basis in the USST and other UEFI forum calls, so
we could simply ask him. However, we've had our share of breakage with
the stub code, which is difficult to debug on anything except
development hardware, and so I think we should not take this change
(and Matt appears to agree). I do like the cleanup, but since there is
no functional change, I'd rather stick with the existing code.

> My guess is that pre-1.10 versions were only ever shipped with Itanium
> and that using LocateHandleBuffer is therefore safe on x86.  But to know
> for sure it would be necessary to find someone who still is in possession
> of earlier specs and knows what shipped when.
>

Regards,
Ard.

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Lukas Wunner]

On Fri, Sep 09, 2016 at 11:52:32AM +0100, Ard Biesheuvel wrote:
> What is the oldest UEFI version we claim to support? For ARM, this is
> not an issue, but it appears that LocateHandleBuffer () was introduced
> in v1.10

UGA was apparently introduced with v1.10, GOP was introduced later
than v1.10, it follows that availability of LocateHandleBuffer()
can be assumed at least when searching for these two protocols.

Would a patch be entertained which uses LocateHandleBuffer for these
two but leaves the PCI ROM retrieval unchanged?


On Wed, Oct 05, 2016 at 05:25:44PM +0100, Ard Biesheuvel wrote:
> However, we've had our share of breakage with
> the stub code, which is difficult to debug on anything except
> development hardware, and so I think we should not take this change
> (and Matt appears to agree).

Thanks for the clarification, however I don't quite follow:
Is the rejection of the patch based on the intent to maintain
support for EFI v1.0+ (which isn't an argument for UGA + GOP
as lined out above) or is it based on a gut feeling that the
patch might cause breakage for whatever reason.
(Perhaps firmware bugs?  Not sure what is meant here.)

Thanks,

Lukas

Re: [PATCH] efi: Use LocateHandleBuffer instead of LocateHandle

[Ard Biesheuvel]

On 6 October 2016 at 10:32, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> On Fri, Sep 09, 2016 at 11:52:32AM +0100, Ard Biesheuvel wrote:
>> What is the oldest UEFI version we claim to support? For ARM, this is
>> not an issue, but it appears that LocateHandleBuffer () was introduced
>> in v1.10
>
> UGA was apparently introduced with v1.10, GOP was introduced later
> than v1.10, it follows that availability of LocateHandleBuffer()
> can be assumed at least when searching for these two protocols.
>
> Would a patch be entertained which uses LocateHandleBuffer for these
> two but leaves the PCI ROM retrieval unchanged?
>

Thanks, but no thanks.

> On Wed, Oct 05, 2016 at 05:25:44PM +0100, Ard Biesheuvel wrote:
>> However, we've had our share of breakage with
>> the stub code, which is difficult to debug on anything except
>> development hardware, and so I think we should not take this change
>> (and Matt appears to agree).
>
> Thanks for the clarification, however I don't quite follow:
> Is the rejection of the patch based on the intent to maintain
> support for EFI v1.0+ (which isn't an argument for UGA + GOP
> as lined out above) or is it based on a gut feeling that the
> patch might cause breakage for whatever reason.
> (Perhaps firmware bugs?  Not sure what is meant here.)
>

The fact that LocateHandleBuffer() should be implemented by fairly old
versions of UEFI does not guarantee that all UEFI systems that we
currently support implement it correctly. So in that sense, we face a
non-zero risk of regressions, which is not justifiable for a simple
code cleanup.