Re: [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library

[Laszlo Ersek <lersek@redhat.com>]

On 11/17/17 17:09, Ard Biesheuvel wrote:
> Equivalent to the PrePi based platforms, this patch implements the
> newly introduced ArmVirtMemInfo library class via a separate PEIM
> and PPI.
>
> The reason is that ArmVirtPlatformLib has populated the ArmPlatformLib
> API function ArmPlatformInitializeSystemMemory () to retrieve memory
> information from the DT, ensuring that it will be present when
> MemoryPeim() is executed. This is a bit of a hack, and someting we
> will need to get rid of if we want to reduce our dependency on
> ArmPlatformLib altogether.

OK, so whenever I try to look at this code, my brain crashes. This
occasion is no exception. All the more reason to clean it all up; thanks
for doing that.

So, I guess we are talking about the following call stack. If you agree,
please add it to the commit message (IMO it's OK to exceed the preferred
74 chars width for such sections):

  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
            //

Here we have two ArmVirtPlatformLib actions:

- The "PCD consumption" half of that has been moved -- well, copied, for
  now -- to QemuVirtMemInfoLib, in patch 12/15.

- And we are now looking for a new home for the "PCD production" half.

> Putting this code in a ArmVirtMemInfo library constructor is
> problematic as well, given that the implementation uses other
> libraries, among which PcdLib, and so we need to find another way to
> run it before MemoryPeim()

Hm, this claim could be true :) , but I'm not totally convinced by the
way you put it.

First off, I notice that
"ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf" does not
spell out PcdLib as a dependency. This is quite bad, because it uses
PCDs.

However, ultimately, "gEfiPeiPcdPpiGuid" does end up in the final DEPEX
of "MemoryInitPeim.inf", according to the ArmVirtQemu build report file.
This must be due to some of the library instances already pulling in
PeiPcdLib.

Therefore, if we modify the constructor of QemuVirtMemInfoLib to parse
the DT and to set the PCD, *plus* we spell out PcdLib in the
QemuVirtMemInfoLib INF file, then the ultimate DEPEX for the
MemoryInitPeim module should remain the same. And, the PeiPcdLib
constructor should run before the QemuVirtMemInfoLib constructor
(parsing the DT and setting the PCD).

What's wrong with this argument?

> So instead, create a separate PEIM that encapsulates the
> ArmVirtMemInfo code and exposes it via a PPI. Another ArmVirtMemInfo
> library class implementation is also provided that depexes on the PPI,
> which ensures that the code is called in the correct order.

I understand what this does, but I find it very complex.

Sometimes, whenever we want to make sure that a PCD is set dynamically
"no later than" it's consumed by a "common" module outside of
ArmVirtPkg, we create a dedicated library instance (with all the right
library dependencies spelled out in its INF file), set the PCD in its
constructor, and hook it into the consumer module via NULL class
resolution.

In this case, we have a lib instance that is used through an actual lib
class already, so I would suggest to add a constructor function, and to
spell out the PcdLib dependency in the INF file.

... In fact, this is something that I missed in patch 12 --
QemuVirtMemInfoLib already uses PCDs, but doesn't include "PcdLib" under
[LibraryClasses]. That is a bug inherited from ArmVirtPlatformLib (see
above), and should be fixed. And then we should only need the
constructor.

What do you think?

Thanks,
Laszlo

>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec                                  |   3 +
>  ArmVirtPkg/Include/Ppi/ArmVirtMemInfo.h                    |  48 ++++++++
>  ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.c   |  46 ++++++++
>  ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.inf |  42 +++++++
>  ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.c       | 121 ++++++++++++++++++++
>  ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf     |  60 ++++++++++
>  6 files changed, 320 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index 8f656fd2739d..260849dc845c 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -39,6 +39,9 @@ [Guids.common]
>
>    gArmVirtVariableGuid   = { 0x50bea1e5, 0xa2c5, 0x46e9, { 0x9b, 0x3a, 0x59, 0x59, 0x65, 0x16, 0xb0, 0x0a } }
>
> +[Ppis]
> +  gArmVirtMemInfoPpiGuid = { 0x3b060b72, 0x8696, 0x4393, { 0xa8, 0x93, 0x34, 0x25, 0x1e, 0x3f, 0x8a, 0x6b } }
> +
>  [Protocols]
>    gFdtClientProtocolGuid = { 0xE11FACA0, 0x4710, 0x4C8E, { 0xA7, 0xA2, 0x01, 0xBA, 0xA2, 0x59, 0x1B, 0x4C } }
>
> diff --git a/ArmVirtPkg/Include/Ppi/ArmVirtMemInfo.h b/ArmVirtPkg/Include/Ppi/ArmVirtMemInfo.h
> new file mode 100644
> index 000000000000..46885d02c384
> --- /dev/null
> +++ b/ArmVirtPkg/Include/Ppi/ArmVirtMemInfo.h
> @@ -0,0 +1,48 @@
> +/** @file
> +
> +  Copyright (c) 2011-2013, ARM Limited. All rights reserved.
> +  Copyright (c) 2017, 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.
> +
> +**/
> +
> +#ifndef _ARM_VIRT_MEMINFO_PPI_H_
> +#define _ARM_VIRT_MEMINFO_PPI_H_
> +
> +#include <Library/ArmLib.h>
> +
> +#define ARM_VIRT_MEMINFO_PPI_GUID \
> +  { 0x3b060b72, 0x8696, 0x4393, { 0xa8, 0x93, 0x34, 0x25, 0x1e, 0x3f, 0x8a, 0x6b } }
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
> +                                    describing a Physical-to-Virtual Memory
> +                                    mapping. This array must be ended by a
> +                                    zero-filled entry
> +
> +**/
> +typedef
> +VOID
> +(EFIAPI * GET_MEMORY_MAP) (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR    **VirtualMemoryMap
> +  );
> +
> +typedef struct {
> +  GET_MEMORY_MAP    GetMemoryMap;
> +} ARM_VIRT_MEMINFO_PPI;
> +
> +extern EFI_GUID gArmVirtMemInfoPpiGuid;
> +
> +#endif
> diff --git a/ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.c b/ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.c
> new file mode 100644
> index 000000000000..ad27b246f980
> --- /dev/null
> +++ b/ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.c
> @@ -0,0 +1,46 @@
> +/** @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 <PiPei.h>
> +#include <Library/ArmLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PeiServicesLib.h>
> +#include <Ppi/ArmVirtMemInfo.h>
> +
> +/**
> +  Return the Virtual Memory Map of your platform
> +
> +  This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU
> +  on your platform.
> +
> +  @param[out]   VirtualMemoryMap    Array of ARM_MEMORY_REGION_DESCRIPTOR
> +                                    describing a Physical-to-Virtual Memory
> +                                    mapping. This array must be ended by a
> +                                    zero-filled entry
> +
> +**/
> +VOID
> +ArmVirtGetMemoryMap (
> +  OUT ARM_MEMORY_REGION_DESCRIPTOR   **VirtualMemoryMap
> +  )
> +{
> +  ARM_VIRT_MEMINFO_PPI    *MemInfo;
> +  EFI_STATUS              Status;
> +
> +  Status = PeiServicesLocatePpi (&gArmVirtMemInfoPpiGuid, 0, NULL,
> +             (VOID **)&MemInfo);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  MemInfo->GetMemoryMap (VirtualMemoryMap);
> +}
> diff --git a/ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.inf b/ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.inf
> new file mode 100644
> index 000000000000..b661c2f43faf
> --- /dev/null
> +++ b/ArmVirtPkg/Library/PeiVirtMemInfoLib/PeiVirtMemInfoLib.inf
> @@ -0,0 +1,42 @@
> +#/* @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                      = PeiVirtMemInfoLib
> +  FILE_GUID                      = 1d7bae0f-9674-4a4b-8d85-9804968cb12b
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmVirtMemInfoLib|PEIM
> +
> +[Sources]
> +  PeiVirtMemInfoLib.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  DebugLib
> +  PeiServicesLib
> +
> +[Ppis]
> +  gArmVirtMemInfoPpiGuid
> +
> +[Depex]
> +  gArmVirtMemInfoPpiGuid
> diff --git a/ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.c b/ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.c
> new file mode 100644
> index 000000000000..90ee552bdba0
> --- /dev/null
> +++ b/ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.c
> @@ -0,0 +1,121 @@
> +/**@file
> +
> +  Copyright (c) 2017, 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.
> +
> +**/
> +
> +#include <PiPei.h>
> +#include <Library/ArmLib.h>
> +#include <Library/ArmVirtMemInfoLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PeiServicesLib.h>
> +#include <libfdt.h>
> +#include <Ppi/ArmVirtMemInfo.h>
> +
> +STATIC ARM_VIRT_MEMINFO_PPI             mArmVirtMeminfoPpi = {
> +  ArmVirtGetMemoryMap
> +};
> +
> +STATIC CONST EFI_PEI_PPI_DESCRIPTOR     mArmVirtMeminfoPpiTable = {
> +  EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> +  &gArmVirtMemInfoPpiGuid,
> +  &mArmVirtMeminfoPpi
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +QemuVirtMemInfoPeimEntryPoint (
> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> +  IN CONST EFI_PEI_SERVICES     **PeiServices
> +  )
> +{
> +  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__));
> +      }
> +    }
> +  }
> +
> +  //
> +  // 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 PeiServicesInstallPpi (&mArmVirtMeminfoPpiTable);
> +}
> diff --git a/ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf b/ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf
> new file mode 100644
> index 000000000000..ac91e065be57
> --- /dev/null
> +++ b/ArmVirtPkg/QemuVirtMemInfoPeim/QemuVirtMemInfoPeim.inf
> @@ -0,0 +1,60 @@
> +## @file
> +#
> +#  Copyright (c) 2017, 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                    = 0x0001001A
> +  BASE_NAME                      = QemuVirtMemInfoPeim
> +  FILE_GUID                      = 91da13af-d0ff-4810-b9b9-b095a9ee6b09
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = QemuVirtMemInfoPeimEntryPoint
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = ARM AARCH64
> +#
> +
> +[Sources]
> +  QemuVirtMemInfoPeim.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  ArmVirtMemInfoLib
> +  DebugLib
> +  FdtLib
> +  PeimEntryPoint
> +  PeiServicesLib
> +
> +[Pcd]
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +[Ppis]
> +  gArmVirtMemInfoPpiGuid
> +
> +[FixedPcd]
> +  gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdFdBaseAddress
> +  gArmTokenSpaceGuid.PcdFdSize
> +  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
> +
> +[Depex]
> +  TRUE
>