I did a search on current MdeModulePkg. I found there are more modules allocating Below4G memory. Besides BDS, we have BootGraphicsResourceTableDxe, BootScriptExecutorDxe, FirmwarePerformanceDataTableDxe, PiDxeS3BootScriptLib, CapsuleRuntimeDxe.
BootGraphicsResourceTableDxe - I am not clear. CapsuleRuntimeDxe - Below4G is designed for PEI capsule. Or we do not need such capability. BootScriptExecutorDxe, FirmwarePerformanceDataTableDxe, PiDxeS3BootScriptLib - data need to be access in PEI phase. I think we might need more clean up for 32bit PEI. I am still thinking if we can reuse old Pcd - we can clarify in PCD description - it is only for 32bit PEI and 64bit DXE. Or a new PCD, and name to Pcd32BitPei? Please allow me collect more feedback on that. In order to separate 32bit PEI issue from ACPI issue, I think we can check in ACPI patch at first, if you want. Thank you Yao Jiewen From: Ni, Ruiyu Sent: Monday, February 22, 2016 11:36 AM To: Laszlo Ersek; 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 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]<mailto:[email protected]>; Tian, Feng; Zeng, Star; [email protected]<mailto:[email protected]>; [email protected]<mailto:[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 > --- > 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. > + # FALSE - PEI accessible memory may be allocated anywhere. > + # @Prompt Allocated PEI accessible memory below 4 GB > + > + gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdPeiAllocMemLimit4GB|TRU > + E|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 _______________________________________________ edk2-devel mailing list [email protected]<mailto:[email protected]> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
