Message-ID: <7BD74F8E52BFD147B6C83AE7A1E893FFE28622FD@atlms1.us.megatrends.com>
Date:	Thu, 20 Jun 2013 18:04:50 +0000
From:	Zachary Bobroff <zacharyb@....com>
To:	"'H. Peter Anvin'" <hpa@...or.com>,
	"matt@...sole-pimps.org" <matt@...sole-pimps.org>
CC:	'Jan Beulich' <jbeulich@...e.com>,
	"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,

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-----
From: H. Peter Anvin [mailto:hpa@...or.com] 
Sent: Wednesday, June 19, 2013 4:53 AM
To: matt@...sole-pimps.org; Zachary Bobroff
Cc: 'Jan Beulich'; 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

The 0xa0000 restriction applies to BIOS really...

"matt@...sole-pimps.org" <matt@...sole-pimps.org> wrote:

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

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

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.

View attachment "eboot.c" of type "text/plain" (31977 bytes)

Powered by blists - more mailing lists