Hi, Ard Do you see any real impact of this issue when you tried to change runtime region allocation strategy?
I just did a quick search for possible updates on Attribute field, please see the calling flow: gBS->SetMemorySpaceCapabilities() -> CoreUpdateMemoryAttributes() -> CoreConvertPagesEx() -> CoreAddRange () -> InsertTailList () it means Attribute field may be set to EFI_MEMORY_RUNTIME by caller, am I right? Thanks Feng -----Original Message----- From: Ard Biesheuvel [mailto:[email protected]] Sent: Wednesday, January 14, 2015 00:19 To: [email protected] Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate on internal memory map 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 ------------------------------------------------------------------------------ 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
