On 02/22/16 04:35, Ni, Ruiyu wrote: > 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.
Good point; I forgot about the possibility that a similar allocation could be made in MdeModulePkg's BDS driver. (Also, although it's not urgent, it would be nice if we could again think about migrating OVMF to the newest BDS in MdeModulePkg. Then hopefully we'll have learned enough to migrate ArmVirtPkg too.) Thanks! Laszlo > > 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
