diff mbox series

[edk2,v2,3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()

Message ID 1488450976-16257-4-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show
Series ArmPkg, ArmVirtpkg ARM: enable strict memory protection | expand

Commit Message

Ard Biesheuvel March 2, 2017, 10:36 a.m. UTC
Enable the use of strict memory permissions on ARM by processing the
EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
bits will preserve the cacheability attributes. Permissions attributes
are not preserved when setting the memory type only: the way the memory
permission attributes are defined does not allows for that, and so this
situation does not deviate from other architectures.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------
 1 file changed, 62 insertions(+), 89 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm March 6, 2017, 2:48 p.m. UTC | #1
On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:
> Enable the use of strict memory permissions on ARM by processing the

> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,

> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP

> bits will preserve the cacheability attributes. Permissions attributes

> are not preserved when setting the memory type only: the way the memory

> permission attributes are defined does not allows for that, and so this

> situation does not deviate from other architectures.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------

>  1 file changed, 62 insertions(+), 89 deletions(-)

> 

> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> index 26b637e7658f..6dd749dadf8b 100644

> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> @@ -374,50 +374,41 @@ UpdatePageEntries (

>  

>    // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)

>    // EntryValue: values at bit positions specified by EntryMask

> -  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;

> -  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;

> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;

> +  if ((Attributes & EFI_MEMORY_XP) != 0) {

> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;

> +  } else {

> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;

> +  }

> +

>    // Although the PI spec is unclear on this the GCD guarantees that only

>    // one Attribute bit is set at a time, so we can safely use a switch statement


But the switch statement is going away.

The change effectively introduces a guaranteed priority of
interpretation if the spec is violated. Say something about this order
being arbitrarily decided due to PI spce guarantee instead?

> -  switch (Attributes) {

> -    case EFI_MEMORY_UC:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> -      // map to strongly ordered

> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> -      break;

> -

> -    case EFI_MEMORY_WC:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> -      // map to normal non-cachable

> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> -      break;

> -

> -    case EFI_MEMORY_WT:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> -      // write through with no-allocate

> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> -      break;

> -

> -    case EFI_MEMORY_WB:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> -      // write back (with allocate)

> -      EntryValue |= TT_DESCRIPTOR_PAGE_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_UCE:

> -      // cannot be implemented UEFI definition unclear for ARM

> -      // Cause a page fault if these ranges are accessed.

> -      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;

> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));

> -      break;

> +  if ((Attributes & EFI_MEMORY_UC) != 0) {


Or should these be "Attributes & Mask" == "ATTRIBUTE"?

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> +    // map to strongly ordered

> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> +    // map to normal non-cachable

> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> +    // write through with no-allocate

> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> +    // write back (with allocate)

> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

> +  }

>  

> -    default:

> -      return EFI_UNSUPPORTED;


Do we not want a fallback handling for the if-form as well?

> +  if ((Attributes & EFI_MEMORY_RO) != 0) {

> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;

> +  } else {

> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;

>    }

>  

>    // Obtain page table base

> @@ -520,53 +511,42 @@ UpdateSectionEntries (

>    // EntryValue: values at bit positions specified by EntryMask

>  

>    // Make sure we handle a section range that is unmapped

> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;

> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |

> +              TT_DESCRIPTOR_SECTION_AP_MASK;

>    EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;

>  

>    // Although the PI spec is unclear on this the GCD guarantees that only

>    // one Attribute bit is set at a time, so we can safely use a switch statement


Repeat of above.

> -  switch(Attributes) {

> -    case EFI_MEMORY_UC:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> -      // map to strongly ordered

> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> -      break;

> -

> -    case EFI_MEMORY_WC:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> -      // map to normal non-cachable

> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> -      break;

> -

> -    case EFI_MEMORY_WT:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> -      // write through with no-allocate

> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> -      break;

> -

> -    case EFI_MEMORY_WB:

> -      // modify cacheability attributes

> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> -      // write back (with allocate)

> -      EntryValue |= 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:

> -      // cannot be implemented UEFI definition unclear for ARM

> -      // Cause a page fault if these ranges are accessed.

> -      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;

> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));

> -      break;

> +  if ((Attributes & EFI_MEMORY_UC) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> +    // map to strongly ordered

> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> +    // map to normal non-cachable

> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> +    // write through with no-allocate

> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {

> +    // modify cacheability attributes

> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> +    // write back (with allocate)

> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

> +  }


(Again, same questions as above.)

>  

> +  if ((Attributes & EFI_MEMORY_RO) != 0) {

> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;

> +  } else {

> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;

> +  }

>  

> -    default:

> -      return EFI_UNSUPPORTED;

> +  if ((Attributes & EFI_MEMORY_XP) != 0) {

> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;

>    }

>  

>    // obtain page table base

> @@ -693,13 +673,6 @@ SetMemoryAttributes (

>    UINT64        ChunkLength;

>    BOOLEAN       FlushTlbs;

>  

> -  //

> -  // Ignore invocations that only modify permission bits

> -  //

> -  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {

> -    return EFI_SUCCESS;

> -  }

> -

>    FlushTlbs = FALSE;

>    while (Length > 0) {

>      if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&

> -- 

> 2.7.4

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel March 6, 2017, 3:11 p.m. UTC | #2
On 6 March 2017 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:

>> Enable the use of strict memory permissions on ARM by processing the

>> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,

>> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP

>> bits will preserve the cacheability attributes. Permissions attributes

>> are not preserved when setting the memory type only: the way the memory

>> permission attributes are defined does not allows for that, and so this

>> situation does not deviate from other architectures.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------

>>  1 file changed, 62 insertions(+), 89 deletions(-)

>>

>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

>> index 26b637e7658f..6dd749dadf8b 100644

>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

>> @@ -374,50 +374,41 @@ UpdatePageEntries (

>>

>>    // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)

>>    // EntryValue: values at bit positions specified by EntryMask

>> -  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;

>> -  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;

>> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;

>> +  if ((Attributes & EFI_MEMORY_XP) != 0) {

>> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;

>> +  } else {

>> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;

>> +  }

>> +

>>    // Although the PI spec is unclear on this the GCD guarantees that only

>>    // one Attribute bit is set at a time, so we can safely use a switch statement

>

> But the switch statement is going away.

>

> The change effectively introduces a guaranteed priority of

> interpretation if the spec is violated. Say something about this order

> being arbitrarily decided due to PI spce guarantee instead?

>


Indeed.

>> -  switch (Attributes) {

>> -    case EFI_MEMORY_UC:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> -      // map to strongly ordered

>> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

>> -      break;

>> -

>> -    case EFI_MEMORY_WC:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> -      // map to normal non-cachable

>> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

>> -      break;

>> -

>> -    case EFI_MEMORY_WT:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> -      // write through with no-allocate

>> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

>> -      break;

>> -

>> -    case EFI_MEMORY_WB:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> -      // write back (with allocate)

>> -      EntryValue |= TT_DESCRIPTOR_PAGE_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_UCE:

>> -      // cannot be implemented UEFI definition unclear for ARM

>> -      // Cause a page fault if these ranges are accessed.

>> -      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;

>> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));

>> -      break;

>> +  if ((Attributes & EFI_MEMORY_UC) != 0) {

>

> Or should these be "Attributes & Mask" == "ATTRIBUTE"?

>


I know this is more idiomatic for EDK2, but the mask *is* the
attribute in this case, and so the former implies the latter. If the
mask were

(EFI_MEMORY_WB | EFI_MEMORY_WT | etc

it would make more sense do to the latter I think

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> +    // map to strongly ordered

>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

>> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> +    // map to normal non-cachable

>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

>> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> +    // write through with no-allocate

>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

>> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

>> +    // write back (with allocate)

>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

>> +  }

>>

>> -    default:

>> -      return EFI_UNSUPPORTED;

>

> Do we not want a fallback handling for the if-form as well?

>


No, and that is actually the point of this patch. SetMemoryAttributes
may be called without a memory type argument, in which case it only
modifies permission attributes, and leaves the memory type attributes
only.

>> +  if ((Attributes & EFI_MEMORY_RO) != 0) {

>> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;

>> +  } else {

>> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;

>>    }

>>

>>    // Obtain page table base

>> @@ -520,53 +511,42 @@ UpdateSectionEntries (

>>    // EntryValue: values at bit positions specified by EntryMask

>>

>>    // Make sure we handle a section range that is unmapped

>> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;

>> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |

>> +              TT_DESCRIPTOR_SECTION_AP_MASK;

>>    EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;

>>

>>    // Although the PI spec is unclear on this the GCD guarantees that only

>>    // one Attribute bit is set at a time, so we can safely use a switch statement

>

> Repeat of above.

>

>> -  switch(Attributes) {

>> -    case EFI_MEMORY_UC:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> -      // map to strongly ordered

>> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

>> -      break;

>> -

>> -    case EFI_MEMORY_WC:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> -      // map to normal non-cachable

>> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

>> -      break;

>> -

>> -    case EFI_MEMORY_WT:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> -      // write through with no-allocate

>> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

>> -      break;

>> -

>> -    case EFI_MEMORY_WB:

>> -      // modify cacheability attributes

>> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> -      // write back (with allocate)

>> -      EntryValue |= 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:

>> -      // cannot be implemented UEFI definition unclear for ARM

>> -      // Cause a page fault if these ranges are accessed.

>> -      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;

>> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));

>> -      break;

>> +  if ((Attributes & EFI_MEMORY_UC) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> +    // map to strongly ordered

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

>> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> +    // map to normal non-cachable

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

>> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> +    // write through with no-allocate

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

>> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {

>> +    // modify cacheability attributes

>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

>> +    // write back (with allocate)

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

>> +  }

>

> (Again, same questions as above.)

>

>>

>> +  if ((Attributes & EFI_MEMORY_RO) != 0) {

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;

>> +  } else {

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;

>> +  }

>>

>> -    default:

>> -      return EFI_UNSUPPORTED;

>> +  if ((Attributes & EFI_MEMORY_XP) != 0) {

>> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;

>>    }

>>

>>    // obtain page table base

>> @@ -693,13 +673,6 @@ SetMemoryAttributes (

>>    UINT64        ChunkLength;

>>    BOOLEAN       FlushTlbs;

>>

>> -  //

>> -  // Ignore invocations that only modify permission bits

>> -  //

>> -  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {

>> -    return EFI_SUCCESS;

>> -  }

>> -

>>    FlushTlbs = FALSE;

>>    while (Length > 0) {

>>      if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&

>> --

>> 2.7.4

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 6, 2017, 4:40 p.m. UTC | #3
On Mon, Mar 06, 2017 at 04:11:50PM +0100, Ard Biesheuvel wrote:
> On 6 March 2017 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:

> >> Enable the use of strict memory permissions on ARM by processing the

> >> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,

> >> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP

> >> bits will preserve the cacheability attributes. Permissions attributes

> >> are not preserved when setting the memory type only: the way the memory

> >> permission attributes are defined does not allows for that, and so this

> >> situation does not deviate from other architectures.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> ---

> >>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------

> >>  1 file changed, 62 insertions(+), 89 deletions(-)

> >>

> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> >> index 26b637e7658f..6dd749dadf8b 100644

> >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> >> @@ -374,50 +374,41 @@ UpdatePageEntries (

> >>

> >>    // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)

> >>    // EntryValue: values at bit positions specified by EntryMask

> >> -  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;

> >> -  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;

> >> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;

> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {

> >> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;

> >> +  } else {

> >> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;

> >> +  }

> >> +

> >>    // Although the PI spec is unclear on this the GCD guarantees that only

> >>    // one Attribute bit is set at a time, so we can safely use a switch statement

> >

> > But the switch statement is going away.

> >

> > The change effectively introduces a guaranteed priority of

> > interpretation if the spec is violated. Say something about this order

> > being arbitrarily decided due to PI spce guarantee instead?

> >

> 

> Indeed.


OK, I'll hold back to see what you come up with :)

> >> -  switch (Attributes) {

> >> -    case EFI_MEMORY_UC:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> -      // map to strongly ordered

> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> >> -      break;

> >> -

> >> -    case EFI_MEMORY_WC:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> -      // map to normal non-cachable

> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> >> -      break;

> >> -

> >> -    case EFI_MEMORY_WT:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> -      // write through with no-allocate

> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> >> -      break;

> >> -

> >> -    case EFI_MEMORY_WB:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> -      // write back (with allocate)

> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_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_UCE:

> >> -      // cannot be implemented UEFI definition unclear for ARM

> >> -      // Cause a page fault if these ranges are accessed.

> >> -      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;

> >> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));

> >> -      break;

> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {

> >

> > Or should these be "Attributes & Mask" == "ATTRIBUTE"?

> >

> 

> I know this is more idiomatic for EDK2, but the mask *is* the

> attribute in this case, and so the former implies the latter. If the

> mask were


OK ... in that case, could you just drop the != 0?

> (EFI_MEMORY_WB | EFI_MEMORY_WT | etc

> 

> it would make more sense do to the latter I think

> 

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> +    // map to strongly ordered

> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> +    // map to normal non-cachable

> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> +    // write through with no-allocate

> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;

> >> +    // write back (with allocate)

> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

> >> +  }

> >>

> >> -    default:

> >> -      return EFI_UNSUPPORTED;

> >

> > Do we not want a fallback handling for the if-form as well?

> >

> 

> No, and that is actually the point of this patch. SetMemoryAttributes

> may be called without a memory type argument, in which case it only

> modifies permission attributes, and leaves the memory type attributes

> only.


But should there not be a final "else if (Attributes &
CACHE_ATTRIBUTE_MASK)" error path then?

/
    Leif

> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {

> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;

> >> +  } else {

> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;

> >>    }

> >>

> >>    // Obtain page table base

> >> @@ -520,53 +511,42 @@ UpdateSectionEntries (

> >>    // EntryValue: values at bit positions specified by EntryMask

> >>

> >>    // Make sure we handle a section range that is unmapped

> >> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;

> >> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |

> >> +              TT_DESCRIPTOR_SECTION_AP_MASK;

> >>    EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;

> >>

> >>    // Although the PI spec is unclear on this the GCD guarantees that only

> >>    // one Attribute bit is set at a time, so we can safely use a switch statement

> >

> > Repeat of above.

> >

> >> -  switch(Attributes) {

> >> -    case EFI_MEMORY_UC:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> -      // map to strongly ordered

> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> >> -      break;

> >> -

> >> -    case EFI_MEMORY_WC:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> -      // map to normal non-cachable

> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> >> -      break;

> >> -

> >> -    case EFI_MEMORY_WT:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> -      // write through with no-allocate

> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> >> -      break;

> >> -

> >> -    case EFI_MEMORY_WB:

> >> -      // modify cacheability attributes

> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> -      // write back (with allocate)

> >> -      EntryValue |= 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:

> >> -      // cannot be implemented UEFI definition unclear for ARM

> >> -      // Cause a page fault if these ranges are accessed.

> >> -      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;

> >> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));

> >> -      break;

> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> +    // map to strongly ordered

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0

> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> +    // map to normal non-cachable

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0

> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> +    // write through with no-allocate

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0

> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {

> >> +    // modify cacheability attributes

> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;

> >> +    // write back (with allocate)

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

> >> +  }

> >

> > (Again, same questions as above.)

> >

> >>

> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;

> >> +  } else {

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;

> >> +  }

> >>

> >> -    default:

> >> -      return EFI_UNSUPPORTED;

> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {

> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;

> >>    }

> >>

> >>    // obtain page table base

> >> @@ -693,13 +673,6 @@ SetMemoryAttributes (

> >>    UINT64        ChunkLength;

> >>    BOOLEAN       FlushTlbs;

> >>

> >> -  //

> >> -  // Ignore invocations that only modify permission bits

> >> -  //

> >> -  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {

> >> -    return EFI_SUCCESS;

> >> -  }

> >> -

> >>    FlushTlbs = FALSE;

> >>    while (Length > 0) {

> >>      if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&

> >> --

> >> 2.7.4

> >>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 26b637e7658f..6dd749dadf8b 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -374,50 +374,41 @@  UpdatePageEntries (
 
   // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
   // EntryValue: values at bit positions specified by EntryMask
-  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
-  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
+  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
+  } else {
+    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
+  }
+
   // Although the PI spec is unclear on this the GCD guarantees that only
   // one Attribute bit is set at a time, so we can safely use a switch statement
-  switch (Attributes) {
-    case EFI_MEMORY_UC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // map to strongly ordered
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
-      break;
-
-    case EFI_MEMORY_WC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // map to normal non-cachable
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
-      break;
-
-    case EFI_MEMORY_WT:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // write through with no-allocate
-      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
-      break;
-
-    case EFI_MEMORY_WB:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-      // write back (with allocate)
-      EntryValue |= TT_DESCRIPTOR_PAGE_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_UCE:
-      // cannot be implemented UEFI definition unclear for ARM
-      // Cause a page fault if these ranges are accessed.
-      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
-      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
-      break;
+  if ((Attributes & EFI_MEMORY_UC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // map to strongly ordered
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // map to normal non-cachable
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // write through with no-allocate
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // write back (with allocate)
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+  }
 
-    default:
-      return EFI_UNSUPPORTED;
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
   }
 
   // Obtain page table base
@@ -520,53 +511,42 @@  UpdateSectionEntries (
   // EntryValue: values at bit positions specified by EntryMask
 
   // Make sure we handle a section range that is unmapped
-  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
+  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
+              TT_DESCRIPTOR_SECTION_AP_MASK;
   EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
 
   // Although the PI spec is unclear on this the GCD guarantees that only
   // one Attribute bit is set at a time, so we can safely use a switch statement
-  switch(Attributes) {
-    case EFI_MEMORY_UC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // map to strongly ordered
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
-      break;
-
-    case EFI_MEMORY_WC:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // map to normal non-cachable
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
-      break;
-
-    case EFI_MEMORY_WT:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // write through with no-allocate
-      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
-      break;
-
-    case EFI_MEMORY_WB:
-      // modify cacheability attributes
-      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-      // write back (with allocate)
-      EntryValue |= 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:
-      // cannot be implemented UEFI definition unclear for ARM
-      // Cause a page fault if these ranges are accessed.
-      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
-      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
-      break;
+  if ((Attributes & EFI_MEMORY_UC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // map to strongly ordered
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // map to normal non-cachable
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // write through with no-allocate
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // write back (with allocate)
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+  }
 
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
+  }
 
-    default:
-      return EFI_UNSUPPORTED;
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
   }
 
   // obtain page table base
@@ -693,13 +673,6 @@  SetMemoryAttributes (
   UINT64        ChunkLength;
   BOOLEAN       FlushTlbs;
 
-  //
-  // Ignore invocations that only modify permission bits
-  //
-  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
-    return EFI_SUCCESS;
-  }
-
   FlushTlbs = FALSE;
   while (Length > 0) {
     if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&