On 01/13/15 16:13, Ard Biesheuvel wrote:
> The function CoreTerminateMemoryMap() performs some final sanity checks
> on the runtime regions in the memory map before allowing ExitBootServices()
> to complete. Unfortunately, it does so by testing the EFI_MEMORY_RUNTIME bit
> in the Attribute field, which is never set anywhere in the code.
>
> Fix it by setting the bit where appropriate in CoreAddRange().
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> MdeModulePkg/Core/Dxe/Mem/Page.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index fa84e2612526..3abf934933d8 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -240,6 +240,10 @@ CoreAddRange (
> }
> }
>
> + if (mMemoryTypeStatistics[Type].Runtime) {
> + Attribute |= EFI_MEMORY_RUNTIME;
> + }
> +
> //
> // Add descriptor
> //
>
I think the check that you imply in CoreTerminateMemoryMap() is indeed dead
code. But, I don't see how your patch would make it less dead. :)
mMemoryTypeStatistics is a static array. Elements in that array never change
their Runtime fields.
The CoreGetMemoryMap() function uses the Runtime field to set the
EFI_MEMORY_RUNTIME bit for various types of memory (EfiRuntimeServicesCode,
EfiRuntimeServicesData, EfiPalCode), but that bit is set only in the map built
for the caller, never in the internal map.
When CoreTerminateMemoryMap() looks at the internal attribute bits, I agree
that EFI_MEMORY_RUNTIME is never set, so that check is dead. However, the first
check within that condition looks for at the type directly,
EfiACPIReclaimMemory and EfiACPIMemoryNVS. Even if the EFI_MEMORY_RUNTIME bit
had fired, this check would remain dead, because for these two memory types the
Runtime bit is never set in the mMemoryTypeStatistics.
... Aha! You're not aiming at the first check here. You are looking at the
start & end alignments.
In that case though, I don't think we should change the invariant
the internal attributes never set EFI_MEMORY_RUNTIME
Instead, I think CoreTerminateMemoryMap() should replicate the check in seen
CoreGetMemoryMap(). Something like:
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index fa84e26..d9e2075 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1790,12 +1790,9 @@ CoreTerminateMemoryMap (
>
> for (Link = gMemoryMap.ForwardLink; Link != &gMemoryMap; Link =
> Link->ForwardLink) {
> Entry = CR(Link, MEMORY_MAP, Link, MEMORY_MAP_SIGNATURE);
> - if ((Entry->Attribute & EFI_MEMORY_RUNTIME) != 0) {
> - if (Entry->Type == EfiACPIReclaimMemory || Entry->Type ==
> EfiACPIMemoryNVS) {
> - DEBUG((DEBUG_ERROR | DEBUG_PAGE, "ExitBootServices: ACPI memory
> entry has RUNTIME attribute set.\n"));
> - Status = EFI_INVALID_PARAMETER;
> - goto Done;
> - }
> + if (mMemoryTypeStatistics[Entry->Type].Runtime) {
> + ASSERT (Entry->Type != EfiACPIReclaimMemory);
> + ASSERT (Entry->Type != EfiACPIMemoryNVS);
> if ((Entry->Start & (EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT -
> 1)) != 0) {
> DEBUG((DEBUG_ERROR | DEBUG_PAGE, "ExitBootServices: A RUNTIME
> memory entry is not on a proper alignment.\n"));
> Status = EFI_INVALID_PARAMETER;
Of course I have no jurisdiction in MdeModulePkg/Core/, so I'm just theorizing
:) Still,
the internal attributes never set EFI_MEMORY_RUNTIME
looks more like a deliberate invariant than an oversight.
Thanks
Laszlo
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
