Ard, So the final decision between you and Jiewen is a new PCD is still needed? If so, could you move the PCD to MdeModulePkg? because the MdeModulePkg/BDS actually may also need this change if it's used in ARM64 platform and MdeModulePkg shouldn't depend on a PCD defined in IntelFrameworkModulePkg.
Thanks, Ray ________________________________________ From: edk2-devel [[email protected]] on behalf of Laszlo Ersek [[email protected]] Sent: Friday, February 19, 2016 21:40 To: Ard Biesheuvel; [email protected]; Tian, Feng; Zeng, Star; [email protected]; [email protected]; Fan, Jeff; Yao, Jiewen Cc: Gao, Liming Subject: Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB if needed On 02/19/16 14:15, Ard Biesheuvel wrote: > The reserved region for storing performance data is allocated below 4 GB, > so that its contents are accessible from PEI upon S3 resume. However, this > is a X64 pecularity, and on AARCH64 systems, which does not have this > restriction (nor does it have S3, btw), this limit may cause the allocation > to fail (if the platform does not have memory below 4 GB) or needlessly > fragment the memory map if it does succeed. > > So make this behavior conditional, based on the new PcdPeiAllocMemLimit4GB > PCD, which defaults to TRUE, but can be overridden by platforms if they > prefer allocations of PEI accessible memory to be served from anywhere. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec | 6 ++++++ > IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf | 7 ++++--- > IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 13 +++++++++++-- > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec > b/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec > index 8bbde8e2c9c8..005d356f5938 100644 > --- a/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec > +++ b/IntelFrameworkModulePkg/IntelFrameworkModulePkg.dec > @@ -161,6 +161,12 @@ [PcdsFeatureFlag] > # @Prompt Enable Boot Logo only > > gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable|FALSE|BOOLEAN|0x00010048 > > + ## Indicates whether allocations for PEI accessible memory should be below > the 4 GB mark > + # TRUE - PEI accessible memory should be allocated below 4 GB.<BR> > + # FALSE - PEI accessible memory may be allocated anywhere.<BR> > + # @Prompt Allocated PEI accessible memory below 4 GB > + > gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB|TRUE|BOOLEAN|0x00010049 > + > [PcdsFixedAtBuild, PcdsPatchableInModule] > ## FFS filename to find the default BMP Logo file. > # @Prompt FFS Name of Boot Logo File > diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf > b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf > index 6afb8a09df9c..fa210836f410 100644 > --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf > +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf > @@ -181,9 +181,10 @@ [Protocols] > gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES > > [FeaturePcd] > - gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate ## CONSUMES > - gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## CONSUMES > - gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable ## CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangDeprecate ## > CONSUMES > + gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport ## > CONSUMES > + gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdBootlogoOnlyEnable ## > CONSUMES > + gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB ## > CONSUMES > > [Pcd] > gEfiMdePkgTokenSpaceGuid.PcdUefiVariableDefaultLangCodes ## > SOMETIMES_CONSUMES > diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c > b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c > index ae7ad2153c51..3b524db94b3d 100644 > --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c > +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c > @@ -474,14 +474,23 @@ BdsAllocateMemoryForPerformanceData ( > EFI_STATUS Status; > EFI_PHYSICAL_ADDRESS AcpiLowMemoryBase; > EDKII_VARIABLE_LOCK_PROTOCOL *VariableLock; > + EFI_ALLOCATE_TYPE AllocType; > > - AcpiLowMemoryBase = 0x0FFFFFFFFULL; > + // > + // The memory we allocate here must be accessible from PEI at resume. > + // > + if (FeaturePcdGet (PcdPeiAllocMemLimit4GB)) { > + AcpiLowMemoryBase = 0x0FFFFFFFFULL; > + AllocType = AllocateMaxAddress; > + } else { > + AllocType = AllocateAnyPages; > + } > > // > // Allocate a block of memory that will contain performance data to OS. > // > Status = gBS->AllocatePages ( > - AllocateMaxAddress, > + AllocType, > EfiReservedMemoryType, > EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH), > &AcpiLowMemoryBase > My review is of course not authoritative for this package, but still: Reviewed-by: Laszlo Ersek <[email protected]> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
