Ard, I am prone to Laszlo's fix and make a little update to avoid array out-of-boundary issue.
Please help review it. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <[email protected]> Review-by: Feng Tian <[email protected]> Thanks Feng -----Original Message----- From: Ard Biesheuvel [mailto:[email protected]] Sent: Thursday, January 15, 2015 16:12 To: Tian, Feng Cc: [email protected] Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate on internal memory map On 15 January 2015 at 07:25, Tian, Feng <[email protected]> wrote: > Hi, Ard > > Do you see any real impact of this issue when you tried to change runtime > region allocation strategy? > Yes. On 64-bit ARM (AArch64), the OS can decide to use 64 KB pages instead of 4 KB pages. As an optimization, I tried to change the allocation strategy for runtime regions in Tianocore so that they are always 64 KB aligned multiple of 64 KB, even if UEFI's native page size is 4 KB. The default allocation can remain at 4 KB, as UEFI itself runs with 4 KB pages regardless. With the following patch applied ---->8---- diff --git a/MdeModulePkg/Core/Dxe/Mem/Imem.h b/MdeModulePkg/Core/Dxe/Mem/Imem.h index d09ff3c5220f..41204c8cf412 100644 --- a/MdeModulePkg/Core/Dxe/Mem/Imem.h +++ b/MdeModulePkg/Core/Dxe/Mem/Imem.h @@ -22,6 +22,14 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (EFI_PAGE_SIZE * 2) #define DEFAULT_PAGE_ALLOCATION (EFI_PAGE_SIZE * 2) +#elif defined (MDE_CPU_AARCH64) +/// +/// OSes on 64-bit ARM may run with 64k pages, so align the runtime /// +regions to 64k as well /// #define +EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT (EFI_PAGE_SIZE * 16) +#define DEFAULT_PAGE_ALLOCATION (EFI_PAGE_SIZE) + #else /// /// For genric EFI machines make the default allocations 4K aligned ---->8---- I only get a partial result: AllocatePages () will adhere to the larger alignment, but AllocatePool () ignores it. This is a separate issues that needs to be addressed in Pool.c But more importantly, the checks in CoreTerminateMemoryMap() do not detect this at all, hence my patch. > I just did a quick search for possible updates on Attribute field, please see > the calling flow: > gBS->SetMemorySpaceCapabilities() -> CoreUpdateMemoryAttributes() -> > gBS->CoreConvertPagesEx() -> CoreAddRange () -> InsertTailList () > > it means Attribute field may be set to EFI_MEMORY_RUNTIME by caller, am I > right? > Yes, it may be set by the caller, but it is clearly not the intention of the sanity check in CoreTerminateMemoryMap() to only detect regions whose attributes where modified by SetMemorySpaceCapabilities() [which only has one caller in the entire code base] The intention is obviously to make ExitBootServices () fail if *any* of the runtime regions do not adhere to EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT, and that is quite clearly broken. Regards, Ard. > -----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
Page.c.patch
Description: Page.c.patch
------------------------------------------------------------------------------ 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
