From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22d.google.com (mail-io0-x22d.google.com [IPv6:2607:f8b0:4001:c06::22d]) (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 01D3B82071 for ; Fri, 10 Feb 2017 10:23:25 -0800 (PST) Received: by mail-io0-x22d.google.com with SMTP id v96so57062727ioi.0 for ; Fri, 10 Feb 2017 10:23:24 -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=NPRqUYJZ60WNBkSw20QzCG0gq3UJ/3EPuYSmgHTfhKM=; b=I4cmPto4SOf7EUzzEp74PiBQJQIjWjwdgye4cEKT6/jMBDLr0IC2os+eMjf8Z3bYhK JVnHn5X/Mq5n91j1O5uOw0O/9Fn2EkZow58ockZiA5JHYKYZ1vV2dqEFU5RGTFi7osbw JuLl3IxOseTvXBaEtOdv2BETCixfJ+s0A+oN8= 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=NPRqUYJZ60WNBkSw20QzCG0gq3UJ/3EPuYSmgHTfhKM=; b=JeDevzUwybU3az+Lgbn1LuUI1Xu6Rem4qWWZX0TOpcG/gVwPleNPqhy8O2gXnKfQ5y 17oU44BrmFAO1jCP/O1icln02oaV0nPIdZD7UGQafxZ+exXwoGxlnVgn1gBCIIf1bi3Z 0us2vaFIWrjjmsQqlGvjAcPnZe1mXpTeFnXcaLNaZtk29iPik/htCVcE4hMBTqborKaW lwB1K2Pd18nPZz93R1iEDaNpgsRJ6mJHSSAMBtYpVppycLeMpBfrqziuqKyS3xXCAnen COhtQnjEc3K5NUm60cw7KHj43HFbHdjt0o8MsRSgiDWClvPTt18njjDwRJpwUrQyhoxl mUOQ== X-Gm-Message-State: AMke39m634atBE8lH25YIhDqdCWuqWr+szM8AEE9saIGOZYjnKaJWccwXHNuRXBiZDUcac89My4QXjTbrhaDrOGL X-Received: by 10.107.53.17 with SMTP id c17mr9237047ioa.45.1486751004335; Fri, 10 Feb 2017 10:23:24 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.144.139 with HTTP; Fri, 10 Feb 2017 10:23:23 -0800 (PST) In-Reply-To: <20170210181621.GP16034@bivouac.eciton.net> References: <1486661891-7888-1-git-send-email-ard.biesheuvel@linaro.org> <1486661891-7888-5-git-send-email-ard.biesheuvel@linaro.org> <20170210181621.GP16034@bivouac.eciton.net> From: Ard Biesheuvel Date: Fri, 10 Feb 2017 18:23:23 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , "Yao, Jiewen" , "Tian, Feng" , "Kinney, Michael D" , "Fan, Jeff" , "Zeng, Star" Subject: Re: [PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions 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: Fri, 10 Feb 2017 18:23:25 -0000 Content-Type: text/plain; charset=UTF-8 On 10 February 2017 at 18:16, Leif Lindholm wrote: > On Thu, Feb 09, 2017 at 05:38:11PM +0000, Ard Biesheuvel wrote: >> Since the new DXE page protection for PE/COFF images may invoke >> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission >> attributes set, add support for this in the AARCH64 MMU code. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 73 +++++++++++++++----- >> 1 file changed, 56 insertions(+), 17 deletions(-) >> >> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >> index 6aa970bc0514..764e54b5d747 100644 >> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c >> @@ -28,6 +28,10 @@ >> // We use this index definition to define an invalid block entry >> #define TT_ATTR_INDX_INVALID ((UINT32)~0) >> >> +#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | EFI_MEMORY_WC | \ >> + EFI_MEMORY_WT | EFI_MEMORY_WB | \ >> + EFI_MEMORY_UCE) >> + > > This is already duplicated between > > ArmPkg/Drivers/CpuDxe/CpuDxe.h > and > UefiCpuPkg/CpuDxe/CpuDxe.h > > Can we avoid adding more? > Good point. Mind if I move it to ArmMmuLib.h? (and keep the UefiCpuPkg one alone) >> STATIC >> UINT64 >> ArmMemoryAttributeToPageAttribute ( >> @@ -101,25 +105,46 @@ PageAttributeToGcdAttribute ( >> return GcdAttributes; >> } >> >> -ARM_MEMORY_REGION_ATTRIBUTES >> -GcdAttributeToArmAttribute ( >> +STATIC >> +UINT64 >> +GcdAttributeToPageAttribute ( >> IN UINT64 GcdAttributes >> ) >> { >> - switch (GcdAttributes & 0xFF) { >> + UINT64 PageAttributes; >> + >> + switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) { >> case EFI_MEMORY_UC: >> - return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; >> + PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY; >> + break; >> case EFI_MEMORY_WC: >> - return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; >> + PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE; >> + break; >> case EFI_MEMORY_WT: >> - return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH; >> + PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE; > > These TT_SH additions look like a bugfix - should they be a separate commit? > No, it's the diff that is confusing here: GcdAttributeToArmAttribute() is removed completely, and replaced with GcdAttributeToPageAttribute(). Due to the case labels, these line up, but they are completely unrelated. >> + break; >> case EFI_MEMORY_WB: >> - return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; >> + PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE; >> + break; >> default: >> - DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes)); >> - ASSERT (0); >> - return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; >> + PageAttributes = TT_ATTR_INDX_MASK; > > OK, so you're doing the same thing here as in the ARM code. > This is a substantial change in behaviour (old behaviour: if unknown, > set to DEVICE; new behaviour: if unknown, set "all types permitted"). > Am I missing something? > Again, completely different function >> + break; >> } >> + >> + if ((GcdAttributes & EFI_MEMORY_XP) != 0 || >> + (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) { >> + if (ArmReadCurrentEL () == AARCH64_EL2) { >> + PageAttributes |= TT_XN_MASK; >> + } else { >> + PageAttributes |= TT_UXN_MASK | TT_PXN_MASK; >> + } >> + } >> + >> + if ((GcdAttributes & EFI_MEMORY_RO) != 0) { >> + PageAttributes |= TT_AP_RO_RO; >> + } >> + >> + return PageAttributes | TT_AF; >> } >> >> #define MIN_T0SZ 16 >> @@ -434,17 +459,31 @@ SetMemoryAttributes ( >> ) >> { >> RETURN_STATUS Status; >> - ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion; >> UINT64 *TranslationTable; >> - >> - MemoryRegion.PhysicalBase = BaseAddress; >> - MemoryRegion.VirtualBase = BaseAddress; >> - MemoryRegion.Length = Length; >> - MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes); >> + UINT64 PageAttributes; >> + UINT64 PageAttributeMask; >> + >> + PageAttributes = GcdAttributeToPageAttribute (Attributes); >> + PageAttributeMask = 0; >> + >> + if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) { >> + // >> + // No memory type was set in Attributes, so we are going to update the >> + // permissions only. >> + // >> + PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK; >> + PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK | >> + TT_PXN_MASK | TT_XN_MASK); >> + } >> >> TranslationTable = ArmGetTTBR0BaseAddress (); >> >> - Status = FillTranslationTable (TranslationTable, &MemoryRegion); >> + Status = UpdateRegionMapping ( >> + TranslationTable, >> + BaseAddress, >> + Length, >> + PageAttributes, >> + PageAttributeMask); >> if (RETURN_ERROR (Status)) { >> return Status; >> } >> -- >> 2.7.4 >>