[PATCH 0/4] Handle EFI_INVALID_PARAMETER from ExitBootServices

[PATCH 0/4] Handle EFI_INVALID_PARAMETER from ExitBootServices

[Jeffrey Hugo]

According to the UEFI spec, the UEFI OS Loader (aka the stub) should
transition from UEFI to the OS by getting the current memory map from UEFI,
then calling ExitBootServices.  The spec states that ExitBootServices may
return EFI_INVALID_PARAMETER if the memory map reference provided by the stub
is not current, ie UEFI handled some event prior to ExitBootServices which
modified the map.  The spec states that to handle this scenario, the stub shall
get the updated map, and invoke ExitBootServices again.  The spec also states
that once ExitBootServices is invoked, even if it returns error, the only
APIs the stub is allowed to invoke is GetMemoryMap and ExitBootServices.

The EFI_INVALID_PARAMETER scenario has been seen in the wild previously in
x86 but the fix - d3768d885c6c ("x86, efi: retry ExitBootServices() on failure")
still violates the spec.  The FDT code does not handle this scenario, but
instances of it are now observed.

This patch series aims to provide a spec complaint solution that can be reused
in all stubs, thus preventing each arch or variant from reinventing the wheel
and likely getting it wrong.

[V1]
-Allocate headspace on the memory map buffer for reuse per Ard Biesheuvel
-Create a shared helper address the issue universally per Matt Fleming

Jeffrey Hugo (4):
  efi/libstub: Allocate headspace in efi_get_memory_map()
  efi/libstub: Introduce ExitBootServices helper
  efi/libstub: Use efi_exit_boot_services() in FDT
  x86/efi: Use efi_exit_boot_services()

 arch/x86/boot/compressed/eboot.c               | 132 +++++++++++++------------
 drivers/firmware/efi/libstub/efi-stub-helper.c | 122 +++++++++++++++++++++--
 drivers/firmware/efi/libstub/fdt.c             |  52 +++++++---
 drivers/firmware/efi/libstub/random.c          |   3 +-
 include/linux/efi.h                            |  22 ++++-
 5 files changed, 244 insertions(+), 87 deletions(-)

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Jeffrey Hugo]

efi_get_memory_map() allocates a buffer to store the memory map that it
retrieves.  This buffer may need to be reused by the client after
ExitBootServices() is called, at which point allocations are not longer
permitted.  To support this usecase, provide the allocated buffer size back
to the client, and allocate some additional headroom to account for any
reasonable growth in the map that is likely to happen between the call to
efi_get_memory_map() and the client reusing the buffer.

Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 arch/x86/boot/compressed/eboot.c               |  4 ++--
 drivers/firmware/efi/libstub/efi-stub-helper.c | 29 +++++++++++++++++++-------
 drivers/firmware/efi/libstub/fdt.c             |  8 ++++---
 drivers/firmware/efi/libstub/random.c          |  3 ++-
 include/linux/efi.h                            |  3 ++-
 5 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 52fef60..9036ec9 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1010,7 +1010,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
 			      void *handle, bool is64)
 {
 	struct efi_info *efi = &boot_params->efi_info;
-	unsigned long map_sz, key, desc_size;
+	unsigned long map_sz, key, desc_size, buff_size;
 	efi_memory_desc_t *mem_map;
 	struct setup_data *e820ext;
 	const char *signature;
@@ -1028,7 +1028,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
 
 get_map:
 	status = efi_get_memory_map(sys_table, &mem_map, &map_sz, &desc_size,
-				    &desc_version, &key);
+				    &desc_version, &key, &buff_size);
 
 	if (status != EFI_SUCCESS)
 		return status;
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3bd127f9..9286ff6 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -68,7 +68,8 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
 				unsigned long *map_size,
 				unsigned long *desc_size,
 				u32 *desc_ver,
-				unsigned long *key_ptr)
+				unsigned long *key_ptr,
+				unsigned long *buff_size)
 {
 	efi_memory_desc_t *m = NULL;
 	efi_status_t status;
@@ -76,12 +77,14 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
 	u32 desc_version;
 
 	*map_size = sizeof(*m) * 32;
+	*buff_size = *map_size;
 again:
 	/*
 	 * Add an additional efi_memory_desc_t because we're doing an
 	 * allocation which may be in a new descriptor region.
 	 */
 	*map_size += sizeof(*m);
+	*buff_size = *map_size;
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
 				*map_size, (void **)&m);
 	if (status != EFI_SUCCESS)
@@ -91,8 +94,17 @@ again:
 	key = 0;
 	status = efi_call_early(get_memory_map, map_size, m,
 				&key, desc_size, &desc_version);
-	if (status == EFI_BUFFER_TOO_SMALL) {
+	if (status == EFI_BUFFER_TOO_SMALL ||
+				(*buff_size - *map_size) / sizeof(*m) < 8) {
 		efi_call_early(free_pool, m);
+		/*
+		 * Make sure there is 8 entries worth of headroom so that the
+		 * buffer can be reused for a new map after allocations are
+		 * no longer permitted.  Its unlikely that the map will grow to
+		 * exceed this headroom once we are ready to trigger
+		 * ExitBootServices()
+		 */
+		*map_size += sizeof(*m) * 8;
 		goto again;
 	}
 
@@ -113,13 +125,14 @@ fail:
 unsigned long get_dram_base(efi_system_table_t *sys_table_arg)
 {
 	efi_status_t status;
-	unsigned long map_size;
+	unsigned long map_size, buff_size;
 	unsigned long membase  = EFI_ERROR;
 	struct efi_memory_map map;
 	efi_memory_desc_t *md;
 
 	status = efi_get_memory_map(sys_table_arg, (efi_memory_desc_t **)&map.map,
-				    &map_size, &map.desc_size, NULL, NULL);
+				    &map_size, &map.desc_size, NULL, NULL,
+				    &buff_size);
 	if (status != EFI_SUCCESS)
 		return membase;
 
@@ -144,7 +157,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 			    unsigned long size, unsigned long align,
 			    unsigned long *addr, unsigned long max)
 {
-	unsigned long map_size, desc_size;
+	unsigned long map_size, desc_size, buff_size;
 	efi_memory_desc_t *map;
 	efi_status_t status;
 	unsigned long nr_pages;
@@ -152,7 +165,7 @@ efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 	int i;
 
 	status = efi_get_memory_map(sys_table_arg, &map, &map_size, &desc_size,
-				    NULL, NULL);
+				    NULL, NULL, &buff_size);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
@@ -230,14 +243,14 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
 			   unsigned long *addr)
 {
-	unsigned long map_size, desc_size;
+	unsigned long map_size, desc_size, buff_size;
 	efi_memory_desc_t *map;
 	efi_status_t status;
 	unsigned long nr_pages;
 	int i;
 
 	status = efi_get_memory_map(sys_table_arg, &map, &map_size, &desc_size,
-				    NULL, NULL);
+				    NULL, NULL, &buff_size);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index e58abfa..878e661 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -175,7 +175,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 					    unsigned long fdt_addr,
 					    unsigned long fdt_size)
 {
-	unsigned long map_size, desc_size;
+	unsigned long map_size, desc_size, buff_size;
 	u32 desc_ver;
 	unsigned long mmap_key;
 	efi_memory_desc_t *memory_map, *runtime_map;
@@ -190,7 +190,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	 * the number of EFI_MEMORY_RUNTIME regions.
 	 */
 	status = efi_get_memory_map(sys_table, &runtime_map, &map_size,
-				    &desc_size, &desc_ver, &mmap_key);
+				    &desc_size, &desc_ver, &mmap_key,
+				    &buff_size);
 	if (status != EFI_SUCCESS) {
 		pr_efi_err(sys_table, "Unable to retrieve UEFI memory map.\n");
 		return status;
@@ -219,7 +220,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		 * exit_boot_services().
 		 */
 		status = efi_get_memory_map(sys_table, &memory_map, &map_size,
-					    &desc_size, &desc_ver, &mmap_key);
+					    &desc_size, &desc_ver, &mmap_key,
+					    &buff_size);
 		if (status != EFI_SUCCESS)
 			goto fail_free_new_fdt;
 
diff --git a/drivers/firmware/efi/libstub/random.c b/drivers/firmware/efi/libstub/random.c
index 53f6d3f..9dd8534 100644
--- a/drivers/firmware/efi/libstub/random.c
+++ b/drivers/firmware/efi/libstub/random.c
@@ -73,12 +73,13 @@ efi_status_t efi_random_alloc(efi_system_table_t *sys_table_arg,
 			      unsigned long random_seed)
 {
 	unsigned long map_size, desc_size, total_slots = 0, target_slot;
+	unsigned long buff_size;
 	efi_status_t status;
 	efi_memory_desc_t *memory_map;
 	int map_offset;
 
 	status = efi_get_memory_map(sys_table_arg, &memory_map, &map_size,
-				    &desc_size, NULL, NULL);
+				    &desc_size, NULL, NULL, &buff_size);
 	if (status != EFI_SUCCESS)
 		return status;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f196dd0..c47fc5f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1434,7 +1434,8 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
 				unsigned long *map_size,
 				unsigned long *desc_size,
 				u32 *desc_ver,
-				unsigned long *key_ptr);
+				unsigned long *key_ptr,
+				unsigned long *buff_size);
 
 efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/4] efi/libstub: Introduce ExitBootServices helper

[Jeffrey Hugo]

The spec allows ExitBootServices to fail with EFI_INVALID_PARAMETER if a
race condition has occurred where the EFI has updated the memory map after
the stub grabbed a reference to the map.  The spec defines a retry
proceedure with specific requirements to handle this scenario.

No current stub implementation correctly follows the spec in this regard,
so add a helper to the stub library that correctly adhears to the spec and
abstracts the complexity from stubs.

Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 93 ++++++++++++++++++++++++++
 include/linux/efi.h                            | 19 ++++++
 2 files changed, 112 insertions(+)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 9286ff6..7b40b8c 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -717,3 +717,96 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 	*cmd_line_len = options_bytes;
 	return (char *)cmdline_addr;
 }
+
+/*
+ * Handle calling ExitBootServices according to the requirements set out by the
+ * spec.  Obtains the current memory map, and returns that info after calling
+ * ExitBootServices.  Client has the option to specify a function to process the
+ * memory map data.  A client specific structure may be passed to the function
+ * via priv.  The client function may be called multiple times.
+ */
+efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table,
+				    void *handle,
+				    efi_memory_desc_t **memory_map,
+				    unsigned long *map_size,
+				    unsigned long *desc_size,
+				    u32 *desc_ver,
+				    unsigned long *mmap_key,
+				    void *priv,
+				    efi_status_t (*priv_func)(
+						efi_system_table_t *sys_table,
+						void *handle,
+						efi_memory_desc_t *memory_map,
+						unsigned long *map_size,
+						unsigned long *desc_size,
+						u32 *desc_ver,
+						unsigned long *mmap_key,
+						unsigned long buff_size,
+						void *priv))
+{
+	efi_status_t status;
+	unsigned long buff_size;
+
+	status = efi_get_memory_map(sys_table, memory_map, map_size,
+				    desc_size, desc_ver, mmap_key, &buff_size);
+
+	if (status != EFI_SUCCESS)
+		goto fail;
+
+	if (priv_func) {
+		status = priv_func(sys_table, handle, *memory_map, map_size,
+				   desc_size, desc_ver, mmap_key, buff_size,
+				   priv);
+		if (status != EFI_SUCCESS)
+			goto free_map;
+	}
+
+	status = sys_table->boottime->exit_boot_services(handle, *mmap_key);
+
+	if (status == EFI_INVALID_PARAMETER) {
+		/*
+		 * The memory map changed between efi_get_memory_map() and
+		 * exit_boot_services().  Per the spec we need to get the
+		 * updated map, and try again.  The spec implies one retry
+		 * should be sufficent, which is confirmed against the EDK2
+		 * implementation.  Per the spec, we can only invoke
+		 * get_memory_map() and exit_boot_services() - we cannot alloc
+		 * so efi_get_memory_map() cannot be used, and we must reuse
+		 * the buffer.  For all practical purposes, the headroom in the
+		 * buffer should account for any changes in the map so the call
+		 * to get_memory_map() is expected to succeed here.
+		 */
+		*map_size = buff_size;
+		status = sys_table->boottime->get_memory_map(map_size,
+							     *memory_map,
+							     mmap_key,
+							     desc_size,
+							     desc_ver);
+		if (status != EFI_SUCCESS)
+			/* exit_boot_services() was called, thus cannot free*/
+			goto fail;
+
+		if (priv_func) {
+			status = priv_func(sys_table, handle, *memory_map,
+					   map_size, desc_size, desc_ver,
+					   mmap_key, buff_size, priv);
+			if (status != EFI_SUCCESS)
+				/* exit_boot_services() called, cannot free*/
+				goto fail;
+		}
+
+		status = sys_table->boottime->exit_boot_services(handle,
+								 *mmap_key);
+	}
+
+	if (status != EFI_SUCCESS)
+		/* exit_boot_services() was called, thus cannot free*/
+		goto fail;
+
+	return EFI_SUCCESS;
+
+free_map:
+	sys_table->boottime->free_pool(*memory_map);
+fail:
+	return status;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c47fc5f..96f7b74 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1466,4 +1466,23 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
 			   unsigned long size);
 
 bool efi_runtime_disabled(void);
+
+efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table,
+				    void *handle,
+				    efi_memory_desc_t **memory_map,
+				    unsigned long *map_size,
+				    unsigned long *desc_size,
+				    u32 *desc_ver,
+				    unsigned long *mmap_key,
+				    void *priv,
+				    efi_status_t (*priv_func)(
+						efi_system_table_t *sys_table,
+						void *handle,
+						efi_memory_desc_t *memory_map,
+						unsigned long *map_size,
+						unsigned long *desc_size,
+						u32 *desc_ver,
+						unsigned long *mmap_key,
+						unsigned long buff_size,
+						void *priv));
 #endif /* _LINUX_EFI_H */
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/4] efi/libstub: Use efi_exit_boot_services() in FDT

[Jeffrey Hugo]

The FDT code directly calls ExitBootServices.  This is inadvisable, and the
FDT code does not handle EFI_INVALID_PARAMETER as required by the spec.
The efi_exit_boot_services() helper handles the EFI_INVALID_PARAMETER
scenario.

Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/libstub/fdt.c | 44 +++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 878e661..fdda4e6 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -152,6 +152,33 @@ fdt_set_fail:
 #define EFI_FDT_ALIGN EFI_PAGE_SIZE
 #endif
 
+struct exit_boot_struct {
+	efi_memory_desc_t *runtime_map;
+	int *runtime_entry_count;
+};
+
+static efi_status_t exit_boot_func(efi_system_table_t *sys_table,
+				   void *handle,
+				   efi_memory_desc_t *memory_map,
+				   unsigned long *map_size,
+				   unsigned long *desc_size,
+				   u32 *desc_ver,
+				   unsigned long *mmap_key,
+				   unsigned long buff_size,
+				   void *priv)
+{
+	struct exit_boot_struct *p = priv;
+	/*
+	 * Update the memory map with virtual addresses. The function will also
+	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
+	 * entries so that we can pass it straight into SetVirtualAddressMap()
+	 */
+	efi_get_virtmap(memory_map, *map_size, *desc_size, p->runtime_map,
+			p->runtime_entry_count);
+
+	return EFI_SUCCESS;
+}
+
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the
@@ -182,6 +209,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	unsigned long new_fdt_size;
 	efi_status_t status;
 	int runtime_entry_count = 0;
+	struct exit_boot_struct priv;
 
 	/*
 	 * Get a copy of the current memory map that we will use to prepare
@@ -252,16 +280,12 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 		}
 	}
 
-	/*
-	 * Update the memory map with virtual addresses. The function will also
-	 * populate @runtime_map with copies of just the EFI_MEMORY_RUNTIME
-	 * entries so that we can pass it straight into SetVirtualAddressMap()
-	 */
-	efi_get_virtmap(memory_map, map_size, desc_size, runtime_map,
-			&runtime_entry_count);
-
-	/* Now we are ready to exit_boot_services.*/
-	status = sys_table->boottime->exit_boot_services(handle, mmap_key);
+	sys_table->boottime->free_pool(memory_map);
+	priv.runtime_map = runtime_map;
+	priv.runtime_entry_count = &runtime_entry_count;
+	status = efi_exit_boot_services(sys_table, handle, &memory_map,
+					&map_size, &desc_size, &desc_ver,
+					&mmap_key, &priv, exit_boot_func);
 
 	if (status == EFI_SUCCESS) {
 		efi_set_virtual_address_map_t *svam;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 4/4] x86/efi: Use efi_exit_boot_services()

[Jeffrey Hugo]

The eboot code directly calls ExitBootServices.  This is inadvisable, and
the eboot code attempts allocations after calling ExitBootSerives which is
not permitted per the spec.  The efi_exit_boot_services() helper handles
this scenario.

Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 arch/x86/boot/compressed/eboot.c | 130 ++++++++++++++++++++-------------------
 1 file changed, 67 insertions(+), 63 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 9036ec9..3d3224d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1006,79 +1006,87 @@ static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
 	return status;
 }
 
+struct exit_boot_struct {
+	struct boot_params *boot_params;
+	struct efi_info *efi;
+	struct setup_data *e820ext;
+	__u32 e820ext_size;
+	bool is64;
+};
+
+static efi_status_t exit_boot_func(efi_system_table_t *sys_table,
+				   void *handle,
+				   efi_memory_desc_t *memory_map,
+				   unsigned long *map_size,
+				   unsigned long *desc_size,
+				   u32 *desc_ver,
+				   unsigned long *mmap_key,
+				   unsigned long buff_size,
+				   void *priv)
+{
+	static bool first = true;
+	const char *signature;
+	__u32 nr_desc;
+	efi_status_t status;
+	struct exit_boot_struct *p = priv;
+
+	if (first) {
+		nr_desc = buff_size / *desc_size;
+		if (nr_desc > ARRAY_SIZE(p->boot_params->e820_map)) {
+			u32 nr_e820ext = nr_desc -
+					ARRAY_SIZE(p->boot_params->e820_map);
+
+			status = alloc_e820ext(nr_e820ext, &p->e820ext,
+					       &p->e820ext_size);
+			if (status != EFI_SUCCESS)
+				return status;
+		}
+		first = false;
+	}
+
+	signature = p->is64 ? EFI64_LOADER_SIGNATURE : EFI32_LOADER_SIGNATURE;
+	memcpy(&p->efi->efi_loader_signature, signature, sizeof(__u32));
+
+	p->efi->efi_systab = (unsigned long)sys_table;
+	p->efi->efi_memdesc_size = *desc_size;
+	p->efi->efi_memdesc_version = *desc_ver;
+	p->efi->efi_memmap = (unsigned long)memory_map;
+	p->efi->efi_memmap_size = *map_size;
+
+#ifdef CONFIG_X86_64
+	p->efi->efi_systab_hi = (unsigned long)sys_table >> 32;
+	p->efi->efi_memmap_hi = (unsigned long)memory_map >> 32;
+#endif
+
+	return EFI_SUCCESS;
+}
+
 static efi_status_t exit_boot(struct boot_params *boot_params,
 			      void *handle, bool is64)
 {
-	struct efi_info *efi = &boot_params->efi_info;
 	unsigned long map_sz, key, desc_size, buff_size;
 	efi_memory_desc_t *mem_map;
 	struct setup_data *e820ext;
-	const char *signature;
 	__u32 e820ext_size;
-	__u32 nr_desc, prev_nr_desc;
 	efi_status_t status;
 	__u32 desc_version;
-	bool called_exit = false;
-	u8 nr_entries;
-	int i;
-
-	nr_desc = 0;
-	e820ext = NULL;
-	e820ext_size = 0;
+	struct exit_boot_struct priv;
 
-get_map:
-	status = efi_get_memory_map(sys_table, &mem_map, &map_sz, &desc_size,
-				    &desc_version, &key, &buff_size);
+	priv.boot_params = boot_params;
+	priv.efi = &boot_params->efi_info;
+	priv.e820ext = NULL;
+	priv.e820ext_size = 0;
+	priv.is64 = is64;
 
+	/* Might as well exit boot services now */
+	status = efi_exit_boot_services(sys_table, handle, &mem_map,
+					&map_sz, &desc_size, &desc_version,
+					&key, &priv, exit_boot_func);
 	if (status != EFI_SUCCESS)
 		return status;
 
-	prev_nr_desc = nr_desc;
-	nr_desc = map_sz / desc_size;
-	if (nr_desc > prev_nr_desc &&
-	    nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
-		u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
-
-		status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
-		if (status != EFI_SUCCESS)
-			goto free_mem_map;
-
-		efi_call_early(free_pool, mem_map);
-		goto get_map; /* Allocated memory, get map again */
-	}
-
-	signature = is64 ? EFI64_LOADER_SIGNATURE : EFI32_LOADER_SIGNATURE;
-	memcpy(&efi->efi_loader_signature, signature, sizeof(__u32));
-
-	efi->efi_systab = (unsigned long)sys_table;
-	efi->efi_memdesc_size = desc_size;
-	efi->efi_memdesc_version = desc_version;
-	efi->efi_memmap = (unsigned long)mem_map;
-	efi->efi_memmap_size = map_sz;
-
-#ifdef CONFIG_X86_64
-	efi->efi_systab_hi = (unsigned long)sys_table >> 32;
-	efi->efi_memmap_hi = (unsigned long)mem_map >> 32;
-#endif
-
-	/* Might as well exit boot services now */
-	status = efi_call_early(exit_boot_services, handle, key);
-	if (status != EFI_SUCCESS) {
-		/*
-		 * ExitBootServices() will fail if any of the event
-		 * handlers change the memory map. In which case, we
-		 * must be prepared to retry, but only once so that
-		 * we're guaranteed to exit on repeated failures instead
-		 * of spinning forever.
-		 */
-		if (called_exit)
-			goto free_mem_map;
-
-		called_exit = true;
-		efi_call_early(free_pool, mem_map);
-		goto get_map;
-	}
-
+	e820ext = priv.e820ext;
+	e820ext_size = priv.e820ext_size;
 	/* Historic? */
 	boot_params->alt_mem_k = 32 * 1024;
 
@@ -1087,10 +1095,6 @@ get_map:
 		return status;
 
 	return EFI_SUCCESS;
-
-free_mem_map:
-	efi_call_early(free_pool, mem_map);
-	return status;
 }
 
 /*
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Mark Rutland]

Hi,

On Sun, Jul 17, 2016 at 02:45:59PM -0600, Jeffrey Hugo wrote:
> +	if (status == EFI_BUFFER_TOO_SMALL ||
> +				(*buff_size - *map_size) / sizeof(*m) < 8) {
>  		efi_call_early(free_pool, m);
> +		/*
> +		 * Make sure there is 8 entries worth of headroom so that the
> +		 * buffer can be reused for a new map after allocations are
> +		 * no longer permitted.  Its unlikely that the map will grow to
> +		 * exceed this headroom once we are ready to trigger
> +		 * ExitBootServices()
> +		 */
> +		*map_size += sizeof(*m) * 8;

It's probably worth having something like...

  #define EFI_MMAP_NR_SLACK_SLOTS	8

... at the top of the file, and changing the comment here to not refer
to the specific value. That way we avoid duplicating the value, and
making it easier to modify / backport in future.

As a general note, it's a shame that we don't know desc_size first time
through. If descs grow, 8 * sizeof(*m) might be too small. That's an
existing problem though, so I guess it makes sense to be consistent and
use sizeof(*m) here.

Otherwise, this looks fine to me.

Thanks,
Mark.

Re: [PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Jeffrey Hugo]

On 7/18/2016 5:00 AM, Mark Rutland wrote:
> Hi,
>
> On Sun, Jul 17, 2016 at 02:45:59PM -0600, Jeffrey Hugo wrote:
>> +	if (status == EFI_BUFFER_TOO_SMALL ||
>> +				(*buff_size - *map_size) / sizeof(*m) < 8) {
>>  		efi_call_early(free_pool, m);
>> +		/*
>> +		 * Make sure there is 8 entries worth of headroom so that the
>> +		 * buffer can be reused for a new map after allocations are
>> +		 * no longer permitted.  Its unlikely that the map will grow to
>> +		 * exceed this headroom once we are ready to trigger
>> +		 * ExitBootServices()
>> +		 */
>> +		*map_size += sizeof(*m) * 8;
>
> It's probably worth having something like...
>
>   #define EFI_MMAP_NR_SLACK_SLOTS	8
>
> ... at the top of the file, and changing the comment here to not refer
> to the specific value. That way we avoid duplicating the value, and
> making it easier to modify / backport in future.

You are right, that does make sense.  I'll wait a bit to see if there is 
any other feedback, and add this suggestion to V2.

> As a general note, it's a shame that we don't know desc_size first time
> through. If descs grow, 8 * sizeof(*m) might be too small. That's an
> existing problem though, so I guess it makes sense to be consistent and
> use sizeof(*m) here.

Yep.  Unless I've missed something, it seems like the two options are 
either make a decent attempt to guess at desc_size the first time 
(current approach), or intentionally fail the first time just to get the 
size.  The current approach doesn't seem perfect, but it does have a 
chance at passing with just one attempt.  I couldn't see a better 
solution, but if someone else has an idea, I'd like to hear it.

>
> Otherwise, this looks fine to me.
>
> Thanks,
> Mark.
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Mark Rutland]

On Mon, Jul 18, 2016 at 09:03:31AM -0600, Jeffrey Hugo wrote:
> On 7/18/2016 5:00 AM, Mark Rutland wrote:
> >Hi,
> >
> >On Sun, Jul 17, 2016 at 02:45:59PM -0600, Jeffrey Hugo wrote:
> >>+	if (status == EFI_BUFFER_TOO_SMALL ||
> >>+				(*buff_size - *map_size) / sizeof(*m) < 8) {
> >> 		efi_call_early(free_pool, m);
> >>+		/*
> >>+		 * Make sure there is 8 entries worth of headroom so that the
> >>+		 * buffer can be reused for a new map after allocations are
> >>+		 * no longer permitted.  Its unlikely that the map will grow to
> >>+		 * exceed this headroom once we are ready to trigger
> >>+		 * ExitBootServices()
> >>+		 */
> >>+		*map_size += sizeof(*m) * 8;

> >As a general note, it's a shame that we don't know desc_size first time
> >through. If descs grow, 8 * sizeof(*m) might be too small. That's an
> >existing problem though, so I guess it makes sense to be consistent and
> >use sizeof(*m) here.
> 
> Yep.  Unless I've missed something, it seems like the two options
> are either make a decent attempt to guess at desc_size the first
> time (current approach), or intentionally fail the first time just
> to get the size.  The current approach doesn't seem perfect, but it
> does have a chance at passing with just one attempt.  I couldn't see
> a better solution, but if someone else has an idea, I'd like to hear
> it.

Sure, that was more of an observation than a critique. In the absence of
any suggestions better, sticking with sizeof(*m) makes sense.

If it does become a problem, we're likely to have to fix up other sites
regardless.

Thanks,
Mark.

Re: [PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Ard Biesheuvel]

On 18 July 2016 at 17:11, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Mon, Jul 18, 2016 at 09:03:31AM -0600, Jeffrey Hugo wrote:
>> On 7/18/2016 5:00 AM, Mark Rutland wrote:
>> >Hi,
>> >
>> >On Sun, Jul 17, 2016 at 02:45:59PM -0600, Jeffrey Hugo wrote:
>> >>+   if (status == EFI_BUFFER_TOO_SMALL ||
>> >>+                           (*buff_size - *map_size) / sizeof(*m) < 8) {
>> >>            efi_call_early(free_pool, m);
>> >>+           /*
>> >>+            * Make sure there is 8 entries worth of headroom so that the
>> >>+            * buffer can be reused for a new map after allocations are
>> >>+            * no longer permitted.  Its unlikely that the map will grow to
>> >>+            * exceed this headroom once we are ready to trigger
>> >>+            * ExitBootServices()
>> >>+            */
>> >>+           *map_size += sizeof(*m) * 8;
>
>> >As a general note, it's a shame that we don't know desc_size first time
>> >through. If descs grow, 8 * sizeof(*m) might be too small. That's an
>> >existing problem though, so I guess it makes sense to be consistent and
>> >use sizeof(*m) here.
>>
>> Yep.  Unless I've missed something, it seems like the two options
>> are either make a decent attempt to guess at desc_size the first
>> time (current approach), or intentionally fail the first time just
>> to get the size.  The current approach doesn't seem perfect, but it
>> does have a chance at passing with just one attempt.  I couldn't see
>> a better solution, but if someone else has an idea, I'd like to hear
>> it.
>
> Sure, that was more of an observation than a critique. In the absence of
> any suggestions better, sticking with sizeof(*m) makes sense.
>
> If it does become a problem, we're likely to have to fix up other sites
> regardless.
>

It is worth noting that the EDK2 implementation deliberately uses a
desc_size that exceeds the size of the type. So '8 * sizeof(*m)' is
almost never going to be 8 slots' worth of headroom.

-- 
Ard.

Re: [PATCH 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Jeffrey Hugo]

On 7/18/2016 9:14 AM, Ard Biesheuvel wrote:
> On 18 July 2016 at 17:11, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> On Mon, Jul 18, 2016 at 09:03:31AM -0600, Jeffrey Hugo wrote:
>>> On 7/18/2016 5:00 AM, Mark Rutland wrote:
>>>> Hi,
>>>>
>>>> On Sun, Jul 17, 2016 at 02:45:59PM -0600, Jeffrey Hugo wrote:
>>>>> +   if (status == EFI_BUFFER_TOO_SMALL ||
>>>>> +                           (*buff_size - *map_size) / sizeof(*m) < 8) {
>>>>>            efi_call_early(free_pool, m);
>>>>> +           /*
>>>>> +            * Make sure there is 8 entries worth of headroom so that the
>>>>> +            * buffer can be reused for a new map after allocations are
>>>>> +            * no longer permitted.  Its unlikely that the map will grow to
>>>>> +            * exceed this headroom once we are ready to trigger
>>>>> +            * ExitBootServices()
>>>>> +            */
>>>>> +           *map_size += sizeof(*m) * 8;
>>
>>>> As a general note, it's a shame that we don't know desc_size first time
>>>> through. If descs grow, 8 * sizeof(*m) might be too small. That's an
>>>> existing problem though, so I guess it makes sense to be consistent and
>>>> use sizeof(*m) here.
>>>
>>> Yep.  Unless I've missed something, it seems like the two options
>>> are either make a decent attempt to guess at desc_size the first
>>> time (current approach), or intentionally fail the first time just
>>> to get the size.  The current approach doesn't seem perfect, but it
>>> does have a chance at passing with just one attempt.  I couldn't see
>>> a better solution, but if someone else has an idea, I'd like to hear
>>> it.
>>
>> Sure, that was more of an observation than a critique. In the absence of
>> any suggestions better, sticking with sizeof(*m) makes sense.
>>
>> If it does become a problem, we're likely to have to fix up other sites
>> regardless.
>>
>
> It is worth noting that the EDK2 implementation deliberately uses a
> desc_size that exceeds the size of the type. So '8 * sizeof(*m)' is
> almost never going to be 8 slots' worth of headroom.
>

Good point, desc_size does exceed sizeof(*m) when I test an EDK2 
implementation.  I think I can rework things to use desc_size instead, 
so that we do get the expected 8 slots of headroom.  I'll incorporate 
that in V2.

-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.