| Message ID | 20171117160913.17292-14-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ArmVirtPkg: get rid of ArmPlatformLib | expand |
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
> 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 >> >
On 11/21/17 18:57, Ard Biesheuvel wrote: > > >> 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. Right, I suspected that this experience was in the background. If I remember correctly, the issue was when some libraries had constructors while some others had none (and required explicit init function calls instead). In some cases this mixing was not possible to avoid due to circular dependencies between constructors, but in turn the explicit calls didn't get ordered right, or some such. *shudder* :) > >>> 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? Hm, I think I missed the impact on ArmVirtQemuKernel. In the current set, its DSC file is only modified in patch 12. (I missed that too.) Are any changes necessary for ArmVirtQemuKernel that are not contained in this set? Either way, what you propose above seems to be the standard approach to me: use two INF files, keep the bulk of the code in one (shared) C file, and add the constructor to another (non-shared) C file (i.e., referenced by only one of the INF files). Should the constructor need shared utility functions from the shared C file, add an internal header for those. Thanks! Laszlo
On 21 November 2017 at 20:17, Laszlo Ersek <lersek@redhat.com> wrote: > On 11/21/17 18:57, Ard Biesheuvel wrote: >> >> >>> 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. > > Right, I suspected that this experience was in the background. If I > remember correctly, the issue was when some libraries had constructors > while some others had none (and required explicit init function calls > instead). In some cases this mixing was not possible to avoid due to > circular dependencies between constructors, but in turn the explicit > calls didn't get ordered right, or some such. *shudder* :) > The core of the issue is that transitive library dependencies are not honoured in the ordering of constructor invocations if they pass through a library that has no constructor itself. I.e., libA with a constructor depending on libB with no constructor depending on libC with a constructor Currently, libA's constructor may be called before libC's constructor, which is clearly a bug, and which is the reason why I /think/ we generally shouldn't be relying on other libraries in constructor implementations. >> >>>> 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? > > Hm, I think I missed the impact on ArmVirtQemuKernel. In the current > set, its DSC file is only modified in patch 12. (I missed that too.) Are > any changes necessary for ArmVirtQemuKernel that are not contained in > this set? > > Either way, what you propose above seems to be the standard approach to > me: use two INF files, keep the bulk of the code in one (shared) C file, > and add the constructor to another (non-shared) C file (i.e., referenced > by only one of the INF files). Should the constructor need shared > utility functions from the shared C file, add an internal header for those. > Would you object to having a single .c file, but only declare the constructor in one of the two .INFs? The code will be pruned anyway, due to our use of -ffunction-sections
On 11/21/17 21:32, Ard Biesheuvel wrote: > On 21 November 2017 at 20:17, Laszlo Ersek <lersek@redhat.com> > wrote: >> On 11/21/17 18:57, Ard Biesheuvel wrote: >>> 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? >> >> Hm, I think I missed the impact on ArmVirtQemuKernel. In the >> current set, its DSC file is only modified in patch 12. (I missed >> that too.) Are any changes necessary for ArmVirtQemuKernel that are >> not contained in this set? >> >> Either way, what you propose above seems to be the standard >> approach to me: use two INF files, keep the bulk of the code in one >> (shared) C file, and add the constructor to another (non-shared) C >> file (i.e., referenced by only one of the INF files). Should the >> constructor need shared utility functions from the shared C file, >> add an internal header for those. >> > > Would you object to having a single .c file, but only declare the > constructor in one of the two .INFs? The code will be pruned anyway, > due to our use of -ffunction-sections > I would frown, but not object. :) Please add a comment above the constructor function about the non-shared use. Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
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. 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() 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. 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(+) -- 2.11.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel