On 7 September 2015 at 11:16, Heyi Guo <heyi....@linaro.org> wrote:
>
>
> 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.
>
OK. Let me grab another cup of coffee and I will go through the patch again.
--
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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
