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

[Celelibi]

2015-09-16 15:24 UTC+02:00, Paulo Alcantara <pcacjr at zytor.com>:
> 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()?

Mainly because I didn't knew it existed. :)

Beside this, I could argue that it's annoying to have to know the
previous buffer size just so that the old data can be copied while I
don't care about the old data.

And last but not least: ReallocatePool may disturb the memory map more
than needed. This function in its current implementation allocate a
new buffer, copy the data and free the old buffer. This will allocate
a new buffer with a new address everytime, thus making it more likely
that the memory map grows. On the other hand, freeing the old buffer
before allocating the new one will probably just return the same
address. Less disturbance in the forc^W memory map means we have less
chance of allocating a buffer too small for the second round of
GetMemoryMap + ExitBootServices.

You'll notice that the first use of allocsizeadd will be 4 times
sizeof(EFI_MEMORY_DESCRIPTOR) while other implementations allocate
only 2 more struct than the size returned by GetMemoryMap (like
Linux). I did this so that on my hardware, the second call to
get_memory_map_realloc won't have to reallocate memory. (You shouldn't
allocate memory after the first call to ExitBootServices.) But the
amount of additional memory descriptors needed is not deterministic.

BTW, don't you think we should try to reboot if the second call to
ExitBootServices fail? At that point, there's just nothing we can do
to get the system back on track.



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

Not sure how I could avoid having two tests for EFI_BUFFER_TOO_SMALL.
GetMemoryMap could also return EFI_INVALID_PARAMETER. And I don't
really like assuming that a function won't return anything else than
what's currently documented. Maybe a future revision of the UEFI spec
will allow it to return more status codes.


>> +		}
>> +	} while (status == EFI_BUFFER_TOO_SMALL);
>> +
>> +	if (status == EFI_SUCCESS) {
>> +		*nr_entries = size / *desc_sz;
>> +		return map;
>> +	}
>> +
>> +	if (map)
>> +		FreePool(map);
>> +	return NULL;
>> +}
>> +


Celelibi