On 13 January 2015 at 16:15, Laszlo Ersek <[email protected]> wrote:
> 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.
>
Correct. I am looking into changing the allocation strategy for
runtime regions so they are aligned multiples of 64k (to accommodate
OSes running with 64k pages)
> 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.
>
OK, I am fine with that as well. In fact, I don't care deeply about
whether or not this check is performed at all, but the current
situation is a bit awkward.
--
Ard.
------------------------------------------------------------------------------
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
