From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x22a.google.com (mail-lf0-x22a.google.com [IPv6:2a00:1450:4010:c07::22a]) (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 1287F803E3 for ; Tue, 14 Mar 2017 10:13:43 -0700 (PDT) Received: by mail-lf0-x22a.google.com with SMTP id a6so79304789lfa.0 for ; Tue, 14 Mar 2017 10:13:42 -0700 (PDT) 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=3kWJyw2rtQMxEAma9I1KeyF1pu4yn1NdyQaqOf1cdZc=; b=KXgC9XZWSEgu4Guatc7/I+ryQTwVG3/HziAm9f8+inKXDqydVlT4siHQP7UuemP+xK bxHv3Yhbj/55XJlVPhI1T5jfhXxZ7pEk7uzyUTlNpAIHsWC8xBY4IZhtplTYhE6Tn608 XBerszwlcEnn8QCsX6ClZRx7SPzVNiE16q0Oc= 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=3kWJyw2rtQMxEAma9I1KeyF1pu4yn1NdyQaqOf1cdZc=; b=Zw6dOO0NWCERAdUDIC1ozOvd+41yTvUnXHCd6I2WNSJL5w1BFxzEOb7h3i+MXY0TDD BF9WbLzfxWgXhvONmNhk3ytsR5IHWTpJqAMX4RAYhKf2JAhiCX8c7A0lT2QKmvxgTNis Tt62AeecIqPV9EUq3UDC3b6LbxwSC4A0pSokuJDQc/OB8ObGURpTfQCRsLQkByopL/jv w2s1Qxjd4HJ7tY+ZvY+n69crLwrBCnYRI+tSUZ6W1FT8PqkdqNxP3/1GN6A2eiwC0zHm 8ZbXUNhW21Lj2psdaeag+lBbjQdula86EtlS5W1PPbsA++NAjbow1wRSMKzWW0JulFuT kKZw== X-Gm-Message-State: AMke39nDjgA9wsbMithv9EK7j5A8rxaurvBsZ6yXw/a8Bc6qzK3rhh8JTxqszt9VfEXqaAXsEn/sfsRy+YzO2RAL X-Received: by 10.25.196.207 with SMTP id u198mr11116942lff.88.1489511620993; Tue, 14 Mar 2017 10:13:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.158.145 with HTTP; Tue, 14 Mar 2017 10:13:40 -0700 (PDT) In-Reply-To: <1489478464-5737-1-git-send-email-ard.biesheuvel@linaro.org> References: <1489478464-5737-1-git-send-email-ard.biesheuvel@linaro.org> From: Ryan Harkin Date: Tue, 14 Mar 2017 17:13:40 +0000 Message-ID: To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Michael Zimmermann , Laszlo Ersek Subject: Re: [PATCH v2] EmbeddedPkg/PrePiLib: allocate code pages for DxeCore X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Mar 2017 17:13:43 -0000 Content-Type: text/plain; charset=UTF-8 Hi Ard, On 14 March 2017 at 08:01, Ard Biesheuvel wrote: > The recently introduced memory protection features inadvertently broke > the boot on all PrePi platforms, because the changes to explicitly use > EfiBootServicesCode for loading the DxeCore PE/COFF image need to be > applied in a different way for PrePi. So add a simple helper function > that sets the type of an allocation to EfiBootServicesCode, and invoke > it to allocate the space for DxeCore. > I'm not quite sure which issue this patch is supposed to fix. EDK2 is broken on Juno for me right now. I see this output at boot: EhcDriverBindingStart: failed to enable 64-bit DMA on 64-bit capable controller @ FD41B898 (Unsupported) ASSERT_EFI_ERROR (Status = Unsupported) ASSERT [ArmJunoDxe] /linaro/platforms/uefi/edk2/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c(361): !EFI_ERROR (Status) Bisecting tells me this patch is to blame: $ git bisect bad e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 is the first bad commit commit e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 Author: Ard Biesheuvel Date: Tue Feb 28 12:13:12 2017 +0000 ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable The primary use case for UncachedMemoryAllocationLib is non-coherent DMA, which implies that such regions are not used to fetch instructions from. So let's map them as non-executable, to avoid creating a security hole when the rest of the platform may be enforcing strict memory permissions on ordinary allocations. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel Reviewed-by: Leif Lindholm :040000 040000 6310e6a81c3b1e9eda315a9f4cad2bc918f74b25 12b11759ebd83f91a127c1f016b8e41cbfd097d4 M ArmPkg Applying the patch from $subject and "[PATCH] Platforms/ARM: enable memory protection features" does not resolve the issue. AFAICT, only reverting e7b24ec9785d206f1d3faf8f646e63a1b540d6a5 makes things work again. I suspect there are further Juno specific changes that could be applied instead of reverting the problematic patch. So, Juno and TC2 boot fine if my edk2 tree looks like this: 50e5735 2017-03-14 Revert "ArmPkg/UncachedMemoryAllocationLib: map uncached allocations non-executable" [Ryan Harkin] 7e9c1b0 2017-03-14 EmbeddedPkg/PrePiLib: allocate code pages for DxeCore [Ard Biesheuvel] c03f5b2 2017-03-10 BaseTools/UPT: Fix an issue in subst command [Hess Chen] And OpenPlatformPkg looks like this: d6a743d 2017-03-07 Platforms/ARM: enable memory protection features [Ard Biesheuvel] 408d600 2016-01-29 HACK: Platforms/ARM: TC2: set gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride TRUE [Ryan Harkin] 17dbc2a 2017-02-24 Treewide: remove references to VirtualUncachedPages libraries and PCDs [Ard Biesheuvel] If I go on to drop $subject patch, Juno and TC2 no longer boot, so I presume that it's supposed to fix the issue where OpenPlatformPkg has enabled the memory protection features. And that the upstream Juno problems are different. Cheers, Ryan. > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > Tested-by: Michael Zimmermann > --- > v2: add missing MemoryAllocationLib.h include > add Michael's T/b > > EmbeddedPkg/Library/PrePiLib/PrePi.h | 1 + > EmbeddedPkg/Library/PrePiLib/PrePiLib.c | 34 +++++++++++++++++++- > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePi.h b/EmbeddedPkg/Library/PrePiLib/PrePi.h > index 607561cd2496..84eca23ec8a6 100644 > --- a/EmbeddedPkg/Library/PrePiLib/PrePi.h > +++ b/EmbeddedPkg/Library/PrePiLib/PrePi.h > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > #include > > diff --git a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c > index 9a1ef344df6e..bba8e7384edc 100644 > --- a/EmbeddedPkg/Library/PrePiLib/PrePiLib.c > +++ b/EmbeddedPkg/Library/PrePiLib/PrePiLib.c > @@ -28,6 +28,38 @@ SecWinNtPeiLoadFile ( > IN EFI_PHYSICAL_ADDRESS *EntryPoint > ); > > +STATIC > +VOID* > +EFIAPI > +AllocateCodePages ( > + IN UINTN Pages > + ) > +{ > + VOID *Alloc; > + EFI_PEI_HOB_POINTERS Hob; > + > + Alloc = AllocatePages (Pages); > + if (Alloc == NULL) { > + return NULL; > + } > + > + // find the HOB we just created, and change the type to EfiBootServicesCode > + Hob.Raw = GetFirstHob (EFI_HOB_TYPE_MEMORY_ALLOCATION); > + while (Hob.Raw != NULL) { > + if (Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress == (UINTN)Alloc) { > + Hob.MemoryAllocation->AllocDescriptor.MemoryType = EfiBootServicesCode; > + return Alloc; > + } > + Hob.Raw = GET_NEXT_HOB (Hob); > + Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, Hob.Raw); > + } > + > + ASSERT (FALSE); > + > + FreePages (Alloc, Pages); > + return NULL; > +} > + > > EFI_STATUS > EFIAPI > @@ -54,7 +86,7 @@ LoadPeCoffImage ( > // > // Allocate Memory for the image > // > - Buffer = AllocatePages (EFI_SIZE_TO_PAGES((UINT32)ImageContext.ImageSize)); > + Buffer = AllocateCodePages (EFI_SIZE_TO_PAGES((UINT32)ImageContext.ImageSize)); > ASSERT (Buffer != 0); > > > -- > 2.7.4 >