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:c06::229; helo=mail-io0-x229.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x229.google.com (mail-io0-x229.google.com [IPv6:2607:f8b0:4001:c06::229]) (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 3ECD621E0B9E5 for ; Thu, 1 Feb 2018 03:35:34 -0800 (PST) Received: by mail-io0-x229.google.com with SMTP id b198so18846043iof.6 for ; Thu, 01 Feb 2018 03:41:12 -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=BFVq5mfzHnUuZcdEDlwU0riTyOsuq6rAxhvMckT/aOc=; b=FpXk4R0ArK3ABr5CKfHKQIn0x4TwGzQcgJpoE3tAWpoU2yUYEU/jCWClZ5bGw/hmVh +ZZjxu5GaV1iUU38G3WPeBxvLSclIpBxSU/3+XI3TjpmIQ7em3S1trU6O421SesWp35w 8gKXWEB5asfGdzgOrwNwnqGGd42BzGocXwsR4= 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=BFVq5mfzHnUuZcdEDlwU0riTyOsuq6rAxhvMckT/aOc=; b=r9L65xNUAs4GY93seTnnk3X6XEYPoD7S6m8S04RxgQjmXiPLEFOuXzEE3EHvWOHz5F A8hX7hltJQbflAOrk6BmLAaTa+HKlW6MVsBFX0EZhk6vg8GbCrr1FIY9U1YAiS3JU85x t4Gh1jOmCuPNUmFfZlEDQm2LD2EAlofYN1iGKZIXpCI87q4it81LtWV104SvRdcms6pv 27DvupGj7bxpPxRJ1hCdYNvSeISBPBVw+4FNxs6Yq0KeJN9vvFQZs+M4hmZvjge7GaG/ wuieAv3fLqfwsnvGsmaRzC/Y9mF4B0mzjLEZYHobg0omOCytHs9AkAXfvLadOF4vYSne qzxQ== X-Gm-Message-State: AKwxytezecXjDDPrf2p7QnQhF2zFCvY1AKJAp/ayD+kBm5s1F9qMO+rf 14zN4cA/xjabh9vawCMPPPeqvRrbU5ocKpKNceblAw== X-Google-Smtp-Source: AH8x224HfpoxE7dFUVj4sjgA/Ng63ijqCU0JyI9KBdelXuejyv1XUdxwHIubJh5mcmsOgY1vhu+kMpTsbZYyndiUX/s= X-Received: by 10.107.20.194 with SMTP id 185mr36604816iou.127.1517485271075; Thu, 01 Feb 2018 03:41:11 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Thu, 1 Feb 2018 03:41:10 -0800 (PST) In-Reply-To: References: <20180102155034.13688-1-ard.biesheuvel@linaro.org> From: Ard Biesheuvel Date: Thu, 1 Feb 2018 11:41:10 +0000 Message-ID: To: Udit Kumar , "leif.lindholm@linaro.org" Cc: "edk2-devel@lists.01.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: Thu, 01 Feb 2018 11:35:35 -0000 Content-Type: text/plain; charset="UTF-8" Leif, On 3 January 2018 at 07:44, Ard Biesheuvel wrote: > 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 >> Any thoughts? I can resend the patch if you like.