From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x231.google.com (mail-wr0-x231.google.com [IPv6:2a00:1450:400c:c0c::231]) (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 E45278209F for ; Sat, 11 Feb 2017 06:36:04 -0800 (PST) Received: by mail-wr0-x231.google.com with SMTP id 89so126226340wrr.2 for ; Sat, 11 Feb 2017 06:36:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=iq2/wKbomO3jkjISzZG/S1KliUD88SzTsqyopr2w8oE=; b=RoxGnCcvuRfwxTWyx/6Aw4MM+bNTIiabBIt2FCdX+M1abcEOsP9e++ycGen/G/QDqG 84MffLGzlR2wbqjUCSRAcvS0xIy/uX8ytfvBuZhjSHG3mGZqFQgjMgq1UT+vEw2kqQPZ YTNOnbDeWkDRuqavAB1o3N+XYgrMlLUVZZqkk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=iq2/wKbomO3jkjISzZG/S1KliUD88SzTsqyopr2w8oE=; b=OvBkrjLBVwiMHNiWMXCRUZLMIaO1qepuQRJjSYj6yvN5x+CRyLA+1egtlSu9+AVbXe EAeQRxj4fBAb4cPsMbRbPMR6zRRkRkM53gCTL0d0glfTqwttKT0pUsbVvGU7EFicah4k oiTHwMxoHR9mFxbGCDkSLyNcu1IQWxyUsglLrdaiuJvIzTUH9heprZJf0nlyqhmSWtSG 0ih6QeeqhLmF2NzkSidMBf6mqKMhgG8VyZ5aGPx3FdhdZXoPLBeqeZCBqlBHK+di4ZyO dh12RCYTVZgymjNBkFV2f5IVPJoZBljcYKW1kuksAYL55snH5bpnKPhUwoEaTxPSrojQ JLWQ== X-Gm-Message-State: AMke39lQsYekmUwPEwBGkaJ1daqlkGp2W22J24/oXnNvFwyyZvXX6rpfFrqfCuyYazEiGOna X-Received: by 10.223.155.135 with SMTP id d7mr11261287wrc.99.1486823763121; Sat, 11 Feb 2017 06:36:03 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 17sm6596126wru.16.2017.02.11.06.36.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 11 Feb 2017 06:36:02 -0800 (PST) Date: Sat, 11 Feb 2017 14:35:59 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , "Yao, Jiewen" , "Tian, Feng" , "Kinney, Michael D" , "Fan, Jeff" , "Zeng, Star" Message-ID: <20170211143559.GS16034@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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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: Sat, 11 Feb 2017 14:36:05 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Feb 10, 2017 at 06:23:23PM +0000, Ard Biesheuvel wrote: > 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) That's fine for now. They need to be squashed at some point, but that doesn't have to be now. > >> 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. Aaaah... > > >> + 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 Sneaky :) OK, I have no issues then. / Leif > >> + 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 > >>