[syslinux] [PATCH] efi: Call ExitBootServices at least twice
Celelibi
celelibi at gmail.com
Mon Nov 2 19:23:39 PST 2015
2015-11-02 12:34 UTC+01:00, Gene Cumm <gene.cumm at gmail.com>:
> 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?
I cannot call get_memory_map at all because I wouldn't control how
much memory is allocated, and this is a crucial point to comply as
much as possible with the UEFI specification. (See below.)
(BTW, I hope we agree that get_memory_map is the function provided by
gnu-efi and GetMemoryMap is the one provided by the firmware.)
>
>> + 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?
There are two different things happening here.
First, allocating some memory *does* increase the size of the memory
map. Thus, allocating the exact amount of memory GetMemoryMap said it
needs and calling it again with that buffer is likely to result in an
error EFI_BUFFER_TOO_SMALL.
Second, ExitBootServices does - again - increase the size of the
memory map on some hardware (although it is forbidden). So, if after
the first call to ExitBootServices we call GetMemoryMap with a buffer
that was exactly fitted for the map *before* calling ExitBootServices,
it is - again - likely to result in an EFI_BUFFER_TOO_SMALL. And this
time, things are really bad since we're not supposed to perform a
memory allocation after we called ExitBootServices.
I specifically choosed that on the first call to AllocatePool, the
variable allocsizeadd is 4 times sizeof(EFI_MEMORY_DESCRIPTOR), so
that on my (buggy) hardware the second call to get_memory_map_realloc,
the memory map can fit into the buffer on the first try. Thus the
non-standard behavior of the firmware is worked around with only
standard code.
If some UEFI implementations require a buffer yet bigger than that for
the second round of GetMemoryMap, well... on a pure standard way, we
lost the game. After calling ExitBootServices the first time we're not
supposed to perform a memory allocation and we can't go back either.
So, why not try a memory allocation anyway? It works on some
implementations (including mine) despite being forbidden by the
specification, but we're in this situation because the firmware was
already not complying with the UEFI spec.
All this is a bit complex. But working around bugs is never trivial.
Something similar is implemented in at least GRUB and Linux.
GRUB fiddle around to find the optimal size for the memory map and in
the end add 3 pages of memory for safety.
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/i386/linux.c?id=206676601eb853fc319df14cd3398fbdfde665ac#n104
GRUB then retry in an infinite loop to call GetMemoryMap +
ExitBootServices. On each iteration it allocates a copy of the memory
map and free it in case of failure. However, GRUB seems to have its
own memory management, which allocate a quarter of the available
memory on startup. This allows those allocation on each iteration to
not disturb the memory map while trying to call ExitBootServices.
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/kern/efi/mm.c?id=206676601eb853fc319df14cd3398fbdfde665ac#n148
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/kern/efi/mm.c?id=206676601eb853fc319df14cd3398fbdfde665ac#n494
Linux does something similar. It calls get_memory_map +
exit_boot_services a maximum of two times. However, it allows itself
to always call allocate_pool (in efi_get_memory_map) even after
exit_boot_services failed once. Which does not comply with the UEFI
specification.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/x86/boot/compressed/eboot.c?id=e86328c489d7ecdca99410a06a3f448caf7857bf#n1299
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/libstub/efi-stub-helper.c?id=e86328c489d7ecdca99410a06a3f448caf7857bf#n66
>
>> + } 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
>
I hope it's a bit clearer now. Sorry for the lengthy text. ^^
Celelibi
More information about the Syslinux
mailing list