| Message ID | CAKv+Gu83sKz3HCRdF_dDZgm_oM0JhSg7=GXNZKGL9sMj=BY1ow@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 19 January 2015 at 05:17, Tian, Feng <feng.tian@intel.com> 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 <ard.biesheuvel@linaro.org> > Review-by: Feng Tian <feng.tian@intel.com> > > 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 <ard.biesheuvel@linaro.org> Regards, Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > Sent: Thursday, January 15, 2015 16:12 > To: Tian, Feng > Cc: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where appropriate on internal memory map > > On 15 January 2015 at 07:25, Tian, Feng <feng.tian@intel.com> 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:ard.biesheuvel@linaro.org] >> Sent: Wednesday, January 14, 2015 00:19 >> To: edk2-devel@lists.sourceforge.net >> Subject: Re: [edk2] [PATCH] DxeCore: set EFI_MEMORY_RUNTIME where >> appropriate on internal memory map >> >> On 13 January 2015 at 16:15, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org> >>>> --- >>>> 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 >> edk2-devel@lists.sourceforge.net >> 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
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 ///