Thanks. I think it is good idea to change default value to FALSE to non-x86 platform.
Thank you Yao Jiewen From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] Sent: Friday, February 19, 2016 7:52 PM To: Laszlo Ersek Cc: Yao, Jiewen; edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; leif.lindh...@linaro.org; graeme.greg...@linaro.org; Fan, Jeff; mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com Subject: Re: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0 On 19 February 2016 at 10:56, Laszlo Ersek wrote: > On 02/19/16 09:53, Ard Biesheuvel wrote: >> On 19 February 2016 at 09:45, Yao, Jiewen wrote: >>> I can explain the reason on allocating >>> >>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. >>> So we have to limit the allocation <4G. >>> >> >> OK, got it. >> >>> Using ACPI version PCD looks strange here. Maybe another PCD, like >>> PcdDxeIplSwitchToLongMode? >>> >> >> But that does not tell us if we are running a 32-bit PEI, right? It >> only tells us if a 32-bit PEI should load a 32-bit DXE core on a >> 64-bit capable machine. > > 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE. > > Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE. > > (This is what we have in the three DSC files of OVMF.) > > So, in order to see in DXE if your PEI is 32-bit, you can do: > > BOOLEAN PeiIs32Bit; > > PeiIs32Bit = FALSE; > if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > // > // this implies a 32->64 switch > // > PeiIs32Bit = TRUE; > } else { > // > // otherwise, PEI and DXE have the same bitness, > // so derive it from DXE's bitness > // > #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM) > PeiIs32Bit = TRUE; > #endif > } > > Alternatively: > > PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) || > sizeof (UINTN) == sizeof (UINT32); > > I'll admit I'm not really sure about EBC. The above mostly treats EBC > as nonexistent. :) > Thanks for clarifying. So the only case where we have to take care to allocate below 4 GB is the 32->64 case, since in all other cases, the memory allocated in DXE will always be addressable in PEI. So I will take Jiewen's suggestion, and only limit the memory allocation if PcdDxeIplSwitchToLongMode is TRUE. But I will still need the MDE_CPU_X64 ifdef, since PcdDxeIplSwitchToLongMode is only defined for IA32 and X64, and since it defaults to TRUE, defining it for other archs would mean we have to change the default value as well. Alternatively, I could propose this: [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64] gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b [PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.IPF] gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b and only test the PCD, but I don't think that is an improvement. -- Ard. >> >>> -----Original Message----- >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>> Sent: Friday, February 19, 2016 4:03 AM >>> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Tian, Feng; >>> Zeng, Star; leif.lindh...@linaro.org<mailto:leif.lindh...@linaro.org>; >>> graeme.greg...@linaro.org<mailto:graeme.greg...@linaro.org>; >>> ler...@redhat.com<mailto:ler...@redhat.com>; Fan, Jeff; Yao, Jiewen >>> Cc: mark.rutl...@arm.com<mailto:mark.rutl...@arm.com>; Gao, Liming; >>> samer.el-haj-mahm...@hpe.com<mailto:samer.el-haj-mahm...@hpe.com>; Ard >>> Biesheuvel >>> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate >>> below 4 GB on ACPI 1.0 >>> >>> It is not entirely clear from the code why, but the reserved region for >>> storing performance data is allocated below 4 GB, and the variable to hold >>> the address of the allocation is called 'AcpiLowMemoryBase' >>> (which is the only mention of ACPI in the entire file). >>> >>> Let's make this allocation dependent on PcdAcpiExposedTableVersions as >>> well, since systems that can deal with ACPI table in high memory are also >>> just as likely to be able to deal with performance data in high memory. >>> This prevents memory fragmentation on systems that don't care about the 4 >>> GB limit. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel >>> --- >>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf | 1 + >>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++++++++++- >>> 2 files changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf >>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf >>> index 6afb8a09df9c..38172780fb49 100644 >>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf >>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf >>> @@ -215,6 +215,7 @@ [Pcd] >>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution ## CONSUMES >>> gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution ## CONSUMES >>> gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable ## CONSUMES >>> + gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions ## CONSUMES >>> >>> [Depex] >>> TRUE >>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> index ae7ad2153c51..bf5bd6524a90 100644 >>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c >>> @@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >>> EXPRESS OR IMPLIED. >>> #include "Hotkey.h" >>> #include "HwErrRecSupport.h" >>> >>> +#include >>> + >>> /// >>> /// BDS arch protocol instance initial value. >>> /// >>> @@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData ( >>> EFI_STATUS Status; >>> EFI_PHYSICAL_ADDRESS AcpiLowMemoryBase; >>> EDKII_VARIABLE_LOCK_PROTOCOL *VariableLock; >>> + EFI_ALLOCATE_TYPE AcpiTableAllocType; >>> >>> AcpiLowMemoryBase = 0x0FFFFFFFFULL; >>> >>> + if ((PcdGet32 (PcdAcpiExposedTableVersions) & >>> EFI_ACPI_TABLE_VERSION_1_0B) != 0) { >>> + AcpiTableAllocType = AllocateMaxAddress; } else { >>> + AcpiTableAllocType = AllocateAnyPages; } >>> + >>> // >>> // Allocate a block of memory that will contain performance data to OS. >>> // >>> Status = gBS->AllocatePages ( >>> - AllocateMaxAddress, >>> + AcpiTableAllocType, >>> EfiReservedMemoryType, >>> EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH), >>> &AcpiLowMemoryBase >>> -- >>> 2.5.0 >>> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
