From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BE3A42118F345 for ; Mon, 26 Nov 2018 04:00:08 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id b5so27126745iti.2 for ; Mon, 26 Nov 2018 04:00:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jk0UfwdrM27c3CsnUuZRBIpg+E9n+Gxc8pQKU7/1pQw=; b=VEz4BUWXpxYfMf7SYftAKfb9OVZ4KvS8GP3kOCGxrTTqkdVzpGcYUUsBRjnCH2mAgG tEQ5blfKqENNZbcbnrmVRr2KPQTVQhS1vgak2gzRtgNuQFgf/Cdyz1hP5FvGMSGY/u/L iRKtrnfueJU4fu9BJ3xN6779jDW4rr/7OyGRI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jk0UfwdrM27c3CsnUuZRBIpg+E9n+Gxc8pQKU7/1pQw=; b=i6QH60oAVd90kKi3Fplqyb9Drgl5vhwxeOjYNsYM/5JHtEzbvT9RRSb5mO5ScRMevA uz8+gijAPETAHj5l0XF53mSJsa3eo/4WpWSbhI7uvXrraaLtrTWY63PkTuJ5d5Fu/fWd nA4TuWP8i1TvzlcCKaoeb7GeSHAZH3YyT5331laHHSzo++aLBavljvFfRyyyJnKiO4AL X3NiJ6x0NA9A+RsGfHJiLlhguYM/VXjQNt4guzFWGHzcguCK0/rB7CIZJyrUtZA3IaXR KYaxd0a8wvV0DpysB+wo/JGtsPVvMxu0N5Fh08U3Is8mtLrbkjH0i1OnlkST5E/gKOYd U7kQ== X-Gm-Message-State: AGRZ1gLykmbJntqt6Ba6HI0WbYtOQ8oD6WA8s9u17bL3tVTl5+JjOmR2 ZbfRQVj6vBmIh31lZm7a/JjJB7btOzp6GUarKiFyjQ== X-Google-Smtp-Source: AFSGD/U5M12KAUniI8p2ZhjCAVVu5V5O9kMF4cbdHpyTauzY4egKFR9yDDLLqRzVh5kXPoksNblJX50+1yR8xIDZpWg= X-Received: by 2002:a24:edc4:: with SMTP id r187mr24871541ith.158.1543233607285; Mon, 26 Nov 2018 04:00:07 -0800 (PST) MIME-Version: 1.0 References: <20181123121431.22353-1-ard.biesheuvel@linaro.org> <20181123121431.22353-5-ard.biesheuvel@linaro.org> <2213ac84-52fd-abd2-bc89-1d07443c346b@redhat.com> In-Reply-To: <2213ac84-52fd-abd2-bc89-1d07443c346b@redhat.com> From: Ard Biesheuvel Date: Mon, 26 Nov 2018 12:59:56 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Andrew Jones , Auger Eric 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 12:00:09 -0000 Content-Type: text/plain; charset="UTF-8" On Mon, 26 Nov 2018 at 11:47, Laszlo Ersek wrote: > > 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() This does nothing on AArch64, and even on ARM, this is something that is enabled by the bare metal boot code, not per-guest. > - building gArmMpCoreInfoGuid > That is for the MPCore versions of PrePi and PrePeiCore, and we only use UniCore versions in ArmVirtPkg I will add some clarification to the commit log. > > > > 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. > Now, I'm not so sure anymore whether this hunk is correct. This describes the extent of DRAM to the generic MMU code, and we only want to map what a) the CPU can support and b) what we actually describe as being populated by anything So if your guest supports 52 bits of PA space in hardware, we want the GCD space to accommodate that (so we can report memory as residing there). However, we cannot map all of that memory in UEFI anyway, and so capping the mapping (this hunk) to the PcdPrePiCpuMemorySize size actually makes sense. > 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? > Works for me.