Message-ID: <7BD74F8E52BFD147B6C83AE7A1E893FFE286040C@atlms1.us.megatrends.com>
Date:	Tue, 18 Jun 2013 22:12:22 +0000
From:	Zachary Bobroff <zacharyb@....com>
To:	'Jan Beulich' <jbeulich@...e.com>,
	"matt@...sole-pimps.org" <matt@...sole-pimps.org>
CC:	"matt.fleming@...el.com" <matt.fleming@...el.com>,
	"mjg59@...f.ucam.org" <mjg59@...f.ucam.org>,
	Joey Lee <JLee@...e.com>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] x86, efi: retry ExitBootServices() on failure

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-----
From: Jan Beulich [mailto:jbeulich@...e.com] 
Sent: Tuesday, June 18, 2013 9:04 AM
To: Zachary Bobroff; matt@...sole-pimps.org
Cc: matt.fleming@...el.com; mjg59@...f.ucam.org; Joey Lee; linux-efi@...r.kernel.org; linux-kernel@...r.kernel.org; stable@...r.kernel.org
Subject: RE: [PATCH] x86, efi: retry ExitBootServices() on failure

>>> Zachary Bobroff <zacharyb@....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


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

Powered by blists - more mailing lists