Re: [PATCH v2 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Mar 25, 2020 at 12:38:46 +0100, Ard Biesheuvel wrote:
> Currently, depending on the size of the region being (re)mapped, the
> page table manipulation code may replace a table entry with a block entry,
> even if the existing table entry uses different mapping attributes to
> describe different parts of the region it covers. This is undesirable, and
> instead, we should avoid doing so unless we are disregarding the original
> attributes anyway. And if we make such a replacement, we should free all
> the page tables that have become orphaned in the process.
> 
> So let's implement this, by taking the table entry path through the code
> for block sized regions if a table entry already exists, and the clear
> mask is set (which means we are preserving attributes from the existing
> mapping). And when we do replace a table entry with a block entry, free
> all the pages that are no longer referenced.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

With Laszlo's Tested-by:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 6f6ef5b05fbc..488156e69057 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -223,8 +223,12 @@ UpdateRegionMappingRecursive (
>      // than a block, and recurse to create the block or page entries at
>      // the next level. No block mappings are allowed at all at level 0,
>      // so in that case, we have to recurse unconditionally.
> +    // If we are changing a table entry and the AttributeClearMask is non-zero,
> +    // we cannot replace it with a block entry without potentially losing
> +    // attribute information, so keep the table entry in that case.
>      //
> -    if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
> +    if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
> +        (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
>        ASSERT (Level < 3);
>  
>        if (!IsTableEntry (*Entry, Level)) {
> @@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
>            InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
>          }
>  
> +        ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> +
>          if (IsBlockEntry (*Entry, Level)) {
>            //
>            // We are splitting an existing block entry, so we have to populate
> @@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
>              FreePages (TranslationTable, 1);
>              return Status;
>            }
> -        } else {
> -          ZeroMem (TranslationTable, EFI_PAGE_SIZE);
>          }
>        } else {
>          TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> @@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
>        EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
>                                   : TT_TYPE_BLOCK_ENTRY;
>  
> -      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> +      if (IsTableEntry (*Entry, Level)) {
> +        //
> +        // We are replacing a table entry with a block entry. This is only
> +        // possible if we are keeping none of the original attributes.
> +        // We can free the table entry's page table, and all the ones below
> +        // it, since we are dropping the only possible reference to it.
> +        //
> +        ASSERT (AttributeClearMask == 0);
> +        TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> +        ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
> +        FreePageTablesRecursive (TranslationTable);
> +      } else {
> +        ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> +      }
>      }
>    }
>    return EFI_SUCCESS;
> -- 
> 2.17.1
>