Syslinux - Nov 2015 - [PATCH] efi: Call ExitBootServices at least twice

From: Sylvain Gault <sylvain.gault at gmail.com>

On some architecture, including my hardware, the function ExitBootServices may
need to be called twice in order to successfully exit the boot services. As
stated by the UEFI spec, the first call to ExitBootServices may perform a
partial shutdown of the services. It seems that during this partial shutdown,
the memory map can be modified, thus making another call to GetMemoryMap
necessary. However GetMemoryMap needs a buffer to fill, but the memory
allocation functions should not be used after the first call to
ExitBootServices despite still working on my hardware. This patch solve this
problem as follow:
- Get the memory map and allocate a buffer larger than necessary
- Call ExitBootServices
  - If it succeed, we're done
- Get the new memory map in the same buffer
- Reallocate this buffer if it is too small anyway

So the call to the memory allocation functions may be called after
ExitBootServices only if the existing buffer was too small despite being
already larger than needed on the first call to GetMemoryMap.

This issue of calling ExitBootServices twice is mentioned around the web and
this patch is quite close to the one implemented in Linux.

Sylvain Gault (1):
  efi: Call ExitBootServices at least twice

 efi/main.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 11 deletions(-)

-- 
2.5.0

From: Sylvain Gault <sylvain.gault at gmail.com>

Some firmware implementations may need ExitBootServices to be called
twice. The second time with an updated memory map key.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/main.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 11 deletions(-)

diff --git a/efi/main.c b/efi/main.c
index 6dbc259..a39ab43 100644
--- a/efi/main.c
+++ b/efi/main.c
@@ -1001,19 +1001,79 @@ static int handle_ramdisks(struct linux_header *hdr,
 	return 0;
 }
 
+/*
+ * Like get_memory_map but try to use the given buffer first, reallocate it if
+ * it's too small and always set the allocated size.
+ */
+static EFI_MEMORY_DESCRIPTOR *
+get_memory_map_realloc(EFI_MEMORY_DESCRIPTOR *map, UINTN *allocsize,
+		       UINTN *nr_entries, UINTN *key, UINTN *desc_sz,
+		       UINT32 *desc_ver)
+{
+	EFI_STATUS status;
+	UINTN size, allocsizeadd;
+
+	allocsizeadd = sizeof(*map) * 2;
+
+	do {
+		size = *allocsize;
+		status = uefi_call_wrapper(BS->GetMemoryMap, 5, &size, map, key,
+		                           desc_sz, desc_ver);
+
+		if (status == EFI_BUFFER_TOO_SMALL) {
+			if (map)
+				FreePool(map);
+			allocsizeadd *= 2;
+			*allocsize = size + allocsizeadd;
+			map = AllocatePool(*allocsize);
+		}
+	} while (status == EFI_BUFFER_TOO_SMALL);
+
+	if (status == EFI_SUCCESS) {
+		*nr_entries = size / *desc_sz;
+		return map;
+	}
+
+	if (map)
+		FreePool(map);
+	return NULL;
+}
+
 static int exit_boot(struct boot_params *bp)
 {
 	struct e820_entry *e820buf, *e;
 	EFI_MEMORY_DESCRIPTOR *map;
 	EFI_STATUS status;
 	uint32_t e820_type;
-	UINTN i, nr_entries, key, desc_sz;
+	UINTN i, nr_entries, key, desc_sz, allocsize;
 	UINT32 desc_ver;
+	int retry;
 
-	/* Build efi memory map */
-	map = get_memory_map(&nr_entries, &key, &desc_sz, &desc_ver);
-	if (!map)
+	/*
+	 * Build efi memory map and call ExitBootServices as soon after as
+	 * possible. Some implementations need two calls to ExitBootServices.
+	 */
+
+	map = NULL;
+	allocsize = 0;
+	retry = 0;
+	do {
+		map = get_memory_map_realloc(map, &allocsize, &nr_entries, &key,
+		                             &desc_sz, &desc_ver);
+		if (!map)
+			return -1;
+
+		status = uefi_call_wrapper(BS->ExitBootServices, 2,
+		                           image_handle, key);
+		retry++;
+	} while (status != EFI_SUCCESS && retry < 3);
+
+	if (status != EFI_SUCCESS) {
+		printf("Failed to exit boot services: 0x%016lx\n", status);
+		FreePool(map);
 		return -1;
+	}
+
 
 	bp->efi.memmap = (uint32_t)(unsigned long)map;
 	bp->efi.memmap_size = nr_entries * desc_sz;
@@ -1088,13 +1148,6 @@ static int exit_boot(struct boot_params *bp)
 
 	bp->e820_entries = e - e820buf;
 
-	status = uefi_call_wrapper(BS->ExitBootServices, 2, image_handle, key);
-	if (status != EFI_SUCCESS) {
-		printf("Failed to exit boot services: 0x%016lx\n", status);
-		FreePool(map);
-		return -1;
-	}
-
 	return 0;
 }
 
-- 
2.5.0

2015-08-26 5:54 UTC+02:00, celelibi at gmail.com <celelibi at
gmail.com>:
> From: Sylvain Gault <sylvain.gault at gmail.com>
>
> On some architecture, including my hardware, the function ExitBootServices
> may
> need to be called twice in order to successfully exit the boot services. As
> stated by the UEFI spec, the first call to ExitBootServices may perform a
> partial shutdown of the services. It seems that during this partial
> shutdown,
> the memory map can be modified, thus making another call to GetMemoryMap
> necessary. However GetMemoryMap needs a buffer to fill, but the memory
> allocation functions should not be used after the first call to
> ExitBootServices despite still working on my hardware. This patch solve
> this
> problem as follow:
> - Get the memory map and allocate a buffer larger than necessary
> - Call ExitBootServices
>   - If it succeed, we're done
> - Get the new memory map in the same buffer
> - Reallocate this buffer if it is too small anyway
>
> So the call to the memory allocation functions may be called after
> ExitBootServices only if the existing buffer was too small despite being
> already larger than needed on the first call to GetMemoryMap.
>
> This issue of calling ExitBootServices twice is mentioned around the web
> and
> this patch is quite close to the one implemented in Linux.
>
> Sylvain Gault (1):
>   efi: Call ExitBootServices at least twice
>
>  efi/main.c | 75
> +++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 11 deletions(-)
>
> --
> 2.5.0
>
>
I included a cover letter because I had things to explain that didn't
fit into the commit message, but the patch is in the other mail
obviously.

On Wed, 26 Aug 2015 05:54:04 +0200
celelibi--- via Syslinux <syslinux at zytor.com> wrote:
> From: Sylvain Gault <sylvain.gault at gmail.com>
> 
> Some firmware implementations may need ExitBootServices to be called
> twice. The second time with an updated memory map key.
> 
> Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
> ---
>  efi/main.c | 75
> +++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file
> changed, 64 insertions(+), 11 deletions(-)
> 
> diff --git a/efi/main.c b/efi/main.c
> index 6dbc259..a39ab43 100644
> --- a/efi/main.c
> +++ b/efi/main.c
> @@ -1001,19 +1001,79 @@ static int handle_ramdisks(struct
> linux_header *hdr, return 0;
>  }
>  
> +/*
> + * Like get_memory_map but try to use the given buffer first,
> reallocate it if
> + * it's too small and always set the allocated size.
> + */
> +static EFI_MEMORY_DESCRIPTOR *
> +get_memory_map_realloc(EFI_MEMORY_DESCRIPTOR *map, UINTN *allocsize,
> +		       UINTN *nr_entries, UINTN *key, UINTN *desc_sz,
> +		       UINT32 *desc_ver)
> +{
> +	EFI_STATUS status;
> +	UINTN size, allocsizeadd;
> +
> +	allocsizeadd = sizeof(*map) * 2;
> +
> +	do {
> +		size = *allocsize;
> +		status = uefi_call_wrapper(BS->GetMemoryMap, 5,
> &size, map, key,
> +		                           desc_sz, desc_ver);
> +
> +		if (status == EFI_BUFFER_TOO_SMALL) {
> +			if (map)
> +				FreePool(map);
> +			allocsizeadd *= 2;
> +			*allocsize = size + allocsizeadd;
> +			map = AllocatePool(*allocsize);
Why don't you use ReallocatePool()?

Besides, you could just return inside loop when GetMemoryMap() returns
EFI_SUCCESS and thus avoiding to check twice for EFI_BUFFER_TOO_SMALL
prior to "status == EFI_SUCCESS" outside loop.

Thanks,

Paulo
> +		}
> +	} while (status == EFI_BUFFER_TOO_SMALL);
> +
> +	if (status == EFI_SUCCESS) {
> +		*nr_entries = size / *desc_sz;
> +		return map;
> +	}
> +
> +	if (map)
> +		FreePool(map);
> +	return NULL;
> +}
> +
>  static int exit_boot(struct boot_params *bp)
>  {
>  	struct e820_entry *e820buf, *e;
>  	EFI_MEMORY_DESCRIPTOR *map;
>  	EFI_STATUS status;
>  	uint32_t e820_type;
> -	UINTN i, nr_entries, key, desc_sz;
> +	UINTN i, nr_entries, key, desc_sz, allocsize;
>  	UINT32 desc_ver;
> +	int retry;
>  
> -	/* Build efi memory map */
> -	map = get_memory_map(&nr_entries, &key, &desc_sz,
&desc_ver);
> -	if (!map)
> +	/*
> +	 * Build efi memory map and call ExitBootServices as soon
> after as
> +	 * possible. Some implementations need two calls to
> ExitBootServices.
> +	 */
> +
> +	map = NULL;
> +	allocsize = 0;
> +	retry = 0;
> +	do {
> +		map = get_memory_map_realloc(map, &allocsize,
> &nr_entries, &key,
> +		                             &desc_sz, &desc_ver);
> +		if (!map)
> +			return -1;
> +
> +		status = uefi_call_wrapper(BS->ExitBootServices, 2,
> +		                           image_handle, key);
> +		retry++;
> +	} while (status != EFI_SUCCESS && retry < 3);
> +
> +	if (status != EFI_SUCCESS) {
> +		printf("Failed to exit boot services: 0x%016lx\n",
> status);
> +		FreePool(map);
>  		return -1;
> +	}
> +
>  
>  	bp->efi.memmap = (uint32_t)(unsigned long)map;
>  	bp->efi.memmap_size = nr_entries * desc_sz;
> @@ -1088,13 +1148,6 @@ static int exit_boot(struct boot_params *bp)
>  
>  	bp->e820_entries = e - e820buf;
>  
> -	status = uefi_call_wrapper(BS->ExitBootServices, 2,
> image_handle, key);
> -	if (status != EFI_SUCCESS) {
> -		printf("Failed to exit boot services: 0x%016lx\n",
> status);
> -		FreePool(map);
> -		return -1;
> -	}
> -
>  	return 0;
>  }
>  


-- 
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.

On Tue, Aug 25, 2015 at 11:54 PM, celelibi--- via Syslinux
<syslinux at zytor.com> wrote:
> From: Sylvain Gault <sylvain.gault at gmail.com>
>
> Some firmware implementations may need ExitBootServices to be called
> twice. The second time with an updated memory map key.
>
> Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
> ---
>  efi/main.c | 75
+++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 11 deletions(-)
>
> diff --git a/efi/main.c b/efi/main.c
> index 6dbc259..a39ab43 100644
> --- a/efi/main.c
> +++ b/efi/main.c
> @@ -1001,19 +1001,79 @@ static int handle_ramdisks(struct linux_header
*hdr,
>         return 0;
>  }
>
> +/*
> + * Like get_memory_map but try to use the given buffer first, reallocate
it if
> + * it's too small and always set the allocated size.
> + */
> +static EFI_MEMORY_DESCRIPTOR *
> +get_memory_map_realloc(EFI_MEMORY_DESCRIPTOR *map, UINTN *allocsize,
> +                      UINTN *nr_entries, UINTN *key, UINTN *desc_sz,
> +                      UINT32 *desc_ver)
> +{
> +       EFI_STATUS status;
> +       UINTN size, allocsizeadd;
> +
> +       allocsizeadd = sizeof(*map) * 2;
> +
> +       do {
> +               size = *allocsize;
> +               status = uefi_call_wrapper(BS->GetMemoryMap, 5,
&size, map, key,
> +                                          desc_sz, desc_ver);
Why not check for NULL and call get_memory_map() first?
> +               if (status == EFI_BUFFER_TOO_SMALL) {
> +                       if (map)
> +                               FreePool(map);
> +                       allocsizeadd *= 2;
> +                       *allocsize = size + allocsizeadd;
> +                       map = AllocatePool(*allocsize);
> +               }
Why guess at the exact size needed?  GetMemoryMap is required to tell
us the exact resize needed in the return of "size", even in UEFI v2.0.
Was there a system that didn't follow this?
> +       } while (status == EFI_BUFFER_TOO_SMALL);
> +
> +       if (status == EFI_SUCCESS) {
> +               *nr_entries = size / *desc_sz;
> +               return map;
> +       }
> +
> +       if (map)
> +               FreePool(map);
> +       return NULL;
> +}
> +
>  static int exit_boot(struct boot_params *bp)
>  {
>         struct e820_entry *e820buf, *e;
>         EFI_MEMORY_DESCRIPTOR *map;
>         EFI_STATUS status;
>         uint32_t e820_type;
> -       UINTN i, nr_entries, key, desc_sz;
> +       UINTN i, nr_entries, key, desc_sz, allocsize;
>         UINT32 desc_ver;
> +       int retry;
>
> -       /* Build efi memory map */
> -       map = get_memory_map(&nr_entries, &key, &desc_sz,
&desc_ver);
> -       if (!map)
> +       /*
> +        * Build efi memory map and call ExitBootServices as soon after as
> +        * possible. Some implementations need two calls to
ExitBootServices.
> +        */
> +
> +       map = NULL;
> +       allocsize = 0;
> +       retry = 0;
> +       do {
> +               map = get_memory_map_realloc(map, &allocsize,
&nr_entries, &key,
> +                                            &desc_sz, &desc_ver);
> +               if (!map)
> +                       return -1;
> +
> +               status = uefi_call_wrapper(BS->ExitBootServices, 2,
> +                                          image_handle, key);
> +               retry++;
> +       } while (status != EFI_SUCCESS && retry < 3);
> +
> +       if (status != EFI_SUCCESS) {
> +               printf("Failed to exit boot services:
0x%016lx\n", status);
> +               FreePool(map);
>                 return -1;
> +       }
> +
Call ExitBootServices() prior to any map parsing.  This makes sense as
long as no other functions get added after this that might want to
allocate more RAM.  This function looks good from here out.
>         bp->efi.memmap = (uint32_t)(unsigned long)map;
>         bp->efi.memmap_size = nr_entries * desc_sz;
> @@ -1088,13 +1148,6 @@ static int exit_boot(struct boot_params *bp)
>
>         bp->e820_entries = e - e820buf;
>
> -       status = uefi_call_wrapper(BS->ExitBootServices, 2,
image_handle, key);
> -       if (status != EFI_SUCCESS) {
> -               printf("Failed to exit boot services:
0x%016lx\n", status);
> -               FreePool(map);
> -               return -1;
> -       }
> -
>         return 0;
>  }
-- 
-Gene

From: Sylvain Gault <sylvain.gault at gmail.com>

Some firmware implementations may need ExitBootServices to be called
twice. The second time with an updated memory map key.

Signed-off-by: Sylvain Gault <sylvain.gault at gmail.com>
---
 efi/main.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 81 insertions(+), 11 deletions(-)

diff --git a/efi/main.c b/efi/main.c
index fd95f5c..94353c7 100644
--- a/efi/main.c
+++ b/efi/main.c
@@ -1001,19 +1001,96 @@ static int handle_ramdisks(struct linux_header *hdr,
 	return 0;
 }
 
+/*
+ * Try to use the given map buffer when calling GetMemoryMap.
+ * Reallocate the buffer if if it's too small.
+ * Make sure there is always at least adddesc free descriptors in the buffer.
+ * Always set *allocsize to the size of the buffer.
+ */
+static EFI_MEMORY_DESCRIPTOR *
+get_memory_map_realloc(EFI_MEMORY_DESCRIPTOR *map, UINTN *allocsize,
+		       UINTN *nr_entries, UINTN *key, UINTN *desc_sz,
+		       UINT32 *desc_ver, UINTN nfreedesc)
+{
+	EFI_STATUS status;
+	UINTN size, doubleaddsize, constaddsize;
+
+	size = *allocsize;
+
+	/*
+	 * First try with the given buffer.
+	 * Also get the minimal buffer size needed and the size of a single
+	 * memory descriptor.
+	 */
+	status = uefi_call_wrapper(BS->GetMemoryMap, 5, &size, map, key,
+	                           desc_sz, desc_ver);
+
+	constaddsize = nfreedesc * *desc_sz;
+	doubleaddsize = *desc_sz;
+
+	while (status == EFI_BUFFER_TOO_SMALL ||
+	       (status == EFI_SUCCESS && size + constaddsize > *allocsize))
{
+		if (map)
+			FreePool(map);
+
+		doubleaddsize *= 2;
+		size += doubleaddsize + constaddsize;
+		*allocsize = size;
+		map = AllocatePool(size);
+
+		status = uefi_call_wrapper(BS->GetMemoryMap, 5, &size, map, key,
+		                           desc_sz, desc_ver);
+	}
+
+	if (status == EFI_SUCCESS) {
+		*nr_entries = size / *desc_sz;
+		return map;
+	}
+
+	if (map)
+		FreePool(map);
+	return NULL;
+}
+
 static int exit_boot(struct boot_params *bp)
 {
 	struct e820_entry *e820buf, *e;
 	EFI_MEMORY_DESCRIPTOR *map;
 	EFI_STATUS status;
 	uint32_t e820_type;
-	UINTN i, nr_entries, key, desc_sz;
+	UINTN i, nr_entries, key, desc_sz, allocsize, addsize;
 	UINT32 desc_ver;
+	int retry;
 
-	/* Build efi memory map */
-	map = get_memory_map(&nr_entries, &key, &desc_sz, &desc_ver);
-	if (!map)
+	/*
+	 * Build efi memory map and call ExitBootServices as soon after as
+	 * possible. Some implementations need two calls to ExitBootServices.
+	 */
+
+	map = NULL;
+	allocsize = 0;
+	addsize = 4;
+	retry = 0;
+	do {
+		map = get_memory_map_realloc(map, &allocsize, &nr_entries, &key,
+		                             &desc_sz, &desc_ver, addsize);
+		if (!map)
+			return -1;
+
+		/* Only request additional memory on the first try. */
+		addsize = 0;
+
+		status = uefi_call_wrapper(BS->ExitBootServices, 2,
+		                           image_handle, key);
+		retry++;
+	} while (status != EFI_SUCCESS && retry < 2);
+
+	if (status != EFI_SUCCESS) {
+		printf("Failed to exit boot services: 0x%016lx\n", status);
+		FreePool(map);
 		return -1;
+	}
+
 
 	bp->efi.memmap = (uint32_t)(unsigned long)map;
 	bp->efi.memmap_size = nr_entries * desc_sz;
@@ -1088,13 +1165,6 @@ static int exit_boot(struct boot_params *bp)
 
 	bp->e820_entries = e - e820buf;
 
-	status = uefi_call_wrapper(BS->ExitBootServices, 2, image_handle, key);
-	if (status != EFI_SUCCESS) {
-		printf("Failed to exit boot services: 0x%016lx\n", status);
-		FreePool(map);
-		return -1;
-	}
-
 	return 0;
 }
 
-- 
2.6.2