On 22 February 2016 at 11:24, Laszlo Ersek <[email protected]> wrote: > 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. >
OK, I will change this. > (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.) > I understand very little about how all the components interact. I was also surprised we use BdsDxe in IntelFrameworkModulePkg, and not the generic one (which contains the exact same code for the perfdata allocation below 4 GB) >> ________________________________________ >> 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
