subject:"\[edk2\] \[PATCH\] ArmPkg\/AArch64Mmu\: disable MMU during page table manipulations"

Re: [edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

2016-04-11 Thread Ard Biesheuvel

On 11 April 2016 at 16:10, Mark Rutland  wrote:
> Hi Ard,
>
> On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote:
>> On ARM, manipulating live page tables is cumbersome since the architecture
>> mandates the use of break-before-make, i.e., replacing a block entry with
>> a table entry requires an intermediate step via an invalid entry, or TLB
>> conflicts may occur.
>>
>> Since it is not generally feasible to decide in the page table manipulation
>> routines whether such an invalid entry will result in those routines
>> themselves to become unavailable, and since UEFI uses an identity mapping
>> anyway, it is far simpler to just disable the MMU, perform the page table
>> manipulations, flush the TLBs and re-enable the MMU.
>
> Perhaps I am missing something, but I suspect that this isn't so simple.

I am afraid it was I who was missing something. Not having a good day ... :-)

> More on that below.
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c 
>> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
>> index b7d23c6f3286..2f8f05df8aec 100644
>> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
>> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
>> @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress (
>>}
>>  } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
>>// If we are not at the last level then we need to split this 
>> BlockEntry
>> +  // Since splitting live block entries may cause TLB conflicts, we 
>> need to
>> +  // enter this function with the MMU disabled and flush the TLBs before
>> +  // re-enabling it. This is the responsibility of the caller.
>> +  ASSERT (!ArmMmuEnabled ());
>> +
>>if (IndexLevel != PageLevel) {
>>  // Retrieve the attributes from the block entry
>>  Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;
>> @@ -453,6 +458,8 @@ SetMemoryAttributes (
>>RETURN_STATUSStatus;
>>ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>>UINT64  *TranslationTable;
>> +  UINTNSavedInterruptState;
>> +  BOOLEAN  SavedMmuState;
>>
>>MemoryRegion.PhysicalBase = BaseAddress;
>>MemoryRegion.VirtualBase = BaseAddress;
>> @@ -461,6 +468,15 @@ SetMemoryAttributes (
>>
>>TranslationTable = ArmGetTTBR0BaseAddress ();
>>
>> +  SavedMmuState = ArmMmuEnabled ();
>> +  if (SavedMmuState) {
>> +SavedInterruptState = ArmGetInterruptState ();
>> +if (SavedInterruptState) {
>> +  ArmDisableInterrupts();
>> +}
>> +ArmDisableMmu();
>> +  }
>
> Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use
> by EDK2 (after the MMU is disabled), this is not safe.
>
> For example, the latest version of your stack (which may be dirty in
> cache) is not guaranteed to be visible after the MMU is disabled. If any
> of that is dirty it may be naturally evicted at any time,
> masking/corrupting data written with the MMU off. Any implicit memory
> accesses generated by the compiler may go wrong.
>
> A similar problem applies to anything previously mapped with cacheable
> attributes.
>

Indeed. I can't believe I didn't spot that. I did have the clarity of
mind to put you on cc though :-)


>> +
>>Status = FillTranslationTable (TranslationTable, );
>>if (RETURN_ERROR (Status)) {
>>  return Status;
>> @@ -468,6 +484,12 @@ SetMemoryAttributes (
>>
>>// Invalidate all TLB entries so changes are synced
>>ArmInvalidateTlb ();
>> +  if (SavedMmuState) {
>> +ArmEnableMmu();
>> +if (SavedInterruptState) {
>> +  ArmEnableInterrupts ();
>> +}
>> +  }
>
> Likewise, now everything written with the MMU off is not guaranteed to
> be visible to subsequent cacheable accesses, due to stale lines
> allocated before the MMU was disabled. That includes the modifications
> to the page tables, which may become eventually become visible to
> cacheable accesses in an unpredictable fashion (e.g. you might still end
> up with the TLBs filling with conflicting entries).
>
>>return RETURN_SUCCESS;
>>  }
>> @@ -483,9 +505,20 @@ SetMemoryRegionAttribute (
>>  {
>>RETURN_STATUSStatus;
>>UINT64   *RootTable;
>> +  UINTNSavedInterruptState;
>> +  BOOLEAN  SavedMmuState;
>>
>>RootTable = ArmGetTTBR0BaseAddress ();
>>
>> +  SavedMmuState = ArmMmuEnabled ();
>> +  if (SavedMmuState) {
>> +SavedInterruptState = ArmGetInterruptState ();
>> +if (SavedInterruptState) {
>> +  ArmDisableInterrupts();
>> +}
>> +ArmDisableMmu();
>> +  }
>> +
>>Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, 
>> BlockEntryMask);
>>if

Re: [edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

2016-04-11 Thread Mark Rutland

Hi Ard,

On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote:
> On ARM, manipulating live page tables is cumbersome since the architecture
> mandates the use of break-before-make, i.e., replacing a block entry with
> a table entry requires an intermediate step via an invalid entry, or TLB
> conflicts may occur.
> 
> Since it is not generally feasible to decide in the page table manipulation
> routines whether such an invalid entry will result in those routines
> themselves to become unavailable, and since UEFI uses an identity mapping
> anyway, it is far simpler to just disable the MMU, perform the page table
> manipulations, flush the TLBs and re-enable the MMU.

Perhaps I am missing something, but I suspect that this isn't so simple.
More on that below.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 
>  1 file changed, 39 insertions(+)
> 
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c 
> b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> index b7d23c6f3286..2f8f05df8aec 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
> @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress (
>}
>  } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
>// If we are not at the last level then we need to split this 
> BlockEntry
> +  // Since splitting live block entries may cause TLB conflicts, we need 
> to
> +  // enter this function with the MMU disabled and flush the TLBs before
> +  // re-enabling it. This is the responsibility of the caller.
> +  ASSERT (!ArmMmuEnabled ());
> +
>if (IndexLevel != PageLevel) {
>  // Retrieve the attributes from the block entry
>  Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;
> @@ -453,6 +458,8 @@ SetMemoryAttributes (
>RETURN_STATUSStatus;
>ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>UINT64  *TranslationTable;
> +  UINTNSavedInterruptState;
> +  BOOLEAN  SavedMmuState;
>  
>MemoryRegion.PhysicalBase = BaseAddress;
>MemoryRegion.VirtualBase = BaseAddress;
> @@ -461,6 +468,15 @@ SetMemoryAttributes (
>  
>TranslationTable = ArmGetTTBR0BaseAddress ();
>  
> +  SavedMmuState = ArmMmuEnabled ();
> +  if (SavedMmuState) {
> +SavedInterruptState = ArmGetInterruptState ();
> +if (SavedInterruptState) {
> +  ArmDisableInterrupts();
> +}
> +ArmDisableMmu();
> +  }

Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use
by EDK2 (after the MMU is disabled), this is not safe.

For example, the latest version of your stack (which may be dirty in
cache) is not guaranteed to be visible after the MMU is disabled. If any
of that is dirty it may be naturally evicted at any time,
masking/corrupting data written with the MMU off. Any implicit memory
accesses generated by the compiler may go wrong.

A similar problem applies to anything previously mapped with cacheable
attributes.

> +
>Status = FillTranslationTable (TranslationTable, );
>if (RETURN_ERROR (Status)) {
>  return Status;
> @@ -468,6 +484,12 @@ SetMemoryAttributes (
>  
>// Invalidate all TLB entries so changes are synced
>ArmInvalidateTlb ();
> +  if (SavedMmuState) {
> +ArmEnableMmu();
> +if (SavedInterruptState) {
> +  ArmEnableInterrupts ();
> +}
> +  }

Likewise, now everything written with the MMU off is not guaranteed to
be visible to subsequent cacheable accesses, due to stale lines
allocated before the MMU was disabled. That includes the modifications
to the page tables, which may become eventually become visible to
cacheable accesses in an unpredictable fashion (e.g. you might still end
up with the TLBs filling with conflicting entries).

>return RETURN_SUCCESS;
>  }
> @@ -483,9 +505,20 @@ SetMemoryRegionAttribute (
>  {
>RETURN_STATUSStatus;
>UINT64   *RootTable;
> +  UINTNSavedInterruptState;
> +  BOOLEAN  SavedMmuState;
>  
>RootTable = ArmGetTTBR0BaseAddress ();
>  
> +  SavedMmuState = ArmMmuEnabled ();
> +  if (SavedMmuState) {
> +SavedInterruptState = ArmGetInterruptState ();
> +if (SavedInterruptState) {
> +  ArmDisableInterrupts();
> +}
> +ArmDisableMmu();
> +  }
> +
>Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, 
> BlockEntryMask);
>if (RETURN_ERROR (Status)) {
>  return Status;
> @@ -493,6 +526,12 @@ SetMemoryRegionAttribute (
>  
>// Invalidate all TLB entries so changes are synced
>ArmInvalidateTlb ();
> +  if (SavedMmuState) {
> +ArmEnableMmu();
> +if (SavedInterruptState) {
> +  ArmEnableInterrupts ();
> +}
> +  }

The same problems apply to both of these.

Thanks,
Mark.

[edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

2016-04-11 Thread Ard Biesheuvel

On ARM, manipulating live page tables is cumbersome since the architecture
mandates the use of break-before-make, i.e., replacing a block entry with
a table entry requires an intermediate step via an invalid entry, or TLB
conflicts may occur.

Since it is not generally feasible to decide in the page table manipulation
routines whether such an invalid entry will result in those routines
themselves to become unavailable, and since UEFI uses an identity mapping
anyway, it is far simpler to just disable the MMU, perform the page table
manipulations, flush the TLBs and re-enable the MMU.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 
 1 file changed, 39 insertions(+)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
index b7d23c6f3286..2f8f05df8aec 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
@@ -286,6 +286,11 @@ GetBlockEntryListFromAddress (
   }
 } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
   // If we are not at the last level then we need to split this BlockEntry
+  // Since splitting live block entries may cause TLB conflicts, we need to
+  // enter this function with the MMU disabled and flush the TLBs before
+  // re-enabling it. This is the responsibility of the caller.
+  ASSERT (!ArmMmuEnabled ());
+
   if (IndexLevel != PageLevel) {
 // Retrieve the attributes from the block entry
 Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;
@@ -453,6 +458,8 @@ SetMemoryAttributes (
   RETURN_STATUSStatus;
   ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
   UINT64  *TranslationTable;
+  UINTNSavedInterruptState;
+  BOOLEAN  SavedMmuState;
 
   MemoryRegion.PhysicalBase = BaseAddress;
   MemoryRegion.VirtualBase = BaseAddress;
@@ -461,6 +468,15 @@ SetMemoryAttributes (
 
   TranslationTable = ArmGetTTBR0BaseAddress ();
 
+  SavedMmuState = ArmMmuEnabled ();
+  if (SavedMmuState) {
+SavedInterruptState = ArmGetInterruptState ();
+if (SavedInterruptState) {
+  ArmDisableInterrupts();
+}
+ArmDisableMmu();
+  }
+
   Status = FillTranslationTable (TranslationTable, );
   if (RETURN_ERROR (Status)) {
 return Status;
@@ -468,6 +484,12 @@ SetMemoryAttributes (
 
   // Invalidate all TLB entries so changes are synced
   ArmInvalidateTlb ();
+  if (SavedMmuState) {
+ArmEnableMmu();
+if (SavedInterruptState) {
+  ArmEnableInterrupts ();
+}
+  }
 
   return RETURN_SUCCESS;
 }
@@ -483,9 +505,20 @@ SetMemoryRegionAttribute (
 {
   RETURN_STATUSStatus;
   UINT64   *RootTable;
+  UINTNSavedInterruptState;
+  BOOLEAN  SavedMmuState;
 
   RootTable = ArmGetTTBR0BaseAddress ();
 
+  SavedMmuState = ArmMmuEnabled ();
+  if (SavedMmuState) {
+SavedInterruptState = ArmGetInterruptState ();
+if (SavedInterruptState) {
+  ArmDisableInterrupts();
+}
+ArmDisableMmu();
+  }
+
   Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, 
BlockEntryMask);
   if (RETURN_ERROR (Status)) {
 return Status;
@@ -493,6 +526,12 @@ SetMemoryRegionAttribute (
 
   // Invalidate all TLB entries so changes are synced
   ArmInvalidateTlb ();
+  if (SavedMmuState) {
+ArmEnableMmu();
+if (SavedInterruptState) {
+  ArmEnableInterrupts ();
+}
+  }
 
   return RETURN_SUCCESS;
 }
-- 
2.5.0

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

Re: [edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

Re: [edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

[edk2] [PATCH] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

3 matches

Site Navigation

Mail list logo

Footer information