Re: [PATCH v2 12/14] ArmVirtPkg: create QemuVirtMemInfoLib version for ArmVirtQemu

[Laszlo Ersek <lersek@redhat.com>]

On 11/22/17 11:07, Ard Biesheuvel wrote:
> The QemuVirtMemInfoLib ArmVirtMemInfoLib implementation created for
> ArmVirtQemuKernel does exactly what we need for ArmVirtQemu, the only
> difference being that the latter is PrePeiCore based, and so it uses
> a different method to ensure that PcdSystemMemorySize is set when
> ArmVirtGetMemoryMap() is called.
> 
> On ArmVirtQemu, we currently abuse the implied ordering guarantees
> provided by ArmPlatformLib, by implementing this as follows:
> 
>   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf [ArmVirtPkg/ArmVirtQemu.dsc]
>     InitializeMemory()                            [ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c]
>       ArmPlatformInitializeSystemMemory()         [ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c]
>         //
>         // set PcdSystemMemorySize from the DT
>         //
>       MemoryPeim()                                [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>         InitMmu()                                 [ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c]
>           ArmPlatformGetVirtualMemoryMap()        [ArmVirtPkg/Library/ArmVirtPlatformLib/VirtMem.c]
>             //
>             // consume PcdSystemMemorySize
>             //
> 
> Given that we are trying to get rid of ArmPlatformLib, or at least remove
> some of these API functions that are never used for their original purpose
> by any platforms, we need to move the PCD assignment elsewhere.
> 
> So create a PEIM-only version of QemuVirtMemInfoLib especially for
> ArmVirtQemu, and add the PCD assignment code to its constructor.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemu.dsc                                               |   3 +
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf          |  58 +++++++++++
>  ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c | 106 ++++++++++++++++++++
>  3 files changed, 167 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index d14a0dd0d1d9..519c2ae2e939 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -67,6 +67,9 @@ [LibraryClasses.common]
>    HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf
>  !endif
>  
> +[LibraryClasses.common.PEIM]
> +  ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> +
>  [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>  
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> new file mode 100644
> index 000000000000..e574a47443d0
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
> @@ -0,0 +1,58 @@
> +#/* @file
> +#
> +#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2014-2017, 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.
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = QemuVirtMemInfoLib

(1) Please update BASE_NAME before pushing.

> +  FILE_GUID                      = 0c4d10cf-d949-49b4-bd13-47a4ae22efce
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +  CONSTRUCTOR                    = QemuVirtMemInfoPeiLibConstructor
> +
> +[Sources]
> +  QemuVirtMemInfoLib.c
> +  QemuVirtMemInfoPeiLibConstructor.c
> +
> +[Sources.ARM]
> +  Arm/PhysAddrTop.S
> +
> +[Sources.AARCH64]
> +  AArch64/PhysAddrTop.S
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +  MemoryAllocationLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
> new file mode 100644
> index 000000000000..ef8ac6e018d1
> --- /dev/null
> +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
> @@ -0,0 +1,106 @@
> +/** @file
> +
> +  Copyright (c) 2014-2017, 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 <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <libfdt.h>
> +
> +RETURN_STATUS
> +EFIAPI
> +QemuVirtMemInfoPeiLibConstructor (
> +  VOID
> +  )
> +{
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
> +
> +  NewBase = 0;
> +  NewSize = 0;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  //
> +  // Make sure we have a valid device tree blob
> +  //
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  //
> +  // Look for the lowest memory node
> +  //
> +  for (Prev = 0;; Prev = Node) {
> +    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> +    if (Node < 0) {
> +      break;
> +    }
> +
> +    //
> +    // Check for memory node
> +    //
> +    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> +    if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> +      //
> +      // Get the 'reg' property of this node. For now, we will assume
> +      // two 8 byte quantities for base and size, respectively.
> +      //
> +      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> +      if (RegProp != 0 && Len == (2 * sizeof (UINT64))) {
> +
> +        CurBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
> +        CurSize = fdt64_to_cpu (ReadUnaligned64 (RegProp + 1));
> +
> +        DEBUG ((DEBUG_INFO, "%a: System RAM @ 0x%lx - 0x%lx\n",
> +          __FUNCTION__, CurBase, CurBase + CurSize - 1));
> +
> +        if (NewBase > CurBase || NewBase == 0) {
> +          NewBase = CurBase;
> +          NewSize = CurSize;
> +        }
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: Failed to parse FDT memory node\n",
> +          __FUNCTION__));

(2) Thanks for improving the indentation of the DEBUGs.

("--find-copies-harder" remains awesome :) )

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

> +      }
> +    }
> +  }
> +
> +  //
> +  // Make sure the start of DRAM matches our expectation
> +  //
> +  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  //
> +  // We need to make sure that the machine we are running on has at least
> +  // 128 MB of memory configured, and is currently executing this binary from
> +  // NOR flash. This prevents a device tree image in DRAM from getting
> +  // clobbered when our caller installs permanent PEI RAM, before we have a
> +  // chance of marking its location as reserved or copy it to a freshly
> +  // allocated block in the permanent PEI RAM in the platform PEIM.
> +  //
> +  ASSERT (NewSize >= SIZE_128MB);
> +  ASSERT (
> +    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
> +      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
> +    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
> +
> +  return RETURN_SUCCESS;
> +}
>