[PATCH v2 0/2] ArmMmuLib ARM: remove cache maintenance

[PATCH v2 0/2] ArmMmuLib ARM: remove cache maintenance

[Ard Biesheuvel]

Follow up series to v2 'ArmMmuLib ARM: remove cache maintenance of block
mapping contents'

Patch #1 updated to remove more cache maintenance of memory *content*

Patch #2 added to remove cache maintenance of page table entries themselves,
and to remove support for storing page tables in memory that is not mapped
writeback cacheable.


Ard Biesheuvel (2):
  ArmPkg/ArmMmuLib ARM: remove cache maintenance of block mapping
    contents
  ArmPkg/ArmMmuLib ARM: assume page tables are in writeback cacheable
    memory

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

-- 
2.17.1

[PATCH v2 1/2] ArmPkg/ArmMmuLib ARM: remove cache maintenance of block mapping contents

[Ard Biesheuvel]

Peculiarly enough, the current page table manipulation code takes it
upon itself to write back and invalidate the memory contents covered
by section mappings when their memory attributes change. It is not
generally the case that data must be written back when such a change
occurs, even when switching from cacheable to non-cacheable attributes,
and in some cases, it is actually causing problems. (The cache
maintenance is also performed on the PCIe MMIO regions as they get
mapped by the PCI bus driver, and under virtualization, each cache
maintenance operation on an emulated MMIO region triggers a round
trip to the host and back)

So let's just drop this code.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 9bf4ba03fd5b..9c2578979e44 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -597,12 +597,6 @@ UpdatePageEntries (
     if (CurrentPageTableEntry  != PageTableEntry) {
       Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
 
-      // Clean/invalidate the cache for this page, but only
-      // if we are modifying the memory type attributes
-      if (((CurrentPageTableEntry ^ PageTableEntry) & TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK) != 0) {
-        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
-      }
-
       // Only need to update if we are changing the entry
       PageTable[PageTableIndex] = PageTableEntry;
       ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
@@ -718,12 +712,6 @@ UpdateSectionEntries (
       if (CurrentDescriptor  != Descriptor) {
         Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
 
-        // Clean/invalidate the cache for this section, but only
-        // if we are modifying the memory type attributes
-        if (((CurrentDescriptor ^ Descriptor) & TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK) != 0) {
-          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
-        }
-
         // Only need to update if we are changing the descriptor
         FirstLevelTable[FirstLevelIdx + i] = Descriptor;
         ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
-- 
2.17.1

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

[Ard Biesheuvel]

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(-)

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;
 
-- 
2.17.1

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

[Leif Lindholm]

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
> 

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

[Ard Biesheuvel]

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
>>