From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::235; helo=mail-it0-x235.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x235.google.com (mail-it0-x235.google.com [IPv6:2607:f8b0:4001:c0b::235]) (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 345A72034B421 for ; Tue, 2 Jan 2018 23:39:50 -0800 (PST) Received: by mail-it0-x235.google.com with SMTP id z6so833877iti.4 for ; Tue, 02 Jan 2018 23:44:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=J8iryPD8A/iEU6c05M8knyyk0dDOaW6JfykZ/XCCCik=; b=OcuD2EfNi0UINYYcI5hrnQCA4Nhz7sxfTMYuDuqduxa5G3xwkKxp1O3yh+EaEx7kL8 Rjl0fc+91zSDHgHMMUPWldYTPtgNvvfxzRY7CiNoYY2EclUj0LnIv+Pex/ep8CUhgQ70 mRE11h3J2mGVgNREgh9fjh8MleO5+ygVJU4Gc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=J8iryPD8A/iEU6c05M8knyyk0dDOaW6JfykZ/XCCCik=; b=RWdUpREJG/YCFKeAy0rC6jNGEpF+WfiAge6AepFpPcfjz6vIQLrwAul+NdyD7Hre8P tkDqd31W+dsoN6+vSS3OLsosnNS4I1WhWTA6YDElLX2LdHzkmDORUmQk/sHDOpb4P++v OPGdX0yBuBzYLdDtJfioJfkvEvquNJ5hJjIYifdcZ037YAl9YIAblhsQ6c2ftuNoYklV slQmlZedV6v/2zqjDzbkPxQ7FRyO0wW1BRyrdy3lRJZNe0Evnb6tMBdjzpolAHGs6Ebk LbbvsakewN2KzV9+VMZp9J9IE32SXsVetIa9I2GJpSM5uo8OcL7TJOsTrxJj6RUUYvGf VhnA== X-Gm-Message-State: AKGB3mIj4eB9SIjOjX8plriRwl2xy4W3hKHFd2apaW5ct7U13heq3FAK c7KzkDahzgv7PwmMXo6cJVUmRw3etMbJEL5O3HM59Q== X-Google-Smtp-Source: ACJfBotIlHsYvdREGMYumm4VGtdoKziQaDBFWyY+yLRTPYDo8ie+H/R4j9SsXNpKtgh97JEVCFG5U4wX2EZaiI5li5Q= X-Received: by 10.36.71.83 with SMTP id t80mr813939itb.48.1514965493214; Tue, 02 Jan 2018 23:44:53 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.37.197 with HTTP; Tue, 2 Jan 2018 23:44:52 -0800 (PST) In-Reply-To: References: <20180102155034.13688-1-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Wed, 3 Jan 2018 07:44:52 +0000 Message-ID: To: Udit Kumar Cc: "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org" , "vladimir.olovyannikov@broadcom.com" , Meenakshi Aggarwal 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, 03 Jan 2018 07:39:51 -0000 Content-Type: text/plain; charset="UTF-8" On 3 January 2018 at 05:09, Udit Kumar wrote: > Thanks Ard, > This works for us as well > Few comments inline > > > Regards > Udit > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: Tuesday, January 02, 2018 9:21 PM >> To: edk2-devel@lists.01.org >> Cc: leif.lindholm@linaro.org; vladimir.olovyannikov@broadcom.com; Udit >> Kumar ; Meenakshi Aggarwal >> ; Ard Biesheuvel >> >> Subject: [PATCH] ArmPlatformPkg/MemoryInitPeiLib: mark primary FV region >> as boot services data >> >> 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 >> --- >> 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 > > Please correct comments, now this memory is present > Yes >> + 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)); > > Hob List is already created for PcdSystemMemoryBase and its size > Within this, we got Fd, then do we want to create another Hob here > The resource descriptor for PcdSystemMemoryBase/Size is updated in the next line so it no longer covers the FD >> + >> + // 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); > > IMO, only this call should be enough to protect FD area. > I agree, but the reality is that it is not enough. in CoreInitializeGcdServices, the first system memory resource descriptor is claimed before the memory allocations are processed, and any memory allocation HOBs that intersect that region are ignored. >> + >> + Found = TRUE; >> + break; >> + } >> + NextHob.Raw = GET_NEXT_HOB (NextHob); >> + } >> + >> + ASSERT(Found); >> + } >> + >> // Build Memory Allocation Hob >> InitMmu (MemoryTable); >> >> -- >> 2.11.0 >