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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 21 Nov 2017, at 17:49, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> 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.
> 

Yes

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

I guess you’re right. Direct dependencies between libraries with constructors are handled correctly by the tools, I simply managed to confuse myself due to the issues with transitive dependencies which you surely remember.

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

I think you’re right.

So how do you propose i go about creating two versions of QemuVirtMemInfoLib, only one of which has a constructor? Share the .c file between two infs in the same directory?


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