Re: [PATCH] ArmPkg/ArmMmuLib ARM: remove cache maintenance of block mapping contents

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

On 21 June 2018 at 00:05, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Jun 20, 2018 at 09:10:07PM +0200, Ard Biesheuvel wrote:
>> 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.
>
> I guess this is a remnant from pre-ARMv7 days, when translation table
> walks weren't inner-cacheable. I think we've already determined that
> ARMv7-A+ is required for PI compliance anyway, so I guess dropping
> this should be fine.
>

No, this one is different. This does not flush the page containing the
page table entry that is modified, but it flushes the memory that the
page table entry maps (a 1 MB section in this case)

> But if so, don't we also want to get rid of the other two instances of
> WriteBackInvalidateDataCacheRange() in this file?
>

If we assume page table walks are coherent, then yes, we could remove
some more code, but that should be a separate patch.

>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
>> index 9bf4ba03fd5b..d1bca4c601b8 100644
>> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
>> @@ -718,12 +718,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
>>