From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2056A2034C5EE for ; Tue, 21 Nov 2017 09:44:57 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B3D207F7DA; Tue, 21 Nov 2017 17:49:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-145.rdu2.redhat.com [10.10.120.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id 89E4860BE0; Tue, 21 Nov 2017 17:49:11 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: leif.lindholm@linaro.org References: <20171117160913.17292-1-ard.biesheuvel@linaro.org> <20171117160913.17292-14-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <0795247f-2296-e486-50f5-0ba4caaa150b@redhat.com> Date: Tue, 21 Nov 2017 18:49:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171117160913.17292-14-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 21 Nov 2017 17:49:12 +0000 (UTC) Subject: Re: [PATCH 13/15] ArmVirtPkg: implement ArmVirtMemInfo PPI, PEIM and library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Nov 2017 17:44:58 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > 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 > + > +#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 > +#include > +#include > +#include > +#include > + > +/** > + 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.
> + > + 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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.
> +# > +# 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 >