Jordan,
UEFI Boot Service FreePages() is mapped to CoreFreePages().
CoreFreePages() calls CoreConvertPages() with NewType of EfiConventionalMemory.
CoreConvertPages() calls CoreAddRange() with NewType of EfiConventionalMemory.
Memory registration occurs in GCD init and additional memory can be registered
dynamically through GCD service calls. All of the GCD paths go through
CoreAddMemoryDescriptor() and CoreAddMemoryDescriptor() calls CoreAddRange().
The patch is in CoreAddRange() that zeros page zero. This is the function that
is common to the registration of memory and free pages operations, so this work
could be done in a single place.
I agree I should have broken this up into two patches since one is a new
feature and the other is a bug. I will break it up for checkin.
Given that the only consumer of DEBUG_CLEAR_MEMORY() is the UEFI Memory Manager
in the DXE Core, I do not think adding more versions of it makes sense.
Another implementation option is to move the checks for clearing of page zero
into the implementation of the DEBUG_CLEAR_MEMORY() macro itself and update the
definition of DEBUG_CLEAR_MEMORY() to state that address range 0x0-0xfff is
ignored.
/**
The macro that calls DebugClearMemory() to clear a buffer to a default value.
If the DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of PcdDebugProperyMask is set,
then this macro calls DebugClearMemory() passing in Address and Length.
@param Address The pointer to a buffer.
@param Length The number of bytes in the buffer to set.
**/
#define DEBUG_CLEAR_MEMORY(Address, Length) \
do { \
if (DebugClearMemoryEnabled ()) { \
DebugClearMemory (Address, Length); \
} \
} while (FALSE)
Thanks,
Mike
-----Original Message-----
From: Jordan Justen [mailto:jljus...@gmail.com]
Sent: Thursday, March 21, 2013 8:23 AM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] MdeModulePkg patch: DxeCore UEFI Memory Manager zero page
zero and page zero DEBUG_CLEAR_MEMORY() bug fix
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
------------------------------------------------------------------------------
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
