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.biesheu...@linaro.org>
With Laszlo's Tested-by: Reviewed-by: Leif Lindholm <l...@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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56289): https://edk2.groups.io/g/devel/message/56289 Mute This Topic: https://groups.io/mt/72538523/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-