diff mbox series

[edk2,v2,2/2] ArmPkg/ArmMmuLib ARM: assume page tables are in writeback cacheable memory

Message ID 20180621081315.16228-3-ard.biesheuvel@linaro.org
State Accepted
Commit 6e275c613e15ffc6dc79901fb244e8cb20af9948
Headers show
Series ArmMmuLib ARM: remove cache maintenance | expand

Commit Message

Ard Biesheuvel June 21, 2018, 8:13 a.m. UTC 
Given that these days, our ARM port only supports ARMv7 and later, we
can assume that the page table walker's memory accesses are cache
coherent, and so there is no need to perform cache maintenance. It
does require the page tables themselves to reside in memory mapped as
writeback cacheable so ASSERT() that this is the case.

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

---
 ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S    |  2 --
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 14 +++-----------
 2 files changed, 3 insertions(+), 13 deletions(-)

-- 
2.17.1

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

Comments

Leif Lindholm June 21, 2018, 2:01 p.m. UTC | #1 
On Thu, Jun 21, 2018 at 10:13:15AM +0200, Ard Biesheuvel wrote:
> Given that these days, our ARM port only supports ARMv7 and later, we

> can assume that the page table walker's memory accesses are cache

> coherent, and so there is no need to perform cache maintenance. It

> does require the page tables themselves to reside in memory mapped as

> writeback cacheable so ASSERT() that this is the case.


One minor nit.

> Contributed-under: TianoCore Contribution Agreement 1.1

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

> ---

>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S    |  2 --

>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 14 +++-----------

>  2 files changed, 3 insertions(+), 13 deletions(-)

> 

> diff --git a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S

> index 149b57e059ee..f2a517671f0a 100644

> --- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S

> +++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S

> @@ -100,8 +100,6 @@ ASM_FUNC(ArmGetTTBR0BaseAddress)

>  //  IN VOID  *MVA                    // R1

>  //  );

>  ASM_FUNC(ArmUpdateTranslationTableEntry)

> -  mcr     p15,0,R0,c7,c14,1     @ DCCIMVAC Clean data cache by MVA

> -  dsb

>    mcr     p15,0,R1,c8,c7,1      @ TLBIMVA TLB Invalidate MVA

>    mcr     p15,0,R9,c7,c5,6      @ BPIALL Invalidate Branch predictor array. R9 == NoOp

>    dsb

> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c

> index 9c2578979e44..3037b642d40c 100644

> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c

> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c

> @@ -343,17 +343,12 @@ ArmConfigureMmu (

>    }

>  

>    // Translate the Memory Attributes into Translation Table Register Attributes

> -  if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED) ||

> -      (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED)) {

> -    TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_NON_CACHEABLE : TTBR_NON_CACHEABLE;

> -  } else if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK) ||

> +  if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK) ||

>        (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK)) {

>      TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_WRITE_BACK_ALLOC : TTBR_WRITE_BACK_ALLOC;

> -  } else if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH) ||

> -      (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH)) {

> -    TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_WRITE_THROUGH : TTBR_WRITE_THROUGH;

>    } else {

> -    ASSERT (0); // No support has been found for the attributes of the memory region that the translation table belongs to.

> +    // Page tables must reside in memory mapped as writeback cacheable


ARM ARM always uses "write-back" - could you add the hyphen to assist
greppability?

If so, for the series:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> +    ASSERT (0);

>      return RETURN_UNSUPPORTED;

>    }

>  

> @@ -461,9 +456,6 @@ ConvertSectionToPages (

>      PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;

>    }

>  

> -  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks

> -  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);

> -

>    // Formulate page table entry, Domain=0, NS=0

>    PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;

>  

> -- 

> 2.17.1

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 21, 2018, 2:10 p.m. UTC | #2 
On 21 June 2018 at 16:01, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Jun 21, 2018 at 10:13:15AM +0200, Ard Biesheuvel wrote:

>> Given that these days, our ARM port only supports ARMv7 and later, we

>> can assume that the page table walker's memory accesses are cache

>> coherent, and so there is no need to perform cache maintenance. It

>> does require the page tables themselves to reside in memory mapped as

>> writeback cacheable so ASSERT() that this is the case.

>

> One minor nit.

>

>> Contributed-under: TianoCore Contribution Agreement 1.1

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

>> ---

>>  ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S    |  2 --

>>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 14 +++-----------

>>  2 files changed, 3 insertions(+), 13 deletions(-)

>>

>> diff --git a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S

>> index 149b57e059ee..f2a517671f0a 100644

>> --- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S

>> +++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S

>> @@ -100,8 +100,6 @@ ASM_FUNC(ArmGetTTBR0BaseAddress)

>>  //  IN VOID  *MVA                    // R1

>>  //  );

>>  ASM_FUNC(ArmUpdateTranslationTableEntry)

>> -  mcr     p15,0,R0,c7,c14,1     @ DCCIMVAC Clean data cache by MVA

>> -  dsb

>>    mcr     p15,0,R1,c8,c7,1      @ TLBIMVA TLB Invalidate MVA

>>    mcr     p15,0,R9,c7,c5,6      @ BPIALL Invalidate Branch predictor array. R9 == NoOp

>>    dsb

>> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c

>> index 9c2578979e44..3037b642d40c 100644

>> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c

>> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c

>> @@ -343,17 +343,12 @@ ArmConfigureMmu (

>>    }

>>

>>    // Translate the Memory Attributes into Translation Table Register Attributes

>> -  if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED) ||

>> -      (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED)) {

>> -    TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_NON_CACHEABLE : TTBR_NON_CACHEABLE;

>> -  } else if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK) ||

>> +  if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK) ||

>>        (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK)) {

>>      TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_WRITE_BACK_ALLOC : TTBR_WRITE_BACK_ALLOC;

>> -  } else if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH) ||

>> -      (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH)) {

>> -    TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_WRITE_THROUGH : TTBR_WRITE_THROUGH;

>>    } else {

>> -    ASSERT (0); // No support has been found for the attributes of the memory region that the translation table belongs to.

>> +    // Page tables must reside in memory mapped as writeback cacheable

>

> ARM ARM always uses "write-back" - could you add the hyphen to assist

> greppability?

>

> If so, for the series:

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>


Thanks

Pushed as c2d6e2bc12b2..6e275c613e15


>> +    ASSERT (0);

>>      return RETURN_UNSUPPORTED;

>>    }

>>

>> @@ -461,9 +456,6 @@ ConvertSectionToPages (

>>      PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;

>>    }

>>

>> -  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks

>> -  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);

>> -

>>    // Formulate page table entry, Domain=0, NS=0

>>    PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;

>>

>> --

>> 2.17.1

>>

_______________________________________________
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/Library/ArmLib/Arm/ArmLibSupport.S b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
index 149b57e059ee..f2a517671f0a 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
+++ b/ArmPkg/Library/ArmLib/Arm/ArmLibSupport.S
@@ -100,8 +100,6 @@  ASM_FUNC(ArmGetTTBR0BaseAddress)
 //  IN VOID  *MVA                    // R1
 //  );
 ASM_FUNC(ArmUpdateTranslationTableEntry)
-  mcr     p15,0,R0,c7,c14,1     @ DCCIMVAC Clean data cache by MVA
-  dsb
   mcr     p15,0,R1,c8,c7,1      @ TLBIMVA TLB Invalidate MVA
   mcr     p15,0,R9,c7,c5,6      @ BPIALL Invalidate Branch predictor array. R9 == NoOp
   dsb
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 9c2578979e44..3037b642d40c 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -343,17 +343,12 @@  ArmConfigureMmu (
   }
 
   // Translate the Memory Attributes into Translation Table Register Attributes
-  if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED) ||
-      (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_UNCACHED_UNBUFFERED)) {
-    TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_NON_CACHEABLE : TTBR_NON_CACHEABLE;
-  } else if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK) ||
+  if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK) ||
       (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK)) {
     TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_WRITE_BACK_ALLOC : TTBR_WRITE_BACK_ALLOC;
-  } else if ((TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH) ||
-      (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_THROUGH)) {
-    TTBRAttributes = ArmHasMpExtensions () ? TTBR_MP_WRITE_THROUGH : TTBR_WRITE_THROUGH;
   } else {
-    ASSERT (0); // No support has been found for the attributes of the memory region that the translation table belongs to.
+    // Page tables must reside in memory mapped as writeback cacheable
+    ASSERT (0);
     return RETURN_UNSUPPORTED;
   }
 
@@ -461,9 +456,6 @@  ConvertSectionToPages (
     PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
   }
 
-  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
-  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);
-
   // Formulate page table entry, Domain=0, NS=0
   PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;