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

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

2015-11-03 11:24 UTC+01:00, Gene Cumm <gene.cumm at
gmail.com>:
> 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.
The way LibMemoryMap implement it is that on the first try it allocate
enough space for a single entry. Then, in a loop, allocate the size
returned by GetMemoryMap and call it again. This should converge to a
fixed point after a few loops (up to 4 under the senarios I could
imagine).

But the main reason I switched to not use it is not efficiency, it's
because it always allocate minimal buffer for the memory map and
cannot take a preallocated buffer. Thus forcing to allocate a new one
after ExitBootServices has been called (see below).

>
>> 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_.
(See below.)
>
>> 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.
I disagree with this. The documentation of ExitBootServices(), section
6.4, say (typo included):
"If MapKey value is incorrect, ExitBootServices() returns
EFI_INVALID_PARAMETER and GetMemoryMap() with ExitBootServices() must
be called again. Firmware implementation may choose to do a partial
shutdown of the boot services during the first call to
ExitBootServices(). EFI OS loader should not make calls to any boot
service function other then GetMemoryMap() after the first call to
ExitBootServices()."

And the memory allocation services are boot services. So I think it's
pretty clear from the last sentence that we shouldn't use the memory
allocation services after a failed call to ExitBootServices.

When they say "first call", it's not "first successful
call". And
since the sentences before talk about how ExitBootServices can fail
and must be called again, I think "first call" definitely include the
case when this first call failed.
>
>> 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
>
Celelibi

On Tue, Nov 3, 2015 at 8:37 AM, Celelibi <celelibi at gmail.com>
wrote:
> 2015-11-03 11:24 UTC+01:00, Gene Cumm <gene.cumm at gmail.com>:
>>> 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.
>
> I disagree with this. The documentation of ExitBootServices(), section
> 6.4, say (typo included):
> "If MapKey value is incorrect, ExitBootServices() returns
> EFI_INVALID_PARAMETER and GetMemoryMap() with ExitBootServices() must
> be called again. Firmware implementation may choose to do a partial
> shutdown of the boot services during the first call to
> ExitBootServices(). EFI OS loader should not make calls to any boot
> service function other then GetMemoryMap() after the first call to
> ExitBootServices()."
>
> And the memory allocation services are boot services. So I think it's
> pretty clear from the last sentence that we shouldn't use the memory
> allocation services after a failed call to ExitBootServices.
>
> When they say "first call", it's not "first successful
call". And
> since the sentences before talk about how ExitBootServices can fail
> and must be called again, I think "first call" definitely include
the
> case when this first call failed.
I somehow missed that when I was reading.  Thank you.

Given this, why not allocate a pool much larger than needed to prevent
the need to call AllocatePool() after the first attempted call to
ExitBootServices()?  Would it be better to be just additive?  Or
perhaps multiplicative with a minimum (ie, the greater of 16 or double
the suggested size)?  Is adding 4 the first round enough?  Or would
adding 8 or 16 be better?

-- 
-Gene