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 1B9102116821B for ; Mon, 26 Nov 2018 02:47:53 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D3821308339A; Mon, 26 Nov 2018 10:47:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-66.rdu2.redhat.com [10.10.121.66]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9D5C2608FA; Mon, 26 Nov 2018 10:47:48 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Andrew Jones , Eric Auger References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> <20181123121431.22353-5-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <2213ac84-52fd-abd2-bc89-1d07443c346b@redhat.com> Date: Mon, 26 Nov 2018 11:47:47 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181123121431.22353-5-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Mon, 26 Nov 2018 10:47:52 +0000 (UTC) Subject: Re: [PATCH 4/5] ArmVirtPkg: disregard PcdPrePiCpuMemorySize PCD when sizing the GCD space X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Nov 2018 10:47:54 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/23/18 13:14, Ard Biesheuvel wrote: > Move the call to BuildCpuHob () into ArmVirtMemoryInitPeiLib (which is > shared between all the ArmVirtPkg targets), and drop the inclusion of > CpuPei.inf [which calls it on ArmVirtQemu] and the BuildCpuHob () call > from ArmVirtPrePiUniCoreRelocatable [for the other targets]. (1) Can you please confirm, in the commit message, that we don't need -- or that we don't exercise anyway -- the other things that "ArmPkg/Drivers/CpuPei/CpuPei.inf" does, namely: - ArmEnableBranchPrediction() - building gArmMpCoreInfoGuid ? > > This makes the size of the GCD address space and page table mappings > depend only on the size of the physical address space as exposed by > the CPU system registers. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirtQemu.dsc | 1 - > ArmVirtPkg/ArmVirtQemu.fdf | 1 - > ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 +++ > ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf | 1 + > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c | 5 +---- > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf | 1 - > ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf | 1 - > ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf | 3 --- > ArmVirtPkg/PrePi/PrePi.c | 3 --- > 9 files changed, 5 insertions(+), 14 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc > index 885c6b14b844..cb59c790afcc 100644 > --- a/ArmVirtPkg/ArmVirtQemu.dsc > +++ b/ArmVirtPkg/ArmVirtQemu.dsc > @@ -226,7 +226,6 @@ > } > ArmPlatformPkg/PlatformPei/PlatformPeim.inf > ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf > - ArmPkg/Drivers/CpuPei/CpuPei.inf > > MdeModulePkg/Universal/Variable/Pei/VariablePei.inf > > diff --git a/ArmVirtPkg/ArmVirtQemu.fdf b/ArmVirtPkg/ArmVirtQemu.fdf > index c6a22dc018f3..12bc570c4cb3 100644 > --- a/ArmVirtPkg/ArmVirtQemu.fdf > +++ b/ArmVirtPkg/ArmVirtQemu.fdf > @@ -106,7 +106,6 @@ READ_LOCK_STATUS = TRUE > INF MdeModulePkg/Core/Pei/PeiMain.inf > INF ArmPlatformPkg/PlatformPei/PlatformPeim.inf > INF ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.inf > - INF ArmPkg/Drivers/CpuPei/CpuPei.inf > INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf > INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf > INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf OK, so this removes the previous CPU HOB building in ArmVirtQemu. Quoting out of order: > diff --git a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf > index 1587bd92f206..e04bd1b7c497 100755 > --- a/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf > +++ b/ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf > @@ -85,9 +85,6 @@ > > gArmPlatformTokenSpaceGuid.PcdCoreCount > > - gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize > - gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize > - > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType > diff --git a/ArmVirtPkg/PrePi/PrePi.c b/ArmVirtPkg/PrePi/PrePi.c > index f6abe2f2016b..ecaaac1545c4 100755 > --- a/ArmVirtPkg/PrePi/PrePi.c > +++ b/ArmVirtPkg/PrePi/PrePi.c > @@ -79,9 +79,6 @@ PrePiMain ( > StacksSize = PcdGet32 (PcdCPUCorePrimaryStackSize); > BuildStackHob (StacksBase, StacksSize); > > - //TODO: Call CpuPei as a library > - BuildCpuHob (PcdGet8 (PcdPrePiCpuMemorySize), PcdGet8 (PcdPrePiCpuIoSize)); > - Best way to solve a TODO is to delete it. :) > // Set the Boot Mode > SetBootMode (BOOT_WITH_FULL_CONFIGURATION); > > OK, this removes the previous CPU HOB building in ArmVirtQemuKernel and ArmVirtXen. > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c > index 3f0e9b3a0579..3d86d31ab50e 100644 > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c > @@ -116,5 +116,8 @@ MemoryPeim ( > BuildMemoryTypeInformationHob (); > } > > + // Publish the CPU memory and io spaces sizes > + BuildCpuHob (ArmGetPhysicalAddressBits (), FixedPcdGet8 (PcdPrePiCpuIoSize)); > + > return EFI_SUCCESS; > } Argh, it took me a while to see that this will indeed supplement the CPU HOB on all ArmVirtPlatforms. The build report file(s) helped. I didn't think of ArmPlatformPkg originally. :) So, yes, this is fine; it's OK to produce the CPU HOB anywhere in the PEI phase. > diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf > index 54879d590a8a..d0e39df84b20 100644 > --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf > +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.inf > @@ -59,6 +59,7 @@ > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiBootServicesData > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderCode > gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiLoaderData > + gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize > > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemoryBase OK, expected. > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > index 4eca9b895354..ded87d604f4f 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c > @@ -46,7 +46,6 @@ ArmVirtGetMemoryMap ( > ) > { > ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable; > - UINT64 TopOfMemory; > > ASSERT (VirtualMemoryMap != NULL); > > @@ -80,11 +79,9 @@ ArmVirtGetMemoryMap ( > VirtualMemoryTable[1].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > // Peripheral space after DRAM > - TopOfMemory = MIN (1ULL << FixedPcdGet8 (PcdPrePiCpuMemorySize), > - TopOfAddressSpace); > VirtualMemoryTable[2].PhysicalBase = VirtualMemoryTable[0].Length + VirtualMemoryTable[1].Length; > VirtualMemoryTable[2].VirtualBase = VirtualMemoryTable[2].PhysicalBase; > - VirtualMemoryTable[2].Length = TopOfMemory - > + VirtualMemoryTable[2].Length = TopOfAddressSpace - > VirtualMemoryTable[2].PhysicalBase; > VirtualMemoryTable[2].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > index f2c461e3b55a..5c5b841051ad 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.inf > @@ -45,4 +45,3 @@ > > [FixedPcd] > gArmTokenSpaceGuid.PcdFdSize > - gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize > diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > index f54fb51ee1d4..d12089760b22 100644 > --- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > +++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf > @@ -49,4 +49,3 @@ > [FixedPcd] > gArmTokenSpaceGuid.PcdFdSize > gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress > - gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize This last part, for QemuVirtMemInfoLib, is functionally valid, in my opinion. We are eliminating the clamping to PcdPrePiCpuMemorySize, which was one of my earlier points, so what remains is "TopOfMemory = TopOfAddressSpace"; good. However: this change does not relate to the GCD memory space. The ArmVirtGetMemoryMap() function partakes in the MMU configuration / page table setup. Its leading comment says, This Virtual Memory Map is used by MemoryInitPei Module to initialize the MMU on your platform. (2) Therefore, it should be split to a separate patch. Earlier, I suggested a slight restructuring for the patch series, so let me refine that: - new patch #1: current patch #1 (introduce helper) - new patch #2: current patch #3 (refactor ArmVirtPkg) - new patch #3: current patch #2 (consider CPU capabilities in generic MMU setup code) - new patch #4: this specific hunk, from current patch #4 (consider CPU capabilities in specific MMU setup code) - new patch #5: the rest of current patch #4 (consider CPU caps in GCD / CPU HOB) - new patch #6: kill PcdPrePiCpuMemorySize in ArmVirtPkg DSC files Does this sound acceptable? Thanks! Laszlo