[syslinux] [PATCH] efi: Call ExitBootServices at least twice

[Celelibi]

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?
>
>> +               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.

I forgot to reply to this.
The justification for moving ExitBootServices earlier is simple:
*Nothing* should happen between GetMemoryMap and ExitBootServices. If
something happened to the memory map, ExitBootServices would fail. So
moving those calls as close together as possible (as recommended by
the UEFI spec) is the most logical thing to do.
And allocating some memory while parsing the memory map may not be the
smartest thing to do as it would invalidate the memory map being
parsed.

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


Celelibi