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);
> }
>
> 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.
>
> 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]>
I am not very familiar with this code, and I am struggling a bit to
understand it.
Since you identify at least 2 issues with this code, could you perhaps
split it up into 2 (or more) patches?
Thanks,
Ard.
> ---
> 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
