From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::244; helo=mail-wr0-x244.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x244.google.com (mail-wr0-x244.google.com [IPv6:2a00:1450:400c:c0c::244]) (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 8090C2034D8F7 for ; Wed, 28 Feb 2018 06:57:08 -0800 (PST) Received: by mail-wr0-x244.google.com with SMTP id p104so2735736wrc.12 for ; Wed, 28 Feb 2018 07:03:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Tfb61h6BMC4jJ4XAOl+Phw5LocG1lVirOQgDjtM9Ir4=; b=TSNhdKsfvQnY7ukqU1I+nJwHQ+33ljy8eMOhpiu3/iBkD/Yh5J1sP+H71/ILFc74sb N+7C6S4+nirZ0ZwOdgIlJo0XTsGnnX3Hcx57IG1/7m/HFIe0lg6cXqJxmJSHIs16gpaO vAvHc1xZS6ybJon9ymdFCp51HGC1nD2eNyMdo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Tfb61h6BMC4jJ4XAOl+Phw5LocG1lVirOQgDjtM9Ir4=; b=adZ+dSS55PdISVsyBXSXc4aQ2ZwRzR3uGKC4AXZvNz5BM8eINcAUusBiHFmIQx0gtR Ui8V90vAozH5HpcJjUj8bWmCvPHEGMvEmh2/NiaSVM+QMqT2vPnBqlJFG5pD8PJYFFID rySAcypfhl1AWGAbPgQfUC2vfpbDvc9Ms2pLdPDoqDU3UDQ3pZCBOzgKGEY46FSArrEr B0qKtYP4gj2wjP3O9SLmDf+AkcExnTeSBflwq+tQcjYRGgSeWW3OgMZ0XAQ80ODkbaq6 6almrmHL60+JyLN0c+2NwJqV0JQtVHIZ62KD7XbCftJD+6Dcvrqwt7d+k6tKs1X95I4x ouFg== X-Gm-Message-State: APf1xPC8bf6Nd3s0pocWXPBDntLDLv2p57zwQ2nYcryLsOfdvvh7zxSj n5fgglbNYUmxkJLy/6pI6+h7+A== X-Google-Smtp-Source: AH8x225ELSKus2snJbAv7Ut6ViF4KhKMa48jNLCSRrGDEBZGSZzJa1siWa10QIIVhAAwbK2IxJPLEQ== X-Received: by 10.223.163.16 with SMTP id c16mr15913047wrb.18.1519830193985; Wed, 28 Feb 2018 07:03:13 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id t135sm2093126wmt.44.2018.02.28.07.03.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 28 Feb 2018 07:03:12 -0800 (PST) Date: Wed, 28 Feb 2018 15:03:11 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, vladimir.olovyannikov@broadcom.com, udit.kumar@nxp.com, meenakshi.aggarwal@nxp.com Message-ID: <20180228150311.h362ivdrpujqfh6w@bivouac.eciton.net> References: <20180102155034.13688-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20180102155034.13688-1-ard.biesheuvel@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region as boot services data X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Feb 2018 14:57:09 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jan 02, 2018 at 03:50:34PM +0000, Ard Biesheuvel wrote: > Commit 8ae5fc182941 ("ArmPlatformPkg/MemoryInitPeiLib: don't reserve > primary FV in memory") deleted the code that removes the memory covering > the primary firmware volume from the memory map. The assumption was that > this is no longer necessary now that we no longer expose compression and > PE/COFF loader library code from the PrePi module to DXE core. > > However, the FV is still declared, and so code may attempt to access it > anyway, which may cause unexpected results depending on whether the > memory has been reused for other purposes in the mean time. > > So reinstate the code that splits off the resource descriptor HOB that > describes the firmware device, but this time, don't mark the memory as > unusable, but create a memory allocation HOB that marks the region as > boot services data. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel So, this looks like a necessary fix. However, there are many line-length violations below. And it's not apparent how fixing that would make the code more readable. So rather than asking for a rewrite - could you split the revert from the modification? / Leif > Vladimir, Udit, Meenakshi: please confirm whether this code works for you. > > ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 74 ++++++++++++++++++++ > 1 file changed, 74 insertions(+) > > diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > index d03214b5df66..d1b5c0be9497 100644 > --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > @@ -70,7 +70,11 @@ MemoryPeim ( > { > ARM_MEMORY_REGION_DESCRIPTOR *MemoryTable; > EFI_RESOURCE_ATTRIBUTE_TYPE ResourceAttributes; > + UINT64 ResourceLength; > EFI_PEI_HOB_POINTERS NextHob; > + EFI_PHYSICAL_ADDRESS FdTop; > + EFI_PHYSICAL_ADDRESS SystemMemoryTop; > + EFI_PHYSICAL_ADDRESS ResourceTop; > BOOLEAN Found; > > // Get Virtual Memory Map from the Platform Library > @@ -117,6 +121,76 @@ MemoryPeim ( > ); > } > > + // > + // Reserve the memory space occupied by the firmware volume > + // > + > + SystemMemoryTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemoryBase) + (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemorySize); > + FdTop = (EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdFdBaseAddress) + (EFI_PHYSICAL_ADDRESS)PcdGet32 (PcdFdSize); > + > + // EDK2 does not have the concept of boot firmware copied into DRAM. To avoid the DXE > + // core to overwrite this area we must create a memory allocation HOB for the region, > + // but this only works if we split off the underlying resource descriptor as well. > + if ((PcdGet64 (PcdFdBaseAddress) >= PcdGet64 (PcdSystemMemoryBase)) && (FdTop <= SystemMemoryTop)) { > + Found = FALSE; > + > + // Search for System Memory Hob that contains the firmware > + NextHob.Raw = GetHobList (); > + while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, NextHob.Raw)) != NULL) { > + if ((NextHob.ResourceDescriptor->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) && > + (PcdGet64 (PcdFdBaseAddress) >= NextHob.ResourceDescriptor->PhysicalStart) && > + (FdTop <= NextHob.ResourceDescriptor->PhysicalStart + NextHob.ResourceDescriptor->ResourceLength)) > + { > + ResourceAttributes = NextHob.ResourceDescriptor->ResourceAttribute; > + ResourceLength = NextHob.ResourceDescriptor->ResourceLength; > + ResourceTop = NextHob.ResourceDescriptor->PhysicalStart + ResourceLength; > + > + if (PcdGet64 (PcdFdBaseAddress) == NextHob.ResourceDescriptor->PhysicalStart) { > + if (SystemMemoryTop != FdTop) { > + // Create the System Memory HOB for the firmware with the non-present attribute > + BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY, > + ResourceAttributes, > + PcdGet64 (PcdFdBaseAddress), > + PcdGet32 (PcdFdSize)); > + > + // Top of the FD is system memory available for UEFI > + NextHob.ResourceDescriptor->PhysicalStart += PcdGet32(PcdFdSize); > + NextHob.ResourceDescriptor->ResourceLength -= PcdGet32(PcdFdSize); > + } > + } else { > + // Create the System Memory HOB for the firmware > + BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY, > + ResourceAttributes, > + PcdGet64 (PcdFdBaseAddress), > + PcdGet32 (PcdFdSize)); > + > + // Update the HOB > + NextHob.ResourceDescriptor->ResourceLength = PcdGet64 (PcdFdBaseAddress) - NextHob.ResourceDescriptor->PhysicalStart; > + > + // If there is some memory available on the top of the FD then create a HOB > + if (FdTop < NextHob.ResourceDescriptor->PhysicalStart + ResourceLength) { > + // Create the System Memory HOB for the remaining region (top of the FD) > + BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY, > + ResourceAttributes, > + FdTop, > + ResourceTop - FdTop); > + } > + } > + > + // Mark the memory covering the Firmware Device as boot services data > + BuildMemoryAllocationHob (PcdGet64 (PcdFdBaseAddress), > + PcdGet32 (PcdFdSize), > + EfiBootServicesData); > + > + Found = TRUE; > + break; > + } > + NextHob.Raw = GET_NEXT_HOB (NextHob); > + } > + > + ASSERT(Found); > + } > + > // Build Memory Allocation Hob > InitMmu (MemoryTable); > > -- > 2.11.0 >