On Fri, 6 Mar 2020 at 19:50, Leif Lindholm <l...@nuviainc.com> wrote:
>
> On Fri, Mar 06, 2020 at 17:12:45 +0100, Ard Biesheuvel wrote:
> > Replace the slightly overcomplicated page table management code with
> > a simplified, recursive implementation that should be far easier to
> > reason about.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > ---
> > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 332 +++++++-------------
> > 1 file changed, 108 insertions(+), 224 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > index 204e33c75f95..e36594fea3ad 100644
> > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > @@ -121,14 +121,16 @@ GetRootTranslationTableInfo (
> >
> > STATIC
> > VOID
> > -ReplaceLiveEntry (
> > +ReplaceTableEntry (
> > IN UINT64 *Entry,
> > IN UINT64 Value,
> > - IN UINT64 RegionStart
> > + IN UINT64 RegionStart,
> > + IN BOOLEAN IsLiveBlockMapping
> > )
> > {
> > - if (!ArmMmuEnabled ()) {
> > + if (!ArmMmuEnabled () || !IsLiveBlockMapping) {
> > *Entry = Value;
> > + ArmUpdateTranslationTableEntry (Entry, (VOID *)(UINTN)RegionStart);
> > } else {
> > ArmReplaceLiveTranslationEntry (Entry, Value, RegionStart);
> > }
> > @@ -165,229 +167,132 @@ LookupAddresstoRootTable (
> > }
> >
> > STATIC
> > -UINT64*
> > -GetBlockEntryListFromAddress (
> > - IN UINT64 *RootTable,
> > - IN UINT64 RegionStart,
> > - OUT UINTN *TableLevel,
> > - IN OUT UINT64 *BlockEntrySize,
> > - OUT UINT64 **LastBlockEntry
> > +EFI_STATUS
> > +UpdateRegionMappingRec (
>
> What does Rec stand for? Record? Recursively?
> Could it be written out?
>
> > + IN UINT64 RegionStart,
> > + IN UINT64 RegionEnd,
> > + IN UINT64 AttributeSetMask,
> > + IN UINT64 AttributeClearMask,
> > + IN UINT64 *PageTable,
> > + IN UINTN Level
>
> This sets the scene for a spectacularly unhelpful diff.
> Would you consider adding UpdateRegionMappingRec() *before*
> LookupAddresstoRootTable() for the only purpose of working against the
> shortcomings of the line diff algorithm? That is what I ended up doing
> locally for the review.
>
Yeah works for me
> > )
> > {
> > - UINTN RootTableLevel;
> > - UINTN RootTableEntryCount;
> > - UINT64 *TranslationTable;
> > - UINT64 *BlockEntry;
> > - UINT64 *SubTableBlockEntry;
> > - UINT64 BlockEntryAddress;
> > - UINTN BaseAddressAlignment;
> > - UINTN PageLevel;
> > - UINTN Index;
> > - UINTN IndexLevel;
> > - UINTN T0SZ;
> > - UINT64 Attributes;
> > - UINT64 TableAttributes;
> > -
> > - // Initialize variable
> > - BlockEntry = NULL;
> > -
> > - // Ensure the parameters are valid
> > - if (!(TableLevel && BlockEntrySize && LastBlockEntry)) {
> > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> > - return NULL;
> > - }
> > -
> > - // Ensure the Region is aligned on 4KB boundary
> > - if ((RegionStart & (SIZE_4KB - 1)) != 0) {
> > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> > - return NULL;
> > - }
> > -
> > - // Ensure the required size is aligned on 4KB boundary and not 0
> > - if ((*BlockEntrySize & (SIZE_4KB - 1)) != 0 || *BlockEntrySize == 0) {
> > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> > - return NULL;
> > - }
> > -
> > - T0SZ = ArmGetTCR () & TCR_T0SZ_MASK;
> > - // Get the Table info from T0SZ
> > - GetRootTranslationTableInfo (T0SZ, &RootTableLevel,
> > &RootTableEntryCount);
> > -
> > - // If the start address is 0x0 then we use the size of the region to
> > identify the alignment
> > - if (RegionStart == 0) {
> > - // Identify the highest possible alignment for the Region Size
> > - BaseAddressAlignment = LowBitSet64 (*BlockEntrySize);
> > - } else {
> > - // Identify the highest possible alignment for the Base Address
> > - BaseAddressAlignment = LowBitSet64 (RegionStart);
> > - }
> > -
> > - // Identify the Page Level the RegionStart must belong to. Note that
> > PageLevel
> > - // should be at least 1 since block translations are not supported at
> > level 0
> > - PageLevel = MAX (3 - ((BaseAddressAlignment - 12) / 9), 1);
> > -
> > - // If the required size is smaller than the current block size then we
> > need to go to the page below.
> > - // The PageLevel was calculated on the Base Address alignment but did
> > not take in account the alignment
> > - // of the allocation size
> > - while (*BlockEntrySize < TT_BLOCK_ENTRY_SIZE_AT_LEVEL (PageLevel)) {
> > - // It does not fit so we need to go a page level above
> > - PageLevel++;
> > - }
> > -
> > - //
> > - // Get the Table Descriptor for the corresponding PageLevel. We need to
> > decompose RegionStart to get appropriate entries
> > - //
> > -
> > - TranslationTable = RootTable;
> > - for (IndexLevel = RootTableLevel; IndexLevel <= PageLevel; IndexLevel++)
> > {
> > - BlockEntry = (UINT64*)TT_GET_ENTRY_FOR_ADDRESS (TranslationTable,
> > IndexLevel, RegionStart);
> > -
> > - if ((IndexLevel != 3) && ((*BlockEntry & TT_TYPE_MASK) ==
> > TT_TYPE_TABLE_ENTRY)) {
> > - // Go to the next table
> > - TranslationTable = (UINT64*)(*BlockEntry &
> > TT_ADDRESS_MASK_DESCRIPTION_TABLE);
> > -
> > - // If we are at the last level then update the last level to next
> > level
> > - if (IndexLevel == PageLevel) {
> > - // Enter the next level
> > - PageLevel++;
> > - }
> > - } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
> > - // If we are not at the last level then we need to split this
> > BlockEntry
> > - if (IndexLevel != PageLevel) {
> > - // Retrieve the attributes from the block entry
> > - Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;
> > -
> > - // Convert the block entry attributes into Table descriptor
> > attributes
> > - TableAttributes = TT_TABLE_AP_NO_PERMISSION;
> > - if (Attributes & TT_NS) {
> > - TableAttributes = TT_TABLE_NS;
> > + UINTN BlockShift;
> > + UINT64 BlockMask;
> > + UINT64 BlockEnd;
> > + UINT64 *Entry;
> > + UINT64 EntryValue;
> > + VOID *TranslationTable;
> > + EFI_STATUS Status;
> > +
> > + ASSERT (((RegionStart | RegionEnd) & EFI_PAGE_MASK) == 0);
> > +
> > + BlockShift = (Level + 1) * BITS_PER_LEVEL + MIN_T0SZ;
> > + BlockMask = MAX_UINT64 >> BlockShift;
> > +
> > + DEBUG ((DEBUG_VERBOSE, "%a(%d): %llx - %llx set %lx clr %lx\n",
> > __FUNCTION__,
> > + Level, RegionStart, RegionEnd, AttributeSetMask, AttributeClearMask));
> > +
> > + for (; RegionStart < RegionEnd; RegionStart = BlockEnd) {
> > + BlockEnd = MIN (RegionEnd, (RegionStart | BlockMask) + 1);
> > + Entry = &PageTable[(RegionStart >> (64 - BlockShift)) &
> > (TT_ENTRY_COUNT - 1)];
> > +
> > + //
> > + // If RegionStart or BlockEnd is not aligned to the block size at this
> > + // level, we will have to create a table mapping in order to map less
> > + // 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 (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
> > + ASSERT (Level < 3);
> > +
> > + if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
> > + //
> > + // No table entry exists yet, so we need to allocate a page table
> > + // for the next level.
> > + //
> > + TranslationTable = AllocatePages (1);
> > + if (TranslationTable == NULL) {
> > + return EFI_OUT_OF_RESOURCES;
> > }
> >
> > - // Get the address corresponding at this entry
> > - BlockEntryAddress = RegionStart;
> > - BlockEntryAddress = BlockEntryAddress >>
> > TT_ADDRESS_OFFSET_AT_LEVEL(IndexLevel);
> > - // Shift back to right to set zero before the effective address
> > - BlockEntryAddress = BlockEntryAddress <<
> > TT_ADDRESS_OFFSET_AT_LEVEL(IndexLevel);
> > -
> > - // Set the correct entry type for the next page level
> > - if ((IndexLevel + 1) == 3) {
> > - Attributes |= TT_TYPE_BLOCK_ENTRY_LEVEL3;
> > + if ((*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
> > + //
> > + // We are splitting an existing block entry, so we have to
> > populate
> > + // the new table with the attributes of the block entry it
> > replaces.
> > + //
> > + Status = UpdateRegionMappingRec (
> > + RegionStart & ~BlockMask,
> > + (RegionStart | BlockMask) + 1,
> > + *Entry & TT_ATTRIBUTES_MASK,
> > + 0,
> > + TranslationTable,
> > + Level + 1);
> > + if (EFI_ERROR (Status)) {
>
> I realise a EFI_OUT_OF_RESOURCES return value here means we would be
> in dire straits already, but it would be nice if we could free the
> local TranslationTable instead of leaking all allocations through the
> tree.
>
> If not worth the effort (or impossible condition), would still be
> worth an explicit comment here.
>
In PEI, it wouldn't help since FreePages() is a NOP there. But this
library is also used by CpuDxe, so it does make sense.
And the nice thing about recursion is that it is much simpler to
reason about: we only need to free the page if it was allocated in the
context of the same call, and everything else comes out in the wash.
> > + return Status;
> > + }
> > } else {
> > - Attributes |= TT_TYPE_BLOCK_ENTRY;
> > + ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> > }
> > + } else {
> > + TranslationTable = (VOID *)(UINTN)(*Entry &
> > TT_ADDRESS_MASK_BLOCK_ENTRY);
> > + }
> >
> > - // Create a new translation table
> > - TranslationTable = AllocatePages (1);
> > - if (TranslationTable == NULL) {
> > - return NULL;
> > - }
> > -
> > - // Populate the newly created lower level table
> > - SubTableBlockEntry = TranslationTable;
> > - for (Index = 0; Index < TT_ENTRY_COUNT; Index++) {
> > - *SubTableBlockEntry = Attributes | (BlockEntryAddress + (Index
> > << TT_ADDRESS_OFFSET_AT_LEVEL(IndexLevel + 1)));
> > - SubTableBlockEntry++;
> > - }
> > + //
> > + // Recurse to the next level
> > + //
> > + Status = UpdateRegionMappingRec (
> > + RegionStart,
> > + BlockEnd,
> > + AttributeSetMask,
> > + AttributeClearMask,
> > + TranslationTable,
> > + Level + 1);
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
> >
> > - // Fill the BlockEntry with the new TranslationTable
> > - ReplaceLiveEntry (BlockEntry,
> > - (UINTN)TranslationTable | TableAttributes | TT_TYPE_TABLE_ENTRY,
> > - RegionStart);
> > + if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
> > + ReplaceTableEntry (Entry,
> > + (UINT64)TranslationTable | TT_TYPE_TABLE_ENTRY,
> > + RegionStart,
> > + (*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY);
> > }
> > } else {
> > - if (IndexLevel != PageLevel) {
> > - //
> > - // Case when we have an Invalid Entry and we are at a page level
> > above of the one targetted.
> > - //
> > + EntryValue = (*Entry & AttributeClearMask) | AttributeSetMask;
> > + EntryValue |= RegionStart;
> > + EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> > + : TT_TYPE_BLOCK_ENTRY;
> >
> > - // Create a new translation table
> > - TranslationTable = AllocatePages (1);
> > - if (TranslationTable == NULL) {
> > - return NULL;
> > - }
> > -
> > - ZeroMem (TranslationTable, TT_ENTRY_COUNT * sizeof(UINT64));
> > -
> > - // Fill the new BlockEntry with the TranslationTable
> > - *BlockEntry = ((UINTN)TranslationTable &
> > TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TT_TYPE_TABLE_ENTRY;
> > - }
> > + ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> > }
> > }
> > -
> > - // Expose the found PageLevel to the caller
> > - *TableLevel = PageLevel;
> > -
> > - // Now, we have the Table Level we can get the Block Size associated to
> > this table
> > - *BlockEntrySize = TT_BLOCK_ENTRY_SIZE_AT_LEVEL (PageLevel);
> > -
> > - // The last block of the root table depends on the number of entry in
> > this table,
> > - // otherwise it is always the (TT_ENTRY_COUNT - 1)th entry in the table.
> > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable,
> > - (PageLevel == RootTableLevel) ? RootTableEntryCount :
> > TT_ENTRY_COUNT);
> > -
> > - return BlockEntry;
> > + return EFI_SUCCESS;
> > }
> >
> > STATIC
> > EFI_STATUS
> > UpdateRegionMapping (
> > - IN UINT64 *RootTable,
> > - IN UINT64 RegionStart,
> > - IN UINT64 RegionLength,
> > - IN UINT64 Attributes,
> > - IN UINT64 BlockEntryMask
> > + IN UINT64 RegionStart,
> > + IN UINT64 RegionSize,
> > + IN UINT64 AttributeSetMask,
> > + IN UINT64 AttributeClearMask
>
> The new indentation here (which arguably is an improvement) sort of
> masks the unchanged input RegionStart and the rename of
> RegionLength->RegionSize. Since it doesn't help anything in this
> series, could it be deferred (or broken out)?
>
I'll revert this change for now (include the name change since it is pointless)
> > )
> > {
> > - UINT32 Type;
> > - UINT64 *BlockEntry;
> > - UINT64 *LastBlockEntry;
> > - UINT64 BlockEntrySize;
> > - UINTN TableLevel;
> > + UINTN RootTableLevel;
> > + UINTN T0SZ;
> >
> > - // Ensure the Length is aligned on 4KB boundary
> > - if ((RegionLength == 0) || ((RegionLength & (SIZE_4KB - 1)) != 0)) {
> > - ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> > + if ((RegionStart & EFI_PAGE_MASK) != 0 || (RegionSize & EFI_PAGE_MASK)
> > != 0) {
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > - do {
> > - // Get the first Block Entry that matches the Virtual Address and also
> > the information on the Table Descriptor
> > - // such as the size of the Block Entry and the address of the last
> > BlockEntry of the Table Descriptor
> > - BlockEntrySize = RegionLength;
> > - BlockEntry = GetBlockEntryListFromAddress (RootTable, RegionStart,
> > &TableLevel, &BlockEntrySize, &LastBlockEntry);
> > - if (BlockEntry == NULL) {
> > - // GetBlockEntryListFromAddress() return NULL when it fails to
> > allocate new pages from the Translation Tables
> > - return EFI_OUT_OF_RESOURCES;
> > - }
> > + T0SZ = ArmGetTCR () & TCR_T0SZ_MASK;
> > + GetRootTranslationTableInfo (T0SZ, &RootTableLevel, NULL);
> >
> > - if (TableLevel != 3) {
> > - Type = TT_TYPE_BLOCK_ENTRY;
> > - } else {
> > - Type = TT_TYPE_BLOCK_ENTRY_LEVEL3;
> > - }
> > -
> > - do {
> > - // Fill the Block Entry with attribute and output block address
> > - *BlockEntry &= BlockEntryMask;
> > - *BlockEntry |= (RegionStart & TT_ADDRESS_MASK_BLOCK_ENTRY) |
> > Attributes | Type;
> > -
> > - ArmUpdateTranslationTableEntry (BlockEntry, (VOID *)RegionStart);
> > -
> > - // Go to the next BlockEntry
> > - RegionStart += BlockEntrySize;
> > - RegionLength -= BlockEntrySize;
> > - BlockEntry++;
> > -
> > - // Break the inner loop when next block is a table
> > - // Rerun GetBlockEntryListFromAddress to avoid page table memory leak
> > - if (TableLevel != 3 && BlockEntry <= LastBlockEntry &&
> > - (*BlockEntry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) {
> > - break;
> > - }
> > - } while ((RegionLength >= BlockEntrySize) && (BlockEntry <=
> > LastBlockEntry));
> > - } while (RegionLength != 0);
> > -
> > - return EFI_SUCCESS;
> > + return UpdateRegionMappingRec (RegionStart, RegionStart + RegionSize,
> > + AttributeSetMask, AttributeClearMask, ArmGetTTBR0BaseAddress (),
> > + RootTableLevel);
> > }
> >
> > STATIC
> > @@ -398,7 +303,6 @@ FillTranslationTable (
> > )
> > {
> > return UpdateRegionMapping (
> > - RootTable,
> > MemoryRegion->VirtualBase,
> > MemoryRegion->Length,
> > ArmMemoryAttributeToPageAttribute (MemoryRegion->Attributes) |
> > TT_AF,
> > @@ -455,8 +359,6 @@ ArmSetMemoryAttributes (
> > IN UINT64 Attributes
> > )
> > {
> > - EFI_STATUS Status;
> > - UINT64 *TranslationTable;
> > UINT64 PageAttributes;
> > UINT64 PageAttributeMask;
> >
> > @@ -473,19 +375,11 @@ ArmSetMemoryAttributes (
> > TT_PXN_MASK | TT_XN_MASK);
> > }
> >
> > - TranslationTable = ArmGetTTBR0BaseAddress ();
> > -
> > - Status = UpdateRegionMapping (
> > - TranslationTable,
> > + return UpdateRegionMapping (
> > BaseAddress,
> > Length,
> > PageAttributes,
> > PageAttributeMask);
>
> While making a neat and clear diff, this messes up coding style.
>
Yup. Will fix.
> /
> Leif
>
> > - if (EFI_ERROR (Status)) {
> > - return Status;
> > - }
> > -
> > - return EFI_SUCCESS;
> > }
> >
> > STATIC
> > @@ -497,17 +391,7 @@ SetMemoryRegionAttribute (
> > IN UINT64 BlockEntryMask
> > )
> > {
> > - EFI_STATUS Status;
> > - UINT64 *RootTable;
> > -
> > - RootTable = ArmGetTTBR0BaseAddress ();
> > -
> > - Status = UpdateRegionMapping (RootTable, BaseAddress, Length,
> > Attributes, BlockEntryMask);
> > - if (EFI_ERROR (Status)) {
> > - return Status;
> > - }
> > -
> > - return EFI_SUCCESS;
> > + return UpdateRegionMapping (BaseAddress, Length, Attributes,
> > BlockEntryMask);
> > }
> >
> > EFI_STATUS
> > --
> > 2.17.1
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55629): https://edk2.groups.io/g/devel/message/55629
Mute This Topic: https://groups.io/mt/71776543/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
