Re: [PATCH 2/3] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

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

On Thu, Jan 26, 2017 at 04:39:14PM +0800, Jiewen Yao wrote:
> 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.

Hi Jiewen,

The change looks sane, given the clarification of the specification,
but there are several instances of EFI_MEMORY_WP in comments left
around from Andrew Fish's original ARM implementation. I think they
should be updated too.

Andrew: can you confirm what the below snipped from
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c was meant to convey?
---
    case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
          //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
---

Regards,

Leif

> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 2 +-
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c     | 5 +----
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index d8bb419..4703b33 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -224,7 +224,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 14fc22d..e375cbe 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -730,9 +730,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 +740,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;
> -- 
> 2.7.4.windows.1
>