From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x22c.google.com (mail-wr0-x22c.google.com [IPv6:2a00:1450:400c:c0c::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 C5BC982035 for ; Fri, 10 Feb 2017 10:18:02 -0800 (PST) Received: by mail-wr0-x22c.google.com with SMTP id i10so114035708wrb.0 for ; Fri, 10 Feb 2017 10:18:02 -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=9URA1DA/08IKl7rPYXATXmNsauupN16B7HdDH6sHvfg=; b=cxg7m7lG6hEoVpuUPK9y/JidN0t1rGonmKKvchVVlNTPd10/kW6i+BSSmIBOTHPVi1 dR4V/Q/yqemxEC0khywS99zAQHWvaqjhkg7dFuRaVRFkcxpIRsDHy2F8IEyZSAK2urOl 5jgrxwET5LFFiSxwUPzT/Ar8KEpsQbxEsF8Lc= 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=9URA1DA/08IKl7rPYXATXmNsauupN16B7HdDH6sHvfg=; b=LaG9JUwsJZo4UZFV12CuS2nKu81qfaLUCNSB0uSUigKTQXBD9Q6ZYZez9RPOuVih09 4RSxednvb01WPg9KwzopGbS2M43g71XbD7gCztj7aOnyFH3YrEv7NCzmCBJZWdIQWP5o +R40qHb38NbYd/7EnihVcrc3pyp2dfkHF1KEFe46S+wbGyA2Dx7h6X7um9ST99t8qU7h DIGQx69PttAPSXdK2vMrHx/RSKY51/uPaqpSZyzliF8nhoOgtuysun9/3oGyZ6jOLy/3 RaLBcqhzVl6QGwhjTz2rkqr7B1oAjlqW3+Vt22BqQXPDNM9aFF8o4jckT7FsVQL1ecDp EnRQ== X-Gm-Message-State: AMke39kVoxpIQxodktbIWcTw8E5q76WwGbF4ShaQGwvZ1xYg0oEE/ZScT5G17SIT3Bna+k8Z X-Received: by 10.223.163.26 with SMTP id c26mr8572567wrb.68.1486750681306; Fri, 10 Feb 2017 10:18:01 -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 x69sm2506497wma.15.2017.02.10.10.18.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Feb 2017 10:18:00 -0800 (PST) Date: Fri, 10 Feb 2017 18:17:58 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, jiewen.yao@intel.com, feng.tian@intel.com, michael.d.kinney@intel.com, jeff.fan@intel.com, star.zeng@intel.com Message-ID: <20170210181758.GQ16034@bivouac.eciton.net> References: <1486661891-7888-1-git-send-email-ard.biesheuvel@linaro.org> <1486661891-7888-2-git-send-email-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <1486661891-7888-2-git-send-email-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage 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:18:03 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote: > From: Jiewen Yao > > Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection, > according to UEFI spec, we should use EFI_MEMORY_RO for write protection. > The EFI_MEMORY_WP is the cache attribute instead of memory attribute. > > Cc: Leif Lindholm > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel No objections to this patch, but I would have expected it to be 4/4, if it caused issues requiring the other 3 to be created? / Leif > --- > ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 3 ++- > ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 14 ++++++-------- > ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 5 +++-- > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ++- > 4 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c > index d8bb41978066..15d5a8173233 100644 > --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c > +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c > @@ -3,6 +3,7 @@ > Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.
> Portions copyright (c) 2010, Apple Inc. All rights reserved.
> Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.
> +Copyright (c) 2017, Intel Corporation. All rights reserved.
> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -224,7 +225,7 @@ EfiAttributeToArmAttribute ( > ArmAttributes |= TT_AF; > > // Determine protection attributes > - if (EfiAttributes & EFI_MEMORY_WP) { > + if (EfiAttributes & EFI_MEMORY_RO) { > ArmAttributes |= TT_AP_RO_RO; > } > > diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > index 14fc22d7a59f..6dcfba69e879 100644 > --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c > @@ -3,6 +3,7 @@ > Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.
> Portions copyright (c) 2010, Apple Inc. All rights reserved.
> Portions copyright (c) 2013, ARM Ltd. All rights reserved.
> +Copyright (c) 2017, Intel Corporation. All rights reserved.
> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -62,7 +63,7 @@ SectionToGcdAttributes ( > // determine protection attributes > switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) { > case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write > - //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP; > + //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP; > break; > > case TT_DESCRIPTOR_SECTION_AP_RW_NO: > @@ -73,7 +74,7 @@ SectionToGcdAttributes ( > // read only cases map to write-protect > case TT_DESCRIPTOR_SECTION_AP_RO_NO: > case TT_DESCRIPTOR_SECTION_AP_RO_RO: > - *GcdAttributes |= EFI_MEMORY_WP; > + *GcdAttributes |= EFI_MEMORY_RO; > break; > > default: > @@ -126,7 +127,7 @@ PageToGcdAttributes ( > // determine protection attributes > switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) { > case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write > - //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP; > + //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP; > break; > > case TT_DESCRIPTOR_PAGE_AP_RW_NO: > @@ -137,7 +138,7 @@ PageToGcdAttributes ( > // read only cases map to write-protect > case TT_DESCRIPTOR_PAGE_AP_RO_NO: > case TT_DESCRIPTOR_PAGE_AP_RO_RO: > - *GcdAttributes |= EFI_MEMORY_WP; > + *GcdAttributes |= EFI_MEMORY_RO; > break; > > default: > @@ -730,9 +731,6 @@ EfiAttributeToArmAttribute ( > ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1 > break; > > - case EFI_MEMORY_WP: > - case EFI_MEMORY_XP: > - case EFI_MEMORY_RP: > case EFI_MEMORY_UCE: > default: > // Cannot be implemented UEFI definition unclear for ARM > @@ -743,7 +741,7 @@ EfiAttributeToArmAttribute ( > } > > // Determine protection attributes > - if (EfiAttributes & EFI_MEMORY_WP) { > + if (EfiAttributes & EFI_MEMORY_RO) { > ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO; > } else { > ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW; > diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c > index 723604d1df96..54d9b0163331 100644 > --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c > +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c > @@ -1,6 +1,7 @@ > /** @file > * > * Copyright (c) 2013, ARM Limited. All rights reserved. > +* Copyright (c) 2017, Intel Corporation. All rights reserved.
> * > * This program and the accompanying materials > * are licensed and made available under the terms and conditions of the BSD License > @@ -236,7 +237,7 @@ CpuConvertPagesToUncachedVirtualAddress ( > // be the PCI address. Code should always use the CPU address, and we will or in VirtualMask > // to that address. > // > - Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_WP, 0); > + Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_RO, 0); > if (!EFI_ERROR (Status)) { > Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_UC, VirtualMask); > } > @@ -264,7 +265,7 @@ CpuReconvertPages ( > // > // Unmap the aliased Address > // > - Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_WP, 0); > + Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_RO, 0); > if (!EFI_ERROR (Status)) { > // > // Restore atttributes > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > index 540069a59b2e..6aa970bc0514 100644 > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > @@ -3,6 +3,7 @@ > * > * Copyright (c) 2011-2014, ARM Limited. All rights reserved. > * Copyright (c) 2016, Linaro Limited. All rights reserved. > +* Copyright (c) 2017, Intel Corporation. All rights reserved.
> * > * This program and the accompanying materials > * are licensed and made available under the terms and conditions of the BSD License > @@ -89,7 +90,7 @@ PageAttributeToGcdAttribute ( > // Determine protection attributes > if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) || ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) { > // Read only cases map to write-protect > - GcdAttributes |= EFI_MEMORY_WP; > + GcdAttributes |= EFI_MEMORY_RO; > } > > // Process eXecute Never attribute > -- > 2.7.4 >