On 09/06/2015 09:42 PM, Ard Biesheuvel wrote:
On 6 September 2015 at 10:15, Heyi Guo <heyi....@linaro.org> 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 <heyi....@linaro.org>
Cc: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
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.
Hi Ard,
There are several issues with the original code, but all can be fixed by
updating PageLevel and getting OUT parameters with the final PageLevel,
which I supposed to be one complete modification.
So I've no good idea on how to split the patch yet.
Below change can be split but I think it is just a minor change. Please
let me know if you think it is better to split it.
And LastBlockEntry is only an OUT parameter.
Thanks.
Heyi
---
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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
