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 CBE41220C1C31 for ; Thu, 23 Nov 2017 08:22:42 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2053C1555A; Thu, 23 Nov 2017 16:27:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-221.rdu2.redhat.com [10.10.120.221]) by smtp.corp.redhat.com (Postfix) with ESMTP id 43F2E5D960; Thu, 23 Nov 2017 16:26:59 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org References: <20171122100731.24525-1-ard.biesheuvel@linaro.org> <20171122100731.24525-13-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <1e355c8e-0bca-7a07-eea1-0cff0b2eb721@redhat.com> Date: Thu, 23 Nov 2017 17:26:58 +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: <20171122100731.24525-13-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 23 Nov 2017 16:27:00 +0000 (UTC) Subject: Re: [PATCH v2 12/14] ArmVirtPkg: create QemuVirtMemInfoLib version for ArmVirtQemu 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: Thu, 23 Nov 2017 16:22:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/22/17 11:07, Ard Biesheuvel wrote: > The QemuVirtMemInfoLib ArmVirtMemInfoLib implementation created for > ArmVirtQemuKernel does exactly what we need for ArmVirtQemu, the only > difference being that the latter is PrePeiCore based, and so it uses > a different method to ensure that PcdSystemMemorySize is set when > ArmVirtGetMemoryMap() is called. > > On ArmVirtQemu, we currently abuse the implied ordering guarantees > provided by ArmPlatformLib, by implementing this as follows: > > 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 > // > > Given that we are trying to get rid of ArmPlatformLib, or at least remove > some of these API functions that are never used for their original purpose > by any platforms, we need to move the PCD assignment elsewhere. > > So create a PEIM-only version of QemuVirtMemInfoLib especially for > ArmVirtQemu, and add the PCD assignment code to its constructor. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirtQemu.dsc | 3 + > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf | 58 +++++++++++ > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c | 106 ++++++++++++++++++++ > 3 files changed, 167 insertions(+) > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc > index d14a0dd0d1d9..519c2ae2e939 100644 > --- a/ArmVirtPkg/ArmVirtQemu.dsc > +++ b/ArmVirtPkg/ArmVirtQemu.dsc > @@ -67,6 +67,9 @@ [LibraryClasses.common] > HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf > !endif > > +[LibraryClasses.common.PEIM] > + ArmVirtMemInfoLib|ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > + > [LibraryClasses.common.UEFI_DRIVER] > UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf > > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > new file mode 100644 > index 000000000000..e574a47443d0 > --- /dev/null > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > @@ -0,0 +1,58 @@ > +#/* @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 = QemuVirtMemInfoLib (1) Please update BASE_NAME before pushing. > + FILE_GUID = 0c4d10cf-d949-49b4-bd13-47a4ae22efce > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ArmVirtMemInfoLib|PEIM > + CONSTRUCTOR = QemuVirtMemInfoPeiLibConstructor > + > +[Sources] > + QemuVirtMemInfoLib.c > + QemuVirtMemInfoPeiLibConstructor.c > + > +[Sources.ARM] > + Arm/PhysAddrTop.S > + > +[Sources.AARCH64] > + AArch64/PhysAddrTop.S > + > +[Packages] > + ArmPkg/ArmPkg.dec > + ArmVirtPkg/ArmVirtPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + ArmLib > + BaseMemoryLib > + DebugLib > + FdtLib > + PcdLib > + MemoryAllocationLib > + > +[Pcd] > + gArmTokenSpaceGuid.PcdFdBaseAddress > + gArmTokenSpaceGuid.PcdSystemMemoryBase > + gArmTokenSpaceGuid.PcdSystemMemorySize > + > +[FixedPcd] > + gArmTokenSpaceGuid.PcdFdSize > + gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress > + gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c > new file mode 100644 > index 000000000000..ef8ac6e018d1 > --- /dev/null > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c > @@ -0,0 +1,106 @@ > +/** @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 > + > +RETURN_STATUS > +EFIAPI > +QemuVirtMemInfoPeiLibConstructor ( > + VOID > + ) > +{ > + 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__)); (2) Thanks for improving the indentation of the DEBUGs. ("--find-copies-harder" remains awesome :) ) Reviewed-by: Laszlo Ersek Thanks! Laszlo > + } > + } > + } > + > + // > + // 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 RETURN_SUCCESS; > +} >