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

[Leif Lindholm <leif.lindholm@linaro.org>]

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.

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

/
    Leif

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