[PATCH] x86, efi: retry ExitBootServices() on failure

[Matt Fleming]

From: Zach Bobroff <zach...@ami.com>

ExitBootServices is absolutely supposed to return a failure if any
ExitBootServices event handler changes the memory map. Basically the
get_map loop should run again if ExitBootServices returns an error the
first time. I would say it would be fair that if ExitBootServices gives
an error the second time then Linux would be fine in returning control
back to BIOS.

The second change is the following line:

again:
size += sizeof(*mem_map) * 2;

Originally you were incrementing it by the size of one memory map entry.
The issue here is all related to the low_alloc routine you are using.
In this routine you are making allocations to get the memory map itself.
Doing this allocation or allocations can affect the memory map by more
than one record.

[ mfleming - changelog, code style ]
Signed-off-by: Zach Bobroff <zach...@ami.com>
Cc: <sta...@vger.kernel.org>
Signed-off-by: Matt Fleming <matt.f...@intel.com>
---
arch/x86/boot/compressed/eboot.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 35ee62f..7c6e5d9 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1037,18 +1037,20 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
efi_memory_desc_t *mem_map;
efi_status_t status;
__u32 desc_version;
+ bool called_exit = false;
u8 nr_entries;
int i;

size = sizeof(*mem_map) * 32;

again:
- size += sizeof(*mem_map);
+ size += sizeof(*mem_map) * 2;
_size = size;
status = low_alloc(size, 1, (unsigned long *)&mem_map);
if (status != EFI_SUCCESS)
return status;

+get_map:
status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
mem_map, &key, &desc_size, &desc_version);
if (status == EFI_BUFFER_TOO_SMALL) {
@@ -1074,8 +1076,20 @@ again:
/* Might as well exit boot services now */
status = efi_call_phys2(sys_table->boottime->exit_boot_services,
handle, key);
- if (status != EFI_SUCCESS)
- goto free_mem_map;
+ if (status != EFI_SUCCESS) {
+ /*
+ * ExitBootServices() will fail if any of the event
+ * handlers change the memory map. In which case, we
+ * must be prepared to retry, but only once so that
+ * we're guaranteed to exit on repeated failures instead
+ * of spinning forever.
+ */
+ if (called_exit)
+ goto free_mem_map;
+
+ called_exit = true;
+ goto get_map;
+ }

/* Historic? */
boot_params->alt_mem_k = 32 * 1024;
--
1.8.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Matt Fleming]

On Tue, 11 Jun, at 07:52:38AM, Matt Fleming wrote:
> From: Zach Bobroff <zach...@ami.com>
>
> ExitBootServices is absolutely supposed to return a failure if any
> ExitBootServices event handler changes the memory map. Basically the
> get_map loop should run again if ExitBootServices returns an error the
> first time. I would say it would be fair that if ExitBootServices gives
> an error the second time then Linux would be fine in returning control
> back to BIOS.
>
> The second change is the following line:
>
> again:
> size += sizeof(*mem_map) * 2;
>
> Originally you were incrementing it by the size of one memory map entry.
> The issue here is all related to the low_alloc routine you are using.
> In this routine you are making allocations to get the memory map itself.
> Doing this allocation or allocations can affect the memory map by more
> than one record.
>
> [ mfleming - changelog, code style ]
> Signed-off-by: Zach Bobroff <zach...@ami.com>
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Matt Fleming <matt.f...@intel.com>
> ---
> arch/x86/boot/compressed/eboot.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)

I've queued this patch up for v3.11 in my 'next' branch.

--
Matt Fleming, Intel Open Source Technology Center

[joeyli]

Hi Zach,

於 二，2013-06-11 於 07:52 +0100，Matt Fleming 提到：

Can we know why increased the size of double *mem_map is big enough?
Does there have any real guarantee to be sufficient?

And, why would doubling the increment be necessary here, but not in
__get_map()?

> if (status != EFI_SUCCESS)
> return status;
>
> +get_map:

The get_map label is being placed such that the size doesn't
get adjusted anymore, yet it is supposed to deal with an allocation
having happened during ExitBootServices(), which could have
grown the map.

Why doesn't direct goto 'again' label?

> status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
> mem_map, &key, &desc_size, &desc_version);
> if (status == EFI_BUFFER_TOO_SMALL) {
> @@ -1074,8 +1076,20 @@ again:
> /* Might as well exit boot services now */
> status = efi_call_phys2(sys_table->boottime->exit_boot_services,
> handle, key);
> - if (status != EFI_SUCCESS)
> - goto free_mem_map;
> + if (status != EFI_SUCCESS) {
> + /*
> + * ExitBootServices() will fail if any of the event
> + * handlers change the memory map. In which case, we
> + * must be prepared to retry, but only once so that
> + * we're guaranteed to exit on repeated failures instead
> + * of spinning forever.
> + */
> + if (called_exit)
> + goto free_mem_map;
> +
> + called_exit = true;

Why a single retry is having reasonable guarantees to work when the
original one failed (nothing prevents an event handler to do another
allocation the next time through).

Why not retry 3, 4, 5 or even unlimited times?

> + goto get_map;
> + }
>
> /* Historic? */
> boot_params->alt_mem_k = 32 * 1024;

Thanks a lot!
Joey Lee

[Matt Fleming]

It's not guaranteed to be sufficient, it's simply a best guess. If we
haven't allocated enough memory to store the memory map we should loop
around to the 'again' label anyway. It's just an optimisation, right
Zach?

> And, why would doubling the increment be necessary here, but not in
> __get_map()?

Again, we'll grow the map if it isn't large enough.

> > if (status != EFI_SUCCESS)
> > return status;
> >
> > +get_map:
>
> The get_map label is being placed such that the size doesn't
> get adjusted anymore, yet it is supposed to deal with an allocation
> having happened during ExitBootServices(), which could have
> grown the map.
>
> Why doesn't direct goto 'again' label?

It makes more sense to query GetMemoryMap() directly first to get the
required size of the memory map. Then we jump to 'again' and allocate
the correct size.

There has to be an upper limit on the number of retries. It seems fair
to retry once but any more than that and it's more likely that there's a
serious problem and we should bail.

--
Matt Fleming, Intel Open Source Technology Center

[Jan Beulich]

>>> On 17.06.13 at 11:21, Matt Fleming <ma...@console-pimps.org> wrote:
> On Fri, 14 Jun, at 12:00:33AM, joeyli wrote:

>> > From: Zach Bobroff <zach...@ami.com>

>> > --- a/arch/x86/boot/compressed/eboot.c
>> > +++ b/arch/x86/boot/compressed/eboot.c
>> > @@ -1037,18 +1037,20 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
>> > efi_memory_desc_t *mem_map;
>> > efi_status_t status;
>> > __u32 desc_version;
>> > + bool called_exit = false;
>> > u8 nr_entries;
>> > int i;
>> >
>> > size = sizeof(*mem_map) * 32;
>> >
>> > again:
>> > - size += sizeof(*mem_map);
>> > + size += sizeof(*mem_map) * 2;
>> > _size = size;
>> > status = low_alloc(size, 1, (unsigned long *)&mem_map);
>>
>> Can we know why increased the size of double *mem_map is big enough?
>> Does there have any real guarantee to be sufficient?
>
> It's not guaranteed to be sufficient, it's simply a best guess. If we
> haven't allocated enough memory to store the memory map we should loop
> around to the 'again' label anyway. It's just an optimisation, right
> Zach?

If that were the case, the code change wouldn't be necessary at
all, i.e. the increment could remain to be a single array element
steps.

>> And, why would doubling the increment be necessary here, but not in
>> __get_map()?
>
> Again, we'll grow the map if it isn't large enough.

But in single element steps only. The question though was why
doubling the step was necessary (and sufficient) there, but isn't
also necessary here.

To me, all this looks like it is being done on phenomenological basis,
taking a particular build of a particular firmware implementation as
the reference. Imo we shouldn't change the code in this way. This
also applies to the fact that the step is being doubled rather than
e.g. tripled: With it ending up a "again" anyway (see below), what's
the point of avoiding one more of the iterations?

Generic considerations would result in the increment being at least
3 * element size; twice the element size assumes that the allocator
would behave in certain ways (like returning the head or tail part of
a larger piece of memory).

>> > if (status != EFI_SUCCESS)
>> > return status;
>> >
>> > +get_map:
>>
>> The get_map label is being placed such that the size doesn't
>> get adjusted anymore, yet it is supposed to deal with an allocation
>> having happened during ExitBootServices(), which could have
>> grown the map.
>>
>> Why doesn't direct goto 'again' label?
>
> It makes more sense to query GetMemoryMap() directly first to get the
> required size of the memory map. Then we jump to 'again' and allocate
> the correct size.

Ah, okay, I didn't pay attention to this indirectly ending up at
"again".

I agree that there ought to be an upper limit. But a single retry here
again looks like a tailored solution to a particular observed (mis-)
behavior, rather than something resulting from general considerations.

Jan

[Matt Fleming]

On Mon, 17 Jun, at 10:46:28AM, Jan Beulich wrote:
> To me, all this looks like it is being done on phenomenological basis,
> taking a particular build of a particular firmware implementation as
> the reference. Imo we shouldn't change the code in this way. This
> also applies to the fact that the step is being doubled rather than
> e.g. tripled: With it ending up a "again" anyway (see below), what's
> the point of avoiding one more of the iterations?
>
> Generic considerations would result in the increment being at least
> 3 * element size; twice the element size assumes that the allocator
> would behave in certain ways (like returning the head or tail part of
> a larger piece of memory).

I have no issue with changing the multiplier. But let's get
clarification from Zach as to what exactly is going on here.

> I agree that there ought to be an upper limit. But a single retry here
> again looks like a tailored solution to a particular observed (mis-)
> behavior, rather than something resulting from general considerations.

What value would you suggest for the retry?

--
Matt Fleming, Intel Open Source Technology Center

[joeyli]

於 一，2013-06-17 於 11:17 +0100，Matt Fleming 提到：

> On Mon, 17 Jun, at 10:46:28AM, Jan Beulich wrote:
> > To me, all this looks like it is being done on phenomenological basis,
> > taking a particular build of a particular firmware implementation as
> > the reference. Imo we shouldn't change the code in this way. This
> > also applies to the fact that the step is being doubled rather than
> > e.g. tripled: With it ending up a "again" anyway (see below), what's
> > the point of avoiding one more of the iterations?
> >
> > Generic considerations would result in the increment being at least
> > 3 * element size; twice the element size assumes that the allocator
> > would behave in certain ways (like returning the head or tail part of
> > a larger piece of memory).
>
> I have no issue with changing the multiplier. But let's get
> clarification from Zach as to what exactly is going on here.
>
> > I agree that there ought to be an upper limit. But a single retry here
> > again looks like a tailored solution to a particular observed (mis-)
> > behavior, rather than something resulting from general considerations.
>
> What value would you suggest for the retry?
>

Currently grub2 retry unlimited times like attached patch.

But, I also agree need have a upper limit.

[Jan Beulich]

>>> On 17.06.13 at 12:17, Matt Fleming <ma...@console-pimps.org> wrote:
> On Mon, 17 Jun, at 10:46:28AM, Jan Beulich wrote:
>> I agree that there ought to be an upper limit. But a single retry here
>> again looks like a tailored solution to a particular observed (mis-)
>> behavior, rather than something resulting from general considerations.
>
> What value would you suggest for the retry?

I'd be considering something like 5...10, but this to some degree
depends on what odd kinds of behavior this in fact needs to
cover.

Jan

[Matt Fleming]

On Mon, 17 Jun, at 12:02:05PM, Jan Beulich wrote:
> >>> On 17.06.13 at 12:17, Matt Fleming <ma...@console-pimps.org> wrote:
> >
> > What value would you suggest for the retry?
>
> I'd be considering something like 5...10, but this to some degree
> depends on what odd kinds of behavior this in fact needs to
> cover.

I genuinely don't see how picking numbers (seemingly) at random is an
improvement over the simple case of "if ExitBootServices() returns an
error, you get one more try". Unless you're aware of firmware that
requires calling ExitBootServices() multiple times? The grub ChangeLog
that Joey linked to is not particularly enlightening.

The scenario that this patch addresses is a very clear one, where the
firmware event handlers that respond to the initial ExitBootServices
event allocate/free memory, thereby invalidating the memory map cookie
we passed to ExitBootServices(), and requiring us to call
ExitBootServices() a second time.

I'm not saying that retrying once is the only solution that will ever
make sense. But certainly until we start seeing reports of problems and
understand why firmware might want us to invoke ExitBootServices()
multiple times, it seems like the most straight forward option.

--
Matt Fleming, Intel Open Source Technology Center

[Zachary Bobroff]

All,

>> Why a single retry is having reasonable guarantees to work when the original one failed (nothing prevents an event handler to do another allocation the next time through).

This patch is being submitted because of the potential issues that occur when 3rd party option ROMs are signaled by the ExitBootServices event. At the time they are signaled, they can perform any number of actions that would change the EFI Memory map. This wasn't actually seen with our default implementation of our firmware, it was seen when this plug-in card's option rom was run.

We only need to retry once because of what is in the UEFI specification 2.3.1 Errata C. The exact verbiage is as follows:

The ExitBootServices() function is called by the currently executing EFI OS loader image to terminate all boot services. On success, the loader becomes responsible for the continued operation of the system. All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES must be signaled before ExitBootServices() returns EFI_SUCCESS. The events are only signaled once even if ExitBootServices() is called multiple times.

Since the firmware will only signal the ExitBootServices event once, the code that has the potential to change the memory map will only be signaled on the first call to exit boot services.

>>Can we know why increased the size of double *mem_map is big enough? Does there have any real guarantee to be sufficient? And, why would doubling the increment be necessary here, but not in __get_map()?

Thinking of this as "doubling the increment" is not very clear. I think a better way to think about this is that the GetMemoryMap is going to return the number of MemoryMap Entries, in bytes. Then the "doubling the increment" can be thought of as us saying we want to allocate space for two additional Efi Memory Map Descriptors.

Here is the logic for why we need space for the two additional Efi Memory Map Descriptors is as follows:
First, we should think of the size that is returned from the GetMemoryMap as being the number of bytes to contain the current memory map. Once we have the size of the current memory map, we, the kernel, have to perform an allocation of memory so that we can store the current memory map into some memory that will not be overwritten. That allocation has the possibility of increasing the current memory map count by one efi_memory_desc_t structure. Since we are going to call low_alloc, which itself calls getmemorymap to determine the lowest address that is available before it performs out requested allocation, we have to increase the memory map by another entry to account for the possibility that its allocation ill increase the memory map by another entry.

It may make more sense to consider trying to allocate the space with AllocateMaxAddress as the type. The allocation routine will allocate space using the address supplied as the maximum(upper bound). It is my understanding that you need this memory map at some low address, perhaps you know the upper bound of where it could exist? This would remove any problems that are being created by low_alloc actually doing another allocation and changing the size of the memory map we just were told we needed.

>> The get_map label is being placed such that the size doesn't get adjusted anymore, yet it is supposed to deal with an allocation having happened during ExitBootServices(), which could have grown the map.

Whatever memory operations that are performed during the ExitBootServices function should be properly reflected by our call to "GetMemoryMap" the second time through. If their operations increased the number of descriptors that GetMemoryMap will return, we should encounter the EFI_BUFFER_TOO_SMALL case and reallocate, If the operations decreased the number of Efi Memory Descriptors, then the current allocation will be okay, it just may contain unused space at the end of the allocated area.

> > What value would you suggest for the retry?

In reality 2 times is probably enough. However, if you guys feel you want to try it more times like grub is, that should be fine. You can read the original response up above.

Best Regards,
Zach

-----Original Message-----
From: Matt Fleming [mailto:ma...@console-pimps.org]
Sent: Monday, June 17, 2013 8:31 AM
To: Jan Beulich
Cc: Zachary Bobroff; Matt Fleming; Matthew Garrett; Joey Lee; linu...@vger.kernel.org; linux-...@vger.kernel.org; sta...@vger.kernel.org
Subject: Re: [PATCH] x86, efi: retry ExitBootServices() on failure

On Mon, 17 Jun, at 12:02:05PM, Jan Beulich wrote:
> >>> On 17.06.13 at 12:17, Matt Fleming <ma...@console-pimps.org> wrote:
> >
> > What value would you suggest for the retry?
>
> I'd be considering something like 5...10, but this to some degree
> depends on what odd kinds of behavior this in fact needs to cover.

I genuinely don't see how picking numbers (seemingly) at random is an improvement over the simple case of "if ExitBootServices() returns an error, you get one more try". Unless you're aware of firmware that requires calling ExitBootServices() multiple times? The grub ChangeLog that Joey linked to is not particularly enlightening.

The scenario that this patch addresses is a very clear one, where the firmware event handlers that respond to the initial ExitBootServices event allocate/free memory, thereby invalidating the memory map cookie we passed to ExitBootServices(), and requiring us to call
ExitBootServices() a second time.

I'm not saying that retrying once is the only solution that will ever make sense. But certainly until we start seeing reports of problems and understand why firmware might want us to invoke ExitBootServices() multiple times, it seems like the most straight forward option.

--
Matt Fleming, Intel Open Source Technology Center

The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

[joeyli]

Hi Zach,

於 二，2013-06-18 於 00:18 +0000，Zachary Bobroff 提到：

> All,
>
> >> Why a single retry is having reasonable guarantees to work when the
> original one failed (nothing prevents an event handler to do another
> allocation the next time through).
>
> This patch is being submitted because of the potential issues that
> occur when 3rd party option ROMs are signaled by the ExitBootServices
> event. At the time they are signaled, they can perform any number of
> actions that would change the EFI Memory map. This wasn't actually
> seen with our default implementation of our firmware, it was seen when
> this plug-in card's option rom was run.
>
> We only need to retry once because of what is in the UEFI
> specification 2.3.1 Errata C. The exact verbiage is as follows:
>
> The ExitBootServices() function is called by the currently executing
> EFI OS loader image to terminate all boot services. On success, the
> loader becomes responsible for the continued operation of the system.
> All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES must be signaled
> before ExitBootServices() returns EFI_SUCCESS. The events are only
> signaled once even if ExitBootServices() is called multiple times.
>
> Since the firmware will only signal the ExitBootServices event once,
> the code that has the potential to change the memory map will only be
> signaled on the first call to exit boot services.

Does it possible have any delay time of 3rd party ROMs to change EFI
memory map after they are signaled?
or ExitBootServices() will wait all 3rd party ROMs updated memory map
finished then return true/false to kernel?

Thanks a lot!
Joey Lee

[Zachary Bobroff]

The timer is shutdown before callbacks on exitbootservices are called. The bios should be entirely single threaded at this point unless Linux has started some other CPUs. So exitbootservices will not return until each until each callback is complete. In short, then it would return the status of failure if the memory map has changed, success otherwise. The callbacks are not called on any subsequent call to exitbootservices, so the map should stay the same then.

Sent from my iPhone

The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

[joeyli]

Thanks for Zach's clarify, then I think that makes sense BIOS has
problem if kernel need call ExitBootServices() more then 2 times.


Thanks
Joey Lee

於 二，2013-06-18 於 04:20 +0000，Zachary Bobroff 提到：

[Jan Beulich]

>>> Zachary Bobroff <zach...@ami.com> 06/18/13 2:25 AM >>>

> We only need to retry once because of what is in the UEFI specification 2.3.1 Errata
> C. The exact verbiage is as follows:
>
> The ExitBootServices() function is called by the currently executing EFI OS loader
> image to terminate all boot services. On success, the loader becomes responsible for
> the continued operation of the system. All events of type EVT_SIGNAL_EXIT_BOOT_SERVICES
> must be signaled before ExitBootServices() returns EFI_SUCCESS. The events are
> only signaled once even if ExitBootServices() is called multiple times.
>
> Since the firmware will only signal the ExitBootServices event once, the code that
> has the potential to change the memory map will only be signaled on the first call to
> exit boot services.

Okay, I'm fine with that aspect then. Let's hope everyone plays by that rule.

> Here is the logic for why we need space for the two additional Efi Memory Map
> Descriptors is as follows:
> First, we should think of the size that is returned from the GetMemoryMap as being
> the number of bytes to contain the current memory map. Once we have the size of
> the current memory map, we, the kernel, have to perform an allocation of memory
> so that we can store the current memory map into some memory that will not be
> overwritten. That allocation has the possibility of increasing the current memory
> map count by one efi_memory_desc_t structure.

Why by one? Splitting some 'free memory' block may result in an increase by more
then one afaict. Assuming the increment can only be one is implying you having
knowledge of the allocator implementation and behavior, which shouldn't be made
use of in kernel code.

Jan

[Zachary Bobroff]

All,

Just to make a few more minor clarifications. According to everything going correctly you will only have two calls to ExitBootServices. In our specific implementation you will only have one possible failure to ExitBootServices and then a success. Just to be fair to the industry there is one other case where there could be more than one call to ExitBootServices that returns a failure. According to the specification, this is the definition of ExitBootServices:

typedef
EFI_STATUS
ExitBootServices (
IN EFI_HANDLEImageHandle,
IN UINTN MapKey
);

With this description of the parameters:
ImageHandle-- Handle that identifies the exiting image. Type EFI_HANDLE is defined in the InstallProtocolInterface() function description.
MapKey-- Key to the latest memory map.

If you pass an invalid MapKey the function is forced to return EFI_INVALID_PARAMETER based upon the following statement:
" 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()." You will note they leave some leeway to what shutdown of services is done if an invalid MapKey is provided. If you are confident that the Linux code wont pass an Invalid MapKey then no problem, 2 calls should be sufficient(Also, it would be a coding mistake if it did pass one). If not, you can feel more comfortable increasing the retry number to something more than 1 retry.

> Okay, I'm fine with that aspect then. Let's hope everyone plays by that rule.

This is all according to specification, so if they are not following these rules they should be corrected. The link to where the current public version of the specification is available is here:
http://www.uefi.org/specs/agreement

I don't think any of the fields are checked so feel free to enter whatever you wish for the items. It will then take you to a download page. The version we have been discussing in this e-mail thread is UEFI 2.3.1C. ExitBootServices is described on pages 256-257 (page numbers 200-201). If you haven't read it before, it is a pretty simple read.

> Why by one? Splitting some 'free memory' block may result in an increase by more then one afaict. Assuming the increment can only be one is >implying you having knowledge of the allocator implementation and behavior, which shouldn't be made use of in kernel code.

We had to actually increment it by two to get it to work correctly. This is all based upon the use of the low_alloc routine in the linux kernel file. I agree there is still some outstanding issue based upon this, but we put it through several different types of tests and it continued to work correctly. The truest solution would be to us the AllocateMaxAddress parameter when using AllocatePages. AllocatePages definition listed here:

typedef
EFI_STATUS
AllocatePages(
IN EFI_ALLOCATE_TYPE Type,
IN EFI_MEMORY_TYPE MemoryType,
IN UINTN Pages,
IN OUT EFI_PHYSICAL_ADDRESS*Memory
);

And description when Type= AllocateMaxAddress:

Allocation requests of Type AllocateMaxAddress allocate any available range of pages whose uppermost address is less than or equal to the address pointed to by Memory on input.

This should be able to get you a low allocation in one allocation as opposed to the low_alloc routine which might go through a few iterations of allocations based upon what it is doing. It was my understanding that the point of this was to allocate the memory map below a certain address in memory because the kernel required it. Matt, can you comment here? I am not aware of what address it needs to be below, but using this function should do the trick. Also, if you want to inform me better of what memory ceiling restrictions there are at this early stage of the kernel, I can rewrite the file without the need of the low_alloc routine entirely.

Hope this clarifies a few things.

Best Regards,
Zach
-----Original Message-----

The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

[ma...@console-pimps.org]

On Tue, 18 Jun, at 10:12:22PM, Zachary Bobroff wrote:
> > Okay, I'm fine with that aspect then. Let's hope everyone plays by
> > that rule.
> This is all according to specification, so if they are not following
> these rules they should be corrected. The link to where the current
> public version of the specification is available is here:
> http://www.uefi.org/specs/agreement

While I agree that the vendor should be informed if their implementation
deviates from the spec in some way, the Linux kernel usually still needs
to support these nonconforming machines once they end up in the hands of
consumers (which is often the point at which we discover these kinds of
issues). Sadly, we're still not in a position where firmware updates can
be applied from OEMs ubiquitously, either because machines are End of
Life'd or because the update needs to be run from Windows.

We tend to adopt the approach of: let's try this until we get reports of
a class of machines where this solution doesn't work.

Though I do find it refreshing to hear engineers talking about the UEFI
spec in such black and white terms. That is certainly the ideal we
should be aiming for.

> > Why by one? Splitting some 'free memory' block may result in an
> > increase by more then one afaict. Assuming the increment can only be
> > one is >implying you having knowledge of the allocator
> > implementation and behavior, which shouldn't be made use of in
> > kernel code.
> We had to actually increment it by two to get it to work correctly.
> This is all based upon the use of the low_alloc routine in the linux
> kernel file. I agree there is still some outstanding issue based upon
> this, but we put it through several different types of tests and it
> continued to work correctly. The truest solution would be to us the
> AllocateMaxAddress parameter when using AllocatePages.

[...]

> It was my understanding that the point of this was to allocate the
> memory map below a certain address in memory because the kernel
> required it. Matt, can you comment here? I am not aware of what
> address it needs to be below, but using this function should do the
> trick. Also, if you want to inform me better of what memory ceiling
> restrictions there are at this early stage of the kernel, I can
> rewrite the file without the need of the low_alloc routine entirely.

The most important restriction is that all allocations in the EFI boot
stub need to be below the 1GB mark, because only the first 1GB of
virtual memory is mapped, unless certain flags are set in the xloadflags
field of the boot_params header. See Documentation/x86/boot.txt.

Further to that, I think I remember some restrictions on the location of
the cmdline pointer - that it needs to be below 0xa0000. Again,
Documentation/x86/boot.xt should have all the info you need.

--
Matt Fleming, Intel Open Source Technology Center

[H. Peter Anvin]

The 0xa0000 restriction applies to BIOS really...

Sent from my mobile phone. Please excuse brevity and lack of formatting.

[Zachary Bobroff]

All,

I am attaching a further updated version of eboot.c . We removed the low_alloc routine from the exit_boot function only. We also removed the goto statements(sorry we just aren’t huge fans of goto's in c, you can change it back to be goto oriented if you want though) and put it in a loop that is counting down from retry count. You can see the loop is based upon this conditional:

while((ExitRetryCount > 0) && (status != EFI_SUCCESS)) {

So we have currently set ExitRetryCount to 2 (a couple of lines above):
int ExitRetryCount = 2;

However, I have a suggestion and im not entirely sure how difficult it would be, im just suggesting it might not be a bad idea. We can initialize this ExitRetryCount to be some default value, but if grub(or a different bootloader) passes some updated value, ExitRetryCount could be updated with this value. Myself, I don’t know the level of complexity it creates pulling a kernel parameter, but given a decent example, I could see about adding that support. Allowing passing of a parameter could eliminate problems with the systems that may be out of specification.

Also, you can see the following lines:
mem_map = (efi_memory_desc_t*)(unsigned long*)(unsigned long)0x40000000;

status = efi_call_phys4(sys_table->boottime->allocate_pages, EFI_ALLOCATE_MAX_ADDRESS, EFI_LOADER_DATA, page_size, &mem_map);

if (status != EFI_SUCCESS) {
return status;
}

This is where we are forcing the allocation below 0x40000000 (1GB), if you prefer it to be at a lower address, feel free to update it, but from what you have said and what is in boot.txt, below 1GB should work. We tested it with 1GB of ram, 4GB of ram and 8 GB of ram and all cases completed successful. All cases were also tried with forcing the ExitBootServices to fail the first time by changing the memory map in an ExitBootServices event. Using this method will guarantee that we only need to increase the size returned by attempting to get the map the first time will only need at most one more entry in the memory map (based upon the allocation we are about to make). So the line:
size += 2*sizeof(*mem_map);
has changed back to:
size += 1*sizeof(*mem_map);

Anyway, let me know your thoughts on if this, we can make further updates to this file and remove the low_alloc altogether if you want. However, this was the only instance of where low_alloc is going to cause a problem for the ExitBootServices call.

Best Regards,
Zach
-----Original Message-----

[ma...@console-pimps.org]

On Thu, 20 Jun, at 06:04:50PM, Zachary Bobroff wrote:
> All,
>
> I am attaching a further updated version of eboot.c . We removed the
> low_alloc routine from the exit_boot function only. We also removed
> the goto statements(sorry we just aren’t huge fans of goto's in c, you
> can change it back to be goto oriented if you want though) and put it
> in a loop that is counting down from retry count. You can see the
> loop is based upon this conditional:

It would be much easier to review these changes if you sent them as a
patch against a git tree, using git to generate the patch. Failing that,
even a plain old diff-format file would be acceptable.

> while((ExitRetryCount > 0) && (status != EFI_SUCCESS)) {
>
> So we have currently set ExitRetryCount to 2 (a couple of lines above):
> int ExitRetryCount = 2;
>
> However, I have a suggestion and im not entirely sure how difficult it
> would be, im just suggesting it might not be a bad idea. We can
> initialize this ExitRetryCount to be some default value, but if
> grub(or a different bootloader) passes some updated value,
> ExitRetryCount could be updated with this value. Myself, I don’t know
> the level of complexity it creates pulling a kernel parameter, but
> given a decent example, I could see about adding that support.
> Allowing passing of a parameter could eliminate problems with the
> systems that may be out of specification.

We try to pull workarounds for these problems into the kernel, rather
than relying on bootloaders to pass in the necessary flags. People
rarely want to update their boot loaders, but most distributions release
updates for installed kernels semi-regularly.

--
Matt Fleming, Intel Open Source Technology Center