Re: [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:
> From: Jiewen Yao <jiewen.yao@intel.com>
> 
> 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 <leif.lindholm@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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.<BR>
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
>  Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  
>  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.<BR>
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
>  Portions copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  
>  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.<BR>
>  *
>  *  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.<BR>
>  *
>  *  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
>