From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH v1 09/21] ArmVirtualizationPkg: implement custom MemoryInitPeiLib Date: Mon, 26 Jan 2015 11:35:26 +0000 Message-ID: References: <1422025390-8036-1-git-send-email-ard.biesheuvel@linaro.org> <1422025390-8036-10-git-send-email-ard.biesheuvel@linaro.org> <54C2B619.7060800@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54C2B619.7060800@redhat.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Laszlo Ersek Cc: Ian Campbell , Olivier Martin , Stefano Stabellini , "edk2-devel@lists.sourceforge.net" , Leif Lindholm , xen-devel@lists.xen.org, Roy Franz , Ilias Biris , Anthony PERARD , Christoffer Dall List-Id: xen-devel@lists.xenproject.org On 23 January 2015 at 20:59, Laszlo Ersek wrote: > some notes > > On 01/23/15 16:02, Ard Biesheuvel wrote: >> This implements a MemoryInitPeiLib instance that differs from the >> stock ArmPlatformPkg version only in the fact that it does not remove >> the memory used by the flash device (FD). The reason is that, when using >> PrePi, the DXE core is started immediately and never returns so there is >> no reason to preserve any of the memory that the flash device occupied >> originally, and it is preferable to release is so that the OS loader >> can reuse it. This is especially important for the relocatable PrePi >> configuration, which is aimed at being launched from a boot loader that >> itself adheres to the Linux arm64 boot protocol. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> .../ArmVirtualizationMemoryInitPeiLib.c | 91 ++++++++++++++++++++++ >> .../ArmVirtualizationMemoryInitPeiLib.inf | 64 +++++++++++++++ >> 2 files changed, 155 insertions(+) >> create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c >> create mode 100644 ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c >> new file mode 100644 >> index 000000000000..5f6cd059c47f >> --- /dev/null >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c >> @@ -0,0 +1,91 @@ >> +/** @file >> +* >> +* Copyright (c) 2011-2014, ARM Limited. All rights reserved. >> +* Copyright (c) 2014, Linaro Limited. All rights reserved. >> +* >> +* This program and the accompanying materials >> +* are licensed and made available under the terms and conditions of the BSD License >> +* which accompanies this distribution. The full text of the license may be found at >> +* http://opensource.org/licenses/bsd-license.php >> +* >> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +* >> +**/ >> + >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +VOID >> +BuildMemoryTypeInformationHob ( >> + VOID >> + ); >> + >> +VOID >> +InitMmu ( >> + VOID >> + ) >> +{ >> + ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable; >> + VOID *TranslationTableBase; >> + UINTN TranslationTableSize; >> + RETURN_STATUS Status; >> + >> + // Get Virtual Memory Map from the Platform Library >> + ArmPlatformGetVirtualMemoryMap (&MemoryTable); >> + >> + //Note: Because we called PeiServicesInstallPeiMemory() before to call InitMmu() the MMU Page Table resides in >> + // DRAM (even at the top of DRAM as it is the first permanent memory allocation) >> + Status = ArmConfigureMmu (MemoryTable, &TranslationTableBase, &TranslationTableSize); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((EFI_D_ERROR, "Error: Failed to enable MMU\n")); >> + } >> +} >> + >> +EFI_STATUS >> +EFIAPI >> +MemoryPeim ( >> + IN EFI_PHYSICAL_ADDRESS UefiMemoryBase, >> + IN UINT64 UefiMemorySize >> + ) >> +{ >> + EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; >> + >> + // Ensure PcdSystemMemorySize has been set >> + ASSERT (PcdGet64 (PcdSystemMemorySize) != 0); >> + >> + // >> + // Now, the permanent memory has been installed, we can call AllocatePages() >> + // >> + ResourceAttributes = ( >> + EFI_RESOURCE_ATTRIBUTE_PRESENT | >> + EFI_RESOURCE_ATTRIBUTE_INITIALIZED | >> + EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE | >> + EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE | >> + EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | >> + EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | >> + EFI_RESOURCE_ATTRIBUTE_TESTED >> + ); >> + >> + BuildResourceDescriptorHob ( >> + EFI_RESOURCE_SYSTEM_MEMORY, >> + ResourceAttributes, >> + PcdGet64 (PcdSystemMemoryBase), >> + PcdGet64 (PcdSystemMemorySize) >> + ); >> + >> + // Build Memory Allocation Hob >> + InitMmu (); >> + >> + if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) { >> + // Optional feature that helps prevent EFI memory map fragmentation. >> + BuildMemoryTypeInformationHob (); >> + } >> + >> + return EFI_SUCCESS; >> +} >> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> new file mode 100644 >> index 000000000000..1031f64fb8de >> --- /dev/null >> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf >> @@ -0,0 +1,64 @@ >> +#/** @file >> +# >> +# Copyright (c) 2011-2014, ARM Ltd. All rights reserved.
>> +# Copyright (c) 2014, Linaro Ltd. All rights reserved.
>> +# This program and the accompanying materials >> +# are licensed and made available under the terms and conditions of the BSD License >> +# which accompanies this distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +# >> +#**/ >> + >> +[Defines] >> + INF_VERSION = 0x00010005 >> + BASE_NAME = ArmVirtMemoryInitPeiLib >> + FILE_GUID = 021b6156-3cc8-4e99-85ee-13d8a871edf2 >> + MODULE_TYPE = SEC >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = PlatformPeiLib >> + >> +[Sources] >> + ArmVirtualizationMemoryInitPeiLib.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + EmbeddedPkg/EmbeddedPkg.dec >> + ArmPkg/ArmPkg.dec >> + ArmPlatformPkg/ArmPlatformPkg.dec >> + >> +[LibraryClasses] >> + DebugLib >> + HobLib >> + ArmLib >> + ArmPlatformLib >> + >> +[Guids] >> + gEfiMemoryTypeInformationGuid >> + >> +[FeaturePcd] >> + gEmbeddedTokenSpaceGuid.PcdPrePiProduceMemoryTypeInformationHob >> + >> +[FixedPcd] >> + gArmTokenSpaceGuid.PcdFdBaseAddress >> + gArmTokenSpaceGuid.PcdFdSize >> + >> + gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize >> + >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode >> + gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData >> + gArmTokenSpaceGuid.PcdSystemMemoryBase >> + gArmTokenSpaceGuid.PcdSystemMemorySize >> + >> +[Depex] >> + TRUE >> > > It makes sense to compare the files added here with their origins: > >> --- MemoryInitPei/MemoryInitPeiLib.inf 2014-10-01 00:12:25.358758489 +0200 >> +++ ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.inf 2015-01-23 21:27:39.718619437 +0100 >> @@ -1,8 +1,9 @@ >> #/** @file >> # >> # Copyright (c) 2011-2014, ARM Ltd. All rights reserved.
>> +# Copyright (c) 2014, Linaro Ltd. All rights reserved.
>> # This program and the accompanying materials >> # are licensed and made available under the terms and conditions of the BSD License >> # which accompanies this distribution. The full text of the license may be found at >> # http://opensource.org/licenses/bsd-license.php >> # >> @@ -11,19 +12,18 @@ >> # >> #**/ >> >> [Defines] >> INF_VERSION = 0x00010005 >> - BASE_NAME = ArmMemoryInitPeiLib >> - FILE_GUID = 55ddb6e0-70b5-11e0-b33e-0002a5d5c51b >> + BASE_NAME = ArmVirtMemoryInitPeiLib >> + FILE_GUID = 021b6156-3cc8-4e99-85ee-13d8a871edf2 > > okay, new GUID > >> MODULE_TYPE = SEC >> VERSION_STRING = 1.0 >> LIBRARY_CLASS = PlatformPeiLib > > Ugh, confusing library class, but it is already confusing in the original! > > Both should say MemoryInitPeiLib I guess. > Lemme fix that >> >> [Sources] >> - MemoryInitPeiLib.c >> - >> + ArmVirtualizationMemoryInitPeiLib.c >> >> [Packages] >> MdePkg/MdePkg.dec >> MdeModulePkg/MdeModulePkg.dec >> EmbeddedPkg/EmbeddedPkg.dec >> @@ -55,12 +55,10 @@ >> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode >> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesCode >> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData >> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode >> gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData >> - >> -[Pcd] >> gArmTokenSpaceGuid.PcdSystemMemoryBase >> gArmTokenSpaceGuid.PcdSystemMemorySize > > Care to explain this a bit? Is this related to the relocation thing? (We need FixedPCDs because thats what we can relocate?) > Exactly. Or PatchPcds perhaps ... But this is not necessarily the place to put such restrictions: I will put the FixedPcd (or PatchPcd) in the module that actually performs the relocations, which will force the entire platform to use those. >> >> [Depex] >> TRUE >> --- MemoryInitPei/MemoryInitPeiLib.c 2014-11-18 19:49:09.922977213 +0100 >> +++ ArmVirtualizationPkg/Library/ArmVirtualizationMemoryInitPeiLib/ArmVirtualizationMemoryInitPeiLib.c 2015-01-23 21:27:39.717619432 +0100 >> @@ -1,8 +1,9 @@ >> /** @file >> * >> * Copyright (c) 2011-2014, ARM Limited. All rights reserved. >> +* Copyright (c) 2014, Linaro Limited. All rights reserved. >> * >> * This program and the accompanying materials >> * are licensed and made available under the terms and conditions of the BSD License >> * which accompanies this distribution. The full text of the license may be found at >> * http://opensource.org/licenses/bsd-license.php >> @@ -44,40 +45,18 @@ InitMmu ( >> if (EFI_ERROR (Status)) { >> DEBUG ((EFI_D_ERROR, "Error: Failed to enable MMU\n")); >> } >> } >> >> -/*++ >> - >> -Routine Description: >> - >> - >> - >> -Arguments: >> - >> - FileHandle - Handle of the file being invoked. >> - PeiServices - Describes the list of possible PEI Services. >> - >> -Returns: >> - >> - Status - EFI_SUCCESS if the boot mode could be set >> - >> ---*/ >> EFI_STATUS >> EFIAPI >> MemoryPeim ( >> IN EFI_PHYSICAL_ADDRESS UefiMemoryBase, >> IN UINT64 UefiMemorySize >> ) >> { >> EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; >> - UINT64 ResourceLength; >> - EFI_PEI_HOB_POINTERS NextHob; >> - EFI_PHYSICAL_ADDRESS FdTop; >> - EFI_PHYSICAL_ADDRESS SystemMemoryTop; >> - EFI_PHYSICAL_ADDRESS ResourceTop; >> - BOOLEAN Found; >> >> // Ensure PcdSystemMemorySize has been set >> ASSERT (PcdGet64 (PcdSystemMemorySize) != 0); >> >> // >> @@ -91,79 +70,17 @@ MemoryPeim ( >> EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE | >> EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | >> EFI_RESOURCE_ATTRIBUTE_TESTED >> ); >> >> - // Reserved the memory space occupied by the firmware volume >> BuildResourceDescriptorHob ( >> EFI_RESOURCE_SYSTEM_MEMORY, >> ResourceAttributes, >> PcdGet64 (PcdSystemMemoryBase), >> PcdGet64 (PcdSystemMemorySize) >> ); >> >> - SystemMemoryTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemoryBase) + (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemorySize); >> - FdTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdFdBaseAddress) + (EFI_PHYSICAL_ADDRESS)PcdGet32 (PcdFdSize); >> - >> - // EDK2 does not have the concept of boot firmware copied into DRAM. To avoid the DXE >> - // core to overwrite this area we must mark the region with the attribute non-present >> - if ((PcdGet64 (PcdFdBaseAddress) >= PcdGet64 (PcdSystemMemoryBase)) && (FdTop <= SystemMemoryTop)) { >> - Found = FALSE; >> - >> - // Search for System Memory Hob that contains the firmware >> - NextHob.Raw = GetHobList (); >> - while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, NextHob.Raw)) != NULL) { >> - if ((NextHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) && >> - (PcdGet64 (PcdFdBaseAddress) >= NextHob.ResourceDescriptor->PhysicalStart) && >> - (FdTop <= NextHob.ResourceDescriptor->PhysicalStart + NextHob.ResourceDescriptor->ResourceLength)) >> - { >> - ResourceAttributes = NextHob.ResourceDescriptor->ResourceAttribute; >> - ResourceLength = NextHob.ResourceDescriptor->ResourceLength; >> - ResourceTop = NextHob.ResourceDescriptor->PhysicalStart + ResourceLength; >> - >> - if (PcdGet64 (PcdFdBaseAddress) == NextHob.ResourceDescriptor->PhysicalStart) { >> - if (SystemMemoryTop == FdTop) { >> - NextHob.ResourceDescriptor->ResourceAttribute = ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT; >> - } else { >> - // Create the System Memory HOB for the firmware with the non-present attribute >> - BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY, >> - ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT, >> - PcdGet64 (PcdFdBaseAddress), >> - PcdGet32 (PcdFdSize)); >> - >> - // Top of the FD is system memory available for UEFI >> - NextHob.ResourceDescriptor->PhysicalStart += PcdGet32(PcdFdSize); >> - NextHob.ResourceDescriptor->ResourceLength -= PcdGet32(PcdFdSize); >> - } >> - } else { >> - // Create the System Memory HOB for the firmware with the non-present attribute >> - BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY, >> - ResourceAttributes & ~EFI_RESOURCE_ATTRIBUTE_PRESENT, >> - PcdGet64 (PcdFdBaseAddress), >> - PcdGet32 (PcdFdSize)); >> - >> - // Update the HOB >> - NextHob.ResourceDescriptor->ResourceLength = PcdGet64 (PcdFdBaseAddress) - NextHob.ResourceDescriptor->PhysicalStart; >> - >> - // If there is some memory available on the top of the FD then create a HOB >> - if (FdTop < NextHob.ResourceDescriptor->PhysicalStart + ResourceLength) { >> - // Create the System Memory HOB for the remaining region (top of the FD) >> - BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY, >> - ResourceAttributes, >> - FdTop, >> - ResourceTop - FdTop); >> - } >> - } >> - Found = TRUE; >> - break; >> - } >> - NextHob.Raw = GET_NEXT_HOB (NextHob); >> - } >> - >> - ASSERT(Found); >> - } >> - >> // Build Memory Allocation Hob >> InitMmu (); >> >> if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) { >> // Optional feature that helps prevent EFI memory map fragmentation. > > So I guess this is what you explain in the commit message. I wanted to ask why it is safe for ArmVirtualizationQemu.dsc. > > And then I noticed -- in ArmVirtualizationQemu.dsc we use PrePei, and here you use PrePi. Meaning, in ArmVirtualizationQemu we use > > ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf > > whereas here you're cloning & modifying > > ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf > > and the latter isn't, nor will be, used at all in ArmVirtualizationQemu, only in the new platform DSC file & corresponding FDF that you'll introduce. IOW, ArmVirtualizationQemu will not be switched over to the code you're adding here. Is that correct? > Correct. I merged the 'runtime relocatable PrePi' and 'xen/arm guest' series, and not all of the patches belonging to the former are fully relevant to the latter. I will try to clean up the irrelevant or confusing bits in v2.