From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x230.google.com (mail-lf0-x230.google.com [IPv6:2a00:1450:4010:c07::230]) (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 9629F803A3 for ; Wed, 15 Mar 2017 02:59:15 -0700 (PDT) Received: by mail-lf0-x230.google.com with SMTP id j90so4628966lfk.2 for ; Wed, 15 Mar 2017 02:59:15 -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=0gQbEMIAnjT5BL7zvdoAB0IpR8e5qz1h96bbI9jrPxI=; b=ClfH9NX9A+WMpQglTxcm86F3vYDxkc/W1y3+/vZcafoODMEr5+Sj0dLVMF0avHacOh BHY62/OGuXYnAZZITfcfsxBQRmcOnCmRFhunQ9aY+gGhKZmmmVK7FShlvvuSpNvwIRU8 LNOBUD3mQdFIjsAf4krJsRmuSxHvKAXOBIi3I= 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=0gQbEMIAnjT5BL7zvdoAB0IpR8e5qz1h96bbI9jrPxI=; b=n7kac0fysIz+MxF0U2WBtJvgWoDNDe5Oc5D2+2+5BqkN6GTMRsHclTEkOSlUpaEQGQ Bnpoi0v9SKuC+1OYNBiHp5pBK+HYFkT/VyecraiHDtdc1Fa93wi/4Hpummrt+3Bk9V5d HFj3JJ/V2Ur3jXfeeyl85xpLxJgQKjRaPVXKBm+v8NXTJ4flgVvF2ezewidf5uNEybrT R+a6WfUr8T0G6K239kRVaKKtNZ6WKKj9cVHeL40dG1LYwgp7wsPPyryXMy5jo443+K00 42UdKKJn0Uu2iufF6BBbkRu4Jmaywn+XYn7VbQ1ybfkE8Hb45IDIbUgKxbPJz5nJC/Ul PqTQ== X-Gm-Message-State: AFeK/H3ANvW5M/eSW5V3fSYcY8qIV05+e1fAYzcxObbLA9KCH7ut2aNBRm3vggqnRKYNeCOwPPo9u8piym9rIpkQ X-Received: by 10.46.20.21 with SMTP id u21mr822447ljd.66.1489571952845; Wed, 15 Mar 2017 02:59:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.158.145 with HTTP; Wed, 15 Mar 2017 02:59:12 -0700 (PDT) In-Reply-To: <20170314202028.GX16034@bivouac.eciton.net> References: <1489521495-2961-1-git-send-email-ard.biesheuvel@linaro.org> <20170314202028.GX16034@bivouac.eciton.net> From: Ryan Harkin Date: Wed, 15 Mar 2017 09:59:12 +0000 Message-ID: To: Leif Lindholm Cc: Ard Biesheuvel , "edk2-devel@lists.01.org" Subject: Re: [PATCH] ArmPkg/UncachedMemoryAllocationLib: set XP bit via CPU arch protocol 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: Wed, 15 Mar 2017 09:59:16 -0000 Content-Type: text/plain; charset=UTF-8 Hi Ard, Thanks for the quick turnaround as always. On 14 March 2017 at 20:20, Leif Lindholm wrote: > On Tue, Mar 14, 2017 at 07:58:15PM +0000, Ard Biesheuvel wrote: >> Commit e7b24ec9785d ("ArmPkg/UncachedMemoryAllocationLib: map uncached >> allocations non-executable") adds code that manipulates the GCD memory >> space attributes of a newly allocated uncached region without checking >> whether this region expose these attributes in its capabilities mask. >> >> Given that the intent is to remove executable permissions from the region, >> this is a fairly pointless exercise to begin with, regardless of whether >> it is correct or not. The reason is that RO/XP memory attributes in the >> GCD memory space map or the UEFI memory map are completely disconnected >> from the actual mapping permissions used in the page tables. >> >> So instead, invoke the CPU arch protocol directly, and add the non-exec >> attributes in the page tables directly. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel Tested-by: Ryan Harkin FVPs and TC2 worked before this change, Juno was hanging on boot as per an earlier email thread. I tested this change on FVP Foundation & AEMv8, TC2 and Juno R0/1/2 and they all work fine after it's applied to the current HEAD of EDK2 (commit 8057622). >> --- >> ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c | 36 ++++++++++++++++---- >> ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf | 8 ++++- >> 2 files changed, 36 insertions(+), 8 deletions(-) >> >> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c >> index b4fbfbcb362b..2530175bab7a 100644 >> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c >> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.c >> @@ -27,6 +27,10 @@ >> #include >> #include >> >> +#include >> + >> +STATIC EFI_CPU_ARCH_PROTOCOL *mCpu; >> + >> VOID * >> UncachedInternalAllocatePages ( >> IN EFI_MEMORY_TYPE MemoryType, >> @@ -150,15 +154,19 @@ AllocatePagesFromList ( >> >> Status = gDS->GetMemorySpaceDescriptor (Memory, &Descriptor); >> if (EFI_ERROR (Status)) { >> - gBS->FreePages (Memory, Pages); >> - return Status; >> + goto FreePages; >> } >> >> Status = gDS->SetMemorySpaceAttributes (Memory, EFI_PAGES_TO_SIZE (Pages), >> - EFI_MEMORY_WC | EFI_MEMORY_XP); >> + EFI_MEMORY_WC); >> if (EFI_ERROR (Status)) { >> - gBS->FreePages (Memory, Pages); >> - return Status; >> + goto FreePages; >> + } >> + >> + Status = mCpu->SetMemoryAttributes (mCpu, Memory, EFI_PAGES_TO_SIZE (Pages), >> + EFI_MEMORY_XP); > > So, all other changes here are straightforward, but this one relies on > the fact that we don't care about EFI_MEMORY_RO being cleared when we > flip to _XP. As discussed on irc, if you can add a small note on that > (and wait for Ryan's Tested-by): > > Reviewed-by: Leif Lindholm > >> + if (EFI_ERROR (Status)) { >> + goto FreePages; >> } >> >> InvalidateDataCacheRange ((VOID *)(UINTN)Memory, EFI_PAGES_TO_SIZE (Pages)); >> @@ -166,8 +174,8 @@ AllocatePagesFromList ( >> NewNode = AllocatePool (sizeof (FREE_PAGE_NODE)); >> if (NewNode == NULL) { >> ASSERT (FALSE); >> - gBS->FreePages (Memory, Pages); >> - return EFI_OUT_OF_RESOURCES; >> + Status = EFI_OUT_OF_RESOURCES; >> + goto FreePages; >> } >> >> NewNode->Base = Memory; >> @@ -181,6 +189,10 @@ AllocatePagesFromList ( >> >> *Allocation = NewNode->Allocation; >> return EFI_SUCCESS; >> + >> +FreePages: >> + gBS->FreePages (Memory, Pages); >> + return Status; >> } >> >> /** >> @@ -238,6 +250,16 @@ FreePagesFromList ( >> */ >> EFI_STATUS >> EFIAPI >> +UncachedMemoryAllocationLibConstructor ( >> + IN EFI_HANDLE ImageHandle, >> + IN EFI_SYSTEM_TABLE *SystemTable >> + ) >> +{ >> + return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu); >> +} >> + >> +EFI_STATUS >> +EFIAPI >> UncachedMemoryAllocationLibDestructor ( >> IN EFI_HANDLE ImageHandle, >> IN EFI_SYSTEM_TABLE *SystemTable >> diff --git a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf >> index d7a0f2f792a1..c637430c9020 100644 >> --- a/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf >> +++ b/ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf >> @@ -22,7 +22,7 @@ [Defines] >> MODULE_TYPE = DXE_DRIVER >> VERSION_STRING = 1.0 >> LIBRARY_CLASS = UncachedMemoryAllocationLib >> - >> + CONSTRUCTOR = UncachedMemoryAllocationLibConstructor >> DESTRUCTOR = UncachedMemoryAllocationLibDestructor >> >> [Sources.common] >> @@ -42,3 +42,9 @@ [LibraryClasses] >> >> [Pcd] >> gArmTokenSpaceGuid.PcdArmFreeUncachedMemorySizeThreshold >> + >> +[Protocols] >> + gEfiCpuArchProtocolGuid >> + >> +[Depex] >> + gEfiCpuArchProtocolGuid >> -- >> 2.7.4 >>