Ard, Ok. I will use assertion here just like Laszlo's patch.
Thanks Feng -----Original Message----- From: Ard Biesheuvel [mailto:[email protected]] Sent: Monday, January 19, 2015 17:25 To: Tian, Feng Cc: [email protected] Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate on internal memory map On 19 January 2015 at 05:17, Tian, Feng <[email protected]> wrote: > 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 > Hello Feng, As Laszlo had already mentioned in his original reply, there is really no point in doing this: + if (mMemoryTypeStatistics[Entry->Type].Runtime) { + if (Entry->Type == EfiACPIReclaimMemory || Entry->Type == EfiACPIMemoryNVS) { because we know the mMemoryTypeStatistics array has the Runtime field cleared for these memory types. Other than that, the patch looks fine. Acked-by: Ard Biesheuvel <[email protected]> Regards, Ard. > -----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 ------------------------------------------------------------------------------ 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
