From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22c.google.com (mail-it0-x22c.google.com [IPv6:2607:f8b0:4001:c0b::22c]) (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 B2DF7803DA for ; Tue, 14 Mar 2017 13:29:43 -0700 (PDT) Received: by mail-it0-x22c.google.com with SMTP id g138so7294914itb.0 for ; Tue, 14 Mar 2017 13:29:43 -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=OE+3l537tOrxtbNuOfUBdq/EA1tHR/LtHTfHui2Ns6w=; b=ArOWap4GDlTFZNSPDjQVAsXhMdVyKoO6sSs090yXTdSjvMDtD3xSRfpHaulDQ93Foj 8o3VSbYqlCsspRbhOomycRrWfMcTEJlSTfka6cypVLbQDPoyAIB477OaBu+Ko6AH4ceP kcP8Qw2Mkl8J9JbGZqv/ZBfddFS8amcmR58f8= 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=OE+3l537tOrxtbNuOfUBdq/EA1tHR/LtHTfHui2Ns6w=; b=uHWuHNK+kqo5gZCTC4xHFHuqBe8rSL1ris9fYTwL6XbHB1glRIvnqUdPLtVnahMO99 jGTlE588Q2HroYJ8FnqNIHzWYE3IPikGWpf5urH4qq2Epm2UBhaRyXfxou4bo5/MQ6Pl ycT/kMblIpIU810zX+PehsZSaKjIb9d04ocsYG+lc5kvvHRlJC7S7R3JOZPW4MDa5YX7 9h7Gr+SOFX/HiCVctH7ZpqZlOpISgskKJer+q6N1Mvz+DIJ05i/zTOGMDQ7ifhZqnEVs Gn1/Y27fWHGmxl1FbP5TmzUyQx5/S5kHRAAfPZM2yjTXPe3z1i5SpldAEZXP/k/tkuhH Bhtg== X-Gm-Message-State: AFeK/H3JFNm9OkmlNv+Ie9Nmo96Xmp7tTfke1ssPsODIdgOpsp/1fWTj9Q1BikEDiE9gHeNq2p31225uggWpKk86 X-Received: by 10.36.77.10 with SMTP id l10mr1825672itb.59.1489523383039; Tue, 14 Mar 2017 13:29:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Tue, 14 Mar 2017 13:29:42 -0700 (PDT) In-Reply-To: References: <1489478464-5737-1-git-send-email-ard.biesheuvel@linaro.org> <20170314090057.GP16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Tue, 14 Mar 2017 20:29:42 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Ryan Harkin , 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 20:29:43 -0000 Content-Type: text/plain; charset=UTF-8 On 14 March 2017 at 09:03, Ard Biesheuvel wrote: > On 14 March 2017 at 09:00, Leif Lindholm wrote: >> On Tue, Mar 14, 2017 at 08:01:04AM +0000, 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. >>> >>> 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 >> >> Mandatory "don't need to fix sorting, but at least don't make it >> worse" comment. Can be folded in. >> > > Yeah, I pondered this for a while an gave up. I can just fix it > entirely if you prefer. > >>> >>> #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); >> >> I've seen this written elsewhere as >> Hob.Raw = GetNextHob (EFI_HOB_TYPE_XXX, GET_NEXT_HOB (Hob)); >> >> Which looks like a nicer pattern to me. (And means I only need to read >> "hob" 5 times instead of 7.) >> >> Fold in if you agree. >> > > OK. I copied this from PrePiMemoryAllocationLib, but I agree that your > suggestion is better. > >>> + } >>> + >>> + 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 >> >> Reviewed-by: Leif Lindholm >> Pushed with the suggested changes Thanks,