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

[Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>]

On 23 January 2015 at 20:59, Laszlo Ersek <lersek@xxxxxxxxxx> 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 <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.
>

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.

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