[syslinux] [PATCH] efi: Call ExitBootServices at least twice
Gene Cumm
gene.cumm at gmail.com
Tue Nov 3 02:24:48 PST 2015
On Mon, Nov 2, 2015 at 10:23 PM, Celelibi <celelibi at gmail.com> wrote:
> 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.)
No, it's the function you effectively replaced.
>>> + 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.
I see. In this way you get "size" (the size you actually need upon
return from GetMemoryMap) plus 4 more descriptors, hopefully enough to
handle the FreePool()/AllocatePool() that's to follow. It's quite
probable that LibMemoryMap (gnu-efi) implements this double call too.
> 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.
After calling ExitBootServices _successfully_.
> 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.
If ExitBootServices is unsuccessful, UEFI firmware memory allocation
should still be valid. I don't believe it's forbidden at all since
ExitBootServices() was NOT successful.
> All this is a bit complex. But working around bugs is never trivial.
> Something similar is implemented in at least GRUB and Linux.
Results vary but can often be non-trivial.
> 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.
It's also noted in the ExitBootServices() function to call
GetMemoryMap() then immediately call ExitBootServices().
>>> 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;
>>> }
> I hope it's a bit clearer now. Sorry for the lengthy text. ^^
>
>
> Celelibi
I'd say it makes more sense.
--
-Gene
More information about the Syslinux
mailing list