Re: [PATCH v3 05/16] ArmPkg/ArmMmuLib: don't replace table entries with block entries

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On 2022-09-26 01:25, Ard Biesheuvel wrote:
> Drop the optimization that replaces table entries with block entries and
> frees the page tables in the subhierarchy that is being replaced. This
> rarely occurs in practice anyway, and will require more elaborate TLB
> maintenance once we switch to a different approach when running at EL1,
> where we no longer disable the MMU and nuke the TLB entirely every time
> we update a descriptor in a way that requires break-before-make (BBM).
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

/
     Leif

> ---
>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 20 ++------------------
>   1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index e5ecc7375153..34f1031c4de3 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -197,12 +197,9 @@ 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) ||
> 
> -        (IsTableEntry (*Entry, Level) && (AttributeClearMask != 0)))
> 
> +        IsTableEntry (*Entry, Level))
> 
>       {
> 
>         ASSERT (Level < 3);
> 
>   
> 
> @@ -294,20 +291,7 @@ UpdateRegionMappingRecursive (
>         EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> 
>                                    : TT_TYPE_BLOCK_ENTRY;
> 
>   
> 
> -      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, Level + 1);
> 
> -      } else {
> 
> -        ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> 
> -      }
> 
> +      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> 
>       }
> 
>     }
> 
>   
>