Re: [Xen-devel] [PATCH v1 09/21] ArmVirtualizationPkg: implement custom MemoryInitPeiLib

[Laszlo Ersek <lersek@xxxxxxxxxx>]

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 <ard.biesheuvel@xxxxxxxxxx>
> ---
>  .../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 <PiPei.h>
> +
> +#include <Library/ArmPlatformLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +
> +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.<BR>
> +#  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
> +#  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.<BR>
> +#  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
>  #  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.

>  
>  [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?)

>  
>  [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?

Thanks
Laszlo

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel