Re: [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 6 March 2017 at 15:27, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 02, 2017 at 10:36:14AM +0000, Ard Biesheuvel wrote:
>> Page and section entries in the page tables are updated using the
>> helper ArmUpdateTranslationTableEntry(), which cleans the page
>> table entry to the PoC, and invalidates the TLB entry covering
>> the page described by the entry being updated.
>>
>> Since we may be updating section entries, we might be leaving stale
>> TLB entries at this point (for all pages in the section except the
>> first one), which will be invalidated wholesale at the end of
>> SetMemoryAttributes(). At that point, all caches are cleaned *and*
>> invalidated as well.
>>
>> This cache maintenance is costly and unnecessary. The TLB maintenance
>> is only necessary if we updated any section entries, since any page
>> by page entries that have been updated will have been invalidated
>> individually by ArmUpdateTranslationTableEntry().
>>
>> So drop the clean/invalidate of the caches, and only perform the
>> full TLB flush if UpdateSectionEntries() was called, or if sections
>> were split by UpdatePageEntries(). Finally, make the cache maintenance
>> on the remapped regions themselves conditional on whether any memory
>> type attributes were modified.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 60 +++++++++++---------
>>  1 file changed, 34 insertions(+), 26 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> index ce4d529bda67..26b637e7658f 100644
>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> @@ -347,10 +347,11 @@ SyncCacheConfig (
>>
>>  EFI_STATUS
>>  UpdatePageEntries (
>> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> -  IN UINT64                    Length,
>> -  IN UINT64                    Attributes,
>> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> +  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>> +  IN  UINT64                    Length,
>> +  IN  UINT64                    Attributes,
>> +  IN  EFI_PHYSICAL_ADDRESS      VirtualMask,
>> +  OUT BOOLEAN                   *FlushTlbs
>>    )
>>  {
>>    EFI_STATUS    Status;
>> @@ -446,6 +447,9 @@ UpdatePageEntries (
>>
>>        // Re-read descriptor
>>        Descriptor = FirstLevelTable[FirstLevelIdx];
>> +      if (FlushTlbs != NULL) {
>
> If FlushTlbs is optional, should it not be marked OPTIONAL above?

Yes

> If you fold that in:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks

>> +        *FlushTlbs = TRUE;
>> +      }
>>      }
>>
>>      // Obtain page table base address
>> @@ -471,15 +475,16 @@ UpdatePageEntries (
>>
>>      if (CurrentPageTableEntry  != PageTableEntry) {
>>        Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
>> -      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
>> -        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
>> -        // Note assumes switch(Attributes), not ARMv7 possibilities
>> -        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
>> -      }
>>
>>        // Only need to update if we are changing the entry
>>        PageTable[PageTableIndex] = PageTableEntry;
>>        ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
>> +
>> +      // 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);
>> +      }
>>      }
>>
>>      Status = EFI_SUCCESS;
>> @@ -581,7 +586,12 @@ UpdateSectionEntries (
>>      // has this descriptor already been coverted to pages?
>>      if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
>>        // forward this 1MB range to page table function instead
>> -      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
>> +      Status = UpdatePageEntries (
>> +                 (FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT,
>> +                 TT_DESCRIPTOR_SECTION_SIZE,
>> +                 Attributes,
>> +                 VirtualMask,
>> +                 NULL);
>>      } else {
>>        // still a section entry
>>
>> @@ -596,15 +606,16 @@ UpdateSectionEntries (
>>
>>        if (CurrentDescriptor  != Descriptor) {
>>          Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>> -        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
>> -          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
>> -          // Note assumes switch(Attributes), not ARMv7 possabilities
>> -          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
>> -        }
>>
>>          // Only need to update if we are changing the descriptor
>>          FirstLevelTable[FirstLevelIdx + i] = Descriptor;
>>          ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
>> +
>> +        // 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);
>> +        }
>>        }
>>
>>        Status = EFI_SUCCESS;
>> @@ -680,6 +691,7 @@ SetMemoryAttributes (
>>  {
>>    EFI_STATUS    Status;
>>    UINT64        ChunkLength;
>> +  BOOLEAN       FlushTlbs;
>>
>>    //
>>    // Ignore invocations that only modify permission bits
>> @@ -688,6 +700,7 @@ SetMemoryAttributes (
>>      return EFI_SUCCESS;
>>    }
>>
>> +  FlushTlbs = FALSE;
>>    while (Length > 0) {
>>      if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>>          Length >= TT_DESCRIPTOR_SECTION_SIZE) {
>> @@ -700,6 +713,8 @@ SetMemoryAttributes (
>>
>>        Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
>>                   VirtualMask);
>> +
>> +      FlushTlbs = TRUE;
>>      } else {
>>
>>        //
>> @@ -717,7 +732,7 @@ SetMemoryAttributes (
>>          BaseAddress, ChunkLength, Attributes));
>>
>>        Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
>> -                 VirtualMask);
>> +                 VirtualMask, &FlushTlbs);
>>      }
>>
>>      if (EFI_ERROR (Status)) {
>> @@ -728,16 +743,9 @@ SetMemoryAttributes (
>>      Length -= ChunkLength;
>>    }
>>
>> -  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
>> -  // flush and invalidate pages
>> -  //TODO: Do we really need to invalidate the caches everytime we change the memory attributes ?
>> -  ArmCleanInvalidateDataCache ();
>> -
>> -  ArmInvalidateInstructionCache ();
>> -
>> -  // Invalidate all TLB entries so changes are synced
>> -  ArmInvalidateTlb ();
>> -
>> +  if (FlushTlbs) {
>> +    ArmInvalidateTlb ();
>> +  }
>>    return Status;
>>  }
>>
>> --
>> 2.7.4
>>