> On 21 Nov 2017, at 17:49, Laszlo Ersek <ler...@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.biesheu...@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
>>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
