[PATCH V2 0/4] Handle EFI_INVALID_PARAMETER from ExitBootServices

[PATCH V2 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.

[V2]
-Define EFI_MMAP_NR_SLACK_SLOTS per Mark Rutland
-Use desc_size as firmware may exceed the defined struct size per Ard Biesheuvel

[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 | 129 ++++++++++++++++++++++--
 drivers/firmware/efi/libstub/fdt.c             |  52 +++++++---
 drivers/firmware/efi/libstub/random.c          |   3 +-
 include/linux/efi.h                            |  22 ++++-
 5 files changed, 249 insertions(+), 89 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 V2 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.

Change-Id: Ib0686811581c59eee2eb60b4b62e1628e649d6f0
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 | 36 +++++++++++++++++++-------
 drivers/firmware/efi/libstub/fdt.c             |  8 +++---
 drivers/firmware/efi/libstub/random.c          |  3 ++-
 include/linux/efi.h                            |  3 ++-
 5 files changed, 37 insertions(+), 17 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..3071269 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -41,6 +41,8 @@ static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
 #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
 #endif
 
+#define EFI_MMAP_NR_SLACK_SLOTS	8
+
 struct file_info {
 	efi_file_handle_t *handle;
 	u64 size;
@@ -68,20 +70,24 @@ 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;
 	unsigned long key;
 	u32 desc_version;
 
-	*map_size = sizeof(*m) * 32;
+	*desc_size = sizeof(*m);
+	*map_size = *desc_size * 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);
+	*map_size += *desc_size;
+	*buff_size = *map_size;
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
 				*map_size, (void **)&m);
 	if (status != EFI_SUCCESS)
@@ -91,8 +97,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) / *desc_size < 8) {
 		efi_call_early(free_pool, m);
+		/*
+		 * Make sure there is some entries 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 += *desc_size * EFI_MMAP_NR_SLACK_SLOTS;
 		goto again;
 	}
 
@@ -113,13 +128,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 +160,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 +168,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 +246,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 V2 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 3071269..d5be0b5 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -720,3 +720,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 V2 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 V2 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 V2 1/4] efi/libstub: Allocate headspace in efi_get_memory_map()

[Matt Fleming]

On Thu, 21 Jul, at 02:28:11PM, Jeffrey Hugo wrote:
> 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.
> 
> Change-Id: Ib0686811581c59eee2eb60b4b62e1628e649d6f0

Please don't include these tags in your patch submission - they don't
mean anything in the upstream kernel and there's always the chance
I'll forget to strip it before applying your patch.

> 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 | 36 +++++++++++++++++++-------
>  drivers/firmware/efi/libstub/fdt.c             |  8 +++---
>  drivers/firmware/efi/libstub/random.c          |  3 ++-
>  include/linux/efi.h                            |  3 ++-
>  5 files changed, 37 insertions(+), 17 deletions(-)

[...]

> index 3bd127f9..3071269 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -41,6 +41,8 @@ static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
>  #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
>  #endif
>  
> +#define EFI_MMAP_NR_SLACK_SLOTS	8
> +
>  struct file_info {
>  	efi_file_handle_t *handle;
>  	u64 size;
> @@ -68,20 +70,24 @@ 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;
>  	unsigned long key;
>  	u32 desc_version;
>  
> -	*map_size = sizeof(*m) * 32;
> +	*desc_size = sizeof(*m);
> +	*map_size = *desc_size * 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);
> +	*map_size += *desc_size;
> +	*buff_size = *map_size;
>  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>  				*map_size, (void **)&m);
>  	if (status != EFI_SUCCESS)

Isn't this chunk of code unnecessary now? If we think 8 entries is
enough headroom for all scenarios then there's no need to allocate 9.

> @@ -91,8 +97,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) / *desc_size < 8) {

Please pull this expression into a static inline wrapper, e.g.

static inline bool mmap_has_headroom(unsigned long buff_size,
				     unsigned long map_size,
				     unsigned long desc_size)
{
	unsigned long slack = buff_size - map_size;

    	return slack / desc_size >= EFI_MMAP_NR_SLACK_SLOTS;
}

...

	if (status == EFI_BUFFER_TOO_SMALL ||
	    !mmap_has_headroom(*buff_size, *map_size, *desc_size)) {

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

[Jeffrey Hugo]

On 7/27/2016 8:57 AM, Matt Fleming wrote:
> On Thu, 21 Jul, at 02:28:11PM, Jeffrey Hugo wrote:
>> 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.
>>
>> Change-Id: Ib0686811581c59eee2eb60b4b62e1628e649d6f0
>
> Please don't include these tags in your patch submission - they don't
> mean anything in the upstream kernel and there's always the chance
> I'll forget to strip it before applying your patch.

Doh, sorry about that.  I apparently was not as vigilant as I should 
have been.  I'll make sure to strip it.

>
>> 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 | 36 +++++++++++++++++++-------
>>  drivers/firmware/efi/libstub/fdt.c             |  8 +++---
>>  drivers/firmware/efi/libstub/random.c          |  3 ++-
>>  include/linux/efi.h                            |  3 ++-
>>  5 files changed, 37 insertions(+), 17 deletions(-)
>
> [...]
>
>> index 3bd127f9..3071269 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -41,6 +41,8 @@ static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
>>  #define EFI_ALLOC_ALIGN		EFI_PAGE_SIZE
>>  #endif
>>
>> +#define EFI_MMAP_NR_SLACK_SLOTS	8
>> +
>>  struct file_info {
>>  	efi_file_handle_t *handle;
>>  	u64 size;
>> @@ -68,20 +70,24 @@ 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;
>>  	unsigned long key;
>>  	u32 desc_version;
>>
>> -	*map_size = sizeof(*m) * 32;
>> +	*desc_size = sizeof(*m);
>> +	*map_size = *desc_size * 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);
>> +	*map_size += *desc_size;
>> +	*buff_size = *map_size;
>>  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>  				*map_size, (void **)&m);
>>  	if (status != EFI_SUCCESS)
>
> Isn't this chunk of code unnecessary now? If we think 8 entries is
> enough headroom for all scenarios then there's no need to allocate 9.

I guess the original logic still made a bit of sense to me at the time, 
but I'm not attached to it.  Will change.

>
>> @@ -91,8 +97,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) / *desc_size < 8) {
>
> Please pull this expression into a static inline wrapper, e.g.
>
> static inline bool mmap_has_headroom(unsigned long buff_size,
> 				     unsigned long map_size,
> 				     unsigned long desc_size)
> {
> 	unsigned long slack = buff_size - map_size;
>
>     	return slack / desc_size >= EFI_MMAP_NR_SLACK_SLOTS;
> }
>
> ...
>
> 	if (status == EFI_BUFFER_TOO_SMALL ||
> 	    !mmap_has_headroom(*buff_size, *map_size, *desc_size)) {
>

Sure.


-- 
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 V2 2/4] efi/libstub: Introduce ExitBootServices helper

[Matt Fleming]

On Thu, 21 Jul, at 02:28:12PM, Jeffrey Hugo wrote:
> 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.

The thing missing from this changelog is the fact that you have run
into this problem in the real world - it's not just a matter of spec
conformance, this is required to boot machines in the wild.

In other words, the current changelog does not reflect the importance
of these patches.

> 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 3071269..d5be0b5 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -720,3 +720,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.
> + */

Why have you made priv_func optional? This series does not contain any
callers of efi_exit_boot_services() that pass NULL for 'priv_func', so
making it optional is over-engineering.

> +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))

This needs a struct passing as the parameter not this huge list.

In fact, efi_get_memory_map() would also benefit from passing a single
struct as an argument.

Also, don't define the function signature inside of another function's
prototype - use a typedef instead.

> +{
> +	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;
> +	}
> +

Why not move the priv_func call until after we know ExitBootServices()
returned successfully? That way we don't have to make two calls and
the callee doesn't need to handle that.

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

[Matt Fleming]

On Thu, 21 Jul, at 02:28:14PM, Jeffrey Hugo wrote:
> 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(-)

FYI, this patch prevents my x86-64 test machines from booting. Of
course, because it's the early boot code there's no useful output on
the console other than "efi_main() failed!".

I'll debug further tomorrow.

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

[Jeffrey Hugo]

On 7/27/2016 12:08 PM, Matt Fleming wrote:
> On Thu, 21 Jul, at 02:28:14PM, Jeffrey Hugo wrote:
>> 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(-)
>
> FYI, this patch prevents my x86-64 test machines from booting. Of
> course, because it's the early boot code there's no useful output on
> the console other than "efi_main() failed!".
>
> I'll debug further tomorrow.
>

Doh.  I'm sorry for that.  Clearly my limited x86 testing was 
insufficient.  I'll see if I reproduce on my end for parallel debug.

-- 
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 V2 2/4] efi/libstub: Introduce ExitBootServices helper

[Jeffrey Hugo]

On 7/27/2016 12:03 PM, Matt Fleming wrote:
> On Thu, 21 Jul, at 02:28:12PM, Jeffrey Hugo wrote:
>> 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.
>
> The thing missing from this changelog is the fact that you have run
> into this problem in the real world - it's not just a matter of spec
> conformance, this is required to boot machines in the wild.
>
> In other words, the current changelog does not reflect the importance
> of these patches.

Sure, I'll amend the text here to clarify that point.

>
>> 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 3071269..d5be0b5 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -720,3 +720,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.
>> + */
>
> Why have you made priv_func optional? This series does not contain any
> callers of efi_exit_boot_services() that pass NULL for 'priv_func', so
> making it optional is over-engineering.

It seemed trivial to make it optional, and reasonable that some future 
client may not require the functionality, so passing in a do nothing 
function seemed inelegant.

Since you feel this is over-engineering, I'll make it mandatory.

>
>> +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))
>
> This needs a struct passing as the parameter not this huge list.
>
> In fact, efi_get_memory_map() would also benefit from passing a single
> struct as an argument.
>
> Also, don't define the function signature inside of another function's
> prototype - use a typedef instead.

Sure, I'll look at addressing the function signature here.

>
>> +{
>> +	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;
>> +	}
>> +
>
> Why not move the priv_func call until after we know ExitBootServices()
> returned successfully? That way we don't have to make two calls and
> the callee doesn't need to handle that.
>

It keeps the code flow for FDT (patch 3 in the series) and x86 (patch 4) 
the same as before, and x86 seems to require that its function occurs 
before ExitBootServices().

exit_boot() calls alloc_e820ext() based on the contents of the current 
memory map, which invokes allocate_pool().  Per my understanding of the 
spec, allocate_pool() should not be called after ExitBootServices(), and 
per my understanding of the x86 code, alloc_e820ext() needs to be called 
between getting the current memory map, and ExitBootServices().  Am I 
incorrect, and there is an option to restructure the x86 code so that 
this ordering is not required?

-- 
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 V2 2/4] efi/libstub: Introduce ExitBootServices helper

[Matt Fleming]

On Wed, 27 Jul, at 01:21:17PM, Jeffrey Hugo wrote:
> On 7/27/2016 12:03 PM, Matt Fleming wrote:
> >
> >Why not move the priv_func call until after we know ExitBootServices()
> >returned successfully? That way we don't have to make two calls and
> >the callee doesn't need to handle that.
> >
> 
> It keeps the code flow for FDT (patch 3 in the series) and x86 (patch 4) the
> same as before, and x86 seems to require that its function occurs before
> ExitBootServices().
> 
> exit_boot() calls alloc_e820ext() based on the contents of the current
> memory map, which invokes allocate_pool().  Per my understanding of the
> spec, allocate_pool() should not be called after ExitBootServices(), and per
> my understanding of the x86 code, alloc_e820ext() needs to be called between
> getting the current memory map, and ExitBootServices().  Am I incorrect, and
> there is an option to restructure the x86 code so that this ordering is not
> required?

On second thoughts, it's fine as-is.