On 6 September 2015 at 10:15, Heyi Guo <[email protected]> wrote: > Below code has bug since *BlockEntrySize and *TableLevel are not > updated accordingly: > > if (IndexLevel == PageLevel) { > // And get the appropriate BlockEntry at the next level > BlockEntry = (UINT64*)TT_GET_ENTRY_FOR_ADDRESS (TranslationTable, \ > IndexLevel + 1, RegionStart); > > // Set the last block for this new table > *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable, \ > TT_ENTRY_COUNT); > } >
Indeed. > Also it doesn't check recursively to get the last level, e.g. the > initial PageLevel is 1 and we already have level 2 and 3 tables at > this address. > Yes, it only advances by 1 level, but ignores any additional levels. > What's more, *LastBlockEntry was not updated when we get a table and > IndexLevel != PageLevel. > > So we reorganize the sequence, only updating TranslationTable, > PageLevel and BlockEntry in the loop, and setting the other output > parameters with the final PageLevel before returning. > > And LastBlockEntry is only an OUT parameter. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Heyi Guo <[email protected]> > Cc: Leif Lindholm <[email protected]> > Cc: Ard Biesheuvel <[email protected]> > --- OK, it took me some time to wrap my head around this code, but I think it is correct. Reviewed-by: Ard Biesheuvel <[email protected]> However, what we are still missing is logic to free table entries if they are superseded by block entries. For instance, if you map a 2 MB block which is already covered by a table entry, we end up mapping the entire block using 4 KB pages, instead of freeing the table and replacing the table entry with a block entry. If we want this (which I am not necessarily convinced that we do btw), it should be a separate patch anyway. > ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 45 > ++++++++++-------------------- > 1 file changed, 15 insertions(+), 30 deletions(-) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > index b039ae1..f7351cd 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c > @@ -244,7 +244,7 @@ GetBlockEntryListFromAddress ( > IN UINT64 RegionStart, > OUT UINTN *TableLevel, > IN OUT UINT64 *BlockEntrySize, > - IN OUT UINT64 **LastBlockEntry > + OUT UINT64 **LastBlockEntry > ) > { > UINTN RootTableLevel; > @@ -282,14 +282,9 @@ GetBlockEntryListFromAddress ( > return NULL; > } > > - // > - // Calculate LastBlockEntry from T0SZ - this is the last block entry of > the root Translation table > - // > T0SZ = ArmGetTCR () & TCR_T0SZ_MASK; > // Get the Table info from T0SZ > GetRootTranslationTableInfo (T0SZ, &RootTableLevel, &RootTableEntryCount); > - // The last block of the root table depends on the number of entry in this > table > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(RootTable, RootTableEntryCount); > > // If the start address is 0x0 then we use the size of the region to > identify the alignment > if (RegionStart == 0) { > @@ -321,12 +316,6 @@ GetBlockEntryListFromAddress ( > PageLevel++; > } > > - // 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); > - > // > // Get the Table Descriptor for the corresponding PageLevel. We need to > decompose RegionStart to get appropriate entries > // > @@ -339,13 +328,10 @@ GetBlockEntryListFromAddress ( > // Go to the next table > TranslationTable = (UINT64*)(*BlockEntry & > TT_ADDRESS_MASK_DESCRIPTION_TABLE); > > - // If we are at the last level then update the output > + // If we are at the last level then update the last level to next level > if (IndexLevel == PageLevel) { > - // And get the appropriate BlockEntry at the next level > - BlockEntry = (UINT64*)TT_GET_ENTRY_FOR_ADDRESS (TranslationTable, > IndexLevel + 1, RegionStart); > - > - // Set the last block for this new table > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable, > TT_ENTRY_COUNT); > + // 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 > @@ -394,11 +380,6 @@ GetBlockEntryListFromAddress ( > > // Fill the BlockEntry with the new TranslationTable > *BlockEntry = ((UINTN)TranslationTable & > TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TableAttributes | TT_TYPE_TABLE_ENTRY; > - // Update the last block entry with the newly created translation > table > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable, > TT_ENTRY_COUNT); > - > - // Block Entry points at the beginning of the Translation Table > - BlockEntry = TranslationTable; > } > } else { > if (IndexLevel != PageLevel) { > @@ -417,17 +398,21 @@ GetBlockEntryListFromAddress ( > > // Fill the new BlockEntry with the TranslationTable > *BlockEntry = ((UINTN)TranslationTable & > TT_ADDRESS_MASK_DESCRIPTION_TABLE) | TT_TYPE_TABLE_ENTRY; > - // Update the last block entry with the newly created translation > table > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable, > TT_ENTRY_COUNT); > - } else { > - // > - // Case when the new region is part of an existing page table > - // > - *LastBlockEntry = TT_LAST_BLOCK_ADDRESS(TranslationTable, > TT_ENTRY_COUNT); > } > } > } > > + // 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; > } > > -- > 2.5.0 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
