On Thu, Mar 21, 2013 at 4:25 AM, Laszlo Ersek <ler...@redhat.com> wrote:
> On 03/21/13 00:38, Kinney, Michael D wrote:
>> Please see the attached patch that guarantees that free memory in the 4K
>> page starting at address 0 is always cleared to 0. The algorithm is to
>> clear page zero if it is registered with the DXE Core with type
>> EfiConventionalMemory,
>> and to also clear page zero if it is freed using
>> the UEFI Boot Service FreePages().
Where does this happen?
>The unit test for this patch exposed
>> a bug in the DXE Core that generated an ASSERT() when the page at
>> address zero is freed and DEBUG_CLEAR_MEMORY() macros are enabled. If
>> DEBUG_CLEAR_MEMORY() is enabled and the page at address 0 is freed, then
>> DEBUG_CLEAR_MEMORY() is invoked skipping over the first 4K page.
This seems to be a bug fix, and I think should be made separately.
>> This patch improves OS compatibility for OSes that may evaluate page 0
>> for legacy data structures. Before this patch, free memory may contain
>> random values which induces random boot failures for some OSes. This
>> patch may also help find NULL pointer bugs sooner because all of the
>> fields in a data structure dereferenced through NULL will also be NULL now.
>
> I'm including the patch here for review:
Thanks for including the patch inline Laszlo. (A nice feature of git
send-email...)
>> Index: Dxe/Mem/Page.c
>> ===================================================================
>> --- Dxe/Mem/Page.c (revision 14133)
>> +++ Dxe/Mem/Page.c (working copy)
>> @@ -1,7 +1,7 @@
>> /** @file
>> UEFI Memory page management functions.
>>
>> -Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2007 - 2013, Intel Corporation. All rights reserved.<BR>
>> This program and the accompanying materials
>> are licensed and made available under the terms and conditions of the BSD
>> License
>> which accompanies this distribution. The full text of the license may be
>> found at
>> @@ -177,8 +177,21 @@
>> ASSERT_LOCKED (&gMemoryLock);
>>
>> DEBUG ((DEBUG_PAGE, "AddRange: %lx-%lx to %d\n", Start, End, Type));
>> -
>> +
>> //
>> + // If memory of type EfiConventionalMemory is being added that includes
>> the page
>> + // starting at address 0, then zero the page starting at address 0. This
>> has
>> + // two benifits. It helps find NULL pointer bugs and it also maximizes
>> + // compatibility with operating systems that may evaluate memory in this
>> page
>> + // for legacy data structures. If memory of any other type is added
>> starting
>> + // at address 0, then do not zero the page at address 0 because the page
>> is being
>> + // used for other purposes.
>> + //
>> + if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE
>> - 1)) {
>> + SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
>> + }
>> +
>> + //
>> // Memory map being altered so updated key
>> //
>> mMemoryMapKey += 1;
>
> This hunk (for function CoreAddRange()) seems OK. "End" is inclusive,
> hence we're checking whether the range being added covers the zero page
> entirely.
This should be a separate patch from the change below. For this part:
Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
>> @@ -834,7 +847,18 @@
>> //
>> CoreAddRange (NewType, Start, RangeEnd, Attribute);
>> if (NewType == EfiConventionalMemory) {
>> - DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd - Start
>> + 1));
>> + //
>> + // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because this
>> + // macro will ASSERT() if address is 0. Instead, CoreAddRange()
>> guarantees
>> + // that the page starting at address 0 is always filled with zeros.
>> + //
>> + if (Start == 0) {
>> + if (RangeEnd > EFI_PAGE_SIZE) {
>> + DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) EFI_PAGE_SIZE, (UINTN)
>> (RangeEnd - EFI_PAGE_SIZE + 1));
>> + }
>> + } else {
>> + DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd -
>> Start + 1));
>> + }
>> }
Maybe another option is to add
DEBUG_CLEAR_MEMORY_AT_0(Length)/DebugClearMemoryAt0
or
DEBUG_CLEAR_MEMORY_UNCHECKED?
-Jordan
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
