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

[Celelibi]

2015-11-04 12:17 UTC+01:00, Gene Cumm <gene.cumm at gmail.com>:
> 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?
>

First, let's say some similar discussion happened in 2013 on the Linux
Kernel Mailing List.
https://lkml.org/lkml/2013/6/11/59

To the question: how much additional memory should be needed w.r.t.
the size GetMemoryMap said it needs? At some point, the Linux answer
was "2. One for the allocation that's about to happen and one for the
memory modified by the event handlers called by ExitBootServices." And
the Linux code was like that at some point.

I heared several strategies people used to implement to deal with this
misbehavior. Some start with a buffer for N descriptors, and if it's
too small, double the size returned by GetMemoryMap. Some just add
several MB of memory to the said-size. None of those strategies has a
real rationale to back them up.

You can see from the discussion on LKML that the only rational way of
dealing with non-standard behavior is to be reactive (instead of
proactive) to what actually exists. There is no point in supporting a
non-standard case that doesn't actually happen. So as long as we do
not encounter a hardware that require more additional memory, there is
no point in wasting memory and potentially reaching the memory limit.
However, we may add a dprintf to ease future debugging if such case
should happen.

To answer more precisely your questions, the current patch has an
increment that is doubled on each iteration.

The do-while loop is necessary because we don't know the
implementation of AllocatePool (the boot service function). Some
implementations may very well add several descriptors to the memory
map or do some weird stuff like allocating some memory for internal
use and never free it (thus adding some descriptors every few calls).
Regarding this, the doubling increment is just an optimization that
should reduce the number of iteration of that loop in the worst case
implementation of AllocatePool.

In addition to this, the doubling increment is used to allocate some
more space for the second round (after ExitBootServices). And the
current code actually allocate 4 additional descriptors because that's
what works on my hardware (actually, 3 would be enough, but an even
number was easier to implement). We may choose anything larger, but
with no guaranty it would be enough on other implementations or that
there will be an implementation that needs more than 4. But if that
make you feel more safe to have at least 16 additional memory
descriptors, then, why not?

Maybe we could make the second use more explicit in the code by
forcing to reallocate a larger buffer if GetMemoryMap didn't leave at
least K free descriptors. This could be done by adding an argument to
get_memory_map_realloc to say how many additional descriptors we want.
Thus allowing to allocate K more on the first call before
ExitBootServices and 0 on the second call. While still kepping the
reallocation fall-back of course.

What do you think about it? I actually like that better than the current patch.

Celelibi